darkhelm/notifications-api
1
0
Fork 0
mirror of https://github.com/GSA/notifications-api.git synced 2026-09-08 16:43:11 -04:00
Code Issues Packages Projects Releases Wiki Activity

We are not handling the case of an unknown status code sent by the SMS provider. This PR attempts to fix that.

Browse Source 
- If the SMS client sends a status code that we do not recognize raise a ClientException and set the notification status to technical-failure
- Simplified the code in process_client_response, using a simple map.
This commit is contained in:
Rebecca Law
2018-02-07 17:49:15 +00:00
parent a5343fb837
commit b2dfa59b1b
7 changed files with 60 additions and 120 deletions
Download Patch File Download Diff File Expand all files Collapse all files

22
app/clients/sms/firetext.py
View File

@@ -5,7 +5,6 @@ from monotonic import monotonic
from requests import request, RequestException from requests import request, RequestException
from app.clients.sms import (SmsClient, SmsClientResponseException) from app.clients.sms import (SmsClient, SmsClientResponseException)
from app.clients import STATISTICS_DELIVERED, STATISTICS_FAILURE
logger = logging.getLogger(__name__) logger = logging.getLogger(__name__)
@@ -15,24 +14,9 @@ logger = logging.getLogger(__name__)
# the notification status to temporary-failure rather than permanent failure. # the notification status to temporary-failure rather than permanent failure.
# See the code in the notification_dao.update_notifications_status_by_id # See the code in the notification_dao.update_notifications_status_by_id
firetext_responses = { firetext_responses = {
'0': { '0': 'delivered',
"message": 'Delivered', '1': 'permanent-failure',
"notification_statistics_status": STATISTICS_DELIVERED, '2': 'pending'
"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'
}
} }

37
app/clients/sms/mmg.py
View File

@@ -1,45 +1,18 @@
import json import json
from monotonic import monotonic from monotonic import monotonic
from requests import (request, RequestException) from requests import (request, RequestException)
from app.clients import (STATISTICS_DELIVERED, STATISTICS_FAILURE)
from app.clients.sms import (SmsClient, SmsClientResponseException) from app.clients.sms import (SmsClient, SmsClientResponseException)
mmg_response_map = { mmg_response_map = {
'2': { '2': 'permanent-failure',
"message": ' Permanent failure', '3': 'delivered',
"notification_statistics_status": STATISTICS_FAILURE, '4': 'temporary-failure',
"success": False, '5': 'permanent-failure'
"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'
}
} }
def get_mmg_responses(status): def get_mmg_responses(status):
return mmg_response_map.get(status, mmg_response_map.get('default')) return mmg_response_map[status]
class MMGClientResponseException(SmsClientResponseException): class MMGClientResponseException(SmsClientResponseException):

27
app/notifications/process_client_response.py
View File

@@ -4,6 +4,7 @@ from datetime import datetime
from flask import current_app from flask import current_app
from app import statsd_client from app import statsd_client
from app.clients import ClientException
from app.dao import notifications_dao from app.dao import notifications_dao
from app.clients.sms.firetext import get_firetext_responses from app.clients.sms.firetext import get_firetext_responses
from app.clients.sms.mmg import get_mmg_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 # validate status
try: try:
response_dict = response_parser(status) notification_status = response_parser(status)
current_app.logger.info('{} callback return status of {} for reference: {}'.format( current_app.logger.info('{} callback return status of {} for reference: {}'.format(
client_name, status, reference) client_name, status, reference)
) )
except KeyError: except KeyError:
msg = "{} callback failed: status {} not found.".format(client_name, status) process_for_status(notification_status='technical-failure', client_name=client_name, reference=reference)
return success, msg raise ClientException("{} callback failed: status {} not found.".format(client_name, status))
notification_status = response_dict['notification_status'] success = process_for_status(notification_status=notification_status, client_name=client_name, reference=reference)
notification_status_message = response_dict['message'] return success, errors
notification_success = response_dict['success']
def process_for_status(notification_status, client_name, reference):
# record stats # record stats
notification = notifications_dao.update_notification_status_by_id(reference, notification_status) 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 " current_app.logger.warning("{} callback failed: notification {} either not found or already updated "
"from sending. Status {}".format(client_name, "from sending. Status {}".format(client_name,
reference, reference,
notification_status_message)) notification_status))
return success, errors return
if not notification_success:
current_app.logger.info(
"{} delivery failed: notification {} has error found. Status {}".format(client_name,
reference,
notification_status_message))
statsd_client.incr('callback.{}.{}'.format(client_name.lower(), notification_status)) statsd_client.incr('callback.{}.{}'.format(client_name.lower(), notification_status))
if notification.sent_at: 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) send_delivery_status_to_service.apply_async([str(notification.id)], queue=QueueNames.CALLBACKS)
success = "{} callback succeeded. reference {} updated".format(client_name, reference) success = "{} callback succeeded. reference {} updated".format(client_name, reference)
return success, errors return success

18
tests/app/clients/test_firetext.py
View File

