From ee4da698feb27c774f653611bf83be7eb7cec0be Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Thu, 21 Apr 2022 16:56:28 +0100 Subject: [PATCH] Standardise timezones for service usage APIs We want to query for service usage in the BST financial year: 2022-04-01T00:00:00+01:00 to 2023-03-31T23:59:59+01:00 => 2022-04-01 to 2023-03-31 # bst_date Previously we were only doing this explicitly for the monthly API and it seemed like the yearly usage API was incorrectly querying: 2022-03-31T23:00:00+00:00 to 2023-03-30T23:00:00+00:00 => 2022-03-31 to 2023-03-30 # "bst_date" However, it turns out this isn't a problem for two reasons: 1. We've been lucky that none of our rates have changed since 2017, which is long ago enough that no one would care. 2. There's a quirk somewhere in Sqlalchemy / Postgres that has been compensating for the lack of explicit BST conversion. To help ensure we do this consistently in future I've DRYed-up the BST conversion into a new utility. I could have just hard-coded the dates but it seemed strange to have the knowledge twice. I've also adjusted the tests so they detect if we accidentally use data from a different financial year. (2) is why none of the test assertions actually need changing and users won't be affected. Sqlalchemy / Postgres quirk =========================== The following queries were run on the same data but results differ: FactBilling.query.filter(FactBilling.bst_date >= datetime(2021,3,31,23,0), FactBilling.bst_date <= '2021-04-05').order_by(FactBilling.bst_date).first().bst_date datetime.date(2021, 4, 1) FactBilling.query.filter(FactBilling.bst_date >= '2021-03-31 23:00:00', FactBilling.bst_date <= '2021-04-05').order_by(FactBilling.bst_date).first().bst_date datetime.date(2021, 3, 31) Looking at the actual query for the first item above still suggests the results should be the same, but for the use of "timestamp". SELECT ... FROM ft_billing WHERE ft_billing.service_id = '16b60315-9dab-45d3-a609-e871fbbf5345'::uuid AND ft_billing.bst_date >= '2016-03-31T23:00:00'::timestamp AND ft_billing.bst_date <= '2017-03-31T22:59:59.999999'::timestamp AND ft_billing.notification_type IN ('email', 'letter') GROUP BY ft_billing.rate, ft_billing.notification_type UNION ALL SELECT sum(ft_billing.notifications_sent) AS notifications_sent, sum(ft_billing.billable_units * ft_billing.rate_multiplier) AS billable_units, ft_billing.rate AS ft_billing_rate, ft_billing.notification_type AS ft_billing_notification_type FROM ft_billing WHERE ft_billing.service_id = '16b60315-9dab-45d3-a609-e871fbbf5345'::uuid AND ft_billing.bst_date >= '2016-03-31T23:00:00'::timestamp AND ft_billing.bst_date <= '2017-03-31T22:59:59.999999'::timestamp AND ft_billing.notification_type = 'sms' GROUP BY ft_billing.rate, ft_billing.notification_type) AS anon_1 ORDER BY anon_1.notification_type, anon_1.rate If we try some manual queries with and without '::timestamp' we get: select distinct(bst_date) from ft_billing where bst_date >= '2022-04-20T23:00:00' order by bst_date desc; bst_date ------------ 2022-04-21 2022-04-20 select distinct(bst_date) from ft_billing where bst_date >= '2022-04-20T23:00:00'::timestamp order by bst_date desc; bst_date ------------ 2022-04-21 2022-04-20 It looks like this is happening because all client connections are aware of the local timezone, and naive datetimes are interpreted as being in UTC - not necessarily true, but saves us here! The monthly API datetimes were pre-converted to dates, so none of this was relevant for deciding exactly which date to use. --- app/dao/date_util.py | 11 ++++++- app/dao/fact_billing_dao.py | 44 ++++++++++++---------------- tests/app/dao/test_ft_billing_dao.py | 13 ++++++-- 3 files changed, 40 insertions(+), 28 deletions(-) diff --git a/app/dao/date_util.py b/app/dao/date_util.py index 64c18e4e2..f58c8fa79 100644 --- a/app/dao/date_util.py +++ b/app/dao/date_util.py @@ -1,7 +1,7 @@ from datetime import date, datetime, time, timedelta import pytz -from notifications_utils.timezones import convert_bst_to_utc +from notifications_utils.timezones import convert_bst_to_utc, convert_utc_to_bst def get_months_for_financial_year(year): @@ -22,6 +22,15 @@ def get_financial_year(year): return get_april_fools(year), get_april_fools(year + 1) - timedelta(microseconds=1) +def get_financial_year_dates(year): + year_start_datetime, year_end_datetime = get_financial_year(year) + + return ( + convert_utc_to_bst(year_start_datetime).date(), + convert_utc_to_bst(year_end_datetime).date() + ) + + def get_current_financial_year(): now = datetime.utcnow() current_month = int(now.strftime('%-m')) diff --git a/app/dao/fact_billing_dao.py b/app/dao/fact_billing_dao.py index aba110721..0fd3282dd 100644 --- a/app/dao/fact_billing_dao.py +++ b/app/dao/fact_billing_dao.py @@ -8,7 +8,7 @@ from sqlalchemy.sql.expression import case, literal from app import db from app.dao.date_util import ( - get_financial_year, + get_financial_year_dates, get_financial_year_for_datetime, ) from app.dao.organisation_dao import dao_get_organisation_live_services @@ -197,7 +197,7 @@ def fetch_letter_line_items_for_all_services(start_date, end_date): def fetch_billing_totals_for_year(service_id, year): - year_start_date, year_end_date = get_financial_year(year) + year_start, year_end = get_financial_year_dates(year) """ Billing for email: only record the total number of emails. Billing for letters: The billing units is used to fetch the correct rate for the sheet count of the letter. @@ -211,8 +211,8 @@ def fetch_billing_totals_for_year(service_id, year): FactBilling.notification_type.label('notification_type') ).filter( FactBilling.service_id == service_id, - FactBilling.bst_date >= year_start_date, - FactBilling.bst_date <= year_end_date, + FactBilling.bst_date >= year_start, + FactBilling.bst_date <= year_end, FactBilling.notification_type.in_([EMAIL_TYPE, LETTER_TYPE]) ).group_by( FactBilling.rate, @@ -228,8 +228,8 @@ def fetch_billing_totals_for_year(service_id, year): FactBilling.notification_type ).filter( FactBilling.service_id == service_id, - FactBilling.bst_date >= year_start_date, - FactBilling.bst_date <= year_end_date, + FactBilling.bst_date >= year_start, + FactBilling.bst_date <= year_end, FactBilling.notification_type == SMS_TYPE ).group_by( FactBilling.rate, @@ -245,14 +245,11 @@ def fetch_billing_totals_for_year(service_id, year): def fetch_monthly_billing_for_year(service_id, year): - year_start_datetime, year_end_datetime = get_financial_year(year) - - year_start_date = convert_utc_to_bst(year_start_datetime).date() - year_end_date = convert_utc_to_bst(year_end_datetime).date() - + year_start, year_end = get_financial_year_dates(year) today = convert_utc_to_bst(datetime.utcnow()).date() + # if year end date is less than today, we are calculating for data in the past and have no need for deltas. - if year_end_date >= today: + if year_end >= today: data = fetch_billing_data_for_day(process_day=today, service_id=service_id, check_permissions=True) for d in data: update_fact_billing(data=d, process_day=today) @@ -266,8 +263,8 @@ def fetch_monthly_billing_for_year(service_id, year): FactBilling.postage ).filter( FactBilling.service_id == service_id, - FactBilling.bst_date >= year_start_date, - FactBilling.bst_date <= year_end_date, + FactBilling.bst_date >= year_start, + FactBilling.bst_date <= year_end, FactBilling.notification_type.in_([EMAIL_TYPE, LETTER_TYPE]) ).group_by( 'month', @@ -285,8 +282,8 @@ def fetch_monthly_billing_for_year(service_id, year): FactBilling.postage ).filter( FactBilling.service_id == service_id, - FactBilling.bst_date >= year_start_date, - FactBilling.bst_date <= year_end_date, + FactBilling.bst_date >= year_start, + FactBilling.bst_date <= year_end, FactBilling.notification_type == SMS_TYPE ).group_by( 'month', @@ -649,15 +646,12 @@ def fetch_sms_billing_for_organisation(organisation_id, start_date, end_date): def fetch_usage_year_for_organisation(organisation_id, year): - year_start_datetime, year_end_datetime = get_financial_year(year) - - year_start_date = convert_utc_to_bst(year_start_datetime).date() - year_end_date = convert_utc_to_bst(year_end_datetime).date() - + year_start, year_end = get_financial_year_dates(year) today = convert_utc_to_bst(datetime.utcnow()).date() services = dao_get_organisation_live_services(organisation_id) + # if year end date is less than today, we are calculating for data in the past and have no need for deltas. - if year_end_date >= today: + if year_end >= today: for service in services: data = fetch_billing_data_for_day(process_day=today, service_id=service.id) for d in data: @@ -677,9 +671,9 @@ def fetch_usage_year_for_organisation(organisation_id, year): 'emails_sent': 0, 'active': service.active } - sms_usages = fetch_sms_billing_for_organisation(organisation_id, year_start_date, year_end_date) - letter_usages = fetch_letter_costs_for_organisation(organisation_id, year_start_date, year_end_date) - email_usages = fetch_email_usage_for_organisation(organisation_id, year_start_date, year_end_date) + sms_usages = fetch_sms_billing_for_organisation(organisation_id, year_start, year_end) + letter_usages = fetch_letter_costs_for_organisation(organisation_id, year_start, year_end) + email_usages = fetch_email_usage_for_organisation(organisation_id, year_start, year_end) for usage in sms_usages: service_with_usage[str(usage.service_id)] = { 'service_id': usage.service_id, diff --git a/tests/app/dao/test_ft_billing_dao.py b/tests/app/dao/test_ft_billing_dao.py index 32d9460bb..b87907d7e 100644 --- a/tests/app/dao/test_ft_billing_dao.py +++ b/tests/app/dao/test_ft_billing_dao.py @@ -45,8 +45,16 @@ def set_up_yearly_data(): email_template = create_template(service=service, template_type="email") letter_template = create_template(service=service, template_type="letter") - start_date = date(2016, 3, 31) - end_date = date(2017, 4, 2) + # use different rates for adjacent financial years to make sure the query + # doesn't accidentally bleed over into them + for dt in (date(2016, 3, 31), date(2017, 4, 1)): + create_ft_billing(bst_date=dt, template=sms_template, rate=0.163) + create_ft_billing(bst_date=dt, template=email_template, rate=0) + create_ft_billing(bst_date=dt, template=letter_template, rate=0.34, postage='second') + create_ft_billing(bst_date=dt, template=letter_template, rate=0.31, postage='second') + + start_date = date(2016, 4, 1) + end_date = date(2017, 4, 1) for n in range((end_date - start_date).days): dt = start_date + timedelta(days=n) @@ -55,6 +63,7 @@ def set_up_yearly_data(): create_ft_billing(bst_date=dt, template=email_template, rate=0) create_ft_billing(bst_date=dt, template=letter_template, rate=0.33, postage='second') create_ft_billing(bst_date=dt, template=letter_template, rate=0.30, postage='second') + return service