From ac4f0e8027f23be960fd730fdfa1c43439f9b82e Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Tue, 19 Nov 2019 16:04:21 +0000 Subject: [PATCH] After a comment from @idavidmcdonald, I asked myself why are not creating the task to upload the pdf and update the notification. The assumption was that S3 would throw an exception if the object was uploaded twice. That's not the case the default behaviour is that if a file already exists it will be overwritten. So it is completely safe to run the task from the alert. It can also mean that we don't need to wait 4hours 15 minutes. Shall I decease the amount of time before restarting the task? --- app/celery/scheduled_tasks.py | 18 +++++++----------- app/dao/notifications_dao.py | 4 ++-- tests/app/celery/test_scheduled_tasks.py | 20 ++++++-------------- 3 files changed, 15 insertions(+), 27 deletions(-) diff --git a/app/celery/scheduled_tasks.py b/app/celery/scheduled_tasks.py index bc1391710..0d5618768 100644 --- a/app/celery/scheduled_tasks.py +++ b/app/celery/scheduled_tasks.py @@ -14,6 +14,7 @@ from app.celery.tasks import ( get_recipient_csv_and_template, process_row ) +from app.celery.letters_pdf_tasks import create_letters_pdf from app.config import QueueNames, TaskNames from app.dao.invited_org_user_dao import delete_org_invitations_created_more_than_two_days_ago from app.dao.invited_user_dao import delete_invitations_created_more_than_two_days_ago @@ -196,18 +197,13 @@ def replay_created_notifications(): if len(letters) > 0: msg = "{} letters were created four hours and 15 minutes ago, " \ "but do not have an updated_at timestamp or billable units. " \ - "It is likely you need to run the " \ - "app.celery.letters_pdf_tasks.create_letters_pdf task again with " \ - "the notification id.\n {}".format(len(letters), - [x.id for x in letters]) + "\n Creating app.celery.letters_pdf_tasks.create_letters tasks to upload letter to S3 " \ + "and update notifications for the following notification ids: " \ + "\n {}".format(len(letters), [x.id for x in letters]) + current_app.logger.info(msg) - if current_app.config['NOTIFY_ENVIRONMENT'] in ['live', 'production', 'test']: - zendesk_client.create_ticket( - subject="[{}] Letters still in created status might be missing from S3".format( - current_app.config['NOTIFY_ENVIRONMENT']), - message=msg, - ticket_type=zendesk_client.TYPE_INCIDENT - ) + for letter in letters: + create_letters_pdf.apply_async([letter.id], queue=QueueNames.LETTERS) @notify_celery.task(name='check-precompiled-letter-state') diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 1a179481a..025b355e5 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -710,11 +710,11 @@ def dao_old_letters_with_created_status(): def letters_missing_from_sending_bucket(seconds_to_subtract): older_than_date = datetime.utcnow() - timedelta(seconds=seconds_to_subtract) - + # We expect letters to have a `created` status, updated_at timestamp and billable units greater than zero. notifications = Notification.query.filter( + Notification.billable_units == 0, Notification.updated_at == None, # noqa Notification.status == NOTIFICATION_CREATED, - Notification.billable_units == 0, Notification.created_at <= older_than_date, Notification.notification_type == LETTER_TYPE, Notification.key_type == KEY_TYPE_NORMAL diff --git a/tests/app/celery/test_scheduled_tasks.py b/tests/app/celery/test_scheduled_tasks.py index 29388eb6a..847b09ffe 100644 --- a/tests/app/celery/test_scheduled_tasks.py +++ b/tests/app/celery/test_scheduled_tasks.py @@ -309,10 +309,10 @@ def test_replay_created_notifications(notify_db_session, sample_service, mocker) queue="send-sms-tasks") -def test_replay_created_notifications_create_zendesk_ticket_for_letters_not_ready_to_send( +def test_replay_created_notifications_create_letters_pdf_tasks_for_letters_not_ready_to_send( sample_letter_template, mocker ): - mock_create_ticket = mocker.patch('app.celery.scheduled_tasks.zendesk_client.create_ticket') + mock_task = mocker.patch('app.celery.scheduled_tasks.create_letters_pdf.apply_async') create_notification(template=sample_letter_template, billable_units=0, created_at=datetime.utcnow() - timedelta(hours=4)) @@ -323,18 +323,10 @@ def test_replay_created_notifications_create_zendesk_ticket_for_letters_not_read replay_created_notifications() - message = "{} letters were created four hours and 15 minutes ago, " \ - "but do not have an updated_at timestamp or billable units. " \ - "It is likely you need to run the " \ - "app.celery.letters_pdf_tasks.create_letters_pdf task again with " \ - "the notification id.\n {}".format(2, - [notification_2.id, notification_1.id]) - - mock_create_ticket.assert_called_with( - message=message, - subject='[test] Letters still in created status might be missing from S3', - ticket_type='incident' - ) + calls = [call([notification_1.id], queue=QueueNames.LETTERS), + call([notification_2.id], queue=QueueNames.LETTERS), + ] + mock_task.assert_has_calls(calls, any_order=True) def test_check_job_status_task_does_not_raise_error(sample_template):