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 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.
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).
No tests are now using the `client` fixture directly so we can rename
it.
Python convention is to use an `_underscore` for things which should be
considered semi private.
This should discourage people from writing new tests with these old
fixtures.
New tests should always use `client_request`.
Want to be logged in with a different user? Call
`client_request.login(user)` first.
Don’t want to be logged in? Call `client_request.logout()` first (most
of our tests need to be logged in).
Need an instance of `Response` object not an instance of
`BeautifulSoup`? Use `client_request.get_response` or
`client_request.post_response`.
Need to pass in a URL, not arguments to `url_for`? Use
`client_request.get_url(…)` or `client_request.post_url(…)`.
Need to pass in a URL and get a response back? Use
`client_request.get_response_from_url(…)` or
`client_request.post_response_from_url(…)`.
Some tests use the `client` fixture but don’t call any of its methods.
The reason for doing this is because the test depends on something in
the request context.
This commit replaces all those instances with `client_request`, which
also sets the request context.
These tests are the last ones that still use the `client` fixture. By
replacing it with `client_request` we will be able to say that no tests
should be using the `client` fixture directly.
We have a `client_request` fixture which does a bunch of useful stuff
like:
- checking the status code of the response
- returning a `BeautifulSoup` object
Lots of our tests still use an older fixture called `client`. This is
not as good because it:
- returns a raw `Response` object
- doesn’t do the additional checks
- means our tests contain a lot of repetetive boilerplate like `page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser')`
This commit converts all the tests which had a `client.get(…)` or
`client.post(…)` statement to use their equivalents on `client_request`
instead.
Subsequent commits will remove uses of `client` in other tests, but
doing it this way means the work can be broken up into more manageable
chunks.
We added a new argument to `client_request.get` and
`client_request.post` to specify that it should return a raw `Response`
object rather than an instance of `BeautifulSoup`.
This is useful because sometimes we need to look at stuff like the
response headers.
However it turns out we already have a separate method for this, so
rather than invent something new I think it’s better to stick with the
thing we already have.
We have a `client_request` fixture which does a bunch of useful stuff
like:
- checking the status code of the response
- returning a `BeautifulSoup` object
A few of our tests still use an older fixture called
`logged_in_client_with_session`. It’s not clear how this is different
from `logged_in_client`, which we have replaced with `client_request`.
So this commit goes ahead and converts all the tests using
`logged_in_client_with_session` to use `client_request` instead.
We have a `client_request` fixture which does a bunch of useful stuff
like:
- checking the status code of the response
- returning a `BeautifulSoup` object
Lots of our tests still use an older fixture called `logged_in_client`.
This is not as good because:
- it returns a raw `Response` object
- doesn’t do the additional checks
- means our tests contain a lot of repetetive boilerplate like `page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser')`
This commit converts all the tests using `logged_in_client` to:
use `client_request` instead.
We have a `client_request` fixture which does a bunch of useful stuff
like:
- checking the status code of the response
- returning a `BeautifulSoup` object
For most tests of a platform admin view we used `platform_admin_client`
instead. This is not as good because it returns a raw `Response` object
and doesn’t do the additional checks.
This commit converts all the tests using `platform_admin_client` to:
use new `client_request` and log in as `platform_admin_user` before
making any requests.
This is also nice because it makes any test easy to parametrize with
additional users, for example to test differences in behaviour dependant
on being platform admin or not.
Some users have reported a problem with the received text message
report:
> I have tested the reply service but in the excel report the mobile
> number is showing as 4.47900E+23. How can I change the format so that
> it is show the mobile number that has replied?
This is happening because Excel is interpreting a phone number in the
format `447900900123` as a number in
[scientific notation](https://en.wikipedia.org/wiki/Scientific_notation),
in other words 4.479 × 10<sup>23</sup>.
`447900900123` is the format that our provider is giving us the number
in – there’s no guarantee it will always be in this format.
We can prevent this behaviour by putting spaces in the numbers. Excel
and Google Sheets won’t try to convert a string with spaces into a
number.
I think we used to do this for the sent text messages report but
probably stopped because we decided it was better to keep the phone
number in the same format as it had been supplied to us for
reconcilliation purposes.
This makes it clearer that this model collection isn’t the organisations
for a user or a service or some other entity, like most model
collections are.
It will also lets us make a separate Organisations model, without the
name conflicting.
the api returns UTC timestamps, we should keep them as UTC timestamps
until the very last moment, and only convert them into BST when we know
we want to return to a user (ie: in contact-list.html and other places
like that)
Previously we were passing a flag to the API which handled this. Now
we are doing it at the time of clicking the link, not at the time of
storing the new password. We don’t need to update the timestamp twice,
so this commit removes the code which tells the API to do it.
This wasn't adding anything now that we have two new and more specific
fixtures, `active_user_create_broadcasts_permission` and
`active_user_approve_broadcasts_permission`, that can be used instead.
`manage_templates` has now been removed from the `create_broadcasts`
permission, so this also adjusts the fixture for a user who can create
broadcasts.
The `send_messages` permission has been deprecated for use with
broadcast services, so we can drop support for it in the code. We
were supporting both the old permissions and new permissions
(`create_broadcasts` and `approve_broadcasts`) while we switched people
over.
This removes `send_messages` from the `user_has_permissions` decorator
around the broadcast routes and from the page to view a broadcast and
broadcast dashboards. We can now git rid of a lot of the parameterization
that was temporarily added to the tests.
We've added new broadcast roles in the database (`create_broadcasts` and
`approve_broadcasts`).
Adding these has meant we've needed to do a bit of a rewrite of the roles and
permissions code since this had been based on the assumption that each
database permission only belongs to one admin role - this is no longer true.
This means that flipping the roles dict round to create a dict which
contains database permissions as the keys is no longer possible. We can't
necessarily tell which admin role someone has given a database permission.
To check if a user has an admin role given a list of database permissions,
the user must now have ALL the database permissions mapped to that role
(instead of just one). This works because no one has the `manage_users`
permission without also having the `manage_settings` (and similar for
the other admin roles which map to multiple database permissions).
Some test data was changed because it was using admin roles where
database permissions are actually used when the app is running. I've kept
the functionality of the `translate_permissions_from_db_to_admin_roles`
function passing through any unknown roles it is passed as an argument.
This is not necessary, so can be changed later if we decide it will not
ever be used. However, removing it would require updating a lot of
tests since the tests rely on this behaviour.
Added two new permissions - `create_broadcasts` and
`approve_broadcasts`. These new permissions get added to the
`has_permissions` decorator of the broadcast routes to allow the routes
to be accessed with either the old permissions on the new ones while we
switch over.
We were using the `send_messages` permission for the broadcast routes.
By having two new permissions we can allow a more granular control of
these routes.
If a job exceeds the daily sending limit, show that on the job page. The job is only created if the sending limit has been reached when the delivery app is processing the job, usually this error is caught at the time the CSV is uploaded and the job is not created.
This allows us to roll out the feature to other users. Note that
the flag is also "True" if the user has "webauthn_auth" as their
auth type, so this is compatible with the more fine-grained check
we have on the authentication parts of the feature. We could do a
more explicit "can_use_webauthn or webauthn_auth" check here, but
the idea is that we'll be able to get rid of this flag eventually,
so I've optimised for brevity instead.
I've modified a couple of the unhappy-path tests to make it more
explicit that the flag is false, since it can be true for Platform
Admins and "normal users" alike.
this is a bit complex, but essentially we're using the test variables
defined in the duolabs py_webauthn library [1]. We're already using
their test variables in tests/app/models/test_webauthn_credential.py and
in the webauthn_credential fixture in conftest.py. By using sample
signature, authenticatordata and clientdatajson from the same key we can
test that the library correctly verifies the signed challenge matches
the original.
We needed to transform some of this data as the yubico/fido2 library we
use has a slightly different way of formatting the fields for the
request body, which is why we're doing things like base64 decoding and
converting from hex to bytes in the post data.
The pytest fixture has changed - before it was incomplete/corrupted and
would error when trying to verify the signature. We took the
credential_data from the pytest fixture, converted it to an
AttestedCredentialData using WebauthnCredential.to_credential_data,
modified the public_key private dictionary to add `public_key[-1]: 1`,
and then called `AttestedCredentialData.create` to re-CBOR-encode the
blob.
The `-1: 1` is the numeric ID of the "SECP256R1" elliptic curve
algorithm. The py_webauthn library forces this particular algorithm,
which differs from the sample creds we took from the fido2 lib tests,
which is why we've had to update our data.
[1] https://github.com/duo-labs/py_webauthn/blob/master/tests/test_webauthn.py#L13-L32
