Merge email, text message + letter templates pages

Right now we have separate pages for email and text message templates. In the future we will also have a separate page for letter templates. This commit changes Notify to only have one page for all templates. What is the problem? --- The left-hand navigation is getting quite crowded, at 8 items for a service that can send letters. Research suggests that the number of objects an average human can hold in working memory is 7 ± 2 [1]. So we’re at the limit of how many items the navigation should have. In the future we will need to search/sort/filter templates by attributes other than type, for example: - show me the ‘confirmation’ templates - show me the most recently used templates - show me all templates containing the placeholder `((ref_no))` These are hypothetical for now, but these needs (or others) may become real in the future. At this point pre-filtering the list of templates by type would restrict what searches a user could do. So by making this change now we’re in a better position to iterate the design in the future. What’s the change? --- This commit replaces the ‘Email templates’, ‘Text message templates’ and ‘Letter templates’ pages with one page called ‘Templates’. This new templates page shows all the templates for the service, sorted by most recently created first (as before). To add a new template there is a new page with a form asking you what kind of template you want to create. This is necessary because in the past we knew what kind of template you wanted to create based on the kind you were looking at. What’s the impact of this change on new users? --- This change alters the onboarding process slightly. We still want to take people through the empty templates page from the call-to-action on the dashboard because it helps them understand that to send a message using Notify you need a template. But because we don’t have separate pages for emails/text messages we will have to send users through the extra step of choosing what kind of template to create. This is a bit clunkier on first use but: - it still gets the point across - it takes them through the actual flow they will be using to create new templates in the future (ie they’re learning how to use Notify, not just being taken through a special onboarding route) I’m not too worried about this change in terms of the experience for new users. Furthermore, by making it now we get to validate whether it’s causing any problems in the lab research booked for next week. What’s the impact of this change on current services? --- Looking at the top 15 services by number of templates[2], most are using either text messages or emails. So this change would not have a significant impact on these services because the page will not get any longer. In other words we wouldn’t be making it worse for them. Those services who do use both are not using as many templates. The worst-case scenario is SSCS, who have 16 templates, evenly split between email and text messages. So they would go from having 8 templates per page to 16, which is still less than half the number that HMPO or Digital Marketplace are managing. References --- 1. https://en.wikipedia.org/wiki/The_Magical_Number_Seven,_Plus_or_Minus_Two 2. Template usage by service Service name | Template count | Template types ---------------------------------------|----------------|--------------- Her Majesty's Passport Office | 40 | sms Digital Marketplace | 40 | email GovWifi-Staging | 19 | sms GovWifi | 18 | sms Digital Apprenticeship Service | 16 | email SSCS | 16 | both Crown Commercial Service MI Collection | 15 | email Help with Prison Visits | 12 | both Digital Future | 12 | email Export Licensing Service | 11 | email Civil Money Claims | 9 | both DVLA Drivers Medical Service | 9 | sms GOV.UK Notify | 8 | both Manage your benefit overpayments | 8 | both Tax Renewals | 8 | both
2026-07-12 17:34:16 -04:00 · 2017-02-28 12:16:35 +00:00
parent 95748a2eb9
commit 43a922638b
15 changed files with 168 additions and 143 deletions
--- a/app/main/forms.py
+++ b/app/main/forms.py
@@ -558,3 +558,23 @@ class DateFilterForm(Form):
    start_date = DateField("Start Date", [validators.optional()])
    end_date = DateField("End Date", [validators.optional()])
    include_from_test_key = BooleanField("Include test keys", default="checked", false_values={"N"})
+
+
+class ChooseTemplateType(Form):
+
+    template_type = RadioField(
+        'What kind of template do you want to add?',
+        validators=[
+            DataRequired()
+        ]
+    )
+
+    def __init__(self, include_letters=False, *args, **kwargs):
+
+        super().__init__(*args, **kwargs)
+
+        self.template_type.choices = filter(None, [
+            ('email', 'Email'),
+            ('sms', 'Text message'),
+            ('letter', 'Letter') if include_letters else None
+        ])
--- a/app/main/views/send.py
+++ b/app/main/views/send.py
@@ -82,35 +82,6 @@ def get_example_letter_address(key):
    }.get(key, '')


