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.
The aggregate names don't need to be sorted, but it helps to do
this for the tests, since the implementation of sets may not be
stable between machines and lead to sporadic test failures.
I did also toy with sorting granular area names so they have a
similar ordering, but this would take them out-of-order with IDs
and really the important thing is that the ordering is stable.
I did consider whether to store this explicitly in the SQLite DB,
but this is less effort for now and we can always switch to that
more robust approach in future if we need to.
This applies some heuristics to try and keep the overall list of
areas short when many are selected in the same wider area.
Currently we only have relationship information between upper and
lower tier local authorities, so we can't / won't aggregate up to
Greater London (it's own special thing) or whole countries.
@benthorner pointed out a few things about these
styles that could do with changes:
- the outline-offset will only appear when the
outline does, which is in forced-color mode when
the browser assigns a colour to it, so it
doesn't need to be assigned in a media query
targeting forced-color mode
- the `&:focus:not(:focus-visible)` selector
stops the focus styles showing in scenarios
where:
1. the browser supports :focus-visible
2. focus comes from something other than tabbing
to the map
...so we don't need to target :focus-visible
specifically.
This applies changes to these styles to remove
those not needed and move some to a better place.
Related to this comment on the associated pull
request:
https://github.com/alphagov/notifications-admin/pull/3996#discussion_r699246969
Depends on: https://github.com/alphagov/notifications-api/pull/3314
Previously:
- We introduced a new "areas_2" API field that's can
populate the "areas" column in the same way.
- We updated the Admin app to use the new field, so that
the old "areas" and "simple_polygons" API fields are unused.
- We repurposed the unused "areas" API field to use
the new "areas_2" format.
This PR:
- We can switch the Admin app back to the "areas" field,
but in the new format.
Future PRs:
- Remove support for the unused "areas_2" field (migration complete)
The map is already in the tabbing order, so can be
moved to by tabbing and by programs like screen
readers or speech recognition, but it doesn't have
an accessible name so when assistive tech' that
requires this for identification gets the contents
read out instead, which is confusing.
This adds an accessible name, via aria-label, made
out of the areas the alert targets.
This also adds some help text, explaining how to
use the map via aria-describedby. This is a pretty
common pattern and is used in native UI like
selectboxes where a range of commands are
available to control the UI 'widget'. Using
aria-describedby means the help text is not used
every time the widget is focused but is available
if the user gets stuck. For example, Voiceover
announces it if the widget is focused but
not interacted with for a period of time, or when
a shortcut key is pressed.
Finally, I also added a role of 'region' because
when I tested with the NVDA screen reader, the
accessible name wasn't announced but this fixed
that. I think it's because the div isn't being
recognised as having a role without it being set
explicitly and is therefore ignored.
