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.
As per https://www.pivotaltracker.com/story/show/170796514 we want to make the delete template confirmation dialog box more consistent and clear.
The API has been updated with a new endpoint that only returns the last-used date, this date is more accurate since it goes to the ft_notification_status table, if the notification table is empty.
make sure everything is using the `nl2br` formatter that properly wraps
it in markdown to keep everything sanitised nicely. Also write a couple
of tests
Since we’re only showing this page to team who are using the API we
don’t have to worry about explaining what’s going on in terms of the
spreadsheet any more.
This makes the page simpler.
We introduced the ‘breaking change’ page[1] partly to help teach people
about the relationship between the placeholders in their template and
the data they were providing. Data can be provided either by API or by
uploading a spreadsheet. The users who we struggled to communicate this
relationship to were the ones using the upload a spreadsheet feature.
We made two changes to the context of this feature:
1. Around the same time we introduced the interactive tour[2], which
ultimately proved to be the thing that helped people understand the
relationship between the data they were providing and the
placeholders in the template.
2. We introduced a way for people to send one-off messages without
using the API or uploading a spreadsheet[3]. So for this page to say
that you’ll need to update a spreadsheet or change an API call if you
change the placeholders in your template is no longer accurate.
Therefore I think it makes sense to only show this page to teams who are
using the API to send messages. The best proxy we have for that is to
look at whether they’ve created any API keys.
***
1. https://github.com/alphagov/notifications-admin/pull/631
2. https://github.com/alphagov/notifications-admin/pull/613
3. https://github.com/alphagov/notifications-admin/pull/1293
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
