From 841a4fc22f57bccd717732186bf50c8ec8d101a9 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Thu, 6 Jan 2022 09:19:59 +0000 Subject: [PATCH] Mark letters as validation-failed if the templated letter is too long. It is possible that the personalisation for a templated letter can make the letter exceed 10 pages or 5 sheets. We are not validating the letters posted via the API for this validation error. It is only possible to validate the letter once we create the PDF in notifications-template-preview. This means that the letter can only get a validation-failed status after the client has received a 201 from the POST to /v2/notifications. NOTE: we only validate the preview row of a CSV for this validation error, this change will mean that it is possible for a letter to be marked as validation-failed after a successful file upload. A new task to update the notification to `validation-failed` has been added to the API. If we find that the letter is too long once we have created the PDF we call the `update-validation-failed-for-templated-letter` task rather than `update-billable-units-for-letter` task. New work flow for a letter in brief: API - receives POST /v2/notifications :: save to db :: put CREATE_LETTERS_PDF task on queue for template preview to consume TEMPLATE-PREVIEW - consumes task CREATE_LETTERS_PDF :: create PDF :: count pages of PDF :: IF page count exceeds 10 pages put in the letters-invalid-pdf S3 bucket with metadata (similar to the precompiled letters) put `update-validation-failed-for-templated-letter` task on the queue for the API to consume ELSE put PDF in the `letters-pdf` bucket put `update-billable-units-for-letter` task on the queue API - consumes `update-billable-units-for-letter` OR `update-validation-failed-for-templated-letter` task :: IF `update-billable-units-for-letter` task: update billable units for notification as usual :: ELSE `update-validation-failed-for-templated-letter`: update notification_status = `validation-failed` ADMIN - view notification page for letter :: show validation letter for templated letter There will be 3 PRs in order to make this change, one for the API, template-preview and the admin app. Deployment plan Deploy Admin first Deploy API Deploy template-preview Related PRs: alphagov/notifications-template-preview#619 alphagov/notifications-admin#4107 https://www.pivotaltracker.com/story/show/169209742 --- app/celery/letters_pdf_tasks.py | 31 ++++++++++------- app/v2/notifications/post_notifications.py | 6 ---- tests/app/celery/test_letters_pdf_tasks.py | 33 ++++++++++++++++--- .../test_post_letter_notifications.py | 5 +-- 4 files changed, 49 insertions(+), 26 deletions(-) diff --git a/app/celery/letters_pdf_tasks.py b/app/celery/letters_pdf_tasks.py index 19e627c8a..913cce6a6 100644 --- a/app/celery/letters_pdf_tasks.py +++ b/app/celery/letters_pdf_tasks.py @@ -4,6 +4,7 @@ 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 @@ -101,19 +102,27 @@ 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_for_letter(self, notification_id, page_count): +def update_billable_units_or_validation_failed_for_templated_letter(self, notification_id, page_count, + create_fake_letter_response_file=None): notification = get_notification_by_id(notification_id, _raise=True) - - billable_units = get_billable_units_for_letter_page_count(page_count) - - if notification.key_type != KEY_TYPE_TEST: - notification.billable_units = billable_units + if page_count > LETTER_MAX_PAGE_COUNT: + notification.status = NOTIFICATION_VALIDATION_FAILED dao_update_notification(notification) - - current_app.logger.info( - f"Letter notification id: {notification_id} reference {notification.reference}: " - f"billable units set to {billable_units}" - ) + 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 + ) @notify_celery.task(name='collate-letter-pdfs-to-be-sent') diff --git a/app/v2/notifications/post_notifications.py b/app/v2/notifications/post_notifications.py index c5ea8d2b3..5aab5067b 100644 --- a/app/v2/notifications/post_notifications.py +++ b/app/v2/notifications/post_notifications.py @@ -19,7 +19,6 @@ 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 @@ -389,11 +388,6 @@ 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 02aae3ce2..db33e398d 100644 --- a/tests/app/celery/test_letters_pdf_tasks.py +++ b/tests/app/celery/test_letters_pdf_tasks.py @@ -26,7 +26,7 @@ from app.celery.letters_pdf_tasks import ( resanitise_pdf, sanitise_letter, send_letters_volume_email_to_dvla, - update_billable_units_for_letter, + update_billable_units_or_validation_failed_for_templated_letter, ) from app.config import QueueNames, TaskNames from app.dao.notifications_dao import get_notifications @@ -146,32 +146,55 @@ def test_get_pdf_for_templated_letter_sets_technical_failure_max_retries(mocker, @pytest.mark.parametrize('number_of_pages, expected_billable_units', [(2, 1), (3, 2), (10, 5)]) -def test_update_billable_units_for_letter(mocker, sample_letter_notification, number_of_pages, expected_billable_units): +def test_update_billable_units_or_validation_failed_for_templated_letter( + 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_for_letter(sample_letter_notification.id, number_of_pages) + update_billable_units_or_validation_failed_for_templated_letter(sample_letter_notification.id, number_of_pages) notification = Notification.query.filter(Notification.reference == sample_letter_notification.reference).one() assert notification.billable_units == expected_billable_units + 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}" ) -def test_update_billable_units_for_letter_doesnt_update_if_sent_with_test_key(mocker, sample_letter_notification): +def test_update_billable_units_or_validation_failed_for_templated_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_for_letter(sample_letter_notification.id, 2) + update_billable_units_or_validation_failed_for_templated_letter(sample_letter_notification.id, 2) notification = Notification.query.filter(Notification.reference == sample_letter_notification.reference).one() assert notification.billable_units == 0 mock_logger.assert_not_called() +@pytest.mark.parametrize('key_type', ['test', 'normal', 'team']) +def test_update_billable_units_or_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) + + assert sample_letter_notification.billable_units == 0 + assert sample_letter_notification.status == NOTIFICATION_VALIDATION_FAILED + mock_logger.assert_called_once_with( + f"Letter notification id: {sample_letter_notification.id} reference {sample_letter_notification.reference}: " + f"validation failed: letter is too long 11" + ) + + @mock_s3 @freeze_time('2020-02-17 18:00:00') def test_get_key_and_size_of_letters_to_be_sent_to_print( diff --git a/tests/app/v2/notifications/test_post_letter_notifications.py b/tests/app/v2/notifications/test_post_letter_notifications.py index c09b9f834..1528391f5 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_and_sets_status_to_sending_and_sends_fake_response_file( +def test_post_letter_notification_with_test_key_creates_pdf( notify_api, client, sample_letter_template, mocker, env): data = { @@ -312,8 +312,6 @@ def test_post_letter_notification_with_test_key_creates_pdf_and_sets_status_to_s } 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 @@ -323,7 +321,6 @@ def test_post_letter_notification_with_test_key_creates_pdf_and_sets_status_to_s 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