darkhelm/notifications-api
1
0
Fork 0
mirror of https://github.com/GSA/notifications-api.git synced 2025-12-22 16:31:15 -05:00
Code Issues Packages Projects Releases Wiki Activity

Return success of the callback is a duplicate or for an id that does not exist

Browse Source 
This PR changes the response to POST /notifications/sms/<mmg | firetext> from a 400 response to a 200 response.
If we get a callback for a notification more than once or for a notification we log that but we return a 200 success response to the provider.
We have found that there is a situation where the send to provider throws a timeout exception but the provider did get the message, but we still send it to them again.
In which case they send the message twice, and callback for the message twice.
Another case where we may get duplicate callbacks is that the network gave the provider two callbacks meaning they pass those two callbacks onto us.

So it is really difficult to know if we sent to the provider twice or just got two callbacks.

The test_callback has many changes because I took the opportunity to use the client conftest fixture rather than the notify_api fixture.
The only 2 tests really changed are test_mmg_callback_returns_200_when_notification_id_not_found_or_already_updated and test_firetext_callback_returns_200_when_notification_id_not_found_or_already_updated
This commit is contained in:
Rebecca Law
2017-01-25 14:37:11 +00:00
parent c3a9d6d5ed
commit 67657ced26
3 changed files with 589 additions and 634 deletions
Download Patch File Download Diff File Expand all files Collapse all files

6
app/notifications/process_client_response.py
View File

