From 02cb6c9c38924208a58489fee6c1279756184a7f Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Fri, 17 Jan 2020 17:03:07 +0000 Subject: [PATCH 1/5] 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, From cc61e87701714ddb0da1e4e1a814c045e0c36802 Mon Sep 17 00:00:00 2001 From: "Pea M. Tyczynska" Date: Tue, 21 Jan 2020 14:52:21 +0000 Subject: [PATCH 2/5] Parametrized options label for the branding request form Co-Authored-By: Chris Hill-Scott --- app/main/forms.py | 1 + 1 file changed, 1 insertion(+) diff --git a/app/main/forms.py b/app/main/forms.py index 28d9ec46c..40b4c09d2 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -1403,6 +1403,7 @@ class BrandingOptions(StripWhitespaceForm): def __init__(self, service, *args, branding_type="email", **kwargs): super().__init__(*args, **kwargs) self.options.choices = tuple(self.get_available_choices(service, branding_type)) + self.options.label.text = 'Choose your new {} branding'.format(branding_type) if self.something_else_is_only_option: self.options.data = self.FALLBACK_OPTION_VALUE From 5a32177982723fdaa024a08a32563a2735e1150a Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Tue, 21 Jan 2020 16:02:41 +0000 Subject: [PATCH 3/5] Delete old letter branding request page --- app/main/views/service_settings.py | 12 +---- app/navigation.py | 4 -- .../branding/branding-options.html | 2 +- .../request-letter-branding.html | 39 -------------- app/templates/views/templates/_template.html | 2 +- tests/app/main/views/test_service_settings.py | 52 ------------------- tests/app/main/views/test_templates.py | 5 +- 7 files changed, 8 insertions(+), 108 deletions(-) delete mode 100644 app/templates/views/service-settings/request-letter-branding.html diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index 1237aba10..c10dde797 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -1001,15 +1001,6 @@ def service_preview_letter_branding(service_id): ) -@main.route("/services//service-settings/request-letter-branding", methods=['GET', 'POST']) -@user_has_permissions('manage_service', 'manage_templates') -def request_letter_branding(service_id): - return render_template( - 'views/service-settings/request-letter-branding.html', - from_template=request.args.get('from_template'), - ) - - @main.route("/services//service-settings/link-service-to-organisation", methods=['GET', 'POST']) @user_is_platform_admin def link_service_to_organisation(service_id): @@ -1085,7 +1076,8 @@ def branding_request(service_id, branding_type): 'views/service-settings/branding/branding-options.html', form=form, branding_type=branding_type, - branding_name=branding_name + branding_name=branding_name, + from_template=request.args.get('from_template') ) diff --git a/app/navigation.py b/app/navigation.py index 354483c7d..2b544bb0f 100644 --- a/app/navigation.py +++ b/app/navigation.py @@ -237,7 +237,6 @@ class HeaderNavigation(Navigation): 'registration_continue', 'remove_user_from_organisation', 'remove_user_from_service', - 'request_letter_branding', 'request_to_go_live', 'resend_email_link', 'resend_email_verification', @@ -404,7 +403,6 @@ class MainNavigation(Navigation): 'branding_request', 'estimate_usage', 'link_service_to_organisation', - 'request_letter_branding', 'request_to_go_live', 'service_add_email_reply_to', 'service_add_letter_contact', @@ -802,7 +800,6 @@ class CaseworkNavigation(Navigation): 'registration_continue', 'remove_user_from_organisation', 'remove_user_from_service', - 'request_letter_branding', 'request_to_go_live', 'resend_email_link', 'resend_email_verification', @@ -1081,7 +1078,6 @@ class OrgNavigation(Navigation): 'register_from_org_invite', 'registration_continue', 'remove_user_from_service', - 'request_letter_branding', 'request_to_go_live', 'resend_email_link', 'resend_email_verification', diff --git a/app/templates/views/service-settings/branding/branding-options.html b/app/templates/views/service-settings/branding/branding-options.html index 2be9c49fe..d7b3cb68f 100644 --- a/app/templates/views/service-settings/branding/branding-options.html +++ b/app/templates/views/service-settings/branding/branding-options.html @@ -14,7 +14,7 @@ {{ page_header( 'Change {} branding'.format(branding_type), - back_link=url_for('main.service_settings', service_id=current_service.id) + back_link=url_for('.view_template', service_id=current_service.id, template_id=from_template) if from_template else url_for('.service_settings', service_id=current_service.id) ) }}

