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
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
This takes a similar approach as in the previous commit. Since the
"training channel" doesn't really exist, we need some extra code
to pre-select it if a service is already in training mode. As in
the previous commit, I've removed a few non-critical test cases
where we really don't need to test exhaustively.
Note that we also need some specific code to avoid pre-selecting an
option for non-broadcast services, which only used to work by fluke:
we would try to populate the field with (False, None, 'all'), which
isn't a valid combination, so nothing was selected.
Previously this field had to mimic the final hyphenated string of
the broadcast account type, even though it was only used to select
one of its components. The new, shorter choices make it easier to
simplify the test for the POST request.
I've also deleted a number of test cases for pre-selected radios.
This functionality isn't critical, so we don't need to exhaustively
test every single possible combination of values.
This allows us to start decoupling the form fields from the final,
hyphenated string, which we'll do in the next commits.
Note that I've also removed the conditional that changes the data
of the network field as part of validating it. We shouldn't change
data in validations, and having the new property directly above
makes it clear there's no need for this code.
Previously we had to cope with two forms of the hyphenated string
we use to represent a pending change in broadcast account type.
Using "all" to mean "all providers" matches the behaviour in the
API [1], and means we can remove some complexity.
"training-test-all" isn't ideal, since the provider is irrelevant
for a training mode service. However, this isn't much worse than
the previous "training-test", noting that the channel also has no
relevance. We'll iterate this in later commits.
[1]: 8e1a144f87/migrations/versions/0352_broadcast_provider_types.py (L14)
Previously we could only select a provider when using the test
channel, but this is also required for others channels when we
do tests on the production network with individual MNOs.
In order to reduce duplication and improve consistency, I've reused
the new broadcast_service_name_tag macro to show the setting.
Because we were redirecting in all cases the error message wasn’t being
shown.
This commit changes the endpoint to respond with content (including an
error message) if the `POST` is not successful.
We want people to be really sure before sending a live broadcast, not
just clicking through the green buttons.
This commit adds a checkbox which explains exactly the consequences of
what they’re about to do, tailored to the channel they’re on, and the
area chosen by the person creating the alert.
Only the test channel has the option to isolate messages to one network.
This commits makes the choices less confusing by only showing the
network choice to those who have selected the test channel.
We have been asked to support the government channel so that:
- it can be tested
- the option to use it is available for the most severe of emergencies,
where the public’s choice to opt-out is outweighed by the widespread
risk to life
It feels quite dangerous that it’s just one click to make an emergency
alerts service live.
This commit adds a confirmation step which explains the consequences of
what you’re about to do.
The current_service.allowed_broadcast_provider is now always "all" or
one of the four providers, which means we can simply the code by not
checking if it is None.
Until all the data is updated to always be "all", we have to handle the
case of provider_restriction being set to None or "all" (which mean the
same thing).
The code can be tidied up once the broadcast provider_restriction is never None.
We're replacing the value of None with the value of all. API has been
updated to accept both values
(1767535def)
so this change starts sending notifications-api the value of "all".
This is the first step in making the UI easier for setting the
options for a broadcast service. Here we remove the options for
"Training mode" test channels. When we create a broadcast message for a trail mode service it is marked as stubbed and does not create a broadcast event that is sent to a provider.
The label for the form and setting page have been updated to reflect the
change.
For someone who has retrieved a template ID from their system the only
way to find it in Notify is:
- hack the URL
- click through every template, visually inspecting the ID shown on the
page until you find the right one
Neither of these is ideal.
This commit adds searching by ID, for those services who have an API
integration. This means we don’t need to confuse teams who aren’t using
the API by talking about IDs.
This is similar to how we let these teams search for notifications by
reference[1]
1. https://github.com/alphagov/notifications-admin/pull/3223/files
WTForms now renders the `required` attribute if there is a validator
such as `DataRequired`. This was flagged in an accessibility audit as
being unnecessary since it doesn't conform to the Design System
recommendations, which state that "all form fields are considered
mandatory when navigating a government service unless otherwise denoted
by the word ‘(optional)’."
This uses the approach here https://github.com/wtforms/wtforms/pull/361
to overwrite the `render_field` method.
Note, no option at the moment to set the service broadcast account type
as None, or back to without the broadcast permission. This has been done
for speed of development given the chance of us needing this is very
low. We can add it later if we need to.
As per ticket and as per Caley's request, so everything can be
edited together.
Also pluralise labels for billing contact info, to indicate
that putting multiple in is ok, and widen the input fields so
that it is more comfortable to input multiple contact details.
Includes changing the code so that the radios
aren't split into two columns in the HTML present
when the page loads. This layout is now added by
the JS.
A comment on the pull request for this branch
pointed out that it's not clear why the 'items'
list is deleted and then reassigned in
extend_params:
https://github.com/alphagov/notifications-admin/pull/3770#pullrequestreview-573067465
The simple reason is that we want to use
merge_jsonlike to merge params and
param_extensions (passed in as extensions) but
merge_jsonlike doesn't merge lists correctly.
I realised that if we just make merge_jsonlike
merge lists correctly, we can use it for
everything extend_params does.
This commit does that, and replaces all calls to
extend_params with merge_jsonlike.
Because extend_params is used across many form
field classes, and so many pages, I took the
following precautions after making those changes:
1. found every use of param_extensions
2. looked at the merges onto params that each would
cause and deduped them to a final list of 6(!)
3. tested pages containing fields from that list
4. added new testcases to the merge_jsonlike tests
for any merges that exist in our codebase but
not in our tests
We think that in some cases alerts will be composed in the moment, and
therefore making people first create a template is:
- not a good use of their time
- adding some conceptual complexity which they don’t need
This commit makes it possible to type some words and have them go
straight into the `content` field in the database.
In the future we might want to progressively enhance the radio buttons
so they show on the same page (like we do with the grey buttons on the
templates page).
The OrganisationAgreementSignedForm class has a
bug causing it to render different HTML when the
page loads to when you subsequently refresh it.
This commit proposes a change to the extend_params
function to fix it.
extend_params, is used by the
OrganisationAgreementSignedForm, as well as all
the other WTForms field classes we added to wrap
GOVUK Frontend components. Fixing it should
therefore fix any similar bugs with them.
All of these fields send a dict of configuration
data to the GOVUK Frontend component when they
call it, at render time. This dict is 'JSON-like',
meaning it's values can be all the primitives as
well as lists and dicts. This also means it can go
quite deep.
Extending the default configuration
The classes have a default dict of this data kept
privately in the params variable. They let you
change it by passing in an argument called
param_extensions on instantiation, after that,
through an attribute of the same name and at
render time as the same argument (in templates).
The extend_params function
The param_extensions dict is used as a collection
of changes to make to the default params dict.
The changes are applied by the extend_params
function. Its code deletes part of the
param_extensions, a side effect that didn't seem a
problem because it isn't used after the function
has run.
The bug
The bug was only with the part of the HTML that
got its data from the part of the param_extensions
dict that was deleted by extend_params. The class
with the bug set param_extensions when the field
is instantiated, as part of its parent form
definition.
My guess is that param_extensions was stored in
memory, as part of the form class, and reused
when the page refreshed. At that point,
extend_params had deleted part of its data,
causing the bug.
