The intention behind the version of node in the
engines property was for that version to be the
minimum required so it was always missing the `>=`
prefix.
This adds that prefix and also adds a setting for
npm, to prevent use of insecure versions. See this
article for details:
https://github.blog/2021-09-08-github-security-update-vulnerabilities-tar-npmcli-arborist/
The way we're using the updateContent.js code is
slightly different to expected and to the
scenarios in our tests. This changes the
tests to match that use.
The expected behaviour was for updates to a
module's HTML to happen to the HTML inside of the
div[data-module=update-content] element.
So with initial HTML of:
<div data-module="update-content" data-key="one">
<div class="ajax-block-container">
Existing content
</div>
</div>
...should be updated to be:
<div data-module="update-content" data-key="one">
<div class="ajax-block-container">
New content
</div>
</div>
Instead the HTML returned by the AJAX requests
replaced the div[data-module=update-content]
element.
So with initial HTML of:
<div data-module="update-content" ..>
<div class="ajax-block-container">
Existing content
</div>
</div>
...will be updated to be:
<div class="ajax-block-container">
New content
</div>
This doesn't seem to create any noticable changes
to the visual interface so, I think, went
unnoticed. The assumption I am making, of this
being unintended, is based on the fact that the
div[data-module=update-content] element has an
aria-live attribute, which authors would normally
want to stay in the page when updates happen.
Note: This commit doesn't try and fix the problem,
as the behaviour still largely works and the lack
of aria-live actually seems to be a positive
thing, meaning non-visual users aren't told of
every update but can discover it themselves if
needed.
A while ago diffDOM moved its code to use ES6
modules and started using various language
features specific to ES6. These two things
happened independently btw.
The result of this is that the version of diffDOM
suitable for our build pipeline, structured as an
immediately invoked function evocation (IIFE),
now requires polyfills of some ES6 features to
work in the older browsers we support, like IE11.
It's also worth noting that in the move to ES6
the maintainers of diffDOM have adopted a process
whereby users who need to support older browsers
now have to add polyfill code for any ES6 features
they choose to use.
This commmit proposes a move to the domdiff
library instead because:
- it runs on all javascript runtimes with no
polyfills
- it is 2KB instead of diffDOM's 25KB
Domdiff takes a different approach to diffDOM, in
that it compares existing nodes and new nodes and
replaces the existing ones with the new ones if
there are differences. By contrast, diffDOM will
make in-place changes to nodes if there are enough
similarities. In other words, in most situations,
diffDOM won't change the node in $component
whereas domdiff will.
Because of this, I've had to change the
updateContent.js code to cache the data-key
attribute's value so we don't lose access to it by
overwrite the $component variable with a different
jQuery selection.
The radio buttons to select the type of service - central, etc. -
are only shown if we can't infer the type based on the user's email
/ default organisation. However, the code to render the page in the
error case didn't accommodate this, nor did it show the version of
the page for adding a local government service.
This fixes the bug by DRYing-up the logic to render the pages. I've
not added a test for this for a couple of reasons:
- It's not a critical bug: no one has complained about it and it
doesn't block the user from adding service.
- It's unlikely to reoccur because the bug involved writing _more_
code than was necessary.
- It's not trivial to test this due to the 3 versions of the page
involved - these are tested for the happy path.
the api returns UTC timestamps, we should keep them as UTC timestamps
until the very last moment, and only convert them into BST when we know
we want to return to a user (ie: in contact-list.html and other places
like that)
since we are hard-coding a generic error message on the front-end, we
have no need to do anything on the back end. This is also nice as it
standardises the two flows to behave more like each other (rather than
previously where one would `flash` an error message and the other would
return CBOR for the js to decode).
Note that the register flow returns 400 while the auth flow returns 403.
The js for both just checks `response.ok` so will handle both. The JS
completely discards any body returned if the status isn't 200 now.
turns out that we're only using errorBanner with a static message, and
it's also full of rich html content. This means that it's probably
better to put it in the html templates with other content, rather than
hidden away in js files if we can help it.
Since there are two places, had to dupe the error message but i think
that's fine as i don't anticipate this error message being used in
significantly more places.
making it a string is a bit gross and means we don't get nice syntax
highlighting on it, but as it needs to be passed in to a jinja macro
that's the way it has to go unfortunately.
the banner is a nicer user experience, and consistent with how we
display errors elsewhere in notify. For now pass through the error
message from JS, but we'll probably want to change that since the erorr
messages themselves are often a bit cryptic and unhelpful
this ensures it's reusable by other components, and easier to unit test
by isolating the separate concerns
note: this is not in Modules since that's designed for classes that are
then bound to an element in the DOM as indicated by a data-module
attribute. This will just live at the window.GOVUK level since we want
there to only ever be one `.banner-dangerous` warning.
Add additional instructions for the service name - this is more consistent with the local government version of this page
Also update tests to use the new content.
The original code to raise the exception was flawed: if a broadcast
only had a single area that was invalid, we would assume it was a
custom broadcast [1]. Since the recent changes [2] fixed the flaw
we're now getting exceptions for broadcasts of this kind.
It's not practical to go and manually fix the invalid broadcasts,
and the likelihood is there will be more in future as the set of
areas we support changes. This takes a pragmatic approach of simply
logging the issue and pretending such broadcasts are custom.
[1]: 926ada2f21
[2]: https://github.com/alphagov/notifications-admin/pull/4014/files#diff-2dd8f77d6df281e7674b20263cdf27a3d58b839dc5930c0087ac8b9749b313e4R92
name is designed for a human readable description of what the thing
you're copying belongs to. (while thing is supposed to describe what the
value represents.
For example on the reply-to email address page, thing="ID" because
you're copying a uuid, and name is the actual name of the email address.
So the talkback speech will read out "copy ID to clipboard for
my@email.com, button".
However, in our case, there's no need to add what the context is for
since each copyable item on the page is something different (a sort
code, a VAT number, etc).
Removing the name makes the talkback just read "Copy sort code to
clipboard", which is what we want. However the macro also only shows a
header if the name is present, so we have to add the header manually.
lets us keep cabinet office financials safe in the credentials repo
the dict in the creds repo will either be an empty dict or a full dict,
so the env var on paas will always contain some parseable json. But
locally it might not, so if it's not set at all then default to the
string `null` so the json parsing doesn't throw a wobbly.
Previously we relied on the API defaulting this field to an empty
array [1], but that conflicts with using it to decide whether a
broadcast is custom or created in this app.
[1]: 3779146cc5/app/models.py (L2342)
Custom broadcasts created directly via the API app won't have area
IDs since [1], where we started to distinguish between "names" (all
broadcasts have these) and IDs (for broadcasts created in this app).
We forgot to propagate the distinction into the code here.
This code fixes the bug for all broadcasts created after [1]. Any
custom broadcasts created before [1] will have their "ids" field set
instead of "names" so we won't show anything for them. This seems
reasonable as we don't support custom broadcasts yet.
[1]: 023a06d5fb
The CSS that cancelled outline on focus events not
fired by the :focus-visible heuristic is being
overridden by the higher precedence of the outline
style for :focus, due to its use of !important.
This adds !important to the cancelling CSS. This
brings that block up to the same level as that for
:focus, meaning the :focus-visible styles will win
because they sit lower in the stylesheet.
Based on these comments on the associated pull
request:
- add area/areas condition to the array used to
build the label prefix
e2af2f63a4 (r55831534)
- use a for loop instead of while when looping
through nodes
e2af2f63a4 (r55831693)
There is a slight variance in how the line between
the map buttons is rendered when in forced-colors
mode and when not. This is not helped by it
alternately being rendered as either:
- a gap between buttons, showing the container
colour
- a border-bottom on the first button
This attempts to flatten these styles so it is
only styled as a gap between the buttons so
changes to how its colour renders in different
modes can just be dealt with on the container.
