There are no changes to appearance of the 'Preview this alert' button or
what it does, but this stops a CSRF token appearing in the query
string when you click the button. We don't need a CSRF token - it's
a simple GET request which doesn't change any data. Before, we had a form
with `method="get"` but because we were using a `page_footer` a CSRF
token was being added.
We can replace both the `<form>` element and `page_footer` with a
`govukButton`. This means that we make a GET request with no CSRF token
without changing the appearance of the button.
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.
This was accidentally missed out of a previous PR. It ensures that
when you visit the `.email_branding_govuk_and_org` endpoint "Settings"
is highlighted in the left hand menu.
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.
File-list items contained by div.keyline-block's
are pushed down 3px more than elsewhere which
creates a gap between the top of the focus style
and the keyline above. This extends the part of
the focus style made up by a box-shadow by 3px so
it covers the gap.
If the :has pseudo-class becomes available in the
browsers we support in future, the code this
branch/pull request adds will be redundant as its
behaviour will be possible only using CSS.
This adds comments to note this to the parts of
the code you'd need to remove.
Includes the following:
Guard for adding duplicate classNames
Stops the code that allows classNames to persist
across updates to the component HTML from adding a
className multiple times to the list of those to
persist. From this comment on the associated pull
request:
https://github.com/alphagov/notifications-admin/pull/4155#discussion_r804639058
Add comment explaining guard for operations on
elements no longer in the DOM
The value of this guard can be unclear and why it
is needed so we add a comment to explain this.
From this comment on the associated pull request:
https://github.com/alphagov/notifications-admin/pull/4155#discussion_r804697189
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
We can't guarantee that elements we stored a
reference to with `classesToPersist.remove` will
still exist so we need to guard against this.
Note: it checks for whether the node is still
attached to the DOM rather than whether it exists
because the standard way to delete a node just
detaches it from the DOM and relies on garbage
collection to delete it from memory.
The current updateContent JS replaces the in-page
HTML with the HTML from the server the first time
an AJAX request is fired, even if the HTML from
the server has no changes. This is because the
code that compares the two operates on two
different things:
The HTML in the page is the component HTML, with
all the data attributes and the partial HTML
(marked with the 'ajax-block-container' class) as
its first child:
```
<div data-module="update-content" data-url="...">
<div class="ajax-block-container">
...
</div>
</div>
```
The HTML from the server only contains the
partial:
```
<div class="ajax-block-container">
...
</div>
```
The diffing code just sees them as different at
the top level so replaces the page HTML with the
partial from the server. This means all subsequent
diffs are between partial HTML and partial HTML so
only update on actual changes.
These replace the component with the partial, as
part of the component initialising. This means all
code that runs on an AJAX response will only
compare like-for-like so will result in actual
changes (or none at all), not just swapping one
element out for another.
Note: this commit also removes the
aria-live="polite" from the ajax_block component.
It has always been overwritten by the first
response so never announces anything to assistive
technologies. Removing it makes this more clear.
Includes:
- JS to add a class to the heading when the link
is focused
- CSS to apply the enlarged focus style via a
selector which uses that class
- changes to the partial to hook in the JS to
track focus on links and to tell the
updateContent JS to persist the classes added
between updates to the HTML
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.
Changes:
53.0.0
---
* `notifications_utils.columns.Columns` has moved to
`notifications_utils.insensitive_dict.InsensitiveDict`
* `notifications_utils.columns.Rows` has moved to
`notifications_utils.recipients.Rows`
* `notifications_utils.columns.Cell` has moved to
`notifications_utils.recipients.Cell`
52.0.0
---
* Deprecate the following unused `redis_client` functions:
- `redis_client.increment_hash_value`
- `redis_client.decrement_hash_value`
- `redis_client.get_all_from_hash`
- `redis_client.set_hash_and_expire`
- `redis_client.expire`
51.3.1
---
* Bump govuk-bank-holidays to cache holidays for next year.
51.3.0
---
* Log exception and stacktrace when Celery tasks fail.
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.
We've changed the URLs for these endpoints but needed to keep the old
URLs in place in addition to the new URLs in case anyone was in the
middle of changing their branding as the change was being deployed.
We can now safely remove the old URLs.
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.
