From e69b158ed70eadd8004227c0b14da51a8d05202f Mon Sep 17 00:00:00 2001 From: Isabella Enriquez <45607721+isabellaenriquez@users.noreply.github.com> Date: Tue, 12 Sep 2023 14:31:00 -0400 Subject: [PATCH] feat(team-workflow): Add subscription for team in `GroupSubscription` (#55825) Updates `subscribe_actor` to no longer read all the team's users and instead subscribe the Team itself if the team-workflow-notifications feature flag is enabled. Also updates `subscribe` so that it can accept a Team as a subscriber, not just Users or RpcUsers. --------- Co-authored-by: getsantry[bot] <66042841+getsantry[bot]@users.noreply.github.com> --- src/sentry/api/endpoints/group_notes.py | 2 +- src/sentry/api/helpers/group_index/update.py | 4 +- src/sentry/issues/status_change.py | 2 +- src/sentry/models/groupsubscription.py | 39 +++++++++++++------ src/sentry/receivers/releases.py | 2 +- tests/sentry/models/test_groupsubscription.py | 30 +++++++++++++- 6 files changed, 60 insertions(+), 19 deletions(-) diff --git a/src/sentry/api/endpoints/group_notes.py b/src/sentry/api/endpoints/group_notes.py index 3d2ce30cffe5b2..90edcf1d35500f 100644 --- a/src/sentry/api/endpoints/group_notes.py +++ b/src/sentry/api/endpoints/group_notes.py @@ -66,7 +66,7 @@ def post(self, request: Request, group) -> Response: ) GroupSubscription.objects.subscribe( - group=group, user=request.user, reason=GroupSubscriptionReason.comment + group=group, subscriber=request.user, reason=GroupSubscriptionReason.comment ) mentioned_users = extract_user_ids_from_mentions(group.organization.id, mentions) diff --git a/src/sentry/api/helpers/group_index/update.py b/src/sentry/api/helpers/group_index/update.py index 0ebd570785e073..8bcc03508f3f06 100644 --- a/src/sentry/api/helpers/group_index/update.py +++ b/src/sentry/api/helpers/group_index/update.py @@ -121,7 +121,7 @@ def self_subscribe_and_assign_issue( # representation of current user if acting_user: GroupSubscription.objects.subscribe( - user=acting_user, group=group, reason=GroupSubscriptionReason.status_change + subscriber=acting_user, group=group, reason=GroupSubscriptionReason.status_change ) if self_assign_issue == "1" and not group.assignee_set.exists(): @@ -736,7 +736,7 @@ def handle_is_bookmarked( user_id=acting_user.id if acting_user else None, ) GroupSubscription.objects.subscribe( - user=acting_user, group=group, reason=GroupSubscriptionReason.bookmark + subscriber=acting_user, group=group, reason=GroupSubscriptionReason.bookmark ) elif is_bookmarked is False: GroupBookmark.objects.filter( diff --git a/src/sentry/issues/status_change.py b/src/sentry/issues/status_change.py index 7c869582cc6ca5..84ce428fc5d1c3 100644 --- a/src/sentry/issues/status_change.py +++ b/src/sentry/issues/status_change.py @@ -109,7 +109,7 @@ def handle_status_update( if not is_bulk: if acting_user: GroupSubscription.objects.subscribe( - user=acting_user, + subscriber=acting_user, group=group, reason=GroupSubscriptionReason.status_change, ) diff --git a/src/sentry/models/groupsubscription.py b/src/sentry/models/groupsubscription.py index 08f355707e7dce..de92ac5b0fe9f3 100644 --- a/src/sentry/models/groupsubscription.py +++ b/src/sentry/models/groupsubscription.py @@ -34,22 +34,33 @@ class GroupSubscriptionManager(BaseManager): def subscribe( self, group: Group, - user: User | RpcUser, + subscriber: User | RpcUser | Team, reason: int = GroupSubscriptionReason.unknown, ) -> bool: """ - Subscribe a user to an issue, but only if the user has not explicitly + Subscribe a user or team to an issue, but only if that user or team has not explicitly unsubscribed. """ + from sentry.models import Team, User + try: with transaction.atomic(router.db_for_write(GroupSubscription)): - self.create( - user_id=user.id, - group=group, - project=group.project, - is_active=True, - reason=reason, - ) + if isinstance(subscriber, User) or isinstance(subscriber, RpcUser): + self.create( + user_id=subscriber.id, + group=group, + project=group.project, + is_active=True, + reason=reason, + ) + elif isinstance(subscriber, Team): + self.create( + team=subscriber, + group=group, + project=group.project, + is_active=True, + reason=reason, + ) except IntegrityError: pass return True @@ -60,14 +71,18 @@ def subscribe_actor( actor: Union[Team, User, RpcUser], reason: int = GroupSubscriptionReason.unknown, ) -> Optional[bool]: + from sentry import features from sentry.models import Team, User if isinstance(actor, RpcUser) or isinstance(actor, User): return self.subscribe(group, actor, reason) if isinstance(actor, Team): - # 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) + if features.has("organizations:team-workflow-notifications", group.organization, actor): + return self.subscribe(group, actor, reason) + 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) raise NotImplementedError("Unknown actor type: %r" % type(actor)) diff --git a/src/sentry/receivers/releases.py b/src/sentry/receivers/releases.py index 3f4c7d7acf5264..59bac71f0bac6c 100644 --- a/src/sentry/receivers/releases.py +++ b/src/sentry/receivers/releases.py @@ -141,7 +141,7 @@ def resolved_in_commit(instance, created, **kwargs): # subscribe every user for user in user_list: GroupSubscription.objects.subscribe( - user=user, + subscriber=user, group=group, reason=GroupSubscriptionReason.status_change, ) diff --git a/tests/sentry/models/test_groupsubscription.py b/tests/sentry/models/test_groupsubscription.py index ff63f91ec744f5..9f5e5b5ddd36b8 100644 --- a/tests/sentry/models/test_groupsubscription.py +++ b/tests/sentry/models/test_groupsubscription.py @@ -12,6 +12,7 @@ from sentry.services.hybrid_cloud.user.service import user_service from sentry.silo import SiloMode from sentry.testutils.cases import TestCase +from sentry.testutils.helpers.features import with_feature from sentry.testutils.silo import assume_test_silo_mode, region_silo_test from sentry.types.integrations import ExternalProviders @@ -21,13 +22,21 @@ class SubscribeTest(TestCase): def test_simple(self): group = self.create_group() user = self.create_user() + team = self.create_team() - GroupSubscription.objects.subscribe(group=group, user=user) + GroupSubscription.objects.subscribe(group=group, subscriber=user) assert GroupSubscription.objects.filter(group=group, user_id=user.id).exists() # should not error - GroupSubscription.objects.subscribe(group=group, user=user) + GroupSubscription.objects.subscribe(group=group, subscriber=user) + + GroupSubscription.objects.subscribe(group=group, subscriber=team) + + assert GroupSubscription.objects.filter(group=group, team=team).exists() + + # should not error + GroupSubscription.objects.subscribe(group=group, subscriber=team) def test_bulk(self): group = self.create_group() @@ -88,6 +97,23 @@ def test_actor_team(self): # should not error GroupSubscription.objects.subscribe_actor(group=group, actor=team) + @with_feature("organizations:team-workflow-notifications") + def test_subscribe_team(self): + org = self.create_organization() + group = self.create_group() + user = self.create_user(email="foo@example.com") + team = self.create_team(organization=org) + self.create_member(user=user, organization=org, role="owner", teams=[team]) + + GroupSubscription.objects.subscribe_actor(group=group, actor=team) + + assert not GroupSubscription.objects.filter(group=group, user_id=user.id).exists() + + assert GroupSubscription.objects.filter(group=group, team=team).exists() + + # should not error + GroupSubscription.objects.subscribe_actor(group=group, actor=team) + @region_silo_test(stable=True) class GetParticipantsTest(TestCase):