feat(assignee): Allow GroupAssignee to also reference a Team #7080

mattrobenolt · 2018-01-26T01:43:20Z

I want to get this schema change out of the way now before affecting the rest of the API changes since this will be a tad hairy in production.

evanpurkhiser

Looks good. Curious what the deploy process in prod will look like for this.

mattrobenolt · 2018-01-26T01:45:19Z

src/sentry/models/groupassignee.py

    def assign(self, group, assigned_to, acting_user=None):
+        from sentry.models import User, Team
+
+        if isinstance(assigned_to, User):


@tkaemming I want your opinion on this.

I originally wanted to do some abstraction of Actor, but turns out, this Actor also would need to encapsulate the same logic, and in this case GroupAssignee is just this actor abstraction. I also chose to do this with two different columns, user_id and team_id instead of an alternative Django "generic" FK style, where there's a column for model and object_id, this allows us to still use actual ForeignKeys, which being a tad more rigid since it can't FK against anything, which is a good thing here.

mattrobenolt · 2018-01-26T01:47:55Z

...ry/south_migrations/0386_auto__add_field_groupassignee_team__chg_field_groupassignee_user.py

+
+    # Flag to indicate if this migration is too risky
+    # to run online and needs to be coordinated for offline
+    is_dangerous = True


I am 100% doing this by hand since the SQL that South spits out is not great at all.

> sentry:0386_auto__add_field_groupassignee_team__chg_field_groupassignee_user = ALTER TABLE "sentry_groupasignee" ADD COLUMN "team_id" bigint NULL; [] = ALTER TABLE "sentry_groupasignee" ALTER COLUMN "team_id" TYPE bigint, ALTER COLUMN "team_id" DROP NOT NULL, ALTER COLUMN "team_id" DROP DEFAULT; [] [...] = ALTER TABLE "sentry_groupasignee" DROP CONSTRAINT "user_id_refs_id_f4dcb8d1" [] = ALTER TABLE "sentry_groupasignee" ALTER COLUMN "user_id" TYPE integer, ALTER COLUMN "user_id" DROP NOT NULL, ALTER COLUMN "user_id" DROP DEFAULT; [] = ALTER TABLE "sentry_groupasignee" ADD CONSTRAINT "user_id_refs_id_f4dcb8d1" FOREIGN KEY ("user_id") REFERENCES "auth_user" ("id") DEFERRABLE INITIALLY DEFERRED; [] = ALTER TABLE "sentry_groupasignee" ADD CONSTRAINT "team_id_refs_id_5b32ca44" FOREIGN KEY ("team_id") REFERENCES "sentry_team" ("id") DEFERRABLE INITIALLY DEFERRED; [] = CREATE INDEX "sentry_groupasignee_team_id" ON "sentry_groupasignee" ("team_id"); []

mattrobenolt · 2018-01-26T01:48:49Z

src/sentry/models/groupassignee.py

+    __repr__ = sane_repr('group_id', 'user_id', 'team_id')
+
+    def save(self, *args, **kwargs):
+        assert self.user_id or self.team_id


I'm thinking about applying a migration as well to put a real constraint on this to enforce it at the database.

+1 on making this a defined check constraint; I'd also strongly consider ensuring that only one of the two is set (require of the fields to be set and only allow one of the fields to be set) both here as well as in the check constraint.

I have no idea how we'd hook these check constraints in to to run correctly during tests… add SQL manually to the migration somehow?

ALTER TABLE sentry_groupasignee ADD CONSTRAINT require_team_or_user_but_not_both CHECK ( NOT (team_id IS NOT NULL AND user_id IS NOT NULL) AND NOT (team_id IS NULL AND user_id IS NULL) );

😎

Added here: https://github.com/getsentry/sentry/blob/8b29ec7dc85367a73ced7f10c8116f7f3f1286a1/src/sentry/south_migrations/0387_auto__add_field_groupassignee_team__chg_field_groupassignee_user.py#L33-L38

Get outta here.

ghost · 2018-01-26T01:53:16Z

	2 Warnings
⚠️	You should update CHANGES due to the size of this PR
⚠️	PR includes migrations

Migration Checklist

new columns need to be nullable (unless table is new)
migration with any new index needs to be done concurrently
data migrations should not be done inside a transaction
before merging, check to make sure there aren't conflicting migration ids

Generated by 🚫 danger

tkaemming · 2018-01-26T02:16:20Z

src/sentry/models/groupassignee.py

+            }).update(**{
+                assignee_type: assigned_to,
+                'date_added': now,
+            })


Shouldn't this null out the other type field if it's updating the row (e.g. set user to None if you're setting team and vice versa.)

Fixed in e1bfec9 and added tests.

tkaemming · 2018-01-26T02:16:55Z

src/sentry/models/groupassignee.py

+    __repr__ = sane_repr('group_id', 'user_id', 'team_id')
+
+    def save(self, *args, **kwargs):
+        assert self.user_id or self.team_id


+1 on making this a defined check constraint; I'd also strongly consider ensuring that only one of the two is set (require of the fields to be set and only allow one of the fields to be set) both here as well as in the check constraint.

I have no idea how we'd hook these check constraints in to to run correctly during tests… add SQL manually to the migration somehow?

tkaemming

LOOKS GOOD

mattrobenolt · 2018-02-02T23:57:57Z

The failure is just selenium being selenium.

mattrobenolt requested review from a team January 26, 2018 01:43

mattrobenolt added the Impact: Migration label Jan 26, 2018

evanpurkhiser previously approved these changes Jan 26, 2018

View reviewed changes

mattrobenolt commented Jan 26, 2018

View reviewed changes

mattrobenolt requested a review from a team January 26, 2018 01:50

tkaemming reviewed Jan 26, 2018

View reviewed changes

mattrobenolt force-pushed the groupassignee-team branch from d26a86c to 1a87c1f Compare February 2, 2018 23:20

feat(assignee): Allow GroupAssignee to also reference a Team

a320370

mattrobenolt force-pushed the groupassignee-team branch from 1a87c1f to 8b29ec7 Compare February 2, 2018 23:25

mattrobenolt added 2 commits February 2, 2018 15:27

Add some tests and fix broken behavior as pointed out by my buddy Ted

e1bfec9

Bring back new migration

d2f7ddf

mattrobenolt force-pushed the groupassignee-team branch from 8b29ec7 to d2f7ddf Compare February 2, 2018 23:27

tkaemming approved these changes Feb 2, 2018

View reviewed changes

mattrobenolt merged commit 4b66584 into master Feb 2, 2018

mattrobenolt deleted the groupassignee-team branch February 2, 2018 23:58

github-actions bot locked and limited conversation to collaborators Dec 22, 2020

Uh oh!

feat(assignee): Allow GroupAssignee to also reference a Team #7080

feat(assignee): Allow GroupAssignee to also reference a Team #7080

Uh oh!

Conversation

mattrobenolt commented Jan 26, 2018

Uh oh!

evanpurkhiser left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Jan 26, 2018 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Migration Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tkaemming left a comment

Choose a reason for hiding this comment

Uh oh!

mattrobenolt commented Feb 2, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ghost commented Jan 26, 2018 •

edited by ghost

Loading