From f9a90485793ff10dffe9d8af672fb97e918572ba Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 11 Jul 2016 10:45:54 +0100 Subject: [PATCH 1/7] =?UTF-8?q?Make=20=E2=80=98temporary=20failure?= =?UTF-8?q?=E2=80=99=20error=20less=20cryptic?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ‘Phone number doesn’t exist’ as opposed to ‘Permanent failure’ has tested well because it talks in terms of things people understand. We should do the same for ‘Temporary failure’, because users are unclear: - why this is happening - if it’s temporary, is Notify going to retry it (it’s not) We think that ‘Phone/inbox not currently accepting messages’ answers makes these things clearer. I’ve reworded it slightly to: - ‘Inbox not accepting messages right now’ - ‘Phone not accepting messages right now’ --- app/__init__.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/app/__init__.py b/app/__init__.py index d4211a033..89e9e91cb 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -235,16 +235,16 @@ def format_notification_status(status, template_type): 'email': { 'failed': 'Failed', 'technical-failure': 'Technical failure', - 'temporary-failure': 'Temporary failure', - 'permanent-failure': 'Email address does not exist', + 'temporary-failure': 'Inbox not accepting messages right now', + 'permanent-failure': 'Email address doesn’t exist', 'delivered': 'Delivered', 'sending': 'Sending' }, 'sms': { 'failed': 'Failed', 'technical-failure': 'Technical failure', - 'temporary-failure': 'Temporary failure', - 'permanent-failure': 'Phone number does not exist', + 'temporary-failure': 'Phone not accepting messages right now', + 'permanent-failure': 'Phone number doesn’t exist', 'delivered': 'Delivered', 'sending': 'Sending' } From 50c20ce68020e5a09f6446a6d9ba3a05082d2310 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 11 Jul 2016 10:49:01 +0100 Subject: [PATCH 2/7] Add formatted notification status to CSV This commit makes the CSV download use the same language for failure reasons as the frontend. It also adds a test around this stuff, which was patchily tested before. --- app/utils.py | 4 +-- tests/__init__.py | 31 +++++++++++++--------- tests/app/main/views/test_jobs.py | 2 +- tests/app/test_utils.py | 44 ++++++++++++++++++++++++++++++- tests/conftest.py | 35 ++++++++++++++++++------ 5 files changed, 92 insertions(+), 24 deletions(-) diff --git a/app/utils.py b/app/utils.py index a6d38de51..6f2c9e3ec 100644 --- a/app/utils.py +++ b/app/utils.py @@ -89,7 +89,7 @@ def get_errors_for_csv(recipients, template_type): def generate_notifications_csv(json_list): - from app import format_datetime + from app import format_datetime, format_notification_status content = StringIO() retval = None with content as csvfile: @@ -102,7 +102,7 @@ def generate_notifications_csv(json_list): x['template']['name'], x['template']['template_type'], x['job']['original_file_name'] if x['job'] else '', - x['status'], + format_notification_status(x['status'], x['template']['template_type']), format_datetime(x['created_at'])]) retval = content.getvalue() return retval diff --git a/tests/__init__.py b/tests/__init__.py index 79dfd64f1..df6d357fb 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -149,13 +149,15 @@ def job_json(service_id, original_file_name="thisisatest.csv", notification_count=1, notifications_sent=1, - status=''): + status=None): if job_id is None: job_id = str(generate_uuid()) if template_id is None: template_id = str(generate_uuid()) if created_at is None: created_at = str(datetime.utcnow().time()) + if status is None: + status = 'Delivered' data = { 'id': job_id, 'service': service_id, @@ -174,16 +176,19 @@ def job_json(service_id, return data -def notification_json(service_id, - job=None, - template=None, - to='07123456789', - status='delivered', - sent_at=None, - job_row_number=None, - created_at=None, - updated_at=None, - with_links=False): +def notification_json( + service_id, + job=None, + template=None, + to='07123456789', + status=None, + sent_at=None, + job_row_number=None, + created_at=None, + updated_at=None, + with_links=False, + rows=5 +): if template is None: template = template_json(service_id, str(generate_uuid())) if sent_at is None: @@ -192,6 +197,8 @@ def notification_json(service_id, created_at = str(datetime.utcnow().time()) if updated_at is None: updated_at = str((datetime.utcnow() + timedelta(minutes=1)).time()) + if status is None: + status = 'delivered' links = {} if with_links: links = { @@ -213,7 +220,7 @@ def notification_json(service_id, 'updated_at': updated_at, 'job_row_number': job_row_number, 'template_version': template['version'] - } for i in range(5)], + } for i in range(rows)], 'total': 5, 'page_size': 50, 'links': links diff --git a/tests/app/main/views/test_jobs.py b/tests/app/main/views/test_jobs.py index 850a6f80a..b7fcc6460 100644 --- a/tests/app/main/views/test_jobs.py +++ b/tests/app/main/views/test_jobs.py @@ -152,7 +152,7 @@ def test_should_show_updates_for_one_job_as_json( assert 'Recipient' in content['notifications'] assert '07123456789' in content['notifications'] assert 'Status' in content['notifications'] - assert job_json['status'] in content['status'] + print(content['notifications']) assert 'Delivered' in content['notifications'] assert '11:10' in content['notifications'] assert 'Uploaded by Test User on 1 January at 11:09' in content['status'] diff --git a/tests/app/test_utils.py b/tests/app/test_utils.py index edb400a33..4f64a5a09 100644 --- a/tests/app/test_utils.py +++ b/tests/app/test_utils.py @@ -1,4 +1,8 @@ -from app.utils import email_safe +import pytest +from io import StringIO +from app.utils import email_safe, generate_notifications_csv +from csv import DictReader +from freezegun import freeze_time def test_email_safe_return_dot_separated_email_domain(): @@ -6,3 +10,41 @@ def test_email_safe_return_dot_separated_email_domain(): expected = 'some.service.withstuff.b123' actual = email_safe(test_name) assert actual == expected + + +@pytest.mark.parametrize( + "status, template_type, expected_status", + [ + ('sending', None, 'Sending'), + ('delivered', None, 'Delivered'), + ('failed', None, 'Failed'), + ('technical-failure', None, 'Technical failure'), + ('temporary-failure', 'email', 'Inbox not accepting messages right now'), + ('permanent-failure', 'email', 'Email address doesn’t exist'), + ('temporary-failure', 'sms', 'Phone not accepting messages right now'), + ('permanent-failure', 'sms', 'Phone number doesn’t exist') + ] +) +@freeze_time("2016-01-01 11:09:00.061258") +def test_generate_csv_from_notifications( + app_, + service_one, + active_user_with_permissions, + mock_get_notifications, + status, + template_type, + expected_status +): + with app_.test_request_context(): + csv_content = generate_notifications_csv( + mock_get_notifications( + service_one['id'], + rows=1, + set_template_type=template_type, + set_status=status + )['notifications'] + ) + + for row in DictReader(StringIO(csv_content)): + assert row['Time'] == 'Friday 01 January 2016 at 11:09' + assert row['Status'] == expected_status diff --git a/tests/conftest.py b/tests/conftest.py index f54853382..35c881a18 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -870,17 +870,36 @@ def mock_get_jobs(mocker, api_user_active): @pytest.fixture(scope='function') def mock_get_notifications(mocker, api_user_active): - def _get_notifications(service_id, - job_id=None, - page=1, - page_size=50, - template_type=None, - status=None, - limit_days=None): + def _get_notifications( + service_id, + job_id=None, + page=1, + page_size=50, + template_type=None, + status=None, + limit_days=None, + rows=5, + set_template_type=None, + set_status=None + ): job = None if job_id is not None: job = job_json(service_id, api_user_active, job_id=job_id) - return notification_json(service_id, job=job) + if set_template_type: + return notification_json( + service_id, + template={'template_type': set_template_type, 'name': 'name', 'id': 'id', 'version': 1}, + rows=rows, + status=set_status, + job=job + ) + else: + return notification_json( + service_id, + rows=rows, + status=set_status, + job=job + ) return mocker.patch( 'app.notification_api_client.get_notifications_for_service', From 8d59b51387738898898144ca998ee6a20a42acf2 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 11 Jul 2016 13:40:27 +0100 Subject: [PATCH 3/7] Use shorter sentences to describe sending state MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It was running on a bit… --- app/templates/views/delivery-and-failure.html | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/templates/views/delivery-and-failure.html b/app/templates/views/delivery-and-failure.html index eb409e73f..a5050a7e3 100644 --- a/app/templates/views/delivery-and-failure.html +++ b/app/templates/views/delivery-and-failure.html @@ -25,9 +25,9 @@ Delivery and failure – GOV.UK Notify

Sending

-

All new messages start with the state ‘Sending’.

+

All messages start in the ‘Sending’ state.

-

This means that we have accepted the message, the message is waiting in a queue to be sent to our email or text message delivery partners.

+

This means that we have accepted the message. It’s waiting in a queue to be sent to our email or text message delivery partners.

Delivered

From 90bdf51e3a65b6263ce33122fbfee8818a3fb3f2 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 11 Jul 2016 13:41:43 +0100 Subject: [PATCH 4/7] Rename failure status on delivery page MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit …so that they agree with what we show in the interface/CSV, eg ‘phone does not exist’ instead of ‘permanent failure’. --- app/templates/views/delivery-and-failure.html | 21 +++++++++++-------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/app/templates/views/delivery-and-failure.html b/app/templates/views/delivery-and-failure.html index a5050a7e3..22221bcb0 100644 --- a/app/templates/views/delivery-and-failure.html +++ b/app/templates/views/delivery-and-failure.html @@ -16,8 +16,9 @@ Delivery and failure – GOV.UK Notify @@ -35,19 +36,21 @@ Delivery and failure – GOV.UK Notify

We can’t tell you if they’ve read it – to do so would require invasive and unreliable tracking techniques.

-

Permanently failed

+

Phone number or email address does not exist

-

This means the email address or mobile number doesn’t exist or is blacklisted – also known as a ‘hard bounce’.

+

You’re still billed for text messages to non-existant phone numbers.

-

You’re still billed for these text messages.

+

You need to remove the email address or mobile number from your database.

-

You need to remove this email address or mobile number from your database.

+

Inbox not accepting messages right now

-

Temporarily failed

+

This can happen for a number of reasons, eg the user’s inbox was full.

-

This means the email address or mobile number was full, or the mobile phone was switched off – also known as a ‘soft bounce’.

+

You can choose to retry this message later or not.

-

We mark messages as ‘Temporarily failed’.

+

Phone not accepting messages right now

+ +

This means the user’s phone was full or hasn’t been switched on in the last 72 hours.

You’re still billed for these messages.

From d22b2f678dff365db79d1a9610efbb24e9913d13 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 11 Jul 2016 13:42:26 +0100 Subject: [PATCH 5/7] =?UTF-8?q?Make=20=E2=80=98message=E2=80=99=20plural?= =?UTF-8?q?=20when=20talking=20about=20retries?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In each of these cases the previous sentence talks about messages, plural, so these lines should agree. --- app/templates/views/delivery-and-failure.html | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/app/templates/views/delivery-and-failure.html b/app/templates/views/delivery-and-failure.html index 22221bcb0..4a9153ef8 100644 --- a/app/templates/views/delivery-and-failure.html +++ b/app/templates/views/delivery-and-failure.html @@ -40,13 +40,13 @@ Delivery and failure – GOV.UK Notify

You’re still billed for text messages to non-existant phone numbers.

-

You need to remove the email address or mobile number from your database.

+

You need to remove these email addresses or phone numbers from your database.

Inbox not accepting messages right now

This can happen for a number of reasons, eg the user’s inbox was full.

-

You can choose to retry this message later or not.

+

You can choose to retry these messages later or not.

Phone not accepting messages right now

@@ -54,7 +54,7 @@ Delivery and failure – GOV.UK Notify

You’re still billed for these messages.

-

You can choose to retry this message later or not.

+

You can choose to retry these messages later or not.

Technical failure

@@ -64,7 +64,7 @@ Delivery and failure – GOV.UK Notify

You won’t be billed for these messages.

-

You need to retry this message yourself later.

+

You need to retry these messages yourself later.

From 5f67560b1ebce9a78f06f306d76f4e1cc74a90e3 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 11 Jul 2016 13:53:55 +0100 Subject: [PATCH 6/7] Link to failure reasons from job/activity pages MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If something has failed and you don’t know why, you should be able to find out why. Let’s try adding a link to the page explaining why, so it’s not just buried in the footer. --- app/__init__.py | 14 +++++++++++++- app/templates/partials/jobs/notifications.html | 6 ++++++ app/templates/views/delivery-and-failure.html | 2 ++ app/templates/views/notifications.html | 6 ++++++ 4 files changed, 27 insertions(+), 1 deletion(-) diff --git a/app/__init__.py b/app/__init__.py index 89e9e91cb..e2852b6b3 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -12,7 +12,9 @@ from flask import ( make_response, current_app, request, - g) + g, + url_for +) from flask._compat import string_types from flask.globals import _lookup_req_object from flask_login import LoginManager @@ -112,6 +114,7 @@ def create_app(): application.add_template_filter(format_date_short) application.add_template_filter(format_notification_status) application.add_template_filter(format_notification_status_as_field_status) + application.add_template_filter(format_notification_status_as_url) application.after_request(useful_headers_after_request) application.after_request(save_service_after_request) @@ -262,6 +265,15 @@ def format_notification_status_as_field_status(status): }.get(status, 'error') +def format_notification_status_as_url(status): + url = partial(url_for, "main.delivery_and_failure") + return { + 'technical-failure': url(_anchor='technical-failure'), + 'temporary-failure': url(_anchor='not-accepting-messages'), + 'permanent-failure': url(_anchor='does-not-exist') + }.get(status) + + @login_manager.user_loader def load_user(user_id): return user_api_client.get_user(user_id) diff --git a/app/templates/partials/jobs/notifications.html b/app/templates/partials/jobs/notifications.html index 506c2007d..74971d1d8 100644 --- a/app/templates/partials/jobs/notifications.html +++ b/app/templates/partials/jobs/notifications.html @@ -34,7 +34,13 @@ align='right', status=item.status|format_notification_status_as_field_status ) %} + {% if item.status|format_notification_status_as_url %} + + {% endif %} {{ item.status|format_notification_status(item.template.template_type) }} + {% if item.status|format_notification_status_as_url %} + + {% endif %} {% endcall %} {% endcall %} diff --git a/app/templates/views/delivery-and-failure.html b/app/templates/views/delivery-and-failure.html index 4a9153ef8..ac3c9a8f8 100644 --- a/app/templates/views/delivery-and-failure.html +++ b/app/templates/views/delivery-and-failure.html @@ -42,6 +42,8 @@ Delivery and failure – GOV.UK Notify

