diff --git a/Makefile b/Makefile index b4a03d2eb..ea6bb8c41 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 ${CF_APP})" | jq ".total_results") -eq 0 ] cf delete -f ${CF_APP}-rollback .PHONY: cf-deploy-api-db-migration diff --git a/app/aws/s3.py b/app/aws/s3.py index a64be797c..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", @@ -99,3 +104,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/app/celery/letters_pdf_tasks.py b/app/celery/letters_pdf_tasks.py new file mode 100644 index 000000000..d70265791 --- /dev/null +++ b/app/celery/letters_pdf_tasks.py @@ -0,0 +1,81 @@ +from flask import current_app +import math +from requests import ( + post as requests_post, + RequestException +) + +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, + dao_update_notification +) +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, _raise=True) + + pdf_data, billable_units = 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=notification.service.crown, filedata=pdf_data) + + notification.billable_units = billable_units + dao_update_notification(notification) + + current_app.logger.info( + 'Letter notification reference {reference}: billable units set to {billable_units}'.format( + reference=str(notification.reference), billable_units=billable_units)) + + except (RequestException, BotoClientError): + 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() + + 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/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/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/app/celery/tasks.py b/app/celery/tasks.py index 54d6090db..7460f3b90 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -4,6 +4,7 @@ from collections import namedtuple from celery.signals import worker_process_shutdown from flask import current_app + from notifications_utils.recipients import ( RecipientCSV ) @@ -29,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 @@ -44,7 +46,7 @@ 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, ) 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,7 +123,10 @@ 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 + 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) else: @@ -136,7 +141,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( @@ -312,6 +317,18 @@ def save_letter( reply_to_text=service.get_default_letter_contact() ) + 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: handle_exception(self, notification, notification_id, e) @@ -399,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): @@ -486,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/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 d553041e3..0205262a0 100644 --- a/app/config.py +++ b/app/config.py @@ -30,6 +30,8 @@ class QueueNames(object): RETRY = 'retry-tasks' NOTIFY = 'notify-internal-tasks' PROCESS_FTP = 'process-ftp-tasks' + CREATE_LETTERS_PDF = 'create-letters-pdf-tasks' + CALLBACKS = 'service-callbacks' @staticmethod def all_queues(): @@ -44,6 +46,8 @@ class QueueNames(object): QueueNames.JOBS, QueueNames.RETRY, QueueNames.NOTIFY, + QueueNames.CREATE_LETTERS_PDF, + QueueNames.CALLBACKS, ] @@ -304,6 +308,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 ### @@ -369,6 +376,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/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index d9886fcfd..29b49ba7c 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, @@ -224,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): @@ -541,14 +546,23 @@ 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 """ 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.api_key != None, # noqa + # Ignore services that have letters_as_pdf permission + ~Service.permissions.any( + ServicePermission.permission == 'letters_as_pdf' + ) ).with_for_update( ).all() 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/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/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/manifest-delivery-base.yml b/manifest-delivery-base.yml index 6b1fc5094..8838ad40b 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 @@ -50,11 +51,11 @@ 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 - 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 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/migrations/versions/0152_kill_service_free_fragments.py b/migrations/versions/0152_kill_service_free_fragments.py new file mode 100644 index 000000000..038b978da --- /dev/null +++ b/migrations/versions/0152_kill_service_free_fragments.py @@ -0,0 +1,24 @@ +""" + +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 = '0152_kill_service_free_fragments' +down_revision = '0151_refactor_letter_rates' + + +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)) 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 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)) 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 } 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' ) 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/celery/test_letters_pdf_tasks.py b/tests/app/celery/test_letters_pdf_tasks.py new file mode 100644 index 000000000..a414fa2db --- /dev/null +++ b/tests/app/celery/test_letters_pdf_tasks.py @@ -0,0 +1,137 @@ +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 app.models import Notification + +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 + } + } + + +@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', '1')) + 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=sample_letter_notification.service.crown, + filedata=b'\x00\x01' + ) + + +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_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', 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') + + 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_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 } diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index aceb525a0..a5837b3a2 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -20,6 +20,7 @@ from app.celery.scheduled_tasks import check_job_status from app.celery.tasks import ( build_dvla_file, create_dvla_file_contents_for_job, + job_complete, process_job, process_row, save_sms, @@ -32,7 +33,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, @@ -1039,6 +1040,85 @@ def test_save_letter_saves_letter_to_database(mocker, notify_db_session): assert notification_db.reply_to_text == "Address contact" +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') + + 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): @@ -1436,7 +1516,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') diff --git a/tests/app/conftest.py b/tests/app/conftest.py index 71b510767..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'} ) @@ -599,7 +599,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') 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..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 @@ -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,20 @@ 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) 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_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' diff --git a/tests/app/test_config.py b/tests/app/test_config.py index 3a40b4db4..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) == 10 + assert len(queues) == 12 assert set([ QueueNames.PRIORITY, QueueNames.PERIODIC, @@ -74,5 +74,7 @@ def test_queue_names_all_queues_correct(): QueueNames.STATISTICS, QueueNames.JOBS, QueueNames.RETRY, - QueueNames.NOTIFY + QueueNames.NOTIFY, + QueueNames.CREATE_LETTERS_PDF, + QueueNames.CALLBACKS ]) == set(queues) 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