From b170b5ed8058098f9aec988c57d9175903e3570c Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Wed, 2 Jun 2021 10:31:38 +0100 Subject: [PATCH 1/2] This change is a temporary fix to allow users for high volume services to use the admin app. The trouble is the aggregate query to return the big blue numbers on the dashboard and /notifications/{notification_type} page is taking too long to return. I have some ideas on how to improve the query, but should take some time to do some more research and test. In the meantime, let's just ignore "todays" total numbers for the high volume services. There are only two services that this will affect. --- app/dao/fact_notification_status_dao.py | 34 ++++++------ .../dao/test_fact_notification_status_dao.py | 52 +++++++++++++++++++ 2 files changed, 70 insertions(+), 16 deletions(-) diff --git a/app/dao/fact_notification_status_dao.py b/app/dao/fact_notification_status_dao.py index ae7d816a3..2caa68379 100644 --- a/app/dao/fact_notification_status_dao.py +++ b/app/dao/fact_notification_status_dao.py @@ -154,23 +154,25 @@ def fetch_notification_status_for_service_for_today_and_7_previous_days(service_ FactNotificationStatus.bst_date >= start_date, FactNotificationStatus.key_type != KEY_TYPE_TEST ) + if str(service_id) in (current_app.config['HIGH_VOLUME_SERVICE']): + all_stats_table = stats_for_7_days.subquery() + else: + stats_for_today = db.session.query( + Notification.notification_type.cast(db.Text), + Notification.status, + *([Notification.template_id] if by_template else []), + func.count().label('count') + ).filter( + Notification.created_at >= get_london_midnight_in_utc(now), + Notification.service_id == service_id, + Notification.key_type != KEY_TYPE_TEST + ).group_by( + Notification.notification_type, + *([Notification.template_id] if by_template else []), + Notification.status + ) - stats_for_today = db.session.query( - Notification.notification_type.cast(db.Text), - Notification.status, - *([Notification.template_id] if by_template else []), - func.count().label('count') - ).filter( - Notification.created_at >= get_london_midnight_in_utc(now), - Notification.service_id == service_id, - Notification.key_type != KEY_TYPE_TEST - ).group_by( - Notification.notification_type, - *([Notification.template_id] if by_template else []), - Notification.status - ) - - all_stats_table = stats_for_7_days.union_all(stats_for_today).subquery() + all_stats_table = stats_for_7_days.union_all(stats_for_today).subquery() query = db.session.query( *([ diff --git a/tests/app/dao/test_fact_notification_status_dao.py b/tests/app/dao/test_fact_notification_status_dao.py index f807541a0..6f9ed01b1 100644 --- a/tests/app/dao/test_fact_notification_status_dao.py +++ b/tests/app/dao/test_fact_notification_status_dao.py @@ -3,6 +3,7 @@ from uuid import UUID import mock import pytest +from flask import current_app from freezegun import freeze_time from app.dao.fact_notification_status_dao import ( @@ -44,6 +45,7 @@ from tests.app.db import ( create_service_data_retention, create_template, ) +from tests.conftest import set_config_values def test_update_fact_notification_status(notify_db_session): @@ -267,6 +269,56 @@ def test_fetch_notification_status_for_service_for_today_and_7_previous_days(not assert results[3].count == 19 +@freeze_time('2018-10-31T18:00:00') +def test_fetch_notification_status_for_service_for_today_and_7_previous_days_for_high_volume_service( + notify_api, notify_db_session +): + service_1 = create_service(service_name='service_1') + sms_template = create_template(service=service_1, template_type=SMS_TYPE) + sms_template_2 = create_template(service=service_1, template_type=SMS_TYPE) + email_template = create_template(service=service_1, template_type=EMAIL_TYPE) + + create_ft_notification_status(date(2018, 10, 29), 'sms', service_1, count=10) + create_ft_notification_status(date(2018, 10, 24), 'sms', service_1, count=8) + create_ft_notification_status(date(2018, 10, 29), 'sms', service_1, notification_status='created') + create_ft_notification_status(date(2018, 10, 29), 'email', service_1, count=3) + create_ft_notification_status(date(2018, 10, 26), 'letter', service_1, count=5) + + create_notification(sms_template, created_at=datetime(2018, 10, 31, 11, 0, 0)) + create_notification(sms_template_2, created_at=datetime(2018, 10, 31, 11, 0, 0)) + create_notification(sms_template, created_at=datetime(2018, 10, 31, 12, 0, 0), status='delivered') + create_notification(email_template, created_at=datetime(2018, 10, 31, 13, 0, 0), status='delivered') + + # too early, shouldn't be included + create_notification(service_1.templates[0], created_at=datetime(2018, 10, 30, 12, 0, 0), status='delivered') + with set_config_values(current_app, { + 'HIGH_VOLUME_SERVICE': [str(service_1.id)], + + }): + results = sorted( + fetch_notification_status_for_service_for_today_and_7_previous_days(service_1.id), + key=lambda x: (x.notification_type, x.status) + ) + + assert len(results) == 4 + + assert results[0].notification_type == 'email' + assert results[0].status == 'delivered' + assert results[0].count == 3 + + assert results[1].notification_type == 'letter' + assert results[1].status == 'delivered' + assert results[1].count == 5 + + assert results[2].notification_type == 'sms' + assert results[2].status == 'created' + assert results[2].count == 1 + + assert results[3].notification_type == 'sms' + assert results[3].status == 'delivered' + assert results[3].count == 18 + + @freeze_time('2018-10-31T18:00:00') def test_fetch_notification_status_by_template_for_service_for_today_and_7_previous_days(notify_db_session): service_1 = create_service(service_name='service_1') From a341536de0d242aa3a8037b9232c177eace7dc85 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Wed, 2 Jun 2021 14:13:31 +0100 Subject: [PATCH 2/2] - Add comment to test and new if statement - Update assert in test --- app/dao/fact_notification_status_dao.py | 3 ++ .../dao/test_fact_notification_status_dao.py | 38 +++---------------- 2 files changed, 8 insertions(+), 33 deletions(-) diff --git a/app/dao/fact_notification_status_dao.py b/app/dao/fact_notification_status_dao.py index 2caa68379..84889cb56 100644 --- a/app/dao/fact_notification_status_dao.py +++ b/app/dao/fact_notification_status_dao.py @@ -155,6 +155,9 @@ def fetch_notification_status_for_service_for_today_and_7_previous_days(service_ FactNotificationStatus.key_type != KEY_TYPE_TEST ) if str(service_id) in (current_app.config['HIGH_VOLUME_SERVICE']): + # As a temporary measure we are not including today's totals for high volume service. This will allow the + # services to view the dashboard and /notification pages more easily. There is a story to try to resolve the + # query performance, https://www.pivotaltracker.com/story/show/178330973 all_stats_table = stats_for_7_days.subquery() else: stats_for_today = db.session.query( diff --git a/tests/app/dao/test_fact_notification_status_dao.py b/tests/app/dao/test_fact_notification_status_dao.py index 6f9ed01b1..319327e69 100644 --- a/tests/app/dao/test_fact_notification_status_dao.py +++ b/tests/app/dao/test_fact_notification_status_dao.py @@ -251,22 +251,8 @@ def test_fetch_notification_status_for_service_for_today_and_7_previous_days(not ) assert len(results) == 4 - - assert results[0].notification_type == 'email' - assert results[0].status == 'delivered' - assert results[0].count == 4 - - assert results[1].notification_type == 'letter' - assert results[1].status == 'delivered' - assert results[1].count == 5 - - assert results[2].notification_type == 'sms' - assert results[2].status == 'created' - assert results[2].count == 3 - - assert results[3].notification_type == 'sms' - assert results[3].status == 'delivered' - assert results[3].count == 19 + assert sorted(results, key=lambda x: (x.notification_type, x.status, x.count)) == \ + [('email', 'delivered', 4), ('letter', 'delivered', 5), ('sms', 'created', 3), ('sms', 'delivered', 19)] @freeze_time('2018-10-31T18:00:00') @@ -283,7 +269,7 @@ def test_fetch_notification_status_for_service_for_today_and_7_previous_days_for create_ft_notification_status(date(2018, 10, 29), 'sms', service_1, notification_status='created') create_ft_notification_status(date(2018, 10, 29), 'email', service_1, count=3) create_ft_notification_status(date(2018, 10, 26), 'letter', service_1, count=5) - + # notifications created today will not be included in the resultset create_notification(sms_template, created_at=datetime(2018, 10, 31, 11, 0, 0)) create_notification(sms_template_2, created_at=datetime(2018, 10, 31, 11, 0, 0)) create_notification(sms_template, created_at=datetime(2018, 10, 31, 12, 0, 0), status='delivered') @@ -301,22 +287,8 @@ def test_fetch_notification_status_for_service_for_today_and_7_previous_days_for ) assert len(results) == 4 - - assert results[0].notification_type == 'email' - assert results[0].status == 'delivered' - assert results[0].count == 3 - - assert results[1].notification_type == 'letter' - assert results[1].status == 'delivered' - assert results[1].count == 5 - - assert results[2].notification_type == 'sms' - assert results[2].status == 'created' - assert results[2].count == 1 - - assert results[3].notification_type == 'sms' - assert results[3].status == 'delivered' - assert results[3].count == 18 + assert sorted(results, key=lambda x: (x.notification_type, x.status, x.count)) == \ + [('email', 'delivered', 3), ('letter', 'delivered', 5), ('sms', 'created', 1), ('sms', 'delivered', 18)] @freeze_time('2018-10-31T18:00:00')