From d69e8b50cd028dd00eaee68c71fa48bb85abfe19 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 26 Oct 2018 08:20:00 +0100 Subject: [PATCH 01/13] Only initialised service model once per request MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `_get_current_service` is a function which gets called every time `current_service` is referenced in a view method or Jinja template. Because the service model was getting initialised inside this function it was being reconstructed many times in one request. On the service settings page, for example, it was getting initialised 43 times, adding about 200ms to the response time. This commit moves its initialisation to the point where we’re getting the data from the API, which only happens once per request. --- app/__init__.py | 6 ++++-- tests/app/notify_client/test_notify_admin_api_client.py | 7 ++++--- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/app/__init__.py b/app/__init__.py index 7e0f9229b..9e749d3d7 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -99,7 +99,7 @@ platform_stats_api_client = PlatformStatsAPIClient() # The current service attached to the request stack. def _get_current_service(): - return Service(_lookup_req_object('service')) + return _lookup_req_object('service') current_service = LocalProxy(_get_current_service) @@ -465,7 +465,9 @@ def load_service_before_request(): if service_id: try: - _request_ctx_stack.top.service = service_api_client.get_service(service_id)['data'] + _request_ctx_stack.top.service = Service( + service_api_client.get_service(service_id)['data'] + ) except HTTPError as exc: # if service id isn't real, then 404 rather than 500ing later because we expect service to be set if exc.status_code == 404: diff --git a/tests/app/notify_client/test_notify_admin_api_client.py b/tests/app/notify_client/test_notify_admin_api_client.py index 4082647b0..3f127abad 100644 --- a/tests/app/notify_client/test_notify_admin_api_client.py +++ b/tests/app/notify_client/test_notify_admin_api_client.py @@ -5,6 +5,7 @@ import pytest import werkzeug from app.notify_client import NotifyAdminAPIClient +from app.notify_client.models import Service from tests import service_json from tests.conftest import api_user_active, platform_admin_user, set_config @@ -29,7 +30,7 @@ def test_active_service_can_be_modified(app_, method, user, service): with app_.test_request_context() as request_context, app_.test_client() as client: client.login(user) - request_context.service = service + request_context.service = Service(service) with patch.object(api_client, 'request') as request: ret = getattr(api_client, method)('url', 'data') @@ -48,7 +49,7 @@ def test_inactive_service_cannot_be_modified_by_normal_user(app_, api_user_activ with app_.test_request_context() as request_context, app_.test_client() as client: client.login(api_user_active) - request_context.service = service_json(active=False) + request_context.service = Service(service_json(active=False)) with patch.object(api_client, 'request') as request: with pytest.raises(werkzeug.exceptions.Forbidden): @@ -67,7 +68,7 @@ def test_inactive_service_can_be_modified_by_platform_admin(app_, platform_admin with app_.test_request_context() as request_context, app_.test_client() as client: client.login(platform_admin_user) - request_context.service = service_json(active=False) + request_context.service = Service(service_json(active=False)) with patch.object(api_client, 'request') as request: ret = getattr(api_client, method)('url', 'data') From 1e6b79a546e2cb706abf3e909061372265c943f2 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 25 Oct 2018 07:59:50 +0100 Subject: [PATCH 02/13] Put templates on service model MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We do a lot of logic around choosing which templates to show. This logic is all inside one view method. It makes it cleaner to break this logic up into functions. But this would mean passing around variables from one function to another. Putting these methods onto a class (the service model) means that there’s a place to store this data (rather than having to pass it around a lot). Making this code more manageable is important so that when we have templates and folders it’s easy to encapsulate the logic around combining the two. --- app/main/views/conversation.py | 10 +-- app/main/views/dashboard.py | 1 - app/main/views/templates.py | 37 ++--------- app/notify_client/models.py | 46 +++++++++---- app/templates/views/dashboard/dashboard.html | 2 +- tests/app/main/views/test_service_settings.py | 65 ++++++++++++------- 6 files changed, 86 insertions(+), 75 deletions(-) diff --git a/app/main/views/conversation.py b/app/main/views/conversation.py index 2dac0478a..540945afb 100644 --- a/app/main/views/conversation.py +++ b/app/main/views/conversation.py @@ -4,7 +4,7 @@ from notifications_python_client.errors import HTTPError from notifications_utils.recipients import format_phone_number_human_readable from notifications_utils.template import SMSPreviewTemplate -from app import notification_api_client, service_api_client +from app import current_service, notification_api_client, service_api_client from app.main import main from app.main.forms import SearchTemplatesForm from app.utils import user_has_permissions @@ -44,13 +44,7 @@ def conversation_reply( service_id, notification_id, ): - - templates = [ - template - for template in service_api_client.get_service_templates(service_id)['data'] - if template['template_type'] == 'sms' - ] - + templates = current_service.templates_by_type('sms') return render_template( 'views/templates/choose-reply.html', templates=templates, diff --git a/app/main/views/dashboard.py b/app/main/views/dashboard.py index 1aa5c1c37..f678babf6 100644 --- a/app/main/views/dashboard.py +++ b/app/main/views/dashboard.py @@ -72,7 +72,6 @@ def service_dashboard(service_id): return render_template( 'views/dashboard/dashboard.html', updates_url=url_for(".service_dashboard_updates", service_id=service_id), - templates=service_api_client.get_service_templates(service_id)['data'], partials=get_dashboard_partials(service_id) ) diff --git a/app/main/views/templates.py b/app/main/views/templates.py index 91e397b1e..ee1d6ec3c 100644 --- a/app/main/views/templates.py +++ b/app/main/views/templates.py @@ -101,24 +101,6 @@ def start_tour(service_id, template_id): @login_required @user_has_permissions() def choose_template(service_id, template_type='all'): - templates = service_api_client.get_service_templates(service_id)['data'] - - letters_available = current_service.has_permission('letter') - - available_template_types = list(filter(None, ( - 'email', - 'sms', - 'letter' if letters_available else None, - ))) - - templates = [ - template for template in templates - if template['template_type'] in available_template_types - ] - - has_multiple_template_types = len({ - template['template_type'] for template in templates - }) > 1 template_nav_items = [ (label, key, url_for('.choose_template', service_id=current_service.id, template_type=key), '') @@ -126,23 +108,18 @@ def choose_template(service_id, template_type='all'): ('All', 'all'), ('Text message', 'sms'), ('Email', 'email'), - ('Letter', 'letter') if letters_available else None, + ('Letter', 'letter') if current_service.has_permission('letter') else None, ]) ] - templates_on_page = [ - template for template in templates - if ( - template_type in ['all', template['template_type']] and - template['template_type'] in available_template_types - ) - ] - return render_template( 'views/templates/choose.html', - templates=templates_on_page, - show_search_box=(len(templates_on_page) > 7), - show_template_nav=has_multiple_template_types and (len(templates) > 2), + templates=current_service.templates_by_type(template_type), + show_search_box=(len(current_service.templates_by_type(template_type)) > 7), + show_template_nav=( + current_service.has_multiple_template_types + and (len(current_service.templates) > 2) + ), template_nav_items=template_nav_items, template_type=template_type, search_form=SearchTemplatesForm(), diff --git a/app/notify_client/models.py b/app/notify_client/models.py index 0d94eba41..137c21b20 100644 --- a/app/notify_client/models.py +++ b/app/notify_client/models.py @@ -319,25 +319,47 @@ class Service(dict): ) > 1 @property - def has_templates(self): + def templates(self): + from app import service_api_client - return service_api_client.count_service_templates( - self.id - ) > 0 + + templates = service_api_client.get_service_templates(self.id)['data'] + + return [ + template for template in templates + if template['template_type'] in self.available_template_types + ] + + def templates_by_type(self, template_type): + return [ + template for template in self.templates + if template_type in {'all', template['template_type']} + ] + + @property + def available_template_types(self): + return [ + channel for channel in ('email', 'sms', 'letter') + if self.has_permission(channel) + ] + + @property + def has_templates(self): + return len(self.templates) > 0 + + @property + def has_multiple_template_types(self): + return len({ + template['template_type'] for template in self.templates + }) > 1 @property def has_email_templates(self): - from app import service_api_client - return service_api_client.count_service_templates( - self.id, template_type='email' - ) > 0 + return len(self.templates_by_type('email')) > 0 @property def has_sms_templates(self): - from app import service_api_client - return service_api_client.count_service_templates( - self.id, template_type='sms' - ) > 0 + return len(self.templates_by_type('sms')) > 0 @property def has_email_reply_to_address(self): diff --git a/app/templates/views/dashboard/dashboard.html b/app/templates/views/dashboard/dashboard.html index fd2f25a2f..3ddcc0f39 100644 --- a/app/templates/views/dashboard/dashboard.html +++ b/app/templates/views/dashboard/dashboard.html @@ -15,7 +15,7 @@

