From d75c1ea398f357802e6dbd516603dd9874717b78 Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Thu, 17 Feb 2022 14:52:24 +0000 Subject: [PATCH 1/5] Split out the govuk and govuk_and_org branding templates The `.email_branding_govuk` and `.email_branding_govuk_and_org` routes shared a template since the content was the same - the only difference was in the action of the button. However, since the pages will no longer be so similar (e.g. the govuk page will show a preview) this splits them up to use separate templates. It may be the case that when the branding work is complete these pages are fairly similar and we decided that one template between the two endpoints is the best option again. --- app/main/views/service_settings.py | 6 +- .../branding/email-branding-govuk-org.html | 43 ++++++++++++ .../branding/email-branding-govuk.html | 2 +- tests/app/main/views/test_service_settings.py | 68 ++++++++++++++++--- 4 files changed, 107 insertions(+), 12 deletions(-) create mode 100644 app/templates/views/service-settings/branding/email-branding-govuk-org.html diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index b667b3777..76183ae3a 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -1196,7 +1196,7 @@ def email_branding_govuk(service_id): check_branding_allowed_for_service('govuk') if request.method == 'POST': - create_email_branding_zendesk_ticket(request.form['branding_choice']) + create_email_branding_zendesk_ticket('govuk') 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=current_service.id)) @@ -1210,12 +1210,12 @@ def email_branding_govuk_and_org(service_id): check_branding_allowed_for_service('govuk_and_org') if request.method == 'POST': - create_email_branding_zendesk_ticket(request.form['branding_choice']) + create_email_branding_zendesk_ticket('govuk_and_org') 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=current_service.id)) - return render_template('views/service-settings/branding/email-branding-govuk.html', with_org=True) + return render_template('views/service-settings/branding/email-branding-govuk-org.html') @main.route("/services//service-settings/email-branding/nhs", methods=['GET', 'POST']) diff --git a/app/templates/views/service-settings/branding/email-branding-govuk-org.html b/app/templates/views/service-settings/branding/email-branding-govuk-org.html new file mode 100644 index 000000000..3fc667faa --- /dev/null +++ b/app/templates/views/service-settings/branding/email-branding-govuk-org.html @@ -0,0 +1,43 @@ +{% extends "withnav_template.html" %} +{% from "components/form.html" import form_wrapper %} +{% from "components/back-link/macro.njk" import govukBackLink %} +{% from "components/page-footer.html" import page_footer %} +{% from "components/page-header.html" import page_header %} + +{% block service_page_title %} + Before you request new branding +{% endblock %} + +{% block backLink %} + {{ govukBackLink({ + "href": url_for('.email_branding_request', service_id=current_service.id) + }) }} +{% endblock %} + +{% block maincolumn_content %} + + {{ page_header('Before you request new branding') }} + +

Check that your new branding matches the rest of your service.

+ +

You can use the GOV.UK logo on your emails if:

+ + +

+ You cannot use GOV.UK branding if your organisation is + independent + from government. +

+ +

We’ll email you once your branding’s ready to use, or if we need any more information.

+ + {% call form_wrapper() %} + {{ page_footer('Request new branding') }} + {% endcall %} + +{% endblock %} diff --git a/app/templates/views/service-settings/branding/email-branding-govuk.html b/app/templates/views/service-settings/branding/email-branding-govuk.html index 925331902..3fc667faa 100644 --- a/app/templates/views/service-settings/branding/email-branding-govuk.html +++ b/app/templates/views/service-settings/branding/email-branding-govuk.html @@ -37,7 +37,7 @@

We’ll email you once your branding’s ready to use, or if we need any more information.

