diff --git a/app/clients/sms/firetext.py b/app/clients/sms/firetext.py index e97091c0b..8f650ff79 100644 --- a/app/clients/sms/firetext.py +++ b/app/clients/sms/firetext.py @@ -5,7 +5,6 @@ from monotonic import monotonic from requests import request, RequestException from app.clients.sms import (SmsClient, SmsClientResponseException) -from app.clients import STATISTICS_DELIVERED, STATISTICS_FAILURE logger = logging.getLogger(__name__) @@ -15,24 +14,9 @@ logger = logging.getLogger(__name__) # the notification status to temporary-failure rather than permanent failure. # See the code in the notification_dao.update_notifications_status_by_id firetext_responses = { - '0': { - "message": 'Delivered', - "notification_statistics_status": STATISTICS_DELIVERED, - "success": True, - "notification_status": 'delivered' - }, - '1': { - "message": 'Declined', - "success": False, - "notification_statistics_status": STATISTICS_FAILURE, - "notification_status": 'permanent-failure' - }, - '2': { - "message": 'Undelivered (Pending with Network)', - "success": True, - "notification_statistics_status": None, - "notification_status": 'pending' - } + '0': 'delivered', + '1': 'permanent-failure', + '2': 'pending' } diff --git a/app/clients/sms/mmg.py b/app/clients/sms/mmg.py index 3c8cf6338..55388119a 100644 --- a/app/clients/sms/mmg.py +++ b/app/clients/sms/mmg.py @@ -1,45 +1,18 @@ import json from monotonic import monotonic from requests import (request, RequestException) -from app.clients import (STATISTICS_DELIVERED, STATISTICS_FAILURE) from app.clients.sms import (SmsClient, SmsClientResponseException) mmg_response_map = { - '2': { - "message": ' Permanent failure', - "notification_statistics_status": STATISTICS_FAILURE, - "success": False, - "notification_status": 'permanent-failure' - }, - '3': { - "message": 'Delivered', - "notification_statistics_status": STATISTICS_DELIVERED, - "success": True, - "notification_status": 'delivered' - }, - '4': { - "message": ' Temporary failure', - "notification_statistics_status": STATISTICS_FAILURE, - "success": False, - "notification_status": 'temporary-failure' - }, - '5': { - "message": 'Permanent failure', - "notification_statistics_status": STATISTICS_FAILURE, - "success": False, - "notification_status": 'permanent-failure' - }, - 'default': { - "message": 'Declined', - "success": False, - "notification_statistics_status": STATISTICS_FAILURE, - "notification_status": 'failed' - } + '2': 'permanent-failure', + '3': 'delivered', + '4': 'temporary-failure', + '5': 'permanent-failure' } def get_mmg_responses(status): - return mmg_response_map.get(status, mmg_response_map.get('default')) + return mmg_response_map[status] class MMGClientResponseException(SmsClientResponseException): diff --git a/app/notifications/process_client_response.py b/app/notifications/process_client_response.py index ad78c744d..8996f8a2b 100644 --- a/app/notifications/process_client_response.py +++ b/app/notifications/process_client_response.py @@ -4,6 +4,7 @@ from datetime import datetime from flask import current_app from app import statsd_client +from app.clients import ClientException from app.dao import notifications_dao from app.clients.sms.firetext import get_firetext_responses from app.clients.sms.mmg import get_mmg_responses @@ -49,17 +50,19 @@ def process_sms_client_response(status, reference, client_name): # validate status try: - response_dict = response_parser(status) + notification_status = response_parser(status) current_app.logger.info('{} callback return status of {} for reference: {}'.format( client_name, status, reference) ) except KeyError: - msg = "{} callback failed: status {} not found.".format(client_name, status) - return success, msg + process_for_status(notification_status='technical-failure', client_name=client_name, reference=reference) + raise ClientException("{} callback failed: status {} not found.".format(client_name, status)) - notification_status = response_dict['notification_status'] - notification_status_message = response_dict['message'] - notification_success = response_dict['success'] + success = process_for_status(notification_status=notification_status, client_name=client_name, reference=reference) + return success, errors + + +def process_for_status(notification_status, client_name, reference): # record stats notification = notifications_dao.update_notification_status_by_id(reference, notification_status) @@ -67,14 +70,8 @@ def process_sms_client_response(status, reference, client_name): current_app.logger.warning("{} callback failed: notification {} either not found or already updated " "from sending. Status {}".format(client_name, reference, - notification_status_message)) - return success, errors - - if not notification_success: - current_app.logger.info( - "{} delivery failed: notification {} has error found. Status {}".format(client_name, - reference, - notification_status_message)) + notification_status)) + return statsd_client.incr('callback.{}.{}'.format(client_name.lower(), notification_status)) if notification.sent_at: @@ -92,4 +89,4 @@ def process_sms_client_response(status, reference, client_name): send_delivery_status_to_service.apply_async([str(notification.id)], queue=QueueNames.CALLBACKS) success = "{} callback succeeded. reference {} updated".format(client_name, reference) - return success, errors + return success diff --git a/tests/app/clients/test_firetext.py b/tests/app/clients/test_firetext.py index 1edcab564..94f7fa11d 100644 --- a/tests/app/clients/test_firetext.py +++ b/tests/app/clients/test_firetext.py @@ -9,27 +9,15 @@ from app.clients.sms.firetext import get_firetext_responses, SmsClientResponseEx def test_should_return_correct_details_for_delivery(): - response_dict = get_firetext_responses('0') - assert response_dict['message'] == 'Delivered' - assert response_dict['notification_status'] == 'delivered' - assert response_dict['notification_statistics_status'] == 'delivered' - assert response_dict['success'] + get_firetext_responses('0') == 'delivered' def test_should_return_correct_details_for_bounced(): - response_dict = get_firetext_responses('1') - assert response_dict['message'] == 'Declined' - assert response_dict['notification_status'] == 'permanent-failure' - assert response_dict['notification_statistics_status'] == 'failure' - assert not response_dict['success'] + get_firetext_responses('1') == 'permanent-failure' def test_should_return_correct_details_for_complaint(): - response_dict = get_firetext_responses('2') - assert response_dict['message'] == 'Undelivered (Pending with Network)' - assert response_dict['notification_status'] == 'pending' - assert response_dict['notification_statistics_status'] is None - assert response_dict['success'] + get_firetext_responses('2') == 'pending' def test_should_be_none_if_unrecognised_status_code(): diff --git a/tests/app/clients/test_mmg.py b/tests/app/clients/test_mmg.py index c2947ca75..f5c875681 100644 --- a/tests/app/clients/test_mmg.py +++ b/tests/app/clients/test_mmg.py @@ -10,27 +10,22 @@ from app.clients.sms.mmg import get_mmg_responses, MMGClientResponseException def test_should_return_correct_details_for_delivery(): - response_dict = get_mmg_responses('3') - assert response_dict['message'] == 'Delivered' - assert response_dict['notification_status'] == 'delivered' - assert response_dict['notification_statistics_status'] == 'delivered' - assert response_dict['success'] + get_mmg_responses('3') == 'delivered' -def test_should_return_correct_details_for_bounced(): - response_dict = get_mmg_responses('50') - assert response_dict['message'] == 'Declined' - assert response_dict['notification_status'] == 'failed' - assert response_dict['notification_statistics_status'] == 'failure' - assert not response_dict['success'] +def test_should_return_correct_details_for_temporary_failure(): + get_mmg_responses('4') == 'temporary-failure' -def test_should_be_none_if_unrecognised_status_code(): - response_dict = get_mmg_responses('blah') - assert response_dict['message'] == 'Declined' - assert response_dict['notification_status'] == 'failed' - assert response_dict['notification_statistics_status'] == 'failure' - assert not response_dict['success'] +@pytest.mark.parametrize('status', ['5', '2']) +def test_should_return_correct_details_for_bounced(status): + get_mmg_responses(status) == 'permanent-failure' + + +def test_should_be_raise_if_unrecognised_status_code(): + with pytest.raises(KeyError) as e: + get_mmg_responses('99') + assert '99' in str(e.value) def test_send_sms_successful_returns_mmg_response(notify_api, mocker): diff --git a/tests/app/notifications/rest/test_callbacks.py b/tests/app/notifications/rest/test_callbacks.py index 6db37085c..e3c3e7844 100644 --- a/tests/app/notifications/rest/test_callbacks.py +++ b/tests/app/notifications/rest/test_callbacks.py @@ -2,10 +2,12 @@ import uuid from datetime import datetime +import pytest from flask import json from freezegun import freeze_time import app.celery.tasks +from app.clients import ClientException from app.dao.notifications_dao import ( get_notification_by_id ) @@ -161,15 +163,17 @@ def test_firetext_callback_should_return_400_if_no_status(client, mocker): assert json_resp['message'] == ['Firetext callback failed: status missing'] -def test_firetext_callback_should_return_400_if_unknown_status(client, mocker): +def test_firetext_callback_should_set_status_technical_failure_if_status_unknown( + client, notify_db, notify_db_session, mocker): + notification = create_sample_notification( + notify_db, notify_db_session, status='sending', sent_at=datetime.utcnow() + ) mocker.patch('app.statsd_client.incr') - data = 'mobile=441234123123&status=99&time=2016-03-10 14:17:00&reference={}'.format(uuid.uuid4()) - response = firetext_post(client, data) - - json_resp = json.loads(response.get_data(as_text=True)) - assert response.status_code == 400 - assert json_resp['result'] == 'error' - assert json_resp['message'] == 'Firetext callback failed: status 99 not found.' + data = 'mobile=441234123123&status=99&time=2016-03-10 14:17:00&reference={}'.format(notification.id) + with pytest.raises(ClientException) as e: + firetext_post(client, data) + assert get_notification_by_id(notification.id).status == 'technical-failure' + assert 'Firetext callback failed: status 99 not found.' in str(e.value) def test_firetext_callback_returns_200_when_notification_id_not_found_or_already_updated(client, mocker): @@ -389,7 +393,7 @@ def test_process_mmg_response_status_4_updates_notification_with_temporary_faile assert get_notification_by_id(notification.id).status == 'temporary-failure' -def test_process_mmg_response_unknown_status_updates_notification_with_failed( +def test_process_mmg_response_unknown_status_updates_notification_with_technical_failure( notify_db, notify_db_session, client, mocker ): send_mock = mocker.patch( @@ -403,12 +407,10 @@ def test_process_mmg_response_unknown_status_updates_notification_with_failed( "MSISDN": "447777349060", "status": 10}) create_service_callback_api(service=notification.service, url="https://original_url.com") - response = mmg_post(client, data) - assert response.status_code == 200 - json_data = json.loads(response.data) - assert json_data['result'] == 'success' - assert json_data['message'] == 'MMG callback succeeded. reference {} updated'.format(notification.id) - assert get_notification_by_id(notification.id).status == 'failed' + with pytest.raises(ClientException) as e: + mmg_post(client, data) + assert 'MMG callback failed: status 10 not found.' in str(e.value) + assert get_notification_by_id(notification.id).status == 'technical-failure' assert send_mock.called diff --git a/tests/app/notifications/test_process_client_response.py b/tests/app/notifications/test_process_client_response.py index 3f49901dd..64a3878ad 100644 --- a/tests/app/notifications/test_process_client_response.py +++ b/tests/app/notifications/test_process_client_response.py @@ -1,5 +1,8 @@ import uuid +import pytest + +from app.clients import ClientException from app.notifications.process_client_response import ( validate_callback_data, process_sms_client_response @@ -96,7 +99,7 @@ def test_process_sms_response_returns_error_bad_reference(mocker): stats_mock.assert_not_called() -def test_process_sms_response_returns_error_for_unknown_sms_client(mocker): +def test_process_sms_response_raises_client_exception_for_unknown_sms_client(mocker): stats_mock = mocker.patch('app.notifications.process_client_response.create_outcome_notification_statistic_tasks') success, error = process_sms_client_response(status='000', reference=str(uuid.uuid4()), client_name='sms-client') @@ -105,10 +108,8 @@ def test_process_sms_response_returns_error_for_unknown_sms_client(mocker): stats_mock.assert_not_called() -def test_process_sms_response_returns_error_for_unknown_status(mocker): - stats_mock = mocker.patch('app.notifications.process_client_response.create_outcome_notification_statistic_tasks') +def test_process_sms_response_raises_client_exception_for_unknown_status(mocker): + with pytest.raises(ClientException) as e: + process_sms_client_response(status='000', reference=str(uuid.uuid4()), client_name='Firetext') - success, error = process_sms_client_response(status='000', reference=str(uuid.uuid4()), client_name='Firetext') - assert success is None - assert error == "{} callback failed: status {} not found.".format('Firetext', '000') - stats_mock.assert_not_called() + assert "{} callback failed: status {} not found.".format('Firetext', '000') in str(e.value)