From 3b15b04fc2a7da4a6e84aee1855686ebbe5df5bc Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 29 Jan 2019 14:51:31 +0000 Subject: [PATCH] Let users switch channels on and off by themselves We let people do this for letters already. We should let them do it for emails and texts too, rather than have to email us. --- app/main/forms.py | 12 ++- app/main/views/service_settings.py | 48 +++++----- app/navigation.py | 12 +-- app/templates/views/service-settings.html | 42 +++++---- .../views/service-settings/set-email.html | 39 ++++----- .../{set-letters.html => set-letter.html} | 0 .../views/service-settings/set-sms.html | 48 +++++----- tests/app/main/views/test_service_settings.py | 87 +++++++++++++++++-- 8 files changed, 183 insertions(+), 105 deletions(-) rename app/templates/views/service-settings/{set-letters.html => set-letter.html} (100%) diff --git a/app/main/forms.py b/app/main/forms.py index f6c27ec75..e79c4d538 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -696,10 +696,18 @@ class ServiceLetterContactBlockForm(StripWhitespaceForm): ) -class ServiceSwitchLettersForm(StripWhitespaceForm): +class ServiceSwitchChannelForm(StripWhitespaceForm): + + def __init__(self, channel, *args, **kwargs): + super().__init__(*args, **kwargs) + self.enabled.label.text = 'Send {}'.format({ + 'email': 'emails', + 'sms': 'text messages', + 'letter': 'letters', + }.get(channel)) enabled = RadioField( - 'Send letters', + 'Choices', choices=[ ('on', 'On'), ('off', 'Off'), diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index fcef0ae12..99146fbc4 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -48,7 +48,7 @@ from app.main.forms import ( ServiceReplyToEmailForm, ServiceSetBranding, ServiceSmsSenderForm, - ServiceSwitchLettersForm, + ServiceSwitchChannelForm, SMSPrefixForm, branding_options_dict, ) @@ -368,15 +368,6 @@ def service_set_contact_link(service_id): return render_template('views/service-settings/contact_link.html', form=form) -@main.route("/services//service-settings/set-email", methods=['GET']) -@login_required -@user_has_permissions('manage_service') -def service_set_email(service_id): - return render_template( - 'views/service-settings/set-email.html', - ) - - @main.route("/services//service-settings/set-reply-to-email", methods=['GET']) @login_required @user_has_permissions('manage_service') @@ -484,15 +475,6 @@ def service_set_inbound_number(service_id): ) -@main.route("/services//service-settings/set-sms", methods=['GET']) -@login_required -@user_has_permissions('manage_service') -def service_set_sms(service_id): - return render_template( - 'views/service-settings/set-sms.html', - ) - - @main.route("/services//service-settings/sms-prefix", methods=['GET', 'POST']) @login_required @user_has_permissions('manage_service') @@ -546,23 +528,41 @@ def service_set_inbound_sms(service_id): ) -@main.route("/services//service-settings/set-letters", methods=['GET', 'POST']) +@main.route("/services//service-settings/set-letters", methods=['GET']) @login_required @user_has_permissions('manage_service') def service_set_letters(service_id): - form = ServiceSwitchLettersForm( - enabled='on' if current_service.has_permission('letter') else 'off' + return redirect(url_for( + '.service_set_channel', + service_id=current_service.id, + channel='letter', + )) + + +@main.route("/services//service-settings/set-", methods=['GET', 'POST']) +@login_required +@user_has_permissions('manage_service') +def service_set_channel(service_id, channel): + + if channel not in {'email', 'sms', 'letter'}: + abort(404) + + form = ServiceSwitchChannelForm( + channel=channel, + enabled='on' if current_service.has_permission(channel) else 'off' ) + if form.validate_on_submit(): current_service.force_permission( - 'letter', + channel, on=(form.enabled.data == 'on'), ) return redirect( url_for(".service_settings", service_id=service_id) ) + return render_template( - 'views/service-settings/set-letters.html', + 'views/service-settings/set-{}.html'.format(channel), form=form, ) diff --git a/app/navigation.py b/app/navigation.py index 8cf8c514b..c207e98c3 100644 --- a/app/navigation.py +++ b/app/navigation.py @@ -227,8 +227,8 @@ class HeaderNavigation(Navigation): 'service_name_change_confirm', 'service_preview_email_branding', 'service_set_auth_type', + 'service_set_channel', 'service_set_contact_link', - 'service_set_email', 'service_set_email_branding', 'service_set_inbound_number', 'service_set_inbound_sms', @@ -237,7 +237,6 @@ class HeaderNavigation(Navigation): 'service_set_letters', 'service_set_postage', 'service_set_reply_to_email', - 'service_set_sms', 'service_set_sms_prefix', 'service_settings', 'service_sms_senders', @@ -354,8 +353,8 @@ class MainNavigation(Navigation): 'service_name_change_confirm', 'service_preview_email_branding', 'service_set_auth_type', + 'service_set_channel', 'service_set_contact_link', - 'service_set_email', 'service_set_email_branding', 'service_set_inbound_number', 'service_set_inbound_sms', @@ -364,7 +363,6 @@ class MainNavigation(Navigation): 'service_set_letters', 'service_set_postage', 'service_set_reply_to_email', - 'service_set_sms', 'service_set_sms_prefix', 'service_settings', 'service_sms_senders', @@ -690,8 +688,8 @@ class CaseworkNavigation(Navigation): 'service_name_change_confirm', 'service_preview_email_branding', 'service_set_auth_type', + 'service_set_channel', 'service_set_contact_link', - 'service_set_email', 'service_set_email_branding', 'service_set_inbound_number', 'service_set_inbound_sms', @@ -700,7 +698,6 @@ class CaseworkNavigation(Navigation): 'service_set_letters', 'service_set_postage', 'service_set_reply_to_email', - 'service_set_sms', 'service_set_sms_prefix', 'service_settings', 'service_sms_senders', @@ -926,8 +923,8 @@ class OrgNavigation(Navigation): 'service_name_change_confirm', 'service_preview_email_branding', 'service_set_auth_type', + 'service_set_channel', 'service_set_contact_link', - 'service_set_email', 'service_set_email_branding', 'service_set_inbound_number', 'service_set_inbound_sms', @@ -936,7 +933,6 @@ class OrgNavigation(Navigation): 'service_set_letters', 'service_set_postage', 'service_set_reply_to_email', - 'service_set_sms', 'service_set_sms_prefix', 'service_settings', 'service_sms_senders', diff --git a/app/templates/views/service-settings.html b/app/templates/views/service-settings.html index 15e6ddb4b..0fb66cca6 100644 --- a/app/templates/views/service-settings.html +++ b/app/templates/views/service-settings.html @@ -71,12 +71,14 @@ {{ text_field('Send emails') }} {{ boolean_field('email' in current_service.permissions) }} {{ edit_field( - 'Change', - url_for('.service_set_email', - service_id=current_service.id), - permissions=['manage_service'] - ) - }} + 'Change', + url_for( + '.service_set_channel', + channel='email', + service_id=current_service.id + ), + permissions=['manage_service'] + )}} {% endcall %} {% call settings_row(if_has_permission='email') %} @@ -121,12 +123,14 @@ {{ text_field('Send text messages') }} {{ boolean_field('sms' in current_service.permissions) }} {{ edit_field( - 'Change', - url_for('.service_set_sms', - service_id=current_service.id), - permissions=['manage_service'] - ) - }} + 'Change', + url_for( + '.service_set_channel', + service_id=current_service.id, + channel='sms' + ), + permissions=['manage_service'] + )}} {% endcall %} {% call settings_row(if_has_permission='sms') %} @@ -197,12 +201,14 @@ {{ text_field('Send letters') }} {{ boolean_field('letter' in current_service.permissions) }} {{ edit_field( - 'Change', - url_for('.service_set_letters', - service_id=current_service.id), - permissions=['manage_service'] - ) - }} + 'Change', + url_for( + '.service_set_channel', + channel='letter', + service_id=current_service.id + ), + permissions=['manage_service'] + )}} {% endcall %} {% call settings_row(if_has_permission='letter') %} diff --git a/app/templates/views/service-settings/set-email.html b/app/templates/views/service-settings/set-email.html index 8ad86c3fb..e3bbfe4b9 100644 --- a/app/templates/views/service-settings/set-email.html +++ b/app/templates/views/service-settings/set-email.html @@ -1,37 +1,28 @@ {% extends "withnav_template.html" %} -{% from "components/textbox.html" import textbox %} +{% from "components/form.html" import form_wrapper %} {% from "components/page-footer.html" import page_footer %} +{% from "components/radios.html" import radios %} {% block service_page_title %} - Emails + Send emails {% endblock %} {% block maincolumn_content %}
-

