From 5f2724c4299ba3000be16ea3c2a775cdc3621bb6 Mon Sep 17 00:00:00 2001 From: Alexey Bezhan Date: Mon, 13 Aug 2018 16:34:04 +0100 Subject: [PATCH 1/3] Add limit_days argument to notification statistics endpoint Allows getting notification counts for a given number of days to support services with custom data retention periods (admin dashboard page should still display counts for the last 7 days, while the notifications page displays all stored notifications). --- app/dao/services_dao.py | 4 ++-- app/service/rest.py | 15 +++++++++++---- tests/app/dao/test_services_dao.py | 10 +++++----- 3 files changed, 18 insertions(+), 11 deletions(-) diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py index b9c66a3a0..40918f7e7 100644 --- a/app/dao/services_dao.py +++ b/app/dao/services_dao.py @@ -246,9 +246,9 @@ def delete_service_and_all_associated_db_objects(service): @statsd(namespace="dao") -def dao_fetch_stats_for_service(service_id): +def dao_fetch_stats_for_service(service_id, limit_days): # We always want between seven and eight days - start_date = midnight_n_days_ago(7) + start_date = midnight_n_days_ago(limit_days) return _stats_for_service_query(service_id).filter( Notification.created_at >= start_date ).all() diff --git a/app/service/rest.py b/app/service/rest.py index 8eb157b98..8975aa9df 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -165,7 +165,11 @@ def get_service_by_id(service_id): @service_blueprint.route('//statistics') def get_service_notification_statistics(service_id): - return jsonify(data=get_service_statistics(service_id, request.args.get('today_only') == 'True')) + return jsonify(data=get_service_statistics( + service_id, + request.args.get('today_only') == 'True', + int(request.args.get('limit_days', 7)) + )) @service_blueprint.route('', methods=['POST']) @@ -423,10 +427,13 @@ def get_detailed_service(service_id, today_only=False): return detailed_service_schema.dump(service).data -def get_service_statistics(service_id, today_only): +def get_service_statistics(service_id, today_only, limit_days=7): # today_only flag is used by the send page to work out if the service will exceed their daily usage by sending a job - stats_fn = dao_fetch_todays_stats_for_service if today_only else dao_fetch_stats_for_service - stats = stats_fn(service_id) + if today_only: + stats = dao_fetch_todays_stats_for_service(service_id) + else: + stats = dao_fetch_stats_for_service(service_id, limit_days=limit_days) + return statistics.format_statistics(stats) diff --git a/tests/app/dao/test_services_dao.py b/tests/app/dao/test_services_dao.py index 2720db90c..e1258b7e8 100644 --- a/tests/app/dao/test_services_dao.py +++ b/tests/app/dao/test_services_dao.py @@ -534,7 +534,7 @@ def test_fetch_stats_filters_on_service(sample_notification): message_limit=1000) dao_create_service(service_two, sample_notification.service.created_by) - stats = dao_fetch_stats_for_service(service_two.id) + stats = dao_fetch_stats_for_service(service_two.id, 7) assert len(stats) == 0 @@ -546,7 +546,7 @@ def test_fetch_stats_ignores_historical_notification_data(sample_notification): assert Notification.query.count() == 0 assert NotificationHistory.query.count() == 1 - stats = dao_fetch_stats_for_service(service_id) + stats = dao_fetch_stats_for_service(service_id, 7) assert len(stats) == 0 @@ -557,7 +557,7 @@ def test_fetch_stats_counts_correctly(notify_db, notify_db_session, sample_templ create_notification(notify_db, notify_db_session, template=sample_email_template, status='technical-failure') create_notification(notify_db, notify_db_session, template=sample_template, status='created') - stats = dao_fetch_stats_for_service(sample_template.service_id) + stats = dao_fetch_stats_for_service(sample_template.service_id, 7) stats = sorted(stats, key=lambda x: (x.notification_type, x.status)) assert len(stats) == 3 @@ -591,7 +591,7 @@ def test_fetch_stats_counts_should_ignore_team_key( create_notification( notify_db, notify_db_session) - stats = dao_fetch_stats_for_service(sample_template.service_id) + stats = dao_fetch_stats_for_service(sample_template.service_id, 7) assert len(stats) == 1 assert stats[0].notification_type == 'sms' assert stats[0].status == 'created' @@ -634,7 +634,7 @@ def test_fetch_stats_should_not_gather_notifications_older_than_7_days(sample_te create_notification_db(sample_template,) with freeze_time('Monday 16th July 2018 12:00'): - stats = dao_fetch_stats_for_service(sample_template.service_id) + stats = dao_fetch_stats_for_service(sample_template.service_id, 7) assert len(stats) == rows_returned From ab428048b93988a3c70c187261c7897e2dae2946 Mon Sep 17 00:00:00 2001 From: Alexey Bezhan Date: Mon, 13 Aug 2018 16:44:24 +0100 Subject: [PATCH 2/3] Add an endpoint to fetch service data retention by type Admin app needs to get the service data retention for the specified notification type, so to avoid iterating through the list of all existing service data retention settings we restore the endpoint to get the individual data retention period. --- app/service/rest.py | 7 +++++++ tests/app/service/test_service_data_retention_rest.py | 9 +++++++++ 2 files changed, 16 insertions(+) diff --git a/app/service/rest.py b/app/service/rest.py index 8975aa9df..d2e383817 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -27,6 +27,7 @@ from app.dao.organisation_dao import dao_get_organisation_by_service_id from app.dao.service_data_retention_dao import ( fetch_service_data_retention, fetch_service_data_retention_by_id, + fetch_service_data_retention_by_notification_type, insert_service_data_retention, update_service_data_retention, ) @@ -759,6 +760,12 @@ def get_data_retention_for_service(service_id): return jsonify([data_retention.serialize() for data_retention in data_retention_list]), 200 +@service_blueprint.route('//data-retention/notification-type/', methods=['GET']) +def get_data_retention_for_service_notification_type(service_id, notification_type): + data_retention = fetch_service_data_retention_by_notification_type(service_id, notification_type) + return jsonify(data_retention.serialize() if data_retention else {}), 200 + + @service_blueprint.route('//data-retention/', methods=['GET']) def get_data_retention_for_service_by_id(service_id, data_retention_id): data_retention = fetch_service_data_retention_by_id(service_id, data_retention_id) diff --git a/tests/app/service/test_service_data_retention_rest.py b/tests/app/service/test_service_data_retention_rest.py index da58d597e..7797f62cb 100644 --- a/tests/app/service/test_service_data_retention_rest.py +++ b/tests/app/service/test_service_data_retention_rest.py @@ -35,6 +35,15 @@ def test_get_service_data_retention_returns_empty_list(client, sample_service): assert len(json.loads(response.get_data(as_text=True))) == 0 +def test_get_data_retention_for_service_notification_type(client, sample_service): + data_retention = create_service_data_retention(service_id=sample_service.id) + response = client.get('/service/{}/data-retention/notification-type/{}'.format(sample_service.id, 'sms'), + headers=[('Content-Type', 'application/json'), create_authorization_header()], + ) + assert response.status_code == 200 + assert json.loads(response.get_data(as_text=True)) == data_retention.serialize() + + def test_get_service_data_retention_by_id(client, sample_service): sms_data_retention = create_service_data_retention(service_id=sample_service.id) create_service_data_retention(service_id=sample_service.id, notification_type='email', From 1e3004ab3a43ed9832090a6b2a5e1f0512b5a55a Mon Sep 17 00:00:00 2001 From: Alexey Bezhan Date: Wed, 15 Aug 2018 11:10:16 +0100 Subject: [PATCH 3/3] Add service stats dao tests with variable days limit --- tests/app/dao/test_services_dao.py | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/tests/app/dao/test_services_dao.py b/tests/app/dao/test_services_dao.py index e1258b7e8..516ffba15 100644 --- a/tests/app/dao/test_services_dao.py +++ b/tests/app/dao/test_services_dao.py @@ -620,21 +620,25 @@ def test_fetch_stats_for_today_only_includes_today(notify_db, notify_db_session, assert stats['created'] == 1 -@pytest.mark.parametrize('created_at, rows_returned', [ - ('Sunday 8th July 2018 12:00', 0), - ('Sunday 8th July 2018 22:59', 0), - ('Sunday 8th July 2018 23:00', 1), - ('Monday 9th July 2018 09:00', 1), - ('Monday 9th July 2018 15:00', 1), - ('Monday 16th July 2018 12:00', 1), +@pytest.mark.parametrize('created_at, limit_days, rows_returned', [ + ('Sunday 8th July 2018 12:00', 7, 0), + ('Sunday 8th July 2018 22:59', 7, 0), + ('Sunday 1th July 2018 12:00', 10, 0), + ('Sunday 8th July 2018 23:00', 7, 1), + ('Monday 9th July 2018 09:00', 7, 1), + ('Monday 9th July 2018 15:00', 7, 1), + ('Monday 16th July 2018 12:00', 7, 1), + ('Sunday 8th July 2018 12:00', 10, 1), ]) -def test_fetch_stats_should_not_gather_notifications_older_than_7_days(sample_template, created_at, rows_returned): +def test_fetch_stats_should_not_gather_notifications_older_than_7_days( + sample_template, created_at, limit_days, rows_returned +): # It's monday today. Things made last monday should still show with freeze_time(created_at): create_notification_db(sample_template,) with freeze_time('Monday 16th July 2018 12:00'): - stats = dao_fetch_stats_for_service(sample_template.service_id, 7) + stats = dao_fetch_stats_for_service(sample_template.service_id, limit_days) assert len(stats) == rows_returned