From 04fa99b3dc043fbee5e0f0d801265cdc15fc8088 Mon Sep 17 00:00:00 2001
From: David McDonald <david.mcdonald@digital.cabinet-office.gov.uk>
Date: Tue, 14 Dec 2021 15:40:12 +0000
Subject: [PATCH] Update `get_notifications_for_service` to make POST requests

This updates the `get_notifications_for_service` of the
`NotificationApiClient` to make POST request to the api when the `to`
field is provided. This is done so that we avoid logging personal
details such as email addreses.

If the `to` field is not present, the method will make a GET reqeust and
there will be no change to how it works.
---
 app/notify_client/notification_api_client.py  | 14 ++++---
 .../notify_client/test_notification_client.py | 39 ++++++++++++++++++-
 2 files changed, 47 insertions(+), 6 deletions(-)

diff --git a/app/notify_client/notification_api_client.py b/app/notify_client/notification_api_client.py
index 87898fe55..5e38a8835 100644
--- a/app/notify_client/notification_api_client.py
+++ b/app/notify_client/notification_api_client.py
@@ -19,7 +19,6 @@ class NotificationApiClient(NotifyAdminAPIClient):
         to=None,
         include_one_off=None,
     ):
-        # TODO: if "to" is included, this should be a POST
         params = {
             'page': page,
             'page_size': page_size,
@@ -35,17 +34,22 @@ class NotificationApiClient(NotifyAdminAPIClient):
 
         params = {k: v for k, v in params.items() if v is not None}
 
+        # if `to` is set it is likely PII like an email address or mobile which
+        # we do not want in our logs, so we do a POST request instead of a GET
+        method = self.post if to else self.get
+        kwargs = {'data': params} if to else {'params': params}
+
         if job_id:
-            return self.get(
+            return method(
                 url='/service/{}/job/{}/notifications'.format(service_id, job_id),
-                params=params
+                **kwargs
             )
         else:
             if limit_days is not None:
                 params['limit_days'] = limit_days
-            return self.get(
+            return method(
                 url='/service/{}/notifications'.format(service_id),
-                params=params
+                **kwargs
             )
 
     def send_notification(self, service_id, *, template_id, recipient, personalisation, sender_id):
diff --git a/tests/app/notify_client/test_notification_client.py b/tests/app/notify_client/test_notification_client.py
index 5741fe219..408d548ce 100644
--- a/tests/app/notify_client/test_notification_client.py
+++ b/tests/app/notify_client/test_notification_client.py
@@ -23,6 +23,10 @@ from tests import notification_json, single_notification_json
         {'include_from_test_key': True},
         {'url': '/service/abcd1234/notifications', 'params': {'include_from_test_key': True}}
     ),
+    (
+        {'page': 48, 'limit_days': 3},
+        {'url': '/service/abcd1234/notifications', 'params': {'page': 48, 'limit_days': 3}}
+    ),
     (
         {'job_id': 'efgh5678'},
         {'url': '/service/abcd1234/job/efgh5678/notifications', 'params': {}}
@@ -30,7 +34,11 @@ from tests import notification_json, single_notification_json
     (
         {'job_id': 'efgh5678', 'page': 48},
         {'url': '/service/abcd1234/job/efgh5678/notifications', 'params': {'page': 48}}
-    )
+    ),
+    (
+        {'job_id': 'efgh5678', 'page': 48, 'limit_days': 3},
+        {'url': '/service/abcd1234/job/efgh5678/notifications', 'params': {'page': 48}}
+    ),
 ])
 def test_client_gets_notifications_for_service_and_job_by_page(mocker, arguments, expected_call):
 
@@ -39,6 +47,35 @@ def test_client_gets_notifications_for_service_and_job_by_page(mocker, arguments
     mock_get.assert_called_once_with(**expected_call)
 
 
+@pytest.mark.parametrize("arguments,expected_call", [
+    (
+        {'to': "0793433"},
+        {'url': '/service/abcd1234/notifications', 'data': {'to': "0793433"}}
+    ),
+    (
+        {'to': "0793433", 'job_id': 'efgh5678'},
+        {'url': '/service/abcd1234/job/efgh5678/notifications', 'data': {'to': "0793433"}}
+    ),
+    (
+        {'to': "0793433", 'page': 99},
+        {'url': '/service/abcd1234/notifications', 'data': {'to': "0793433", 'page': 99}}
+    ),
+    (
+        {'to': "0793433", 'limit_days': 3},
+        {'url': '/service/abcd1234/notifications', 'data': {'to': "0793433", 'limit_days': 3}}
+    ),
+    (
+        {'to': "0793433", 'job_id': 'efgh5678', 'limit_days': 3},
+        {'url': '/service/abcd1234/job/efgh5678/notifications', 'data': {'to': "0793433"}}
+    ),
+])
+def test_client_gets_notifications_for_service_and_job_by_page_posts_for_to(mocker, arguments, expected_call):
+
+    mock_post = mocker.patch('app.notify_client.notification_api_client.NotificationApiClient.post')
+    NotificationApiClient().get_notifications_for_service('abcd1234', **arguments)
+    mock_post.assert_called_once_with(**expected_call)
+
+
 def test_send_notification(mocker, logged_in_client, active_user_with_permissions):
     mock_post = mocker.patch('app.notify_client.notification_api_client.NotificationApiClient.post')
     NotificationApiClient().send_notification(