From 574b1d3c6357f1641b136439866f4e55ea929d47 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Wed, 16 Feb 2022 12:30:41 +0000 Subject: [PATCH] Fix not deleting dead rows in status table To address: https://github.com/alphagov/notifications-api/pull/3454#pullrequestreview-880302729 Since we now delete all rows before inserting fresh ones, we no longer need to worry about conflicts. I've also extended the old test to check all three kinds of overwrite: new, changed, gone. --- app/dao/fact_notification_status_dao.py | 43 ++++++++++++------------ tests/app/celery/test_reporting_tasks.py | 17 +++++++--- 2 files changed, 33 insertions(+), 27 deletions(-) diff --git a/app/dao/fact_notification_status_dao.py b/app/dao/fact_notification_status_dao.py index 61b997688..c37dae011 100644 --- a/app/dao/fact_notification_status_dao.py +++ b/app/dao/fact_notification_status_dao.py @@ -39,6 +39,13 @@ def update_fact_notification_status(process_day, notification_type, service_id): start_date = get_london_midnight_in_utc(process_day) end_date = get_london_midnight_in_utc(process_day + timedelta(days=1)) + # delete any existing rows in case some no longer exist e.g. if all messages are sent + FactNotificationStatus.query.filter( + FactNotificationStatus.bst_date == process_day, + FactNotificationStatus.notification_type == notification_type, + FactNotificationStatus.service_id == service_id, + ).delete() + # query notifications or notification_history for the day, depending on their data retention service = Service.query.get(service_id) source_table = get_notification_table_to_use( @@ -71,30 +78,22 @@ def update_fact_notification_status(process_day, notification_type, service_id): source_table.status ) - stmt = insert(FactNotificationStatus.__table__).from_select( - [ - FactNotificationStatus.bst_date, - FactNotificationStatus.template_id, - FactNotificationStatus.service_id, - FactNotificationStatus.job_id, - FactNotificationStatus.notification_type, - FactNotificationStatus.key_type, - FactNotificationStatus.notification_status, - FactNotificationStatus.notification_count - ], - query + db.session.connection().execute( + insert(FactNotificationStatus.__table__).from_select( + [ + FactNotificationStatus.bst_date, + FactNotificationStatus.template_id, + FactNotificationStatus.service_id, + FactNotificationStatus.job_id, + FactNotificationStatus.notification_type, + FactNotificationStatus.key_type, + FactNotificationStatus.notification_status, + FactNotificationStatus.notification_count + ], + query + ) ) - stmt = stmt.on_conflict_do_update( - constraint="ft_notification_status_pkey", - set_={ - FactNotificationStatus.notification_count: stmt.excluded.notification_count, - FactNotificationStatus.updated_at: datetime.utcnow() - } - ) - - db.session.connection().execute(stmt) - def fetch_notification_status_for_service_by_month(start_date, end_date, service_id): return db.session.query( diff --git a/tests/app/celery/test_reporting_tasks.py b/tests/app/celery/test_reporting_tasks.py index 1e83161f7..3b5f5b701 100644 --- a/tests/app/celery/test_reporting_tasks.py +++ b/tests/app/celery/test_reporting_tasks.py @@ -599,9 +599,10 @@ def test_create_nightly_notification_status_for_service_and_day(notify_db_sessio def test_create_nightly_notification_status_for_service_and_day_overwrites_old_data(notify_db_session): first_service = create_service(service_name='First Service') first_template = create_template(service=first_service) - create_notification(template=first_template, status='delivered') - process_day = date.today() + + # first run: one notification, expect one row (just one status) + notification = create_notification(template=first_template, status='sending') create_nightly_notification_status_for_service_and_day(str(process_day), first_service.id, 'sms') new_fact_data = FactNotificationStatus.query.order_by( @@ -611,8 +612,11 @@ def test_create_nightly_notification_status_for_service_and_day_overwrites_old_d assert len(new_fact_data) == 1 assert new_fact_data[0].notification_count == 1 + assert new_fact_data[0].notification_status == 'sending' - create_notification(template=first_template, status='delivered') + # second run: status changed, still expect one row (one status) + notification.status = 'delivered' + create_notification(template=first_template, status='created') create_nightly_notification_status_for_service_and_day(str(process_day), first_service.id, 'sms') updated_fact_data = FactNotificationStatus.query.order_by( @@ -620,8 +624,11 @@ def test_create_nightly_notification_status_for_service_and_day_overwrites_old_d FactNotificationStatus.notification_type ).all() - assert len(updated_fact_data) == 1 - assert updated_fact_data[0].notification_count == 2 + assert len(updated_fact_data) == 2 + assert updated_fact_data[0].notification_count == 1 + assert updated_fact_data[0].notification_status == 'created' + assert updated_fact_data[1].notification_count == 1 + assert updated_fact_data[1].notification_status == 'delivered' # the job runs at 12:30am London time. 04/01 is in BST.