darkhelm/notifications-api
1
0
Fork 0
mirror of https://github.com/GSA/notifications-api.git synced 2026-04-27 04:40:03 -04:00
Code Issues Packages Projects Releases Wiki Activity

Fix calculating remaining free allowance for SMS

Browse Source 
The way it was done before, the remainder was incorrect in the
billing report and in the org usage query - it was the sms remainder
left at the start of the report period, not at the end of that period.

This became apparent when we tried to show sms_remainder on the org
usage report, where start date is always the start of the financial year.
We saw that sms sent by services did not reduce their free allowance
remainder according to the report. As a result of this, we had to
temporarily remove of sms_remainder column from the report, until
we fix the bug - it has been fixed now, yay!

I think the bug has snuck in partially because our fixtures for testing
this part of the code are quite complex, so it was
harder to see that numbers don't add up. I have added comments
to the tests to try and make it a bit clearer why the results are
as they are.

I also added comments to the code, and renamed some variables,
to make it easier to understand, as there are quite a few
moving parts in it - subqueries and the like.
I also renamed the fetch_sms_free_allowance_remainder method to
fetch_sms_free_allowance_remainder_until_date so it is clearer
what it does.
This commit is contained in:
Pea Tyczynska
2021-12-09 17:50:03 +00:00
parent 6435b57cd1
commit a74d1b026f
3 changed files with 68 additions and 34 deletions
Download Patch File Download Diff File Expand all files Collapse all files

63
app/dao/fact_billing_dao.py
View File

