Merge pull request #3268 from alphagov/proper-form-for-letter-branding

Parametrise branding request flow so it serves both email and letter branding

app/main/forms.py

@@ -1392,35 +1392,46 @@ class LinkOrganisationsForm(StripWhitespaceForm):
     )
 
 
-class BrandingOptionsEmail(StripWhitespaceForm):
+class BrandingOptions(StripWhitespaceForm):
 
     FALLBACK_OPTION_VALUE = 'something_else'
     FALLBACK_OPTION = (FALLBACK_OPTION_VALUE, 'Something else')
 
-    options = RadioField('Choose your new email branding')
+    options = RadioField('Choose your new branding')
     something_else = TextAreaField('Describe the branding you want')
 
-    def __init__(self, service, *args, **kwargs):
+    def __init__(self, service, *args, branding_type="email", **kwargs):
         super().__init__(*args, **kwargs)
-        self.options.choices = tuple(self.get_available_choices(service))
+        self.options.choices = tuple(self.get_available_choices(service, branding_type))
+        self.options.label.text = 'Choose your new {} branding'.format(branding_type)
         if self.something_else_is_only_option:
             self.options.data = self.FALLBACK_OPTION_VALUE
 
     @staticmethod
-    def get_available_choices(service):
+    def get_available_choices(service, branding_type):
+        if branding_type == "email":
+            organisation_branding_id = service.organisation.email_branding_id if service.organisation else None
+            service_branding_id = service.email_branding_id
+            service_branding_name = service.email_branding_name
+        elif branding_type == "letter":
+            organisation_branding_id = service.organisation.letter_branding_id if service.organisation else None
+            service_branding_id = service.letter_branding_id
+            service_branding_name = service.letter_branding_name
 
         if (
-            service.organisation_type == Organisation.TYPE_CENTRAL and
-            service.organisation.email_branding_id is None and
-            service.email_branding_id is not None
+            service.organisation_type == Organisation.TYPE_CENTRAL
+            and organisation_branding_id is None
+            and service_branding_id is not None
+            and branding_type == "email"
         ):
             yield ('govuk', 'GOV.UK')
 
         if (
-            service.organisation_type == Organisation.TYPE_CENTRAL and
-            service.organisation and
-            service.organisation.email_branding_id is None and
-            service.email_branding_name.lower() != 'GOV.UK and {}'.format(service.organisation.name).lower()
+            service.organisation_type == Organisation.TYPE_CENTRAL
+            and service.organisation
+            and organisation_branding_id is None
+            and service_branding_name.lower() != 'GOV.UK and {}'.format(service.organisation.name).lower()
+            and branding_type == "email"
         ):
             yield ('govuk_and_org', 'GOV.UK and {}'.format(service.organisation.name))
 
@@ -1429,24 +1440,26 @@ class BrandingOptionsEmail(StripWhitespaceForm):
                 Organisation.TYPE_NHS_CENTRAL,
                 Organisation.TYPE_NHS_LOCAL,
                 Organisation.TYPE_NHS_GP,