-@main.route("/services/<service_id>/send/<template_type>", methods=['GET'])
-@login_required
-@user_has_permissions('view_activity',
-                      'send_texts',
-                      'send_emails',
-                      'manage_templates',
-                      'manage_api_keys',
-                      admin_override=True, any_=True)
-def choose_template(service_id, template_type):
-    if template_type not in ['email', 'sms', 'letter']:
-        abort(404)
-    if not current_service['can_send_letters'] and template_type == 'letter':
-        abort(403)
-    return render_template(
-        'views/templates/choose.html',
-        templates=[
-            get_template(
-                template,
-                current_service,
-                letter_preview_url=url_for('.view_template', service_id=service_id, template_id=template['id']),
-            )
-            for template in service_api_client.get_service_templates(service_id)['data']
-            if template['template_type'] == template_type
-        ],
-        template_type=template_type,
-        page_heading=get_page_headings(template_type)
-    )
-
-
@main.route("/services/<service_id>/send/<template_id>/csv", methods=['GET', 'POST'])
@login_required
@user_has_permissions('send_texts', 'send_emails', 'send_letters')
@@ -285,7 +256,7 @@ def _check_messages(service_id, template_type, upload_id, letters_as_pdf=False):
            )
        else:
            back_link = url_for(
-                '.choose_template', service_id=service_id, template_type=template.template_type, **extra_args
+                '.choose_template', service_id=service_id, **extra_args
            )
        choose_time_form = None
    else:
@@ -359,7 +330,7 @@ def check_messages_as_png(service_id, template_type, upload_id):
 def recheck_messages(service_id, template_type, upload_id):

    if not session.get('upload_data'):
-        return redirect(url_for('main.choose_template', service_id=service_id, template_type=template_type))
+        return redirect(url_for('main.choose_template', service_id=service_id))

    return send_messages(service_id, session['upload_data'].get('template_id'))

@@ -412,4 +383,4 @@ def get_check_messages_back_url(service_id, template_type):
        if len(templates) == 1:
            return url_for('.send_test', service_id=service_id, template_id=templates[0]['id'], help=1)

-    return url_for('main.choose_template', service_id=service_id, template_type=template_type)
+    return url_for('main.choose_template', service_id=service_id)
--- a/app/main/views/templates.py
+++ b/app/main/views/templates.py
@@ -12,7 +12,7 @@ from notifications_python_client.errors import HTTPError

 from app.main import main
 from app.utils import user_has_permissions, get_template, png_from_pdf
-from app.main.forms import SMSTemplateForm, EmailTemplateForm, LetterTemplateForm
+from app.main.forms import ChooseTemplateType, SMSTemplateForm, EmailTemplateForm, LetterTemplateForm
 from app.main.views.send import get_example_csv_rows
 from app import service_api_client, current_service, template_statistics_client

@@ -29,6 +29,32 @@ page_headings = {
 }


+@main.route("/services/<service_id>/templates", methods=['GET'])
+@login_required
+@user_has_permissions(
+    'view_activity',
+    'send_texts',
+    'send_emails',
+    'manage_templates',
+    'manage_api_keys',
+    admin_override=True,
+    any_=True,
+)
+def choose_template(service_id):
+    return render_template(
+        'views/templates/choose.html',
+        templates=[
+            get_template(
+                template,
+                current_service,
+                letter_preview_url=url_for('.view_template', service_id=service_id, template_id=template['id']),
+            )
+            for template in service_api_client.get_service_templates(service_id)['data']
+            if should_show_template(template['template_type'])
+        ],
+    )
+
+
@main.route("/services/<service_id>/templates/<template_id>")
@login_required
@user_has_permissions(
@@ -139,6 +165,25 @@ def view_template_version_as_png(service_id, template_id, version):
    ))


