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.

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

feat(team-workflow): Update bulk_subscribe to accept teams #55943

Merged
merged 10 commits into from
Sep 13, 2023
34 changes: 28 additions & 6 deletions src/sentry/models/groupsubscription.py
Original file line number Diff line number Diff line change
Expand Up @@ -82,22 +82,26 @@ def subscribe_actor(
else:
# subscribe the members of the team
team_users_ids = list(actor.member_set.values_list("user_id", flat=True))
return self.bulk_subscribe(group, team_users_ids, reason)
return self.bulk_subscribe(group=group, user_ids=team_users_ids, reason=reason)

raise NotImplementedError("Unknown actor type: %r" % type(actor))

def bulk_subscribe(
self,
group: Group,
user_ids: Iterable[int],
user_ids: Iterable[int] | None = None,
team_ids: Iterable[int] | None = None,
reason: int = GroupSubscriptionReason.unknown,
) -> bool:
"""
Subscribe a list of user ids to an issue, but only if the users are not explicitly
Subscribe a list of user ids and/or teams to an issue, but only if the users/teams are not explicitly
unsubscribed.
"""
# Unique the IDs.
user_ids = set(user_ids)
user_ids = set(user_ids) if user_ids else set()

# Unique the teams.
team_ids = set(team_ids) if team_ids else set()

# 5 retries for race conditions where
# concurrent subscription attempts cause integrity errors
Expand All @@ -117,10 +121,28 @@ def bulk_subscribe(
is_active=True,
reason=reason,
)
for user_id in user_ids
if user_id not in existing_subscriptions
for user_id in user_ids.difference(existing_subscriptions)
]

existing_team_subscriptions = set(
isabellaenriquez marked this conversation as resolved.
Show resolved Hide resolved
GroupSubscription.objects.filter(
team_id__in=team_ids, group=group, project=group.project
).values_list("team_id", flat=True)
)

subscriptions.extend(
[
GroupSubscription(
team_id=team_id,
group=group,
project=group.project,
is_active=True,
reason=reason,
)
for team_id in team_ids.difference(existing_team_subscriptions)
]
)

try:
with transaction.atomic(router.db_for_write(GroupSubscription)):
self.bulk_create(subscriptions)
Expand Down
56 changes: 50 additions & 6 deletions tests/sentry/models/test_groupsubscription.py
Original file line number Diff line number Diff line change
Expand Up @@ -42,21 +42,26 @@ def test_bulk(self):
group = self.create_group()

user_ids = []
for i in range(20):
team_ids = []
for _ in range(20):
user = self.create_user()
user_ids.append(user.id)
team = self.create_team()
team_ids.append(team.id)

GroupSubscription.objects.bulk_subscribe(group=group, user_ids=user_ids)
GroupSubscription.objects.bulk_subscribe(group=group, user_ids=user_ids, team_ids=team_ids)

assert len(GroupSubscription.objects.filter(group=group)) == 20
assert len(GroupSubscription.objects.filter(group=group)) == 40

one_more = self.create_user()
user_ids.append(one_more.id)

team_ids.append(self.create_team().id)

# should not error
GroupSubscription.objects.bulk_subscribe(group=group, user_ids=user_ids)
GroupSubscription.objects.bulk_subscribe(group=group, user_ids=user_ids, team_ids=team_ids)

assert len(GroupSubscription.objects.filter(group=group)) == 21
assert len(GroupSubscription.objects.filter(group=group)) == 42

def test_bulk_dupes(self):
group = self.create_group()
Expand All @@ -67,9 +72,48 @@ def test_bulk_dupes(self):
user_ids.append(user.id)
user_ids.append(user.id)

team_ids = []

team = self.create_team()
team_ids.append(team.id)
team_ids.append(team.id)

GroupSubscription.objects.bulk_subscribe(group=group, user_ids=user_ids, team_ids=team_ids)

assert len(GroupSubscription.objects.filter(group=group)) == 2

def test_bulk_user_on_team(self):
"""
Test that ensures bulk_subscribe subscribes users and teams individually, even if one of those users is part of one of those teams.
Copy link
Member

Choose a reason for hiding this comment

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

great comment!

"""
group = self.create_group()
team = self.create_team()
user = self.create_user()
self.create_member(user=user, organization=self.organization, role="member", teams=[team])

team_ids = [team.id]
user_ids = [user.id]

GroupSubscription.objects.bulk_subscribe(group=group, user_ids=user_ids, team_ids=team_ids)

assert len(GroupSubscription.objects.filter(group=group)) == 2

def test_bulk_users_xor_team(self):
"""
Test that ensures bulk_subscribe can be called with just a list of user IDs or just a list of teams; it does not need both.
"""
group = self.create_group()
user = self.create_user()
team = self.create_team()

user_ids = [user.id]
team_ids = [team.id]

# should not error
GroupSubscription.objects.bulk_subscribe(group=group, user_ids=user_ids)

assert len(GroupSubscription.objects.filter(group=group)) == 1
# should not error
GroupSubscription.objects.bulk_subscribe(group=group, team_ids=team_ids)

def test_actor_user(self):
group = self.create_group()
Expand Down
Loading