We mostly rely on the API returning a 404 to generate 404s for trying
to get things with non-UUID IDs. This is fine, except our tests often
mock these API calls. So it could look like everything is working fine,
except the thing your passing in might never be a valid UUID, and thus
would 404 in a non-test environment.
So this commit:
1. uses the `uuid` URL converter everywhere there’s something that looks
like an ID in a URL parameter
2. adds a test which automates checking for 1.
Because it means you often have to cast to string in your application
code just to get your tests passing.
The method being monkey patched is originally defined here: b81aa0f18c/src/werkzeug/routing.py (L1272)
Rather than force us to write the decorators in a specific order let’s
just have one decorator call the other. This should make fewer lines of
code, and fewer annoying test failures. It also means that the same way
of raising a `401` (through the `current_app` method) is used
everywhere.
At the moment we mostly have `user_has_permissions` execute first. It
shouldn’t matter, but it feels right for us to check that a user is
logged in before we check their permissions to a service. Otherwise a
malicious user could (maybe) check if a service ID belongs to a real
service, and go on to do something malicious with that information.
This commit adds some extra test code to enforce that the order is
always the same.
N.B. decorators in Python execute from closest to furthest (from the
line on which the function is defined).
This fixes the bug where if you have three placeholders:
> ((one)) ((two)) ((three))
The first one you are asked to fill in is `((three))` (ie
`template.placeholders[-1]`).
This reintroduces a bug where if you use the ‘Use my phone number’ link
you skip straight to filling in `((two))` and can never proceed because
you’re never given the chance to fill in `((one))`. This commit also
fixes that bug.
The data flow of other bits of our application looks like this:
```
API (returns JSON)
⬇
API client (returns a built in type, usually `dict`)
⬇
Model (returns an instance, eg of type `Service`)
⬇
View (returns HTML)
```
The user API client was architected weirdly, in that it returned a model
directly, like this:
```
API (returns JSON)
⬇
API client (returns a model, of type `User`, `InvitedUser`, etc)
⬇
View (returns HTML)
```
This mixing of different layers of the application is bad because it
makes it hard to write model code that doesn’t have circular
dependencies. As our application gets more complicated we will be
relying more on models to manage this complexity, so we should make it
easy, not hard to write them.
It also means that most of our mocking was of the User model, not just
the underlying JSON. So it would have been easy to introduce subtle bugs
to the user model, because it wasn’t being comprehensively tested. A lot
of the changed lines of code in this commit mean changing the tests to
mock only the JSON, which means that the model layer gets implicitly
tested.
For those reasons this commit changes the user API client to return
JSON, not an instance of `User` or other models.
first way round and then collected placeholders again. Now the flow
collects all placeholders in one round.
Also fix the back link for step-1 for test flow so it goes back
to choosing recipient number
Also move operators around following flake8's advice :)
Doing a lookup with `step_index - 1` means that on step `0` we were
looking up `placeholders[-1]`, ie we were making people fill in the last
placeholder first.
Fixing this reintroduces the bug fixed by this pull request:
https://github.com/alphagov/notifications-admin/pull/2551
So this commit also re-fixes that bug but in a different way.
Instead of using the API client directly views are now calling one
of two Service model methods:
`get_template` is used for view actions, where the user should see
the template page even if they don't have access to the template
folder (since all templates are still inked from the dashboard or
the sent notifications pages).
`get_template_with_user_permission_or_403` will check if the user
has access to the template's folder first and return 403 otherwise.
This method is used for any endpoints that result in an action: editing
template attributes, deleting templates or sending messages.
Upgraded pyexcel-io from 0.5.14 to 0.5.16. This change causes Werkzeug
to be upgraded from 0.14.1 to 0.15.1 which requires some changes:
* ProxyFix now needs to be imported from a different location
* The status code of RequestRedirect has changed from 301 to 308. Since
status code 308 is not currently supported on Internet Explorer with
Windows 7 and 8.1, this subclasses RequestRedirect to keep the status
code of 301.
changelog: https://werkzeug.palletsprojects.com/en/0.15.x/changes/#version-0-15-0
Separated s3_client.py into 3 files - for logos, CSV files and the MOU.
This helps to keep things clearer now that we need to add lots more logo
functions for letters.
We have some teams who haver a series of files they have to send each
day. It’s easy to get muddled up and accidentally send the same file
again, if you think you haven’t already sent it.
This commit blocks you from sending the same combination of template
version and filename more than once on the same day[1].
This won’t affect teams who re-use the same template to give (for
example) updates on an incident for business continuity. These teams
edit the template between each send, thereby updating the version
number of the template.
1. This is based on how the `limit_days` argument to the API works - you
can dig into the code here: 2bd4f74ad0/app/dao/jobs_dao.py (L50)
Because we’re going to use a sticky footer on this page we don’t need
to show the email in its collapsed form. Showing the email is only
needed to:
- fit more things on a page
- make sure a button comes up above the fold (which the sticky footer
will solve)
Currently, a user can select a reply-to email address or text message
sender when uploading a CSV file but this is ignored and the default is
always used instead. As a first step towards changing this, this adds
the sender_id (if selected) to the S3 metadata so that this information
can be used when processing the job.
We didn’t used to allow this because it wasn’t really possible with the
old DVLA set up and we didn’t think there’s a need.
We think it’s possible now because, even though it’s cumbersome, it’s
better than the manual process.
There’s a lot of code in service settings which:
- talks to the API directly through the clients
- passes that information through to the Jinja template
By encapsulating this logic in the service model:
- the Jinja template can access the data directly
- the logic can be reused across multiple methods
We’ve had a support ticket saying:
> Hi, where a letter goes over to two sides, is there a way in the
> 'Preview' screen (or anywhere else) that I can see page two? I can
> see page one OK, but can't work out how to see what's generated on the
> second page.
Whether you’re about to send 1000s of letters – or just want to preview
how one will look – it’s probably useful to be able to see more than
just the first page.
Admin, API and utils were all defining a value for SMS_CHAR_COUNT_LIMIT.
This value has been updated in notifications-utils to allow text
messages to be 4 fragments long and notifications-admin now gets the value of
SMS_CHAR_COUNT_LIMIT from notifications-utils instead of defining it in
config.
If you skip past the templates page (because you don’t have the edit
permission) but then click back you end up in a loop which redirects you
to the page you’re already on.
This commit makes sure that you’re sent back a step further, so you
don’t get stuck in that loop.
The one downside of skipping the template page is that you no longer
get such strong confirmation that you’ve picked the correct template.
You still see the preview of the template, but it’s further down the
page, and the name of the template has disappeared.
This commit adds the name of the template to the page title, to:
- have some continuity from the previous page
- make it easier to double-check you’ve chosen the correct template
‘Upload recipients’ and ‘Send to one recipient’ have always been
slightly clunky phrases.
Now that basic view jumps straight into the ‘Send to one recipient’
flow there’s no way for users to get to the ‘Upload recipients’ flow.
By adding a link to it from the ‘Send to one recipient’ flow it’s
possible for users of basic view to access it.
But we don’t want to introduce too much inconsistency between basic view
and admin view because users will be migrating from one to another. They
might also be talking to their manager, who wouldn’t be able to tell
them where to click if they were looking at two completely different
interfaces.
This also means that we can keep the left-hand navigation in basic view
nice and simple with the two options (‘Templates’ and ‘Sent messages’),
rather than trying to introduce something like ‘Send one message’ and
‘Send lots of messages’ later on.
This is better than just keying into the JSON because it means you get
an exception straight away when looking up a key that doesn’t exist
(which via mocking you could ordinarily miss).
Having the service floating about as JSON is a bit flakey. Could easily
introduce a mistake where you mistype the name of a key and silently
get `None`.
Also means doing awkward things like `if 'permission' in
current_service['permissions']`, whereas for users we can do the
much cleaner `user.has_permission()`.
So this commit:
- introduces a model
- adds a `.has_permission` method similar to the one we have for users
Commit 58cc1604a7 sanitises any non-ascii
characters in the headers. CSV filenames get used as a header value, so
this fixed a bug that occurred when non-ascii characters were used.
The CSV filename also gets used as part of the metadata when uploading
the file to S3. Since the S3 metadata can only contain ASII characters,
we also need to sanitise the filename before uploading it to S3.
This link is useful for people who are setting up templates and want to
test out how they look/how Notify works.
‘Caseworker’ users shouldn’t need to send themselves messages on a
regular basis, so this link is another thing we can take away.
Caseworkers skip the template page in their message sending journey.
Instead they go straight from picking a template to the first step of
sending. So the ‘Back’ link should send them straight back to the
picking a template page, skipping the individual template page (which
they don’t have permission to view).
