Merge pull request #2967 from alphagov/fix-linking-notification-status

Only link to message status if message has failed

app/__init__.py

@@ -442,9 +442,14 @@ def format_notification_status_as_field_status(status, notification_type):
     ).get(status, 'error')
 
 
-def format_notification_status_as_url(notification_type):
+def format_notification_status_as_url(status, notification_type):
     url = partial(url_for, "main.message_status")
 
+    if status not in {
+        'technical-failure', 'temporary-failure', 'permanent-failure',
+    }:
+        return None
+
     return {
         'email': url(_anchor='email-statuses'),
         'sms': url(_anchor='sms-statuses')

app/templates/components/table.html

@@ -154,11 +154,11 @@
       align='right'
     ) %}
       {% if displayed_on_single_line %}<span class="align-with-message-body">{% endif %}
-      {% if notification.notification_type|format_notification_status_as_url %}
+      {% if notification.status|format_notification_status_as_url(notification.notification_type) %}
-        <a href="{{ notification.notification_type|format_notification_status_as_url }}">
+        <a href="{{ notification.status|format_notification_status_as_url(notification.notification_type) }}">
       {% endif %}
       {{ notification.status|format_notification_status(notification.template.template_type) }}
-      {% if notification.notification_type|format_notification_status_as_url %}
+      {% if notification.status|format_notification_status_as_url(notification.notification_type) %}
         </a>
       {% endif %}
       <span class="status-hint">

app/templates/partials/notifications/status.html

@@ -1,12 +1,12 @@
 <div class="ajax-block-container">
   <p class="notification-status {{ notification.status|format_notification_status_as_field_status(notification.notification_type) }}">
-    {% if notification.notification_type|format_notification_status_as_url %}
+    {% if notification.status|format_notification_status_as_url(notification.notification_type) %}
-      <a href="{{ notification.notification_type|format_notification_status_as_url }}">
+      <a href="{{ notification.status|format_notification_status_as_url(notification.notification_type) }}">
     {% endif %}
     {{ notification.status|format_notification_status(
       notification.template.template_type
     ) }}
-    {% if notification.notification_type|format_notification_status_as_url %}
+    {% if notification.status|format_notification_status_as_url(notification.notification_type) %}
       </a>
     {% endif %}
     {% if sent_with_test_key %}

tests/app/main/test_formatters.py

@@ -0,0 +1,35 @@
+from functools import partial
+
+import pytest
+from flask import url_for
+
+from app import format_notification_status_as_url
+
+
+@pytest.mark.parametrize('status, notification_type, expected', (
+    # Successful statuses aren’t linked
+    ('created', 'email', lambda: None),
+    ('sending', 'email', lambda: None),
+    ('delivered', 'email', lambda: None),
+    # Failures are linked to the channel-specific page
+    ('temporary-failure', 'email', partial(url_for, 'main.message_status', _anchor='email-statuses')),
+    ('permanent-failure', 'email', partial(url_for, 'main.message_status', _anchor='email-statuses')),
+    ('technical-failure', 'email', partial(url_for, 'main.message_status', _anchor='email-statuses')),
+    ('temporary-failure', 'sms', partial(url_for, 'main.message_status', _anchor='sms-statuses')),
+    ('permanent-failure', 'sms', partial(url_for, 'main.message_status', _anchor='sms-statuses')),
+    ('technical-failure', 'sms', partial(url_for, 'main.message_status', _anchor='sms-statuses')),
+    # Letter statuses are never linked
+    ('technical-failure', 'letter', lambda: None),
+    ('cancelled', 'letter', lambda: None),
+    ('accepted', 'letter', lambda: None),
+    ('received', 'letter', lambda: None),
+))
+def test_format_notification_status_as_url(
+    client,
+    status,
+    notification_type,
+    expected,
+):
+    assert format_notification_status_as_url(
+        status, notification_type
+    ) == expected()