From 6925b36b274dafff368a3e1b5477d89ab9721f4f Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Tue, 19 Mar 2019 16:08:52 +0000 Subject: [PATCH 1/3] Get templates from all folders a user can see --- app/models/service.py | 24 ++++++++++---- tests/app/models/test_service.py | 55 ++++++++++++++++++++++++++++++++ 2 files changed, 73 insertions(+), 6 deletions(-) diff --git a/app/models/service.py b/app/models/service.py index 6408ebdba..844bcaaae 100644 --- a/app/models/service.py +++ b/app/models/service.py @@ -142,7 +142,6 @@ class Service(): @cached_property def all_templates(self): - templates = service_api_client.get_service_templates(self.id)['data'] return [ @@ -154,6 +153,13 @@ class Service(): def all_template_ids(self): return {template['id'] for template in self.all_templates} + def get_templates_for_folder(self, template_type, all_templates, folder_id): + return [ + template for template in all_templates + if (set([template_type]) & {'all', template['template_type']}) + and (template.get('folder') == folder_id) + ] + def get_templates(self, template_type='all', template_folder_id=None, user_id=None): if user_id and template_folder_id and self.has_permission('edit_folder_permissions'): folder = self.get_template_folder(template_folder_id) @@ -164,11 +170,17 @@ class Service(): template_type = [template_type] if template_folder_id: template_folder_id = str(template_folder_id) - return [ - template for template in self.all_templates - if (set(template_type) & {'all', template['template_type']}) - and template.get('folder') == template_folder_id - ] + return self.get_templates_for_folder(template_type, self.all_templates, template_folder_id) + + def get_user_templates_across_folders(self, user_id, template_type='all'): + folders = self.all_template_folders + all_templates = self.all_templates + user_templates = [] + for folder in folders: + if user_id in folder.get("users_with_permission", []): + user_templates += self.get_templates_for_folder(template_type, all_templates, folder["id"]) + user_templates += self.get_templates_for_folder(template_type, all_templates, None) + return user_templates @property def available_template_types(self): diff --git a/tests/app/models/test_service.py b/tests/app/models/test_service.py index 3ba9063b4..bd7ce2d1f 100644 --- a/tests/app/models/test_service.py +++ b/tests/app/models/test_service.py @@ -67,6 +67,15 @@ def _get_all_folders(active_user_with_permissions): ] +def _template(template_type, name, parent=None, template_id=None): + return { + 'id': template_id or str(uuid.uuid4()), + 'name': name, + 'template_type': template_type, + 'folder': parent, + } + + def test_get_user_template_folders_only_returns_folders_visible_to_user( mock_get_template_folders, service_one, @@ -172,3 +181,49 @@ def test_get_template_folders_shows_all_folders_when_user_id_not_passed_in( 'users_with_permission': [active_user_with_permissions.id] } ] + + +def test_get_user_templates_across_folders( + mock_get_template_folders, + service_one, + active_user_with_permissions, + mocker +): + all_templates = {'data': [ + _template('sms', 'sms_template_one', parent=INV_CHILD_1_FOLDER_ID), + _template('sms', 'sms_template_two'), + _template('email', 'email_template_one', parent=VIS_PARENT_FOLDER_ID), + _template('letter', 'letter_template_one') + ]} + mock_get_template_folders.return_value = _get_all_folders(active_user_with_permissions) + mocker.patch('app.service_api_client.get_service_templates', return_value=all_templates) + service_one['permissions'] = ['edit_folder_permissions', 'letter', 'email', 'sms'] + service = Service(service_one) + result = service.get_user_templates_across_folders(active_user_with_permissions.id) + assert result == [ + {'folder': 'bbbb222b-2b22-2b22-222b-b222b22b2222', 'id': mocker.ANY, + 'name': 'email_template_one', 'template_type': 'email'}, + {'folder': None, 'id': mocker.ANY, + 'name': 'sms_template_two', 'template_type': 'sms'}, + {'folder': None, 'id': mocker.ANY, + 'name': 'letter_template_one', 'template_type': 'letter'}] + + +def test_get_user_templates_across_folders_sms_only( + mock_get_template_folders, + service_one, + active_user_with_permissions, + mocker +): + all_templates = {'data': [ + _template('sms', 'sms_template_one', parent=INV_CHILD_1_FOLDER_ID), + _template('sms', 'sms_template_two'), + _template('email', 'email_template_one', parent=VIS_PARENT_FOLDER_ID), + _template('letter', 'letter_template_one') + ]} + mock_get_template_folders.return_value = _get_all_folders(active_user_with_permissions) + mocker.patch('app.service_api_client.get_service_templates', return_value=all_templates) + service_one['permissions'] = ['edit_folder_permissions', 'letter', 'email', 'sms'] + service = Service(service_one) + result = service.get_user_templates_across_folders(active_user_with_permissions.id, template_type='sms') + assert result == [{'folder': None, 'id': mocker.ANY, 'name': 'sms_template_two', 'template_type': 'sms'}] From 3fc4f6866c93d496727915dd96cc677ea697a9ef Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Tue, 19 Mar 2019 18:30:05 +0000 Subject: [PATCH 2/3] When replying to inbound sms show templates in all user's folders --- app/main/views/conversation.py | 4 +-- app/models/service.py | 6 ++-- tests/app/main/views/test_conversation.py | 35 +++++++++++++++++-- tests/app/main/views/test_template_folders.py | 10 +----- tests/app/models/test_service.py | 17 +++------ tests/conftest.py | 11 +++++- 6 files changed, 55 insertions(+), 28 deletions(-) diff --git a/app/main/views/conversation.py b/app/main/views/conversation.py index 65016bca3..2916c66de 100644 --- a/app/main/views/conversation.py +++ b/app/main/views/conversation.py @@ -1,5 +1,5 @@ from flask import jsonify, redirect, render_template, session, url_for -from flask_login import login_required +from flask_login import current_user, login_required from notifications_python_client.errors import HTTPError from notifications_utils.recipients import format_phone_number_human_readable from notifications_utils.template import SMSPreviewTemplate @@ -44,7 +44,7 @@ def conversation_reply( service_id, notification_id, ): - templates = current_service.get_templates('sms') + templates = current_service.get_user_templates_across_folders(current_user.id, template_type='sms') return render_template( 'views/templates/choose-reply.html', templates=templates, diff --git a/app/models/service.py b/app/models/service.py index 844bcaaae..f37dfea8d 100644 --- a/app/models/service.py +++ b/app/models/service.py @@ -154,9 +154,11 @@ class Service(): return {template['id'] for template in self.all_templates} def get_templates_for_folder(self, template_type, all_templates, folder_id): + if isinstance(template_type, str): + template_type = [template_type] return [ template for template in all_templates - if (set([template_type]) & {'all', template['template_type']}) + if (set(template_type) & {'all', template['template_type']}) and (template.get('folder') == folder_id) ] @@ -176,10 +178,10 @@ class Service(): folders = self.all_template_folders all_templates = self.all_templates user_templates = [] + user_templates += self.get_templates_for_folder(template_type, all_templates, None) for folder in folders: if user_id in folder.get("users_with_permission", []): user_templates += self.get_templates_for_folder(template_type, all_templates, folder["id"]) - user_templates += self.get_templates_for_folder(template_type, all_templates, None) return user_templates @property diff --git a/tests/app/main/views/test_conversation.py b/tests/app/main/views/test_conversation.py index adbbcae45..59b1be442 100644 --- a/tests/app/main/views/test_conversation.py +++ b/tests/app/main/views/test_conversation.py @@ -10,10 +10,14 @@ from notifications_python_client.errors import HTTPError from app.main.views.conversation import get_user_number from tests.conftest import ( SERVICE_ONE_ID, + _template, mock_get_notifications, normalize_spaces, ) +INV_PARENT_FOLDER_ID = '7e979e79-d970-43a5-ac69-b625a8d147b0' +VIS_PARENT_FOLDER_ID = 'bbbb222b-2b22-2b22-222b-b222b22b2222' + def test_get_user_phone_number_when_only_inbound_exists(mocker): @@ -255,6 +259,7 @@ def test_conversation_reply_shows_link_to_add_templates_if_service_has_no_templa client_request, fake_uuid, mock_get_service_templates_when_no_templates_exist, + mock_get_template_folders, ): page = client_request.get( 'main.conversation_reply', @@ -275,8 +280,32 @@ def test_conversation_reply_shows_link_to_add_templates_if_service_has_no_templa def test_conversation_reply_shows_templates( client_request, fake_uuid, - mock_get_service_templates, + mocker, + mock_get_template_folders, + active_user_with_permissions ): + + all_templates = {'data': [ + _template('sms', 'sms_template_one', parent=INV_PARENT_FOLDER_ID), + _template('sms', 'sms_template_two'), + _template('sms', 'sms_template_three', parent=VIS_PARENT_FOLDER_ID), + _template('letter', 'letter_template_one') + ]} + mocker.patch('app.service_api_client.get_service_templates', return_value=all_templates) + mock_get_template_folders.return_value = [ + { + 'name': "Parent 1 - invisible", + 'id': INV_PARENT_FOLDER_ID, + 'parent_id': None, + 'users_with_permission': [] + }, + { + 'name': "Parent 2 - visible", + 'id': VIS_PARENT_FOLDER_ID, + 'parent_id': None, + 'users_with_permission': [active_user_with_permissions.id] + }, + ] page = client_request.get( 'main.conversation_reply', service_id=SERVICE_ONE_ID, @@ -284,8 +313,8 @@ def test_conversation_reply_shows_templates( ) for index, expected in enumerate([ - 'sms_template_one', 'sms_template_two', + 'sms_template_three' ]): link = page.select('.message-name')[index] assert normalize_spaces(link.text) == expected @@ -303,6 +332,7 @@ def test_conversation_reply_shows_live_search_if_list_of_templates_taller_than_s client_request, fake_uuid, mock_get_more_service_templates_than_can_fit_onscreen, + mock_get_template_folders, ): page = client_request.get( 'main.conversation_reply', @@ -317,6 +347,7 @@ def test_conversation_reply_shows_live_search_if_list_of_templates_fits_onscreen client_request, fake_uuid, mock_get_service_templates, + mock_get_template_folders, ): page = client_request.get( 'main.conversation_reply', diff --git a/tests/app/main/views/test_template_folders.py b/tests/app/main/views/test_template_folders.py index 1b0db751a..df15bef50 100644 --- a/tests/app/main/views/test_template_folders.py +++ b/tests/app/main/views/test_template_folders.py @@ -7,6 +7,7 @@ from notifications_python_client.errors import HTTPError from tests.conftest import ( SERVICE_ONE_ID, TEMPLATE_ONE_ID, + _template, active_caseworking_user, active_user_view_permissions, active_user_with_permissions, @@ -31,15 +32,6 @@ def _folder(name, folder_id=None, parent=None, users_with_permission=None): } -def _template(template_type, name, parent=None, template_id=None): - return { - 'id': template_id or str(uuid.uuid4()), - 'name': name, - 'template_type': template_type, - 'folder': parent, - } - - @pytest.mark.parametrize( ( 'expected_title_tag,' diff --git a/tests/app/models/test_service.py b/tests/app/models/test_service.py index bd7ce2d1f..8abdd21c3 100644 --- a/tests/app/models/test_service.py +++ b/tests/app/models/test_service.py @@ -1,6 +1,7 @@ import uuid from app.models.service import Service +from tests.conftest import _template INV_PARENT_FOLDER_ID = '7e979e79-d970-43a5-ac69-b625a8d147b0' INV_CHILD_1_FOLDER_ID = '92ee1ee0-e4ee-4dcc-b1a7-a5da9ebcfa2b' @@ -67,15 +68,6 @@ def _get_all_folders(active_user_with_permissions): ] -def _template(template_type, name, parent=None, template_id=None): - return { - 'id': template_id or str(uuid.uuid4()), - 'name': name, - 'template_type': template_type, - 'folder': parent, - } - - def test_get_user_template_folders_only_returns_folders_visible_to_user( mock_get_template_folders, service_one, @@ -201,12 +193,13 @@ def test_get_user_templates_across_folders( service = Service(service_one) result = service.get_user_templates_across_folders(active_user_with_permissions.id) assert result == [ - {'folder': 'bbbb222b-2b22-2b22-222b-b222b22b2222', 'id': mocker.ANY, - 'name': 'email_template_one', 'template_type': 'email'}, {'folder': None, 'id': mocker.ANY, 'name': 'sms_template_two', 'template_type': 'sms'}, {'folder': None, 'id': mocker.ANY, - 'name': 'letter_template_one', 'template_type': 'letter'}] + 'name': 'letter_template_one', 'template_type': 'letter'}, + {'folder': 'bbbb222b-2b22-2b22-222b-b222b22b2222', 'id': mocker.ANY, + 'name': 'email_template_one', 'template_type': 'email'} + ] def test_get_user_templates_across_folders_sms_only( diff --git a/tests/conftest.py b/tests/conftest.py index de4566c59..314f89a81 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -3,7 +3,7 @@ import os from contextlib import contextmanager from datetime import date, datetime, timedelta from unittest.mock import Mock -from uuid import UUID +from uuid import UUID, uuid4 import pytest from bs4 import BeautifulSoup @@ -986,6 +986,15 @@ def create_service_templates(service_id, number_of_templates=6): return {'data': service_templates} +def _template(template_type, name, parent=None, template_id=None): + return { + 'id': template_id or str(uuid4()), + 'name': name, + 'template_type': template_type, + 'folder': parent, + } + + @pytest.fixture(scope='function') def mock_get_service_templates(mocker): def _create(service_id): From 0743a68e099b835dc06e3c9d14af875e0ae6a4d9 Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Thu, 21 Mar 2019 15:57:52 +0000 Subject: [PATCH 3/3] Reflect template folder structure on inbound conversation reply page --- app/main/views/conversation.py | 15 ++++-- app/models/service.py | 26 +++------ .../views/templates/choose-reply.html | 33 +++++++++--- tests/app/main/views/test_conversation.py | 53 +++++++++---------- tests/app/models/test_service.py | 48 ----------------- 5 files changed, 67 insertions(+), 108 deletions(-) diff --git a/app/main/views/conversation.py b/app/main/views/conversation.py index 2916c66de..83a06cfb7 100644 --- a/app/main/views/conversation.py +++ b/app/main/views/conversation.py @@ -7,6 +7,7 @@ from notifications_utils.template import SMSPreviewTemplate from app import current_service, notification_api_client, service_api_client from app.main import main from app.main.forms import SearchByNameForm +from app.models.template_list import TemplateList from app.utils import user_has_permissions @@ -38,20 +39,26 @@ def conversation_updates(service_id, notification_id): @main.route("/services//conversation//reply-with") +@main.route("/services//conversation//reply-with/from-folder/") @login_required @user_has_permissions('send_messages') def conversation_reply( service_id, notification_id, + from_folder=None, ): - templates = current_service.get_user_templates_across_folders(current_user.id, template_type='sms') return render_template( 'views/templates/choose-reply.html', - templates=templates, - show_search_box=(len(templates) > 7), - template_type='sms', + templates_and_folders=TemplateList( + current_service, + template_folder_id=from_folder, + user_id=current_user.id, + template_type='sms' + ), + template_folder_path=current_service.get_template_folder_path(from_folder), search_form=SearchByNameForm(), notification_id=notification_id, + template_type='sms' ) diff --git a/app/models/service.py b/app/models/service.py index f37dfea8d..6408ebdba 100644 --- a/app/models/service.py +++ b/app/models/service.py @@ -142,6 +142,7 @@ class Service(): @cached_property def all_templates(self): + templates = service_api_client.get_service_templates(self.id)['data'] return [ @@ -153,15 +154,6 @@ class Service(): def all_template_ids(self): return {template['id'] for template in self.all_templates} - def get_templates_for_folder(self, template_type, all_templates, folder_id): - if isinstance(template_type, str): - template_type = [template_type] - return [ - template for template in all_templates - if (set(template_type) & {'all', template['template_type']}) - and (template.get('folder') == folder_id) - ] - def get_templates(self, template_type='all', template_folder_id=None, user_id=None): if user_id and template_folder_id and self.has_permission('edit_folder_permissions'): folder = self.get_template_folder(template_folder_id) @@ -172,17 +164,11 @@ class Service(): template_type = [template_type] if template_folder_id: template_folder_id = str(template_folder_id) - return self.get_templates_for_folder(template_type, self.all_templates, template_folder_id) - - def get_user_templates_across_folders(self, user_id, template_type='all'): - folders = self.all_template_folders - all_templates = self.all_templates - user_templates = [] - user_templates += self.get_templates_for_folder(template_type, all_templates, None) - for folder in folders: - if user_id in folder.get("users_with_permission", []): - user_templates += self.get_templates_for_folder(template_type, all_templates, folder["id"]) - return user_templates + return [ + template for template in self.all_templates + if (set(template_type) & {'all', template['template_type']}) + and template.get('folder') == template_folder_id + ] @property def available_template_types(self): diff --git a/app/templates/views/templates/choose-reply.html b/app/templates/views/templates/choose-reply.html index 05856fb89..23b5a68bf 100644 --- a/app/templates/views/templates/choose-reply.html +++ b/app/templates/views/templates/choose-reply.html @@ -2,6 +2,7 @@ {% from "components/message-count-label.html" import message_count_label %} {% from "components/textbox.html" import textbox %} {% from "components/live-search.html" import live_search %} +{% from "components/folder-path.html" import folder_path %} {% extends "withnav_template.html" %} @@ -11,9 +12,12 @@ {% block maincolumn_content %} +

Choose a template

+ {{ folder_path(template_folder_path, current_service.id, template_type) }} +
- {% if not templates %} + {% if not templates_and_folders.templates_to_show %} {% if current_user.has_permissions('manage_templates') %}

@@ -29,20 +33,35 @@ {% else %} - {{ live_search(target_selector='#template-list .column-whole', show=show_search_box, form=search_form) }} + {{ live_search(target_selector='#template-list .column-whole', show=True, form=search_form) }} -