darkhelm/notifications-admin
1
0
Fork 0
mirror of https://github.com/GSA/notifications-admin.git synced 2026-02-05 19:03:30 -05:00
Code Issues Packages Projects Releases Wiki Activity

Merge pull request #4258 from alphagov/speed-up-templates-page-179736794

Browse Source 
Optimise load time for service "Templates" page
This commit is contained in:
Ben Thorner
2022-06-08 13:37:58 +01:00
committed by GitHub
parent f52a8c56d7 5b52aa2381
commit 543be77776
6 changed files with 294 additions and 326 deletions
Download Patch File Download Diff File Expand all files Collapse all files

11
app/main/views/templates.py
View File

@@ -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(

95
app/models/service.py
View File

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

181
app/models/template_list.py
View File

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

34
tests/app/main/views/test_template_folders.py
View File

@@ -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',

174
tests/app/models/test_service.py
View File

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

125
tests/app/models/test_template_list.py Normal file
View File

@@ -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",
)