From f23074596b5229646096cc5c34f21012a6b6ce06 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Wed, 13 Dec 2017 15:52:38 +0000 Subject: [PATCH 1/2] Update billable units for letters pdf task --- app/celery/letters_pdf_tasks.py | 33 +++++++++++-- tests/app/celery/test_letters_pdf_tasks.py | 55 +++++++++++++++++++++- 2 files changed, 83 insertions(+), 5 deletions(-) diff --git a/app/celery/letters_pdf_tasks.py b/app/celery/letters_pdf_tasks.py index 24c7347ae..a7b565139 100644 --- a/app/celery/letters_pdf_tasks.py +++ b/app/celery/letters_pdf_tasks.py @@ -1,4 +1,6 @@ +from datetime import datetime from flask import current_app +import math from requests import ( post as requests_post, RequestException @@ -9,7 +11,11 @@ from botocore.exceptions import ClientError as BotoClientError from app import notify_celery from app.aws import s3 from app.config import QueueNames -from app.dao.notifications_dao import get_notification_by_id, update_notification_status_by_id +from app.dao.notifications_dao import ( + get_notification_by_id, + update_notification_status_by_id, + dao_update_notifications_by_reference +) from app.statsd_decorators import statsd @@ -19,7 +25,7 @@ def create_letters_pdf(self, notification_id): try: notification = get_notification_by_id(notification_id, _raise=True) - pdf_data = get_letters_pdf( + pdf_data, billable_units = get_letters_pdf( notification.template, contact_block=notification.reply_to_text, org_id=notification.service.dvla_organisation.id, @@ -28,6 +34,24 @@ def create_letters_pdf(self, notification_id): current_app.logger.info("PDF Letter {} reference {} created at {}, {} bytes".format( notification.id, notification.reference, notification.created_at, len(pdf_data))) s3.upload_letters_pdf(reference=notification.reference, crown=notification.service.crown, filedata=pdf_data) + + updated_count = dao_update_notifications_by_reference( + references=[notification.reference], + update_dict={ + "billable_units": billable_units, + "updated_at": datetime.utcnow() + } + ) + + if not updated_count: + msg = "Update letter notification billing units failed: notification not found with reference {}".format( + notification.reference) + current_app.logger.error(msg) + else: + 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): try: current_app.logger.exception( @@ -62,4 +86,7 @@ def get_letters_pdf(template, contact_block, org_id, values): ) resp.raise_for_status() - return resp.content + pages_per_sheet = 2 + billable_units = math.ceil(int(resp.headers.get("X-pdf-page-count", 0)) / pages_per_sheet) + + return resp.content, billable_units diff --git a/tests/app/celery/test_letters_pdf_tasks.py b/tests/app/celery/test_letters_pdf_tasks.py index 0af5c78ab..c540a2388 100644 --- a/tests/app/celery/test_letters_pdf_tasks.py +++ b/tests/app/celery/test_letters_pdf_tasks.py @@ -10,6 +10,7 @@ from app.celery.letters_pdf_tasks import ( create_letters_pdf, get_letters_pdf, ) +from app.models import Notification from tests.conftest import set_config_values @@ -46,8 +47,36 @@ def test_get_letters_pdf_calls_notifications_template_preview_service_correctly( } +@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' + dvla_org_id = '002' + + 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, org_id=dvla_org_id, values=None) + + assert billable_units == expected_billable_units + + def test_create_letters_pdf_calls_upload_letters_pdf(mocker, sample_letter_notification): - mocker.patch('app.celery.letters_pdf_tasks.get_letters_pdf', return_value=b'\x00\x01') + mocker.patch('app.celery.letters_pdf_tasks.get_letters_pdf', return_value=(b'\x00\x01', '1')) mock_s3 = mocker.patch('app.celery.tasks.s3.upload_letters_pdf') create_letters_pdf(sample_letter_notification.id) @@ -59,6 +88,28 @@ def test_create_letters_pdf_calls_upload_letters_pdf(mocker, sample_letter_notif ) +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.celery.tasks.s3.upload_letters_pdf') + + 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_handles_update_failure(mocker, sample_letter_notification): + mocker.patch('app.celery.letters_pdf_tasks.get_letters_pdf', return_value=(b'\x00\x01', 1)) + mocker.patch('app.celery.tasks.s3.upload_letters_pdf') + mocker.patch('app.celery.tasks.letters_pdf_tasks.dao_update_notifications_by_reference', return_value=0) + mock_error_logger = mocker.patch('app.celery.letters_pdf_tasks.current_app.logger.error') + + create_letters_pdf(sample_letter_notification.id) + + mock_error_logger.assert_called_once_with( + "Update letter notification billing units failed: notification not found with reference {}".format( + sample_letter_notification.reference)) + + def test_create_letters_pdf_non_existent_notification(notify_api, mocker, fake_uuid): with pytest.raises(expected_exception=NoResultFound): create_letters_pdf(fake_uuid) @@ -75,7 +126,7 @@ def test_create_letters_pdf_handles_request_errors(mocker, sample_letter_notific def test_create_letters_pdf_handles_s3_errors(mocker, sample_letter_notification): - mocker.patch('app.celery.letters_pdf_tasks.get_letters_pdf') + mocker.patch('app.celery.letters_pdf_tasks.get_letters_pdf', return_value=(b'\x00\x01', 1)) mock_s3 = mocker.patch('app.celery.tasks.s3.upload_letters_pdf', side_effect=ClientError({}, 'operation_name')) mock_retry = mocker.patch('app.celery.letters_pdf_tasks.create_letters_pdf.retry') From 0045cd6b72149a3cff620fb016281a035eef981f Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Thu, 14 Dec 2017 11:38:17 +0000 Subject: [PATCH 2/2] Use `dao_update_notification` to update `billable_units` for letter notifications --- app/celery/letters_pdf_tasks.py | 23 ++++++---------------- tests/app/celery/test_letters_pdf_tasks.py | 13 ------------ 2 files changed, 6 insertions(+), 30 deletions(-) diff --git a/app/celery/letters_pdf_tasks.py b/app/celery/letters_pdf_tasks.py index a7b565139..d70265791 100644 --- a/app/celery/letters_pdf_tasks.py +++ b/app/celery/letters_pdf_tasks.py @@ -1,4 +1,3 @@ -from datetime import datetime from flask import current_app import math from requests import ( @@ -14,7 +13,7 @@ from app.config import QueueNames from app.dao.notifications_dao import ( get_notification_by_id, update_notification_status_by_id, - dao_update_notifications_by_reference + dao_update_notification ) from app.statsd_decorators import statsd @@ -35,22 +34,12 @@ def create_letters_pdf(self, notification_id): notification.id, notification.reference, notification.created_at, len(pdf_data))) s3.upload_letters_pdf(reference=notification.reference, crown=notification.service.crown, filedata=pdf_data) - updated_count = dao_update_notifications_by_reference( - references=[notification.reference], - update_dict={ - "billable_units": billable_units, - "updated_at": datetime.utcnow() - } - ) + notification.billable_units = billable_units + dao_update_notification(notification) - if not updated_count: - msg = "Update letter notification billing units failed: notification not found with reference {}".format( - notification.reference) - current_app.logger.error(msg) - else: - current_app.logger.info( - 'Letter notification reference {reference}: billable units set to {billable_units}'.format( - reference=str(notification.reference), billable_units=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)) except (RequestException, BotoClientError): try: diff --git a/tests/app/celery/test_letters_pdf_tasks.py b/tests/app/celery/test_letters_pdf_tasks.py index c540a2388..a414fa2db 100644 --- a/tests/app/celery/test_letters_pdf_tasks.py +++ b/tests/app/celery/test_letters_pdf_tasks.py @@ -97,19 +97,6 @@ def test_create_letters_pdf_sets_billable_units(mocker, sample_letter_notificati assert noti.billable_units == 1 -def test_create_letters_pdf_handles_update_failure(mocker, sample_letter_notification): - mocker.patch('app.celery.letters_pdf_tasks.get_letters_pdf', return_value=(b'\x00\x01', 1)) - mocker.patch('app.celery.tasks.s3.upload_letters_pdf') - mocker.patch('app.celery.tasks.letters_pdf_tasks.dao_update_notifications_by_reference', return_value=0) - mock_error_logger = mocker.patch('app.celery.letters_pdf_tasks.current_app.logger.error') - - create_letters_pdf(sample_letter_notification.id) - - mock_error_logger.assert_called_once_with( - "Update letter notification billing units failed: notification not found with reference {}".format( - sample_letter_notification.reference)) - - def test_create_letters_pdf_non_existent_notification(notify_api, mocker, fake_uuid): with pytest.raises(expected_exception=NoResultFound): create_letters_pdf(fake_uuid)