From 78ac345148a11ac93a242f5ef2a516f25b79e7f2 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 17 Feb 2020 12:58:05 +0000 Subject: [PATCH 01/18] Put day of week on This helps people understand how delivery dates fit into the week, because if sending a letter spans the weekend it will take a bit longer. We can remove `|string` now because `utc_string_to_aware_gmt_datetime` now accepts datetimes. --- app/__init__.py | 5 +++++ app/templates/views/notifications/notification.html | 2 +- tests/app/main/views/test_notifications.py | 6 ++++-- 3 files changed, 10 insertions(+), 3 deletions(-) diff --git a/app/__init__.py b/app/__init__.py index 4daf81b80..0242fea30 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -334,6 +334,10 @@ def format_date_human(date): return get_human_day(date) +def format_day_of_week(date): + return utc_string_to_aware_gmt_datetime(date).strftime('%A') + + def _format_datetime_short(datetime): return datetime.strftime('%d %B').lstrip('0') @@ -743,6 +747,7 @@ def add_template_filters(application): format_date_normal, format_date_short, format_datetime_relative, + format_day_of_week, format_delta, format_notification_status, format_notification_type, diff --git a/app/templates/views/notifications/notification.html b/app/templates/views/notifications/notification.html index f422414ea..f2004a6ec 100644 --- a/app/templates/views/notifications/notification.html +++ b/app/templates/views/notifications/notification.html @@ -67,7 +67,7 @@ {{ letter_print_day }}

- Estimated delivery date: {{ estimated_letter_delivery_date|string|format_date_short }} + Estimated delivery date: {{ estimated_letter_delivery_date|format_day_of_week }} {{ estimated_letter_delivery_date|format_date_short }}

