From ca9fe6c8f659fd034ccb90c436fe34b12f7000c9 Mon Sep 17 00:00:00 2001
From: Chris Hill-Scott <me@quis.cc>
Date: Fri, 13 Jul 2018 16:13:24 +0100
Subject: [PATCH 1/2] =?UTF-8?q?Rename=20=E2=80=98Send=20a=20message?=
 =?UTF-8?q?=E2=80=99=20to=20=E2=80=98Templates=E2=80=99=20in=20basic=20vie?=
 =?UTF-8?q?w?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

From Karl:

> Templates – this should be consistent with Admin view. Users may
> switch from Basic to Admin view (or vice versa), they will also
> interact with users who have a different view or permissions to them.
> Neither should have to learn new interfaces and language if possible.

> ‘Send a message’ was a nice, active label – but Notify options aren’t
> usually actions. If we’re going to change this we should be consistent
> across both Admin and Basic views.

> For the same reason, I have rejected ‘see’, ‘search’ and ‘view sent
> messages’. It will be interesting to see in user testing whether users
> read ‘sent messages’ as ‘send messages’.
---
 app/main/views/templates.py                   | 6 ------
 app/templates/main_nav.html                   | 2 +-
 app/templates/views/templates/choose.html     | 2 ++
 tests/app/main/views/test_accept_invite.py    | 2 +-
 tests/app/main/views/test_service_settings.py | 2 +-
 tests/app/main/views/test_templates.py        | 4 ++--
 tests/app/test_navigation.py                  | 2 +-
 7 files changed, 8 insertions(+), 12 deletions(-)

diff --git a/app/main/views/templates.py b/app/main/views/templates.py
index b2e36a94d..1029b5fd8 100644
--- a/app/main/views/templates.py
+++ b/app/main/views/templates.py
@@ -140,14 +140,8 @@ def choose_template(service_id, template_type='all'):
         )
     ]
 
