If there is an invalid letter that has not been updated to validation-failed because the update-validation-failed-for-templated-letter has not been picked up off the letter-tasks queue and the collate-letter-pdfs-to-be-sent has started.

1. The number of letters that we send to DVLA will be not be correct (see https://github.com/alphagov/notifications-api/blob/20ead82463a9df1ce3a6325d103a00306609f5fb/app/celery/letters_pdf_tasks.py#L136)
This may raise an alert with DVLA when they find we have sent them fewer letter than we have reported.
2. When we get the PDF from S3 we will get a file not found https://github.com/alphagov/notifications-api/blob/20ead82463a9df1ce3a6325d103a00306609f5fb/app/celery/letters_pdf_tasks.py#L244
The error will not prevent the collate task from completing but we will see an alert email for the exception and raise questions.

Although this situation is very unlikely because we have a 15 minute window between the last letter deadline date and the time we kick off the collate task we should still mitigate these issues. I updated the queries to only return letters with billable_units > 0, all valid letters should have at least 1 billable unit.

app/celery/letters_pdf_tasks.py

@@ -4,7 +4,6 @@ from hashlib import sha512
 
 from botocore.exceptions import ClientError as BotoClientError
 from flask import current_app
-from notifications_utils import LETTER_MAX_PAGE_COUNT
 from notifications_utils.letter_timings import LETTER_PROCESSING_DEADLINE
 from notifications_utils.postal_address import PostalAddress
 from notifications_utils.timezones import convert_utc_to_bst
@@ -102,27 +101,30 @@ def get_pdf_for_templated_letter(self, notification_id):
 
 
 @notify_celery.task(bind=True, name="update-billable-units-for-letter", max_retries=15, default_retry_delay=300)
-def update_billable_units_or_validation_failed_for_templated_letter(self, notification_id, page_count,
+def update_billable_units_for_letter(self, notification_id, page_count):
-                                                                    create_fake_letter_response_file=None):
     notification = get_notification_by_id(notification_id, _raise=True)
-    if page_count > LETTER_MAX_PAGE_COUNT:
+
-        notification.status = NOTIFICATION_VALIDATION_FAILED
+    billable_units = get_billable_units_for_letter_page_count(page_count)
+
+    if notification.key_type != KEY_TYPE_TEST:
+        notification.billable_units = billable_units
         dao_update_notification(notification)
-        current_app.logger.info(f"Letter notification id: {notification_id} reference {notification.reference}: "
+
-                                f"validation failed: letter is too long {page_count}")
+        current_app.logger.info(
-    else:
+            f"Letter notification id: {notification_id} reference {notification.reference}: "
-        if notification.key_type != KEY_TYPE_TEST:
+            f"billable units set to {billable_units}"
-            notification.billable_units = get_billable_units_for_letter_page_count(page_count)
+        )
-            dao_update_notification(notification)
+
-            current_app.logger.info(f"Letter notification id: {notification_id} reference {notification.reference}: "
+
-                                    f"billable units set to {notification.billable_units}")
+@notify_celery.task(
-        if notification.key_type == KEY_TYPE_TEST \
+    bind=True, name="update-validation-failed-for-templated-letter", max_retries=15, default_retry_delay=300
-                and current_app.config['NOTIFY_ENVIRONMENT'] in ['preview', 'development']:
+)
-            # For test letters send a fake letter response
+def update_validation_failed_for_templated_letter(self, notification_id, page_count):
-            create_fake_letter_response_file.apply_async(
+    notification = get_notification_by_id(notification_id, _raise=True)
-                (notification.reference,),
+    notification.status = NOTIFICATION_VALIDATION_FAILED
-                queue=QueueNames.RESEARCH_MODE
+    dao_update_notification(notification)
-            )
+    current_app.logger.info(f"Letter notification id: {notification_id} reference {notification.reference}: "
+                            f"validation failed: letter is too long {page_count}")
 
 
 @notify_celery.task(name='collate-letter-pdfs-to-be-sent')

app/dao/notifications_dao.py

@@ -711,7 +711,8 @@ def dao_get_letters_to_be_printed(print_run_deadline, postage, query_limit=10000
         Notification.notification_type == LETTER_TYPE,
         Notification.status == NOTIFICATION_CREATED,
         Notification.key_type == KEY_TYPE_NORMAL,
-        Notification.postage == postage
+        Notification.postage == postage,
+        Notification.billable_units > 0
     ).order_by(
         Notification.service_id,
         Notification.created_at
@@ -729,6 +730,7 @@ def dao_get_letters_and_sheets_volume_by_postage(print_run_deadline):
         Notification.notification_type == LETTER_TYPE,
         Notification.status == NOTIFICATION_CREATED,
         Notification.key_type == KEY_TYPE_NORMAL,
+        Notification.billable_units > 0
     ).group_by(
         Notification.postage
     ).order_by(

app/v2/notifications/post_notifications.py

@@ -19,6 +19,7 @@ from app.celery.letters_pdf_tasks import (
     get_pdf_for_templated_letter,
     sanitise_letter,
 )
+from app.celery.research_mode_tasks import create_fake_letter_response_file
 from app.celery.tasks import save_api_email, save_api_sms
 from app.clients.document_download import DocumentDownloadError
 from app.config import QueueNames, TaskNames
@@ -388,6 +389,12 @@ def process_letter_notification(
         queue=queue
     )
 
+    if test_key and current_app.config['NOTIFY_ENVIRONMENT'] in ['preview', 'development']:
+        create_fake_letter_response_file.apply_async(
+            (notification.reference,),
+            queue=queue
+        )
+
     resp = create_response_for_post_notification(
         notification_id=notification.id,
         client_reference=notification.client_reference,

tests/app/celery/test_letters_pdf_tasks.py

@@ -26,7 +26,8 @@ from app.celery.letters_pdf_tasks import (
     resanitise_pdf,
     sanitise_letter,
     send_letters_volume_email_to_dvla,
-    update_billable_units_or_validation_failed_for_templated_letter,
+    update_billable_units_for_letter,
+    update_validation_failed_for_templated_letter,
 )
 from app.config import QueueNames, TaskNames
 from app.dao.notifications_dao import get_notifications
@@ -145,32 +146,31 @@ def test_get_pdf_for_templated_letter_sets_technical_failure_max_retries(mocker,
     mock_update_noti.assert_called_once_with(sample_letter_notification.id, 'technical-failure')
 
 
-@pytest.mark.parametrize('number_of_pages, expected_billable_units', [(2, 1), (3, 2), (10, 5)])
+def test_update_billable_units_for_letter(
-def test_update_billable_units_or_validation_failed_for_templated_letter(
+        mocker, sample_letter_notification
-        mocker, sample_letter_notification, number_of_pages, expected_billable_units
 ):
     sample_letter_notification.billable_units = 0
     mock_logger = mocker.patch('app.celery.tasks.current_app.logger.info')
 
-    update_billable_units_or_validation_failed_for_templated_letter(sample_letter_notification.id, number_of_pages)
+    update_billable_units_for_letter(sample_letter_notification.id, 4)
 
     notification = Notification.query.filter(Notification.reference == sample_letter_notification.reference).one()
-    assert notification.billable_units == expected_billable_units
+    assert notification.billable_units == 2
     assert sample_letter_notification.status == NOTIFICATION_CREATED
     mock_logger.assert_called_once_with(
         f"Letter notification id: {sample_letter_notification.id} reference {sample_letter_notification.reference}:"
-        f" billable units set to {expected_billable_units}"
+        f" billable units set to 2"
     )
 
 
-def test_update_billable_units_or_validation_failed_for_templated_letter_doesnt_update_if_sent_with_test_key(
+def test_update_billable_units_for_letter_doesnt_update_if_sent_with_test_key(
         mocker, sample_letter_notification
 ):
     sample_letter_notification.billable_units = 0
     sample_letter_notification.key_type = KEY_TYPE_TEST
     mock_logger = mocker.patch('app.celery.tasks.current_app.logger.info')
 
-    update_billable_units_or_validation_failed_for_templated_letter(sample_letter_notification.id, 2)
+    update_billable_units_for_letter(sample_letter_notification.id, 2)
 
     notification = Notification.query.filter(Notification.reference == sample_letter_notification.reference).one()
     assert notification.billable_units == 0
@@ -178,14 +178,14 @@ def test_update_billable_units_or_validation_failed_for_templated_letter_doesnt_
 
 
 @pytest.mark.parametrize('key_type', ['test', 'normal', 'team'])
-def test_update_billable_units_or_validation_failed_for_templated_letter_with_too_many_pages(
+def test_update_validation_failed_for_templated_letter_with_too_many_pages(
         mocker, sample_letter_notification, key_type
 ):
     sample_letter_notification.billable_units = 0
     sample_letter_notification.key_type = key_type
     mock_logger = mocker.patch('app.celery.tasks.current_app.logger.info')
 
-    update_billable_units_or_validation_failed_for_templated_letter(sample_letter_notification.id, 11)
+    update_validation_failed_for_templated_letter(sample_letter_notification.id, 11)
 
     assert sample_letter_notification.billable_units == 0
     assert sample_letter_notification.status == NOTIFICATION_VALIDATION_FAILED

tests/app/dao/notification_dao/test_notification_dao.py

@@ -1698,20 +1698,35 @@ def test_letters_to_be_printed_sort_by_service(notify_db_session):
     assert [x.id for x in results] == [x.id for x in letters_ordered_by_service_then_time]
 
 
+def test_letters_to_be_printed_does_not_include_letters_without_billable_units_set(
+        notify_db_session, sample_letter_template):
+    included_letter = create_notification(
+        template=sample_letter_template, created_at=datetime(2020, 12, 1, 9, 30), billable_units=3)
+    create_notification(
+        template=sample_letter_template, created_at=datetime(2020, 12, 1, 9, 31), billable_units=0)
+
+    results = list(
+        dao_get_letters_to_be_printed(print_run_deadline=datetime(2020, 12, 1, 17, 30), postage='second', query_limit=4)
+    )
+    assert len(results) == 1
+    assert results[0].id == included_letter.id
+
+
 def test_dao_get_letters_and_sheets_volume_by_postage(notify_db_session):
     first_service = create_service(service_name='first service', service_id='3a5cea08-29fd-4bb9-b582-8dedd928b149')
     second_service = create_service(service_name='second service', service_id='642bf33b-54b5-45f2-8c13-942a46616704')
     first_template = create_template(service=first_service, template_type='letter', postage='second')
     second_template = create_template(service=second_service, template_type='letter', postage='second')
-    create_notification(template=first_template, created_at=datetime(2020, 12, 1, 9, 30), postage='first'),
+    create_notification(template=first_template, created_at=datetime(2020, 12, 1, 9, 30), postage='first')
-    create_notification(template=first_template, created_at=datetime(2020, 12, 1, 12, 30), postage='europe'),
+    create_notification(template=first_template, created_at=datetime(2020, 12, 1, 12, 30), postage='europe')
-    create_notification(template=first_template, created_at=datetime(2020, 12, 1, 13, 30), postage='rest-of-world'),
+    create_notification(template=first_template, created_at=datetime(2020, 12, 1, 13, 30), postage='rest-of-world')
-    create_notification(template=first_template, created_at=datetime(2020, 12, 1, 14, 30), billable_units=3),
+    create_notification(template=first_template, created_at=datetime(2020, 12, 1, 14, 30), billable_units=3)
-    create_notification(template=first_template, created_at=datetime(2020, 12, 1, 15, 30)),
+    create_notification(template=first_template, created_at=datetime(2020, 12, 1, 14, 30), billable_units=0)
-    create_notification(template=second_template, created_at=datetime(2020, 12, 1, 8, 30), postage='first'),
+    create_notification(template=first_template, created_at=datetime(2020, 12, 1, 15, 30))
-    create_notification(template=second_template, created_at=datetime(2020, 12, 1, 8, 31), postage='first'),
+    create_notification(template=second_template, created_at=datetime(2020, 12, 1, 8, 30), postage='first')
-    create_notification(template=second_template, created_at=datetime(2020, 12, 1, 8, 32)),
+    create_notification(template=second_template, created_at=datetime(2020, 12, 1, 8, 31), postage='first')
-    create_notification(template=second_template, created_at=datetime(2020, 12, 1, 8, 33)),
+    create_notification(template=second_template, created_at=datetime(2020, 12, 1, 8, 32))
+    create_notification(template=second_template, created_at=datetime(2020, 12, 1, 8, 33))
     create_notification(template=second_template, created_at=datetime(2020, 12, 1, 8, 34))
 
     results = dao_get_letters_and_sheets_volume_by_postage(print_run_deadline=datetime(2020, 12, 1, 17, 30))

tests/app/letters/test_letter_utils.py

@@ -12,6 +12,7 @@ from app.letters.utils import (
     ScanErrorType,
     find_letter_pdf_in_s3,
     generate_letter_pdf_filename,
+    get_billable_units_for_letter_page_count,
     get_bucket_name_and_prefix_for_notification,
     get_folder_name,
     get_letter_pdf_and_metadata,
@@ -433,3 +434,9 @@ def test_letter_print_day_returns_today_if_letter_was_printed_today():
 @freeze_time('2017-07-07 16:30:00')
 def test_letter_print_day_returns_formatted_date_if_letter_printed_before_1730_yesterday(created_at, formatted_date):
     assert letter_print_day(created_at) == formatted_date
+
+
+@pytest.mark.parametrize('number_of_pages, expected_billable_units', [(2, 1), (3, 2), (10, 5)])
+def test_get_billable_units_for_letter_page_count(number_of_pages, expected_billable_units):
+    result = get_billable_units_for_letter_page_count(number_of_pages)
+    assert result == expected_billable_units

tests/app/v2/notifications/test_post_letter_notifications.py

@@ -296,7 +296,7 @@ def test_post_letter_notification_with_test_key_creates_pdf_and_sets_status_to_d
     'development',
     'preview',
 ])
-def test_post_letter_notification_with_test_key_creates_pdf(
+def test_post_letter_notification_with_test_key_creates_pdf_and_sets_status_to_sending_and_sends_fake_response_file(
         notify_api, client, sample_letter_template, mocker, env):
 
     data = {
@@ -312,7 +312,8 @@ def test_post_letter_notification_with_test_key_creates_pdf(
     }
 
     fake_create_letter_task = mocker.patch('app.celery.letters_pdf_tasks.get_pdf_for_templated_letter.apply_async')
-
+    fake_create_dvla_response_task = mocker.patch(
+        'app.celery.research_mode_tasks.create_fake_letter_response_file.apply_async')
     with set_config_values(notify_api, {
         'NOTIFY_ENVIRONMENT': env
     }):
@@ -321,6 +322,7 @@ def test_post_letter_notification_with_test_key_creates_pdf(
     notification = Notification.query.one()
 
     fake_create_letter_task.assert_called_once_with([str(notification.id)], queue='research-mode-tasks')
+    assert fake_create_dvla_response_task.called
     assert notification.status == NOTIFICATION_SENDING