@@ -58,11 +58,11 @@ def process_sms_client_response(status, reference, client_name):
# 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)
if not notification: if not notification:
status_error = "{} callback failed: notification {} either not found or already updated " \ current_app.logger.info("{} 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_message))
return success, status_error return success, errors
if not notification_success: if not notification_success:
current_app.logger.info( current_app.logger.info(

3
app/notifications/rest.py
View File

@@ -149,10 +149,7 @@ def process_firetext_response():
if errors: if errors:
raise InvalidRequest(errors, status_code=400) raise InvalidRequest(errors, status_code=400)
response_code = request.form.get('code')
status = request.form.get('status') status = request.form.get('status')
current_app.logger.info('Firetext status: {}, extended error code: {}'.format(status, response_code))
success, errors = process_sms_client_response(status=status, success, errors = process_sms_client_response(status=status,
reference=request.form.get('reference'), reference=request.form.get('reference'),
client_name=client_name) client_name=client_name)

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

@@ -1,9 +1,9 @@
import uuid import uuid
from datetime import datetime from datetime import datetime
from flask import json from flask import json
from freezegun import freeze_time from freezegun import freeze_time
from unittest.mock import call
import app.celery.tasks import app.celery.tasks
from app.dao.notifications_dao import ( from app.dao.notifications_dao import (
@@ -13,9 +13,7 @@ from app.models import NotificationStatistics
from tests.app.conftest import sample_notification as create_sample_notification from tests.app.conftest import sample_notification as create_sample_notification
def test_firetext_callback_should_not_need_auth(notify_api, mocker): def test_firetext_callback_should_not_need_auth(client, mocker):
with notify_api.test_request_context():
with notify_api.test_client() as client:
mocker.patch('app.statsd_client.incr') mocker.patch('app.statsd_client.incr')
response = client.post( response = client.post(
path='/notifications/sms/firetext', path='/notifications/sms/firetext',
@@ -25,9 +23,7 @@ def test_firetext_callback_should_not_need_auth(notify_api, mocker):
assert response.status_code == 200 assert response.status_code == 200
def test_firetext_callback_should_return_400_if_empty_reference(notify_api, mocker): def test_firetext_callback_should_return_400_if_empty_reference(client, mocker):
with notify_api.test_request_context():
with notify_api.test_client() as client:
mocker.patch('app.statsd_client.incr') mocker.patch('app.statsd_client.incr')
response = client.post( response = client.post(
path='/notifications/sms/firetext', path='/notifications/sms/firetext',
@@ -40,9 +36,7 @@ def test_firetext_callback_should_return_400_if_empty_reference(notify_api, mock
assert json_resp['message'] == ['Firetext callback failed: reference missing'] assert json_resp['message'] == ['Firetext callback failed: reference missing']
def test_firetext_callback_should_return_400_if_no_reference(notify_api, mocker): def test_firetext_callback_should_return_400_if_no_reference(client, mocker):
with notify_api.test_request_context():
with notify_api.test_client() as client:
mocker.patch('app.statsd_client.incr') mocker.patch('app.statsd_client.incr')
response = client.post( response = client.post(
path='/notifications/sms/firetext', path='/notifications/sms/firetext',
@@ -55,9 +49,7 @@ def test_firetext_callback_should_return_400_if_no_reference(notify_api, mocker)
assert json_resp['message'] == ['Firetext callback failed: reference missing'] assert json_resp['message'] == ['Firetext callback failed: reference missing']
def test_firetext_callback_should_return_200_if_send_sms_reference(notify_api, mocker): def test_firetext_callback_should_return_200_if_send_sms_reference(client, mocker):
with notify_api.test_request_context():
with notify_api.test_client() as client:
mocker.patch('app.statsd_client.incr') mocker.patch('app.statsd_client.incr')
response = client.post( response = client.post(
path='/notifications/sms/firetext', path='/notifications/sms/firetext',
@@ -70,9 +62,7 @@ def test_firetext_callback_should_return_200_if_send_sms_reference(notify_api, m
assert json_resp['message'] == 'Firetext callback succeeded: send-sms-code' assert json_resp['message'] == 'Firetext callback succeeded: send-sms-code'
def test_firetext_callback_should_return_400_if_no_status(notify_api, mocker): def test_firetext_callback_should_return_400_if_no_status(client, mocker):
with notify_api.test_request_context():
with notify_api.test_client() as client:
mocker.patch('app.statsd_client.incr') mocker.patch('app.statsd_client.incr')
response = client.post( response = client.post(
path='/notifications/sms/firetext', path='/notifications/sms/firetext',
@@ -85,9 +75,7 @@ def test_firetext_callback_should_return_400_if_no_status(notify_api, 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(notify_api, mocker): def test_firetext_callback_should_return_400_if_unknown_status(client, mocker):
with notify_api.test_request_context():
with notify_api.test_client() as client:
mocker.patch('app.statsd_client.incr') mocker.patch('app.statsd_client.incr')
response = client.post( response = client.post(
path='/notifications/sms/firetext', path='/notifications/sms/firetext',
@@ -100,9 +88,7 @@ def test_firetext_callback_should_return_400_if_unknown_status(notify_api, mocke
assert json_resp['message'] == 'Firetext callback failed: status 99 not found.' assert json_resp['message'] == 'Firetext callback failed: status 99 not found.'
def test_firetext_callback_should_return_400_if_invalid_guid_notification_id(notify_api, mocker): def test_firetext_callback_returns_200_when_notification_id_not_found_or_already_updated(client, mocker):
with notify_api.test_request_context():
with notify_api.test_client() as client:
mocker.patch('app.statsd_client.incr') mocker.patch('app.statsd_client.incr')
response = client.post( response = client.post(
path='/notifications/sms/firetext', path='/notifications/sms/firetext',
@@ -115,14 +101,12 @@ def test_firetext_callback_should_return_400_if_invalid_guid_notification_id(not
assert json_resp['message'] == 'Firetext callback with invalid reference 1234' assert json_resp['message'] == 'Firetext callback with invalid reference 1234'
def test_firetext_callback_should_return_404_if_cannot_find_notification_id( def test_callback_should_return_200_if_cannot_find_notification_id(
notify_db, notify_db,
notify_db_session, notify_db_session,
notify_api, client,
mocker mocker
): ):
with notify_api.test_request_context():
with notify_api.test_client() as client:
mocker.patch('app.statsd_client.incr') mocker.patch('app.statsd_client.incr')
missing_notification_id = uuid.uuid4() missing_notification_id = uuid.uuid4()
response = client.post( response = client.post(
@@ -133,20 +117,13 @@ def test_firetext_callback_should_return_404_if_cannot_find_notification_id(
headers=[('Content-Type', 'application/x-www-form-urlencoded')]) headers=[('Content-Type', 'application/x-www-form-urlencoded')])
json_resp = json.loads(response.get_data(as_text=True)) json_resp = json.loads(response.get_data(as_text=True))
assert response.status_code == 400 assert response.status_code == 200
assert json_resp['result'] == 'error' assert json_resp['result'] == 'success'
assert json_resp['message'] == 'Firetext callback failed: notification {} either not found ' \
'or already updated from sending. Status {}'.format(
missing_notification_id,
'Delivered'
)
def test_firetext_callback_should_update_notification_status( def test_firetext_callback_should_update_notification_status(
notify_db, notify_db_session, notify_api, sample_email_template, mocker notify_db, notify_db_session, client, sample_email_template, mocker
): ):
with notify_api.test_request_context():
with notify_api.test_client() as client:
mocker.patch('app.statsd_client.incr') mocker.patch('app.statsd_client.incr')
notification = create_sample_notification( notification = create_sample_notification(
@@ -179,10 +156,8 @@ def test_firetext_callback_should_update_notification_status(
def test_firetext_callback_should_update_notification_status_failed( def test_firetext_callback_should_update_notification_status_failed(
notify_db, notify_db_session, notify_api, sample_template, mocker notify_db, notify_db_session, client, sample_template, mocker
): ):
with notify_api.test_request_context():
with notify_api.test_client() as client:
mocker.patch('app.statsd_client.incr') mocker.patch('app.statsd_client.incr')
notification = create_sample_notification( notification = create_sample_notification(
@@ -212,9 +187,7 @@ def test_firetext_callback_should_update_notification_status_failed(
assert get_notification_by_id(notification.id).status == 'permanent-failure' assert get_notification_by_id(notification.id).status == 'permanent-failure'
def test_firetext_callback_should_update_notification_status_pending(notify_api, notify_db, notify_db_session, mocker): def test_firetext_callback_should_update_notification_status_pending(client, notify_db, notify_db_session, mocker):
with notify_api.test_request_context():
with notify_api.test_client() as client:
mocker.patch('app.statsd_client.incr') mocker.patch('app.statsd_client.incr')
notification = create_sample_notification( notification = create_sample_notification(
notify_db, notify_db_session, status='sending', sent_at=datetime.utcnow() notify_db, notify_db_session, status='sending', sent_at=datetime.utcnow()
@@ -239,13 +212,11 @@ def test_firetext_callback_should_update_notification_status_pending(notify_api,
def test_firetext_callback_should_update_multiple_notification_status_sent( def test_firetext_callback_should_update_multiple_notification_status_sent(
notify_api, client,
notify_db, notify_db,
notify_db_session, notify_db_session,
mocker mocker
): ):
with notify_api.test_request_context():
with notify_api.test_client() as client:
mocker.patch('app.statsd_client.incr') mocker.patch('app.statsd_client.incr')
notification1 = create_sample_notification( notification1 = create_sample_notification(
notify_db, notify_db_session, status='sending', sent_at=datetime.utcnow() notify_db, notify_db_session, status='sending', sent_at=datetime.utcnow()
@@ -279,12 +250,10 @@ def test_firetext_callback_should_update_multiple_notification_status_sent(
headers=[('Content-Type', 'application/x-www-form-urlencoded')]) headers=[('Content-Type', 'application/x-www-form-urlencoded')])
def test_process_mmg_response_return_200_when_cid_is_send_sms_code(notify_api): def test_process_mmg_response_return_200_when_cid_is_send_sms_code(client):
with notify_api.test_request_context():
data = '{"reference": "10100164", "CID": "send-sms-code", "MSISDN": "447775349060", "status": "3", \ data = '{"reference": "10100164", "CID": "send-sms-code", "MSISDN": "447775349060", "status": "3", \
"deliverytime": "2016-04-05 16:01:07"}' "deliverytime": "2016-04-05 16:01:07"}'
with notify_api.test_client() as client:
response = client.post(path='notifications/sms/mmg', response = client.post(path='notifications/sms/mmg',
data=data, data=data,
headers=[('Content-Type', 'application/json')]) headers=[('Content-Type', 'application/json')])
@@ -295,9 +264,8 @@ def test_process_mmg_response_return_200_when_cid_is_send_sms_code(notify_api):
def test_process_mmg_response_returns_200_when_cid_is_valid_notification_id( def test_process_mmg_response_returns_200_when_cid_is_valid_notification_id(
notify_db, notify_db_session, notify_api notify_db, notify_db_session, client
): ):
with notify_api.test_client() as client:
notification = create_sample_notification( notification = create_sample_notification(
notify_db, notify_db_session, status='sending', sent_at=datetime.utcnow() notify_db, notify_db_session, status='sending', sent_at=datetime.utcnow()
) )
@@ -318,9 +286,8 @@ def test_process_mmg_response_returns_200_when_cid_is_valid_notification_id(
def test_process_mmg_response_status_5_updates_notification_with_permanently_failed( def test_process_mmg_response_status_5_updates_notification_with_permanently_failed(
notify_db, notify_db_session, notify_api notify_db, notify_db_session, client
): ):
with notify_api.test_client() as client:
notification = create_sample_notification( notification = create_sample_notification(
notify_db, notify_db_session, status='sending', sent_at=datetime.utcnow() notify_db, notify_db_session, status='sending', sent_at=datetime.utcnow()
) )
@@ -341,9 +308,8 @@ def test_process_mmg_response_status_5_updates_notification_with_permanently_fai
def test_process_mmg_response_status_2_updates_notification_with_permanently_failed( def test_process_mmg_response_status_2_updates_notification_with_permanently_failed(
notify_db, notify_db_session, notify_api notify_db, notify_db_session, client
): ):
with notify_api.test_client() as client:
notification = create_sample_notification( notification = create_sample_notification(
notify_db, notify_db_session, status='sending', sent_at=datetime.utcnow() notify_db, notify_db_session, status='sending', sent_at=datetime.utcnow()
) )
@@ -363,9 +329,8 @@ def test_process_mmg_response_status_2_updates_notification_with_permanently_fai
def test_process_mmg_response_status_4_updates_notification_with_temporary_failed( def test_process_mmg_response_status_4_updates_notification_with_temporary_failed(
notify_db, notify_db_session, notify_api notify_db, notify_db_session, client
): ):
with notify_api.test_client() as client:
notification = create_sample_notification( notification = create_sample_notification(
notify_db, notify_db_session, status='sending', sent_at=datetime.utcnow() notify_db, notify_db_session, status='sending', sent_at=datetime.utcnow()
) )
@@ -386,10 +351,8 @@ def test_process_mmg_response_status_4_updates_notification_with_temporary_faile
def test_process_mmg_response_unknown_status_updates_notification_with_failed( def test_process_mmg_response_unknown_status_updates_notification_with_failed(
notify_db, notify_db_session, notify_api notify_db, notify_db_session, client
): ):
with notify_api.test_client() as client:
notification = create_sample_notification( notification = create_sample_notification(
notify_db, notify_db_session, status='sending', sent_at=datetime.utcnow() notify_db, notify_db_session, status='sending', sent_at=datetime.utcnow()
) )
@@ -408,8 +371,7 @@ def test_process_mmg_response_unknown_status_updates_notification_with_failed(
assert get_notification_by_id(notification.id).status == 'failed' assert get_notification_by_id(notification.id).status == 'failed'
def test_process_mmg_response_returns_400_for_malformed_data(notify_api): def test_process_mmg_response_returns_400_for_malformed_data(client):
with notify_api.test_client() as client:
data = json.dumps({"reference": "mmg_reference", data = json.dumps({"reference": "mmg_reference",
"monkey": 'random thing', "monkey": 'random thing',
"MSISDN": "447777349060", "MSISDN": "447777349060",
@@ -427,9 +389,7 @@ def test_process_mmg_response_returns_400_for_malformed_data(notify_api):
assert "{} callback failed: {} missing".format('MMG', 'CID') in json_data['message'] assert "{} callback failed: {} missing".format('MMG', 'CID') in json_data['message']
def test_ses_callback_should_not_need_auth(notify_api): def test_ses_callback_should_not_need_auth(client):
with notify_api.test_request_context():
with notify_api.test_client() as client:
response = client.post( response = client.post(
path='/notifications/email/ses', path='/notifications/email/ses',
data=ses_notification_callback(), data=ses_notification_callback(),
@@ -438,9 +398,26 @@ def test_ses_callback_should_not_need_auth(notify_api):
assert response.status_code == 404 assert response.status_code == 404
def test_ses_callback_should_fail_if_invalid_json(notify_api): def test_mmg_callback_returns_200_when_notification_id_not_found_or_already_updated(client):
with notify_api.test_request_context(): data = '{"reference": "10100164", "CID": "send-sms-code", "MSISDN": "447775349060", "status": "3", \
with notify_api.test_client() as client: "deliverytime": "2016-04-05 16:01:07"}'
response = client.post(path='notifications/sms/mmg',
data=data,
headers=[('Content-Type', 'application/json')])
assert response.status_code == 200
def test_ses_callback_should_not_need_auth(client):
response = client.post(
path='/notifications/email/ses',
data=ses_notification_callback(),
headers=[('Content-Type', 'text/plain; charset=UTF-8')]
)
assert response.status_code == 404
def test_ses_callback_should_fail_if_invalid_json(client):
response = client.post( response = client.post(
path='/notifications/email/ses', path='/notifications/email/ses',
data="nonsense", data="nonsense",
@@ -452,9 +429,7 @@ def test_ses_callback_should_fail_if_invalid_json(notify_api):
assert json_resp['message'] == 'SES callback failed: invalid json' assert json_resp['message'] == 'SES callback failed: invalid json'
def test_ses_callback_should_fail_if_invalid_notification_type(notify_api): def test_ses_callback_should_fail_if_invalid_notification_type(client):
with notify_api.test_request_context():
with notify_api.test_client() as client:
response = client.post( response = client.post(
path='/notifications/email/ses', path='/notifications/email/ses',
data=ses_invalid_notification_type_callback(), data=ses_invalid_notification_type_callback(),
@@ -466,9 +441,7 @@ def test_ses_callback_should_fail_if_invalid_notification_type(notify_api):
assert json_resp['message'] == 'SES callback failed: status Unknown not found' assert json_resp['message'] == 'SES callback failed: status Unknown not found'
def test_ses_callback_should_fail_if_missing_message_id(notify_api): def test_ses_callback_should_fail_if_missing_message_id(client):
with notify_api.test_request_context():
with notify_api.test_client() as client:
response = client.post( response = client.post(
path='/notifications/email/ses', path='/notifications/email/ses',
data=ses_missing_notification_id_callback(), data=ses_missing_notification_id_callback(),
@@ -480,9 +453,7 @@ def test_ses_callback_should_fail_if_missing_message_id(notify_api):
assert json_resp['message'] == 'SES callback failed: messageId missing' assert json_resp['message'] == 'SES callback failed: messageId missing'
def test_ses_callback_should_fail_if_notification_cannot_be_found(notify_db, notify_db_session, notify_api): def test_ses_callback_should_fail_if_notification_cannot_be_found(notify_db, notify_db_session, client):
with notify_api.test_request_context():
with notify_api.test_client() as client:
response = client.post( response = client.post(
path='/notifications/email/ses', path='/notifications/email/ses',
data=ses_invalid_notification_id_callback(), data=ses_invalid_notification_id_callback(),
@@ -495,13 +466,11 @@ def test_ses_callback_should_fail_if_notification_cannot_be_found(notify_db, not
def test_ses_callback_should_update_notification_status( def test_ses_callback_should_update_notification_status(
notify_api, client,
notify_db, notify_db,
notify_db_session, notify_db_session,
sample_email_template, sample_email_template,
mocker): mocker):
with notify_api.test_request_context():
with notify_api.test_client() as client:
with freeze_time('2001-01-01T12:00:00'): with freeze_time('2001-01-01T12:00:00'):
mocker.patch('app.statsd_client.incr') mocker.patch('app.statsd_client.incr')
mocker.patch('app.statsd_client.timing_with_dates') mocker.patch('app.statsd_client.timing_with_dates')
@@ -533,13 +502,11 @@ def test_ses_callback_should_update_notification_status(
def test_ses_callback_should_update_multiple_notification_status_sent( def test_ses_callback_should_update_multiple_notification_status_sent(
notify_api, client,
notify_db, notify_db,
notify_db_session, notify_db_session,
sample_email_template, sample_email_template,
mocker): mocker):
with notify_api.test_request_context():
with notify_api.test_client() as client:
notification1 = create_sample_notification( notification1 = create_sample_notification(
notify_db, notify_db,
notify_db_session, notify_db_session,
@@ -585,12 +552,10 @@ def test_ses_callback_should_update_multiple_notification_status_sent(
assert resp3.status_code == 200 assert resp3.status_code == 200
def test_ses_callback_should_set_status_to_temporary_failure(notify_api, def test_ses_callback_should_set_status_to_temporary_failure(client,
notify_db, notify_db,
notify_db_session, notify_db_session,
sample_email_template): sample_email_template):
with notify_api.test_request_context():
with notify_api.test_client() as client:
notification = create_sample_notification( notification = create_sample_notification(
notify_db, notify_db,
notify_db_session, notify_db_session,
@@ -613,12 +578,10 @@ def test_ses_callback_should_set_status_to_temporary_failure(notify_api,
assert get_notification_by_id(notification.id).status == 'temporary-failure' assert get_notification_by_id(notification.id).status == 'temporary-failure'
def test_ses_callback_should_not_set_status_once_status_is_delivered(notify_api, def test_ses_callback_should_not_set_status_once_status_is_delivered(client,
notify_db, notify_db,
notify_db_session, notify_db_session,
sample_email_template): sample_email_template):
with notify_api.test_request_context():
with notify_api.test_client() as client:
notification = create_sample_notification( notification = create_sample_notification(
notify_db, notify_db,
notify_db_session, notify_db_session,
@@ -642,12 +605,10 @@ def test_ses_callback_should_not_set_status_once_status_is_delivered(notify_api,
assert get_notification_by_id(notification.id).status == 'delivered' assert get_notification_by_id(notification.id).status == 'delivered'
def test_ses_callback_should_set_status_to_permanent_failure(notify_api, def test_ses_callback_should_set_status_to_permanent_failure(client,
notify_db, notify_db,
notify_db_session, notify_db_session,
sample_email_template): sample_email_template):
with notify_api.test_request_context():
with notify_api.test_client() as client:
notification = create_sample_notification( notification = create_sample_notification(
notify_db, notify_db,
notify_db_session, notify_db_session,
@@ -671,8 +632,7 @@ def test_ses_callback_should_set_status_to_permanent_failure(notify_api,
assert get_notification_by_id(notification.id).status == 'permanent-failure' assert get_notification_by_id(notification.id).status == 'permanent-failure'
def test_process_mmg_response_records_statsd(notify_db, notify_db_session, notify_api, mocker): def test_process_mmg_response_records_statsd(notify_db, notify_db_session, client, mocker):
with notify_api.test_client() as client:
with freeze_time('2001-01-01T12:00:00'): with freeze_time('2001-01-01T12:00:00'):
mocker.patch('app.statsd_client.incr') mocker.patch('app.statsd_client.incr')
@@ -697,9 +657,7 @@ def test_process_mmg_response_records_statsd(notify_db, notify_db_session, notif
) )
def test_firetext_callback_should_record_statsd(notify_api, notify_db, notify_db_session, mocker): def test_firetext_callback_should_record_statsd(client, notify_db, notify_db_session, mocker):
with notify_api.test_request_context():
with notify_api.test_client() as client:
with freeze_time('2001-01-01T12:00:00'): with freeze_time('2001-01-01T12:00:00'):
mocker.patch('app.statsd_client.incr') mocker.patch('app.statsd_client.incr')