{% endif %} {% endif %} diff --git a/tests/app/main/views/test_notifications.py b/tests/app/main/views/test_notifications.py index 012ccaa1a..426aeade2 100644 --- a/tests/app/main/views/test_notifications.py +++ b/tests/app/main/views/test_notifications.py @@ -202,7 +202,7 @@ def test_notification_page_shows_page_for_letter_notification( 'Printing starts today at 5:30pm' ) assert normalize_spaces(page.select('main p:nth-of-type(3)')[0].text) == ( - 'Estimated delivery date: 6 January' + 'Estimated delivery date: Wednesday 6 January' ) assert len(page.select('.letter-postage')) == 1 assert normalize_spaces(page.select_one('.letter-postage').text) == ( @@ -473,7 +473,9 @@ def test_notification_page_shows_page_for_first_class_letter_notification( ) assert normalize_spaces(page.select('main p:nth-of-type(2)')[0].text) == 'Printing starts tomorrow at 5:30pm' - assert normalize_spaces(page.select('main p:nth-of-type(3)')[0].text) == 'Estimated delivery date: 5 January' + assert normalize_spaces(page.select('main p:nth-of-type(3)')[0].text) == ( + 'Estimated delivery date: Tuesday 5 January' + ) assert normalize_spaces(page.select_one('.letter-postage').text) == ( 'Postage: first class' ) From 904007201f8adf32fb7f9cf71bc7cc255a869018 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 19 Feb 2020 17:11:31 +0000 Subject: [PATCH 02/18] Make date of sending relative MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is a friendlier and better way of showing dates anywhere except in a report which might be archived and referred back to later. Bit trickier to implement here because a dat requires ‘on’ beforehand, but we don’t say ‘on today’ in English. --- app/__init__.py | 21 +++++++-- .../views/notifications/notification.html | 2 +- tests/app/main/views/test_notifications.py | 45 ++++++++++++++----- 3 files changed, 52 insertions(+), 16 deletions(-) diff --git a/app/__init__.py b/app/__init__.py index 0242fea30..c2921bafc 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -291,7 +291,7 @@ def format_time_24h(date): return utc_string_to_aware_gmt_datetime(date).strftime('%H:%M') -def get_human_day(time): +def get_human_day(time, date_prefix=''): # Add 1 minute to transform 00:00 into ‘midnight today’ instead of ‘midnight tomorrow’ date = (utc_string_to_aware_gmt_datetime(time) - timedelta(minutes=1)).date() @@ -304,8 +304,15 @@ def get_human_day(time): if date == (now - timedelta(days=1)).date(): return 'yesterday' if date.strftime('%Y') != now.strftime('%Y'): - return '{} {}'.format(_format_datetime_short(date), date.strftime('%Y')) - return _format_datetime_short(date) + return '{} {} {}'.format( + date_prefix, + _format_datetime_short(date), + date.strftime('%Y'), + ).strip() + return '{} {}'.format( + date_prefix, + _format_datetime_short(date), + ).strip() def format_time(date): @@ -334,6 +341,13 @@ def format_date_human(date): return get_human_day(date) +def format_datetime_human(date, date_prefix=''): + return '{} at {}'.format( + get_human_day(date, date_prefix='on'), + format_time(date), + ) + + def format_day_of_week(date): return utc_string_to_aware_gmt_datetime(date).strftime('%A') @@ -746,6 +760,7 @@ def add_template_filters(application): format_date_human, format_date_normal, format_date_short, + format_datetime_human, format_datetime_relative, format_day_of_week, format_delta, diff --git a/app/templates/views/notifications/notification.html b/app/templates/views/notifications/notification.html index f2004a6ec..825adc81f 100644 --- a/app/templates/views/notifications/notification.html +++ b/app/templates/views/notifications/notification.html @@ -34,7 +34,7 @@ {% elif created_by %} by {{ created_by.name }} {% endif %} - on {{ created_at|format_datetime_short }} + {{ created_at|format_datetime_human }}

{% if template.template_type == 'letter' %} diff --git a/tests/app/main/views/test_notifications.py b/tests/app/main/views/test_notifications.py index 426aeade2..10fc2d4d2 100644 --- a/tests/app/main/views/test_notifications.py +++ b/tests/app/main/views/test_notifications.py @@ -152,22 +152,43 @@ def test_notification_status_shows_expected_back_link( assert back_link is None -@freeze_time("2012-01-01 01:01") +@pytest.mark.parametrize('time_of_viewing_page, expected_message', ( + ('2012-01-01 01:01', ( + "‘sample template’ was sent by Test User today at 1:01am" + )), + ('2012-01-02 01:01', ( + "‘sample template’ was sent by Test User yesterday at 1:01am" + )), + ('2012-01-03 01:01', ( + "‘sample template’ was sent by Test User on 1 January at 1:01am" + )), + ('2013-01-03 01:01', ( + "‘sample template’ was sent by Test User on 1 January 2012 at 1:01am" + )), +)) def test_notification_page_doesnt_link_to_template_in_tour( + mocker, client_request, fake_uuid, mock_get_notification, + time_of_viewing_page, + expected_message, ): - page = client_request.get( - 'main.view_notification', - service_id=SERVICE_ONE_ID, - notification_id=fake_uuid, - help=3, - ) + with freeze_time('2012-01-01 01:01'): + notification = create_notification() + mocker.patch('app.notification_api_client.get_notification', return_value=notification) + + with freeze_time(time_of_viewing_page): + page = client_request.get( + 'main.view_notification', + service_id=SERVICE_ONE_ID, + notification_id=fake_uuid, + help=3, + ) assert normalize_spaces(page.select('main p:nth-of-type(1)')[0].text) == ( - "‘sample template’ was sent by Test User on 1 January at 1:01am" + expected_message ) assert len(page.select('main p:nth-of-type(1) a')) == 0 @@ -196,7 +217,7 @@ def test_notification_page_shows_page_for_letter_notification( ) assert normalize_spaces(page.select('main p:nth-of-type(1)')[0].text) == ( - "‘sample template’ was sent by Test User on 1 January at 1:01am" + "‘sample template’ was sent by Test User today at 1:01am" ) assert normalize_spaces(page.select('main p:nth-of-type(2)')[0].text) == ( 'Printing starts today at 5:30pm' @@ -230,13 +251,13 @@ def test_notification_page_shows_page_for_letter_notification( @pytest.mark.parametrize('is_precompiled_letter, expected_p1, expected_p2, expected_postage', ( ( True, - 'Provided as PDF on 1 January at 1:01am', + 'Provided as PDF today at 1:01am', 'This letter passed our checks, but we will not print it because you used a test key.', 'Postage: second class' ), ( False, - '‘sample template’ was sent on 1 January at 1:01am', + '‘sample template’ was sent today at 1:01am', 'We will not print this letter because you used a test key.', 'Postage: second class', ), @@ -377,7 +398,7 @@ def test_notification_page_shows_cancelled_or_failed_letter( ) assert normalize_spaces(page.select('main p')[0].text) == ( - "‘sample template’ was sent by Test User on 1 January at 1:01am" + "‘sample template’ was sent by Test User today at 1:01am" ) assert normalize_spaces(page.select('main p')[1].text) == ( expected_message From 46e16c64fc5732673a76432e32485ea3fa20a566 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 19 Feb 2020 17:18:03 +0000 Subject: [PATCH 03/18] =?UTF-8?q?Say=20=E2=80=98Uploaded=E2=80=99=20for=20?= =?UTF-8?q?one-off=20precompiled=20letters?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This calls back to what the user has actively done. _Provided as a PDF_ is more passive and suitable for PDF letters sent using the API. --- .../views/notifications/notification.html | 6 ++- tests/app/main/views/test_notifications.py | 45 +++++++++++++++++++ 2 files changed, 50 insertions(+), 1 deletion(-) diff --git a/app/templates/views/notifications/notification.html b/app/templates/views/notifications/notification.html index 825adc81f..8aa91cb08 100644 --- a/app/templates/views/notifications/notification.html +++ b/app/templates/views/notifications/notification.html @@ -17,7 +17,11 @@ ) }}

{% if is_precompiled_letter %} - Provided as PDF + {% if created_by %} + Uploaded + {% else %} + Provided as PDF + {% endif %} {% else %} {% if help %} ‘{{ template.name }}’ diff --git a/tests/app/main/views/test_notifications.py b/tests/app/main/views/test_notifications.py index 10fc2d4d2..590e61c89 100644 --- a/tests/app/main/views/test_notifications.py +++ b/tests/app/main/views/test_notifications.py @@ -247,6 +247,51 @@ def test_notification_page_shows_page_for_letter_notification( assert mock_page_count.call_args_list[0][1]['values'] == {'name': 'Jo'} +@freeze_time("2020-01-01 00:00") +def test_notification_page_shows_uploaded_letter( + client_request, + mocker, + fake_uuid, +): + mocker.patch( + 'app.main.views.notifications.view_letter_notification_as_preview', + return_value=(b'foo', { + 'message': '', + 'invalid_pages': '[]', + 'page_count': '1' + }) + ) + mocker.patch( + 'app.main.views.notifications.pdf_page_count', + return_value=1 + ) + mocker.patch( + 'app.main.views.notifications.get_page_count_for_letter', + return_value=1, + ) + + notification = create_notification( + notification_status='created', + template_type='letter', + is_precompiled_letter=True, + sent_one_off=True, + ) + mocker.patch('app.notification_api_client.get_notification', return_value=notification) + + page = client_request.get( + 'main.view_notification', + service_id=SERVICE_ONE_ID, + notification_id=fake_uuid, + ) + + assert normalize_spaces(page.select('main p:nth-of-type(1)')[0].text) == ( + 'Uploaded by Test User yesterday at midnight' + ) + assert normalize_spaces(page.select('main p:nth-of-type(2)')[0].text) == ( + 'Printing starts today at 5:30pm' + ) + + @freeze_time("2016-01-01 01:01") @pytest.mark.parametrize('is_precompiled_letter, expected_p1, expected_p2, expected_postage', ( ( From 8a98c73b080c1dcc8c8c0059d1981b05180527f3 Mon Sep 17 00:00:00 2001 From: Tom Byers Date: Thu, 20 Feb 2020 11:03:03 +0000 Subject: [PATCH 04/18] Make analytics send the same title for all pages Sets it on the tracker which means this value gets sent for each: - pageview - event See: https://developers.google.com/analytics/devguides/collection/analyticsjs/pages We can only test that this has been set on the tracker, not how that effects what is sent to GA, in the JS tests. This change has been tested in-browser with the Chrome Analytics Debugger. This revealed the contents of what is in the beacon sent to GA and allowed confirmation the title was being set correctly. See: https://chrome.google.com/webstore/detail/google-analytics-debugger/jnkmfdileelhofjcijamephohjechhna/related --- app/assets/javascripts/analytics/analytics.js | 1 + tests/javascripts/analytics/analytics.test.js | 1 + tests/javascripts/analytics/init.test.js | 6 +++--- tests/javascripts/cookieSettings.test.js | 4 ++-- 4 files changed, 7 insertions(+), 5 deletions(-) diff --git a/app/assets/javascripts/analytics/analytics.js b/app/assets/javascripts/analytics/analytics.js index 711f6ec80..a776f93a2 100644 --- a/app/assets/javascripts/analytics/analytics.js +++ b/app/assets/javascripts/analytics/analytics.js @@ -15,6 +15,7 @@ window.ga('set', 'anonymizeIp', config.anonymizeIp); window.ga('set', 'allowAdFeatures', config.allowAdFeatures); window.ga('set', 'transport', config.transport); + window.ga('set', 'title', 'GOV.UK Notify'); }; diff --git a/tests/javascripts/analytics/analytics.test.js b/tests/javascripts/analytics/analytics.test.js index ea4b91fe2..1f11544e8 100644 --- a/tests/javascripts/analytics/analytics.test.js +++ b/tests/javascripts/analytics/analytics.test.js @@ -52,6 +52,7 @@ describe("Analytics", () => { expect(setUpArguments[1]).toEqual(['set', 'anonymizeIp', true]); expect(setUpArguments[2]).toEqual(['set', 'allowAdFeatures', false]); expect(setUpArguments[3]).toEqual(['set', 'transport', 'beacon']); + expect(setUpArguments[4]).toEqual(['set', 'title', 'GOV.UK Notify']); }); diff --git a/tests/javascripts/analytics/init.test.js b/tests/javascripts/analytics/init.test.js index 8384b826f..6bf7a31c8 100644 --- a/tests/javascripts/analytics/init.test.js +++ b/tests/javascripts/analytics/init.test.js @@ -111,10 +111,10 @@ describe("Analytics init", () => { test("A pageview will be registered", () => { - expect(window.ga.mock.calls.length).toEqual(5); + expect(window.ga.mock.calls.length).toEqual(6); - // The first 4 calls configure the analytics tracker. All subsequent calls send data - expect(window.ga.mock.calls[4]).toEqual(['send', 'pageview', '/privacy']); + // The first 5 calls configure the analytics tracker. All subsequent calls send data + expect(window.ga.mock.calls[5]).toEqual(['send', 'pageview', '/privacy']); }); diff --git a/tests/javascripts/cookieSettings.test.js b/tests/javascripts/cookieSettings.test.js index a6c6f1344..4bc85a213 100644 --- a/tests/javascripts/cookieSettings.test.js +++ b/tests/javascripts/cookieSettings.test.js @@ -231,8 +231,8 @@ describe("Cookie settings", () => { expect(window.GOVUK.initAnalytics).toHaveBeenCalled(); expect(window.ga).toHaveBeenCalled(); - // the first 4 calls are configuration - expect(window.ga.mock.calls[4]).toEqual(['send', 'pageview', '/privacy']); + // the first 5 calls are configuration + expect(window.ga.mock.calls[5]).toEqual(['send', 'pageview', '/privacy']); }); From 74e70ed8bc30bac99dbaef04eda70c2aacc3bfc4 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 12 Feb 2020 14:03:51 +0000 Subject: [PATCH 05/18] Refactor summaries into model MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This lets us encapsulate some of the logic that’s currently cluttering up the view/template layer. --- app/main/views/dashboard.py | 4 ---- app/main/views/returned_letters.py | 5 ++--- app/models/service.py | 10 ++++++++++ app/templates/views/dashboard/_inbox.html | 12 ++++++------ 4 files changed, 18 insertions(+), 13 deletions(-) diff --git a/app/main/views/dashboard.py b/app/main/views/dashboard.py index 2ae4394b0..b66eedf8f 100644 --- a/app/main/views/dashboard.py +++ b/app/main/views/dashboard.py @@ -296,10 +296,6 @@ def get_dashboard_partials(service_id): ), 'inbox': render_template( 'views/dashboard/_inbox.html', - inbound_sms_summary=( - service_api_client.get_inbound_sms_summary(service_id) - if current_service.has_permission('inbound_sms') else None - ), ), 'totals': render_template( 'views/dashboard/_totals.html', diff --git a/app/main/views/returned_letters.py b/app/main/views/returned_letters.py index cdc06e464..bb81f3449 100644 --- a/app/main/views/returned_letters.py +++ b/app/main/views/returned_letters.py @@ -2,7 +2,7 @@ from collections import OrderedDict from flask import render_template -from app import service_api_client +from app import current_service, service_api_client from app.main import main from app.utils import Spreadsheet, user_has_permissions @@ -10,10 +10,9 @@ from app.utils import Spreadsheet, user_has_permissions @main.route("/services//returned-letters") @user_has_permissions('view_activity') def returned_letter_summary(service_id): - summary = service_api_client.get_returned_letter_summary(service_id) return render_template( 'views/returned-letter-summary.html', - data=summary, + data=current_service.returned_letter_summary, ) diff --git a/app/models/service.py b/app/models/service.py index 9e4fad965..c7b3f2737 100644 --- a/app/models/service.py +++ b/app/models/service.py @@ -480,6 +480,12 @@ class Service(JSONModel): def has_inbound_number(self): return bool(self.inbound_number) + @cached_property + def inbound_sms_summary(self): + if not self.has_permission('inbound_sms'): + return None + return service_api_client.get_inbound_sms_summary(self.id) + @cached_property def all_template_folders(self): return sorted( @@ -660,3 +666,7 @@ class Service(JSONModel): ): if test: yield BASE + '_incomplete' + tag + + @cached_property + def returned_letter_summary(self): + return service_api_client.get_returned_letter_summary(self.id) diff --git a/app/templates/views/dashboard/_inbox.html b/app/templates/views/dashboard/_inbox.html index 9d3586903..57a9d85b6 100644 --- a/app/templates/views/dashboard/_inbox.html +++ b/app/templates/views/dashboard/_inbox.html @@ -2,17 +2,17 @@ {% from "components/message-count-label.html" import message_count_label %}

- {% if inbound_sms_summary != None %} - From f369f76ae4860d3505a0924b7a2bdbdbc89a013e Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 12 Feb 2020 14:19:21 +0000 Subject: [PATCH 06/18] Count recently-returned letters on the dashboard Currently you have no way of getting to the returned letter page. This commit adds a link to it from the dashboard, following the pattern of the new received text messages banner. --- app/__init__.py | 17 +++ app/models/service.py | 25 ++++ app/templates/views/dashboard/_inbox.html | 13 ++ tests/app/main/views/test_accept_invite.py | 2 + tests/app/main/views/test_dashboard.py | 160 +++++++++++++++++++-- tests/app/main/views/test_sign_out.py | 1 + tests/conftest.py | 8 ++ 7 files changed, 218 insertions(+), 8 deletions(-) diff --git a/app/__init__.py b/app/__init__.py index 4daf81b80..bffed68b2 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -356,6 +356,22 @@ def format_delta(date): ) +def format_delta_days(date): + now = datetime.now(timezone.utc) + date = utc_string_to_aware_gmt_datetime(date) + delta = now - date + if date.strftime('%Y-%M-%D') == now.strftime('%Y-%M-%D'): + return "today" + if date.strftime('%Y-%M-%D') == (now - timedelta(days=1)).strftime('%Y-%M-%D'): + return "yesterday" + return ago.human( + delta, + precision=1, + future_tense='{} from now', + past_tense='{} ago', + ) + + def valid_phone_number(phone_number): try: validate_phone_number(phone_number) @@ -744,6 +760,7 @@ def add_template_filters(application): format_date_short, format_datetime_relative, format_delta, + format_delta_days, format_notification_status, format_notification_type, format_notification_status_as_time, diff --git a/app/models/service.py b/app/models/service.py index c7b3f2737..132ac157c 100644 --- a/app/models/service.py +++ b/app/models/service.py @@ -1,4 +1,8 @@ +from datetime import datetime, timedelta + +from dateutil.parser import parse from flask import abort, current_app +from notifications_utils.timezones import local_timezone from werkzeug.utils import cached_property from app.models import JSONModel @@ -670,3 +674,24 @@ class Service(JSONModel): @cached_property def returned_letter_summary(self): return service_api_client.get_returned_letter_summary(self.id) + + @property + def most_recent_returned_letter_report(self): + if not self.returned_letter_summary: + return None + return parse( + self.returned_letter_summary[0]['reported_at'] + " 00:00:00" + ).replace(tzinfo=local_timezone) + + @property + def count_of_returned_letters_in_last_7_days(self): + seven_days_ago = ( + datetime.now() - timedelta(days=7) + ).replace( + hour=0, minute=0, second=0 + ) + return sum( + report['returned_letter_count'] + for report in self.returned_letter_summary + if parse(report['reported_at'] + " 00:00:00") >= seven_days_ago + ) diff --git a/app/templates/views/dashboard/_inbox.html b/app/templates/views/dashboard/_inbox.html index 57a9d85b6..ebcd6fb84 100644 --- a/app/templates/views/dashboard/_inbox.html +++ b/app/templates/views/dashboard/_inbox.html @@ -17,4 +17,17 @@ {% endif %} {% endif %} + {% if current_service.returned_letter_summary %} + + {% endif %}
diff --git a/tests/app/main/views/test_accept_invite.py b/tests/app/main/views/test_accept_invite.py index cbd280ffe..b00b4209b 100644 --- a/tests/app/main/views/test_accept_invite.py +++ b/tests/app/main/views/test_accept_invite.py @@ -149,6 +149,7 @@ def test_accepting_invite_removes_invite_from_session( mock_get_billable_units, mock_get_free_sms_fragment_limit, mock_get_inbound_sms_summary, + mock_get_returned_letter_summary_with_no_returned_letters, fake_uuid, user, landing_page_title, @@ -470,6 +471,7 @@ def test_new_invited_user_verifies_and_added_to_service( mock_get_service_statistics, mock_get_usage, mock_get_free_sms_fragment_limit, + mock_get_returned_letter_summary_with_no_returned_letters, mock_create_event, mocker, ): diff --git a/tests/app/main/views/test_dashboard.py b/tests/app/main/views/test_dashboard.py index fd95ee7b9..0aafafb88 100644 --- a/tests/app/main/views/test_dashboard.py +++ b/tests/app/main/views/test_dashboard.py @@ -143,7 +143,8 @@ def test_get_started( mock_get_service_statistics, mock_get_usage, mock_get_free_sms_fragment_limit, - mock_get_inbound_sms_summary + mock_get_inbound_sms_summary, + mock_get_returned_letter_summary_with_no_returned_letters, ): mocker.patch( 'app.template_statistics_client.get_template_statistics_for_service', @@ -167,7 +168,8 @@ def test_get_started_is_hidden_once_templates_exist( mock_get_service_statistics, mock_get_usage, mock_get_free_sms_fragment_limit, - mock_get_inbound_sms_summary + mock_get_inbound_sms_summary, + mock_get_returned_letter_summary_with_no_returned_letters, ): mocker.patch( 'app.template_statistics_client.get_template_statistics_for_service', @@ -184,6 +186,7 @@ def test_get_started_is_hidden_once_templates_exist( def test_inbound_messages_not_visible_to_service_without_permissions( client_request, + mocker, service_one, mock_get_service_templates_when_no_templates_exist, mock_get_jobs, @@ -191,7 +194,8 @@ def test_inbound_messages_not_visible_to_service_without_permissions( mock_get_template_statistics, mock_get_usage, mock_get_free_sms_fragment_limit, - mock_get_inbound_sms_summary + mock_get_inbound_sms_summary, + mock_get_returned_letter_summary_with_no_returned_letters, ): service_one['permissions'] = [] @@ -216,6 +220,7 @@ def test_inbound_messages_shows_count_of_messages_when_there_are_messages( mock_get_usage, mock_get_free_sms_fragment_limit, mock_get_inbound_sms_summary, + mock_get_returned_letter_summary_with_no_returned_letters, ): service_one['permissions'] = ['inbound_sms'] page = client_request.get( @@ -242,6 +247,7 @@ def test_inbound_messages_shows_count_of_messages_when_there_are_no_messages( mock_get_usage, mock_get_free_sms_fragment_limit, mock_get_inbound_sms_summary_with_no_messages, + mock_get_returned_letter_summary_with_no_returned_letters, ): service_one['permissions'] = ['inbound_sms'] page = client_request.get( @@ -472,6 +478,126 @@ def test_download_inbox_strips_formulae( assert expected_cell in response.get_data(as_text=True).split('\r\n')[1] +def test_returned_letters_not_visible_if_service_has_no_returned_letters( + client_request, + mocker, + service_one, + mock_get_service_templates_when_no_templates_exist, + mock_get_jobs, + mock_get_service_statistics, + mock_get_template_statistics, + mock_get_usage, + mock_get_free_sms_fragment_limit, + mock_get_inbound_sms_summary, + mock_get_returned_letter_summary_with_no_returned_letters, +): + page = client_request.get( + 'main.service_dashboard', + service_id=SERVICE_ONE_ID, + ) + assert not page.select('#total-returned-letters') + + +@freeze_time('2020-01-10') +def test_returned_letters_shows_count_of_recently_returned_letters( + client_request, + mocker, + service_one, + mock_get_service_templates_when_no_templates_exist, + mock_get_jobs, + mock_get_service_statistics, + mock_get_template_statistics, + mock_get_usage, + mock_get_free_sms_fragment_limit, + mock_get_inbound_sms_summary, +): + mocker.patch( + 'app.service_api_client.get_returned_letter_summary', + return_value=[ + # Today (should be counted) + { + 'returned_letter_count': 1000, 'reported_at': '2020-01-10' + }, + # Just within the last 7 days (should be counted) + { + 'returned_letter_count': 3000, 'reported_at': '2020-01-3' + }, + # Just after the last 7 days (should not be counted) + { + 'returned_letter_count': 2000, 'reported_at': '2020-01-2' + }, + ], + ) + page = client_request.get( + 'main.service_dashboard', + service_id=SERVICE_ONE_ID, + ) + banner = page.select_one('#total-returned-letters') + assert normalize_spaces( + banner.text + ) == '4,000 returned letters latest report today' + assert banner['href'] == url_for( + 'main.returned_letter_summary', service_id=SERVICE_ONE_ID + ) + + +@pytest.mark.parametrize('reporting_date, expected_message', ( + ('2020-02-02', ( + '1 returned letter latest report today' + )), + ('2020-02-01', ( + '1 returned letter latest report yesterday' + )), + ('2020-01-31', ( + '1 returned letter latest report 2 days ago' + )), + ('2020-01-26', ( + '1 returned letter latest report 7 days ago' + )), + ('2020-01-25', ( + '0 returned letters latest report 8 days ago' + )), + ('2019-09-09', ( + '0 returned letters latest report 146 days ago' + )), + ('2010-10-10', ( + '0 returned letters latest report 9 years ago' + )), +)) +@freeze_time('2020-02-02') +def test_returned_letters_only_counts_recently_returned_letters( + client_request, + mocker, + service_one, + mock_get_service_templates_when_no_templates_exist, + mock_get_jobs, + mock_get_service_statistics, + mock_get_template_statistics, + mock_get_usage, + mock_get_free_sms_fragment_limit, + mock_get_inbound_sms_summary_with_no_messages, + reporting_date, + expected_message, +): + mocker.patch( + 'app.service_api_client.get_returned_letter_summary', + return_value=[ + { + 'returned_letter_count': 1, 'reported_at': reporting_date + }, + ], + ) + page = client_request.get( + 'main.service_dashboard', + service_id=SERVICE_ONE_ID, + ) + banner = page.select_one('#total-returned-letters') + assert normalize_spaces(banner.text) == expected_message + assert banner['href'] == url_for( + 'main.returned_letter_summary', service_id=SERVICE_ONE_ID + ) + + def test_should_show_recent_templates_on_dashboard( client_request, mocker, @@ -480,7 +606,8 @@ def test_should_show_recent_templates_on_dashboard( mock_get_service_statistics, mock_get_usage, mock_get_free_sms_fragment_limit, - mock_get_inbound_sms_summary + mock_get_inbound_sms_summary, + mock_get_returned_letter_summary_with_no_returned_letters, ): mock_template_stats = mocker.patch('app.template_statistics_client.get_template_statistics_for_service', return_value=copy.deepcopy(stub_template_stats)) @@ -534,6 +661,7 @@ def test_should_not_show_recent_templates_on_dashboard_if_only_one_template_used mock_get_usage, mock_get_free_sms_fragment_limit, mock_get_inbound_sms_summary, + mock_get_returned_letter_summary_with_no_returned_letters, stats, ): mock_template_stats = mocker.patch( @@ -687,7 +815,8 @@ def test_should_show_upcoming_jobs_on_dashboard( mock_get_jobs, mock_get_usage, mock_get_free_sms_fragment_limit, - mock_get_inbound_sms_summary + mock_get_inbound_sms_summary, + mock_get_returned_letter_summary_with_no_returned_letters, ): page = client_request.get( 'main.service_dashboard', @@ -738,6 +867,7 @@ def test_correct_font_size_for_big_numbers( mock_get_jobs, mock_get_usage, mock_get_free_sms_fragment_limit, + mock_get_returned_letter_summary_with_no_returned_letters, service_one, permissions, totals, @@ -773,7 +903,8 @@ def test_should_show_recent_jobs_on_dashboard( mock_get_jobs, mock_get_usage, mock_get_free_sms_fragment_limit, - mock_get_inbound_sms_summary + mock_get_inbound_sms_summary, + mock_get_returned_letter_summary_with_no_returned_letters, ): page = client_request.get( 'main.service_dashboard', @@ -981,6 +1112,7 @@ def test_menu_send_messages( mock_get_usage, mock_get_inbound_sms_summary, mock_get_free_sms_fragment_limit, + mock_get_returned_letter_summary_with_no_returned_letters, ): service_one['permissions'] = ['email', 'sms', 'letter', 'upload_letters'] @@ -1039,6 +1171,7 @@ def test_menu_manage_service( mock_get_service_statistics, mock_get_usage, mock_get_inbound_sms_summary, + mock_get_returned_letter_summary_with_no_returned_letters, mock_get_free_sms_fragment_limit, ): with app_.test_request_context(): @@ -1070,6 +1203,7 @@ def test_menu_manage_api_keys( mock_get_service_statistics, mock_get_usage, mock_get_inbound_sms_summary, + mock_get_returned_letter_summary_with_no_returned_letters, mock_get_free_sms_fragment_limit, ): with app_.test_request_context(): @@ -1099,6 +1233,7 @@ def test_menu_all_services_for_platform_admin_user( mock_get_service_statistics, mock_get_usage, mock_get_inbound_sms_summary, + mock_get_returned_letter_summary_with_no_returned_letters, mock_get_free_sms_fragment_limit, ): with app_.test_request_context(): @@ -1130,7 +1265,8 @@ def test_route_for_service_permissions( mock_get_service_statistics, mock_get_usage, mock_get_free_sms_fragment_limit, - mock_get_inbound_sms_summary + mock_get_inbound_sms_summary, + mock_get_returned_letter_summary_with_no_returned_letters, ): with app_.test_request_context(): validate_route_permission( @@ -1183,7 +1319,8 @@ def test_service_dashboard_updates_gets_dashboard_totals( mock_get_jobs, mock_get_usage, mock_get_free_sms_fragment_limit, - mock_get_inbound_sms_summary + mock_get_inbound_sms_summary, + mock_get_returned_letter_summary_with_no_returned_letters, ): mocker.patch('app.main.views.dashboard.get_dashboard_totals', return_value={ 'email': {'requested': 123, 'delivered': 0, 'failed': 0}, @@ -1368,6 +1505,7 @@ def test_should_show_all_jobs_with_valid_statuses( mock_get_jobs, mock_get_usage, mock_get_inbound_sms_summary, + mock_get_returned_letter_summary_with_no_returned_letters, mock_get_free_sms_fragment_limit, ): logged_in_client.get(url_for('main.service_dashboard', service_id=SERVICE_ONE_ID)) @@ -1397,6 +1535,7 @@ def test_org_breadcrumbs_do_not_show_if_service_has_no_org( mock_get_jobs, mock_get_usage, mock_get_free_sms_fragment_limit, + mock_get_returned_letter_summary_with_no_returned_letters, ): page = client_request.get('main.service_dashboard', service_id=SERVICE_ONE_ID) @@ -1411,6 +1550,7 @@ def test_org_breadcrumbs_do_not_show_if_user_is_not_an_org_member( active_caseworking_user, client_request, mock_get_template_folders, + mock_get_returned_letter_summary_with_no_returned_letters, ): # active_caseworking_user is not an org member @@ -1433,6 +1573,7 @@ def test_org_breadcrumbs_show_if_user_is_a_member_of_the_services_org( mock_get_jobs, mock_get_usage, mock_get_free_sms_fragment_limit, + mock_get_returned_letter_summary_with_no_returned_letters, active_user_with_permissions, client_request, ): @@ -1460,6 +1601,7 @@ def test_org_breadcrumbs_do_not_show_if_user_is_a_member_of_the_services_org_but mock_get_jobs, mock_get_usage, mock_get_free_sms_fragment_limit, + mock_get_returned_letter_summary_with_no_returned_letters, active_user_with_permissions, client_request, ): @@ -1484,6 +1626,7 @@ def test_org_breadcrumbs_show_if_user_is_platform_admin( mock_get_jobs, mock_get_usage, mock_get_free_sms_fragment_limit, + mock_get_returned_letter_summary_with_no_returned_letters, platform_admin_user, platform_admin_client, ): @@ -1515,6 +1658,7 @@ def test_should_show_usage_on_dashboard( mock_get_jobs, mock_get_usage, mock_get_free_sms_fragment_limit, + mock_get_returned_letter_summary_with_no_returned_letters, permissions, ): service_one['permissions'] = permissions diff --git a/tests/app/main/views/test_sign_out.py b/tests/app/main/views/test_sign_out.py index 12e93f33b..2d1be4c2f 100644 --- a/tests/app/main/views/test_sign_out.py +++ b/tests/app/main/views/test_sign_out.py @@ -31,6 +31,7 @@ def test_sign_out_user( mock_get_usage, mock_get_free_sms_fragment_limit, mock_get_inbound_sms_summary, + mock_get_returned_letter_summary_with_no_returned_letters, ): with client_request.session_transaction() as session: assert session.get('user_id') is not None diff --git a/tests/conftest.py b/tests/conftest.py index dfa844724..f08fd2d9c 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -3342,6 +3342,14 @@ def mock_get_service_history(mocker): }) +@pytest.fixture(scope='function') +def mock_get_returned_letter_summary_with_no_returned_letters(mocker): + return mocker.patch( + 'app.service_api_client.get_returned_letter_summary', + return_value=[], + ) + + def create_api_user_active(with_unique_id=False): return { 'id': str(uuid4()) if with_unique_id else sample_uuid(), From 9590643527643f264e2abe940a9a1f1769843140 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 14 Feb 2020 15:09:03 +0000 Subject: [PATCH 07/18] Use humanize for fuzzy time differences MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It seems to do a bit better than ago (e.g. 4 months vs 146 days), and looks like it’s maintained more often. --- app/__init__.py | 17 +++-------------- requirements-app.txt | 1 + requirements.txt | 5 +++-- tests/app/main/views/test_dashboard.py | 8 ++++---- 4 files changed, 11 insertions(+), 20 deletions(-) diff --git a/app/__init__.py b/app/__init__.py index bffed68b2..6b7197f3c 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -5,7 +5,7 @@ from datetime import datetime, timedelta, timezone from functools import partial from time import monotonic -import ago +import humanize import jinja2 from flask import ( Markup, @@ -348,28 +348,17 @@ def format_delta(date): return "just now" if delta < timedelta(seconds=60): return "in the last minute" - return ago.human( - delta, - future_tense='{} from now', # No-one should ever see this - past_tense='{} ago', - precision=1 - ) + return humanize.naturaltime(delta) def format_delta_days(date): now = datetime.now(timezone.utc) date = utc_string_to_aware_gmt_datetime(date) - delta = now - date if date.strftime('%Y-%M-%D') == now.strftime('%Y-%M-%D'): return "today" if date.strftime('%Y-%M-%D') == (now - timedelta(days=1)).strftime('%Y-%M-%D'): return "yesterday" - return ago.human( - delta, - precision=1, - future_tense='{} from now', - past_tense='{} ago', - ) + return humanize.naturaltime(now - date) def valid_phone_number(phone_number): diff --git a/requirements-app.txt b/requirements-app.txt index d2bd53f6d..ea804c360 100644 --- a/requirements-app.txt +++ b/requirements-app.txt @@ -2,6 +2,7 @@ # with package version changes made in requirements-app.txt ago==0.0.93 +humanize==1.0.0 Flask==1.1.1 Flask-WTF==0.14.3 Flask-Login==0.4.1 diff --git a/requirements.txt b/requirements.txt index 89e522545..9b7ca375f 100644 --- a/requirements.txt +++ b/requirements.txt @@ -4,6 +4,7 @@ # with package version changes made in requirements-app.txt ago==0.0.93 +humanize==1.0.0 Flask==1.1.1 Flask-WTF==0.14.3 Flask-Login==0.4.1 @@ -29,10 +30,10 @@ git+https://github.com/alphagov/notifications-utils.git@36.6.0#egg=notifications git+https://github.com/alphagov/govuk-frontend-jinja.git@v0.5.1-alpha#egg=govuk-frontend-jinja==0.5.1-alpha ## The following requirements were added by pip freeze: -awscli==1.17.15 +awscli==1.18.2 bleach==3.1.0 boto3==1.10.38 -botocore==1.14.15 +botocore==1.15.2 certifi==2019.11.28 chardet==3.0.4 Click==7.0 diff --git a/tests/app/main/views/test_dashboard.py b/tests/app/main/views/test_dashboard.py index 0aafafb88..6920b840c 100644 --- a/tests/app/main/views/test_dashboard.py +++ b/tests/app/main/views/test_dashboard.py @@ -262,9 +262,9 @@ def test_inbound_messages_shows_count_of_messages_when_there_are_no_messages( @pytest.mark.parametrize('index, expected_row', enumerate([ - '07900 900000 message-1 1 hour ago', - '07900 900000 message-2 1 hour ago', - '07900 900000 message-3 1 hour ago', + '07900 900000 message-1 an hour ago', + '07900 900000 message-2 an hour ago', + '07900 900000 message-3 an hour ago', '07900 900002 message-4 3 hours ago', '07900 900004 message-5 5 hours ago', '07900 900006 message-6 7 hours ago', @@ -558,7 +558,7 @@ def test_returned_letters_shows_count_of_recently_returned_letters( '0 returned letters latest report 8 days ago' )), ('2019-09-09', ( - '0 returned letters latest report 146 days ago' + '0 returned letters latest report 4 months ago' )), ('2010-10-10', ( '0 returned letters latest report 9 years ago' From 64074eed03e597c7e2342d15f8dd832fe6ff3bab Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Sat, 15 Feb 2020 18:29:58 +0000 Subject: [PATCH 08/18] =?UTF-8?q?Say=20=E2=80=981=20hour/month=20ago?= =?UTF-8?q?=E2=80=99=20not=20=E2=80=98an=20hour/a=20month=20ago=E2=80=99?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit I think it read better without the indefinite article when it’s, for example, placed alongside messages that read ‘2 hours ago’. --- app/__init__.py | 13 +++++++++++-- tests/app/main/views/test_dashboard.py | 9 ++++++--- 2 files changed, 17 insertions(+), 5 deletions(-) diff --git a/app/__init__.py b/app/__init__.py index 6b7197f3c..494c05720 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -1,5 +1,6 @@ import itertools import os +import re import urllib from datetime import datetime, timedelta, timezone from functools import partial @@ -338,6 +339,14 @@ def _format_datetime_short(datetime): return datetime.strftime('%d %B').lstrip('0') +def naturaltime_without_indefinite_article(date): + return re.sub( + 'an? (.*) ago', + lambda match: '1 {} ago'.format(match.group(1)), + humanize.naturaltime(date), + ) + + def format_delta(date): delta = ( datetime.now(timezone.utc) @@ -348,7 +357,7 @@ def format_delta(date): return "just now" if delta < timedelta(seconds=60): return "in the last minute" - return humanize.naturaltime(delta) + return naturaltime_without_indefinite_article(delta) def format_delta_days(date): @@ -358,7 +367,7 @@ def format_delta_days(date): return "today" if date.strftime('%Y-%M-%D') == (now - timedelta(days=1)).strftime('%Y-%M-%D'): return "yesterday" - return humanize.naturaltime(now - date) + return naturaltime_without_indefinite_article(now - date) def valid_phone_number(phone_number): diff --git a/tests/app/main/views/test_dashboard.py b/tests/app/main/views/test_dashboard.py index 6920b840c..3f09b4e18 100644 --- a/tests/app/main/views/test_dashboard.py +++ b/tests/app/main/views/test_dashboard.py @@ -262,9 +262,9 @@ def test_inbound_messages_shows_count_of_messages_when_there_are_no_messages( @pytest.mark.parametrize('index, expected_row', enumerate([ - '07900 900000 message-1 an hour ago', - '07900 900000 message-2 an hour ago', - '07900 900000 message-3 an hour ago', + '07900 900000 message-1 1 hour ago', + '07900 900000 message-2 1 hour ago', + '07900 900000 message-3 1 hour ago', '07900 900002 message-4 3 hours ago', '07900 900004 message-5 5 hours ago', '07900 900006 message-6 7 hours ago', @@ -557,6 +557,9 @@ def test_returned_letters_shows_count_of_recently_returned_letters( ('2020-01-25', ( '0 returned letters latest report 8 days ago' )), + ('2020-01-01', ( + '0 returned letters latest report 1 month ago' + )), ('2019-09-09', ( '0 returned letters latest report 4 months ago' )), From d00e37541cee92391f09b022998634f03a23a7d4 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 17 Feb 2020 10:23:59 +0000 Subject: [PATCH 09/18] Make dashboard navigation active for returned letters --- app/navigation.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/navigation.py b/app/navigation.py index 503e47ebe..dd6aeecc4 100644 --- a/app/navigation.py +++ b/app/navigation.py @@ -354,6 +354,8 @@ class MainNavigation(Navigation): 'conversation', 'inbox', 'monthly', + 'returned_letter_summary', + 'returned_letters', 'service_dashboard', 'template_usage', 'view_job', @@ -581,8 +583,6 @@ class MainNavigation(Navigation): 'resend_email_link', 'resend_email_verification', 'resume_service', - 'returned_letter_summary', - 'returned_letters', 'returned_letters_report', 'revalidate_email_sent', 'roadmap', From 678c3df53ce9c77c9846e034bcbdae3ad5626da0 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 20 Feb 2020 11:48:34 +0000 Subject: [PATCH 10/18] Add some context to the list of reports We reckon people need some context/expectation setting about what the date of the report is. --- .../views/returned-letter-summary.html | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/app/templates/views/returned-letter-summary.html b/app/templates/views/returned-letter-summary.html index 5be1fc0b3..a68c2b267 100644 --- a/app/templates/views/returned-letter-summary.html +++ b/app/templates/views/returned-letter-summary.html @@ -1,5 +1,6 @@ {% from "components/table.html" import list_table, row_heading %} {% from "components/message-count-label.html" import message_count_label %} +{% from "components/page-header.html" import page_header %} {% extends "withnav_template.html" %} {% block service_page_title %} @@ -7,9 +8,20 @@ {% endblock %} {% block maincolumn_content %} -

- Returned letters -

+
+
+ {{ page_header( + 'Returned letters', + back_link=url_for('main.service_dashboard', service_id=current_service.id) + ) }} +

+ Reports are published once a month. +

+

+ You’ll only get a report if one or more of your letters is returned. +

+
+
{% call(item, row_number) list_table( data, From 0e8143da0405e4da2335dbb2963d9abaf6fd9b25 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 20 Feb 2020 11:51:41 +0000 Subject: [PATCH 11/18] =?UTF-8?q?Remove=20=E2=80=98originally=E2=80=99=20f?= =?UTF-8?q?rom=20returned=20letters=20report?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We should remove Originally on the individual report pages. It sounds like we're saying the letter has been sent more than once since that date. The dictionary definition for 'originally' as an adverb says: > used to describe the situation that existed at the beginning of a > particular period or activity, especially before something was changed Examples given include: > The book was originally published in 1935. --- app/templates/views/returned-letters.html | 2 +- tests/app/main/views/test_returned_letters.py | 12 ++++++------ 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/app/templates/views/returned-letters.html b/app/templates/views/returned-letters.html index fe0e85462..f033a33bc 100644 --- a/app/templates/views/returned-letters.html +++ b/app/templates/views/returned-letters.html @@ -42,7 +42,7 @@ {% call field(align='right') %} - Originally sent {{ item.created_at|format_date_normal }} + Sent {{ item.created_at|format_date_normal }} {% endcall %} diff --git a/tests/app/main/views/test_returned_letters.py b/tests/app/main/views/test_returned_letters.py index 89b944e03..c8aadafc5 100644 --- a/tests/app/main/views/test_returned_letters.py +++ b/tests/app/main/views/test_returned_letters.py @@ -88,12 +88,12 @@ def test_returned_letters_page( assert [ 'Template name Originally sent', - 'Example template Reference ABC123 Originally sent 24 December 2019', - 'Example template Sent from Example spreadsheet.xlsx Originally sent 24 December 2019', - 'Example template No reference provided Originally sent 24 December 2019', - 'Example precompiled.pdf Reference DEF456 Originally sent 24 December 2019', - 'Example one-off.pdf No reference provided Originally sent 24 December 2019', - 'Provided as PDF Reference XYZ999 Originally sent 24 December 2019', + 'Example template Reference ABC123 Sent 24 December 2019', + 'Example template Sent from Example spreadsheet.xlsx Sent 24 December 2019', + 'Example template No reference provided Sent 24 December 2019', + 'Example precompiled.pdf Reference DEF456 Sent 24 December 2019', + 'Example one-off.pdf No reference provided Sent 24 December 2019', + 'Provided as PDF Reference XYZ999 Sent 24 December 2019', ] == [ normalize_spaces(row.text) for row in page.select('tr') ] From 1dcbf7755a0f8b99c8fdc832b4568767fb21b4fa Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 20 Feb 2020 12:12:53 +0000 Subject: [PATCH 12/18] Add a back link to the received messages page MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It’s not a top level page (sits within the dashboard) so it should link back to its parent page. --- app/templates/views/dashboard/inbox.html | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/app/templates/views/dashboard/inbox.html b/app/templates/views/dashboard/inbox.html index 39189c3de..8ea050b0b 100644 --- a/app/templates/views/dashboard/inbox.html +++ b/app/templates/views/dashboard/inbox.html @@ -1,4 +1,5 @@ {% from "components/ajax-block.html" import ajax_block %} +{% from "components/page-header.html" import page_header %} {% extends "withnav_template.html" %} @@ -8,9 +9,10 @@ {% block maincolumn_content %} -

- Received text messages -

+ {{ page_header( + 'Received text messages', + back_link=url_for('main.service_dashboard', service_id=current_service.id) + ) }} {{ ajax_block( partials, From 75927a8d05ac8028b9411a3d27c74d16dc909a5c Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 20 Feb 2020 15:11:10 +0000 Subject: [PATCH 13/18] Fix links to current service pages from guidance MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This wasn’t working because: - macros don’t have access to `current_service` from within their scope - there were no tests for this behaviour --- ...nt-service-link.html => service-link.html} | 6 ++-- .../guidance/branding-and-customisation.html | 10 +++---- .../guidance/create-and-send-messages.html | 4 +-- .../guidance/edit-and-format-messages.html | 8 ++--- .../views/guidance/upload-a-letter.html | 4 +-- tests/app/main/views/test_index.py | 30 ++++++++++++++++++- 6 files changed, 45 insertions(+), 17 deletions(-) rename app/templates/components/{current-service-link.html => service-link.html} (58%) diff --git a/app/templates/components/current-service-link.html b/app/templates/components/service-link.html similarity index 58% rename from app/templates/components/current-service-link.html rename to app/templates/components/service-link.html index 807ba8ac4..c4f128877 100644 --- a/app/templates/components/current-service-link.html +++ b/app/templates/components/service-link.html @@ -1,6 +1,6 @@ -{% macro current_service_link(endpoint, link_text) %} - {% if current_service %} - {{ link_text }} +{% macro service_link(service, endpoint, link_text) %} + {% if service %} + {{ link_text }} {% else %} {{ link_text|capitalize }} {% endif %} diff --git a/app/templates/views/guidance/branding-and-customisation.html b/app/templates/views/guidance/branding-and-customisation.html index cc68bcb75..44d8c1072 100644 --- a/app/templates/views/guidance/branding-and-customisation.html +++ b/app/templates/views/guidance/branding-and-customisation.html @@ -1,5 +1,5 @@ {% extends "content_template.html" %} -{% from "components/current-service-link.html" import current_service_link %} +{% from "components/service-link.html" import service_link %} {% block per_page_title %} Branding and customisation @@ -25,7 +25,7 @@

To change your email branding:

    -
  1. Go to the Email settings section of the {{ current_service_link('main.service_settings', 'settings') }} page.
    2. +
  2. Go to the Email settings section of the {{ service_link(current_service, 'main.service_settings', 'settings') }} page.
  3. Select Change on the Email branding row.
@@ -36,7 +36,7 @@

To add a reply-to email address:

    -
  1. Go to the Email settings section of the {{ current_service_link('main.service_settings', 'settings') }} page.
    2. +
  2. Go to the Email settings section of the {{ service_link(current_service, 'main.service_settings', 'settings') }} page.
  3. Select Manage on the Reply-to email addresses row.
  4. Select Add reply-to address.
@@ -48,7 +48,7 @@

To change the text message sender from the default of ‘GOVUK’:

    -
  1. Go to the Text message settings section of the {{ current_service_link('main.service_settings', 'settings') }} page.
    2. +
  2. Go to the Text message settings section of the {{ service_link(current_service, 'main.service_settings', 'settings') }} page.
  3. Select Manage on the Text message senders row.
  4. Select Change or Add text message sender.
@@ -60,7 +60,7 @@

To add a logo to your letters:

    -
  1. Go to the Letter settings section of the {{ current_service_link('main.service_settings', 'settings') }} page.
    2. +
  2. Go to the Letter settings section of the {{ service_link(current_service, 'main.service_settings', 'settings') }} page.
  3. Select Change on the Letter branding row.
diff --git a/app/templates/views/guidance/create-and-send-messages.html b/app/templates/views/guidance/create-and-send-messages.html index fa905b33d..5f94387d7 100644 --- a/app/templates/views/guidance/create-and-send-messages.html +++ b/app/templates/views/guidance/create-and-send-messages.html @@ -1,5 +1,5 @@ {% extends "content_template.html" %} -{% from "components/current-service-link.html" import current_service_link %} +{% from "components/service-link.html" import service_link %} {% block per_page_title %} Send messages @@ -15,7 +15,7 @@

To send a batch of messages at once:

    -
  1. Go to the {{ current_service_link('main.choose_template', 'templates') }} page and choose an existing template.
    2. +
  2. Go to the {{ service_link(current_service, 'main.choose_template', 'templates') }} page and choose an existing template.
  3. Select Send.
  4. If you’re sending emails, select Upload a list of email addresses. If you’re sending text messages, select Upload a list of phone numbers. If you’re sending letters, select Upload a list of addresses.
    5. diff --git a/app/templates/views/guidance/edit-and-format-messages.html b/app/templates/views/guidance/edit-and-format-messages.html index a80655a81..eb74e2e7d 100644 --- a/app/templates/views/guidance/edit-and-format-messages.html +++ b/app/templates/views/guidance/edit-and-format-messages.html @@ -1,5 +1,5 @@ {% extends "content_template.html" %} -{% from "components/current-service-link.html" import current_service_link %} +{% from "components/service-link.html" import service_link %} {% block per_page_title %} Edit and format messages @@ -22,7 +22,7 @@

    You can see a list of formatting instructions on the edit template page:

      -
    1. Go to the {{ current_service_link('main.choose_template', 'templates') }} page.
      2. +
    2. Go to the {{ service_link(current_service, 'main.choose_template', 'templates') }} page.
    3. Add a new template or choose an existing template and select Edit.
    @@ -61,7 +61,7 @@

    To add a placeholder to the template:

      -
    1. Go to the {{ current_service_link('main.choose_template', 'templates') }} page.
      2. +
    2. Go to the {{ service_link(current_service, 'main.choose_template', 'templates') }} page.
    3. Add a new template or choose an existing template and select Edit.
    4. Add a placeholder using double brackets. For example: Hello ((first name)), your reference is ((ref number)).
    5. Select Save.
      6. @@ -81,7 +81,7 @@

      To add optional content to your emails and text messages:

        -
      1. Go to the {{ current_service_link('main.choose_template', 'templates') }} page.
        2. +
      2. Go to the {{ service_link(current_service, 'main.choose_template', 'templates') }} page.
      3. Add a new template or choose an existing template and select Edit.
      4. Use double brackets and ?? to define optional content. For example, if you only want to show something to people who are under 18: ((under18??Please get your application signed by a parent or guardian.))
      5. Select Save.
        6. diff --git a/app/templates/views/guidance/upload-a-letter.html b/app/templates/views/guidance/upload-a-letter.html index 42f33d765..619b2a5c3 100644 --- a/app/templates/views/guidance/upload-a-letter.html +++ b/app/templates/views/guidance/upload-a-letter.html @@ -1,5 +1,5 @@ {% extends "content_template.html" %} -{% from "components/current-service-link.html" import current_service_link %} +{% from "components/service-link.html" import service_link %} {% block per_page_title %} Upload a letter @@ -14,7 +14,7 @@

        To upload and send a letter from a PDF file:

          -
        1. Go to the {{ current_service_link('main.uploads', 'uploads') }} page.
          2. +
        2. Go to the {{ service_link(current_service, 'main.uploads', 'uploads') }} page.
        3. Select Upload a letter.
        4. Select Choose file.
        diff --git a/tests/app/main/views/test_index.py b/tests/app/main/views/test_index.py index 3b0de3657..bd4cea5a4 100644 --- a/tests/app/main/views/test_index.py +++ b/tests/app/main/views/test_index.py @@ -5,7 +5,7 @@ from bs4 import BeautifulSoup from flask import url_for from app.main.forms import FieldWithNoneOption -from tests.conftest import normalize_spaces, sample_uuid +from tests.conftest import SERVICE_ONE_ID, normalize_spaces, sample_uuid def test_non_logged_in_user_can_see_homepage( @@ -109,6 +109,34 @@ def test_static_pages( request() +def test_guidance_pages_link_to_service_pages_when_signed_in( + client_request, +): + request = partial(client_request.get, 'main.edit_and_format_messages') + selector = '.list-number li a' + + # Check the page loads when user is signed in + page = request() + assert page.select_one(selector)['href'] == url_for( + 'main.choose_template', + service_id=SERVICE_ONE_ID, + ) + + # Check it still works when they don’t have a recent service + with client_request.session_transaction() as session: + session['service_id'] = None + page = request() + assert not page.select_one(selector) + + # Check it still works when they sign out + client_request.logout() + with client_request.session_transaction() as session: + session['service_id'] = None + session['user_id'] = None + page = request() + assert not page.select_one(selector) + + @pytest.mark.parametrize('view, expected_view', [ ('information_risk_management', 'security'), ('old_integration_testing', 'integration_testing'), From f64f5725d16cea8ca3546941e5a36ee2b2709ceb Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 20 Feb 2020 17:48:48 +0000 Subject: [PATCH 14/18] Treat 400s from the api as internal server errors if we expect a 400 (for example, the api returns 400 if the service name is already in use) then we should handle that from the view function so we can correctly display a relevation error message to the user. But if it returns a 400 that we didn't expect (for example, because we sent variables of the wrong type through to an endpoint with a schema), then that is a bug that we should fix as any other raised exception. By treating it as a 500 we encourage users to report it to us, and also will get an alert email --- app/__init__.py | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/app/__init__.py b/app/__init__.py index c2921bafc..10c82a8a1 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -614,15 +614,10 @@ def register_errorhandlers(application): # noqa (C901 too complex) error.message )) error_code = error.status_code - if error_code == 400: - if isinstance(error.message, str): - msg = [error.message] - else: - msg = list(itertools.chain(*[error.message[x] for x in error.message.keys()])) - resp = make_response(render_template("error/400.html", message=msg)) - return useful_headers_after_request(resp) - elif error_code not in [401, 404, 403, 410]: - # probably a 500 or 503 + if error_code not in [401, 404, 403, 410]: + # probably a 500 or 503. + # it might be a 400, which we should handle as if it's an internal server error. If the API might + # legitimately return a 400, we should handle that within the view or the client that calls it. application.logger.exception("API {} failed with status {} message {}".format( error.response.url if error.response else 'unknown', error.status_code, From da4ff4b448ecf509124865697dd83698ce93f343 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 21 Feb 2020 09:29:37 +0000 Subject: [PATCH 15/18] Add Design System class to remove visited state from links Co-Authored-By: Tom Byers --- app/templates/components/service-link.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/templates/components/service-link.html b/app/templates/components/service-link.html index c4f128877..b5906cf5f 100644 --- a/app/templates/components/service-link.html +++ b/app/templates/components/service-link.html @@ -1,6 +1,6 @@ {% macro service_link(service, endpoint, link_text) %} {% if service %} - {{ link_text }} + {{ link_text }} {% else %} {{ link_text|capitalize }} {% endif %} From a5b7e3b93aa1af24d5213ad13a88de4f724514a3 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 21 Feb 2020 11:37:01 +0000 Subject: [PATCH 16/18] Add Design System link classes Co-Authored-By: Tom Byers --- app/templates/views/dashboard/_inbox.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/templates/views/dashboard/_inbox.html b/app/templates/views/dashboard/_inbox.html index ebcd6fb84..a73ee296c 100644 --- a/app/templates/views/dashboard/_inbox.html +++ b/app/templates/views/dashboard/_inbox.html @@ -18,7 +18,7 @@ {% endif %} {% if current_service.returned_letter_summary %} -

      6. message templates with examples of the content you plan to send. You can use our guidance to help you.

        +

        Add message templates with examples of the content you plan to send. You can use our guidance to help you.

      7. Set up your service

        {% if not current_user.is_authenticated or not current_service %}

        Review your settings to add message branding, reply-to addresses and sender information.

        -

        Add team members and check their permissions.

        +

        Add team members and check their permissions.

        {% else %}

        Review your settings to add message branding, reply-to addresses and sender information.

        -

        Add team members and check their permissions.

        +

        Add team members and check their permissions.

        {% endif %}