This is because that function is used both when broadcast status
is updated via API and via admin, so it's a shared resource.
Also move and update tests for updating broadcast message status
so things are tested at source and repetition is avoided.
In one case ("areas=['manchester']") the format was even invalid,
but in general the original value of the column is pretty much
irrelevant for tests that involve updating it (it's highly unlikely
the column would default to the same value as the test data).
This is necessary until:
- The Admin app is using the new "areas(_2)" format to store and
retrieve data.
- We've migrated all existing broadcast messages to use the new
format.
Note that "areas" / "ids" isn't actually used for anything except
printing out the PagerDuty message - it's not sent to the proxy [1].
[1]: 6edc6c70aa/app/celery/broadcast_message_tasks.py (L190-L193)
Currently we have:
- An "areas" column in the DB that stores a JSON blob.
- An "areas" field inside the "areas" JSON that stores area IDs.
- Each field has to be manually copied into the JSON column.
We want to move to:
- An "areas" column in the DB (unchanged).
- An "ids" field inside the "areas" JSON (to replace "areas").
- The Admin app sending other data inside an "areas" JSON field.
The API design for areas is confusing and difficult to extend.
Here we duplicate the current API functionality using an "areas_2"
field. Once the Admin app is using this field, we'll be able to
rename it to just "areas", which is where we want to get to.
In the next commits we'll build on this to support the migration
from "areas"."areas" to "areas"."ids".
This is a temporary feature to make it easy to migrate the format
of the "areas" column and backfill extra data for it.
It's not possible to use this feature to update the status of an
old broadcast message, so the risk from this override is minimal.
We found that non-templated broadcast messages weren’t having their
content normalised before saving into an event.
This means that stuff like `\r\n` and curly quotes were being passed
through to the CBC proxy.
This commit firstly changes templated events to use
`str(BroadcastMessageTemplate)` to normalise the content, because it’s
non-obvious that calling
`BroadcastMessageTemplate.content_with_placeholders_filled_in` also
normalises content.
Then it changes the non-templated route to also call
`str(BroadcastMessageTemplate)`, where previously it was passing the
content straight through.
This changes the content length validation of the internal API to match
the validation of the public broadcast API[1].
This removes the length check from JSONSchema, which isn’t sophisticated
enough to deal with things like normalising newlines or handling
different encodings.
The admin app should catch these errors before they’re raised here, but
it’s best to be belt and braces.
1.7ab0403ae7/app/v2/broadcast/post_broadcast.py (L53-L63)
Also update error message for when someone does not have permissions.
The message referenced approving broadcasts specifically, whereas
people would also see it if they try to cancel or reject
broadcast without permission.
Why we allow platform admins to cancel broadcasts:
we do this so they can react quickly if a broadcast was
approved by accident.
We only want to send a broadcast if the broadcast message is not stubbed
and the service is live at the point at which the broadcast event should
be created. This is to prevent the situation where a broadcast service is
switched to live / trial mode in between the message being created and
approved (we log an error if this happens).
A stubbed broadcast message with a trial mode service at the point of
approval is not an issue - trial mode services can approve their own
broadcasts. In this situation, we don't create the broadcast event but
also don't need to log an error.
If we're not going to send a broadcast, we don't need to create the
BroadcastEvent in the database. The BroadcastMessage contains all the
data we need - the BroadcastEvent is not used.
Not creating the event when we won't send the broadcast (e.g. when the
broadcast message was created when the service was in trial mode) adds
an extra layer of security.
We previously allowed MNOs to approve a broadcast themselves in training
mode and have it go out to their integration environment as per
https://github.com/alphagov/notifications-api/pull/3114
However, we want to remove this use case as it means we have to support
configuration for training mode services to do things like pick a
channel and send out alerts which we definteily don't want to do in
production.
By making this change, we reduce the chance of a single bug meaning an
alert will go out in prod that shouldn't.
Note, will also make it harder for development environment testing but I
think it is still worth it as https://www.pivotaltracker.com/story/show/177584959
will make it much harder in our code to allow some environments to send
alerts whilst in training mode.
otherwise we run into issues where we dont issue the cancel as we say
"oh look the expiry time just passed, so we shouldnt send this message
as it's already been removed from the cbc".
This has been added in for speed of development but now we are getting
close to integrating with production systems, we will be turning off
these helpful hacks to reduce the risk of someone sending a real
broadcast to citizens.
Note, platforms are still able to approve broadcasts when their service
is in training mode.
In the admin app we need something to use in show in lieu of template
name when a template isn’t used. Let’s store this in the reference field
for now.
new broadcast messages will have content filled whether they have a
tempalte or not, but old ones won't so populate.
Stole the session constructor from 0044_jos_to_notification_hist.py
ensures code remains backwards compatible during the deploy. this commit
should be reverted once all broadcast_message.content fields have been
back-filled.
we want to be able to create broadcast messages without templates. To
start with, these will come from the API, but in future we may want to
let people create via the admin interface without creating a template
too.
populate a non-nullable content field with the values supplied via the
template (or supplied directly if via api).
Brings in:
- [x] https://github.com/alphagov/notifications-utils/pull/801
Formats the content of the template at the time of creating the event.
This means that any downstream code (eg Celery tasks) can assume the
content is already formatted correctly.
Also, these downstream tasks don’t know which template the broadcast
was created from, so if we support personalisation in the future this is
the most sensible place to bring together the template and the
personalisation.
---
I had to re-create some of the deleted code from utils for stuff like
formatting the timestamp to the CAP standard.
use the new endpoint from cbc proxy. create a new task that just
serializes the event and sends it across rather than sending a template
and the broadcast message.
some changes to serialize to make it json friendly etc. it also expects
sent_at and transmitted_finishes_at to always be set (we set them in the
code but don't enforce it n the DB right now), as they're required by
utils template. not sure whether we'll update db constraints to be more
strict or utils template to be more permissive just yet, wait until we
find out more about the requirements of the CBCs we integrate with.
we won't let trial mode services send real broadcasts, and it's helpful
for users to see the flow of messages without having to have a second
person with them
