This is part of the overall migration of the "_template_folders"
methods to the TemplateList class. Moving the existing tests now
will make the actual migration easier to follow.
To emulate the second and third tests, we need to grab a specific
folder from the TemplateList and then look at its folders - these
are set based on "get_template_folders" as before.
This will make it easier to understand the diff when we move these
tests to operate via TemplateList. Despite the verbosity, the only
attribute we were actually checking here was the name, as a way of
identifying which folders had been returned.
Looking at the three tests:
1. The first is checking we can correctly filter all folders that a
user can access, which involves appending the names of any parent
folders the user doesn't have direct access to.
2. The second is checking the same thing but also that we filter the
set of folders to just the children of a parent.
3. The third is just checking the filtering of child folders, without
any user filtering or name aggregation applied.
I've adapted tests (2) and (3) to make it clearer what is tested,
focussing the tests on a specific folder and its contents.
This is a very low impact bug since a user can always create such
templates after their service is live and not be subject to checks
we do before that point. Still, we may as well fix it.
The main benefit of this change is actually to contribute towards
moving methods like "get_templates" out of the Service class so
we can simplify and cache their results more effectively.
Note: I wanted to simplify the Service class further as the two
"has_" properties are only used once in the app code. Unfortunately
they are tightly coupled in one of the tests as well [^1].
[^1]: bef0382cca/tests/app/main/views/service_settings/test_service_settings.py (L1961-L1962)
This represents a bug where a user can request to go live without
setting a reply-to email address or SMS sender despite the service
having one or more email or SMS templates, respectively.
We will make these tests pass in the next commit.
This naming was introduced in 2016 without explanation [1]. I find it
confusing because:
- It's reminiscent of "_app", which is a Python convention indicating
the variable is internal, so maybe avoid using it.
- It suggests there's some other "app" fixture I should be using (there
isn't, though).
The Python style guide describes using an underscore suffix to avoid
clashes with inbuilt names [1], which is sort of applicable if we need
to import the "app" module [2]. However, we can also avoid clashes by
choosing a different name, without the strange underscore.
[1]: 3b1d521c10
[2]: 78824f54fd/tests/app/main/views/test_forgot_password.py (L5)
We’re caching the organisation name, but still talking to the API
to see if the organisation exists.
`Service().organisation_id` only goes to the JSON for the service.
`Service().organisation` makes a separate API call.
We only need the former to know if a service belongs to an organisation.
A lot of pages in the admin app are now generated entirely from Redis,
without touching the API.
The one remaining API call that a lot of pages make, when the user is
platform admin or a member of an organisation, is to get the name of
the current service’s organisation.
This commit adds some code to start caching that as well, which should
speed up page load times for when we’re clicking around the admin app
(it’s typically 100ms just to get the organisation, and more than that
when the API is under load).
This means changing the service model to get the organisation from the
API by ID, not by service ID. Otherwise it would be very hard to clear
the cache if the name of the organisation ever changed.
We can’t cache the whole organisation because it has a
`count_of_live_services` field which can change at any time, without an
update being made.
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.
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.
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.
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.
Putting the permission check in the get_user_template_folders allows
us to replace `all_template_folders` usage with the new method without
having to worry about the temporary service permission flag.
