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.
This test was calling `.load` on model objects, when it should have
been calling `.dump`. This was not working as expected before the
marshmallow upgrade either - the objects returned were errors and not
template versions.
Boolean fields in marshmallow have various values that get changed to
True or False. The value 'Yes' now gets changed to True, which was
causing a test to start failing. We could change the schemas to stop
'Yes' from being changed to True, but the data for boolean fields comes
from admin, where it is only allowed to have certain values anyway so
this just fixes the test.
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.
we add a row in AnnualBilling table whenever a new service is created,
and our billing code assumes this is done. Yet when we were writing
some of the tests, this was not a thing yet, so now we are updating
those tests so they reflect our system well.
Co-authored-by: Pea Tyczynska <pea.tyczynska@digital.cabinet-office.gov.uk>
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.
* notify_db fixture creates the database connection and ensures the test
db exists and has migrations applied etc. It will run once per session
(test run).
* notify_db_session fixture runs after your test finishes and deletes
all non static (eg type table) data.
In unit tests that hit the database (ie: most of them), 99% of the time
we will need to use notify_db_session to ensure everything is reset. The
only time we don't need to use it is when we're querying things such as
"ensure get X works when database is empty". This is such a low
percentage of tests that it's easier for us to just use
notify_db_session every time, and ensure that all our tests run much
more consistently, at the cost of a small bit of performance when
running tests.
We used to use notify_db to access the session object for manually
adding, committing, etc. To dissuade usage of that fixture I've moved
that to the `notify_db_session`. I've then removed all uses of notify_db
that I could find in the codebase.
As a note, if you're writing a test that uses a `sample_x` fixture, all
of those fixtures rely on notify_db_session so you'll get the teardown
functionality for free. If you're just calling eg `create_x` db.py
functions, then you'll need to make you add notify_db_session fixture to
your test, even if you aren't manually accessing the session.
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.
an API build failed because one of the tests expected the database to be
empty, but it actually wasn't. This is probably because another test run
earlier on that worker did not clear down properly.
I took the opportunity to refresh all of these tests to ensure they all
correctly tear down, and also use the more modern admin_request fixture
ratehr than the old client one that required us to specify headers and
do json parsing etc.
