diff --git a/app/main/forms.py b/app/main/forms.py index 25d9c1077..5eac807b5 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -2255,10 +2255,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 3db150c68..08a1ebc1a 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): + # 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, 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,33 @@ 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( + service_one, + client_request, + no_reply_to_email_addresses, + single_sms_sender, + single_branding_option, + expected_href, +): + 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,