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

feat: convert organization.general_log_channel_id to organization.general_log_slack_channel #5191

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

feat: convert organization.general_log_channel_id to organization.general_log_slack_channel #5191

wants to merge 4 commits into from

Conversation

joeyorlando
Copy link
Contributor

@joeyorlando joeyorlando commented Oct 17, 2024 •
edited
Loading

What this PR does

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

Right now general_log_channel_id is just a string value representing the Slack Channel ID (ex. C043HQ70QMB). This PR migrates this instead to be a foreign key relationship on the slack_slackchannel table and updates all references to general_log_channel_id.

Tested migrations locally:

Operations to perform:
  Apply all migrations: [redacted secret grafana-admin-creds:admin-user], alerts, auth, auth_token, base, contenttypes, email, exotel, fcm_django, google, heartbeat, labels, mobile_app, oss_installation, phone_notifications, schedules, sessions, slack, social_django, telegram, twilioapp, user_management, webhooks, zvonok
Running migrations:
  Applying user_management.0024_organization_general_log_slack_channel... OK
source=engine:app google_trace_id=none logger=apps.user_management.migrations.0025_auto_20241017_1919 Starting migration to populate general_log_slack_channel field.
source=engine:app google_trace_id=none logger=apps.user_management.migrations.0025_auto_20241017_1919 Total organizations to process: 1
source=engine:app google_trace_id=none logger=apps.user_management.migrations.0025_auto_20241017_1919 Organization 1 updated with SlackChannel 2 (slack_id: C043LL6RTS7).
source=engine:app google_trace_id=none logger=apps.user_management.migrations.0025_auto_20241017_1919 Finished migration. Total organizations processed: 1. Organizations updated: 1. Missing SlackChannels: 0.
  Applying user_management.0025_auto_20241017_1919... OK

Future incoming PRs

  • Drop Organization.general_log_channel_id column
  • Migrate ChannelFilter.slack_channel_id and ResolutionNoteSlackMessage.slack_channel_id to use foreign key relationships

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 Oct 17, 2024
@joeyorlando joeyorlando requested a review from a team as a code owner October 17, 2024 19:35
@joeyorlando joeyorlando changed the title feat: convert organization.general_log_channel_id to organization.general_log_slack_channel feat: convert organization.general_log_channel_id to organization.general_log_slack_channel Oct 17, 2024
Ferril
Ferril reviewed Oct 18, 2024
View reviewed changes
engine/apps/user_management/models/organization.py
general_log_channel_id = models.CharField(max_length=100, null=True, default=None)

general_log_slack_channel = models.ForeignKey(
Copy link
Member

Choose a reason for hiding this comment

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

nit: suggest renaming this field to default_slack_channel or general_slack_channel. wdyt?

Ferril
Ferril reviewed Oct 18, 2024
View reviewed changes
engine/apps/user_management/migrations/0025_auto_20241017_1919.py
slack_channel = SlackChannel.objects.get(slack_id=slack_id, slack_team_identity=slack_team_identity)

org.general_log_slack_channel = slack_channel
org.save(update_fields=['general_log_slack_channel'])
Copy link
Member

Choose a reason for hiding this comment

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

I'd propose to use bulk_update here

Ferril
Ferril reviewed Oct 18, 2024
View reviewed changes
engine/apps/slack/tests/test_scenario_steps/test_distribute_alerts.py
organization = make_organization(
slack_team_identity=slack_team_identity, general_log_channel_id="DEFAULT_CHANNEL_ID"
)
slack_channel = make_slack_channel(slack_team_identity)
Copy link
Member

Choose a reason for hiding this comment

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

this should fix failing test (channel slack id is checked below (line 111)):

Suggested change
slack_channel = make_slack_channel(slack_team_identity)
slack_channel = make_slack_channel(slack_team_identity, slack_id="DEFAULT_CHANNEL_ID")

🙏

Ferril
Ferril reviewed Oct 18, 2024
View reviewed changes
engine/apps/api/serializers/organization.py
return None
try:
channel = obj.slack_team_identity.get_cached_channels().get(slack_id=obj.general_log_channel_id)
channel = slack_team_identity.get_cached_channels().get(slack_id=org_general_log_channel_id)
Copy link
Member

Choose a reason for hiding this comment

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

can we do channel = obj.general_log_slack_channel here instead of slack_team_identity.get_cached_channels().get? 🤔

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

@Ferril Ferril Ferril 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 @Ferril