We don’t really want you modifying lines of the address after you’ve
entered it. Especially when it might not be obvious that modifying the
address line placeholder will modify the address you’re sending the
letter to.
Optional address placeholders aren’t a thing for one-off letters any
more, so we can tidy up the code a bit by removing the parts of the flow
that are accounting for them.
If you have an placeholder from the address block elsewhere in your
letter then you currently get redirected to the address block page
instead of being offered to fill that placeholder in. This commit
tightens up the check to only do this when the placeholder is in the
first 7 placeholders, which is where we store the address placeholders.
`dir(object)` is a useful Python function that tells you what attributes
and methods an object has. It’s also used by tools like iPython and IDEs
for code completion.
Some of the attributes of a `JSONModel` are dynamic, based on what
fields we expect in the underlying JSON. Therefore they don’t
automatically end up in the result of calling `dir`. To get around this
we can implement our own `__dir__` method, which also returns the names
of the fields we’re expecting the the JSON.
Inspired by this Raymond Hettinger tweet:
> #python tip: If you add attributes to an API with __getattr__() or
> __getattribute__(), remember to update __dir__() to make the extension
> introspectable.
— https://twitter.com/raymondh/status/1249860863525146624
By default our AJAX calls were 2 seconds. Then they were 5 seconds
because someone reckoned 2 seconds was putting too much load on the
system. Then we made them 10 seconds while we were having an incident.
Then we made them 20 seconds for the heaviest pages, but back to 5
seconds or 2 seconds for the rest of the pages.
This is not a good situation because:
- it slows all services down equally, no matter how much traffic they
have, or which features they have switched on
- it slows everything down by the same amount, no matter how much load
the platform is under
- the values are set based on our worst performance, until we manually
remember to switch them back
- we spend time during incidents deploying changes to slow down the
dashboard refresh time because it’s a nothing-to-lose change that
might relieve some symptoms, when we could be spending time digging
into the underlying cause
This pull request makes the Javascript smarter about how long it waits
until it makes another AJAX call. It bases the delay on how long the
server takes to respond (as a proxy for how much load the server is
under).
It’s based on the square root of the response time, so is more sensitive
to slow downs early on, and less sensitive to slow downs later on. This
helps us give a more pronounced difference in delay between an AJAX call
that is fast (for example the page for a single notification) and one
that is slow (for example a dashboard for a service with lots of
traffic).
*Some examples of what this would mean for various pages*
Page | Response time | Wait until next AJAX call
---|---|---
Check a reply to address | 130ms | 1,850ms
Brand new service dashboard | 229ms | 2,783ms
HM Passport Office dashboard | 634ms | 5,294ms
NHS Coronavirus Service dashboard | 779ms | 5,977ms
_Example of the kind of slowness we’ve seen during an incident_ | 6,000ms | 18,364ms
GOV.UK email dashboard | `HTTP 504` | 😬
We had a report that when clicking on the 'Download this letter' link on
the notification page the file was not being downloaded as a PDF file
but was given a `.htm` file extension instead. We should be able to stop
that happening by using Flask's `send_file` function with the right mimetype.
This change updates the `view_letter_notification_as_preview` to use
`send_file` and splits out code to get the file data into a separate
function.
Mocks in the tests have been updated and some unused mocks removed.
As part of making the API call we extra the recipient from the first
line of the address. This code was assuming that the recipient would
always have the key `address line 1`, but we’re no longer guaranteeing
that it will be capitalised and spaced exactly like that.
We’re doing this everywhere else now, so this completes the story.
It uses the same regex as elsewhere and the error messaging is
consistent (but not uniform) with the other places.
Since we’re doing normalisation and line-count-checking of addresses in
multiple places it makes sense for that code to be shared. Which is
what happened here:
https://github.com/alphagov/notifications-utils/pull/713
This commit refactors the admin code to make use of the new utils code.
Note about placeholders:
- they now go into the session as `address_line_1` instead of `address
line 1` because this is the format the API uses, so should be
considered canonical
- they are now fetched from the session in a way that isn’t sensitive
to case or underscores (using the `Columns` class)
- the API doesn’t care about case or underscores vs spaces in
placeholder names because it’s checking an instance of `Template` to
see if all the required placeholders are present (see
401c8e41d6/app/notifications/process_notifications.py (L40))
We’re caching the organisation name, but still talking to the API
to see if the organisation exists.
`Service().organisation_id` only goes to the JSON for the service.
`Service().organisation` makes a separate API call.
We only need the former to know if a service belongs to an organisation.
The app is now always getting an organisation by its `id` (or domain),
and never by `service_id`.
This means that the client method and associated mocking can be removed.
I think this must come from a time when the service response didn’t
include `organisation_id`, but now it always does.
A lot of pages in the admin app are now generated entirely from Redis,
without touching the API.
The one remaining API call that a lot of pages make, when the user is
platform admin or a member of an organisation, is to get the name of
the current service’s organisation.
This commit adds some code to start caching that as well, which should
speed up page load times for when we’re clicking around the admin app
(it’s typically 100ms just to get the organisation, and more than that
when the API is under load).
This means changing the service model to get the organisation from the
API by ID, not by service ID. Otherwise it would be very hard to clear
the cache if the name of the organisation ever changed.
We can’t cache the whole organisation because it has a
`count_of_live_services` field which can change at any time, without an
update being made.
It was saying ‘16 hours ago’ instead of today. This is because, in
strftime:
- `%M` means minute, not month
- `%D` means short MM/DD/YY date, not day of the month
The test wasn’t catching this because the freeze time and mocked value
from the API were set to the same minute.
if someone starts a new one-off flow they'll get taken to the address
page. However, if someone hits the back button, they'll cycle backwards
through placeholders and will end up on the individual line pages. Lets
redirect them to the correct place.
We'll additionally need to reconstruct the address block from the
various session variables that may or may not be populated
