Browse Source

fix(mail_adapter): Fix bug in issue alerts where mail won't send directly to users in some cases (#20741)

This fixes a bug where when a user has disabled notifications for a given project, and also have an
alert set up to email them directly on that project they are not sent the notification. We missed
this edge case when originally working on the mail adapter.
Dan Fuller 4 years ago
parent
commit
46b7f9cba0

+ 4 - 5
src/sentry/mail/actions.py

@@ -106,7 +106,9 @@ class NotifyEmailAction(EventAction):
         extra = {"event_id": event.event_id}
         group = event.group
 
-        if not mail_adapter.should_notify(group=group):
+        target_type = ActionTargetType(self.data["targetType"])
+
+        if not mail_adapter.should_notify(target_type, group=group):
             extra["group_id"] = group.id
             self.logger.info("rule.fail.should_notify", extra=extra)
             return
@@ -114,10 +116,7 @@ class NotifyEmailAction(EventAction):
         metrics.incr("notifications.sent", instance=self.metrics_slug, skip_internal=False)
         yield self.future(
             lambda event, futures: mail_adapter.rule_notify(
-                event,
-                futures,
-                ActionTargetType(self.data["targetType"]),
-                self.data.get("targetIdentifier", None),
+                event, futures, target_type, self.data.get("targetIdentifier", None)
             )
         )
 

+ 5 - 4
src/sentry/mail/adapter.py

@@ -151,10 +151,11 @@ class MailAdapter(object):
         """
         return project.get_notification_recipients(self.alert_option_key)
 
-    def should_notify(self, group):
+    def should_notify(self, target_type, group):
         metrics.incr("mail_adapter.should_notify")
-        # only notify if we have users to notify
-        return self.get_sendable_users(group.project)
+        # only notify if we have users to notify. We always want to notify if targeting
+        # a member directly.
+        return target_type == ActionTargetType.MEMBER or self.get_sendable_users(group.project)
 
     def get_send_to(self, project, target_type, target_identifier=None, event=None):
         """
@@ -304,7 +305,7 @@ class MailAdapter(object):
 
         rules = []
         for rule in notification.rules:
-            rule_link = "/organizations/%s/alerts/rules/%s/%s/" % (org.slug, project.slug, rule.id,)
+            rule_link = "/organizations/%s/alerts/rules/%s/%s/" % (org.slug, project.slug, rule.id)
 
             rules.append((rule.label, rule_link))
 

+ 1 - 1
tests/sentry/mail/test_actions.py

@@ -97,7 +97,7 @@ class NotifyEmailTest(RuleTestCase):
 
     def test_simple(self):
         event = self.get_event()
-        rule = self.get_rule()
+        rule = self.get_rule(data={"targetType": "IssueOwners"})
 
         results = list(rule.after(event=event, state=self.get_state()))
         assert len(results) == 1

+ 9 - 2
tests/sentry/mail/test_adapter.py

@@ -560,13 +560,20 @@ class MailAdapterRuleNotifyTest(BaseMailAdapterTest, TestCase):
 
 class MailAdapterShouldNotifyTest(BaseMailAdapterTest, TestCase):
     def test_should_notify(self):
-        assert self.adapter.should_notify(self.group)
+        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):
         UserOption.objects.set_value(
             user=self.user, key="mail:alert", value=0, project=self.project
         )
-        assert not self.adapter.should_notify(self.group)
+        assert not self.adapter.should_notify(ActionTargetType.ISSUE_OWNERS, self.group)
+
+    def test_should_always_notify_target_member(self):
+        UserOption.objects.set_value(
+            user=self.user, key="mail:alert", value=0, project=self.project
+        )
+        assert self.adapter.should_notify(ActionTargetType.MEMBER, self.group)
 
 
 class MailAdapterGetSendToOwnersTest(BaseMailAdapterTest, TestCase):