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
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 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.
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.
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)
These will be used as a fallback for display in gov.uk/alerts. It
also helps to have them in the DB to aid in quickly identifying
where an alert was sent, which is hard from the IDs.
We will look at backfilling names for past alerts in future work.
Resolves: https://github.com/alphagov/notifications-admin/pull/3980#discussion_r692919874
Previously it was unclear what kinds of areas this method returned,
and whether there would be any duplicates (due to the hierarchy of
areas we work with). This clarifies that.
In addition, the areas returned may not overlap with the custom one
[1], so we should reword to avoid falsely implying that. We could do
the overlap check as part of the method as an alternative, but that
would create extra work when calculating the ratio of intersection.
We could always add "overlapping areas" as a complementary method to
this one in future.
[1]: https://github.com/alphagov/notifications-admin/pull/3980#discussion_r692919874
Previously we were passing a flag to the API which handled this. Now
we are doing it at the time of clicking the link, not at the time of
storing the new password. We don’t need to update the timestamp twice,
so this commit removes the code which tells the API to do it.
Accepting an invite means that you’ve just clicked a link in your email
inbox. This shows that you have access to your email.
We can make a record of this, thereby extending the time before we ask
you to revalidate your email address.
When someone uses a fresh password reset link they have proved that they
have access to their inbox.
At the moment, when revalidating a user’s email address we wait until
after they’ve put in the 2FA code before updating the timestamp which
records when they last validated their email address[1].
We can’t think of a good reason that we need the extra assurance of a
valid 2FA code to assert that the user has access to their email –
they’ve done that just by clicking the link. When the user clicks the
link we already update their failed login count before they 2fa. Think
it makes sense to handle `email_access_validated_at` then too.
As a bonus, the functional tests never go as far as getting a 2FA code
after a password reset[2], so the functional test user never gets its
timestamp updated. This causes the functional tests start failing after
90 days. By moving the update to this point we ensure that the
functional tests will keep passing indefinitely.
1. This code in the API (91542ad33e/app/dao/users_dao.py (L131))
which is called by this code in the admin app (9ba37249a4/app/utils/login.py (L26))
2. 5837eb01dc/tests/functional/preview_and_dev/test_email_auth.py (L43-L46)
Theoretically the maximum expiry time of a broadcast should be 24 hours.
If it goes over 24 hours there can be problems.
However we want to make it more conservative to mitigate two potential
issues:
1. The CBC has a repetition period (eg 60 seconds) and a count (eg
1,440). If these were slightly innaccurate or generous it could take
us over 24 hours. For this reason we should give ourselves half an
hour of buffer.
2. It’s possibly that the CBC could interpret a UTC time as BST or vice
versa. Until we’re sure that it’s using UTC everywhere, we need to
remove another whole hour as buffer.
In total this means we remove 1 hour 30 minutes from 24 hours, giving an
expiry time of 22 hours 30 minutes.
While testing alerts on these channels the MNOs sometimes need to
restart their CBCs to make sure everything is failing over properly.
If the CBC does not come back up, for whatever reason, then we are left
in a state where the alert can’t be cancelled.
To minimise the impact to the public in this scenario we should keep the
expiry time at 4 hours for alerts sent on test channels. We recently
increased it back up to 24 hours for all channels, so this in effect is
reverting that change for channels that won’t be used in a real
emergency.
By using the simplified polygons instead of the full resolutions ones
we:
- query less data from SQLite
- pass less data around
- give Shapely a less complicated shape to do its calculations on
This makes it faster to calculate how much of each electoral ward a
custom area overlaps.
For the two areas in our tests:
Place represented by custom area | Before | After
---------------------------------|--------|--------
Bristol | 0.07s | 0.02s
Skye | 0.02s | 0.01s
The page_header macro includes an optional back link. Since the
page_header is always used inside `<main>`, where the back link should
not be, this stops setting the back link in the page header and instead
sets it in the new `backLink` block.
This renames the two functions we have to translate between UI and
DB permissions, as well as some of their associated variables to
make it clearer which kind of permission they contain.
These tests are unrelated to the others in test_permissions.py. We
should try and structure our tests the same as the code under test
so that it's clear where new tests should go.
This is so we can allow the sender name 'UC' for DWP.
Note, this is specifically only straight single quotes and not curly
quotes or double quotes. Curly quotes are not supported in the GSM
character set (https://en.wikipedia.org/wiki/GSM_03.38). There is
currently no defined user ask to support double quotes in sms sender
names.
I have tested this by sending a message through both Firetext and MMG to
make sure they both support the single quote character in SMS sender
names.
DWP also have had no particular issues using the SMS sender name with
their existing system in the past either.
No reason this shouldn't be parametrized. It will be neater, more
extendable and give better error messages
No functionality change, however I did slightly adapt one or two of the
test cases (for example to change the 11 characters or fewer test to
test on the boundary with 12 characters rather than many more).
Previously, to get the the `.approve_broadcast_message` endpoint, we
were issuing a POST request to `.view_current_broadcast`. The
`.view_current_broadcast` only has a GET endpoint defined for the
`/services/<uuid:service_id>/current-alerts/<uuid:broadcast_message_id>`
route, so the request would end up at the `.approve_broadcast_message`
which defines the POST endpoint for that path.
This changes the tests to POST directly to `.approve_broadcast_message`
to avoid confusion.
The 'New alert' button was being tested in the same tests as the tests
for the page content for the broadcast dashboards. Now that you only see
the button if you have the `create_broadcasts` permission, this adds a
new test just for the button so that the test for the page content can
be used for both broadcast permissions.
This wasn't adding anything now that we have two new and more specific
fixtures, `active_user_create_broadcasts_permission` and
`active_user_approve_broadcasts_permission`, that can be used instead.
`manage_templates` has now been removed from the `create_broadcasts`
permission, so this also adjusts the fixture for a user who can create
broadcasts.
The `send_messages` permission has been deprecated for use with
broadcast services, so we can drop support for it in the code. We
were supporting both the old permissions and new permissions
(`create_broadcasts` and `approve_broadcasts`) while we switched people
over.
This removes `send_messages` from the `user_has_permissions` decorator
around the broadcast routes and from the page to view a broadcast and
broadcast dashboards. We can now git rid of a lot of the parameterization
that was temporarily added to the tests.
At the moment we say that you either ‘add’ an alert or ‘send’ it.
This is confusing because:
- an alert isn’t received on people’s phones until it’s approved, so
this is really when it is ‘sent’ conceptually
- an alert can be rejected before anyone receives it, so the UI can say
an alert that no-one ever received was sent
This commit re-labels things so that the the first part of the process
is ‘creating’ the alert.
This makes all the permissions nice and distinct from each other. Adding
templates and adding alerts feel conceptually quite different things
(what are you adding the alert to?).
It will likely be the same people who have permission to create alerts
and edit templates (maybe someone in a comms role).
But combining the two permissions makes the options presented in the
form feel clunky because ‘alerts’ and ‘templates’ are conceptually quite
different.
So I think it makes sense to keep the templates permission the same as
it is for regular Notify services.
