Merge pull request #2774 from alphagov/fix-report-and-delete-tasks

Fix report and delete tasks

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}:
+        if notification.status not in [NOTIFICATION_SENDING, NOTIFICATION_PENDING]:
-            notifications_dao._duplicate_update_warning(notification, notification_status)
+            notifications_dao._duplicate_update_warning(
-            return
+                notification=notification,
-
+                status=notification_status
-        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']
-                )
             )
+            return
         else:
-            current_app.logger.info('SES callback return status of {} for notification: {}'.format(
+            notifications_dao.dao_update_notifications_by_reference(
-                notification_status, notification.id
+                references=[reference],
-            ))
+                update_dict={'status': notification_status}
+            )
 
         statsd_client.incr('callback.ses.{}'.format(notification_status))
 

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(

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

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(

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
 
 

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",

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,
+    n1 = create_notification(template=sample_email_template,
-                        created_at=datetime.utcnow() + timedelta(minutes=4), status='delivered')
+                             created_at=datetime.utcnow() - timedelta(days=1, minutes=4), status='delivered')
-    create_notification(template=sample_email_template,
+    n2 = create_notification(template=sample_email_template,
-                        created_at=datetime.utcnow() + timedelta(minutes=20), status='permanent-failure')
+                             created_at=datetime.utcnow() - timedelta(days=1, minutes=20), status='permanent-failure')
-    create_notification(template=sample_email_template,
+    n3 = create_notification(template=sample_email_template,
-                        created_at=datetime.utcnow() + timedelta(minutes=30), status='temporary-failure')
+                             created_at=datetime.utcnow() - timedelta(days=1, minutes=30), status='temporary-failure')
-    create_notification(template=sample_email_template,
+    n4 = create_notification(template=sample_email_template,
-                        created_at=datetime.utcnow() + timedelta(minutes=59), status='temporary-failure')
+                             created_at=datetime.utcnow() - timedelta(days=1, minutes=59), status='temporary-failure')
-
+    n5 = create_notification(template=sample_email_template,
-    # should NOT be deleted
+                             created_at=datetime.utcnow() - timedelta(days=1, hours=1), status='sending')
-    create_notification(template=sample_email_template,
+    n6 = create_notification(template=sample_email_template,
-                        created_at=datetime.utcnow() + timedelta(hours=1), status='delivered')
+                             created_at=datetime.utcnow() - timedelta(days=1, minutes=61), status='pending')
-    create_notification(template=sample_email_template,
+    n7 = create_notification(template=sample_email_template,
-                        created_at=datetime.utcnow() + timedelta(minutes=61), status='temporary-failure')
+                             created_at=datetime.utcnow() - timedelta(days=1, hours=1, seconds=1),
-    create_notification(template=sample_email_template,
+                             status='validation-failed')
-                        created_at=datetime.utcnow() + timedelta(hours=1, seconds=1), status='temporary-failure')
+    n8 = create_notification(template=sample_email_template,
-    create_notification(template=sample_email_template,
+                             created_at=datetime.utcnow() - timedelta(days=1, minutes=20), status='created')
-                        created_at=datetime.utcnow() + timedelta(minutes=20), status='created')
     # should NOT be deleted - wrong status
-    create_notification(template=sample_email_template,
+    n9 = create_notification(template=sample_email_template,
-                        created_at=datetime.utcnow() - timedelta(days=1), status='sending')
+                             created_at=datetime.utcnow() - timedelta(hours=1), status='delivered')
-    create_notification(template=sample_email_template,
+    n10 = create_notification(template=sample_email_template,
-                        created_at=datetime.utcnow() - timedelta(days=1), status='technical-failure')
+                              created_at=datetime.utcnow() - timedelta(hours=1), status='technical-failure')
-    create_notification(template=sample_email_template,
+    n11 = create_notification(template=sample_email_template,
-                        created_at=datetime.utcnow() - timedelta(hours=1), status='created')
+                              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))
+        timestamp_to_delete_backwards_from=datetime.utcnow() - timedelta(days=1))
-
+    assert del_count == 8
-    assert del_count == 4
     notifications = Notification.query.all()
     history_rows = NotificationHistory.query.all()
-    assert len(history_rows) == 4
+    assert len(history_rows) == 8
-    assert len(notifications) == 7
+    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):