diff --git a/app/main/views/templates.py b/app/main/views/templates.py index ba12244dc..261f578ff 100644 --- a/app/main/views/templates.py +++ b/app/main/views/templates.py @@ -29,7 +29,7 @@ from app.main.forms import ( ) from app.main.views.send import get_example_csv_rows, get_sender_details from app.models.service import Service -from app.models.template_list import TemplateList +from app.models.template_list import TemplateList, TemplateLists from app.template_previews import TemplatePreview, get_page_count_for_letter from app.utils import ( email_or_sms_not_enabled, @@ -322,16 +322,44 @@ def _add_template_by_type(template_type, template_folder_id): @main.route("/services//templates/copy") +@main.route("/services//templates/copy/from-folder/") +@main.route("/services//templates/copy/from-service/") +@main.route("/services//templates/copy/from-service//from-folder/") @login_required @user_has_permissions('manage_templates') -def choose_template_to_copy(service_id): - return render_template( - 'views/templates/copy.html', - services=[ - Service(service) - for service in user_api_client.get_services_for_user(current_user) - ], - ) +def choose_template_to_copy( + service_id, + from_service=None, + from_folder=None, +): + + if from_service: + + current_user.belongs_to_service_or_403(from_service) + service = Service( + service_api_client.get_service(from_service)['data'] + ) + + return render_template( + 'views/templates/copy.html', + services_templates_and_folders=TemplateList( + service, + template_folder_id=from_folder, + ), + template_folder_path=service.get_template_folder_path(from_folder), + from_service=service, + search_form=SearchTemplatesForm(), + ) + + else: + return render_template( + 'views/templates/copy.html', + services_templates_and_folders=TemplateLists([ + Service(service) for service in + user_api_client.get_services_for_user(current_user) + ]), + search_form=SearchTemplatesForm(), + ) @main.route("/services//templates/copy/", methods=['GET', 'POST']) @@ -339,10 +367,7 @@ def choose_template_to_copy(service_id): @user_has_permissions('manage_templates') def copy_template(service_id, template_id): - if not user_api_client.user_belongs_to_service( - current_user, request.args.get('from_service') - ): - abort(403) + current_user.belongs_to_service_or_403(request.args.get('from_service')) template = service_api_client.get_service_template( request.args.get('from_service'), diff --git a/app/models/service.py b/app/models/service.py index 3337d43ca..e159e5afa 100644 --- a/app/models/service.py +++ b/app/models/service.py @@ -61,7 +61,7 @@ class Service(): def _get_by_id(self, things, id): try: - return next(thing for thing in things if thing['id'] == id) + return next(thing for thing in things if thing['id'] == str(id)) except StopIteration: abort(404) @@ -132,6 +132,8 @@ class Service(): def get_templates(self, template_type='all', template_folder_id=None): if isinstance(template_type, str): 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']}) @@ -330,6 +332,10 @@ class Service(): return {folder['id'] for folder in self.all_template_folders} def get_template_folders(self, template_type='all', parent_folder_id=None): + + if parent_folder_id: + parent_folder_id = str(parent_folder_id) + return [ folder for folder in self.all_template_folders if ( diff --git a/app/models/template_list.py b/app/models/template_list.py index 329ceab1f..be8e4e121 100644 --- a/app/models/template_list.py +++ b/app/models/template_list.py @@ -30,6 +30,7 @@ class TemplateList(): self.template_type, item['id'] ), ancestors=ancestors, + service_id=self.service.id, ) for sub_item in self.get_templates_and_folders( template_type, item['id'], ancestors + [item] @@ -42,6 +43,7 @@ class TemplateList(): yield TemplateListTemplate( item, ancestors=ancestors, + service_id=self.service.id, ) @property @@ -59,8 +61,47 @@ class TemplateList(): )) +class TemplateLists(): + + def __init__(self, services): + self.services = sorted( + services, + key=lambda service: service.name.lower(), + ) + + def __iter__(self): + + if len(self.services) == 1: + + for template_or_folder in TemplateList(self.services[0]): + yield template_or_folder + + return + + for service in self.services: + + template_list_service = TemplateListService(service) + + yield template_list_service + + for service_templates_and_folders in TemplateList( + service + ).get_templates_and_folders( + template_type='all', + template_folder_id=None, + ancestors=[template_list_service], + ): + yield service_templates_and_folders + + @property + def templates_to_show(self): + return bool(self.services) + + class TemplateListItem(): + is_service = False + def __init__( self, template_or_folder, @@ -79,8 +120,10 @@ class TemplateListTemplate(TemplateListItem): self, template, ancestors, + service_id, ): super().__init__(template, ancestors) + self.service_id = service_id self.hint = { 'email': 'Email template', 'sms': 'Text message template', @@ -98,8 +141,10 @@ class TemplateListFolder(TemplateListItem): templates, folders, ancestors, + service_id, ): super().__init__(folder, ancestors) + self.service_id = service_id self.number_of_templates = len(templates) self.number_of_folders = len(folders) @@ -122,3 +167,24 @@ class TemplateListFolder(TemplateListItem): @property def hint(self): return ', '.join(self._hint_parts) + + +class TemplateListService(TemplateListFolder): + + is_service = True + + def __init__( + self, + service, + ): + super().__init__( + folder=service._dict, + templates=service.get_templates( + template_folder_id=None, + ), + folders=service.get_template_folders( + parent_folder_id=None, + ), + ancestors=[], + service_id=service.id, + ) diff --git a/app/models/user.py b/app/models/user.py index 7b9ffd6ec..aa92a3f5d 100644 --- a/app/models/user.py +++ b/app/models/user.py @@ -1,6 +1,6 @@ from itertools import chain -from flask import request, session +from flask import abort, request, session from flask_login import AnonymousUserMixin, UserMixin from app.utils import is_gov_user @@ -151,6 +151,10 @@ class User(UserMixin): def has_permission_for_service(self, service_id, permission): return permission in self._permissions.get(service_id, []) + def belongs_to_service_or_403(self, service_id): + if str(service_id) not in self.services: + abort(403) + def is_locked(self): return self.failed_login_count >= self.max_failed_login_count diff --git a/app/notify_client/user_api_client.py b/app/notify_client/user_api_client.py index dfc3af797..869e8030b 100644 --- a/app/notify_client/user_api_client.py +++ b/app/notify_client/user_api_client.py @@ -230,7 +230,7 @@ class UserApiClient(NotifyAdminAPIClient): } def user_belongs_to_service(self, user, service_id): - return service_id in self.get_service_ids_for_user(user) + return str(service_id) in self.get_service_ids_for_user(user) user_api_client = UserApiClient() diff --git a/app/templates/components/folder-path.html b/app/templates/components/folder-path.html index bfcadd3bc..c19af80f9 100644 --- a/app/templates/components/folder-path.html +++ b/app/templates/components/folder-path.html @@ -28,6 +28,37 @@ {% endmacro %} +{% macro copy_folder_path( + folder_path, + current_service_id, + from_service +) %} + {% if folder_path %} +

+ {% if folder_path|length == 1 %} + Services + {{ folder_path_separator() }} + {% endif %} + {% for folder in folder_path %} + {% if loop.last %} + + {{ folder.name if folder.id else from_service.name }} + + {% else %} + {% if folder.id %} + {{ folder.name }} {% if not loop.last %}{{ folder_path_separator() }}{% endif %} + {% elif folder.parent_id == None %} + {{ from_service.name }} {% if not loop.last %}{{ folder_path_separator() }}{% endif %} + {% else %} + {{ from_service.name }} {% if not loop.last %}{{ folder_path_separator() }}{% endif %} + {% endif %} + {% endif %} + {% endfor %} +

+ {% endif %} +{% endmacro %} + + {% macro page_title_folder_path( folders, fallback_page_title=None, diff --git a/app/templates/views/templates/copy.html b/app/templates/views/templates/copy.html index 404e93db4..3b1c08c66 100644 --- a/app/templates/views/templates/copy.html +++ b/app/templates/views/templates/copy.html @@ -1,4 +1,5 @@ -{% from "components/message-count-label.html" import message_count_label %} +{% from "components/folder-path.html" import copy_folder_path, page_title_folder_path %} +{% from "components/live-search.html" import live_search %} {% extends "withnav_template.html" %} @@ -8,30 +9,49 @@ {% block maincolumn_content %} -
+

Copy an existing template

+ {{ copy_folder_path(template_folder_path, current_service.id, from_service) }}
- + {% endif %} {% endblock %} diff --git a/tests/app/main/views/test_template_folders.py b/tests/app/main/views/test_template_folders.py index 296b3d94e..fbde3fd8b 100644 --- a/tests/app/main/views/test_template_folders.py +++ b/tests/app/main/views/test_template_folders.py @@ -28,9 +28,9 @@ def _folder(name, folder_id=None, parent=None): } -def _template(template_type, name, parent=None): +def _template(template_type, name, parent=None, template_id=None): return { - 'id': str(uuid.uuid4()), + 'id': template_id or str(uuid.uuid4()), 'name': name, 'template_type': template_type, 'folder': parent, diff --git a/tests/app/main/views/test_templates.py b/tests/app/main/views/test_templates.py index 786d9dfbc..2f7625c23 100644 --- a/tests/app/main/views/test_templates.py +++ b/tests/app/main/views/test_templates.py @@ -17,7 +17,13 @@ from tests import ( template_json, validate_route_permission, ) -from tests.app.main.views.test_template_folders import PARENT_FOLDER_ID, _folder +from tests.app.main.views.test_template_folders import ( + CHILD_FOLDER_ID, + FOLDER_TWO_ID, + PARENT_FOLDER_ID, + _folder, + _template, +) from tests.conftest import ( SERVICE_ONE_ID, SERVICE_TWO_ID, @@ -772,6 +778,7 @@ def test_choosing_to_copy_redirects( def test_choose_a_template_to_copy( client_request, mock_get_service_templates, + mock_get_template_folders, mock_get_non_empty_organisations_and_services_for_user, ): page = client_request.get( @@ -779,45 +786,141 @@ def test_choose_a_template_to_copy( service_id=SERVICE_ONE_ID, ) - assert normalize_spaces( - page.select_one('main nav').text - ) == normalize_spaces( - 'Org 1 service 1 ' - ' sms_template_one' - ' Text message template' - ' sms_template_two' - ' Text message template' - ' email_template_one' - ' Email template' - ' email_template_two' - ' Email template ' - 'Org 1 service 2 ' - ' sms_template_one' - ' Text message template' - ' sms_template_two' - ' Text message template' - ' email_template_one' - ' Email template' - ' email_template_two' - ' Email template ' - 'Service 1 ' - ' sms_template_one ' - ' Text message template' - ' sms_template_two Text message template' - ' email_template_one Email template' - ' email_template_two Email template ' - 'Service 2 ' - ' sms_template_one' - ' Text message template' - ' sms_template_two' - ' Text message template' - ' email_template_one' - ' Email template' - ' email_template_two' - ' Email template' - ) + assert page.select('.folder-heading') == [] - assert page.select_one('main nav a')['href'] == url_for( + expected = [ + ( + 'Org 1 service 1 ' + '6 templates' + ), + ( + 'Org 1 service 1 / sms_template_one ' + 'Text message template' + ), + ( + 'Org 1 service 1 / sms_template_two ' + 'Text message template' + ), + ( + 'Org 1 service 1 / email_template_one ' + 'Email template' + ), + ( + 'Org 1 service 1 / email_template_two ' + 'Email template' + ), + ( + 'Org 1 service 1 / letter_template_one ' + 'Letter template' + ), + ( + 'Org 1 service 1 / letter_template_two ' + 'Letter template' + ), + ( + 'Org 1 service 2 ' + '6 templates' + ), + ( + 'Org 1 service 2 / sms_template_one ' + 'Text message template' + ), + ( + 'Org 1 service 2 / sms_template_two ' + 'Text message template' + ), + ( + 'Org 1 service 2 / email_template_one ' + 'Email template' + ), + ( + 'Org 1 service 2 / email_template_two ' + 'Email template' + ), + ( + 'Org 1 service 2 / letter_template_one ' + 'Letter template' + ), + ( + 'Org 1 service 2 / letter_template_two ' + 'Letter template' + ), + ( + 'Service 1 ' + '6 templates' + ), + ( + 'Service 1 / sms_template_one ' + 'Text message template' + ), + ( + 'Service 1 / sms_template_two ' + 'Text message template' + ), + ( + 'Service 1 / email_template_one ' + 'Email template' + ), + ( + 'Service 1 / email_template_two ' + 'Email template' + ), + ( + 'Service 1 / letter_template_one ' + 'Letter template' + ), + ( + 'Service 1 / letter_template_two ' + 'Letter template' + ), + ( + 'Service 2 ' + '6 templates' + ), + ( + 'Service 2 / sms_template_one ' + 'Text message template' + ), + ( + 'Service 2 / sms_template_two ' + 'Text message template' + ), + ( + 'Service 2 / email_template_one ' + 'Email template' + ), + ( + 'Service 2 / email_template_two ' + 'Email template' + ), + ( + 'Service 2 / letter_template_one ' + 'Letter template' + ), + ( + 'Service 2 / letter_template_two ' + 'Letter template' + ), + ] + actual = page.select('.template-list-item') + + assert len(actual) == len(expected) + + for actual, expected in zip(actual, expected): + assert normalize_spaces(actual.text) == expected + + links = page.select('main nav a') + assert links[0]['href'] == url_for( + 'main.choose_template_to_copy', + service_id=SERVICE_ONE_ID, + from_service=SERVICE_TWO_ID, + ) + assert links[1]['href'] == url_for( + 'main.choose_template_to_copy', + service_id=SERVICE_ONE_ID, + from_service=SERVICE_TWO_ID, + ) + assert links[2]['href'] == url_for( 'main.copy_template', service_id=SERVICE_ONE_ID, template_id=TEMPLATE_ONE_ID, @@ -825,6 +928,160 @@ def test_choose_a_template_to_copy( ) +def test_choose_a_template_to_copy_when_user_has_one_service( + client_request, + mock_get_service_templates, + mock_get_template_folders, + mock_get_empty_organisations_and_one_service_for_user, +): + page = client_request.get( + 'main.choose_template_to_copy', + service_id=SERVICE_ONE_ID, + ) + + assert page.select('.folder-heading') == [] + + expected = [ + ( + 'sms_template_one ' + 'Text message template' + ), + ( + 'sms_template_two ' + 'Text message template' + ), + ( + 'email_template_one ' + 'Email template' + ), + ( + 'email_template_two ' + 'Email template' + ), + ( + 'letter_template_one ' + 'Letter template' + ), + ( + 'letter_template_two ' + 'Letter template' + ), + ] + actual = page.select('.template-list-item') + + assert len(actual) == len(expected) + + for actual, expected in zip(actual, expected): + assert normalize_spaces(actual.text) == expected + + assert page.select('main nav a')[0]['href'] == url_for( + 'main.copy_template', + service_id=SERVICE_ONE_ID, + template_id=TEMPLATE_ONE_ID, + from_service=SERVICE_TWO_ID, + ) + + +def test_choose_a_template_to_copy_from_folder_within_service( + mocker, + client_request, + mock_get_template_folders, + mock_get_non_empty_organisations_and_services_for_user, +): + mock_get_template_folders.return_value = [ + _folder('Parent folder', PARENT_FOLDER_ID), + _folder('Child folder empty', CHILD_FOLDER_ID, parent=PARENT_FOLDER_ID), + _folder('Child folder non-empty', FOLDER_TWO_ID, parent=PARENT_FOLDER_ID), + ] + mocker.patch( + 'app.service_api_client.get_service_templates', + return_value={'data': [ + _template( + 'sms', + 'Should not appear in list (at service root)', + ), + _template( + 'sms', + 'Should appear in list (at same level)', + parent=PARENT_FOLDER_ID, + ), + _template( + 'sms', + 'Should appear in list (nested)', + parent=FOLDER_TWO_ID, + template_id=TEMPLATE_ONE_ID, + ), + ]} + ) + page = client_request.get( + 'main.choose_template_to_copy', + service_id=SERVICE_ONE_ID, + from_service=SERVICE_ONE_ID, + from_folder=PARENT_FOLDER_ID, + ) + + assert normalize_spaces(page.select_one('.folder-heading').text) == ( + 'service one / Parent folder' + ) + breadcrumb_links = page.select('.folder-heading a') + assert len(breadcrumb_links) == 1 + assert breadcrumb_links[0]['href'] == url_for( + 'main.choose_template_to_copy', + service_id=SERVICE_ONE_ID, + from_service=SERVICE_ONE_ID, + ) + + expected = [ + ( + 'Child folder empty ' + 'Empty' + ), + ( + 'Child folder non-empty ' + '1 template' + ), + ( + 'Child folder non-empty / Should appear in list (nested) ' + 'Text message template' + ), + ( + 'Should appear in list (at same level) ' + 'Text message template' + ), + ] + actual = page.select('.template-list-item') + + assert len(actual) == len(expected) + + for actual, expected in zip(actual, expected): + assert normalize_spaces(actual.text) == expected + + links = page.select('main nav a') + assert links[0]['href'] == url_for( + 'main.choose_template_to_copy', + service_id=SERVICE_ONE_ID, + from_service=SERVICE_ONE_ID, + from_folder=CHILD_FOLDER_ID, + ) + assert links[1]['href'] == url_for( + 'main.choose_template_to_copy', + service_id=SERVICE_ONE_ID, + from_service=SERVICE_ONE_ID, + from_folder=FOLDER_TWO_ID, + ) + assert links[2]['href'] == url_for( + 'main.choose_template_to_copy', + service_id=SERVICE_ONE_ID, + from_folder=FOLDER_TWO_ID, + ) + assert links[3]['href'] == url_for( + 'main.copy_template', + service_id=SERVICE_ONE_ID, + template_id=TEMPLATE_ONE_ID, + from_service=SERVICE_ONE_ID, + ) + + @pytest.mark.parametrize('existing_template_names, expected_name', ( ( ['Two week reminder'], @@ -853,6 +1110,7 @@ def test_choose_a_template_to_copy( )) def test_load_edit_template_with_copy_of_template( client_request, + active_user_with_permission_to_two_services, mock_get_service_templates, mock_get_service_email_template, mock_get_non_empty_organisations_and_services_for_user, @@ -863,6 +1121,7 @@ def test_load_edit_template_with_copy_of_template( {'name': existing_template_name, 'template_type': 'sms'} for existing_template_name in existing_template_names ]} + client_request.login(active_user_with_permission_to_two_services) page = client_request.get( 'main.copy_template', service_id=SERVICE_ONE_ID, diff --git a/tests/conftest.py b/tests/conftest.py index db5389bb0..33f972e07 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -966,7 +966,7 @@ def create_service_templates(service_id, number_of_templates=6): service_templates.append(template_json( service_id, - TEMPLATE_ONE_ID if _ == 1 else str(generate_uuid), + TEMPLATE_ONE_ID if _ == 1 else str(generate_uuid()), "{}_template_{}".format(template_type, template_number), template_type, "{} template {} content".format(template_type, template_number), @@ -1192,6 +1192,41 @@ def active_user_with_permissions(fake_uuid): return user +@pytest.fixture(scope='function') +def active_user_with_permission_to_two_services(fake_uuid): + from app.notify_client.user_api_client import User + + permissions = [ + 'send_texts', + 'send_emails', + 'send_letters', + 'manage_users', + 'manage_templates', + 'manage_settings', + 'manage_api_keys', + 'view_activity', + ] + + return User({ + 'id': fake_uuid, + 'name': 'Test User', + 'password': 'somepassword', + 'password_changed_at': str(datetime.utcnow()), + 'email_address': 'test@user.gov.uk', + 'mobile_number': '07700 900762', + 'state': 'active', + 'failed_login_count': 0, + 'permissions': { + SERVICE_ONE_ID: permissions, + SERVICE_TWO_ID: permissions, + }, + 'platform_admin': False, + 'auth_type': 'sms_auth', + 'organisations': [ORGANISATION_ID], + 'services': [SERVICE_ONE_ID, SERVICE_TWO_ID], + }) + + @pytest.fixture(scope='function') def active_caseworking_user(fake_uuid): from app.notify_client.user_api_client import User @@ -3224,6 +3259,24 @@ def mock_get_non_empty_organisations_and_services_for_user(mocker, organisation_ ) +@pytest.fixture +def mock_get_empty_organisations_and_one_service_for_user(mocker, organisation_one, api_user_active): + + def _get_orgs_and_services(user_id): + return { + 'organisations': [], + 'services_without_organisations': [{ + 'name': 'Only service', + 'id': SERVICE_TWO_ID, + }] + } + + return mocker.patch( + 'app.user_api_client.get_organisations_and_services_for_user', + side_effect=_get_orgs_and_services + ) + + @pytest.fixture def mock_create_event(mocker): """