Merge pull request #2803 from alphagov/firetext-response-codes

Use firetext response code to see if temporary or permanent failure if available

app/celery/process_sms_client_response_tasks.py

@@ -24,7 +24,7 @@ sms_response_mapper = {
 
 @notify_celery.task(bind=True, name="process-sms-client-response", max_retries=5, default_retry_delay=300)
 @statsd(namespace="tasks")
-def process_sms_client_response(self, status, provider_reference, client_name):
+def process_sms_client_response(self, status, provider_reference, client_name, code=None):
     # validate reference
     try:
         uuid.UUID(provider_reference, version=4)
@@ -51,16 +51,18 @@ def process_sms_client_response(self, status, provider_reference, client_name):
     _process_for_status(
         notification_status=notification_status,
         client_name=client_name,
-        provider_reference=provider_reference
+        provider_reference=provider_reference,
+        code=code
     )
 
 
-def _process_for_status(notification_status, client_name, provider_reference):
+def _process_for_status(notification_status, client_name, provider_reference, code=None):
     # record stats
     notification = notifications_dao.update_notification_status_by_id(
         notification_id=provider_reference,
         status=notification_status,
-        sent_by=client_name.lower()
+        sent_by=client_name.lower(),
+        code=code
     )
     if not notification:
         return

app/clients/sms/firetext.py

@@ -19,11 +19,27 @@ firetext_responses = {
     '2': 'pending'
 }
 
+firetext_codes = {
+    '101': {'status': 'permanent-failure', 'reason': 'Unknown Subscriber'},
+    '102': {'status': 'temporary-failure', 'reason': 'Absent Subscriber'},
+    '103': {'status': 'temporary-failure', 'reason': 'Subscriber Busy'},
+    '104': {'status': 'temporary-failure', 'reason': 'No Subscriber Memory'},
+    '201': {'status': 'permanent-failure', 'reason': 'Invalid Number'},
+    '301': {'status': 'permanent-failure', 'reason': 'SMS Not Supported'},
+    '302': {'status': 'temporary-failure', 'reason': 'SMS Not Supported'},
+    '401': {'status': 'permanent-failure', 'reason': 'Message Rejected'},
+    '900': {'status': 'temporary-failure', 'reason': 'Routing Error'},
+}
+
 
 def get_firetext_responses(status):
     return firetext_responses[status]
 
 
+def get_message_status_and_reason_from_firetext_code(code):
+    return firetext_codes[code]['status'], firetext_codes[code]['reason']
+
+
 class FiretextClientResponseException(SmsClientResponseException):
     def __init__(self, response, exception):
         status_code = response.status_code if response is not None else 504

app/dao/notifications_dao.py

@@ -52,6 +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_and_reason_from_firetext_code
 
 
 @statsd(namespace="dao")
@@ -89,10 +90,18 @@ def dao_create_notification(notification):
     db.session.add(notification)
 
 
-def _decide_permanent_temporary_failure(current_status, status):
+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 delivered we need to set failure type as temporary-failure
+    # If we go from pending to failure we need to set failure type as temporary-failure,
-    if current_status == NOTIFICATION_PENDING and status == NOTIFICATION_PERMANENT_FAILURE:
+    # unless we get a detailed code from firetext. Then we should use that code to set status instead.
+    if code:
+        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.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
 
@@ -102,8 +111,8 @@ def country_records_delivery(phone_prefix):
     return dlr and dlr.lower() == 'yes'
 
 
-def _update_notification_status(notification, status):
+def _update_notification_status(notification, status, code=None):
-    status = _decide_permanent_temporary_failure(current_status=notification.status, status=status)
+    status = _decide_permanent_temporary_failure(status=status, notification=notification, code=code)
     notification.status = status
     dao_update_notification(notification)
     return notification
@@ -111,7 +120,7 @@ def _update_notification_status(notification, status):
 
 @statsd(namespace="dao")
 @transactional
-def update_notification_status_by_id(notification_id, status, sent_by=None):
+def update_notification_status_by_id(notification_id, status, sent_by=None, code=None):
     notification = Notification.query.with_for_update().filter(Notification.id == notification_id).first()
 
     if not notification:
@@ -137,7 +146,8 @@ def update_notification_status_by_id(notification_id, status, sent_by=None):
         notification.sent_by = sent_by
     return _update_notification_status(
         notification=notification,
-        status=status
+        status=status,
+        code=code
     )
 
 

app/notifications/notifications_sms_callback.py

@@ -48,6 +48,7 @@ def process_firetext_response():
         raise InvalidRequest(errors, status_code=400)
 
     status = request.form.get('status')
+    code = request.form.get('code')
     provider_reference = request.form.get('reference')
 
     safe_to_log = dict(request.form).copy()
@@ -57,7 +58,7 @@ def process_firetext_response():
     )
 
     process_sms_client_response.apply_async(
-        [status, provider_reference, client_name],
+        [status, provider_reference, client_name, code],
         queue=QueueNames.SMS_CALLBACKS,
     )
 

tests/app/notifications/rest/test_callbacks.py

@@ -147,7 +147,23 @@ def test_firetext_callback_should_return_200_and_call_task_with_valid_data(clien
     assert json_resp['result'] == 'success'
 
     mock_celery.assert_called_once_with(
-        ['0', 'notification_id', 'Firetext'],
+        ['0', 'notification_id', 'Firetext', None],
+        queue='sms-callbacks',
+    )
+
+
+def test_firetext_callback_including_a_code_should_return_200_and_call_task_with_valid_data(client, mocker):
+    mock_celery = mocker.patch(
+        'app.notifications.notifications_sms_callback.process_sms_client_response.apply_async')
+
+    data = 'mobile=441234123123&status=1&code=101&time=2016-03-10 14:17:00&reference=notification_id'
+    response = firetext_post(client, data)
+    json_resp = json.loads(response.get_data(as_text=True))
+    assert response.status_code == 200
+    assert json_resp['result'] == 'success'
+
+    mock_celery.assert_called_once_with(
+        ['1', 'notification_id', 'Firetext', '101'],
         queue='sms-callbacks',
     )
 

tests/app/notifications/test_process_client_response.py

@@ -57,6 +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, reason', [
+    ('101', 'permanent-failure', 'Unknown Subscriber'),
+    ('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',