diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 735941070..57232b632 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -219,8 +219,9 @@ def dao_update_notification(notification): def update_notification_status_by_id(notification_id, status, notification_statistics_status): - count = db.session.query(Notification).filter_by( - id=notification_id + count = db.session.query(Notification).filter( + Notification.id == notification_id, + Notification.status == 'sending' ).update({ Notification.status: status }) diff --git a/app/notifications/process_client_response.py b/app/notifications/process_client_response.py index 01f1b4c62..3e2e2e90e 100644 --- a/app/notifications/process_client_response.py +++ b/app/notifications/process_client_response.py @@ -58,9 +58,10 @@ def process_sms_client_response(status, reference, client_name): notification_status, notification_statistics_status) if update_success == 0: - status_error = "{} callback failed: notification {} not found. Status {}".format(client_name, - reference, - notification_status_message) + status_error = "{} callback failed: notification {} either not found or already updated " \ + "from sending. Status {}".format(client_name, + reference, + notification_status_message) return success, status_error if not notification_success: diff --git a/tests/app/dao/test_notification_dao.py b/tests/app/dao/test_notification_dao.py index b7aba158d..674f018a4 100644 --- a/tests/app/dao/test_notification_dao.py +++ b/tests/app/dao/test_notification_dao.py @@ -73,15 +73,18 @@ def test_should_by_able_to_update_status_by_id(sample_notification): count = update_notification_status_by_id(sample_notification.id, 'delivered', 'delivered') assert count == 1 assert Notification.query.get(sample_notification.id).status == 'delivered' - assert NotificationStatistics.query.filter_by( - service_id=sample_notification.service.id - ).one().sms_delivered == 1 - assert NotificationStatistics.query.filter_by( - service_id=sample_notification.service.id - ).one().sms_requested == 1 - assert NotificationStatistics.query.filter_by( - service_id=sample_notification.service.id - ).one().sms_failed == 0 + stats = NotificationStatistics.query.filter_by(service_id=sample_notification.service.id).one() + assert stats.sms_delivered == 1 + assert stats.sms_requested == 1 + assert stats.sms_failed == 0 + + +def test_should_not_update_status_by_id_if_not_sending(notify_db, notify_db_session): + notification = sample_notification(notify_db, notify_db_session, status='delivered') + assert Notification.query.get(notification.id).status == 'delivered' + count = update_notification_status_by_id(notification.id, 'failed', 'failure') + assert count == 0 + assert Notification.query.get(notification.id).status == 'delivered' def test_should_not_update_status_one_notification_status_is_delivered(sample_email_template, ses_provider): @@ -100,15 +103,10 @@ def test_should_not_update_status_one_notification_status_is_delivered(sample_em update_notification_status_by_reference('reference', 'delivered', 'temporary-failure') assert Notification.query.get(notification.id).status == 'delivered' - assert NotificationStatistics.query.filter_by( - service_id=sample_email_template.service.id - ).one().emails_delivered == 1 - assert NotificationStatistics.query.filter_by( - service_id=sample_email_template.service.id - ).one().emails_requested == 1 - assert NotificationStatistics.query.filter_by( - service_id=sample_email_template.service.id - ).one().emails_failed == 0 + stats = NotificationStatistics.query.filter_by(service_id=sample_email_template.service.id).one() + assert stats.emails_delivered == 1 + assert stats.emails_requested == 1 + assert stats.emails_failed == 0 def test_should_be_able_to_record_statistics_failure_for_sms(sample_notification): @@ -116,15 +114,10 @@ def test_should_be_able_to_record_statistics_failure_for_sms(sample_notification count = update_notification_status_by_id(sample_notification.id, 'delivered', 'failure') assert count == 1 assert Notification.query.get(sample_notification.id).status == 'delivered' - assert NotificationStatistics.query.filter_by( - service_id=sample_notification.service.id - ).one().sms_delivered == 0 - assert NotificationStatistics.query.filter_by( - service_id=sample_notification.service.id - ).one().sms_requested == 1 - assert NotificationStatistics.query.filter_by( - service_id=sample_notification.service.id - ).one().sms_failed == 1 + stats = NotificationStatistics.query.filter_by(service_id=sample_notification.service.id).one() + assert stats.sms_delivered == 0 + assert stats.sms_requested == 1 + assert stats.sms_failed == 1 def test_should_be_able_to_record_statistics_failure_for_email(sample_email_template, ses_provider): @@ -137,15 +130,10 @@ def test_should_be_able_to_record_statistics_failure_for_email(sample_email_temp count = update_notification_status_by_reference('reference', 'failed', 'failure') assert count == 1 assert Notification.query.get(notification.id).status == 'failed' - assert NotificationStatistics.query.filter_by( - service_id=notification.service.id - ).one().emails_delivered == 0 - assert NotificationStatistics.query.filter_by( - service_id=notification.service.id - ).one().emails_requested == 1 - assert NotificationStatistics.query.filter_by( - service_id=notification.service.id - ).one().emails_failed == 1 + stats = NotificationStatistics.query.filter_by(service_id=notification.service.id).one() + assert stats.emails_delivered == 0 + assert stats.emails_requested == 1 + assert stats.emails_failed == 1 def test_should_return_zero_count_if_no_notification_with_id(): diff --git a/tests/app/notifications/test_rest.py b/tests/app/notifications/test_rest.py index b6a94ffea..4f7aea7c1 100644 --- a/tests/app/notifications/test_rest.py +++ b/tests/app/notifications/test_rest.py @@ -1158,7 +1158,8 @@ def test_firetext_callback_should_return_404_if_cannot_find_notification_id(noti 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: notification {} not found. Status {}'.format( + assert json_resp['message'] == 'Firetext callback failed: notification {} either not found ' \ + 'or already updated from sending. Status {}'.format( missing_notification_id, 'Delivered' ) @@ -1221,9 +1222,9 @@ def test_firetext_callback_should_update_notification_status_failed(notify_api, def test_firetext_callback_should_update_notification_status_sent(notify_api, notify_db, notify_db_session): with notify_api.test_request_context(): with notify_api.test_client() as client: - notification = create_sample_notification(notify_db, notify_db_session, status='delivered') + notification = create_sample_notification(notify_db, notify_db_session, status='sending') original = get_notification_by_id(notification.id) - assert original.status == 'delivered' + assert original.status == 'sending' response = client.post( path='/notifications/sms/firetext', @@ -1248,9 +1249,9 @@ def test_firetext_callback_should_update_notification_status_sent(notify_api, no def test_firetext_callback_should_update_multiple_notification_status_sent(notify_api, notify_db, notify_db_session): with notify_api.test_request_context(): with notify_api.test_client() as client: - notification1 = create_sample_notification(notify_db, notify_db_session, status='delivered') - notification2 = create_sample_notification(notify_db, notify_db_session, status='delivered') - notification3 = create_sample_notification(notify_db, notify_db_session, status='delivered') + notification1 = create_sample_notification(notify_db, notify_db_session, status='sending') + notification2 = create_sample_notification(notify_db, notify_db_session, status='sending') + notification3 = create_sample_notification(notify_db, notify_db_session, status='sending') client.post( path='/notifications/sms/firetext', @@ -1330,8 +1331,8 @@ def test_process_mmg_response_status_5_updates_notification_with_permanently_fai assert get_notification_by_id(sample_notification.id).status == 'permanent-failure' -def test_process_mmg_response_status_2_or_4_updates_notification_with_temporary_failed(notify_api, - sample_notification): +def test_process_mmg_response_status_2_updates_notification_with_temporary_failed(notify_api, + sample_notification): with notify_api.test_client() as client: data = json.dumps({"reference": "mmg_reference", "CID": str(sample_notification.id), @@ -1347,6 +1348,10 @@ def test_process_mmg_response_status_2_or_4_updates_notification_with_temporary_ assert json_data['message'] == 'MMG callback succeeded. reference {} updated'.format(sample_notification.id) assert get_notification_by_id(sample_notification.id).status == 'temporary-failure' + +def test_process_mmg_response_status_4_updates_notification_with_temporary_failed(notify_api, + sample_notification): + with notify_api.test_client() as client: data = json.dumps({"reference": "mmg_reference", "CID": str(sample_notification.id), "MSISDN": "447777349060", @@ -1510,21 +1515,21 @@ def test_ses_callback_should_update_multiple_notification_status_sent( notify_db_session, template=sample_email_template, reference='ref1', - status='delivered') + status='sending') notification2 = create_sample_notification( notify_db, notify_db_session, template=sample_email_template, reference='ref2', - status='delivered') + status='sending') notification3 = create_sample_notification( notify_db, notify_db_session, template=sample_email_template, reference='ref2', - status='delivered') + status='sending') client.post( path='/notifications/sms/firetext', @@ -1766,7 +1771,7 @@ def test_firetext_callback_should_record_statsd(notify_api, notify_db, notify_db with notify_api.test_request_context(): with notify_api.test_client() as client: mocker.patch('app.statsd_client.incr') - notification = create_sample_notification(notify_db, notify_db_session, status='delivered') + notification = create_sample_notification(notify_db, notify_db_session, status='sending') client.post( path='/notifications/sms/firetext',