@@ -32,9 +32,9 @@ from app.models import (
from app.utils import get_london_midnight_in_utc, get_notification_table_to_use
def fetch_sms_free_allowance_remainder(start_date):
def fetch_sms_free_allowance_remainder_until_date(end_date):
# ASSUMPTION: AnnualBilling has been populated for year.
billing_year = get_financial_year_for_datetime(start_date)
billing_year = get_financial_year_for_datetime(end_date)
start_of_year = date(billing_year, 4, 1)
billable_units = func.coalesce(func.sum(FactBilling.billable_units * FactBilling.rate_multiplier), 0)
@@ -50,7 +50,7 @@ def fetch_sms_free_allowance_remainder(start_date):
FactBilling, and_(
AnnualBilling.service_id == FactBilling.service_id,
FactBilling.bst_date >= start_of_year,
FactBilling.bst_date < start_date,
FactBilling.bst_date < end_date,
FactBilling.notification_type == SMS_TYPE,
)
).filter(
@@ -65,14 +65,23 @@ def fetch_sms_free_allowance_remainder(start_date):
def fetch_sms_billing_for_all_services(start_date, end_date):
# ASSUMPTION: AnnualBilling has been populated for year.
free_allowance_remainder = fetch_sms_free_allowance_remainder(start_date).subquery()
allowance_left_at_start_date_query = fetch_sms_free_allowance_remainder_until_date(start_date).subquery()
sms_billable_units = func.sum(FactBilling.billable_units * FactBilling.rate_multiplier)
sms_remainder = func.coalesce(
free_allowance_remainder.c.sms_remainder,
free_allowance_remainder.c.free_sms_fragment_limit
allowance_left_at_start_date = func.coalesce(
allowance_left_at_start_date_query.c.sms_remainder,
allowance_left_at_start_date_query.c.free_sms_fragment_limit
)
chargeable_sms = func.greatest(sms_billable_units - sms_remainder, 0)
# subtract sms_billable_units units accrued since report's start date to get up-to-date
# allowance remainder
sms_allowance_left = func.greatest(allowance_left_at_start_date - sms_billable_units, 0)
# billable units here are for period between start date and end date only, so to see
# how many are chargeable, we need to see how much free allowance was used up in the
# period up until report's start date and then do a subtraction
chargeable_sms = func.greatest(sms_billable_units - allowance_left_at_start_date, 0)
sms_cost = chargeable_sms * FactBilling.rate
query = db.session.query(
@@ -80,16 +89,16 @@ def fetch_sms_billing_for_all_services(start_date, end_date):
Organisation.id.label('organisation_id'),
Service.name.label("service_name"),
Service.id.label("service_id"),
free_allowance_remainder.c.free_sms_fragment_limit,
allowance_left_at_start_date_query.c.free_sms_fragment_limit,
FactBilling.rate.label('sms_rate'),
sms_remainder.label("sms_remainder"),
sms_allowance_left.label("sms_remainder"),
sms_billable_units.label('sms_billable_units'),
chargeable_sms.label("chargeable_billable_sms"),
sms_cost.label('sms_cost'),
).select_from(
Service
).outerjoin(
free_allowance_remainder, Service.id == free_allowance_remainder.c.service_id
allowance_left_at_start_date_query, Service.id == allowance_left_at_start_date_query.c.service_id
).outerjoin(
Service.organisation
).join(
@@ -103,8 +112,8 @@ def fetch_sms_billing_for_all_services(start_date, end_date):
Organisation.id,
Service.id,
Service.name,
free_allowance_remainder.c.free_sms_fragment_limit,
free_allowance_remainder.c.sms_remainder,
allowance_left_at_start_date_query.c.free_sms_fragment_limit,
allowance_left_at_start_date_query.c.sms_remainder,
FactBilling.rate,
).order_by(
Organisation.name,
@@ -593,22 +602,30 @@ def fetch_email_usage_for_organisation(organisation_id, start_date, end_date):
def fetch_sms_billing_for_organisation(organisation_id, start_date, end_date):
# ASSUMPTION: AnnualBilling has been populated for year.
free_allowance_remainder = fetch_sms_free_allowance_remainder(start_date).subquery()
allowance_left_at_start_date_query = fetch_sms_free_allowance_remainder_until_date(start_date).subquery()
sms_billable_units = func.sum(FactBilling.billable_units * FactBilling.rate_multiplier)
sms_remainder = func.coalesce(
free_allowance_remainder.c.sms_remainder,
free_allowance_remainder.c.free_sms_fragment_limit
allowance_left_at_start_date = func.coalesce(
allowance_left_at_start_date_query.c.sms_remainder,
allowance_left_at_start_date_query.c.free_sms_fragment_limit
)
chargeable_sms = func.greatest(sms_billable_units - sms_remainder, 0)
# subtract sms_billable_units units accrued since report's start date to get up-to-date
# allowance remainder
sms_allowance_left = func.greatest(allowance_left_at_start_date - sms_billable_units, 0)
# billable units here are for period between start date and end date only, so to see
# how many are chargeable, we need to see how much free allowance was used up in the
# period up until report's start date and then do a subtraction
chargeable_sms = func.greatest(sms_billable_units - allowance_left_at_start_date, 0)
sms_cost = chargeable_sms * FactBilling.rate
query = db.session.query(
Service.name.label("service_name"),
Service.id.label("service_id"),
free_allowance_remainder.c.free_sms_fragment_limit,
allowance_left_at_start_date_query.c.free_sms_fragment_limit,
FactBilling.rate.label('sms_rate'),
sms_remainder.label("sms_remainder"),
sms_allowance_left.label("sms_remainder"),
sms_billable_units.label('sms_billable_units'),
chargeable_sms.label("chargeable_billable_sms"),
sms_cost.label('sms_cost'),
@@ -616,7 +633,7 @@ def fetch_sms_billing_for_organisation(organisation_id, start_date, end_date):
).select_from(
Service
).outerjoin(
free_allowance_remainder, Service.id == free_allowance_remainder.c.service_id
allowance_left_at_start_date_query, Service.id == allowance_left_at_start_date_query.c.service_id
).join(
FactBilling, FactBilling.service_id == Service.id,
).filter(
@@ -628,8 +645,8 @@ def fetch_sms_billing_for_organisation(organisation_id, start_date, end_date):
).group_by(
Service.id,
Service.name,
free_allowance_remainder.c.free_sms_fragment_limit,
free_allowance_remainder.c.sms_remainder,
allowance_left_at_start_date_query.c.free_sms_fragment_limit,
allowance_left_at_start_date_query.c.sms_remainder,
FactBilling.rate,
).order_by(
Service.name

37
tests/app/dao/test_ft_billing_dao.py
View File

@@ -14,7 +14,7 @@ from app.dao.fact_billing_dao import (
fetch_letter_line_items_for_all_services,
fetch_monthly_billing_for_year,
fetch_sms_billing_for_all_services,
fetch_sms_free_allowance_remainder,
fetch_sms_free_allowance_remainder_until_date,
fetch_usage_year_for_organisation,
get_rate,
get_rates_for_billing,
@@ -516,7 +516,7 @@ def test_delete_billing_data(notify_db_session):
)
def test_fetch_sms_free_allowance_remainder_with_two_services(notify_db_session):
def test_fetch_sms_free_allowance_remainder_until_date_with_two_services(notify_db_session):
service = create_service(service_name='has free allowance')
template = create_template(service=service)
org = create_organisation(name="Org for {}".format(service.name))
@@ -534,7 +534,7 @@ def test_fetch_sms_free_allowance_remainder_with_two_services(notify_db_session)
create_ft_billing(template=template_2, bst_date=datetime(2016, 4, 22), billable_unit=10, rate=0.11)
create_ft_billing(template=template_2, bst_date=datetime(2016, 5, 20), billable_unit=3, rate=0.11)
results = fetch_sms_free_allowance_remainder(datetime(2016, 5, 1)).all()
results = fetch_sms_free_allowance_remainder_until_date(datetime(2016, 5, 1)).all()
assert len(results) == 2
service_result = [row for row in results if row[0] == service.id]
assert service_result[0] == (service.id, 10, 2, 8)
@@ -552,7 +552,7 @@ def test_fetch_sms_billing_for_all_services_for_first_quarter(notify_db_session)
create_ft_billing(template=template, bst_date=datetime(2019, 4, 20), billable_unit=44, rate=0.11)
results = fetch_sms_billing_for_all_services(datetime(2019, 4, 1), datetime(2019, 5, 30))
assert len(results) == 1
assert results[0] == (org.name, org.id, service.name, service.id, 25000, Decimal('0.11'), 25000, 44, 0,
assert results[0] == (org.name, org.id, service.name, service.id, 25000, Decimal('0.11'), 24956, 44, 0,
Decimal('0'))
@@ -592,10 +592,22 @@ def test_fetch_sms_billing_for_all_services_with_remainder(notify_db_session):
results = fetch_sms_billing_for_all_services(datetime(2019, 5, 1), datetime(2019, 5, 31))
assert len(results) == 3
assert results[0] == (org.name, org.id, service.name, service.id, 10, Decimal('0.11'), 8, 3, 0, Decimal('0'))
# Field names are: organisation_name, organisation_id, service_name, service_id, free_sms_fragment_limit,
# sms_rate, sms_remainder, sms_billable_units, chargeable_billable_units, sms_cost
# sms_remainder is 5, because service_1_sms_and_letter has 5 sms_billing_units. 2 of them for a period before
# the requested report's start date.
assert results[0] == (org.name, org.id, service.name, service.id, 10, Decimal('0.11'), 5, 3, 0, Decimal('0'))
# sms remainder is 0, because this service sent SMS worth 15 billable units, 12 of which were sent
# before requested report's start date
assert results[1] == (org_2.name, org_2.id, service_2.name, service_2.id, 10, Decimal('0.11'), 0, 3, 3,
Decimal('0.33'))
assert results[2] == (org_3.name, org_3.id, service_3.name, service_3.id, 10, Decimal('0.11'), 5, 7, 2,
# sms remainder is 0, because this service sent SMS worth 12 billable units, 5 of which were sent
# before requested report's start date
assert results[2] == (org_3.name, org_3.id, service_3.name, service_3.id, 10, Decimal('0.11'), 0, 7, 2,
Decimal('0.22'))
@@ -604,14 +616,19 @@ def test_fetch_sms_billing_for_all_services_without_an_organisation_appears(noti
results = fetch_sms_billing_for_all_services(datetime(2019, 5, 1), datetime(2019, 5, 31))
assert len(results) == 3
# organisation_name, organisation_id, service_name, service_id, free_sms_fragment_limit,
# Field names are: organisation_name, organisation_id, service_name, service_id, free_sms_fragment_limit,
# sms_rate, sms_remainder, sms_billable_units, chargeable_billable_units, sms_cost
# sms_remainder is 5, because service_1_sms_and_letter has 5 sms_billing_units. 2 of them for a period before
# the requested report's start date.
assert results[0] == (
fixtures["org_1"].name,
fixtures["org_1"].id,
fixtures["service_1_sms_and_letter"].name,
fixtures["service_1_sms_and_letter"].id,
10, Decimal('0.11'), 8, 3, 0, Decimal('0'))
10, Decimal('0.11'), 5, 3, 0, Decimal('0'))
# sms remainder is 0, because this service sent SMS worth 15 billable units, 12 of which were sent
# before requested report's start date
assert results[1] == (
None,
None,
@@ -624,7 +641,7 @@ def test_fetch_sms_billing_for_all_services_without_an_organisation_appears(noti
None,
fixtures["service_with_sms_within_allowance"].name,
fixtures["service_with_sms_within_allowance"].id,
10, Decimal('0.11'), 10, 2, 0, Decimal('0.00')
10, Decimal('0.11'), 8, 2, 0, Decimal('0.00')
)
@@ -713,7 +730,7 @@ def test_fetch_usage_year_for_organisation(notify_db_session):
assert first_row['service_id'] == fixtures["service_1_sms_and_letter"].id
assert first_row['service_name'] == fixtures["service_1_sms_and_letter"].name
assert first_row['free_sms_limit'] == 10
assert first_row['sms_remainder'] == 10
assert first_row['sms_remainder'] == 5 # because there are 5 billable units
assert first_row['chargeable_billable_sms'] == 0
assert first_row['sms_cost'] == 0.0
assert first_row['letter_cost'] == 3.4

2
tests/app/organisation/test_rest.py
View File

@@ -831,7 +831,7 @@ def test_get_organisation_services_usage(admin_request, notify_db_session):
assert service_usage['free_sms_limit'] == 10
assert service_usage['letter_cost'] == 0
assert service_usage['sms_billable_units'] == 19
assert service_usage['sms_remainder'] == 10
assert service_usage['sms_remainder'] == 0
assert service_usage['sms_cost'] == 0.54