From 841a4fc22f57bccd717732186bf50c8ec8d101a9 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Thu, 6 Jan 2022 09:19:59 +0000 Subject: [PATCH 1/3] 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 From 6cd7a23d3cc53de2944b20b96a8c005bdb09dab4 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Fri, 7 Jan 2022 09:15:21 +0000 Subject: [PATCH 2/3] 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 From c01c81326c6516abc25fe2267b608ae71897e595 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Mon, 24 Jan 2022 12:25:53 +0000 Subject: [PATCH 3/3] Update log message to something a little easier to read and query for. --- app/celery/letters_pdf_tasks.py | 3 +-- tests/app/celery/test_letters_pdf_tasks.py | 3 +-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/app/celery/letters_pdf_tasks.py b/app/celery/letters_pdf_tasks.py index 0f26c1fdc..6e9bc6210 100644 --- a/app/celery/letters_pdf_tasks.py +++ b/app/celery/letters_pdf_tasks.py @@ -123,8 +123,7 @@ def update_validation_failed_for_templated_letter(self, notification_id, page_co 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}") + current_app.logger.info(f"Validation failed: letter is too long {page_count} for letter with id: {notification_id}") @notify_celery.task(name='collate-letter-pdfs-to-be-sent') diff --git a/tests/app/celery/test_letters_pdf_tasks.py b/tests/app/celery/test_letters_pdf_tasks.py index 1de2d8379..a2419508a 100644 --- a/tests/app/celery/test_letters_pdf_tasks.py +++ b/tests/app/celery/test_letters_pdf_tasks.py @@ -190,8 +190,7 @@ def test_update_validation_failed_for_templated_letter_with_too_many_pages( 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" + f"Validation failed: letter is too long 11 for letter with id: {sample_letter_notification.id}" )