From f1b04193ca6b9a6a65f2a96334d207155c09b4a2 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Wed, 12 Sep 2018 17:16:34 +0100 Subject: [PATCH] In this PR we remove trigger-letter-pdfs-for-day scheduled task and just call collate_letter_pdfs_for_day instead. There was a datetime bug in the query which resulted in files not being sent to the postal provider. The trigger-letter-pdfs-for-day task is no longer needed, so rather than fix the query just call collate_letter_pdfs_for_day directly. Less code is always better. Deployment considerations: I realized this is strictly not backwards compatible if the scheduled job is in progress and a task is on the queue that no longer exists. This is ok since we will deploy this well before 17:50. --- app/celery/letters_pdf_tasks.py | 7 +- app/celery/scheduled_tasks.py | 16 ---- app/config.py | 6 +- app/dao/notifications_dao.py | 33 -------- tests/app/celery/test_letters_pdf_tasks.py | 8 ++ tests/app/celery/test_scheduled_tasks.py | 27 ------- .../notification_dao/test_notification_dao.py | 77 ------------------- 7 files changed, 18 insertions(+), 156 deletions(-) diff --git a/app/celery/letters_pdf_tasks.py b/app/celery/letters_pdf_tasks.py index 2df7d680e..7efe6ef2c 100644 --- a/app/celery/letters_pdf_tasks.py +++ b/app/celery/letters_pdf_tasks.py @@ -105,7 +105,12 @@ def get_letters_pdf(template, contact_block, org_id, values): @notify_celery.task(name='collate-letter-pdfs-for-day') -def collate_letter_pdfs_for_day(date): +def collate_letter_pdfs_for_day(date=None): + if not date: + # Using the truncated date is ok because UTC to BST does not make a difference to the date, + # since it is triggered mid afternoon. + date = datetime.utcnow().strftime("%Y-%m-%d") + letter_pdfs = s3.get_s3_bucket_objects( current_app.config['LETTERS_PDF_BUCKET_NAME'], subfolder=date diff --git a/app/celery/scheduled_tasks.py b/app/celery/scheduled_tasks.py index 0b0b6ad85..8c8577b86 100644 --- a/app/celery/scheduled_tasks.py +++ b/app/celery/scheduled_tasks.py @@ -1,5 +1,4 @@ from datetime import ( - date, datetime, timedelta ) @@ -32,7 +31,6 @@ from app.dao.notifications_dao import ( dao_timeout_notifications, is_delivery_slow_for_provider, delete_notifications_created_more_than_a_week_ago_by_type, - dao_get_count_of_letters_to_process_for_date, dao_get_scheduled_notifications, set_scheduled_notification_to_processed, notifications_not_yet_sent @@ -385,20 +383,6 @@ 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="trigger-letter-pdfs-for-day") -@statsd(namespace="tasks") -def trigger_letter_pdfs_for_day(): - letter_pdfs_count = dao_get_count_of_letters_to_process_for_date() - if letter_pdfs_count: - notify_celery.send_task( - name='collate-letter-pdfs-for-day', - args=(date.today().strftime("%Y-%m-%d"),), - queue=QueueNames.LETTERS - ) - current_app.logger.info("{} letter pdfs to be process by {} task".format( - letter_pdfs_count, 'collate-letter-pdfs-for-day')) - - @notify_celery.task(name='check-job-status') @statsd(namespace="tasks") def check_job_status(): diff --git a/app/config.py b/app/config.py index 419f1bb65..ef5fef795 100644 --- a/app/config.py +++ b/app/config.py @@ -242,8 +242,10 @@ class Config(object): 'schedule': crontab(hour=16, minute=30), 'options': {'queue': QueueNames.PERIODIC} }, - 'trigger-letter-pdfs-for-day': { - 'task': 'trigger-letter-pdfs-for-day', + # The collate-letter-pdf does assume it is called in an hour that BST does not make a + # difference to the truncate date which translates to the filename to process + 'collate-letter-pdfs-for-day': { + 'task': 'collate-letter-pdfs-for-day', 'schedule': crontab(hour=17, minute=50), 'options': {'queue': QueueNames.PERIODIC} }, diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 1bc683813..95896e4da 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -3,7 +3,6 @@ import string from datetime import ( datetime, timedelta, - date ) from boto.exception import BotoClientError @@ -31,7 +30,6 @@ from app.models import ( Notification, NotificationHistory, ScheduledNotification, - Service, Template, TemplateHistory, KEY_TYPE_TEST, @@ -607,37 +605,6 @@ def dao_get_last_notification_added_for_job_id(job_id): return last_notification_added -def dao_get_count_of_letters_to_process_for_date(date_to_process=None): - """ - Returns a count of letter notifications to be processed today if no - argument passed in otherwise will return the count for the date passed in. - Records processed today = yesterday 17:30 to today 17:29:59 - - Note - services in research mode are ignored - """ - if date_to_process is None: - date_to_process = date.today() - - day_before = date_to_process - timedelta(days=1) - letter_deadline_time = current_app.config.get('LETTER_PROCESSING_DEADLINE') - - start_datetime = datetime.combine(day_before, letter_deadline_time) - end_datetime = start_datetime + timedelta(days=1) - - count_of_letters_to_process_for_date = Notification.query.join( - Service - ).filter( - Notification.created_at >= start_datetime, - Notification.created_at < end_datetime, - Notification.notification_type == LETTER_TYPE, - Notification.status == NOTIFICATION_CREATED, - Notification.key_type != KEY_TYPE_TEST, - Service.research_mode.is_(False) - ).count() - - return count_of_letters_to_process_for_date - - def notifications_not_yet_sent(should_be_sending_after_seconds, notification_type): older_than_date = datetime.utcnow() - timedelta(seconds=should_be_sending_after_seconds) diff --git a/tests/app/celery/test_letters_pdf_tasks.py b/tests/app/celery/test_letters_pdf_tasks.py index 88262961c..53c54d37e 100644 --- a/tests/app/celery/test_letters_pdf_tasks.py +++ b/tests/app/celery/test_letters_pdf_tasks.py @@ -199,6 +199,14 @@ def test_collate_letter_pdfs_for_day(notify_api, mocker): ) +@freeze_time('2018-09-12 17:50:00') +def test_collate_letter_pdfs_for_day_works_without_date_param(notify_api, mocker): + mock_s3 = mocker.patch('app.celery.tasks.s3.get_s3_bucket_objects') + collate_letter_pdfs_for_day() + expected_date = '2018-09-12' + mock_s3.assert_called_once_with('test-letters-pdf', subfolder=expected_date) + + def test_group_letters_splits_on_file_size(notify_api, mocker): mocker.patch('app.celery.letters_pdf_tasks.letter_in_created_state', return_value=True) letters = [ diff --git a/tests/app/celery/test_scheduled_tasks.py b/tests/app/celery/test_scheduled_tasks.py index 35b4bf2e7..44d8ce896 100644 --- a/tests/app/celery/test_scheduled_tasks.py +++ b/tests/app/celery/test_scheduled_tasks.py @@ -26,7 +26,6 @@ from app.celery.scheduled_tasks import ( remove_transformed_dvla_files, run_scheduled_jobs, run_letter_jobs, - trigger_letter_pdfs_for_day, s3, send_daily_performance_platform_stats, send_scheduled_notifications, @@ -763,32 +762,6 @@ def test_run_letter_jobs(client, mocker, sample_letter_template): queue=QueueNames.PROCESS_FTP) -@freeze_time("2017-12-18 17:50") -def test_trigger_letter_pdfs_for_day(client, mocker, sample_letter_template): - create_notification(template=sample_letter_template, created_at='2017-12-17 17:30:00') - create_notification(template=sample_letter_template, created_at='2017-12-18 17:29:59') - - mock_celery = mocker.patch("app.celery.tasks.notify_celery.send_task") - - trigger_letter_pdfs_for_day() - - mock_celery.assert_called_once_with(name='collate-letter-pdfs-for-day', - args=('2017-12-18',), - queue=QueueNames.LETTERS) - - -@freeze_time("2017-12-18 17:50") -def test_trigger_letter_pdfs_for_day_send_task_not_called_if_no_notis_for_day( - client, mocker, sample_letter_template): - create_notification(template=sample_letter_template, created_at='2017-12-15 17:30:00') - - mock_celery = mocker.patch("app.celery.tasks.notify_celery.send_task") - - trigger_letter_pdfs_for_day() - - assert not mock_celery.called - - def test_run_letter_jobs_does_nothing_if_no_ready_jobs(client, mocker, sample_letter_template): create_job(sample_letter_template, job_status=JOB_STATUS_IN_PROGRESS) create_job(sample_letter_template, job_status=JOB_STATUS_SENT_TO_DVLA) diff --git a/tests/app/dao/notification_dao/test_notification_dao.py b/tests/app/dao/notification_dao/test_notification_dao.py index 00333179c..029d41e34 100644 --- a/tests/app/dao/notification_dao/test_notification_dao.py +++ b/tests/app/dao/notification_dao/test_notification_dao.py @@ -10,7 +10,6 @@ from app.dao.notifications_dao import ( dao_create_notification, dao_created_scheduled_notification, dao_delete_notifications_and_history_by_id, - dao_get_count_of_letters_to_process_for_date, dao_get_last_notification_added_for_job_id, dao_get_last_template_usage, dao_get_notifications_by_to_field, @@ -1760,82 +1759,6 @@ def test_dao_get_notification_history_by_reference_with_no_matches_raises_error( dao_get_notification_history_by_reference('REF1') -@freeze_time("2017-12-18 17:50") -def test_dao_get_count_of_letters_to_process_for_today(sample_letter_template): - # expected - create_notification(template=sample_letter_template, created_at='2017-12-17 17:30:00') - create_notification(template=sample_letter_template, created_at='2017-12-18 17:29:59') - - # not expected - create_notification(template=sample_letter_template, created_at='2017-12-17 17:29:59') - create_notification(template=sample_letter_template, created_at='2017-12-18 17:30:00') - - count_for_date = dao_get_count_of_letters_to_process_for_date() - - assert count_for_date == 2 - - -@freeze_time("2017-12-18 17:50") -def test_dao_get_count_of_letters_to_process_for_date_in_past(sample_letter_template): - # expected - create_notification(template=sample_letter_template, created_at='2017-12-15 17:29:59') - - # not expected - create_notification(template=sample_letter_template, created_at='2017-12-15 17:30:00') - create_notification(template=sample_letter_template, created_at='2017-12-18 17:29:00') - - count_for_date = dao_get_count_of_letters_to_process_for_date(date(2017, 12, 15)) - - assert count_for_date == 1 - - -@freeze_time("2017-12-18 17:50") -def test_dao_get_count_of_letters_to_process_for_date_in_future_does_not_raise_error(sample_letter_template): - # not expected - create_notification(template=sample_letter_template, created_at='2017-12-18 17:30:00') - create_notification(template=sample_letter_template, created_at='2017-12-19 17:29:59') - - count_for_date = dao_get_count_of_letters_to_process_for_date(date(2017, 12, 20)) - - assert count_for_date == 0 - - -def test_dao_get_count_of_letters_to_process_for_today_without_notis_does_not_raise_error(sample_letter_template): - count_for_date = dao_get_count_of_letters_to_process_for_date() - - assert count_for_date == 0 - - -@freeze_time("2017-12-18 17:50") -def test_dao_get_count_of_letters_to_process_for_date_ignores_research_mode_services(sample_letter_template): - research_service = create_service(service_name='research service', research_mode=True) - research_template = create_template(research_service, template_type='letter') - - # not expected - create_notification(template=research_template, created_at='2017-12-18 17:29:00') - - # expected - create_notification(template=sample_letter_template, created_at='2017-12-18 17:29:10') - create_notification(template=sample_letter_template, created_at='2017-12-18 17:29:20') - - count_for_date = dao_get_count_of_letters_to_process_for_date() - assert count_for_date == 2 - - -@freeze_time("2017-12-18 17:50") -def test_dao_get_count_of_letters_to_process_for_date_ignores_test_keys(sample_letter_template): - # not expected - create_notification(template=sample_letter_template, key_type=KEY_TYPE_TEST, created_at='2017-12-18 17:29:00') - - # expected - create_notification(template=sample_letter_template, created_at='2017-12-18 17:29:10') - create_notification(template=sample_letter_template, created_at='2017-12-18 17:29:20') - - count_for_date = dao_get_count_of_letters_to_process_for_date() - - assert count_for_date == 2 - - @pytest.mark.parametrize("notification_type", ["letter", "email", "sms"] )