From 9782cbc5b5d8db109ebca9b0697696ca38d9cc0c Mon Sep 17 00:00:00 2001
From: Pea Tyczynska <pea.tyczynska@digital.cabinet-office.gov.uk>
Date: Tue, 21 Apr 2020 13:30:42 +0100
Subject: [PATCH] Check failure code on failure only.

We should check error code on failure only, because if we get an
error code on pending code, we should still set the notification
to pending status.
---
 app/clients/sms/firetext.py                         |  1 +
 app/dao/notifications_dao.py                        | 11 +++++++----
 .../notifications/test_process_client_response.py   | 13 +++++++++++++
 3 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/app/clients/sms/firetext.py b/app/clients/sms/firetext.py
index 2e8086d50..36692d29c 100644
--- a/app/clients/sms/firetext.py
+++ b/app/clients/sms/firetext.py
@@ -20,6 +20,7 @@ firetext_responses = {
 }
 
 firetext_codes = {
+    # code '000' means 'No errors reported'
     '101': {'status': 'permanent-failure', 'reason': 'Unknown Subscriber'},
     '102': {'status': 'temporary-failure', 'reason': 'Absent Subscriber'},
     '103': {'status': 'temporary-failure', 'reason': 'Subscriber Busy'},
diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py
index d959ecb13..a5a40db8a 100644
--- a/app/dao/notifications_dao.py
+++ b/app/dao/notifications_dao.py
@@ -91,16 +91,19 @@ def dao_create_notification(notification):
 
 
 def _decide_permanent_temporary_failure(status, notification, code=None):
-    # Firetext will send pending, then send either succes or fail.
-    # If we go from pending to failure we need to set failure type as temporary-failure,
-    # unless we get a detailed code from firetext. Then we should use that code to set status instead.
-    if code and code != '000':
+    # If we get failure status from Firetext, we want to know if this is temporary or permanent failure.
+    # So we check the failure code to learn that.
+    # If there is no failure code, or we do not recognise the failure code, we do the following:
+    # if notifitcation goes form status pending to status failure, we mark it as temporary failure;
+    # if notification goes straight to status failure, we mark it as permanent failure.
+    if status == NOTIFICATION_PERMANENT_FAILURE and code not in [None, '000']:
         try:
             status, reason = get_message_status_and_reason_from_firetext_code(code)
             current_app.logger.info(f'Updating notification id {notification.id} to status {status}, reason: {reason}')
             return status
         except KeyError:
             current_app.logger.warning(f'Failure code {code} from Firetext not recognised')
+    # fallback option:
     if notification.status == NOTIFICATION_PENDING and status == NOTIFICATION_PERMANENT_FAILURE:
         status = NOTIFICATION_TEMPORARY_FAILURE
     return status
diff --git a/tests/app/notifications/test_process_client_response.py b/tests/app/notifications/test_process_client_response.py
index 9457528e7..859e79eac 100644
--- a/tests/app/notifications/test_process_client_response.py
+++ b/tests/app/notifications/test_process_client_response.py
@@ -83,6 +83,19 @@ def test_process_sms_client_response_updates_notification_status_when_called_sec
     assert sample_notification.status == expected_notification_status
 
 
+@pytest.mark.parametrize('code', ['102', None, '000'])
+def test_process_sms_client_response_updates_notification_status_to_pending_with_and_without_failire_code_present(
+    sample_notification,
+    mocker,
+    code
+):
+    sample_notification.status = 'sending'
+
+    process_sms_client_response('2', str(sample_notification.id), 'Firetext', code)
+
+    assert sample_notification.status == 'pending'
+
+
 def test_process_sms_client_response_updates_notification_status_when_code_unknown(
     sample_notification,
     mocker,