Marshmallow v3 has changed the way that DateTimes get serialized
(https://marshmallow.readthedocs.io/en/stable/upgrading.html#datetime-leaves-timezone-information-untouched-during-serialization).
In order to avoid breaking anything, we want to keep the existing way of
handling DateTimes for now - this could be changed later. We can't just
pass a `format` argument to a DateTime field with the old format, which
looked like this `2017-09-19T00:00:00+00:00`. When we tried that,
Marshmallow then expected data that we are loading to also have that
format, which it doesn't.
This adds a new field, which serializes data in the old format but which
doesn't require data that is being deserialized to have such a precise
format.
Due to a difference in marshmallow 3, when calling
`service_schema.load()` with permissions data the permissions data was
being dropped. We could fix this by allowing the ServiceSchema to
include any unknown keys using but that have unexpected consequences.
Instead, this change adds a method so that the schema knows how to
deserialize permissions. This uses the same code that the `pre_load`
method uses, but there were errors when it wasn't included in both places.
When we nest the `TemplateSchema` as a field on the
`NotificationWithTemplateSchema`, we want to include the
`is_precompiled_letter` field. However, we don't want the
`is_precompiled_letter` in any of the other places that we use the
`TemplateSchema`.
The way we had the code was giving errors in version 3 of Marshmallow
since `is_precompiled_letter` was not defined on the `TemplateSchema`.
Instead of writing complicated logic around when the field should be
included or excluded, this adds a new schema which we can use in the one
place where we do want to include `is_precompiled_letter`.
https://marshmallow.readthedocs.io/en/stable/upgrading.html#schemas-are-always-strict
`.load` doesn't return a `(data, errors)` tuple any more - only data is
returned. A `ValidationError` is raised if validation fails. The code
now relies on the `marshmallow_validation_error` error handler to handle
errors instead of having to raise an `InvalidRequest`. This has no
effect on the response that is returned (a test has been modified to
check).
Also added a new `password` field to the `UserSchema` so that we don't
have to specially check for password errors in the `.create_user` endpoint
- we can let marshmallow handle them.
The tests were previously covering a shared function that's now
only used once, so I've inlined it and merged the tests together
with a common naming that's consistent with the code under test.
This is so we have granular data about billable units and costs
so that we can handle multiple sms rates within one financial
year.
We also cast chargeable_units_used_so_far in that subquery
to integer so we don't have type mismatch.
Co-authored-by: Leo Hemsted <leo.hemsted@digital.cabinet-office.gov.uk>
This is functionally very similar to query_service_sms_usage_for_year,
except this query filters by organisation and returns for all live services
within that organisation.
To ensure that the cumulative free allowance counter resets properly for
each service, we use the `partition_by` flag to group up the window
function[^1]. This magically handles all the free allowances
independently for each service.
[^1]: https://www.postgresql.org/docs/current/tutorial-window.html
Co-authored-by: Leo Hemsted <leo.hemsted@digital.cabinet-office.gov.uk>
This effectively reverts [^1], which was only a temporary change.
I suspect the performance problem will go away with [^2].
While we've clearly been managing without this change, it resulted
in several rows being left as incorrect when letter receipts were
delayed. It makes sense for us to run this task for the same period
as we do to aggregate statuses, as status affects billing.
[^1]: e5c76ffda7
[^2]: https://github.com/alphagov/notifications-api/pull/3542
This follows the same pattern as for status aggregations [^1]. We
haven't seen this problem for a long time because of [^2], but now
we're trying to re-run the aggregation for some incorrect rows it's
becoming apparent we need to fix it.
The following query currently fails in Production after the 30 min
SQLAlchemy timeout:
select template_id, rate_multiplier, international, sum(billable_units), count(*)
from notification_history
where notification_status in ('delivered', 'sending')
and key_type != 'test'
and notification_type = 'sms'
and service_id = '539d63a1-701d-400d-ab11-f3ee2319d4d4'
and created_at >= '2021-07-07 23:00'
and created_at < '2021-07-08 23:00'
group by 1,2,3,4;
Running a quick "explain analyze" with this change applied returns
near immediately, but hangs without it. This is enough evidence for
me that this change will fix the issue.
[^1]: https://github.com/alphagov/notifications-api/pull/3417
[^2]: e5c76ffda7
This can happen in the following scenario (primarily for letters):
1. A service has a mixture of "delivered" and "sending" letters,
which the status task aggregates into two rows:
sending | 123
delivered | 456
2. After the 7 day retention has passed, only the "delivered" letters
will be archived [^1].
3. The status task now looks at the history table [^2], which means
it only sees the "delivered" letters.
4. The "sending" letters are eventually "delivered" and archived (before
the 10 day aggregation cutoff).
5. But the status aggregation task doesn't run.
This commit fixes (5).
[^1]: https://github.com/alphagov/notifications-api/pull/3063
[^2]: f87ebb094d/app/dao/fact_notification_status_dao.py (L51)
We had the `only` defined in the Meta class, and this wasn't working -
any extra fields were also being loaded. This moves it to the point
where the class is instantiated, which now works.
We have a lot of cases in the schemas where we're excluding a field that
doesn't actually exist on the schema anyway. This is often because a
model has been deleted, and the schema has not been updated. These
excluded fields have no effect at the moment, but Marshmallow 3 does
raise an error if you try and exclude non-existent fields.
There should be no change to what gets (de)serialized after this change.
this was added five years ago but never used. if we want to bring back
variable rates per client we might as well get a fresh start since a lot
has changed since then.
This can be calculated from the "free_allowance_used" field and the
"chargeable_units" field, but having it included separately is more
convenient as it can be used directly in Admin [^1].
[^1]: 417e7370bb/app/templates/views/usage.html (L38-L39)
This represents the number of chargeable_units that were actually
free due to the free allowance - they won't be included in "cost".
Although the existing calculations in Admin [^1][^2] will still be
correct with a change in SMS rates - it's cost that's the problem
- it makes sense to have all the knowledge about calculating usage
consistently in these two APIs.
Note that the Integer casting is covered by the API-level tests in
test_rest.
[^1]: 474d7dfda8/app/main/views/dashboard.py (L490)
[^2]: c63660d56d/app/main/views/dashboard.py (L350)
This will replace the manual calculations in Admin [^1][^2] for SMS
and also in API [^3] for annual letter costs.
Doing the calculation here also means we correctly attribute free
allowance to the earliest rows in the billing table - Admin doesn't
know when a given rate was applied so can't do this without making
assumptions about when we change our rates.
Since the calculation now depends on annual billing, we need to
change all the tests to make sure a suitable row exists. I've also
adjusted the test data to match the assumption that there can only
be one SMS rate per bst_date.
Note about "OVER" clause
========================
Using "rows=" ("ROWS BETWEEN") makes more sense than "range=" as
we want the remainder to be incremental within each group in a
"GROUP BY" clause, as well as between groups i.e
# ROWS BETWEEN (arbitrary numbers to illustrate)
date=2021-04-03, units=3, cost=3.29
date=2021-04-03, units=2, cost=4.17
date=2021-04-04, units=2, cost=5.10
vs.
# RANGE BETWEEN
date=2021-04-03, units=3, cost=4.17
date=2021-04-03, units=2, cost=4.17
date=2021-04-04, units=2, cost=5.10
See [^4] for more details and examples.
[^1]: https://github.com/alphagov/notifications-admin/blob/master/app/templates/views/usage.html#L60
[^2]: 072c3b2079/app/billing/billing_schemas.py (L37)
[^3]: 474d7dfda8/app/templates/views/usage.html (L98)
[^4]: https://learnsql.com/blog/difference-between-rows-range-window-functions/
There is no such thing as a "billing unit". The data this field
contained was also a confusing mixture of two types:
- For emails and letters, it was just "notifications_sent".
- For SMS, it was the "chargeable_units" (billable * multiplier).
This replaces the single, ambiguous "billing_units" field with
"chargeable_units" and "notifications_sent" in both usage APIs.
Once Admin is using them we can remove the old field.
This makes it easier to extend each function with costs and free
allowances - especially for SMS.
I've chosen to duplicate the "WHERE" clause in each subquery vs.
the top-level query. This will make more sense in later commits
where we start adding free allowance calculations, which need to
be done on a yearly basis - knowledge the subqueries should have.
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.
you can still use this flag locally but we have it enabled for all
environments and it doesn't need to be toggleable from credentials as it
isn't a secret value.
If we wish to turn redis off for a specific environment we can create a
PR to change the config.
The big breaking change for our code (not mentioned in the changelog) is
that the built-in validator for the `date-time` format now requires the
`rfc3339-validator` package instead of the `strict-rfc3339` package.
This updates the requirements file to use `rfc3339-validator`. Without
this change, wrong `date-time` formats would always silently pass validation.
We were using the Draft4Validator in one place, so this updates it to
the Draft7Validator instead.
The schemas were mostly using draft 4 of the JSON schema, though there
were a couple of schemas that were already of version 7. This updates
them all to version 7, which is the latest version fully supported by
the jsonschema Python package. There are some breaking changes in the
newer version of the schema, but I could not see anywhere would these
affect us. Some of these schemas were not valid in version 4, but are
now valid in version 7 because `"required": []` was not valid in earlier
versions.