@@ -9,27 +9,15 @@ from app.clients.sms.firetext import get_firetext_responses, SmsClientResponseEx
def test_should_return_correct_details_for_delivery(): def test_should_return_correct_details_for_delivery():
response_dict = get_firetext_responses('0') get_firetext_responses('0') == 'delivered'
assert response_dict['message'] == 'Delivered'
assert response_dict['notification_status'] == 'delivered'
assert response_dict['notification_statistics_status'] == 'delivered'
assert response_dict['success']
def test_should_return_correct_details_for_bounced(): def test_should_return_correct_details_for_bounced():
response_dict = get_firetext_responses('1') get_firetext_responses('1') == 'permanent-failure'
assert response_dict['message'] == 'Declined'
assert response_dict['notification_status'] == 'permanent-failure'
assert response_dict['notification_statistics_status'] == 'failure'
assert not response_dict['success']
def test_should_return_correct_details_for_complaint(): def test_should_return_correct_details_for_complaint():
response_dict = get_firetext_responses('2') get_firetext_responses('2') == 'pending'
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']
def test_should_be_none_if_unrecognised_status_code(): def test_should_be_none_if_unrecognised_status_code():

29
tests/app/clients/test_mmg.py
View File

@@ -10,27 +10,22 @@ from app.clients.sms.mmg import get_mmg_responses, MMGClientResponseException
def test_should_return_correct_details_for_delivery(): def test_should_return_correct_details_for_delivery():
response_dict = get_mmg_responses('3') get_mmg_responses('3') == 'delivered'
assert response_dict['message'] == 'Delivered'
assert response_dict['notification_status'] == 'delivered'
assert response_dict['notification_statistics_status'] == 'delivered'
assert response_dict['success']
def test_should_return_correct_details_for_bounced(): def test_should_return_correct_details_for_temporary_failure():
response_dict = get_mmg_responses('50') get_mmg_responses('4') == 'temporary-failure'
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_be_none_if_unrecognised_status_code(): @pytest.mark.parametrize('status', ['5', '2'])
response_dict = get_mmg_responses('blah') def test_should_return_correct_details_for_bounced(status):
assert response_dict['message'] == 'Declined' get_mmg_responses(status) == 'permanent-failure'
assert response_dict['notification_status'] == 'failed'
assert response_dict['notification_statistics_status'] == 'failure'
assert not response_dict['success'] 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): def test_send_sms_successful_returns_mmg_response(notify_api, mocker):

32
tests/app/notifications/rest/test_callbacks.py
View File

@@ -2,10 +2,12 @@ import uuid
from datetime import datetime from datetime import datetime
import pytest
from flask import json from flask import json
from freezegun import freeze_time from freezegun import freeze_time
import app.celery.tasks import app.celery.tasks
from app.clients import ClientException
from app.dao.notifications_dao import ( from app.dao.notifications_dao import (
get_notification_by_id 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'] 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') mocker.patch('app.statsd_client.incr')
data = 'mobile=441234123123&status=99&time=2016-03-10 14:17:00&reference={}'.format(uuid.uuid4()) data = 'mobile=441234123123&status=99&time=2016-03-10 14:17:00&reference={}'.format(notification.id)
response = firetext_post(client, data) with pytest.raises(ClientException) as e:
firetext_post(client, data)
json_resp = json.loads(response.get_data(as_text=True)) assert get_notification_by_id(notification.id).status == 'technical-failure'
assert response.status_code == 400 assert 'Firetext callback failed: status 99 not found.' in str(e.value)
assert json_resp['result'] == 'error'
assert json_resp['message'] == 'Firetext callback failed: status 99 not found.'
def test_firetext_callback_returns_200_when_notification_id_not_found_or_already_updated(client, mocker): 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' 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 notify_db, notify_db_session, client, mocker
): ):
send_mock = mocker.patch( send_mock = mocker.patch(
@@ -403,12 +407,10 @@ def test_process_mmg_response_unknown_status_updates_notification_with_failed(
"MSISDN": "447777349060", "MSISDN": "447777349060",
"status": 10}) "status": 10})
create_service_callback_api(service=notification.service, url="https://original_url.com") create_service_callback_api(service=notification.service, url="https://original_url.com")
response = mmg_post(client, data) with pytest.raises(ClientException) as e:
assert response.status_code == 200 mmg_post(client, data)
json_data = json.loads(response.data) assert 'MMG callback failed: status 10 not found.' in str(e.value)
assert json_data['result'] == 'success' assert get_notification_by_id(notification.id).status == 'technical-failure'
assert json_data['message'] == 'MMG callback succeeded. reference {} updated'.format(notification.id)
assert get_notification_by_id(notification.id).status == 'failed'
assert send_mock.called assert send_mock.called

15
tests/app/notifications/test_process_client_response.py
View File

@@ -1,5 +1,8 @@
import uuid import uuid
import pytest
from app.clients import ClientException
from app.notifications.process_client_response import ( from app.notifications.process_client_response import (
validate_callback_data, validate_callback_data,
process_sms_client_response process_sms_client_response
@@ -96,7 +99,7 @@ def test_process_sms_response_returns_error_bad_reference(mocker):
stats_mock.assert_not_called() 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') 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') 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() stats_mock.assert_not_called()
def test_process_sms_response_returns_error_for_unknown_status(mocker): def test_process_sms_response_raises_client_exception_for_unknown_status(mocker):
stats_mock = mocker.patch('app.notifications.process_client_response.create_outcome_notification_statistic_tasks') 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 "{} callback failed: status {} not found.".format('Firetext', '000') in str(e.value)
assert success is None
assert error == "{} callback failed: status {} not found.".format('Firetext', '000')
stats_mock.assert_not_called()