Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove emergency alerts code #3976

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Conversation

leohemsted
Copy link
Contributor

@leohemsted leohemsted commented Dec 14, 2023

This doesn't deal with any database migration, but it does get rid of all code.

TODO before we merge:

  • check nothing in admin app will break. for example, what happens with existing older broadcast templates? do they still exist, are they viewable? will they break if you go view them?
  • check nothing with redis will break
  • figure out what to do with broadcast data in the db. especially tables that are used for other things (eg templates, services, etc)

@quis
Copy link
Member

quis commented Dec 19, 2023

At the moment if you try to view a broadcast template in the admin app you will get an exception (the app doesn’t know how to preview one of these templates).

But the only way you can get to that page is by manually entering the id of the template in the URL. You can’t see a list of those templates on the templates page for example. I don’t think this PR will change that behaviour, so I don’t think handling it needs to be a blocker to merging.

We could put in a workaround to return a 404. Or we could do a migration later on to delete all broadcast templates.

@leohemsted
Copy link
Contributor Author

leohemsted commented Dec 19, 2023

i think to start with we could put 404s on any broadcast template or broadcast service endpoint, and then delete the data later

i'd be keen on deleting the data in the DB earlier rather than later so we dont get surprised by weird data if we want to update schemas on those tables or migrate data based on columns listed in the models or things like that

@idavidmcdonald
Copy link
Contributor

FYI, we can't delete any of the broadcast data out of our database for 1 year please. Either that or we need to take a snapshot of that data and store it elsewhere for a year. This is because emergency alerts need it as an audit trail for any security incidents (precedent is to store it for a year, just like Notify does).

The 1 year is from the time they migrated to their own system (was that like early November maybe?)

@leohemsted
Copy link
Contributor Author

thanks, that's useful to know david. I'd probably be keen on snapshotting and storing it in a pgdump in s3 ideally

@leohemsted leohemsted marked this pull request as draft December 28, 2023 11:40
@leohemsted leohemsted force-pushed the remove-emergency-alerts-code branch from bbf8b56 to 4bbec83 Compare January 10, 2024 21:15
@leohemsted leohemsted force-pushed the remove-emergency-alerts-code branch from 4bbec83 to c6d66eb Compare February 7, 2024 10:07
@leohemsted leohemsted force-pushed the remove-emergency-alerts-code branch from c6d66eb to 85bbc98 Compare September 27, 2024 15:32
this is a data migration (so no downgrade) that deletes broadcast
services and all associated data - this includes obvious things like
broadcast events and templates, but also everything connected to those
services like user permissions, inbound senders, ft_billing rows, etc

Although some of these theoretically shouldn't exist for broadcast
services (eg returned_letters), issue the deletes out of an abundance of
caution. These migrations need to pass on preview and staging as well as
production, and for example there was a broadcast service on staging
with an inbound sms number

This has been tested against a copy of the staging database.
@leohemsted leohemsted force-pushed the remove-emergency-alerts-code branch from 85bbc98 to 9ce67d6 Compare October 1, 2024 14:43
no longer used. there might be additional cleanup we can do of other
utils functions that dont have "alert" or "broadcast" in the name that
are no longer used
the schema was originally created so that it could not show content.
However, then it needed content conditionally for broadcast messages so
content was added back in.

Now that broadcasts no longer exist we can revert that - however, for a
template that has a specific list of expected keys, it's nicer to just
list those rather than maintain a huge list of exceptions that needs to
be modified every time a field changes on the template model
@leohemsted leohemsted force-pushed the remove-emergency-alerts-code branch from 9ce67d6 to dd6210a Compare October 1, 2024 14:54
@leohemsted leohemsted marked this pull request as ready for review October 1, 2024 15:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants