From 1bf9b29905c8eafce5b67d1c5829e6ddd603c9d9 Mon Sep 17 00:00:00 2001 From: David McDonald Date: Wed, 16 Dec 2020 10:39:31 +0000 Subject: [PATCH] Document behaviour of s3 letter deleting The behaviour was a bit of opaque so I have added tests around it so it's clear what it is doing and why. No functionality has changed --- app/dao/notifications_dao.py | 4 +- ...t_notification_dao_delete_notifications.py | 48 +++++++++++++++++-- tests/app/db.py | 2 +- 3 files changed, 48 insertions(+), 6 deletions(-) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index aa57779f7..ba50aea25 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -428,7 +428,9 @@ def _delete_letters_from_s3( ).limit(query_limit).all() for letter in letters_to_delete_from_s3: bucket_name = current_app.config['LETTERS_PDF_BUCKET_NAME'] - # I don't think we need this anymore, we should update the query to get letters sent 7 days ago + # although letters without a `sent_at` timestamp do have PDFs, they do not exist in the + # production-letters-pdf bucket as they never made it that far so we do not try and delete + # them from it if letter.sent_at: prefix = get_letter_pdf_filename(reference=letter.reference, crown=letter.service.crown, 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 8d4a2fb71..9ab5872e0 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 @@ -212,20 +212,60 @@ def test_delete_notifications_delete_notification_type_for_default_time_if_no_da assert Notification.query.filter_by(notification_type='email').count() == 1 -def test_delete_notifications_doesnt_try_to_delete_from_s3_when_letter_has_not_sent(sample_service, mocker): +@pytest.mark.parametrize( + 'notification_status', ['created', 'validation-failed', 'virus-scan-failed', 'pending-virus-check'] +) +def test_delete_notifications_deletes_letters_not_sent_from_table_but_not_s3( + sample_service, mocker, notification_status +): mock_get_s3 = mocker.patch("app.dao.notifications_dao.get_s3_bucket_objects") letter_template = create_template(service=sample_service, template_type='letter') - create_notification( template=letter_template, - status='created', + status=notification_status, reference='LETTER_REF', created_at=datetime.utcnow() - timedelta(days=14) ) - delete_notifications_older_than_retention_by_type('letter', qry_limit=1) + assert Notification.query.count() == 1 + assert NotificationHistory.query.count() == 0 + + delete_notifications_older_than_retention_by_type('letter') + + assert Notification.query.count() == 0 + assert NotificationHistory.query.count() == 1 mock_get_s3.assert_not_called() +@pytest.mark.parametrize('notification_status', ['sending', 'delivered', 'returned-letter', 'technical-failure']) +def test_delete_notifications_deletes_letters_sent_from_table_and_s3( + sample_service, mocker, notification_status +): + mock_get_s3 = mocker.patch("app.dao.notifications_dao.get_s3_bucket_objects") + letter_template = create_template(service=sample_service, template_type='letter') + eight_days_ago = datetime.utcnow() - timedelta(days=8) + create_notification( + template=letter_template, + status=notification_status, + reference='LETTER_REF', + created_at=eight_days_ago, + sent_at=eight_days_ago + ) + assert Notification.query.count() == 1 + assert NotificationHistory.query.count() == 0 + + delete_notifications_older_than_retention_by_type('letter') + + assert Notification.query.count() == 0 + assert NotificationHistory.query.count() == 1 + mock_get_s3.assert_called_once_with( + bucket_name=current_app.config['LETTERS_PDF_BUCKET_NAME'], + subfolder="{}/NOTIFY.LETTER_REF.D.2.C.C.{}.PDF".format( + str(eight_days_ago.date()), + eight_days_ago.strftime('%Y%m%d%H%M%S') + ) + ) + + @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), diff --git a/tests/app/db.py b/tests/app/db.py index 8208fee49..336c0f50b 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -256,7 +256,7 @@ def create_notification( if to_field is None: to_field = '+447700900855' if template.template_type == SMS_TYPE else 'test@example.com' - if status != 'created': + if status not in ('created', 'validation-failed', 'virus-scan-failed', 'pending-virus-check'): sent_at = sent_at or datetime.utcnow() updated_at = updated_at or datetime.utcnow()