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.
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.
In previous iterations of the classPersister, we
found issues with the implementation meant classes
it should have added back to elements were also
added to other elements. This adds tests for this
scenario to ensure it doesn't happen again.
Also includes changes to fix a linting error with
the JS which complained about a function being
defined in a loop while referencing variables in
the outer scope.
The assumption that the classes you want to
persist will always have parity with the elements
that have those classes, at that point, won't
always be true.
Because of that, this changes the way elements
with those classes are stored, to be in a map
between classes and the elements with them (at
that point).
Also includes an extra test for a scenario where
more than one updating component is in the page
with classes that need to persist through updates.
The updateContent JS was changed in this commit so
the replacement of the original HTML (with GOVUK
modules data-attributes) was moved into the start
method rather than being a slightly odd side
effect of the render function diffing:
476ed1593c
This adds a test to make it more clear this
happens, as requested in this comment:
https://github.com/alphagov/notifications-admin/pull/4155#discussion_r804689618
Wrap the code that updates the HTML with changes
from the server with code that stores and
re-applies specified classes.
This is to allow other JS to add classes which
change the visual state of the HTML without them
being considered by the code that diffs our
in-page HTML against that from the server.
They are called classesToPersist because this
should make the visual state they create persist
between updates.
Includes the addition of tests for updateContent
that cover the addition/deletion of elements so we
can write a test for classNames persisting through
updates. The existing tests only cover updates
that change the content of elements. Just adding
the test for these changes to those would simulate
a scenario that doesn't exist in the app. Writing
extra tests for the kind of updates these changes
act on keeps them in line with the app code.
The endpoint to change the email branding to "GOV.UK" branding and
"GOV.UK and organisation" branding was the same but with a query string
used to determine which of the two options had been selected. This makes
them two separate endpoints, which makes the code a bit simpler and
hopefully means there is less chance of things not working as expected.
This changes the URLs for someone to request new email or letter
branding to match the new URLs we've agreed for the new email branding
changes. The old URLs are still in place for now too to keep backwards
compatibility.
It shouldn't be possible to view the page to confirm that you want a
particular type of email branding if that branding is not allowed for
your service. Although we don't show banned branding options on the
branding form, it would have been possible to visit the relevant URLs
directly.
We now give a `404` status page if you visit a page to select branding
that isn't allowed.
For the "Something else" branding form we want the form label to be the
title. This brings in the textarea component from GOV.UK Frontend in
order to do this since that contains code to set a the textarea label as
the page heading in an accessible way.
The rest of the textarea fields have not been switched to use the new
component yet.
We were showing the form to request email branding with a button which
submits your choice immediately. Now, we only submit the form
immediately if "Something else" is the only branding option available to
you. If you select any other radio button (or select "Something else"
when it's not the only option) we take you to another page which either
contains more information or a textbox to fill in the details for the
branding you want.
There is currently some duplication between the new pages and their
tests, but these will be changed in future versions of the work so will
start to differ more.
The existing tests were parameterized to contain the cases both where
the branding form is successfully submitted and those where it isn't. We
were using `pytest.mark.xfail` to check that the tests fail as expected
if there were errors on the form. However, since I'll be changing how
the form validation works, I want to make sure that these tests were
actually failing because of the form validation and not because of
another reason, such as a slight difference in Zendesk ticket output.
This creates separate tests for cases where data entered in the form
is invalid.
The endpoint used to handle both email and letter branding, but this
replaces `.branding_request` with `.email_branding_request` and
`.letter_branding_request` instead. This is in preparation for changing
how email branding works.
The `from_template` arg was only possible for letter branding, so I've
removed that from the `.email_branding_request` endpoint.
We added domdiff to replace the DiffDOM library
here:
87f54d1e88
DiffDOM had updated its code to be written to the
ECMAScript 6 (ES6) standard and so needed extra
work to work with the older browsers in our
support matrix. This was recorded as an issue
here:
https://www.pivotaltracker.com/n/projects/1443052/stories/165380360
Domdiff didn't work (see below for more details)
so this replaces it with the morphdom library.
Morphdom supports the same browsers as us and is
relied on by a range of large open source
projects:
https://github.com/patrick-steele-idem/morphdom#what-projects-are-using-morphdom
It was tricky to find alternatives to DiffDOM so
if we have to source alternatives in future, other
options could be:
- https://github.com/choojs/nanomorph
- https://diffhtml.org/index.html (using its
outerHTML method)
Why domdiff didn't work
Turns out that domdiff was replacing the page HTML
with the HTML from the AJAX response every time,
not just when they differed. This isn't a bug.
Domdiff is bare bones enough that it compares old
DOM nodes to new DOM nodes with ===. With our
code, this always results to false because our new
nodes are made from HTML strings from AJAX
response so are never the same node as the old
one.
Having this as a function which does string parsing and manipulation
surprised me a bit when I was trying to figure out why something wasn’t
working.
It’s more in line with the way we do other config like this (for example
`ASSET_PATH`) to make it a simple config variable, rather than trying to
be clever and guess things based on other config variables.
It’s also less code, and is explicit enough that it doesn’t need tests.
This field caused some confusion and lots of unnecessary work
to our colleague because of unclear name.
The field was named sms_fragments, where in fact the value of
the field is: those sms fragments that go above free allowance
multiplied by the rate multiplier.
The new name was chosen through consultation with colleagues
who use billing report the most.
for a job to be finished there are two requirements:
* the status to be "finished"
* the percentage complete to be 100%
The job status is set to finished when the process_job task finishes
(even though not all process_row may have finished). The
percentage_complete is calculated by comparing the number of
notifications in the database with the number of rows in the
spreadsheet.
This was inadvertently changed from an "and" to an "or" clause two years
ago. This meant that people could download a report when the status was
finished but not all notifications were present in the database. Lets
change it back.
7d52ac97f1 (diff-44b012cad205379c481bed244ddb2294bae5ee85dcd01f4aee932a2bd85b67b2L86-R100)
