diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index b7047c989..85949dee9 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -47,7 +47,6 @@ from app.models import ( Notification, NotificationHistory, ProviderDetails, - Service, ServiceDataRetention, ) from app.utils import ( @@ -323,16 +322,31 @@ def delete_notifications_older_than_retention_by_type(notification_type, qry_lim deleted += _move_notifications_to_notification_history( notification_type, f.service_id, day_to_delete_backwards_from, qry_limit) - current_app.logger.info( - 'Deleting {} notifications for services without flexible data retention'.format(notification_type)) - seven_days_ago = get_london_midnight_in_utc(convert_utc_to_bst(datetime.utcnow()).date()) - timedelta(days=7) - services_with_data_retention = [x.service_id for x in flexible_data_retention] - service_ids_to_purge = Service.query.filter(Service.id.notin_(services_with_data_retention)).all() + service_ids_with_data_retention = {x.service_id for x in flexible_data_retention} - for service in service_ids_to_purge: + # get a list of all service ids that we'll need to delete for. Typically that might only be 5% of services. + # This query takes a couple of mins to run. + service_ids_that_have_sent_notifications_recently = { + row.service_id + for row in db.session.query( + Notification.service_id + ).filter( + Notification.notification_type == notification_type, + Notification.created_at < seven_days_ago + ).distinct() + } + + service_ids_to_purge = service_ids_that_have_sent_notifications_recently - service_ids_with_data_retention + + current_app.logger.info('Deleting {} notifications for {} services without flexible data retention'.format( + notification_type, + len(service_ids_to_purge) + )) + + for service_id in service_ids_to_purge: deleted += _move_notifications_to_notification_history( - notification_type, service.id, seven_days_ago, qry_limit) + notification_type, service_id, seven_days_ago, qry_limit) current_app.logger.info('Finished deleting {} notifications'.format(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 808b15386..db83e31f8 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 @@ -1,4 +1,5 @@ from datetime import date, datetime, timedelta +from unittest.mock import ANY, call import boto3 import pytest @@ -446,3 +447,57 @@ def test_insert_notification_history_delete_notifications_insert_for_key_type(sa assert len(notifications) == 1 assert with_test_key.id == notifications[0].id assert len(history_rows) == 2 + + +@freeze_time('2020-12-10') +def test_delete_notifications_only_runs_move_notifications_for_services_that_have_things_to_delete(mocker): + move_notifications_mock = mocker.patch( + 'app.dao.notifications_dao._move_notifications_to_notification_history', + return_value=0 + ) + + service_retention_will_delete = create_service(service_name='a') + service_retention_nothing_to_delete = create_service(service_name='b') + service_will_delete = create_service(service_name='c') + service_nothing_to_delete = create_service(service_name='d') + + create_service_data_retention( + service_retention_will_delete, + notification_type='sms', + days_of_retention=3 + ) + create_service_data_retention( + service_retention_nothing_to_delete, + notification_type='sms', + days_of_retention=3 + ) + + create_template(service_retention_will_delete) + create_template(service_retention_nothing_to_delete) + create_template(service_will_delete) + nothing_to_delete_sms_template = create_template(service_nothing_to_delete, template_type='sms') + nothing_to_delete_email_template = create_template(service_nothing_to_delete, template_type='email') + + # this notification will be deleted because it's past retention + create_notification(service_retention_will_delete.templates[0], created_at=datetime.now() - timedelta(days=8)) + + # will be deleted as service has no custom retention, but past our default 7 days + create_notification(service_will_delete.templates[0], created_at=datetime.now() - timedelta(days=8)) + + # will be kept as it's recent, but we'll still run _move_notifications_to_notification_history + # as it's for a service with custom retention + create_notification(service_retention_nothing_to_delete.templates[0], created_at=datetime.now() - timedelta(days=2)) + + # will be kept as it's recent, and we won't run _move_notifications_to_notification_history + create_notification(nothing_to_delete_sms_template, created_at=datetime.now() - timedelta(days=2)) + # this is an old notification, but for email not sms, so we won't run _move_notifications_to_notification_history + create_notification(nothing_to_delete_email_template, created_at=datetime.now() - timedelta(days=2)) + + delete_notifications_older_than_retention_by_type('sms') + + # called for all services except for "service_nothing_to_delete" + move_notifications_mock.assert_has_calls([ + call('sms', service_retention_will_delete.id, ANY, ANY), + call('sms', service_retention_nothing_to_delete.id, ANY, ANY), + call('sms', service_will_delete.id, ANY, ANY), + ], any_order=True)