From dcf0d22d7b3772632d016eac89ae0cea0c1b367d Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Tue, 10 Oct 2017 15:04:55 +0100 Subject: [PATCH] Added alert when job.notification_count doesn't match total notification for job - Added log for when a job starts so that we will know when the processing of a job starts with the number of notifications - Added dao method to get total notifications for a job id - Added a test to check whether the number of notifications in the table matches the job notification_count --- app/celery/tasks.py | 24 +++++++++++++------ app/dao/notifications_dao.py | 10 ++++++++ tests/app/celery/test_tasks.py | 23 ++++++++++++++++++ .../notification_dao/test_notification_dao.py | 18 +++++++++++++- 4 files changed, 67 insertions(+), 8 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index a64bb9cae..599d7b972 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -29,6 +29,7 @@ from app.dao.jobs_dao import ( dao_update_job_status) from app.dao.notifications_dao import ( get_notification_by_id, + dao_get_total_notifications_for_job_id, dao_update_notifications_for_job_to_sent_to_dvla, dao_update_notifications_by_reference ) @@ -85,6 +86,8 @@ def process_job(job_id): TemplateClass = get_template_class(db_template.template_type) template = TemplateClass(db_template.__dict__) + current_app.logger.info("Starting job {} processing {} notifications".format(job_id, job.notification_count)) + for row_number, recipient, personalisation in RecipientCSV( s3.get_job_from_s3(str(service.id), str(job_id)), template_type=template.template_type, @@ -92,14 +95,21 @@ def process_job(job_id): ).enumerated_recipients_and_personalisation: process_row(row_number, recipient, personalisation, template, job, service) - if template.template_type == LETTER_TYPE: - if service.research_mode: - update_job_to_sent_to_dvla.apply_async([str(job.id)], queue=QueueNames.RESEARCH_MODE) - else: - build_dvla_file.apply_async([str(job.id)], queue=QueueNames.JOBS) - current_app.logger.info("send job {} to build-dvla-file in the {} queue".format(job_id, QueueNames.JOBS)) + notification_total_in_db = dao_get_total_notifications_for_job_id(job.id) + + if job.notification_count != notification_total_in_db: + current_app.logger.error("Job {} is missing {} notifications".format( + job.id, notification_total_in_db - notification_total_in_db)) + job.job_status = JOB_STATUS_ERROR else: - job.job_status = JOB_STATUS_FINISHED + if template.template_type == LETTER_TYPE: + if service.research_mode: + update_job_to_sent_to_dvla.apply_async([str(job.id)], queue=QueueNames.RESEARCH_MODE) + else: + build_dvla_file.apply_async([str(job.id)], queue=QueueNames.JOBS) + current_app.logger.info("send job {} to build-dvla-file in the {} queue".format(job_id, QueueNames.JOBS)) + else: + job.job_status = JOB_STATUS_FINISHED finished = datetime.utcnow() job.processing_started = start diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 4183c6f4c..72f9a9a89 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -442,6 +442,16 @@ def get_total_sent_notifications_in_date_range(start_date, end_date, notificatio return result or 0 +def dao_get_total_notifications_for_job_id(job_id): + result = db.session.query( + func.count(Notification.id).label('count') + ).filter( + Notification.job_id == job_id + ).scalar() + + return result or 0 + + def is_delivery_slow_for_provider( sent_at, provider, diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 574ed2eb7..cc01fc25c 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -360,6 +360,29 @@ def test_should_process_all_sms_job(sample_job_with_placeholdered_template, assert job.job_status == 'finished' +def test_should_error_log_missing_notifications( + sample_job_with_placeholdered_template, mocker): + multiple_sms = load_example_csv('multiple_sms').strip() + num_phone_numbers_after_header = len(multiple_sms.split('\n')[1:]) + sample_job_with_placeholdered_template.notification_count = num_phone_numbers_after_header + + mocker.patch('app.celery.tasks.s3.get_job_from_s3', return_value=multiple_sms) + mocker.patch('app.celery.tasks.send_sms.apply_async') + mocker.patch('app.encryption.encrypt', return_value="something_encrypted") + mocker.patch('app.celery.tasks.create_uuid', return_value="uuid") + # deliberately return wrong total notifications to trigger error log + mocker.patch( + 'app.celery.tasks.dao_get_total_notifications_for_job_id', + return_value=num_phone_numbers_after_header - 1 + ) + error_log = mocker.patch('app.celery.tasks.current_app.logger.error') + + process_job(sample_job_with_placeholdered_template.id) + + job = jobs_dao.dao_get_job_by_id(sample_job_with_placeholdered_template.id) + assert job.job_status == 'error' + assert error_log.called + # -------------- process_row tests -------------- # diff --git a/tests/app/dao/notification_dao/test_notification_dao.py b/tests/app/dao/notification_dao/test_notification_dao.py index 3124b3a13..818b44141 100644 --- a/tests/app/dao/notification_dao/test_notification_dao.py +++ b/tests/app/dao/notification_dao/test_notification_dao.py @@ -35,6 +35,7 @@ from app.dao.notifications_dao import ( dao_get_potential_notification_statistics_for_day, dao_get_scheduled_notifications, dao_get_template_usage, + dao_get_total_notifications_for_job_id, dao_timeout_notifications, dao_update_notification, dao_update_notifications_for_job_to_sent_to_dvla, @@ -52,7 +53,12 @@ from app.dao.notifications_dao import ( ) from app.dao.services_dao import dao_update_service -from tests.app.db import create_notification, create_api_key, create_reply_to_email +from tests.app.db import ( + create_api_key, + create_job, + create_notification, + create_reply_to_email +) from tests.app.conftest import ( sample_notification, sample_template, @@ -1995,3 +2001,13 @@ def test_dao_get_notification_ememail_reply_toail_reply_for_notification(sample_ def test_dao_get_notification_email_reply_for_notification_where_no_mapping(notify_db_session, fake_uuid): assert dao_get_notification_email_reply_for_notification(fake_uuid) is None + + +def test_dao_get_total_notifications_for_job_id(sample_job): + job = create_job(sample_job.template) + create_notification(sample_job.template, job=sample_job) + create_notification(sample_job.template, job=sample_job) + create_notification(sample_job.template, job=sample_job) + create_notification(sample_job.template, job=job) + + assert dao_get_total_notifications_for_job_id(sample_job.id) == 3