Check Firetext code first to determine status and log the reason

If there is no code, fall back to old way of determining status.
If the code is not recognised, log an Error so we are notified
and can look into it.

app/clients/sms/firetext.py

@@ -20,15 +20,15 @@ firetext_responses = {
 }
 
 firetext_codes = {
-    '101': 'permanent-failure',  # Unknown Subscriber
+    '101': {'status': 'permanent-failure', 'reason': 'Unknown Subscriber'},
-    '102': 'temporary-failure',  # Absent Subscriber
+    '102': {'status': 'temporary-failure', 'reason': 'Absent Subscriber'},
-    '103': 'temporary-failure',  # Subscriber Busy
+    '103': {'status': 'temporary-failure', 'reason': 'Subscriber Busy'},
-    '104': 'temporary-failure',  # No Subscriber Memory
+    '104': {'status': 'temporary-failure', 'reason': 'No Subscriber Memory'},
-    '201': 'permanent-failure',  # Invalid Number
+    '201': {'status': 'permanent-failure', 'reason': 'Invalid Number'},
-    '301': 'permanent-failure',  # SMS Not Supported
+    '301': {'status': 'permanent-failure', 'reason': 'SMS Not Supported'},
-    '302': 'temporary-failure',  # SMS Not Supported
+    '302': {'status': 'temporary-failure', 'reason': 'SMS Not Supported'},
-    '401': 'permanent-failure',  # Message Rejected
+    '401': {'status': 'permanent-failure', 'reason': 'Message Rejected'},
-    '900': 'temporary-failure',  # Routing Error
+    '900': {'status': 'temporary-failure', 'reason': 'Routing Error'},
 }
 
 
@@ -36,8 +36,8 @@ def get_firetext_responses(status):
     return firetext_responses[status]
 
 
-def get_message_status_from_firetext_code(code):
+def get_message_status_and_reason_from_firetext_code(code):
-    return firetext_codes[code]
+    return firetext_codes[code]['status'], firetext_codes[code]['reason']
 
 
 class FiretextClientResponseException(SmsClientResponseException):

app/dao/notifications_dao.py

@@ -52,7 +52,7 @@ from app.models import (
 )
 from app.utils import get_london_midnight_in_utc
 from app.utils import midnight_n_days_ago, escape_special_characters
-from app.clients.sms.firetext import get_message_status_from_firetext_code
+from app.clients.sms.firetext import get_message_status_and_reason_from_firetext_code
 
 
 @statsd(namespace="dao")
@@ -90,15 +90,19 @@ def dao_create_notification(notification):
     db.session.add(notification)
 
 
-def _decide_permanent_temporary_failure(current_status, status, code=None):
+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 current_status == NOTIFICATION_PENDING and status == NOTIFICATION_PERMANENT_FAILURE:
+    if code:
-        if code:
+        try:
-            status = get_message_status_from_firetext_code(code)
+            status, reason = get_message_status_and_reason_from_firetext_code(code)
-        else:
+            current_app.logger.info(f'Updating notification id {notification.id} to status {status}, reason: {reason}')
-            status = NOTIFICATION_TEMPORARY_FAILURE
+            return status
+        except KeyError:
+            current_app.logger.error(f'Failure code {code} from Firetext not recognised')
+    if notification.status == NOTIFICATION_PENDING and status == NOTIFICATION_PERMANENT_FAILURE:
+        status = NOTIFICATION_TEMPORARY_FAILURE
     return status
 
 
@@ -108,7 +112,7 @@ def country_records_delivery(phone_prefix):
 
 
 def _update_notification_status(notification, status, code=None):
-    status = _decide_permanent_temporary_failure(current_status=notification.status, status=status, code=code)
+    status = _decide_permanent_temporary_failure(status=status, notification=notification, code=code)
     notification.status = status
     dao_update_notification(notification)
     return notification

tests/app/notifications/test_process_client_response.py

@@ -57,23 +57,45 @@ def test_process_sms_client_response_updates_notification_status(
     assert sample_notification.status == expected_notification_status
 
 
-@pytest.mark.parametrize('code, expected_notification_status', [
+@pytest.mark.parametrize('code, expected_notification_status, reason', [
-    ('101', 'permanent-failure'),
+    ('101', 'permanent-failure', 'Unknown Subscriber'),
-    ('102', 'temporary-failure'),
+    ('102', 'temporary-failure', 'Absent Subscriber'),
+    (None, 'temporary-failure', None),
 ])
 def test_process_sms_client_response_updates_notification_status_when_called_second_time(
     sample_notification,
     mocker,
     code,
     expected_notification_status,
+    reason
 ):
+    mock_logger = mocker.patch('app.celery.tasks.current_app.logger.info')
     sample_notification.status = 'sending'
     process_sms_client_response('2', str(sample_notification.id), 'Firetext')
+
     process_sms_client_response('1', str(sample_notification.id), 'Firetext', code)
 
+    if code:
+        message = f'Updating notification id {sample_notification.id} to status {expected_notification_status}, reason: {reason}'  # noqa
+        mock_logger.assert_called_with(message)
+
     assert sample_notification.status == expected_notification_status
 
 
+def test_process_sms_client_response_updates_notification_status_when_code_unknown(
+    sample_notification,
+    mocker,
+):
+    mock_logger = mocker.patch('app.celery.tasks.current_app.logger.error')
+    sample_notification.status = 'sending'
+    process_sms_client_response('2', str(sample_notification.id), 'Firetext')
+
+    process_sms_client_response('1', str(sample_notification.id), 'Firetext', '789')
+
+    mock_logger.assert_called_once_with('Failure code 789 from Firetext not recognised')
+    assert sample_notification.status == 'temporary-failure'
+
+
 def test_sms_response_does_not_send_callback_if_notification_is_not_in_the_db(sample_service, mocker):
     mocker.patch(
         'app.celery.process_sms_client_response_tasks.get_service_delivery_status_callback_api_for_service',