From 884cb24bfa11a2dc68baf0995dd5069841c4ed9d Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 5 Dec 2019 15:59:40 +0000 Subject: [PATCH 1/3] remove day_start from create nightly notification status it makes less sense once we introduce different start dates for letters and emails. Also, we never use it, since we just call the day tasks ourselves from commands.py --- app/celery/reporting_tasks.py | 11 +++-------- tests/app/celery/test_reporting_tasks.py | 13 ++++++------- 2 files changed, 9 insertions(+), 15 deletions(-) diff --git a/app/celery/reporting_tasks.py b/app/celery/reporting_tasks.py index 8231e0613..d88c62deb 100644 --- a/app/celery/reporting_tasks.py +++ b/app/celery/reporting_tasks.py @@ -67,14 +67,9 @@ def create_nightly_billing_for_day(process_day): @notify_celery.task(name="create-nightly-notification-status") @cronitor("create-nightly-notification-status") @statsd(namespace="tasks") -def create_nightly_notification_status(day_start=None): - # day_start is a datetime.date() object. e.g. - # 4 days of data counting back from day_start is consolidated - if day_start is None: - day_start = convert_utc_to_bst(datetime.utcnow()).date() - timedelta(days=1) - else: - # When calling the task its a string in the format of "YYYY-MM-DD" - day_start = datetime.strptime(day_start, "%Y-%m-%d").date() +def create_nightly_notification_status(): + day_start = convert_utc_to_bst(datetime.utcnow()).date() - timedelta(days=1) + for i in range(0, 4): process_day = day_start - timedelta(days=i) for notification_type in [SMS_TYPE, EMAIL_TYPE, LETTER_TYPE]: diff --git a/tests/app/celery/test_reporting_tasks.py b/tests/app/celery/test_reporting_tasks.py index e8d86d1a0..f6fb40a3b 100644 --- a/tests/app/celery/test_reporting_tasks.py +++ b/tests/app/celery/test_reporting_tasks.py @@ -50,16 +50,15 @@ def test_create_nightly_billing_triggers_tasks_for_days(notify_api, mocker, day_ @freeze_time('2019-08-01') -@pytest.mark.parametrize('day_start, expected_kwargs', [ - (None, ['2019-07-31', '2019-07-30', '2019-07-29', '2019-07-28']), - ('2019-07-21', ['2019-07-21', '2019-07-20', '2019-07-19', '2019-07-18']), -]) -def test_create_nightly_notification_status_triggers_tasks_for_days(notify_api, mocker, day_start, expected_kwargs): +def test_create_nightly_notification_status_triggers_tasks_for_days(notify_api, mocker): mock_celery = mocker.patch('app.celery.reporting_tasks.create_nightly_notification_status_for_day') - create_nightly_notification_status(day_start) + create_nightly_notification_status() assert mock_celery.apply_async.call_count == 4 * 3 # four days, three notification types - for process_date, notification_type in itertools.product(expected_kwargs, ['sms', 'email', 'letter']): + for process_date, notification_type in itertools.product( + ['2019-07-31', '2019-07-30', '2019-07-29', '2019-07-28'], + ['sms', 'email', 'letter'] + ): mock_celery.apply_async.assert_any_call( kwargs={'process_day': process_date, 'notification_type': notification_type}, queue=QueueNames.REPORTING From 6ac45952241b93dfcb49d83040c1e59a88f433a4 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 5 Dec 2019 16:12:16 +0000 Subject: [PATCH 2/3] process letters for 10 days when updating ft_notification_status sms and emails have a very predictable 72 hour lifecycle. letters, on the other hand, have ridiculously complex lifecycles - they might not get sent because it's a weekend, they might not get sent because they're second class and are only processed on alternate days, they might not get sent because a different letter in the same batch had an error that we didn't know about. Either way, it's apparent that four days is definitely not enough time to guarantee that letters have gone from sending to delivered. Extend the amount of days we process for letters to 10 days. Keep emails and sms down at 4 to keep run-times shorter We're deliberately not thinking about returned letters here at all. --- app/celery/reporting_tasks.py | 16 ++++++++++++---- tests/app/celery/test_reporting_tasks.py | 15 +++++++++++++-- 2 files changed, 25 insertions(+), 6 deletions(-) diff --git a/app/celery/reporting_tasks.py b/app/celery/reporting_tasks.py index d88c62deb..72ac2fd97 100644 --- a/app/celery/reporting_tasks.py +++ b/app/celery/reporting_tasks.py @@ -68,15 +68,23 @@ def create_nightly_billing_for_day(process_day): @cronitor("create-nightly-notification-status") @statsd(namespace="tasks") def create_nightly_notification_status(): - day_start = convert_utc_to_bst(datetime.utcnow()).date() - timedelta(days=1) + yesterday = convert_utc_to_bst(datetime.utcnow()).date() - timedelta(days=1) - for i in range(0, 4): - process_day = day_start - timedelta(days=i) - for notification_type in [SMS_TYPE, EMAIL_TYPE, LETTER_TYPE]: + # email and sms + for i in range(4): + process_day = yesterday - timedelta(days=i) + for notification_type in [SMS_TYPE, EMAIL_TYPE]: create_nightly_notification_status_for_day.apply_async( kwargs={'process_day': process_day.isoformat(), 'notification_type': notification_type}, queue=QueueNames.REPORTING ) + # letters get modified for a longer time period than sms and email, so we need to reprocess for more days + for i in range(10): + process_day = yesterday - timedelta(days=i) + create_nightly_notification_status_for_day.apply_async( + kwargs={'process_day': process_day.isoformat(), 'notification_type': LETTER_TYPE}, + queue=QueueNames.REPORTING + ) @notify_celery.task(name="create-nightly-notification-status-for-day") diff --git a/tests/app/celery/test_reporting_tasks.py b/tests/app/celery/test_reporting_tasks.py index f6fb40a3b..c0fc69074 100644 --- a/tests/app/celery/test_reporting_tasks.py +++ b/tests/app/celery/test_reporting_tasks.py @@ -54,16 +54,27 @@ def test_create_nightly_notification_status_triggers_tasks_for_days(notify_api, mock_celery = mocker.patch('app.celery.reporting_tasks.create_nightly_notification_status_for_day') create_nightly_notification_status() - assert mock_celery.apply_async.call_count == 4 * 3 # four days, three notification types + assert mock_celery.apply_async.call_count == ( + (4 * 3) # four days, three notification types + + + 6 # six more days of just letters + ) + for process_date, notification_type in itertools.product( ['2019-07-31', '2019-07-30', '2019-07-29', '2019-07-28'], - ['sms', 'email', 'letter'] + [SMS_TYPE, EMAIL_TYPE, LETTER_TYPE] ): mock_celery.apply_async.assert_any_call( kwargs={'process_day': process_date, 'notification_type': notification_type}, queue=QueueNames.REPORTING ) + for process_date in ['2019-07-27', '2019-07-26', '2019-07-25', '2019-07-24', '2019-07-23', '2019-07-22']: + mock_celery.apply_async.assert_any_call( + kwargs={'process_day': process_date, 'notification_type': LETTER_TYPE}, + queue=QueueNames.REPORTING + ) + @pytest.mark.parametrize('second_rate, records_num, billable_units, multiplier', [(1.0, 1, 2, [1]), From ef6b7d564a5c862aec0b8158668080cdf70e2d71 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Fri, 6 Dec 2019 16:10:11 +0000 Subject: [PATCH 3/3] make migrate-ft-notification-status command dates inclusive --- app/commands.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/commands.py b/app/commands.py index fcaa6b3af..b0d1c1964 100644 --- a/app/commands.py +++ b/app/commands.py @@ -505,7 +505,7 @@ def migrate_data_to_ft_notification_status(start_date, end_date, notification_ty start_date = start_date.date() end_date = end_date.date() - for day_diff in range((end_date - start_date).days): + for day_diff in range((end_date - start_date).days + 1): process_day = start_date + timedelta(days=day_diff) for notification_type in notification_types: print('create_nightly_notification_status_for_day triggered for {} and {}'.format(