diff --git a/app/clients/sms/firetext.py b/app/clients/sms/firetext.py index 2cc24a67c..2e8086d50 100644 --- a/app/clients/sms/firetext.py +++ b/app/clients/sms/firetext.py @@ -20,15 +20,15 @@ firetext_responses = { } firetext_codes = { - '101': 'permanent-failure', # Unknown Subscriber - '102': 'temporary-failure', # Absent Subscriber - '103': 'temporary-failure', # Subscriber Busy - '104': 'temporary-failure', # No Subscriber Memory - '201': 'permanent-failure', # Invalid Number - '301': 'permanent-failure', # SMS Not Supported - '302': 'temporary-failure', # SMS Not Supported - '401': 'permanent-failure', # Message Rejected - '900': 'temporary-failure', # Routing Error + '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'}, } @@ -36,8 +36,8 @@ def get_firetext_responses(status): return firetext_responses[status] -def get_message_status_from_firetext_code(code): - return firetext_codes[code] +def get_message_status_and_reason_from_firetext_code(code): + return firetext_codes[code]['status'], firetext_codes[code]['reason'] class FiretextClientResponseException(SmsClientResponseException): diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 30591bb57..01a58f28c 100644 --- a/app/dao/notifications_dao.py +++ b/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: - status = get_message_status_from_firetext_code(code) - else: - status = NOTIFICATION_TEMPORARY_FAILURE + 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 @@ -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 diff --git a/tests/app/notifications/test_process_client_response.py b/tests/app/notifications/test_process_client_response.py index f62125fa1..11ff3abe4 100644 --- a/tests/app/notifications/test_process_client_response.py +++ b/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', [ - ('101', 'permanent-failure'), - ('102', 'temporary-failure'), +@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',