diff --git a/app/templates/views/service-settings/request-letter-branding.html b/app/templates/views/service-settings/request-letter-branding.html deleted file mode 100644 index 508182001..000000000 --- a/app/templates/views/service-settings/request-letter-branding.html +++ /dev/null @@ -1,39 +0,0 @@ -{% extends "withnav_template.html" %} -{% from "components/radios.html" import radios %} -{% from "components/page-header.html" import page_header %} -{% from "components/page-footer.html" import page_footer %} - -{% block service_page_title %} - Letter branding -{% endblock %} - -{% block maincolumn_content %} - - {{ page_header( - 'Letter branding', - back_link=url_for('.view_template', service_id=current_service.id, template_id=from_template) if from_template else url_for('.service_settings', service_id=current_service.id) - ) }} - -

-
- {% if current_service.letter_branding_id %} -

- Your letters have the {{ current_service.letter_branding.name }} logo. -

-

- Contact us - if you want to use a different logo. -

- {% else %} -

- Your letters do not have a logo. -

-

- Contact us - if you want to add your organisation’s logo. -

- {% endif %} -
-
- -{% endblock %} diff --git a/app/templates/views/templates/_template.html b/app/templates/views/templates/_template.html index f236a5a1a..eecee2ece 100644 --- a/app/templates/views/templates/_template.html +++ b/app/templates/views/templates/_template.html @@ -51,7 +51,7 @@
{% if current_user.has_permissions('manage_templates') and template.template_type == 'letter' %} {% if not current_service.letter_branding_id %} - Add logo + Add logo {% endif %} Change Edit diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index 6449ed6a3..0127e2b80 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -3024,58 +3024,6 @@ def test_set_letter_contact_block_has_max_10_lines( assert error_message == 'Contains 11 lines, maximum is 10' -@pytest.mark.parametrize('extra_args, expected_partial_url', ( - ( - {}, - partial(url_for, 'main.service_settings') - ), - ( - {'from_template': FAKE_TEMPLATE_ID}, - partial(url_for, 'main.view_template', template_id=FAKE_TEMPLATE_ID) - ), -)) -def test_request_letter_branding( - client_request, - mock_get_letter_branding_by_id, - extra_args, - expected_partial_url, -): - request_page = client_request.get( - 'main.request_letter_branding', - service_id=SERVICE_ONE_ID, - **extra_args - ) - assert request_page.select_one('main p').text.strip() == 'Your letters do not have a logo.' - back_link_href = request_page.select('main a')[0]['href'] - link_href = request_page.select('main a')[1]['href'] - assert link_href == url_for( - 'main.feedback', - ticket_type='ask-question-give-feedback', - body='letter-branding', - ) - feedback_page = client_request.get_url(link_href) - assert feedback_page.select_one('textarea').text.strip() == ( - 'I would like my own logo on my letter templates.' - ) - assert back_link_href == expected_partial_url(service_id=SERVICE_ONE_ID) - - -def test_request_letter_branding_if_already_have_branding( - client_request, - mock_get_letter_branding_by_id, - service_one, -): - service_one['letter_branding'] = uuid4() - - request_page = client_request.get( - 'main.request_letter_branding', - service_id=SERVICE_ONE_ID, - ) - - mock_get_letter_branding_by_id.assert_called_once_with(service_one['letter_branding']) - assert request_page.select_one('main p').text.strip() == 'Your letters have the HM Government logo.' - - def test_service_set_letter_branding_platform_admin_only( client_request, ): diff --git a/tests/app/main/views/test_templates.py b/tests/app/main/views/test_templates.py index fde9b8f0b..fd79523f9 100644 --- a/tests/app/main/views/test_templates.py +++ b/tests/app/main/views/test_templates.py @@ -437,7 +437,10 @@ def test_user_with_only_send_and_view_sees_letter_page( @pytest.mark.parametrize('letter_branding, expected_link, expected_link_text', ( ( None, - partial(url_for, 'main.request_letter_branding', from_template=TEMPLATE_ONE_ID), + partial( + url_for, 'main.branding_request', + service_id=SERVICE_ONE_ID, branding_type="letter", from_template=TEMPLATE_ONE_ID + ), 'Add logo', ), ( From fdff9d3b83f05581f9dd8599d1b702761929e2e8 Mon Sep 17 00:00:00 2001 From: "Pea M. Tyczynska" Date: Tue, 21 Jan 2020 16:39:15 +0000 Subject: [PATCH 4/5] Ensure gov.uk branding only available for emails and not for letters Also align and statements --- app/main/forms.py | 34 ++-- tests/app/main/views/test_service_settings.py | 179 ++++++++++++++++-- 2 files changed, 177 insertions(+), 36 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index 40b4c09d2..e9a0bb48f 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -1419,17 +1419,19 @@ class BrandingOptions(StripWhitespaceForm): service_branding_name = service.letter_branding_name if ( - service.organisation_type == Organisation.TYPE_CENTRAL and - organisation_branding_id is None and - service_branding_id is not None + service.organisation_type == Organisation.TYPE_CENTRAL + and organisation_branding_id is None + and service_branding_id is not None + and branding_type == "email" ): yield ('govuk', 'GOV.UK') if ( - service.organisation_type == Organisation.TYPE_CENTRAL and - service.organisation and - organisation_branding_id is None and - service_branding_name.lower() != 'GOV.UK and {}'.format(service.organisation.name).lower() + service.organisation_type == Organisation.TYPE_CENTRAL + and service.organisation + and organisation_branding_id is None + and service_branding_name.lower() != 'GOV.UK and {}'.format(service.organisation.name).lower() + and branding_type == "email" ): yield ('govuk_and_org', 'GOV.UK and {}'.format(service.organisation.name)) @@ -1438,19 +1440,21 @@ class BrandingOptions(StripWhitespaceForm): Organisation.TYPE_NHS_CENTRAL, Organisation.TYPE_NHS_LOCAL, Organisation.TYPE_NHS_GP, - } and service_branding_name != 'NHS' + } + and service_branding_name != 'NHS' ): yield ('nhs', 'NHS') if ( - service.organisation and - service.organisation_type not in { + service.organisation + and service.organisation_type not in { Organisation.TYPE_NHS_LOCAL, Organisation.TYPE_NHS_CENTRAL, Organisation.TYPE_NHS_GP, - } and ( - service_branding_id is None or - service_branding_id != organisation_branding_id + } + and ( + service_branding_id is None + or service_branding_id != organisation_branding_id ) ): yield ('organisation', service.organisation.name) @@ -1463,8 +1467,8 @@ class BrandingOptions(StripWhitespaceForm): def validate_something_else(self, field): if ( - self.something_else_is_only_option or - self.options.data == self.FALLBACK_OPTION_VALUE + self.something_else_is_only_option + or self.options.data == self.FALLBACK_OPTION_VALUE ) and not field.data: raise ValidationError('Cannot be empty') diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index 0127e2b80..df6aa9173 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -4454,11 +4454,6 @@ def test_show_branding_request_page_when_no_branding_is_set( @pytest.mark.parametrize('branding_type', ['email', 'letter']) @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'), @@ -4517,24 +4512,65 @@ def test_show_branding_request_page_when_no_branding_is_set_but_organisation_exi ] == expected_options -@pytest.mark.parametrize('branding_type', ['email', 'letter']) -def test_show_branding_request_page_when_branding_is_set( +@pytest.mark.parametrize('organisation_type, expected_options, branding_type', ( + ('central', [ + ('govuk_and_org', 'GOV.UK and Test Organisation'), + ('organisation', 'Test Organisation'), + ('something_else', 'Something else'), + ], 'email'), + ('central', [ + ('organisation', 'Test Organisation'), + ('something_else', 'Something else'), + ], 'letter'), +)) +def test_show_branding_request_page_when_no_branding_is_set_but_organisation_exists_central_org( mocker, service_one, client_request, mock_get_email_branding, mock_get_letter_branding_by_id, - active_user_with_permissions, + organisation_type, + expected_options, branding_type ): - service_one['{}_branding'.format(branding_type)] = sample_uuid() + service_one['{}_branding'.format(branding_type)] = 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, branding_type=branding_type + ) + + mock_get_email_branding.assert_not_called() + mock_get_letter_branding_by_id.assert_not_called() + + 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( + 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, branding_type=branding_type + '.branding_request', service_id=SERVICE_ONE_ID, branding_type="email" ) assert [ ( @@ -4550,6 +4586,34 @@ def test_show_branding_request_page_when_branding_is_set( ] +def test_show_letter_branding_request_page_when_letter_branding_is_set( + mocker, + service_one, + client_request, + mock_get_letter_branding_by_id, + active_user_with_permissions, +): + service_one['letter_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, branding_type="letter" + ) + assert [ + ( + radio['value'], + page.select_one('label[for={}]'.format(radio['id'])).text.strip() + ) + for radio in page.select('input[type=radio]') + ] == [ + ('organisation', 'Test Organisation'), + ('something_else', 'Something else'), + ] + + @pytest.mark.parametrize('branding_type', ['email', 'letter']) def test_show_branding_request_page_when_branding_is_same_as_org( mocker, @@ -4584,9 +4648,6 @@ def test_show_branding_request_page_when_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', ( ( { @@ -4625,24 +4686,21 @@ def test_show_branding_request_page_when_branding_is_same_as_org( (None, 'Can’t tell (domain is user.gov.uk)'), ('Test Organisation', 'Test Organisation'), )) -def test_submit_branding_request( +def test_submit_email_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['{}_branding'.format(branding_type)] = sample_uuid() + service_one['email_branding'] = sample_uuid() mocker.patch( 'app.organisations_client.get_service_organisation', @@ -4655,7 +4713,7 @@ def test_submit_branding_request( ) page = client_request.post( - '.branding_request', service_id=SERVICE_ONE_ID, branding_type=branding_type, + '.branding_request', service_id=SERVICE_ONE_ID, branding_type="email", _data=data, _follow_redirects=True, ) @@ -4667,10 +4725,89 @@ def test_submit_branding_request( 'http://localhost/services/596364a0-858e-42c8-9062-a8fe822260eb', '', '---', - 'Current branding: {}'.format(current_branding), + 'Current branding: Organisation name', 'Branding requested: {}\n', ]).format(expected_organisation, requested_branding), - subject='{} branding request - service one'.format(branding_type.capitalize()), + subject='Email branding request - service one', + ticket_type='question', + user_email='test@user.gov.uk', + user_name='Test User', + tags=['notify_action', 'notify_branding'], + ) + assert normalize_spaces(page.select_one('.banner-default').text) == ( + 'Thanks for your branding request. We’ll get back to you ' + 'within one working day.' + ) + + +@pytest.mark.parametrize('data, requested_branding', ( + ( + { + 'options': 'something_else', + 'something_else': 'Homer Simpson' + }, + 'Something else\n\nHomer Simpson' + ), + pytest.param( + { + 'options': 'something_else', + }, + '[Missing details]', + marks=pytest.mark.xfail(raises=AssertionError), + ), + 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)'), + ('Test Organisation', 'Test Organisation'), +)) +def test_submit_letter_branding_request( + client_request, + service_one, + mocker, + data, + requested_branding, + mock_get_service_settings_page_common, + mock_get_letter_branding_by_id, + no_reply_to_email_addresses, + no_letter_contact_blocks, + single_sms_sender, + org_name, + expected_organisation, +): + service_one['letter_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( + 'app.main.views.service_settings.zendesk_client.create_ticket', + autospec=True, + ) + + page = client_request.post( + '.branding_request', service_id=SERVICE_ONE_ID, branding_type="letter", + _data=data, + _follow_redirects=True, + ) + + zendesk.assert_called_once_with( + message='\n'.join([ + 'Organisation: {}', + 'Service: service one', + 'http://localhost/services/596364a0-858e-42c8-9062-a8fe822260eb', + '', + '---', + 'Current branding: HM Government', + 'Branding requested: {}\n', + ]).format(expected_organisation, requested_branding), + subject='Letter branding request - service one', ticket_type='question', user_email='test@user.gov.uk', user_name='Test User', From a578ec23a38c4f2eaa32de7ebe577940875e9c6b Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Thu, 23 Jan 2020 15:54:13 +0000 Subject: [PATCH 5/5] Redirect to template after succesful branding request if user entered branding request flow from a template. --- app/main/views/service_settings.py | 9 +-- tests/app/main/views/test_service_settings.py | 71 +++++++++++++++++++ 2 files changed, 76 insertions(+), 4 deletions(-) diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index c10dde797..4408cd85d 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -1032,8 +1032,8 @@ def link_service_to_organisation(service_id): @main.route("/services//branding-request/", methods=['GET', 'POST']) @user_has_permissions('manage_service') def branding_request(service_id, branding_type): - form = BrandingOptions(current_service, branding_type=branding_type) + from_template = request.args.get('from_template') if branding_type == "email": branding_name = current_service.email_branding_name elif branding_type == "letter": @@ -1065,19 +1065,20 @@ def branding_request(service_id, branding_type): user_name=current_user.name, tags=['notify_action', 'notify_branding'], ) - flash(( 'Thanks for your branding request. We’ll get back to you ' 'within one working day.' ), 'default') - return redirect(url_for('.service_settings', service_id=service_id)) + return redirect(url_for( + '.view_template', service_id=current_service.id, template_id=from_template + ) if from_template else url_for('.service_settings', service_id=current_service.id)) return render_template( 'views/service-settings/branding/branding-options.html', form=form, branding_type=branding_type, branding_name=branding_name, - from_template=request.args.get('from_template') + from_template=from_template ) diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index df6aa9173..d1bd6da55 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -4614,6 +4614,39 @@ def test_show_letter_branding_request_page_when_letter_branding_is_set( ] +@pytest.mark.parametrize('branding_type', ['email', 'letter']) +@pytest.mark.parametrize('from_template,back_link_url', [ + (None, '/services/{}/service-settings'.format(SERVICE_ONE_ID),), + (TEMPLATE_ONE_ID, '/services/{}/templates/{}'.format(SERVICE_ONE_ID, TEMPLATE_ONE_ID),) +]) +def test_back_link_on_branding_request_page( + mocker, + service_one, + client_request, + mock_get_email_branding, + mock_get_letter_branding_by_id, + active_user_with_permissions, + from_template, + back_link_url, + branding_type, +): + mocker.patch( + 'app.organisations_client.get_service_organisation', + return_value=organisation_json(), + ) + if from_template: + page = client_request.get( + '.branding_request', service_id=SERVICE_ONE_ID, branding_type=branding_type, from_template=from_template + ) + else: + page = client_request.get( + '.branding_request', service_id=SERVICE_ONE_ID, branding_type=branding_type + ) + + back_link = page.select('a[class=govuk-back-link]') + assert back_link[0].attrs['href'] == back_link_url + + @pytest.mark.parametrize('branding_type', ['email', 'letter']) def test_show_branding_request_page_when_branding_is_same_as_org( mocker, @@ -4819,6 +4852,44 @@ def test_submit_letter_branding_request( ) +@pytest.mark.parametrize('branding_type', ['email', 'letter']) +@pytest.mark.parametrize('from_template', [ + None, + TEMPLATE_ONE_ID +]) +def test_submit_letter_branding_request_redirects_if_from_template_is_set( + client_request, + service_one, + mocker, + mock_get_service_settings_page_common, + mock_get_letter_branding_by_id, + no_reply_to_email_addresses, + no_letter_contact_blocks, + single_sms_sender, + from_template, + branding_type, + +): + mocker.patch('app.organisations_client.get_service_organisation', return_value=None) + mocker.patch('app.main.views.service_settings.zendesk_client.create_ticket', autospec=True) + data = {'options': 'something_else', 'something_else': 'Homer Simpson'} + + if from_template: + client_request.post( + '.branding_request', service_id=SERVICE_ONE_ID, branding_type="letter", from_template=from_template, + _data=data, + _expected_redirect=url_for( + 'main.view_template', service_id=SERVICE_ONE_ID, template_id=from_template, _external=True + ) + ) + else: + client_request.post( + '.branding_request', service_id=SERVICE_ONE_ID, branding_type="letter", + _data=data, + _expected_redirect=url_for('main.service_settings', service_id=SERVICE_ONE_ID, _external=True) + ) + + @pytest.mark.parametrize('branding_type,current_branding', [ ('email', 'GOV.UK'), ('letter', 'no') ])