+@main.route("/services/<service_id>/templates/add", methods=['GET', 'POST'])
+@login_required
+@user_has_permissions('manage_templates', admin_override=True)
+def add_template_by_type(service_id):
+
+    form = ChooseTemplateType(
+        include_letters=current_service['can_send_letters']
+    )
+
+    if form.validate_on_submit():
+        return redirect(url_for(
+            '.add_service_template',
+            service_id=service_id,
+            template_type=form.template_type.data,
+        ))
+
+    return render_template('views/templates/add.html', form=form)
+
+
@main.route("/services/<service_id>/templates/add-<template_type>", methods=['GET', 'POST'])
@login_required
@user_has_permissions('manage_templates', admin_override=True)
@@ -173,7 +218,7 @@ def add_service_template(service_id, template_type):
                raise e
        else:
            return redirect(
-                url_for('.choose_template', service_id=service_id, template_type=template_type)
+                url_for('.choose_template', service_id=service_id)
            )

    return render_template(
@@ -362,3 +407,10 @@ def get_human_readable_delta(from_time, until_time):
    else:
        days = delta.days
        return '{} day{}'.format(days, '' if days == 1 else 's')
+
+
+def should_show_template(template_type):
+    return (
+        template_type != 'letter' or
+        current_service['can_send_letters']
+    )
--- a/app/templates/main_nav.html
+++ b/app/templates/main_nav.html
@@ -45,11 +45,7 @@
  <ul>
    <li><a href="{{ url_for('.service_dashboard', service_id=current_service.id) }}">Dashboard</a></li>
  {% if current_user.has_permissions(['view_activity', 'manage_templates', 'manage_api_keys'], admin_override=True, any_=True) %}
-    <li><a href="{{ url_for('.choose_template', service_id=current_service.id, template_type='email') }}">Email templates</a></li>
-    <li><a href="{{ url_for('.choose_template', service_id=current_service.id, template_type='sms') }}">Text message templates</a></li>
-    {% if current_service.can_send_letters %}
-      <li><a href="{{ url_for('.choose_template', service_id=current_service.id, template_type='letter') }}">Letter templates</a></li>
-    {% endif %}
+    <li><a href="{{ url_for('.choose_template', service_id=current_service.id) }}">Templates</a></li>
  {% endif %}
  {% if current_user.has_permissions(['manage_users', 'manage_settings'], admin_override=True) %}
    <li><a href="{{ url_for('.manage_users', service_id=current_service.id) }}">Team members</a></li>
--- a/app/templates/views/dashboard/write-first-messages.html
+++ b/app/templates/views/dashboard/write-first-messages.html
@@ -1,10 +1,11 @@
 <h2 class="heading-medium">Get started</h2>

-<nav class="grid-row">
-  <div class="column-half">
-    <a class="pill-separate-item" href="{{ url_for('.choose_template', service_id=current_service.id, template_type='email') }}">Write an email</a>
-  </div>
-  <div class="column-half">
-    <a class="pill-separate-item" href="{{ url_for('.choose_template', service_id=current_service.id, template_type='sms') }}">Write a text message</a>
-  </div>
+<nav>
+  <a class="pill-separate-item" href="{{ url_for('.choose_template', service_id=current_service.id) }}">
+    {% if current_service.can_send_letters %}
+      Write an email, text message or letter
+    {% else %}
+      Write an email or text message
+    {% endif %}
+  </a>
 </nav>
--- a/app/templates/views/templates/add.html
+++ b/app/templates/views/templates/add.html
@@ -0,0 +1,21 @@
+{% from "components/radios.html" import radios %}
+{% from "components/page-footer.html" import page_footer %}
+
+{% extends "withnav_template.html" %}
+
+{% block service_page_title %}
+  Add new template
+{% endblock %}
+
+{% block maincolumn_content %}
+
+    <h1 class="heading-large">Add new template</h1>
+
+    <form method="post">
+      {{ radios(form.template_type) }}
+      {{ page_footer(
+        'Next'
+      ) }}
+    </form>
+
+{% endblock %}
--- a/app/templates/views/templates/choose.html
+++ b/app/templates/views/templates/choose.html
@@ -1,34 +1,45 @@
 {% extends "withnav_template.html" %}

 {% block service_page_title %}
-  {{ page_heading }}
+  Templates
 {% endblock %}

 {% block maincolumn_content %}

  {% if not templates %}

-    <h1 class="heading-large">{{ page_heading }}</h1>
+    <h1 class="heading-large">Templates</h1>

    {% if current_user.has_permissions(permissions=['manage_templates'], any_=True) %}
       <p class="bottom-gutter">
         You need a template before you can send
-         {{ 'emails' if 'email' == template_type else 'text messages' }}
+         {% if current_service.can_send_letters %}
+           emails, text messages or letters
+         {%- else -%}
+           emails or text messages
+         {%- endif %}.
       </p>
-      <a href="{{ url_for('.add_service_template', service_id=current_service.id, template_type=template_type) }}" class="button">Add a new template</a>
+      <a href="{{ url_for('.add_template_by_type', service_id=current_service.id) }}" class="button">Add a new template</a>
    {% else %}
-      <p>You need to ask your service manager to add templates before you can send messages</p>
+      <p>
+        You need to ask your service manager to add templates before you can send
+        {% if current_service.can_send_letters %}
+          emails, text messages or letters
+        {%- else -%}
+          emails or text messages
+        {%- endif %}.
+      </p>
    {% endif %}

  {% else %}

    <div class="grid-row">
      <div class="column-two-thirds">
-        <h1 class="heading-large">{{ page_heading }}</h1>
+        <h1 class="heading-large">Templates</h1>
      </div>
      {% if current_user.has_permissions(permissions=['manage_templates'], admin_override=True) %}
        <div class="column-one-third">
-          <a href="{{ url_for('.add_service_template', service_id=current_service.id, template_type=template_type) }}" class="button align-with-heading">Add new template</a>
+          <a href="{{ url_for('.add_template_by_type', service_id=current_service.id) }}" class="button align-with-heading">Add new template</a>
        </div>
      {% endif %}
    </div>
--- a/app/templates/views/templates/choose_history.html
+++ b/app/templates/views/templates/choose_history.html
@@ -14,6 +14,6 @@
    {% endfor %}
  </div>
  <p>
-    <a href="{{ url_for(".choose_template", service_id=current_service.id, template_type=versions[0].template_type) }}">Back to current templates</a>
+    <a href="{{ url_for(".choose_template", service_id=current_service.id) }}">Back to current templates</a>
  </p>
 {% endblock %}
--- a/app/templates/views/templates/template.html
+++ b/app/templates/views/templates/template.html
@@ -18,10 +18,8 @@
  </div>

  {{ page_footer(
-    secondary_link=url_for('.choose_template', service_id=current_service.id, template_type=template.template_type),
-    secondary_link_text='All {} templates'.format(
-      'email' if 'email' == template.template_type else 'text message' if 'sms' == template.template_type else 'letter'
-    )
+    secondary_link=url_for('.choose_template', service_id=current_service.id),
+    secondary_link_text='All templates'
  ) }}

 {% endblock %}
--- a/tests/app/main/views/test_dashboard.py
+++ b/tests/app/main/views/test_dashboard.py
@@ -324,11 +324,7 @@ def test_menu_send_messages(
        assert url_for(
            'main.choose_template',
            service_id=service_one['id'],
-            template_type='email') in page
-        assert url_for(
-            'main.choose_template',
-            service_id=service_one['id'],
-            template_type='sms') in page
+        ) in page
        assert url_for('main.manage_users', service_id=service_one['id']) in page

        assert url_for('main.service_settings', service_id=service_one['id']) not in page
@@ -358,11 +354,7 @@ def test_menu_manage_service(
        assert url_for(
            'main.choose_template',
            service_id=service_one['id'],
-            template_type='email') in page
-        assert url_for(
-            'main.choose_template',
-            service_id=service_one['id'],
-            template_type='sms') in page
+        ) in page
        assert url_for('main.manage_users', service_id=service_one['id']) in page
        assert url_for('main.service_settings', service_id=service_one['id']) in page

@@ -391,11 +383,7 @@ def test_menu_manage_api_keys(
        assert url_for(
            'main.choose_template',
            service_id=service_one['id'],
-            template_type='email') in page
-        assert url_for(
-            'main.choose_template',
-            service_id=service_one['id'],
-            template_type='sms') in page
+        ) in page
        assert url_for('main.manage_users', service_id=service_one['id']) in page
        assert url_for('main.service_settings', service_id=service_one['id']) not in page

@@ -421,8 +409,7 @@ def test_menu_all_services_for_platform_admin_user(
            service_one,
            [])
        page = resp.get_data(as_text=True)
-        assert url_for('main.choose_template', service_id=service_one['id'], template_type='sms') in page
-        assert url_for('main.choose_template', service_id=service_one['id'], template_type='email') in page
+        assert url_for('main.choose_template', service_id=service_one['id']) in page
        assert url_for('main.manage_users', service_id=service_one['id']) in page
        assert url_for('main.service_settings', service_id=service_one['id']) in page
        assert url_for('main.view_notifications', service_id=service_one['id'], message_type='email') in page
--- a/tests/app/main/views/test_letters.py
+++ b/tests/app/main/views/test_letters.py
@@ -1,11 +1,11 @@
 import pytest
+from bs4 import BeautifulSoup
 from flask import url_for
 from functools import partial

 from tests import service_json

 letters_urls = [
-    partial(url_for, 'main.choose_template', template_type='letter'),
    partial(url_for, 'main.add_service_template', template_type='letter'),
 ]

@@ -49,3 +49,29 @@ def test_letters_lets_in_without_permission(

    assert api_user_active.permissions == {}
    assert response.status_code == 200
+
+
+@pytest.mark.parametrize('can_send_letters, choices', [
+    (True, ['Email', 'Text message', 'Letter']),
+    (False, ['Email', 'Text message'])
+])
+def test_given_option_to_add_letters_if_allowed(
+    logged_in_client,
+    service_one,
+    mocker,
+    can_send_letters,
+    choices,
+):
+    service_one['can_send_letters'] = can_send_letters
+    mocker.patch('app.service_api_client.get_service', return_value={"data": service_one})
+
+    response = logged_in_client.get(url_for('main.add_template_by_type', service_id=service_one['id']))
+
+    assert response.status_code == 200
+    page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser')
+    radios = page.select('input[type=radio]')
+
+    assert len(radios) == len(choices)
+
+    for index, choice in enumerate(choices):
+        assert radios[index].text.strip() == choice
--- a/tests/app/main/views/test_main_nav.py
+++ b/tests/app/main/views/test_main_nav.py
@@ -1,58 +0,0 @@
-from flask import url_for
-from bs4 import BeautifulSoup
-
-from tests.conftest import mock_get_user
-
-
-def test_can_see_letters_if_allowed(
-    logged_in_client,
-    service_one,
-    mocker,
-    mock_get_users_by_service,
-    mock_get_invites_for_service
-):
-    service_one['can_send_letters'] = True
-    mocker.patch('app.service_api_client.get_service', return_value={"data": service_one})
-
-    response = logged_in_client.get(url_for('main.manage_users', service_id=service_one['id']))
-
-    assert response.status_code == 200
-    page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser')
-    assert 'Letter templates' in page.find('nav', class_='navigation').text
-
-
-def test_cant_see_letters_if_not_allowed(
-    logged_in_client,
-    service_one,
-    mocker,
-    mock_get_users_by_service,
-    mock_get_invites_for_service
-):
-    service_one['can_send_letters'] = False
-    mocker.patch('app.service_api_client.get_service', return_value={"data": service_one})
-
-    response = logged_in_client.get(url_for('main.manage_users', service_id=service_one['id']))
-
-    assert response.status_code == 200
-    page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser')
-    assert 'Letter templates' not in page.find('nav', class_='navigation').text
-
-
-def test_can_see_letters_without_edit_permissions(
-    client,
-    mocker,
-    active_user_view_permissions,
-    mock_get_users_by_service,
-    mock_get_invites_for_service,
-    service_one
-):
-    mock_get_user(mocker, user=active_user_view_permissions)
-    service_one['can_send_letters'] = True
-    mocker.patch('app.service_api_client.get_service', return_value={"data": service_one})
-
-    client.login(active_user_view_permissions)
-    response = client.get(url_for('main.manage_users', service_id=service_one['id']))
-
-    assert response.status_code == 200
-    page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser')
-    assert 'Letter templates' in page.find('nav', class_='navigation').text
--- a/tests/app/main/views/test_send.py
+++ b/tests/app/main/views/test_send.py
@@ -596,8 +596,8 @@ def test_route_permissions(
        url_for(
            route,
            service_id=service_one['id'],
-            template_type='sms',
-            template_id=fake_uuid),
+            template_id=fake_uuid
+        ),
        ['send_texts', 'send_emails', 'send_letters'],
        api_user_active,
        service_one)
@@ -660,7 +660,7 @@ def test_route_choose_template_manage_service_permissions(
        url_for(
            'main.choose_template',
            service_id=service_one['id'],
-            template_type='sms'),
+        ),
        ['manage_users', 'manage_templates', 'manage_settings'],
        api_user_active,
        service_one)
