From f61ccd8ff038df560527ef2b37e8eee759baf00f Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 13 Sep 2017 15:25:05 +0100 Subject: [PATCH 01/12] add run_letter_notifications scheduled task this task grabs all notifications that are sent via the API, and are still in created - and sends them off to DVLA. --- app/celery/scheduled_tasks.py | 26 +++++++++++++++++++++++++- app/celery/tasks.py | 14 ++++++++++++-- app/config.py | 10 +++++++++- tests/app/celery/test_tasks.py | 6 +++--- 4 files changed, 49 insertions(+), 7 deletions(-) diff --git a/app/celery/scheduled_tasks.py b/app/celery/scheduled_tasks.py index e18cbe8e6..d23eaee0a 100644 --- a/app/celery/scheduled_tasks.py +++ b/app/celery/scheduled_tasks.py @@ -5,6 +5,7 @@ from datetime import ( from flask import current_app from sqlalchemy.exc import SQLAlchemyError +from notifications_utils.s3 import s3upload from app.aws import s3 from app import notify_celery @@ -37,7 +38,7 @@ from app.dao.users_dao import delete_codes_older_created_more_than_a_day_ago from app.models import LETTER_TYPE, JOB_STATUS_READY_TO_SEND from app.notifications.process_notifications import send_notification_to_queue from app.statsd_decorators import statsd -from app.celery.tasks import process_job +from app.celery.tasks import process_job, create_dvla_file_contents_for_notifications from app.config import QueueNames, TaskNames from app.utils import convert_utc_to_bst @@ -319,3 +320,26 @@ def run_letter_jobs(): queue=QueueNames.PROCESS_FTP ) current_app.logger.info("Queued {} ready letter job ids onto {}".format(len(job_ids), QueueNames.PROCESS_FTP)) + + +@notify_celery.task(name="run-letter-notifications") +@statsd(namespace="tasks") +def run_letter_notifications(): + notifications = dao_get_created_letter_api_notifications_that_dont_belong_to_jobs() + + file_contents = create_dvla_file_contents_for_notifications(notifications) + s3upload( + filedata=file_contents + '\n', + region=current_app.config['AWS_REGION'], + bucket_name=current_app.config['DVLA_UPLOAD_BUCKET_NAME'], + file_location='2017-09-12-dvla-notifications.txt' + ) + + # set noti statuses to pending or something + + notify_celery.send_task( + name=TaskNames.DVLA_NOTIFICATIONS, + kwargs={'date': '2017-09-12'}, + queue=QueueNames.PROCESS_FTP + ) + current_app.logger.info("Queued {} ready letter job ids onto {}".format(len(notifications), QueueNames.PROCESS_FTP)) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index e621350ce..71896224b 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -309,6 +309,10 @@ def update_job_to_sent_to_dvla(self, job_id): "Updated {} job to {}".format(updated_count, job_id, JOB_STATUS_SENT_TO_DVLA)) +def update_notifications_to_sent_to_dvla(self, notification_references): + + + @notify_celery.task(bind=True, name='update-letter-job-to-error') @statsd(namespace="tasks") def update_dvla_job_to_error(self, job_id): @@ -316,7 +320,13 @@ def update_dvla_job_to_error(self, job_id): current_app.logger.info("Updated {} job to {}".format(job_id, JOB_STATUS_ERROR)) -def create_dvla_file_contents(job_id): +def create_dvla_file_contents_for_job(job_id): + notifications = dao_get_all_notifications_for_job(job_id) + + return create_dvla_file_contents_for_notifications(notifications) + + +def create_dvla_file_contents_for_notifications(notifications): file_contents = '\n'.join( str(LetterDVLATemplate( notification.template.__dict__, @@ -325,7 +335,7 @@ def create_dvla_file_contents(job_id): contact_block=notification.service.letter_contact_block, org_id=notification.service.dvla_organisation.id, )) - for notification in dao_get_all_notifications_for_job(job_id) + for notification in notifications ) return file_contents diff --git a/app/config.py b/app/config.py index daf552348..a5f481855 100644 --- a/app/config.py +++ b/app/config.py @@ -229,6 +229,11 @@ class Config(object): 'task': 'run-letter-jobs', 'schedule': crontab(hour=17, minute=30), 'options': {'queue': QueueNames.PERIODIC} + }, + 'run-letter-notifications': { + 'task': 'run-letter-notifications', + 'schedule': crontab(hour=17, minute=35), + 'options': {'queue': QueueNames.PERIODIC} } } CELERY_QUEUES = [] @@ -253,7 +258,10 @@ class Config(object): FUNCTIONAL_TEST_PROVIDER_SERVICE_ID = None FUNCTIONAL_TEST_PROVIDER_SMS_TEMPLATE_ID = None - DVLA_UPLOAD_BUCKET_NAME = "{}-dvla-file-per-job".format(os.getenv('NOTIFY_ENVIRONMENT')) + DVLA_BUCKETS = { + 'job': '{}-dvla-file-per-job'.format(os.getenv('NOTIFY_ENVIRONMENT')), + 'notification': '{}-dvla-file-per-job'.format(os.getenv('NOTIFY_ENVIRONMENT')) + } API_KEY_LIMITS = { KEY_TYPE_TEAM: { diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index d82249953..e17b1e05a 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -17,7 +17,7 @@ from app.celery import tasks from app.celery.tasks import ( s3, build_dvla_file, - create_dvla_file_contents, + create_dvla_file_contents_for_job, update_dvla_job_to_error, process_job, process_row, @@ -1081,7 +1081,7 @@ def test_build_dvla_file_retries_if_all_notifications_are_not_created(sample_let mocked_send_task.assert_not_called() -def test_create_dvla_file_contents(sample_letter_template, mocker): +def test_create_dvla_file_contents_for_job(sample_letter_template, mocker): job = create_job(template=sample_letter_template, notification_count=2) create_notification(template=job.template, job=job, reference=1) create_notification(template=job.template, job=job, reference=2) @@ -1089,7 +1089,7 @@ def test_create_dvla_file_contents(sample_letter_template, mocker): mocked_letter_template_instance = mocked_letter_template.return_value mocked_letter_template_instance.__str__.return_value = "dvla|string" - create_dvla_file_contents(job.id) + create_dvla_file_contents_for_job(job.id) calls = mocked_letter_template.call_args_list # Template assert calls[0][0][0]['subject'] == 'Template subject' From 7dd3c1df5a51ed77adf64b4d801b138b4cf6b13b Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Fri, 15 Sep 2017 17:46:08 +0100 Subject: [PATCH 02/12] set letter notifications to pending while notify-ftp does its stuff this means that if the task is accidentally ran twice (eg we autoscale notify-celery-worker-beat to 2), it won't send letters twice. Additionally, update some function names and config variables to make it clear that they are referring to letter jobs, rather than all letter content --- app/aws/s3.py | 2 +- app/celery/scheduled_tasks.py | 25 +++++++---- app/celery/tasks.py | 15 +++++-- app/dao/notifications_dao.py | 43 ++++++++++++++++++- tests/app/aws/test_s3.py | 2 +- tests/app/celery/test_tasks.py | 2 +- .../notification_dao/test_notification_dao.py | 13 +++--- 7 files changed, 81 insertions(+), 21 deletions(-) diff --git a/app/aws/s3.py b/app/aws/s3.py index 48a54e709..927eb0ad1 100644 --- a/app/aws/s3.py +++ b/app/aws/s3.py @@ -72,7 +72,7 @@ def remove_s3_object(bucket_name, object_key): def remove_transformed_dvla_file(job_id): - bucket_name = current_app.config['DVLA_UPLOAD_BUCKET_NAME'] + bucket_name = current_app.config['DVLA_BUCKETS']['job'] file_location = '{}-dvla-job.text'.format(job_id) obj = get_s3_object(bucket_name, file_location) return obj.delete() diff --git a/app/celery/scheduled_tasks.py b/app/celery/scheduled_tasks.py index d23eaee0a..ca429a2c0 100644 --- a/app/celery/scheduled_tasks.py +++ b/app/celery/scheduled_tasks.py @@ -28,7 +28,9 @@ from app.dao.notifications_dao import ( is_delivery_slow_for_provider, delete_notifications_created_more_than_a_week_ago_by_type, dao_get_scheduled_notifications, - set_scheduled_notification_to_processed) + set_scheduled_notification_to_processed, + dao_set_created_live_letter_api_notifications_to_pending, +) from app.dao.statistics_dao import dao_timeout_job_statistics from app.dao.provider_details_dao import ( get_current_provider, @@ -325,21 +327,28 @@ def run_letter_jobs(): @notify_celery.task(name="run-letter-notifications") @statsd(namespace="tasks") def run_letter_notifications(): - notifications = dao_get_created_letter_api_notifications_that_dont_belong_to_jobs() + current_time = datetime.utcnow().isoformat() + + notifications = dao_set_created_live_letter_api_notifications_to_pending() file_contents = create_dvla_file_contents_for_notifications(notifications) + + file_location = '{}-dvla-notifications.txt'.format(current_time) s3upload( filedata=file_contents + '\n', region=current_app.config['AWS_REGION'], - bucket_name=current_app.config['DVLA_UPLOAD_BUCKET_NAME'], - file_location='2017-09-12-dvla-notifications.txt' + bucket_name=current_app.config['DVLA_BUCKETS']['notification'], + file_location=file_location ) - # set noti statuses to pending or something - notify_celery.send_task( name=TaskNames.DVLA_NOTIFICATIONS, - kwargs={'date': '2017-09-12'}, + kwargs={'filename': file_location}, queue=QueueNames.PROCESS_FTP ) - current_app.logger.info("Queued {} ready letter job ids onto {}".format(len(notifications), QueueNames.PROCESS_FTP)) + current_app.logger.info( + "Queued {} ready letter api notifications onto {}".format( + len(notifications), + QueueNames.PROCESS_FTP + ) + ) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 71896224b..95d0c4a57 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -27,7 +27,7 @@ from app.dao.jobs_dao import ( all_notifications_are_created_for_job, dao_get_all_notifications_for_job, dao_update_job_status) -from app.dao.notifications_dao import get_notification_by_id, dao_update_notifications_sent_to_dvla +from app.dao.notifications_dao import get_notification_by_id, dao_update_notifications_for_job_to_sent_to_dvla from app.dao.provider_details_dao import get_current_provider from app.dao.service_inbound_api_dao import get_service_inbound_api_for_service from app.dao.services_dao import dao_fetch_service_by_id, fetch_todays_total_message_count @@ -279,11 +279,11 @@ def persist_letter( def build_dvla_file(self, job_id): try: if all_notifications_are_created_for_job(job_id): - file_contents = create_dvla_file_contents(job_id) + file_contents = create_dvla_file_contents_for_job(job_id) s3upload( filedata=file_contents + '\n', region=current_app.config['AWS_REGION'], - bucket_name=current_app.config['DVLA_UPLOAD_BUCKET_NAME'], + bucket_name=current_app.config['DVLA_BUCKETS']['job'], file_location="{}-dvla-job.text".format(job_id) ) dao_update_job_status(job_id, JOB_STATUS_READY_TO_SEND) @@ -302,15 +302,22 @@ def update_job_to_sent_to_dvla(self, job_id): # and update all notifications for this job to sending, provider = DVLA provider = get_current_provider(LETTER_TYPE) - updated_count = dao_update_notifications_sent_to_dvla(job_id, provider.identifier) + updated_count = dao_update_notifications_for_job_to_sent_to_dvla(job_id, provider.identifier) dao_update_job_status(job_id, JOB_STATUS_SENT_TO_DVLA) current_app.logger.info("Updated {} letter notifications to sending. " "Updated {} job to {}".format(updated_count, job_id, JOB_STATUS_SENT_TO_DVLA)) +@notify_celery.task(bind=True, name='update-notifications-to-sent') +@statsd(namespace="tasks") def update_notifications_to_sent_to_dvla(self, notification_references): + # This task will be called by the FTP app to update notifications as sent to DVLA + provider = get_current_provider(LETTER_TYPE) + updated_count = dao_update_notifications_for_job_to_sent_to_dvla(references, provider.identifier) + + current_app.logger.info("Updated {} letter notifications to sending. ".format(updated_count)) @notify_celery.task(bind=True, name='update-letter-job-to-error') diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 17e388422..b390d5abf 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -460,7 +460,22 @@ def is_delivery_slow_for_provider( @statsd(namespace="dao") @transactional -def dao_update_notifications_sent_to_dvla(job_id, provider): +def dao_update_notifications_for_job_to_sent_to_dvla(job_id, provider): + now = datetime.utcnow() + updated_count = db.session.query( + Notification).filter(Notification.job_id == job_id).update( + {'status': NOTIFICATION_SENDING, "sent_by": provider, "sent_at": now}) + + db.session.query( + NotificationHistory).filter(NotificationHistory.job_id == job_id).update( + {'status': NOTIFICATION_SENDING, "sent_by": provider, "sent_at": now, "updated_at": now}) + + return updated_count + + +@statsd(namespace="dao") +@transactional +def dao_update_notifications_by_reference_sent_to_dvla(notification_references, provider): now = datetime.utcnow() updated_count = db.session.query( Notification).filter(Notification.job_id == job_id).update( @@ -555,3 +570,29 @@ def dao_get_total_notifications_sent_per_day_for_performance_platform(start_date NotificationHistory.key_type != KEY_TYPE_TEST, NotificationHistory.notification_type != LETTER_TYPE ).one() + + +def dao_set_created_live_letter_api_notifications_to_pending(): + """ + Sets all past scheduled jobs to pending, and then returns them for further processing. + + 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. + """ + return db.session.query( + Notification.id + ).filter( + Notification.notification_type == LETTER_TYPE, + Notification.status == NOTIFICATION_CREATED, + Notification.key_type == KEY_TYPE_NORMAL, + ).with_for_update( + ).all() + + for notification in notifications: + notification.notification_status = NOTIFICATION_PENDING + + db.session.add_all(notifications) + db.session.commit() + + return notifications diff --git a/tests/app/aws/test_s3.py b/tests/app/aws/test_s3.py index b44d66fc8..d6e4c1b75 100644 --- a/tests/app/aws/test_s3.py +++ b/tests/app/aws/test_s3.py @@ -39,7 +39,7 @@ def test_remove_transformed_dvla_file_makes_correct_call(notify_api, mocker): remove_transformed_dvla_file(fake_uuid) s3_mock.assert_has_calls([ - call(current_app.config['DVLA_UPLOAD_BUCKET_NAME'], '{}-dvla-job.text'.format(fake_uuid)), + call(current_app.config['DVLA_BUCKETS']['job'], '{}-dvla-job.text'.format(fake_uuid)), call().delete() ]) diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index e17b1e05a..48b4ac3b7 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -1058,7 +1058,7 @@ def test_build_dvla_file(sample_letter_template, mocker): mocked_upload.assert_called_once_with( filedata="dvla|string\ndvla|string\n", region=current_app.config['AWS_REGION'], - bucket_name=current_app.config['DVLA_UPLOAD_BUCKET_NAME'], + bucket_name=current_app.config['DVLA_BUCKETS']['job'], file_location="{}-dvla-job.text".format(job.id) ) assert Job.query.get(job.id).job_status == 'ready to send' diff --git a/tests/app/dao/notification_dao/test_notification_dao.py b/tests/app/dao/notification_dao/test_notification_dao.py index 7d191da56..2b4e9479c 100644 --- a/tests/app/dao/notification_dao/test_notification_dao.py +++ b/tests/app/dao/notification_dao/test_notification_dao.py @@ -40,7 +40,7 @@ from app.dao.notifications_dao import ( dao_delete_notifications_and_history_by_id, dao_timeout_notifications, is_delivery_slow_for_provider, - dao_update_notifications_sent_to_dvla, + dao_update_notifications_for_job_to_sent_to_dvla, dao_get_notifications_by_to_field, dao_created_scheduled_notification, dao_get_scheduled_notifications, set_scheduled_notification_to_processed) @@ -1713,11 +1713,11 @@ def test_slow_provider_delivery_does_not_return_for_standard_delivery_time( assert not slow_delivery -def test_dao_update_notifications_sent_to_dvla(notify_db, notify_db_session, sample_letter_template): +def test_dao_update_notifications_for_job_to_sent_to_dvla(notify_db, notify_db_session, sample_letter_template): job = sample_job(notify_db=notify_db, notify_db_session=notify_db_session, template=sample_letter_template) notification = create_notification(template=sample_letter_template, job=job) - updated_count = dao_update_notifications_sent_to_dvla(job_id=job.id, provider='some provider') + updated_count = dao_update_notifications_for_job_to_sent_to_dvla(job_id=job.id, provider='some provider') assert updated_count == 1 updated_notification = Notification.query.get(notification.id) @@ -1732,7 +1732,7 @@ def test_dao_update_notifications_sent_to_dvla(notify_db, notify_db_session, sam assert history.updated_at -def test_dao_update_notifications_sent_to_dvla_does_update_history_if_test_key(sample_letter_job): +def test_dao_update_notifications_for_job_to_sent_to_dvla_does_update_history_if_test_key(sample_letter_job): api_key = create_api_key(sample_letter_job.service, key_type=KEY_TYPE_TEST) notification = create_notification( sample_letter_job.template, @@ -1740,7 +1740,10 @@ def test_dao_update_notifications_sent_to_dvla_does_update_history_if_test_key(s api_key=api_key ) - updated_count = dao_update_notifications_sent_to_dvla(job_id=sample_letter_job.id, provider='some provider') + updated_count = dao_update_notifications_for_job_to_sent_to_dvla( + job_id=sample_letter_job.id, + provider='some provider' + ) assert updated_count == 1 updated_notification = Notification.query.get(notification.id) From 17ba8db97f548de5c63251b83fabaa90ee957848 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 20 Sep 2017 11:12:37 +0100 Subject: [PATCH 03/12] remove jobs from letter api and make success/error ftp callback tasks 1. No longer create jobs when creating letters from api :tada: 2. Bulk update notifications based on the notification references after we send them to DVLA - either as success or as error --- app/celery/tasks.py | 53 ++++++++++++++----- app/config.py | 2 +- app/dao/notifications_dao.py | 18 ++++--- .../process_letter_notifications.py | 10 ++-- app/v2/notifications/post_notifications.py | 3 +- 5 files changed, 59 insertions(+), 27 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 95d0c4a57..e78a1298d 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -27,7 +27,11 @@ from app.dao.jobs_dao import ( all_notifications_are_created_for_job, dao_get_all_notifications_for_job, dao_update_job_status) -from app.dao.notifications_dao import get_notification_by_id, dao_update_notifications_for_job_to_sent_to_dvla +from app.dao.notifications_dao import ( + get_notification_by_id, + dao_update_notifications_for_job_to_sent_to_dvla, + dao_update_notifications_by_reference +) from app.dao.provider_details_dao import get_current_provider from app.dao.service_inbound_api_dao import get_service_inbound_api_for_service from app.dao.services_dao import dao_fetch_service_by_id, fetch_todays_total_message_count @@ -309,17 +313,6 @@ def update_job_to_sent_to_dvla(self, job_id): "Updated {} job to {}".format(updated_count, job_id, JOB_STATUS_SENT_TO_DVLA)) -@notify_celery.task(bind=True, name='update-notifications-to-sent') -@statsd(namespace="tasks") -def update_notifications_to_sent_to_dvla(self, notification_references): - # This task will be called by the FTP app to update notifications as sent to DVLA - provider = get_current_provider(LETTER_TYPE) - - updated_count = dao_update_notifications_for_job_to_sent_to_dvla(references, provider.identifier) - - current_app.logger.info("Updated {} letter notifications to sending. ".format(updated_count)) - - @notify_celery.task(bind=True, name='update-letter-job-to-error') @statsd(namespace="tasks") def update_dvla_job_to_error(self, job_id): @@ -327,6 +320,42 @@ def update_dvla_job_to_error(self, job_id): current_app.logger.info("Updated {} job to {}".format(job_id, JOB_STATUS_ERROR)) +@notify_celery.task(bind=True, name='update-letter-notifications-to-sent') +@statsd(namespace="tasks") +def update_letter_notifications_to_sent_to_dvla(self, notification_references): + # This task will be called by the FTP app to update notifications as sent to DVLA + provider = get_current_provider(LETTER_TYPE) + + updated_count = dao_update_notifications_by_reference( + notification_references, + { + 'status': NOTIFICATION_SENDING, + 'sent_by': provider.identifier, + 'sent_at': datetime.utcnow(), + 'updated_at': datetime.utcnow() + } + ) + + current_app.logger.info("Updated {} letter notifications to sending".format(updated_count)) + + +@notify_celery.task(bind=True, name='update-letter-notifications-to-error') +@statsd(namespace="tasks") +def update_letter_notifications_to_error(self, notification_references): + # This task will be called by the FTP app to update notifications as sent to DVLA + provider = get_current_provider(LETTER_TYPE) + + updated_count = dao_update_notifications_by_reference( + notification_references, + { + 'status': NOTIFICATION_TECHNICAL_FAILURE, + 'updated_at': datetime.utcnow() + } + ) + + current_app.logger.info("Updated {} letter notifications to technical-failure".format(updated_count)) + + def create_dvla_file_contents_for_job(job_id): notifications = dao_get_all_notifications_for_job(job_id) diff --git a/app/config.py b/app/config.py index a5f481855..edfc5f2d0 100644 --- a/app/config.py +++ b/app/config.py @@ -260,7 +260,7 @@ class Config(object): DVLA_BUCKETS = { 'job': '{}-dvla-file-per-job'.format(os.getenv('NOTIFY_ENVIRONMENT')), - 'notification': '{}-dvla-file-per-job'.format(os.getenv('NOTIFY_ENVIRONMENT')) + 'notification': '{}-dvla-file-letter-api'.format(os.getenv('NOTIFY_ENVIRONMENT')) } API_KEY_LIMITS = { diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index b390d5abf..6c92d357b 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -475,15 +475,19 @@ def dao_update_notifications_for_job_to_sent_to_dvla(job_id, provider): @statsd(namespace="dao") @transactional -def dao_update_notifications_by_reference_sent_to_dvla(notification_references, provider): +def dao_update_notifications_by_reference(references, update_dict): now = datetime.utcnow() - updated_count = db.session.query( - Notification).filter(Notification.job_id == job_id).update( - {'status': NOTIFICATION_SENDING, "sent_by": provider, "sent_at": now}) + updated_count = Notification.query().filter( + Notification.reference.in_(references) + ).update( + update_dict + ) - db.session.query( - NotificationHistory).filter(NotificationHistory.job_id == job_id).update( - {'status': NOTIFICATION_SENDING, "sent_by": provider, "sent_at": now, "updated_at": now}) + NotificationHistory.query().filter( + NotificationHistory.reference.in_(references) + ).update( + update_dict + ) return updated_count diff --git a/app/notifications/process_letter_notifications.py b/app/notifications/process_letter_notifications.py index 332f6ed32..103746231 100644 --- a/app/notifications/process_letter_notifications.py +++ b/app/notifications/process_letter_notifications.py @@ -26,10 +26,10 @@ def create_letter_api_job(template): return job -def create_letter_notification(letter_data, job, api_key): +def create_letter_notification(letter_data, template, api_key): notification = persist_notification( - template_id=job.template.id, - template_version=job.template.version, + template_id=template.id, + template_version=template.version, # we only accept addresses_with_underscores from the API (from CSV we also accept dashes, spaces etc) recipient=letter_data['personalisation']['address_line_1'], service=job.service, @@ -37,8 +37,8 @@ def create_letter_notification(letter_data, job, api_key): notification_type=LETTER_TYPE, api_key_id=api_key.id, key_type=api_key.key_type, - job_id=job.id, - job_row_number=0, + job_id=None, + job_row_number=None, reference=create_random_identifier(), client_reference=letter_data.get('reference') ) diff --git a/app/v2/notifications/post_notifications.py b/app/v2/notifications/post_notifications.py index 47a47a711..5e5a5c5b1 100644 --- a/app/v2/notifications/post_notifications.py +++ b/app/v2/notifications/post_notifications.py @@ -153,8 +153,7 @@ def process_letter_notification(*, letter_data, api_key, template): if api_key.service.restricted and api_key.key_type != KEY_TYPE_TEST: raise BadRequestError(message='Cannot send letters when service is in trial mode', status_code=403) - job = create_letter_api_job(template) - notification = create_letter_notification(letter_data, job, api_key) + notification = create_letter_notification(letter_data, template, api_key) if api_key.service.research_mode or api_key.key_type == KEY_TYPE_TEST: # distinguish real API jobs from test jobs by giving the test jobs a different filename From 04412fd3147276474a3a57fa582b374d4c66b2c2 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 21 Sep 2017 16:54:19 +0100 Subject: [PATCH 04/12] update queue name --- app/config.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/config.py b/app/config.py index edfc5f2d0..2e1a272ba 100644 --- a/app/config.py +++ b/app/config.py @@ -49,7 +49,7 @@ class QueueNames(object): class TaskNames(object): DVLA_JOBS = 'send-jobs-to-dvla' - DVLA_NOTIFICATIONS = 'send-notifications-to-dvla' + DVLA_NOTIFICATIONS = 'send-api-notifications-to-dvla' class Config(object): From cdc8acb49ad1623d2f2404be2b5a034808c86fc6 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Fri, 22 Sep 2017 14:06:31 +0100 Subject: [PATCH 05/12] only trigger DVLA tasks if there is data to send --- app/celery/scheduled_tasks.py | 52 ++++++++++++++++++----------------- 1 file changed, 27 insertions(+), 25 deletions(-) diff --git a/app/celery/scheduled_tasks.py b/app/celery/scheduled_tasks.py index ca429a2c0..9550eb406 100644 --- a/app/celery/scheduled_tasks.py +++ b/app/celery/scheduled_tasks.py @@ -316,12 +316,13 @@ def populate_monthly_billing(): @statsd(namespace="tasks") def run_letter_jobs(): job_ids = dao_get_letter_job_ids_by_status(JOB_STATUS_READY_TO_SEND) - notify_celery.send_task( - name=TaskNames.DVLA_JOBS, - args=(job_ids,), - queue=QueueNames.PROCESS_FTP - ) - current_app.logger.info("Queued {} ready letter job ids onto {}".format(len(job_ids), QueueNames.PROCESS_FTP)) + if job_ids: + notify_celery.send_task( + name=TaskNames.DVLA_JOBS, + args=(job_ids,), + queue=QueueNames.PROCESS_FTP + ) + current_app.logger.info("Queued {} ready letter job ids onto {}".format(len(job_ids), QueueNames.PROCESS_FTP)) @notify_celery.task(name="run-letter-notifications") @@ -331,24 +332,25 @@ def run_letter_notifications(): notifications = dao_set_created_live_letter_api_notifications_to_pending() - file_contents = create_dvla_file_contents_for_notifications(notifications) + if notifications: + file_contents = create_dvla_file_contents_for_notifications(notifications) - file_location = '{}-dvla-notifications.txt'.format(current_time) - s3upload( - filedata=file_contents + '\n', - region=current_app.config['AWS_REGION'], - bucket_name=current_app.config['DVLA_BUCKETS']['notification'], - file_location=file_location - ) - - notify_celery.send_task( - name=TaskNames.DVLA_NOTIFICATIONS, - kwargs={'filename': file_location}, - queue=QueueNames.PROCESS_FTP - ) - current_app.logger.info( - "Queued {} ready letter api notifications onto {}".format( - len(notifications), - QueueNames.PROCESS_FTP + file_location = '{}-dvla-notifications.txt'.format(current_time) + s3upload( + filedata=file_contents + '\n', + region=current_app.config['AWS_REGION'], + bucket_name=current_app.config['DVLA_BUCKETS']['notification'], + file_location=file_location + ) + + notify_celery.send_task( + name=TaskNames.DVLA_NOTIFICATIONS, + kwargs={'filename': file_location}, + queue=QueueNames.PROCESS_FTP + ) + current_app.logger.info( + "Queued {} ready letter api notifications onto {}".format( + len(notifications), + QueueNames.PROCESS_FTP + ) ) - ) From 5e230943c4b21d353fc9e525782a34a096be3524 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Mon, 25 Sep 2017 18:07:57 +0100 Subject: [PATCH 06/12] remove trailing comma from CELERY_ENABLE_UTC, a beat config option To run beat after this, you must remove the celerybeat-schedule file from your notifications-api project root --- app/config.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/config.py b/app/config.py index 2e1a272ba..ef6b7e745 100644 --- a/app/config.py +++ b/app/config.py @@ -137,7 +137,7 @@ class Config(object): 'visibility_timeout': 310, 'queue_name_prefix': NOTIFICATION_QUEUE_PREFIX } - CELERY_ENABLE_UTC = True, + CELERY_ENABLE_UTC = True CELERY_TIMEZONE = 'Europe/London' CELERY_ACCEPT_CONTENT = ['json'] CELERY_TASK_SERIALIZER = 'json' From daf1dc4dca91229be4f8263db5ba98f30f5ec198 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Mon, 25 Sep 2017 18:28:10 +0100 Subject: [PATCH 07/12] fix bucket names and change crontab param order for clarity --- app/config.py | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/app/config.py b/app/config.py index ef6b7e745..6824df9c6 100644 --- a/app/config.py +++ b/app/config.py @@ -165,27 +165,27 @@ class Config(object): }, 'delete-sms-notifications': { 'task': 'delete-sms-notifications', - 'schedule': crontab(minute=0, hour=0), + 'schedule': crontab(hour=0, minute=0), 'options': {'queue': QueueNames.PERIODIC} }, 'delete-email-notifications': { 'task': 'delete-email-notifications', - 'schedule': crontab(minute=20, hour=0), + 'schedule': crontab(hour=0, minute=20), 'options': {'queue': QueueNames.PERIODIC} }, 'delete-letter-notifications': { 'task': 'delete-letter-notifications', - 'schedule': crontab(minute=40, hour=0), + 'schedule': crontab(hour=0, minute=40), 'options': {'queue': QueueNames.PERIODIC} }, 'delete-inbound-sms': { 'task': 'delete-inbound-sms', - 'schedule': crontab(minute=0, hour=1), + 'schedule': crontab(hour=1, minute=0), 'options': {'queue': QueueNames.PERIODIC} }, 'send-daily-performance-platform-stats': { 'task': 'send-daily-performance-platform-stats', - 'schedule': crontab(minute=0, hour=2), + 'schedule': crontab(hour=2, minute=0), 'options': {'queue': QueueNames.PERIODIC} }, 'switch-current-sms-provider-on-slow-delivery': { @@ -195,44 +195,44 @@ class Config(object): }, 'timeout-sending-notifications': { 'task': 'timeout-sending-notifications', - 'schedule': crontab(minute=0, hour=3), + 'schedule': crontab(hour=3, minute=0), 'options': {'queue': QueueNames.PERIODIC} }, 'remove_sms_email_jobs': { 'task': 'remove_csv_files', - 'schedule': crontab(minute=0, hour=4), + 'schedule': crontab(hour=4, minute=0), 'options': {'queue': QueueNames.PERIODIC}, 'kwargs': {'job_types': [EMAIL_TYPE, SMS_TYPE]} }, 'remove_letter_jobs': { 'task': 'remove_csv_files', - 'schedule': crontab(minute=20, hour=4), + 'schedule': crontab(hour=4, minute=20), 'options': {'queue': QueueNames.PERIODIC}, 'kwargs': {'job_types': [LETTER_TYPE]} }, 'remove_transformed_dvla_files': { 'task': 'remove_transformed_dvla_files', - 'schedule': crontab(minute=40, hour=4), + 'schedule': crontab(hour=4, minute=40), 'options': {'queue': QueueNames.PERIODIC} }, 'timeout-job-statistics': { 'task': 'timeout-job-statistics', - 'schedule': crontab(minute=0, hour=5), + 'schedule': crontab(hour=5, minute=0), 'options': {'queue': QueueNames.PERIODIC} }, 'populate_monthly_billing': { 'task': 'populate_monthly_billing', - 'schedule': crontab(minute=10, hour=5), + 'schedule': crontab(hour=5, minute=10), 'options': {'queue': QueueNames.PERIODIC} }, 'run-letter-jobs': { 'task': 'run-letter-jobs', - 'schedule': crontab(hour=17, minute=30), + 'schedule': crontab(hour=5, minute=30), 'options': {'queue': QueueNames.PERIODIC} }, 'run-letter-notifications': { 'task': 'run-letter-notifications', - 'schedule': crontab(hour=17, minute=35), + 'schedule': crontab(hour=5, minute=40), 'options': {'queue': QueueNames.PERIODIC} } } @@ -260,7 +260,7 @@ class Config(object): DVLA_BUCKETS = { 'job': '{}-dvla-file-per-job'.format(os.getenv('NOTIFY_ENVIRONMENT')), - 'notification': '{}-dvla-file-letter-api'.format(os.getenv('NOTIFY_ENVIRONMENT')) + 'notification': '{}-dvla-letter-api-files'.format(os.getenv('NOTIFY_ENVIRONMENT')) } API_KEY_LIMITS = { From f3db920c712a53a4942aa1bff005b3863491d1fd Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 26 Sep 2017 09:56:09 +0100 Subject: [PATCH 08/12] remove jobs from letter api calls we now no longer create a job. At the end of the post there is no action, as we don't have any tasks to queue immediately - if it's a real notification it'll get picked up in the evening scheduled task. If it's a test notification, we create it with an initial status of sending so that we can be sure it'll never get picked up - and then we trigger the update-letter-notifications-to-sent-to-dvla task to sent the sent-at/by. --- app/celery/scheduled_tasks.py | 6 +-- app/celery/tasks.py | 6 ++- app/dao/notifications_dao.py | 13 ++++--- .../process_letter_notifications.py | 32 +++------------- app/notifications/process_notifications.py | 11 +++--- app/v2/notifications/post_notifications.py | 38 +++++++++++-------- .../test_process_letter_notifications.py | 1 - 7 files changed, 48 insertions(+), 59 deletions(-) diff --git a/app/celery/scheduled_tasks.py b/app/celery/scheduled_tasks.py index 9550eb406..432e92c58 100644 --- a/app/celery/scheduled_tasks.py +++ b/app/celery/scheduled_tasks.py @@ -335,17 +335,17 @@ def run_letter_notifications(): if notifications: file_contents = create_dvla_file_contents_for_notifications(notifications) - file_location = '{}-dvla-notifications.txt'.format(current_time) + filename = '{}-dvla-notifications.txt'.format(current_time) s3upload( filedata=file_contents + '\n', region=current_app.config['AWS_REGION'], bucket_name=current_app.config['DVLA_BUCKETS']['notification'], - file_location=file_location + file_location=filename ) notify_celery.send_task( name=TaskNames.DVLA_NOTIFICATIONS, - kwargs={'filename': file_location}, + kwargs={'filename': filename}, queue=QueueNames.PROCESS_FTP ) current_app.logger.info( diff --git a/app/celery/tasks.py b/app/celery/tasks.py index e78a1298d..a34cae506 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -46,7 +46,10 @@ from app.models import ( JOB_STATUS_IN_PROGRESS, JOB_STATUS_FINISHED, JOB_STATUS_READY_TO_SEND, - JOB_STATUS_SENT_TO_DVLA, JOB_STATUS_ERROR) + JOB_STATUS_SENT_TO_DVLA, JOB_STATUS_ERROR, + NOTIFICATION_SENDING, + NOTIFICATION_TECHNICAL_FAILURE +) from app.notifications.process_notifications import persist_notification from app.service.utils import service_allowed_to_send_to from app.statsd_decorators import statsd @@ -343,7 +346,6 @@ def update_letter_notifications_to_sent_to_dvla(self, notification_references): @statsd(namespace="tasks") def update_letter_notifications_to_error(self, notification_references): # This task will be called by the FTP app to update notifications as sent to DVLA - provider = get_current_provider(LETTER_TYPE) updated_count = dao_update_notifications_by_reference( notification_references, diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 6c92d357b..38f48e265 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -477,16 +477,18 @@ def dao_update_notifications_for_job_to_sent_to_dvla(job_id, provider): @transactional def dao_update_notifications_by_reference(references, update_dict): now = datetime.utcnow() - updated_count = Notification.query().filter( + updated_count = Notification.query.filter( Notification.reference.in_(references) ).update( - update_dict + update_dict, + synchronize_session=False ) - NotificationHistory.query().filter( + NotificationHistory.query.filter( NotificationHistory.reference.in_(references) ).update( - update_dict + update_dict, + synchronize_session=False ) return updated_count @@ -585,11 +587,12 @@ def dao_set_created_live_letter_api_notifications_to_pending(): from completing until it commits. """ return db.session.query( - Notification.id + Notification ).filter( Notification.notification_type == LETTER_TYPE, Notification.status == NOTIFICATION_CREATED, Notification.key_type == KEY_TYPE_NORMAL, + Notification.api_key != None ).with_for_update( ).all() diff --git a/app/notifications/process_letter_notifications.py b/app/notifications/process_letter_notifications.py index 103746231..32b151f86 100644 --- a/app/notifications/process_letter_notifications.py +++ b/app/notifications/process_letter_notifications.py @@ -1,38 +1,15 @@ from app import create_random_identifier -from app.models import LETTER_TYPE, JOB_STATUS_READY_TO_SEND, Job -from app.dao.jobs_dao import dao_create_job +from app.models import LETTER_TYPE from app.notifications.process_notifications import persist_notification -from app.v2.errors import InvalidRequest -from app.variables import LETTER_API_FILENAME -def create_letter_api_job(template): - service = template.service - if not service.active: - raise InvalidRequest('Service {} is inactive'.format(service.id), 403) - if template.archived: - raise InvalidRequest('Template {} is deleted'.format(template.id), 400) - - job = Job( - original_file_name=LETTER_API_FILENAME, - service=service, - template=template, - template_version=template.version, - notification_count=1, - job_status=JOB_STATUS_READY_TO_SEND, - created_by=None - ) - dao_create_job(job) - return job - - -def create_letter_notification(letter_data, template, api_key): +def create_letter_notification(letter_data, template, api_key, status): notification = persist_notification( template_id=template.id, template_version=template.version, # we only accept addresses_with_underscores from the API (from CSV we also accept dashes, spaces etc) recipient=letter_data['personalisation']['address_line_1'], - service=job.service, + service=template.service, personalisation=letter_data['personalisation'], notification_type=LETTER_TYPE, api_key_id=api_key.id, @@ -40,6 +17,7 @@ def create_letter_notification(letter_data, template, api_key): job_id=None, job_row_number=None, reference=create_random_identifier(), - client_reference=letter_data.get('reference') + client_reference=letter_data.get('reference'), + status=status ) return notification diff --git a/app/notifications/process_notifications.py b/app/notifications/process_notifications.py index e7e8779f2..8a75473b4 100644 --- a/app/notifications/process_notifications.py +++ b/app/notifications/process_notifications.py @@ -3,6 +3,7 @@ from datetime import datetime from flask import current_app +from notifications_utils.clients import redis from notifications_utils.recipients import ( get_international_phone_info, validate_and_format_phone_number, @@ -11,10 +12,8 @@ from notifications_utils.recipients import ( from app import redis_store from app.celery import provider_tasks -from notifications_utils.clients import redis - from app.config import QueueNames -from app.models import SMS_TYPE, Notification, KEY_TYPE_TEST, EMAIL_TYPE, ScheduledNotification +from app.models import SMS_TYPE, Notification, KEY_TYPE_TEST, EMAIL_TYPE, NOTIFICATION_CREATED, ScheduledNotification from app.dao.notifications_dao import (dao_create_notification, dao_delete_notifications_and_history_by_id, dao_created_scheduled_notification) @@ -52,7 +51,8 @@ def persist_notification( client_reference=None, notification_id=None, simulated=False, - created_by_id=None + created_by_id=None, + status=NOTIFICATION_CREATED ): notification_created_at = created_at or datetime.utcnow() if not notification_id: @@ -73,7 +73,8 @@ def persist_notification( job_row_number=job_row_number, client_reference=client_reference, reference=reference, - created_by_id=created_by_id + created_by_id=created_by_id, + status=status ) if notification_type == SMS_TYPE: diff --git a/app/v2/notifications/post_notifications.py b/app/v2/notifications/post_notifications.py index 5e5a5c5b1..b449b64ec 100644 --- a/app/v2/notifications/post_notifications.py +++ b/app/v2/notifications/post_notifications.py @@ -4,16 +4,23 @@ from flask import request, jsonify, current_app, abort from app import api_user, authenticated_service from app.config import QueueNames -from app.dao.jobs_dao import dao_update_job -from app.models import SMS_TYPE, EMAIL_TYPE, LETTER_TYPE, PRIORITY, KEY_TYPE_TEST, KEY_TYPE_TEAM -from app.celery.tasks import build_dvla_file, update_job_to_sent_to_dvla +from app.models import ( + SMS_TYPE, + EMAIL_TYPE, + LETTER_TYPE, + PRIORITY, + KEY_TYPE_TEST, + KEY_TYPE_TEAM, + NOTIFICATION_CREATED, + NOTIFICATION_SENDING +) +from app.celery.tasks import update_letter_notifications_to_sent_to_dvla from app.notifications.process_notifications import ( persist_notification, send_notification_to_queue, simulated_recipient, persist_scheduled_notification) from app.notifications.process_letter_notifications import ( - create_letter_api_job, create_letter_notification ) from app.notifications.validators import ( @@ -153,18 +160,17 @@ def process_letter_notification(*, letter_data, api_key, template): if api_key.service.restricted and api_key.key_type != KEY_TYPE_TEST: raise BadRequestError(message='Cannot send letters when service is in trial mode', status_code=403) - notification = create_letter_notification(letter_data, template, api_key) + should_send = not (api_key.service.research_mode or api_key.key_type == KEY_TYPE_TEST) - if api_key.service.research_mode or api_key.key_type == KEY_TYPE_TEST: - # distinguish real API jobs from test jobs by giving the test jobs a different filename - job.original_file_name = LETTER_TEST_API_FILENAME - dao_update_job(job) - 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) + # if we don't want to actually send the letter, then start it off in SENDING so we don't pick it up + status = NOTIFICATION_CREATED if should_send else NOTIFICATION_SENDING + + notification = create_letter_notification(letter_data, template, api_key, status=status) + + if not should_send: + update_letter_notifications_to_sent_to_dvla.apply_async( + kwargs={'notification_references': [notification.reference]}, + queue=QueueNames.RESEARCH_MODE + ) - current_app.logger.info("send job {} for api notification {} to build-dvla-file in the process-job queue".format( - job.id, - notification.id - )) return notification diff --git a/tests/app/notifications/test_process_letter_notifications.py b/tests/app/notifications/test_process_letter_notifications.py index 8d964b706..aaf43c4db 100644 --- a/tests/app/notifications/test_process_letter_notifications.py +++ b/tests/app/notifications/test_process_letter_notifications.py @@ -5,7 +5,6 @@ from app.models import Job from app.models import JOB_STATUS_READY_TO_SEND from app.models import LETTER_TYPE from app.models import Notification -from app.notifications.process_letter_notifications import create_letter_api_job from app.notifications.process_letter_notifications import create_letter_notification from app.v2.errors import InvalidRequest from app.variables import LETTER_API_FILENAME From b1928b928cc6a5e4897bf008c3f260e72d2dbcc0 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 26 Sep 2017 11:28:54 +0100 Subject: [PATCH 09/12] update process_letter tests --- app/dao/notifications_dao.py | 2 +- app/v2/notifications/post_notifications.py | 1 - .../test_process_letter_notifications.py | 58 +++---------------- .../test_post_letter_notifications.py | 48 +++++++-------- 4 files changed, 34 insertions(+), 75 deletions(-) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 38f48e265..f7b15b4dc 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -592,7 +592,7 @@ def dao_set_created_live_letter_api_notifications_to_pending(): Notification.notification_type == LETTER_TYPE, Notification.status == NOTIFICATION_CREATED, Notification.key_type == KEY_TYPE_NORMAL, - Notification.api_key != None + Notification.api_key != None # noqa ).with_for_update( ).all() diff --git a/app/v2/notifications/post_notifications.py b/app/v2/notifications/post_notifications.py index b449b64ec..b3cf1c0f8 100644 --- a/app/v2/notifications/post_notifications.py +++ b/app/v2/notifications/post_notifications.py @@ -43,7 +43,6 @@ from app.v2.notifications.create_response import ( create_post_email_response_from_notification, create_post_letter_response_from_notification ) -from app.variables import LETTER_TEST_API_FILENAME @v2_notification_blueprint.route('/', methods=['POST']) diff --git a/tests/app/notifications/test_process_letter_notifications.py b/tests/app/notifications/test_process_letter_notifications.py index aaf43c4db..24f37c06f 100644 --- a/tests/app/notifications/test_process_letter_notifications.py +++ b/tests/app/notifications/test_process_letter_notifications.py @@ -1,52 +1,10 @@ -import pytest - -from app.dao.services_dao import dao_archive_service -from app.models import Job -from app.models import JOB_STATUS_READY_TO_SEND from app.models import LETTER_TYPE from app.models import Notification +from app.models import NOTIFICATION_CREATED from app.notifications.process_letter_notifications import create_letter_notification -from app.v2.errors import InvalidRequest -from app.variables import LETTER_API_FILENAME - -from tests.app.db import create_service -from tests.app.db import create_template -def test_create_job_rejects_inactive_service(notify_db_session): - service = create_service() - template = create_template(service, template_type=LETTER_TYPE) - dao_archive_service(service.id) - - with pytest.raises(InvalidRequest) as exc_info: - create_letter_api_job(template) - - assert exc_info.value.message == 'Service {} is inactive'.format(service.id) - - -def test_create_job_rejects_archived_template(sample_letter_template): - sample_letter_template.archived = True - - with pytest.raises(InvalidRequest) as exc_info: - create_letter_api_job(sample_letter_template) - - assert exc_info.value.message == 'Template {} is deleted'.format(sample_letter_template.id) - - -def test_create_job_creates_job(sample_letter_template): - job = create_letter_api_job(sample_letter_template) - - assert job == Job.query.one() - assert job.original_file_name == LETTER_API_FILENAME - assert job.service == sample_letter_template.service - assert job.template_id == sample_letter_template.id - assert job.template_version == sample_letter_template.version - assert job.notification_count == 1 - assert job.job_status == JOB_STATUS_READY_TO_SEND - assert job.created_by is None - - -def test_create_letter_notification_creates_notification(sample_letter_job, sample_api_key): +def test_create_letter_notification_creates_notification(sample_letter_template, sample_api_key): data = { 'personalisation': { 'address_line_1': 'The Queen', @@ -55,20 +13,20 @@ def test_create_letter_notification_creates_notification(sample_letter_job, samp } } - notification = create_letter_notification(data, sample_letter_job, sample_api_key) + notification = create_letter_notification(data, sample_letter_template, sample_api_key, NOTIFICATION_CREATED) assert notification == Notification.query.one() - assert notification.job == sample_letter_job - assert notification.template == sample_letter_job.template + assert notification.job is None + assert notification.status == NOTIFICATION_CREATED + assert notification.template == sample_letter_template assert notification.api_key == sample_api_key assert notification.notification_type == LETTER_TYPE assert notification.key_type == sample_api_key.key_type - assert notification.job_row_number == 0 assert notification.reference is not None assert notification.client_reference is None -def test_create_letter_notification_sets_reference(sample_letter_job, sample_api_key): +def test_create_letter_notification_sets_reference(sample_letter_template, sample_api_key): data = { 'personalisation': { 'address_line_1': 'The Queen', @@ -78,6 +36,6 @@ def test_create_letter_notification_sets_reference(sample_letter_job, sample_api 'reference': 'foo' } - notification = create_letter_notification(data, sample_letter_job, sample_api_key) + notification = create_letter_notification(data, sample_letter_template, sample_api_key, NOTIFICATION_CREATED) assert notification.client_reference == 'foo' diff --git a/tests/app/v2/notifications/test_post_letter_notifications.py b/tests/app/v2/notifications/test_post_letter_notifications.py index b46f093ca..ccfa39b2f 100644 --- a/tests/app/v2/notifications/test_post_letter_notifications.py +++ b/tests/app/v2/notifications/test_post_letter_notifications.py @@ -11,12 +11,11 @@ from app.models import KEY_TYPE_TEAM from app.models import KEY_TYPE_TEST from app.models import LETTER_TYPE from app.models import Notification +from app.models import NOTIFICATION_SENDING from app.models import SMS_TYPE from app.schema_validation import validate from app.v2.errors import RateLimitError from app.v2.notifications.notification_schemas import post_letter_response -from app.variables import LETTER_TEST_API_FILENAME -from app.variables import LETTER_API_FILENAME from tests import create_authorization_header from tests.app.db import create_service, create_template @@ -45,7 +44,6 @@ def letter_request(client, data, service_id, key_type=KEY_TYPE_NORMAL, _expected @pytest.mark.parametrize('reference', [None, 'reference_from_client']) def test_post_letter_notification_returns_201(client, sample_letter_template, mocker, reference): - mocked = mocker.patch('app.celery.tasks.build_dvla_file.apply_async') data = { 'template_id': str(sample_letter_template.id), 'personalisation': { @@ -63,8 +61,7 @@ def test_post_letter_notification_returns_201(client, sample_letter_template, mo resp_json = letter_request(client, data, service_id=sample_letter_template.service_id) assert validate(resp_json, post_letter_response) == resp_json - job = Job.query.one() - assert job.original_file_name == LETTER_API_FILENAME + assert Job.query.count() == 0 notification = Notification.query.one() notification_id = notification.id assert resp_json['id'] == str(notification_id) @@ -82,8 +79,6 @@ def test_post_letter_notification_returns_201(client, sample_letter_template, mo ) assert not resp_json['scheduled_for'] - mocked.assert_called_once_with([str(job.id)], queue='job-tasks') - def test_post_letter_notification_returns_400_and_missing_template( client, @@ -220,15 +215,14 @@ def test_post_letter_notification_returns_403_if_not_allowed_to_send_notificatio (True, KEY_TYPE_NORMAL), (False, KEY_TYPE_TEST) ]) -def test_post_letter_notification_doesnt_queue_task( +def test_post_letter_notification_queues_success( client, notify_db_session, mocker, research_mode, key_type ): - real_task = mocker.patch('app.celery.tasks.build_dvla_file.apply_async') - fake_task = mocker.patch('app.celery.tasks.update_job_to_sent_to_dvla.apply_async') + fake_task = mocker.patch('app.celery.tasks.update_letter_notifications_to_sent_to_dvla.apply_async') service = create_service(research_mode=research_mode, service_permissions=[LETTER_TYPE]) template = create_template(service, template_type=LETTER_TYPE) @@ -240,10 +234,12 @@ def test_post_letter_notification_doesnt_queue_task( letter_request(client, data, service_id=service.id, key_type=key_type) - job = Job.query.one() - assert job.original_file_name == LETTER_TEST_API_FILENAME - assert not real_task.called - fake_task.assert_called_once_with([str(job.id)], queue='research-mode-tasks') + notification = Notification.query.one() + assert notification.status == NOTIFICATION_SENDING + fake_task.assert_called_once_with( + kwargs={'notification_references': [notification.reference]}, + queue='research-mode-tasks' + ) def test_post_letter_notification_doesnt_accept_team_key(client, sample_letter_template): @@ -282,17 +278,23 @@ def test_post_letter_notification_doesnt_send_in_trial(client, sample_trial_lett {'error': 'BadRequestError', 'message': 'Cannot send letters when service is in trial mode'}] -def test_post_letter_notification_calls_update_job_sent_to_dvla_when_service_is_in_trial_mode_but_using_test_key( - client, sample_trial_letter_template, mocker): - build_dvla_task = mocker.patch('app.celery.tasks.build_dvla_file.apply_async') - update_job_task = mocker.patch('app.celery.tasks.update_job_to_sent_to_dvla.apply_async') +def test_post_letter_notification_fakes_dvla_when_service_is_in_trial_mode_but_using_test_key( + client, + sample_trial_letter_template, + mocker +): + update_task = mocker.patch('app.celery.tasks.update_letter_notifications_to_sent_to_dvla.apply_async') data = { "template_id": sample_trial_letter_template.id, "personalisation": {'address_line_1': 'Foo', 'address_line_2': 'Bar', 'postcode': 'Baz'} } - letter_request(client, data=data, service_id=sample_trial_letter_template.service_id, - key_type=KEY_TYPE_TEST) - job = Job.query.one() - update_job_task.assert_called_once_with([str(job.id)], queue='research-mode-tasks') - assert not build_dvla_task.called + + letter_request(client, data=data, service_id=sample_trial_letter_template.service_id, key_type=KEY_TYPE_TEST) + + notification = Notification.query.one() + assert notification.status == NOTIFICATION_SENDING + update_task.assert_called_once_with( + kwargs={'notification_references': [notification.reference]}, + queue='research-mode-tasks' + ) From aaadf09562bc0b5b488ee65b49dfebafec385bb7 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 26 Sep 2017 12:03:06 +0100 Subject: [PATCH 10/12] add tests for new sched task --- app/celery/scheduled_tasks.py | 4 +- app/config.py | 4 +- app/dao/notifications_dao.py | 4 +- tests/app/celery/test_scheduled_tasks.py | 76 ++++++++++++++++++++++++ 4 files changed, 82 insertions(+), 6 deletions(-) diff --git a/app/celery/scheduled_tasks.py b/app/celery/scheduled_tasks.py index 432e92c58..f014c65ed 100644 --- a/app/celery/scheduled_tasks.py +++ b/app/celery/scheduled_tasks.py @@ -325,9 +325,9 @@ def run_letter_jobs(): current_app.logger.info("Queued {} ready letter job ids onto {}".format(len(job_ids), QueueNames.PROCESS_FTP)) -@notify_celery.task(name="run-letter-notifications") +@notify_celery.task(name="run-letter-api-notifications") @statsd(namespace="tasks") -def run_letter_notifications(): +def run_letter_api_notifications(): current_time = datetime.utcnow().isoformat() notifications = dao_set_created_live_letter_api_notifications_to_pending() diff --git a/app/config.py b/app/config.py index 6824df9c6..b020e0b9d 100644 --- a/app/config.py +++ b/app/config.py @@ -230,8 +230,8 @@ class Config(object): 'schedule': crontab(hour=5, minute=30), 'options': {'queue': QueueNames.PERIODIC} }, - 'run-letter-notifications': { - 'task': 'run-letter-notifications', + 'run-letter-api-notifications': { + 'task': 'run-letter-api-notifications', 'schedule': crontab(hour=5, minute=40), 'options': {'queue': QueueNames.PERIODIC} } diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index f7b15b4dc..fef7b9567 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -586,7 +586,7 @@ def dao_set_created_live_letter_api_notifications_to_pending(): 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. """ - return db.session.query( + notifications = db.session.query( Notification ).filter( Notification.notification_type == LETTER_TYPE, @@ -597,7 +597,7 @@ def dao_set_created_live_letter_api_notifications_to_pending(): ).all() for notification in notifications: - notification.notification_status = NOTIFICATION_PENDING + notification.status = NOTIFICATION_PENDING db.session.add_all(notifications) db.session.commit() diff --git a/tests/app/celery/test_scheduled_tasks.py b/tests/app/celery/test_scheduled_tasks.py index fca018597..6023a403e 100644 --- a/tests/app/celery/test_scheduled_tasks.py +++ b/tests/app/celery/test_scheduled_tasks.py @@ -21,6 +21,7 @@ from app.celery.scheduled_tasks import ( remove_transformed_dvla_files, run_scheduled_jobs, run_letter_jobs, + run_letter_api_notifications, s3, send_daily_performance_platform_stats, send_scheduled_notifications, @@ -41,6 +42,11 @@ from app.models import ( Service, Template, SMS_TYPE, LETTER_TYPE, JOB_STATUS_READY_TO_SEND, + JOB_STATUS_IN_PROGRESS, + JOB_STATUS_SENT_TO_DVLA, + NOTIFICATION_PENDING, + NOTIFICATION_CREATED, + KEY_TYPE_TEST, MonthlyBilling) from app.utils import get_london_midnight_in_utc from tests.app.db import create_notification, create_service, create_template, create_job, create_rate @@ -693,3 +699,73 @@ def test_run_letter_jobs(client, mocker, sample_letter_template): mock_celery.assert_called_once_with(name=TaskNames.DVLA_JOBS, args=(job_ids,), queue=QueueNames.PROCESS_FTP) + + +def test_run_letter_jobs_does_nothing_if_no_ready_jobs(client, mocker, sample_letter_template): + job_ids = [ + str(create_job(sample_letter_template, job_status=JOB_STATUS_IN_PROGRESS).id), + str(create_job(sample_letter_template, job_status=JOB_STATUS_SENT_TO_DVLA).id) + ] + + mock_celery = mocker.patch("app.celery.tasks.notify_celery.send_task") + + run_letter_jobs() + + assert not mock_celery.called + + +def test_run_letter_api_notifications_triggers_ftp_task(client, mocker, sample_letter_notification): + file_contents_mock = mocker.patch( + 'app.celery.scheduled_tasks.create_dvla_file_contents_for_notifications', + return_value='foo\nbar' + ) + s3upload = mocker.patch('app.celery.scheduled_tasks.s3upload') + mock_celery = mocker.patch('app.celery.tasks.notify_celery.send_task') + filename = '2017-01-01T12:00:00-dvla-notifications.txt' + + with freeze_time('2017-01-01 12:00:00'): + run_letter_api_notifications() + + assert sample_letter_notification.status == NOTIFICATION_PENDING + file_contents_mock.assert_called_once_with([sample_letter_notification]) + s3upload.assert_called_once_with( + # with trailing new line added + filedata='foo\nbar\n', + region='eu-west-1', + bucket_name='test-dvla-letter-api-files', + file_location=filename + ) + mock_celery.assert_called_once_with( + name=TaskNames.DVLA_NOTIFICATIONS, + kwargs={'filename': filename}, + queue=QueueNames.PROCESS_FTP + ) + +def test_run_letter_api_notifications_does_nothing_if_no_created_notifications( + client, + mocker, + sample_letter_template, + sample_letter_job, + sample_api_key +): + letter_job_notification = create_notification( + sample_letter_template, + job=sample_letter_job + ) + pending_letter_notification = create_notification( + sample_letter_template, + status=NOTIFICATION_PENDING, + api_key=sample_api_key + ) + test_api_key_notification = create_notification( + sample_letter_template, + key_type=KEY_TYPE_TEST + ) + + mock_celery = mocker.patch('app.celery.tasks.notify_celery.send_task') + + run_letter_api_notifications() + + assert not mock_celery.called + assert letter_job_notification.status == NOTIFICATION_CREATED + assert test_api_key_notification.status == NOTIFICATION_CREATED From 18ed90158e8905d07f74d535c0e57a86766aef58 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 26 Sep 2017 12:16:33 +0100 Subject: [PATCH 11/12] add tests for new noti dao function --- .../test_letter_api_notification_dao.py | 51 +++++++++++++++++++ .../notification_dao/test_notification_dao.py | 5 +- 2 files changed, 55 insertions(+), 1 deletion(-) create mode 100644 tests/app/dao/notification_dao/test_letter_api_notification_dao.py 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 new file mode 100644 index 000000000..41869b15e --- /dev/null +++ b/tests/app/dao/notification_dao/test_letter_api_notification_dao.py @@ -0,0 +1,51 @@ +from app.models import ( + NOTIFICATION_CREATED, + NOTIFICATION_PENDING, + NOTIFICATION_SENDING, + KEY_TYPE_TEST, + KEY_TYPE_NORMAL +) +from app.dao.notifications_dao import dao_set_created_live_letter_api_notifications_to_pending + +from tests.app.db import create_notification + + +def test_should_only_get_letter_notifications( + sample_letter_notification, + sample_email_notification, + sample_notification +): + ret = dao_set_created_live_letter_api_notifications_to_pending() + + assert sample_letter_notification.status == NOTIFICATION_PENDING + assert sample_email_notification.status == NOTIFICATION_CREATED + assert sample_notification.status == NOTIFICATION_CREATED + 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) + create_notification(sample_letter_template, status=NOTIFICATION_SENDING) + + ret = dao_set_created_live_letter_api_notifications_to_pending() + + assert ret == [created_noti] + + +def test_should_only_get_api_letters(sample_letter_template, sample_letter_job): + api_noti = create_notification(sample_letter_template) + job_noti = create_notification(sample_letter_template, job=sample_letter_job) + + ret = dao_set_created_live_letter_api_notifications_to_pending() + + assert ret == [api_noti] + + +def test_should_only_get_normal_api_letters(sample_letter_template): + live_noti = create_notification(sample_letter_template, key_type=KEY_TYPE_NORMAL) + test_noti = create_notification(sample_letter_template, key_type=KEY_TYPE_TEST) + + ret = dao_set_created_live_letter_api_notifications_to_pending() + + assert ret == [live_noti] diff --git a/tests/app/dao/notification_dao/test_notification_dao.py b/tests/app/dao/notification_dao/test_notification_dao.py index 2b4e9479c..ba8378827 100644 --- a/tests/app/dao/notification_dao/test_notification_dao.py +++ b/tests/app/dao/notification_dao/test_notification_dao.py @@ -42,7 +42,10 @@ from app.dao.notifications_dao import ( is_delivery_slow_for_provider, dao_update_notifications_for_job_to_sent_to_dvla, dao_get_notifications_by_to_field, - dao_created_scheduled_notification, dao_get_scheduled_notifications, set_scheduled_notification_to_processed) + dao_created_scheduled_notification, + dao_get_scheduled_notifications, + set_scheduled_notification_to_processed +) from app.dao.services_dao import dao_update_service from tests.app.db import create_notification, create_api_key From c97b1305409d473e6cc52f0b5c232cd07a2e7a9e Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 26 Sep 2017 12:33:59 +0100 Subject: [PATCH 12/12] test dvla callback update tasks --- tests/app/celery/test_ftp_update_tasks.py | 127 ++++++++++++++++++++++ tests/app/celery/test_scheduled_tasks.py | 1 + tests/app/celery/test_tasks.py | 78 +------------ tests/app/db.py | 6 +- 4 files changed, 136 insertions(+), 76 deletions(-) create mode 100644 tests/app/celery/test_ftp_update_tasks.py diff --git a/tests/app/celery/test_ftp_update_tasks.py b/tests/app/celery/test_ftp_update_tasks.py new file mode 100644 index 000000000..05680d32b --- /dev/null +++ b/tests/app/celery/test_ftp_update_tasks.py @@ -0,0 +1,127 @@ +from datetime import datetime + +import pytest +from freezegun import freeze_time +from flask import current_app + +from app.models import ( + Job, + Notification, + NOTIFICATION_SENDING, + NOTIFICATION_CREATED, + NOTIFICATION_TECHNICAL_FAILURE +) +from app.celery.tasks import ( + update_job_to_sent_to_dvla, + update_dvla_job_to_error, + update_letter_notifications_statuses, + update_letter_notifications_to_sent_to_dvla, + update_letter_notifications_to_error, + process_updates_from_file +) + +from tests.app.db import create_notification +from tests.conftest import set_config + + +def test_update_job_to_sent_to_dvla(sample_letter_template, sample_letter_job): + create_notification(template=sample_letter_template, job=sample_letter_job) + create_notification(template=sample_letter_template, job=sample_letter_job) + update_job_to_sent_to_dvla(job_id=sample_letter_job.id) + + updated_notifications = Notification.query.all() + assert [(n.status == 'sending', n.sent_by == 'dvla') for n in updated_notifications] + + assert Job.query.filter_by(id=sample_letter_job.id).one().job_status == 'sent to dvla' + + +def test_update_dvla_job_to_error(sample_letter_template, sample_letter_job): + create_notification(template=sample_letter_template, job=sample_letter_job) + create_notification(template=sample_letter_template, job=sample_letter_job) + update_dvla_job_to_error(job_id=sample_letter_job.id) + + updated_notifications = Notification.query.all() + for n in updated_notifications: + assert n.status == 'created' + assert not n.sent_by + + assert Job.query.filter_by(id=sample_letter_job.id).one().job_status == 'error' + + +def test_update_letter_notifications_statuses_raises_for_invalid_format(notify_api, mocker): + invalid_file = 'ref-foo|Sent|1|Unsorted\nref-bar|Sent|2' + mocker.patch('app.celery.tasks.s3.get_s3_file', return_value=invalid_file) + + with pytest.raises(TypeError): + update_letter_notifications_statuses(filename='foo.txt') + + +def test_update_letter_notifications_statuses_calls_with_correct_bucket_location(notify_api, mocker): + s3_mock = mocker.patch('app.celery.tasks.s3.get_s3_object') + + with set_config(notify_api, 'NOTIFY_EMAIL_DOMAIN', 'foo.bar'): + update_letter_notifications_statuses(filename='foo.txt') + s3_mock.assert_called_with('{}-ftp'.format(current_app.config['NOTIFY_EMAIL_DOMAIN']), 'foo.txt') + + +def test_update_letter_notifications_statuses_builds_updates_from_content(notify_api, mocker): + valid_file = 'ref-foo|Sent|1|Unsorted\nref-bar|Sent|2|Sorted' + mocker.patch('app.celery.tasks.s3.get_s3_file', return_value=valid_file) + update_mock = mocker.patch('app.celery.tasks.process_updates_from_file') + + update_letter_notifications_statuses(filename='foo.txt') + + update_mock.assert_called_with('ref-foo|Sent|1|Unsorted\nref-bar|Sent|2|Sorted') + + +def test_update_letter_notifications_statuses_builds_updates_list(notify_api, mocker): + valid_file = 'ref-foo|Sent|1|Unsorted\nref-bar|Sent|2|Sorted' + updates = process_updates_from_file(valid_file) + + assert len(updates) == 2 + + assert updates[0].reference == 'ref-foo' + assert updates[0].status == 'Sent' + assert updates[0].page_count == '1' + assert updates[0].cost_threshold == 'Unsorted' + + assert updates[1].reference == 'ref-bar' + assert updates[1].status == 'Sent' + assert updates[1].page_count == '2' + assert updates[1].cost_threshold == 'Sorted' + + +def test_update_letter_notifications_to_sent_to_dvla_updates_based_on_notification_references( + client, + sample_letter_template +): + first = create_notification(sample_letter_template) + second = create_notification(sample_letter_template) + + dt = datetime.utcnow() + with freeze_time(dt): + update_letter_notifications_to_sent_to_dvla([first.reference]) + + assert first.status == NOTIFICATION_SENDING + assert first.sent_by == 'dvla' + assert first.sent_at == dt + assert first.updated_at == dt + assert second.status == NOTIFICATION_CREATED + + +def test_update_letter_notifications_to_error_updates_based_on_notification_references( + client, + sample_letter_template +): + first = create_notification(sample_letter_template) + second = create_notification(sample_letter_template) + + dt = datetime.utcnow() + with freeze_time(dt): + update_letter_notifications_to_error([first.reference]) + + assert first.status == NOTIFICATION_TECHNICAL_FAILURE + assert first.sent_by is None + assert first.sent_at is None + assert first.updated_at == dt + assert second.status == NOTIFICATION_CREATED diff --git a/tests/app/celery/test_scheduled_tasks.py b/tests/app/celery/test_scheduled_tasks.py index 6023a403e..9d770888b 100644 --- a/tests/app/celery/test_scheduled_tasks.py +++ b/tests/app/celery/test_scheduled_tasks.py @@ -741,6 +741,7 @@ def test_run_letter_api_notifications_triggers_ftp_task(client, mocker, sample_l queue=QueueNames.PROCESS_FTP ) + def test_run_letter_api_notifications_does_nothing_if_no_created_notifications( client, mocker, diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 48b4ac3b7..e16f819ea 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -25,9 +25,6 @@ from app.celery.tasks import ( send_email, persist_letter, get_template_class, - update_job_to_sent_to_dvla, - update_letter_notifications_statuses, - process_updates_from_file, send_inbound_sms_to_service) from app.config import QueueNames from app.dao import jobs_dao, services_dao @@ -39,11 +36,9 @@ from app.models import ( SMS_TYPE, EMAIL_TYPE, LETTER_TYPE, - Job, - JOB_STATUS_ERROR) + Job) from tests.app import load_example_csv -from tests.conftest import set_config from tests.app.conftest import ( sample_service as create_sample_service, sample_template as create_sample_template, @@ -642,7 +637,7 @@ def test_should_update_job_to_sent_to_dvla_in_research_mode_for_letter_job( A1,A2,A3,A4,A_POST,Alice """ mocker.patch('app.celery.tasks.s3.get_job_from_s3', return_value=csv) - mocker.patch('app.celery.tasks.update_job_to_sent_to_dvla.apply_async') + mock_update_job_task = mocker.patch('app.celery.tasks.update_job_to_sent_to_dvla.apply_async') mocker.patch('app.celery.tasks.persist_letter.apply_async') mocker.patch('app.celery.tasks.create_uuid', return_value=fake_uuid) mocker.patch('app.celery.tasks.encryption.encrypt', return_value=test_encrypted_data) @@ -662,7 +657,7 @@ def test_should_update_job_to_sent_to_dvla_in_research_mode_for_letter_job( queue=QueueNames.RESEARCH_MODE ) - update_job_to_sent_to_dvla.apply_async.assert_called_once_with( + mock_update_job_task.assert_called_once_with( [str(job.id)], queue=QueueNames.RESEARCH_MODE) @@ -1114,73 +1109,6 @@ def test_dvla_letter_template(sample_letter_notification): assert str(letter) == "140|500|001||random-string|||||||||||||A1||A2|A3|A4|A5|A6|A_POST|||||||||23 March 2017

