From 6a5e9472203b44ff8e1aa54a5a5be53cc4c11cb6 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 30 Sep 2016 17:17:28 +0100 Subject: [PATCH 1/8] Add DAO for getting billable units/financial year MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In order to invoice people we need to know how many text message fragments they’ve sent per month. This should be per (government) financial year, ie April 1st to April 1st because we’ll only ever show a page for one year (because the 250,000 allowance is topped up at the start of every financial year). This commit only does the DAO bit, not the REST bit. --- app/dao/notifications_dao.py | 27 ++++++++++++++- tests/app/dao/test_notification_dao.py | 46 +++++++++++++++++++++++++- 2 files changed, 71 insertions(+), 2 deletions(-) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index c289f60ed..3985f3ea0 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -7,7 +7,7 @@ from datetime import ( from flask import current_app from werkzeug.datastructures import MultiDict -from sqlalchemy import (desc, func, or_, and_, asc) +from sqlalchemy import (desc, func, or_, and_, asc, cast, Text) from sqlalchemy.orm import joinedload from app import db @@ -209,6 +209,24 @@ def get_notifications_for_job(service_id, job_id, filter_dict=None, page=1, page ) +@statsd(namespace="dao") +def get_notification_billable_unit_count_per_month(service_id, year): + start, end = get_financial_year(year) + return db.session.query( + func.to_char(NotificationHistory.created_at, "FMMonth"), + func.sum(NotificationHistory.billable_units) + ).group_by( + func.to_char(NotificationHistory.created_at, "FMMonth"), + func.to_char(NotificationHistory.created_at, "YYYY-MM") + ).order_by( + func.to_char(NotificationHistory.created_at, "YYYY-MM") + ).filter( + NotificationHistory.service_id == service_id, + NotificationHistory.created_at >= start, + NotificationHistory.created_at < end + ).all() + + @statsd(namespace="dao") def get_notification_with_personalisation(service_id, notification_id, key_type): filter_dict = {'service_id': service_id, 'id': notification_id} @@ -319,3 +337,10 @@ def dao_timeout_notifications(timeout_period_in_seconds): update({'status': NOTIFICATION_TEMPORARY_FAILURE, 'updated_at': update_at}, synchronize_session=False) db.session.commit() return updated + + +def get_financial_year(year): + return ( + date(year, 4, 1), + date(year + 1, 4, 1) + ) diff --git a/tests/app/dao/test_notification_dao.py b/tests/app/dao/test_notification_dao.py index 97885f50a..7e7ac65d8 100644 --- a/tests/app/dao/test_notification_dao.py +++ b/tests/app/dao/test_notification_dao.py @@ -31,13 +31,15 @@ from app.dao.notifications_dao import ( delete_notifications_created_more_than_a_week_ago, get_notification_by_id, get_notification_for_job, + get_notification_billable_unit_count_per_month, get_notification_with_personalisation, get_notifications_for_job, get_notifications_for_service, update_notification_status_by_id, update_notification_status_by_reference, dao_delete_notifications_and_history_by_id, - dao_timeout_notifications) + dao_timeout_notifications, + get_financial_year) from notifications_utils.template import get_sms_fragment_count @@ -685,6 +687,42 @@ def test_get_all_notifications_for_job_by_status(notify_db, notify_db_session, s assert len(notifications(filter_dict={'status': NOTIFICATION_STATUS_TYPES[:3]}).items) == 3 +def test_get_notification_billable_unit_count_per_month(notify_db, notify_db_session, sample_service): + + for year, month, day in ( + (2017, 1, 1), + (2016, 8, 1), + (2016, 7, 31), + (2016, 4, 6), + (2016, 4, 6), + (2016, 4, 1), + (2016, 3, 31), + (2016, 1, 1) + ): + sample_notification( + notify_db, notify_db_session, service=sample_service, + created_at=date(year, month, day) + ) + + for financial_year, months in ( + ( + 2017, + [] + ), + ( + 2016, + [('April', 3), ('July', 1), ('August', 1), ('January', 1)] + ), + ( + 2015, + [('January', 1), ('March', 1)] + ) + ): + assert get_notification_billable_unit_count_per_month( + sample_service.id, financial_year + ) == months + + def test_update_notification(sample_notification, sample_template): assert sample_notification.status == 'created' sample_notification.status = 'failed' @@ -1158,3 +1196,9 @@ def test_should_exclude_test_key_notifications_by_default( all_notifications = get_notifications_for_service(sample_service.id, limit_days=1, key_type=KEY_TYPE_TEST).items assert len(all_notifications) == 1 + + +def test_get_financial_year(): + start, end = get_financial_year(2000) + assert start == date(2000, 4, 1) + assert end == date(2001, 4, 1) From def1d253aa523508a6c61bf27804ea1291659220 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 30 Sep 2016 19:44:13 +0100 Subject: [PATCH 2/8] Add endpoint to get billable units/financial year MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `/services/ef7a665d-11a4-425a-a180-a67ca00b69d7/billable-units?year=2016` Pretty much just passes through to the DAO layer. Validates that year is: - present (there’s no need for unbounded queries on this endpoint) - an integer --- app/service/rest.py | 10 ++++++++++ tests/app/service/test_rest.py | 24 ++++++++++++++++++++++++ 2 files changed, 34 insertions(+) diff --git a/app/service/rest.py b/app/service/rest.py index 7aa823f80..2cc7dab90 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -310,3 +310,13 @@ def update_whitelist(service_id): else: dao_add_and_commit_whitelisted_contacts(whitelist_objs) return '', 204 + + +@service_blueprint.route('//billable-units') +def get_billable_unit_count(service_id): + try: + return jsonify(notifications_dao.get_notification_billable_unit_count_per_month( + service_id, int(request.args.get('year')) + )) + except TypeError: + return jsonify(result='error', message='No valid year provided'), 400 diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 179d9383a..69e382c4e 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -1300,3 +1300,27 @@ def test_get_detailed_services_only_includes_todays_notifications(notify_db, not 'email': {'delivered': 0, 'failed': 0, 'requested': 0}, 'sms': {'delivered': 0, 'failed': 0, 'requested': 2} } + + +@freeze_time('2012-12-12T12:00:01') +def test_get_notification_billable_unit_count(client, notify_db, notify_db_session): + notification = create_sample_notification(notify_db, notify_db_session) + response = client.get( + '/service/{}/billable-units?year=2012'.format(notification.service_id), + headers=[create_authorization_header(service_id=notification.service_id)] + ) + assert response.status_code == 200 + assert json.loads(response.get_data(as_text=True)) == { + 'December': 1 + } + + +def test_get_notification_billable_unit_count_missing_year(client, sample_service): + response = client.get( + '/service/{}/billable-units'.format(sample_service.id), + headers=[create_authorization_header(service_id=sample_service.id)] + ) + assert response.status_code == 400 + assert json.loads(response.get_data(as_text=True)) == { + 'message': 'No valid year provided', 'result': 'error' + } From 621e015f5ffee7aa26ccff9a33769cdc580a4f2d Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 3 Oct 2016 14:15:41 +0100 Subject: [PATCH 3/8] Test that a year with no notifications returns empty list --- tests/app/dao/test_notification_dao.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/app/dao/test_notification_dao.py b/tests/app/dao/test_notification_dao.py index 7e7ac65d8..51fa9284b 100644 --- a/tests/app/dao/test_notification_dao.py +++ b/tests/app/dao/test_notification_dao.py @@ -716,6 +716,10 @@ def test_get_notification_billable_unit_count_per_month(notify_db, notify_db_ses ( 2015, [('January', 1), ('March', 1)] + ), + ( + 2014, + [] ) ): assert get_notification_billable_unit_count_per_month( From 7abe40b50606123aaead21dd07b73dc688af05e4 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 3 Oct 2016 14:55:00 +0100 Subject: [PATCH 4/8] Make billing year aware of British Summer Time MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit April 1st is in British summer time, ie 1hr ahead of UTC. The database stores everything in UTC, so for accurate comparisions we need to make sure that `get_financial_year()` returns a UTC, datetime-aware timestamp that is 1hr ahead of midnight. This also means that when we group notifications by month, the months need to be in BST. So the line between one year and another is actually 01:00 on April 1st, _not_ 00:00 on April 1st. There’s no way we’ve found to do this in SQLAlchemy or raw Postgres, especially because we don’t store the timestamps with a timezone in the database. So the grouping and summing of the notifications has to be done in Python. --- app/dao/notifications_dao.py | 39 ++++++++++++++++++-------- tests/app/dao/test_notification_dao.py | 20 ++++++++----- 2 files changed, 41 insertions(+), 18 deletions(-) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 3985f3ea0..cebb1964d 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -1,9 +1,11 @@ import uuid +import pytz from datetime import ( datetime, timedelta, date ) +from itertools import groupby from flask import current_app from werkzeug.datastructures import MultiDict @@ -212,20 +214,25 @@ def get_notifications_for_job(service_id, job_id, filter_dict=None, page=1, page @statsd(namespace="dao") def get_notification_billable_unit_count_per_month(service_id, year): start, end = get_financial_year(year) - return db.session.query( - func.to_char(NotificationHistory.created_at, "FMMonth"), - func.sum(NotificationHistory.billable_units) - ).group_by( - func.to_char(NotificationHistory.created_at, "FMMonth"), - func.to_char(NotificationHistory.created_at, "YYYY-MM") + + notifications = db.session.query( + NotificationHistory.created_at, + NotificationHistory.billable_units ).order_by( - func.to_char(NotificationHistory.created_at, "YYYY-MM") + NotificationHistory.created_at ).filter( NotificationHistory.service_id == service_id, NotificationHistory.created_at >= start, NotificationHistory.created_at < end ).all() + return [ + (month, sum(count for _, count in row)) + for month, row in groupby( + notifications, lambda row: get_bst_month(row[0]) + ) + ] + @statsd(namespace="dao") def get_notification_with_personalisation(service_id, notification_id, key_type): @@ -340,7 +347,17 @@ def dao_timeout_notifications(timeout_period_in_seconds): def get_financial_year(year): - return ( - date(year, 4, 1), - date(year + 1, 4, 1) - ) + return (get_april_fools(year), get_april_fools(year + 1)) + + +def get_april_fools(year): + return datetime( + year, 4, 1, 0, 0, 0, 0, + pytz.timezone("Europe/London") + ).astimezone(pytz.utc) + + +def get_bst_month(datetime): + return pytz.utc.localize(datetime).replace( + tzinfo=pytz.timezone("Europe/London") + ).strftime('%B') diff --git a/tests/app/dao/test_notification_dao.py b/tests/app/dao/test_notification_dao.py index 51fa9284b..7e7ac1d18 100644 --- a/tests/app/dao/test_notification_dao.py +++ b/tests/app/dao/test_notification_dao.py @@ -1,4 +1,5 @@ from datetime import datetime, timedelta, date +import pytz import uuid from functools import partial @@ -690,18 +691,21 @@ def test_get_all_notifications_for_job_by_status(notify_db, notify_db_session, s def test_get_notification_billable_unit_count_per_month(notify_db, notify_db_session, sample_service): for year, month, day in ( - (2017, 1, 1), + (2017, 1, 1), # ↓ 2016 financial year (2016, 8, 1), (2016, 7, 31), (2016, 4, 6), (2016, 4, 6), - (2016, 4, 1), + (2016, 4, 1), # ↓ 2015 financial year (2016, 3, 31), (2016, 1, 1) ): sample_notification( notify_db, notify_db_session, service=sample_service, - created_at=date(year, month, day) + created_at=datetime( + year, month, day, 0, 0, 0, 0, + tzinfo=pytz.utc + ) ) for financial_year, months in ( @@ -711,11 +715,11 @@ def test_get_notification_billable_unit_count_per_month(notify_db, notify_db_ses ), ( 2016, - [('April', 3), ('July', 1), ('August', 1), ('January', 1)] + [('April', 2), ('July', 1), ('August', 1), ('January', 1)] ), ( 2015, - [('January', 1), ('March', 1)] + [('January', 1), ('March', 1), ('April', 1)] ), ( 2014, @@ -1204,5 +1208,7 @@ def test_should_exclude_test_key_notifications_by_default( def test_get_financial_year(): start, end = get_financial_year(2000) - assert start == date(2000, 4, 1) - assert end == date(2001, 4, 1) + assert start.tzinfo == pytz.utc + assert start.isoformat() == '2000-04-01T00:01:00+00:00' + assert end.tzinfo == pytz.utc + assert end.isoformat() == '2001-04-01T00:01:00+00:00' From 76d5f14952ab755854fa4b38e6320221c727eacd Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 4 Oct 2016 09:51:41 +0100 Subject: [PATCH 5/8] Filter unbillable notifications before calculating MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Notifications with a `billable_units` count of `0` wont have any effect on the result, but including them in the query will slow down the grouping and summing of the results because it’ll have to loop over more rows. --- app/dao/notifications_dao.py | 1 + 1 file changed, 1 insertion(+) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index cebb1964d..037a0b7b4 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -221,6 +221,7 @@ def get_notification_billable_unit_count_per_month(service_id, year): ).order_by( NotificationHistory.created_at ).filter( + NotificationHistory.billable_units != 0, NotificationHistory.service_id == service_id, NotificationHistory.created_at >= start, NotificationHistory.created_at < end From d352c0eed9a70d8133c4ef4f3988ab18edc6effe Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 4 Oct 2016 13:00:37 +0100 Subject: [PATCH 6/8] Really fix the timezones MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two main changes: - uses `astimezone` instead of `replace` because `replace` doesn’t handle daylight savings time correctly [1] - create the notifications one second before midnight in BST, because midnight is actually counted as being start of the _next_ day, month, etc 1. http://www.saltycrane.com/blog/2009/05/converting-time-zones-datetime-objects-python/#add-timezone-localize --- app/dao/notifications_dao.py | 4 ++-- tests/app/dao/test_notification_dao.py | 19 +++++++++---------- 2 files changed, 11 insertions(+), 12 deletions(-) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 037a0b7b4..870239837 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -359,6 +359,6 @@ def get_april_fools(year): def get_bst_month(datetime): - return pytz.utc.localize(datetime).replace( - tzinfo=pytz.timezone("Europe/London") + return pytz.utc.localize(datetime).astimezone( + pytz.timezone("Europe/London") ).strftime('%B') diff --git a/tests/app/dao/test_notification_dao.py b/tests/app/dao/test_notification_dao.py index 7e7ac1d18..ea7779ecb 100644 --- a/tests/app/dao/test_notification_dao.py +++ b/tests/app/dao/test_notification_dao.py @@ -691,21 +691,20 @@ def test_get_all_notifications_for_job_by_status(notify_db, notify_db_session, s def test_get_notification_billable_unit_count_per_month(notify_db, notify_db_session, sample_service): for year, month, day in ( - (2017, 1, 1), # ↓ 2016 financial year + (2017, 1, 15), # ↓ 2016 financial year (2016, 8, 1), - (2016, 7, 31), - (2016, 4, 6), - (2016, 4, 6), + (2016, 7, 15), + (2016, 4, 15), + (2016, 4, 15), (2016, 4, 1), # ↓ 2015 financial year (2016, 3, 31), - (2016, 1, 1) + (2016, 1, 15) ): sample_notification( notify_db, notify_db_session, service=sample_service, created_at=datetime( - year, month, day, 0, 0, 0, 0, - tzinfo=pytz.utc - ) + year, month, day, 0, 0, 0, 0 + ) - timedelta(hours=1, seconds=1) # one second before midnight ) for financial_year, months in ( @@ -715,11 +714,11 @@ def test_get_notification_billable_unit_count_per_month(notify_db, notify_db_ses ), ( 2016, - [('April', 2), ('July', 1), ('August', 1), ('January', 1)] + [('April', 2), ('July', 2), ('January', 1)] ), ( 2015, - [('January', 1), ('March', 1), ('April', 1)] + [('January', 1), ('March', 2)] ), ( 2014, From 219d5943c1be33dfdb667330a640cf7db9c99f52 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 4 Oct 2016 13:04:42 +0100 Subject: [PATCH 7/8] Return iterator from query for speed --- app/dao/notifications_dao.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 870239837..40c1e8cb2 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -225,7 +225,7 @@ def get_notification_billable_unit_count_per_month(service_id, year): NotificationHistory.service_id == service_id, NotificationHistory.created_at >= start, NotificationHistory.created_at < end - ).all() + ) return [ (month, sum(count for _, count in row)) From 826eaaf5b3509e2f10fcd6f44c3ddb59803ec2c8 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 4 Oct 2016 13:05:15 +0100 Subject: [PATCH 8/8] Remove unecessary brackets when returning tuple --- app/dao/notifications_dao.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 40c1e8cb2..dd23d32b5 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -348,7 +348,7 @@ def dao_timeout_notifications(timeout_period_in_seconds): def get_financial_year(year): - return (get_april_fools(year), get_april_fools(year + 1)) + return get_april_fools(year), get_april_fools(year + 1) def get_april_fools(year):