From a4b942cf6c8304bae6dfffaeabf03d9254fb8e57 Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Wed, 27 May 2020 18:03:55 +0100 Subject: [PATCH] Log detailed sms delivery status for mmg from process_sms_client_response task. Also log detailed delivery status for firetext in the same place in addition to it being logged from notifications_dao. Logging detailed delivery statuses will help us see why messages fail to deliver. In the future we could persist detailed delivery status in the database. --- .../process_sms_client_response_tasks.py | 5 +- app/clients/sms/firetext.py | 5 +- app/clients/sms/mmg.py | 46 ++++++++++++++++--- .../notifications_sms_callback.py | 4 +- tests/app/clients/test_firetext.py | 20 +++++--- tests/app/clients/test_mmg.py | 27 +++++++---- .../app/notifications/rest/test_callbacks.py | 3 +- .../test_process_client_response.py | 24 ++++++---- 8 files changed, 98 insertions(+), 36 deletions(-) diff --git a/app/celery/process_sms_client_response_tasks.py b/app/celery/process_sms_client_response_tasks.py index 30f5c9329..f9dc99f11 100644 --- a/app/celery/process_sms_client_response_tasks.py +++ b/app/celery/process_sms_client_response_tasks.py @@ -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, 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( diff --git a/app/clients/sms/firetext.py b/app/clients/sms/firetext.py index 36692d29c..f647a66d2 100644 --- a/app/clients/sms/firetext.py +++ b/app/clients/sms/firetext.py @@ -33,8 +33,9 @@ firetext_codes = { } -def get_firetext_responses(status): - return firetext_responses[status] +def get_firetext_responses(status, code=None): + substatus = firetext_codes[code]['reason'] if firetext_codes.get(code, None) else None + return firetext_responses[status], substatus def get_message_status_and_reason_from_firetext_code(code): diff --git a/app/clients/sms/mmg.py b/app/clients/sms/mmg.py index b122d45dc..2c3db3d40 100644 --- a/app/clients/sms/mmg.py +++ b/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', - '3': 'delivered', - '4': 'temporary-failure', - '5': 'permanent-failure' + '2': {'status': 'permanent-failure', 'substatus': { + "1": "Number does not exist", + "4": "Rejected by operator", + "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): - return mmg_response_map[status] +def get_mmg_responses(status, substatus=None): + return (mmg_response_map[status]["status"], mmg_response_map[status]["substatus"].get(substatus, None)) class MMGClientResponseException(SmsClientResponseException): diff --git a/app/notifications/notifications_sms_callback.py b/app/notifications/notifications_sms_callback.py index 2c623a846..447acab50 100644 --- a/app/notifications/notifications_sms_callback.py +++ b/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')) + substatus = 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, substatus], queue=QueueNames.SMS_CALLBACKS, ) diff --git a/tests/app/clients/test_firetext.py b/tests/app/clients/test_firetext.py index db7b405dd..773549f2b 100644 --- a/tests/app/clients/test_firetext.py +++ b/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(): - get_firetext_responses('0') == 'delivered' +@pytest.mark.parametrize('code, result', [ + (None, ('delivered', None)), ('000', ('delivered', None)) +]) +def test_get_firetext_responses_should_return_correct_details_for_delivery(code, result): + assert get_firetext_responses('0', code) == result -def test_should_return_correct_details_for_bounced(): - get_firetext_responses('1') == 'permanent-failure' +@pytest.mark.parametrize('code, result', [ + (None, ('permanent-failure', None)), ('401', ('permanent-failure', 'Message Rejected')) +]) +def test_get_firetext_responses_should_return_correct_details_for_bounced(code, result): + assert get_firetext_responses('1', code) == result -def test_should_return_correct_details_for_complaint(): - get_firetext_responses('2') == 'pending' +def test_get_firetext_responses_should_return_correct_details_for_complaint(): + assert get_firetext_responses('2') == ('pending', None) -def test_should_be_none_if_unrecognised_status_code(): +def test_get_firetext_responses_should_be_none_if_unrecognised_status_code(): with pytest.raises(KeyError) as e: get_firetext_responses('99') assert '99' in str(e.value) diff --git a/tests/app/clients/test_mmg.py b/tests/app/clients/test_mmg.py index 8752d8cbe..026325c0e 100644 --- a/tests/app/clients/test_mmg.py +++ b/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(): - get_mmg_responses('3') == 'delivered' +@pytest.mark.parametrize('substatus, result', [ + (None, ('delivered', None)), ('5', ('delivered', 'Delivered to handset')) +]) +def test_get_mmg_responses_should_return_correct_details_for_delivery(substatus, result): + assert get_mmg_responses('3', substatus) == result -def test_should_return_correct_details_for_temporary_failure(): - get_mmg_responses('4') == 'temporary-failure' +@pytest.mark.parametrize('substatus, result', [ + (None, ('temporary-failure', None)), ('15', ('temporary-failure', 'Expired')) +]) +def test_get_mmg_responses_should_return_correct_details_for_temporary_failure(substatus, result): + assert get_mmg_responses('4', substatus) == result -@pytest.mark.parametrize('status', ['5', '2']) -def test_should_return_correct_details_for_bounced(status): - get_mmg_responses(status) == 'permanent-failure' +@pytest.mark.parametrize('status, substatus, result', [ + ('2', None, ('permanent-failure', None)), + ('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, substatus, result): + assert get_mmg_responses(status, substatus) == result -def test_should_be_raise_if_unrecognised_status_code(): +def test_get_mmg_responses_should_be_raise_if_unrecognised_status_code(): with pytest.raises(KeyError) as e: get_mmg_responses('99') assert '99' in str(e.value) diff --git a/tests/app/notifications/rest/test_callbacks.py b/tests/app/notifications/rest/test_callbacks.py index 3b0810048..2b4dc2a60 100644 --- a/tests/app/notifications/rest/test_callbacks.py +++ b/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', ) diff --git a/tests/app/notifications/test_process_client_response.py b/tests/app/notifications/test_process_client_response.py index 859e79eac..b849d96c4 100644 --- a/tests/app/notifications/test_process_client_response.py +++ b/tests/app/notifications/test_process_client_response.py @@ -35,25 +35,31 @@ 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', [ - ('0', 'Firetext', 'delivered'), - ('1', 'Firetext', 'permanent-failure'), - ('2', 'Firetext', 'pending'), - ('2', 'MMG', 'permanent-failure'), - ('3', 'MMG', 'delivered'), - ('4', 'MMG', 'temporary-failure'), - ('5', 'MMG', 'permanent-failure'), +@pytest.mark.parametrize('status, substatus, sms_provider, expected_notification_status, reason', [ + ('0', None, 'Firetext', 'delivered', None), + ('1', '101', 'Firetext', 'permanent-failure', 'Unknown Subscriber'), + ('2', '102', 'Firetext', 'pending', 'Absent Subscriber'), + ('2', '1', 'MMG', 'permanent-failure', "Number does not exist"), + ('3', '2', 'MMG', 'delivered', "Delivered to operator"), + ('4', '27', 'MMG', 'temporary-failure', "Absent Subscriber"), + ('5', '13', 'MMG', 'permanent-failure', "Sender id blacklisted"), ]) def test_process_sms_client_response_updates_notification_status( sample_notification, mocker, status, + substatus, 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, substatus) + + 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