This fixes a bug where a third inactive provider would (potentially)
appear in place of one of the two active ones, depending on the order
of its identifier compared to the other two.
In future we may look at simplifying this to cope with more than two
active providers. For now, the existing UI will continue to work when
we add a new, inactive SMS provider for Reach.
Previously I found it hard to audit these tests when they flipped
between emails and letters when scrolling vertically.
While these have much in common, they are still separate features
and will evolve separately.
This is now covered by the unit tests for the new branding module.
Showing the "something else" text box is also already covered by
another UI test, so we don't need it for that either.
Users who have no mobile number set, users who are not on email auth
and users who are not on "Change mobile number" page should not
see the delete link.
We added this for the 25 May demo. We’re unlikely to need it again.
We’re also unlikely to need this library again, so this commit removes
the source and the code that creates it.
All of the mock / UI assertions in these tests are covered by the
tests above them - these tests were mostly targetting which options
were being shown, which we can check at a lower level.
This is much simpler than trying to test the function via the page,
although there are still two scenarios to test there:
- The page with radio buttons (using NHS as an example).
- The page with a text form (using "other" as an example).
In future work we could split this test in two to make it clearer
what it's trying to test. For now, this keeps the diff simple.
On the ‘find user’ page it says ‘sms_auth’ instead of ‘Text message
code’.
This commit fixes that, and adds a handy formatter so it’s easier to do
the right thing in the future.
This is to fix a bug where a user creates an account but doesn't
complete registration, then they are invited to a service that
changes their auth to email_auth, and then when they try to
complete registration they are still asked for sms code.
It should save users some pain, and reduce number of support tickets.
So we do not have to go into the db when we need to change user
auth.
We do not allow this for users who use webauthn. We do not want to
enable security downgrade for those users.
Previously we duplicated the "something else" email branding form
on its own page and embedded in the choices form (if it was the
only option). See [^1] for how this looks - it's inconsistent.
This DRYs-up the "something else" form by bypassing the choices
form when "something else" is the only option. I've also tweaked
the "Back" button to be consistent with this behaviour.
Making this change also simplifies the choices form, which we'll
be adding pool options to shortly. I'd like to make the letters
form consistent, but let's see how emails pan out first.
Note that the choices form will now show a single radio button if
"something else" is the only option. I think that's OK as nothing
will link to the page, and the form still works.
[^1]: https://github.com/alphagov/notifications-admin/pull/4163#issuecomment-1050088088
We want to be able to set the free allowance for a service to 0, but the
form was not allowing this - it gave an error message of `Cannot be
empty`. This can be fixed by changing the WTForms validator from
`DataRequired` (which coerces 0 to falsey) to the `InputRequired`
validator.
Sometimes users ask us to delete their mobile numbers for them.
If those users are on email auth, they should be able to
delete their number themselves.
This will save them writing a support ticket and save us
going into the database.
This adds a preview of the current branding to users on the page where
they can select which new branding they want. Also includes a tiny
content change to match the new content doc for this story.
The pages you were redirected to if you selected either GOV.UK branding
or NHS branding used to give information about the branding and have a
button that submitted a Zendesk ticket. Now, we show a preview of the
new branding and the button applies it.
The `.email_branding_govuk` and `.email_branding_govuk_and_org` routes
shared a template since the content was the same - the only difference
was in the action of the button. However, since the pages will no longer
be so similar (e.g. the govuk page will show a preview) this splits them
up to use separate templates.
It may be the case that when the branding work is complete these pages
are fairly similar and we decided that one template between the two
endpoints is the best option again.
Having to submit the form for each choice separately slowed us down
during an incident where Redis was unavailable and came back with
stale data, which we had to clear manually.
Note: we don't want to use the "flush" feature in case there are other
keys in Redis, which may not be safe to remove.
This will make it easier to add another test / feature to clear all
the cache keys. It's debatable which of "sum" and "max" is useful:
- "max" is a better (although still not accurate) indicator of the
number of "things" affected e.g. templates, services, etc.
- "sum" makes sense in places where "max" doesn't e.g. when we clear
the "organisations" group, which doesn't equate to individual orgs.
Using "sum() ... across" seems like a reasonable compromise and makes
it clear that we're iterating over different kinds of keys.
While the pluralisation is nice, I don't think it's worth the effort
to make it work for both "object(s)" and "format(s)".
This repeats the pattern we already have for previewing a letter,
where we assume the error is because the notification has already
been sent and redirect the user to see it.
I've improved the original pattern a bit:
- I've DRYed-up the low-level boto code and moved the error handler
there so it can be reused.
- I've introduced a custom exception, which the calling code can
choose to log.
- I've introduced the moto library, which we use elsewhere, to make
it easier to test S3 code.
I've used an error level log when sending a notification - now that
we have a more descriptive log, we can verify the assumption is true
and then make an informed decision to downgrade the log.
In future we may want to merge this handler with the similar code
in utils [1], but we'll need to be careful as the utils handler is
superficial - it doesn't check the reason for the error.
[1]: bce0f4e596/notifications_utils/s3.py (L52)
This strengthens the initial check of what's in the session to make
sure it contains some kind of recipient. Without this, we get:
Traceback (most recent call last):
File "/home/vcap/deps/0/python/lib/python3.9/site-packages/flask/app.py", line 1950, in full_dispatch_request
rv = self.dispatch_request()
File "/home/vcap/deps/0/python/lib/python3.9/site-packages/flask/app.py", line 1936, in dispatch_request
return self.view_functions[rule.endpoint](**req.view_args)
File "/home/vcap/app/app/utils/user.py", line 26, in wrap_func
return func(*args, **kwargs)
File "/home/vcap/app/app/main/views/send.py", line 1041, in send_notification
recipient=session['recipient'] or InsensitiveDict(session['placeholders'])['address line 1'],
File "/home/vcap/deps/0/python/lib/python3.9/site-packages/notifications_utils/insensitive_dict.py", line 41, in __getitem__
return super().__getitem__(self.make_key(key))
KeyError: 'addressline1'
I'm not sure how to reproduce this, but this should at least give
the user a better experience, instead of a 500 page.
