From 42cb4b0d9922baf9da03627ecfedf0fffecd3c98 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 1 Apr 2016 10:42:23 +0100 Subject: [PATCH 1/3] Remove the verbs from the navigation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Because ‘Send text messages’ isn’t very helpful if you’re looking to edit a template. It also helps front-load the navigation, ie ‘Team’ is the first word, rather than the more generic ‘Manage’. --- app/main/views/send.py | 21 ++++----------------- app/templates/main_nav.html | 17 ++++++----------- app/templates/views/manage-users.html | 6 +----- app/templates/views/service-settings.html | 2 +- tests/app/main/views/test_manage_users.py | 10 +++++----- 5 files changed, 17 insertions(+), 39 deletions(-) diff --git a/app/main/views/send.py b/app/main/views/send.py index 6b1fe2618..40622b360 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -29,18 +29,6 @@ from app import (job_api_client, service_api_client) from app.utils import user_has_permissions, get_errors_for_csv -send_messages_page_headings = { - 'email': 'Send emails', - 'sms': 'Send text messages' -} - - -manage_templates_page_headings = { - 'email': 'Email templates', - 'sms': 'Text message templates' -} - - def get_send_button_text(template_type, number_of_messages): if 1 == number_of_messages: return { @@ -55,11 +43,10 @@ def get_send_button_text(template_type, number_of_messages): def get_page_headings(template_type): - # User has manage_service role - if current_user.has_permissions(['send_texts', 'send_emails', 'send_letters']): - return send_messages_page_headings[template_type] - else: - return manage_templates_page_headings[template_type] + return { + 'email': 'Email templates', + 'sms': 'Text message templates' + }[template_type] @main.route("/services//send/", methods=['GET']) diff --git a/app/templates/main_nav.html b/app/templates/main_nav.html index 5cff65c33..7fa2d0330 100644 --- a/app/templates/main_nav.html +++ b/app/templates/main_nav.html @@ -4,15 +4,10 @@ {% if current_user.has_permissions(['view_activity'], admin_override=True) %} {% endif %} - {% if current_user.has_permissions(['send_texts', 'send_emails', 'send_letters']) %} - - {% elif current_user.has_permissions(['view_activity', 'manage_templates','manage_api_keys'], admin_override=True, any_=True) %} + {% if current_user.has_permissions(['view_activity', 'manage_templates', 'manage_api_keys'], admin_override=True, any_=True) %}
  • Text message templates
  • Email templates
    • @@ -20,17 +15,17 @@ {% endif %} {% if current_user.has_permissions(['manage_users', 'manage_settings'], admin_override=True) %} {% elif current_user.has_permissions(['view_activity']) %} {% endif %} {% if current_user.has_permissions(['manage_api_keys']) %} {% endif %} {% if current_user.has_permissions(admin_override=True) %} diff --git a/app/templates/views/manage-users.html b/app/templates/views/manage-users.html index 5f0cc021f..1f88b9631 100644 --- a/app/templates/views/manage-users.html +++ b/app/templates/views/manage-users.html @@ -19,11 +19,7 @@ Manage users – GOV.UK Notify

    - {% if current_user.has_permissions(['manage_users']) %} - Manage team - {% else %} - View team members - {% endif %} + Team members

    {% if current_user.has_permissions(['manage_users']) %} diff --git a/app/templates/views/service-settings.html b/app/templates/views/service-settings.html index eb72fb62e..a444cec9f 100644 --- a/app/templates/views/service-settings.html +++ b/app/templates/views/service-settings.html @@ -7,7 +7,7 @@ {% block maincolumn_content %} -

    Manage settings

    +

    Settings

    {{ browse_list([ { diff --git a/tests/app/main/views/test_manage_users.py b/tests/app/main/views/test_manage_users.py index 92c4f0686..7c264d9e8 100644 --- a/tests/app/main/views/test_manage_users.py +++ b/tests/app/main/views/test_manage_users.py @@ -18,7 +18,7 @@ def test_should_show_overview_page( mocker.patch('app.user_api_client.get_users_for_service', return_value=[active_user_with_permissions]) response = client.get(url_for('main.manage_users', service_id=service['id'])) - assert 'Manage team' in response.get_data(as_text=True) + assert 'Team members' in response.get_data(as_text=True) assert response.status_code == 200 app.user_api_client.get_users_for_service.assert_called_once_with(service_id=service['id']) @@ -158,7 +158,7 @@ def test_invite_user( assert response.status_code == 200 page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') - assert page.h1.string.strip() == 'Manage team' + assert page.h1.string.strip() == 'Team members' flash_banner = page.find('div', class_='banner-default-with-tick').string.strip() assert flash_banner == 'Invite sent to test@example.gov.uk' excpected_permissions = 'manage_api_keys,manage_service,send_messages,view_activity' @@ -203,7 +203,7 @@ def test_manage_users_shows_invited_user(app_, assert response.status_code == 200 page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') - assert page.h1.string.strip() == 'Manage team' + assert page.h1.string.strip() == 'Team members' invites_table = page.find_all('table')[1] cols = invites_table.find_all('td') assert cols[0].text.strip() == 'invited_user@test.gov.uk' @@ -231,7 +231,7 @@ def test_manage_users_does_not_show_accepted_invite(app_, assert response.status_code == 200 page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') - assert page.h1.string.strip() == 'Manage team' + assert page.h1.string.strip() == 'Team members' tables = page.find_all('table') assert len(tables) == 1 assert not page.find(text='invited_user@test.gov.uk') @@ -275,7 +275,7 @@ def test_no_permission_manage_users_page(app_, resp_text = response.get_data(as_text=True) assert url_for('.invite_user', service_id=service_one['id']) not in resp_text assert "Edit permission" not in resp_text - assert "Manage team" not in resp_text + assert "Team members" not in resp_text def test_get_remove_user_from_service(app_, From 95b75d8220bb02ee4055757f964bbfd75f400837 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Sun, 3 Apr 2016 12:10:00 +0100 Subject: [PATCH 2/3] =?UTF-8?q?Add=20a=20hint=20to=20=E2=80=98Template=20n?= =?UTF-8?q?ame=E2=80=99=20text=20input?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We’ve seen lots of users unsure what the ‘template name’ field is for, especially on first use. Is it the subject line of the text message? Will it be who the message comes from? Answer: neither. So this commit adds a hint to make that clearer. --- app/templates/views/edit-email-template.html | 2 +- app/templates/views/edit-sms-template.html | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/templates/views/edit-email-template.html b/app/templates/views/edit-email-template.html index 0505fc97c..e49412f6d 100644 --- a/app/templates/views/edit-email-template.html +++ b/app/templates/views/edit-email-template.html @@ -15,7 +15,7 @@
    - {{ textbox(form.name, width='1-1') }} + {{ textbox(form.name, width='1-1', hint='Your recipients won’t see this') }} {{ textbox(form.subject, width='1-1') }}
    diff --git a/app/templates/views/edit-sms-template.html b/app/templates/views/edit-sms-template.html index a571c300a..be460358d 100644 --- a/app/templates/views/edit-sms-template.html +++ b/app/templates/views/edit-sms-template.html @@ -15,7 +15,7 @@
    - {{ textbox(form.name, width='1-1') }} + {{ textbox(form.name, width='1-1', hint='Your recipients won’t see this') }}
    {{ textbox(form.template_content, highlight_tags=True, width='1-1') }} From 7625e184e2a012d6732823b34159bf6f18fb4d1c Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 1 Apr 2016 14:24:39 +0100 Subject: [PATCH 3/3] =?UTF-8?q?Replace=20=E2=80=98send=20a=20batch?= =?UTF-8?q?=E2=80=99=20with=20=E2=80=98send=20from=20a=20CSV=20file?= =?UTF-8?q?=E2=80=99?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is better. Not done enough testing to know if it’s the best, but let’s try it. --- app/templates/views/choose-template.html | 2 +- app/templates/views/send.html | 9 +++++---- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/app/templates/views/choose-template.html b/app/templates/views/choose-template.html index 2d0b56551..dd0f31503 100644 --- a/app/templates/views/choose-template.html +++ b/app/templates/views/choose-template.html @@ -77,7 +77,7 @@
    {% if current_user.has_permissions(permissions=['send_texts', 'send_emails', 'send_letters']) %} - Send a batch + Send from a CSV file Send yourself a test {% endif %} {% if current_user.has_permissions(permissions=['manage_api_keys']) %} diff --git a/app/templates/views/send.html b/app/templates/views/send.html index 6bfdfd596..d41c715cf 100644 --- a/app/templates/views/send.html +++ b/app/templates/views/send.html @@ -11,7 +11,7 @@ {% block maincolumn_content %} -

    Send a batch

    +

    Send from a CSV file

    {% if 'sms' == template.template_type %}
    @@ -31,13 +31,14 @@

    - Add recipients by uploading a .csv file with + You need {{ template.placeholders|length + 1 }} {% if template.placeholders %} - columns: + columns {% else %} - column: + column {% endif %} + in your file:

    {{ recipient_column }}