From 24a89c1c19fee553dbae69889fba40792dc7943a Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Thu, 30 Apr 2020 17:49:12 +0100 Subject: [PATCH] Modify tasks for getting letter pdf and updating billable units So that they talk with new template preview task for pdf creation --- app/celery/letters_pdf_tasks.py | 97 +++++----- app/config.py | 1 + tests/app/celery/test_letters_pdf_tasks.py | 204 ++++++--------------- 3 files changed, 106 insertions(+), 196 deletions(-) diff --git a/app/celery/letters_pdf_tasks.py b/app/celery/letters_pdf_tasks.py index 0d749e057..2a7f14fcc 100644 --- a/app/celery/letters_pdf_tasks.py +++ b/app/celery/letters_pdf_tasks.py @@ -1,15 +1,10 @@ -import math from datetime import datetime, timedelta from hashlib import sha512 from base64 import urlsafe_b64encode from botocore.exceptions import ClientError as BotoClientError from flask import current_app -from requests import ( - post as requests_post, - RequestException -) -from celery.exceptions import MaxRetriesExceededError + from notifications_utils.statsd_decorators import statsd from notifications_utils.letter_timings import LETTER_PROCESSING_DEADLINE from notifications_utils.timezones import convert_utc_to_bst @@ -31,7 +26,6 @@ from app.exceptions import NotificationTechnicalFailureException from app.letters.utils import ( get_billable_units_for_letter_page_count, get_reference_from_filename, - upload_letter_pdf, ScanErrorType, move_failed_pdf, move_sanitised_letter_to_test_or_live_pdf_bucket, @@ -57,62 +51,63 @@ from app.cronitor import cronitor def create_letters_pdf(self, notification_id): try: notification = get_notification_by_id(notification_id, _raise=True) - pdf_data, billable_units = get_letters_pdf( - notification.template, - contact_block=notification.reply_to_text, - filename=notification.service.letter_branding and notification.service.letter_branding.filename, - values=notification.personalisation + + letter_filename = get_letter_pdf_filename( + reference=notification.reference, + crown=notification.service.crown, + sending_date=notification.created_at, + dont_use_sending_date=notification.key_type == KEY_TYPE_TEST, + postage=notification.postage ) + letter_data = { + 'letter_contact_block': notification.reply_to_text, + 'template': { + "subject": notification.template.subject, + "content": notification.template.content, + "template_type": notification.template.template_type + }, + 'values': notification.personalisation, + 'logo_filename': notification.service.letter_branding and notification.service.letter_branding.filename, + 'letter_filename': letter_filename, + "notification_id": str(notification_id), + 'key_type': notification.key_type + } - upload_letter_pdf(notification, pdf_data) + encrypted_data = encryption.encrypt(letter_data) - if notification.key_type != KEY_TYPE_TEST: - notification.billable_units = billable_units - dao_update_notification(notification) - - current_app.logger.info( - 'Letter notification reference {reference}: billable units set to {billable_units}'.format( - reference=str(notification.reference), billable_units=billable_units)) - - except (RequestException, BotoClientError): + notify_celery.send_task( + name=TaskNames.CREATE_LETTER_PDF, + args=(encrypted_data,), + queue=QueueNames.SANITISE_LETTERS + ) + except Exception: try: current_app.logger.exception( - "Letters PDF notification creation for id: {} failed".format(notification_id) + "RETRY: calling create-letter-pdf task for notification {} failed".format(notification_id) ) self.retry(queue=QueueNames.RETRY) - except MaxRetriesExceededError: - current_app.logger.error( - "RETRY FAILED: task create_letters_pdf failed for notification {}".format(notification_id), - ) - update_notification_status_by_id(notification_id, 'technical-failure') + except self.MaxRetriesExceededError: + message = "RETRY FAILED: Max retries reached. " \ + "The task create-letter-pdf failed for notification {}. " \ + "Notification has been updated to technical-failure".format(notification_id) + update_notification_status_by_id(notification_id, NOTIFICATION_TECHNICAL_FAILURE) + raise NotificationTechnicalFailureException(message) -def get_letters_pdf(template, contact_block, filename, values): - template_for_letter_print = { - "subject": template.subject, - "content": template.content, - "template_type": template.template_type - } +@notify_celery.task(bind=True, name="update-billable-units-for-letter", max_retries=15, default_retry_delay=300) +@statsd(namespace="tasks") +def update_billable_units_for_letter(self, notification_id, page_count): + notification = get_notification_by_id(notification_id, _raise=True) - data = { - 'letter_contact_block': contact_block, - 'template': template_for_letter_print, - 'values': values, - 'filename': filename, - } - resp = requests_post( - '{}/print.pdf'.format( - current_app.config['TEMPLATE_PREVIEW_API_HOST'] - ), - json=data, - headers={'Authorization': 'Token {}'.format(current_app.config['TEMPLATE_PREVIEW_API_KEY'])} - ) - resp.raise_for_status() + billable_units = get_billable_units_for_letter_page_count(page_count) - pages_per_sheet = 2 - billable_units = math.ceil(int(resp.headers.get("X-pdf-page-count", 0)) / pages_per_sheet) + if notification.key_type != KEY_TYPE_TEST: + notification.billable_units = billable_units + dao_update_notification(notification) - return resp.content, billable_units + current_app.logger.info( + 'Letter notification reference {reference}: billable units set to {billable_units}'.format( + reference=str(notification.reference), billable_units=billable_units)) @notify_celery.task(name='collate-letter-pdfs-to-be-sent') diff --git a/app/config.py b/app/config.py index 42902f8e1..52de6b46c 100644 --- a/app/config.py +++ b/app/config.py @@ -59,6 +59,7 @@ class TaskNames(object): ZIP_AND_SEND_LETTER_PDFS = 'zip-and-send-letter-pdfs' SCAN_FILE = 'scan-file' SANITISE_LETTER = 'sanitise-and-upload-letter' + CREATE_LETTER_PDF = 'create-letter-pdf' class Config(object): diff --git a/tests/app/celery/test_letters_pdf_tasks.py b/tests/app/celery/test_letters_pdf_tasks.py index 6f4e1562a..e066f9e18 100644 --- a/tests/app/celery/test_letters_pdf_tasks.py +++ b/tests/app/celery/test_letters_pdf_tasks.py @@ -6,10 +6,8 @@ from moto import mock_s3 from flask import current_app from freezegun import freeze_time import pytest -import requests_mock from botocore.exceptions import ClientError from celery.exceptions import MaxRetriesExceededError -from requests import RequestException from sqlalchemy.orm.exc import NoResultFound from app import encryption @@ -17,7 +15,6 @@ from app.errors import VirusScanError from app.exceptions import NotificationTechnicalFailureException from app.celery.letters_pdf_tasks import ( create_letters_pdf, - get_letters_pdf, collate_letter_pdfs_to_be_sent, get_key_and_size_of_letters_to_be_sent_to_print, group_letters, @@ -26,6 +23,7 @@ from app.celery.letters_pdf_tasks import ( process_virus_scan_error, replay_letters_in_error, sanitise_letter, + update_billable_units_for_letter, _move_invalid_letter_and_update_status, ) from app.config import QueueNames, TaskNames @@ -58,188 +56,104 @@ def test_should_have_decorated_tasks_functions(): assert process_sanitised_letter.__wrapped__.__name__ == 'process_sanitised_letter' -@pytest.mark.parametrize('personalisation', [{'name': 'test'}, None]) -def test_get_letters_pdf_calls_notifications_template_preview_service_correctly( - notify_api, mocker, client, sample_letter_template, personalisation): - contact_block = 'Mr Foo,\n1 Test Street,\nLondon\nN1' - filename = 'opg' +def test_create_letters_pdf(mocker, sample_letter_notification): + mock_celery = mocker.patch('app.celery.letters_pdf_tasks.notify_celery.send_task') + mocker.patch('app.celery.letters_pdf_tasks.get_letter_pdf_filename', return_value='LETTER.PDF') + create_letters_pdf(sample_letter_notification.id) - with set_config_values(notify_api, { - 'TEMPLATE_PREVIEW_API_HOST': 'http://localhost/notifications-template-preview', - 'TEMPLATE_PREVIEW_API_KEY': 'test-key' - }): - with requests_mock.Mocker() as request_mock: - mock_post = request_mock.post( - 'http://localhost/notifications-template-preview/print.pdf', content=b'\x00\x01', status_code=200) - - get_letters_pdf( - sample_letter_template, - contact_block=contact_block, - filename=filename, - values=personalisation) - - assert mock_post.last_request.json() == { - 'values': personalisation, - 'letter_contact_block': contact_block, - 'filename': filename, + letter_data = { + 'letter_contact_block': sample_letter_notification.reply_to_text, 'template': { - 'subject': sample_letter_template.subject, - 'content': sample_letter_template.content, - 'template_type': sample_letter_template.template_type - } + "subject": sample_letter_notification.template.subject, + "content": sample_letter_notification.template.content, + "template_type": sample_letter_notification.template.template_type + }, + 'values': sample_letter_notification.personalisation, + 'logo_filename': None, # no logo + 'letter_filename': 'LETTER.PDF', + "notification_id": str(sample_letter_notification.id), + 'key_type': sample_letter_notification.key_type } + encrypted_data = encryption.encrypt(letter_data) -@pytest.mark.parametrize('page_count,expected_billable_units', [ - ('1', 1), - ('2', 1), - ('3', 2) -]) -def test_get_letters_pdf_calculates_billing_units( - notify_api, mocker, client, sample_letter_template, page_count, expected_billable_units): - contact_block = 'Mr Foo,\n1 Test Street,\nLondon\nN1' - filename = 'opg' - - with set_config_values(notify_api, { - 'TEMPLATE_PREVIEW_API_HOST': 'http://localhost/notifications-template-preview', - 'TEMPLATE_PREVIEW_API_KEY': 'test-key' - }): - with requests_mock.Mocker() as request_mock: - request_mock.post( - 'http://localhost/notifications-template-preview/print.pdf', - content=b'\x00\x01', - headers={'X-pdf-page-count': page_count}, - status_code=200 - ) - - _, billable_units = get_letters_pdf( - sample_letter_template, contact_block=contact_block, filename=filename, values=None) - - assert billable_units == expected_billable_units - - -@freeze_time("2017-12-04 17:31:00") -def test_create_letters_pdf_calls_s3upload(mocker, sample_letter_template): - mocker.patch('app.celery.letters_pdf_tasks.get_letters_pdf', return_value=(b'\x00\x01', '1')) - mock_s3 = mocker.patch('app.letters.utils.s3upload') - notification = create_notification(template=sample_letter_template, reference='FOO', key_type='normal') - - create_letters_pdf(notification.id) - - mock_s3.assert_called_with( - bucket_name=current_app.config['LETTERS_PDF_BUCKET_NAME'], - file_location='2017-12-05/NOTIFY.FOO.D.2.C.C.20171204173100.PDF', - filedata=b'\x00\x01', - region=current_app.config['AWS_REGION'] + mock_celery.assert_called_once_with( + name=TaskNames.CREATE_LETTER_PDF, + args=(encrypted_data,), + queue=QueueNames.SANITISE_LETTERS ) -@freeze_time("2017-12-04 17:31:00") -def test_create_letters_pdf_calls_s3upload_for_test_letters(mocker, sample_letter_template): - mocker.patch('app.celery.letters_pdf_tasks.get_letters_pdf', return_value=(b'\x00\x01', '1')) - mock_s3 = mocker.patch('app.letters.utils.s3upload') - notification = create_notification(template=sample_letter_template, reference='FOO', key_type='test') - - create_letters_pdf(notification.id) - - mock_s3.assert_called_with( - bucket_name=current_app.config['TEST_LETTERS_BUCKET_NAME'], - file_location='NOTIFY.FOO.D.2.C.C.20171204173100.PDF', - filedata=b'\x00\x01', - region=current_app.config['AWS_REGION'] - ) - - -def test_create_letters_pdf_sets_billable_units(mocker, sample_letter_notification): - mocker.patch('app.celery.letters_pdf_tasks.get_letters_pdf', return_value=(b'\x00\x01', 1)) - mocker.patch('app.letters.utils.s3upload') - - create_letters_pdf(sample_letter_notification.id) - noti = Notification.query.filter(Notification.reference == sample_letter_notification.reference).one() - assert noti.billable_units == 1 - - def test_create_letters_pdf_non_existent_notification(notify_api, mocker, fake_uuid): with pytest.raises(expected_exception=NoResultFound): create_letters_pdf(fake_uuid) -def test_create_letters_pdf_handles_request_errors(mocker, sample_letter_notification): - mock_get_letters_pdf = mocker.patch('app.celery.letters_pdf_tasks.get_letters_pdf', side_effect=RequestException) +def test_create_letters_pdf_retries_upon_error(mocker, sample_letter_notification): + mock_celery = mocker.patch('app.celery.letters_pdf_tasks.notify_celery.send_task', side_effect=Exception()) + mocker.patch('app.celery.letters_pdf_tasks.get_letter_pdf_filename', return_value='LETTER.PDF') mock_retry = mocker.patch('app.celery.letters_pdf_tasks.create_letters_pdf.retry') create_letters_pdf(sample_letter_notification.id) - assert mock_get_letters_pdf.called - assert mock_retry.called - - -def test_create_letters_pdf_handles_s3_errors(mocker, sample_letter_notification): - mocker.patch('app.celery.letters_pdf_tasks.get_letters_pdf', return_value=(b'\x00\x01', 1)) - error_response = { - 'Error': { - 'Code': 'InvalidParameterValue', - 'Message': 'some error message from amazon', - 'Type': 'Sender' - } - } - mock_s3 = mocker.patch('app.letters.utils.s3upload', side_effect=ClientError(error_response, 'operation_name')) - mock_retry = mocker.patch('app.celery.letters_pdf_tasks.create_letters_pdf.retry') - - create_letters_pdf(sample_letter_notification.id) - - assert mock_s3.called + assert mock_celery.called assert mock_retry.called def test_create_letters_pdf_sets_technical_failure_max_retries(mocker, sample_letter_notification): - mock_get_letters_pdf = mocker.patch('app.celery.letters_pdf_tasks.get_letters_pdf', side_effect=RequestException) + mock_celery = mocker.patch('app.celery.letters_pdf_tasks.notify_celery.send_task', side_effect=Exception()) + mocker.patch('app.celery.letters_pdf_tasks.get_letter_pdf_filename', return_value='LETTER.PDF') mock_retry = mocker.patch( 'app.celery.letters_pdf_tasks.create_letters_pdf.retry', side_effect=MaxRetriesExceededError) mock_update_noti = mocker.patch('app.celery.letters_pdf_tasks.update_notification_status_by_id') - create_letters_pdf(sample_letter_notification.id) + with pytest.raises(NotificationTechnicalFailureException): + create_letters_pdf(sample_letter_notification.id) - assert mock_get_letters_pdf.called + assert mock_celery.called assert mock_retry.called mock_update_noti.assert_called_once_with(sample_letter_notification.id, 'technical-failure') -def test_create_letters_gets_the_right_logo_when_service_has_no_logo( - notify_api, mocker, sample_letter_notification -): - mock_get_letters_pdf = mocker.patch('app.celery.letters_pdf_tasks.get_letters_pdf', return_value=(b'\x00\x01', 1)) - mocker.patch('app.letters.utils.s3upload') - mocker.patch('app.celery.letters_pdf_tasks.update_notification_status_by_id') - - create_letters_pdf(sample_letter_notification.id) - mock_get_letters_pdf.assert_called_once_with( - sample_letter_notification.template, - contact_block=sample_letter_notification.reply_to_text, - filename=None, - values=sample_letter_notification.personalisation - ) - - # We only need this while we are migrating to the new letter_branding model def test_create_letters_gets_the_right_logo_when_service_has_letter_branding_logo( notify_api, mocker, sample_letter_notification ): letter_branding = create_letter_branding(name='test brand', filename='test-brand') sample_letter_notification.service.letter_branding = letter_branding - mock_get_letters_pdf = mocker.patch('app.celery.letters_pdf_tasks.get_letters_pdf', return_value=(b'\x00\x01', 1)) - mocker.patch('app.letters.utils.s3upload') - mocker.patch('app.celery.letters_pdf_tasks.update_notification_status_by_id') - + mock_celery = mocker.patch('app.celery.letters_pdf_tasks.notify_celery.send_task') + mocker.patch('app.celery.letters_pdf_tasks.get_letter_pdf_filename', return_value='LETTER.PDF') create_letters_pdf(sample_letter_notification.id) - mock_get_letters_pdf.assert_called_once_with( - sample_letter_notification.template, - contact_block=sample_letter_notification.reply_to_text, - filename=sample_letter_notification.service.letter_branding.filename, - values=sample_letter_notification.personalisation + + letter_data = { + 'letter_contact_block': sample_letter_notification.reply_to_text, + 'template': { + "subject": sample_letter_notification.template.subject, + "content": sample_letter_notification.template.content, + "template_type": sample_letter_notification.template.template_type + }, + 'values': sample_letter_notification.personalisation, + 'logo_filename': sample_letter_notification.service.letter_branding.filename, + 'letter_filename': 'LETTER.PDF', + "notification_id": str(sample_letter_notification.id), + 'key_type': sample_letter_notification.key_type + } + + encrypted_data = encryption.encrypt(letter_data) + + mock_celery.assert_called_once_with( + name=TaskNames.CREATE_LETTER_PDF, + args=(encrypted_data,), + queue=QueueNames.SANITISE_LETTERS ) +def test_update_billable_units_for_letter(mocker, sample_letter_notification): + update_billable_units_for_letter(sample_letter_notification.id, 2) + noti = Notification.query.filter(Notification.reference == sample_letter_notification.reference).one() + assert noti.billable_units == 1 + + @freeze_time('2020-02-17 18:00:00') def test_get_key_and_size_of_letters_to_be_sent_to_print(notify_api, mocker, sample_letter_template): create_notification(