From 02cb6c9c38924208a58489fee6c1279756184a7f Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Fri, 17 Jan 2020 17:03:07 +0000 Subject: [PATCH] Create a letter branding request flow to match email branding request Test if service settings links to branding request page for letters Parametrize all branding tests so they also work for letter branding --- app/main/forms.py | 34 +++--- app/main/views/service_settings.py | 21 ++-- app/models/service.py | 6 ++ app/templates/views/service-settings.html | 4 +- ...ail-options.html => branding-options.html} | 8 +- tests/app/main/views/test_service_settings.py | 102 +++++++++++++----- 6 files changed, 123 insertions(+), 52 deletions(-) rename app/templates/views/service-settings/branding/{email-options.html => branding-options.html} (87%) 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,