diff --git a/app/celery/process_sms_client_response_tasks.py b/app/celery/process_sms_client_response_tasks.py index 30f5c9329..947c4e782 100644 --- a/app/celery/process_sms_client_response_tasks.py +++ b/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 diff --git a/app/clients/sms/firetext.py b/app/clients/sms/firetext.py index 25848691b..a8b803613 100644 --- a/app/clients/sms/firetext.py +++ b/app/clients/sms/firetext.py @@ -33,12 +33,15 @@ firetext_codes = { } -def get_firetext_responses(status): - return firetext_responses[status] +def get_firetext_responses(status, detailed_status_code=None): + 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): - return firetext_codes[code]['status'], firetext_codes[code]['reason'] +def get_message_status_and_reason_from_firetext_code(detailed_status_code): + return firetext_codes[detailed_status_code]['status'], firetext_codes[detailed_status_code]['reason'] class FiretextClientResponseException(SmsClientResponseException): diff --git a/app/clients/sms/mmg.py b/app/clients/sms/mmg.py index e1c45a45f..752ff7792 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, detailed_status_code=None): + return (mmg_response_map[status]["status"], mmg_response_map[status]["substatus"].get(detailed_status_code, None)) class MMGClientResponseException(SmsClientResponseException): diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 17318e1ab..6629a118c 100644 --- a/app/dao/notifications_dao.py +++ b/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): - status = _decide_permanent_temporary_failure(status=status, notification=notification, code=code) +def _update_notification_status(notification, status, detailed_status_code=None): + 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 ) diff --git a/app/notifications/notifications_sms_callback.py b/app/notifications/notifications_sms_callback.py index 2c623a846..7789f592a 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')) + 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, ) diff --git a/tests/app/clients/test_firetext.py b/tests/app/clients/test_firetext.py index db7b405dd..3494ef83b 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('detailed_status_code, result', [ + (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(): - get_firetext_responses('1') == 'permanent-failure' +@pytest.mark.parametrize('detailed_status_code, result', [ + (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(): - 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_raises_KeyError_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..a547e2df5 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('detailed_status_code, result', [ + (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(): - get_mmg_responses('4') == 'temporary-failure' +@pytest.mark.parametrize('detailed_status_code, result', [ + (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']) -def test_should_return_correct_details_for_bounced(status): - get_mmg_responses(status) == 'permanent-failure' +@pytest.mark.parametrize('status, detailed_status_code, 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, 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) 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..c60da0003 100644 --- a/tests/app/notifications/test_process_client_response.py +++ b/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', [ - ('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, detailed_status_code, 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, + 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']) -def test_process_sms_client_response_updates_notification_status_to_pending_with_and_without_failire_code_present( +@pytest.mark.parametrize('detailed_status_code', ['102', None, '000']) +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, ):