Dashboard

- {% if current_user.has_permissions('manage_templates') and not templates %} + {% if current_user.has_permissions('manage_templates') and not current_service.templates %} {% include 'views/dashboard/write-first-messages.html' %} {% endif %} diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index 4e073dcc6..85b4c69dd 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -551,20 +551,28 @@ def test_should_show_request_to_go_live_checklist( expected_reply_to_checklist_item, ): - def _count_templates(service_id, template_type=None): + def _templates_by_type(template_type): return { - 'email': count_of_email_templates, - 'sms': 0, - }.get(template_type, count_of_templates) + 'email': list(range(0, count_of_email_templates)), + 'sms': [], + }.get(template_type) mock_count_users = mocker.patch( 'app.main.views.service_settings.user_api_client.get_count_of_users_with_permission', return_value=count_of_users_with_manage_service ) - mock_count_templates = mocker.patch( - 'app.main.views.service_settings.service_api_client.count_service_templates', - side_effect=_count_templates + + mock_templates = mocker.patch( + 'app.notify_client.models.Service.templates', + new_callable=PropertyMock, + return_value=list(range(0, count_of_templates)), ) + + mock_templates_by_type = mocker.patch( + 'app.notify_client.models.Service.templates_by_type', + side_effect=_templates_by_type, + ) + mock_get_reply_to_email_addresses = mocker.patch( 'app.main.views.service_settings.service_api_client.get_reply_to_email_addresses', return_value=reply_to_email_addresses @@ -587,10 +595,12 @@ def test_should_show_request_to_go_live_checklist( ) mock_count_users.assert_called_once_with(SERVICE_ONE_ID, 'manage_service') - assert mock_count_templates.call_args_list == [ - call(SERVICE_ONE_ID), - call(SERVICE_ONE_ID, template_type='email'), - call(SERVICE_ONE_ID, template_type='sms'), + assert mock_templates.call_args_list == [ + call(), + ] + assert mock_templates_by_type.call_args_list == [ + call('email'), + call('sms'), ] if count_of_email_templates: @@ -662,20 +672,26 @@ def test_should_check_for_sms_sender_on_go_live( service_one['organisation_type'] = organisation_type - def _count_templates(service_id, template_type=None): - return { + def _templates_by_type(template_type): + return list(range(0, { 'email': 0, 'sms': count_of_sms_templates, - }.get(template_type, count_of_sms_templates) + }.get(template_type, count_of_sms_templates))) mocker.patch( 'app.main.views.service_settings.user_api_client.get_count_of_users_with_permission', return_value=99, ) - mock_count_templates = mocker.patch( - 'app.main.views.service_settings.service_api_client.count_service_templates', - side_effect=_count_templates, + mock_templates = mocker.patch( + 'app.notify_client.models.Service.templates', + new_callable=PropertyMock, + side_effect=partial(_templates_by_type, 'all'), ) + mock_templates_by_type = mocker.patch( + 'app.notify_client.models.Service.templates_by_type', + side_effect=_templates_by_type, + ) + mock_get_sms_senders = mocker.patch( 'app.main.views.service_settings.service_api_client.get_sms_senders', return_value=sms_senders, @@ -693,10 +709,12 @@ def test_should_check_for_sms_sender_on_go_live( checklist_items = page.select('.task-list .task-list-item') assert normalize_spaces(checklist_items[2].text) == expected_sms_sender_checklist_item - assert mock_count_templates.call_args_list == [ - call(SERVICE_ONE_ID), - call(SERVICE_ONE_ID, template_type='email'), - call(SERVICE_ONE_ID, template_type='sms'), + assert mock_templates.call_args_list == [ + call(), + ] + assert mock_templates_by_type.call_args_list == [ + call('email'), + call('sms'), ] mock_get_sms_senders.assert_called_once_with(SERVICE_ONE_ID) @@ -729,8 +747,9 @@ def test_should_check_for_mou_on_request_to_go_live( return_value=0, ) mocker.patch( - 'app.main.views.service_settings.service_api_client.count_service_templates', - return_value=0, + 'app.notify_client.models.Service.templates', + new_callable=PropertyMock, + return_value=[], ) mocker.patch( 'app.main.views.service_settings.service_api_client.get_sms_senders', From b48305c50d0d6b1674a39e0fd5a3c4768d281a66 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 26 Oct 2018 11:19:50 +0100 Subject: [PATCH 03/13] =?UTF-8?q?Don=E2=80=99t=20have=20service=20model=20?= =?UTF-8?q?inherit=20from=20`dict`?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Inheriting from `dict` has some unexpected side effects that don’t happen with plain object. The one we want to avoid right now is that a dict doesn’t seem to implement `__dict__` in a normal way, which is required by `werkzeug.utils.cached_property`. --- app/notify_client/models.py | 22 +++++++++++++++---- .../partials/check/too-many-messages.html | 2 +- app/templates/views/service-settings.html | 4 ++-- 3 files changed, 21 insertions(+), 7 deletions(-) diff --git a/app/notify_client/models.py b/app/notify_client/models.py index 137c21b20..c3bf3749c 100644 --- a/app/notify_client/models.py +++ b/app/notify_client/models.py @@ -268,7 +268,7 @@ class AnonymousUser(AnonymousUserMixin): return False -class Service(dict): +class Service(): ALLOWED_PROPERTIES = { 'active', @@ -291,16 +291,30 @@ class Service(dict): def __init__(self, _dict): # in the case of a bad request current service may be `None` - super().__init__(_dict or {}) + self._dict = _dict or {} + if 'permissions' not in self._dict: + self.permissions = {'email', 'sms', 'letter'} + + def __bool__(self): + return self._dict != {} def __getattr__(self, attr): if attr in self.ALLOWED_PROPERTIES: - return self[attr] + return self._dict[attr] raise AttributeError('`{}` is not a service attribute'.format(attr)) + def __getitem__(self, attr): + return self.__getattr__(attr) + + def get(self, attr, default=None): + try: + return self._dict[attr] + except KeyError: + return default + @property def trial_mode(self): - return self['restricted'] + return self._dict['restricted'] def has_permission(self, permission): return permission in self.permissions diff --git a/app/templates/partials/check/too-many-messages.html b/app/templates/partials/check/too-many-messages.html index b75dbb65f..073f906d7 100644 --- a/app/templates/partials/check/too-many-messages.html +++ b/app/templates/partials/check/too-many-messages.html @@ -7,7 +7,7 @@

You can only send {{ current_service.message_limit }} messages per day - {%- if current_service.restricted %} + {%- if current_service.trial_mode %} in trial mode {%- endif -%} . diff --git a/app/templates/views/service-settings.html b/app/templates/views/service-settings.html index e4b8e16cd..b6aa23ad3 100644 --- a/app/templates/views/service-settings.html +++ b/app/templates/views/service-settings.html @@ -253,7 +253,7 @@ {% endcall %}

- {% if current_service.restricted %} + {% if current_service.trial_mode %}

Your service is in trial mode