Merge pull request #2823 from alphagov/hide-folders-when-no-permissions

Only show folders that user has permission to see

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(),
         )
 

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

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

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')

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]
+        }
+    ]