From b70cd37ab2cc2f832f07dc93600f68b8de3bef5c Mon Sep 17 00:00:00 2001 From: isabellaenriquez Date: Wed, 6 Sep 2023 16:34:16 -0700 Subject: [PATCH 1/8] Subscribe the team not the actors :triumph: --- src/sentry/models/groupsubscription.py | 37 +++++++++++++++++--------- 1 file changed, 25 insertions(+), 12 deletions(-) diff --git a/src/sentry/models/groupsubscription.py b/src/sentry/models/groupsubscription.py index 3dd432930c136..4892ce3883697 100644 --- a/src/sentry/models/groupsubscription.py +++ b/src/sentry/models/groupsubscription.py @@ -34,22 +34,31 @@ 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. """ 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 +69,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)) From 3e78bf9cd0e1028820d9ba12b372cee1d93933bb Mon Sep 17 00:00:00 2001 From: isabellaenriquez Date: Wed, 6 Sep 2023 16:50:29 -0700 Subject: [PATCH 2/8] Fix tests --- src/sentry/models/groupsubscription.py | 2 ++ tests/sentry/models/test_groupsubscription.py | 4 ++-- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/src/sentry/models/groupsubscription.py b/src/sentry/models/groupsubscription.py index 4892ce3883697..4600056fa7517 100644 --- a/src/sentry/models/groupsubscription.py +++ b/src/sentry/models/groupsubscription.py @@ -41,6 +41,8 @@ def subscribe( 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)): if isinstance(subscriber, User) or isinstance(subscriber, RpcUser): diff --git a/tests/sentry/models/test_groupsubscription.py b/tests/sentry/models/test_groupsubscription.py index ff63f91ec744f..ae4f3753126d5 100644 --- a/tests/sentry/models/test_groupsubscription.py +++ b/tests/sentry/models/test_groupsubscription.py @@ -22,12 +22,12 @@ def test_simple(self): group = self.create_group() user = self.create_user() - 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) def test_bulk(self): group = self.create_group() From f12ca5b9235c2ef4b3d7392d6ab0d4a8a24cac90 Mon Sep 17 00:00:00 2001 From: "getsantry[bot]" <66042841+getsantry[bot]@users.noreply.github.com> Date: Wed, 6 Sep 2023 23:56:59 +0000 Subject: [PATCH 3/8] :knife: regenerate mypy module blocklist --- pyproject.toml | 1 + 1 file changed, 1 insertion(+) diff --git a/pyproject.toml b/pyproject.toml index 03bdcbf5afdaf..dbe6f2d8b53e6 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -561,6 +561,7 @@ module = [ "sentry.migrations.0418_add_actor_constraints", "sentry.models.artifactbundle", "sentry.models.auditlogentry", + "sentry.models.groupsubscription", "sentry.models.integrations.external_issue", "sentry.models.integrations.sentry_app", "sentry.models.integrations.sentry_app_installation", From 16996ae4c272ff7c79790b2fd2aa89b310f6bcb6 Mon Sep 17 00:00:00 2001 From: isabellaenriquez Date: Thu, 7 Sep 2023 10:14:21 -0700 Subject: [PATCH 4/8] Fix failing tests --- 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/receivers/releases.py | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/sentry/api/endpoints/group_notes.py b/src/sentry/api/endpoints/group_notes.py index 3d2ce30cffe5b..90edcf1d35500 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 0ebd570785e07..8bcc03508f3f0 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 7c869582cc6ca..84ce428fc5d1c 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/receivers/releases.py b/src/sentry/receivers/releases.py index 3f4c7d7acf526..59bac71f0bac6 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, ) From 562fbc44f5ce664ea490bd6f623cfd2a69e8ae24 Mon Sep 17 00:00:00 2001 From: "getsantry[bot]" <66042841+getsantry[bot]@users.noreply.github.com> Date: Thu, 7 Sep 2023 17:21:08 +0000 Subject: [PATCH 5/8] :knife: regenerate mypy module blocklist --- pyproject.toml | 1 - 1 file changed, 1 deletion(-) diff --git a/pyproject.toml b/pyproject.toml index dbe6f2d8b53e6..03bdcbf5afdaf 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -561,7 +561,6 @@ module = [ "sentry.migrations.0418_add_actor_constraints", "sentry.models.artifactbundle", "sentry.models.auditlogentry", - "sentry.models.groupsubscription", "sentry.models.integrations.external_issue", "sentry.models.integrations.sentry_app", "sentry.models.integrations.sentry_app_installation", From f77673f6fec0796cf61e7a73c3892ef1186571d5 Mon Sep 17 00:00:00 2001 From: isabellaenriquez Date: Thu, 7 Sep 2023 11:12:07 -0700 Subject: [PATCH 6/8] Add tests for subscribing teams --- tests/sentry/models/test_groupsubscription.py | 26 +++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/tests/sentry/models/test_groupsubscription.py b/tests/sentry/models/test_groupsubscription.py index ae4f3753126d5..9f5e5b5ddd36b 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,6 +22,7 @@ class SubscribeTest(TestCase): def test_simple(self): group = self.create_group() user = self.create_user() + team = self.create_team() GroupSubscription.objects.subscribe(group=group, subscriber=user) @@ -29,6 +31,13 @@ def test_simple(self): # should not error 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): From b283c642dbe230552614ac6d75a13f3426c1e443 Mon Sep 17 00:00:00 2001 From: isabellaenriquez Date: Tue, 12 Sep 2023 10:42:45 -0700 Subject: [PATCH 7/8] Add RpcTeam --- src/sentry/models/groupsubscription.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/sentry/models/groupsubscription.py b/src/sentry/models/groupsubscription.py index de92ac5b0fe9f..c6d836e7ee30e 100644 --- a/src/sentry/models/groupsubscription.py +++ b/src/sentry/models/groupsubscription.py @@ -23,6 +23,7 @@ from sentry.notifications.types import GroupSubscriptionReason, NotificationSettingTypes from sentry.services.hybrid_cloud.actor import RpcActor from sentry.services.hybrid_cloud.notifications import notifications_service +from sentry.services.hybrid_cloud.organization.model import RpcTeam from sentry.services.hybrid_cloud.user import RpcUser if TYPE_CHECKING: @@ -34,7 +35,7 @@ class GroupSubscriptionManager(BaseManager): def subscribe( self, group: Group, - subscriber: User | RpcUser | Team, + subscriber: User | RpcUser | Team | RpcTeam, reason: int = GroupSubscriptionReason.unknown, ) -> bool: """ @@ -53,7 +54,7 @@ def subscribe( is_active=True, reason=reason, ) - elif isinstance(subscriber, Team): + elif isinstance(subscriber, Team) or isinstance(subscriber, RpcTeam): self.create( team=subscriber, group=group, @@ -68,7 +69,7 @@ def subscribe( def subscribe_actor( self, group: Group, - actor: Union[Team, User, RpcUser], + actor: Union[Team, User, RpcTeam, RpcUser], reason: int = GroupSubscriptionReason.unknown, ) -> Optional[bool]: from sentry import features @@ -76,7 +77,7 @@ def subscribe_actor( if isinstance(actor, RpcUser) or isinstance(actor, User): return self.subscribe(group, actor, reason) - if isinstance(actor, Team): + if isinstance(actor, Team) or isinstance(actor, RpcTeam): if features.has("organizations:team-workflow-notifications", group.organization, actor): return self.subscribe(group, actor, reason) else: From bb9d84cbee37bff936781d735c614674cb7efe8a Mon Sep 17 00:00:00 2001 From: isabellaenriquez Date: Tue, 12 Sep 2023 11:01:25 -0700 Subject: [PATCH 8/8] nvm take rpcteam out :) --- src/sentry/models/groupsubscription.py | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/sentry/models/groupsubscription.py b/src/sentry/models/groupsubscription.py index c6d836e7ee30e..de92ac5b0fe9f 100644 --- a/src/sentry/models/groupsubscription.py +++ b/src/sentry/models/groupsubscription.py @@ -23,7 +23,6 @@ from sentry.notifications.types import GroupSubscriptionReason, NotificationSettingTypes from sentry.services.hybrid_cloud.actor import RpcActor from sentry.services.hybrid_cloud.notifications import notifications_service -from sentry.services.hybrid_cloud.organization.model import RpcTeam from sentry.services.hybrid_cloud.user import RpcUser if TYPE_CHECKING: @@ -35,7 +34,7 @@ class GroupSubscriptionManager(BaseManager): def subscribe( self, group: Group, - subscriber: User | RpcUser | Team | RpcTeam, + subscriber: User | RpcUser | Team, reason: int = GroupSubscriptionReason.unknown, ) -> bool: """ @@ -54,7 +53,7 @@ def subscribe( is_active=True, reason=reason, ) - elif isinstance(subscriber, Team) or isinstance(subscriber, RpcTeam): + elif isinstance(subscriber, Team): self.create( team=subscriber, group=group, @@ -69,7 +68,7 @@ def subscribe( def subscribe_actor( self, group: Group, - actor: Union[Team, User, RpcTeam, RpcUser], + actor: Union[Team, User, RpcUser], reason: int = GroupSubscriptionReason.unknown, ) -> Optional[bool]: from sentry import features @@ -77,7 +76,7 @@ def subscribe_actor( if isinstance(actor, RpcUser) or isinstance(actor, User): return self.subscribe(group, actor, reason) - if isinstance(actor, Team) or isinstance(actor, RpcTeam): + if isinstance(actor, Team): if features.has("organizations:team-workflow-notifications", group.organization, actor): return self.subscribe(group, actor, reason) else: