darkhelm/notifications-api
1
0
Fork 0
mirror of https://github.com/GSA/notifications-api.git synced 2026-02-03 01:41:05 -05:00
Code Issues Packages Projects Releases Wiki Activity

Change as per code review

Browse Source 
- 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
This commit is contained in:
Rebecca Law
2020-03-24 14:09:13 +00:00
parent f04210ba7d
commit 3ca73a35ed
2 changed files with 13 additions and 10 deletions
Download Patch File Download Diff File Expand all files Collapse all files

11
app/dao/notifications_dao.py
View File

@@ -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( day_to_delete_backwards_from = get_london_midnight_in_utc(
convert_utc_to_bst(datetime.utcnow()).date()) - timedelta(days=f.days_of_retention) convert_utc_to_bst(datetime.utcnow()).date()) - timedelta(days=f.days_of_retention)
delete_count_per_call = 1
deleted += _move_notifications_to_notification_history( while delete_count_per_call > 0:
notification_type, f.service_id, day_to_delete_backwards_from, qry_limit) 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( current_app.logger.info(
'Deleting {} notifications for services without flexible data retention'.format(notification_type)) 'Deleting {} notifications for services without flexible data retention'.format(notification_type))
@@ -350,7 +352,7 @@ def insert_notification_history_delete_notifications(
FROM notifications FROM notifications
WHERE service_id = :service_id WHERE service_id = :service_id
AND notification_type = :notification_type 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 key_type = 'normal'
AND notification_status in ('delivered', 'permanent-failure', 'temporary-failure') AND notification_status in ('delivered', 'permanent-failure', 'temporary-failure')
limit :qry_limit limit :qry_limit
@@ -415,6 +417,7 @@ def _move_notifications_to_notification_history(notification_type, service_id, d
qry_limit=qry_limit qry_limit=qry_limit
) )
# Deleting test Notifications, test notifications are not persisted to NotificationHistory
Notification.query.filter( Notification.query.filter(
Notification.notification_type == notification_type, Notification.notification_type == notification_type,
Notification.service_id == service_id, Notification.service_id == service_id,

12
tests/app/dao/notification_dao/test_notification_dao_delete_notifications.py
View File

@@ -156,7 +156,7 @@ def test_delete_notifications_inserts_notification_history(sample_service):
assert NotificationHistory.query.count() == 2 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 sample_email_template, mocker
): ):
mocker.patch("app.dao.notifications_dao.get_s3_bucket_objects") 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') created_at=datetime.utcnow() + timedelta(minutes=30), status='temporary-failure')
create_notification(template=sample_email_template, create_notification(template=sample_email_template,
created_at=datetime.utcnow() + timedelta(minutes=59), status='temporary-failure') 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 # 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, create_notification(template=sample_email_template,
created_at=datetime.utcnow() + timedelta(minutes=61), status='temporary-failure') created_at=datetime.utcnow() + timedelta(minutes=61), status='temporary-failure')
create_notification(template=sample_email_template, 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, 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(hours=1))
assert del_count == 5 assert del_count == 4
notifications = Notification.query.all() notifications = Notification.query.all()
history_rows = NotificationHistory.query.all() history_rows = NotificationHistory.query.all()
assert len(history_rows) == 5 assert len(history_rows) == 4
assert len(notifications) == 6 assert len(notifications) == 7
def test_insert_notification_history_delete_notifications_more_notifications_than_query_limit(sample_template): def test_insert_notification_history_delete_notifications_more_notifications_than_query_limit(sample_template):