Emails

- {% if 'email' in current_service.permissions %} -

- Your service can send emails. -

-

- If you want to turn it off, - get in touch with the GOV.UK Notify team. -

- {% else %} -

- Sending emails is an invitation‑only feature. -

-

- If you want to try it out, - get in touch with the GOV.UK Notify team. -

- {% endif %} - {{ page_footer( - back_link=url_for('.service_settings', service_id=current_service.id), - back_link_text='Back to settings' - ) }} +

Send emails

+

+ It’s free to send emails through GOV.UK Notify. +

+ {% call form_wrapper() %} + {{ radios(form.enabled) }} + {{ page_footer( + 'Save', + back_link=url_for('.service_settings', service_id=current_service.id), + back_link_text='Back to settings' + ) }} + {% endcall %}
diff --git a/app/templates/views/service-settings/set-letters.html b/app/templates/views/service-settings/set-letter.html similarity index 100% rename from app/templates/views/service-settings/set-letters.html rename to app/templates/views/service-settings/set-letter.html diff --git a/app/templates/views/service-settings/set-sms.html b/app/templates/views/service-settings/set-sms.html index 61df15af0..8139013a9 100644 --- a/app/templates/views/service-settings/set-sms.html +++ b/app/templates/views/service-settings/set-sms.html @@ -1,37 +1,37 @@ {% extends "withnav_template.html" %} -{% from "components/textbox.html" import textbox %} +{% from "components/form.html" import form_wrapper %} {% from "components/page-footer.html" import page_footer %} +{% from "components/radios.html" import radios %} {% block service_page_title %} - Text messages + Send text messages {% endblock %} {% block maincolumn_content %}
-

