The helper function handles both tests that pass, and tests that are
expected to fail (either by raising an exception or returning a redirect
to the login page).
By moving the handling of cases which are expected to fail out of the
helper function we can make the helper function less complex, which will
make further refactoring easier.
We were blocked on using the very latest version of itsdangerous because
it was only compatible with versions of Flask greater than 2.
Now that we’re using the latest version of Flask we can also keep this
dependency up to date.
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)
The failing test here[1] does two things:
1. makes a request to /sign-out
2. calls the index route, without actually making a request
This means that when the `login_manager.unauthorized_handler`[2] looks
at Flask’s `request` object it gets the request context from 1., not 2.,
because 2. isn’t actually a request. The means it sets the value of the
`next` parameter to that of the request, not of the index route.
Basically at some point Flask has changed and decided that 2. isn’t a
proper request, so won’t set new request context.
This isn’t a realistic test because nothing would call the index
function directly, it would always be as part of a request to that page.
But to make the minimal change to fix the breaking tests this commit
makes the check a bit more general, i.e. that the redirect is to the
sign in page with any `next` parameter, not a specific `next` parameter.
1. 9111a7fc86/tests/app/utils/test_user.py (L130-L138)
2. 9111a7fc86/app/main/views/sign_in.py (L86)
I can’t find the changelog for this but it looks like somewhere someone
has decided that commas don’t need to be URL-encoded. This is true for
use in `href` attributes because it’s unambiguous that the comma is part
of the URL (unlike a closing quote for example, which could be
misinterpreted as HTML syntax).
This commit jut changes the test to reflect that the URLs generated by
Flask now have raw commas in them.
Now that we’ve upgraded itsdangerous to the latest version we are:
- unblocked from upgrading to Flask 2, which requires a recent version
of itsdangerous
- unblocked from upgrading Jinja and Werkzeug to the latest versions,
which require Flask 2
This commit just does the version upgrades, breaking changes will be
addressed in subsequent commits.
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.
