Browse Source

feat(team-workflow): Remove invalid teams as recipients (#58820)

This PR removes any teams that don't use a valid team provider as
recipients for notifications, falling back to notifying users
individually. Valid team providers are those that can be used to send
notifications to a team. Right now, the only valid team provider is
Slack, so we check that the team has a Slack channel linked.

--

This PR re-reverts #58656. 

Once #58807 is merged, which adds flags to `RpcOrganizationMapping`,
this feature can be safely re-merged. Previously, an issue occurred
where flags was accessed on an instance of `RpcOrganizationMapping`, so
the original PR needed to be reverted.
Isabella Enriquez 1 year ago
parent
commit
8174ebf4da

+ 50 - 0
src/sentry/notifications/helpers.py

@@ -5,8 +5,11 @@ from collections import defaultdict
 from typing import TYPE_CHECKING, Any, Iterable, Mapping, MutableMapping
 
 from django.contrib.auth.models import AnonymousUser
+from django.db.models import Subquery
 
 from sentry import features
+from sentry.models.organizationmembermapping import OrganizationMemberMapping
+from sentry.models.organizationmemberteamreplica import OrganizationMemberTeamReplica
 from sentry.notifications.defaults import (
     NOTIFICATION_SETTING_DEFAULTS,
     NOTIFICATION_SETTINGS_ALL_SOMETIMES,
@@ -32,6 +35,7 @@ from sentry.services.hybrid_cloud.notifications import RpcNotificationSetting
 from sentry.services.hybrid_cloud.user.model import RpcUser
 from sentry.types.integrations import (
     EXTERNAL_PROVIDERS,
+    PERSONAL_NOTIFICATION_PROVIDERS_AS_INT,
     ExternalProviderEnum,
     ExternalProviders,
     get_provider_enum_from_string,
@@ -715,6 +719,52 @@ def get_recipient_from_team_or_user(user_id: int | None, team_id: int | None) ->
     return recipient
 
 
+def team_is_valid_recipient(team: Team | RpcActor) -> bool:
+    """
+    A team is a valid recipient if it has a linked integration (ie. linked Slack channel)
+    for any one of the providers allowed for personal notifications.
+    """
+    from sentry.models.integrations.external_actor import ExternalActor
+
+    linked_integration = ExternalActor.objects.filter(
+        team_id=team.id,
+        provider__in=PERSONAL_NOTIFICATION_PROVIDERS_AS_INT,
+    )
+    if linked_integration:
+        return True
+    return False
+
+
+def get_team_members(team: Team | RpcActor) -> list[RpcActor]:
+    if recipient_is_team(team):  # handles type error below
+        team_id = team.id
+    else:  # team is either Team or RpcActor, so if recipient_is_team returns false it is because RpcActor has a different type
+        raise Exception(
+            "RpcActor team has ActorType %s, expected ActorType Team", team.actor_type  # type: ignore
+        )
+
+    # get organization member IDs of all members in the team
+    team_members = OrganizationMemberTeamReplica.objects.filter(team_id=team_id)
+
+    # use the first member to get the org id + determine if there are any members to begin with
+    first_member = team_members.first()
+    if not first_member:
+        return []
+    org_id = first_member.organization_id
+
+    # get user IDs for all members in the team
+    members = OrganizationMemberMapping.objects.filter(
+        organization_id=org_id,
+        organizationmember_id__in=Subquery(team_members.values("organizationmember_id")),
+    )
+
+    return [
+        RpcActor(id=user_id, actor_type=ActorType.USER)
+        for user_id in members.values_list("user_id", flat=True)
+        if user_id
+    ]
+
+
 PROVIDER_DEFAULTS: list[ExternalProviderEnum] = get_provider_defaults()
 TYPE_DEFAULTS: Mapping[
     NotificationSettingEnum, NotificationSettingsOptionEnum

+ 29 - 3
src/sentry/notifications/notificationcontroller.py

@@ -5,15 +5,19 @@ from typing import Iterable, Mapping, MutableMapping, Union
 
 from django.db.models import Q
 
+from sentry import features
 from sentry.models.notificationsettingoption import NotificationSettingOption
 from sentry.models.notificationsettingprovider import NotificationSettingProvider
+from sentry.models.organizationmapping import OrganizationMapping
 from sentry.models.team import Team
 from sentry.models.user import User
 from sentry.notifications.helpers import (
     get_default_for_provider,
+    get_team_members,
     get_type_defaults,
     recipient_is_team,
     recipient_is_user,
+    team_is_valid_recipient,
 )
 from sentry.notifications.types import (
     GroupSubscriptionStatus,
@@ -22,6 +26,7 @@ from sentry.notifications.types import (
     NotificationSettingsOptionEnum,
 )
 from sentry.services.hybrid_cloud.actor import ActorType, RpcActor
+from sentry.services.hybrid_cloud.organization_mapping.serial import serialize_organization_mapping
 from sentry.services.hybrid_cloud.user.model import RpcUser
 from sentry.types.integrations import (
     EXTERNAL_PROVIDERS_REVERSE,
@@ -54,19 +59,40 @@ class NotificationController:
 
     def __init__(
         self,
-        recipients: Iterable[RpcActor] | Iterable[Team] | Iterable[RpcUser] | Iterable[User],
+        recipients: Iterable[Recipient],
         project_ids: Iterable[int] | None = None,
         organization_id: int | None = None,
         type: NotificationSettingEnum | None = None,
         provider: ExternalProviderEnum | None = None,
     ) -> None:
-        self.recipients = recipients
         self.project_ids = project_ids
         self.organization_id = organization_id
         self.type = type
         self.provider = provider
 
-        if recipients:
+        org = (
+            serialize_organization_mapping(
+                OrganizationMapping.objects.filter(organization_id=organization_id).first()
+            )
+            if organization_id
+            else None
+        )
+        if org and features.has("organizations:team-workflow-notifications", org):
+            self.recipients: list[Recipient] = []
+            for recipient in recipients:
+                if recipient_is_team(
+                    recipient
+                ):  # this call assures the recipient type is okay (so can safely ignore below type errors)
+                    if team_is_valid_recipient(recipient):  # type: ignore
+                        self.recipients.append(recipient)
+                    else:
+                        self.recipients += get_team_members(recipient)  # type: ignore
+                else:
+                    self.recipients.append(recipient)
+        else:
+            self.recipients = list(recipients)
+
+        if self.recipients:
             query = self._get_query()
             type_filter = Q(type=self.type.value) if self.type else Q()
             provider_filter = Q(provider=self.provider.value) if self.provider else Q()

+ 6 - 0
src/sentry/types/integrations.py

@@ -96,5 +96,11 @@ def get_provider_enum_from_string(provider: str) -> ExternalProviders:
     raise InvalidProviderException("Invalid provider ${provider}")
 
 
+PERSONAL_NOTIFICATION_PROVIDERS_AS_INT = [
+    get_provider_enum_from_string(provider_name).value
+    for provider_name in PERSONAL_NOTIFICATION_PROVIDERS
+]
+
+
 class InvalidProviderException(Exception):
     pass

+ 15 - 0
tests/sentry/notifications/test_helpers.py

@@ -8,6 +8,7 @@ from sentry.notifications.helpers import (
     get_scope_type,
     get_settings_by_provider,
     get_subscription_from_attributes,
+    get_team_members,
     get_values_by_provider_by_type,
     validate,
 )
@@ -22,6 +23,7 @@ from sentry.notifications.utils import (
     get_group_settings_link,
     get_rules,
 )
+from sentry.services.hybrid_cloud.actor import RpcActor
 from sentry.testutils.cases import TestCase
 from sentry.types.integrations import ExternalProviders
 
@@ -206,3 +208,16 @@ class NotificationHelpersTest(TestCase):
             }
             for rule_detail in rule_details
         }
+
+    def test_get_team_members(self):
+        user1 = self.create_user()
+        user2 = self.create_user()
+        team1 = self.create_team()
+        team2 = self.create_team()
+        team3 = self.create_team()
+        self.create_member(organization=self.organization, teams=[team1], user=user1)
+        self.create_member(organization=self.organization, teams=[team2], user=user2)
+
+        assert get_team_members(team1) == [RpcActor.from_object(user1)]
+        assert get_team_members(team2) == [RpcActor.from_object(user2)]
+        assert get_team_members(team3) == []

+ 45 - 0
tests/sentry/notifications/test_notificationcontroller.py

@@ -9,6 +9,8 @@ from sentry.notifications.types import (
 )
 from sentry.services.hybrid_cloud.actor import ActorType, RpcActor
 from sentry.testutils.cases import TestCase
+from sentry.testutils.helpers.features import with_feature
+from sentry.testutils.helpers.slack import link_team
 from sentry.types.integrations import ExternalProviderEnum, ExternalProviders
 
 
@@ -795,3 +797,46 @@ class NotificationControllerTest(TestCase):
             type=NotificationSettingEnum.REPORTS,
         )
         assert controller.get_users_for_weekly_reports() == []
+
+    @with_feature("organizations:team-workflow-notifications")
+    def test_fallback_if_invalid_team_provider(self):
+        team = self.create_team()
+        user1 = self.create_user()
+        user2 = self.create_user()
+        self.create_member(user=user1, organization=self.organization, role="member", teams=[team])
+        self.create_member(user=user2, organization=self.organization, role="member", teams=[team])
+
+        controller = NotificationController(
+            recipients=[team],
+            organization_id=self.organization.id,
+        )
+
+        assert len(controller.recipients) == 2
+
+    @with_feature("organizations:team-workflow-notifications")
+    def test_keeps_team_as_recipient_if_valid_provider(self):
+        team = self.create_team()
+        user1 = self.create_user()
+        user2 = self.create_user()
+        self.create_member(user=user1, organization=self.organization, role="member", teams=[team])
+        self.create_member(user=user2, organization=self.organization, role="member", teams=[team])
+        link_team(team, self.integration, "#team-channel", "team_channel_id")
+
+        controller = NotificationController(
+            recipients=[team],
+            organization_id=self.organization.id,
+        )
+
+        assert len(controller.recipients) == 1
+
+    @with_feature("organizations:team-workflow-notifications")
+    def test_non_team_recipients_remain(self):
+        user1 = self.create_user()
+        user2 = self.create_user()
+
+        controller = NotificationController(
+            recipients=[user1, user2],
+            organization_id=self.organization.id,
+        )
+
+        assert len(controller.recipients) == 2