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 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 syntax makes it clearer what is being tested here, because it’s
unusual to see a decorator being manually called with function as its
first argument.
It’s also consistent with how the later tests in this file are written.
This user is only re-used once, which isn’t a big saving. By putting it
inside the test it’s easy to see what special conditions are being set
up that result in the expected outcometest result.
By making the one platform admin case a separate test we no longer
need to pass in a `user` or `kwargs` to the parametrize every time,
making it easier to read.
It’s easier to see what the different test cases are when they are laid
out with `parametrize`, rather than separate functions with lots of
boilerplate.
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.
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.
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
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.
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.
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
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
This goal wasn't clear from the original test, which was checking
the entire return value, even though this is covered implicitly by
tests of the usage page itself.
This doesn't need to test variable rates for every postage class,
which is more an aspect of grouping. It only needs to check that
some out-of-order usage gets reordered appropriately.
This is covered sufficiently by the main "test_usage" assertions,
which prove the usage is broken down by postage.
I don't think we need to explicitly test the usage is broken down
by month as we already prove this for SMS and we also check the
usage is associated with the correct month in the "ordering" test.
This is now only used for letters and represents the number sent
[^1]. We could use the chargeable_units field, but using "_sent"
is more consistent with the annual attributes [^2].
In fact, chargeable_units isn't actually used anywhere, but I've
kept it in the test data as it is part of the real API and helps
clarify the other values for SMS - free vs. charged.
Note: for SMS I've used an arbitrary "1234" for "chargeable_units"
to indicate it's not used and may be different to the number sent -
for SMS it's related to the number of fragments.
[^1]: bb62d22f25/app/dao/fact_billing_dao.py (L339)
[^2]: 3a1ac189ff/app/main/views/dashboard.py (L339)
This starts using the sms_{cost, charged, free_allowance_used}
fields in the new API to replace the "get_free_paid_breakdown"
function we had before, which could not support multiple rates.
In order to use "get_free_paid_breakdown" the calling method had
to store a "cumulative" variable to calculate the free allowance
used so far, which is now done by the API.
To calculate the data for conftest.py, I had to start from the
bottom ("April") and manually calculate the free allowance used
to emulate the API - this is what "cumulative" used to do.
It has never been possible to get multiple rows for the same month
and rate. This was making it hard to switch to the new API fields,
which will require some manual calculations. I've added the billing
units together in the remaining data so the tests still pass.
I've also moved the "April" row to the end as it was out-of-order
with all the others: it's the _start_ of the financial year.