@@ -977,8 +977,7 @@ def test_get_check_messages_back_url_returns_to_correct_select_template(client,

    assert get_check_messages_back_url('1234', template_type) == url_for(
        'main.choose_template',
-        service_id='1234',
-        template_type=template_type
+        service_id='1234'
    )


@@ -1011,5 +1010,4 @@ def test_check_messages_back_from_help_handles_unexpected_templates(client, mock
    assert get_check_messages_back_url('1234', 'sms') == url_for(
        'main.choose_template',
        service_id='1234',
-        template_type='sms'
    )
--- a/tests/app/main/views/test_templates.py
+++ b/tests/app/main/views/test_templates.py
@@ -561,7 +561,7 @@ def test_route_permissions_for_choose_template(
        url_for(
            'main.choose_template',
            service_id=service_one['id'],
-            template_type='sms'),
+        ),
        ['view_activity'],
        api_user_active,
        service_one)
--- a/tests/conftest.py
+++ b/tests/conftest.py
@@ -458,10 +458,12 @@ def mock_get_service_templates(mocker):
                service_id, uuid2, "sms_template_two", "sms", "sms template two content"
            ),
            template_json(
-                service_id, uuid3, "email_template_one", "email", "email template one content"
+                service_id, uuid3, "email_template_one", "email", "email template one content",
+                subject='email template one subject',
            ),
            template_json(
-                service_id, uuid4, "email_template_two", "email", "email template two content"
+                service_id, uuid4, "email_template_two", "email", "email template two content",
+                subject='email template two subject',
            )
        ]}