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
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)
- 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.
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.
We removed whitespace in the HTML of the copy to clipboard component
in https://github.com/alphagov/notifications-admin/pull/4236/files
When the Javascript on the page loads it re-renders the component,
using HTML which is embedded in the .js file.
This means we also need to apply the same change to the .js file
to remove any extraneous whitespace.
If there is whitespace in the element containing the value to be copied
then Firefox[1] includes that space in the value it puts in the clipboard.
This is obviously annoying since `foo-bar` might be a valid API key where
`foo-bar ` is not.
This commit fixes that by using the `-` in Jinja to gobble whitespace.
I also looked at doing this in the Javascript, but the browser API for
selecting some text and copying it doesn’t give an obvious place for
using `String.prototype.trim()`.
1. Tested with Firefox 100.0 on Mac OS 12.2.1
When we changed the layout for each alert on the
current/past/rejected alerts pages to use flexbox,
we added a fallback for older browsers that set
text-align: justify on the container:
https://github.com/alphagov/notifications-admin/pull/4171
This has led to items in the list of areas an
alert will be sent to being laid out as justified
content when they were left-aligned.
These changes set the correct alignment.
At the moment if a user is pending we don’t show the ‘change’ link.
This is unhelpful because:
- there’s no way to remove this user
- there’s no way to change their phone number, if the reason that
they are still pending is because they’ve been unable to receive
the two factor code at the number they first provided
Direct string comparison in multiple places is prone to typos. It
also means that a consumer of the class needs to know that whether
a user is pending or active is held in the `state` property, which
is an implementation detail.
To keep the conditionals in the Jinja template more readable, this
commit moves the logic into a method on the model, where it can
be split over multiple statements and lines.
We fixed a problem with the focus styles of list
items in this pull request:
https://github.com/alphagov/notifications-admin/pull/4141
This missed out pages for areas with counties in
them. This adds the fix to those pages.
These changes also include some extra spacing
between the end of the list and the button which
is lost by the new styles we're giving the list
items.
