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) )