Flask will pick the first route that matches. Decorators get applied
from innermost to outermost.
So if the same endpoint is served at `/abc` and `/123` the one used
when `url_for` is generating a URL is whichever decorator is lowest
(in terms of line number).
It doesn’t functionally make a difference, but it’s causing the
functional tests to fail at the moment. And shorter URLs are nicer, so
I think it makes sense to change here, rather than change the tests.
`all` is not a real template type, so for links to template folders that
apply to all template types we have a URL that looks like:
```
/services/<uuid:service_id>/templates
```
However Flask only generates this url when `url_for` is called with
`template_type=None`. If called with `template_type=all` then Flask will
generate a URL like
```
/services/<uuid:service_id>/templates/all
```
However attempting to load this URL will now 404, since `all` is not a
template type recognised by the regex introduced in
https://github.com/alphagov/notifications-admin/pull/3176
It would be nice to not have URLs with `all` in them at all, but since
people might have bookmarked them we need to support them indefinitely.
Also considered but decided against adding `all` to the set of template
types because it might cause other problems, for example attempting to
create a new template with a type of `all` would never work.
`all` is not a real template type, so for links to template folders that
apply to all template types we have a URL that looks like:
```
/services/<uuid:service_id>/templates/folders/<uuid:template_folder_id>
```
However Flask only generates this url when `url_for` is called with
`template_type=None`. If called with `template_type=all` then Flask will
generate a URL like
```
/services/<uuid:service_id>/templates/all/folders/<uuid:template_folder_id>
```
However attempting to load this URL will now 404, since `all` is not a
template type recognised by the regex introduced in
https://github.com/alphagov/notifications-admin/pull/3176
It would be nice to not have URLs with `all` in them at all, but since
people might have bookmarked them we need to support them indefinitely.
Also considered but decided against adding `all` to the set of template
types because it might cause other problems, for example attempting to
create a new template with a type of `all` would never work.
The `action_blocked` endpoint needed a variation of the URL without
a `template_id` parameter, because `None` is no longer a valid
`template_id` (because it’s not a UUID).
This change was made in 265931d21746918c4ddfc19c4ad3f8cb5683c1bf, which
also removed the `return_to` parameter, because the back link on the
`action blocked` page only ever goes to `add_new_template` if there’s
no `template_id` provided.
However this was conflating the two things, so I’ve wound it back a bit
so that:
- there’s still a new route, whose URL doesn’t include `template_id`
as a parameter
- `return_to` is always required
I’ve also refactored the code a bit to move the looking up of the back
link from the Jinja into the view layer, so that the related code is in
one place and easier to reason about.
Sometimes we manually check that a URL parameter is in a required set.
Sometimes we don’t bother.
This commit adds a URL converter to do this so that:
- we don’t have to re-write the same code every time
- it’s easier to apply this check to other endpoints
This means endpoints that previously allowed a `template_type` or
`message_type` of `None` now 404. So I’ve had to add new routes for
with URLs that don’t include such parameters.
So this…:
```
/services/128b91b6-2996-4107-bb65-51b7c24a728d/notifications/sms.csv
/services/128b91b6-2996-4107-bb65-51b7c24a728d/notifications/None.csv
```
…becomes:
```
/services/128b91b6-2996-4107-bb65-51b7c24a728d/notifications/sms.csv
/services/128b91b6-2996-4107-bb65-51b7c24a728d/notifications.csv
```
This matches what we do for the HTML-responding equivalent (see
265931d217/app/main/views/jobs.py (L215-L216))
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)
Presume this is from when we used to render the letter preview in a
different way. Removing this saves us an API call every time we’re
loading this page.
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 makes it:
- nicer, by having access to sensibly named things like
`Service.trial_mode` instead of `service['restricted']`.
- less likely to write Jinja code like `service.trail_mode`, which would
fail silently if `service` was a dictionary
For consistency with `.organisations`/`.organisation_ids`.
`.services` returns a list of semi-rich dictionaries for each service.
`.service_ids` returns service IDs only.
This removes the edit_folder_permission checks from the code, enabling
the folder permissions for all services.
This also fixes folder-related tests to set up appropriate user
permissions.
This should only be merged right after alphagov/notifications-api#2428,
when all other permission stories are done.
The Design System has standardised on back links being at the top of the
page, decorated with a small text-coloured arrow.
I think this makes more sense than having them at the bottom, because it
suggests, in some way, being able to go back before commiting to any of
the forms on the page. Whereas the things at the bottom of the page
should be performing actions on what’s in the page.
The reason for making this change now is that it de-clutters the area
around the green buttons. This was presenting a design challenge where
multiple levels of interaction were happening in the same form. Moving
these back links to the top of the page should mean that, in these
complicated forms, there’s one fewer thing to compete for the user’s
attention.
I’ve componentised this into a `page_header` macro so that the change is
easier to roll out and maintain.
Hides action links ('Send', 'Edit', 'Delete' and 'Redact' fro templates
and 'Manage' for template folders) and buttons ('New template', 'New folder')
if the user doesn't have permission to view current folder or template's
parent folder.
Copying a template from another service is one place where we can't
use the `current_service` method since the source template can belong
to a different service the user has access to, so we're using an API
client method.
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.
User model is the most natural place for a permission check method,
however this means that we need to pass the full user object to
service model methods and TemplateList instead of user_id.
This adds a filter by user permissions to the list of template folders
displayed in:
* the 'choose a template page'
* "Move to" form to choose a destination folder
* "Copy an existing template" selection form
To keep the H1/titles of new templates consistent with the sticky menu options.
Updated default letter template name from `Untitled` to `New letter template`
The current user already has a list of service IDs. The current user
- is an API call we have to make anyway to render this page
- is usually cached in Redis
This adds a preview pane which is visible when updating a letter brand.
If JavaScript is enabled, the preview pane shows on the set-letter-branding
page, and submitting the form saves updates the letter brand for a service
immediately. If Javascript is not enabled, there is a separate 'Preview email
branding' page which shows a preview of the brand and has a 'Save' button on it.
The endpoint was removed, but was still linked to in a couple of
places. Some old links were no longer needed, so have been removed.
We do still need a link to `add_template_by_type` on the 'Choose reply'
page - this page is used to allow to let someone pick a template to
reply to inbound SMS with. Since the link only appears if they have no
SMS templates, we now link to `.choose_template` with the templates and
folders form already opened at the option to add a template.
it wouldn't show search if there were under a certain amount of letter
or email branding options - however we know there will always be more
than that amount so lets remove some complexity.
Also, rename the SearchTemplatesForm because it can search anything -
it just prompts you to search by name is all.
