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.
- update check_sms_content_char_count to use the SMSTemplate.is_message_too_long function, and updated the error message to align with the message returned by the admin app.
- Update the the code used by version 1 of the api to use the validate_template method.
- I did find a couple of services still using the old api, however, this change should not affect them as I checked the messages being sent and they are not too long.
- We will be sending a message to them to see if they can upgrade.
- Update the log message for authenication to include the URL - makes it easier to track if a service is using version 1 of the api.
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
If you have some returned letters, but none in the last 7 days, we don’t
count how many you have in the last 7 days. But we should test to make
sure we’re not going to the database again.
Currently the dashboard in the admin app pull the entire returned letter
summary for a service to calculate how many letters have been returned
in the last seven days.
Adding a separate endpoint for this purpose is better because:
- it’s a more efficient query
- it’s less data to send down the pipe
- it gives us a place to return the complete datetime, so the dashboard
can be more precise about when the most recent report was
Soon enough every service will have this permission, and they won’t be
able to switch it off. So we should clean up our codebase and make it
so there’s no dependancy on a row existing in the permissions table.
This is the first step of that process for the API. Before we can remove
it, we have to stop checking from it. Next step will be to stop
inserting the permission, then finally remove it from the database.
If your caseworking system always spits out files with the same name it
will be hard to differentiate them when looking at the uploads page.
Seeing who the letter was sent to will help you differentiate them.
We can’t do this until the API returns the recipient.
This is so we can display letter jobs in a different way on the admin
app (because it doesn’t make sense for them to have failed/delivered
counts like it does for email and text message jobs).
As elsewhere we use `fields.Method` to avoid serializing the whole
template object.
This will switch on this feature for new services.
After this we will:
- give existing services this permission with a database migration
- remove this permission from the codebase entirely so that everyone has
this feature and can’t switch it off
This endpoint may need to change, but we'd like to see how this performs, so we'll test this with a real data set. Then come back to make sure the format is correct and check for missing tests for the endpoint,
In previous tests we check that we can deal with files that end in `pdf`
in various forms of upper and lowercase. I've moved the testing of this
behaviour into it's own test so that's explicit and not just implied
that we care about behaviour on the casing of filenames.
Note however that s3 is case sensitive and we upload all our files in
upper case so technically we'd never expect to see a file ending in
`pdf`. I've updated some of our test data to reflect this but didn't
bother doing it everywhere. I've left the test in anyway but it could be
argued that is is redundant as we don't ever expect to see that case
anyway.
Previously, when running the `collate_letter_pdfs_for_day` task, we
would only send letters that were created between 5:30pm yesterday and
5:30 today.
Now we send letters that were created before 5:30pm today and that are
still waiting to be sent. This will help us automatically attempt to
send letters that may have fallen through the gaps and not been sent the
previous day when they should have been.
Previously we solved the problem of letters that had fallen the gap by
having to run the task with a date parameter for example
`collate_letter_pdfs_for_day('2020-02-18'). We no longer need this date
parameter as we will always look back across previous days too for
letters that still need sending.
Note, we have to change from using the pagination `list_objects_v2` to
instead getting each individual notification from s3. We reduce load by
using `HEAD` rather than `GET` but this will still greatly increase the
number of API calls. We acknowledge there will be a small cost to this,
say 50p for 5000 letters and think this is tolerable. Boto3 also handles
retries itself so if when making one of the many HEAD requests, there is
a networking blip then it should be retried automatically for us.
This will allow us to accept two different ones and therefore allow us
to rotate the secret that the admin client is sending to the API
Due to how the notifications-python-client throws exceptions, we run
into exactly the same issue with not being able to distinguish if a
`TokenDecodeError` is thrown because the token was encrypted with a
different secret key or if because there was a different error when
decoding. I've copied the TODO from `requires_auth` as this is exactly
the same issue.
I've also added a test case for functionality that was missing for an
out of date admin token (old IAT).
And support this change across our code. Note, this is a halfway step
where it is not a list rather than a string but still only supports a
single secret, ie one item in the list.
