From 08c4f3f4d64cf6e57d2e4b172e0194fe2cbf0829 Mon Sep 17 00:00:00 2001 From: David McDonald Date: Tue, 15 Dec 2020 17:29:58 +0000 Subject: [PATCH] Do not delete s3 letters if 'created' or 'sending' A few weeks ago, we deleted some pdf letters that had reached their retention period. However, these letters were in the 'created' state so it's very arguable that we should not have deleted them because we were expecting to resend them and were unable to. Part of the reason for this is that we marked the letters back to `created` as the status but we did not nullify the `sent_at` timestamp, meaning the check on https://github.com/alphagov/notifications-api/blob/ebb43082d51b9f27b17190abb0537a710a544408/app/dao/notifications_dao.py#L346 did not catch it. This commit does three things Firstly, it changes the check from looking at `sent_at` to looking at `notification_status`. Status is a much more reliable way of determinig if the letter should be sent or not, in case we forget to nullify the `sent_at` field in the future. Secondly, it changes it so letters still in `sending` will not be deleted from s3. This is important because this protects against the case were we hand the letter to DVLA, they accept it and it gets marked as `sending` and then the letter is deleted before they come back to us and say there are problems and we need to resend it to them. Thirdly, it improves test coverage for letter deleting for a range of letter statuses. Note, this will change will NOT stop letters in `created` or `sending` from still being removed from the `notifications` table and put in the `notification_history` table. It would be nice to do this too but not as part of this commit. It does mean there could be a case were a letter is still in sending when it gets moved to `notification_history` and we retain the PDF. Then the letter is updated to `delivered`. However at this point, although the letter has reached a final state, nothing will come along and tell s3 to delete that PDF. However, as a backup, there is a 90 day default deletion on letters in the `production-letters-pdf` bucket so it may be kept slightly longer than intended but not forever. This feels like a tolerable risk, that already existed for letters in `created` state` and is likely a better situation than deleting the PDF too early. --- app/dao/notifications_dao.py | 7 +++++-- ...t_notification_dao_delete_notifications.py | 20 +++++++++++++------ 2 files changed, 19 insertions(+), 8 deletions(-) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index aa57779f7..b3c698d71 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -43,6 +43,7 @@ from app.models import ( NOTIFICATION_TEMPORARY_FAILURE, NOTIFICATION_PERMANENT_FAILURE, NOTIFICATION_SENT, + NOTIFICATION_STATUS_TYPES_COMPLETED, SMS_TYPE, EMAIL_TYPE, ServiceDataRetention, @@ -428,8 +429,10 @@ 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 - if letter.sent_at: + # We don't want to delete PDFs for letters until they have transitioned past created and sending, as at that + # point we are confident that our print provider could successfully handle them and there is no chance of us + # needing to resend them + if letter.status in NOTIFICATION_STATUS_TYPES_COMPLETED: prefix = get_letter_pdf_filename(reference=letter.reference, crown=letter.service.crown, created_at=letter.created_at, 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..bda92114d 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 @@ -133,12 +133,17 @@ def test_delete_notifications_for_days_of_retention(sample_service, notification @freeze_time('2019-09-01 04:30') -def test_delete_notifications_deletes_letters_from_s3(sample_letter_template, mocker): +@pytest.mark.parametrize('notification_status', ['delivered', 'returned-letter', 'technical-failure']) +def test_delete_notifications_deletes_letters_from_s3(sample_letter_template, mocker, notification_status): mock_get_s3 = mocker.patch("app.dao.notifications_dao.get_s3_bucket_objects") eight_days_ago = datetime.utcnow() - timedelta(days=8) - create_notification(template=sample_letter_template, status='delivered', - reference='LETTER_REF', created_at=eight_days_ago, sent_at=eight_days_ago - ) + create_notification( + template=sample_letter_template, + status=notification_status, + reference='LETTER_REF', + created_at=eight_days_ago, + sent_at=eight_days_ago + ) delete_notifications_older_than_retention_by_type(notification_type='letter') 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( @@ -212,13 +217,16 @@ 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', 'sending']) +def test_delete_notifications_doesnt_try_to_delete_from_s3_when_letter_has_not_finished_sending( + 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) )