From 33c6ca0989208286e163cee5a0be4c703ac8818a Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Wed, 2 Mar 2022 15:36:28 +0000 Subject: [PATCH 1/2] Link directly to email branding "something else" Previously we duplicated the "something else" email branding form on its own page and embedded in the choices form (if it was the only option). See [^1] for how this looks - it's inconsistent. This DRYs-up the "something else" form by bypassing the choices form when "something else" is the only option. I've also tweaked the "Back" button to be consistent with this behaviour. Making this change also simplifies the choices form, which we'll be adding pool options to shortly. I'd like to make the letters form consistent, but let's see how emails pan out first. Note that the choices form will now show a single radio button if "something else" is the only option. I think that's OK as nothing will link to the page, and the form still works. [^1]: https://github.com/alphagov/notifications-admin/pull/4163#issuecomment-1050088088 --- app/main/forms.py | 5 +- app/main/views/service_settings.py | 28 +-- app/templates/views/service-settings.html | 8 +- .../branding/email-branding-options.html | 30 +-- .../email-branding-something-else.html | 10 +- tests/app/main/views/test_service_settings.py | 205 ++++++++---------- 6 files changed, 120 insertions(+), 166 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index 2c89859a4..29093ba02 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -2245,10 +2245,7 @@ class BrandingOptions(StripWhitespaceForm): return self.options.choices == (self.FALLBACK_OPTION,) def validate_something_else(self, field): - if self.branding_type == 'email': - if self.something_else_is_only_option and not field.data: - raise ValidationError('Cannot be empty') - elif self.branding_type == 'letter': + if self.branding_type == 'letter': if ( self.something_else_is_only_option or self.options.data == self.FALLBACK_OPTION_VALUE diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index 4d47e0056..e8403acae 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -88,7 +88,8 @@ NHS_BRANDING_ID = 'a7dc4e56-660b-4db7-8cff-12c37b12b5ea' def service_settings(service_id): return render_template( 'views/service-settings.html', - service_permissions=PLATFORM_ADMIN_SERVICE_PERMISSIONS + service_permissions=PLATFORM_ADMIN_SERVICE_PERMISSIONS, + email_branding_options=BrandingOptions(current_service, branding_type='email') ) @@ -1161,21 +1162,12 @@ def email_branding_request(service_id): form = BrandingOptions(current_service, branding_type='email') branding_name = current_service.email_branding_name if form.validate_on_submit(): - if form.something_else_is_only_option: - create_email_branding_zendesk_ticket( - form_option_selected=form.options.data, - detail=form.something_else.data, - ) - - 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)) - else: - return redirect( - url_for( - f'.email_branding_{form.options.data}', - service_id=current_service.id, - ) + return redirect( + url_for( + f'.email_branding_{form.options.data}', + service_id=current_service.id, ) + ) return render_template( 'views/service-settings/branding/email-branding-options.html', @@ -1261,7 +1253,11 @@ def email_branding_something_else(service_id): 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-something-else.html', form=form) + return render_template( + 'views/service-settings/branding/email-branding-something-else.html', + form=form, + branding_options=BrandingOptions(current_service, branding_type='email') + ) @main.route("/services//service-settings/letter-branding", methods=['GET', 'POST']) diff --git a/app/templates/views/service-settings.html b/app/templates/views/service-settings.html index 91677f0a5..633565acb 100644 --- a/app/templates/views/service-settings.html +++ b/app/templates/views/service-settings.html @@ -92,12 +92,18 @@ }} {% endcall %} + {% if email_branding_options.something_else_is_only_option %} + {% set email_request_url = url_for('.email_branding_something_else', service_id=current_service.id) %} + {% else %} + {% set email_request_url = url_for('.email_branding_request', service_id=current_service.id) %} + {% endif %} + {% call settings_row(if_has_permission='email') %} {{ text_field('Email branding') }} {{ text_field(current_service.email_branding_name) }} {{ edit_field( 'Change', - url_for('.email_branding_request', service_id=current_service.id), + email_request_url, permissions=['manage_service'], suffix='email branding', )}} 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 7354e448d..9beee6142 100644 --- a/app/templates/views/service-settings/branding/email-branding-options.html +++ b/app/templates/views/service-settings/branding/email-branding-options.html @@ -34,27 +34,13 @@

