fix(notification platform): Send issue owner team Slack notifications (…

…#29495) * fix(notification platform): Send issue owner team Slack notifications
getsentry · Oct 26, 2021 · 78bbf79 · 78bbf79
1 parent eb3b29b
commit 78bbf79
Show file tree

Hide file tree

Showing 7 changed files with 141 additions and 58 deletions.
diff --git a/src/sentry/digests/__init__.py b/src/sentry/digests/__init__.py
@@ -1,12 +1,15 @@
 from collections import namedtuple
 from datetime import datetime
-from typing import Any, Mapping, Optional
+from typing import TYPE_CHECKING, Mapping, Optional, Sequence
 
 from django.conf import settings
 
 from sentry.utils.dates import to_datetime
 from sentry.utils.services import LazyServiceWrapper
 
+if TYPE_CHECKING:
+    from sentry.models import Rule, Group
+
 from .backends.base import Backend  # NOQA
 from .backends.dummy import DummyBackend  # NOQA
 
@@ -26,7 +29,7 @@ def datetime(self) -> Optional[datetime]:
 
 OPTIONS = frozenset(("increment_delay", "maximum_delay", "minimum_delay"))
 
-Digest = Mapping[str, Mapping[str, Any]]
+Digest = Mapping["Rule", Mapping["Group", Sequence[Record]]]
 
 
 def get_option_key(plugin: str, option: str) -> str:

diff --git a/src/sentry/digests/utilities.py b/src/sentry/digests/utilities.py
@@ -22,11 +22,12 @@ def get_digest_metadata(
 
         for group, records in groups.items():
             for record in records:
-                if start is None or record.datetime < start:
-                    start = record.datetime
+                if record.datetime:
+                    if start is None or record.datetime < start:
+                        start = record.datetime
 
-                if end is None or record.datetime > end:
-                    end = record.datetime
+                    if end is None or record.datetime > end:
+                        end = record.datetime
 
     return start, end, counts
 

diff --git a/src/sentry/mail/actions.py b/src/sentry/mail/actions.py
@@ -1,6 +1,7 @@
 from sentry.mail import mail_adapter
 from sentry.mail.forms.notify_email import NotifyEmailForm
 from sentry.notifications.types import ACTION_CHOICES, ActionTargetType
+from sentry.notifications.utils.participants import determine_eligible_recipients
 from sentry.rules.actions.base import EventAction
 from sentry.utils import metrics
 
@@ -20,16 +21,17 @@ def after(self, event, state):
         group = event.group
 
         target_type = ActionTargetType(self.data["targetType"])
+        target_identifier = self.data.get("targetIdentifier", None)
 
-        if not mail_adapter.should_notify(target_type, group=group):
+        if not determine_eligible_recipients(group.project, target_type, target_identifier, event):
             extra["group_id"] = group.id
             self.logger.info("rule.fail.should_notify", extra=extra)
             return
 
         metrics.incr("notifications.sent", instance=self.metrics_slug, skip_internal=False)
         yield self.future(
             lambda event, futures: mail_adapter.rule_notify(
-                event, futures, target_type, self.data.get("targetIdentifier", None)
+                event, futures, target_type, target_identifier
             )
         )
 

diff --git a/src/sentry/mail/adapter.py b/src/sentry/mail/adapter.py
@@ -96,19 +96,6 @@ def get_sendable_users(self, project):
         users = self.get_sendable_user_objects(project)
         return [user.id for user in users]
 
-    def should_notify(self, target_type, group):
-        metrics.incr("mail_adapter.should_notify")
-        # only notify if we have users to notify. We always want to notify if targeting
-        # a member directly.
-        return (
-            target_type
-            in [
-                ActionTargetType.MEMBER,
-                ActionTargetType.TEAM,
-            ]
-            or self.get_sendable_user_objects(group.project)
-        )
-
     @staticmethod
     def notify(notification, target_type, target_identifier=None, **kwargs):
         AlertRuleNotification(notification, target_type, target_identifier).send()

diff --git a/src/sentry/notifications/notifications/digest.py b/src/sentry/notifications/notifications/digest.py
@@ -40,10 +40,17 @@ def __init__(
         self.target_identifier = target_identifier
 
     def get_participants(self) -> Mapping[ExternalProviders, Iterable[Union["Team", "User"]]]:
+        event = [
+            record
+            for records_by_group in self.digest.values()
+            for records in records_by_group.values()
+            for record in records
+        ][0].value.event
         return get_send_to(
             project=self.project,
             target_type=self.target_type,
             target_identifier=self.target_identifier,
+            event=event,
         )
 
     def get_filename(self) -> str:
@@ -104,15 +111,30 @@ def get_extra_context(self, recipient_ids: Iterable[int]) -> Mapping[int, Mappin
         return extra_context
 
     def send(self) -> None:
+        from sentry.models import User
+
         if not self.should_email():
             return
 
+        # Only calculate shared context once.
+        shared_context = self.get_context()
+
+        if should_send_as_alert_notification(shared_context):
+            return send_as_alert_notification(
+                shared_context, self.target_type, self.target_identifier
+            )
+
         participants_by_provider = self.get_participants()
         if not participants_by_provider:
             return
 
         # Get every user ID for every provider as a set.
-        user_ids = {user.id for users in participants_by_provider.values() for user in users}
+        user_ids = {
+            participant.id
+            for participants in participants_by_provider.values()
+            for participant in participants
+            if isinstance(participant, User)
+        }
 
         logger.info(
             "mail.adapter.notify_digest",
@@ -124,14 +146,6 @@ def send(self) -> None:
             },
         )
 
-        # Only calculate shared context once.
-        shared_context = self.get_context()
-
-        if should_send_as_alert_notification(shared_context):
-            return send_as_alert_notification(
-                shared_context, self.target_type, self.target_identifier
-            )
-
         # Calculate the per-user context. It's fine that we're doing extra work
         # to get personalized digests for the non-email users.
         extra_context: Mapping[int, Mapping[str, Any]] = {}

diff --git a/tests/sentry/integrations/slack/notifications/test_issue_alert.py b/tests/sentry/integrations/slack/notifications/test_issue_alert.py
@@ -198,6 +198,110 @@ def test_issue_alert_team_issue_owners(self, mock_func):
             == f"{self.project.slug} | <http://testserver/settings/{self.organization.slug}/teams/{self.team.slug}/notifications/?referrer=AlertRuleSlackTeam|Notification Settings>"
         )
 
+    @responses.activate
+    @mock.patch("sentry.notifications.notify.notify", side_effect=send_notification)
+    @patch.object(sentry, "digests")
+    def test_issue_alert_team_issue_owners_user_settings_off_digests(self, digests, mock_func):
+        """Test that issue alerts are sent to a team in Slack via an Issue Owners rule action
+        even when the users' issue alert notification settings are off and digests are triggered."""
+
+        backend = RedisBackend()
+        digests.digest = backend.digest
+        digests.enabled.return_value = True
+
+        # turn off the user's issue alert notification settings
+        # there was a bug where issue alerts to a team's Slack channel
+        # were only firing if this was set to ALWAYS
+        NotificationSetting.objects.update_settings(
+            ExternalProviders.SLACK,
+            NotificationSettingTypes.ISSUE_ALERTS,
+            NotificationSettingOptionValues.NEVER,
+            user=self.user,
+        )
+        # add a second user to the team so we can be sure it's only
+        # sent once (to the team, and not to each individual user)
+        user2 = self.create_user(is_superuser=False)
+        self.create_member(teams=[self.team], user=user2, organization=self.organization)
+        self.idp = IdentityProvider.objects.create(type="slack", external_id="TXXXXXXX2", config={})
+        self.identity = Identity.objects.create(
+            external_id="UXXXXXXX2",
+            idp=self.idp,
+            user=user2,
+            status=IdentityStatus.VALID,
+            scopes=[],
+        )
+        NotificationSetting.objects.update_settings(
+            ExternalProviders.SLACK,
+            NotificationSettingTypes.ISSUE_ALERTS,
+            NotificationSettingOptionValues.NEVER,
+            user=user2,
+        )
+        # update the team's notification settings
+        ExternalActor.objects.create(
+            actor=self.team.actor,
+            organization=self.organization,
+            integration=self.integration,
+            provider=ExternalProviders.SLACK.value,
+            external_name="goma",
+            external_id="CXXXXXXX2",
+        )
+        NotificationSetting.objects.update_settings(
+            ExternalProviders.SLACK,
+            NotificationSettingTypes.ISSUE_ALERTS,
+            NotificationSettingOptionValues.ALWAYS,
+            team=self.team,
+        )
+
+        rule = GrammarRule(Matcher("path", "*"), [Owner("team", self.team.slug)])
+        ProjectOwnership.objects.create(
+            project_id=self.project.id, schema=dump_schema([rule]), fallthrough=True
+        )
+
+        event = self.store_event(
+            data={
+                "message": "Hello world",
+                "level": "error",
+                "stacktrace": {"frames": [{"filename": "foo.py"}]},
+            },
+            project_id=self.project.id,
+        )
+
+        action_data = {
+            "id": "sentry.mail.actions.NotifyEmailAction",
+            "targetType": "IssueOwners",
+            "targetIdentifier": "",
+        }
+        rule = Rule.objects.create(
+            project=self.project,
+            label="ja rule",
+            data={
+                "match": "all",
+                "actions": [action_data],
+            },
+        )
+
+        key = f"mail:p:{self.project.id}"
+        backend.add(key, event_to_record(event, [rule]), increment_delay=0, maximum_delay=0)
+
+        with self.tasks():
+            deliver_digest(key)
+
+        # check that only one was sent out - more would mean each user is being notified
+        # rather than the team
+        assert len(responses.calls) == 1
+
+        # check that the team got a notification
+        data = parse_qs(responses.calls[0].request.body)
+        assert data["channel"] == ["CXXXXXXX2"]
+        assert "attachments" in data
+        attachments = json.loads(data["attachments"][0])
+        assert len(attachments) == 1
+        assert attachments[0]["title"] == "Hello world"
+        assert (
+            attachments[0]["footer"]
+            == f"{self.project.slug} | <http://testserver/settings/{self.organization.slug}/teams/{self.team.slug}/notifications/?referrer=AlertRuleSlackTeam|Notification Settings>"
+        )
+
     @responses.activate
     @mock.patch("sentry.notifications.notify.notify", side_effect=send_notification)
     def test_issue_alert_team(self, mock_func):
@@ -490,8 +594,6 @@ def test_digest_enabled(self, digests, mock_func):
         with self.tasks():
             deliver_digest(key)
 
-        assert digests.call_count == 0
-
         attachment, text = get_attachment()
 
         assert attachment["title"] == "Hello world"

diff --git a/tests/sentry/mail/test_adapter.py b/tests/sentry/mail/test_adapter.py
@@ -645,32 +645,6 @@ def test_digest(self, digests):
         assert digests.add.call_count == 1
 
 
-class MailAdapterShouldNotifyTest(BaseMailAdapterTest):
-    def test_should_notify(self):
-        assert self.adapter.should_notify(ActionTargetType.ISSUE_OWNERS, self.group)
-        assert self.adapter.should_notify(ActionTargetType.MEMBER, self.group)
-
-    def test_should_not_notify_no_users(self):
-        NotificationSetting.objects.update_settings(
-            ExternalProviders.EMAIL,
-            NotificationSettingTypes.ISSUE_ALERTS,
-            NotificationSettingOptionValues.NEVER,
-            user=self.user,
-            project=self.project,
-        )
-        assert not self.adapter.should_notify(ActionTargetType.ISSUE_OWNERS, self.group)
-
-    def test_should_always_notify_target_member(self):
-        NotificationSetting.objects.update_settings(
-            ExternalProviders.EMAIL,
-            NotificationSettingTypes.ISSUE_ALERTS,
-            NotificationSettingOptionValues.NEVER,
-            user=self.user,
-            project=self.project,
-        )
-        assert self.adapter.should_notify(ActionTargetType.MEMBER, self.group)
-
-
 class MailAdapterNotifyAboutActivityTest(BaseMailAdapterTest):
     def test_assignment(self):
         NotificationSetting.objects.update_settings(