You need to remove these email addresses or phone numbers from your database.

+ +

Inbox not accepting messages right now

This can happen for a number of reasons, eg the user’s inbox was full.

diff --git a/app/templates/views/notifications.html b/app/templates/views/notifications.html index 592eb3ec1..69d8ed40e 100644 --- a/app/templates/views/notifications.html +++ b/app/templates/views/notifications.html @@ -73,7 +73,13 @@ ) }} {% call field(status=item.status|format_notification_status_as_field_status, align='right') %} + {% if item.status|format_notification_status_as_url %} + + {% endif %} {{ item.status|format_notification_status(item.template.template_type) }} + {% if item.status|format_notification_status_as_url %} + + {% endif %} {% endcall %} {% endcall %} From 488de1ce7c86cf930ea4e72408049205fef0bbad Mon Sep 17 00:00:00 2001 From: Pete Herlihy Date: Mon, 11 Jul 2016 15:01:28 +0100 Subject: [PATCH 7/7] Removed 'states are the same for email and text' blurb. --- app/templates/views/delivery-and-failure.html | 2 -- 1 file changed, 2 deletions(-) diff --git a/app/templates/views/delivery-and-failure.html b/app/templates/views/delivery-and-failure.html index ac3c9a8f8..e5de2bc10 100644 --- a/app/templates/views/delivery-and-failure.html +++ b/app/templates/views/delivery-and-failure.html @@ -22,8 +22,6 @@ Delivery and failure – GOV.UK Notify
  • Technical failure
    • -

    Our delivery states are the same for both email and text message.

    -

    Sending

    All messages start in the ‘Sending’ state.