When we get a support ticket we put a link to the service at the end.
As part of 8b7f2fbf04 we accidentally made
these URLs relative, rather than absolute. This means they aren’t made
into links by email clients or Zendesk.
This commit fixes the links to include the domain again.
In response to: [^1][^2][^3].
Originally there were three tests:
- One was testing the "get_user_template_folders" method directly.
- The other two were testing "get_template_folders", which calls
the "_user_" method if a user is passed - this is what the tests
were primarily checking, by passing or not passing a User object.
The two tests of "get_template_folders" also implicitly checked
that it filtered folders based on a "parent_folder_id". I wanted
to emulate the tests but it makes more sense to simplify them, as
the methods are now only used by TemplateList internally.
To simplify, we just keep just two tests to check that the overall
set of folders is filtered based on the presence of a User. We do
lose the implicit check of filtering by "parent_folder_id", but:
- We are still checking this implicitly in the sense that the loop
to generate the overall set of folders terminates. If the method
didn't do any filtering, the loop would be infinite.
- We need to acknowledge that these tests were incomplete to begin
with. There is also lots of coverage in higher level tests [^4],
although it's harder to reason exactly what is covered.
[^1]: https://github.com/alphagov/notifications-admin/pull/4258#discussion_r890144076
[^2]: https://github.com/alphagov/notifications-admin/pull/4258#discussion_r890151220
[^3]: https://github.com/alphagov/notifications-admin/pull/4258#discussion_r890147506
[^4]: 1787f9f42f/tests/app/main/views/test_template_folders.py
This gives us the performance gains identified in [^1] for the test
service described in the spike:
- user_template_folders - from 10s to a little above 3s on its own
- get_templates_and_folders - from 10s to below 6s on its own
In combination, these two uses of caching reduce the test page load
time from 10s to a little above 3s. This is slightly higher than in
the spike PR due to all the extra work we're doing to generate the
"move to" list of folders, as described in a previous commit.
The render time is unchanged for services with few folders. We start
to see the benefit of this change at around 200 templates + folders,
with no evidence that any service will experience worse render times,
despite the extra work we're doing from previous commits.
[^1]: https://github.com/alphagov/notifications-admin/pull/4251
This is a straight move with a few minor tweaks:
- Some references to self.X in the code from Service now need to
become self.service.X.
- Some reference to self.service.Y in TemplateList can now become
self.Y for the methods that were migrated.
The remaining _template_ methods in Service are still called from
multiple places and there's no performance gain to motivate moving
them out of the Service class, which is now a more manageable size.
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.
TemplateListServices are used when we want to show the service
as an additional layer of hierarchy when a user copies a template,
potentially across services [^1].
Normally a TemplateFolder is given "folders" and "templates" by
TemplateList [^2]; TemplateListService was doing it the other
way round and getting its own instead.
Using a subclass of TemplateList means we can make the approach
consistent, which will support the caching approach later on, as
well as simplifying how we work with templates and folders.
[^1]: 2e637f801f/app/main/views/templates.py (L356)
[^2]: bef0382cca/app/models/template_list.py (L31-L36)
It looks like, by default, Flask no longer makes full URLs, for example
`https://example.com/path`. Instead it does `/path`. This will still
work fine, and if anything is better because it reduces the number of
bytes of HTML we are sending.
It won’t mean that requests go over `http` instead of `https` without
the protocol because we set the appropriate HSTS header here:
0c57da7781/ansible/roles/paas-proxy/templates/admin.conf.j2 (L11)
This commit changes all our tests to reflect that URLs no longer have
the protocol and domain in them. `_external=True` is Flask’s way of
saying whether a URL should be generated with the domain and protocol
(`True`) or without it (`False`).
Again, I can’t find the changelog or diff where this was introuduced,
but if you’d like to go spelunking then here’s a starting point:
50374e3cfe/src/flask/helpers.py (L192)
This is slightly less efficient than getting the folder dicts from
"get_user_template_folders" directly, since:
- TemplateList returns both templates and folders.
- TemplateList encapsulates the dicts in model classes.
We'll compensate for this later on:
- We'll introduce a new caching approach to make the call fast.
- We'll expose a property to avoid the "if" in the comprehension.
Part of moving "get_template_folders" et al. into TemplateList so we
can cache it more effectively. This is slightly less efficient as
iterating a TemplateList will instantiate an object for each item
in the folder; but the difference is minimal.
Note that:
- The default template_type for TemplateList is "all".
- We need to pass realistic template "JSON" in the test now.
The only impactful change is the major version itself, where I've
fixed the breaking changes due to the upgrade of PyPDF2 [^1] and
checked there are no deprecation warnings when I run the tests.
[^1]: https://github.com/alphagov/notifications-utils/pull/973
