Fixes a bug where we were calling a wrapper method when instead we
should have been calling the redis_client. This had resulted in no
actual calls to redis happening.
'Session expired' or similar makes it sound like a new error.
It could confuse the user and make them think the sign in didn't work
and that their session has expired again.
So we went with:
The change you made was not saved. Please try again.
the upload preview page has a file_id - this corresponds to the file in
the transient pdf uploads bucket. However, if the user already hit send
(and then navigated back) the file's no longer in that bcuket, it's been
moved to the regular letters-pdf bucket. So the s3 get request fails. To
avoid this, simply redirect to the notifications page if the file isn't
in the transient bucket. This is better for the user as it'll stop them
trying to submit it twice, and will provide more clarity on the status
of the notification too.
When the admin app gets user objects from the API, these include a dict
of permissions by service for what the user can do to that services.
Permissions for inactive services are not included in the response as
per:
87cb6f2597/app/dao/permissions_dao.py (L66)
However, this causes a bug where a service is archived but cached user
data still tells us that the user has permissions to view the service.
This should not be the case and causes errors where users can still see
the archived service page, it's settings, and even request to go live
for it, because they are using old cached data for the user.
We solve this by deleting the users who are part of the service from the
cache.
We also delete the templates for this service from the cache as the
templates are also archived when we ask the API to archive the service
as per:
d95c0131e0/app/service/rest.py (L597)
Note, one decision I had to make was whether to delete the user cache
for just active team members or also invited users. Assuming an invited
user can't see the service until they've accepted their invite anyway, it
shouldn't make any difference whether we delete their cache or not.
International letters don’t have a choice of postage. Under the hood
they are either `europe` or `rest-of-world`.
So, for letters that we detect are international, this commit:
- removes the radios buttons that give users the choice of postage
- passes through either `europe` or `rest-of-world` to the API,
depending on what address we find in the letter
This will cause the API to 500 until it can accept `europe` or
`rest-of-world` as postage types, but this is probably OK because it’s
only our services that have international letters switched on at the
moment.
The endpoint works fine with it in the URL now instead, so we need stop
posting it. We can’t stop expecting it yet, because some old instances
will still be posting to the endpoint without the ID in the url.
In the future we need to get the metadata from the file in order to work
out what form validation rules should apply (postage is only required
for UK letters).
To start doing this we need all instances of the app accepting `post`
requests with the `file_id` in the URL, as well as in the form data (for
backwards compatibility).
API gives an error if it tries to add a user to a service and that user is
already a member of the service. This situation shouldn't occur - admin checks
if an invited user is a member of a service before calling API, but we
have seen this error occurring when there are two requests processing at
the same time.
This change catches the errors from API if a user is already a member of
a service and redirects the user to the service dashboard so that they
don't see an error page.
This is for consistency with how we do it for filenames in the previous
commit and moves the decoding into the `LetterMetadata` class for
abstracting this behaviour.
Small refactor of the LetterMetadata class needed to handle None case as
recipient can be None.
S3 can only handle ascii characters, therefore for filename which could
include non ascii characters, for example a filename with the character
'£' in it, we must encode these using urllib before saving it as s3
metadata. We then also make sure that it comes back decoded when
presenting it to the user.
These args are not inputs to the function under test, neither as way of
named arguments or as GET query parameters. I assume this has been
leftover from a previous refactor of behaviour.
S3 metadata only supports ascii characters. Whenever we save data to it
we need to make sure we encode it to save it and then decode it to
display it again to users. This abstraction will act as the place for
that decoding to happen so the rest of the code in our views doesn't
need to care about the encoding abstraction.
Up till now, when adding new organisation domain, if it was already
in use, we didn't handle the 400 we got back from API. This PR
adds handling for that error.
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)
If you’ve come to look at a notification via the uploaded letters page
then the ‘< back’ link should take you back there, not to the usual
activity page.
Some teams have started uploading quite a lot of letters (in the
hundreds per week). They’re also uploading CSVs of emails. This means
the uploads page ends up quite jumbled.
This is because:
- there’s just a lot of items to scan through
- conceptually it’s a bit odd to have batches of things displayed
alongside individual things on the same page
So instead we’re going to start grouping together uploaded letters. This
will be by the date on which we ‘start’ printing them, or in other
words the time at which they can no longer be cancelled.
This feels like a natural grouping, and it matches what we know about
people’s mental models of ‘batches’ and ‘runs’ when talking about
printing.
This grouping will be done in the API, so all this commit need to do is:
- be ready to display this new type of pseudo-job
- link to the page that displays all the uploaded letters for a given
print day
Because we won’t be showing uploaded letters individually on the uploads
page any more we need a way of listing them. This should be by printing
day, to match how we’re grouping them on the uploads page.
This code reuses the notifications.html template, but flips the
precedence of the filename and recipient because I reckon when you’re
looking at uploads you’re thinking filename-first.
From a question on cross-government Slack:
> re the Usage tab - currently it shows 3 financial years - last year,
> this year and next year. is it possible to replace the "next year" tab
> with something more useful? its always going to be blank! I was
> thinking it would be good to have 2 financial years ago, 1 financial
> year ago and this financial year.
This seems like a reasonable idea, and is something we’ve talked about
before. The original intention[1] was that seeing your (unchanged) free
allowance for next year would be useful, but that doesn’t really seem to
be a user need.
***
1. See https://github.com/alphagov/notifications-admin/pull/1094
> so that you can check what your SMS allowance is going to be before
> you actually get into it
The search form makes a post request, so that phone numbers and email
addresses don’t show up in logs or browser history.
At most the API will return 50 results, with some pagination links. We
can’t easily give you links to click in the admin app, because links can
only perform get requests.
Because the value of seeing more than 50 results feels quite low (users
will probably make their search more specific before scrolling through
all 50) let’s just show a message saying only the first 50 results are
displayed.
We were throwing an exception when instantiating a LetterImageTemplate
as we weren't giving it all the arguments it needed.
Now we give it all the correct parameters and add a
test for the method. Ideally we would add a unit test for the flask
route for downloading a letter job CSV (which is currently lacking) but
I did the minimal to be confident I've fixed the bug as I think this
whole code may be fresh for a bit of a rewrite according to Chris.
Original error:
```
File "/Users/davidmcdonald/.virtualenvs/notifications-admin/lib/python3.6/site-packages/notifications_utils/template.py", line 669, in __init__
raise TypeError('image_url is required')
TypeError: image_url is required
```
Some email clients will pre-fetch links in emails to check whether
they’re safe. This has the unfortunate side effect of claiming the token
that’s in the link.
Long term, we don’t want to let the link be used multiple times, because
this reduces how secure it is (eg someone with access to your browser
history could re-use the link even if you’d signed out).
Instead, this commit adds an extra page which is served when the user
clicks the link from the email. This page includes a form which submits
to the actual URL that uses the token, thereby not claiming the token as
soon as the page is loaded.
For convenience, this page also includes some Javascript which clicks
the link on the user’s behalf. If the user has Javascript turned off
they will see the link and can click it themselves. This is going on the
assumption that whatever the email clients are doing when prefetching
the link doesn’t involve running any Javascript.
This Javascript is inlined so that:
- it is run as fast as possible
- it’s more resilient – even if our assets domain is unreachable or the
connection is interrupted, it will still run
We’re going to add an interstitial page that redirects to this new URL.
But we don’t want that redirect to 404 while the change is deploying,
because some boxes will have the new URL and some won’t. So let’s deploy
the new URL to all the boxes first, then the redirect page can safely
take over the new one.
The new URL is going to be `post` not `get` because that feels more
HTTP-y, so we need to make sure that’s part of this change too.
If a service has permission to send international letters then the admin
app should tell template preview, so that template preview knows what
rules to apply when it’s validating the address of the letter.
We don’t need to wait for template preview to start looking at this
query string argument – it will just ignore it for now.
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.
