From f500db44f1faec291521ead03db81c1429aa4a4f Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Thu, 26 May 2022 13:53:12 +0100 Subject: [PATCH 01/12] Reuse TemplateList class when deleting a folder Part of moving "get_template_folders" et al. into TemplateList so we can cache it more effectively. This is slightly less efficient as iterating a TemplateList will instantiate an object for each item in the folder; but the difference is minimal. Note that: - The default template_type for TemplateList is "all". - We need to pass realistic template "JSON" in the test now. --- app/main/views/templates.py | 5 ++--- app/models/service.py | 6 ------ tests/app/main/views/test_template_folders.py | 4 ++-- 3 files changed, 4 insertions(+), 11 deletions(-) diff --git a/app/main/views/templates.py b/app/main/views/templates.py index f3939a481..670f167a2 100644 --- a/app/main/views/templates.py +++ b/app/main/views/templates.py @@ -470,10 +470,9 @@ def manage_template_folder(service_id, template_folder_id): @user_has_permissions('manage_templates') def delete_template_folder(service_id, template_folder_id): template_folder = current_service.get_template_folder_with_user_permission_or_403(template_folder_id, current_user) + template_list = TemplateList(service=current_service, template_folder_id=template_folder_id) - if len(current_service.get_template_folders_and_templates( - template_type="all", template_folder_id=template_folder_id - )) > 0: + if not template_list.folder_is_empty: flash("You must empty this folder before you can delete it", 'info') return redirect( url_for( diff --git a/app/models/service.py b/app/models/service.py index 3694d8487..161cd2f60 100644 --- a/app/models/service.py +++ b/app/models/service.py @@ -650,12 +650,6 @@ class Service(JSONModel, SortByNameMixin): template, ] - def get_template_folders_and_templates(self, template_type, template_folder_id): - return ( - self.get_templates(template_type, template_folder_id) - + self.get_template_folders(template_type, template_folder_id) - ) - @property def count_of_templates_and_folders(self): return len(self.all_templates + self.all_template_folders) diff --git a/tests/app/main/views/test_template_folders.py b/tests/app/main/views/test_template_folders.py index 1b67611f4..77279e190 100644 --- a/tests/app/main/views/test_template_folders.py +++ b/tests/app/main/views/test_template_folders.py @@ -5,7 +5,7 @@ from flask import abort, url_for from notifications_python_client.errors import HTTPError from app.models.user import User -from tests import sample_uuid +from tests import sample_uuid, template_json from tests.conftest import ( SERVICE_ONE_ID, TEMPLATE_ONE_ID, @@ -968,7 +968,7 @@ def test_delete_template_folder_should_detect_non_empty_folder_on_get( ] mocker.patch( 'app.models.service.Service.get_templates', - return_value=[{'id': template_id, 'name': 'template'}], + return_value=[template_json(service_one['id'], template_id)], ) client_request.get( 'main.delete_template_folder', service_id=service_one['id'], From ad4ef1225145fae5ed1fb1214a6a863745158aab Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Thu, 26 May 2022 16:46:17 +0100 Subject: [PATCH 02/12] Reuse TemplateList to display move-to options This is slightly less efficient than getting the folder dicts from "get_user_template_folders" directly, since: - TemplateList returns both templates and folders. - TemplateList encapsulates the dicts in model classes. We'll compensate for this later on: - We'll introduce a new caching approach to make the call fast. - We'll expose a property to avoid the "if" in the comprehension. --- app/main/views/templates.py | 6 +++++- app/models/template_list.py | 1 + 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/app/main/views/templates.py b/app/main/views/templates.py index 670f167a2..ad713267d 100644 --- a/app/main/views/templates.py +++ b/app/main/views/templates.py @@ -104,8 +104,12 @@ def choose_template(service_id, template_type='all', template_folder_id=None): template_list = TemplateList(current_service, template_type, template_folder_id, current_user) + all_template_folders = [ + item.folder for item in TemplateList(service=current_service, user=current_user) if item.is_folder + ] + templates_and_folders_form = TemplateAndFoldersSelectionForm( - all_template_folders=current_service.get_user_template_folders(current_user), + all_template_folders=all_template_folders, template_list=template_list, template_type=template_type, available_template_types=current_service.available_template_types, diff --git a/app/models/template_list.py b/app/models/template_list.py index 144a4675a..e0e8b4563 100644 --- a/app/models/template_list.py +++ b/app/models/template_list.py @@ -157,6 +157,7 @@ class TemplateListFolder(TemplateListItem): service_id, ): super().__init__(folder, ancestors) + self.folder = folder self.service_id = service_id self.number_of_templates = len(templates) self.number_of_folders = len(folders) From a87138b9f90b96e2dae017659d82cf916da45249 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Thu, 26 May 2022 13:59:16 +0100 Subject: [PATCH 03/12] Refactor loop over TemplateList items This is a technique we'll use in later commits. --- app/models/template_list.py | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/app/models/template_list.py b/app/models/template_list.py index e0e8b4563..4f48c0863 100644 --- a/app/models/template_list.py +++ b/app/models/template_list.py @@ -76,12 +76,11 @@ class TemplateLists(): self.user = user def __iter__(self): - if len(self.services) == 1: - - for template_or_folder in TemplateList(self.services[0], user=self.user): - yield template_or_folder - + yield from TemplateList( + service=self.services[0], + user=self.user, + ) return for service in self.services: From b97bf19b45ffa297ff53bddd5aa5979a6a672d45 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Thu, 26 May 2022 16:15:56 +0100 Subject: [PATCH 04/12] Make creating TemplateListServices consistent TemplateListServices are used when we want to show the service as an additional layer of hierarchy when a user copies a template, potentially across services [^1]. Normally a TemplateFolder is given "folders" and "templates" by TemplateList [^2]; TemplateListService was doing it the other way round and getting its own instead. Using a subclass of TemplateList means we can make the approach consistent, which will support the caching approach later on, as well as simplifying how we work with templates and folders. [^1]: https://github.com/alphagov/notifications-admin/blob/2e637f801f1f1ebfe1ff19fba7e19bbf40b7cfb4/app/main/views/templates.py#L356 [^2]: https://github.com/alphagov/notifications-admin/blob/bef0382cca16ba87d04430f8cd391092b1dc91ef/app/models/template_list.py#L31-L36 --- app/models/template_list.py | 49 ++++++++++++++++++++++--------------- 1 file changed, 29 insertions(+), 20 deletions(-) diff --git a/app/models/template_list.py b/app/models/template_list.py index 4f48c0863..3e43aa30c 100644 --- a/app/models/template_list.py +++ b/app/models/template_list.py @@ -66,6 +66,28 @@ class TemplateList(): )) +class ServiceTemplateList(TemplateList): + def __iter__(self): + template_list_service = TemplateListService( + self.service, + templates=self.service.get_templates( + template_folder_id=None, + ), + folders=self.service.get_template_folders( + parent_folder_id=None, + ), + ) + + yield template_list_service + + yield from self.get_templates_and_folders( + self.template_type, + self.template_folder_id, + self.user, + ancestors=[template_list_service] + ) + + class TemplateLists(): def __init__(self, user): @@ -84,20 +106,10 @@ class TemplateLists(): return for service in self.services: - - template_list_service = TemplateListService(service) - - yield template_list_service - - for service_templates_and_folders in TemplateList( - service, user=self.user - ).get_templates_and_folders( - template_type='all', - template_folder_id=None, + yield from ServiceTemplateList( + service=service, user=self.user, - ancestors=[template_list_service], - ): - yield service_templates_and_folders + ) @property def templates_to_show(self): @@ -183,21 +195,18 @@ class TemplateListFolder(TemplateListItem): class TemplateListService(TemplateListFolder): - is_service = True def __init__( self, service, + templates, + folders, ): super().__init__( folder=service._dict, - templates=service.get_templates( - template_folder_id=None, - ), - folders=service.get_template_folders( - parent_folder_id=None, - ), + templates=templates, + folders=folders, ancestors=[], service_id=service.id, ) From 057b4ee7a5c5a0a67f7b0a7a57a13d4033e331e2 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Mon, 30 May 2022 14:20:41 +0100 Subject: [PATCH 05/12] Simplify tests for get_(user)_template_folders This will make it easier to understand the diff when we move these tests to operate via TemplateList. Despite the verbosity, the only attribute we were actually checking here was the name, as a way of identifying which folders had been returned. Looking at the three tests: 1. The first is checking we can correctly filter all folders that a user can access, which involves appending the names of any parent folders the user doesn't have direct access to. 2. The second is checking the same thing but also that we filter the set of folders to just the children of a parent. 3. The third is just checking the filtering of child folders, without any user filtering or name aggregation applied. I've adapted tests (2) and (3) to make it clearer what is tested, focussing the tests on a specific folder and its contents. --- tests/app/models/test_service.py | 110 ++++++++++--------------------- 1 file changed, 35 insertions(+), 75 deletions(-) diff --git a/tests/app/models/test_service.py b/tests/app/models/test_service.py index 908b49037..8a7454673 100644 --- a/tests/app/models/test_service.py +++ b/tests/app/models/test_service.py @@ -82,39 +82,20 @@ def test_get_user_template_folders_only_returns_folders_visible_to_user( ): mock_get_template_folders.return_value = _get_all_folders(active_user_with_permissions) service = Service(service_one) - result = service.get_user_template_folders(User(active_user_with_permissions)) - assert result == [ - { - 'name': ["Parent 1 - invisible", "1's Visible child"], - 'id': mocker.ANY, - 'parent_id': None, - 'users_with_permission': [active_user_with_permissions['id']], - }, - { - 'name': ["Parent 1 - invisible", ["1's Invisible child", "1's Visible grandchild"]], - 'id': mocker.ANY, - 'parent_id': None, - 'users_with_permission': [active_user_with_permissions['id']], - }, - { - 'name': "2's Visible child", - 'id': mocker.ANY, - 'parent_id': VIS_PARENT_FOLDER_ID, - 'users_with_permission': [active_user_with_permissions['id']], - }, - { - 'name': ["2's Invisible child", "2's Visible grandchild"], - 'id': mocker.ANY, - 'parent_id': VIS_PARENT_FOLDER_ID, - 'users_with_permission': [active_user_with_permissions['id']], - }, - { - 'name': "Parent 2 - visible", - 'id': VIS_PARENT_FOLDER_ID, - 'parent_id': None, - 'users_with_permission': [active_user_with_permissions['id']], - }, - ] + user = User(active_user_with_permissions) + + result_folder_names = tuple( + result['name'] for result in + service.get_user_template_folders(user) + ) + + assert result_folder_names == ( + ["Parent 1 - invisible", "1's Visible child"], + ["Parent 1 - invisible", ["1's Invisible child", "1's Visible grandchild"]], + "2's Visible child", + ["2's Invisible child", "2's Visible grandchild"], + "Parent 2 - visible", + ) def test_get_template_folders_shows_user_folders_when_user_id_passed_in( @@ -126,27 +107,17 @@ def test_get_template_folders_shows_user_folders_when_user_id_passed_in( ): mock_get_template_folders.return_value = _get_all_folders(active_user_with_permissions) service = Service(service_one) - result = service.get_template_folders(user=User(active_user_with_permissions)) - assert result == [ - { - 'name': ["Parent 1 - invisible", "1's Visible child"], - 'id': mocker.ANY, - 'parent_id': None, - 'users_with_permission': [active_user_with_permissions['id']] - }, - { - 'name': ["Parent 1 - invisible", ["1's Invisible child", "1's Visible grandchild"]], - 'id': mocker.ANY, - 'parent_id': None, - 'users_with_permission': [active_user_with_permissions['id']] - }, - { - 'name': "Parent 2 - visible", - 'id': VIS_PARENT_FOLDER_ID, - 'parent_id': None, - 'users_with_permission': [active_user_with_permissions['id']] - }, - ] + user = User(active_user_with_permissions) + + result_folder_names = tuple( + result['name'] for result in + service.get_template_folders(parent_folder_id=VIS_PARENT_FOLDER_ID, user=user) + ) + + assert result_folder_names == ( + "2's Visible child", + ["2's Invisible child", "2's Visible grandchild"], + ) def test_get_template_folders_shows_all_folders_when_user_id_not_passed_in( @@ -157,27 +128,16 @@ def test_get_template_folders_shows_all_folders_when_user_id_not_passed_in( ): mock_get_template_folders.return_value = _get_all_folders(active_user_with_permissions) service = Service(service_one) - result = service.get_template_folders() - assert result == [ - { - 'name': "Invisible folder", - 'id': mocker.ANY, - 'parent_id': None, - 'users_with_permission': [] - }, - { - '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']], - } - ] + + result_folder_names = tuple( + result['name'] for result in + service.get_template_folders(parent_folder_id=VIS_PARENT_FOLDER_ID) + ) + + assert result_folder_names == ( + "2's Invisible child", + "2's Visible child", + ) def test_organisation_type_when_services_organisation_has_no_org_type(mocker, service_one): From 487dc1b488e37bbd1af041b59470acb1471a1f43 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Mon, 30 May 2022 15:29:03 +0100 Subject: [PATCH 06/12] Test _template_folders functions via TemplateList This is part of the overall migration of the "_template_folders" methods to the TemplateList class. Moving the existing tests now will make the actual migration easier to follow. To emulate the second and third tests, we need to grab a specific folder from the TemplateList and then look at its folders - these are set based on "get_template_folders" as before. --- app/models/template_list.py | 1 + tests/app/models/test_service.py | 134 ---------------------- tests/app/models/test_template_list.py | 148 +++++++++++++++++++++++++ 3 files changed, 149 insertions(+), 134 deletions(-) create mode 100644 tests/app/models/test_template_list.py diff --git a/app/models/template_list.py b/app/models/template_list.py index 3e43aa30c..4621f6735 100644 --- a/app/models/template_list.py +++ b/app/models/template_list.py @@ -170,6 +170,7 @@ class TemplateListFolder(TemplateListItem): super().__init__(folder, ancestors) self.folder = folder self.service_id = service_id + self.folders = folders self.number_of_templates = len(templates) self.number_of_folders = len(folders) diff --git a/tests/app/models/test_service.py b/tests/app/models/test_service.py index 8a7454673..d10199a9a 100644 --- a/tests/app/models/test_service.py +++ b/tests/app/models/test_service.py @@ -1,144 +1,10 @@ -import uuid - import pytest from app.models.organisation import Organisation from app.models.service import Service -from app.models.user import User from tests import organisation_json, service_json from tests.conftest import ORGANISATION_ID, create_folder, create_template -INV_PARENT_FOLDER_ID = '7e979e79-d970-43a5-ac69-b625a8d147b0' -INV_CHILD_1_FOLDER_ID = '92ee1ee0-e4ee-4dcc-b1a7-a5da9ebcfa2b' -VIS_PARENT_FOLDER_ID = 'bbbb222b-2b22-2b22-222b-b222b22b2222' -INV_CHILD_2_FOLDER_ID = 'fafe723f-1d39-4a10-865f-e551e03d8886' - - -def _get_all_folders(active_user_with_permissions): - return [ - { - 'name': "Invisible folder", - 'id': str(uuid.uuid4()), - 'parent_id': None, - 'users_with_permission': [] - }, - { - 'name': "Parent 1 - invisible", - 'id': INV_PARENT_FOLDER_ID, - 'parent_id': None, - 'users_with_permission': [] - }, - { - 'name': "1's Visible child", - 'id': str(uuid.uuid4()), - 'parent_id': INV_PARENT_FOLDER_ID, - 'users_with_permission': [active_user_with_permissions['id']], - }, - { - 'name': "1's Invisible child", - 'id': INV_CHILD_1_FOLDER_ID, - 'parent_id': INV_PARENT_FOLDER_ID, - 'users_with_permission': [] - }, - { - 'name': "1's Visible grandchild", - 'id': str(uuid.uuid4()), - 'parent_id': INV_CHILD_1_FOLDER_ID, - 'users_with_permission': [active_user_with_permissions['id']], - }, - { - 'name': "Parent 2 - visible", - 'id': VIS_PARENT_FOLDER_ID, - 'parent_id': None, - 'users_with_permission': [active_user_with_permissions['id']], - }, - { - 'name': "2's Visible child", - 'id': str(uuid.uuid4()), - 'parent_id': VIS_PARENT_FOLDER_ID, - 'users_with_permission': [active_user_with_permissions['id']], - }, - { - 'name': "2's Invisible child", - 'id': INV_CHILD_2_FOLDER_ID, - 'parent_id': VIS_PARENT_FOLDER_ID, - 'users_with_permission': [] - }, - { - 'name': "2's Visible grandchild", - 'id': str(uuid.uuid4()), - 'parent_id': INV_CHILD_2_FOLDER_ID, - 'users_with_permission': [active_user_with_permissions['id']], - }, - ] - - -def test_get_user_template_folders_only_returns_folders_visible_to_user( - notify_admin, - mock_get_template_folders, - service_one, - active_user_with_permissions, - mocker -): - mock_get_template_folders.return_value = _get_all_folders(active_user_with_permissions) - service = Service(service_one) - user = User(active_user_with_permissions) - - result_folder_names = tuple( - result['name'] for result in - service.get_user_template_folders(user) - ) - - assert result_folder_names == ( - ["Parent 1 - invisible", "1's Visible child"], - ["Parent 1 - invisible", ["1's Invisible child", "1's Visible grandchild"]], - "2's Visible child", - ["2's Invisible child", "2's Visible grandchild"], - "Parent 2 - visible", - ) - - -def test_get_template_folders_shows_user_folders_when_user_id_passed_in( - notify_admin, - mock_get_template_folders, - service_one, - active_user_with_permissions, - mocker -): - mock_get_template_folders.return_value = _get_all_folders(active_user_with_permissions) - service = Service(service_one) - user = User(active_user_with_permissions) - - result_folder_names = tuple( - result['name'] for result in - service.get_template_folders(parent_folder_id=VIS_PARENT_FOLDER_ID, user=user) - ) - - assert result_folder_names == ( - "2's Visible child", - ["2's Invisible child", "2's Visible grandchild"], - ) - - -def test_get_template_folders_shows_all_folders_when_user_id_not_passed_in( - mock_get_template_folders, - service_one, - active_user_with_permissions, - mocker -): - mock_get_template_folders.return_value = _get_all_folders(active_user_with_permissions) - service = Service(service_one) - - result_folder_names = tuple( - result['name'] for result in - service.get_template_folders(parent_folder_id=VIS_PARENT_FOLDER_ID) - ) - - assert result_folder_names == ( - "2's Invisible child", - "2's Visible child", - ) - def test_organisation_type_when_services_organisation_has_no_org_type(mocker, service_one): service = Service(service_one) diff --git a/tests/app/models/test_template_list.py b/tests/app/models/test_template_list.py new file mode 100644 index 000000000..b8a3d5f4f --- /dev/null +++ b/tests/app/models/test_template_list.py @@ -0,0 +1,148 @@ +import uuid + +from app.models.service import Service +from app.models.template_list import TemplateList +from app.models.user import User + +INV_PARENT_FOLDER_ID = '7e979e79-d970-43a5-ac69-b625a8d147b0' +INV_CHILD_1_FOLDER_ID = '92ee1ee0-e4ee-4dcc-b1a7-a5da9ebcfa2b' +VIS_PARENT_FOLDER_ID = 'bbbb222b-2b22-2b22-222b-b222b22b2222' +INV_CHILD_2_FOLDER_ID = 'fafe723f-1d39-4a10-865f-e551e03d8886' + + +def _get_all_folders(active_user_with_permissions): + return [ + { + 'name': "Invisible folder", + 'id': str(uuid.uuid4()), + 'parent_id': None, + 'users_with_permission': [] + }, + { + 'name': "Parent 1 - invisible", + 'id': INV_PARENT_FOLDER_ID, + 'parent_id': None, + 'users_with_permission': [] + }, + { + 'name': "1's Visible child", + 'id': str(uuid.uuid4()), + 'parent_id': INV_PARENT_FOLDER_ID, + 'users_with_permission': [active_user_with_permissions['id']], + }, + { + 'name': "1's Invisible child", + 'id': INV_CHILD_1_FOLDER_ID, + 'parent_id': INV_PARENT_FOLDER_ID, + 'users_with_permission': [] + }, + { + 'name': "1's Visible grandchild", + 'id': str(uuid.uuid4()), + 'parent_id': INV_CHILD_1_FOLDER_ID, + 'users_with_permission': [active_user_with_permissions['id']], + }, + { + 'name': "Parent 2 - visible", + 'id': VIS_PARENT_FOLDER_ID, + 'parent_id': None, + 'users_with_permission': [active_user_with_permissions['id']], + }, + { + 'name': "2's Visible child", + 'id': str(uuid.uuid4()), + 'parent_id': VIS_PARENT_FOLDER_ID, + 'users_with_permission': [active_user_with_permissions['id']], + }, + { + 'name': "2's Invisible child", + 'id': INV_CHILD_2_FOLDER_ID, + 'parent_id': VIS_PARENT_FOLDER_ID, + 'users_with_permission': [] + }, + { + 'name': "2's Visible grandchild", + 'id': str(uuid.uuid4()), + 'parent_id': INV_CHILD_2_FOLDER_ID, + 'users_with_permission': [active_user_with_permissions['id']], + }, + ] + + +def test_template_list_yields_folders_visible_to_user( + notify_admin, + mock_get_template_folders, + mock_get_service_templates, + service_one, + active_user_with_permissions, + mocker +): + mock_get_template_folders.return_value = _get_all_folders(active_user_with_permissions) + service = Service(service_one) + user = User(active_user_with_permissions) + + result_folder_names = tuple( + result.name for result in + TemplateList(service=service, user=user) + if result.is_folder + ) + + assert result_folder_names == ( + ["Parent 1 - invisible", "1's Visible child"], + ["Parent 1 - invisible", ["1's Invisible child", "1's Visible grandchild"]], + "Parent 2 - visible", + "2's Visible child", + ["2's Invisible child", "2's Visible grandchild"], + ) + + +def test_template_list_folder_yields_user_folders_when_user_id_passed_in( + notify_admin, + mock_get_template_folders, + mock_get_service_templates, + service_one, + active_user_with_permissions, + mocker +): + mock_get_template_folders.return_value = _get_all_folders(active_user_with_permissions) + service = Service(service_one) + user = User(active_user_with_permissions) + + second_parent = next( + result for result in TemplateList(service=service, user=user) + if result.id == VIS_PARENT_FOLDER_ID + ) + + result_folder_names = tuple( + result['name'] for result in second_parent.folders + ) + + assert result_folder_names == ( + "2's Visible child", + ["2's Invisible child", "2's Visible grandchild"], + ) + + +def test_template_list_folder_yields_all_folders_when_user_id_not_passed_in( + mock_get_template_folders, + mock_get_service_templates, + service_one, + active_user_with_permissions, + mocker +): + mock_get_template_folders.return_value = _get_all_folders(active_user_with_permissions) + service = Service(service_one) + + second_parent = next( + result for result in TemplateList(service=service) + if result.id == VIS_PARENT_FOLDER_ID + ) + + result_folder_names = tuple( + result['name'] for result in second_parent.folders + ) + + assert result_folder_names == ( + "2's Invisible child", + "2's Visible child", + ) From bcfc6ce707519b300c569b418d34fa4c24ccf007 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Thu, 26 May 2022 18:47:42 +0100 Subject: [PATCH 07/12] Decouple deletion tests from "get_templates" fn Using create_template here is easier than template_json as it has various parameters preset [^1]. [^1]: https://github.com/alphagov/notifications-admin/blob/master/tests/conftest.py#L3986 --- tests/app/main/views/test_template_folders.py | 36 +++++++++++-------- 1 file changed, 21 insertions(+), 15 deletions(-) diff --git a/tests/app/main/views/test_template_folders.py b/tests/app/main/views/test_template_folders.py index 77279e190..297796ddd 100644 --- a/tests/app/main/views/test_template_folders.py +++ b/tests/app/main/views/test_template_folders.py @@ -5,7 +5,7 @@ from flask import abort, url_for from notifications_python_client.errors import HTTPError from app.models.user import User -from tests import sample_uuid, template_json +from tests import sample_uuid from tests.conftest import ( SERVICE_ONE_ID, TEMPLATE_ONE_ID, @@ -13,6 +13,7 @@ from tests.conftest import ( create_active_caseworking_user, create_active_user_view_permissions, create_active_user_with_permissions, + create_template, normalize_spaces, ) @@ -920,17 +921,17 @@ def test_manage_folder_users_doesnt_change_permissions_current_user_cannot_manag def test_delete_template_folder_should_request_confirmation( - client_request, service_one, mock_get_template_folders, mocker, + client_request, + service_one, + mock_get_template_folders, + mocker, + mock_get_service_templates_when_no_templates_exist, ): mocker.patch('app.models.service.Service.active_users', []) folder_id = str(uuid.uuid4()) mock_get_template_folders.side_effect = [[ _folder('sacrifice', folder_id, None), ], []] - mocker.patch( - 'app.models.service.Service.get_templates', - return_value=[], - ) page = client_request.get( 'main.delete_template_folder', service_id=service_one['id'], template_folder_id=folder_id, @@ -958,17 +959,19 @@ def test_delete_template_folder_should_request_confirmation( def test_delete_template_folder_should_detect_non_empty_folder_on_get( - client_request, service_one, mock_get_template_folders, mocker + client_request, + service_one, + mock_get_template_folders, + mocker ): folder_id = str(uuid.uuid4()) - template_id = str(uuid.uuid4()) mock_get_template_folders.side_effect = [ [_folder("can't touch me", folder_id, None)], [] ] mocker.patch( - 'app.models.service.Service.get_templates', - return_value=[template_json(service_one['id'], template_id)], + 'app.service_api_client.get_service_templates', + return_value={'data': [create_template(folder=folder_id)]} ) client_request.get( 'main.delete_template_folder', service_id=service_one['id'], @@ -988,16 +991,19 @@ def test_delete_template_folder_should_detect_non_empty_folder_on_get( None, PARENT_FOLDER_ID, )) -def test_delete_folder(client_request, service_one, mock_get_template_folders, mocker, parent_folder_id): +def test_delete_folder( + client_request, + service_one, + mock_get_template_folders, + mocker, + parent_folder_id, + mock_get_service_templates_when_no_templates_exist, +): mock_delete = mocker.patch('app.template_folder_api_client.delete_template_folder') folder_id = str(uuid.uuid4()) mock_get_template_folders.side_effect = [[ _folder('sacrifice', folder_id, parent_folder_id), ], []] - mocker.patch( - 'app.models.service.Service.get_templates', - return_value=[], - ) client_request.post( 'main.delete_template_folder', From 9126c8ca5ec1af20404610289045c950e95f5886 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Mon, 30 May 2022 15:37:54 +0100 Subject: [PATCH 08/12] Move _template_ methods to TemplateList class This is a straight move with a few minor tweaks: - Some references to self.X in the code from Service now need to become self.service.X. - Some reference to self.service.Y in TemplateList can now become self.Y for the methods that were migrated. The remaining _template_ methods in Service are still called from multiple places and there's no performance gain to motivate moving them out of the Service class, which is now a more manageable size. --- app/models/service.py | 89 ------------------------------- app/models/template_list.py | 101 +++++++++++++++++++++++++++++++++--- 2 files changed, 95 insertions(+), 95 deletions(-) diff --git a/app/models/service.py b/app/models/service.py index 161cd2f60..5698b1e25 100644 --- a/app/models/service.py +++ b/app/models/service.py @@ -223,22 +223,6 @@ class Service(JSONModel, SortByNameMixin): def all_template_ids(self): return {template['id'] for template in self.all_templates} - def get_templates(self, template_type='all', template_folder_id=None, user=None): - if user and template_folder_id: - folder = self.get_template_folder(template_folder_id) - if not user.has_template_folder_permission(folder): - return [] - - 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']}) - and template.get('folder') == template_folder_id - ] - def get_template(self, template_id, version=None): return service_api_client.get_service_template(self.id, template_id, version)['data'] @@ -552,63 +536,6 @@ class Service(JSONModel, SortByNameMixin): def all_template_folder_ids(self): return {folder['id'] for folder in self.all_template_folders} - def get_user_template_folders(self, user): - """Returns a modified list of folders a user has permission to view - - For each folder, we do the following: - - if user has no permission to view the folder, skip it - - if folder is visible and its parent is visible, we add it to the list of folders - we later return without modifying anything - - if folder is visible, but the parent is not, we iterate through the parent until we - either find a visible parent or reach root folder. On each iteration we concatenate - invisible parent folder name to the front of our folder name, modifying the name, and we - change parent_folder_id attribute to a higher level parent. This flattens the path to the - folder making sure it displays in the closest visible parent. - - """ - user_folders = [] - for folder in self.all_template_folders: - if not user.has_template_folder_permission(folder, service=self): - continue - parent = self.get_template_folder(folder["parent_id"]) - if user.has_template_folder_permission(parent, service=self): - user_folders.append(folder) - else: - folder_attrs = { - "id": folder["id"], "name": folder["name"], "parent_id": folder["parent_id"], - "users_with_permission": folder["users_with_permission"] - } - while folder_attrs["parent_id"] is not None: - folder_attrs["name"] = [ - parent["name"], - folder_attrs["name"], - ] - if parent["parent_id"] is None: - folder_attrs["parent_id"] = None - else: - parent = self.get_template_folder(parent["parent_id"]) - folder_attrs["parent_id"] = parent.get("id", None) - if user.has_template_folder_permission(parent, service=self): - break - user_folders.append(folder_attrs) - return user_folders - - def get_template_folders(self, template_type='all', parent_folder_id=None, user=None): - if user: - folders = self.get_user_template_folders(user) - else: - folders = self.all_template_folders - if parent_folder_id: - parent_folder_id = str(parent_folder_id) - - return [ - folder for folder in folders - if ( - folder['parent_id'] == parent_folder_id - and self.is_folder_visible(folder['id'], template_type, user) - ) - ] - def get_template_folder(self, folder_id): if folder_id is None: return { @@ -618,22 +545,6 @@ class Service(JSONModel, SortByNameMixin): } return self._get_by_id(self.all_template_folders, folder_id) - def is_folder_visible(self, template_folder_id, template_type='all', user=None): - - if template_type == 'all': - return True - - if self.get_templates(template_type, template_folder_id): - return True - - if any( - self.is_folder_visible(child_folder['id'], template_type, user) - for child_folder in self.get_template_folders(template_type, template_folder_id, user) - ): - return True - - return False - def get_template_folder_path(self, template_folder_id): folder = self.get_template_folder(template_folder_id) diff --git a/app/models/template_list.py b/app/models/template_list.py index 4621f6735..13f8e2d51 100644 --- a/app/models/template_list.py +++ b/app/models/template_list.py @@ -23,15 +23,15 @@ class TemplateList(): def get_templates_and_folders(self, template_type, template_folder_id, user, ancestors): - for item in self.service.get_template_folders( + for item in self.get_template_folders( template_type, template_folder_id, user, ): yield TemplateListFolder( item, - folders=self.service.get_template_folders( + folders=self.get_template_folders( template_type, item['id'], user ), - templates=self.service.get_templates( + templates=self.get_templates( template_type, item['id'] ), ancestors=ancestors, @@ -42,7 +42,7 @@ class TemplateList(): ): yield sub_item - for item in self.service.get_templates( + for item in self.get_templates( template_type, template_folder_id, user ): yield TemplateListTemplate( @@ -51,6 +51,95 @@ class TemplateList(): service_id=self.service.id, ) + def get_templates(self, template_type='all', template_folder_id=None, user=None): + if user and template_folder_id: + folder = self.service.get_template_folder(template_folder_id) + if not user.has_template_folder_permission(folder): + return [] + + 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.service.all_templates + if (set(template_type) & {'all', template['template_type']}) + and template.get('folder') == template_folder_id + ] + + def get_user_template_folders(self, user): + """Returns a modified list of folders a user has permission to view + + For each folder, we do the following: + - if user has no permission to view the folder, skip it + - if folder is visible and its parent is visible, we add it to the list of folders + we later return without modifying anything + - if folder is visible, but the parent is not, we iterate through the parent until we + either find a visible parent or reach root folder. On each iteration we concatenate + invisible parent folder name to the front of our folder name, modifying the name, and we + change parent_folder_id attribute to a higher level parent. This flattens the path to the + folder making sure it displays in the closest visible parent. + + """ + user_folders = [] + for folder in self.service.all_template_folders: + if not user.has_template_folder_permission(folder, service=self.service): + continue + parent = self.service.get_template_folder(folder["parent_id"]) + if user.has_template_folder_permission(parent, service=self.service): + user_folders.append(folder) + else: + folder_attrs = { + "id": folder["id"], "name": folder["name"], "parent_id": folder["parent_id"], + "users_with_permission": folder["users_with_permission"] + } + while folder_attrs["parent_id"] is not None: + folder_attrs["name"] = [ + parent["name"], + folder_attrs["name"], + ] + if parent["parent_id"] is None: + folder_attrs["parent_id"] = None + else: + parent = self.service.get_template_folder(parent["parent_id"]) + folder_attrs["parent_id"] = parent.get("id", None) + if user.has_template_folder_permission(parent, service=self.service): + break + user_folders.append(folder_attrs) + return user_folders + + def get_template_folders(self, template_type='all', parent_folder_id=None, user=None): + if user: + folders = self.get_user_template_folders(user) + else: + folders = self.service.all_template_folders + if parent_folder_id: + parent_folder_id = str(parent_folder_id) + + return [ + folder for folder in folders + if ( + folder['parent_id'] == parent_folder_id + and self.is_folder_visible(folder['id'], template_type, user) + ) + ] + + def is_folder_visible(self, template_folder_id, template_type='all', user=None): + + if template_type == 'all': + return True + + if self.get_templates(template_type, template_folder_id): + return True + + if any( + self.is_folder_visible(child_folder['id'], template_type, user) + for child_folder in self.get_template_folders(template_type, template_folder_id, user) + ): + return True + + return False + @property def as_id_and_name(self): return [(item.id, item.name) for item in self] @@ -70,10 +159,10 @@ class ServiceTemplateList(TemplateList): def __iter__(self): template_list_service = TemplateListService( self.service, - templates=self.service.get_templates( + templates=self.get_templates( template_folder_id=None, ), - folders=self.service.get_template_folders( + folders=self.get_template_folders( parent_folder_id=None, ), ) From 2771ae12748de673d5bae41e777da588510997be Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Mon, 30 May 2022 15:59:00 +0100 Subject: [PATCH 09/12] Remove redundant user param from template methods This will finally make it possible to cache some of the methods. --- app/models/template_list.py | 43 ++++++++++++++++++------------------- 1 file changed, 21 insertions(+), 22 deletions(-) diff --git a/app/models/template_list.py b/app/models/template_list.py index 13f8e2d51..c077b0dc0 100644 --- a/app/models/template_list.py +++ b/app/models/template_list.py @@ -17,19 +17,19 @@ class TemplateList(): def __iter__(self): for item in self.get_templates_and_folders( - self.template_type, self.template_folder_id, self.user, ancestors=[] + self.template_type, self.template_folder_id, ancestors=[] ): yield item - def get_templates_and_folders(self, template_type, template_folder_id, user, ancestors): + def get_templates_and_folders(self, template_type, template_folder_id, ancestors): for item in self.get_template_folders( - template_type, template_folder_id, user, + template_type, template_folder_id, ): yield TemplateListFolder( item, folders=self.get_template_folders( - template_type, item['id'], user + template_type, item['id'], ), templates=self.get_templates( template_type, item['id'] @@ -38,12 +38,12 @@ class TemplateList(): service_id=self.service.id, ) for sub_item in self.get_templates_and_folders( - template_type, item['id'], user, ancestors + [item] + template_type, item['id'], ancestors + [item] ): yield sub_item for item in self.get_templates( - template_type, template_folder_id, user + template_type, template_folder_id, ): yield TemplateListTemplate( item, @@ -51,10 +51,10 @@ class TemplateList(): service_id=self.service.id, ) - def get_templates(self, template_type='all', template_folder_id=None, user=None): - if user and template_folder_id: + def get_templates(self, template_type='all', template_folder_id=None): + if self.user and template_folder_id: folder = self.service.get_template_folder(template_folder_id) - if not user.has_template_folder_permission(folder): + if not self.user.has_template_folder_permission(folder): return [] if isinstance(template_type, str): @@ -67,7 +67,7 @@ class TemplateList(): and template.get('folder') == template_folder_id ] - def get_user_template_folders(self, user): + def get_user_template_folders(self): """Returns a modified list of folders a user has permission to view For each folder, we do the following: @@ -83,10 +83,10 @@ class TemplateList(): """ user_folders = [] for folder in self.service.all_template_folders: - if not user.has_template_folder_permission(folder, service=self.service): + if not self.user.has_template_folder_permission(folder, service=self.service): continue parent = self.service.get_template_folder(folder["parent_id"]) - if user.has_template_folder_permission(parent, service=self.service): + if self.user.has_template_folder_permission(parent, service=self.service): user_folders.append(folder) else: folder_attrs = { @@ -103,14 +103,14 @@ class TemplateList(): else: parent = self.service.get_template_folder(parent["parent_id"]) folder_attrs["parent_id"] = parent.get("id", None) - if user.has_template_folder_permission(parent, service=self.service): + if self.user.has_template_folder_permission(parent, service=self.service): break user_folders.append(folder_attrs) return user_folders - def get_template_folders(self, template_type='all', parent_folder_id=None, user=None): - if user: - folders = self.get_user_template_folders(user) + def get_template_folders(self, template_type='all', parent_folder_id=None): + if self.user: + folders = self.get_user_template_folders() else: folders = self.service.all_template_folders if parent_folder_id: @@ -120,11 +120,11 @@ class TemplateList(): folder for folder in folders if ( folder['parent_id'] == parent_folder_id - and self.is_folder_visible(folder['id'], template_type, user) + and self.is_folder_visible(folder['id'], template_type) ) ] - def is_folder_visible(self, template_folder_id, template_type='all', user=None): + def is_folder_visible(self, template_folder_id, template_type='all'): if template_type == 'all': return True @@ -133,8 +133,8 @@ class TemplateList(): return True if any( - self.is_folder_visible(child_folder['id'], template_type, user) - for child_folder in self.get_template_folders(template_type, template_folder_id, user) + self.is_folder_visible(child_folder['id'], template_type) + for child_folder in self.get_template_folders(template_type, template_folder_id) ): return True @@ -151,7 +151,7 @@ class TemplateList(): @property def folder_is_empty(self): return not any(self.get_templates_and_folders( - 'all', self.template_folder_id, self.user, [] + 'all', self.template_folder_id, [] )) @@ -172,7 +172,6 @@ class ServiceTemplateList(TemplateList): yield from self.get_templates_and_folders( self.template_type, self.template_folder_id, - self.user, ancestors=[template_list_service] ) From 1c4a2b47900d615e4e126cca93724a03fcec90b6 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Mon, 30 May 2022 16:00:48 +0100 Subject: [PATCH 10/12] Cache templates and folders in TemplateList This gives us the performance gains identified in [^1] for the test service described in the spike: - user_template_folders - from 10s to a little above 3s on its own - get_templates_and_folders - from 10s to below 6s on its own In combination, these two uses of caching reduce the test page load time from 10s to a little above 3s. This is slightly higher than in the spike PR due to all the extra work we're doing to generate the "move to" list of folders, as described in a previous commit. The render time is unchanged for services with few folders. We start to see the benefit of this change at around 200 templates + folders, with no evidence that any service will experience worse render times, despite the extra work we're doing from previous commits. [^1]: https://github.com/alphagov/notifications-admin/pull/4251 --- app/models/template_list.py | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/app/models/template_list.py b/app/models/template_list.py index c077b0dc0..cfdff1f11 100644 --- a/app/models/template_list.py +++ b/app/models/template_list.py @@ -1,3 +1,5 @@ +from werkzeug.utils import cached_property + from app import format_notification_type @@ -16,10 +18,13 @@ class TemplateList(): self.user = user def __iter__(self): - for item in self.get_templates_and_folders( + yield from self.items + + @cached_property + def items(self): + return list(self.get_templates_and_folders( self.template_type, self.template_folder_id, ancestors=[] - ): - yield item + )) def get_templates_and_folders(self, template_type, template_folder_id, ancestors): @@ -67,7 +72,8 @@ class TemplateList(): and template.get('folder') == template_folder_id ] - def get_user_template_folders(self): + @cached_property + def user_template_folders(self): """Returns a modified list of folders a user has permission to view For each folder, we do the following: @@ -110,7 +116,7 @@ class TemplateList(): def get_template_folders(self, template_type='all', parent_folder_id=None): if self.user: - folders = self.get_user_template_folders() + folders = self.user_template_folders else: folders = self.service.all_template_folders if parent_folder_id: From 5db2581669e3f45ff73e705cac4576840cbc46df Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Tue, 7 Jun 2022 11:14:23 +0100 Subject: [PATCH 11/12] Simplify tests for TemplateList class In response to: [^1][^2][^3]. Originally there were three tests: - One was testing the "get_user_template_folders" method directly. - The other two were testing "get_template_folders", which calls the "_user_" method if a user is passed - this is what the tests were primarily checking, by passing or not passing a User object. The two tests of "get_template_folders" also implicitly checked that it filtered folders based on a "parent_folder_id". I wanted to emulate the tests but it makes more sense to simplify them, as the methods are now only used by TemplateList internally. To simplify, we just keep just two tests to check that the overall set of folders is filtered based on the presence of a User. We do lose the implicit check of filtering by "parent_folder_id", but: - We are still checking this implicitly in the sense that the loop to generate the overall set of folders terminates. If the method didn't do any filtering, the loop would be infinite. - We need to acknowledge that these tests were incomplete to begin with. There is also lots of coverage in higher level tests [^4], although it's harder to reason exactly what is covered. [^1]: https://github.com/alphagov/notifications-admin/pull/4258#discussion_r890144076 [^2]: https://github.com/alphagov/notifications-admin/pull/4258#discussion_r890151220 [^3]: https://github.com/alphagov/notifications-admin/pull/4258#discussion_r890147506 [^4]: https://github.com/alphagov/notifications-admin/blob/1787f9f42f0f71854c9fb621c2f0a8f4f7c76286/tests/app/main/views/test_template_folders.py --- app/models/template_list.py | 1 - tests/app/models/test_template_list.py | 45 +++++++------------------- 2 files changed, 11 insertions(+), 35 deletions(-) diff --git a/app/models/template_list.py b/app/models/template_list.py index cfdff1f11..6fc5ab8fa 100644 --- a/app/models/template_list.py +++ b/app/models/template_list.py @@ -264,7 +264,6 @@ class TemplateListFolder(TemplateListItem): super().__init__(folder, ancestors) self.folder = folder self.service_id = service_id - self.folders = folders self.number_of_templates = len(templates) self.number_of_folders = len(folders) diff --git a/tests/app/models/test_template_list.py b/tests/app/models/test_template_list.py index b8a3d5f4f..7aad9959e 100644 --- a/tests/app/models/test_template_list.py +++ b/tests/app/models/test_template_list.py @@ -96,34 +96,7 @@ def test_template_list_yields_folders_visible_to_user( ) -def test_template_list_folder_yields_user_folders_when_user_id_passed_in( - notify_admin, - mock_get_template_folders, - mock_get_service_templates, - service_one, - active_user_with_permissions, - mocker -): - mock_get_template_folders.return_value = _get_all_folders(active_user_with_permissions) - service = Service(service_one) - user = User(active_user_with_permissions) - - second_parent = next( - result for result in TemplateList(service=service, user=user) - if result.id == VIS_PARENT_FOLDER_ID - ) - - result_folder_names = tuple( - result['name'] for result in second_parent.folders - ) - - assert result_folder_names == ( - "2's Visible child", - ["2's Invisible child", "2's Visible grandchild"], - ) - - -def test_template_list_folder_yields_all_folders_when_user_id_not_passed_in( +def test_template_list_yields_all_folders_without_user( mock_get_template_folders, mock_get_service_templates, service_one, @@ -133,16 +106,20 @@ def test_template_list_folder_yields_all_folders_when_user_id_not_passed_in( mock_get_template_folders.return_value = _get_all_folders(active_user_with_permissions) service = Service(service_one) - second_parent = next( - result for result in TemplateList(service=service) - if result.id == VIS_PARENT_FOLDER_ID - ) - result_folder_names = tuple( - result['name'] for result in second_parent.folders + result.name for result in + TemplateList(service=service) + if result.is_folder ) assert result_folder_names == ( + "Invisible folder", + "Parent 1 - invisible", + "1's Invisible child", + "1's Visible grandchild", + "1's Visible child", + "Parent 2 - visible", "2's Invisible child", + "2's Visible grandchild", "2's Visible child", ) From 5b52aa2381d5c10e830be8833bd164686f4765fc Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Tue, 7 Jun 2022 11:28:38 +0100 Subject: [PATCH 12/12] Simplify test setup for Template List model --- tests/app/models/test_template_list.py | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/tests/app/models/test_template_list.py b/tests/app/models/test_template_list.py index 7aad9959e..119bf2180 100644 --- a/tests/app/models/test_template_list.py +++ b/tests/app/models/test_template_list.py @@ -1,5 +1,7 @@ import uuid +import pytest + from app.models.service import Service from app.models.template_list import TemplateList from app.models.user import User @@ -10,8 +12,12 @@ VIS_PARENT_FOLDER_ID = 'bbbb222b-2b22-2b22-222b-b222b22b2222' INV_CHILD_2_FOLDER_ID = 'fafe723f-1d39-4a10-865f-e551e03d8886' -def _get_all_folders(active_user_with_permissions): - return [ +@pytest.fixture +def mock_get_hierarchy_of_folders( + mock_get_template_folders, + active_user_with_permissions +): + mock_get_template_folders.return_value = [ { 'name': "Invisible folder", 'id': str(uuid.uuid4()), @@ -70,14 +76,11 @@ def _get_all_folders(active_user_with_permissions): def test_template_list_yields_folders_visible_to_user( - notify_admin, - mock_get_template_folders, + mock_get_hierarchy_of_folders, mock_get_service_templates, service_one, active_user_with_permissions, - mocker ): - mock_get_template_folders.return_value = _get_all_folders(active_user_with_permissions) service = Service(service_one) user = User(active_user_with_permissions) @@ -97,13 +100,10 @@ def test_template_list_yields_folders_visible_to_user( def test_template_list_yields_all_folders_without_user( - mock_get_template_folders, + mock_get_hierarchy_of_folders, mock_get_service_templates, service_one, - active_user_with_permissions, - mocker ): - mock_get_template_folders.return_value = _get_all_folders(active_user_with_permissions) service = Service(service_one) result_folder_names = tuple(