From 87fb3944ce6db1de90cbd54c559ecdac6a4a15d4 Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Fri, 1 Mar 2019 16:25:15 +0000 Subject: [PATCH 01/10] Get template folders visible to the current user get_template_folders can filter out folders invisible to a user --- app/models/service.py | 37 +++++++++++++++++++++++++++++++------ 1 file changed, 31 insertions(+), 6 deletions(-) diff --git a/app/models/service.py b/app/models/service.py index 06c1403f7..56ec55a91 100644 --- a/app/models/service.py +++ b/app/models/service.py @@ -379,16 +379,41 @@ 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): + 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 parent is not None: + folder_attrs["name"] = parent["name"] + "/" + folder_attrs["name"] + 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) ) ] @@ -435,8 +460,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 From de237e9e6f4017f3f2370ff3606aed855f989e79 Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Fri, 1 Mar 2019 17:50:36 +0000 Subject: [PATCH 02/10] Test that folders are filtered based on user permission at service level --- app/models/service.py | 13 ++- tests/app/models/test_service.py | 172 +++++++++++++++++++++++++++++++ 2 files changed, 180 insertions(+), 5 deletions(-) create mode 100644 tests/app/models/test_service.py diff --git a/app/models/service.py b/app/models/service.py index 56ec55a91..0bc6223f8 100644 --- a/app/models/service.py +++ b/app/models/service.py @@ -392,12 +392,15 @@ class Service(): "id": folder["id"], "name": folder["name"], "parent_id": folder["parent_id"], "users_with_permission": folder["users_with_permission"] } - while parent is not None: + while folder_attrs["parent_id"] is not None: folder_attrs["name"] = parent["name"] + "/" + folder_attrs["name"] - 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 + 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 diff --git a/tests/app/models/test_service.py b/tests/app/models/test_service.py new file mode 100644 index 000000000..8cc79b5e9 --- /dev/null +++ b/tests/app/models/test_service.py @@ -0,0 +1,172 @@ +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 = 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 = 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] + } + ] From 1fb7a2515f6446567ac1c646315d94bfe6628acc Mon Sep 17 00:00:00 2001 From: Alexey Bezhan Date: Wed, 6 Mar 2019 14:54:53 +0000 Subject: [PATCH 03/10] Return all templates for the user if the folder permissions flag is off Putting the permission check in the get_user_template_folders allows us to replace `all_template_folders` usage with the new method without having to worry about the temporary service permission flag. --- app/models/service.py | 3 +++ tests/app/models/test_service.py | 2 ++ 2 files changed, 5 insertions(+) diff --git a/app/models/service.py b/app/models/service.py index 0bc6223f8..bbcbca3c6 100644 --- a/app/models/service.py +++ b/app/models/service.py @@ -380,6 +380,9 @@ class Service(): return {folder['id'] for folder in self.all_template_folders} def get_user_template_folders(self, user_id): + 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", []): diff --git a/tests/app/models/test_service.py b/tests/app/models/test_service.py index 8cc79b5e9..660263013 100644 --- a/tests/app/models/test_service.py +++ b/tests/app/models/test_service.py @@ -74,6 +74,7 @@ def test_get_user_template_folders_only_returns_folders_visible_to_user( 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 == [ @@ -117,6 +118,7 @@ def test_get_template_folders_shows_user_folders_when_user_id_passed_in( 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 == [ From 80bfd8e347fdaf44801ea371f91238636249ccfb Mon Sep 17 00:00:00 2001 From: Alexey Bezhan Date: Wed, 6 Mar 2019 14:56:56 +0000 Subject: [PATCH 04/10] Add space around folder name separator when flattening folder path --- app/models/service.py | 2 +- tests/app/models/test_service.py | 10 +++++----- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/app/models/service.py b/app/models/service.py index bbcbca3c6..161d77116 100644 --- a/app/models/service.py +++ b/app/models/service.py @@ -396,7 +396,7 @@ class Service(): "users_with_permission": folder["users_with_permission"] } while folder_attrs["parent_id"] is not None: - folder_attrs["name"] = parent["name"] + "/" + folder_attrs["name"] + folder_attrs["name"] = parent["name"] + " / " + folder_attrs["name"] if parent["parent_id"] is None: folder_attrs["parent_id"] = None else: diff --git a/tests/app/models/test_service.py b/tests/app/models/test_service.py index 660263013..3ba9063b4 100644 --- a/tests/app/models/test_service.py +++ b/tests/app/models/test_service.py @@ -79,13 +79,13 @@ def test_get_user_template_folders_only_returns_folders_visible_to_user( result = service.get_user_template_folders(active_user_with_permissions.id) assert result == [ { - 'name': "Parent 1 - invisible/1's Visible child", + '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", + '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] @@ -97,7 +97,7 @@ def test_get_user_template_folders_only_returns_folders_visible_to_user( 'users_with_permission': [active_user_with_permissions.id] }, { - 'name': "2's Invisible child/2's Visible grandchild", + '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] @@ -123,13 +123,13 @@ def test_get_template_folders_shows_user_folders_when_user_id_passed_in( result = service.get_template_folders(user_id=active_user_with_permissions.id) assert result == [ { - 'name': "Parent 1 - invisible/1's Visible child", + '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", + '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] From 6638a0aecb77a7f5959f1811724e26eb1277afb3 Mon Sep 17 00:00:00 2001 From: Alexey Bezhan Date: Wed, 6 Mar 2019 14:57:51 +0000 Subject: [PATCH 05/10] Add user_id argument to TemplateList to allow filtering folders by user Switches TemplateList to use get_user_template_folders by setting the user_id. --- app/models/template_list.py | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/app/models/template_list.py b/app/models/template_list.py index d3b1ee2b6..e8bf0a9e2 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,7 +35,7 @@ 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 @@ -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 From 70e6732255c6210cabd08ebb362a5497f9211e0f Mon Sep 17 00:00:00 2001 From: Alexey Bezhan Date: Wed, 6 Mar 2019 16:23:42 +0000 Subject: [PATCH 06/10] Only display folders the user has permission for This adds a filter by user permissions to the list of template folders displayed in: * the 'choose a template page' * "Move to" form to choose a destination folder * "Copy an existing template" selection form --- app/main/views/templates.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/app/main/views/templates.py b/app/main/views/templates.py index 44eddb4b4..5dfa8904c 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(), ) From 3d401ce8568a1fc4d9a99a1d5d9a359e2f05ef57 Mon Sep 17 00:00:00 2001 From: Alexey Bezhan Date: Thu, 7 Mar 2019 11:05:44 +0000 Subject: [PATCH 07/10] Hide current folder templates if user doesn't have a folder permission TemplateList gets a list of templates in a current folder separately, so we need to make sure `service.get_templates` checks for the appropriate user permission --- app/models/service.py | 7 ++++++- app/models/template_list.py | 2 +- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/app/models/service.py b/app/models/service.py index 161d77116..3595c1dcb 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: diff --git a/app/models/template_list.py b/app/models/template_list.py index e8bf0a9e2..ad479b115 100644 --- a/app/models/template_list.py +++ b/app/models/template_list.py @@ -40,7 +40,7 @@ class TemplateList(): 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, From 33c3b14febcc3b2a956a528facc71557a59c4f16 Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Thu, 7 Mar 2019 12:02:02 +0000 Subject: [PATCH 08/10] Test how user folder permissions work on the page --- tests/app/main/views/test_template_folders.py | 153 +++++++++++++++++- 1 file changed, 152 insertions(+), 1 deletion(-) 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') From 347955a378eba13d124ff74ac7bfffc72e96cd28 Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Thu, 7 Mar 2019 15:23:18 +0000 Subject: [PATCH 09/10] Make sure that only templates inside visible folders are considered When filtering by template type we should ignore any templates that are inside folders user does not have permission for. Otherwise the parent folder can show up as empty instead of not showing up at all. This adds check for user_permissions to is_folder_visible on the service model in admin. --- app/models/service.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/app/models/service.py b/app/models/service.py index 3595c1dcb..92b88d8a1 100644 --- a/app/models/service.py +++ b/app/models/service.py @@ -424,7 +424,7 @@ class Service(): folder for folder in folders if ( folder['parent_id'] == parent_folder_id - and self.is_folder_visible(folder['id'], template_type) + and self.is_folder_visible(folder['id'], template_type, user_id) ) ] @@ -437,7 +437,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 @@ -446,8 +446,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 From 577e25bd5276172cd3460fa252d675cb612eeaa0 Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Thu, 7 Mar 2019 17:09:21 +0000 Subject: [PATCH 10/10] Explain the workings of get_user_template_folders --- app/models/service.py | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/app/models/service.py b/app/models/service.py index 92b88d8a1..55abc4526 100644 --- a/app/models/service.py +++ b/app/models/service.py @@ -385,6 +385,19 @@ class Service(): return {folder['id'] for folder in self.all_template_folders} 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