From 3c6598cbc236c3f142a65ead26f62b104aed7db1 Mon Sep 17 00:00:00 2001
From: Chris Hill-Scott <me@quis.cc>
Date: Mon, 13 May 2019 13:25:39 +0100
Subject: [PATCH] Only link to message status if message has failed

This retores the behaviour to as it was before
https://github.com/alphagov/notifications-admin/pull/2962
which inadvertently started linking to the guidance for messages that
were delivered or in sending.
---
 app/__init__.py                               |  7 +++-
 app/templates/components/table.html           |  6 ++--
 .../partials/notifications/status.html        |  6 ++--
 tests/app/main/test_formatters.py             | 35 +++++++++++++++++++
 4 files changed, 47 insertions(+), 7 deletions(-)
 create mode 100644 tests/app/main/test_formatters.py

diff --git a/app/__init__.py b/app/__init__.py
index 4bbe79a4b..436fae237 100644
--- a/app/__init__.py
+++ b/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')
diff --git a/app/templates/components/table.html b/app/templates/components/table.html
index b1bfbabbb..656ef1c47 100644
--- a/app/templates/components/table.html
+++ b/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 %}
-        <a href="{{ notification.notification_type|format_notification_status_as_url }}">
+      {% if notification.status|format_notification_status_as_url(notification.notification_type) %}
+        <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">
diff --git a/app/templates/partials/notifications/status.html b/app/templates/partials/notifications/status.html
index 8c92ed907..d90e82462 100644
--- a/app/templates/partials/notifications/status.html
+++ b/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 %}
-      <a href="{{ notification.notification_type|format_notification_status_as_url }}">
+    {% if notification.status|format_notification_status_as_url(notification.notification_type) %}
+      <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 %}
diff --git a/tests/app/main/test_formatters.py b/tests/app/main/test_formatters.py
new file mode 100644
index 000000000..5ef5c28da
--- /dev/null
+++ b/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()