Remove emergency alerts db schema #4202

leohemsted · 2024-10-01T13:30:43Z

Lots to read in the commits

I've tested this against a backup of staging - both upgrades and downgrades - i'll chat about that in a tech catchup soon

delete data (Remove emergency alerts data #4201)
clear redis
delete code (Remove emergency alerts code #3976)
delete db schemas 🚨 you are here 🚨

leohemsted · 2024-10-01T15:14:38Z

the squawk failures are:

warning: ban-drop-table

note: Dropping a table may break existing clients.

warning: changing-column-type

note: Requires an ACCESS EXCLUSIVE lock on the table which blocks reads.
note: Changing the type may break existing clients.

i think these are okay. we've done the work to remove reference to these tables so that we can drop them
the access exclusive lock on the tables is unavoidable. if we're concerned (and we might be), we might want to consider running each table (templates, templates_history, service_contact_list) in their own migrations 🤔

Full output

/tmp/upgrade.sql:96:2: warning: ban-drop-table

  96 | -- Running upgrade 0465_delete_broadcast_data -> 0466_delete_broadcast_tables
  97 |
  98 | DROP TABLE service_broadcast_settings;

  note: Dropping a table may break existing clients.

/tmp/upgrade.sql:100:2: warning: ban-drop-table

  100 | DROP TABLE broadcast_channel_types;

  note: Dropping a table may break existing clients.

/tmp/upgrade.sql:102:2: warning: ban-drop-table

  102 | DROP TABLE broadcast_provider_message_number;

  note: Dropping a table may break existing clients.

/tmp/upgrade.sql:104:2: warning: ban-drop-table

  104 | DROP TABLE broadcast_provider_message;

  note: Dropping a table may break existing clients.

/tmp/upgrade.sql:106:2: warning: ban-drop-table

  106 | DROP TABLE broadcast_event;

  note: Dropping a table may break existing clients.

/tmp/upgrade.sql:108:2: warning: ban-drop-table

  108 | DROP TABLE broadcast_provider_message_status_type;

  note: Dropping a table may break existing clients.

/tmp/upgrade.sql:110:2: warning: ban-drop-table

  110 | DROP TABLE broadcast_message;

  note: Dropping a table may break existing clients.

/tmp/upgrade.sql:112:2: warning: ban-drop-table

  112 | DROP TABLE broadcast_provider_types;

  note: Dropping a table may break existing clients.

/tmp/upgrade.sql:114:2: warning: ban-drop-table

  114 | DROP TABLE service_broadcast_provider_restriction;

  note: Dropping a table may break existing clients.

/tmp/upgrade.sql:116:2: warning: ban-drop-table

  116 | DROP TABLE broadcast_status_type;

  note: Dropping a table may break existing clients.

/tmp/upgrade.sql:118:2: warning: ban-drop-column

  118 | ALTER TABLE templates DROP COLUMN broadcast_data;

  note: Dropping a column may break existing clients.

/tmp/upgrade.sql:120:2: warning: ban-drop-column

  120 | ALTER TABLE templates_history DROP COLUMN broadcast_data;

  note: Dropping a column may break existing clients.

/tmp/upgrade.sql:136:2: warning: changing-column-type

  136 | ALTER TABLE permissions ALTER COLUMN permission TYPE permission_types USING permission::text::permission_types;

  note: Requires an ACCESS EXCLUSIVE lock on the table which blocks reads.
  note: Changing the type may break existing clients.

/tmp/upgrade.sql:164:2: warning: changing-column-type

  164 | ALTER TABLE templates ALTER COLUMN template_type TYPE template_type USING template_type::text::template_type;

  note: Requires an ACCESS EXCLUSIVE lock on the table which blocks reads.
  note: Changing the type may break existing clients.

/tmp/upgrade.sql:166:2: warning: changing-column-type

  166 | ALTER TABLE templates_history ALTER COLUMN template_type TYPE template_type USING template_type::text::template_type;

  note: Requires an ACCESS EXCLUSIVE lock on the table which blocks reads.
  note: Changing the type may break existing clients.

/tmp/upgrade.sql:168:2: warning: changing-column-type

  168 | ALTER TABLE service_contact_list ALTER COLUMN template_type TYPE template_type USING template_type::text::template_type;

  note: Requires an ACCESS EXCLUSIVE lock on the table which blocks reads.
  note: Changing the type may break existing clients.

risicle · 2024-10-04T09:51:53Z

test_current_alembic_head is also unhappy

risicle · 2024-10-04T10:05:28Z

app/models.py

@@ -1145,8 +1127,6 @@ def _as_utils_template(self):
            return PlainTextEmailTemplate(self.__dict__)
        if self.template_type == SMS_TYPE:
            return SMSMessageTemplate(self.__dict__)
-        if self.template_type == BROADCAST_TYPE:
-            return SMSMessageTemplate(self.__dict__ | {"template_type": "sms"})


Definitely no templates with this template_type in preview/staging/prod while not having a broadcasting service id? Didn't create any and then remove a service's broadcast permissions?

risicle · 2024-10-04T10:25:24Z

app/schemas.py

@@ -220,8 +220,6 @@ class ServiceSchema(BaseSchema, UUIDsAsStringsMixin):
    sms_message_limit = field_for(models.Service, "sms_message_limit", required=True)
    letter_message_limit = field_for(models.Service, "letter_message_limit", required=True)
    go_live_at = field_for(models.Service, "go_live_at", format=DATETIME_FORMAT_NO_TIMEZONE)
-    allowed_broadcast_provider = fields.Method(dump_only=True, serialize="_get_allowed_broadcast_provider")
-    broadcast_channel = fields.Method(dump_only=True, serialize="_get_broadcast_channel")


And there's not some component of ours still supplying this value in its api calls and we're not strict enough to reject it if we get unknown keys...?

risicle · 2024-10-04T10:42:55Z

tests/app/clients/test_aws_ses.py

@@ -190,7 +190,7 @@ def test_send_email_does_not_raise_AwsSesClientThrottlingSendRateException_if_no
        )


-def test_send_email_raises_other_errs_as_AwsSesClientException(mocker):


mocker should generally come after notify_api so that the teardowns happen in the right order.

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.

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

now that there's no data that depends on these, we can delete the tables completely. this was important to do as separate steps, or we could end up in a case where have have fragments of broadcast services sitting in our database but without the broadcast-specific tables, which would leave us unsure as to what those services were.

the enum dance - rename the enum to have a temp name, create a new type with the proper name that doesn't have the unwanted values, and then alter the column on the permissions table to change the type (casting the old value from old_enum -> text -> new_enum), then we drop the old type with the temp name.

we can't _just_ do the same process as 0467 because there are check constraints that depend on these. When you say "alter column template_type set type = new_type using old_type::text::new_type", it takes that "using" statement as a way of translating the prev enum into the new enum, in our case just casting to a str as an intermediate step. however, the template_type column has several check constraints against it (to say only letters can have a letter_attachment for example). Postgres isn't smart enough to take the `using` clause and apply it to these constraints. There are two ways around this: * You could create a comparator function that tells postgres "if you have to compare old_type with new_type, here's a custom function"[^1] * You can drop the old constraints and recreate them after the migration is done We've gone with the second option as it's a bit simpler and I could copy and paste from our previous scripts [^1]: https://stackoverflow.com/a/57952345 was very helpful for this

leohemsted force-pushed the remove-emergency-alerts-db-schema branch 2 times, most recently from 67a99d0 to 35c650f Compare October 1, 2024 14:55

This was referenced Oct 1, 2024

Remove emergency alerts data #4201

Merged

Remove emergency alerts code #3976

Open

leohemsted force-pushed the remove-emergency-alerts-db-schema branch from 35c650f to 8781f8a Compare October 2, 2024 09:59

risicle reviewed Oct 4, 2024

View reviewed changes

leohemsted added 9 commits October 4, 2024 14:40

dont delete service-specific things if theres no services

cad920a

remove emergency alerts code

2df11c0

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

fix flaky test

189b9b3

wip

ed4671c

leohemsted force-pushed the remove-emergency-alerts-db-schema branch from 8781f8a to ed4671c Compare October 4, 2024 13:40

leohemsted marked this pull request as draft November 4, 2024 14:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove emergency alerts db schema #4202

Remove emergency alerts db schema #4202

leohemsted commented Oct 1, 2024 •

edited

Loading

leohemsted commented Oct 1, 2024

risicle commented Oct 4, 2024

risicle Oct 4, 2024

risicle Oct 4, 2024

risicle Oct 4, 2024

		@@ -190,7 +190,7 @@ def test_send_email_does_not_raise_AwsSesClientThrottlingSendRateException_if_no
		)


		def test_send_email_raises_other_errs_as_AwsSesClientException(mocker):

Remove emergency alerts db schema #4202

Are you sure you want to change the base?

Remove emergency alerts db schema #4202

Conversation

leohemsted commented Oct 1, 2024 • edited Loading

leohemsted commented Oct 1, 2024

risicle commented Oct 4, 2024

risicle Oct 4, 2024

Choose a reason for hiding this comment

risicle Oct 4, 2024

Choose a reason for hiding this comment

risicle Oct 4, 2024

Choose a reason for hiding this comment

leohemsted commented Oct 1, 2024 •

edited

Loading