It’s possible to delete default letter contact blocks because there is a
fallback – having a blank letter contact block. This is different to SMS
senders and reply to addresses.
For this to make sense it also means:
- adding the ‘blank’ letter contact block to the list of letter contact
blocks
- having a way of setting the default back to being blank
We used to give users the right version of the agreement by guessing
their organisation from their email address.
Now we do it by looking at the organisation of the service they’re
looking at.
In other words, users should only be downloading the agreement as part
of the go live journey, not outside it. This is because we think that
users will get confused if they download the agreement and:
- find there’s nowhere to physically sign it
- think that accepting the agreement is all they need to do to go live
Maintaining two paths to download the agreement also makes the code more
complicated, and makes it harder to update the content on these pages.
`__getattr__` is called whenever an attribute error is raised.
This means that if something deep inside a property on a model raised
an attribute error, that error would be caught by `__getattr__`, which
would then raise an exception that looked like the property itself
didn’t exist. Very confusing.
The solution seems to be to override `__getattribute__` instead, which
handles _all_ attributes, not just those that aren’t explicitly defined.
We then only intervene if the desired attribute is one of the
`ALLOWED_PROPERTIES`, otherwise falling through to the built in methods
of the underlying `object`.
If we change our mind and decide whether a service should/should not be
counted in the list of live services then we should also drop the cache
which stores the count of how many live services there are.
It looks weird to have two different visual treatments for showing a
navigable hierarchy.
I reckon losing the slash won’t make things less folder like – Windows
for example uses chevrons as foler separators.
An invited user can’t be added to an organisation or service, only a
real user can. So the methods to do this should be on the user model,
and take the details of the invite as arguments.
Organisation team members will be ultimately interested in the detailed
usage of each service, but shouldn't necessarily have access to the
personal data of that services users.
So we should allow these organisation team members to navigate to live
services usage page from the organisation page. They may need to contact
the team so they should also be able to view the team members page.
So they'll then see just usage and team members pages.
If they are actually a team member of the service they're viewing, then
they'll see the full range of options as usual.
This commit implement the above by adding an extra flag to the
`user.has_permissions` decorator which allows certain pages to be marked
as viewable by an organisation user. The default (for all other existing
pages) is that organisation users don’t have permission.
It’s a bit more concise to use these methods, rather than access the
lists directly.
And because it’s easier to read it will make later refactoring less
bothersome.
At the moment, the process for accepting the data sharing and financial
agreement is:
1. download a pdf
* print it out
* get someone to sign it
* scan it
* email it back to us
* we rename the file and save it in Google Drive
* we then update the organisation to say the MOU is signed
* sometimes we also:
* print it out and get it counter-signed
* scan it again
* email it back to the service
Let's not do that any more.
When the first service for an organisation that doesn't have the
agreement in place is in the process of going live, then they should
be able to accept the agreement online as part of the go live flow. This
commit adds the pages that let someone do that.
Where the checklist shows the agreement as **[not completed]** then
they can follow a link where they can download it (as happens now).
From here, they should then also be able to provide some info to accept
it. The info that we need is:
**Version** – because we version the agreements occasionally, we need to
know which version they are accepting. It may not be the latest one if
they downloaded it a while ago and it took time to be signed off
**Who is accepting the agreement** – this will often be someone in the
finance team, and not necessarily a team member, so we should let the
person either accept as themselves, or on behalf of someone else. If
it's on behalf of someone else we need to the name and email address of
that person so we have that on record. Obvs if it's them accepting it
themselves, we have that already (so we just store their user ID and
not their name or email address).
We then replay the collected info back in a sort of legally
binding kind of way pulling in the organisation name too. The wording
we’re using is inspired by what GOV.UK Pay have. Then there’s a big
green button they can click to accept the agreement, which stores their
user ID and and timestamp.
so that platform admins (us) can view pages as regular users do easily.
Simply adds a flag in the session cookie that overrides the actual
platform admin flag on the user model if set. This way it's safe, since
this only downgrades existing functionality, so if someone managed to
alter it they could only get less permissions, not more.
You can change this value from the user profile page if either:
* you're a platform admin
* the flag is set (to any value) on the cookie.
This slightly weird check means that we don't check the underlying
`user._platform_admin` flag anywhere in the code, even when toggling
the suppression.
The view code shouldn’t need to know the internals of a service’s data
structure; the idea of having a service model is to abstract this kind
of thing.
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.
In reality we shouldn’t have any live services that don’t have an
organisation. But we probably do locally, in preview, etc., and we
shouldn’t lose a way of accessing them.
At the moment the service list doesn’t disambiguate between live and
trial mode services. This makes it hard to tell which of the things are
important and which aren’t.
The first step towards making this page clearer is to list trial mode
services separately.
The API needs the id of the user, not the id of the invite.
The problem with the tests is that the update mock returned a different
user ID than the user it was being passed. So the tests didn’t catch
this.
the api always returns exactly:
```
id
name
email_address
auth_type
current_session_id
failed_login_count
logged_in_at
mobile_number
organisations
password_changed_at
permissions
platform_admin
services
state
```
it does this through `models.py::User.serialize` – there is an old
Marshmallow `user_schema` in `schemas.py` but this isn’t used for
dumping return data, only parsing the json in the create user rest
endpoint.
This means we can rely on these keys always being in the dictionary.
Our other models inherit from `JSONModel`, rather than manually doing
lookups of the JSON in the `__init__` method. This commit changes the
user model to work in the same way.
I had to add a new concept (`DEFAULTS`) to account for some properties
not always being present in the (mocked) JSON. In reality it might be
that the API does always return these values. This should be looked at
in future work, to see which defaults can be safely removed. At least
now they:
- do not mean any changes are needed to the existing tests
- are explicitly separated from the attributes that we do expect to
always be in the JSON response
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.
