This test was a duplicate of that for branding
showing. This renames them both to better explain
what they do and makes the later one check for
branding styles.
This also removes the
`mock_get_letter_email_branding` param from the
first test as it wasn't being used.
Service contact details are needed if the upload document permission is
enabled - this used to be a link but services can now choose to use a
link, email address or phone number. The form to add or change service
contact details now gives these options and validates the data according
to the type of contact details provided.
When validating phone numbers we can't use the existing validation
because we want to allow landlines too, so there is a basic check that
the phone number is the right length and doesn't include certain
characters.
Includes moving code that tests an API call to
update the service with the new branding to the
preview step submission.
Also includes a change to the HTTP params sent by
the set-email-branding step. Think it was missed
out of this PR:
https://github.com/alphagov/notifications-admin/pull/1843
...so was never changed to 'branding_style'.
- corrects target page for set_email_branding to
new preview step instead of itself
- removed check for helper method being called in
email page test
- updates expected result for test of global
headers to include changes to `frame-src`
- updates navigation config with brand preview page
There are some teams who send jobs on a daily/weekly basis. They have
team members who only use Notify for this purpose. So they would
probably benefit from basic view, because they don’t need to see the
dashboard.
This commit:
- adds a new item (uploaded files) to the basic view navigation for
teams that have sent at least one job
- makes the job pages visible to basic view users
I think we should do this now, rather than as a later enhancement to
basic view. We only have one chance to announce the feature, so teams
who do send jobs may otherwise discount it as not useful for them and
the opportunity to have them use it is lost.
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
From Karl:
> Templates – this should be consistent with Admin view. Users may
> switch from Basic to Admin view (or vice versa), they will also
> interact with users who have a different view or permissions to them.
> Neither should have to learn new interfaces and language if possible.
> ‘Send a message’ was a nice, active label – but Notify options aren’t
> usually actions. If we’re going to change this we should be consistent
> across both Admin and Basic views.
> For the same reason, I have rejected ‘see’, ‘search’ and ‘view sent
> messages’. It will be interesting to see in user testing whether users
> read ‘sent messages’ as ‘send messages’.
One of the big things we found in user research was that people were
uncertain what the effect of giving someone basic view was.
So in the spirit of ‘show don’t tell’, this commit adds a way for users
to preview basic view. They can go into the preview and click around as
much as they like, just as if they really had the basic view assigned to
them.
Once they have seen enough they can return to the settings page where
they can decide whether or not to switch basic view on for real.
This commit adds radio buttons to the ‘basic view’ page. This will let
users choose whether basic view is on or off for their service.
As before, this page will only be linked to if a service already has
basic view, so this commit does not launch the new feature.
This commit adds:
- a row to the settings page…
- …which links to a page explaining what basic view is
The new row (and link) will only appear for services who already have
the feature switched on. This is because we are not launching the
feature yet, so it shouldn’t be available to just anyone.
Sometimes a user will hit the green button without actually changing
the service name (we saw this in a usability testing session).
Currently this gives an error saying ‘service name already in use’,
which is technically true (it’s in use by the current service) but
practically just annoying.
This commit adds some logic to silently ignore such requests.
When a platform admin user clicks on the button to add the
'upload_document' permission to a service, they should be taken to the
page to add a contact_link if the service does not already have one.
The contact link for the service now gets displayed on the service
settings page if the service has the 'upload_document' permission. There
are no changes to the appearance of the service settings page if a
service does not have the 'upload_document' permission.
Added a page which lets users with the 'manage_service' permission change the
contact link for their service. There are no links to this page yet
since only services using document download will need to set a contact
link.
At the moment branding is an undocumented feature. We get a bunch of
support tickets from teams asking its possible.
This commit:
- lets people know it’s possible, and what the options are
- is the first step towards making this process as self-service as
possible
In some cases we will be able to infer a user’s organisation from there
email address, and Google image search their logo. So the experience for
them is that they press a button and government just sorts it out for
you (also known as "the dream").
In other cases we will have to get back to people asking for a copy of
their logo, or to find out about their service, but this is what we have
to do at the moment anyway.
For both SMS senders and email reply to addresses this commit adds:
- a delete link
- a confirmation loop
It doesn’t let users delete:
- default SMS senders or reply to addresses (they always have to have
one)
- inbound numbers
It assumes that the API will allow updating of an attribute named
`active` on the respective database rows. It could work in a different
way. We can’t do complete deletion though because these will still be
keyed to notifications.
we reckon users will like to see gov reply-to email addresses because
it will improve their confidence in the email.
however, some services, for a few complex reasons, don't want a gov
reply to address. rather than add their specific domains to the
whitelist for signups etc, just make reply tos allowed from any domain.
We vet reply-tos before services go live anyway.
we were seeing isort produce different outputs locally and in docker -
this was due to it having different opinions about whether the tests
module (ie all our unit tests) is a first party (local) or third party
(pip installed) import. It's a first party import, so by defining this
in the setup.cfg isort settings, we can force it to be consistent
between environments.
Note: I don't know why it was different in the first place though
Most of the time spent by the admin app to generate a page is spent
waiting for the API. This is slow for three reasons:
1. Talking to the API means going out to the internet, then through
nginx, the Flask app, SQLAlchemy, down to the database, and then
serialising the result to JSON and making it into a HTTP response
2. Each call to the API is synchronous, therefore if a page needs 3 API
calls to render then the second API call won’t be made until the
first has finished, and the third won’t start until the second has
finished
3. Every request for a service page in the admin app makes a minimum
of two requests to the API (`GET /service/…` and `GET /user/…`)
Hitting the database will always be the slowest part of an app like
Notify. But this slowness is exacerbated by 2. and 3. Conversely every
speedup made to 1. is multiplied by 2. and 3.
So this pull request aims to make 1. a _lot_ faster by taking nginx,
Flask, SQLAlchemy and the database out of the equation. It replaces them
with Redis, which as an in-memory key/value store is a lot faster than
Postgres. There is still the overhead of going across the network to
talk to Redis, but the net improvement is vast.
This commit only caches the `GET /service` response, but is written in
such a way that we can easily expand to caching other responses down the
line.
The tradeoff here is that our code is more complex, and we risk
introducing edge cases where a cache becomes stale. The mitigations
against this are:
- invalidating all caches after 24h so a stale cache doesn’t remain
around indefinitely
- being careful when we add new stuff to the service response
---
Some indicative numbers, based on:
- `GET http://localhost:6012/services/<service_id>/template/<template_id>`
- with the admin app running locally
- talking to Redis running locally
- also talking to the API running locally, itself talking to a local
Postgres instance
- times measured with Chrome web inspector, average of 10 requests
╲ | No cache | Cache service | Cache service and user | Cache service, user and template
-- | -- | -- | -- | --
**Request time** | 136ms | 97ms | 73ms | 37ms
**Improvement** | 0% | 41% | 88% | 265%
---
Estimates of how much storage this requires:
- Services: 1,942 on production × 2kb = 4Mb
- Users: 4,534 on production × 2kb = 9Mb
- Templates: 7,079 on production × 4kb = 28Mb
Adding a ‘testing’ template it not enough. It needs to have some real
looking content, so that we can:
- work out what a service is doing
- assess whether that’s a reasonable (ie meeting the terms of use) thing
to be doing with Notify
At the moment we’re having to go back to services quite a lot when they
request to go live and ask them for this stuff.
`<h1>`s should be unique across the site. This page’s `<h1>` matches
that of the previous page (the one with the checklist).
This commit re-titles it to:
- be unique
- more accurately describe the content of the page
This question was designed to make people feel like it was OK to submit
their request without getting the MoU signed. We reckoned that this was
the fastest way of getting their service live (because the MoU is the
bit that’s most likely to slow them down).
We now have a better way of telling people:
- if they’ve signed the MoU already
- or to contact us if they haven’t (which is what the majority of teams
seem to do now)
We were never actually using the answer to this question – we were still
checking for every service whether they had it signed.
So this commit removes this now-redundant question.
We were counting users who had the `manage_settings` permission. This
is the old name for it, therefore there would never be any users with
this permission, so the tick would never go green.
The new name for the permission is `manage_service`. This commit fixes
the error, and adds an extra safeguard against something like this
happening again.
This makes it easier to write a good message in the request to go live
submission. And encapsulating it in the `GovernmentDomain` class keeps
the view nice and clean.
When we process a go live request it’s a pain to have to go fishing in
the Google Drive to see if there’s a signed agreement in there. This
should make it easier when we know there’s already an agreement.
