From 24ee280632c74449098b83813c3d57a36fb25155 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 1 Mar 2019 14:11:33 +0000 Subject: [PATCH 1/3] Make reply to address dependent on sending intent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We have a number of go live requests where people have said they’re sending email, but haven’t set up a reply-to address. At the moment we don’t prompt them to, because we look at whether they have email templates as indicative of whether they’re going to send email. Now that we explicitly ask for the volumes of email they’re sending we should use this to determine whether or not we prompt them to set up an email reply to address because it’s a stronger signal of intent than what templates they’ve set up. --- app/models/service.py | 2 +- tests/app/main/views/test_service_settings.py | 11 +++++------ 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/app/models/service.py b/app/models/service.py index 06c1403f7..52ed19f90 100644 --- a/app/models/service.py +++ b/app/models/service.py @@ -226,7 +226,7 @@ class Service(): @property def needs_to_add_email_reply_to_address(self): - return self.has_email_templates and not self.has_email_reply_to_address + return self.volume_email and not self.has_email_reply_to_address @property def shouldnt_use_govuk_as_sms_sender(self): diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index de723e0cb..e58d7b9ac 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -1160,7 +1160,7 @@ def test_should_redirect_after_request_to_go_live( True, True, True, - 1, 1, 1, + 1, 0, 0, 'Yes', True, [ @@ -1194,7 +1194,7 @@ def test_should_redirect_after_request_to_go_live( True, True, False, - 1, 1, 1, + 0, 1, 0, 'Yes', True, [ @@ -1210,7 +1210,7 @@ def test_should_redirect_after_request_to_go_live( True, True, True, - 1, 1, 1, + 0, 1, 0, 'No', True, [ @@ -1246,7 +1246,7 @@ def test_should_redirect_after_request_to_go_live( True, True, False, - 1, 1, 1, + 0, 1, 0, 'No', True, [ @@ -1256,7 +1256,7 @@ def test_should_redirect_after_request_to_go_live( 'notify_request_to_go_live_incomplete_template_content', ], ), - ( # Everything is wrong + ( # Not done anything yet False, False, True, @@ -1273,7 +1273,6 @@ def test_should_redirect_after_request_to_go_live( 'notify_request_to_go_live_incomplete_volumes', 'notify_request_to_go_live_incomplete_checklist', 'notify_request_to_go_live_incomplete_mou', - 'notify_request_to_go_live_incomplete_email_reply_to', 'notify_request_to_go_live_incomplete_team_member', 'notify_request_to_go_live_incomplete_template_content', 'notify_request_to_go_live_incomplete_sms_sender', From 531219b87e308568f0eb61844f33b52f12310a1d Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 1 Mar 2019 15:10:51 +0000 Subject: [PATCH 2/3] Make SMS sender dependent on estimated SMS volumes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We have a number of go live requests where people have said they’re sending text messages, but haven’t changed the text message sender from the default of `GOVUK` (we ask teams who aren’t central government to do this). At the moment we don’t prompt them to, because we look at whether they have text message templates as indicative of whether they’re going to send text messages. Now that we explicitly ask for the volumes of text messages they’re sending we should use this to determine whether or not we prompt them to change their text message sender because it’s a stronger signal of intent than what templates they’ve set up. --- app/models/service.py | 2 +- tests/app/main/views/test_service_settings.py | 5 ++--- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/app/models/service.py b/app/models/service.py index 52ed19f90..c789dca8b 100644 --- a/app/models/service.py +++ b/app/models/service.py @@ -269,7 +269,7 @@ class Service(): @property def needs_to_change_sms_sender(self): return all(( - self.has_sms_templates, + self.volume_sms, self.shouldnt_use_govuk_as_sms_sender, self.sms_sender_is_govuk, )) diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index e58d7b9ac..747a8b884 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -1176,7 +1176,7 @@ def test_should_redirect_after_request_to_go_live( False, True, True, - 1, 1, 1, + 1, 0, 1, 'No', True, [ @@ -1228,7 +1228,7 @@ def test_should_redirect_after_request_to_go_live( True, True, False, - 1, 1, 1, + 1, 0, 0, 'No', True, [ @@ -1275,7 +1275,6 @@ def test_should_redirect_after_request_to_go_live( 'notify_request_to_go_live_incomplete_mou', 'notify_request_to_go_live_incomplete_team_member', 'notify_request_to_go_live_incomplete_template_content', - 'notify_request_to_go_live_incomplete_sms_sender', ], ), ), From 87c62e6c52da34f29701adfdb0db7873f4d84f22 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 5 Mar 2019 10:30:40 +0000 Subject: [PATCH 3/3] Only hide channel-specific tasks if volume is 0 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If you have email templates but haven’t told us what volumes you’re sending we should assume you are going to send emails. We should only stop asking you to add a reply-to address once you’ve told us for sure you’re not going to send any emails. This also applies to changing the text message sender – this should only be hidden if you don’t have text message templates or you’ve said you’re not going to be sending any text messages. --- .../service-settings/request-to-go-live.html | 11 ++- tests/app/main/views/test_service_settings.py | 69 +++++++++++++++---- 2 files changed, 64 insertions(+), 16 deletions(-) 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 d9e25079a..27afc4b53 100644 --- a/app/templates/views/service-settings/request-to-go-live.html +++ b/app/templates/views/service-settings/request-to-go-live.html @@ -27,14 +27,21 @@ 'Add templates with examples of the content you plan to send', url_for('main.choose_template', service_id=current_service.id), ) }} - {% if current_service.has_email_templates %} + {% if ( + current_service.has_email_templates + and (current_service.volume_email != 0) + ) %} {{ task_list_item( current_service.has_email_reply_to_address, 'Add an email reply-to address', url_for('main.service_email_reply_to', service_id=current_service.id), ) }} {% endif %} - {% if current_service.has_sms_templates and current_service.shouldnt_use_govuk_as_sms_sender %} + {% if ( + current_service.has_sms_templates + and current_service.shouldnt_use_govuk_as_sms_sender + and (current_service.volume_sms != 0) + ) %} {{ task_list_item( not current_service.sms_sender_is_govuk, 'Change your text message sender name', diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index 747a8b884..c23238016 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -522,6 +522,45 @@ def test_should_raise_duplicate_name_handled( ((1, 0, 0), True, 'Tell us how many messages you expect to send Completed'), ((9, 99, 999), True, 'Tell us how many messages you expect to send Completed'), ]) +def test_should_check_if_estimated_volumes_provided( + client_request, + mocker, + single_sms_sender, + single_reply_to_email_address, + mock_get_service_templates, + mock_get_users_by_service, + volumes, + consent_to_research, + expected_estimated_volumes_item, +): + + for volume, channel in zip(volumes, ('sms', 'email', 'letter')): + mocker.patch( + 'app.models.service.Service.volume_{}'.format(channel), + create=True, + new_callable=PropertyMock, + return_value=volume, + ) + + mocker.patch( + 'app.models.service.Service.consent_to_research', + create=True, + new_callable=PropertyMock, + return_value=consent_to_research, + ) + + page = client_request.get( + 'main.request_to_go_live', service_id=SERVICE_ONE_ID + ) + assert page.h1.text == 'Before you request to go live' + + assert normalize_spaces( + page.select_one('.task-list .task-list-item').text + ) == ( + expected_estimated_volumes_item + ) + + @pytest.mark.parametrize('count_of_users_with_manage_service, expected_user_checklist_item', [ (1, 'Add a team member who can manage settings, team and usage Not completed'), (2, 'Add a team member who can manage settings, team and usage Completed'), @@ -537,7 +576,7 @@ def test_should_raise_duplicate_name_handled( (1, [], 'Add an email reply-to address Not completed'), (1, [{}], 'Add an email reply-to address Completed'), ]) -def test_should_show_request_to_go_live_checklist( +def test_should_check_for_sending_things_right( client_request, mocker, single_sms_sender, @@ -548,9 +587,6 @@ def test_should_show_request_to_go_live_checklist( count_of_email_templates, reply_to_email_addresses, expected_reply_to_checklist_item, - volumes, - consent_to_research, - expected_estimated_volumes_item, ): def _templates_by_type(template_type): @@ -580,21 +616,14 @@ def test_should_show_request_to_go_live_checklist( return_value=reply_to_email_addresses ) - for volume, channel in zip(volumes, ('sms', 'email', 'letter')): + for channel, volume in (('email', 1), ('sms', 0), ('letter', 1)): mocker.patch( 'app.models.service.Service.volume_{}'.format(channel), create=True, new_callable=PropertyMock, - return_value=volume, + return_value=1, ) - mocker.patch( - 'app.models.service.Service.consent_to_research', - create=True, - new_callable=PropertyMock, - return_value=consent_to_research, - ) - page = client_request.get( 'main.request_to_go_live', service_id=SERVICE_ONE_ID ) @@ -602,7 +631,6 @@ def test_should_show_request_to_go_live_checklist( checklist_items = page.select('.task-list .task-list-item') - assert normalize_spaces(checklist_items[0].text) == expected_estimated_volumes_item assert normalize_spaces(checklist_items[1].text) == expected_user_checklist_item assert normalize_spaces(checklist_items[2].text) == expected_templates_checklist_item assert normalize_spaces(checklist_items[3].text) == expected_reply_to_checklist_item @@ -623,6 +651,11 @@ def test_should_show_request_to_go_live_checklist( mock_get_reply_to_email_addresses.assert_called_once_with(SERVICE_ONE_ID) +@pytest.mark.parametrize('estimated_sms_volume', ( + pytest.param(None), + pytest.param(1), + pytest.param(0, marks=pytest.mark.xfail(raises=IndexError)), +)) @pytest.mark.parametrize('organisation_type,count_of_sms_templates, sms_senders, expected_sms_sender_checklist_item', [ pytest.param( 'local', @@ -688,6 +721,7 @@ def test_should_check_for_sms_sender_on_go_live( count_of_sms_templates, sms_senders, expected_sms_sender_checklist_item, + estimated_sms_volume, ): service_one['organisation_type'] = organisation_type @@ -721,6 +755,13 @@ def test_should_check_for_sms_sender_on_go_live( return_value=[], ) + mocker.patch( + 'app.models.service.Service.volume_sms', + create=True, + new_callable=PropertyMock, + return_value=estimated_sms_volume, + ) + page = client_request.get( 'main.request_to_go_live', service_id=SERVICE_ONE_ID )