It looks like, by default, Flask no longer makes full URLs, for example
`https://example.com/path`. Instead it does `/path`. This will still
work fine, and if anything is better because it reduces the number of
bytes of HTML we are sending.
It won’t mean that requests go over `http` instead of `https` without
the protocol because we set the appropriate HSTS header here:
0c57da7781/ansible/roles/paas-proxy/templates/admin.conf.j2 (L11)
This commit changes all our tests to reflect that URLs no longer have
the protocol and domain in them. `_external=True` is Flask’s way of
saying whether a URL should be generated with the domain and protocol
(`True`) or without it (`False`).
Again, I can’t find the changelog or diff where this was introuduced,
but if you’d like to go spelunking then here’s a starting point:
50374e3cfe/src/flask/helpers.py (L192)
Similar to the bug shown here
https://www.pivotaltracker.com/story/show/181513431, but to fix the case
when previewing a letter send using a CSV upload it wasn't using
template values to calculate the page length.
This strengthens the initial check of what's in the session to make
sure it contains some kind of recipient. Without this, we get:
Traceback (most recent call last):
File "/home/vcap/deps/0/python/lib/python3.9/site-packages/flask/app.py", line 1950, in full_dispatch_request
rv = self.dispatch_request()
File "/home/vcap/deps/0/python/lib/python3.9/site-packages/flask/app.py", line 1936, in dispatch_request
return self.view_functions[rule.endpoint](**req.view_args)
File "/home/vcap/app/app/utils/user.py", line 26, in wrap_func
return func(*args, **kwargs)
File "/home/vcap/app/app/main/views/send.py", line 1041, in send_notification
recipient=session['recipient'] or InsensitiveDict(session['placeholders'])['address line 1'],
File "/home/vcap/deps/0/python/lib/python3.9/site-packages/notifications_utils/insensitive_dict.py", line 41, in __getitem__
return super().__getitem__(self.make_key(key))
KeyError: 'addressline1'
I'm not sure how to reproduce this, but this should at least give
the user a better experience, instead of a 500 page.
Some tests use the `client` fixture but don’t call any of its methods.
The reason for doing this is because the test depends on something in
the request context.
This commit replaces all those instances with `client_request`, which
also sets the request context.
These tests are the last ones that still use the `client` fixture. By
replacing it with `client_request` we will be able to say that no tests
should be using the `client` fixture directly.
We added a new argument to `client_request.get` and
`client_request.post` to specify that it should return a raw `Response`
object rather than an instance of `BeautifulSoup`.
This is useful because sometimes we need to look at stuff like the
response headers.
However it turns out we already have a separate method for this, so
rather than invent something new I think it’s better to stick with the
thing we already have.
We have a `client_request` fixture which does a bunch of useful stuff
like:
- checking the status code of the response
- returning a `BeautifulSoup` object
Lots of our tests still use an older fixture called `logged_in_client`.
This is not as good because:
- it returns a raw `Response` object
- doesn’t do the additional checks
- means our tests contain a lot of repetetive boilerplate like `page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser')`
This commit converts all the tests using `logged_in_client` to:
use `client_request` instead.
We have a `client_request` fixture which does a bunch of useful stuff
like:
- checking the status code of the response
- returning a `BeautifulSoup` object
For most tests of a platform admin view we used `platform_admin_client`
instead. This is not as good because it returns a raw `Response` object
and doesn’t do the additional checks.
This commit converts all the tests using `platform_admin_client` to:
use new `client_request` and log in as `platform_admin_user` before
making any requests.
This is also nice because it makes any test easy to parametrize with
additional users, for example to test differences in behaviour dependant
on being platform admin or not.
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
This commits adds test coverage for ther HTML in several of the forms
which had broken autofocus.
It means that if we make changes to the HTML which triggers autofocus in
the future it should be more obvious that something is depending on the
attributes being added/removed.
In https://github.com/alphagov/notifications-admin/pull/3663/files we
made specific routes for sending the ‘tour’ text message, rather than
sharing the ‘one-off’ routes in `send.py`.
This commit moves the final route in the tour journey into `tour.py` as
well, which is where I expected to find it when I was looking for it
just now.
the api returns UTC timestamps, we should keep them as UTC timestamps
until the very last moment, and only convert them into BST when we know
we want to return to a user (ie: in contact-list.html and other places
like that)
The page_header macro includes an optional back link. Since the
page_header is always used inside `<main>`, where the back link should
not be, this stops setting the back link in the page header and instead
sets it in the new `backLink` block.
This naming was introduced in 2016 without explanation [1]. I find it
confusing because:
- It's reminiscent of "_app", which is a Python convention indicating
the variable is internal, so maybe avoid using it.
- It suggests there's some other "app" fixture I should be using (there
isn't, though).
The Python style guide describes using an underscore suffix to avoid
clashes with inbuilt names [1], which is sort of applicable if we need
to import the "app" module [2]. However, we can also avoid clashes by
choosing a different name, without the strange underscore.
[1]: 3b1d521c10
[2]: 78824f54fd/tests/app/main/views/test_forgot_password.py (L5)
How this happens: a user starts to send a letter job, then in another tab starts a SMS
or email job, the sender_id is set in the session. Then the user goes
back to the letter job tab and creates the job. The sender_id is set in
the metadata of the csv file, and causes an exception when trying to persist
the letter notification.
This PR adds a check to ensure the sender_id is not set for letter jobs.
This will catch a small use case where the user has multiple tabs open
and has started sending an SMS or email job, then tries to send a letter
job.
This makes the preview of the email / SMS to send consistent with
the final screen, which we previously changed to show the "reply
to" text irrespective of whether the user had selected anything.
Because we’re be grouping jobs under their parent contact lists it’s
good to have some information ‘scent’ to help people find their jobs,
ie by clicking into a contact list. It also lets you see which list have
been used more than others, maybe because the update hasn’t been sent
to that group of people yet.
The hint text under uploads always says when they were used. For contact
lists this is a bit more complicated, since they can:
- never have been used
- been used multiple times
This commit makes use of the new fields being returned by the API to say
determine when these messages are relevant. They also let us
differentiate between a contact list that’s never been used, and one
that has been used, but not recently enough to show any jobs against it.
We look for `original_file_name` in the metadata now. Initially we were
still checking the query string too, but now that the change to add the
filename to the metadata has been deployed for a while there shouldn't
be any cases of the filename still being in a query string.
Since the `original_file_name` is not being added to the metadata in
`.check_messages` (it has happened earlier in the process) a few tests
are no longer needed.
When sending from an uploaded CSV `.send_messages` now puts the filename
in the metadata. It previously used the query string to pass the
filename to `.check_messages`, where it can be lost.
The `.send_from_contact_list` function redirected to `.check_messages`
with `original_file_name` in the query string. Contact lists already
have `original_file_name` as part of their metadata, so we can stop
sending it in the query string and use the metadata instead.
We don't need these anymore as all users will use the `one-off/step`
routes.
This has mostly involved tidying up the tests which are still a little
disorganised and not as good as I'd like but it's a step in the right
direction.
More refactoring is still possible to the routes, it may come in a later
PR if I have time.
There is no real reason to have to support both 'one-off' steps and also
'test' steps when sending a one off notification. It's a lot of complex
code, just to now set the one of the placeholders in the session.
We make our code much simpler but no longer using the 'test' routes but
instead adding a new endpoint to set the notification recipient when
sending to yourself before continuing on with the rest of the 'one-off'
flow.
After this is deployed for a day then we can completely remove the
'test' routes and this will help remove a lot of code complexity.
We no longer need the `start_tour` page as this has been replaced with
the new `begin_tour` page.
We also no longer need to handle the `help` argument in the
`send_test_step` or `send_one_off_step` as these no longer are
responsible for the tour and don't need to show the help text.
Worth pointing out, the new tour joins into the send one off flow. When
doing a GET `check_tour_notification`, and submitting the form shown on
this page you are POSTed to `send_notification` with `help=3`. Also for
general sending of one off notifications, the POST to
`send_notification` is done with `help=0` which is a bit of a hack to
make sure that we don't show a back link on the `view_notification` page
for when someone gets there having just sent a one off notification.
This use of `help=0` may be a candidate for a refactor in the future as
it feels like a bit of a hacky way of doing things and is therefore not
as clear to developers what is going on.
Also removes the help argument from the csv routes used here. There is
no reason that we need to ever show help for CSVs and this is leftover
code from when we used to do the tour that way.
If a service doesn’t have permission to send international letters but
someone tries to upload a letter with a valid international address we
just tell them that the last line must be a UK postcode.
This is a bit opaque and:
- suggests that we’re not recognising at all that it’s not a UK letter
- doesn’t explain why it must be a UK postcode
This commit adds a new, error message which tells users why their letter
can’t be sent. And hopefully will give them a better idea of how to
resolve the problem, if they really do need to be able to send
international letters.
At the moment we don’t check whether a one-off letter is international
until the user’s clicked send. It’s more accurate to show that the
letter will be sent internationally as soon as we know the address.
Depends on:
- [ ] https://github.com/alphagov/notifications-utils/pull/786
error pages
The skiplink in those pages is in the error summary
at the top. In this case, it seems redundant
because:
- it jumps to a section of the page just below it
- the section it would jump to is the only error
on the page
Because of this, and because The Digital
Accessibility Centre (DAC) reported that the
skiplinks in these pages may not be needed, we
decided to remove it.
This commit also adds a prefix to the table caption to
indicate that it only contains the errors from the
CSV, not the rest of its contents.
Changes those fields (and sometimes also regular text input fields)
in the following forms:
- LoginForm
- RegisterUserForm
- ChangeEmailForm
- FeedbackOrProblem
- AcceptAgreementForm
- ChangeNameForm (only name field here, but used in the same template
field as ChangeEmailForm here: app/templates/views/user-profile/change.html)
Also includes changes to templates that use this form
and associated tests.
This now adds validation for invalid characters on the
LetterAddressForm for one off letters. It also adds a validation failed
message for uploaded letters, precompiled letters sent through the API,
and CSV rows with errors.
At the moment the page is the same as for text message templates,
except:
- different H1
- no guidance about personalisation, links, etc (until we decide how
these should work)
For now you won’t be able to really create a broadcast template, because
the API doesn’t support it (the API will respond with a 400). But that’s
OK because no real services have the broadcast permission yet.
This required a bit of refactoring of how we check which template types
a service can use, because there were some hard-coded assumptions about
emails and text messages.
Why we did this originally[1]:
> Calculating the number of pages in a letter is quite slow. And the
> send yourself a test pages need to load _fast_. Since filling in
> placeholders is very unlikely to change the number of pages in the
> resultant letter, it’s pretty safe to cache that count, and makes the
> subsequent pages load a lot faster.
However things have changed since then:
- this journey is used for sending real letters, not just test ones
- we’re doing enough letters that even an unlikely discrepancy will (and
does) happen
- we cache the generation of the PDF now[2], so at least it’s not
generating the PDF twice, once for the preview and once for the page
count
- it’s no longer necessary to step through each address placeholder to
populate a one-off letter, so a little bit slower isn’t so bad
1. e7896f283a
2. c9c6271aa0/app/preview.py (L140)
For services with permission, they can now put international addresses
into their spreadsheets without getting a postcode error.
This also means they can start using address line 7 instead of postcode,
since it doesn’t make sense to put a country in a field called
‘postcode’. But this will be undocumented to start with, because we’re
not giving any real users the permission.
It does now mean that the number of possible placeholders (7 + postcode)
is greater than the number of allowed placeholders (7), so we have to
account for that in the one-off address flow where we’re populating the
placeholders automatically. We’re sticking with 6 + postcode here for
backwards compatibility.
