From 88d92d607007607c49cd1997238537f3828d0b57 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Fri, 28 Apr 2017 16:55:41 +0100 Subject: [PATCH 1/3] Fix the logic gettting the rates for a financial year. The is_between_end_date_exclusive is a bit funny. Perhaps the better way to handle it is to make the function is_between but change the financial year function return an enddate that is one millisecond less. That way we can always use the between logic and it will be easier to use. --- app/dao/notification_usage_dao.py | 23 +++++++-- tests/app/dao/test_notification_usage_dao.py | 54 ++++++++++++++++++-- 2 files changed, 70 insertions(+), 7 deletions(-) diff --git a/app/dao/notification_usage_dao.py b/app/dao/notification_usage_dao.py index fb412d849..7d29f7315 100644 --- a/app/dao/notification_usage_dao.py +++ b/app/dao/notification_usage_dao.py @@ -1,7 +1,7 @@ from datetime import datetime from sqlalchemy import Float, Integer -from sqlalchemy import func, case, cast +from sqlalchemy import func, case, cast, desc, between from sqlalchemy import literal_column from app import db @@ -103,8 +103,25 @@ def sms_yearly_billing_data_query(rate, service_id, start_date, end_date): def get_rates_for_year(start_date, end_date, notification_type): - return Rate.query.filter(Rate.valid_from >= start_date, Rate.valid_from < end_date, - Rate.notification_type == notification_type).order_by(Rate.valid_from).all() + rates = Rate.query.filter(Rate.notification_type == notification_type).order_by(Rate.valid_from).all() + results = [] + for current_rate, current_rate_expiry_date in zip(rates, rates[1:]): + if is_between_end_date_exclusive(current_rate.valid_from, start_date, end_date) or \ + is_between_end_date_exclusive(current_rate_expiry_date.valid_from, start_date, end_date): + results.append(current_rate) + + if is_between_end_date_exclusive(rates[-1].valid_from, start_date, end_date): + results.append(rates[-1]) + + if not results: + if start_date >= rates[-1].valid_from: + results.append(rates[-1]) + + return results + + +def is_between_end_date_exclusive(date, start_date, end_date): + return start_date <= date < end_date def sms_billing_data_per_month_query(rate, service_id, start_date, end_date): diff --git a/tests/app/dao/test_notification_usage_dao.py b/tests/app/dao/test_notification_usage_dao.py index 0c3ddc952..efeb85d51 100644 --- a/tests/app/dao/test_notification_usage_dao.py +++ b/tests/app/dao/test_notification_usage_dao.py @@ -1,6 +1,7 @@ import uuid from datetime import datetime +from app.dao.date_util import get_financial_year from app.dao.notification_usage_dao import (get_rates_for_year, get_yearly_billing_data, get_monthly_billing_data) from app.models import Rate @@ -9,20 +10,64 @@ from tests.app.db import create_notification def test_get_rates_for_year(notify_db, notify_db_session): set_up_rate(notify_db, datetime(2016, 4, 1), 1.50) + set_up_rate(notify_db, datetime(2016, 9, 1), 1.60) set_up_rate(notify_db, datetime(2017, 6, 1), 1.75) - rates = get_rates_for_year(datetime(2016, 4, 1), datetime(2017, 3, 31), 'sms') - assert len(rates) == 1 + rates = get_rates_for_year(datetime(2016, 3, 31), datetime(2017, 3, 31), 'sms') + assert len(rates) == 2 assert datetime.strftime(rates[0].valid_from, '%Y-%m-%d %H:%M:%S') == "2016-04-01 00:00:00" assert rates[0].rate == 1.50 - rates = get_rates_for_year(datetime(2017, 4, 1), datetime(2018, 3, 31), 'sms') - assert len(rates) == 1 + assert datetime.strftime(rates[1].valid_from, '%Y-%m-%d %H:%M:%S') == "2016-09-01 00:00:00" + assert rates[1].rate == 1.6 + start_date, end_date = get_financial_year(2017) + rates_2017 = get_rates_for_year(start_date, end_date, 'sms') + assert len(rates_2017) == 2 + assert datetime.strftime(rates_2017[0].valid_from, '%Y-%m-%d %H:%M:%S') == "2016-09-01 00:00:00" + assert rates_2017[0].rate == 1.60 + assert datetime.strftime(rates_2017[1].valid_from, '%Y-%m-%d %H:%M:%S') == "2017-06-01 00:00:00" + assert rates_2017[1].rate == 1.75 + + +def test_get_rates_for_year_in_the_future(notify_db, notify_db_session): + set_up_rate(notify_db, datetime(2016, 4, 1), 1.50) + set_up_rate(notify_db, datetime(2017, 6, 1), 1.75) + start_date, end_date = get_financial_year(2018) + rates = get_rates_for_year(start_date, end_date, 'sms') assert datetime.strftime(rates[0].valid_from, '%Y-%m-%d %H:%M:%S') == "2017-06-01 00:00:00" assert rates[0].rate == 1.75 +def test_get_rates_for_year_returns_empty_list_if_year_is_before_earliest_rate(notify_db, notify_db_session): + set_up_rate(notify_db, datetime(2016, 4, 1), 1.50) + set_up_rate(notify_db, datetime(2017, 6, 1), 1.75) + start_date, end_date = get_financial_year(2015) + rates = get_rates_for_year(start_date, end_date, 'sms') + assert rates == [] + + +def test_get_rates_for_year_early_rate(notify_db, notify_db_session): + set_up_rate(notify_db, datetime(2015, 6, 1), 1.40) + set_up_rate(notify_db, datetime(2016, 6, 1), 1.50) + set_up_rate(notify_db, datetime(2016, 9, 1), 1.60) + set_up_rate(notify_db, datetime(2017, 6, 1), 1.75) + start_date, end_date = get_financial_year(2016) + rates = get_rates_for_year(start_date, end_date, 'sms') + assert len(rates) == 3 + + +def test_get_rates_for_year_edge_case(notify_db, notify_db_session): + set_up_rate(notify_db, datetime(2016, 3, 31, 23, 00), 1.50) + set_up_rate(notify_db, datetime(2017, 3, 31, 23, 00), 1.75) + start_date, end_date = get_financial_year(2016) + rates = get_rates_for_year(start_date, end_date, 'sms') + assert len(rates) == 1 + assert datetime.strftime(rates[0].valid_from, '%Y-%m-%d %H:%M:%S') == "2016-03-31 23:00:00" + assert rates[0].rate == 1.50 + + def test_get_yearly_billing_data(notify_db, notify_db_session, sample_template, sample_email_template): set_up_rate(notify_db, datetime(2016, 4, 1), 1.40) set_up_rate(notify_db, datetime(2016, 6, 1), 1.58) + set_up_rate(notify_db, datetime(2017, 6, 1), 1.65) # previous year create_notification(template=sample_template, created_at=datetime(2016, 3, 31), sent_at=datetime(2016, 3, 31), status='sending', billable_units=1) @@ -131,6 +176,7 @@ def test_get_monthly_billing_data_with_multiple_rates(notify_db, notify_db_sessi sample_email_template): set_up_rate(notify_db, datetime(2016, 4, 1), 1.40) set_up_rate(notify_db, datetime(2016, 6, 5), 1.75) + set_up_rate(notify_db, datetime(2017, 7, 5), 1.80) # previous year create_notification(template=sample_template, created_at=datetime(2016, 3, 31), sent_at=datetime(2016, 3, 31), status='sending', billable_units=1) From 3e0221adecca80d99ec335d62f5b377527a4e06d Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Tue, 2 May 2017 10:00:47 +0100 Subject: [PATCH 2/3] Change get_financial_year to return ending date as 1 microsecond earlier. That way we can write the queries as between start and end dates, making it easier to read. This makes more sense. --- app/dao/date_util.py | 4 ++-- app/dao/notification_usage_dao.py | 15 +++++++-------- app/dao/notifications_dao.py | 3 +-- app/dao/services_dao.py | 6 ++---- tests/app/dao/test_date_utils.py | 2 +- 5 files changed, 13 insertions(+), 17 deletions(-) diff --git a/app/dao/date_util.py b/app/dao/date_util.py index 92932bb4c..2471865bd 100644 --- a/app/dao/date_util.py +++ b/app/dao/date_util.py @@ -1,10 +1,10 @@ -from datetime import datetime +from datetime import datetime, timedelta import pytz 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) - timedelta(microseconds=1) def get_april_fools(year): diff --git a/app/dao/notification_usage_dao.py b/app/dao/notification_usage_dao.py index 7d29f7315..cb1cd219e 100644 --- a/app/dao/notification_usage_dao.py +++ b/app/dao/notification_usage_dao.py @@ -1,7 +1,7 @@ from datetime import datetime from sqlalchemy import Float, Integer -from sqlalchemy import func, case, cast, desc, between +from sqlalchemy import func, case, cast from sqlalchemy import literal_column from app import db @@ -49,8 +49,7 @@ def get_monthly_billing_data(service_id, year): def billing_data_filter(notification_type, start_date, end_date, service_id): return [ NotificationHistory.notification_type == notification_type, - NotificationHistory.created_at >= start_date, - NotificationHistory.created_at < end_date, + NotificationHistory.created_at.between(start_date, end_date), NotificationHistory.service_id == service_id, NotificationHistory.status.in_(NOTIFICATION_STATUS_TYPES_BILLABLE), NotificationHistory.key_type != KEY_TYPE_TEST @@ -106,11 +105,11 @@ def get_rates_for_year(start_date, end_date, notification_type): rates = Rate.query.filter(Rate.notification_type == notification_type).order_by(Rate.valid_from).all() results = [] for current_rate, current_rate_expiry_date in zip(rates, rates[1:]): - if is_between_end_date_exclusive(current_rate.valid_from, start_date, end_date) or \ - is_between_end_date_exclusive(current_rate_expiry_date.valid_from, start_date, end_date): + if is_between(current_rate.valid_from, start_date, end_date) or \ + is_between(current_rate_expiry_date.valid_from, start_date, end_date): results.append(current_rate) - if is_between_end_date_exclusive(rates[-1].valid_from, start_date, end_date): + if is_between(rates[-1].valid_from, start_date, end_date): results.append(rates[-1]) if not results: @@ -120,8 +119,8 @@ def get_rates_for_year(start_date, end_date, notification_type): return results -def is_between_end_date_exclusive(date, start_date, end_date): - return start_date <= date < end_date +def is_between(date, start_date, end_date): + return start_date <= date <= end_date def sms_billing_data_per_month_query(rate, service_id, start_date, end_date): diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 6bb5337f8..aefc7a1fd 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -250,8 +250,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_date, - NotificationHistory.created_at < end_date + NotificationHistory.created_at.between(start_date, end_date) ).group_by( month ).order_by( diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py index fe25469da..f55b6ec4c 100644 --- a/app/dao/services_dao.py +++ b/app/dao/services_dao.py @@ -237,8 +237,7 @@ def dao_fetch_monthly_historical_stats_by_template_for_service(service_id, year) func.count().label('count') ).filter( NotificationHistory.service_id == service_id, - NotificationHistory.created_at >= start_date, - NotificationHistory.created_at < end_date + NotificationHistory.created_at.between(start_date, end_date) ).group_by( month, @@ -273,8 +272,7 @@ def dao_fetch_monthly_historical_stats_for_service(service_id, year): func.count(NotificationHistory.id).label('count') ).filter( NotificationHistory.service_id == service_id, - NotificationHistory.created_at >= start_date, - NotificationHistory.created_at < end_date + NotificationHistory.created_at.between(start_date, end_date) ).group_by( NotificationHistory.notification_type, NotificationHistory.status, diff --git a/tests/app/dao/test_date_utils.py b/tests/app/dao/test_date_utils.py index d6be85da2..760923c5a 100644 --- a/tests/app/dao/test_date_utils.py +++ b/tests/app/dao/test_date_utils.py @@ -4,7 +4,7 @@ from app.dao.date_util import get_financial_year, get_april_fools def test_get_financial_year(): start, end = get_financial_year(2000) assert str(start) == '2000-03-31 23:00:00' - assert str(end) == '2001-03-31 23:00:00' + assert str(end) == '2001-03-31 22:59:59.999999' def test_get_april_fools(): From c40111a57609080c8013e4473dd1009e0cac357a Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Tue, 2 May 2017 11:20:01 +0100 Subject: [PATCH 3/3] Split up a test --- tests/app/dao/test_notification_usage_dao.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tests/app/dao/test_notification_usage_dao.py b/tests/app/dao/test_notification_usage_dao.py index efeb85d51..738510719 100644 --- a/tests/app/dao/test_notification_usage_dao.py +++ b/tests/app/dao/test_notification_usage_dao.py @@ -18,6 +18,12 @@ def test_get_rates_for_year(notify_db, notify_db_session): assert rates[0].rate == 1.50 assert datetime.strftime(rates[1].valid_from, '%Y-%m-%d %H:%M:%S') == "2016-09-01 00:00:00" assert rates[1].rate == 1.6 + + +def test_get_rates_for_year_returns_correct_rates(notify_db, notify_db_session): + set_up_rate(notify_db, datetime(2016, 4, 1), 1.50) + set_up_rate(notify_db, datetime(2016, 9, 1), 1.60) + set_up_rate(notify_db, datetime(2017, 6, 1), 1.75) start_date, end_date = get_financial_year(2017) rates_2017 = get_rates_for_year(start_date, end_date, 'sms') assert len(rates_2017) == 2