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

@@ -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.branding_type == 'letter':
-            if self.something_else_is_only_option and not field.data:
-                raise ValidationError('Cannot be empty')
-        elif self.branding_type == 'letter':
             if (
                 self.something_else_is_only_option
                 or self.options.data == self.FALLBACK_OPTION_VALUE

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:
+        return redirect(
-            create_email_branding_zendesk_ticket(
+            url_for(
-                form_option_selected=form.options.data,
+                f'.email_branding_{form.options.data}',
-                detail=form.something_else.data,
+                service_id=current_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))
-        else:
-            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/<uuid:service_id>/service-settings/letter-branding", methods=['GET', 'POST'])

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',
             )}}

app/templates/views/service-settings/branding/email-branding-options.html

@@ -34,27 +34,13 @@
     </p>
   {% endif %}
 
-  {% if form.something_else_is_only_option %}
+  {% call form_wrapper() %}
-    {% call form_wrapper() %}
+      {% call select_wrapper(form.options) %}
-        {{ textbox(
+        {% for option in form.options %}
-          form.something_else,
+          {{ radio(option) }}
-          hint='Include links to your brand guidelines or examples of how to use your branding.',
+        {% endfor %}
-          width='1-1',
+      {% endcall %}
-        ) }}
+    {{ page_footer('Continue') }}
-      <p class="form-group">
+  {% endcall %}
-        We’ll email you when your branding is ready, or if we need any more information.
-      </p>
-      {{ 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 %}
 
 {% endblock %}

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({
+  {{ govukBackLink({"href": back_url}) }}
-    "href": url_for('.email_branding_request', service_id=current_service.id)
-  }) }}
 {% endblock %}
 
 {% block maincolumn_content %}

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),
+    ('central', [
-    ('local', None),
+        ('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),
+    ('emergency_service', [
-    ('other', None),
+        ('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 [
-        assert [
+        (
-            (
+            radio['value'],
-                radio['value'],
+            page.select_one('label[for={}]'.format(radio['id'])).text.strip()
-                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 not page.select('.conditional-radios-panel')
+        for radio in page.select('input[type=radio]')
-        assert button_text == 'Request new branding'
+    ] == 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_letter_branding_request_page_when_branding_is_same_as_org(
-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,
     mock_get_service_organisation,
     active_user_with_permissions,
-    branding_type
 ):
-    service_one['{}_branding'.format(branding_type)] = sample_uuid()
+    service_one['letter_branding'] = sample_uuid()
-    if branding_type == 'email':
+    mocker.patch(
-        mocker.patch(
+        'app.organisations_client.get_organisation',
-            'app.organisations_client.get_organisation',
+        return_value=organisation_json(letter_branding_id=service_one['letter_branding']),
-            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']),
-        )
 
     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', [
+def test_submit_letter_branding_when_something_else_is_only_option(
-    ('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,
 ):
     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,