refactor `SlackMessage.channel_id` (`CHAR` field) to `SlackMessage.channel` (foreign key relationship) #5292

joeyorlando · 2024-11-22T11:48:31Z

What this PR does

Related to https://github.com/grafana/oncall-private/issues/2947

NOTE

This PR introduces steps 1 and 2 of the 3 part migration proposed here. Step 3, swapping reads to be from the new-column and dropping dual-writes, will be done in a future PR/release.

I’m tackling this work now because ultimately I want to move AlertReceiveChannel.rate_limited_in_slack_at to SlackChannel.rate_limited_at , but first I sorta need to refactor SlackMessage.channel_id from a CHAR field to a foreign key relationship (because in the spots where we touch Slack rate limiting, like here for example, we only have slack_message.channel_id, which means I need to do extra queries to fetch the appropriate SlackChannel to then be able to get/set SlackChannel.rate_limited_at

Other minor stuffs:

it also prepares us to drop SlackMessage._slack_team_identity. We already have a @property of SlackMessage.slack_team_identity (which previously had some hacky logic). I've refactored SlackMessage.slack_team_identity to simply point to self.organization.slack_team_identity + updated our code to stop setting SlackMessage._slack_team_identity (will drop this column in future release)

Checklist

Unit, integration, and e2e (if applicable) tests updated
Documentation added (or pr:no public docs PR label added if not required)
Added the relevant release notes label (see labels prefixed w/ release:). These labels dictate how your PR will
show up in the autogenerated release notes.

…tionship

joeyorlando · 2024-11-22T16:14:14Z

engine/apps/slack/models/slack_message.py

-        # Don't send request for permalink if there is no slack_team_identity or slack token has been revoked
-        if self.cached_permalink or not self.slack_team_identity or self.slack_team_identity.detected_token_revoked:


I verified (in production) and don't see any records where slack_team_identity is null for SlackMessage (fwiw, I think this field should eventually be dropped from this model and we should use slack_message.organization.slack_team_identity instead; see comments above)

joeyorlando · 2024-11-22T16:15:05Z

engine/apps/slack/models/slack_message.py

-                logger.warning(
-                    f"SlackMessage (pk: {self.pk}) fields _slack_team_identity and organization is None. "
-                    f"It is strange!"
-                )


I wasn't able to find any mentions of this log message for the last 90 days + there's no git history available unfortunately for why this is here. As I am proposing to (eventually) drop slack_team_identity from this model, I propose to remove this

joeyorlando · 2024-11-26T09:51:09Z

engine/apps/slack/models/slack_message.py


-    def send_slack_notification(self, user, alert_group, notification_policy):


I tried removing alert_group here (since self.alert_group is a thing..) but it appears that when notifying a user in the Alert Group message thread, the second SlackMessage that gets persisted (the one that represents the thread notification message) has a null alert_group (and I believe it had null for another field such as organization?)

vstpme · 2024-11-26T10:11:12Z

engine/apps/slack/migrations/0006_rename_channel_id_slackmessage__channel_id_and_more.py

+# NOTE: the foreign key checks are disabled and re-enabled in this migration
+# see the following resources as to why
+#
+# https://arc.net/l/quote/hgqztayk


https://arc.net/l/quote/... TIL 😎

YIL (yesterday-I-learned) as well 😄:

"Copy Link to Highlight" in Arc seems to have its own fancy "URL shortener"

…ated_at`

# What this PR does Related to #5287 Few random "clean-ups", type improvements, etc. Additionally, fixes a change made in #5292; we should wait to read from `slack_message.channel.slack_id`, until we've performed the data-migration mentioned in that PR (in the mean-time we should continue to use `slack_message._channel_id`). ## Checklist - [x] Unit, integration, and e2e (if applicable) tests updated - [x] Documentation added (or `pr:no public docs` PR label added if not required) - [x] Added the relevant release notes label (see labels prefixed w/ `release:`). These labels dictate how your PR will show up in the autogenerated release notes.

# What this PR does - As a follow-up to #5292, and now that `SlackMessage.channel` has been migrated via [`engine/apps/slack/migrations/0007_migrate_slackmessage_channel_id.py`](https://github.com/grafana/oncall/pull/5292/files#diff-8aebe133401715a4262baad9b2c5c9fc59367c18d6bd6ac2b3c462fcdabafd66), this PR removes reads/writes from `SlackMessage._channel_id` to `SlackMessage.channel`. In a separate PR I will focus on dropping that column from the model/db. - Drops `SlackMessage.active_update_task_id`. There're zero references to this column in the codebase. - Removes two Django `manage.py` commands that're no longer needed: - `engine/engine/management/commands/alertmanager_v2_migrate.py` (and it's associated tests) - `engine/engine/management/commands/batch_migrate_slack_message_channel.py` ## Checklist - [x] Unit, integration, and e2e (if applicable) tests updated - [x] Documentation added (or `pr:no public docs` PR label added if not required) - [x] Added the relevant release notes label (see labels prefixed w/ `release:`). These labels dictate how your PR will show up in the autogenerated release notes.

WIP

b3681e8

joeyorlando had a problem deploying to github-pages November 22, 2024 11:48 — with GitHub Actions Failure

joeyorlando had a problem deploying to github-pages November 22, 2024 11:49 — with GitHub Actions Failure

joeyorlando added pr:no public docs Added to a PR that does not require public documentation updates release:patch PR will be added to "Other Changes" section of release notes labels Nov 22, 2024

wip

f22d7a0

joeyorlando had a problem deploying to github-pages November 22, 2024 14:13 — with GitHub Actions Error

Merge branch 'dev' into jorlando/refactor-slack-messages-channel-rela…

e7bf5de

…tionship

joeyorlando temporarily deployed to github-pages November 22, 2024 14:14 — with GitHub Actions Inactive

wip

a94137b

joeyorlando temporarily deployed to github-pages November 22, 2024 14:25 — with GitHub Actions Inactive

wip

7f36f36

joeyorlando temporarily deployed to github-pages November 22, 2024 15:09 — with GitHub Actions Inactive

joeyorlando commented Nov 22, 2024

View reviewed changes

wip

bde7b00

joeyorlando had a problem deploying to github-pages November 22, 2024 16:23 — with GitHub Actions Error

make migration compatible w/ sqlite and postgres

be7efee

joeyorlando had a problem deploying to github-pages November 22, 2024 16:33 — with GitHub Actions Error

joeyorlando had a problem deploying to github-pages November 25, 2024 21:30 — with GitHub Actions Error

joeyorlando requested a review from vstpme November 25, 2024 21:37

tests + linting

7ba3cde

joeyorlando temporarily deployed to github-pages November 25, 2024 21:40 — with GitHub Actions Inactive

wip

e6a5d1e

joeyorlando temporarily deployed to github-pages November 26, 2024 09:44 — with GitHub Actions Inactive

joeyorlando commented Nov 26, 2024

View reviewed changes

vstpme approved these changes Nov 26, 2024

View reviewed changes

update batch migrator script to use id for ordering instead of `cre…

6fb318a

…ated_at`

joeyorlando had a problem deploying to github-pages November 26, 2024 10:37 — with GitHub Actions Error

linting

7515122

joeyorlando temporarily deployed to github-pages November 26, 2024 10:41 — with GitHub Actions Inactive

joeyorlando enabled auto-merge November 26, 2024 10:41

joeyorlando added this pull request to the merge queue Nov 26, 2024

Merged via the queue into dev with commit a29e35c Nov 26, 2024
26 checks passed

joeyorlando deleted the jorlando/refactor-slack-messages-channel-relationship branch November 26, 2024 11:13

This was referenced Nov 26, 2024

fix: improve Slack rate limiting logic when updating alert groups #5287

Merged

chore: random slack code cleanup #5307

Merged

joeyorlando mentioned this pull request Dec 4, 2024

chore: remove references to SlackMessage._channel_id #5325

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor `SlackMessage.channel_id` (`CHAR` field) to `SlackMessage.channel` (foreign key relationship) #5292

refactor `SlackMessage.channel_id` (`CHAR` field) to `SlackMessage.channel` (foreign key relationship) #5292

joeyorlando commented Nov 22, 2024 •

edited

Loading

joeyorlando Nov 22, 2024 •

edited

Loading

joeyorlando Nov 22, 2024

joeyorlando Nov 26, 2024

vstpme Nov 26, 2024

joeyorlando Nov 26, 2024

		# Don't send request for permalink if there is no slack_team_identity or slack token has been revoked
		if self.cached_permalink or not self.slack_team_identity or self.slack_team_identity.detected_token_revoked:


		def send_slack_notification(self, user, alert_group, notification_policy):

refactor SlackMessage.channel_id (CHAR field) to SlackMessage.channel (foreign key relationship) #5292

refactor SlackMessage.channel_id (CHAR field) to SlackMessage.channel (foreign key relationship) #5292

Conversation

joeyorlando commented Nov 22, 2024 • edited Loading

What this PR does

Checklist

joeyorlando Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

joeyorlando Nov 22, 2024

Choose a reason for hiding this comment

joeyorlando Nov 26, 2024

Choose a reason for hiding this comment

vstpme Nov 26, 2024

Choose a reason for hiding this comment

joeyorlando Nov 26, 2024

Choose a reason for hiding this comment

refactor `SlackMessage.channel_id` (`CHAR` field) to `SlackMessage.channel` (foreign key relationship) #5292

refactor `SlackMessage.channel_id` (`CHAR` field) to `SlackMessage.channel` (foreign key relationship) #5292

joeyorlando commented Nov 22, 2024 •

edited

Loading

joeyorlando Nov 22, 2024 •

edited

Loading