From f9a7a78e55ff192d25f616bb7beee086b2efcfec Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Wed, 11 Oct 2017 21:48:47 +0100 Subject: [PATCH] Refactor code - removed redundant dao method - also handle letter jobs --- app/celery/tasks.py | 8 ++++---- app/dao/notifications_dao.py | 10 ---------- tests/app/celery/test_tasks.py | 13 +++++++++++-- .../dao/notification_dao/test_notification_dao.py | 11 ----------- 4 files changed, 15 insertions(+), 27 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index d73fe93d7..9122302d0 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -29,7 +29,6 @@ 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 ) @@ -89,8 +88,7 @@ def process_job(job_id): current_app.logger.info("Starting job {} processing {} notifications".format(job_id, job.notification_count)) - if template.template_type != LETTER_TYPE: - check_job_status.apply_async([str(job.id)], queue=QueueNames.JOBS) + check_job_status.apply_async([str(job.id)], queue=QueueNames.JOBS) for row_number, recipient, personalisation in RecipientCSV( s3.get_job_from_s3(str(service.id), str(job_id)), @@ -334,7 +332,9 @@ def update_dvla_job_to_error(self, job_id): @statsd(namespace="tasks") def check_job_status(self, job_id): job = dao_get_job_by_id(job_id) - if job.job_status != JOB_STATUS_FINISHED: + + if (job.template.template_type == LETTER_TYPE and job.job_status != JOB_STATUS_SENT_TO_DVLA) or\ + (job.template.template_type != LETTER_TYPE and job.job_status != JOB_STATUS_FINISHED): raise JobIncompleteError("Job {} did not complete".format(job_id)) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 72f9a9a89..4183c6f4c 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -442,16 +442,6 @@ 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 beb62a780..3b787e05e 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -32,6 +32,7 @@ from app.models import ( EMAIL_TYPE, JOB_STATUS_ERROR, JOB_STATUS_FINISHED, + JOB_STATUS_SENT_TO_DVLA, KEY_TYPE_NORMAL, KEY_TYPE_TEAM, KEY_TYPE_TEST, @@ -1234,6 +1235,14 @@ def test_job_incomplete_raises_job_incomplete_error(sample_job): assert e.value.message == 'Job {} did not complete'.format(sample_job.id) -def test_job_finished_does_not_raises_job_incomplete_error(sample_job): - sample_job.job_status = JOB_STATUS_FINISHED +@pytest.mark.parametrize('job_status,notification_type', + [ + (JOB_STATUS_FINISHED, SMS_TYPE), + (JOB_STATUS_SENT_TO_DVLA, LETTER_TYPE) + ] +) +def test_job_finished_does_not_raises_job_incomplete_error( + sample_job, job_status, notification_type): + sample_job.job_status = job_status + sample_job.template.template_type = notification_type check_job_status(str(sample_job.id)) diff --git a/tests/app/dao/notification_dao/test_notification_dao.py b/tests/app/dao/notification_dao/test_notification_dao.py index 818b44141..5ec851edc 100644 --- a/tests/app/dao/notification_dao/test_notification_dao.py +++ b/tests/app/dao/notification_dao/test_notification_dao.py @@ -35,7 +35,6 @@ 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, @@ -2001,13 +2000,3 @@ 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