From 39963d978441ca44da038956f6403fc1ab5f44e5 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Mon, 31 Dec 2018 16:08:08 +0000 Subject: [PATCH 1/3] Created a query to get the notification status counts per notification type and service for all service for a given date range. The query follows the same pattern as the other queries, getting the statistics from the fact_notification_status table for dates older than today and union that with today. Tests required. --- app/dao/fact_notification_status_dao.py | 94 ++++++++++++++++++- .../dao/test_fact_notification_status_dao.py | 11 ++- 2 files changed, 103 insertions(+), 2 deletions(-) diff --git a/app/dao/fact_notification_status_dao.py b/app/dao/fact_notification_status_dao.py index 8925c81f0..3389225d7 100644 --- a/app/dao/fact_notification_status_dao.py +++ b/app/dao/fact_notification_status_dao.py @@ -8,7 +8,7 @@ from sqlalchemy.sql.expression import literal from sqlalchemy.types import DateTime, Integer from app import db -from app.models import Notification, NotificationHistory, FactNotificationStatus, KEY_TYPE_TEST +from app.models import Notification, NotificationHistory, FactNotificationStatus, KEY_TYPE_TEST, Service from app.utils import get_london_midnight_in_utc, midnight_n_days_ago @@ -197,3 +197,95 @@ def fetch_notification_statuses_for_job(job_id): ).group_by( FactNotificationStatus.notification_status ).all() + + +def fetch_stats_for_all_services_by_date_range(start_date, end_date,include_from_test_key=True): + stats = db.session.query( + FactNotificationStatus.service_id.label('service_id'), + Service.name.label('name'), + Service.restricted.label('restricted'), + Service.research_mode.label('research_mode'), + Service.active.label('active'), + Service.created_at.label('created_at'), + FactNotificationStatus.notification_type.label('notification_type'), + FactNotificationStatus.notification_status.label('status'), + func.sum(FactNotificationStatus.notification_count).label('count') + ).filter( + FactNotificationStatus.bst_date >= start_date, + FactNotificationStatus.bst_date <= end_date, + FactNotificationStatus.service_id == Service.id, + ).group_by( + FactNotificationStatus.service_id.label('service_id'), + Service.name, + Service.restricted, + Service.research_mode, + Service.active, + Service.created_at, + FactNotificationStatus.notification_type, + FactNotificationStatus.notification_status, + ).order_by( + FactNotificationStatus.service_id, + FactNotificationStatus.notification_type + ) + if not include_from_test_key: + stats = stats.filter(FactNotificationStatus.key_type != KEY_TYPE_TEST) + + today = get_london_midnight_in_utc(datetime.utcnow()) + if start_date <= today.date() <= end_date: + subquery = db.session.query( + Notification.notification_type.cast(db.Text).label('notification_type'), + Notification.status.label('status'), + Notification.service_id.label('service_id'), + func.count(Notification.id).label('count') + ).filter( + Notification.created_at >= today + ).group_by( + Notification.notification_type, + Notification.status, + Notification.service_id + ) + if not include_from_test_key: + subquery = subquery.filter(FactNotificationStatus.key_type != KEY_TYPE_TEST) + subquery = subquery.subquery() + + stats_for_today = db.session.query( + Service.id.label('service_id'), + Service.name.label('name'), + Service.restricted.label('restricted'), + Service.research_mode.label('research_mode'), + Service.active.label('active'), + Service.created_at.label('created_at'), + subquery.c.notification_type.label('notification_type'), + subquery.c.status.label('status'), + subquery.c.count.label('count') + ).outerjoin( + subquery, + subquery.c.service_id == Service.id + ).order_by(Service.id) + + all_stats_table = stats.union_all(stats_for_today).subquery() + query = db.session.query( + all_stats_table.c.service_id, + all_stats_table.c.name, + all_stats_table.c.restricted, + all_stats_table.c.research_mode, + all_stats_table.c.active, + all_stats_table.c.created_at, + all_stats_table.c.notification_type, + all_stats_table.c.status, + func.cast(func.sum(all_stats_table.c.count), Integer).label('count'), + ).group_by( + all_stats_table.c.service_id, + all_stats_table.c.name, + all_stats_table.c.restricted, + all_stats_table.c.research_mode, + all_stats_table.c.active, + all_stats_table.c.created_at, + all_stats_table.c.notification_type, + all_stats_table.c.status, + ).order_by( + all_stats_table.c.service_id + ) + else: + query = stats + return query.all() diff --git a/tests/app/dao/test_fact_notification_status_dao.py b/tests/app/dao/test_fact_notification_status_dao.py index db90c6d61..15a5b7605 100644 --- a/tests/app/dao/test_fact_notification_status_dao.py +++ b/tests/app/dao/test_fact_notification_status_dao.py @@ -11,7 +11,7 @@ from app.dao.fact_notification_status_dao import ( fetch_notification_status_for_service_for_today_and_7_previous_days, fetch_notification_status_totals_for_all_services, fetch_notification_statuses_for_job, -) + fetch_stats_for_all_services_by_date_range) from app.models import FactNotificationStatus, KEY_TYPE_TEST, KEY_TYPE_TEAM, EMAIL_TYPE, SMS_TYPE, LETTER_TYPE from freezegun import freeze_time from tests.app.db import create_notification, create_service, create_template, create_ft_notification_status, create_job @@ -304,3 +304,12 @@ def test_fetch_notification_statuses_for_job(sample_template): 'created': 5, 'delivered': 2 } + + +@freeze_time('2018-10-31 14:00') +def test_fetch_stats_for_all_services_by_date_range(notify_db_session): + set_up_data() + results = fetch_stats_for_all_services_by_date_range( start_date=date(2018, 10, 29), + end_date=date(2018, 10, 31)) + print(results) + assert len(results) == 2 From bd9a6352fdcbdf796fd74eb980d7d8660e5082c2 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Fri, 4 Jan 2019 16:45:39 +0000 Subject: [PATCH 2/3] Optimise the query for getting the platform statistics for all services. The page should render for all time after this change. This is one step closer to eliminating the need to read from NotificationHistory. --- app/dao/fact_notification_status_dao.py | 10 +++-- app/service/rest.py | 9 ++--- .../dao/test_fact_notification_status_dao.py | 35 +++++++++++++--- tests/app/service/test_rest.py | 40 +++++++++++-------- 4 files changed, 63 insertions(+), 31 deletions(-) diff --git a/app/dao/fact_notification_status_dao.py b/app/dao/fact_notification_status_dao.py index 3389225d7..c273ca066 100644 --- a/app/dao/fact_notification_status_dao.py +++ b/app/dao/fact_notification_status_dao.py @@ -199,7 +199,7 @@ def fetch_notification_statuses_for_job(job_id): ).all() -def fetch_stats_for_all_services_by_date_range(start_date, end_date,include_from_test_key=True): +def fetch_stats_for_all_services_by_date_range(start_date, end_date, include_from_test_key=True): stats = db.session.query( FactNotificationStatus.service_id.label('service_id'), Service.name.label('name'), @@ -230,8 +230,8 @@ def fetch_stats_for_all_services_by_date_range(start_date, end_date,include_from if not include_from_test_key: stats = stats.filter(FactNotificationStatus.key_type != KEY_TYPE_TEST) - today = get_london_midnight_in_utc(datetime.utcnow()) - if start_date <= today.date() <= end_date: + if start_date <= datetime.utcnow().date() <= end_date: + today = get_london_midnight_in_utc(datetime.utcnow()) subquery = db.session.query( Notification.notification_type.cast(db.Text).label('notification_type'), Notification.status.label('status'), @@ -284,7 +284,9 @@ def fetch_stats_for_all_services_by_date_range(start_date, end_date,include_from all_stats_table.c.notification_type, all_stats_table.c.status, ).order_by( - all_stats_table.c.service_id + all_stats_table.c.name, + all_stats_table.c.notification_type, + all_stats_table.c.status ) else: query = stats diff --git a/app/service/rest.py b/app/service/rest.py index f1c4f947e..f62a99967 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -23,8 +23,8 @@ from app.dao.api_key_dao import ( from app.dao.fact_notification_status_dao import ( fetch_notification_status_for_service_by_month, fetch_notification_status_for_service_for_day, - fetch_notification_status_for_service_for_today_and_7_previous_days -) + fetch_notification_status_for_service_for_today_and_7_previous_days, + fetch_stats_for_all_services_by_date_range) from app.dao.inbound_numbers_dao import dao_allocate_number_for_service from app.dao.organisation_dao import dao_get_organisation_by_service_id from app.dao.service_data_retention_dao import ( @@ -56,7 +56,6 @@ from app.dao.services_dao import ( dao_remove_user_from_service, dao_suspend_service, dao_update_service, - fetch_stats_by_date_range_for_all_services ) from app.dao.service_whitelist_dao import ( dao_fetch_service_whitelist, @@ -472,10 +471,10 @@ def get_detailed_services(start_date, end_date, only_active=False, include_from_ only_active=only_active) else: - stats = fetch_stats_by_date_range_for_all_services(start_date=start_date, + stats = fetch_stats_for_all_services_by_date_range(start_date=start_date, end_date=end_date, include_from_test_key=include_from_test_key, - only_active=only_active) + ) results = [] for service_id, rows in itertools.groupby(stats, lambda x: x.service_id): rows = list(rows) diff --git a/tests/app/dao/test_fact_notification_status_dao.py b/tests/app/dao/test_fact_notification_status_dao.py index 15a5b7605..64b6b5bc3 100644 --- a/tests/app/dao/test_fact_notification_status_dao.py +++ b/tests/app/dao/test_fact_notification_status_dao.py @@ -289,6 +289,7 @@ def set_up_data(): create_notification(sms_template, 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') + return service_1, service_2 def test_fetch_notification_statuses_for_job(sample_template): @@ -308,8 +309,32 @@ def test_fetch_notification_statuses_for_job(sample_template): @freeze_time('2018-10-31 14:00') def test_fetch_stats_for_all_services_by_date_range(notify_db_session): - set_up_data() - results = fetch_stats_for_all_services_by_date_range( start_date=date(2018, 10, 29), - end_date=date(2018, 10, 31)) - print(results) - assert len(results) == 2 + service_1, service_2 = set_up_data() + results = fetch_stats_for_all_services_by_date_range(start_date=date(2018, 10, 29), + end_date=date(2018, 10, 31)) + assert len(results) == 5 + + assert results[0].service_id == service_1.id + assert results[0].notification_type == 'email' + assert results[0].status == 'delivered' + assert results[0].count == 4 + + assert results[1].service_id == service_1.id + assert results[1].notification_type == 'sms' + assert results[1].status == 'created' + assert results[1].count == 2 + + assert results[2].service_id == service_1.id + assert results[2].notification_type == 'sms' + assert results[2].status == 'delivered' + assert results[2].count == 11 + + assert results[3].service_id == service_2.id + assert results[3].notification_type == 'letter' + assert results[3].status == 'delivered' + assert results[3].count == 10 + + assert results[4].service_id == service_2.id + assert not results[4].notification_type + assert not results[4].status + assert not results[4].count diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index a1a40b3a8..737846aa5 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -1642,31 +1642,37 @@ def test_get_detailed_services_only_includes_todays_notifications(notify_db, not } -@pytest.mark.parametrize( - 'set_time', - ['2017-03-28T12:00:00', '2017-01-28T12:00:00', '2017-01-02T12:00:00', '2017-10-31T12:00:00'] -) -def test_get_detailed_services_for_date_range(notify_db, notify_db_session, set_time): +@pytest.mark.parametrize("start_date_delta, end_date_delta", + [(2, 1), + (3, 2), + (1, 0) + ]) +@freeze_time('2017-03-28T12:00:00') +def test_get_detailed_services_for_date_range(sample_template, start_date_delta, end_date_delta): from app.service.rest import get_detailed_services - with freeze_time(set_time): - create_sample_notification(notify_db, notify_db_session, created_at=datetime.utcnow() - timedelta(days=3)) - create_sample_notification(notify_db, notify_db_session, created_at=datetime.utcnow() - timedelta(days=2)) - create_sample_notification(notify_db, notify_db_session, created_at=datetime.utcnow() - timedelta(days=1)) - create_sample_notification(notify_db, notify_db_session, created_at=datetime.utcnow()) + create_ft_notification_status(bst_date=(datetime.utcnow() - timedelta(days=3)).date(), + service=sample_template.service, + notification_type='sms') + create_ft_notification_status(bst_date=(datetime.utcnow() - timedelta(days=2)).date(), + service=sample_template.service, + notification_type='sms') + create_ft_notification_status(bst_date=(datetime.utcnow() - timedelta(days=1)).date(), + service=sample_template.service, + notification_type='sms') - start_date = (datetime.utcnow() - timedelta(days=2)).date() - end_date = (datetime.utcnow() - timedelta(days=1)).date() + create_notification(template=sample_template, created_at=datetime.utcnow(), status='delivered') + + start_date = (datetime.utcnow() - timedelta(days=start_date_delta)).date() + end_date = (datetime.utcnow() - timedelta(days=end_date_delta)).date() data = get_detailed_services(only_active=False, include_from_test_key=True, start_date=start_date, end_date=end_date) assert len(data) == 1 - assert data[0]['statistics'] == { - EMAIL_TYPE: {'delivered': 0, 'failed': 0, 'requested': 0}, - SMS_TYPE: {'delivered': 0, 'failed': 0, 'requested': 2}, - LETTER_TYPE: {'delivered': 0, 'failed': 0, 'requested': 0} - } + assert data[0]['statistics'][EMAIL_TYPE] == {'delivered': 0, 'failed': 0, 'requested': 0} + assert data[0]['statistics'][SMS_TYPE] == {'delivered': 2, 'failed': 0, 'requested': 2} + assert data[0]['statistics'][LETTER_TYPE] == {'delivered': 0, 'failed': 0, 'requested': 0} def test_search_for_notification_by_to_field(client, sample_template, sample_email_template): From 8fbe60bb904932fa346d59f1310321fb068d5639 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Mon, 7 Jan 2019 15:37:26 +0000 Subject: [PATCH 3/3] Remove unused query --- app/dao/services_dao.py | 45 ----------------- tests/app/dao/test_services_dao.py | 78 ------------------------------ 2 files changed, 123 deletions(-) diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py index 40918f7e7..fb5cba297 100644 --- a/app/dao/services_dao.py +++ b/app/dao/services_dao.py @@ -335,51 +335,6 @@ def dao_fetch_todays_stats_for_all_services(include_from_test_key=True, only_act return query.all() -@statsd(namespace='dao') -def fetch_stats_by_date_range_for_all_services(start_date, end_date, include_from_test_key=True, only_active=True): - start_date = get_london_midnight_in_utc(start_date) - end_date = get_london_midnight_in_utc(end_date + timedelta(days=1)) - table = NotificationHistory - - if start_date >= datetime.utcnow() - timedelta(days=7): - table = Notification - subquery = db.session.query( - table.notification_type, - table.status, - table.service_id, - func.count(table.id).label('count') - ).filter( - table.created_at >= start_date, - table.created_at < end_date - ).group_by( - table.notification_type, - table.status, - table.service_id - ) - if not include_from_test_key: - subquery = subquery.filter(table.key_type != KEY_TYPE_TEST) - subquery = subquery.subquery() - - query = db.session.query( - Service.id.label('service_id'), - Service.name, - Service.restricted, - Service.research_mode, - Service.active, - Service.created_at, - subquery.c.notification_type, - subquery.c.status, - subquery.c.count - ).outerjoin( - subquery, - subquery.c.service_id == Service.id - ).order_by(Service.id) - if only_active: - query = query.filter(Service.active) - - return query.all() - - @transactional @version_class(Service) @version_class(ApiKey) diff --git a/tests/app/dao/test_services_dao.py b/tests/app/dao/test_services_dao.py index 6ea7123e3..024b1c019 100644 --- a/tests/app/dao/test_services_dao.py +++ b/tests/app/dao/test_services_dao.py @@ -27,7 +27,6 @@ from app.dao.services_dao import ( dao_fetch_todays_stats_for_service, fetch_todays_total_message_count, dao_fetch_todays_stats_for_all_services, - fetch_stats_by_date_range_for_all_services, dao_suspend_service, dao_resume_service, dao_fetch_active_users_for_service, @@ -775,25 +774,6 @@ def test_dao_fetch_todays_stats_for_all_services_can_exclude_from_test_key(notif assert stats[0].count == 2 -def test_fetch_stats_by_date_range_for_all_services(notify_db_session): - template = create_template(service=create_service()) - create_notification(template=template, created_at=datetime.now() - timedelta(days=4)) - create_notification(template=template, created_at=datetime.now() - timedelta(days=3)) - result_one = create_notification(template=template, created_at=datetime.now() - timedelta(days=2)) - create_notification(template=template, created_at=datetime.now() - timedelta(days=1)) - create_notification(template=template, created_at=datetime.now()) - - start_date = (datetime.utcnow() - timedelta(days=2)).date() - end_date = (datetime.utcnow() - timedelta(days=1)).date() - - results = fetch_stats_by_date_range_for_all_services(start_date, end_date) - - assert len(results) == 1 - assert results[0] == (result_one.service.id, result_one.service.name, result_one.service.restricted, - result_one.service.research_mode, result_one.service.active, - result_one.service.created_at, 'sms', 'created', 2) - - @freeze_time('2001-01-01T23:59:00') def test_dao_suspend_service_marks_service_as_inactive_and_expires_api_keys(notify_db_session): service = create_service() @@ -807,64 +787,6 @@ def test_dao_suspend_service_marks_service_as_inactive_and_expires_api_keys(noti assert api_key.expiry_date == datetime(2001, 1, 1, 23, 59, 00) -@pytest.mark.parametrize("start_delta, end_delta, expected", - [("5", "1", "4"), # a date range less than 7 days ago returns test and normal notifications - ("9", "8", "1"), # a date range older than 9 days does not return test notifications. - ("8", "4", "2")]) # a date range that starts more than 7 days ago -@freeze_time('2017-10-23T00:00:00') -def test_fetch_stats_by_date_range_for_all_services_returns_test_notifications(notify_db_session, - start_delta, - end_delta, - expected): - template = create_template(service=create_service()) - result_one = create_notification(template=template, created_at=datetime.now(), key_type='test') - create_notification(template=template, created_at=datetime.now() - timedelta(days=2), key_type='test') - create_notification(template=template, created_at=datetime.now() - timedelta(days=3), key_type='test') - create_notification(template=template, created_at=datetime.now() - timedelta(days=4), key_type='normal') - create_notification(template=template, created_at=datetime.now() - timedelta(days=4), key_type='test') - create_notification(template=template, created_at=datetime.now() - timedelta(days=8), key_type='test') - create_notification(template=template, created_at=datetime.now() - timedelta(days=8), key_type='normal') - - start_date = (datetime.utcnow() - timedelta(days=int(start_delta))).date() - end_date = (datetime.utcnow() - timedelta(days=int(end_delta))).date() - - results = fetch_stats_by_date_range_for_all_services(start_date, end_date, include_from_test_key=True) - - assert len(results) == 1 - assert results[0] == (result_one.service.id, result_one.service.name, result_one.service.restricted, - result_one.service.research_mode, result_one.service.active, result_one.service.created_at, - 'sms', 'created', int(expected)) - - -@pytest.mark.parametrize("start_delta, end_delta, expected", - [("5", "1", "4"), # a date range less than 7 days ago returns test and normal notifications - ("9", "8", "1"), # a date range older than 9 days does not return test notifications. - ("8", "4", "2")]) # a date range that starts more than 7 days ago -@freeze_time('2017-10-23T23:00:00') -def test_fetch_stats_by_date_range_during_bst_hour_for_all_services_returns_test_notifications( - notify_db_session, start_delta, end_delta, expected -): - template = create_template(service=create_service()) - result_one = create_notification(template=template, created_at=datetime.now(), key_type='test') - create_notification(template=template, created_at=datetime.now() - timedelta(days=2), key_type='test') - create_notification(template=template, created_at=datetime.now() - timedelta(days=3), key_type='test') - create_notification(template=template, created_at=datetime.now() - timedelta(days=4), key_type='normal') - create_notification(template=template, created_at=datetime.now() - timedelta(days=4), key_type='test') - create_notification(template=template, created_at=datetime.now() - timedelta(days=8), key_type='normal') - create_notification(template=template, created_at=datetime.now() - timedelta(days=9), key_type='normal') - create_notification(template=template, created_at=datetime.now() - timedelta(days=9), key_type='test') - - start_date = (datetime.utcnow() - timedelta(days=int(start_delta))).date() - end_date = (datetime.utcnow() - timedelta(days=int(end_delta))).date() - - results = fetch_stats_by_date_range_for_all_services(start_date, end_date, include_from_test_key=True) - - assert len(results) == 1 - assert results[0] == (result_one.service.id, result_one.service.name, result_one.service.restricted, - result_one.service.research_mode, result_one.service.active, result_one.service.created_at, - 'sms', 'created', int(expected)) - - @freeze_time('2001-01-01T23:59:00') def test_dao_resume_service_marks_service_as_active_and_api_keys_are_still_revoked(notify_db_session): service = create_service()