In the effort to reduce the number of database connections I introduced a small bug. This only affected the test templated letter flow, a None type error would happen when trying to creathe completed_at timestamp for a delivered message.

In the previous PR I removed the `update_notification` method to reduce the need for another update query. However, that meant the notification was marked as delivered without an updated_at timestamp.

It is weird to set the updated_at when we create the notification. So is this a better fix? Or do I put the update back now?

I recommend we push this fix now.

app/notifications/process_letter_notifications.py

@@ -5,7 +5,9 @@ from app.models import LETTER_TYPE
 from app.notifications.process_notifications import persist_notification
 
 
-def create_letter_notification(letter_data, template, api_key, status, reply_to_text=None, billable_units=None):
+def create_letter_notification(
+    letter_data, template, api_key, status, reply_to_text=None, billable_units=None, updated_at=None
+):
     notification = persist_notification(
         template_id=template.id,
         template_version=template.version,
@@ -24,6 +26,7 @@ def create_letter_notification(letter_data, template, api_key, status, reply_to_
         status=status,
         reply_to_text=reply_to_text,
         billable_units=billable_units,
-        postage=letter_data.get('postage')
+        postage=letter_data.get('postage'),
+        updated_at=updated_at
     )
     return notification

app/notifications/process_notifications.py

@@ -79,7 +79,8 @@ def persist_notification(
     billable_units=None,
     postage=None,
     template_postage=None,
-    document_download_count=None
+    document_download_count=None,
+    updated_at=None
 ):
     notification_created_at = created_at or datetime.utcnow()
     if not notification_id:
@@ -105,6 +106,7 @@ def persist_notification(
         reply_to_text=reply_to_text,
         billable_units=billable_units,
         document_download_count=document_download_count,
+        updated_at=updated_at
     )
 
     if notification_type == SMS_TYPE:

app/v2/notifications/post_notifications.py

@@ -347,6 +347,7 @@ def process_letter_notification(
     test_key = api_key.key_type == KEY_TYPE_TEST
 
     status = NOTIFICATION_CREATED
+    updated_at = None
     if test_key:
         # if we don't want to actually send the letter, then start it off in SENDING so we don't pick it up
         if current_app.config['NOTIFY_ENVIRONMENT'] in ['preview', 'development']:
@@ -354,6 +355,7 @@ def process_letter_notification(
         # mark test letter as delivered and do not create a fake response later
         else:
             status = NOTIFICATION_DELIVERED
+            updated_at = datetime.utcnow()
 
     queue = QueueNames.CREATE_LETTERS_PDF if not test_key else QueueNames.RESEARCH_MODE
 
@@ -361,7 +363,9 @@ def process_letter_notification(
                                               template=template,
                                               api_key=api_key,
                                               status=status,
-                                              reply_to_text=reply_to_text)
+                                              reply_to_text=reply_to_text,
+                                              updated_at=updated_at
+                                              )
 
     get_pdf_for_templated_letter.apply_async(
         [str(notification.id)],

tests/app/v2/notifications/test_post_letter_notifications.py

@@ -245,6 +245,7 @@ def test_post_letter_notification_with_test_key_creates_pdf_and_sets_status_to_d
     fake_create_letter_task.assert_called_once_with([str(notification.id)], queue='research-mode-tasks')
     assert not fake_create_dvla_response_task.called
     assert notification.status == NOTIFICATION_DELIVERED
+    assert notification.updated_at is not None
 
 
 @pytest.mark.parametrize('env', [