From 5f4280cf81a7ea2848af3a2e67584f9471ce1d21 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 27 Feb 2019 17:05:02 +0000 Subject: [PATCH 1/2] Let people go live without filling the volumes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit At the moment it 500s because it can’t format the `None` values as numbers. In the future we will stop people requesting to go live until they’ve provided this info. For now it has to be optional. --- app/main/views/service_settings.py | 23 +++++++--- tests/app/main/views/test_service_settings.py | 45 ++++++++++++++++--- 2 files changed, 56 insertions(+), 12 deletions(-) diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index 4d0333e51..5f581f5cf 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -201,9 +201,9 @@ def submit_request_to_go_live(service_id): '\nOrganisation type: {organisation_type}' '\nAgreement signed: {agreement}' '\nChecklist completed: {checklist}' - '\nEmails in next year: {volume_email:,.0f}' - '\nText messages in next year: {volume_sms:,.0f}' - '\nLetters in next year: {volume_letter:,.0f}' + '\nEmails in next year: {volume_email_formatted}' + '\nText messages in next year: {volume_sms_formatted}' + '\nLetters in next year: {volume_letter_formatted}' '\nConsent to research: {research_consent}' '\nOther live services: {existing_live}' '\n' @@ -225,9 +225,12 @@ def submit_request_to_go_live(service_id): organisation_type=str(current_service.organisation_type).title(), agreement=AgreementInfo.from_current_user().as_human_readable, checklist=current_service.go_live_checklist_completed_as_yes_no, - volume_email=current_service.volume_email, - volume_sms=current_service.volume_sms, - volume_letter=current_service.volume_letter, + volume_email=print_if_number(current_service.volume_email), + volume_email_formatted=format_if_number(current_service.volume_email), + volume_sms=print_if_number(current_service.volume_sms), + volume_sms_formatted=format_if_number(current_service.volume_sms), + volume_letter=print_if_number(current_service.volume_letter), + volume_letter_formatted=format_if_number(current_service.volume_letter), research_consent='Yes' if current_service.consent_to_research else 'No', existing_live='Yes' if user_api_client.user_has_live_services(current_user) else 'No', service_id=current_service.id, @@ -1070,3 +1073,11 @@ def _get_request_to_go_live_tags(service, agreement_signed): ): if test: yield BASE + '_incomplete' + tag + + +def print_if_number(value): + return value if isinstance(value, int) else '' + + +def format_if_number(value): + return '{:,.0f}'.format(value) if isinstance(value, int) else '' diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index 2e85005aa..3974a6bea 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -1017,6 +1017,26 @@ def test_non_gov_users_cant_request_to_go_live( ) +@pytest.mark.parametrize('volumes, displayed_volumes, formatted_displayed_volumes', ( + ( + (('email', None), ('sms', None), ('letter', None)), + ', , ', + ( + 'Emails in next year: \n' + 'Text messages in next year: \n' + 'Letters in next year: \n' + ), + ), + ( + (('email', 1234), ('sms', 0), ('letter', 999)), + '0, 1234, 999', # This is a different order to match the spreadsheet + ( + 'Emails in next year: 1,234\n' + 'Text messages in next year: 0\n' + 'Letters in next year: 999\n' + ), + ), +)) @freeze_time("2012-12-21") def test_should_redirect_after_request_to_go_live( client_request, @@ -1030,7 +1050,17 @@ def test_should_redirect_after_request_to_go_live( mock_get_service_settings_page_common, mock_get_service_templates, mock_get_users_by_service, + volumes, + displayed_volumes, + formatted_displayed_volumes, ): + for channel, volume in volumes: + mocker.patch( + 'app.models.service.Service.volume_{}'.format(channel), + create=True, + new_callable=PropertyMock, + return_value=volume, + ) mock_post = mocker.patch('app.main.views.service_settings.zendesk_client.create_ticket', autospec=True) page = client_request.post( 'main.request_to_go_live', @@ -1053,21 +1083,24 @@ def test_should_redirect_after_request_to_go_live( ) assert mock_post.call_args[1]['message'] == ( 'Service: service one\n' - 'http://localhost/services/{}\n' + 'http://localhost/services/{service_id}\n' '\n' '---\n' 'Organisation type: Central\n' 'Agreement signed: Can’t tell (domain is user.gov.uk)\n' 'Checklist completed: No\n' - 'Emails in next year: 111,111\n' - 'Text messages in next year: 222,222\n' - 'Letters in next year: 333,333\n' + '{formatted_displayed_volumes}' 'Consent to research: Yes\n' 'Other live services: No\n' '\n' '---\n' - '{}, None, service one, Test User, test@user.gov.uk, -, 21/12/2012, 222222, 111111, 333333' - ).format(SERVICE_ONE_ID, SERVICE_ONE_ID) + '{service_id}, None, service one, Test User, test@user.gov.uk, -, 21/12/2012, ' + '{displayed_volumes}' + ).format( + service_id=SERVICE_ONE_ID, + displayed_volumes=displayed_volumes, + formatted_displayed_volumes=formatted_displayed_volumes, + ) assert normalize_spaces(page.select_one('.banner-default').text) == ( 'Thanks for your request to go live. We’ll get back to you within one working day.' From 7ac9884dd5a6e6a66d5b3eab19755d6aecd3cab0 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 27 Feb 2019 17:15:09 +0000 Subject: [PATCH 2/2] =?UTF-8?q?Tag=20tickets=20that=20haven=E2=80=99t=20fi?= =?UTF-8?q?lled=20volumes?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- app/main/views/service_settings.py | 1 + app/models/service.py | 9 ++++++ tests/app/main/views/test_service_settings.py | 29 ++++++++++++++++++- 3 files changed, 38 insertions(+), 1 deletion(-) diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index 5f581f5cf..0852a3419 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -1064,6 +1064,7 @@ def _get_request_to_go_live_tags(service, agreement_signed): for test, tag in ( (True, ''), + (not service.volumes, '_volumes'), (not service.go_live_checklist_completed, '_checklist'), (not agreement_signed, '_mou'), (service.needs_to_add_email_reply_to_address, '_email_reply_to'), diff --git a/app/models/service.py b/app/models/service.py index 7dd75925e..06c1403f7 100644 --- a/app/models/service.py +++ b/app/models/service.py @@ -294,9 +294,18 @@ class Service(): def get_letter_contact_block(self, id): return service_api_client.get_letter_contact(self.id, id) + @property + def volumes(self): + return sum(filter(None, ( + self.volume_email, + self.volume_sms, + self.volume_letter, + ))) + @property def go_live_checklist_completed(self): return all(( + bool(self.volumes), self.has_team_members, self.has_templates, not self.needs_to_add_email_reply_to_address, diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index 3974a6bea..681d0c379 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -1017,7 +1017,7 @@ def test_non_gov_users_cant_request_to_go_live( ) -@pytest.mark.parametrize('volumes, displayed_volumes, formatted_displayed_volumes', ( +@pytest.mark.parametrize('volumes, displayed_volumes, formatted_displayed_volumes, extra_tags', ( ( (('email', None), ('sms', None), ('letter', None)), ', , ', @@ -1026,6 +1026,7 @@ def test_non_gov_users_cant_request_to_go_live( 'Text messages in next year: \n' 'Letters in next year: \n' ), + ['notify_request_to_go_live_incomplete_volumes'] ), ( (('email', 1234), ('sms', 0), ('letter', 999)), @@ -1035,6 +1036,7 @@ def test_non_gov_users_cant_request_to_go_live( 'Text messages in next year: 0\n' 'Letters in next year: 999\n' ), + [], ), )) @freeze_time("2012-12-21") @@ -1053,6 +1055,7 @@ def test_should_redirect_after_request_to_go_live( volumes, displayed_volumes, formatted_displayed_volumes, + extra_tags, ): for channel, volume in volumes: mocker.patch( @@ -1076,6 +1079,7 @@ def test_should_redirect_after_request_to_go_live( tags=[ 'notify_request_to_go_live', 'notify_request_to_go_live_incomplete', + ] + extra_tags + [ 'notify_request_to_go_live_incomplete_checklist', 'notify_request_to_go_live_incomplete_mou', 'notify_request_to_go_live_incomplete_team_member', @@ -1119,6 +1123,9 @@ def test_should_redirect_after_request_to_go_live( 'has_email_reply_to_address,' 'shouldnt_use_govuk_as_sms_sender,' 'sms_sender_is_govuk,' + 'volume_email,' + 'volume_sms,' + 'volume_letter,' 'expected_readyness,' 'agreement_signed,' 'expected_tags,' @@ -1132,6 +1139,7 @@ def test_should_redirect_after_request_to_go_live( True, True, True, + 1, 1, 1, 'Yes', True, [ @@ -1147,6 +1155,7 @@ def test_should_redirect_after_request_to_go_live( False, True, True, + 1, 1, 1, 'No', True, [ @@ -1164,6 +1173,7 @@ def test_should_redirect_after_request_to_go_live( True, True, False, + 1, 1, 1, 'Yes', True, [ @@ -1179,6 +1189,7 @@ def test_should_redirect_after_request_to_go_live( True, True, True, + 1, 1, 1, 'No', True, [ @@ -1196,6 +1207,7 @@ def test_should_redirect_after_request_to_go_live( True, True, False, + 1, 1, 1, 'No', True, [ @@ -1213,6 +1225,7 @@ def test_should_redirect_after_request_to_go_live( True, True, False, + 1, 1, 1, 'No', True, [ @@ -1230,11 +1243,13 @@ def test_should_redirect_after_request_to_go_live( False, True, True, + 0, None, 0, 'No', False, [ 'notify_request_to_go_live', 'notify_request_to_go_live_incomplete', + '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', @@ -1255,6 +1270,9 @@ def test_ready_to_go_live( has_email_reply_to_address, shouldnt_use_govuk_as_sms_sender, sms_sender_is_govuk, + volume_email, + volume_sms, + volume_letter, expected_readyness, agreement_signed, expected_tags, @@ -1273,6 +1291,15 @@ def test_ready_to_go_live( new_callable=PropertyMock ).return_value = locals()[prop] + mocker.patch( + 'app.models.service.Service.__getattr__', + side_effect=lambda prop: { + 'volume_email': volume_email, + 'volume_sms': volume_sms, + 'volume_letter': volume_letter, + }.get(prop) + ) + assert app.models.service.Service({ 'id': SERVICE_ONE_ID }).go_live_checklist_completed_as_yes_no == expected_readyness