diff --git a/app/celery/process_ses_receipts_tasks.py b/app/celery/process_ses_receipts_tasks.py index 3380047f3..ea4838e79 100644 --- a/app/celery/process_ses_receipts_tasks.py +++ b/app/celery/process_ses_receipts_tasks.py @@ -39,7 +39,7 @@ def process_ses_results(self, response): reference = ses_message['mail']['messageId'] try: - notification = notifications_dao.dao_get_notification_by_reference(reference) + notification = notifications_dao.dao_get_notification_or_history_by_reference(reference=reference) except NoResultFound: message_time = iso8601.parse_date(ses_message['mail']['timestamp']).replace(tzinfo=None) if datetime.utcnow() - message_time < timedelta(minutes=5): @@ -50,22 +50,17 @@ def process_ses_results(self, response): ) return - if notification.status not in {NOTIFICATION_SENDING, NOTIFICATION_PENDING}: - notifications_dao._duplicate_update_warning(notification, notification_status) - return - - notifications_dao._update_notification_status(notification=notification, status=notification_status) - - if not aws_response_dict['success']: - current_app.logger.info( - "SES delivery failed: notification id {} and reference {} has error found. Status {}".format( - notification.id, reference, aws_response_dict['message'] - ) + if notification.status not in [NOTIFICATION_SENDING, NOTIFICATION_PENDING]: + notifications_dao._duplicate_update_warning( + notification=notification, + status=notification_status ) + return else: - current_app.logger.info('SES callback return status of {} for notification: {}'.format( - notification_status, notification.id - )) + notifications_dao.dao_update_notifications_by_reference( + references=[reference], + update_dict={'status': notification_status} + ) statsd_client.incr('callback.ses.{}'.format(notification_status)) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index fa29de4f9..02291d728 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -41,7 +41,7 @@ from app.dao.notifications_dao import ( dao_update_notifications_by_reference, dao_get_last_notification_added_for_job_id, update_notification_status_by_reference, - dao_get_notification_history_by_reference, + dao_get_notification_or_history_by_reference, ) from app.dao.provider_details_dao import get_provider_details_by_notification_type from app.dao.returned_letters_dao import insert_or_update_returned_letters @@ -303,7 +303,6 @@ def save_api_email(self, service = dao_fetch_service_by_id(notification['service_id']) try: - current_app.logger.info(f"Persisting notification {notification['id']}") persist_notification( notification_id=notification["id"], @@ -327,7 +326,7 @@ def save_api_email(self, [notification['id']], queue=q ) - current_app.logger.info(f"Email {notification['id']} has been persisted.") + current_app.logger.info(f"Email {notification['id']} has been persisted and sent to delivery queue.") except IntegrityError: current_app.logger.info(f"Email {notification['id']} already exists.") @@ -557,7 +556,7 @@ def update_letter_notification(filename, temporary_failures, update): def check_billable_units(notification_update): - notification = dao_get_notification_history_by_reference(notification_update.reference) + notification = dao_get_notification_or_history_by_reference(notification_update.reference) if int(notification_update.page_count) != notification.billable_units: msg = 'Notification with id {} has {} billable_units but DVLA says page count is {}'.format( diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index d142fb595..caeccb33b 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -352,7 +352,6 @@ def insert_notification_history_delete_notifications( AND notification_type = :notification_type AND created_at < :timestamp_to_delete_backwards_from AND key_type = 'normal' - AND notification_status in ('delivered', 'permanent-failure', 'temporary-failure') limit :qry_limit """ # Insert into NotificationHistory if the row already exists do nothing. @@ -632,7 +631,7 @@ def dao_get_notification_by_reference(reference): @statsd(namespace="dao") -def dao_get_notification_history_by_reference(reference): +def dao_get_notification_or_history_by_reference(reference): try: # This try except is necessary because in test keys and research mode does not create notification history. # Otherwise we could just search for the NotificationHistory object diff --git a/app/notifications/notifications_ses_callback.py b/app/notifications/notifications_ses_callback.py index 496ad9321..101ff1ecc 100644 --- a/app/notifications/notifications_ses_callback.py +++ b/app/notifications/notifications_ses_callback.py @@ -1,7 +1,7 @@ from flask import current_app from app.dao.complaint_dao import save_complaint -from app.dao.notifications_dao import dao_get_notification_history_by_reference +from app.dao.notifications_dao import dao_get_notification_or_history_by_reference from app.dao.service_callback_api_dao import ( get_service_delivery_status_callback_api_for_service, get_service_complaint_callback_api_for_service ) @@ -33,7 +33,7 @@ def handle_complaint(ses_message): except KeyError as e: current_app.logger.exception("Complaint from SES failed to get reference from message", e) return - notification = dao_get_notification_history_by_reference(reference) + notification = dao_get_notification_or_history_by_reference(reference) ses_complaint = ses_message.get('complaint', None) complaint = Complaint( diff --git a/tests/app/celery/test_process_ses_receipts_tasks.py b/tests/app/celery/test_process_ses_receipts_tasks.py index 8e7a5b637..2e7462904 100644 --- a/tests/app/celery/test_process_ses_receipts_tasks.py +++ b/tests/app/celery/test_process_ses_receipts_tasks.py @@ -1,17 +1,23 @@ import json from datetime import datetime - from freezegun import freeze_time from app import statsd_client, encryption from app.celery.process_ses_receipts_tasks import process_ses_results -from app.celery.research_mode_tasks import ses_hard_bounce_callback, ses_soft_bounce_callback, ses_notification_callback +from app.celery.research_mode_tasks import ( + ses_hard_bounce_callback, + ses_soft_bounce_callback, + ses_notification_callback +) from app.celery.service_callback_tasks import create_delivery_status_callback_data from app.dao.notifications_dao import get_notification_by_id from app.models import Complaint, Notification -from app.notifications.notifications_ses_callback import remove_emails_from_complaint, remove_emails_from_bounce +from app.notifications.notifications_ses_callback import ( + remove_emails_from_complaint, + remove_emails_from_bounce +) from tests.app.db import ( create_notification, @@ -29,7 +35,7 @@ def test_process_ses_results(sample_email_template): def test_process_ses_results_retry_called(sample_email_template, notify_db, mocker): create_notification(sample_email_template, reference='ref1', sent_at=datetime.utcnow(), status='sending') - mocker.patch("app.dao.notifications_dao._update_notification_status", side_effect=Exception("EXPECTED")) + mocker.patch("app.dao.notifications_dao.dao_update_notifications_by_reference", side_effect=Exception("EXPECTED")) mocked = mocker.patch('app.celery.process_ses_receipts_tasks.process_ses_results.retry') process_ses_results(response=ses_notification_callback(reference='ref1')) assert mocked.call_count != 0 @@ -90,13 +96,15 @@ def test_ses_callback_should_update_notification_status( def test_ses_callback_should_not_update_notification_status_if_already_delivered(sample_email_template, mocker): mock_dup = mocker.patch('app.celery.process_ses_receipts_tasks.notifications_dao._duplicate_update_warning') - mock_upd = mocker.patch('app.celery.process_ses_receipts_tasks.notifications_dao._update_notification_status') + mock_upd = mocker.patch( + 'app.celery.process_ses_receipts_tasks.notifications_dao.dao_update_notifications_by_reference' + ) notification = create_notification(template=sample_email_template, reference='ref', status='delivered') assert process_ses_results(ses_notification_callback(reference='ref')) is None assert get_notification_by_id(notification.id).status == 'delivered' - mock_dup.assert_called_once_with(notification, 'delivered') + mock_dup.assert_called_once_with(notification=notification, status='delivered') assert mock_upd.call_count == 0 diff --git a/tests/app/dao/notification_dao/test_notification_dao.py b/tests/app/dao/notification_dao/test_notification_dao.py index 4f406da9b..726dde272 100644 --- a/tests/app/dao/notification_dao/test_notification_dao.py +++ b/tests/app/dao/notification_dao/test_notification_dao.py @@ -31,7 +31,7 @@ from app.dao.notifications_dao import ( update_notification_status_by_reference, dao_get_notification_by_reference, dao_get_notifications_by_references, - dao_get_notification_history_by_reference, + dao_get_notification_or_history_by_reference, notifications_not_yet_sent, ) from app.models import ( @@ -1567,7 +1567,9 @@ def test_dao_update_notifications_by_reference_set_returned_letter_status(sample assert updated_count == 1 assert updated_history_count == 0 - assert Notification.query.get(notification.id).status == 'returned-letter' + updated_notification = Notification.query.get(notification.id) + assert updated_notification.status == 'returned-letter' + assert updated_notification.updated_at <= datetime.utcnow() def test_dao_update_notifications_by_reference_updates_history_when_one_of_two_notifications_exists( @@ -1618,28 +1620,28 @@ def test_dao_get_notifications_by_references(sample_template): assert notifications[1].id in [notification_1.id, notification_2.id] -def test_dao_get_notification_history_by_reference_with_one_match_returns_notification( +def test_dao_get_notification_or_history_by_reference_with_one_match_returns_notification( sample_letter_template ): create_notification(template=sample_letter_template, reference='REF1') - notification = dao_get_notification_history_by_reference('REF1') + notification = dao_get_notification_or_history_by_reference('REF1') assert notification.reference == 'REF1' -def test_dao_get_notification_history_by_reference_with_multiple_matches_raises_error( +def test_dao_get_notification_or_history_by_reference_with_multiple_matches_raises_error( sample_letter_template ): create_notification(template=sample_letter_template, reference='REF1') create_notification(template=sample_letter_template, reference='REF1') with pytest.raises(SQLAlchemyError): - dao_get_notification_history_by_reference('REF1') + dao_get_notification_or_history_by_reference('REF1') -def test_dao_get_notification_history_by_reference_with_no_matches_raises_error(notify_db): +def test_dao_get_notification_or_history_by_reference_with_no_matches_raises_error(notify_db): with pytest.raises(SQLAlchemyError): - dao_get_notification_history_by_reference('REF1') + dao_get_notification_or_history_by_reference('REF1') @pytest.mark.parametrize("notification_type", diff --git a/tests/app/dao/notification_dao/test_notification_dao_delete_notifications.py b/tests/app/dao/notification_dao/test_notification_dao_delete_notifications.py index 88a517a1a..0f37a1e90 100644 --- a/tests/app/dao/notification_dao/test_notification_dao_delete_notifications.py +++ b/tests/app/dao/notification_dao/test_notification_dao_delete_notifications.py @@ -252,42 +252,44 @@ def test_delete_notifications_returns_sum_correctly(sample_template): @freeze_time('2020-03-20 14:00') def test_insert_notification_history_delete_notifications(sample_email_template): # should be deleted - create_notification(template=sample_email_template, - created_at=datetime.utcnow() + timedelta(minutes=4), status='delivered') - create_notification(template=sample_email_template, - created_at=datetime.utcnow() + timedelta(minutes=20), status='permanent-failure') - create_notification(template=sample_email_template, - created_at=datetime.utcnow() + timedelta(minutes=30), status='temporary-failure') - create_notification(template=sample_email_template, - created_at=datetime.utcnow() + timedelta(minutes=59), status='temporary-failure') - - # should NOT be deleted - create_notification(template=sample_email_template, - created_at=datetime.utcnow() + timedelta(hours=1), status='delivered') - create_notification(template=sample_email_template, - created_at=datetime.utcnow() + timedelta(minutes=61), status='temporary-failure') - create_notification(template=sample_email_template, - created_at=datetime.utcnow() + timedelta(hours=1, seconds=1), status='temporary-failure') - create_notification(template=sample_email_template, - created_at=datetime.utcnow() + timedelta(minutes=20), status='created') + n1 = create_notification(template=sample_email_template, + created_at=datetime.utcnow() - timedelta(days=1, minutes=4), status='delivered') + n2 = create_notification(template=sample_email_template, + created_at=datetime.utcnow() - timedelta(days=1, minutes=20), status='permanent-failure') + n3 = create_notification(template=sample_email_template, + created_at=datetime.utcnow() - timedelta(days=1, minutes=30), status='temporary-failure') + n4 = create_notification(template=sample_email_template, + created_at=datetime.utcnow() - timedelta(days=1, minutes=59), status='temporary-failure') + n5 = create_notification(template=sample_email_template, + created_at=datetime.utcnow() - timedelta(days=1, hours=1), status='sending') + n6 = create_notification(template=sample_email_template, + created_at=datetime.utcnow() - timedelta(days=1, minutes=61), status='pending') + n7 = create_notification(template=sample_email_template, + created_at=datetime.utcnow() - timedelta(days=1, hours=1, seconds=1), + status='validation-failed') + n8 = create_notification(template=sample_email_template, + created_at=datetime.utcnow() - timedelta(days=1, minutes=20), status='created') # should NOT be deleted - wrong status - create_notification(template=sample_email_template, - created_at=datetime.utcnow() - timedelta(days=1), status='sending') - create_notification(template=sample_email_template, - created_at=datetime.utcnow() - timedelta(days=1), status='technical-failure') - create_notification(template=sample_email_template, - created_at=datetime.utcnow() - timedelta(hours=1), status='created') + n9 = create_notification(template=sample_email_template, + created_at=datetime.utcnow() - timedelta(hours=1), status='delivered') + n10 = create_notification(template=sample_email_template, + created_at=datetime.utcnow() - timedelta(hours=1), status='technical-failure') + n11 = create_notification(template=sample_email_template, + created_at=datetime.utcnow() - timedelta(hours=23, minutes=59), status='created') + ids_to_move = sorted([n1.id, n2.id, n3.id, n4.id, n5.id, n6.id, n7.id, n8.id]) + ids_to_keep = sorted([n9.id, n10.id, n11.id]) del_count = insert_notification_history_delete_notifications( notification_type=sample_email_template.template_type, service_id=sample_email_template.service_id, - timestamp_to_delete_backwards_from=datetime.utcnow() + timedelta(hours=1)) - - assert del_count == 4 + timestamp_to_delete_backwards_from=datetime.utcnow() - timedelta(days=1)) + assert del_count == 8 notifications = Notification.query.all() history_rows = NotificationHistory.query.all() - assert len(history_rows) == 4 - assert len(notifications) == 7 + assert len(history_rows) == 8 + assert ids_to_move == sorted([x.id for x in history_rows]) + assert len(notifications) == 3 + assert ids_to_keep == sorted([x.id for x in notifications]) def test_insert_notification_history_delete_notifications_more_notifications_than_query_limit(sample_template):