Merge pull request #4180 from alphagov/dedup-something-else-181415991

Link directly to email branding "something else"

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.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):
+    # 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,