Merge pull request #2856 from alphagov/mmg_detailed_response

Capture detailed delivery receipt status from MMG

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, code=None):
+def process_sms_client_response(self, status, provider_reference, client_name, detailed_status_code=None):
     # validate reference
     try:
         uuid.UUID(provider_reference, version=4)
@@ -36,9 +36,10 @@ def process_sms_client_response(self, status, provider_reference, client_name, c
 
     # validate status
     try:
-        notification_status = response_parser(status)
+        notification_status, detailed_status = response_parser(status, detailed_status_code)
         current_app.logger.info(
-            f'{client_name} callback returned status of {status} for reference: {provider_reference}'
+            f'{client_name} callback returned '
+            f'status of {notification_status}: {detailed_status} for reference: {provider_reference}'
         )
     except KeyError:
         _process_for_status(
@@ -52,17 +53,17 @@ def process_sms_client_response(self, status, provider_reference, client_name, c
         notification_status=notification_status,
         client_name=client_name,
         provider_reference=provider_reference,
-        code=code
+        detailed_status_code=detailed_status_code
     )
 
 
-def _process_for_status(notification_status, client_name, provider_reference, code=None):
+def _process_for_status(notification_status, client_name, provider_reference, detailed_status_code=None):
     # record stats
     notification = notifications_dao.update_notification_status_by_id(
         notification_id=provider_reference,
         status=notification_status,
         sent_by=client_name.lower(),
-        code=code
+        detailed_status_code=detailed_status_code
     )
     if not notification:
         return

app/clients/sms/firetext.py

@@ -33,12 +33,15 @@ firetext_codes = {
 }
 
 
-def get_firetext_responses(status):
+def get_firetext_responses(status, detailed_status_code=None):
-    return firetext_responses[status]
+    detailed_status = firetext_codes[detailed_status_code]['reason'] if firetext_codes.get(
+        detailed_status_code, None
+    ) else None
+    return (firetext_responses[status], detailed_status)
 
 
-def get_message_status_and_reason_from_firetext_code(code):
+def get_message_status_and_reason_from_firetext_code(detailed_status_code):
-    return firetext_codes[code]['status'], firetext_codes[code]['reason']
+    return firetext_codes[detailed_status_code]['status'], firetext_codes[detailed_status_code]['reason']
 
 
 class FiretextClientResponseException(SmsClientResponseException):

app/clients/sms/mmg.py

@@ -4,15 +4,49 @@ from requests import (request, RequestException)
 from app.clients.sms import (SmsClient, SmsClientResponseException)
 
 mmg_response_map = {
-    '2': 'permanent-failure',
+    '2': {'status': 'permanent-failure', 'substatus': {
-    '3': 'delivered',
+        "1": "Number does not exist",
-    '4': 'temporary-failure',
+        "4": "Rejected by operator",
-    '5': 'permanent-failure'
+        "5": "Unidentified Subscriber",
+        "9": "Undelivered",
+        "11": "Service for Subscriber suspended",
+        "12": "Illegal equipment",
+        "2049": "Subscriber IMSI blacklisted",
+        "2050": "Number blacklisted in do-not-disturb blacklist",
+        "2052": "Destination number blacklisted",
+        "2053": "Source address blacklisted"
+    }},
+    '3': {'status': 'delivered', 'substatus': {"2": "Delivered to operator", "5": "Delivered to handset"}},
+    '4': {'status': 'temporary-failure', 'substatus': {
+        "6": "Absent Subscriber",
+        "8": "Roaming not allowed",
+        "13": "SMS Not Supported",
+        "15": "Expired",
+        "27": "Absent Subscriber",
+        "29": "Invalid delivery report",
+        "32": "Delivery Failure",
+    }},
+    '5': {'status': 'permanent-failure', 'substatus': {
+        "6": "Network out of coverage",
+        "8": "Incorrect number prefix",
+        "10": "Number on do-not-disturb service",
+        "11": "Sender id not registered",
+        "13": "Sender id blacklisted",
+        "14": "Destination number blacklisted",
+        "19": "Routing unavailable",
+        "20": "Rejected by anti-flooding mechanism",
+        "21": "System error",  # it says to retry those messages or contact support
+        "23": "Duplicate message id",
+        "24": "Message formatted incorrectly",
+        "25": "Message too long",
+        "51": "Missing recipient value",
+        "52": "Invalid destination",
+    }},
 }
 
 
-def get_mmg_responses(status):
+def get_mmg_responses(status, detailed_status_code=None):
-    return mmg_response_map[status]
+    return (mmg_response_map[status]["status"], mmg_response_map[status]["substatus"].get(detailed_status_code, None))
 
 
 class MMGClientResponseException(SmsClientResponseException):

app/dao/notifications_dao.py

@@ -89,19 +89,19 @@ def dao_create_notification(notification):
     db.session.add(notification)
 
 
-def _decide_permanent_temporary_failure(status, notification, code=None):
+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.
     # 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']:
+    if status == NOTIFICATION_PERMANENT_FAILURE and detailed_status_code not in [None, '000']:
         try:
-            status, reason = get_message_status_and_reason_from_firetext_code(code)
+            status, reason = get_message_status_and_reason_from_firetext_code(detailed_status_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')
+            current_app.logger.warning(f'Failure code {detailed_status_code} from Firetext not recognised')
     # fallback option:
     if notification.status == NOTIFICATION_PENDING and status == NOTIFICATION_PERMANENT_FAILURE:
         status = NOTIFICATION_TEMPORARY_FAILURE
@@ -113,8 +113,10 @@ def country_records_delivery(phone_prefix):
     return dlr and dlr.lower() == 'yes'
 
 
-def _update_notification_status(notification, status, code=None):
+def _update_notification_status(notification, status, detailed_status_code=None):
-    status = _decide_permanent_temporary_failure(status=status, notification=notification, code=code)
+    status = _decide_permanent_temporary_failure(
+        status=status, notification=notification, detailed_status_code=detailed_status_code
+    )
     notification.status = status
     dao_update_notification(notification)
     return notification
@@ -122,7 +124,7 @@ def _update_notification_status(notification, status, code=None):
 
 @statsd(namespace="dao")
 @transactional
-def update_notification_status_by_id(notification_id, status, sent_by=None, code=None):
+def update_notification_status_by_id(notification_id, status, sent_by=None, detailed_status_code=None):
     notification = Notification.query.with_for_update().filter(Notification.id == notification_id).first()
 
     if not notification:
@@ -149,7 +151,7 @@ def update_notification_status_by_id(notification_id, status, sent_by=None, code
     return _update_notification_status(
         notification=notification,
         status=status,
-        code=code
+        detailed_status_code=detailed_status_code
     )
 
 

app/notifications/notifications_sms_callback.py

@@ -22,10 +22,12 @@ def process_mmg_response():
         raise InvalidRequest(errors, status_code=400)
 
     status = str(data.get('status'))
+    detailed_status_code = str(data.get('substatus'))
+
     provider_reference = data.get('CID')
 
     process_sms_client_response.apply_async(
-        [status, provider_reference, client_name],
+        [status, provider_reference, client_name, detailed_status_code],
         queue=QueueNames.SMS_CALLBACKS,
     )
 
@@ -48,7 +50,7 @@ def process_firetext_response():
         raise InvalidRequest(errors, status_code=400)
 
     status = request.form.get('status')
-    code = request.form.get('code')
+    detailed_status_code = request.form.get('code')
     provider_reference = request.form.get('reference')
 
     safe_to_log = dict(request.form).copy()
@@ -58,7 +60,7 @@ def process_firetext_response():
     )
 
     process_sms_client_response.apply_async(
-        [status, provider_reference, client_name, code],
+        [status, provider_reference, client_name, detailed_status_code],
         queue=QueueNames.SMS_CALLBACKS,
     )
 

tests/app/clients/test_firetext.py

@@ -8,19 +8,25 @@ import requests_mock
 from app.clients.sms.firetext import get_firetext_responses, SmsClientResponseException, FiretextClientResponseException
 
 
-def test_should_return_correct_details_for_delivery():
+@pytest.mark.parametrize('detailed_status_code, result', [
-    get_firetext_responses('0') == 'delivered'
+    (None, ('delivered', None)), ('000', ('delivered', None))
+])
+def test_get_firetext_responses_should_return_correct_details_for_delivery(detailed_status_code, result):
+    assert get_firetext_responses('0', detailed_status_code) == result
 
 
-def test_should_return_correct_details_for_bounced():
+@pytest.mark.parametrize('detailed_status_code, result', [
-    get_firetext_responses('1') == 'permanent-failure'
+    (None, ('permanent-failure', None)), ('401', ('permanent-failure', 'Message Rejected'))
+])
+def test_get_firetext_responses_should_return_correct_details_for_bounced(detailed_status_code, result):
+    assert get_firetext_responses('1', detailed_status_code) == result
 
 
-def test_should_return_correct_details_for_complaint():
+def test_get_firetext_responses_should_return_correct_details_for_complaint():
-    get_firetext_responses('2') == 'pending'
+    assert get_firetext_responses('2') == ('pending', None)
 
 
-def test_should_be_none_if_unrecognised_status_code():
+def test_get_firetext_responses_raises_KeyError_if_unrecognised_status_code():
     with pytest.raises(KeyError) as e:
         get_firetext_responses('99')
     assert '99' in str(e.value)

tests/app/clients/test_mmg.py

@@ -9,20 +9,31 @@ from app.clients.sms import SmsClientResponseException
 from app.clients.sms.mmg import get_mmg_responses, MMGClientResponseException
 
 
-def test_should_return_correct_details_for_delivery():
+@pytest.mark.parametrize('detailed_status_code, result', [
-    get_mmg_responses('3') == 'delivered'
+    (None, ('delivered', None)), ('5', ('delivered', 'Delivered to handset'))
+])
+def test_get_mmg_responses_should_return_correct_details_for_delivery(detailed_status_code, result):
+    assert get_mmg_responses('3', detailed_status_code) == result
 
 
-def test_should_return_correct_details_for_temporary_failure():
+@pytest.mark.parametrize('detailed_status_code, result', [
-    get_mmg_responses('4') == 'temporary-failure'
+    (None, ('temporary-failure', None)), ('15', ('temporary-failure', 'Expired'))
+])
+def test_get_mmg_responses_should_return_correct_details_for_temporary_failure(detailed_status_code, result):
+    assert get_mmg_responses('4', detailed_status_code) == result
 
 
-@pytest.mark.parametrize('status', ['5', '2'])
+@pytest.mark.parametrize('status, detailed_status_code, result', [
-def test_should_return_correct_details_for_bounced(status):
+    ('2', None, ('permanent-failure', None)),
-    get_mmg_responses(status) == 'permanent-failure'
+    ('2', '12', ('permanent-failure', "Illegal equipment")),
+    ('5', None, ('permanent-failure', None)),
+    ('5', '20', ('permanent-failure', 'Rejected by anti-flooding mechanism'))
+])
+def test_get_mmg_responses_should_return_correct_details_for_bounced(status, detailed_status_code, result):
+    assert get_mmg_responses(status, detailed_status_code) == result
 
 
-def test_should_be_raise_if_unrecognised_status_code():
+def test_get_mmg_responses_raises_KeyError_if_unrecognised_status_code():
     with pytest.raises(KeyError) as e:
         get_mmg_responses('99')
     assert '99' in str(e.value)

tests/app/notifications/rest/test_callbacks.py

@@ -203,6 +203,7 @@ def test_mmg_callback_should_return_200_and_call_task_with_valid_data(client, mo
                        "CID": "notification_id",
                        "MSISDN": "447777349060",
                        "status": "3",
+                       "substatus": "5",
                        "deliverytime": "2016-04-05 16:01:07"})
 
     response = mmg_post(client, data)
@@ -212,7 +213,7 @@ def test_mmg_callback_should_return_200_and_call_task_with_valid_data(client, mo
     assert json_data['result'] == 'success'
 
     mock_celery.assert_called_once_with(
-        ['3', 'notification_id', 'MMG'],
+        ['3', 'notification_id', 'MMG', '5'],
         queue='sms-callbacks',
     )
 

tests/app/notifications/test_process_client_response.py

@@ -35,29 +35,35 @@ def test_process_sms_response_raises_client_exception_for_unknown_status(
     assert sample_notification.status == NOTIFICATION_TECHNICAL_FAILURE
 
 
-@pytest.mark.parametrize('status, sms_provider, expected_notification_status', [
+@pytest.mark.parametrize('status, detailed_status_code, sms_provider, expected_notification_status, reason', [
-    ('0', 'Firetext', 'delivered'),
+    ('0', None, 'Firetext', 'delivered', None),
-    ('1', 'Firetext', 'permanent-failure'),
+    ('1', '101', 'Firetext', 'permanent-failure', 'Unknown Subscriber'),
-    ('2', 'Firetext', 'pending'),
+    ('2', '102', 'Firetext', 'pending', 'Absent Subscriber'),
-    ('2', 'MMG', 'permanent-failure'),
+    ('2', '1', 'MMG', 'permanent-failure', "Number does not exist"),
-    ('3', 'MMG', 'delivered'),
+    ('3', '2', 'MMG', 'delivered', "Delivered to operator"),
-    ('4', 'MMG', 'temporary-failure'),
+    ('4', '27', 'MMG', 'temporary-failure', "Absent Subscriber"),
-    ('5', 'MMG', 'permanent-failure'),
+    ('5', '13', 'MMG', 'permanent-failure', "Sender id blacklisted"),
 ])
 def test_process_sms_client_response_updates_notification_status(
     sample_notification,
     mocker,
     status,
+    detailed_status_code,
     sms_provider,
     expected_notification_status,
+    reason
 ):
+    mock_logger = mocker.patch('app.celery.tasks.current_app.logger.info')
     sample_notification.status = 'sending'
-    process_sms_client_response(status, str(sample_notification.id), sms_provider)
 
+    process_sms_client_response(status, str(sample_notification.id), sms_provider, detailed_status_code)
+
+    message = f"{sms_provider} callback returned status of {expected_notification_status}: {reason} for reference: {sample_notification.id}"  # noqa
+    mock_logger.assert_any_call(message)
     assert sample_notification.status == expected_notification_status
 
 
-@pytest.mark.parametrize('code, expected_notification_status, reason', [
+@pytest.mark.parametrize('detailed_status_code, expected_notification_status, reason', [
     ('101', 'permanent-failure', 'Unknown Subscriber'),
     ('102', 'temporary-failure', 'Absent Subscriber'),
     (None, 'temporary-failure', None),
@@ -66,7 +72,7 @@ def test_process_sms_client_response_updates_notification_status(
 def test_process_sms_client_response_updates_notification_status_when_called_second_time(
     sample_notification,
     mocker,
-    code,
+    detailed_status_code,
     expected_notification_status,
     reason
 ):
@@ -74,29 +80,29 @@ def test_process_sms_client_response_updates_notification_status_when_called_sec
     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)
+    process_sms_client_response('1', str(sample_notification.id), 'Firetext', detailed_status_code)
 
-    if code and code != '000':
+    if detailed_status_code and detailed_status_code != '000':
         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
 
 
-@pytest.mark.parametrize('code', ['102', None, '000'])
+@pytest.mark.parametrize('detailed_status_code', ['102', None, '000'])
-def test_process_sms_client_response_updates_notification_status_to_pending_with_and_without_failire_code_present(
+def test_process_sms_client_response_updates_notification_status_to_pending_with_and_without_failure_code_present(
     sample_notification,
     mocker,
-    code
+    detailed_status_code
 ):
     sample_notification.status = 'sending'
 
-    process_sms_client_response('2', str(sample_notification.id), 'Firetext', code)
+    process_sms_client_response('2', str(sample_notification.id), 'Firetext', detailed_status_code)
 
     assert sample_notification.status == 'pending'
 
 
-def test_process_sms_client_response_updates_notification_status_when_code_unknown(
+def test_process_sms_client_response_updates_notification_status_when_detailed_status_code_not_recognised(
     sample_notification,
     mocker,
 ):