From ca3fdfd90702b17044204c00d9b1785a9b47d58c Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 27 Feb 2018 11:33:26 +0000 Subject: [PATCH 1/5] Check for team members on request to go live page One of the things that we want to check before a service goes live is that they have at least two team members with the manage service permission. Anyone who can make a request to go live has this permission, so that means one additional user is needed. This is what we can automatically communicate to the user. Under the hood this makes use of the logic added in https://github.com/alphagov/notifications-admin/pull/1891 --- app/main/views/service_settings.py | 9 ++++++++- app/templates/components/tick-cross.html | 11 ++++++++--- .../service-settings/request-to-go-live.html | 19 +++++++++++++------ tests/app/main/views/test_service_settings.py | 13 +++++++++++++ 4 files changed, 42 insertions(+), 10 deletions(-) diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index 10fb6412a..fcc845654 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -162,7 +162,14 @@ def service_name_change_confirm(service_id): @login_required @user_has_permissions('manage_settings', admin_override=True) def request_to_go_live(service_id): - return render_template('views/service-settings/request-to-go-live.html') + return render_template( + 'views/service-settings/request-to-go-live.html', + has_team_members=( + user_api_client.get_count_of_users_with_permission( + service_id, 'manage_settings' + ) > 1 + ), + ) @main.route("/services//service-settings/submit-request-to-go-live", methods=['GET', 'POST']) diff --git a/app/templates/components/tick-cross.html b/app/templates/components/tick-cross.html index 6467b38ca..f6fbc9ecf 100644 --- a/app/templates/components/tick-cross.html +++ b/app/templates/components/tick-cross.html @@ -1,15 +1,20 @@ -{% macro tick_cross(yes, label) %} +{% macro tick_cross(yes, label, truthy_hint='Can', falsey_hint='Can’t') %}
  • {% if yes %} - + {{ label}} {% else %} - + {{ label}} {% endif %}
    • {% endmacro %} + + +{% macro tick_cross_done_not_done(yes, label) %} + {{ tick_cross(yes, label, truthy_hint='Done: ', falsey_hint='Not done: ') }} +{% endmacro %} 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 ed8628485..65b1b6ac8 100644 --- a/app/templates/views/service-settings/request-to-go-live.html +++ b/app/templates/views/service-settings/request-to-go-live.html @@ -4,6 +4,7 @@ {% from "components/radios.html" import radios %} {% from "components/page-footer.html" import page_footer %} {% from "components/banner.html" import banner_wrapper %} +{% from "components/tick-cross.html" import tick_cross_done_not_done %} {% block service_page_title %} Request to go live @@ -14,21 +15,27 @@

    Request to go live

    - Before you request to go live, make sure you’ve: + Before you request to go live, make sure that: +

    +
      + {{ tick_cross_done_not_done( + has_team_members, + 'Another person in your team has the ‘Manage service’ permission', + ) }} +
    +

    + You also need to:

    • read our terms of use
    • - added team members to your account -
      • -
    • - specified your reply to email address or text message sender in your + specify your reply to email address or text message sender in your settings page
    • - added the templates you want to start with, making sure they follow the GOV.UK Service Manual standards for + add the templates you want to start with, making sure they follow the GOV.UK Service Manual standards for writing text messages and emails
    diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index e7e945a87..ee0e80387 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -455,17 +455,30 @@ def test_should_raise_duplicate_name_handled( assert mock_verify_password.called +@pytest.mark.parametrize('count_of_users_with_manage_service, expected_checklist_item', [ + (1, 'Not done: Another person in your team has the ‘Manage service’ permission'), + (2, 'Done: Another person in your team has the ‘Manage service’ permission'), +]) def test_should_show_request_to_go_live_checklist( client_request, + mocker, + count_of_users_with_manage_service, + expected_checklist_item, ): + 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 + ) page = client_request.get( 'main.request_to_go_live', service_id=SERVICE_ONE_ID ) assert page.h1.text == 'Request to go live' + assert normalize_spaces(page.select('main ul li')[0].text) == expected_checklist_item assert page.select_one('main .button')['href'] == url_for( 'main.submit_request_to_go_live', service_id=SERVICE_ONE_ID, ) + mock_count_users.assert_called_once_with(SERVICE_ONE_ID, 'manage_settings') def test_should_show_request_to_go_live( From 7be08e2f741ade2d147a73b129fee85da5a5cbe5 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 27 Feb 2018 12:07:50 +0000 Subject: [PATCH 2/5] Check for templates before going live We need users to have created some templates before they go live, so we can see what kind of messages they intend to send. We can do this automatically based on the work done in https://github.com/alphagov/notifications-admin/pull/1892 --- app/main/views/service_settings.py | 3 +++ .../service-settings/request-to-go-live.html | 6 ++++- tests/app/main/views/test_service_settings.py | 25 ++++++++++++++++--- 3 files changed, 30 insertions(+), 4 deletions(-) diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index fcc845654..60e6f1529 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -169,6 +169,9 @@ def request_to_go_live(service_id): service_id, 'manage_settings' ) > 1 ), + has_templates=( + service_api_client.count_service_templates(service_id) > 0 + ), ) 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 65b1b6ac8..0670015cd 100644 --- a/app/templates/views/service-settings/request-to-go-live.html +++ b/app/templates/views/service-settings/request-to-go-live.html @@ -22,6 +22,10 @@ has_team_members, 'Another person in your team has the ‘Manage service’ permission', ) }} + {{ tick_cross_done_not_done( + has_templates, + 'You’ve added some templates', + ) }}

    You also need to: @@ -35,7 +39,7 @@ settings page

  • - add the templates you want to start with, making sure they follow the GOV.UK Service Manual standards for + make sure your messages follow the GOV.UK Service Manual standards for writing text messages and emails
    • diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index ee0e80387..e1730de70 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -455,30 +455,46 @@ def test_should_raise_duplicate_name_handled( assert mock_verify_password.called -@pytest.mark.parametrize('count_of_users_with_manage_service, expected_checklist_item', [ +@pytest.mark.parametrize('count_of_users_with_manage_service, expected_user_checklist_item', [ (1, 'Not done: Another person in your team has the ‘Manage service’ permission'), (2, 'Done: Another person in your team has the ‘Manage service’ permission'), ]) +@pytest.mark.parametrize('count_of_templates, expected_templates_checklist_item', [ + (0, 'Not done: You’ve added some templates'), + (1, 'Done: You’ve added some templates'), + (2, 'Done: You’ve added some templates'), +]) def test_should_show_request_to_go_live_checklist( client_request, mocker, count_of_users_with_manage_service, - expected_checklist_item, + expected_user_checklist_item, + count_of_templates, + expected_templates_checklist_item, ): 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', + return_value=count_of_templates + ) + page = client_request.get( 'main.request_to_go_live', service_id=SERVICE_ONE_ID ) assert page.h1.text == 'Request to go live' - assert normalize_spaces(page.select('main ul li')[0].text) == expected_checklist_item + + assert normalize_spaces(page.select('main ul li')[0].text) == expected_user_checklist_item + assert normalize_spaces(page.select('main ul li')[1].text) == expected_templates_checklist_item + assert page.select_one('main .button')['href'] == url_for( 'main.submit_request_to_go_live', service_id=SERVICE_ONE_ID, ) mock_count_users.assert_called_once_with(SERVICE_ONE_ID, 'manage_settings') + mock_count_templates.assert_called_once_with(SERVICE_ONE_ID) def test_should_show_request_to_go_live( @@ -577,6 +593,7 @@ def test_route_permissions( single_sms_sender, route, mock_get_service_settings_page_common, + mock_get_service_templates, ): validate_route_permission( mocker, @@ -606,6 +623,7 @@ def test_route_invalid_permissions( api_user_active, service_one, route, + mock_get_service_templates, ): validate_route_permission( mocker, @@ -637,6 +655,7 @@ def test_route_for_platform_admin( single_sms_sender, route, mock_get_service_settings_page_common, + mock_get_service_templates, ): validate_route_permission(mocker, app_, From d0ffff9e028dd076aeb1c03bfcca56bbb1e5285e Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 27 Feb 2018 14:25:20 +0000 Subject: [PATCH 3/5] Check for reply to email address before going live MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We require that a user has a real reply-to email address before going live. We can partially automate this by at least telling users who haven’t done this. This only applies for users that have email templates; we shouldn’t bother users who aren’t going to send emails about this. --- app/main/views/service_settings.py | 6 +++ .../service-settings/request-to-go-live.html | 12 ++++-- tests/app/main/views/test_service_settings.py | 37 +++++++++++++++++-- 3 files changed, 47 insertions(+), 8 deletions(-) diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index 60e6f1529..edd32ba15 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -172,6 +172,12 @@ def request_to_go_live(service_id): has_templates=( service_api_client.count_service_templates(service_id) > 0 ), + has_email_templates=( + service_api_client.count_service_templates(service_id, template_type='email') > 0 + ), + has_email_reply_to_address=bool( + service_api_client.get_reply_to_email_addresses(service_id) + ) ) 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 0670015cd..16446d9fb 100644 --- a/app/templates/views/service-settings/request-to-go-live.html +++ b/app/templates/views/service-settings/request-to-go-live.html @@ -26,6 +26,14 @@ has_templates, 'You’ve added some templates', ) }} + {% if has_email_templates %} + {{ tick_cross_done_not_done( + has_email_reply_to_address, + 'You’ve added an email reply to address on the settings page'.format( + url_for('main.service_settings', service_id=current_service.id) + )|safe, + ) }} + {% endif %}

    You also need to: @@ -34,10 +42,6 @@

  • read our terms of use
    • -
  • - specify your reply to email address or text message sender in your - settings page -
  • make sure your messages follow the GOV.UK Service Manual standards for writing text messages and emails diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index e1730de70..b9f819bef 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -464,6 +464,12 @@ def test_should_raise_duplicate_name_handled( (1, 'Done: You’ve added some templates'), (2, 'Done: You’ve added some templates'), ]) +@pytest.mark.parametrize('count_of_email_templates, reply_to_email_addresses, expected_reply_to_checklist_item', [ + pytest.mark.xfail((0, [], ''), raises=IndexError), + pytest.mark.xfail((0, [{}], ''), raises=IndexError), + (1, [], 'Not done: You’ve added an email reply to address on the settings page'), + (1, [{}], 'Done: You’ve added an email reply to address on the settings page'), +]) def test_should_show_request_to_go_live_checklist( client_request, mocker, @@ -471,14 +477,27 @@ def test_should_show_request_to_go_live_checklist( expected_user_checklist_item, count_of_templates, expected_templates_checklist_item, + count_of_email_templates, + reply_to_email_addresses, + expected_reply_to_checklist_item, ): + + def _count_templates(service_id, template_type=None): + return { + 'email': count_of_email_templates + }.get(template_type, count_of_templates) + 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', - return_value=count_of_templates + side_effect=_count_templates + ) + 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 ) page = client_request.get( @@ -486,15 +505,25 @@ def test_should_show_request_to_go_live_checklist( ) assert page.h1.text == 'Request to go live' - assert normalize_spaces(page.select('main ul li')[0].text) == expected_user_checklist_item - assert normalize_spaces(page.select('main ul li')[1].text) == expected_templates_checklist_item + checklist_items = page.select('main ul[class=bottom-gutter] li') + + assert normalize_spaces(checklist_items[0].text) == expected_user_checklist_item + assert normalize_spaces(checklist_items[1].text) == expected_templates_checklist_item + assert normalize_spaces(checklist_items[2].text) == expected_reply_to_checklist_item assert page.select_one('main .button')['href'] == url_for( 'main.submit_request_to_go_live', service_id=SERVICE_ONE_ID, ) + mock_count_users.assert_called_once_with(SERVICE_ONE_ID, 'manage_settings') - mock_count_templates.assert_called_once_with(SERVICE_ONE_ID) + assert mock_count_templates.call_args_list == [ + call(SERVICE_ONE_ID), + call(SERVICE_ONE_ID, template_type='email'), + ] + + if count_of_email_templates: + mock_get_reply_to_email_addresses.assert_called_once_with(SERVICE_ONE_ID) def test_should_show_request_to_go_live( From d90cdc1f819f5809d069b3eb957def1c2d5d9b95 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 6 Mar 2018 13:51:50 +0000 Subject: [PATCH 4/5] =?UTF-8?q?Don=E2=80=99t=20change=20colour=20of=20sent?= =?UTF-8?q?=20date=20for=20letters?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit For text messages/emails it makes sense for ‘sending’ to be gray and ‘delivered’ to be black. But since we don’t show sending/delivered for letters it doesn’t make sense for the text to change colour. --- app/__init__.py | 35 +++++++++++++------ app/templates/components/table.html | 5 ++- .../partials/notifications/status.html | 2 +- 3 files changed, 30 insertions(+), 12 deletions(-) diff --git a/app/__init__.py b/app/__init__.py index c9d2242f6..cd58b8c3a 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -356,17 +356,32 @@ def format_notification_status_as_time(status, created, updated): }.get(status, updated) -def format_notification_status_as_field_status(status): +def format_notification_status_as_field_status(status, notification_type): return { - 'failed': 'error', - 'technical-failure': 'error', - 'temporary-failure': 'error', - 'permanent-failure': 'error', - 'delivered': None, - 'sent': None, - 'sending': 'default', - 'created': 'default' - }.get(status, 'error') + 'letter': { + 'failed': 'error', + 'technical-failure': 'error', + 'temporary-failure': 'error', + 'permanent-failure': 'error', + 'delivered': None, + 'sent': None, + 'sending': None, + 'created': None, + 'accepted': None, + } + }.get( + notification_type, + { + 'failed': 'error', + 'technical-failure': 'error', + 'temporary-failure': 'error', + 'permanent-failure': 'error', + 'delivered': None, + 'sent': None, + 'sending': 'default', + 'created': 'default' + } + ).get(status, 'error') def format_notification_status_as_url(status): diff --git a/app/templates/components/table.html b/app/templates/components/table.html index 380d595d0..49b554120 100644 --- a/app/templates/components/table.html +++ b/app/templates/components/table.html @@ -130,7 +130,10 @@ {% if not notification %} {% call field(align='right') %}{% endcall %} {% else %} - {% call field(status=notification.status|format_notification_status_as_field_status, align='right') %} + {% call field( + status=notification.status|format_notification_status_as_field_status(notification.notification_type), + align='right' + ) %} {% if notification.status in ['created', 'sending', 'delivered'] %}{% endif %} {% if notification.status|format_notification_status_as_url %} diff --git a/app/templates/partials/notifications/status.html b/app/templates/partials/notifications/status.html index 730eb6954..780445799 100644 --- a/app/templates/partials/notifications/status.html +++ b/app/templates/partials/notifications/status.html @@ -1,5 +1,5 @@
    -

    +

    {% if notification.status|format_notification_status_as_url %} {% endif %} From 99ff30d3b3646f56098cbd4347cf91590b8ff7cd Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 8 Mar 2018 10:30:57 +0000 Subject: [PATCH 5/5] Fix typo --- app/templates/views/pricing.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/templates/views/pricing.html b/app/templates/views/pricing.html index 6cdb834c4..1dc619705 100644 --- a/app/templates/views/pricing.html +++ b/app/templates/views/pricing.html @@ -27,7 +27,7 @@

    You have a free allowance of text messages each financial year. You’ll get:

    • 250,000 free text messages for central government services
      • -
    • 25,000 free text messages for other public sector service
      • +
    • 25,000 free text messages for other public sector services

    It costs 1.58 pence (plus VAT) for each text message you send after your free allowance.