-            } and service.email_branding_name != 'NHS'
+            }
+            and service_branding_name != 'NHS'
         ):
             yield ('nhs', 'NHS')
 
         if (
-            service.organisation and
-            service.organisation_type not in {
+            service.organisation
+            and service.organisation_type not in {
                 Organisation.TYPE_NHS_LOCAL,
                 Organisation.TYPE_NHS_CENTRAL,
                 Organisation.TYPE_NHS_GP,
-            } and (
-                service.email_branding_id is None or
-                service.email_branding_id != service.organisation.email_branding_id
+            }
+            and (
+                service_branding_id is None
+                or service_branding_id != organisation_branding_id
             )
         ):
             yield ('organisation', service.organisation.name)
 
-        yield BrandingOptionsEmail.FALLBACK_OPTION
+        yield BrandingOptions.FALLBACK_OPTION
 
     @property
     def something_else_is_only_option(self):
@@ -1454,8 +1467,8 @@ class BrandingOptionsEmail(StripWhitespaceForm):
 
     def validate_something_else(self, field):
         if (
-            self.something_else_is_only_option or
-            self.options.data == self.FALLBACK_OPTION_VALUE
+            self.something_else_is_only_option
+            or self.options.data == self.FALLBACK_OPTION_VALUE
         ) and not field.data:
             raise ValidationError('Cannot be empty')
 

app/main/views/service_settings.py

@@ -30,7 +30,7 @@ from app import (
 from app.extensions import zendesk_client
 from app.main import main
 from app.main.forms import (
-    BrandingOptionsEmail,
+    BrandingOptions,
     ConfirmPasswordForm,
     EstimateUsageForm,
     FreeSMSAllowance,
@@ -1001,15 +1001,6 @@ def service_preview_letter_branding(service_id):
     )
 
 
-@main.route("/services/<uuid:service_id>/service-settings/request-letter-branding", methods=['GET', 'POST'])
-@user_has_permissions('manage_service', 'manage_templates')
-def request_letter_branding(service_id):
-    return render_template(
-        'views/service-settings/request-letter-branding.html',
-        from_template=request.args.get('from_template'),
-    )
-
-
 @main.route("/services/<uuid:service_id>/service-settings/link-service-to-organisation", methods=['GET', 'POST'])
 @user_is_platform_admin
 def link_service_to_organisation(service_id):
@@ -1038,15 +1029,18 @@ def link_service_to_organisation(service_id):
     )
 
 
-@main.route("/services/<uuid:service_id>/branding-request/email", methods=['GET', 'POST'])
+@main.route("/services/<uuid:service_id>/branding-request/<branding_type>", methods=['GET', 'POST'])
 @user_has_permissions('manage_service')
-def branding_request(service_id):
-
-    form = BrandingOptionsEmail(current_service)
-
+def branding_request(service_id, branding_type):
+    form = BrandingOptions(current_service, branding_type=branding_type)
+    from_template = request.args.get('from_template')
+    if branding_type == "email":
+        branding_name = current_service.email_branding_name
+    elif branding_type == "letter":
+        branding_name = current_service.letter_branding_name
     if form.validate_on_submit():
         zendesk_client.create_ticket(
-            subject='Email branding request - {}'.format(current_service.name),
+            subject='{} branding request - {}'.format(branding_type.capitalize(), current_service.name),
             message=(
                 'Organisation: {organisation}\n'
                 'Service: {service_name}\n'
@@ -1061,7 +1055,7 @@ def branding_request(service_id):
                 organisation=current_service.organisation.as_info_for_branding_request(current_user.email_domain),
                 service_name=current_service.name,
                 dashboard_url=url_for('main.service_dashboard', service_id=current_service.id, _external=True),
-                current_branding=current_service.email_branding_name,
+                current_branding=branding_name,
                 branding_requested=dict(form.options.choices)[form.options.data],
                 new_paragraph='\n\n' if form.something_else.data else '',
                 detail=form.something_else.data or ''
@@ -1071,16 +1065,20 @@ def branding_request(service_id):
             user_name=current_user.name,
             tags=['notify_action', 'notify_branding'],
         )
-
         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=service_id))
+        return redirect(url_for(
+            '.view_template', service_id=current_service.id, template_id=from_template
+        ) if from_template else url_for('.service_settings', service_id=current_service.id))
 
     return render_template(
-        'views/service-settings/branding/email-options.html',
+        'views/service-settings/branding/branding-options.html',
         form=form,
+        branding_type=branding_type,
+        branding_name=branding_name,
+        from_template=from_template
     )
 
 

app/models/service.py

@@ -436,6 +436,12 @@ class Service(JSONModel):
             return 'GOV.UK'
         return self.email_branding['name']
 
+    @cached_property
+    def letter_branding_name(self):
+        if self.letter_branding is None:
+            return 'no'
+        return self.letter_branding['name']
+
     @property
     def needs_to_change_email_branding(self):
         return self.email_branding_id is None and self.organisation_type != Organisation.TYPE_CENTRAL

app/navigation.py

@@ -238,7 +238,6 @@ class HeaderNavigation(Navigation):
         'registration_continue',
         'remove_user_from_organisation',
         'remove_user_from_service',
-        'request_letter_branding',
         'request_to_go_live',
         'resend_email_link',
         'resend_email_verification',
@@ -405,7 +404,6 @@ class MainNavigation(Navigation):
             'branding_request',
             'estimate_usage',
             'link_service_to_organisation',
-            'request_letter_branding',
             'request_to_go_live',
             'service_add_email_reply_to',
             'service_add_letter_contact',
