From 08b21bc0f4fa758c324ee58d613b326c3deba2af Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Fri, 1 Dec 2017 17:00:39 +0000 Subject: [PATCH 01/31] remove fragments from db --- .../0151_kill_service_free_fragments.py | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) create mode 100644 migrations/versions/0151_kill_service_free_fragments.py diff --git a/migrations/versions/0151_kill_service_free_fragments.py b/migrations/versions/0151_kill_service_free_fragments.py new file mode 100644 index 000000000..ecc3c126d --- /dev/null +++ b/migrations/versions/0151_kill_service_free_fragments.py @@ -0,0 +1,24 @@ +""" + +Revision ID: 0151_kill_service_free_fragments +Revises: 0150_another_letter_org +Create Date: 2017-12-01 16:49:51.178455 + +""" +from alembic import op +import sqlalchemy as sa + +revision = '0151_kill_service_free_fragments' +down_revision = '0150_another_letter_org' + + +def upgrade(): + # ### commands auto generated by Alembic - please adjust! ### + op.drop_column('services', 'free_sms_fragment_limit') + op.drop_column('services_history', 'free_sms_fragment_limit') + + +def downgrade(): + # ### commands auto generated by Alembic - please adjust! ### + op.add_column('services_history', sa.Column('free_sms_fragment_limit', sa.BIGINT(), autoincrement=False, nullable=True)) + op.add_column('services', sa.Column('free_sms_fragment_limit', sa.BIGINT(), autoincrement=False, nullable=True)) From da468681f236fcc4d19e4fdb7cd9f2f518d83d37 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 7 Dec 2017 10:41:10 +0000 Subject: [PATCH 02/31] add warning to fix_migrations script for when the filename is different --- scripts/fix_migrations.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/scripts/fix_migrations.py b/scripts/fix_migrations.py index 3965ed411..4bf07b7cb 100755 --- a/scripts/fix_migrations.py +++ b/scripts/fix_migrations.py @@ -46,8 +46,11 @@ def reorder_revisions(revisions, old_base, new_base): file_data = rev_file.read() file_data = file_data.replace(head.revision, new_revision_id).replace(old_base, new_base) + new_filename = head.path.replace(head.revision, new_revision_id) - with open(head.path.replace(head.revision, new_revision_id), 'w') as rev_file: + assert head.path != new_filename, 'Old filename not same as revision id, please rename file before continuing' + + with open(new_filename, 'w') as rev_file: rev_file.write(file_data) print("Removing {}".format(head.path)) From 02c105328cbc366d5e55bb0e8efac53ecd7f24f1 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 11 Dec 2017 16:37:16 +0000 Subject: [PATCH 03/31] Add undocumented support for
s in emails Implements: - [x] https://github.com/alphagov/notifications-utils/pull/275/files --- requirements.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements.txt b/requirements.txt index 0c0b2432e..9b28fa639 100644 --- a/requirements.txt +++ b/requirements.txt @@ -26,7 +26,7 @@ notifications-python-client==4.6.0 awscli==1.14.4 awscli-cwlogs>=1.4,<1.5 -git+https://github.com/alphagov/notifications-utils.git@23.3.1#egg=notifications-utils==23.3.1 +git+https://github.com/alphagov/notifications-utils.git@23.3.4#egg=notifications-utils==23.3.4 git+https://github.com/alphagov/boto.git@2.43.0-patch3#egg=boto==2.43.0-patch3 From a26588decd653ea729c82b94abfd07073c5877db Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Mon, 11 Dec 2017 17:14:36 +0000 Subject: [PATCH 04/31] Update the json in the service callback task to read completed_at rather than updated_at --- app/celery/service_callback_tasks.py | 2 +- tests/app/celery/test_service_callback_tasks.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/celery/service_callback_tasks.py b/app/celery/service_callback_tasks.py index 3bf033a80..19f322842 100644 --- a/app/celery/service_callback_tasks.py +++ b/app/celery/service_callback_tasks.py @@ -34,7 +34,7 @@ def send_delivery_status_to_service(self, notification_id): "to": notification.to, "status": notification.status, "created_at": notification.created_at.strftime(DATETIME_FORMAT), # the time GOV.UK email sent the request - "updated_at": notification.updated_at.strftime(DATETIME_FORMAT), # the last time the status was updated + "completed_at": notification.updated_at.strftime(DATETIME_FORMAT), # the last time the status was updated "sent_at": notification.sent_at.strftime(DATETIME_FORMAT), # the time the email was sent "notification_type": notification.notification_type } diff --git a/tests/app/celery/test_service_callback_tasks.py b/tests/app/celery/test_service_callback_tasks.py index f09333d2d..a7c9aaeb4 100644 --- a/tests/app/celery/test_service_callback_tasks.py +++ b/tests/app/celery/test_service_callback_tasks.py @@ -55,7 +55,7 @@ def test_send_delivery_status_to_service_post_https_request_to_service(notify_db "to": notification.to, "status": notification.status, "created_at": datestr.strftime(DATETIME_FORMAT), # the time GOV.UK email sent the request - "updated_at": datestr.strftime(DATETIME_FORMAT), # the last time the status was updated + "completed_at": datestr.strftime(DATETIME_FORMAT), # the last time the status was updated "sent_at": datestr.strftime(DATETIME_FORMAT), # the time the email was sent "notification_type": notification_type } From c8a434fe98831ce1abef6f50aaa32425fe9b5394 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Wed, 6 Dec 2017 13:51:52 +0000 Subject: [PATCH 05/31] Updated config for template preview env vars --- app/config.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/app/config.py b/app/config.py index d553041e3..27ba485fb 100644 --- a/app/config.py +++ b/app/config.py @@ -304,6 +304,9 @@ class Config(object): # {"dataset_1": "token_1", ...} PERFORMANCE_PLATFORM_ENDPOINTS = json.loads(os.environ.get('PERFORMANCE_PLATFORM_ENDPOINTS', '{}')) + TEMPLATE_PREVIEW_API_HOST = os.environ.get('TEMPLATE_PREVIEW_API_HOST', 'http://localhost:6013') + TEMPLATE_PREVIEW_API_KEY = os.environ.get('TEMPLATE_PREVIEW_API_KEY', 'my-secret-key') + ###################### # Config overrides ### From 16136317f9a2e826a5f352c02939ccc57efbc73a Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Wed, 6 Dec 2017 13:54:02 +0000 Subject: [PATCH 06/31] Add letters pdf task --- app/celery/tasks.py | 78 ++++++++++++++++++++++++++++++++-- tests/app/celery/test_tasks.py | 2 +- tests/app/conftest.py | 4 ++ 3 files changed, 80 insertions(+), 4 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 54d6090db..08cd4a92e 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -4,6 +4,8 @@ from collections import namedtuple from celery.signals import worker_process_shutdown from flask import current_app +import requests + from notifications_utils.recipients import ( RecipientCSV ) @@ -18,6 +20,7 @@ from requests import ( RequestException ) from sqlalchemy.exc import SQLAlchemyError +from sqlalchemy.orm.exc import NoResultFound from botocore.exceptions import ClientError as BotoClientError from app import ( @@ -44,7 +47,8 @@ from app.dao.notifications_dao import ( dao_update_notifications_for_job_to_sent_to_dvla, dao_update_notifications_by_reference, dao_get_last_notification_added_for_job_id, - dao_get_notifications_by_references + dao_get_notifications_by_references, + update_notification_status_by_id ) from app.dao.provider_details_dao import get_current_provider from app.dao.service_inbound_api_dao import get_service_inbound_api_for_service @@ -121,11 +125,17 @@ def process_job(job_id): def job_complete(job, service, template_type, resumed=False, start=None): - if template_type == LETTER_TYPE: + if ( + template_type == LETTER_TYPE and + 'letters_as_pdf' not in [p.permission for p in service.permissions] + ): if service.research_mode: update_job_to_sent_to_dvla.apply_async([str(job.id)], queue=QueueNames.RESEARCH_MODE) else: - build_dvla_file.apply_async([str(job.id)], queue=QueueNames.JOBS) + build_dvla_file.apply_async( + [str(job.id)], + queue=QueueNames.JOBS + ) current_app.logger.info("send job {} to build-dvla-file in the {} queue".format(job.id, QueueNames.JOBS)) else: job.job_status = JOB_STATUS_FINISHED @@ -312,6 +322,14 @@ def save_letter( reply_to_text=service.get_default_letter_contact() ) + if ( + 'letters_as_pdf' in [p.permission for p in service.permissions] and not service.research_mode + ): + create_letters_pdf.apply_async( + [str(saved_notification.id)], + queue=QueueNames.JOBS + ) + current_app.logger.info("Letter {} created at {}".format(saved_notification.id, saved_notification.created_at)) except SQLAlchemyError as e: handle_exception(self, notification, notification_id, e) @@ -581,3 +599,57 @@ def process_incomplete_job(job_id): process_row(row_number, recipient, personalisation, template, job, job.service) job_complete(job, job.service, template.template_type, resumed=True) + + +@notify_celery.task(bind=True, name="create-letters-pdf", max_retries=15, default_retry_delay=300) +@statsd(namespace="tasks") +def create_letters_pdf(self, notification_id): + try: + notification = get_notification_by_id(notification_id) + if not notification: + raise NoResultFound() + + pdf_data = get_letters_pdf( + notification.template, + contact_block=notification.reply_to_text, + org_id=notification.service.dvla_organisation.id, + values=notification.personalisation + ) + 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=True, filedata=pdf_data) + except (RequestException, BotoClientError): + try: + current_app.logger.exception( + "Letters PDF notification creation for id: {} failed".format(notification_id) + ) + self.retry(queue=QueueNames.RETRY) + except self.MaxRetriesExceededError: + current_app.logger.exception( + "RETRY FAILED: task create_letters_pdf failed for notification {}".format(notification_id), + ) + update_notification_status_by_id(notification_id, 'technical-failure') + + +def get_letters_pdf(template, contact_block=None, org_id='001', values=None): + template_for_letter_print = { + "subject": template.subject, + "content": template.content + } + + data = { + 'letter_contact_block': contact_block, + 'template': template_for_letter_print, + 'values': values, + 'dvla_org_id': org_id, + } + 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() + + return resp.content diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index aceb525a0..f76eac84c 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -1436,7 +1436,6 @@ def test_process_incomplete_job_email(mocker, sample_email_template): def test_process_incomplete_job_letter(mocker, sample_letter_template): - mocker.patch('app.celery.tasks.s3.get_job_from_s3', return_value=load_example_csv('multiple_letter')) mock_letter_saver = mocker.patch('app.celery.tasks.save_letter.apply_async') mock_build_dvla = mocker.patch('app.celery.tasks.build_dvla_file.apply_async') @@ -1456,3 +1455,4 @@ def test_process_incomplete_job_letter(mocker, sample_letter_template): assert mock_build_dvla.called assert mock_letter_saver.call_count == 8 + \ No newline at end of file diff --git a/tests/app/conftest.py b/tests/app/conftest.py index 71b510767..3706494f2 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -299,6 +299,10 @@ def sample_template_without_email_permission(notify_db, notify_db_session): @pytest.fixture def sample_letter_template(sample_service_full_permissions): + # remove letters_as_pdf from fixture until we drop building of dvla files + from app.dao.service_permissions_dao import dao_remove_service_permission + dao_remove_service_permission(sample_service_full_permissions.id, 'letters_as_pdf') + return create_template(sample_service_full_permissions, template_type=LETTER_TYPE) From bad129c5e9c6e4c4342d1c2a7eb3c2613bea9a7f Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Thu, 7 Dec 2017 17:04:23 +0000 Subject: [PATCH 07/31] Add extra logging for upload_letters_pdf --- app/aws/s3.py | 3 +++ tests/app/celery/test_tasks.py | 1 - 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/app/aws/s3.py b/app/aws/s3.py index a64be797c..4551c8c72 100644 --- a/app/aws/s3.py +++ b/app/aws/s3.py @@ -99,3 +99,6 @@ def upload_letters_pdf(reference, crown, filedata): bucket_name=current_app.config['LETTERS_PDF_BUCKET_NAME'], file_location=upload_file_name ) + + current_app.logger.info("Uploading letters PDF {} to {}".format( + upload_file_name, current_app.config['LETTERS_PDF_BUCKET_NAME'])) diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index f76eac84c..481cfd41b 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -1455,4 +1455,3 @@ def test_process_incomplete_job_letter(mocker, sample_letter_template): assert mock_build_dvla.called assert mock_letter_saver.call_count == 8 - \ No newline at end of file From 6b118ec1ef25a4a69f6823cf5ef523eaec20a8db Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Fri, 8 Dec 2017 17:27:05 +0000 Subject: [PATCH 08/31] Add create-letters-pdf queue name --- app/config.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/config.py b/app/config.py index 27ba485fb..4ec4c3f75 100644 --- a/app/config.py +++ b/app/config.py @@ -30,6 +30,7 @@ class QueueNames(object): RETRY = 'retry-tasks' NOTIFY = 'notify-internal-tasks' PROCESS_FTP = 'process-ftp-tasks' + CREATE_LETTERS_PDF = 'create-letters-pdf' @staticmethod def all_queues(): @@ -44,6 +45,7 @@ class QueueNames(object): QueueNames.JOBS, QueueNames.RETRY, QueueNames.NOTIFY, + QueueNames.CREATE_LETTERS_PDF, ] From 9784ee438a3ab518faaceeeff2c80dec2cda501d Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Fri, 8 Dec 2017 17:32:18 +0000 Subject: [PATCH 09/31] Refactored code in tasks.py --- app/celery/tasks.py | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 08cd4a92e..9b4d2e398 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -132,10 +132,7 @@ def job_complete(job, service, template_type, resumed=False, start=None): if service.research_mode: update_job_to_sent_to_dvla.apply_async([str(job.id)], queue=QueueNames.RESEARCH_MODE) else: - build_dvla_file.apply_async( - [str(job.id)], - queue=QueueNames.JOBS - ) + build_dvla_file.apply_async([str(job.id)], queue=QueueNames.JOBS) current_app.logger.info("send job {} to build-dvla-file in the {} queue".format(job.id, QueueNames.JOBS)) else: job.job_status = JOB_STATUS_FINISHED @@ -146,7 +143,7 @@ def job_complete(job, service, template_type, resumed=False, start=None): if resumed: current_app.logger.info( - "Resumed Job {} completed at {}".format(job.id, job.created_at, start, finished) + "Resumed Job {} completed at {}".format(job.id, job.created_at) ) else: current_app.logger.info( @@ -327,7 +324,7 @@ def save_letter( ): create_letters_pdf.apply_async( [str(saved_notification.id)], - queue=QueueNames.JOBS + queue=QueueNames.CREATE_LETTERS_PDF ) current_app.logger.info("Letter {} created at {}".format(saved_notification.id, saved_notification.created_at)) @@ -631,7 +628,7 @@ def create_letters_pdf(self, notification_id): update_notification_status_by_id(notification_id, 'technical-failure') -def get_letters_pdf(template, contact_block=None, org_id='001', values=None): +def get_letters_pdf(template, contact_block, org_id, values=None): template_for_letter_print = { "subject": template.subject, "content": template.content From bc316dd1fe8ca384d70160d661b5cc3c3843ef5b Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Fri, 8 Dec 2017 17:32:57 +0000 Subject: [PATCH 10/31] Add reference as part of sample_letter_notification --- tests/app/conftest.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/app/conftest.py b/tests/app/conftest.py index 3706494f2..c8f06e8c4 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -603,7 +603,7 @@ def sample_letter_notification(sample_letter_template): 'address_line_6': 'A6', 'postcode': 'A_POST' } - return create_notification(sample_letter_template, personalisation=address) + return create_notification(sample_letter_template, reference='foo', personalisation=address) @pytest.fixture(scope='function') From 7970cd3d4e97bd45f260b62a7d7215c007a5ecb7 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Fri, 8 Dec 2017 17:33:13 +0000 Subject: [PATCH 11/31] Update test for queue names --- tests/app/test_config.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/app/test_config.py b/tests/app/test_config.py index 3a40b4db4..c023fa37a 100644 --- a/tests/app/test_config.py +++ b/tests/app/test_config.py @@ -63,7 +63,7 @@ def test_cloudfoundry_config_has_different_defaults(): def test_queue_names_all_queues_correct(): # Need to ensure that all_queues() only returns queue names used in API queues = QueueNames.all_queues() - assert len(queues) == 10 + assert len(queues) == 11 assert set([ QueueNames.PRIORITY, QueueNames.PERIODIC, @@ -74,5 +74,6 @@ def test_queue_names_all_queues_correct(): QueueNames.STATISTICS, QueueNames.JOBS, QueueNames.RETRY, - QueueNames.NOTIFY + QueueNames.NOTIFY, + QueueNames.CREATE_LETTERS_PDF ]) == set(queues) From aca52952d8db95a25a9101e0229302bc06600528 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Fri, 8 Dec 2017 17:40:00 +0000 Subject: [PATCH 12/31] Add tests for letters as pdf permission - test that `create_letters_pdf` only gets called when `letters_as_pdf` permission set - test that `build_dvla_file` does not get called when `letters_as_pdf` permission set - test creating the pdf call to notifications template preview service - test uploading the data to s3 calls upload_letters_pdf function --- tests/app/celery/test_tasks.py | 134 ++++++++++++++++++++++++++++++++- 1 file changed, 132 insertions(+), 2 deletions(-) diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 481cfd41b..7af5c0a06 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -9,8 +9,9 @@ from flask import current_app from freezegun import freeze_time from requests import RequestException from sqlalchemy.exc import SQLAlchemyError -from celery.exceptions import Retry +from celery.exceptions import Retry, MaxRetriesExceededError from botocore.exceptions import ClientError +from sqlalchemy.orm.exc import NoResultFound from notifications_utils.template import SMSMessageTemplate, WithSubjectTemplate, LetterDVLATemplate from app import (encryption, DATETIME_FORMAT) @@ -20,6 +21,9 @@ from app.celery.scheduled_tasks import check_job_status from app.celery.tasks import ( build_dvla_file, create_dvla_file_contents_for_job, + create_letters_pdf, + get_letters_pdf, + job_complete, process_job, process_row, save_sms, @@ -32,7 +36,7 @@ from app.celery.tasks import ( send_inbound_sms_to_service, ) from app.config import QueueNames -from app.dao import jobs_dao, services_dao +from app.dao import jobs_dao, services_dao, service_permissions_dao from app.models import ( Job, Notification, @@ -47,6 +51,7 @@ from app.models import ( SMS_TYPE ) +from tests.conftest import set_config_values from tests.app import load_example_csv from tests.app.conftest import ( sample_service as create_sample_service, @@ -94,6 +99,7 @@ def test_should_have_decorated_tasks_functions(): assert save_sms.__wrapped__.__name__ == 'save_sms' assert save_email.__wrapped__.__name__ == 'save_email' assert save_letter.__wrapped__.__name__ == 'save_letter' + assert create_letters_pdf.__wrapped__.__name__ == 'create_letters_pdf' @pytest.fixture @@ -1039,6 +1045,50 @@ def test_save_letter_saves_letter_to_database(mocker, notify_db_session): assert notification_db.reply_to_text == "Address contact" +def test_save_letter_saves_letter_calls_create_letters_as_pdf_with_letters_as_pdf_permission( + mocker, notify_db_session, sample_letter_job): + service_permissions_dao.dao_add_service_permission(sample_letter_job.service.id, 'letters_as_pdf') + mock_create_letters_pdf = mocker.patch('app.celery.tasks.create_letters_pdf.apply_async') + + personalisation = { + 'addressline1': 'Foo', + 'addressline2': 'Bar', + 'postcode': 'Flob', + } + notification_json = _notification_json( + template=sample_letter_job.template, + to='Foo', + personalisation=personalisation, + job_id=sample_letter_job.id, + row_number=1 + ) + notification_id = uuid.uuid4() + + save_letter( + sample_letter_job.service_id, + notification_id, + encryption.encrypt(notification_json), + ) + + assert mock_create_letters_pdf.called + mock_create_letters_pdf.assert_called_once_with( + [str(notification_id)], + queue=QueueNames.CREATE_LETTERS_PDF + ) + + +def test_job_complete_does_not_call_build_dvla_file_with_letters_as_pdf_permission( + mocker, notify_db_session, sample_letter_job): + service_permissions_dao.dao_add_service_permission(sample_letter_job.service.id, 'letters_as_pdf') + mock_build_dvla_files = mocker.patch('app.celery.tasks.build_dvla_file.apply_async') + + job_complete(sample_letter_job, sample_letter_job.service, sample_letter_job.template.template_type) + + assert not sample_letter_job.service.research_mode + assert not mock_build_dvla_files.called + assert sample_letter_job.job_status == JOB_STATUS_FINISHED + + def test_should_cancel_job_if_service_is_inactive(sample_service, sample_job, mocker): @@ -1455,3 +1505,83 @@ def test_process_incomplete_job_letter(mocker, sample_letter_template): assert mock_build_dvla.called assert mock_letter_saver.call_count == 8 + + +@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' + 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: + 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, org_id=dvla_org_id, values=personalisation) + + assert mock_post.last_request.json() == { + 'values': personalisation, + 'letter_contact_block': contact_block, + 'dvla_org_id': dvla_org_id, + 'template': { + 'subject': sample_letter_template.subject, + 'content': sample_letter_template.content + } + } + + +def test_create_letters_pdf_calls_upload_letters_pdf(mocker, sample_letter_notification): + mocker.patch('app.celery.tasks.get_letters_pdf', return_value=b'\x00\x01') + mock_s3 = mocker.patch('app.celery.tasks.s3.upload_letters_pdf') + + create_letters_pdf(sample_letter_notification.id) + + mock_s3.assert_called_with( + reference=sample_letter_notification.reference, + crown=True, + filedata=b'\x00\x01' + ) + + +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.tasks.get_letters_pdf', side_effect=RequestException) + mock_retry = mocker.patch('app.celery.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.tasks.get_letters_pdf') + mock_s3 = mocker.patch('app.celery.tasks.s3.upload_letters_pdf', side_effect=ClientError({}, 'operation_name')) + mock_retry = mocker.patch('app.celery.tasks.create_letters_pdf.retry') + + create_letters_pdf(sample_letter_notification.id) + + assert mock_s3.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.tasks.get_letters_pdf', side_effect=RequestException) + mock_retry = mocker.patch('app.celery.tasks.create_letters_pdf.retry', side_effect=MaxRetriesExceededError) + mock_update_noti = mocker.patch('app.celery.tasks.update_notification_status_by_id') + + create_letters_pdf(sample_letter_notification.id) + + assert mock_get_letters_pdf.called + assert mock_retry.called + assert mock_update_noti.called + mock_update_noti.assert_called_once_with(sample_letter_notification.id, 'technical-failure') From 3464336aff0b8d3d4ce12c218eb69a3a7b44eee4 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Mon, 11 Dec 2017 11:00:27 +0000 Subject: [PATCH 13/31] Refactored tasks.py to split out letters_pdf tasks - Added has_permission helper in models.py to check permission in service - Moved letters pdf tasks to separate file - Moved letters pdf tests to own file --- app/celery/letters_pdf_tasks.py | 68 +++++++++++++++ app/celery/tasks.py | 64 +------------- app/models.py | 3 + tests/app/celery/test_letters_pdf_tasks.py | 99 ++++++++++++++++++++++ tests/app/celery/test_tasks.py | 89 +------------------ 5 files changed, 176 insertions(+), 147 deletions(-) create mode 100644 app/celery/letters_pdf_tasks.py create mode 100644 tests/app/celery/test_letters_pdf_tasks.py diff --git a/app/celery/letters_pdf_tasks.py b/app/celery/letters_pdf_tasks.py new file mode 100644 index 000000000..69e18e645 --- /dev/null +++ b/app/celery/letters_pdf_tasks.py @@ -0,0 +1,68 @@ +from flask import current_app +from requests import ( + post as requests_post, + RequestException +) + +from botocore.exceptions import ClientError as BotoClientError +from sqlalchemy.orm.exc import NoResultFound + +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.statsd_decorators import statsd + + +@notify_celery.task(bind=True, name="create-letters-pdf", max_retries=15, default_retry_delay=300) +@statsd(namespace="tasks") +def create_letters_pdf(self, notification_id): + try: + notification = get_notification_by_id(notification_id) + if not notification: + raise NoResultFound() + + pdf_data = get_letters_pdf( + notification.template, + contact_block=notification.reply_to_text, + org_id=notification.service.dvla_organisation.id, + values=notification.personalisation + ) + 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=True, filedata=pdf_data) + except (RequestException, BotoClientError): + try: + current_app.logger.exception( + "Letters PDF notification creation for id: {} failed".format(notification_id) + ) + self.retry(queue=QueueNames.RETRY) + except self.MaxRetriesExceededError: + current_app.logger.exception( + "RETRY FAILED: task create_letters_pdf failed for notification {}".format(notification_id), + ) + update_notification_status_by_id(notification_id, 'technical-failure') + + +def get_letters_pdf(template, contact_block, org_id, values): + template_for_letter_print = { + "subject": template.subject, + "content": template.content + } + + data = { + 'letter_contact_block': contact_block, + 'template': template_for_letter_print, + 'values': values, + 'dvla_org_id': org_id, + } + 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() + + return resp.content diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 9b4d2e398..f1260a6f0 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -4,7 +4,6 @@ from collections import namedtuple from celery.signals import worker_process_shutdown from flask import current_app -import requests from notifications_utils.recipients import ( RecipientCSV @@ -20,7 +19,6 @@ from requests import ( RequestException ) from sqlalchemy.exc import SQLAlchemyError -from sqlalchemy.orm.exc import NoResultFound from botocore.exceptions import ClientError as BotoClientError from app import ( @@ -32,6 +30,7 @@ from app import ( ) from app.aws import s3 from app.celery import provider_tasks +from app.celery import letters_pdf_tasks from app.celery.service_callback_tasks import send_delivery_status_to_service from app.config import QueueNames from app.dao.inbound_sms_dao import dao_get_inbound_sms_by_id @@ -48,7 +47,6 @@ from app.dao.notifications_dao import ( dao_update_notifications_by_reference, dao_get_last_notification_added_for_job_id, dao_get_notifications_by_references, - update_notification_status_by_id ) from app.dao.provider_details_dao import get_current_provider from app.dao.service_inbound_api_dao import get_service_inbound_api_for_service @@ -127,7 +125,7 @@ def process_job(job_id): def job_complete(job, service, template_type, resumed=False, start=None): if ( template_type == LETTER_TYPE and - 'letters_as_pdf' not in [p.permission for p in service.permissions] + not service.has_permission('letters_as_pdf') ): if service.research_mode: update_job_to_sent_to_dvla.apply_async([str(job.id)], queue=QueueNames.RESEARCH_MODE) @@ -320,9 +318,9 @@ def save_letter( ) if ( - 'letters_as_pdf' in [p.permission for p in service.permissions] and not service.research_mode + service.has_permission('letters_as_pdf') and not service.research_mode ): - create_letters_pdf.apply_async( + letters_pdf_tasks.create_letters_pdf.apply_async( [str(saved_notification.id)], queue=QueueNames.CREATE_LETTERS_PDF ) @@ -596,57 +594,3 @@ def process_incomplete_job(job_id): process_row(row_number, recipient, personalisation, template, job, job.service) job_complete(job, job.service, template.template_type, resumed=True) - - -@notify_celery.task(bind=True, name="create-letters-pdf", max_retries=15, default_retry_delay=300) -@statsd(namespace="tasks") -def create_letters_pdf(self, notification_id): - try: - notification = get_notification_by_id(notification_id) - if not notification: - raise NoResultFound() - - pdf_data = get_letters_pdf( - notification.template, - contact_block=notification.reply_to_text, - org_id=notification.service.dvla_organisation.id, - values=notification.personalisation - ) - 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=True, filedata=pdf_data) - except (RequestException, BotoClientError): - try: - current_app.logger.exception( - "Letters PDF notification creation for id: {} failed".format(notification_id) - ) - self.retry(queue=QueueNames.RETRY) - except self.MaxRetriesExceededError: - current_app.logger.exception( - "RETRY FAILED: task create_letters_pdf failed for notification {}".format(notification_id), - ) - update_notification_status_by_id(notification_id, 'technical-failure') - - -def get_letters_pdf(template, contact_block, org_id, values=None): - template_for_letter_print = { - "subject": template.subject, - "content": template.content - } - - data = { - 'letter_contact_block': contact_block, - 'template': template_for_letter_print, - 'values': values, - 'dvla_org_id': org_id, - } - 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() - - return resp.content diff --git a/app/models.py b/app/models.py index bd016b4d0..2f196c7a5 100644 --- a/app/models.py +++ b/app/models.py @@ -283,6 +283,9 @@ class Service(db.Model, Versioned): default_letter_contact = [x for x in self.letter_contacts if x.is_default] return default_letter_contact[0].contact_block if default_letter_contact else None + def has_permission(self, permission): + return permission in [p.permission for p in self.permissions] + class AnnualBilling(db.Model): __tablename__ = "annual_billing" diff --git a/tests/app/celery/test_letters_pdf_tasks.py b/tests/app/celery/test_letters_pdf_tasks.py new file mode 100644 index 000000000..27f727571 --- /dev/null +++ b/tests/app/celery/test_letters_pdf_tasks.py @@ -0,0 +1,99 @@ +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.celery.letters_pdf_tasks import ( + create_letters_pdf, + get_letters_pdf, +) + +from tests.conftest import set_config_values + + +def test_should_have_decorated_tasks_functions(): + assert create_letters_pdf.__wrapped__.__name__ == 'create_letters_pdf' + + +@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' + 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: + 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, org_id=dvla_org_id, values=personalisation) + + assert mock_post.last_request.json() == { + 'values': personalisation, + 'letter_contact_block': contact_block, + 'dvla_org_id': dvla_org_id, + 'template': { + 'subject': sample_letter_template.subject, + 'content': sample_letter_template.content + } + } + + +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') + mock_s3 = mocker.patch('app.celery.tasks.s3.upload_letters_pdf') + + create_letters_pdf(sample_letter_notification.id) + + mock_s3.assert_called_with( + reference=sample_letter_notification.reference, + crown=True, + filedata=b'\x00\x01' + ) + + +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) + 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') + 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') + + create_letters_pdf(sample_letter_notification.id) + + assert mock_s3.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_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) + + assert mock_get_letters_pdf.called + assert mock_retry.called + assert mock_update_noti.called + mock_update_noti.assert_called_once_with(sample_letter_notification.id, 'technical-failure') diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 7af5c0a06..ae892291d 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -9,9 +9,8 @@ from flask import current_app from freezegun import freeze_time from requests import RequestException from sqlalchemy.exc import SQLAlchemyError -from celery.exceptions import Retry, MaxRetriesExceededError +from celery.exceptions import Retry from botocore.exceptions import ClientError -from sqlalchemy.orm.exc import NoResultFound from notifications_utils.template import SMSMessageTemplate, WithSubjectTemplate, LetterDVLATemplate from app import (encryption, DATETIME_FORMAT) @@ -21,8 +20,6 @@ from app.celery.scheduled_tasks import check_job_status from app.celery.tasks import ( build_dvla_file, create_dvla_file_contents_for_job, - create_letters_pdf, - get_letters_pdf, job_complete, process_job, process_row, @@ -51,7 +48,6 @@ from app.models import ( SMS_TYPE ) -from tests.conftest import set_config_values from tests.app import load_example_csv from tests.app.conftest import ( sample_service as create_sample_service, @@ -99,7 +95,6 @@ def test_should_have_decorated_tasks_functions(): assert save_sms.__wrapped__.__name__ == 'save_sms' assert save_email.__wrapped__.__name__ == 'save_email' assert save_letter.__wrapped__.__name__ == 'save_letter' - assert create_letters_pdf.__wrapped__.__name__ == 'create_letters_pdf' @pytest.fixture @@ -1048,7 +1043,7 @@ def test_save_letter_saves_letter_to_database(mocker, notify_db_session): def test_save_letter_saves_letter_calls_create_letters_as_pdf_with_letters_as_pdf_permission( mocker, notify_db_session, sample_letter_job): service_permissions_dao.dao_add_service_permission(sample_letter_job.service.id, 'letters_as_pdf') - mock_create_letters_pdf = mocker.patch('app.celery.tasks.create_letters_pdf.apply_async') + mock_create_letters_pdf = mocker.patch('app.celery.letters_pdf_tasks.create_letters_pdf.apply_async') personalisation = { 'addressline1': 'Foo', @@ -1505,83 +1500,3 @@ def test_process_incomplete_job_letter(mocker, sample_letter_template): assert mock_build_dvla.called assert mock_letter_saver.call_count == 8 - - -@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' - 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: - 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, org_id=dvla_org_id, values=personalisation) - - assert mock_post.last_request.json() == { - 'values': personalisation, - 'letter_contact_block': contact_block, - 'dvla_org_id': dvla_org_id, - 'template': { - 'subject': sample_letter_template.subject, - 'content': sample_letter_template.content - } - } - - -def test_create_letters_pdf_calls_upload_letters_pdf(mocker, sample_letter_notification): - mocker.patch('app.celery.tasks.get_letters_pdf', return_value=b'\x00\x01') - mock_s3 = mocker.patch('app.celery.tasks.s3.upload_letters_pdf') - - create_letters_pdf(sample_letter_notification.id) - - mock_s3.assert_called_with( - reference=sample_letter_notification.reference, - crown=True, - filedata=b'\x00\x01' - ) - - -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.tasks.get_letters_pdf', side_effect=RequestException) - mock_retry = mocker.patch('app.celery.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.tasks.get_letters_pdf') - mock_s3 = mocker.patch('app.celery.tasks.s3.upload_letters_pdf', side_effect=ClientError({}, 'operation_name')) - mock_retry = mocker.patch('app.celery.tasks.create_letters_pdf.retry') - - create_letters_pdf(sample_letter_notification.id) - - assert mock_s3.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.tasks.get_letters_pdf', side_effect=RequestException) - mock_retry = mocker.patch('app.celery.tasks.create_letters_pdf.retry', side_effect=MaxRetriesExceededError) - mock_update_noti = mocker.patch('app.celery.tasks.update_notification_status_by_id') - - create_letters_pdf(sample_letter_notification.id) - - assert mock_get_letters_pdf.called - assert mock_retry.called - assert mock_update_noti.called - mock_update_noti.assert_called_once_with(sample_letter_notification.id, 'technical-failure') From 02b67c95e5a110c6be06559b637d6488adf4ebc7 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Mon, 11 Dec 2017 16:12:07 +0000 Subject: [PATCH 14/31] Use crown flag from service --- app/celery/letters_pdf_tasks.py | 2 +- tests/app/celery/test_letters_pdf_tasks.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/celery/letters_pdf_tasks.py b/app/celery/letters_pdf_tasks.py index 69e18e645..5b675f34b 100644 --- a/app/celery/letters_pdf_tasks.py +++ b/app/celery/letters_pdf_tasks.py @@ -30,7 +30,7 @@ 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=True, filedata=pdf_data) + s3.upload_letters_pdf(reference=notification.reference, crown=notification.service.crown, filedata=pdf_data) except (RequestException, BotoClientError): try: current_app.logger.exception( diff --git a/tests/app/celery/test_letters_pdf_tasks.py b/tests/app/celery/test_letters_pdf_tasks.py index 27f727571..0af5c78ab 100644 --- a/tests/app/celery/test_letters_pdf_tasks.py +++ b/tests/app/celery/test_letters_pdf_tasks.py @@ -54,7 +54,7 @@ def test_create_letters_pdf_calls_upload_letters_pdf(mocker, sample_letter_notif mock_s3.assert_called_with( reference=sample_letter_notification.reference, - crown=True, + crown=sample_letter_notification.service.crown, filedata=b'\x00\x01' ) From 3e71e9a294cfc2ef5e030f282b5469ca5a1f7291 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Mon, 11 Dec 2017 16:13:41 +0000 Subject: [PATCH 15/31] Update save_letter to handle research mode --- app/celery/tasks.py | 18 ++++++++++------- tests/app/celery/test_tasks.py | 37 +++++++++++++++++++++++++++++++++- 2 files changed, 47 insertions(+), 8 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index f1260a6f0..8657cfb5a 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -317,13 +317,17 @@ def save_letter( reply_to_text=service.get_default_letter_contact() ) - if ( - service.has_permission('letters_as_pdf') and not service.research_mode - ): - letters_pdf_tasks.create_letters_pdf.apply_async( - [str(saved_notification.id)], - queue=QueueNames.CREATE_LETTERS_PDF - ) + if service.has_permission('letters_as_pdf'): + if not service.research_mode: + letters_pdf_tasks.create_letters_pdf.apply_async( + [str(saved_notification.id)], + queue=QueueNames.CREATE_LETTERS_PDF + ) + else: + update_letter_notifications_to_sent_to_dvla.apply_async( + kwargs={'notification_references': [saved_notification.reference]}, + queue=QueueNames.RESEARCH_MODE + ) current_app.logger.info("Letter {} created at {}".format(saved_notification.id, saved_notification.created_at)) except SQLAlchemyError as e: diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index ae892291d..a5837b3a2 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -1040,7 +1040,42 @@ def test_save_letter_saves_letter_to_database(mocker, notify_db_session): assert notification_db.reply_to_text == "Address contact" -def test_save_letter_saves_letter_calls_create_letters_as_pdf_with_letters_as_pdf_permission( +def test_save_letter_calls_update_noti_to_sent_task_with_letters_as_pdf_permission_in_research_mode( + mocker, notify_db_session, sample_letter_job): + sample_letter_job.service.research_mode = True + service_permissions_dao.dao_add_service_permission(sample_letter_job.service.id, 'letters_as_pdf') + mock_update_letter_noti_sent = mocker.patch( + 'app.celery.tasks.update_letter_notifications_to_sent_to_dvla.apply_async') + mocker.patch('app.celery.tasks.create_random_identifier', return_value="this-is-random-in-real-life") + + personalisation = { + 'addressline1': 'Foo', + 'addressline2': 'Bar', + 'postcode': 'Flob', + } + notification_json = _notification_json( + template=sample_letter_job.template, + to='Foo', + personalisation=personalisation, + job_id=sample_letter_job.id, + row_number=1 + ) + notification_id = uuid.uuid4() + + save_letter( + sample_letter_job.service_id, + notification_id, + encryption.encrypt(notification_json), + ) + + assert mock_update_letter_noti_sent.called + mock_update_letter_noti_sent.assert_called_once_with( + kwargs={'notification_references': ['this-is-random-in-real-life']}, + queue=QueueNames.RESEARCH_MODE + ) + + +def test_save_letter_calls_create_letters_pdf_task_with_letters_as_pdf_permission_and_not_in_research( mocker, notify_db_session, sample_letter_job): service_permissions_dao.dao_add_service_permission(sample_letter_job.service.id, 'letters_as_pdf') mock_create_letters_pdf = mocker.patch('app.celery.letters_pdf_tasks.create_letters_pdf.apply_async') From d0fcfa7d0cee022f109e80a63f824412da0bf77d Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Mon, 11 Dec 2017 16:17:07 +0000 Subject: [PATCH 16/31] Update sample_service_full_perms to leave out letters_as_pdf permissions - as letters_as_pdf is a temporary service permission until the build dvla file process is deprecated rather than update all the tests to remove the permission so that they pass, lets remove it here instead --- tests/app/conftest.py | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/tests/app/conftest.py b/tests/app/conftest.py index c8f06e8c4..e51aedd7d 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -161,7 +161,7 @@ def sample_service( 'message_limit': limit, 'restricted': restricted, 'email_from': email_from, - 'created_by': user + 'created_by': user, } service = Service.query.filter_by(name=service_name).first() if not service: @@ -188,7 +188,7 @@ def sample_service_full_permissions(notify_db, notify_db_session): notify_db_session, # ensure name doesn't clash with regular sample service service_name="sample service full permissions", - permissions=SERVICE_PERMISSION_TYPES + permissions=set(SERVICE_PERMISSION_TYPES) - {'letters_as_pdf'} ) @@ -299,10 +299,6 @@ def sample_template_without_email_permission(notify_db, notify_db_session): @pytest.fixture def sample_letter_template(sample_service_full_permissions): - # remove letters_as_pdf from fixture until we drop building of dvla files - from app.dao.service_permissions_dao import dao_remove_service_permission - dao_remove_service_permission(sample_service_full_permissions.id, 'letters_as_pdf') - return create_template(sample_service_full_permissions, template_type=LETTER_TYPE) From f42df8af730a0b6a696f800bda82ee014a439d77 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Mon, 11 Dec 2017 16:20:31 +0000 Subject: [PATCH 17/31] Ignore services that have letters_as_pdf permission - We don't want to process the letters as a build file for api calls when the service is generating letters as pdf. --- app/dao/notifications_dao.py | 18 ++++++++++++++- .../test_letter_api_notification_dao.py | 22 +++++++++++++++++-- 2 files changed, 37 insertions(+), 3 deletions(-) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index d9886fcfd..1ecef5f7c 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -26,6 +26,8 @@ from app.models import ( Notification, NotificationHistory, ScheduledNotification, + Service, + ServicePermission, Template, TemplateHistory, KEY_TYPE_NORMAL, @@ -541,14 +543,28 @@ def dao_set_created_live_letter_api_notifications_to_pending(): this is used in the run_scheduled_jobs task, so we put a FOR UPDATE lock on the job table for the duration of the transaction so that if the task is run more than once concurrently, one task will block the other select from completing until it commits. + + Note - do not process services that have letters_as_pdf permission as they + will get processed when the letters PDF zip task is created """ + + # Ignore services that have letters_as_pdf permission + services_without_letters_as_pdf = [ + s.id for s in Service.query.filter( + ~Service.permissions.any( + ServicePermission.permission == 'letters_as_pdf' + ) + ).all() + ] + notifications = db.session.query( Notification ).filter( Notification.notification_type == LETTER_TYPE, Notification.status == NOTIFICATION_CREATED, Notification.key_type == KEY_TYPE_NORMAL, - Notification.api_key != None # noqa + Notification.api_key != None, # noqa + Notification.service_id.in_(services_without_letters_as_pdf) ).with_for_update( ).all() diff --git a/tests/app/dao/notification_dao/test_letter_api_notification_dao.py b/tests/app/dao/notification_dao/test_letter_api_notification_dao.py index 4a624f3eb..96f79ceff 100644 --- a/tests/app/dao/notification_dao/test_letter_api_notification_dao.py +++ b/tests/app/dao/notification_dao/test_letter_api_notification_dao.py @@ -1,13 +1,15 @@ from app.models import ( + Notification, NOTIFICATION_CREATED, NOTIFICATION_PENDING, NOTIFICATION_SENDING, KEY_TYPE_TEST, - KEY_TYPE_NORMAL + KEY_TYPE_NORMAL, + LETTER_TYPE ) from app.dao.notifications_dao import dao_set_created_live_letter_api_notifications_to_pending -from tests.app.db import create_notification +from tests.app.db import create_notification, create_service, create_template def test_should_only_get_letter_notifications( @@ -23,6 +25,22 @@ def test_should_only_get_letter_notifications( assert ret == [sample_letter_notification] +def test_should_ignore_letters_as_pdf( + sample_letter_notification, +): + service = create_service(service_permissions=[LETTER_TYPE, 'letters_as_pdf']) + template = create_template(service, template_type=LETTER_TYPE) + create_notification(template) + + all_noti = Notification.query.all() + assert len(all_noti) == 2 + + ret = dao_set_created_live_letter_api_notifications_to_pending() + + assert sample_letter_notification.status == NOTIFICATION_PENDING + assert ret == [sample_letter_notification] + + def test_should_only_get_created_letters(sample_letter_template): created_noti = create_notification(sample_letter_template, status=NOTIFICATION_CREATED) create_notification(sample_letter_template, status=NOTIFICATION_PENDING) From ab72d6f555fa25226e82a531bf9d2a6f692f2822 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Mon, 11 Dec 2017 16:23:09 +0000 Subject: [PATCH 18/31] Call the create letters pdf task in api calls for services with letters as pdf --- app/v2/notifications/post_notifications.py | 7 ++++++ .../test_post_letter_notifications.py | 25 +++++++++++++++++++ 2 files changed, 32 insertions(+) diff --git a/app/v2/notifications/post_notifications.py b/app/v2/notifications/post_notifications.py index 08847f2cb..ed9d67c7f 100644 --- a/app/v2/notifications/post_notifications.py +++ b/app/v2/notifications/post_notifications.py @@ -14,6 +14,7 @@ from app.models import ( NOTIFICATION_CREATED, NOTIFICATION_SENDING ) +from app.celery.letters_pdf_tasks import create_letters_pdf from app.celery.tasks import update_letter_notifications_to_sent_to_dvla from app.notifications.process_notifications import ( persist_notification, @@ -187,6 +188,12 @@ def process_letter_notification(*, letter_data, api_key, template): queue=QueueNames.RESEARCH_MODE ) + if api_key.service.has_permission('letters_as_pdf'): + create_letters_pdf.apply_async( + [str(notification.id)], + queue=QueueNames.CREATE_LETTERS_PDF + ) + return notification diff --git a/tests/app/v2/notifications/test_post_letter_notifications.py b/tests/app/v2/notifications/test_post_letter_notifications.py index d6eca3c8d..b433f83cd 100644 --- a/tests/app/v2/notifications/test_post_letter_notifications.py +++ b/tests/app/v2/notifications/test_post_letter_notifications.py @@ -4,6 +4,8 @@ from flask import json from flask import url_for import pytest +from app.config import QueueNames +from app.dao import service_permissions_dao from app.models import EMAIL_TYPE from app.models import Job from app.models import KEY_TYPE_NORMAL @@ -80,6 +82,29 @@ def test_post_letter_notification_returns_201(client, sample_letter_template, mo assert not notification.reply_to_text +def test_post_letter_notification_for_letters_as_pdf_calls_celery_task(client, sample_letter_template, mocker): + service_permissions_dao.dao_add_service_permission(sample_letter_template.service.id, 'letters_as_pdf') + + data = { + 'template_id': str(sample_letter_template.id), + 'personalisation': { + 'address_line_1': 'Her Royal Highness Queen Elizabeth II', + 'address_line_2': 'Buckingham Palace', + 'address_line_3': 'London', + 'postcode': 'SW1 1AA', + 'name': 'Lizzie' + }, + 'reference': 'foo' + } + fake_task = mocker.patch('app.celery.tasks.letters_pdf_tasks.create_letters_pdf.apply_async') + + letter_request(client, data, service_id=sample_letter_template.service_id) + + notification = Notification.query.one() + + fake_task.assert_called_once_with([str(notification.id)], queue=QueueNames.CREATE_LETTERS_PDF) + + def test_post_letter_notification_returns_400_and_missing_template( client, sample_service_full_permissions From da93ba296eb58334912da113e5905317dc73716e Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Tue, 12 Dec 2017 11:56:42 +0000 Subject: [PATCH 19/31] Refactored notifications_dao - Introduce a `_raise` flag for `get_notification_by_id` so that sql alchemy will raise the NoResults error rather than the app - Refactor `dao_set_created_live_letter_api_notifications_to_pending` to use a join for getting services that don't have `letters_as_pdf` as marginally faster. --- app/celery/letters_pdf_tasks.py | 5 +--- app/dao/notifications_dao.py | 24 +++++++++---------- .../test_letter_api_notification_dao.py | 4 +--- 3 files changed, 13 insertions(+), 20 deletions(-) diff --git a/app/celery/letters_pdf_tasks.py b/app/celery/letters_pdf_tasks.py index 5b675f34b..24c7347ae 100644 --- a/app/celery/letters_pdf_tasks.py +++ b/app/celery/letters_pdf_tasks.py @@ -5,7 +5,6 @@ from requests import ( ) from botocore.exceptions import ClientError as BotoClientError -from sqlalchemy.orm.exc import NoResultFound from app import notify_celery from app.aws import s3 @@ -18,9 +17,7 @@ from app.statsd_decorators import statsd @statsd(namespace="tasks") def create_letters_pdf(self, notification_id): try: - notification = get_notification_by_id(notification_id) - if not notification: - raise NoResultFound() + notification = get_notification_by_id(notification_id, _raise=True) pdf_data = get_letters_pdf( notification.template, diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 1ecef5f7c..29b49ba7c 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -226,8 +226,11 @@ def get_notification_with_personalisation(service_id, notification_id, key_type) @statsd(namespace="dao") -def get_notification_by_id(notification_id): - return Notification.query.filter_by(id=notification_id).first() +def get_notification_by_id(notification_id, _raise=False): + if _raise: + return Notification.query.filter_by(id=notification_id).one() + else: + return Notification.query.filter_by(id=notification_id).first() def get_notifications(filter_dict=None): @@ -547,24 +550,19 @@ def dao_set_created_live_letter_api_notifications_to_pending(): Note - do not process services that have letters_as_pdf permission as they will get processed when the letters PDF zip task is created """ - - # Ignore services that have letters_as_pdf permission - services_without_letters_as_pdf = [ - s.id for s in Service.query.filter( - ~Service.permissions.any( - ServicePermission.permission == 'letters_as_pdf' - ) - ).all() - ] - notifications = db.session.query( Notification + ).join( + Service ).filter( Notification.notification_type == LETTER_TYPE, Notification.status == NOTIFICATION_CREATED, Notification.key_type == KEY_TYPE_NORMAL, Notification.api_key != None, # noqa - Notification.service_id.in_(services_without_letters_as_pdf) + # Ignore services that have letters_as_pdf permission + ~Service.permissions.any( + ServicePermission.permission == 'letters_as_pdf' + ) ).with_for_update( ).all() diff --git a/tests/app/dao/notification_dao/test_letter_api_notification_dao.py b/tests/app/dao/notification_dao/test_letter_api_notification_dao.py index 96f79ceff..c52de01e1 100644 --- a/tests/app/dao/notification_dao/test_letter_api_notification_dao.py +++ b/tests/app/dao/notification_dao/test_letter_api_notification_dao.py @@ -25,9 +25,7 @@ def test_should_only_get_letter_notifications( assert ret == [sample_letter_notification] -def test_should_ignore_letters_as_pdf( - sample_letter_notification, -): +def test_should_ignore_letters_as_pdf(sample_letter_notification): service = create_service(service_permissions=[LETTER_TYPE, 'letters_as_pdf']) template = create_template(service, template_type=LETTER_TYPE) create_notification(template) From ebfd78f3cf05913dcc3b84cde0bc6e4e5e6be4db Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Tue, 12 Dec 2017 14:53:38 +0000 Subject: [PATCH 20/31] Add template preview host url and key to cf config --- app/cloudfoundry_config.py | 7 +++++++ app/config.py | 1 + manifest-delivery-base.yml | 1 + manifest-delivery-preview.yml | 1 + manifest-delivery-staging.yml | 1 + tests/app/test_cloudfoundry_config.py | 25 +++++++++++++++++++++++-- 6 files changed, 34 insertions(+), 2 deletions(-) diff --git a/app/cloudfoundry_config.py b/app/cloudfoundry_config.py index 4244823a8..45f4eeee7 100644 --- a/app/cloudfoundry_config.py +++ b/app/cloudfoundry_config.py @@ -36,6 +36,8 @@ def set_config_env_vars(vcap_services): extract_redis_config(s) elif s['name'] == 'performance-platform': extract_performance_platform_config(s) + elif s['name'] == 'notify-template-preview': + extract_template_preview_config(s) def extract_notify_config(notify_config): @@ -77,3 +79,8 @@ def extract_firetext_config(firetext_config): def extract_redis_config(redis_config): os.environ['REDIS_ENABLED'] = redis_config['credentials']['redis_enabled'] os.environ['REDIS_URL'] = redis_config['credentials']['redis_url'] + + +def extract_template_preview_config(template_preview_config): + os.environ['TEMPLATE_PREVIEW_API_HOST'] = template_preview_config['credentials']['api_host'] + os.environ['TEMPLATE_PREVIEW_API_KEY'] = template_preview_config['credentials']['api_key'] diff --git a/app/config.py b/app/config.py index 4ec4c3f75..b0c73e122 100644 --- a/app/config.py +++ b/app/config.py @@ -374,6 +374,7 @@ class Test(Config): SMS_INBOUND_WHITELIST = ['203.0.113.195'] FIRETEXT_INBOUND_SMS_AUTH = ['testkey'] + TEMPLATE_PREVIEW_API_HOST = 'http://localhost:9999' class Preview(Config): diff --git a/manifest-delivery-base.yml b/manifest-delivery-base.yml index 6b1fc5094..1f09a01dd 100644 --- a/manifest-delivery-base.yml +++ b/manifest-delivery-base.yml @@ -7,6 +7,7 @@ services: - notify-aws - notify-config - notify-db + - notify-template-preview - mmg - firetext - hosted-graphite diff --git a/manifest-delivery-preview.yml b/manifest-delivery-preview.yml index ec8203cac..8de718214 100644 --- a/manifest-delivery-preview.yml +++ b/manifest-delivery-preview.yml @@ -6,6 +6,7 @@ services: - notify-aws - notify-config - notify-db + - notify-template-preview - mmg - firetext - hosted-graphite diff --git a/manifest-delivery-staging.yml b/manifest-delivery-staging.yml index 5c7c1dea7..7e7f37c0e 100644 --- a/manifest-delivery-staging.yml +++ b/manifest-delivery-staging.yml @@ -6,6 +6,7 @@ services: - notify-aws - notify-config - notify-db + - notify-template-preview - mmg - firetext - hosted-graphite diff --git a/tests/app/test_cloudfoundry_config.py b/tests/app/test_cloudfoundry_config.py index dabfc1e83..cfa8f8875 100644 --- a/tests/app/test_cloudfoundry_config.py +++ b/tests/app/test_cloudfoundry_config.py @@ -101,6 +101,17 @@ def performance_platform_config(): } +@pytest.fixture +def template_preview_config(): + return { + 'name': 'notify-template-preview', + 'credentials': { + 'api_host': 'template-preview api host', + 'api_key': 'template-preview api key' + } + } + + @pytest.fixture def cloudfoundry_config( postgres_config, @@ -110,7 +121,8 @@ def cloudfoundry_config( mmg_config, firetext_config, redis_config, - performance_platform_config + performance_platform_config, + template_preview_config ): return { 'postgres': postgres_config, @@ -121,7 +133,8 @@ def cloudfoundry_config( mmg_config, firetext_config, redis_config, - performance_platform_config + performance_platform_config, + template_preview_config ] } @@ -227,3 +240,11 @@ def test_performance_platform_config(): 'foo': 'my_token', 'bar': 'other_token' } + + +@pytest.mark.usefixtures('os_environ', 'cloudfoundry_environ') +def test_template_preview_config(): + extract_cloudfoundry_config() + + assert os.environ['TEMPLATE_PREVIEW_API_HOST'] == 'template-preview api host' + assert os.environ['TEMPLATE_PREVIEW_API_KEY'] == 'template-preview api key' From 689ece75a05ab16b9980fee840135a2dfcb86087 Mon Sep 17 00:00:00 2001 From: venusbb Date: Wed, 13 Dec 2017 10:03:40 +0000 Subject: [PATCH 21/31] db version change --- ...e_fragments.py => 0152_kill_service_free_fragments.py} | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) rename migrations/versions/{0151_kill_service_free_fragments.py => 0152_kill_service_free_fragments.py} (78%) diff --git a/migrations/versions/0151_kill_service_free_fragments.py b/migrations/versions/0152_kill_service_free_fragments.py similarity index 78% rename from migrations/versions/0151_kill_service_free_fragments.py rename to migrations/versions/0152_kill_service_free_fragments.py index ecc3c126d..038b978da 100644 --- a/migrations/versions/0151_kill_service_free_fragments.py +++ b/migrations/versions/0152_kill_service_free_fragments.py @@ -1,15 +1,15 @@ """ -Revision ID: 0151_kill_service_free_fragments -Revises: 0150_another_letter_org +Revision ID: 0152_kill_service_free_fragments +Revises: 0151_refactor_letter_rates Create Date: 2017-12-01 16:49:51.178455 """ from alembic import op import sqlalchemy as sa -revision = '0151_kill_service_free_fragments' -down_revision = '0150_another_letter_org' +revision = '0152_kill_service_free_fragments' +down_revision = '0151_refactor_letter_rates' def upgrade(): From 11152ab117382ec2da4a14ca6373da0e7a7762dd Mon Sep 17 00:00:00 2001 From: venusbb Date: Wed, 13 Dec 2017 10:57:08 +0000 Subject: [PATCH 22/31] Add new queue for callbacks --- app/config.py | 2 ++ manifest-delivery-base.yml | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/app/config.py b/app/config.py index b0c73e122..f2d37a769 100644 --- a/app/config.py +++ b/app/config.py @@ -31,6 +31,7 @@ class QueueNames(object): NOTIFY = 'notify-internal-tasks' PROCESS_FTP = 'process-ftp-tasks' CREATE_LETTERS_PDF = 'create-letters-pdf' + CALLBACKS = 'service-callbacks' @staticmethod def all_queues(): @@ -46,6 +47,7 @@ class QueueNames(object): QueueNames.RETRY, QueueNames.NOTIFY, QueueNames.CREATE_LETTERS_PDF, + QueueNames.CALLBACKS, ] diff --git a/manifest-delivery-base.yml b/manifest-delivery-base.yml index 1f09a01dd..2d784c3f6 100644 --- a/manifest-delivery-base.yml +++ b/manifest-delivery-base.yml @@ -56,6 +56,6 @@ applications: NOTIFY_APP_NAME: delivery-worker - name: notify-delivery-worker-receipts - command: scripts/run_app_paas.sh celery -A run_celery.notify_celery worker --loglevel=INFO --concurrency=11 -Q ses-callbacks + command: scripts/run_app_paas.sh celery -A run_celery.notify_celery worker --loglevel=INFO --concurrency=11 -Q ses-callbacks, service-callbacks env: NOTIFY_APP_NAME: delivery-worker-receipts From fdf97fd8b5266a334abaf6d10e03ff271b7b6405 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Wed, 13 Dec 2017 11:34:05 +0000 Subject: [PATCH 23/31] Add create-letters-pdf-tasks to notify-delivery-worker --- app/config.py | 2 +- manifest-delivery-base.yml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/config.py b/app/config.py index b0c73e122..1ef10ba6f 100644 --- a/app/config.py +++ b/app/config.py @@ -30,7 +30,7 @@ class QueueNames(object): RETRY = 'retry-tasks' NOTIFY = 'notify-internal-tasks' PROCESS_FTP = 'process-ftp-tasks' - CREATE_LETTERS_PDF = 'create-letters-pdf' + CREATE_LETTERS_PDF = 'create-letters-pdf-tasks' @staticmethod def all_queues(): diff --git a/manifest-delivery-base.yml b/manifest-delivery-base.yml index 1f09a01dd..d8cde3454 100644 --- a/manifest-delivery-base.yml +++ b/manifest-delivery-base.yml @@ -51,7 +51,7 @@ applications: NOTIFY_APP_NAME: delivery-worker-priority - name: notify-delivery-worker - command: scripts/run_app_paas.sh celery -A run_celery.notify_celery worker --loglevel=INFO --concurrency=11 -Q job-tasks,retry-tasks,notify-internal-tasks + command: scripts/run_app_paas.sh celery -A run_celery.notify_celery worker --loglevel=INFO --concurrency=11 -Q job-tasks,retry-tasks,notify-internal-tasks,create-letters-pdf-tasks env: NOTIFY_APP_NAME: delivery-worker From cef8d6e29425a583c45a216e4752730e4dd5e53b Mon Sep 17 00:00:00 2001 From: venusbb Date: Wed, 13 Dec 2017 11:55:08 +0000 Subject: [PATCH 24/31] use callback queue --- app/celery/scheduled_tasks.py | 2 +- app/celery/tasks.py | 4 ++-- app/notifications/notifications_ses_callback.py | 2 +- app/notifications/process_client_response.py | 2 +- tests/app/celery/test_ftp_update_tasks.py | 4 ++-- tests/app/notifications/test_notifications_ses_callback.py | 2 +- tests/app/notifications/test_process_client_response.py | 2 +- tests/app/test_config.py | 5 +++-- 8 files changed, 12 insertions(+), 11 deletions(-) diff --git a/app/celery/scheduled_tasks.py b/app/celery/scheduled_tasks.py index ed14be2e4..7b28b0fb4 100644 --- a/app/celery/scheduled_tasks.py +++ b/app/celery/scheduled_tasks.py @@ -199,7 +199,7 @@ def timeout_notifications(): service_callback_api = get_service_callback_api_for_service(service_id=notification.service_id) if service_callback_api: - send_delivery_status_to_service.apply_async([str(id)], queue=QueueNames.NOTIFY) + send_delivery_status_to_service.apply_async([str(id)], queue=QueueNames.CALLBACKS) current_app.logger.info( "Timeout period reached for {} notifications, status has been updated.".format(len(notifications))) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 8657cfb5a..7460f3b90 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -416,7 +416,7 @@ def update_letter_notifications_to_error(self, notification_references): service_callback_api = get_service_callback_api_for_service(service_id=notifications[0].service_id) if service_callback_api: for notification in notifications: - send_delivery_status_to_service.apply_async([str(notification.id)], queue=QueueNames.NOTIFY) + send_delivery_status_to_service.apply_async([str(notification.id)], queue=QueueNames.CALLBACKS) def create_dvla_file_contents_for_job(job_id): @@ -503,7 +503,7 @@ def update_letter_notifications_statuses(self, filename): service_callback_api = get_service_callback_api_for_service(service_id=notifications[0].service_id) if service_callback_api: for notification in notifications: - send_delivery_status_to_service.apply_async([str(notification.id)], queue=QueueNames.NOTIFY) + send_delivery_status_to_service.apply_async([str(notification.id)], queue=QueueNames.CALLBACKS) def process_updates_from_file(response_file): diff --git a/app/notifications/notifications_ses_callback.py b/app/notifications/notifications_ses_callback.py index 28fa96f2e..cf3da7508 100644 --- a/app/notifications/notifications_ses_callback.py +++ b/app/notifications/notifications_ses_callback.py @@ -99,4 +99,4 @@ def _check_and_queue_callback_task(notification_id, service_id): # queue callback task only if the service_callback_api exists service_callback_api = get_service_callback_api_for_service(service_id=service_id) if service_callback_api: - send_delivery_status_to_service.apply_async([str(notification_id)], queue=QueueNames.NOTIFY) + send_delivery_status_to_service.apply_async([str(notification_id)], queue=QueueNames.CALLBACKS) diff --git a/app/notifications/process_client_response.py b/app/notifications/process_client_response.py index ed825766a..ad78c744d 100644 --- a/app/notifications/process_client_response.py +++ b/app/notifications/process_client_response.py @@ -89,7 +89,7 @@ def process_sms_client_response(status, reference, client_name): service_callback_api = get_service_callback_api_for_service(service_id=notification.service_id) if service_callback_api: - send_delivery_status_to_service.apply_async([str(notification.id)], queue=QueueNames.NOTIFY) + send_delivery_status_to_service.apply_async([str(notification.id)], queue=QueueNames.CALLBACKS) success = "{} callback succeeded. reference {} updated".format(client_name, reference) return success, errors diff --git a/tests/app/celery/test_ftp_update_tasks.py b/tests/app/celery/test_ftp_update_tasks.py index 2c6bc4393..cf3e90ade 100644 --- a/tests/app/celery/test_ftp_update_tasks.py +++ b/tests/app/celery/test_ftp_update_tasks.py @@ -116,8 +116,8 @@ def test_update_letter_notifications_statuses_persisted(notify_api, mocker, samp assert failed_letter.billable_units == 2 assert failed_letter.updated_at - calls = [call([str(failed_letter.id)], queue="notify-internal-tasks"), - call([str(sent_letter.id)], queue="notify-internal-tasks")] + calls = [call([str(failed_letter.id)], queue="service-callbacks"), + call([str(sent_letter.id)], queue="service-callbacks")] send_mock.assert_has_calls(calls, any_order=True) diff --git a/tests/app/notifications/test_notifications_ses_callback.py b/tests/app/notifications/test_notifications_ses_callback.py index 49ce90829..157b22ae3 100644 --- a/tests/app/notifications/test_notifications_ses_callback.py +++ b/tests/app/notifications/test_notifications_ses_callback.py @@ -47,7 +47,7 @@ def test_ses_callback_should_update_notification_status( ) statsd_client.incr.assert_any_call("callback.ses.delivered") stats_mock.assert_called_once_with(notification) - send_mock.assert_called_once_with([str(notification.id)], queue="notify-internal-tasks") + send_mock.assert_called_once_with([str(notification.id)], queue="service-callbacks") def test_ses_callback_does_not_call_send_delivery_status_if_no_db_entry( diff --git a/tests/app/notifications/test_process_client_response.py b/tests/app/notifications/test_process_client_response.py index 40e2a608c..3f49901dd 100644 --- a/tests/app/notifications/test_process_client_response.py +++ b/tests/app/notifications/test_process_client_response.py @@ -61,7 +61,7 @@ def test_outcome_statistics_called_for_successful_callback(sample_notification, success, error = process_sms_client_response(status='3', reference=reference, client_name='MMG') assert success == "MMG callback succeeded. reference {} updated".format(str(reference)) assert error is None - send_mock.assert_called_once_with([str(sample_notification.id)], queue="notify-internal-tasks") + send_mock.assert_called_once_with([str(sample_notification.id)], queue="service-callbacks") stats_mock.assert_called_once_with(sample_notification) diff --git a/tests/app/test_config.py b/tests/app/test_config.py index c023fa37a..2f643ba5c 100644 --- a/tests/app/test_config.py +++ b/tests/app/test_config.py @@ -63,7 +63,7 @@ def test_cloudfoundry_config_has_different_defaults(): def test_queue_names_all_queues_correct(): # Need to ensure that all_queues() only returns queue names used in API queues = QueueNames.all_queues() - assert len(queues) == 11 + assert len(queues) == 12 assert set([ QueueNames.PRIORITY, QueueNames.PERIODIC, @@ -75,5 +75,6 @@ def test_queue_names_all_queues_correct(): QueueNames.JOBS, QueueNames.RETRY, QueueNames.NOTIFY, - QueueNames.CREATE_LETTERS_PDF + QueueNames.CREATE_LETTERS_PDF, + QueueNames.CALLBACKS ]) == set(queues) From 5d3a3ab042de564b9ba18a88dd0cbb57602eda1c Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 14 Dec 2017 13:26:38 +0000 Subject: [PATCH 25/31] Can't have spaces between queue names --- manifest-delivery-base.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/manifest-delivery-base.yml b/manifest-delivery-base.yml index 71dc5bcc9..8838ad40b 100644 --- a/manifest-delivery-base.yml +++ b/manifest-delivery-base.yml @@ -56,6 +56,6 @@ applications: NOTIFY_APP_NAME: delivery-worker - name: notify-delivery-worker-receipts - command: scripts/run_app_paas.sh celery -A run_celery.notify_celery worker --loglevel=INFO --concurrency=11 -Q ses-callbacks, service-callbacks + command: scripts/run_app_paas.sh celery -A run_celery.notify_celery worker --loglevel=INFO --concurrency=11 -Q ses-callbacks,service-callbacks env: NOTIFY_APP_NAME: delivery-worker-receipts From c6e6fad01fb5ed4caa65cca82a7caa0422babbb2 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 14 Dec 2017 14:23:32 +0000 Subject: [PATCH 26/31] if apps crash on startup, then fail deploy process we saw an issue where the app started, then immediately crashed due to a setup error. However, jenkins had already returned positively, and the deploy continued. cf-deploy should fail if the app doesn't start up. We do this by looking through the cloudfoundry events, and aborting if there are any `app.crash` events for the new GUID. --- Makefile | 3 +++ scripts/run_app_paas.sh | 2 -- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/Makefile b/Makefile index b4a03d2eb..d9fe648df 100644 --- a/Makefile +++ b/Makefile @@ -281,6 +281,9 @@ cf-deploy: ## Deploys the app to Cloud Foundry # sleep for 10 seconds to try and make sure that all worker threads (either web api or celery) have finished before we delete # when we delete the DB is unbound from the app, which can cause "permission denied for relation" psycopg2 errors. sleep 10 + + # get the new GUID, and find all crash events for that. If there were any crashes we will abort the deploy. + [ $(cf curl "/v2/events?q=type:app.crash&q=actee:$$(cf app --guid notify-delivery-worker-receipts)" | jq ".total_results") -eq 0 ] cf delete -f ${CF_APP}-rollback .PHONY: cf-deploy-api-db-migration diff --git a/scripts/run_app_paas.sh b/scripts/run_app_paas.sh index 78610c1bd..2a4d2988d 100755 --- a/scripts/run_app_paas.sh +++ b/scripts/run_app_paas.sh @@ -55,8 +55,6 @@ function on_exit { break fi done - echo "Application process terminated, waiting 10 seconds" - sleep 10 echo "Terminating remaining subprocesses.." kill 0 } From f23074596b5229646096cc5c34f21012a6b6ce06 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Wed, 13 Dec 2017 15:52:38 +0000 Subject: [PATCH 27/31] 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 28/31] 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) From 7d1c4ea7225c5a02ea9f70f94c3634d0e3e99b4a Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 14 Dec 2017 14:23:32 +0000 Subject: [PATCH 29/31] fix makefile syntax --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index d9fe648df..ea6bb8c41 100644 --- a/Makefile +++ b/Makefile @@ -283,7 +283,7 @@ cf-deploy: ## Deploys the app to Cloud Foundry sleep 10 # get the new GUID, and find all crash events for that. If there were any crashes we will abort the deploy. - [ $(cf curl "/v2/events?q=type:app.crash&q=actee:$$(cf app --guid notify-delivery-worker-receipts)" | jq ".total_results") -eq 0 ] + [ $$(cf curl "/v2/events?q=type:app.crash&q=actee:$$(cf app --guid ${CF_APP})" | jq ".total_results") -eq 0 ] cf delete -f ${CF_APP}-rollback .PHONY: cf-deploy-api-db-migration From 0ad43f0c5b1601e8ab3d36e6e16f340585688b58 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Thu, 14 Dec 2017 16:00:51 +0000 Subject: [PATCH 30/31] Create letters pdf queue was renamed with tasks, but was lost in another merge - needs to be correct name otherwise the delivery worker will not pick up the queue --- app/config.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/config.py b/app/config.py index f2d37a769..0205262a0 100644 --- a/app/config.py +++ b/app/config.py @@ -30,7 +30,7 @@ class QueueNames(object): RETRY = 'retry-tasks' NOTIFY = 'notify-internal-tasks' PROCESS_FTP = 'process-ftp-tasks' - CREATE_LETTERS_PDF = 'create-letters-pdf' + CREATE_LETTERS_PDF = 'create-letters-pdf-tasks' CALLBACKS = 'service-callbacks' @staticmethod From 1ca252dcf91175e9ba547ce7bc1dbf5028d429bb Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 12 Dec 2017 17:28:17 +0000 Subject: [PATCH 31/31] put pdfs in tomorrow's dvla bucket after 17:30 So if someone sends a letter in the evening, it gets picked up the next day --- app/aws/s3.py | 9 +++++++-- tests/app/aws/test_s3.py | 18 ++++++++++++++++-- 2 files changed, 23 insertions(+), 4 deletions(-) diff --git a/app/aws/s3.py b/app/aws/s3.py index 4551c8c72..5380544c8 100644 --- a/app/aws/s3.py +++ b/app/aws/s3.py @@ -1,4 +1,4 @@ -from datetime import datetime, timedelta +from datetime import datetime, timedelta, time from flask import current_app @@ -83,8 +83,13 @@ def remove_transformed_dvla_file(job_id): def upload_letters_pdf(reference, crown, filedata): now = datetime.utcnow() + + print_datetime = now + if now.time() > time(17, 30): + print_datetime = now + timedelta(days=1) + upload_file_name = LETTERS_PDF_FILE_LOCATION_STRUCTURE.format( - folder=now.date().isoformat(), + folder=print_datetime.date(), reference=reference, duplex="D", letter_class="2", diff --git a/tests/app/aws/test_s3.py b/tests/app/aws/test_s3.py index e610560db..23f890b9f 100644 --- a/tests/app/aws/test_s3.py +++ b/tests/app/aws/test_s3.py @@ -147,7 +147,7 @@ def test_get_s3_bucket_objects_does_not_return_outside_of_date_range(notify_api, (True, 'C'), (False, 'N'), ]) -@freeze_time("2017-12-04 15:00:00") +@freeze_time("2017-12-04 17:29:00") def test_upload_letters_pdf_calls_utils_s3upload_with_correct_args( notify_api, mocker, crown_flag, expected_crown_text): s3_upload_mock = mocker.patch('app.aws.s3.utils_s3upload') @@ -157,5 +157,19 @@ def test_upload_letters_pdf_calls_utils_s3upload_with_correct_args( filedata='some_data', region='eu-west-1', bucket_name='test-letters-pdf', - file_location='2017-12-04/NOTIFY.FOO.D.2.C.{}.20171204150000.PDF'.format(expected_crown_text) + file_location='2017-12-04/NOTIFY.FOO.D.2.C.{}.20171204172900.PDF'.format(expected_crown_text) + ) + + +@freeze_time("2017-12-04 17:31:00") +def test_upload_letters_pdf_puts_in_tomorrows_bucket_after_half_five(notify_api, mocker): + s3_upload_mock = mocker.patch('app.aws.s3.utils_s3upload') + upload_letters_pdf(reference='foo', crown=True, filedata='some_data') + + s3_upload_mock.assert_called_with( + filedata='some_data', + region='eu-west-1', + bucket_name='test-letters-pdf', + # in tomorrow's folder, but still has this evening's timestamp + file_location='2017-12-05/NOTIFY.FOO.D.2.C.C.20171204173100.PDF' )