{% endif %} - {% if form.something_else_is_only_option %} - {% call form_wrapper() %} - {{ textbox( - form.something_else, - hint='Include links to your brand guidelines or examples of how to use your branding.', - width='1-1', - ) }} -

- We’ll email you when your branding is ready, or if we need any more information. -

- {{ page_footer('Request new branding') }} - {% endcall %} - {% else %} - {% call form_wrapper() %} - {% call select_wrapper(form.options) %} - {% for option in form.options %} - {{ radio(option) }} - {% endfor %} - {% endcall %} - {{ page_footer('Continue') }} - {% endcall %} - {% endif %} + {% call form_wrapper() %} + {% call select_wrapper(form.options) %} + {% for option in form.options %} + {{ radio(option) }} + {% endfor %} + {% endcall %} + {{ page_footer('Continue') }} + {% endcall %} {% endblock %} diff --git a/app/templates/views/service-settings/branding/email-branding-something-else.html b/app/templates/views/service-settings/branding/email-branding-something-else.html index c3301f6db..dfb200a72 100644 --- a/app/templates/views/service-settings/branding/email-branding-something-else.html +++ b/app/templates/views/service-settings/branding/email-branding-something-else.html @@ -9,10 +9,14 @@ Describe the branding you want {% endblock %} +{% if branding_options.something_else_is_only_option %} + {% set back_url = url_for('.service_settings', service_id=current_service.id) %} +{% else %} + {% set back_url = url_for('.email_branding_request', service_id=current_service.id) %} +{% endif %} + {% block backLink %} - {{ govukBackLink({ - "href": url_for('.email_branding_request', service_id=current_service.id) - }) }} + {{ govukBackLink({"href": back_url}) }} {% endblock %} {% block maincolumn_content %} diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index 9d22a36ec..08b1d2d27 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -4753,8 +4753,12 @@ def test_update_service_organisation_does_not_update_if_same_value( @pytest.mark.parametrize('organisation_type, expected_options', ( - ('central', None), - ('local', None), + ('central', [ + ('something_else', 'Something else'), + ]), + ('local', [ + ('something_else', 'Something else'), + ]), ('nhs_central', [ ('nhs', 'NHS'), ('something_else', 'Something else'), @@ -4767,8 +4771,12 @@ def test_update_service_organisation_does_not_update_if_same_value( ('nhs', 'NHS'), ('something_else', 'Something else'), ]), - ('emergency_service', None), - ('other', None), + ('emergency_service', [ + ('something_else', 'Something else'), + ]), + ('other', [ + ('something_else', 'Something else'), + ]) )) def test_show_email_branding_request_page_when_no_branding_is_set( service_one, @@ -4798,24 +4806,15 @@ def test_show_email_branding_request_page_when_no_branding_is_set( button_text = normalize_spaces(page.select_one('.page-footer button').text) - if expected_options: - assert [ - ( - radio['value'], - page.select_one('label[for={}]'.format(radio['id'])).text.strip() - ) - for radio in page.select('input[type=radio]') - ] == expected_options - - assert button_text == 'Continue' - else: - assert page.select_one( - 'textarea' - )['name'] == ( - 'something_else' + assert [ + ( + radio['value'], + page.select_one('label[for={}]'.format(radio['id'])).text.strip() ) - assert not page.select('.conditional-radios-panel') - assert button_text == 'Request new branding' + for radio in page.select('input[type=radio]') + ] == expected_options + + assert button_text == 'Continue' @pytest.mark.parametrize('organisation_type, expected_options', ( @@ -5085,31 +5084,22 @@ def test_back_link_on_letter_branding_request_page( 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( +def test_show_letter_branding_request_page_when_branding_is_same_as_org( mocker, service_one, client_request, - mock_get_email_branding, mock_get_letter_branding_by_id, mock_get_service_organisation, active_user_with_permissions, - branding_type ): - service_one['{}_branding'.format(branding_type)] = sample_uuid() - if branding_type == 'email': - mocker.patch( - 'app.organisations_client.get_organisation', - return_value=organisation_json(email_branding_id=service_one['email_branding']), - ) - else: - mocker.patch( - 'app.organisations_client.get_organisation', - return_value=organisation_json(letter_branding_id=service_one['letter_branding']), - ) + service_one['letter_branding'] = sample_uuid() + mocker.patch( + 'app.organisations_client.get_organisation', + return_value=organisation_json(letter_branding_id=service_one['letter_branding']), + ) page = client_request.get( - f'.{branding_type}_branding_request', service_id=SERVICE_ONE_ID + '.letter_branding_request', service_id=SERVICE_ONE_ID ) # Central government organisations who have their own default @@ -5165,7 +5155,7 @@ def test_show_branding_request_page_when_branding_is_same_as_org( 'main.email_branding_nhs', ), )) -def test_submit_email_branding_request_when_something_else_is_not_the_only_option( +def test_submit_email_branding_request( client_request, service_one, mocker, @@ -5197,75 +5187,6 @@ def test_submit_email_branding_request_when_something_else_is_not_the_only_optio ) -def test_submit_email_branding_request_when_something_else_is_only_option( - client_request, - service_one, - mocker, - mock_get_service_settings_page_common, - mock_get_email_branding, - no_reply_to_email_addresses, - no_letter_contact_blocks, - single_sms_sender, -): - service_one['email_branding'] = sample_uuid() - service_one['organisation_type'] = '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_request', - service_id=SERVICE_ONE_ID, - _data={'options': 'something_else', 'something_else': 'Homer Simpson'}, - _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: Something else\n\nHomer Simpson\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='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.' - ) - - -def test_submit_email_branding_request_when_something_else_is_only_option_and_textbox_is_empty( - client_request, - service_one, - mock_get_email_branding, -): - service_one['email_branding'] = sample_uuid() - service_one['organisation_type'] = 'local' - - page = client_request.post( - '.email_branding_request', service_id=SERVICE_ONE_ID, - _data={'options': 'something_else', 'something_else': ''}, - _follow_redirects=True, - ) - assert page.h1.text == 'Change email branding' - assert normalize_spaces(page.select_one('.error-message').text) == 'Cannot be empty' - - def test_submit_email_branding_request_when_no_radio_button_is_selected( client_request, service_one, @@ -5412,18 +5333,12 @@ def test_submit_letter_branding_request_redirects_if_from_template_is_set( ) -@pytest.mark.parametrize('branding_type,current_branding', [ - ('email', 'GOV.UK'), ('letter', 'no') -]) -def test_submit_branding_when_something_else_is_only_option( +def test_submit_letter_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, ): mock_create_ticket = mocker.spy(NotifySupportTicket, '__init__') mocker.patch( @@ -5432,7 +5347,7 @@ def test_submit_branding_when_something_else_is_only_option( ) client_request.post( - f'.{branding_type}_branding_request', + '.letter_branding_request', service_id=SERVICE_ONE_ID, _data={ 'something_else': 'Homer Simpson', @@ -5440,10 +5355,10 @@ def test_submit_branding_when_something_else_is_only_option( ) assert ( - 'Current branding: {}\n' + 'Current branding: no\n' 'Branding requested: Something else\n' '\n' - 'Homer Simpson'.format(current_branding) + 'Homer Simpson' ) in mock_create_ticket.call_args_list[0][1]['message'] @@ -5509,7 +5424,11 @@ def test_get_email_branding_govuk_and_nhs_pages( assert normalize_spaces(page.select_one('.page-footer button').text) == 'Use this branding' -def test_get_email_branding_something_else_page(client_request): +def test_get_email_branding_something_else_page(client_request, service_one, mocker): + # expect to have a "NHS" option as well as the + # fallback, so back button goes to choices page + service_one['organisation_type'] = 'nhs_central' + page = client_request.get( 'main.email_branding_something_else', service_id=SERVICE_ONE_ID, @@ -5517,6 +5436,23 @@ def test_get_email_branding_something_else_page(client_request): assert normalize_spaces(page.h1.text) == 'Describe the branding you want' assert page.select_one('textarea')['name'] == ('something_else') assert normalize_spaces(page.select_one('.page-footer button').text) == 'Request new branding' + assert page.select_one('.govuk-back-link')['href'] == url_for( + 'main.email_branding_request', service_id=SERVICE_ONE_ID, + ) + + +def test_get_email_branding_something_else_page_is_only_option(client_request, mocker, service_one): + # should only have a "something else" option + # so back button goes back to settings page + service_one['organisation_type'] = 'other' + + page = client_request.get( + 'main.email_branding_something_else', + service_id=SERVICE_ONE_ID, + ) + assert page.select_one('.govuk-back-link')['href'] == url_for( + 'main.service_settings', service_id=SERVICE_ONE_ID, + ) @pytest.mark.parametrize('endpoint', [ @@ -5789,11 +5725,9 @@ 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, ): service_one['permissions'].append('letter') page = client_request.get( @@ -5802,6 +5736,37 @@ def test_service_settings_links_to_branding_request_page_for_letters( assert len(page.find_all('a', attrs={'href': f'/services/{SERVICE_ONE_ID}/service-settings/letter-branding'})) == 1 +@pytest.mark.parametrize('single_branding_option, expected_href', [ + (True, f'/services/{SERVICE_ONE_ID}/service-settings/email-branding/something-else'), + (False, f'/services/{SERVICE_ONE_ID}/service-settings/email-branding'), +]) +def test_service_settings_links_to_branding_request_page_for_emails( + mocker, + service_one, + client_request, + no_reply_to_email_addresses, + no_letter_contact_blocks, + single_sms_sender, + single_branding_option, + expected_href, +): + service_one['permissions'].append('letter') + + if single_branding_option: + # should only have a "something else" option + # so we go straight to that form + service_one['organisation_type'] = 'other' + else: + # expect to have a "NHS" option as well as the + # fallback one, so ask user to choose + service_one['organisation_type'] = 'nhs_central' + + page = client_request.get( + '.service_settings', service_id=SERVICE_ONE_ID + ) + assert len(page.find_all('a', attrs={'href': expected_href})) == 1 + + def test_show_service_data_retention( client_request, platform_admin_user, From c22d3412fad4b74490f85365317dbe0777480bd6 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Mon, 7 Mar 2022 10:22:52 +0000 Subject: [PATCH 2/2] Remove redundant fixtures and test setup In response to [^1] and [^2]. [^1]: https://github.com/alphagov/notifications-admin/pull/4180/files#r819673624 [^2]: https://github.com/alphagov/notifications-admin/pull/4180/files#r819673799 --- tests/app/main/views/test_service_settings.py | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index 08b1d2d27..255ff2e3e 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -5424,7 +5424,7 @@ def test_get_email_branding_govuk_and_nhs_pages( assert normalize_spaces(page.select_one('.page-footer button').text) == 'Use this branding' -def test_get_email_branding_something_else_page(client_request, service_one, mocker): +def test_get_email_branding_something_else_page(client_request, service_one): # expect to have a "NHS" option as well as the # fallback, so back button goes to choices page service_one['organisation_type'] = 'nhs_central' @@ -5441,7 +5441,7 @@ def test_get_email_branding_something_else_page(client_request, service_one, moc ) -def test_get_email_branding_something_else_page_is_only_option(client_request, mocker, service_one): +def test_get_email_branding_something_else_page_is_only_option(client_request, service_one): # should only have a "something else" option # so back button goes back to settings page service_one['organisation_type'] = 'other' @@ -5741,17 +5741,13 @@ def test_service_settings_links_to_branding_request_page_for_letters( (False, f'/services/{SERVICE_ONE_ID}/service-settings/email-branding'), ]) def test_service_settings_links_to_branding_request_page_for_emails( - mocker, service_one, client_request, no_reply_to_email_addresses, - no_letter_contact_blocks, single_sms_sender, single_branding_option, expected_href, ): - service_one['permissions'].append('letter') - if single_branding_option: # should only have a "something else" option # so we go straight to that form