@@ -805,7 +803,6 @@ class CaseworkNavigation(Navigation):
         'registration_continue',
         'remove_user_from_organisation',
         'remove_user_from_service',
-        'request_letter_branding',
         'request_to_go_live',
         'resend_email_link',
         'resend_email_verification',
@@ -1085,7 +1082,6 @@ class OrgNavigation(Navigation):
         'register_from_org_invite',
         'registration_continue',
         'remove_user_from_service',
-        'request_letter_branding',
         'request_to_go_live',
         'resend_email_link',
         'resend_email_verification',

app/templates/views/service-settings.html

@@ -105,7 +105,7 @@
           {{ text_field(current_service.email_branding_name) }}
           {{ edit_field(
             'Change',
-            url_for('.branding_request', service_id=current_service.id),
+            url_for('.branding_request', service_id=current_service.id, branding_type="email"),
             permissions=['manage_service'],
           )}}
         {% endcall %}
@@ -241,7 +241,7 @@
           {{ optional_text_field(current_service.letter_branding.name) }}
           {{ edit_field(
             'Change',
-            url_for('.request_letter_branding', service_id=current_service.id),
+            url_for('.branding_request', service_id=current_service.id, branding_type="letter"),
             permissions=['manage_service']
           )}}
         {% endcall %}

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

@@ -7,21 +7,21 @@
 {% from "components/form.html" import form_wrapper %}
 
 {% block service_page_title %}
-  Change email branding
+  Change {{ branding_type }} branding
 {% endblock %}
 
 {% block maincolumn_content %}
 
   {{ page_header(
-    'Change email branding',
-    back_link=url_for('main.service_settings', service_id=current_service.id)
+    'Change {} branding'.format(branding_type),
+    back_link=url_for('.view_template', service_id=current_service.id, template_id=from_template) if from_template else url_for('.service_settings', service_id=current_service.id)
   ) }}
 
   <p>
-    Your emails currently have {{ current_service.email_branding_name }} branding.
+    Your {{ branding_type }}s currently have {{ branding_name }} branding.
   </p>
 
-  {% if current_service.needs_to_change_email_branding %}
+  {% if current_service.needs_to_change_email_branding and branding_type == "email" %}
     <p>
       You should be using your own branding instead. We can help you to set this up.
     </p>

app/templates/views/service-settings/request-letter-branding.html

@@ -1,39 +0,0 @@
-{% extends "withnav_template.html" %}
-{% from "components/radios.html" import radios %}
-{% from "components/page-header.html" import page_header %}
-{% from "components/page-footer.html" import page_footer %}
-
-{% block service_page_title %}
-  Letter branding
-{% endblock %}
-
-{% block maincolumn_content %}
-
-  {{ page_header(
-    'Letter branding',
-    back_link=url_for('.view_template', service_id=current_service.id, template_id=from_template) if from_template else url_for('.service_settings', service_id=current_service.id)
-  ) }}
-
-  <div class="grid-row">
-    <div class="column-three-quarters">
-      {% if current_service.letter_branding_id %}
-        <p>
-          Your letters have the {{ current_service.letter_branding.name }} logo.
-        </p>
-        <p>
-          <a href="{{ url_for('main.feedback', ticket_type='ask-question-give-feedback', body='letter-branding') }}">Contact us</a>
-          if you want to use a different logo.
-        </p>
-      {% else %}
-        <p>
-          Your letters do not have a logo.
-        </p>
-        <p>
-          <a href="{{ url_for('main.feedback', ticket_type='ask-question-give-feedback', body='letter-branding') }}">Contact us</a>
-          if you want to add your organisation’s logo.
-        </p>
-      {% endif %}
-    </div>
-  </div>
-
-{% endblock %}

app/templates/views/templates/_template.html

@@ -51,7 +51,7 @@
 <div class="column-whole template-container">
   {% if current_user.has_permissions('manage_templates') and template.template_type == 'letter' %}
     {% if not current_service.letter_branding_id %}
-      <a href="{{ url_for(".request_letter_branding", service_id=current_service.id, from_template=template.id) }}" class="edit-template-link-letter-branding">Add logo</a>
+      <a href="{{ url_for(".branding_request", service_id=current_service.id, branding_type="letter", from_template=template.id) }}" class="edit-template-link-letter-branding">Add logo</a>
     {% endif %}
     <a href="{{ url_for(".edit_template_postage", service_id=current_service.id, template_id=template.id) }}" class="edit-template-link-letter-postage">Change</a>
     <a href="{{ url_for(".edit_service_template", service_id=current_service.id, template_id=template.id) }}" class="edit-template-link-letter-body">Edit</a>

