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)