diff --git a/app/main/views/templates.py b/app/main/views/templates.py index f3939a481..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, @@ -470,10 +474,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..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) @@ -650,12 +561,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/app/models/template_list.py b/app/models/template_list.py index 144a4675a..6fc5ab8fa 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,34 +18,37 @@ class TemplateList(): self.user = user def __iter__(self): - for item in self.get_templates_and_folders( - self.template_type, self.template_folder_id, self.user, ancestors=[] - ): - yield item + yield from self.items - def get_templates_and_folders(self, template_type, template_folder_id, user, ancestors): + @cached_property + def items(self): + return list(self.get_templates_and_folders( + self.template_type, self.template_folder_id, ancestors=[] + )) - for item in self.service.get_template_folders( - template_type, template_folder_id, user, + def get_templates_and_folders(self, template_type, template_folder_id, ancestors): + + for item in self.get_template_folders( + template_type, template_folder_id, ): yield TemplateListFolder( item, - folders=self.service.get_template_folders( - template_type, item['id'], user + folders=self.get_template_folders( + template_type, item['id'], ), - templates=self.service.get_templates( + templates=self.get_templates( template_type, item['id'] ), ancestors=ancestors, 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.service.get_templates( - template_type, template_folder_id, user + for item in self.get_templates( + template_type, template_folder_id, ): yield TemplateListTemplate( item, @@ -51,6 +56,96 @@ class TemplateList(): service_id=self.service.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 self.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 + ] + + @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: + - 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 self.user.has_template_folder_permission(folder, service=self.service): + continue + parent = self.service.get_template_folder(folder["parent_id"]) + if self.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 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): + if self.user: + folders = self.user_template_folders + 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) + ) + ] + + def is_folder_visible(self, template_folder_id, template_type='all'): + + 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) + for child_folder in self.get_template_folders(template_type, template_folder_id) + ): + return True + + return False + @property def as_id_and_name(self): return [(item.id, item.name) for item in self] @@ -62,10 +157,31 @@ 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, [] )) +class ServiceTemplateList(TemplateList): + def __iter__(self): + template_list_service = TemplateListService( + self.service, + templates=self.get_templates( + template_folder_id=None, + ), + folders=self.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, + ancestors=[template_list_service] + ) + + class TemplateLists(): def __init__(self, user): @@ -76,29 +192,18 @@ 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: - - 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): @@ -157,6 +262,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) @@ -183,21 +289,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, ) diff --git a/tests/app/main/views/test_template_folders.py b/tests/app/main/views/test_template_folders.py index 64cb20493..2eb256535 100644 --- a/tests/app/main/views/test_template_folders.py +++ b/tests/app/main/views/test_template_folders.py @@ -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, ) @@ -916,17 +917,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, @@ -954,17 +955,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=[{'id': template_id, 'name': 'template'}], + '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'], @@ -983,16 +986,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', diff --git a/tests/app/models/test_service.py b/tests/app/models/test_service.py index 908b49037..d10199a9a 100644 --- a/tests/app/models/test_service.py +++ b/tests/app/models/test_service.py @@ -1,184 +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) - 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']], - }, - ] - - -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) - 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']] - }, - ] - - -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 = 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']], - } - ] - 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..119bf2180 --- /dev/null +++ b/tests/app/models/test_template_list.py @@ -0,0 +1,125 @@ +import uuid + +import pytest + +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' + + +@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()), + '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( + mock_get_hierarchy_of_folders, + mock_get_service_templates, + service_one, + 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_yields_all_folders_without_user( + mock_get_hierarchy_of_folders, + mock_get_service_templates, + service_one, +): + service = Service(service_one) + + result_folder_names = tuple( + 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", + )