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

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

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

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