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
ebb43082d5/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

@@ -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,

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