diff --git a/app/celery/nightly_tasks.py b/app/celery/nightly_tasks.py index 5479baff3..0da5ccc66 100644 --- a/app/celery/nightly_tasks.py +++ b/app/celery/nightly_tasks.py @@ -5,6 +5,7 @@ from flask import current_app from notifications_utils.clients.zendesk.zendesk_client import ( NotifySupportTicket, ) +from notifications_utils.timezones import convert_utc_to_bst from sqlalchemy import func from sqlalchemy.exc import SQLAlchemyError @@ -21,7 +22,11 @@ from app.dao.jobs_dao import ( from app.dao.notifications_dao import ( dao_get_notifications_processing_time_stats, dao_timeout_notifications, - delete_notifications_older_than_retention_by_type, + get_service_ids_that_have_notifications_from_before_timestamp, + move_notifications_to_notification_history, +) +from app.dao.service_data_retention_dao import ( + fetch_service_data_retention_for_all_services_by_notification_type, ) from app.models import ( EMAIL_TYPE, @@ -68,45 +73,76 @@ def delete_notifications_older_than_retention(): @notify_celery.task(name="delete-sms-notifications") @cronitor("delete-sms-notifications") def delete_sms_notifications_older_than_retention(): - start = datetime.utcnow() - deleted = delete_notifications_older_than_retention_by_type('sms') - current_app.logger.info( - "Delete {} job started {} finished {} deleted {} sms notifications".format( - 'sms', - start, - datetime.utcnow(), - deleted - ) - ) + _delete_notifications_older_than_retention_by_type('sms') @notify_celery.task(name="delete-email-notifications") @cronitor("delete-email-notifications") def delete_email_notifications_older_than_retention(): - start = datetime.utcnow() - deleted = delete_notifications_older_than_retention_by_type('email') - current_app.logger.info( - "Delete {} job started {} finished {} deleted {} email notifications".format( - 'email', - start, - datetime.utcnow(), - deleted - ) - ) + _delete_notifications_older_than_retention_by_type('email') @notify_celery.task(name="delete-letter-notifications") @cronitor("delete-letter-notifications") def delete_letter_notifications_older_than_retention(): - start = datetime.utcnow() - deleted = delete_notifications_older_than_retention_by_type('letter') - current_app.logger.info( - "Delete {} job started {} finished {} deleted {} letter notifications".format( - 'letter', - start, - datetime.utcnow(), - deleted + _delete_notifications_older_than_retention_by_type('letter') + + +def _delete_notifications_older_than_retention_by_type(notification_type): + flexible_data_retention = fetch_service_data_retention_for_all_services_by_notification_type(notification_type) + + for f in flexible_data_retention: + day_to_delete_backwards_from = get_london_midnight_in_utc( + convert_utc_to_bst(datetime.utcnow()).date() - timedelta(days=f.days_of_retention) ) + + delete_notifications_for_service_and_type.apply_async(queue=QueueNames.REPORTING, kwargs={ + 'service_id': f.service_id, + 'notification_type': notification_type, + 'datetime_to_delete_before': day_to_delete_backwards_from + }) + + seven_days_ago = get_london_midnight_in_utc(convert_utc_to_bst(datetime.utcnow()).date() - timedelta(days=7)) + service_ids_with_data_retention = {x.service_id for x in flexible_data_retention} + + # 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 = get_service_ids_that_have_notifications_from_before_timestamp( + notification_type, + seven_days_ago + ) + + service_ids_to_purge = service_ids_that_have_sent_notifications_recently - service_ids_with_data_retention + + for service_id in service_ids_to_purge: + delete_notifications_for_service_and_type.apply_async(queue=QueueNames.REPORTING, kwargs={ + 'service_id': service_id, + 'notification_type': notification_type, + 'datetime_to_delete_before': seven_days_ago + }) + + current_app.logger.info( + f'delete-notifications-older-than-retention: triggered subtasks for notification_type {notification_type}: ' + f'{len(service_ids_with_data_retention)} services with flexible data retention, ' + f'{len(service_ids_to_purge)} services without flexible data retention' + ) + + +@notify_celery.task(name='delete-notifications-for-service-and-type') +def delete_notifications_for_service_and_type(service_id, notification_type, datetime_to_delete_before): + start = datetime.utcnow() + num_deleted = move_notifications_to_notification_history( + notification_type, + service_id, + datetime_to_delete_before, + ) + end = datetime.utcnow() + current_app.logger.info( + f'delete-notifications-for-service-and-type: ' + f'service: {service_id}, ' + f'notification_type: {notification_type}, ' + f'count deleted: {num_deleted}, ' + f'duration: {(end - start).seconds} seconds' ) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 39cac60cd..eb1561e5b 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -45,13 +45,8 @@ from app.models import ( Notification, NotificationHistory, ProviderDetails, - ServiceDataRetention, -) -from app.utils import ( - escape_special_characters, - get_london_midnight_in_utc, - midnight_n_days_ago, ) +from app.utils import escape_special_characters, midnight_n_days_ago def dao_get_last_date_template_was_used(template_id, service_id): @@ -304,55 +299,6 @@ def _filter_query(query, filter_dict=None): return query -def delete_notifications_older_than_retention_by_type(notification_type, qry_limit=50000): - current_app.logger.info( - 'Deleting {} notifications for services with flexible data retention'.format(notification_type)) - - flexible_data_retention = ServiceDataRetention.query.filter( - ServiceDataRetention.notification_type == notification_type - ).all() - deleted = 0 - for f in flexible_data_retention: - current_app.logger.info( - "Deleting {} notifications for service id: {}".format(notification_type, f.service_id)) - - 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) - - seven_days_ago = get_london_midnight_in_utc(convert_utc_to_bst(datetime.utcnow()).date()) - timedelta(days=7) - service_ids_with_data_retention = {x.service_id for x in flexible_data_retention} - - # 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) - - current_app.logger.info('Finished deleting {} notifications'.format(notification_type)) - - return deleted - - @autocommit def insert_notification_history_delete_notifications( notification_type, service_id, timestamp_to_delete_backwards_from, qry_limit=50000 @@ -431,18 +377,23 @@ def insert_notification_history_delete_notifications( return result -def _move_notifications_to_notification_history(notification_type, service_id, day_to_delete_backwards_from, qry_limit): +def move_notifications_to_notification_history( + notification_type, + service_id, + timestamp_to_delete_backwards_from, + qry_limit=50000 +): deleted = 0 if notification_type == LETTER_TYPE: _delete_letters_from_s3( - notification_type, service_id, day_to_delete_backwards_from, qry_limit + notification_type, service_id, timestamp_to_delete_backwards_from, qry_limit ) delete_count_per_call = 1 while delete_count_per_call > 0: delete_count_per_call = insert_notification_history_delete_notifications( notification_type=notification_type, service_id=service_id, - timestamp_to_delete_backwards_from=day_to_delete_backwards_from, + timestamp_to_delete_backwards_from=timestamp_to_delete_backwards_from, qry_limit=qry_limit ) deleted += delete_count_per_call @@ -451,7 +402,7 @@ def _move_notifications_to_notification_history(notification_type, service_id, d Notification.query.filter( Notification.notification_type == notification_type, Notification.service_id == service_id, - Notification.created_at < day_to_delete_backwards_from, + Notification.created_at < timestamp_to_delete_backwards_from, Notification.key_type == KEY_TYPE_TEST ).delete(synchronize_session=False) db.session.commit() @@ -840,3 +791,15 @@ def _duplicate_update_warning(notification, status): sent_by=notification.sent_by ) ) + + +def get_service_ids_that_have_notifications_from_before_timestamp(notification_type, timestamp): + return { + row.service_id + for row in db.session.query( + Notification.service_id + ).filter( + Notification.notification_type == notification_type, + Notification.created_at < timestamp + ).distinct() + } diff --git a/app/dao/service_data_retention_dao.py b/app/dao/service_data_retention_dao.py index cf6e7116d..c77def87e 100644 --- a/app/dao/service_data_retention_dao.py +++ b/app/dao/service_data_retention_dao.py @@ -50,3 +50,7 @@ def update_service_data_retention(service_data_retention_id, service_id, days_of } ) return updated_count + + +def fetch_service_data_retention_for_all_services_by_notification_type(notification_type): + return ServiceDataRetention.query.filter(ServiceDataRetention.notification_type == notification_type).all() diff --git a/tests/app/celery/test_nightly_tasks.py b/tests/app/celery/test_nightly_tasks.py index ade428b5f..43c1395b3 100644 --- a/tests/app/celery/test_nightly_tasks.py +++ b/tests/app/celery/test_nightly_tasks.py @@ -11,6 +11,7 @@ from notifications_utils.clients.zendesk.zendesk_client import ( from app.celery import nightly_tasks from app.celery.nightly_tasks import ( + _delete_notifications_older_than_retention_by_type, delete_email_notifications_older_than_retention, delete_inbound_sms, delete_letter_notifications_older_than_retention, @@ -143,24 +144,35 @@ def test_remove_csv_files_filters_by_type(mocker, sample_service): def test_delete_sms_notifications_older_than_retention_calls_child_task(notify_api, mocker): - mocked = mocker.patch('app.celery.nightly_tasks.delete_notifications_older_than_retention_by_type') + mocked = mocker.patch('app.celery.nightly_tasks._delete_notifications_older_than_retention_by_type') delete_sms_notifications_older_than_retention() mocked.assert_called_once_with('sms') def test_delete_email_notifications_older_than_retentions_calls_child_task(notify_api, mocker): mocked_notifications = mocker.patch( - 'app.celery.nightly_tasks.delete_notifications_older_than_retention_by_type') + 'app.celery.nightly_tasks._delete_notifications_older_than_retention_by_type') delete_email_notifications_older_than_retention() mocked_notifications.assert_called_once_with('email') def test_delete_letter_notifications_older_than_retention_calls_child_task(notify_api, mocker): - mocked = mocker.patch('app.celery.nightly_tasks.delete_notifications_older_than_retention_by_type') + mocked = mocker.patch('app.celery.nightly_tasks._delete_notifications_older_than_retention_by_type') delete_letter_notifications_older_than_retention() mocked.assert_called_once_with('letter') +def test_should_not_update_status_of_letter_notifications(client, sample_letter_template): + created_at = datetime.utcnow() - timedelta(days=5) + not1 = create_notification(template=sample_letter_template, status='sending', created_at=created_at) + not2 = create_notification(template=sample_letter_template, status='created', created_at=created_at) + + timeout_notifications() + + assert not1.status == 'sending' + assert not2.status == 'created' + + @freeze_time("2021-12-13T10:00") def test_timeout_notifications(mocker, sample_notification): mock_update = mocker.patch('app.celery.nightly_tasks.check_and_queue_callback_task') @@ -427,3 +439,97 @@ def test_save_daily_notification_processing_time_when_in_bst(mocker, sample_temp assert persisted_to_db[0].bst_date == date(2021, 4, 17) assert persisted_to_db[0].messages_total == 2 assert persisted_to_db[0].messages_within_10_secs == 2 + + +@freeze_time('2021-06-05 03:00') +def test_delete_notifications_task_calls_task_for_services_with_data_retention_of_same_type(notify_db_session, mocker): + sms_service = create_service(service_name='a') + email_service = create_service(service_name='b') + letter_service = create_service(service_name='c') + + create_service_data_retention(sms_service, notification_type='sms') + create_service_data_retention(email_service, notification_type='email') + create_service_data_retention(letter_service, notification_type='letter') + + mock_subtask = mocker.patch('app.celery.nightly_tasks.delete_notifications_for_service_and_type') + + _delete_notifications_older_than_retention_by_type('sms') + + mock_subtask.apply_async.assert_called_once_with(queue='reporting-tasks', kwargs={ + 'service_id': sms_service.id, + 'notification_type': 'sms', + # three days of retention, its morn of 5th, so we want to keep all messages from 4th, 3rd and 2nd. + 'datetime_to_delete_before': datetime(2021, 6, 1, 23, 0), + }) + + +@freeze_time('2021-04-05 03:00') +def test_delete_notifications_task_calls_task_for_services_with_data_retention_by_looking_at_retention( + notify_db_session, + mocker +): + service_14_days = create_service(service_name='a') + service_3_days = create_service(service_name='b') + create_service_data_retention(service_14_days, days_of_retention=14) + create_service_data_retention(service_3_days, days_of_retention=3) + + mock_subtask = mocker.patch('app.celery.nightly_tasks.delete_notifications_for_service_and_type') + + _delete_notifications_older_than_retention_by_type('sms') + + assert mock_subtask.apply_async.call_count == 2 + mock_subtask.apply_async.assert_has_calls(any_order=True, calls=[ + call(queue=ANY, kwargs={ + 'service_id': service_14_days.id, + 'notification_type': 'sms', + 'datetime_to_delete_before': datetime(2021, 3, 22, 0, 0) + }), + call(queue=ANY, kwargs={ + 'service_id': service_3_days.id, + 'notification_type': 'sms', + 'datetime_to_delete_before': datetime(2021, 4, 1, 23, 0) + }), + ]) + + +@freeze_time('2021-04-03 03:00') +def test_delete_notifications_task_calls_task_for_services_that_have_sent_notifications_recently( + notify_db_session, + mocker +): + + service_will_delete_1 = create_service(service_name='a') + service_will_delete_2 = create_service(service_name='b') + service_nothing_to_delete = create_service(service_name='c') + + create_template(service_will_delete_1) + create_template(service_will_delete_2) + 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') + + # will be deleted as service has no custom retention, but past our default 7 days + create_notification(service_will_delete_1.templates[0], created_at=datetime.now() - timedelta(days=8)) + create_notification(service_will_delete_2.templates[0], created_at=datetime.now() - timedelta(days=8)) + + # will be kept as it's recent, and we won't run delete_notifications_for_service_and_type + 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 delete_notifications_for_service_and_type + create_notification(nothing_to_delete_email_template, created_at=datetime.now() - timedelta(days=8)) + + mock_subtask = mocker.patch('app.celery.nightly_tasks.delete_notifications_for_service_and_type') + + _delete_notifications_older_than_retention_by_type('sms') + + assert mock_subtask.apply_async.call_count == 2 + mock_subtask.apply_async.assert_has_calls(any_order=True, calls=[ + call(queue=ANY, kwargs={ + 'service_id': service_will_delete_1.id, + 'notification_type': 'sms', + 'datetime_to_delete_before': datetime(2021, 3, 27, 0, 0) + }), + call(queue=ANY, kwargs={ + 'service_id': service_will_delete_2.id, + 'notification_type': 'sms', + 'datetime_to_delete_before': datetime(2021, 3, 27, 0, 0) + }), + ]) 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 db83e31f8..b1f5f09bc 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,5 +1,5 @@ -from datetime import date, datetime, timedelta -from unittest.mock import ANY, call +import uuid +from datetime import datetime, timedelta import boto3 import pytest @@ -8,133 +8,27 @@ from freezegun import freeze_time from moto import mock_s3 from app.dao.notifications_dao import ( - delete_notifications_older_than_retention_by_type, insert_notification_history_delete_notifications, + move_notifications_to_notification_history, +) +from app.models import ( + KEY_TYPE_NORMAL, + KEY_TYPE_TEAM, + KEY_TYPE_TEST, + Notification, + NotificationHistory, ) -from app.models import Notification, NotificationHistory from tests.app.db import ( create_notification, create_notification_history, create_service, - create_service_data_retention, create_template, ) -def create_test_data(notification_type, sample_service, days_of_retention=3): - service_with_default_data_retention = create_service(service_name='default data retention') - email_template, letter_template, sms_template = _create_templates(sample_service) - default_email_template, default_letter_template, default_sms_template = _create_templates( - service_with_default_data_retention) - create_notification(template=email_template, status='delivered') - create_notification(template=sms_template, status='permanent-failure') - create_notification(template=letter_template, status='temporary-failure', - reference='LETTER_REF', created_at=datetime.utcnow(), sent_at=datetime.utcnow()) - create_notification(template=email_template, status='delivered', - created_at=datetime.utcnow() - timedelta(days=4)) - create_notification(template=sms_template, status='permanent-failure', - created_at=datetime.utcnow() - timedelta(days=4)) - create_notification(template=letter_template, status='temporary-failure', - reference='LETTER_REF', sent_at=datetime.utcnow(), - created_at=datetime.utcnow() - timedelta(days=4)) - create_notification(template=default_email_template, status='delivered', - created_at=datetime.utcnow() - timedelta(days=8)) - create_notification(template=default_sms_template, status='permanent-failure', - created_at=datetime.utcnow() - timedelta(days=8)) - create_notification(template=default_letter_template, status='temporary-failure', - reference='LETTER_REF', sent_at=datetime.utcnow(), - created_at=datetime.utcnow() - timedelta(days=8)) - create_service_data_retention(service=sample_service, notification_type=notification_type, - days_of_retention=days_of_retention) - - -def _create_templates(sample_service): - email_template = create_template(service=sample_service, template_type='email') - sms_template = create_template(service=sample_service) - letter_template = create_template(service=sample_service, template_type='letter') - return email_template, letter_template, sms_template - - -@pytest.mark.parametrize('month, delete_run_time', - [(4, '2016-04-10 23:40'), (1, '2016-01-11 00:40')]) -@pytest.mark.parametrize( - 'notification_type, expected_sms_count, expected_email_count, expected_letter_count', - [('sms', 7, 10, 10), - ('email', 10, 7, 10), - ('letter', 10, 10, 7)] -) -def test_should_delete_notifications_by_type_after_seven_days( - sample_service, - mocker, - month, - delete_run_time, - notification_type, - expected_sms_count, - expected_email_count, - expected_letter_count -): - mocker.patch("app.dao.notifications_dao.find_letter_pdf_in_s3") - email_template, letter_template, sms_template = _create_templates(sample_service) - # create one notification a day between 1st and 10th from 11:00 to 19:00 of each type - for i in range(1, 11): - past_date = '2016-0{0}-{1:02d} {1:02d}:00:00.000000'.format(month, i) - with freeze_time(past_date): - create_notification(template=email_template, created_at=datetime.utcnow(), status="permanent-failure") - create_notification(template=sms_template, created_at=datetime.utcnow(), status="delivered") - create_notification(template=letter_template, created_at=datetime.utcnow(), status="temporary-failure") - assert Notification.query.count() == 30 - - # Records from before 3rd should be deleted - with freeze_time(delete_run_time): - delete_notifications_older_than_retention_by_type(notification_type) - - remaining_sms_notifications = Notification.query.filter_by(notification_type='sms').all() - remaining_letter_notifications = Notification.query.filter_by(notification_type='letter').all() - remaining_email_notifications = Notification.query.filter_by(notification_type='email').all() - assert len(remaining_sms_notifications) == expected_sms_count - assert len(remaining_email_notifications) == expected_email_count - assert len(remaining_letter_notifications) == expected_letter_count - - if notification_type == 'sms': - notifications_to_check = remaining_sms_notifications - if notification_type == 'email': - notifications_to_check = remaining_email_notifications - if notification_type == 'letter': - notifications_to_check = remaining_letter_notifications - for notification in notifications_to_check: - assert notification.created_at.date() >= date(2016, month, 3) - - -@freeze_time("2016-01-10 12:00:00.000000") -def test_should_not_delete_notification_history(sample_service, mocker): - with freeze_time('2016-01-01 12:00'): - email_template, letter_template, sms_template = _create_templates(sample_service) - create_notification(template=email_template, status='permanent-failure') - create_notification(template=sms_template, status='permanent-failure') - create_notification(template=letter_template, status='permanent-failure') - assert Notification.query.count() == 3 - delete_notifications_older_than_retention_by_type('sms') - assert Notification.query.count() == 2 - assert NotificationHistory.query.count() == 1 - - -@pytest.mark.parametrize('notification_type', ['sms', 'email', 'letter']) -def test_delete_notifications_for_days_of_retention(sample_service, notification_type, mocker): - mock_s3_object = mocker.patch('app.dao.notifications_dao.find_letter_pdf_in_s3').return_value - create_test_data(notification_type, sample_service) - assert Notification.query.count() == 9 - delete_notifications_older_than_retention_by_type(notification_type) - assert Notification.query.count() == 7 - assert Notification.query.filter_by(notification_type=notification_type).count() == 1 - if notification_type == 'letter': - assert mock_s3_object.delete.call_count == 2 - else: - mock_s3_object.delete.assert_not_called() - - @mock_s3 @freeze_time('2019-09-01 04:30') -def test_delete_notifications_deletes_letters_from_s3(sample_letter_template, mocker): +def test_move_notifications_deletes_letters_from_s3(sample_letter_template, mocker): s3 = boto3.client('s3', region_name='eu-west-1') bucket_name = current_app.config['LETTERS_PDF_BUCKET_NAME'] s3.create_bucket( @@ -151,22 +45,13 @@ def test_delete_notifications_deletes_letters_from_s3(sample_letter_template, mo ) s3.put_object(Bucket=bucket_name, Key=filename, Body=b'foo') - delete_notifications_older_than_retention_by_type(notification_type='letter') + move_notifications_to_notification_history('letter', sample_letter_template.service_id, datetime(2020, 1, 2)) with pytest.raises(s3.exceptions.NoSuchKey): s3.get_object(Bucket=bucket_name, Key=filename) -def test_delete_notifications_inserts_notification_history(sample_service): - create_test_data('sms', sample_service) - assert Notification.query.count() == 9 - delete_notifications_older_than_retention_by_type('sms') - assert Notification.query.count() == 7 - - assert NotificationHistory.query.count() == 2 - - -def test_delete_notifications_does_nothing_if_notification_history_row_already_exists( +def test_move_notifications_does_nothing_if_notification_history_row_already_exists( sample_email_template, mocker ): notification = create_notification( @@ -178,52 +63,18 @@ def test_delete_notifications_does_nothing_if_notification_history_row_already_e created_at=datetime.utcnow() - timedelta(days=8), status='delivered' ) - delete_notifications_older_than_retention_by_type("email") + move_notifications_to_notification_history("email", sample_email_template.service_id, datetime.utcnow(), 1) + assert Notification.query.count() == 0 history = NotificationHistory.query.all() assert len(history) == 1 assert history[0].status == 'delivered' -def test_delete_notifications_keep_data_for_days_of_retention_is_longer(sample_service): - create_test_data('sms', sample_service, 15) - assert Notification.query.count() == 9 - delete_notifications_older_than_retention_by_type('sms') - assert Notification.query.count() == 8 - assert Notification.query.filter(Notification.notification_type == 'sms').count() == 2 - - -def test_delete_notifications_with_test_keys(sample_template, mocker): - create_notification(template=sample_template, key_type='test', created_at=datetime.utcnow() - timedelta(days=8)) - delete_notifications_older_than_retention_by_type('sms') - assert Notification.query.count() == 0 - - -def test_delete_notifications_delete_notification_type_for_default_time_if_no_days_of_retention_for_type( - sample_service -): - create_service_data_retention(service=sample_service, notification_type='sms', - days_of_retention=15) - email_template, letter_template, sms_template = _create_templates(sample_service) - create_notification(template=email_template, status='delivered') - create_notification(template=sms_template, status='permanent-failure') - create_notification(template=letter_template, status='temporary-failure') - create_notification(template=email_template, status='delivered', - created_at=datetime.utcnow() - timedelta(days=14)) - create_notification(template=sms_template, status='permanent-failure', - created_at=datetime.utcnow() - timedelta(days=14)) - create_notification(template=letter_template, status='temporary-failure', - created_at=datetime.utcnow() - timedelta(days=14)) - assert Notification.query.count() == 6 - delete_notifications_older_than_retention_by_type('email') - assert Notification.query.count() == 5 - assert Notification.query.filter_by(notification_type='email').count() == 1 - - @pytest.mark.parametrize( 'notification_status', ['validation-failed', 'virus-scan-failed'] ) -def test_delete_notifications_deletes_letters_not_sent_and_in_final_state_from_table_but_not_s3( +def test_move_notifications_deletes_letters_not_sent_and_in_final_state_from_table_but_not_s3( sample_service, mocker, notification_status ): mock_s3_object = mocker.patch("app.dao.notifications_dao.find_letter_pdf_in_s3").return_value @@ -237,7 +88,7 @@ def test_delete_notifications_deletes_letters_not_sent_and_in_final_state_from_t assert Notification.query.count() == 1 assert NotificationHistory.query.count() == 0 - delete_notifications_older_than_retention_by_type('letter') + move_notifications_to_notification_history('letter', sample_service.id, datetime.utcnow()) assert Notification.query.count() == 0 assert NotificationHistory.query.count() == 1 @@ -247,7 +98,7 @@ def test_delete_notifications_deletes_letters_not_sent_and_in_final_state_from_t @mock_s3 @freeze_time('2020-12-24 04:30') @pytest.mark.parametrize('notification_status', ['delivered', 'returned-letter', 'technical-failure']) -def test_delete_notifications_deletes_letters_sent_and_in_final_state_from_table_and_s3( +def test_move_notifications_deletes_letters_sent_and_in_final_state_from_table_and_s3( sample_service, mocker, notification_status ): bucket_name = current_app.config['LETTERS_PDF_BUCKET_NAME'] @@ -275,7 +126,7 @@ def test_delete_notifications_deletes_letters_sent_and_in_final_state_from_table ) s3.put_object(Bucket=bucket_name, Key=filename, Body=b'foo') - delete_notifications_older_than_retention_by_type('letter') + move_notifications_to_notification_history('letter', sample_service.id, datetime.utcnow()) assert Notification.query.count() == 0 assert NotificationHistory.query.count() == 1 @@ -285,7 +136,7 @@ def test_delete_notifications_deletes_letters_sent_and_in_final_state_from_table @pytest.mark.parametrize('notification_status', ['pending-virus-check', 'created', 'sending']) -def test_delete_notifications_does_not_delete_letters_not_yet_in_final_state( +def test_move_notifications_does_not_delete_letters_not_yet_in_final_state( sample_service, mocker, notification_status ): mock_s3_object = mocker.patch("app.dao.notifications_dao.find_letter_pdf_in_s3").return_value @@ -299,38 +150,103 @@ def test_delete_notifications_does_not_delete_letters_not_yet_in_final_state( assert Notification.query.count() == 1 assert NotificationHistory.query.count() == 0 - delete_notifications_older_than_retention_by_type('letter') + move_notifications_to_notification_history('letter', sample_service.id, datetime.utcnow()) assert Notification.query.count() == 1 assert NotificationHistory.query.count() == 0 mock_s3_object.assert_not_called() -@freeze_time('2020-03-25 00:01') -def test_delete_notifications_calls_subquery_multiple_times(sample_template): - create_notification(template=sample_template, created_at=datetime.now() - timedelta(days=7, minutes=3), - status='delivered') - create_notification(template=sample_template, created_at=datetime.now() - timedelta(days=7, minutes=3), - status='delivered') - create_notification(template=sample_template, created_at=datetime.now() - timedelta(days=7, minutes=3), - status='delivered') +def test_move_notifications_only_moves_notifications_older_than_provided_timestamp(sample_template): + delete_time = datetime(2020, 6, 1, 12) + one_second_before = delete_time - timedelta(seconds=1) + one_second_after = delete_time + timedelta(seconds=1) + old_notification = create_notification(template=sample_template, created_at=one_second_before) + new_notification = create_notification(template=sample_template, created_at=one_second_after) + + # need to take a copy of the ID since the old_notification object will stop being accessible once removed + old_notification_id = old_notification.id + + result = move_notifications_to_notification_history('sms', sample_template.service_id, delete_time) + assert result == 1 + + assert Notification.query.one().id == new_notification.id + assert NotificationHistory.query.one().id == old_notification_id + + +def test_move_notifications_keeps_calling_until_no_more_to_delete_and_then_returns_total_deleted( + mocker +): + mock_insert = mocker.patch( + 'app.dao.notifications_dao.insert_notification_history_delete_notifications', + side_effect=[5, 5, 1, 0] + ) + service_id = uuid.uuid4() + timestamp = datetime(2021, 1, 1) + + result = move_notifications_to_notification_history('sms', service_id, timestamp, qry_limit=5) + assert result == 11 + + mock_insert.asset_called_with( + notification_type='sms', + service_id=service_id, + timestamp_to_delete_backwards_from=timestamp, + qry_limit=5 + ) + assert mock_insert.call_count == 4 + + +def test_move_notifications_only_moves_for_given_notification_type(sample_service): + delete_time = datetime(2020, 6, 1, 12) + one_second_before = delete_time - timedelta(seconds=1) + + sms_template = create_template(sample_service, 'sms') + email_template = create_template(sample_service, 'email') + letter_template = create_template(sample_service, 'letter') + create_notification(sms_template, created_at=one_second_before) + create_notification(email_template, created_at=one_second_before) + create_notification(letter_template, created_at=one_second_before) + + result = move_notifications_to_notification_history('sms', sample_service.id, delete_time) + assert result == 1 + assert {x.notification_type for x in Notification.query} == {'email', 'letter'} + assert NotificationHistory.query.one().notification_type == 'sms' + + +def test_move_notifications_only_moves_for_given_service(notify_db_session): + delete_time = datetime(2020, 6, 1, 12) + one_second_before = delete_time - timedelta(seconds=1) + + service = create_service(service_name='service') + other_service = create_service(service_name='other') + + template = create_template(service, 'sms') + other_template = create_template(other_service, 'sms') + + create_notification(template, created_at=one_second_before) + create_notification(other_template, created_at=one_second_before) + + result = move_notifications_to_notification_history('sms', service.id, delete_time) + assert result == 1 + + assert NotificationHistory.query.one().service_id == service.id + assert Notification.query.one().service_id == other_service.id + + +def test_move_notifications_just_deletes_test_key_notifications(sample_template): + delete_time = datetime(2020, 6, 1, 12) + one_second_before = delete_time - timedelta(seconds=1) + create_notification(template=sample_template, created_at=one_second_before, key_type=KEY_TYPE_NORMAL) + create_notification(template=sample_template, created_at=one_second_before, key_type=KEY_TYPE_TEAM) + create_notification(template=sample_template, created_at=one_second_before, key_type=KEY_TYPE_TEST) + + result = move_notifications_to_notification_history('sms', sample_template.service_id, delete_time) + + assert result == 2 - assert Notification.query.count() == 3 - delete_notifications_older_than_retention_by_type('sms', qry_limit=1) assert Notification.query.count() == 0 - - -def test_delete_notifications_returns_sum_correctly(sample_template): - create_notification(template=sample_template, created_at=datetime.now() - timedelta(days=8), status='delivered') - create_notification(template=sample_template, created_at=datetime.now() - timedelta(days=8), status='delivered') - - s2 = create_service(service_name='s2') - t2 = create_template(s2, template_type='sms') - create_notification(template=t2, created_at=datetime.now() - timedelta(days=8), status='delivered') - create_notification(template=t2, created_at=datetime.now() - timedelta(days=8), status='delivered') - - ret = delete_notifications_older_than_retention_by_type('sms', qry_limit=1) - assert ret == 4 + assert NotificationHistory.query.count() == 2 + assert NotificationHistory.query.filter(NotificationHistory.key_type == KEY_TYPE_TEST).count() == 0 @freeze_time('2020-03-20 14:00') @@ -447,57 +363,3 @@ 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)