We strip most whitespace as of:
https://github.com/alphagov/notifications-admin/pull/1701
However we are still getting some bad email addresses through, for
example one that had a leading zero-width space character. This means
that the user sees a validation error; really we should just deal with
the mess for them.
So this commit also includes characters without Unicode character
property "WSpace=Y" (which includes zero-width space) to those which are
stripped from form submissions.
List taken from here: https://en.wikipedia.org/wiki/Whitespace_character
See issue and discussion here: https://bugs.python.org/issue13391
It is standard practice when using GOV.UK template to highlight the
selected navigation item in the propositional navigation (black bar) by
colouring it blue.
This commit adds a new subclass of `Navigation` with the mapping needed
to decide which pages belong to which item in the navigation (or none
at all).
Because we have multiple navigations, which will share the same methods
(by subclassing) but different mappings of navigation items to endpoints
by overriding the `.mapping` and `.exclude` attributes.
In research I’ve sometimes seen people click the wrong nav item. I
reckon that people’s concept of which pages live behind which navigation
items isn’t very strong.
We can reinforce this relationship by showing, for every page, which is
the corresponding nav item. The conventional way of doing this is either
with some kind of emphasis, typically colour or bold. I’ve gone for bold
because colour would be weird.
---
The implementation of this is quite loosely coupled to our application
code because:
- our application code is not well structured (eg we don’t make any use
of blueprints)
- spreading this change across lots of files in our application would
make it harder to test without actually hitting each endpoints; such
tests would be slow and verbose
So I’ve gone for more of a meta approach. Rather than testing that each
endpoint has a specific navigation item selected, I’ve gone for
validating that:
- all endpoints being mapped to are real
- all endpoints have _a_ selected navigation item (or are specifically
excluded)
This means that it’s impossible to add, change or remove an endpoint
without also updating which navigation item should be selected. And the
actual mapping is so declarative that it testing it would be redundant.
Redis is giving us a big performance boost (it’s roughly halved the
median request time on the admin app).
Once we’re confident that it’s working properly[1] we can eke out a bit
more performance from it by keeping the caches alive for longer. As
far as I can tell we’re still using Redis in a very low-volume way[2],
so increasing the number of things we’re storing shouldn’t start taxing
our Redis server at all. But reducing the number of times we have to
hit the API to refresh the cache _should_ result in some performance
increase.
---
1. ie we’re not seeing instances of stale caches not being invalidated
2. We have 2.5G of available space in Redis. Here is our current usage:
```
used_memory:7728960
used_memory_human:7.37M
used_memory_rss:7728960
used_memory_peak:16563776
used_memory_peak_human:15.79M
used_memory_lua:37888
```
This is easier to read than having to understand the arguments 1…n of
the cache decorator are ‘magic’, and gives us more flexibility about
how the cache keys are formatted, eg being able to add words in the
middle of them.
Also changes the key format for all templates to be
`service-{service_id}-templates` instead of `templates-{service_id}`
because then it’s clearer what the ID represents.
A lot of the frequently-used pages in the admin app rely on the API to
get templates.
So this commit adds three new caches:
- a single template version (including a key without a version number,
which is the current version)
- all the templates for a service
- all versions of a template
The first will be the most crucial for performance, but there’s not much
cost to adding the other two.
`@cache.delete('user', 'user_id')` is easier to read and understand than
`@cache.delete('user', key_from_args=[1])`. This will become even more
apparent if we have to start doing stuff like `key_from_args=[1, 5]`,
which is a lot more opaque than just saying
`'service_id', 'template_id'`.
It does make the implementation a bit more complex, but I’m not too
worried about that because:
- the tests are solid
- it’s nicely encapsulated
This `*params` argument seems to be copy/pasted boilerplate. It’s not
used by any consumers of this client, and makes it harder to write a
decorator for this function.
Accepting an invite changes:
- the `user_to_service` list of users returned by `GET /service/<id>`
- the `services` list return by `GET /user/<id>`
The latter change is causing the functional tests to fail.
Changed the Python link on the Documentation page to link to the new
Python docs (docs.notifications.service.gov.uk/python.html) instead of
the Python client GitHub repo.
In the same way, and for the same reasons that we’re caching the service
object.
Here’s a sample of the data returned by the API – so we should make sure
that any changes to this data invalidate the cache.
If we ever change a user’s phone number (for example) directly in the
database, then we will need to invalidate this cache manually.
```python
{
'data':{
'organisations':[
'4c707b81-4c6d-4d33-9376-17f0de6e0405'
],
'logged_in_at':'2018-04-10T11:41:03.781990Z',
'id':'2c45486e-177e-40b8-997d-5f4f81a461ca',
'email_address':'test@example.gov.uk',
'platform_admin':False,
'password_changed_at':'2018-01-01 10:10:10.100000',
'permissions':{
'42a9d4f2-1444-4e22-9133-52d9e406213f':[
'manage_api_keys',
'send_letters',
'manage_users',
'manage_templates',
'view_activity',
'send_texts',
'send_emails',
'manage_settings'
],
'a928eef8-0f25-41ca-b480-0447f29b2c20':[
'manage_users',
'manage_templates',
'manage_settings',
'send_texts',
'send_emails',
'send_letters',
'manage_api_keys',
'view_activity'
],
},
'state':'active',
'mobile_number':'07700900123',
'failed_login_count':0,
'name':'Example',
'services':[
'6078a8c0-52f5-4c4f-b724-d7d1ff2d3884',
'6afe3c1c-7fda-4d8d-aa8d-769c4bdf7803',
],
'current_session_id':'fea2ade1-db0a-4c90-93e7-c64a877ce83e',
'auth_type':'sms_auth'
}
}
```
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
* Juror Central Summoning Bureau - Online – HM Courts & Tribunals Service
* Richmond and Wandsworth Council – Richmond and Wandsworth Council
* ESFA feedback – Department for Education
* PINS Digital Service – Planning Inspectorate
* HMPO Belfast – Home Office
* food.gov.uk – Food Standards Agency
* NCC Mobile Team – Newcastle City Council
* Cambs CC Emergency planning – Cambridgeshire County Council
* HMPO Durham – Home Office
* HMPO London – Home Office
* HMPO Glasgow – Home Office
* HMPO Peterborough – Home Office
* HMPO Southport – Home Office
* Student Loans Company. Password Reset. – Student Loans Company
* DigITS – Crown Commercial Service
* Ofsted Reports Beta – Ofsted
* Revenue Accounts – Ministry of Housing, Communities & Local Government
* Luton Council - Electoral Registration – Luton Council
* Cael eich Pensiwn y Wladwriaeth – Department for Work and Pensions
* Warwick Crown Court – Ministry of Justice
* ProLive - Housing Repairs – Pembrokeshire County Council
* Wirral Council – Wirral Council
* DfT Bus Open Data – Department for Transport
* Paris File Watcher – Pembrokeshire County Council
* WFDC Electoral Services – Wyre Forest District Council
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.
The check page expects template ID to be passed through in the URL not
the session now. The send test letter page wasn’t changed.
This commit changes it, and adds a test to make sure this path is
covered.
The start job endpoint needs the template ID in order to make the API
call.
It doesn’t make sense to add it to the start job URL, because users
could potentially start a job with the wrong template by hacking the URL
(which would blow up at some point, if the template didn’t match the
columns in the file).
