Browse Source

feat(slack): Get parent notifications on filters (#68142)

- Add functionality to get parent notifications based on filters for
future work.
  - Add tests
- Add ability to find rule action based on rule action uuid.
  - Add tests
- Fix typo in error message
- Refactor some tests for easier iteration and consolidation

Resolves:
https://github.com/getsentry/team-core-product-foundations/issues/188
Yash Kamothi 11 months ago
parent
commit
316d6efcdc

+ 1 - 1
src/sentry/integrations/msteams/notifications.py

@@ -113,7 +113,7 @@ def send_notification_as_msteams(
 
                         notification.record_notification_sent(recipient, ExternalProviders.MSTEAMS)
                     except Exception:
-                        logger.exception("Exception occured while trying to send the notification")
+                        logger.exception("Exception occurred while trying to send the notification")
 
     metrics.incr(
         f"{notification.metrics_key}.notifications.sent",

+ 42 - 3
src/sentry/integrations/repository/issue_alert.py

@@ -1,8 +1,11 @@
 from __future__ import annotations
 
+from collections.abc import Generator
 from dataclasses import dataclass
 from logging import Logger, getLogger
 
+from django.db.models import Q
+
 from sentry.integrations.repository.base import (
     BaseNewNotificationMessage,
     BaseNotificationMessage,
@@ -87,6 +90,15 @@ class IssueAlertNotificationMessageRepository:
     def default(cls) -> IssueAlertNotificationMessageRepository:
         return cls(logger=_default_logger)
 
+    @classmethod
+    def _parent_notification_message_base_filter(cls) -> Q:
+        """
+        Returns the query used to filter the notification messages for parent notification messages.
+        Parent notification messages are notification message instances without a parent notification message itself,
+        and where the error code is null.
+        """
+        return Q(parent_notification_message__isnull=True, error_code__isnull=True)
+
     def get_parent_notification_message(
         self, rule_id: int, group_id: int, rule_action_uuid: str
     ) -> IssueAlertNotificationMessage | None:
@@ -95,12 +107,11 @@ class IssueAlertNotificationMessageRepository:
         Will raise an exception if the query fails and logs the error with associated data.
         """
         try:
-            instance: NotificationMessage = self._model.objects.get(
+            base_filter = self._parent_notification_message_base_filter()
+            instance: NotificationMessage = self._model.objects.filter(base_filter).get(
                 rule_fire_history__rule__id=rule_id,
                 rule_fire_history__group__id=group_id,
                 rule_action_uuid=rule_action_uuid,
-                parent_notification_message__isnull=True,
-                error_code__isnull=True,
             )
             return IssueAlertNotificationMessage.from_model(instance=instance)
         except NotificationMessage.DoesNotExist:
@@ -140,3 +151,31 @@ class IssueAlertNotificationMessageRepository:
                 extra=data.__dict__,
             )
             raise
+
+    def get_all_parent_notification_messages_by_filters(
+        self, group_ids: list[int] | None = None, project_ids: list[int] | None = None
+    ) -> Generator[IssueAlertNotificationMessage, None, None]:
+        """
+        If no filters are passed, then all parent notification objects are returned.
+
+        Because an unbounded amount of parent notification objects can be returned, this method leverages generator to
+        control the usage of memory in the application.
+        It is up to the caller to iterate over all the data, or store in memory if they need all objects concurrently.
+        """
+        group_id_filter = Q(rule_fire_history__group__id__in=group_ids) if group_ids else Q()
+        project_id_filter = Q(rule_fire_history__project_id__in=project_ids) if project_ids else Q()
+
+        query = self._model.objects.filter(group_id_filter & project_id_filter).filter(
+            self._parent_notification_message_base_filter()
+        )
+
+        try:
+            for instance in query:
+                yield IssueAlertNotificationMessage.from_model(instance=instance)
+        except Exception as e:
+            self._logger.exception(
+                "Failed to get parent notifications on filters",
+                exc_info=e,
+                extra=filter.__dict__,
+            )
+            raise

+ 17 - 1
src/sentry/models/rule.py

@@ -1,6 +1,6 @@
 from collections.abc import Sequence
 from enum import Enum, IntEnum
-from typing import ClassVar, Self
+from typing import Any, ClassVar, Self
 
 from django.db import models
 from django.utils import timezone
@@ -110,6 +110,22 @@ class Rule(Model):
             "environment": self.environment_id,
         }
 
+    def get_rule_action_details_by_uuid(self, rule_action_uuid: str) -> dict[str, Any] | None:
+        actions = self.data.get("actions", None)
+        if not actions:
+            return None
+
+        for action in actions:
+            action_uuid = action.get("uuid", None)
+            if action_uuid is None:
+                # This should not happen, but because the data object is a dictionary, it's better to be safe
+                continue
+
+            if action_uuid == rule_action_uuid:
+                return action
+
+        return None
+
 
 class RuleActivityType(Enum):
     CREATED = 1

+ 107 - 38
tests/sentry/integrations/repository/issue_alert/test_issue_alert_notification_message_repository.py

@@ -10,24 +10,29 @@ from sentry.models.rulefirehistory import RuleFireHistory
 from sentry.testutils.cases import TestCase
 
 
-class TestGetParentNotificationMessage(TestCase):
+class BaseIssueAlertNotificationMessageRepositoryTest(TestCase):
     def setUp(self) -> None:
         self.action_uuid = str(uuid4())
+        self.notify_issue_owners_action = [
+            {
+                "targetType": "IssueOwners",
+                "fallthroughType": "ActiveMembers",
+                "id": "sentry.mail.actions.NotifyEmailAction",
+                "targetIdentifier": "",
+                "uuid": self.action_uuid,
+            }
+        ]
         self.rule = self.create_project_rule(
-            project=self.project,
-            action_match=[
-                {
-                    "id": "sentry.rules.actions.notify_event_service.NotifyEventServiceAction",
-                    "service": "mail",
-                    "name": "Send a notification via mail",
-                    "uuid": self.action_uuid,
-                },
-            ],
+            project=self.project, action_match=self.notify_issue_owners_action
         )
+        self.event_id = 456
+        self.notification_uuid = str(uuid4())
         self.rule_fire_history = RuleFireHistory.objects.create(
             project=self.project,
             rule=self.rule,
             group=self.group,
+            event_id=self.event_id,
+            notification_uuid=self.notification_uuid,
         )
         self.parent_notification_message = NotificationMessage.objects.create(
             rule_fire_history=self.rule_fire_history,
@@ -36,6 +41,8 @@ class TestGetParentNotificationMessage(TestCase):
         )
         self.repository = IssueAlertNotificationMessageRepository.default()
 
+
+class TestGetParentNotificationMessage(BaseIssueAlertNotificationMessageRepositoryTest):
     def test_returns_parent_notification_message(self) -> None:
         instance = self.repository.get_parent_notification_message(
             rule_id=self.rule.id,
@@ -79,37 +86,12 @@ class TestGetParentNotificationMessage(TestCase):
         )
 
 
-class TestCreateNotificationMessage(TestCase):
-    def setUp(self):
-        self.action_uuid = str(uuid4())
-        self.notify_issue_owners_action = [
-            {
-                "targetType": "IssueOwners",
-                "fallthroughType": "ActiveMembers",
-                "id": "sentry.mail.actions.NotifyEmailAction",
-                "targetIdentifier": "",
-                "uuid": self.action_uuid,
-            }
-        ]
-        self.rule = self.create_project_rule(
-            project=self.project, action_match=self.notify_issue_owners_action
-        )
-        self.event_id = 456
-        self.notification_uuid = str(uuid4())
-        self.rule_fire_history = RuleFireHistory.objects.create(
-            project=self.project,
-            rule=self.rule,
-            group=self.group,
-            event_id=self.event_id,
-            notification_uuid=self.notification_uuid,
-        )
-        self.repository = IssueAlertNotificationMessageRepository.default()
-
+class TestCreateNotificationMessage(BaseIssueAlertNotificationMessageRepositoryTest):
     def test_simple(self) -> None:
         message_identifier = "1a2b3c"
         data = NewIssueAlertNotificationMessage(
             rule_fire_history_id=self.rule_fire_history.id,
-            rule_action_uuid=self.action_uuid,
+            rule_action_uuid=str(uuid4()),
             message_identifier=message_identifier,
         )
 
@@ -128,7 +110,7 @@ class TestCreateNotificationMessage(TestCase):
         }
         data = NewIssueAlertNotificationMessage(
             rule_fire_history_id=self.rule_fire_history.id,
-            rule_action_uuid=self.action_uuid,
+            rule_action_uuid=str(uuid4()),
             error_code=405,
             error_details=error_detail,
         )
@@ -136,3 +118,90 @@ class TestCreateNotificationMessage(TestCase):
         result = self.repository.create_notification_message(data=data)
         assert result is not None
         assert result.error_details == error_detail
+
+
+class TestGetAllParentNotificationMessagesByFilters(
+    BaseIssueAlertNotificationMessageRepositoryTest
+):
+    def test_returns_all_when_no_filters(self) -> None:
+        # Create additional notification messages
+        additional_notification = NotificationMessage.objects.create(
+            rule_fire_history=self.rule_fire_history,
+            rule_action_uuid=str(uuid4()),
+            message_identifier="123abc",
+        )
+
+        # Call the method without any filters
+        parent_notifications = list(
+            self.repository.get_all_parent_notification_messages_by_filters()
+        )
+
+        result_ids = []
+        for parent_notification in parent_notifications:
+            result_ids.append(parent_notification.id)
+        # Check if all notification messages are returned
+        assert len(result_ids) == 2
+        assert additional_notification.id in result_ids
+        assert self.parent_notification_message.id in result_ids
+
+    def test_returns_filtered_messages_for_project_id(self) -> None:
+        # Create some notification message that should not be returned
+        new_project = self.create_project()
+        additional_rule_fire_history = RuleFireHistory.objects.create(
+            project=new_project,
+            rule=self.rule,
+            group=self.group,
+            event_id=self.event_id,
+            notification_uuid=str(uuid4()),
+        )
+        notification_message_that_should_not_be_returned = NotificationMessage.objects.create(
+            rule_fire_history=additional_rule_fire_history,
+            rule_action_uuid=str(uuid4()),
+            message_identifier="zxcvb",
+        )
+
+        # Call the method with filters specifying the specific project id
+        parent_notifications = list(
+            self.repository.get_all_parent_notification_messages_by_filters(
+                project_ids=[self.project.id]
+            )
+        )
+
+        result_ids = []
+        for parent_notification in parent_notifications:
+            result_ids.append(parent_notification.id)
+        # Check if only the notifications related to the specified project
+        assert len(result_ids) == 1
+        assert result_ids[0] == self.parent_notification_message.id
+        assert notification_message_that_should_not_be_returned.id not in result_ids
+
+    def test_returns_filtered_messages_for_group_id(self) -> None:
+        # Create some notification message that should not be returned
+        new_group = self.create_group(project=self.project)
+        additional_rule_fire_history = RuleFireHistory.objects.create(
+            project=self.project,
+            rule=self.rule,
+            group=new_group,
+            event_id=self.event_id,
+            notification_uuid=str(uuid4()),
+        )
+        notification_message_that_should_not_be_returned = NotificationMessage.objects.create(
+            rule_fire_history=additional_rule_fire_history,
+            rule_action_uuid=str(uuid4()),
+            message_identifier="zxcvb",
+        )
+
+        # Call the method with filters specifying the specific project id
+        parent_notifications = list(
+            self.repository.get_all_parent_notification_messages_by_filters(
+                group_ids=[self.group.id]
+            )
+        )
+
+        result_ids = []
+        for parent_notification in parent_notifications:
+            result_ids.append(parent_notification.id)
+        # Check if only the notifications related to the specified project
+        assert len(result_ids) == 1
+        assert result_ids[0] == self.parent_notification_message.id
+        assert notification_message_that_should_not_be_returned.id not in result_ids

+ 75 - 0
tests/sentry/models/test_rule.py

@@ -0,0 +1,75 @@
+from uuid import uuid4
+
+from sentry.testutils.cases import TestCase
+
+
+class TestRule_GetRuleActionDetailsByUuid(TestCase):
+    def setUp(self) -> None:
+        self.action_uuid = str(uuid4())
+        self.action = {
+            "targetType": "IssueOwners",
+            "fallthroughType": "ActiveMembers",
+            "id": "sentry.mail.actions.NotifyEmailAction",
+            "targetIdentifier": "",
+            "uuid": self.action_uuid,
+        }
+        self.notify_issue_owners_action = [
+            self.action,
+            {
+                "targetType": "IssueOwners",
+                "fallthroughType": "ActiveMembers",
+                "id": "sentry.mail.actions.NotifyEmailAction",
+                "targetIdentifier": "",
+                "uuid": str(uuid4()),
+            },
+        ]
+        self.rule = self.create_project_rule(
+            project=self.project, action_match=self.notify_issue_owners_action
+        )
+
+    def test_simple(self) -> None:
+        result = self.rule.get_rule_action_details_by_uuid(self.action_uuid)
+        assert result == self.action
+
+    def test_returns_none(self) -> None:
+        result = self.rule.get_rule_action_details_by_uuid(str(uuid4()))
+        assert result is None
+
+    def test_when_no_actions_are_in_rule(self) -> None:
+        rule = self.create_project_rule(
+            project=self.project,
+            action_match=[],
+        )
+        result = rule.get_rule_action_details_by_uuid(str(uuid4()))
+        assert result is None
+
+    def test_when_actions_have_missing_uuid_key(self) -> None:
+        rule = self.create_project_rule(
+            project=self.project,
+            action_match=[
+                {
+                    "targetType": "IssueOwners",
+                    "fallthroughType": "ActiveMembers",
+                    "id": "sentry.mail.actions.NotifyEmailAction",
+                    "targetIdentifier": "",
+                }
+            ],
+        )
+        result = rule.get_rule_action_details_by_uuid(str(uuid4()))
+        assert result is None
+
+    def test_when_action_has_missing_uuid_value(self) -> None:
+        rule = self.create_project_rule(
+            project=self.project,
+            action_match=[
+                {
+                    "targetType": "IssueOwners",
+                    "fallthroughType": "ActiveMembers",
+                    "id": "sentry.mail.actions.NotifyEmailAction",
+                    "targetIdentifier": "",
+                    "uuid": "",
+                }
+            ],
+        )
+        result = rule.get_rule_action_details_by_uuid(str(uuid4()))
+        assert result is None