diff --git a/app/celery/process_sms_client_response_tasks.py b/app/celery/process_sms_client_response_tasks.py index 1ee86634e..30f5c9329 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): +def process_sms_client_response(self, status, provider_reference, client_name, code=None): # validate reference try: uuid.UUID(provider_reference, version=4) @@ -51,16 +51,18 @@ def process_sms_client_response(self, status, provider_reference, client_name): _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/clients/sms/firetext.py b/app/clients/sms/firetext.py index feb81ce3e..2e8086d50 100644 --- a/app/clients/sms/firetext.py +++ b/app/clients/sms/firetext.py @@ -19,11 +19,27 @@ firetext_responses = { '2': 'pending' } +firetext_codes = { + '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'}, +} + def get_firetext_responses(status): return firetext_responses[status] +def get_message_status_and_reason_from_firetext_code(code): + return firetext_codes[code]['status'], firetext_codes[code]['reason'] + + class FiretextClientResponseException(SmsClientResponseException): def __init__(self, response, exception): status_code = response.status_code if response is not None else 504 diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 333b1308d..01a58f28c 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_and_reason_from_firetext_code @statsd(namespace="dao") @@ -89,10 +90,18 @@ def dao_create_notification(notification): db.session.add(notification) -def _decide_permanent_temporary_failure(current_status, status): +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 delivered we need to set failure type as temporary-failure - if current_status == NOTIFICATION_PENDING and status == NOTIFICATION_PERMANENT_FAILURE: + # 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 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 @@ -102,8 +111,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(status=status, notification=notification, code=code) notification.status = status dao_update_notification(notification) return notification @@ -111,7 +120,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 +146,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/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, ) diff --git a/tests/app/notifications/rest/test_callbacks.py b/tests/app/notifications/rest/test_callbacks.py index bbbc44e2c..3b0810048 100644 --- a/tests/app/notifications/rest/test_callbacks.py +++ b/tests/app/notifications/rest/test_callbacks.py @@ -147,7 +147,23 @@ 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', + ) + + +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', ) diff --git a/tests/app/notifications/test_process_client_response.py b/tests/app/notifications/test_process_client_response.py index 9ccd05546..11ff3abe4 100644 --- a/tests/app/notifications/test_process_client_response.py +++ b/tests/app/notifications/test_process_client_response.py @@ -57,6 +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, 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',