From b962dcac5d327c840e6515be2957cea83ee3c593 Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Mon, 20 Apr 2020 18:20:01 +0100 Subject: [PATCH] Check Firetext code first to determine status and log the reason If there is no code, fall back to old way of determining status. If the code is not recognised, log an Error so we are notified and can look into it. --- app/clients/sms/firetext.py | 22 +++++++-------- app/dao/notifications_dao.py | 20 +++++++------ .../test_process_client_response.py | 28 +++++++++++++++++-- 3 files changed, 48 insertions(+), 22 deletions(-) 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',