Text messages

- {% if 'sms' in current_service.permissions %} -

- Your service can send text messages. -

-

- If you want to turn it off, - get in touch with the GOV.UK Notify team. -

- {% else %} -

- Sending text messages is an invitation‑only feature. -

-

- If you want to try it out, - get in touch with the GOV.UK Notify team. -

- {% endif %} - {{ page_footer( - back_link=url_for('.service_settings', service_id=current_service.id), - back_link_text='Back to settings' - ) }} +

Send text messages

+

+ You have a free allowance of + {{ '{:,}'.format(current_service.free_sms_fragment_limit) }} text messages each + financial year. +

+

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

+

+ See pricing for more details. +

+ {% call form_wrapper() %} + {{ radios(form.enabled) }} + {{ page_footer( + 'Save', + back_link=url_for('.service_settings', service_id=current_service.id), + back_link_text='Back to settings' + ) }} + {% endcall %}
diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index 123606001..e55432f75 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -2542,19 +2542,91 @@ def test_should_set_sms_allowance( ) +def test_old_set_letters_page_redirects( + client_request, +): + client_request.get( + 'main.service_set_letters', + service_id=SERVICE_ONE_ID, + _expected_status=302, + _expected_redirect=url_for( + 'main.service_set_channel', + service_id=SERVICE_ONE_ID, + channel='letter', + _external=True, + ) + ) + + +def test_unknown_channel_404s( + client_request, +): + client_request.get( + 'main.service_set_channel', + service_id=SERVICE_ONE_ID, + channel='message-in-a-bottle', + _expected_status=404, + ) + + @pytest.mark.parametrize(( + 'channel,' + 'expected_first_para,' + 'expected_legend,' + 'initial_permissions,' 'expected_initial_value,' 'posted_value,' - 'initial_permissions,' 'expected_updated_permissions' ), [ - ('off', 'on', ['email', 'sms'], ['email', 'sms', 'letter']), - ('on', 'off', ['email', 'sms', 'letter'], ['email', 'sms']), + ( + 'letter', + 'It costs between 30p and 76p to send a letter using Notify.', + 'Send letters', + ['email', 'sms'], + 'off', 'on', + ['email', 'sms', 'letter'], + ), + ( + 'letter', + 'It costs between 30p and 76p to send a letter using Notify.', + 'Send letters', + ['email', 'sms', 'letter'], + 'on', 'off', + ['email', 'sms'], + ), + ( + 'sms', + 'You have a free allowance of 250,000 text messages each financial year.', + 'Send text messages', + [], + 'off', 'on', + ['sms'], + ), + ( + 'email', + 'It’s free to send emails through GOV.UK Notify.', + 'Send emails', + [], + 'off', 'on', + ['email'], + ), + ( + 'email', + 'It’s free to send emails through GOV.UK Notify.', + 'Send emails', + ['email', 'sms', 'letter'], + 'on', 'on', + ['email', 'sms', 'letter'], + ), ]) def test_switch_service_enable_letters( client_request, service_one, mocker, + mock_get_free_sms_fragment_limit, + channel, + expected_first_para, + expected_legend, expected_initial_value, posted_value, initial_permissions, @@ -2564,16 +2636,21 @@ def test_switch_service_enable_letters( service_one['permissions'] = initial_permissions page = client_request.get( - 'main.service_set_letters', + 'main.service_set_channel', service_id=service_one['id'], + channel=channel, ) + assert normalize_spaces(page.select_one('main p').text) == expected_first_para + assert normalize_spaces(page.select_one('legend').text) == expected_legend + assert page.select_one('input[checked]')['value'] == expected_initial_value assert len(page.select('input[checked]')) == 1 client_request.post( - 'main.service_set_letters', + 'main.service_set_channel', service_id=service_one['id'], + channel=channel, _data={'enabled': posted_value}, _expected_redirect=url_for( 'main.service_settings',