diff --git a/app/main/forms.py b/app/main/forms.py index 940f730e9..28d9ec46c 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -1392,35 +1392,43 @@ class LinkOrganisationsForm(StripWhitespaceForm): ) -class BrandingOptionsEmail(StripWhitespaceForm): +class BrandingOptions(StripWhitespaceForm): FALLBACK_OPTION_VALUE = 'something_else' FALLBACK_OPTION = (FALLBACK_OPTION_VALUE, 'Something else') - options = RadioField('Choose your new email branding') + options = RadioField('Choose your new branding') something_else = TextAreaField('Describe the branding you want') - def __init__(self, service, *args, **kwargs): + def __init__(self, service, *args, branding_type="email", **kwargs): super().__init__(*args, **kwargs) - self.options.choices = tuple(self.get_available_choices(service)) + self.options.choices = tuple(self.get_available_choices(service, branding_type)) if self.something_else_is_only_option: self.options.data = self.FALLBACK_OPTION_VALUE @staticmethod - def get_available_choices(service): + def get_available_choices(service, branding_type): + if branding_type == "email": + organisation_branding_id = service.organisation.email_branding_id if service.organisation else None + service_branding_id = service.email_branding_id + service_branding_name = service.email_branding_name + elif branding_type == "letter": + organisation_branding_id = service.organisation.letter_branding_id if service.organisation else None + service_branding_id = service.letter_branding_id + service_branding_name = service.letter_branding_name if ( service.organisation_type == Organisation.TYPE_CENTRAL and - service.organisation.email_branding_id is None and - service.email_branding_id is not None + organisation_branding_id is None and + service_branding_id is not None ): yield ('govuk', 'GOV.UK') if ( service.organisation_type == 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() + organisation_branding_id is None and + service_branding_name.lower() != 'GOV.UK and {}'.format(service.organisation.name).lower() ): yield ('govuk_and_org', 'GOV.UK and {}'.format(service.organisation.name)) @@ -1429,7 +1437,7 @@ class BrandingOptionsEmail(StripWhitespaceForm): Organisation.TYPE_NHS_CENTRAL, Organisation.TYPE_NHS_LOCAL, Organisation.TYPE_NHS_GP, - } and service.email_branding_name != 'NHS' + } and service_branding_name != 'NHS' ): yield ('nhs', 'NHS') @@ -1440,13 +1448,13 @@ class BrandingOptionsEmail(StripWhitespaceForm): Organisation.TYPE_NHS_CENTRAL, Organisation.TYPE_NHS_GP, } and ( - service.email_branding_id is None or - service.email_branding_id != service.organisation.email_branding_id + service_branding_id is None or + service_branding_id != organisation_branding_id ) ): yield ('organisation', service.organisation.name) - yield BrandingOptionsEmail.FALLBACK_OPTION + yield BrandingOptions.FALLBACK_OPTION @property def something_else_is_only_option(self): diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index 79447ebe5..1237aba10 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -30,7 +30,7 @@ from app import ( from app.extensions import zendesk_client from app.main import main from app.main.forms import ( - BrandingOptionsEmail, + BrandingOptions, ConfirmPasswordForm, EstimateUsageForm, FreeSMSAllowance, @@ -1038,15 +1038,18 @@ def link_service_to_organisation(service_id): ) -@main.route("/services//branding-request/email", methods=['GET', 'POST']) +@main.route("/services//branding-request/", methods=['GET', 'POST']) @user_has_permissions('manage_service') -def branding_request(service_id): - - form = BrandingOptionsEmail(current_service) +def branding_request(service_id, branding_type): + form = BrandingOptions(current_service, branding_type=branding_type) + if branding_type == "email": + branding_name = current_service.email_branding_name + elif branding_type == "letter": + branding_name = current_service.letter_branding_name if form.validate_on_submit(): zendesk_client.create_ticket( - subject='Email branding request - {}'.format(current_service.name), + subject='{} branding request - {}'.format(branding_type.capitalize(), current_service.name), message=( 'Organisation: {organisation}\n' 'Service: {service_name}\n' @@ -1061,7 +1064,7 @@ def branding_request(service_id): organisation=current_service.organisation.as_info_for_branding_request(current_user.email_domain), 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, + current_branding=branding_name, branding_requested=dict(form.options.choices)[form.options.data], new_paragraph='\n\n' if form.something_else.data else '', detail=form.something_else.data or '' @@ -1079,8 +1082,10 @@ def branding_request(service_id): return redirect(url_for('.service_settings', service_id=service_id)) return render_template( - 'views/service-settings/branding/email-options.html', + 'views/service-settings/branding/branding-options.html', form=form, + branding_type=branding_type, + branding_name=branding_name ) diff --git a/app/models/service.py b/app/models/service.py index 5fdda9085..16e5a2539 100644 --- a/app/models/service.py +++ b/app/models/service.py @@ -417,6 +417,12 @@ class Service(JSONModel): return 'GOV.UK' return self.email_branding['name'] + @cached_property + def letter_branding_name(self): + if self.letter_branding is None: + return 'no' + return self.letter_branding['name'] + @property def needs_to_change_email_branding(self): return self.email_branding_id is None and self.organisation_type != Organisation.TYPE_CENTRAL diff --git a/app/templates/views/service-settings.html b/app/templates/views/service-settings.html index 7da88458b..1f118a5e4 100644 --- a/app/templates/views/service-settings.html +++ b/app/templates/views/service-settings.html @@ -105,7 +105,7 @@ {{ text_field(current_service.email_branding_name) }} {{ edit_field( 'Change', - url_for('.branding_request', service_id=current_service.id), + url_for('.branding_request', service_id=current_service.id, branding_type="email"), permissions=['manage_service'], )}} {% endcall %} @@ -241,7 +241,7 @@ {{ optional_text_field(current_service.letter_branding.name) }} {{ edit_field( 'Change', - url_for('.request_letter_branding', service_id=current_service.id), + url_for('.branding_request', service_id=current_service.id, branding_type="letter"), permissions=['manage_service'] )}} {% endcall %} diff --git a/app/templates/views/service-settings/branding/email-options.html b/app/templates/views/service-settings/branding/branding-options.html similarity index 87% rename from app/templates/views/service-settings/branding/email-options.html rename to app/templates/views/service-settings/branding/branding-options.html index 2fd7563f4..2be9c49fe 100644 --- a/app/templates/views/service-settings/branding/email-options.html +++ b/app/templates/views/service-settings/branding/branding-options.html @@ -7,21 +7,21 @@ {% from "components/form.html" import form_wrapper %} {% block service_page_title %} - Change email branding + Change {{ branding_type }} branding {% endblock %} {% block maincolumn_content %} {{ page_header( - 'Change email branding', + 'Change {} branding'.format(branding_type), back_link=url_for('main.service_settings', service_id=current_service.id) ) }}

- Your emails currently have {{ current_service.email_branding_name }} branding. + Your {{ branding_type }}s currently have {{ branding_name }} branding.

- {% if current_service.needs_to_change_email_branding %} + {% if current_service.needs_to_change_email_branding and branding_type == "email" %}

You should be using your own branding instead. We can help you to set this up.

diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index de9e6316f..6449ed6a3 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -4439,6 +4439,7 @@ def test_update_service_organisation_does_not_update_if_same_value( mock_update_service_organisation.called is False +@pytest.mark.parametrize('branding_type', ['email', 'letter']) @pytest.mark.parametrize('organisation_type, expected_options', ( ('central', None), ('local', None), @@ -4457,15 +4458,17 @@ def test_update_service_organisation_does_not_update_if_same_value( ('emergency_service', None), ('other', None), )) -def test_show_email_branding_request_page_when_no_email_branding_is_set( +def test_show_branding_request_page_when_no_branding_is_set( mocker, service_one, client_request, mock_get_email_branding, + mock_get_letter_branding_by_id, organisation_type, expected_options, + branding_type ): - service_one['email_branding'] = None + service_one['{}_branding'.format(branding_type)] = None service_one['organisation_type'] = organisation_type mocker.patch( 'app.organisations_client.get_service_organisation', @@ -4473,10 +4476,11 @@ def test_show_email_branding_request_page_when_no_email_branding_is_set( ) page = client_request.get( - '.branding_request', service_id=SERVICE_ONE_ID + '.branding_request', service_id=SERVICE_ONE_ID, branding_type=branding_type ) mock_get_email_branding.assert_not_called() + mock_get_letter_branding_by_id.assert_not_called() if expected_options: assert [ @@ -4500,6 +4504,7 @@ def test_show_email_branding_request_page_when_no_email_branding_is_set( assert not page.select('.conditional-radios-panel') +@pytest.mark.parametrize('branding_type', ['email', 'letter']) @pytest.mark.parametrize('organisation_type, expected_options', ( ('central', [ ('govuk_and_org', 'GOV.UK and Test Organisation'), @@ -4531,15 +4536,17 @@ def test_show_email_branding_request_page_when_no_email_branding_is_set( ('something_else', 'Something else'), ]), )) -def test_show_email_branding_request_page_when_no_email_branding_is_set_but_organisation_exists( +def test_show_branding_request_page_when_no_branding_is_set_but_organisation_exists( mocker, service_one, client_request, mock_get_email_branding, + mock_get_letter_branding_by_id, organisation_type, expected_options, + branding_type ): - service_one['email_branding'] = None + service_one['{}_branding'.format(branding_type)] = None service_one['organisation_type'] = organisation_type mocker.patch( 'app.organisations_client.get_service_organisation', @@ -4547,10 +4554,11 @@ def test_show_email_branding_request_page_when_no_email_branding_is_set_but_orga ) page = client_request.get( - '.branding_request', service_id=SERVICE_ONE_ID + '.branding_request', service_id=SERVICE_ONE_ID, branding_type=branding_type ) mock_get_email_branding.assert_not_called() + mock_get_letter_branding_by_id.assert_not_called() assert [ ( @@ -4561,21 +4569,24 @@ def test_show_email_branding_request_page_when_no_email_branding_is_set_but_orga ] == expected_options -def test_show_email_branding_request_page_when_email_branding_is_set( +@pytest.mark.parametrize('branding_type', ['email', 'letter']) +def test_show_branding_request_page_when_branding_is_set( mocker, service_one, client_request, mock_get_email_branding, + mock_get_letter_branding_by_id, active_user_with_permissions, + branding_type ): - service_one['email_branding'] = sample_uuid() + service_one['{}_branding'.format(branding_type)] = 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 + '.branding_request', service_id=SERVICE_ONE_ID, branding_type=branding_type ) assert [ ( @@ -4591,21 +4602,30 @@ def test_show_email_branding_request_page_when_email_branding_is_set( ] -def test_show_email_branding_request_page_when_email_branding_is_same_as_org( +@pytest.mark.parametrize('branding_type', ['email', 'letter']) +def test_show_branding_request_page_when_branding_is_same_as_org( mocker, service_one, client_request, mock_get_email_branding, + mock_get_letter_branding_by_id, active_user_with_permissions, + branding_type ): - 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']), - ) + service_one['{}_branding'.format(branding_type)] = sample_uuid() + if branding_type == 'email': + mocker.patch( + 'app.organisations_client.get_service_organisation', + return_value=organisation_json(email_branding_id=service_one['email_branding']), + ) + else: + mocker.patch( + 'app.organisations_client.get_service_organisation', + return_value=organisation_json(letter_branding_id=service_one['letter_branding']), + ) page = client_request.get( - '.branding_request', service_id=SERVICE_ONE_ID + '.branding_request', service_id=SERVICE_ONE_ID, branding_type=branding_type ) # Central government organisations who have their own default @@ -4616,6 +4636,9 @@ def test_show_email_branding_request_page_when_email_branding_is_same_as_org( assert page.select_one('textarea')['name'] == 'something_else' +@pytest.mark.parametrize('branding_type,current_branding', [ + ('email', 'Organisation name'), ('letter', 'HM Government') +]) @pytest.mark.parametrize('data, requested_branding', ( ( { @@ -4654,21 +4677,25 @@ def test_show_email_branding_request_page_when_email_branding_is_same_as_org( (None, 'Can’t tell (domain is user.gov.uk)'), ('Test Organisation', 'Test Organisation'), )) -def test_submit_email_branding_request( +def test_submit_branding_request( client_request, service_one, mocker, data, requested_branding, + branding_type, + current_branding, mock_get_service_settings_page_common, mock_get_email_branding, + mock_get_letter_branding_by_id, no_reply_to_email_addresses, no_letter_contact_blocks, single_sms_sender, org_name, expected_organisation, ): - service_one['email_branding'] = sample_uuid() + service_one['{}_branding'.format(branding_type)] = sample_uuid() + mocker.patch( 'app.organisations_client.get_service_organisation', return_value=organisation_json(name=org_name) if org_name else None, @@ -4680,7 +4707,7 @@ def test_submit_email_branding_request( ) page = client_request.post( - '.branding_request', service_id=SERVICE_ONE_ID, + '.branding_request', service_id=SERVICE_ONE_ID, branding_type=branding_type, _data=data, _follow_redirects=True, ) @@ -4692,10 +4719,10 @@ def test_submit_email_branding_request( 'http://localhost/services/596364a0-858e-42c8-9062-a8fe822260eb', '', '---', - 'Current branding: Organisation name', + 'Current branding: {}'.format(current_branding), 'Branding requested: {}\n', ]).format(expected_organisation, requested_branding), - subject='Email branding request - service one', + subject='{} branding request - service one'.format(branding_type.capitalize()), ticket_type='question', user_email='test@user.gov.uk', user_name='Test User', @@ -4707,12 +4734,18 @@ def test_submit_email_branding_request( ) -def test_submit_email_branding_when_something_else_is_only_option( +@pytest.mark.parametrize('branding_type,current_branding', [ + ('email', 'GOV.UK'), ('letter', 'no') +]) +def test_submit_branding_when_something_else_is_only_option( client_request, service_one, mocker, mock_get_service_settings_page_common, mock_get_email_branding, + mock_get_letter_branding_by_id, + branding_type, + current_branding, ): mocker.patch( 'app.organisations_client.get_service_organisation', @@ -4726,20 +4759,39 @@ def test_submit_email_branding_when_something_else_is_only_option( client_request.post( '.branding_request', - service_id=SERVICE_ONE_ID, + service_id=SERVICE_ONE_ID, branding_type=branding_type, _data={ 'something_else': 'Homer Simpson', }, ) assert ( - 'Current branding: GOV.UK\n' + 'Current branding: {}\n' 'Branding requested: Something else\n' '\n' - 'Homer Simpson' + 'Homer Simpson'.format(current_branding) ) in zendesk.call_args_list[0][1]['message'] +def test_service_settings_links_to_branding_request_page_for_letters( + mocker, + service_one, + client_request, + active_user_with_permissions, + no_reply_to_email_addresses, + no_letter_contact_blocks, + single_sms_sender, + mock_get_service_settings_page_common, + mock_get_service_organisation, +): + service_one["restricted"] is False + service_one['permissions'].append('letter') + page = client_request.get( + '.service_settings', service_id=SERVICE_ONE_ID + ) + assert len(page.findAll('a', attrs={'href': '/services/{}/branding-request/letter'.format(SERVICE_ONE_ID)})) == 1 + + def test_show_service_data_retention( platform_admin_client, service_one,