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.
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.
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)
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
These are "countryless" phone numbers used by e.g. satellite phone
providers [^1]. We decided not to support them for now because:
- The use case is low: only one service is asking for this prefix.
- MMG will charge at 4x for them but apparently they cost more.
We can't put this "negative" data in the usual place [^2] because
this would _enable_ sending via this number. Until we have more
cases like this it's easiest to just tack on this fake info.
[^1]: https://en.wikipedia.org/wiki/International_Networks_(country_code)
[^2]: ca2506c6a6/notifications_utils/recipients.py (L569)
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 replaces multiple stubs with a single stub on the lower level
API client method to return the desired set of templates. You can
see this most clearly in the diff for the "_sms_sender_" test:
- Add a stub for "get_service_templates"
- Remove stubs for "all_templates" and "get_templates"
In order to make the change, I had to separate the reply-to set of
tests from the "_things_right" tests because "count_of_templates"
was actually in conflict with "count_of_email_templates". To make
the new test I copied the original and removed unnecessary stubs
from both of them depending on what's being tested.
I'm not sure what the "_things_right" test name means; the name of
the new test is at least consistent with others in the file.
Note: I also removed the "assert mock_templates.called is True"
lines as they wasn't adding any value that I can see.
This upgrades itsdangerous by a major version.
When testing most routes we:
* use the `client_request` fixture
* under the hood this logs in the user with `TestClient.login`
* logging in the user signs their session with a secret and the current time
For some tests we also:
* wrap the test method with a `freeze_time()` decorator to simulate a past date and time
When Pytest calls the wrapped test method:
* any application code which tries to get the current time will get the frozen time
* any application code getting the current user means decoding the session
* the code which decodes the session will see that the session was created in the future, in other words it has a negative age
* as of ItsDangerous 2.0.0 signatures with a negative age raise an exception
To avoid all the tests which freeze time failing, this adds itsdangerous
to the list of packages that freezegun ignores.
We can't yet upgrade to a version of itsdangerous that is >= 2.1.0
because there are compatibility issues with Flask 1.x.
- Changed the 'contact us' link to point to our support form, not the
Notify homepage
- Updated the link to the details about CHECK based testing, since the
site we were linking to no longer exists.
We can’t upgrade Jinja or Werkzeug until we’re on Flask 2.x.x. We can’t
upgrade Flask to 1.1.3 because it pins older versions of Jinja and
Werkzeug than the ones we’re using. We can’t upgrade Flask to 2.x.x
until we upgrade itsdangerous to 2.x.x, which is blocked by
https://github.com/alphagov/notifications-admin/pull/4044/files
Similar to the bug shown here
https://www.pivotaltracker.com/story/show/181513431, but to fix the case
when previewing a letter send using a CSV upload it wasn't using
template values to calculate the page length.
`.xlsm` files are like `.xlxs` files but with macros enabled. They store
data in the same XML-based format as `.xlsx` files.
Pyexcel will try to use the xlrd package to parse `.xlsm` files. This
used to work because xlrd used to support reading `.xlsx` files. xlrd
has dropped support for `.xlsx` files in version 2 because of security
concerns. This means that when pyexcel asks xlrd to parse a `.xlsm` file
it causes an error.
This commit adds some branching to force `.xlsm` files to be opened
with pyexcel-xlsx instead, which does support `.xlsx` files.
xlrd is a library for reading data and formatting information from
Excel files in the historical .xls format.
Version 2 of xlrd no longer supports anything other than .xls files.
We were using it to also support reading .xlsm files (old Excel files
with macro support).
We could keep using the old version of this dependency, but hopefully
this niche version of an ancient file format is obscure enough that
no-one is using it, and we can drop support, keeping our dependencies
up to date.
