From f3d1dc4a9c93c466b70979cd8e1426158154e6a8 Mon Sep 17 00:00:00 2001
From: Chris Hill-Scott <me@quis.cc>
Date: Thu, 15 Nov 2018 17:04:11 +0000
Subject: [PATCH] =?UTF-8?q?Don=E2=80=99t=20ask=20for=20data=20retention=20?=
 =?UTF-8?q?unless=20channel=20is=20known?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

If the user is looking at the notifications page for all message types
(which is what we show ‘caseworkers’) then it doesn’t make sense to ask
the API for the data retention period for that message type (because
it will be `None`). Doing so causes the API to return a `404`, which
then causes the admin app to return `404`.

Passing through `None` as the value of limit days will just cause the
API to return everything in the `notifications` table, which is fine
for us.
---
 app/main/views/jobs.py                |  8 +++--
 tests/app/main/views/test_activity.py | 43 +++++++++++++++++++++++----
 2 files changed, 43 insertions(+), 8 deletions(-)

diff --git a/app/main/views/jobs.py b/app/main/views/jobs.py
index cf50029f1..107a096e5 100644
--- a/app/main/views/jobs.py
+++ b/app/main/views/jobs.py
@@ -216,10 +216,12 @@ def get_notifications(service_id, message_type, status_override=None):
         abort(404)
     filter_args = parse_filter_args(request.args)
     filter_args['status'] = set_status_filters(filter_args)
+    service_data_retention_days = None
 
-    service_data_retention_days = service_api_client.get_service_data_retention_by_notification_type(
-        service_id, message_type
-    ).get('days_of_retention', current_app.config['ACTIVITY_STATS_LIMIT_DAYS'])
+    if message_type is not None:
+        service_data_retention_days = service_api_client.get_service_data_retention_by_notification_type(
+            service_id, message_type
+        ).get('days_of_retention', current_app.config['ACTIVITY_STATS_LIMIT_DAYS'])
 
     if request.path.endswith('csv') and current_user.has_permissions('view_activity'):
         return Response(
diff --git a/tests/app/main/views/test_activity.py b/tests/app/main/views/test_activity.py
index 2a87968d2..f4327a1e8 100644
--- a/tests/app/main/views/test_activity.py
+++ b/tests/app/main/views/test_activity.py
@@ -21,10 +21,28 @@ from tests.conftest import (
 
 
 @pytest.mark.parametrize(
-    "user,extra_args,expected_update_endpoint,page_title", [
-        (active_user_view_permissions, {'message_type': 'email'}, '/email.json', 'Emails'),
-        (active_user_view_permissions, {'message_type': 'sms'}, '/sms.json', 'Text messages'),
-        (active_caseworking_user, {}, '.json', 'Sent messages'),
+    "user,extra_args,expected_update_endpoint,expected_limit_days,page_title", [
+        (
+            active_user_view_permissions,
+            {'message_type': 'email'},
+            '/email.json',
+            7,
+            'Emails',
+        ),
+        (
+            active_user_view_permissions,
+            {'message_type': 'sms'},
+            '/sms.json',
+            7,
+            'Text messages',
+        ),
+        (
+            active_caseworking_user,
+            {},
+            '.json',
+            None,
+            'Sent messages',
+        ),
     ]
 )
 @pytest.mark.parametrize(
@@ -76,6 +94,7 @@ def test_can_show_notifications(
     user,
     extra_args,
     expected_update_endpoint,
+    expected_limit_days,
     page_title,
     status_argument,
     expected_api_call,
@@ -131,7 +150,7 @@ def test_can_show_notifications(
     assert 'to' not in query_dict
 
     mock_get_notifications.assert_called_with(
-        limit_days=7,
+        limit_days=expected_limit_days,
         page=expected_page_argument,
         service_id=service_one['id'],
         status=expected_api_call,
@@ -149,6 +168,20 @@ def test_can_show_notifications(
     assert json_content.keys() == {'counts', 'notifications', 'service_data_retention_days'}
 
 
+def test_can_show_notifications_if_data_retention_not_available(
+    client_request,
+    mock_get_notifications,
+    mock_get_service_statistics,
+    mock_has_no_jobs,
+):
+    page = client_request.get(
+        'main.view_notifications',
+        service_id=SERVICE_ONE_ID,
+        status='sending,delivered,failed',
+    )
+    assert page.h1.text.strip() == 'Messages'
+
+
 @pytest.mark.parametrize('user, query_parameters, expected_download_link', [
     (
         active_user_with_permissions,