This changeset pulls in all of the notification_utils code directly into the API and removes it as an external dependency. We are doing this to cut down on operational maintenance of the project and will begin removing parts of it no longer needed for the API.
Signed-off-by: Carlo Costino <carlo.costino@gsa.gov>
This deletes a big ol' chunk of code related to letters. It's not everything—there are still a few things that might be tied to sms/email—but it's the the heart of letters function. SMS and email function should be untouched by this.
Areas affected:
- Things obviously about letters
- PDF tasks, used for precompiling letters
- Virus scanning, used for those PDFs
- FTP, used to send letters to the printer
- Postage stuff
The only impactful change is the major version itself, where I've
fixed the breaking changes due to the upgrade of PyPDF2 [^1] and
checked there are no deprecation warnings when I run the tests.
[^1]: https://github.com/alphagov/notifications-utils/pull/973
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.
- sqlalchemy.sql.expression.case must include an else statement.
- clearly define list of columns for inbound_sms_history insert, getting the list from InboundSmsHistory.__table__.c was causing data type errors.
- remove relationships when not needed, the foreign key relationship is established in the creation of the column. This will get rid of the warnings referenced here: http://sqlalche.me/e/14/qzyx.
- update queries now that he user relationship in ServiceUser db model has been removed.
- move the check that a template is archived to the view instead of the dao method. The check was clearing the session before the version history could be done.
Deleting notifications in the night tasks still needs to be
investigated. The raw sql is causing an error.
- sqlalchemy.sql.expression.case must include an else statement.
- clearly define list of columns for inbound_sms_history insert, getting the list from InboundSmsHistory.__table__.c was causing data type errors.
- remove relationships when not needed, the foreign key relationship is established in the creation of the column. This will get rid of the warnings referenced here: http://sqlalche.me/e/14/qzyx.
- update queries now that he user relationship in ServiceUser db model has been removed.
- move the check that a template is archived to the view instead of the dao method. The check was clearing the session before the version history could be done.
Deleting notifications in the night tasks still needs to be
investigated. The raw sql is causing an error.
This is a belt-and-braces check because the admin app already checks
this. But since we do it for SMS already it makes sense to replicate it
for broadcast templates.
We were asking for the latest version of a letter template rather than
the version that the notification was sent with. This mean that if you
previewed a letter and had made edits to the template since it was sent
you would be shown an incorrect preview.
By serialising these straight away we can:
- not go back to the database later, potentially closing the connection
sooner
- potentially cache the serialised data, meaning we don’t touch the
database at all
Content and subject are user-submitted so are effectively unbounded in
size. And we’re serialising them for every template when sending the
list of templates to the admin app.
For the service with the most templates this results in a 1.3Mb blob of
JSON going over the wire, and being cached in Redis.
And then the admin app completely ignores these fields, because it does
show template content until you’ve clicked into a single template.
This commit adds a new query parameter, `detailed`, that the admin app
can set to `False`. When it does only the fields needed to render the
`/templates` page are returned.
This is done with a new parameter so as not to break the V1 API.
Although I looked in Kibana and it doesn’t seem like anyone external is
using this endpoint we’ve come this far without breaking the API so…
We did not have a JSON schema for updating a template. Since we will
remove the postage constraint from the templates table, this adds a JSON
schema for updating a template so that we can use it to check that the
postage is one of the allowed values.
We’ve added some new properties to the templates in utils that we can
use instead of doing weird things like
`WithSubjectTemplate.__str__(another_instance)`
https://github.com/alphagov/notifications-api/pull/2742/files#diff-d9a6761afaff4a491e60b64f6063654fL265
introduced a change in behaviour which causes template preview to 500
for a precompiled letter that is invalid but not because the content is
outside the printable area.
The changes it back to the previous behaviour, where we b64encode and
utf-8 decode for a page that is invalid for reasons that are not content
outside the printable area, for example a non portrait a4 letter.
I didn't end up writing a unit test for this because I really can't
figure out the existing behaviour we expect for this use case. This
method is too big and complex but I'm confident by looking at the change
in the commit mentioned above that this puts behaviour back to how it
was. Manual testing also confirms the fix.
I've also taken this opportunity to improve two test names whilst I was
looking at things.
If the png page is invalid then show the overlay for that page
If the png page is not invalid, even if other pages are invalid, then
don't shown the overlay for that page
These keeps the behaviour were if you get the pdf for a pdf which has
invalid pages then the whole PDF is requested (not just a single page)
and all of the pages include the overlay
Tests have been improved to be more explicit about what we are testing
Also use this metadata to decide whether preview pages need
overlay or not. So far we have always added overlay when validation
has failed. Now we will only show it when validation failed due to
content being outside of printable area.
We want a way of getting the hidden precompiled template from admin,
so this adds an endpoint which gets the template or creates it if it doesn't
exist. The function to get or create the hidden template already existed
but has been moved to the template DAO now that it is used in more
places.
The template preview app now accepts a null value for the `filename`
parameter. If a service doesn't have a letter branding option set,
previously we defaulted to their dvla_organisation (probably HM
Government). Now, we pass through None, so that we generate letters
without any logo or branding.
