Merge pull request #775 from alphagov/reasons-for-failure

Make ‘temporary failure’ clearer in frontend and CSV downloads

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)
@@ -235,16 +238,16 @@ def format_notification_status(status, template_type):
         'email': {
             'failed': 'Failed',
             'technical-failure': 'Technical failure',
-            'temporary-failure': 'Temporary failure',
+            'temporary-failure': 'Inbox not accepting messages right now',
-            'permanent-failure': 'Email address does not exist',
+            'permanent-failure': 'Email address doesn’t exist',
             'delivered': 'Delivered',
             'sending': 'Sending'
         },
         'sms': {
             'failed': 'Failed',
             'technical-failure': 'Technical failure',
-            'temporary-failure': 'Temporary failure',
+            'temporary-failure': 'Phone not accepting messages right now',
-            'permanent-failure': 'Phone number does not exist',
+            'permanent-failure': 'Phone number doesn’t exist',
             'delivered': 'Delivered',
             'sending': 'Sending'
         }
@@ -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)

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 %}
+        <a href="{{ 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 %}
+        </a>
+      {% endif %}
     {% endcall %}
   {% endcall %}
 

app/templates/views/delivery-and-failure.html

@@ -16,18 +16,17 @@ Delivery and failure – GOV.UK Notify
     <ul class="list list-bullet">
       <li><a href="#sending">Sending</a></li>
       <li><a href="#delivered">Delivered</a></li>
-      <li><a href="#permanent-failure">Permanently failed</a></li>
+      <li><a href="#does-not-exist">Phone number or email address does not exist</a></li>
-      <li><a href="#temporary-failure">Temporarily failed</a></li>
+      <li><a href="#inbox-not-accepting-messages">Inbox not accepting messages right now</a></li>
+      <li><a href="#phone-not-accepting-messages">Phone not accepting messages right now</a></li>
       <li><a href="#technical-failure">Technical failure</a></li>
     </ul>
 
-    <p>Our delivery states are the same for both email and text message.</p>
-
     <h2 id="sending" class="heading-medium">Sending</h2>
 
-    <p>All new messages start with the state ‘Sending’.</p>
+    <p>All messages start in the ‘Sending’ state.</p>
 
-    <p>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.</p>
+    <p>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.</p>
 
     <h2 id="delivered" class="heading-medium">Delivered</h2>
 
@@ -35,23 +34,27 @@ Delivery and failure – GOV.UK Notify
 
     <p>We can’t tell you if they’ve read it – to do so would require invasive and unreliable tracking techniques.</p>
 
-    <h2 id="permanent-failure" class="heading-medium">Permanently failed</h2>
+    <h2 id="does-not-exist" class="heading-medium">Phone number or email address does not exist</h2>
 
-    <p>This means the email address or mobile number doesn’t exist or is blacklisted – also known as a ‘hard bounce’.</p>
+    <p>You’re still billed for text messages to non-existant phone numbers.</p>
 
-    <p>You’re still billed for these text messages.</p>
+    <p><strong>You need to remove these email addresses or phone numbers from your database.</strong></p>
 
-    <p><strong>You need to remove this email address or mobile number from your database.</strong></p>
+    <a id="not-accepting-messages"></a>
 
-    <h2 id="temporary-failure" class="heading-medium">Temporarily failed</h2>
+    <h2 id="inbox-not-accepting-messages" class="heading-medium">Inbox not accepting messages right now</h2>
 
-    <p>This means the email address or mobile number was full, or the mobile phone was switched off – also known as a ‘soft bounce’.</p>
+    <p>This can happen for a number of reasons, eg the user’s inbox was full.</p>
 
-    <p>We mark messages as ‘Temporarily failed’.</p>
+    <p><strong>You can choose to retry these messages later or not.</strong></p>
+
+    <h2 id="phone-not-accepting-messages" class="heading-medium">Phone not accepting messages right now</h2>
+
+    <p>This means the user’s phone was full or hasn’t been switched on in the last 72 hours.</p>
 
     <p>You’re still billed for these messages.</p>
 
-    <p><strong>You can choose to retry this message later or not.</strong></p>
+    <p><strong>You can choose to retry these messages later or not.</strong></p>
 
     <h2 id="technical-failure" class="heading-medium">Technical failure</h2>
 
@@ -61,7 +64,7 @@ Delivery and failure – GOV.UK Notify
 
     <p>You won’t be billed for these messages.</p>
 
-    <p><strong>You need to retry this message yourself later.</strong></p>
+    <p><strong>You need to retry these messages yourself later.</strong></p>
 
   </div>
 </div>

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 %}
+          <a href="{{ 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 %}
+          </a>
+        {% endif %}
       {% endcall %}
 
     {% endcall %}

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

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,
+def notification_json(
-                      job=None,
+    service_id,
-                      template=None,
+    job=None,
-                      to='07123456789',
+    template=None,
-                      status='delivered',
+    to='07123456789',
-                      sent_at=None,
+    status=None,
-                      job_row_number=None,
+    sent_at=None,
-                      created_at=None,
+    job_row_number=None,
-                      updated_at=None,
+    created_at=None,
-                      with_links=False):
+    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

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']

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

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,
+    def _get_notifications(
-                           job_id=None,
+        service_id,
-                           page=1,
+        job_id=None,
-                           page_size=50,
+        page=1,
-                           template_type=None,
+        page_size=50,
-                           status=None,
+        template_type=None,
-                           limit_days=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',