From 56ba653f488aad84ec54e8272e55497a2aed9711 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 30 Jan 2017 15:17:26 +0000 Subject: [PATCH 1/3] Add endpoint for breakdown of activity by month MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This endpoint will eventualy replace the weekly breakdown one. By month for a given financial year is better, because it gives us consistency with the breakdown of financial usage (and eventually consistency with the template usage). The code to do this is a bit convoluted, in order to fill out the counts for months and statuses where we don’t have notifications. This will make the admin side of this easier, because we can rely on there always being numbers available. The admin side will deal with summing the statuses (eg `temporary-failure` > `failed`) because this is presentational. This commit also modifies the usage count to use `.between()` for consistency. --- app/dao/notifications_dao.py | 3 +- app/dao/services_dao.py | 56 +++++++++++++++++++++++++++++- app/service/rest.py | 16 ++++++++- tests/app/dao/test_services_dao.py | 45 ++++++++++++++++++++++++ tests/app/service/test_rest.py | 32 +++++++++++++++++ 5 files changed, 148 insertions(+), 4 deletions(-) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 3dff3f390..518811a28 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -241,8 +241,7 @@ def get_notification_billable_unit_count_per_month(service_id, year): ).filter( NotificationHistory.billable_units != 0, NotificationHistory.service_id == service_id, - NotificationHistory.created_at >= start, - NotificationHistory.created_at < end + NotificationHistory.created_at.between(*get_financial_year(year)), ).group_by( month ).order_by( diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py index eb53c769c..50f582ad2 100644 --- a/app/dao/services_dao.py +++ b/app/dao/services_dao.py @@ -9,6 +9,7 @@ from app.dao.dao_utils import ( transactional, version_class ) +from app.dao.notifications_dao import get_financial_year from app.models import ( NotificationStatistics, TemplateStatistics, @@ -24,7 +25,10 @@ from app.models import ( User, InvitedUser, Service, - KEY_TYPE_TEST) + KEY_TYPE_TEST, + NOTIFICATION_STATUS_TYPES, + TEMPLATE_TYPES, +) from app.statsd_decorators import statsd @@ -238,6 +242,56 @@ def dao_fetch_weekly_historical_stats_for_service(service_id): ).all() +@statsd(namespace="dao") +def dao_fetch_monthly_historical_stats_for_service(service_id, year): + monday_of_notification_week = func.date_trunc('week', NotificationHistory.created_at).label('week_start') + start, end = get_financial_year(year) + + month = func.date_trunc( + "month", + func.timezone( + "Europe/London", + func.timezone("UTC", NotificationHistory.created_at) + ) + ) + + rows = db.session.query( + NotificationHistory.notification_type, + NotificationHistory.status, + month, + func.count(NotificationHistory.id).label('count') + ).filter( + NotificationHistory.service_id == service_id, + NotificationHistory.created_at.between(*get_financial_year(year)), + ).group_by( + NotificationHistory.notification_type, + NotificationHistory.status, + month + ).order_by( + month + ) + + months = { + datetime.strftime(date, '%Y-%m'): dict.fromkeys( + TEMPLATE_TYPES, + dict.fromkeys( + NOTIFICATION_STATUS_TYPES, + 0 + ) + ) + for date in [ + datetime(year, month, 1) for month in range(4, 13) + ] + [ + datetime(year + 1, month, 1) for month in range(1, 4) + ] + } + + for notification_type, status, date, count in rows: + months[datetime.strftime(date, "%Y-%m")][notification_type][status] = count + + return months + + @statsd(namespace='dao') def dao_fetch_todays_stats_for_all_services(include_from_test_key=True): query = db.session.query( diff --git a/app/service/rest.py b/app/service/rest.py index 416bace74..8ba9230e0 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -30,7 +30,9 @@ from app.dao.services_dao import ( dao_archive_service, fetch_stats_by_date_range_for_all_services, dao_suspend_service, - dao_resume_service) + dao_resume_service, + dao_fetch_monthly_historical_stats_for_service, +) from app.dao.service_whitelist_dao import ( dao_fetch_service_whitelist, dao_add_and_commit_whitelisted_contacts, @@ -273,6 +275,18 @@ def get_weekly_notification_stats(service_id): return jsonify(data={week.date().isoformat(): statistics for week, statistics in stats.items()}) +@service_blueprint.route('//notifications/monthly', methods=['GET']) +def get_monthly_notification_stats(service_id): + service = dao_fetch_service_by_id(service_id) + try: + return jsonify(data=dao_fetch_monthly_historical_stats_for_service( + service.id, + int(request.args.get('year', 'NaN')) + )) + except ValueError: + raise InvalidRequest('Year must be a number', status_code=400) + + def get_detailed_service(service_id, today_only=False): service = dao_fetch_service_by_id(service_id) stats_fn = dao_fetch_todays_stats_for_service if today_only else dao_fetch_stats_for_service diff --git a/tests/app/dao/test_services_dao.py b/tests/app/dao/test_services_dao.py index 52624e0d1..943831ae9 100644 --- a/tests/app/dao/test_services_dao.py +++ b/tests/app/dao/test_services_dao.py @@ -20,6 +20,7 @@ from app.dao.services_dao import ( dao_fetch_stats_for_service, dao_fetch_todays_stats_for_service, dao_fetch_weekly_historical_stats_for_service, + dao_fetch_monthly_historical_stats_for_service, fetch_todays_total_message_count, dao_fetch_todays_stats_for_all_services, fetch_stats_by_date_range_for_all_services, @@ -507,6 +508,50 @@ def test_fetch_weekly_historical_stats_separates_weeks(notify_db, notify_db_sess assert ret[-1].count == 1 +def test_fetch_monthly_historical_stats_separates_weeks(notify_db, notify_db_session, sample_template): + notification_history = functools.partial( + create_notification_history, + notify_db, + notify_db_session, + sample_template + ) + _before_start_of_financial_year = notification_history(created_at=datetime(2016, 3, 31)) + start_of_financial_year = notification_history(created_at=datetime(2016, 4, 1)) + start_of_summer = notification_history(created_at=datetime(2016, 6, 20)) + start_of_autumn = notification_history(created_at=datetime(2016, 9, 22)) + start_of_winter = notification_history(created_at=datetime(2016, 12, 1), status='delivered') + start_of_spring = notification_history(created_at=datetime(2017, 3, 11)) + end_of_financial_year = notification_history(created_at=datetime(2017, 3, 31)) + _after_end_of_financial_year = notification_history(created_at=datetime(2017, 4, 1)) + + result = dao_fetch_monthly_historical_stats_for_service(sample_template.service_id, 2016) + + assert result['2016-04']['sms']['created'] == 1 + assert result['2016-04']['sms']['sending'] == 0 + assert result['2016-04']['sms']['delivered'] == 0 + assert result['2016-04']['sms']['pending'] == 0 + assert result['2016-04']['sms']['failed'] == 0 + assert result['2016-04']['sms']['technical-failure'] == 0 + assert result['2016-04']['sms']['temporary-failure'] == 0 + assert result['2016-04']['sms']['permanent-failure'] == 0 + + assert result['2016-06']['sms']['created'] == 1 + + assert result['2016-09']['sms']['created'] == 1 + + assert result['2016-12']['sms']['created'] == 0 + assert result['2016-12']['sms']['delivered'] == 1 + + assert result['2017-03']['sms']['created'] == 2 + + assert result.keys() == { + '2016-04', '2016-05', '2016-06', + '2016-07', '2016-08', '2016-09', + '2016-10', '2016-11', '2016-12', + '2017-01', '2017-02', '2017-03', + } + + def test_fetch_weekly_historical_stats_ignores_second_service(notify_db, notify_db_session, service_factory): template_1 = service_factory.get('1').templates[0] template_2 = service_factory.get('2').templates[0] diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 5835d5159..936789e33 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -1221,6 +1221,38 @@ def test_get_weekly_notification_stats(notify_api, notify_db, notify_db_session) } +@pytest.mark.parametrize( + 'url, expected_status, expected_json', [ + ( + '/service/{}/notifications/monthly?year=2001', + 200, + {'data': {'foo': 'bar'}}, + ), + ( + '/service/{}/notifications/monthly?year=baz', + 400, + {'message': 'Year must be a number', 'result': 'error'}, + ), + ( + '/service/{}/notifications/monthly', + 400, + {'message': 'Year must be a number', 'result': 'error'}, + ), + ] +) +def test_get_monthly_notification_stats(mocker, client, sample_service, url, expected_status, expected_json): + mock_dao = mocker.patch( + 'app.service.rest.dao_fetch_monthly_historical_stats_for_service', + return_value={'foo': 'bar'}, + ) + response = client.get( + url.format(sample_service.id), + headers=[create_authorization_header()] + ) + assert response.status_code == expected_status + assert json.loads(response.get_data(as_text=True)) == expected_json + + def test_get_services_with_detailed_flag(notify_api, notify_db, notify_db_session): notifications = [ create_sample_notification(notify_db, notify_db_session), From 8381a5e11ca00d72d081b9c93b99a5ca2fe7042e Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 30 Jan 2017 16:34:27 +0000 Subject: [PATCH 2/3] Ensure that monthly service usage groups by BST --- tests/app/dao/test_services_dao.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/app/dao/test_services_dao.py b/tests/app/dao/test_services_dao.py index 943831ae9..1a76eb50d 100644 --- a/tests/app/dao/test_services_dao.py +++ b/tests/app/dao/test_services_dao.py @@ -518,11 +518,11 @@ def test_fetch_monthly_historical_stats_separates_weeks(notify_db, notify_db_ses _before_start_of_financial_year = notification_history(created_at=datetime(2016, 3, 31)) start_of_financial_year = notification_history(created_at=datetime(2016, 4, 1)) start_of_summer = notification_history(created_at=datetime(2016, 6, 20)) - start_of_autumn = notification_history(created_at=datetime(2016, 9, 22)) + start_of_autumn = notification_history(created_at=datetime(2016, 9, 30, 23, 30, 0)) # October because BST start_of_winter = notification_history(created_at=datetime(2016, 12, 1), status='delivered') start_of_spring = notification_history(created_at=datetime(2017, 3, 11)) end_of_financial_year = notification_history(created_at=datetime(2017, 3, 31)) - _after_end_of_financial_year = notification_history(created_at=datetime(2017, 4, 1)) + _after_end_of_financial_year = notification_history(created_at=datetime(2017, 3, 31, 23, 30)) # after because BST result = dao_fetch_monthly_historical_stats_for_service(sample_template.service_id, 2016) @@ -537,7 +537,7 @@ def test_fetch_monthly_historical_stats_separates_weeks(notify_db, notify_db_ses assert result['2016-06']['sms']['created'] == 1 - assert result['2016-09']['sms']['created'] == 1 + assert result['2016-10']['sms']['created'] == 1 assert result['2016-12']['sms']['created'] == 0 assert result['2016-12']['sms']['delivered'] == 1 From 45b505f3790b69fa75e17156a37a95db73db66fb Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 30 Jan 2017 16:46:47 +0000 Subject: [PATCH 3/3] Split out handling of BST queries for reuse --- app/dao/notifications_dao.py | 13 +++---------- app/dao/services_dao.py | 9 ++------- app/utils.py | 17 +++++++++++++++++ 3 files changed, 22 insertions(+), 17 deletions(-) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 518811a28..5560a55d1 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -27,6 +27,7 @@ from app.models import ( from app.dao.dao_utils import transactional from app.statsd_decorators import statsd +from app.utils import get_london_month_from_utc_column def dao_get_notification_statistics_for_service_and_day(service_id, day): @@ -225,16 +226,8 @@ def get_notifications_for_job(service_id, job_id, filter_dict=None, page=1, page def get_notification_billable_unit_count_per_month(service_id, year): start, end = get_financial_year(year) - """ - The query needs to sum the billable_units per month, but this needs to be the month in BST (British Standard Time). - The database stores all timestamps as UTC without the timezone. - - First set the timezone on created_at to UTC - - then convert the timezone to BST (or Europe/London) - - lastly truncate the datetime to month to group the sum of the billable_units - """ - month = func.date_trunc("month", - func.timezone("Europe/London", func.timezone("UTC", - NotificationHistory.created_at))) + month = get_london_month_from_utc_column(NotificationHistory.created_at) + notifications = db.session.query( month, func.sum(NotificationHistory.billable_units) diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py index 50f582ad2..b9ad66ae9 100644 --- a/app/dao/services_dao.py +++ b/app/dao/services_dao.py @@ -30,6 +30,7 @@ from app.models import ( TEMPLATE_TYPES, ) from app.statsd_decorators import statsd +from app.utils import get_london_month_from_utc_column def dao_fetch_all_services(only_active=False): @@ -247,13 +248,7 @@ def dao_fetch_monthly_historical_stats_for_service(service_id, year): monday_of_notification_week = func.date_trunc('week', NotificationHistory.created_at).label('week_start') start, end = get_financial_year(year) - month = func.date_trunc( - "month", - func.timezone( - "Europe/London", - func.timezone("UTC", NotificationHistory.created_at) - ) - ) + month = get_london_month_from_utc_column(NotificationHistory.created_at) rows = db.session.query( NotificationHistory.notification_type, diff --git a/app/utils.py b/app/utils.py index e8430b937..522d008f5 100644 --- a/app/utils.py +++ b/app/utils.py @@ -2,6 +2,7 @@ from datetime import datetime, timedelta import pytz from flask import url_for +from sqlalchemy import func from notifications_utils.template import SMSMessageTemplate, PlainTextEmailTemplate @@ -46,3 +47,19 @@ def get_london_midnight_in_utc(date): def get_midnight_for_day_before(date): day_before = date - timedelta(1) return get_london_midnight_in_utc(day_before) + + +def get_london_month_from_utc_column(column): + """ + Where queries need to count notifications by month it needs to be + the month in BST (British Summer Time). + The database stores all timestamps as UTC without the timezone. + - First set the timezone on created_at to UTC + - then convert the timezone to BST (or Europe/London) + - lastly truncate the datetime to month with which we can group + queries + """ + return func.date_trunc( + "month", + func.timezone("Europe/London", func.timezone("UTC", column)) + )