Merge pull request #3176 from alphagov/fix-update-notification-method

Refactor method for deciding the failure type

app/clients/sms/firetext.py

@@ -21,6 +21,7 @@ firetext_responses = {
 
 firetext_codes = {
     # code '000' means 'No errors reported'
+    '000': {'status': 'temporary-failure', 'reason': 'No error reported'},
     '101': {'status': 'permanent-failure', 'reason': 'Unknown Subscriber'},
     '102': {'status': 'temporary-failure', 'reason': 'Absent Subscriber'},
     '103': {'status': 'temporary-failure', 'reason': 'Subscriber Busy'},

app/dao/notifications_dao.py

@@ -88,21 +88,21 @@ def dao_create_notification(notification):
 
 
 def _decide_permanent_temporary_failure(status, notification, detailed_status_code=None):
-    # If we get failure status from Firetext, we want to know if this is temporary or permanent failure.
+    # Firetext will send us a pending status, followed by a success or failure status.
-    # So we check the failure code to learn that.
+    # When we get a failure status we need to look at the detailed_status_code to determine if the failure type
-    # If there is no failure code, or we do not recognise the failure code, we do the following:
+    # is a permanent-failure or temporary-failure.
-    # if notifitcation goes form status pending to status failure, we mark it as temporary failure;
+    if notification.sent_by == 'firetext':
-    # if notification goes straight to status failure, we mark it as permanent failure.
+        if status == NOTIFICATION_PERMANENT_FAILURE and detailed_status_code:
-    if status == NOTIFICATION_PERMANENT_FAILURE and detailed_status_code not in [None, '000']:
+            try:
-        try:
+                status, reason = get_message_status_and_reason_from_firetext_code(detailed_status_code)
-            status, reason = get_message_status_and_reason_from_firetext_code(detailed_status_code)
+                current_app.logger.info(
-            current_app.logger.info(f'Updating notification id {notification.id} to status {status}, reason: {reason}')
+                    f'Updating notification id {notification.id} to status {status}, reason: {reason}')
-            return status
+                return status
-        except KeyError:
+            except KeyError:
-            current_app.logger.warning(f'Failure code {detailed_status_code} from Firetext not recognised')
+                current_app.logger.warning(f'Failure code {detailed_status_code} from Firetext not recognised')
-    # fallback option:
+        # fallback option:
-    if notification.status == NOTIFICATION_PENDING and status == NOTIFICATION_PERMANENT_FAILURE:
+        if status == NOTIFICATION_PERMANENT_FAILURE and notification.status == NOTIFICATION_PENDING:
-        status = NOTIFICATION_TEMPORARY_FAILURE
+            status = NOTIFICATION_TEMPORARY_FAILURE
     return status
 
 

tests/app/clients/test_firetext.py

@@ -9,7 +9,7 @@ from app.clients.sms.firetext import get_firetext_responses, SmsClientResponseEx
 
 
 @pytest.mark.parametrize('detailed_status_code, result', [
-    (None, ('delivered', None)), ('000', ('delivered', None))
+    (None, ('delivered', None)), ('000', ('delivered', 'No error reported'))
 ])
 def test_get_firetext_responses_should_return_correct_details_for_delivery(detailed_status_code, result):
     assert get_firetext_responses('0', detailed_status_code) == result

tests/app/dao/notification_dao/test_notification_dao.py

@@ -216,7 +216,7 @@ def test_should_by_able_to_update_status_by_id_from_pending_to_delivered(sample_
 
 
 def test_should_by_able_to_update_status_by_id_from_pending_to_temporary_failure(sample_template, sample_job):
-    notification = create_notification(template=sample_template, job=sample_job, status='sending')
+    notification = create_notification(template=sample_template, job=sample_job, status='sending', sent_by='firetext')
 
     assert update_notification_status_by_id(notification_id=notification.id, status='pending')
     assert Notification.query.get(notification.id).status == 'pending'

tests/app/notifications/test_process_client_response.py

@@ -67,7 +67,7 @@ def test_process_sms_client_response_updates_notification_status(
     ('101', 'permanent-failure', 'Unknown Subscriber'),
     ('102', 'temporary-failure', 'Absent Subscriber'),
     (None, 'temporary-failure', None),
-    ('000', 'temporary-failure', None)
+    ('000', 'temporary-failure', 'No error reported')
 ])
 def test_process_sms_client_response_updates_notification_status_when_called_second_time(
     sample_notification,
@@ -82,7 +82,7 @@ def test_process_sms_client_response_updates_notification_status_when_called_sec
 
     process_sms_client_response('1', str(sample_notification.id), 'Firetext', detailed_status_code)
 
-    if detailed_status_code and detailed_status_code != '000':
+    if detailed_status_code:
         message = f'Updating notification id {sample_notification.id} to status {expected_notification_status}, reason: {reason}'  # noqa
         mock_logger.assert_called_with(message)