tests/app/main/views/test_service_settings.py

@@ -3037,58 +3037,6 @@ def test_set_letter_contact_block_has_max_10_lines(
     assert error_message == 'Contains 11 lines, maximum is 10'
 
 
-@pytest.mark.parametrize('extra_args, expected_partial_url', (
-    (
-        {},
-        partial(url_for, 'main.service_settings')
-    ),
-    (
-        {'from_template': FAKE_TEMPLATE_ID},
-        partial(url_for, 'main.view_template', template_id=FAKE_TEMPLATE_ID)
-    ),
-))
-def test_request_letter_branding(
-    client_request,
-    mock_get_letter_branding_by_id,
-    extra_args,
-    expected_partial_url,
-):
-    request_page = client_request.get(
-        'main.request_letter_branding',
-        service_id=SERVICE_ONE_ID,
-        **extra_args
-    )
-    assert request_page.select_one('main p').text.strip() == 'Your letters do not have a logo.'
-    back_link_href = request_page.select('main a')[0]['href']
-    link_href = request_page.select('main a')[1]['href']
-    assert link_href == url_for(
-        'main.feedback',
-        ticket_type='ask-question-give-feedback',
-        body='letter-branding',
-    )
-    feedback_page = client_request.get_url(link_href)
-    assert feedback_page.select_one('textarea').text.strip() == (
-        'I would like my own logo on my letter templates.'
-    )
-    assert back_link_href == expected_partial_url(service_id=SERVICE_ONE_ID)
-
-
-def test_request_letter_branding_if_already_have_branding(
-    client_request,
-    mock_get_letter_branding_by_id,
-    service_one,
-):
-    service_one['letter_branding'] = uuid4()
-
-    request_page = client_request.get(
-        'main.request_letter_branding',
-        service_id=SERVICE_ONE_ID,
-    )
-
-    mock_get_letter_branding_by_id.assert_called_once_with(service_one['letter_branding'])
-    assert request_page.select_one('main p').text.strip() == 'Your letters have the HM Government logo.'
-
-
 def test_service_set_letter_branding_platform_admin_only(
     client_request,
 ):
@@ -4452,6 +4400,7 @@ def test_update_service_organisation_does_not_update_if_same_value(
     mock_update_service_organisation.called is False
 
 
+@pytest.mark.parametrize('branding_type', ['email', 'letter'])
 @pytest.mark.parametrize('organisation_type, expected_options', (
     ('central', None),
     ('local', None),
@@ -4470,15 +4419,17 @@ def test_update_service_organisation_does_not_update_if_same_value(
     ('emergency_service', None),
     ('other', None),
 ))
-def test_show_email_branding_request_page_when_no_email_branding_is_set(
+def test_show_branding_request_page_when_no_branding_is_set(
     mocker,
     service_one,
     client_request,
     mock_get_email_branding,
+    mock_get_letter_branding_by_id,
     organisation_type,
     expected_options,
+    branding_type
 ):
-    service_one['email_branding'] = None
+    service_one['{}_branding'.format(branding_type)] = None
     service_one['organisation_type'] = organisation_type
     mocker.patch(
         'app.organisations_client.get_service_organisation',
@@ -4486,10 +4437,11 @@ def test_show_email_branding_request_page_when_no_email_branding_is_set(
     )
 
     page = client_request.get(
-        '.branding_request', service_id=SERVICE_ONE_ID
+        '.branding_request', service_id=SERVICE_ONE_ID, branding_type=branding_type
     )
 
     mock_get_email_branding.assert_not_called()
+    mock_get_letter_branding_by_id.assert_not_called()
 
     if expected_options:
         assert [
@@ -4513,12 +4465,8 @@ def test_show_email_branding_request_page_when_no_email_branding_is_set(
         assert not page.select('.conditional-radios-panel')
 
 
+@pytest.mark.parametrize('branding_type', ['email', 'letter'])
 @pytest.mark.parametrize('organisation_type, expected_options', (
-    ('central', [
-        ('govuk_and_org', 'GOV.UK and Test Organisation'),
-        ('organisation', 'Test Organisation'),
-        ('something_else', 'Something else'),
-    ]),
     ('local', [
         ('organisation', 'Test Organisation'),
         ('something_else', 'Something else'),
@@ -4544,15 +4492,17 @@ def test_show_email_branding_request_page_when_no_email_branding_is_set(
         ('something_else', 'Something else'),
     ]),
 ))
-def test_show_email_branding_request_page_when_no_email_branding_is_set_but_organisation_exists(
+def test_show_branding_request_page_when_no_branding_is_set_but_organisation_exists(
     mocker,
     service_one,
     client_request,
     mock_get_email_branding,
+    mock_get_letter_branding_by_id,
     organisation_type,
     expected_options,
+    branding_type
 ):
-    service_one['email_branding'] = None
+    service_one['{}_branding'.format(branding_type)] = None
     service_one['organisation_type'] = organisation_type
     mocker.patch(
         'app.organisations_client.get_service_organisation',
@@ -4560,10 +4510,55 @@ def test_show_email_branding_request_page_when_no_email_branding_is_set_but_orga
     )
 
     page = client_request.get(
-        '.branding_request', service_id=SERVICE_ONE_ID
+        '.branding_request', service_id=SERVICE_ONE_ID, branding_type=branding_type
     )
 
     mock_get_email_branding.assert_not_called()
+    mock_get_letter_branding_by_id.assert_not_called()
+
+    assert [
+        (
+            radio['value'],
+            page.select_one('label[for={}]'.format(radio['id'])).text.strip()
+        )
+        for radio in page.select('input[type=radio]')
+    ] == expected_options
+
+
+@pytest.mark.parametrize('organisation_type, expected_options, branding_type', (
+    ('central', [
+        ('govuk_and_org', 'GOV.UK and Test Organisation'),
+        ('organisation', 'Test Organisation'),
+        ('something_else', 'Something else'),
+    ], 'email'),
+    ('central', [
+        ('organisation', 'Test Organisation'),
+        ('something_else', 'Something else'),
+    ], 'letter'),
+))
+def test_show_branding_request_page_when_no_branding_is_set_but_organisation_exists_central_org(
+    mocker,
+    service_one,
+    client_request,
+    mock_get_email_branding,
+    mock_get_letter_branding_by_id,
+    organisation_type,
+    expected_options,
+    branding_type
+):
+    service_one['{}_branding'.format(branding_type)] = None
+    service_one['organisation_type'] = organisation_type
+    mocker.patch(
+        'app.organisations_client.get_service_organisation',
+        return_value=organisation_json(organisation_type=organisation_type),
+    )
+
+    page = client_request.get(
+        '.branding_request', service_id=SERVICE_ONE_ID, branding_type=branding_type
+    )
+
+    mock_get_email_branding.assert_not_called()
+    mock_get_letter_branding_by_id.assert_not_called()
 
     assert [
         (
@@ -4588,7 +4583,7 @@ def test_show_email_branding_request_page_when_email_branding_is_set(
     )
 
     page = client_request.get(
-        '.branding_request', service_id=SERVICE_ONE_ID
+        '.branding_request', service_id=SERVICE_ONE_ID, branding_type="email"
     )
     assert [
         (
@@ -4604,21 +4599,91 @@ def test_show_email_branding_request_page_when_email_branding_is_set(
     ]
 
 
-def test_show_email_branding_request_page_when_email_branding_is_same_as_org(
+def test_show_letter_branding_request_page_when_letter_branding_is_set(
+    mocker,
+    service_one,
+    client_request,
+    mock_get_letter_branding_by_id,
+    active_user_with_permissions,
+):
+    service_one['letter_branding'] = sample_uuid()
+    mocker.patch(
+        'app.organisations_client.get_service_organisation',
+        return_value=organisation_json(),
+    )
+
+    page = client_request.get(
+        '.branding_request', service_id=SERVICE_ONE_ID, branding_type="letter"
+    )
+    assert [
+        (
+            radio['value'],
+            page.select_one('label[for={}]'.format(radio['id'])).text.strip()
+        )
+        for radio in page.select('input[type=radio]')
+    ] == [
+        ('organisation', 'Test Organisation'),
+        ('something_else', 'Something else'),
+    ]
+
+
+@pytest.mark.parametrize('branding_type', ['email', 'letter'])
+@pytest.mark.parametrize('from_template,back_link_url', [
+    (None, '/services/{}/service-settings'.format(SERVICE_ONE_ID),),
+    (TEMPLATE_ONE_ID, '/services/{}/templates/{}'.format(SERVICE_ONE_ID, TEMPLATE_ONE_ID),)
+])
+def test_back_link_on_branding_request_page(
     mocker,
     service_one,
     client_request,
     mock_get_email_branding,
+    mock_get_letter_branding_by_id,
     active_user_with_permissions,
+    from_template,
+    back_link_url,
+    branding_type,
 ):
-    service_one['email_branding'] = sample_uuid()
     mocker.patch(
         'app.organisations_client.get_service_organisation',
-        return_value=organisation_json(email_branding_id=service_one['email_branding']),
+        return_value=organisation_json(),
     )
+    if from_template:
+        page = client_request.get(
+            '.branding_request', service_id=SERVICE_ONE_ID, branding_type=branding_type, from_template=from_template
+        )
+    else:
+        page = client_request.get(
+            '.branding_request', service_id=SERVICE_ONE_ID, branding_type=branding_type
+        )
+
+    back_link = page.select('a[class=govuk-back-link]')
+    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(
+    mocker,
+    service_one,
+    client_request,
+    mock_get_email_branding,
+    mock_get_letter_branding_by_id,
+    active_user_with_permissions,
+    branding_type
+):
+    service_one['{}_branding'.format(branding_type)] = sample_uuid()
+    if branding_type == 'email':
+        mocker.patch(
+            'app.organisations_client.get_service_organisation',
+            return_value=organisation_json(email_branding_id=service_one['email_branding']),
+        )
+    else:
+        mocker.patch(
+            'app.organisations_client.get_service_organisation',
+            return_value=organisation_json(letter_branding_id=service_one['letter_branding']),
+        )
 
     page = client_request.get(
-        '.branding_request', service_id=SERVICE_ONE_ID
+        '.branding_request', service_id=SERVICE_ONE_ID, branding_type=branding_type
     )
 
     # Central government organisations who have their own default
@@ -4682,6 +4747,7 @@ def test_submit_email_branding_request(
     expected_organisation,
 ):
     service_one['email_branding'] = sample_uuid()
+
     mocker.patch(
         'app.organisations_client.get_service_organisation',
         return_value=organisation_json(name=org_name) if org_name else None,
@@ -4693,7 +4759,7 @@ def test_submit_email_branding_request(
     )
 
     page = client_request.post(
-        '.branding_request', service_id=SERVICE_ONE_ID,
+        '.branding_request', service_id=SERVICE_ONE_ID, branding_type="email",
         _data=data,
         _follow_redirects=True,
     )
@@ -4720,12 +4786,135 @@ def test_submit_email_branding_request(
     )
 
 
-def test_submit_email_branding_when_something_else_is_only_option(
+@pytest.mark.parametrize('data, requested_branding', (
+    (
+        {
+            'options': 'something_else',
+            'something_else': 'Homer Simpson'
+        },
+        'Something else\n\nHomer Simpson'
+    ),
+    pytest.param(
+        {
+            'options': 'something_else',
+        },
+        '[Missing details]',
+        marks=pytest.mark.xfail(raises=AssertionError),
+    ),
+    pytest.param(
+        {'options': 'foo'},
+        'Nope',
+        marks=pytest.mark.xfail(raises=AssertionError),
+    ),
+))
+@pytest.mark.parametrize('org_name, expected_organisation', (
+    (None, 'Can’t tell (domain is user.gov.uk)'),
+    ('Test Organisation', 'Test Organisation'),
+))
+def test_submit_letter_branding_request(
+    client_request,
+    service_one,
+    mocker,
+    data,
+    requested_branding,
+    mock_get_service_settings_page_common,
+    mock_get_letter_branding_by_id,
+    no_reply_to_email_addresses,
+    no_letter_contact_blocks,
+    single_sms_sender,
+    org_name,
+    expected_organisation,
+):
+    service_one['letter_branding'] = sample_uuid()
+
+    mocker.patch(
+        'app.organisations_client.get_service_organisation',
+        return_value=organisation_json(name=org_name) if org_name else None,
+    )
+
+    zendesk = mocker.patch(
+        'app.main.views.service_settings.zendesk_client.create_ticket',
+        autospec=True,
+    )
+
+    page = client_request.post(
+        '.branding_request', service_id=SERVICE_ONE_ID, branding_type="letter",
+        _data=data,
+        _follow_redirects=True,
+    )
+
+    zendesk.assert_called_once_with(
+        message='\n'.join([
+            'Organisation: {}',
+            'Service: service one',
+            'http://localhost/services/596364a0-858e-42c8-9062-a8fe822260eb',
+            '',
+            '---',
+            'Current branding: HM Government',
+            'Branding requested: {}\n',
+        ]).format(expected_organisation, requested_branding),
+        subject='Letter branding request - service one',
+        ticket_type='question',
+        user_email='test@user.gov.uk',
+        user_name='Test User',
+        tags=['notify_action', 'notify_branding'],
+    )
+    assert normalize_spaces(page.select_one('.banner-default').text) == (
+        'Thanks for your branding request. We’ll get back to you '
+        'within one working day.'
+    )
+
+
+@pytest.mark.parametrize('branding_type', ['email', 'letter'])
+@pytest.mark.parametrize('from_template', [
+    None,
+    TEMPLATE_ONE_ID
+])
+def test_submit_letter_branding_request_redirects_if_from_template_is_set(
+    client_request,
+    service_one,
+    mocker,
+    mock_get_service_settings_page_common,
+    mock_get_letter_branding_by_id,
+    no_reply_to_email_addresses,
+    no_letter_contact_blocks,
+    single_sms_sender,
+    from_template,
+    branding_type,
+
+):
+    mocker.patch('app.organisations_client.get_service_organisation', return_value=None)
+    mocker.patch('app.main.views.service_settings.zendesk_client.create_ticket', autospec=True)
+    data = {'options': 'something_else', 'something_else': 'Homer Simpson'}
+
+    if from_template:
+        client_request.post(
+            '.branding_request', service_id=SERVICE_ONE_ID, branding_type="letter", from_template=from_template,
+            _data=data,
+            _expected_redirect=url_for(
+                'main.view_template', service_id=SERVICE_ONE_ID, template_id=from_template, _external=True
+            )
+        )
+    else:
+        client_request.post(
+            '.branding_request', service_id=SERVICE_ONE_ID, branding_type="letter",
+            _data=data,
+            _expected_redirect=url_for('main.service_settings', service_id=SERVICE_ONE_ID, _external=True)
+        )
+
+
+@pytest.mark.parametrize('branding_type,current_branding', [
+    ('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,
 ):
     mocker.patch(
         'app.organisations_client.get_service_organisation',
@@ -4739,20 +4928,39 @@ def test_submit_email_branding_when_something_else_is_only_option(
 
     client_request.post(
         '.branding_request',
-        service_id=SERVICE_ONE_ID,
+        service_id=SERVICE_ONE_ID, branding_type=branding_type,
         _data={
             'something_else': 'Homer Simpson',
         },
     )
 
     assert (
-        'Current branding: GOV.UK\n'
+        'Current branding: {}\n'
         'Branding requested: Something else\n'
         '\n'
-        'Homer Simpson'
+        'Homer Simpson'.format(current_branding)
     ) in zendesk.call_args_list[0][1]['message']
 
 
+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,
+    mock_get_service_organisation,
+):
+    service_one["restricted"] is False
+    service_one['permissions'].append('letter')
+    page = client_request.get(
+        '.service_settings', service_id=SERVICE_ONE_ID
+    )
+    assert len(page.findAll('a', attrs={'href': '/services/{}/branding-request/letter'.format(SERVICE_ONE_ID)})) == 1
+
+
 def test_show_service_data_retention(
         platform_admin_client,
         service_one,

tests/app/main/views/test_templates.py

@@ -437,7 +437,10 @@ def test_user_with_only_send_and_view_sees_letter_page(
 @pytest.mark.parametrize('letter_branding, expected_link, expected_link_text', (
     (
         None,
-        partial(url_for, 'main.request_letter_branding', from_template=TEMPLATE_ONE_ID),
+        partial(
+            url_for, 'main.branding_request',
+            service_id=SERVICE_ONE_ID, branding_type="letter", from_template=TEMPLATE_ONE_ID
+        ),
         'Add logo',
     ),
     (