From 6cd7a23d3cc53de2944b20b96a8c005bdb09dab4 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Fri, 7 Jan 2022 09:15:21 +0000 Subject: [PATCH] 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 | 42 ++++++++++--------- app/dao/notifications_dao.py | 4 +- app/v2/notifications/post_notifications.py | 7 ++++ tests/app/celery/test_letters_pdf_tasks.py | 22 +++++----- .../notification_dao/test_notification_dao.py | 33 +++++++++++---- tests/app/letters/test_letter_utils.py | 7 ++++ .../test_post_letter_notifications.py | 6 ++- 7 files changed, 78 insertions(+), 43 deletions(-) diff --git a/app/celery/letters_pdf_tasks.py b/app/celery/letters_pdf_tasks.py index 913cce6a6..0f26c1fdc 100644 --- a/app/celery/letters_pdf_tasks.py +++ b/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, - create_fake_letter_response_file=None): +def update_billable_units_for_letter(self, notification_id, page_count): 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}") - else: - if notification.key_type != KEY_TYPE_TEST: - 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}") - if notification.key_type == KEY_TYPE_TEST \ - and current_app.config['NOTIFY_ENVIRONMENT'] in ['preview', 'development']: - # For test letters send a fake letter response - create_fake_letter_response_file.apply_async( - (notification.reference,), - queue=QueueNames.RESEARCH_MODE - ) + + current_app.logger.info( + f"Letter notification id: {notification_id} reference {notification.reference}: " + f"billable units set to {billable_units}" + ) + + +@notify_celery.task( + bind=True, name="update-validation-failed-for-templated-letter", max_retries=15, default_retry_delay=300 +) +def update_validation_failed_for_templated_letter(self, notification_id, page_count): + notification = get_notification_by_id(notification_id, _raise=True) + notification.status = NOTIFICATION_VALIDATION_FAILED + 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') diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 6b4c3baac..9f6826790 100644 --- a/app/dao/notifications_dao.py +++ b/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( diff --git a/app/v2/notifications/post_notifications.py b/app/v2/notifications/post_notifications.py index 5aab5067b..f2c82d2a2 100644 --- a/app/v2/notifications/post_notifications.py +++ b/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, diff --git a/tests/app/celery/test_letters_pdf_tasks.py b/tests/app/celery/test_letters_pdf_tasks.py index db33e398d..1de2d8379 100644 --- a/tests/app/celery/test_letters_pdf_tasks.py +++ b/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_or_validation_failed_for_templated_letter( - mocker, sample_letter_notification, number_of_pages, expected_billable_units +def test_update_billable_units_for_letter( + mocker, sample_letter_notification ): 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 diff --git a/tests/app/dao/notification_dao/test_notification_dao.py b/tests/app/dao/notification_dao/test_notification_dao.py index bafc9861c..e4776135d 100644 --- a/tests/app/dao/notification_dao/test_notification_dao.py +++ b/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, 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, 14, 30), billable_units=3), - 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, 30), postage='first'), - 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, 32)), - create_notification(template=second_template, created_at=datetime(2020, 12, 1, 8, 33)), + 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, 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=0) + 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, 30), postage='first') + 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, 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)) diff --git a/tests/app/letters/test_letter_utils.py b/tests/app/letters/test_letter_utils.py index f4821a95d..ae9b188fa 100644 --- a/tests/app/letters/test_letter_utils.py +++ b/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 diff --git a/tests/app/v2/notifications/test_post_letter_notifications.py b/tests/app/v2/notifications/test_post_letter_notifications.py index 1528391f5..4687f5ae2 100644 --- a/tests/app/v2/notifications/test_post_letter_notifications.py +++ b/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