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