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.
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.
So we don’t have to deploy a change on a Saturday.
In the future this could pull from the rates in the database, but while
that code is shifting around I didn’t want to touoch it. We’d also have
to think about caching so as not to have a non-authenticated route
hitting the database.
Note: I've removed the pricing assertion in the "0_free_allowance"
test as it's covered elsewhere - the value of the test is really to
check that we don't show the remainder if there never was any.
The previous, manual calculation could be incorrect depending on
which SMS rates the free allowance was attributed to.
The new field also supersedes the old "letter_total" bolt-on so we
can get cost information consistently for both types.
This adds missing assertions for email and SMS usage, as well as
letters with the help of some additional test data.
Previously we were only checking monthly usage (in other tests).
The "with_letters" was mostly a duplicate of the one before - no
change in test setup - bar the three assertions at the end.
Having the assertions in a separate test will help keep the one
above manageable as we add more assertions for the annual usage.
At the moment, we put the sms rate on the usage page for each
months billing data by taking the single sms rate for the year.
The assumption that there will be a single sms rate for the year is
no longer going to be true. Therefore, instead we take the sms
rate from the monthly data itself which tells us the rate for
a months worth of sent SMS.
