From 1ef4e72e8784783269c1cb5f6d680d59107283f5 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 23 Aug 2018 16:11:08 +0100 Subject: [PATCH] Check text message sender before going live MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We often check that a service has an appropriate text message sender as a condition of them going live. We don’t mention this anywhere. The services for whom GOVUK is definitely not an appropriate sender are those in local government. As we have more of these teams starting to use Notify, we should streamline the process by making this check automated. This commit adds that check, for teams who: - have text message templates - have self-declared as NHS or local government --- app/main/views/service_settings.py | 20 +-- .../service-settings/request-to-go-live.html | 6 + app/utils.py | 8 ++ tests/app/main/views/test_service_settings.py | 114 +++++++++++++++++- 4 files changed, 140 insertions(+), 8 deletions(-) diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index e7c87f6a7d5..ef734e901ce 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -53,6 +53,7 @@ AgreementInfo, email_safe, get_cdn_domain, + get_default_sms_sender, user_has_permissions, user_is_platform_admin, ) @@ -83,10 +84,6 @@ def service_settings(service_id): (Field(x['contact_block'], html='escape') for x in letter_contact_details if x['is_default']), "Not set" ) sms_senders = service_api_client.get_sms_senders(service_id) - sms_sender_count = len(sms_senders) - default_sms_sender = next( - (Field(x['sms_sender'], html='escape') for x in sms_senders if x['is_default']), "None" - ) free_sms_fragment_limit = billing_api_client.get_free_sms_fragment_limit_for_year(service_id) data_retention = service_api_client.get_service_data_retention(service_id) @@ -103,8 +100,8 @@ def service_settings(service_id): reply_to_email_address_count=reply_to_email_address_count, default_letter_contact_block=default_letter_contact_block, letter_contact_details_count=letter_contact_details_count, - default_sms_sender=default_sms_sender, - sms_sender_count=sms_sender_count, + default_sms_sender=get_default_sms_sender(sms_senders), + sms_sender_count=len(sms_senders), free_sms_fragment_limit=free_sms_fragment_limit, prefix_sms=current_service.prefix_sms, organisation=organisation, @@ -192,9 +189,18 @@ def request_to_go_live(service_id): has_email_templates=( service_api_client.count_service_templates(service_id, template_type='email') > 0 ), + has_sms_templates=( + service_api_client.count_service_templates(service_id, template_type='sms') > 0 + ), has_email_reply_to_address=bool( service_api_client.get_reply_to_email_addresses(service_id) - ) + ), + shouldnt_use_govuk_as_sms_sender=( + current_service.organisation_type in {'local', 'nhs'} + ), + sms_sender_is_govuk=get_default_sms_sender( + service_api_client.get_sms_senders(service_id) + ) in {'GOVUK', 'None'}, ) diff --git a/app/templates/views/service-settings/request-to-go-live.html b/app/templates/views/service-settings/request-to-go-live.html index 4858714caa4..fbd228fea81 100644 --- a/app/templates/views/service-settings/request-to-go-live.html +++ b/app/templates/views/service-settings/request-to-go-live.html @@ -38,6 +38,12 @@

Request to go live

)|safe, ) }} {% endif %} + {% if has_sms_templates and shouldnt_use_govuk_as_sms_sender %} + {{ task_list_item( + not sms_sender_is_govuk, + 'Change your text message sender from GOVUK' + ) }} + {% endif %} {% endcall %}

You also need to accept our terms of use. diff --git a/app/utils.py b/app/utils.py index ac3251bf43b..0590dd5851f 100644 --- a/app/utils.py +++ b/app/utils.py @@ -25,6 +25,7 @@ url_for, ) from flask_login import current_user +from notifications_utils.field import Field from notifications_utils.formatters import make_quotes_smart from notifications_utils.recipients import RecipientCSV from notifications_utils.take import Take @@ -663,3 +664,10 @@ def should_skip_template_page(template_type): not current_user.has_permissions('manage_templates', 'manage_api_keys') and template_type != 'letter' ) + + +def get_default_sms_sender(sms_senders): + return str(next(( + Field(x['sms_sender'], html='escape') + for x in sms_senders if x['is_default'] + ), "None")) diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index c26ab1194f1..c7118e5408b 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -517,9 +517,13 @@ def test_should_raise_duplicate_name_handled( (1, [], 'An email reply-to address Not completed'), (1, [{}], 'An email reply-to address Completed'), ]) +@pytest.mark.parametrize('expected_sms_sender_item', [ + ('Templates showing the kind of messages you plan to send Not completed'), +]) def test_should_show_request_to_go_live_checklist( client_request, mocker, + single_sms_sender, count_of_users_with_manage_service, expected_user_checklist_item, count_of_templates, @@ -527,11 +531,13 @@ def test_should_show_request_to_go_live_checklist( count_of_email_templates, reply_to_email_addresses, expected_reply_to_checklist_item, + expected_sms_sender_item, ): def _count_templates(service_id, template_type=None): return { - 'email': count_of_email_templates + 'email': count_of_email_templates, + 'sms': 0, }.get(template_type, count_of_templates) mock_count_users = mocker.patch( @@ -567,12 +573,118 @@ def _count_templates(service_id, template_type=None): 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'), ] if count_of_email_templates: mock_get_reply_to_email_addresses.assert_called_once_with(SERVICE_ONE_ID) +@pytest.mark.parametrize('organisation_type,count_of_sms_templates, sms_senders, expected_sms_sender_checklist_item', [ + pytest.mark.xfail(( + 'local', + 0, + [], + '', + ), raises=IndexError), + pytest.mark.xfail(( + 'local', + 0, + [{'is_default': True, 'sms_sender': 'GOVUK'}], + '', + ), raises=IndexError), + pytest.mark.xfail(( + None, + 99, + [{'is_default': True, 'sms_sender': 'GOVUK'}], + '', + ), raises=IndexError), + pytest.mark.xfail(( + 'central', + 99, + [{'is_default': True, 'sms_sender': 'GOVUK'}], + '', + ), raises=IndexError), + ( + 'local', + 1, + [], + 'Change your text message sender from GOVUK Not completed', + ), + ( + 'local', + 1, + [{'is_default': True, 'sms_sender': 'GOVUK'}], + 'Change your text message sender from GOVUK Not completed', + ), + ( + 'local', + 1, + [ + {'is_default': False, 'sms_sender': 'GOVUK'}, + {'is_default': True, 'sms_sender': 'KUVOG'}, + ], + 'Change your text message sender from GOVUK Completed', + ), + ( + 'nhs', + 1, + [{'is_default': True, 'sms_sender': 'KUVOG'}], + 'Change your text message sender from GOVUK Completed', + ), +]) +def test_should_check_for_sms_sender_on_go_live( + client_request, + service_one, + mocker, + organisation_type, + count_of_sms_templates, + sms_senders, + expected_sms_sender_checklist_item, +): + + service_one['organisation_type'] = organisation_type + + def _count_templates(service_id, template_type=None): + return { + 'email': 0, + 'sms': 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_get_sms_senders = mocker.patch( + 'app.main.views.service_settings.service_api_client.get_sms_senders', + return_value=sms_senders, + ) + mocker.patch( + 'app.main.views.service_settings.service_api_client.get_reply_to_email_addresses', + return_value=[], + ) + + page = client_request.get( + 'main.request_to_go_live', service_id=SERVICE_ONE_ID + ) + assert page.h1.text == 'Request to 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'), + ] + + mock_get_sms_senders.assert_called_once_with(SERVICE_ONE_ID) + + def test_should_show_request_to_go_live( client_request, ):