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.

Sign up for GitHub

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

Jump to bottom

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

Open
wants to merge 9 commits into
base: dev
Choose a base branch
from
Open

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

wants to merge 9 commits into from

Conversation

joeyorlando
Copy link
Contributor

@joeyorlando joeyorlando commented Nov 22, 2024 •
edited
Loading

What this PR does

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

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

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.

@joeyorlando 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
joeyorlando
joeyorlando commented Nov 22, 2024
View reviewed changes
engine/apps/slack/models/slack_message.py
Comment on lines -88 to -89
# 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:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.channel.slack_team_identity instead; see comments above)

joeyorlando
joeyorlando commented Nov 22, 2024
View reviewed changes
engine/apps/slack/models/slack_message.py
Comment on lines -77 to -80
logger.warning(
f"SlackMessage (pk: {self.pk}) fields _slack_team_identity and organization is None. "
f"It is strange!"
)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
joeyorlando commented Nov 22, 2024
View reviewed changes
engine/apps/slack/migrations/0006_rename_channel_id_slackmessage__channel_id_and_more.py Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

main change

engine/apps/slack/migrations/0007_migrate_slackmessage_channel_id.py
Comment on lines +46 to +58
elif db_type == settings.DATABASE_TYPES.MYSQL:
sql = f"""
UPDATE
{SlackMessage._meta.db_table} AS sm
JOIN {SlackChannel._meta.db_table} AS sc
ON sc.slack_id = sm._channel_id
AND sc.slack_team_identity_id = sm.slack_team_identity
SET
sm.channel_id = sc.id
WHERE
sm._channel_id IS NOT NULL
AND sm.slack_team_identity IS NOT NULL;
"""
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

main change. Added queries for sqlite/postgres here to avoid #5244 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: I'm planning on running this SQL statement against a prod clone just to get an idea of performance before merging this PR

joeyorlando
joeyorlando commented Nov 22, 2024
View reviewed changes
engine/apps/slack/models/slack_message.py

def send_slack_notification(self, user, alert_group, notification_policy):
def send_slack_notification(self, user: "User", notification_policy: "UserNotificationPolicy") -> None:
Copy link
Contributor Author

@joeyorlando joeyorlando Nov 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

given that this is an instance method (only used here), self.alert_group already is a thing, so I don't think we need to re-pass in alert_group?

joeyorlando
joeyorlando commented Nov 22, 2024
View reviewed changes
engine/apps/schedules/models/shift_swap_request.py
Comment on lines +167 to +172
def slack_channel(self) -> typing.Optional["SlackChannel"]:
"""
This is only set if the schedule associated with the shift swap request
has a Slack channel configured for it.
"""
return self.schedule.slack_channel
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added for this

@joeyorlando joeyorlando marked this pull request as ready for review November 22, 2024 17:09
@joeyorlando joeyorlando requested a review from a team as a code owner November 22, 2024 17:09
vadimkerr
vadimkerr reviewed Nov 22, 2024
View reviewed changes
engine/apps/slack/models/slack_message.py
Comment on lines +41 to +47
channel = models.ForeignKey(
"slack.SlackChannel", on_delete=models.CASCADE, null=True, default=None, related_name="slack_messages"
)
"""
TODO: once we've migrated the data in `_channel_id` to this field, set `null=False`
as we should always have a `channel` associated with a message
"""
Copy link
Member

@vadimkerr vadimkerr Nov 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what if Slack channel gets deleted (for example here)? will it still be possible to use SlackMessage.permalink to redirect users to Slack?

on_delete=models.CASCADE this means if alert group was posted to #alerts and then #alerts is deleted, Slack messages will be deleted and we will not be able to reference them (for things like Slack permalink, resolution notes, etc.). I think we don't want that right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good question! I was also questioning what the right on_delete would be here. My thought was that if you delete a Slack channel.. everything inside the channel vanishes. So permalinks/resolution notes/etc are no longer relevant for those SlackMessages (but maybe I'm overlooking something?)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vadimkerr vadimkerr vadimkerr left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
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
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@joeyorlando @vadimkerr