We hide the radio field in the HTML for platform admins, as we don't
want anyone to be able to change their auth type. However, when the form
is validated, the form has a field called login_authentication that it
expects a value for. It silently fails as it complains that when the
user POSTed they didn't select a value for that radio field, but the
error message is on the radio fields that don't get displayed to the
user so they'd never know.
Fixing this is actually pretty hard.
We use this form in two places, one where we have a user to edit, one
where we are creating an invite from scratch. So sometimes we don't know
about a user's auth type. In addition, radio buttons are mandatory by
design, but now sometimes we don't just want to make it optional but
explicitly ignore the value being passed in? To solve this, remove the
field entirely from the form if the user is a platform admin. This means
that if the code in manage_users.py tries to access the
login_authentication value from the form, it'll error, but I think
that's okay to leave for now given we concede that this isn't a perfect
final solution.
The tests didn't flag this previously as they tried to set from sms_auth
(the default for `platform_admin_user`) TO email_auth or sms_auth. Also,
the diagnosis of this bug was confounded further by the fact that
`mock_get_users_by_service` sets what is returned by the API - the
service model then takes the IDs out of that response and calls
`User.get_user_by_id` for the matching ID (as in, the code only uses
get_users_by_service to ensure the user belongs to that service). This
means that we accidentally set the form editing the current user, as
when we log in we set `get_user_by_id` to return the user of our choice
This continues the work from Template Preview [1], so that we have
a complete store of original PDFs to use for testing changes to it.
Previously we did store some originals, but these were only invalid
PDFs that had failed sanitisation; for valid PDFs, the "transient"
bucket only contains the sanitised versions, which the API deletes
/ moves when the notification is sent [2].
Since the notification is only created at a later stage [3], there's
no easy way to get the final name of the PDF we send to DVLA. Instead,
we use the "upload_id", which eventually becomes the notification ID
[4]. This should be enough to trace the file for specific debugging.
Note that we only want to store original PDFs if they're valid (and
virus free!), since there's no point testing changes with bad data.
[1]: https://github.com/alphagov/notifications-template-preview/pull/545
[2]: c44ec57c17/app/service/send_notification.py (L212)
[3]: 7930a53a58/app/main/views/uploads.py (L362)
[4]: 7930a53a58/app/main/views/uploads.py (L373)
We have a label saying "other live services". This label means
other live services for a user making the request, but it could
also be interpreted as other live services for an organisation.
Hence, we are changing the label to "other live services for
that user" to avoid confusion
Previously some of the tests for code in the "formatters" module
were in tests for the "utils" module. This moves them to where
they should be. While two of these methods are probably more utils
than formatters, I'd like to postpone a refactor of that module
for now, and focus on slimming down test for utils/__init__.py.
Previously this was duplicated between the "two_factor" and the
"webauthn" views, and required more test setup. This DRYs up the
check and tests it once, using mocks to simplify the view tests.
As part of DRYing up the check into a util module, I've also moved
the "is_less_than_days_ago" function it uses.
Was just in one of those meetings where it felt like writing this would
take less time than I’d already spent talking about its relative
priority…
---
In the admin app you can already set the broadcast channel as 'test', 'severe' or 'government'.
Aim:
- Add the 'operator' channel to the list of channels you can pick for the admin app broadcast services
Note:
- The API already supports this - https://github.com/alphagov/notifications-api/pull/3262
- The CBC proxy does not yet support the operator channel and this will need a separate card. That card has not yet been written because the interface has not been agreed between us and the MNOs yet.
- Will need to have the ability to select the operator channel for just a single MNO like we do for the other channels
- If we add this, we shouldn't actually start using it until the MNO in question gives us the go ahead.
---
https://www.pivotaltracker.com/story/show/178485177
Previously this was hidden away in an anonymous __init__.py file.
I did think about splitting the models into individual files, like
we do with the top-level models for the app. Since the models are
only imported in one place - i.e. are all used together - it didn't
seem worth the hassle, so I've kept them in one file.
We only need to assert on the URL for the subsequent POST back to
the server, at which point we can call the test "done()". This is
a technique we use in the following tests as well, so we don't need
to comment about it here.
This is easier than re-assigning the mock functions manually, as
we're reusing Jest's in-built behaviour. Because all the mocks
are restored, we need to move the ones we had in the beforeAll
block into the beforeEach block.
Note: "require('./support/teardown.js')" also resets all Jest
mocks, but "require" only runs once, so we can't use it in a
beforeEach block [1]. We could do a "jest.resetModules()" to fix
that, which seems worse on the whole. I think there's a broader
discussion here about whether we could / should have a global
reset of Jest mocks after each test - I quickly tried this and
it causes some existing tests to fail :-|.
[1]: https://stackoverflow.com/questions/48989643/how-to-reset-module-imported-between-tests
is unable to print the letter we need to mark the letter as failed.
If we mark the letter as a technical-failure, we say that we will fix
the issue, which is wrong because we can not fix the issue.
If we mark the letter as validation-failed, the letter is in wrong
bucket so the letter is not viewable/downloadable by the client.
This PR updates the message for a letter marked as permanent-failure to
better reflect what has actually happened.
Otherwise we get the following error:
________________________________________ ERROR collecting tests/app/utils/test_user.py ________________________________________
import file mismatch:
imported module 'test_user' has this __file__ attribute:
/Users/benthorner/Documents/Projects/admin/tests/app/models/test_user.py
which is not the same as the test file we want to collect:
/Users/benthorner/Documents/Projects/admin/tests/app/utils/test_user.py
HINT: remove __pycache__ / .pyc files and/or use a unique basename for your test file modules
Previously these were lumped together with integration-level tests
for specific endpoints, which test the decorator was applied to the
endpoint in question.
This saves a bit of repetition, and lets us attach other methods to the
collection, rather than having multiple methods on the user object
prefixed with the same name, or random functions floating about.
