From 6d0d10e8decea8c365c1bb1a2b3a7e1b551eb5ca Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 12 Sep 2019 13:45:35 +0100 Subject: [PATCH] Only show relevant choices of email branding MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Users who work in local government can’t have GOV.UK branding on their emails. And only those working for Companies House (for example) can request the Companies House branding. This commit adds: - new choices of email branding, which offer the name of the branding, rather than the style - logic to filter this list to only the applicable options, based on what we know about the user, service and organisation This is a change from the previous approach which put the onus on users to figure out the style of branding they wanted, when we might already know that a lot of the options weren’t available to them, or would be inconsistent with the branding of other services in their organisation. --- app/main/forms.py | 52 +++- app/main/views/service_settings.py | 12 +- app/models/organisation.py | 1 + app/templates/views/service-settings.html | 2 +- .../branding/email-options.html | 23 +- tests/app/main/views/test_service_settings.py | 232 ++++++++++++++---- tests/conftest.py | 2 + 7 files changed, 235 insertions(+), 89 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index 27146b068..363e9d3be 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -1357,25 +1357,55 @@ class LinkOrganisationsForm(StripWhitespaceForm): ) -branding_options = ( - ('govuk', 'GOV.UK only'), - ('both', 'GOV.UK and logo'), - ('org', 'Your logo'), - ('org_banner', 'Your logo on a colour'), -) -branding_options_dict = dict(branding_options) - - class BrandingOptionsEmail(StripWhitespaceForm): options = RadioField( - 'Branding options', - choices=branding_options, + 'Choose your new email branding', validators=[ DataRequired() ], ) + def __init__(self, service, *args, **kwargs): + super().__init__(*args, **kwargs) + self.options.choices = tuple(self.get_available_choices(service)) + + @staticmethod + def get_available_choices(service): + + if ( + service.organisation_type == 'central' and + service.organisation.email_branding_id is None and + service.email_branding_id is not None + ): + yield ('govuk', 'GOV.UK') + + if ( + service.organisation_type == 'central' and + service.organisation and + service.organisation.email_branding_id is None and + service.email_branding_name.lower() != 'GOV.UK and {}'.format(service.organisation.name).lower() + ): + yield ('govuk_and_org', 'GOV.UK and {}'.format(service.organisation.name)) + + if ( + service.organisation_type in {'nhs_local', 'nhs_central', 'nhs_gp'} and + service.email_branding_name != 'NHS' + ): + yield ('nhs', 'NHS') + + if ( + service.organisation and + service.organisation_type not in {'nhs_local', 'nhs_central', 'nhs_gp'} and + ( + service.email_branding_id is None or + service.email_branding_id != service.organisation.email_branding_id + ) + ): + yield ('organisation', service.organisation.name) + + yield ('something_else', 'Something else') + class ServiceDataRetentionForm(StripWhitespaceForm): diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index 2a4733ff3..4036f6d74 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -52,7 +52,6 @@ from app.main.forms import ( SetEmailBranding, SetLetterBranding, SMSPrefixForm, - branding_options_dict, ) from app.utils import ( DELIVERED_STATUSES, @@ -1036,14 +1035,7 @@ def link_service_to_organisation(service_id): @user_has_permissions('manage_service') def branding_request(service_id): - branding_type = 'govuk' - - if current_service.email_branding: - branding_type = current_service.email_branding['brand_type'] - - form = BrandingOptionsEmail( - options=branding_type - ) + form = BrandingOptionsEmail(current_service) if form.validate_on_submit(): zendesk_client.create_ticket( @@ -1060,7 +1052,7 @@ def branding_request(service_id): service_name=current_service.name, dashboard_url=url_for('main.service_dashboard', service_id=current_service.id, _external=True), current_branding=current_service.email_branding_name, - branding_requested=branding_options_dict[form.options.data], + branding_requested=dict(form.options.choices)[form.options.data], ), ticket_type=zendesk_client.TYPE_QUESTION, user_email=current_user.email_address, diff --git a/app/models/organisation.py b/app/models/organisation.py index c5b631fbe..1d5fcb2e3 100644 --- a/app/models/organisation.py +++ b/app/models/organisation.py @@ -83,6 +83,7 @@ class Organisation(JSONModel): self.domains = [] self.organisation_type = None self.request_to_go_live_notes = None + self.email_branding_id = None def as_agreement_statement_for_go_live_request(self, fallback_domain): if self.agreement_signed: diff --git a/app/templates/views/service-settings.html b/app/templates/views/service-settings.html index 6f4febc85..205b7f251 100644 --- a/app/templates/views/service-settings.html +++ b/app/templates/views/service-settings.html @@ -102,7 +102,7 @@ {% call settings_row(if_has_permission='email') %} {{ text_field('Email branding') }} - {{ text_field('Your branding
({})'.format(current_service.email_branding_name)|safe if current_service.email_branding else current_service.email_branding_name) }} + {{ text_field(current_service.email_branding_name) }} {{ edit_field( 'Change', url_for('.branding_request', service_id=current_service.id), diff --git a/app/templates/views/service-settings/branding/email-options.html b/app/templates/views/service-settings/branding/email-options.html index 0b675b95b..81b65c764 100644 --- a/app/templates/views/service-settings/branding/email-options.html +++ b/app/templates/views/service-settings/branding/email-options.html @@ -1,5 +1,5 @@ {% extends "withnav_template.html" %} -{% from "components/radios.html" import radio %} +{% from "components/radios.html" import radios %} {% from "components/textbox.html" import textbox %} {% from "components/page-header.html" import page_header %} {% from "components/page-footer.html" import page_footer %} @@ -17,26 +17,7 @@ ) }} {% call form_wrapper() %} -
- - Choose the branding you’d like on your emails. - - {% if form.options.errors %} - - You need to choose an option - - {% endif %} -
- {% for option in form.options %} -
- - {{ radio(option) }} -
- {% endfor %} -
-
+ {{ radios(form.options) }}

We’ll email you once your branding’s ready to use, or if we need any more information. diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index 1aa3f9ff3..110826fe7 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -202,7 +202,7 @@ def test_organisation_name_links_to_org_dashboard( 'Label Value Action', 'Send emails On Change', 'Reply-to email addresses test@example.com Manage', - 'Email branding Your branding (Organisation name) Change', + 'Email branding Organisation name Change', 'Label Value Action', 'Send text messages On Change', @@ -223,7 +223,7 @@ def test_organisation_name_links_to_org_dashboard( 'Label Value Action', 'Send emails On Change', 'Reply-to email addresses test@example.com Manage', - 'Email branding Your branding (Organisation name) Change', + 'Email branding Organisation name Change', 'Label Value Action', 'Send text messages On Change', @@ -4377,63 +4377,203 @@ def test_update_service_organisation_does_not_update_if_same_value( mock_update_service_organisation.called is False +@pytest.mark.parametrize('organisation_type, expected_options', ( + ('central', [ + ('something_else', 'Something else'), + ]), + ('local', [ + ('something_else', 'Something else'), + ]), + ('nhs_central', [ + ('nhs', 'NHS'), + ('something_else', 'Something else'), + ]), + ('nhs_local', [ + ('nhs', 'NHS'), + ('something_else', 'Something else'), + ]), + ('nhs_gp', [ + ('nhs', 'NHS'), + ('something_else', 'Something else'), + ]), + ('emergency_service', [ + ('something_else', 'Something else'), + ]), + ('other', [ + ('something_else', 'Something else'), + ]), +)) def test_show_email_branding_request_page_when_no_email_branding_is_set( + mocker, + service_one, client_request, - mock_get_email_branding + mock_get_email_branding, + organisation_type, + expected_options, ): + service_one['email_branding'] = None + service_one['organisation_type'] = organisation_type + mocker.patch( + 'app.organisations_client.get_service_organisation', + return_value=None, + ) + page = client_request.get( '.branding_request', service_id=SERVICE_ONE_ID ) mock_get_email_branding.assert_not_called() - radios = page.select('input[type=radio]') - - for index, option in enumerate(( - 'govuk', - 'both', - 'org', - 'org_banner', - )): - assert radios[index]['name'] == 'options' - assert radios[index]['value'] == option + assert [ + ( + radio['value'], + page.select_one('label[for={}]'.format(radio['id'])).text.strip() + ) + for radio in page.select('input[type=radio]') + ] == expected_options -def test_show_email_branding_request_page_when_email_branding_is_set( +@pytest.mark.parametrize('organisation_type, expected_options', ( + ('central', [ + ('govuk_and_org', 'GOV.UK and Test Organisation'), + ('organisation', 'Test Organisation'), + ('something_else', 'Something else'), + ]), + ('local', [ + ('organisation', 'Test Organisation'), + ('something_else', 'Something else'), + ]), + ('nhs_central', [ + ('nhs', 'NHS'), + ('something_else', 'Something else'), + ]), + ('nhs_local', [ + ('nhs', 'NHS'), + ('something_else', 'Something else'), + ]), + ('nhs_gp', [ + ('nhs', 'NHS'), + ('something_else', 'Something else'), + ]), + ('emergency_service', [ + ('organisation', 'Test Organisation'), + ('something_else', 'Something else'), + ]), + ('other', [ + ('organisation', 'Test Organisation'), + ('something_else', 'Something else'), + ]), +)) +def test_show_email_branding_request_page_when_no_email_branding_is_set_but_organisation_exists( + mocker, + service_one, client_request, mock_get_email_branding, - active_user_with_permissions, + organisation_type, + expected_options, ): - - service_one = service_json(email_branding='1234') - client_request.login(active_user_with_permissions, service=service_one) + service_one['email_branding'] = None + service_one['organisation_type'] = organisation_type + mocker.patch( + 'app.organisations_client.get_service_organisation', + return_value=organisation_json(organisation_type=organisation_type), + ) page = client_request.get( '.branding_request', service_id=SERVICE_ONE_ID ) - mock_get_email_branding.called_once_with('1234') + mock_get_email_branding.assert_not_called() - radios = page.select('input[type=radio]') - - for index, option in enumerate(( - 'govuk', - 'both', - 'org', - 'org_banner', - )): - assert radios[index]['name'] == 'options' - assert radios[index]['value'] == option - if option == 'org': - assert 'checked' in radios[index].attrs + assert [ + ( + radio['value'], + page.select_one('label[for={}]'.format(radio['id'])).text.strip() + ) + for radio in page.select('input[type=radio]') + ] == expected_options -@pytest.mark.parametrize('choice, requested_branding', ( - ('govuk', 'GOV.UK only'), - ('both', 'GOV.UK and logo'), - ('org', 'Your logo'), - ('org_banner', 'Your logo on a colour'), - pytest.param('foo', 'Nope', marks=pytest.mark.xfail(raises=AssertionError)), +def test_show_email_branding_request_page_when_email_branding_is_set( + mocker, + service_one, + client_request, + mock_get_email_branding, + active_user_with_permissions, +): + service_one['email_branding'] = sample_uuid() + mocker.patch( + 'app.organisations_client.get_service_organisation', + return_value=organisation_json(), + ) + + page = client_request.get( + '.branding_request', service_id=SERVICE_ONE_ID + ) + assert [ + ( + radio['value'], + page.select_one('label[for={}]'.format(radio['id'])).text.strip() + ) + for radio in page.select('input[type=radio]') + ] == [ + ('govuk', 'GOV.UK'), + ('govuk_and_org', 'GOV.UK and Test Organisation'), + ('organisation', 'Test Organisation'), + ('something_else', 'Something else'), + ] + + +def test_show_email_branding_request_page_when_email_branding_is_same_as_org( + mocker, + service_one, + client_request, + mock_get_email_branding, + active_user_with_permissions, +): + service_one['email_branding'] = sample_uuid() + mocker.patch( + 'app.organisations_client.get_service_organisation', + return_value=organisation_json(email_branding_id=service_one['email_branding']), + ) + + page = client_request.get( + '.branding_request', service_id=SERVICE_ONE_ID + ) + + assert [ + ( + radio['value'], + page.select_one('label[for={}]'.format(radio['id'])).text.strip() + ) + for radio in page.select('input[type=radio]') + ] == [ + # Central government organisations who have their own default + # branding will do so because they’re exempt from GOV.UK. + # We also don’t show their organisation’s branding because they + # have it already. + ('something_else', 'Something else'), + ] + + +@pytest.mark.parametrize('data, requested_branding', ( + ( + { + 'options': 'govuk', + }, + 'GOV.UK', + ), + ( + { + 'options': 'something_else', + }, + 'Something else' + ), + pytest.param( + {'options': 'foo'}, + 'Nope', + marks=pytest.mark.xfail(raises=AssertionError), + ), )) @pytest.mark.parametrize('org_name, expected_organisation', ( (None, 'Can’t tell (domain is user.gov.uk)'), @@ -4441,20 +4581,22 @@ def test_show_email_branding_request_page_when_email_branding_is_set( )) def test_submit_email_branding_request( client_request, + service_one, mocker, - choice, + data, requested_branding, mock_get_service_settings_page_common, + mock_get_email_branding, no_reply_to_email_addresses, no_letter_contact_blocks, single_sms_sender, org_name, expected_organisation, ): - - mock_get_service_organisation( - mocker, - name=org_name, + service_one['email_branding'] = sample_uuid() + mocker.patch( + 'app.organisations_client.get_service_organisation', + return_value=organisation_json(name=org_name) if org_name else None, ) zendesk = mocker.patch( @@ -4464,9 +4606,7 @@ def test_submit_email_branding_request( page = client_request.post( '.branding_request', service_id=SERVICE_ONE_ID, - _data={ - 'options': choice, - }, + _data=data, _follow_redirects=True, ) @@ -4477,7 +4617,7 @@ def test_submit_email_branding_request( 'http://localhost/services/596364a0-858e-42c8-9062-a8fe822260eb', '', '---', - 'Current branding: GOV.UK', + 'Current branding: Organisation name', 'Branding requested: {}', ]).format(expected_organisation, requested_branding), subject='Email branding request - service one', diff --git a/tests/conftest.py b/tests/conftest.py index dc89436b7..2b4f3e93b 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -3208,6 +3208,7 @@ def mock_get_service_organisation( name=False, crown=True, agreement_signed=None, + organisation_type=None, ): def _get_service_organisation(service_id): return organisation_json( @@ -3215,6 +3216,7 @@ def mock_get_service_organisation( name, crown=crown, agreement_signed=agreement_signed, + organisation_type=organisation_type, ) return mocker.patch('app.organisations_client.get_service_organisation', side_effect=_get_service_organisation)