-    if current_user.has_permissions('view_activity'):
-        page_title = 'Templates'
-    else:
-        page_title = 'Choose a template'
-
     return render_template(
         'views/templates/choose.html',
-        page_title=page_title,
         templates=templates_on_page,
         show_search_box=(len(templates_on_page) > 7),
         show_template_nav=has_multiple_template_types and (len(templates) > 2),
diff --git a/app/templates/main_nav.html b/app/templates/main_nav.html
index 5ff2dac9f..72e52cf48 100644
--- a/app/templates/main_nav.html
+++ b/app/templates/main_nav.html
@@ -20,7 +20,7 @@
 {% else %}
 <nav class="navigation">
   <ul>
-    <li><a href="{{ url_for('.choose_template', service_id=current_service.id) }}" {{ casework_navigation.is_selected('send-one-off') }}>Send a message</a></li>
+    <li><a href="{{ url_for('.choose_template', service_id=current_service.id) }}" {{ casework_navigation.is_selected('send-one-off') }}>Templates</a></li>
   </ul>
   <ul>
     <li><a href="{{ url_for('.view_notifications', service_id=current_service.id, status='sending,delivered,failed') }}" {{ casework_navigation.is_selected('sent-messages') }}>Sent messages</a></li>
diff --git a/app/templates/views/templates/choose.html b/app/templates/views/templates/choose.html
index 8a5e98a7b..74f1c989e 100644
--- a/app/templates/views/templates/choose.html
+++ b/app/templates/views/templates/choose.html
@@ -4,6 +4,8 @@
 
 {% extends "withnav_template.html" %}
 
+{% set page_title = 'Templates' %}
+
 {% block service_page_title %}
   {{ page_title }}
 {% endblock %}
diff --git a/tests/app/main/views/test_accept_invite.py b/tests/app/main/views/test_accept_invite.py
index 8e5513322..56c18dcfb 100644
--- a/tests/app/main/views/test_accept_invite.py
+++ b/tests/app/main/views/test_accept_invite.py
@@ -122,7 +122,7 @@ def test_invite_goes_in_session(
 
 @pytest.mark.parametrize('user, landing_page_title', [
     (active_user_with_permissions, 'Dashboard'),
-    (active_caseworking_user, 'Choose a template'),
+    (active_caseworking_user, 'Templates'),
 ])
 def test_accepting_invite_removes_invite_from_session(
     client_request,
diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py
index dcb37e915..5a36514dd 100644
--- a/tests/app/main/views/test_service_settings.py
+++ b/tests/app/main/views/test_service_settings.py
@@ -2459,7 +2459,7 @@ def test_preview_basic_view(
     with client_request.session_transaction() as session:
         assert session['basic'] is True
 
-    assert page.h1.text.strip() == 'Choose a template'
+    assert page.h1.text.strip() == 'Templates'
     page.select('.navigation-service-basic-view-preview')
     assert normalize_spaces(page.select_one('.navigation-service').text) == (
         'service one '
diff --git a/tests/app/main/views/test_templates.py b/tests/app/main/views/test_templates.py
index 56056444d..464a15c22 100644
--- a/tests/app/main/views/test_templates.py
+++ b/tests/app/main/views/test_templates.py
@@ -70,7 +70,7 @@ from tests.conftest import single_letter_contact_block
         ),
         (
             active_caseworking_user,
-            'Choose a template',
+            'Templates',
             {},
             ['Text message', 'Email'],
             [
@@ -82,7 +82,7 @@ from tests.conftest import single_letter_contact_block
         ),
         (
             active_caseworking_user,
-            'Choose a template',
+            'Templates',
             {'template_type': 'email'},
             ['All', 'Text message'],
             ['email_template_one', 'email_template_two'],
diff --git a/tests/app/test_navigation.py b/tests/app/test_navigation.py
index 3cbada0e5..1a260ac50 100644
--- a/tests/app/test_navigation.py
+++ b/tests/app/test_navigation.py
@@ -159,5 +159,5 @@ def test_caseworkers_get_caseworking_navigation(
     )
     page = client_request.get('main.choose_template', service_id=SERVICE_ONE_ID)
     assert normalize_spaces(page.select_one('#content nav').text) == (
-        'Send a message Sent messages'
+        'Templates Sent messages'
     )

From b946a6008ddad0d09a155fae61554073a9efb343 Mon Sep 17 00:00:00 2001
From: Chris Hill-Scott <me@quis.cc>
Date: Fri, 13 Jul 2018 16:21:22 +0100
Subject: [PATCH 2/2] Add H1 to sent messages page
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

> Suggest making the H1 visible here for consistency, but also to make
> it clear to users what they’re looking at.

> This screen is similar to – but not exactly the same as – the
> individual text, email and letter dashboard screens from Admin view,
> so the H1 could help to distinguish it from them for users who may
> have interacted with both.
---
 app/templates/views/notifications.html | 12 +++--
 tests/app/main/views/test_activity.py  | 63 +++++++++++++-------------
 2 files changed, 39 insertions(+), 36 deletions(-)

diff --git a/app/templates/views/notifications.html b/app/templates/views/notifications.html
index be9dc9b60..75986b51a 100644
--- a/app/templates/views/notifications.html
+++ b/app/templates/views/notifications.html
@@ -4,16 +4,20 @@
 {% from "components/page-footer.html" import page_footer %}
 {% from "components/textbox.html" import textbox %}
 
+{% set page_title = (
+  message_count_label(99, message_type, suffix='') | capitalize
+  if current_user.has_permissions('view_activity')
+  else 'Sent messages'
+) %}
+
 {% block service_page_title %}
-  {{ message_count_label(99, message_type, suffix='') | capitalize }}
+  {{ page_title }}
 {% endblock %}
 
 {% block maincolumn_content %}
 
   <h1 class="heading-large">
-    {% if not current_user.has_permissions('view_activity') %}<span class="visually-hidden">{% endif %}
-      {{ message_count_label(99, message_type, suffix='') | capitalize }}
-    {% if not current_user.has_permissions('view_activity') %}</span>{% endif %}
+    {{ page_title }}
   </h1>
   {% if not message_type == "letter" %}
 
diff --git a/tests/app/main/views/test_activity.py b/tests/app/main/views/test_activity.py
index a128445cd..67fb3d9ab 100644
--- a/tests/app/main/views/test_activity.py
+++ b/tests/app/main/views/test_activity.py
@@ -19,14 +19,11 @@ from tests.conftest import (
 )
 
 
-@pytest.mark.parametrize('user', (
-    active_user_view_permissions,
-    active_caseworking_user,
-))
 @pytest.mark.parametrize(
-    "message_type,page_title", [
-        ('email', 'Emails'),
-        ('sms', 'Text messages')
+    "user,extra_args,expected_update_endpoint,page_title", [
+        (active_user_view_permissions, {'message_type': 'email'}, '/email.json', 'Emails'),
+        (active_user_view_permissions, {'message_type': 'sms'}, '/sms.json', 'Text messages'),
+        (active_caseworking_user, {}, '.json', 'Sent messages'),
     ]
 )
 @pytest.mark.parametrize(
@@ -68,11 +65,14 @@ from tests.conftest import (
     ]
 )
 def test_can_show_notifications(
+    client_request,
     logged_in_client,
     service_one,
     mock_get_notifications,
     mock_get_service_statistics,
-    message_type,
+    user,
+    extra_args,
+    expected_update_endpoint,
     page_title,
     status_argument,
     expected_api_call,
@@ -81,33 +81,29 @@ def test_can_show_notifications(
     to_argument,
     expected_to_argument,
     mocker,
-    user,
     fake_uuid,
 ):
-    mocker.patch('app.user_api_client.get_user', return_value=user(fake_uuid))
+    client_request.login(user(fake_uuid))
     if expected_to_argument:
-        response = logged_in_client.post(
-            url_for(
-                'main.view_notifications',
-                service_id=service_one['id'],
-                message_type=message_type,
-                status=status_argument,
-                page=page_argument,
-            ),
-            data={
-                'to': to_argument
-            }
-        )
-    else:
-        response = logged_in_client.get(url_for(
+        page = client_request.post(
             'main.view_notifications',
             service_id=service_one['id'],
-            message_type=message_type,
             status=status_argument,
             page=page_argument,
-        ))
-    assert response.status_code == 200
-    page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser')
+            _data={
+                'to': to_argument
+            },
+            _expected_status=200,
+            **extra_args
+        )
+    else:
+        page = client_request.get(
+            'main.view_notifications',
+            service_id=service_one['id'],
+            status=status_argument,
+            page=page_argument,
+            **extra_args
+        )
     text_of_first_row = page.select('tbody tr')[0].text
     assert '07123456789' in text_of_first_row
     assert (
@@ -120,7 +116,10 @@ def test_can_show_notifications(
     path_to_json = page.find("div", {'data-key': 'notifications'})['data-resource']
 
     url = urlparse(path_to_json)
-    assert url.path == '/services/{}/notifications/{}.json'.format(service_one['id'], message_type)
+    assert url.path == '/services/{}/notifications{}'.format(
+        service_one['id'],
+        expected_update_endpoint,
+    )
     query_dict = parse_qs(url.query)
     if status_argument:
         assert query_dict['status'] == [status_argument]
@@ -133,15 +132,15 @@ def test_can_show_notifications(
         page=expected_page_argument,
         service_id=service_one['id'],
         status=expected_api_call,
-        template_type=[message_type],
+        template_type=list(extra_args.values()),
         to=expected_to_argument,
     )
 
     json_response = logged_in_client.get(url_for(
         'main.get_notifications_as_json',
         service_id=service_one['id'],
-        message_type=message_type,
-        status=status_argument
+        status=status_argument,
+        **extra_args
     ))
     json_content = json.loads(json_response.get_data(as_text=True))
     assert json_content.keys() == {'counts', 'notifications'}