{% call form_wrapper() %} - {{ page_footer('Request new branding', button_name='branding_choice', button_value=('govuk_and_org' if with_org else 'govuk')) }} + {{ page_footer('Request new branding') }} {% endcall %} {% endblock %} diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index bb94c099d..4cf9184d9 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -5495,10 +5495,6 @@ def test_get_email_branding_description_pages_give_404_if_selected_branding_not_ ) -@pytest.mark.parametrize('branding_choice, branding_description', [ - ('govuk', 'GOV.UK'), - ('govuk_and_org', 'GOV.UK and organisation one'), -]) def test_submit_email_branding_request_from_govuk_description_page( mocker, client_request, @@ -5508,8 +5504,6 @@ def test_submit_email_branding_request_from_govuk_description_page( no_reply_to_email_addresses, mock_get_email_branding, single_sms_sender, - branding_choice, - branding_description, ): mocker.patch( 'app.organisations_client.get_organisation', @@ -5531,7 +5525,6 @@ def test_submit_email_branding_request_from_govuk_description_page( page = client_request.post( '.email_branding_govuk', service_id=SERVICE_ONE_ID, - _data={'branding_choice': branding_choice}, _follow_redirects=True, ) @@ -5544,7 +5537,66 @@ def test_submit_email_branding_request_from_govuk_description_page( '', '---', 'Current branding: Organisation name', - f'Branding requested: {branding_description}\n', + 'Branding requested: GOV.UK\n', + ]), + subject='Email branding request - service one', + ticket_type='question', + user_name='Test User', + user_email='test@user.gov.uk', + org_id=ORGANISATION_ID, + org_type='central', + service_id=SERVICE_ONE_ID + ) + mock_send_ticket_to_zendesk.assert_called_once() + assert normalize_spaces(page.select_one('.banner-default').text) == ( + 'Thanks for your branding request. We’ll get back to you ' + 'within one working day.' + ) + + +def test_submit_email_branding_request_from_govuk_and_org_description_page( + mocker, + client_request, + service_one, + organisation_one, + mock_get_service_settings_page_common, + no_reply_to_email_addresses, + mock_get_email_branding, + single_sms_sender, +): + mocker.patch( + 'app.organisations_client.get_organisation', + return_value=organisation_one, + ) + mocker.patch( + 'app.models.service.Service.organisation_id', + new_callable=PropertyMock, + return_value=ORGANISATION_ID, + ) + service_one['email_branding'] = sample_uuid() + + mock_create_ticket = mocker.spy(NotifySupportTicket, '__init__') + mock_send_ticket_to_zendesk = mocker.patch( + 'app.main.views.service_settings.zendesk_client.send_ticket_to_zendesk', + autospec=True, + ) + + page = client_request.post( + '.email_branding_govuk_and_org', + service_id=SERVICE_ONE_ID, + _follow_redirects=True, + ) + + mock_create_ticket.assert_called_once_with( + ANY, + message='\n'.join([ + 'Organisation: organisation one', + 'Service: service one', + 'http://localhost/services/596364a0-858e-42c8-9062-a8fe822260eb', + '', + '---', + 'Current branding: Organisation name', + 'Branding requested: GOV.UK and organisation one\n', ]), subject='Email branding request - service one', ticket_type='question', From 87e6737f9e0ee5553cf54ae0fe71a7557fe56e10 Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Tue, 22 Feb 2022 13:15:29 +0000 Subject: [PATCH 2/5] Highlight "Settings" in menu if visiting govuk and org branding page This was accidentally missed out of a previous PR. It ensures that when you visit the `.email_branding_govuk_and_org` endpoint "Settings" is highlighted in the left hand menu. --- app/navigation.py | 1 + 1 file changed, 1 insertion(+) diff --git a/app/navigation.py b/app/navigation.py index a928000b4..d528ba855 100644 --- a/app/navigation.py +++ b/app/navigation.py @@ -220,6 +220,7 @@ class MainNavigation(Navigation): 'add_organisation_from_gp_service', 'add_organisation_from_nhs_local_service', 'email_branding_govuk', + 'email_branding_govuk_and_org', 'email_branding_nhs', 'email_branding_organisation', 'email_branding_request', From 5ada431da955425ff7a940516447bc9a9672bdb9 Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Tue, 22 Feb 2022 11:37:40 +0000 Subject: [PATCH 3/5] NHS and GOV.UK email branding pages now show preview and apply branding The pages you were redirected to if you selected either GOV.UK branding or NHS branding used to give information about the branding and have a button that submitted a Zendesk ticket. Now, we show a preview of the new branding and the button applies it. --- app/main/views/service_settings.py | 12 +- .../branding/email-branding-govuk.html | 23 ++-- .../branding/email-branding-nhs.html | 18 ++- tests/app/main/views/test_service_settings.py | 121 ++++++++---------- 4 files changed, 82 insertions(+), 92 deletions(-) diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index 76183ae3a..4d47e0056 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -80,6 +80,8 @@ PLATFORM_ADMIN_SERVICE_PERMISSIONS = OrderedDict([ ('international_letters', {'title': 'Send international letters', 'requires': 'letter'}), ]) +NHS_BRANDING_ID = 'a7dc4e56-660b-4db7-8cff-12c37b12b5ea' + @main.route("/services//service-settings") @user_has_permissions('manage_service', 'manage_api_keys') @@ -1196,9 +1198,9 @@ def email_branding_govuk(service_id): check_branding_allowed_for_service('govuk') if request.method == 'POST': - create_email_branding_zendesk_ticket('govuk') + current_service.update(email_branding=None) - flash('Thanks for your branding request. We’ll get back to you within one working day.', 'default') + flash('You’ve updated your email branding', 'default') return redirect(url_for('.service_settings', service_id=current_service.id)) return render_template('views/service-settings/branding/email-branding-govuk.html') @@ -1224,12 +1226,12 @@ def email_branding_nhs(service_id): check_branding_allowed_for_service('nhs') if request.method == 'POST': - create_email_branding_zendesk_ticket('nhs') + current_service.update(email_branding=NHS_BRANDING_ID) - flash('Thanks for your branding request. We’ll get back to you within one working day.', 'default') + flash('You’ve updated your email branding', 'default') return redirect(url_for('.service_settings', service_id=current_service.id)) - return render_template('views/service-settings/branding/email-branding-nhs.html') + return render_template('views/service-settings/branding/email-branding-nhs.html', nhs_branding_id=NHS_BRANDING_ID) @main.route("/services//service-settings/email-branding/organisation", methods=['GET', 'POST']) diff --git a/app/templates/views/service-settings/branding/email-branding-govuk.html b/app/templates/views/service-settings/branding/email-branding-govuk.html index 3fc667faa..a1f3ada20 100644 --- a/app/templates/views/service-settings/branding/email-branding-govuk.html +++ b/app/templates/views/service-settings/branding/email-branding-govuk.html @@ -5,7 +5,7 @@ {% from "components/page-header.html" import page_header %} {% block service_page_title %} - Before you request new branding + Check your new branding {% endblock %} {% block backLink %} @@ -16,16 +16,17 @@ {% block maincolumn_content %} - {{ page_header('Before you request new branding') }} + {{ page_header('Check your new branding') }} -

Check that your new branding matches the rest of your service.

+

+ Emails from {{ current_service.name }} will look like this. +

-

You can use the GOV.UK logo on your emails if:

-
    -
  • your website looks like GOV.UK
    • -
  • your email links to a website that looks like GOV.UK
    • -
  • people get an email from your service after using GOV.UK
    • -
+ + +

Before you continue

+ +

You can only use GOV.UK branding if people go to GOV.UK to access your service.

You cannot use GOV.UK branding if your organisation is @@ -34,10 +35,8 @@ from government.

-

We’ll email you once your branding’s ready to use, or if we need any more information.

- {% call form_wrapper() %} - {{ page_footer('Request new branding') }} + {{ page_footer('Use this branding') }} {% endcall %} {% endblock %} diff --git a/app/templates/views/service-settings/branding/email-branding-nhs.html b/app/templates/views/service-settings/branding/email-branding-nhs.html index 015dd68a4..9bdcc84c6 100644 --- a/app/templates/views/service-settings/branding/email-branding-nhs.html +++ b/app/templates/views/service-settings/branding/email-branding-nhs.html @@ -5,7 +5,7 @@ {% from "components/page-header.html" import page_header %} {% block service_page_title %} - Before you request new branding + Check your new branding {% endblock %} {% block backLink %} @@ -16,18 +16,24 @@ {% block maincolumn_content %} - {{ page_header('Before you request new branding') }} + {{ page_header('Check your new branding') }}

- Check that your service is allowed to use the NHS identity. + Emails from {{ current_service.name }} will look like this.

-

Your new branding should match the rest of your service.

+ -

We’ll email you once your branding’s ready to use, or if we need any more information.

+

Before you continue

+ +

Make sure you’re allowed to use NHS branding.

+ +

+ If you’re not sure, check the guidance on the NHS website. +

{% call form_wrapper() %} - {{ page_footer('Request new branding') }} + {{ page_footer('Use this branding') }} {% endcall %} {% endblock %} diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index 4cf9184d9..d9c302b82 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -14,6 +14,7 @@ from notifications_utils.clients.zendesk.zendesk_client import ( ) import app +from app.main.views.service_settings import NHS_BRANDING_ID from tests import ( find_element_by_tag_and_partial_text, invite_json, @@ -5432,24 +5433,19 @@ def test_submit_branding_when_something_else_is_only_option( ) in mock_create_ticket.call_args_list[0][1]['message'] -@pytest.mark.parametrize('endpoint, service_org_type, expected_heading', [ - ('main.email_branding_govuk', 'central', 'Before you request new branding'), - ('main.email_branding_govuk_and_org', 'central', 'Before you request new branding'), - ('main.email_branding_govuk', 'central', 'Before you request new branding'), - ('main.email_branding_nhs', 'nhs_local', 'Before you request new branding'), - ('main.email_branding_organisation', 'central', 'When you request new branding'), +@pytest.mark.parametrize('endpoint, expected_heading', [ + ('main.email_branding_govuk_and_org', 'Before you request new branding'), + ('main.email_branding_organisation', 'When you request new branding'), ]) -def test_get_email_branding_description_pages( +def test_get_email_branding_description_pages_for_org_branding( client_request, mocker, service_one, organisation_one, mock_get_email_branding, endpoint, - service_org_type, expected_heading, ): - organisation_one['organisation_type'] = service_org_type service_one['email_branding'] = sample_uuid() service_one['organisation'] = organisation_one @@ -5466,6 +5462,39 @@ def test_get_email_branding_description_pages( assert normalize_spaces(page.select_one('.page-footer button').text) == 'Request new branding' +@pytest.mark.parametrize('endpoint, service_org_type, branding_preview_id', [ + ('main.email_branding_govuk', 'central', '__NONE__'), + ('main.email_branding_nhs', 'nhs_local', NHS_BRANDING_ID), +]) +def test_get_email_branding_govuk_and_nhs_pages( + client_request, + mocker, + service_one, + organisation_one, + mock_get_email_branding, + endpoint, + service_org_type, + branding_preview_id, +): + organisation_one['organisation_type'] = service_org_type + service_one['email_branding'] = sample_uuid() + service_one['organisation'] = organisation_one + + mocker.patch( + 'app.organisations_client.get_organisation', + return_value=organisation_one, + ) + + page = client_request.get( + endpoint, + service_id=SERVICE_ONE_ID, + ) + assert page.h1.text == 'Check your new branding' + assert 'Emails from service one will look like this' in normalize_spaces(page.text) + assert page.find('iframe')['src'] == url_for('main.email_template', branding_style=branding_preview_id) + assert normalize_spaces(page.select_one('.page-footer button').text) == 'Use this branding' + + def test_get_email_branding_something_else_page(client_request): page = client_request.get( 'main.email_branding_something_else', @@ -5495,7 +5524,7 @@ def test_get_email_branding_description_pages_give_404_if_selected_branding_not_ ) -def test_submit_email_branding_request_from_govuk_description_page( +def test_update_email_branding_from_govuk_preview_page( mocker, client_request, service_one, @@ -5504,6 +5533,7 @@ def test_submit_email_branding_request_from_govuk_description_page( no_reply_to_email_addresses, mock_get_email_branding, single_sms_sender, + mock_update_service, ): mocker.patch( 'app.organisations_client.get_organisation', @@ -5516,42 +5546,18 @@ def test_submit_email_branding_request_from_govuk_description_page( ) service_one['email_branding'] = sample_uuid() - mock_create_ticket = mocker.spy(NotifySupportTicket, '__init__') - mock_send_ticket_to_zendesk = mocker.patch( - 'app.main.views.service_settings.zendesk_client.send_ticket_to_zendesk', - autospec=True, - ) - page = client_request.post( '.email_branding_govuk', service_id=SERVICE_ONE_ID, _follow_redirects=True, ) - mock_create_ticket.assert_called_once_with( - ANY, - message='\n'.join([ - 'Organisation: organisation one', - 'Service: service one', - 'http://localhost/services/596364a0-858e-42c8-9062-a8fe822260eb', - '', - '---', - 'Current branding: Organisation name', - 'Branding requested: GOV.UK\n', - ]), - subject='Email branding request - service one', - ticket_type='question', - user_name='Test User', - user_email='test@user.gov.uk', - org_id=ORGANISATION_ID, - org_type='central', - service_id=SERVICE_ONE_ID - ) - mock_send_ticket_to_zendesk.assert_called_once() - assert normalize_spaces(page.select_one('.banner-default').text) == ( - 'Thanks for your branding request. We’ll get back to you ' - 'within one working day.' + mock_update_service.assert_called_once_with( + SERVICE_ONE_ID, + email_branding=None, ) + assert page.h1.text == 'Settings' + assert normalize_spaces(page.select_one('.banner-default').text) == 'You’ve updated your email branding' def test_submit_email_branding_request_from_govuk_and_org_description_page( @@ -5613,7 +5619,7 @@ def test_submit_email_branding_request_from_govuk_and_org_description_page( ) -def test_submit_email_branding_request_from_nhs_description_page( +def test_update_email_branding_from_nhs_preview_page( mocker, client_request, service_one, @@ -5622,46 +5628,23 @@ def test_submit_email_branding_request_from_nhs_description_page( no_reply_to_email_addresses, mock_get_email_branding, single_sms_sender, + mock_update_service, ): service_one['email_branding'] = sample_uuid() service_one['organisation_type'] = 'nhs_local' - mock_create_ticket = mocker.spy(NotifySupportTicket, '__init__') - mock_send_ticket_to_zendesk = mocker.patch( - 'app.main.views.service_settings.zendesk_client.send_ticket_to_zendesk', - autospec=True, - ) - page = client_request.post( '.email_branding_nhs', service_id=SERVICE_ONE_ID, _follow_redirects=True, ) - mock_create_ticket.assert_called_once_with( - ANY, - message='\n'.join([ - 'Organisation: Can’t tell (domain is user.gov.uk)', - 'Service: service one', - 'http://localhost/services/596364a0-858e-42c8-9062-a8fe822260eb', - '', - '---', - 'Current branding: Organisation name', - 'Branding requested: NHS\n', - ]), - subject='Email branding request - service one', - ticket_type='question', - user_name='Test User', - user_email='test@user.gov.uk', - org_id=None, - org_type='nhs_local', - service_id=SERVICE_ONE_ID - ) - mock_send_ticket_to_zendesk.assert_called_once() - assert normalize_spaces(page.select_one('.banner-default').text) == ( - 'Thanks for your branding request. We’ll get back to you ' - 'within one working day.' + mock_update_service.assert_called_once_with( + SERVICE_ONE_ID, + email_branding=NHS_BRANDING_ID, ) + assert page.h1.text == 'Settings' + assert normalize_spaces(page.select_one('.banner-default').text) == 'You’ve updated your email branding' def test_submit_email_branding_request_from_organisation_description_page( From 77e1c015c4cdd2a86e7e739cb566a18b40017c4c Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Tue, 22 Feb 2022 13:11:23 +0000 Subject: [PATCH 4/5] Update GOV.UK and org email branding content To make it consistent with the GOV.UK email branding page content. --- .../branding/email-branding-govuk-org.html | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/app/templates/views/service-settings/branding/email-branding-govuk-org.html b/app/templates/views/service-settings/branding/email-branding-govuk-org.html index 3fc667faa..7fa100ee9 100644 --- a/app/templates/views/service-settings/branding/email-branding-govuk-org.html +++ b/app/templates/views/service-settings/branding/email-branding-govuk-org.html @@ -18,14 +18,7 @@ {{ page_header('Before you request new branding') }} -

Check that your new branding matches the rest of your service.

- -

You can use the GOV.UK logo on your emails if:

-
    -
  • your website looks like GOV.UK
    • -
  • your email links to a website that looks like GOV.UK
    • -
  • people get an email from your service after using GOV.UK
    • -
+

You can only use GOV.UK branding if people go to GOV.UK to access your service.

You cannot use GOV.UK branding if your organisation is From 4e409f7d93f80142775e57288abefe80225a27fe Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Tue, 22 Feb 2022 14:57:49 +0000 Subject: [PATCH 5/5] Show preview of current branding This adds a preview of the current branding to users on the page where they can select which new branding they want. Also includes a tiny content change to match the new content doc for this story. --- .../branding/email-branding-options.html | 5 ++++- tests/app/main/views/test_service_settings.py | 15 ++++++++++++++- 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/app/templates/views/service-settings/branding/email-branding-options.html b/app/templates/views/service-settings/branding/email-branding-options.html index 7e9d7edc1..7354e448d 100644 --- a/app/templates/views/service-settings/branding/email-branding-options.html +++ b/app/templates/views/service-settings/branding/email-branding-options.html @@ -25,9 +25,12 @@ Your emails currently have {{ branding_name }} branding.

+ {% set branding_id = current_service.email_branding_id if current_service.email_branding else '__NONE__' %} + + {% if current_service.needs_to_change_email_branding %}

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

{% endif %} diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index d9c302b82..1ead2e1f4 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -4772,6 +4772,7 @@ def test_update_service_organisation_does_not_update_if_same_value( def test_show_email_branding_request_page_when_no_branding_is_set( service_one, client_request, + mocker, mock_get_email_branding, mock_get_letter_branding_by_id, organisation_type, @@ -4780,11 +4781,18 @@ def test_show_email_branding_request_page_when_no_branding_is_set( service_one['email_branding'] = None service_one['organisation_type'] = organisation_type + mocker.patch( + 'app.models.service.Service.email_branding_id', + new_callable=PropertyMock, + return_value=None, + ) + page = client_request.get( '.email_branding_request', service_id=SERVICE_ONE_ID ) assert mock_get_email_branding.called is False + assert page.find('iframe')['src'] == url_for('main.email_template', branding_style='__NONE__') assert mock_get_letter_branding_by_id.called is False button_text = normalize_spaces(page.select_one('.page-footer button').text) @@ -4984,17 +4992,22 @@ def test_show_email_branding_request_page_when_email_branding_is_set( client_request, mock_get_email_branding, mock_get_service_organisation, - active_user_with_permissions, ): service_one['email_branding'] = sample_uuid() mocker.patch( 'app.organisations_client.get_organisation', return_value=organisation_json(), ) + mocker.patch( + 'app.models.service.Service.email_branding_id', + new_callable=PropertyMock, + return_value='1234-abcd', + ) page = client_request.get( '.email_branding_request', service_id=SERVICE_ONE_ID ) + assert page.find('iframe')['src'] == url_for('main.email_template', branding_style='1234-abcd') assert [ ( radio['value'],