Template subjectDear Sir/Madam, Hello. Yours Truly, The Government." # noqa -def test_update_job_to_sent_to_dvla(sample_letter_template, sample_letter_job): - create_notification(template=sample_letter_template, job=sample_letter_job) - create_notification(template=sample_letter_template, job=sample_letter_job) - update_job_to_sent_to_dvla(job_id=sample_letter_job.id) - - updated_notifications = Notification.query.all() - assert [(n.status == 'sending', n.sent_by == 'dvla') for n in updated_notifications] - - assert 'sent to dvla' == Job.query.filter_by(id=sample_letter_job.id).one().job_status - - -def test_update_dvla_job_to_error(sample_letter_template, sample_letter_job): - create_notification(template=sample_letter_template, job=sample_letter_job) - create_notification(template=sample_letter_template, job=sample_letter_job) - update_dvla_job_to_error(job_id=sample_letter_job.id) - - updated_notifications = Notification.query.all() - for n in updated_notifications: - assert n.status == 'created' - assert not n.sent_by - - assert 'error' == Job.query.filter_by(id=sample_letter_job.id).one().job_status - - -def test_update_letter_notifications_statuses_raises_for_invalid_format(notify_api, mocker): - invalid_file = 'ref-foo|Sent|1|Unsorted\nref-bar|Sent|2' - mocker.patch('app.celery.tasks.s3.get_s3_file', return_value=invalid_file) - - with pytest.raises(TypeError): - update_letter_notifications_statuses(filename='foo.txt') - - -def test_update_letter_notifications_statuses_calls_with_correct_bucket_location(notify_api, mocker): - s3_mock = mocker.patch('app.celery.tasks.s3.get_s3_object') - - with set_config(notify_api, 'NOTIFY_EMAIL_DOMAIN', 'foo.bar'): - update_letter_notifications_statuses(filename='foo.txt') - s3_mock.assert_called_with('{}-ftp'.format(current_app.config['NOTIFY_EMAIL_DOMAIN']), 'foo.txt') - - -def test_update_letter_notifications_statuses_builds_updates_from_content(notify_api, mocker): - valid_file = 'ref-foo|Sent|1|Unsorted\nref-bar|Sent|2|Sorted' - mocker.patch('app.celery.tasks.s3.get_s3_file', return_value=valid_file) - update_mock = mocker.patch('app.celery.tasks.process_updates_from_file') - - update_letter_notifications_statuses(filename='foo.txt') - - update_mock.assert_called_with('ref-foo|Sent|1|Unsorted\nref-bar|Sent|2|Sorted') - - -def test_update_letter_notifications_statuses_builds_updates_list(notify_api, mocker): - valid_file = 'ref-foo|Sent|1|Unsorted\nref-bar|Sent|2|Sorted' - updates = process_updates_from_file(valid_file) - - assert len(updates) == 2 - - assert updates[0].reference == 'ref-foo' - assert updates[0].status == 'Sent' - assert updates[0].page_count == '1' - assert updates[0].cost_threshold == 'Unsorted' - - assert updates[1].reference == 'ref-bar' - assert updates[1].status == 'Sent' - assert updates[1].page_count == '2' - assert updates[1].cost_threshold == 'Sorted' - - def test_send_inbound_sms_to_service_post_https_request_to_service(notify_api, sample_service): inbound_api = create_service_inbound_api(service=sample_service, url="https://some.service.gov.uk/", bearer_token="something_unique") diff --git a/tests/app/db.py b/tests/app/db.py index f271d70e9..b3b6ba61c 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -1,7 +1,7 @@ from datetime import datetime import uuid -from app import db +from app import db, create_random_identifier from app.dao.jobs_dao import dao_create_job from app.dao.service_inbound_api_dao import save_service_inbound_api from app.models import ( @@ -22,6 +22,7 @@ from app.models import ( InboundNumber, Organisation, EMAIL_TYPE, + LETTER_TYPE, SMS_TYPE, INBOUND_SMS_TYPE, KEY_TYPE_NORMAL, @@ -142,6 +143,9 @@ def create_notification( if not api_key: api_key = create_api_key(template.service, key_type=key_type) + if template.template_type == LETTER_TYPE and reference is None: + reference = create_random_identifier() + data = { 'id': uuid.uuid4(), 'to': to_field,