From 3ca73a35edbd58f6c6f4c53de0336aa612e83df1 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Tue, 24 Mar 2020 14:09:13 +0000 Subject: [PATCH] Change as per code review - fix test name - make query filter consistent with each other - add comment for clarity - add inner loop to continue to insert and delete notifications while the delete count is greater than 0 --- app/dao/notifications_dao.py | 11 +++++++---- .../test_notification_dao_delete_notifications.py | 12 ++++++------ 2 files changed, 13 insertions(+), 10 deletions(-) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index b82f5da2e..1a6e1ad3d 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -311,9 +311,11 @@ def delete_notifications_older_than_retention_by_type(notification_type, qry_lim day_to_delete_backwards_from = get_london_midnight_in_utc( convert_utc_to_bst(datetime.utcnow()).date()) - timedelta(days=f.days_of_retention) - - deleted += _move_notifications_to_notification_history( - notification_type, f.service_id, day_to_delete_backwards_from, qry_limit) + delete_count_per_call = 1 + while delete_count_per_call > 0: + delete_count_per_call = _move_notifications_to_notification_history( + notification_type, f.service_id, day_to_delete_backwards_from, qry_limit) + deleted += delete_count_per_call current_app.logger.info( 'Deleting {} notifications for services without flexible data retention'.format(notification_type)) @@ -350,7 +352,7 @@ def insert_notification_history_delete_notifications( FROM notifications WHERE service_id = :service_id AND notification_type = :notification_type - AND created_at <= :timestamp_to_delete_backwards_from + AND created_at < :timestamp_to_delete_backwards_from AND key_type = 'normal' AND notification_status in ('delivered', 'permanent-failure', 'temporary-failure') limit :qry_limit @@ -415,6 +417,7 @@ def _move_notifications_to_notification_history(notification_type, service_id, d qry_limit=qry_limit ) + # Deleting test Notifications, test notifications are not persisted to NotificationHistory Notification.query.filter( Notification.notification_type == notification_type, Notification.service_id == service_id, 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 4c02ff548..78e559e96 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 @@ -156,7 +156,7 @@ def test_delete_notifications_inserts_notification_history(sample_service): assert NotificationHistory.query.count() == 2 -def test_delete_notifications_does_nothing_if_notification_history_if_row_already_exists( +def test_delete_notifications_does_nothing_if_notification_history_row_already_exists( sample_email_template, mocker ): mocker.patch("app.dao.notifications_dao.get_s3_bucket_objects") @@ -256,10 +256,10 @@ def test_insert_notification_history_delete_notifications(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') - create_notification(template=sample_email_template, - created_at=datetime.utcnow() + timedelta(hours=1), status='delivered') # 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, @@ -279,11 +279,11 @@ def test_insert_notification_history_delete_notifications(sample_email_template) service_id=sample_email_template.service_id, timestamp_to_delete_backwards_from=datetime.utcnow() + timedelta(hours=1)) - assert del_count == 5 + assert del_count == 4 notifications = Notification.query.all() history_rows = NotificationHistory.query.all() - assert len(history_rows) == 5 - assert len(notifications) == 6 + assert len(history_rows) == 4 + assert len(notifications) == 7 def test_insert_notification_history_delete_notifications_more_notifications_than_query_limit(sample_template):