diff --git a/app/main/views/templates.py b/app/main/views/templates.py index 22dc8d57f..a16564748 100644 --- a/app/main/views/templates.py +++ b/app/main/views/templates.py @@ -111,10 +111,10 @@ def start_tour(service_id, template_id): @user_has_permissions() def choose_template(service_id, template_type='all', template_folder_id=None): - template_list = TemplateList(current_service, template_type, template_folder_id) + template_list = TemplateList(current_service, template_type, template_folder_id, current_user.id) templates_and_folders_form = TemplateAndFoldersSelectionForm( - all_template_folders=current_service.all_template_folders, + all_template_folders=current_service.get_user_template_folders(current_user.id), template_list=template_list, template_type=template_type, allow_adding_letter_template=current_service.has_permission('letter'), @@ -348,6 +348,7 @@ def choose_template_to_copy( services_templates_and_folders=TemplateList( service, template_folder_id=from_folder, + user_id=current_user.id ), template_folder_path=service.get_template_folder_path(from_folder), from_service=service, @@ -360,7 +361,7 @@ def choose_template_to_copy( services_templates_and_folders=TemplateLists([ Service(service) for service in user_api_client.get_services_for_user(current_user) - ]), + ], user_id=current_user.id), search_form=SearchByNameForm(), ) diff --git a/app/models/service.py b/app/models/service.py index 33113c2e3..df03ee0fb 100644 --- a/app/models/service.py +++ b/app/models/service.py @@ -154,7 +154,12 @@ class Service(): 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): + def get_templates(self, template_type='all', template_folder_id=None, user_id=None): + if user_id and template_folder_id and self.has_permission('edit_folder_permissions'): + folder = self.get_template_folder(template_folder_id) + if user_id not in folder.get("users_with_permission", []): + return [] + if isinstance(template_type, str): template_type = [template_type] if template_folder_id: @@ -391,16 +396,60 @@ class Service(): def all_template_folder_ids(self): return {folder['id'] for folder in self.all_template_folders} - def get_template_folders(self, template_type='all', parent_folder_id=None): + def get_user_template_folders(self, user_id): + """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. + + """ + if not self.has_permission("edit_folder_permissions"): + return self.all_template_folders + + user_folders = [] + for folder in self.all_template_folders: + if user_id not in folder.get("users_with_permission", []): + continue + parent = self.get_template_folder(folder["parent_id"]) + if user_id in parent.get("users_with_permission", []): + 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_id in parent.get("users_with_permission", []): + break + user_folders.append(folder_attrs) + return user_folders + + def get_template_folders(self, template_type='all', parent_folder_id=None, user_id=None): + if user_id: + folders = self.get_user_template_folders(user_id) + else: + folders = self.all_template_folders if parent_folder_id: parent_folder_id = str(parent_folder_id) return [ - folder for folder in self.all_template_folders + folder for folder in folders if ( - folder['parent_id'] == parent_folder_id and - self.is_folder_visible(folder['id'], template_type) + folder['parent_id'] == parent_folder_id + and self.is_folder_visible(folder['id'], template_type, user_id) ) ] @@ -413,7 +462,7 @@ class Service(): } return self._get_by_id(self.all_template_folders, folder_id) - def is_folder_visible(self, template_folder_id, template_type='all'): + def is_folder_visible(self, template_folder_id, template_type='all', user_id=None): if template_type == 'all': return True @@ -422,8 +471,8 @@ class Service(): 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) + self.is_folder_visible(child_folder['id'], template_type, user_id) + for child_folder in self.get_template_folders(template_type, template_folder_id, user_id) ): return True @@ -447,8 +496,8 @@ class Service(): 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) + self.get_templates(template_type, template_folder_id) + + self.get_template_folders(template_type, template_folder_id) ) @property diff --git a/app/models/template_list.py b/app/models/template_list.py index d3b1ee2b6..ad479b115 100644 --- a/app/models/template_list.py +++ b/app/models/template_list.py @@ -5,26 +5,28 @@ class TemplateList(): service, template_type='all', template_folder_id=None, + user_id=None, ): self.service = service self.template_type = template_type self.template_folder_id = template_folder_id + self.user_id = user_id def __iter__(self): for item in self.get_templates_and_folders( - self.template_type, self.template_folder_id, ancestors=[] + self.template_type, self.template_folder_id, self.user_id, ancestors=[] ): yield item - def get_templates_and_folders(self, template_type, template_folder_id, ancestors): + def get_templates_and_folders(self, template_type, template_folder_id, user_id, ancestors): for item in self.service.get_template_folders( - template_type, template_folder_id + template_type, template_folder_id, user_id, ): yield TemplateListFolder( item, folders=self.service.get_template_folders( - template_type, item['id'] + template_type, item['id'], user_id ), templates=self.service.get_templates( template_type, item['id'] @@ -33,12 +35,12 @@ class TemplateList(): service_id=self.service.id, ) for sub_item in self.get_templates_and_folders( - template_type, item['id'], ancestors + [item] + template_type, item['id'], user_id, ancestors + [item] ): yield sub_item for item in self.service.get_templates( - template_type, template_folder_id + template_type, template_folder_id, user_id ): yield TemplateListTemplate( item, @@ -57,23 +59,24 @@ class TemplateList(): @property def folder_is_empty(self): return not any(self.get_templates_and_folders( - 'all', self.template_folder_id, [] + 'all', self.template_folder_id, self.user_id, [] )) class TemplateLists(): - def __init__(self, services): + def __init__(self, services, user_id=None): self.services = sorted( services, key=lambda service: service.name.lower(), ) + self.user_id = user_id def __iter__(self): if len(self.services) == 1: - for template_or_folder in TemplateList(self.services[0]): + for template_or_folder in TemplateList(self.services[0], user_id=self.user_id): yield template_or_folder return @@ -85,10 +88,11 @@ class TemplateLists(): yield template_list_service for service_templates_and_folders in TemplateList( - service + service, user_id=self.user_id ).get_templates_and_folders( template_type='all', template_folder_id=None, + user_id=self.user_id, ancestors=[template_list_service], ): yield service_templates_and_folders diff --git a/tests/app/main/views/test_template_folders.py b/tests/app/main/views/test_template_folders.py index 8d391b0b9..00f22ef34 100644 --- a/tests/app/main/views/test_template_folders.py +++ b/tests/app/main/views/test_template_folders.py @@ -18,13 +18,16 @@ PARENT_FOLDER_ID = '7e979e79-d970-43a5-ac69-b625a8d147b0' CHILD_FOLDER_ID = '92ee1ee0-e4ee-4dcc-b1a7-a5da9ebcfa2b' GRANDCHILD_FOLDER_ID = 'fafe723f-1d39-4a10-865f-e551e03d8886' FOLDER_TWO_ID = 'bbbb222b-2b22-2b22-222b-b222b22b2222' +FOLDER_B_ID = 'dddb222b-2b22-2b22-222b-b222b22b6789' +FOLDER_C_ID = 'ccbb222b-2b22-2b22-222b-b222b22b2345' -def _folder(name, folder_id=None, parent=None): +def _folder(name, folder_id=None, parent=None, users_with_permission=None): return { 'name': name, 'id': folder_id or str(uuid.uuid4()), 'parent_id': parent, + 'users_with_permission': users_with_permission or [], } @@ -1124,3 +1127,151 @@ def test_show_custom_error_message( ) assert page.select_one('div.banner-dangerous').text.strip() == error_msg + + +@pytest.mark.parametrize( + ( + 'extra_args,' + 'expected_displayed_items, ' + 'expected_items, ' + 'expected_empty_message ' + ), + [ + ( + {}, + [ + ['folder_A', '1 template, 2 folders'], + ['folder_E / folder_F / folder_G', '1 template'], + ['email_template_root', 'Email template'], + ], + [ + ['folder_A', '1 template, 2 folders'], + ['folder_A', '/', 'folder_C', '1 template'], + ['folder_A', '/', 'folder_C', '/', 'sms_template_C', 'Text message template'], + ['folder_A', '/', 'folder_D', 'Empty'], + ['folder_A', '/', 'sms_template_A', 'Text message template'], + ['folder_E / folder_F / folder_G', '1 template'], + ['folder_E / folder_F / folder_G', '/', 'email_template_G', 'Email template'], + ['email_template_root', 'Email template'], + ], + None, + ), + ( + {'template_type': 'email'}, + [ + ['folder_E / folder_F / folder_G', '1 template'], + ['email_template_root', 'Email template'], + ], + [ + ['folder_E / folder_F / folder_G', '1 template'], + ['folder_E / folder_F / folder_G', '/', 'email_template_G', 'Email template'], + ['email_template_root', 'Email template'], + ], + None, + ), + ( + {'template_type': 'sms'}, + [ + ['folder_A', '1 template, 1 folder'], + ], + [ + ['folder_A', '1 template, 1 folder'], + ['folder_A', '/', 'folder_C', '1 template'], + ['folder_A', '/', 'folder_C', '/', 'sms_template_C', 'Text message template'], + ['folder_A', '/', 'sms_template_A', 'Text message template'], + ], + None, + ), + ( + {'template_type': 'letter'}, + [], + [], + None, + ), + ( + { + 'template_type': 'email', + 'template_folder_id': FOLDER_C_ID, + }, + [], + [], + 'There are no email templates in this folder', + ), + ( + {'template_folder_id': CHILD_FOLDER_ID}, + [], + [], + 'This folder is empty', + ), + ( + {'template_folder_id': CHILD_FOLDER_ID, 'template_type': 'sms'}, + [], + [], + 'This folder is empty', + ), + ] +) +def test_should_filter_templates_folder_page_based_on_user_permissions( + client_request, + mock_get_template_folders, + mock_has_no_jobs, + service_one, + mocker, + active_user_with_permissions, + extra_args, + expected_displayed_items, + expected_items, + expected_empty_message, +): + service_one['permissions'] += ['edit_folder_permissions', 'letter'] + mock_get_template_folders.return_value = [ + _folder('folder_A', FOLDER_TWO_ID, None, [active_user_with_permissions.id]), + _folder('folder_B', FOLDER_B_ID, FOLDER_TWO_ID), + _folder('folder_C', FOLDER_C_ID, FOLDER_TWO_ID, [active_user_with_permissions.id]), + _folder('folder_D', None, FOLDER_TWO_ID, [active_user_with_permissions.id]), + _folder('folder_E', PARENT_FOLDER_ID), + _folder('folder_F', CHILD_FOLDER_ID, parent=PARENT_FOLDER_ID), + _folder('folder_G', GRANDCHILD_FOLDER_ID, CHILD_FOLDER_ID, [active_user_with_permissions.id]), + ] + mocker.patch( + 'app.service_api_client.get_service_templates', + return_value={'data': [ + _template('email', 'email_template_root'), + _template('sms', 'sms_template_A', parent=FOLDER_TWO_ID), + _template('sms', 'sms_template_C', parent=FOLDER_C_ID), + _template('email', 'email_template_B', parent=FOLDER_B_ID), + _template('email', 'email_template_G', parent=GRANDCHILD_FOLDER_ID), + _template('letter', 'letter_template_F', parent=CHILD_FOLDER_ID), + ]} + ) + + page = client_request.get( + 'main.choose_template', + service_id=SERVICE_ONE_ID, + _test_page_title=False, + **extra_args + ) + + displayed_page_items = page.find_all(lambda tag: ( + tag.has_attr('class') + and 'template-list-item' in tag['class'] + and 'template-list-item-hidden-by-default' not in tag['class'] + )) + + assert [ + [i.strip() for i in e.text.split("\n") if i.strip()] + for e in displayed_page_items + ] == expected_displayed_items + + all_page_items = page.select('.template-list-item') + assert [ + [i.strip() for i in e.text.split("\n") if i.strip()] + for e in all_page_items + ] == expected_items + + if expected_empty_message: + assert normalize_spaces(page.select_one('.template-list-empty').text) == ( + expected_empty_message + ) + else: + assert not page.select('.template-list-empty') diff --git a/tests/app/models/test_service.py b/tests/app/models/test_service.py new file mode 100644 index 000000000..3ba9063b4 --- /dev/null +++ b/tests/app/models/test_service.py @@ -0,0 +1,174 @@ +import uuid + +from app.models.service import Service + +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( + 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_one['permissions'] = ['edit_folder_permissions'] + service = Service(service_one) + result = service.get_user_template_folders(active_user_with_permissions.id) + 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( + 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_one['permissions'] = ['edit_folder_permissions'] + service = Service(service_one) + result = service.get_template_folders(user_id=active_user_with_permissions.id) + 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] + } + ]