From 1e6b79a546e2cb706abf3e909061372265c943f2 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 25 Oct 2018 07:59:50 +0100 Subject: [PATCH] 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',