From 992ef259b2ced2c4ab16ddc30ae93f6f007176f2 Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Wed, 7 Nov 2018 16:39:23 +0000 Subject: [PATCH 1/6] Rearrange nightly tasks to maintain the correct order Timeout sending notifications updates up to 4 days of notifications older than 72 hours to correct failure status. It needs to run before we update ft_notifications_status table. Otherwise the changes don't get picked up. Notifications deletion tasks have to run after those jobs in case our users set short data retention policy. --- app/config.py | 88 ++++++++++++++++++++++++++------------------------- 1 file changed, 45 insertions(+), 43 deletions(-) diff --git a/app/config.py b/app/config.py index 5a17d006f..80e98bf9b 100644 --- a/app/config.py +++ b/app/config.py @@ -175,49 +175,66 @@ class Config(object): 'schedule': timedelta(minutes=66), 'options': {'queue': QueueNames.PERIODIC} }, - 'delete-sms-notifications': { - 'task': 'delete-sms-notifications', - 'schedule': crontab(hour=0, minute=0), - 'options': {'queue': QueueNames.PERIODIC} - }, - 'delete-email-notifications': { - 'task': 'delete-email-notifications', - 'schedule': crontab(hour=0, minute=20), - 'options': {'queue': QueueNames.PERIODIC} - }, - 'delete-letter-notifications': { - 'task': 'delete-letter-notifications', - 'schedule': crontab(hour=0, minute=40), - 'options': {'queue': QueueNames.PERIODIC} - }, - 'delete-inbound-sms': { - 'task': 'delete-inbound-sms', - 'schedule': crontab(hour=1, minute=0), - 'options': {'queue': QueueNames.PERIODIC} - }, - 'send-daily-performance-platform-stats': { - 'task': 'send-daily-performance-platform-stats', - 'schedule': crontab(hour=2, minute=0), - 'options': {'queue': QueueNames.PERIODIC} - }, 'switch-current-sms-provider-on-slow-delivery': { 'task': 'switch-current-sms-provider-on-slow-delivery', 'schedule': crontab(), # Every minute 'options': {'queue': QueueNames.PERIODIC} }, + 'check-job-status': { + 'task': 'check-job-status', + 'schedule': crontab(), + 'options': {'queue': QueueNames.PERIODIC} + }, + 'replay-created-notifications': { + 'task': 'replay-created-notifications', + 'schedule': crontab(minute='0, 15, 30, 45'), + 'options': {'queue': QueueNames.PERIODIC} + }, + # nightly tasks: 'timeout-sending-notifications': { 'task': 'timeout-sending-notifications', - 'schedule': crontab(hour=3, minute=0), + 'schedule': crontab(hour=0, minute=5), + 'options': {'queue': QueueNames.PERIODIC} + }, + 'daily-stats-template-usage-by-month': { + 'task': 'daily-stats-template-usage-by-month', + 'schedule': crontab(hour=0, minute=10), 'options': {'queue': QueueNames.PERIODIC} }, 'create-nightly-billing': { 'task': 'create-nightly-billing', - 'schedule': crontab(hour=3, minute=30), + 'schedule': crontab(hour=0, minute=15), 'options': {'queue': QueueNames.PERIODIC} }, 'create-nightly-notification-status': { 'task': 'create-nightly-notification-status', - 'schedule': crontab(hour=4, minute=30), + 'schedule': crontab(hour=0, minute=30), # after 'timeout-sending-notifications' + 'options': {'queue': QueueNames.PERIODIC} + }, + 'delete-sms-notifications': { + 'task': 'delete-sms-notifications', + 'schedule': crontab(hour=0, minute=45), # after 'create-nightly-notification-status' + 'options': {'queue': QueueNames.PERIODIC} + }, + 'delete-email-notifications': { + 'task': 'delete-email-notifications', + 'schedule': crontab(hour=1, minute=0), # after 'create-nightly-notification-status' + 'options': {'queue': QueueNames.PERIODIC} + }, + 'delete-letter-notifications': { + 'task': 'delete-letter-notifications', + 'schedule': crontab(hour=1, minute=20), # after 'create-nightly-notification-status' + 'options': {'queue': QueueNames.PERIODIC} + }, + 'delete-inbound-sms': { + 'task': 'delete-inbound-sms', + 'schedule': crontab(hour=1, minute=40), + 'options': {'queue': QueueNames.PERIODIC} + }, + + 'send-daily-performance-platform-stats': { + 'task': 'send-daily-performance-platform-stats', + 'schedule': crontab(hour=2, minute=0), 'options': {'queue': QueueNames.PERIODIC} }, 'remove_sms_email_jobs': { @@ -254,21 +271,6 @@ class Config(object): 'schedule': crontab(hour=23, minute=00), 'options': {'queue': QueueNames.PERIODIC} }, - 'check-job-status': { - 'task': 'check-job-status', - 'schedule': crontab(), - 'options': {'queue': QueueNames.PERIODIC} - }, - 'daily-stats-template-usage-by-month': { - 'task': 'daily-stats-template-usage-by-month', - 'schedule': crontab(hour=0, minute=5), - 'options': {'queue': QueueNames.PERIODIC} - }, - 'replay-created-notifications': { - 'task': 'replay-created-notifications', - 'schedule': crontab(minute='0, 15, 30, 45'), - 'options': {'queue': QueueNames.PERIODIC} - } } CELERY_QUEUES = [] From 987445f1bfc80016ed37da9a69499a45a162386d Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Wed, 7 Nov 2018 16:49:48 +0000 Subject: [PATCH 2/6] ft_notification_status now updates data for 4 days back This was done so when notification is timed out from sending/pending to temporary_failure, this change has to always be caught in the ft_notification_status --- app/celery/reporting_tasks.py | 6 +++--- app/config.py | 8 ++++---- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/app/celery/reporting_tasks.py b/app/celery/reporting_tasks.py index eb2741be1..efe3dae99 100644 --- a/app/celery/reporting_tasks.py +++ b/app/celery/reporting_tasks.py @@ -15,7 +15,7 @@ from app.dao.fact_notification_status_dao import fetch_notification_status_for_d @statsd(namespace="tasks") def create_nightly_billing(day_start=None): # day_start is a datetime.date() object. e.g. - # 3 days of data counting back from day_start is consolidated + # up to 10 days of data counting back from day_start is consolidated if day_start is None: day_start = datetime.today() - timedelta(days=1) else: @@ -37,13 +37,13 @@ def create_nightly_billing(day_start=None): @statsd(namespace="tasks") def create_nightly_notification_status(day_start=None): # day_start is a datetime.date() object. e.g. - # 3 days of data counting back from day_start is consolidated + # 4 days of data counting back from day_start is consolidated if day_start is None: day_start = datetime.today() - 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") - for i in range(0, 3): + for i in range(0, 4): process_day = day_start - timedelta(days=i) transit_data = fetch_notification_status_for_day(process_day=process_day) diff --git a/app/config.py b/app/config.py index 80e98bf9b..c828b4644 100644 --- a/app/config.py +++ b/app/config.py @@ -208,22 +208,22 @@ class Config(object): }, 'create-nightly-notification-status': { 'task': 'create-nightly-notification-status', - 'schedule': crontab(hour=0, minute=30), # after 'timeout-sending-notifications' + 'schedule': crontab(hour=0, minute=30), # after 'timeout-sending-notifications' 'options': {'queue': QueueNames.PERIODIC} }, 'delete-sms-notifications': { 'task': 'delete-sms-notifications', - 'schedule': crontab(hour=0, minute=45), # after 'create-nightly-notification-status' + 'schedule': crontab(hour=0, minute=45), # after 'create-nightly-notification-status' 'options': {'queue': QueueNames.PERIODIC} }, 'delete-email-notifications': { 'task': 'delete-email-notifications', - 'schedule': crontab(hour=1, minute=0), # after 'create-nightly-notification-status' + 'schedule': crontab(hour=1, minute=0), # after 'create-nightly-notification-status' 'options': {'queue': QueueNames.PERIODIC} }, 'delete-letter-notifications': { 'task': 'delete-letter-notifications', - 'schedule': crontab(hour=1, minute=20), # after 'create-nightly-notification-status' + 'schedule': crontab(hour=1, minute=20), # after 'create-nightly-notification-status' 'options': {'queue': QueueNames.PERIODIC} }, 'delete-inbound-sms': { From ca2db56b9df3cb557cebee8fb6039723a6b335f5 Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Thu, 8 Nov 2018 10:42:01 +0000 Subject: [PATCH 3/6] Update ft_notification_status now deletes old version of data instead of overwriting on top of it --- app/dao/fact_notification_status_dao.py | 16 +++------------- tests/app/celery/test_reporting_tasks.py | 10 +++++----- 2 files changed, 8 insertions(+), 18 deletions(-) diff --git a/app/dao/fact_notification_status_dao.py b/app/dao/fact_notification_status_dao.py index 017eb7e9a..566d5f40e 100644 --- a/app/dao/fact_notification_status_dao.py +++ b/app/dao/fact_notification_status_dao.py @@ -49,12 +49,9 @@ def fetch_notification_status_for_day(process_day, service_id=None): def update_fact_notification_status(data, process_day): table = FactNotificationStatus.__table__ - ''' - This uses the Postgres upsert to avoid race conditions when two threads try to insert - at the same row. The excluded object refers to values that we tried to insert but were - rejected. - http://docs.sqlalchemy.org/en/latest/dialects/postgresql.html#insert-on-conflict-upsert - ''' + FactNotificationStatus.query.filter( + FactNotificationStatus.bst_date == process_day.date() + ).delete() for row in data: stmt = insert(table).values( bst_date=process_day.date(), @@ -66,13 +63,6 @@ def update_fact_notification_status(data, process_day): notification_status=row.status, notification_count=row.notification_count, ) - - stmt = stmt.on_conflict_do_update( - constraint="ft_notification_status_pkey", - set_={"notification_count": stmt.excluded.notification_count, - "updated_at": datetime.utcnow() - } - ) db.session.connection().execute(stmt) db.session.commit() diff --git a/tests/app/celery/test_reporting_tasks.py b/tests/app/celery/test_reporting_tasks.py index 32f7bb5cc..8918a33ce 100644 --- a/tests/app/celery/test_reporting_tasks.py +++ b/tests/app/celery/test_reporting_tasks.py @@ -460,24 +460,24 @@ def test_create_nightly_notification_status(notify_db_session): create_notification(template=first_template, status='delivered') create_notification(template=first_template, status='delivered', created_at=datetime.utcnow() - timedelta(days=1)) create_notification(template=first_template, status='delivered', created_at=datetime.utcnow() - timedelta(days=2)) - create_notification(template=first_template, status='delivered', created_at=datetime.utcnow() - timedelta(days=3)) create_notification(template=first_template, status='delivered', created_at=datetime.utcnow() - timedelta(days=4)) + create_notification(template=first_template, status='delivered', created_at=datetime.utcnow() - timedelta(days=5)) create_notification(template=second_template, status='temporary-failure') create_notification(template=second_template, status='temporary-failure', created_at=datetime.utcnow() - timedelta(days=1)) create_notification(template=second_template, status='temporary-failure', created_at=datetime.utcnow() - timedelta(days=2)) - create_notification(template=second_template, status='temporary-failure', - created_at=datetime.utcnow() - timedelta(days=3)) create_notification(template=second_template, status='temporary-failure', created_at=datetime.utcnow() - timedelta(days=4)) + create_notification(template=second_template, status='temporary-failure', + created_at=datetime.utcnow() - timedelta(days=5)) create_notification(template=third_template, status='created') create_notification(template=third_template, status='created', created_at=datetime.utcnow() - timedelta(days=1)) create_notification(template=third_template, status='created', created_at=datetime.utcnow() - timedelta(days=2)) - create_notification(template=third_template, status='created', created_at=datetime.utcnow() - timedelta(days=3)) create_notification(template=third_template, status='created', created_at=datetime.utcnow() - timedelta(days=4)) + create_notification(template=third_template, status='created', created_at=datetime.utcnow() - timedelta(days=5)) assert len(FactNotificationStatus.query.all()) == 0 @@ -487,6 +487,6 @@ def test_create_nightly_notification_status(notify_db_session): FactNotificationStatus.notification_type ).all() assert len(new_data) == 9 - assert str(new_data[0].bst_date) == datetime.strftime(datetime.utcnow() - timedelta(days=3), "%Y-%m-%d") + assert str(new_data[0].bst_date) == datetime.strftime(datetime.utcnow() - timedelta(days=4), "%Y-%m-%d") assert str(new_data[3].bst_date) == datetime.strftime(datetime.utcnow() - timedelta(days=2), "%Y-%m-%d") assert str(new_data[6].bst_date) == datetime.strftime(datetime.utcnow() - timedelta(days=1), "%Y-%m-%d") From 1c4d2c76255b370c8be439e37562c1a66dce0c3c Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Thu, 8 Nov 2018 11:07:33 +0000 Subject: [PATCH 4/6] Update existing ft_notification_status data to get rid of repeats --- app/commands.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/app/commands.py b/app/commands.py index a4244fb33..918a25242 100644 --- a/app/commands.py +++ b/app/commands.py @@ -527,6 +527,12 @@ def migrate_data_to_ft_notification_status(start_date, end_date): while process_date < end_date: start_time = datetime.now() # migrate data into ft_notification_status and update if record already exists + + db.session.execute( + 'delete from ft_notification_status where bst_date = :process_date', + {"process_date": process_date} + ) + sql = \ """ insert into ft_notification_status (bst_date, template_id, service_id, job_id, notification_type, key_type, @@ -546,9 +552,6 @@ def migrate_data_to_ft_notification_status(start_date, end_date): and n.created_at < (date :end + time '00:00:00') at time zone 'Europe/London' at time zone 'UTC' group by bst_date, template_id, service_id, job_id, notification_type, key_type, notification_status order by bst_date - on conflict on constraint ft_notification_status_pkey do update set - notification_count = excluded.notification_count, - updated_at = now() """ result = db.session.execute(sql, {"start": process_date, "end": process_date + timedelta(days=1)}) db.session.commit() From 7f918d7c4870b817016d5aaf68ff2ba92a51fb9e Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Thu, 8 Nov 2018 11:16:59 +0000 Subject: [PATCH 5/6] Remove relationship to folder when archiving a template MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a template is archived, it should no longer belong to any folder. If we don’t do this it will make it very hard to delete folders later (because folders can only be deleted if they have no templates or folders inside them). We originally tried to check if the link between a template and folder should be removed with `if template.archived and template.folder:` instead of using `if template.archived:`. However, this caused issues because checking `template.folder` flushes the session. Since the session is no longer dirty, the versioning decorator doesn't work as expected and doesn't create a new row in `TemplateHistory`. --- app/dao/templates_dao.py | 3 +++ tests/app/dao/test_templates_dao.py | 30 +++++++++++++++++++++++++++++ 2 files changed, 33 insertions(+) diff --git a/app/dao/templates_dao.py b/app/dao/templates_dao.py index 2a9164429..e5e93199f 100644 --- a/app/dao/templates_dao.py +++ b/app/dao/templates_dao.py @@ -38,6 +38,9 @@ def dao_create_template(template): @transactional @version_class(Template, TemplateHistory) def dao_update_template(template): + if template.archived: + template.folder = None + db.session.add(template) diff --git a/tests/app/dao/test_templates_dao.py b/tests/app/dao/test_templates_dao.py index cd2eb8992..b3d27a6d6 100644 --- a/tests/app/dao/test_templates_dao.py +++ b/tests/app/dao/test_templates_dao.py @@ -15,6 +15,7 @@ from app.dao.templates_dao import ( ) from app.models import ( Template, + TemplateFolder, TemplateHistory, TemplateRedacted ) @@ -93,6 +94,35 @@ def test_update_template(sample_service, sample_user): assert dao_get_all_templates_for_service(sample_service.id)[0].name == 'new name' +def test_update_template_in_a_folder_to_archived(sample_service, sample_user): + template_data = { + 'name': 'Sample Template', + 'template_type': "sms", + 'content': "Template content", + 'service': sample_service, + 'created_by': sample_user + } + template = Template(**template_data) + + template_folder_data = { + 'name': 'My Folder', + 'service_id': sample_service.id, + } + template_folder = TemplateFolder(**template_folder_data) + + template.folder = template_folder + dao_create_template(template) + + template.archived = True + dao_update_template(template) + + template_folder = TemplateFolder.query.one() + archived_template = Template.query.one() + + assert template_folder + assert not archived_template.folder + + def test_dao_update_template_reply_to_none_to_some(sample_service, sample_user): letter_contact = create_letter_contact(sample_service, 'Edinburgh, ED1 1AA') From 22ad14fcee68cf7c8243e221d7f6f09823200560 Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Fri, 9 Nov 2018 11:49:01 +0000 Subject: [PATCH 6/6] Fix logging for create_nightly_notification_status --- app/celery/reporting_tasks.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/app/celery/reporting_tasks.py b/app/celery/reporting_tasks.py index efe3dae99..4d14c0e64 100644 --- a/app/celery/reporting_tasks.py +++ b/app/celery/reporting_tasks.py @@ -51,4 +51,7 @@ def create_nightly_notification_status(day_start=None): update_fact_notification_status(transit_data, process_day) current_app.logger.info( - "create-nightly-notification-status task: {} rows updated for day: {}") + "create-nightly-notification-status task: {} rows updated for day: {}".format( + len(transit_data), process_day + ) + )