Otherwise we start spamming Sentry with every 404 error log. Even
if the erorr is a 5xx, it depends on how we handle it in the calling
code as to whether we would want to consider it an error.
I didn't spot this in initial testing on Preview because the 404s in
Preview are only triggered due to the functional tests, which only run
when we're deploying something.
Arguably we shouldn't be logging at error level in our Python Client,
since we're also raising an exception [1]. But changing that would be
a can of worms as it's not an internal-only library.
[1]: 74a958de00/notifications_python_client/base.py (L118)
This will capture and send various events to Sentry:
- Any unhandled exceptions.
- Any logger.error calls.
- Some request traces.
The latter are severely limited to avoid going over the free tier
limits for Sentry, and to avoid excess effort on our end.
If an organisation doesn’t have any live services then there’s no data
to download. To make things less confusing we should hide the link in
this case.
This commit also modifies the existing test so that the assertions are
consistent.
While the package can always fetch new holidays via the GOV.UK API,
the latest version of the packages also caches ones for next year,
which means we can avoid unnecessary web requests.
We can't assign `hscni.net` to an organisation because it is used
by GP surgeries, so we don't always know which org it should be
associated with. This change allows people with the an `hscni.net`
email address to sign up and create a service.
If the organisation table contains an entry for `agreement_signed_by_id`
and for `agreement_signed_on_behalf_of_name` then we should the person
who signed the MOU as being the `agreement_signed_on_behalf_of_name`.
This was wrongly showing the `agreement_signed_by_id` as the person who
signed the agreement.
We use the `ChangeEmailForm` if you want to change your own email
address or someone else's email address. This has various validators
which get run. We check if the email address is valid (by using a
function from utils) and if the email address is already in use
(by calling API).
If the email address is not valid, we should not call API to see if it's
already in use because this will cause an exception in API leading to a
`500` in admin. We now only call API if there were no other errors with
the email address.
(The `test_should_redirect_after_name_change` test didn't need the
`mock_email_is_not_already_in_use` fixture, so this has been removed.)
This brings a few performance improvements for RecipientCSV, which
we use to preview and process CSVs. One change also renames one of
the attributes for the class to "guestlist".
We don't currently have any radio fields or check boxes where it's
possible to get more than one validation error. However, since we
never want to show more than one error at a time for a field, this
changes the error messages for the relevant widgets to only show the
first error if there ever were multiple.
If there were multiple errors, this widget was joining the messages
together and displaying all error messages. If a text input field does
have more than one validation error, we only want to show one.
This adds a link to the user profile of the person who requested to go
live for "Request to go live" Zendesk tickets. Viewing a user's profile
page helps us to check for duplicate organisations and services from
that user.
We want organisation team members to be able to see the MOU details for
their organisation. This change creates a new page called billing, which
contains these details. It's only visible to platform admin users now -
the plan is to add more information to this page, then to make it visible
to all organisation users.
The page showing the MOU covers the case of when agreement_signed is
True, when an agreement_signed is False, and when agreement_signed is
None. The case when an agreement_signed is None is very rare - it
signifies that the agreement is not signed but that we have some
service-specific agreements in place. We only have a few organisations
in this state, so it's unlikely that the content for this scenario will
be seen.
When an organisation has signed the agreement we may know the full
details (signing date, version signed, the person who signed it or who it
was signed on behalf of), or we may only have the name of the person who
signed the agreement. We show the more detailed content if possible, and
a less detailed version of the content if not.
There's a new route for downloading the agreement which is almost
identical to the existing `.service_download_agreement` route (plus the
test is almost the same), except that it takes an organisation ID
instead of a service ID.
In
a9617d4df6
we upgraded the version of utils to 49.1 which brought in a renamed
`TTL` as `DEFAULT_TTL`.
However, not only did it change the name, it also changed its type
from an `int` to a `float`:
https://github.com/alphagov/notifications-utils/pull/923/files
We thought that would be OK as in the utils, we moved the conversion
to an integer to happen in the `set` method but it turns out that
caused an issue in the admin app where setting the `has_jobs...`
redis keys will error:
```
Redis error performing set on has_jobs-4bd11cb2-cc17-44e1-b241-8547990db245
...
...
redis.exceptions.ResponseError: value is not an integer or out of range
```
It looks like this is because we are passing a float instead of an
int to `ex`
See a similar post describing the importance of ints rather than
floats for other parameters:
https://developpaper.com/question/redis-err-value-is-not-an-integer-or-out-of-range/
An interesting note is our test
`test_client_creates_job_data_correctly` didn't catch this because
`float(604800) == int(604800`.
I've gone for the quickest solution which is to wrap `DEFAULT_TTL`
in an int. The reason I've done this now is that to do the long
term and more durable fix is to add this fix to utils, however
there are several breaking changes infront of it that would take
me a while to bring in to the admin app first. I've checked the
admin and API apps and this is the only place we are directly
using `DEFAULT_TTL`.
Makes the mock up of an alert we show use an
inline SVG instead of it as a background image.
This means it can use the colour of the heading
text next to it in a way that adapts when high
contrast mode is on.
https://github.com/alphagov/notifications-utils/pull/922
Making the icon an inline SVG lets it inherit
colours from the page styles. This helps in forced
colour modes, like Windows high contrast mode,
where it will match the colour of the text next to
it, whatever it is set to.
Making it inline requires some changes to the CSS
to allow its position to match that of the current
background image.
This also sets `forced-color-adjust` to `auto` on
the `<svg>` element, which tells the browser it
can control its colours in forced colour modes.
This is required because the browsers that support
forced colour mode set it to `none` for the
`<svg>` element by default.
This stat is shown on a few of our pages, such as our homepage,
the performance page and also a platform admin page
and is currently catched for the
default TTL of 1 week. I think there is no reason we can't make
this only cache once an hour and give slightly more up to date stats
which will update more regularly.
This mimics the approach and also the TTL choice of 1 hour that has
been added for the performance page (although there is no
particular bug here to fix, it is just nice to have slightly more up
up to date data).
Note, the API call only takes about 0.3 seconds at the moment
so it is not particularly intensive on the DB to run this more
regularly.
This is a quick additional check to protect the user:
- From getting a CloudFront 502 error if the file takes too
long to upload. I was surprised to find it takes about 1 minute
to upload a 70Mb file to S3.*
- From getting a CloudFront 502 error when we follow the redirect
and run through the slow processing code in utils that builds a
RecipientCSV [1].
For context, a CSV with 100K rows and a few columns is around 5Mb,
so a 10Mb limit should be enough. Analysis over the past week shows
that the vast majority of CSV uploads are actually < 2.5Mb.
I haven't added any tests for this because:
- The check isn't critical, as the worst case scenario is the user
gets a worse error than this in-app one.
- There's no easy way to mock the validation, and I didn't want to
have a test that depends on a 10Mb+ file.
*We're using "key.put" to upload the file, when we could be doing
a multipart upload [2]. However, I tried this myself with a chunk
size of 1000 bytes and found it only led to a marginal improvement.
[1]: https://github.com/alphagov/notifications-utils/pull/930
[2]: https://boto3.amazonaws.com/v1/documentation/api/latest/guide/s3-uploading-files.html
