From 91fe68eed492e82530cf681d40326513a9a3c533 Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Thu, 9 Apr 2020 17:50:05 +0100 Subject: [PATCH 1/5] WIP read firetext response codes --- app/celery/process_sms_client_response_tasks.py | 4 ++-- app/clients/sms/firetext.py | 16 ++++++++++++++++ app/notifications/notifications_sms_callback.py | 3 ++- 3 files changed, 20 insertions(+), 3 deletions(-) diff --git a/app/celery/process_sms_client_response_tasks.py b/app/celery/process_sms_client_response_tasks.py index 1ee86634e..c9950609e 100644 --- a/app/celery/process_sms_client_response_tasks.py +++ b/app/celery/process_sms_client_response_tasks.py @@ -7,7 +7,7 @@ from notifications_utils.template import SMSMessageTemplate from app import notify_celery, statsd_client from app.clients import ClientException -from app.clients.sms.firetext import get_firetext_responses +from app.clients.sms.firetext import get_firetext_responses, get_message_status_from_firetext_code from app.clients.sms.mmg import get_mmg_responses from app.celery.service_callback_tasks import send_delivery_status_to_service, create_delivery_status_callback_data from app.config import QueueNames @@ -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): +def process_sms_client_response(self, status, provider_reference, client_name, code=None): # validate reference try: uuid.UUID(provider_reference, version=4) diff --git a/app/clients/sms/firetext.py b/app/clients/sms/firetext.py index feb81ce3e..2cc24a67c 100644 --- a/app/clients/sms/firetext.py +++ b/app/clients/sms/firetext.py @@ -19,11 +19,27 @@ firetext_responses = { '2': 'pending' } +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 +} + def get_firetext_responses(status): return firetext_responses[status] +def get_message_status_from_firetext_code(code): + return firetext_codes[code] + + class FiretextClientResponseException(SmsClientResponseException): def __init__(self, response, exception): status_code = response.status_code if response is not None else 504 diff --git a/app/notifications/notifications_sms_callback.py b/app/notifications/notifications_sms_callback.py index afc705846..2c623a846 100644 --- a/app/notifications/notifications_sms_callback.py +++ b/app/notifications/notifications_sms_callback.py @@ -48,6 +48,7 @@ def process_firetext_response(): raise InvalidRequest(errors, status_code=400) status = request.form.get('status') + code = request.form.get('code') provider_reference = request.form.get('reference') safe_to_log = dict(request.form).copy() @@ -57,7 +58,7 @@ def process_firetext_response(): ) process_sms_client_response.apply_async( - [status, provider_reference, client_name], + [status, provider_reference, client_name, code], queue=QueueNames.SMS_CALLBACKS, ) From a4507dbb3f54b6cd4c9f1ca86333c225a5d58e21 Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Tue, 14 Apr 2020 15:55:17 +0100 Subject: [PATCH 2/5] Use firetext response code to see if temporary or permanent failure if available --- .../process_sms_client_response_tasks.py | 10 ++++++---- app/dao/notifications_dao.py | 20 ++++++++++++------- .../app/notifications/rest/test_callbacks.py | 2 +- .../test_process_client_response.py | 17 ++++++++++++++++ 4 files changed, 37 insertions(+), 12 deletions(-) diff --git a/app/celery/process_sms_client_response_tasks.py b/app/celery/process_sms_client_response_tasks.py index c9950609e..30f5c9329 100644 --- a/app/celery/process_sms_client_response_tasks.py +++ b/app/celery/process_sms_client_response_tasks.py @@ -7,7 +7,7 @@ from notifications_utils.template import SMSMessageTemplate from app import notify_celery, statsd_client from app.clients import ClientException -from app.clients.sms.firetext import get_firetext_responses, get_message_status_from_firetext_code +from app.clients.sms.firetext import get_firetext_responses from app.clients.sms.mmg import get_mmg_responses from app.celery.service_callback_tasks import send_delivery_status_to_service, create_delivery_status_callback_data from app.config import QueueNames @@ -51,16 +51,18 @@ def process_sms_client_response(self, status, provider_reference, client_name, c _process_for_status( notification_status=notification_status, client_name=client_name, - provider_reference=provider_reference + provider_reference=provider_reference, + code=code ) -def _process_for_status(notification_status, client_name, provider_reference): +def _process_for_status(notification_status, client_name, provider_reference, code=None): # record stats notification = notifications_dao.update_notification_status_by_id( notification_id=provider_reference, status=notification_status, - sent_by=client_name.lower() + sent_by=client_name.lower(), + code=code ) if not notification: return diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 333b1308d..d141e7422 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -52,6 +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 @statsd(namespace="dao") @@ -89,11 +90,15 @@ def dao_create_notification(notification): db.session.add(notification) -def _decide_permanent_temporary_failure(current_status, status): +def _decide_permanent_temporary_failure(current_status, status, code=None): # Firetext will send pending, then send either succes or fail. - # If we go from pending to delivered we need to set failure type as temporary-failure + # If we go from pending to failure we need to set failure type as temporary-failure + # if we get a detailed code from firetext, we should use that code to set status instead if current_status == NOTIFICATION_PENDING and status == NOTIFICATION_PERMANENT_FAILURE: - status = NOTIFICATION_TEMPORARY_FAILURE + if code: + status = get_message_status_from_firetext_code(code) + else: + status = NOTIFICATION_TEMPORARY_FAILURE return status @@ -102,8 +107,8 @@ def country_records_delivery(phone_prefix): return dlr and dlr.lower() == 'yes' -def _update_notification_status(notification, status): - status = _decide_permanent_temporary_failure(current_status=notification.status, status=status) +def _update_notification_status(notification, status, code=None): + status = _decide_permanent_temporary_failure(current_status=notification.status, status=status, code=code) notification.status = status dao_update_notification(notification) return notification @@ -111,7 +116,7 @@ def _update_notification_status(notification, status): @statsd(namespace="dao") @transactional -def update_notification_status_by_id(notification_id, status, sent_by=None): +def update_notification_status_by_id(notification_id, status, sent_by=None, code=None): notification = Notification.query.with_for_update().filter(Notification.id == notification_id).first() if not notification: @@ -137,7 +142,8 @@ def update_notification_status_by_id(notification_id, status, sent_by=None): notification.sent_by = sent_by return _update_notification_status( notification=notification, - status=status + status=status, + code=code ) diff --git a/tests/app/notifications/rest/test_callbacks.py b/tests/app/notifications/rest/test_callbacks.py index bbbc44e2c..5cb0af990 100644 --- a/tests/app/notifications/rest/test_callbacks.py +++ b/tests/app/notifications/rest/test_callbacks.py @@ -147,7 +147,7 @@ def test_firetext_callback_should_return_200_and_call_task_with_valid_data(clien assert json_resp['result'] == 'success' mock_celery.assert_called_once_with( - ['0', 'notification_id', 'Firetext'], + ['0', 'notification_id', 'Firetext', None], queue='sms-callbacks', ) diff --git a/tests/app/notifications/test_process_client_response.py b/tests/app/notifications/test_process_client_response.py index 9ccd05546..f62125fa1 100644 --- a/tests/app/notifications/test_process_client_response.py +++ b/tests/app/notifications/test_process_client_response.py @@ -57,6 +57,23 @@ 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'), +]) +def test_process_sms_client_response_updates_notification_status_when_called_second_time( + sample_notification, + mocker, + code, + expected_notification_status, +): + 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) + + assert sample_notification.status == expected_notification_status + + 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', From 470d00c26ebada5b02d7a2a99efffb05bf173098 Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Thu, 16 Apr 2020 13:10:47 +0100 Subject: [PATCH 3/5] Make comment clearer following review --- app/dao/notifications_dao.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index d141e7422..30591bb57 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -92,8 +92,8 @@ def dao_create_notification(notification): def _decide_permanent_temporary_failure(current_status, status, 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 - # if we get a detailed code from firetext, we should use that code to set status instead + # 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) From fef03920f08f86a9b2b3f63cade408ab370344ca Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Thu, 16 Apr 2020 13:14:29 +0100 Subject: [PATCH 4/5] Add test for a callback with a code --- tests/app/notifications/rest/test_callbacks.py | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/tests/app/notifications/rest/test_callbacks.py b/tests/app/notifications/rest/test_callbacks.py index 5cb0af990..3b0810048 100644 --- a/tests/app/notifications/rest/test_callbacks.py +++ b/tests/app/notifications/rest/test_callbacks.py @@ -152,6 +152,22 @@ def test_firetext_callback_should_return_200_and_call_task_with_valid_data(clien ) +def test_firetext_callback_including_a_code_should_return_200_and_call_task_with_valid_data(client, mocker): + mock_celery = mocker.patch( + 'app.notifications.notifications_sms_callback.process_sms_client_response.apply_async') + + data = 'mobile=441234123123&status=1&code=101&time=2016-03-10 14:17:00&reference=notification_id' + response = firetext_post(client, data) + json_resp = json.loads(response.get_data(as_text=True)) + assert response.status_code == 200 + assert json_resp['result'] == 'success' + + mock_celery.assert_called_once_with( + ['1', 'notification_id', 'Firetext', '101'], + queue='sms-callbacks', + ) + + def test_mmg_callback_should_not_need_auth(client, mocker, sample_notification): mocker.patch('app.notifications.notifications_sms_callback.process_sms_client_response') data = json.dumps({"reference": "mmg_reference", From b962dcac5d327c840e6515be2957cea83ee3c593 Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Mon, 20 Apr 2020 18:20:01 +0100 Subject: [PATCH 5/5] 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',