From b9f0b7f37271ca6270a283896f5c24dd1cad1cb7 Mon Sep 17 00:00:00 2001 From: Kyle Consalus Date: Wed, 30 Sep 2026 15:04:35 -0700 Subject: [PATCH 01/12] ref(notifications): Create NotificationRule to be used in place of punned Rule --- src/sentry/digests/notifications.py | 4 +- .../discord/message_builder/issues.py | 4 +- .../integrations/messaging/message_builder.py | 3 +- .../msteams/actions/notification.py | 10 ++++- .../msteams/card_builder/issues.py | 12 ++++-- .../opsgenie/actions/notification.py | 10 ++++- .../pagerduty/actions/notification.py | 5 +-- .../slack/actions/notification.py | 5 +-- .../slack/message_builder/issues.py | 5 ++- .../slack/message_builder/util.py | 3 +- src/sentry/mail/adapter.py | 1 + .../notification_action/types.py | 12 +++--- .../platform/discord/renderers/issue.py | 2 +- .../platform/msteams/renderers/issue.py | 21 ++++++---- .../platform/slack/renderers/issue.py | 2 +- .../notifications/platform/templates/issue.py | 20 ++++------ src/sentry/notifications/types.py | 17 +++++++- .../notifications/utils/participants.py | 1 - src/sentry/notifications/utils/rules.py | 19 ++++++--- src/sentry/rules/actions/base.py | 9 +++-- src/sentry/rules/actions/integrations/base.py | 5 +-- .../integrations/create_ticket/base.py | 4 +- .../integrations/create_ticket/utils.py | 14 ++++--- src/sentry/rules/base.py | 13 ++++--- src/sentry/rules/processing/processor.py | 11 +++--- src/sentry/sentry_apps/tasks/sentry_apps.py | 8 ++-- src/sentry/types/rules.py | 1 - .../integrations/github/test_ticket_action.py | 2 +- .../github_enterprise/test_ticket_action.py | 2 +- .../integrations/jira/test_notify_action.py | 2 +- .../integrations/jira/test_ticket_action.py | 2 +- .../jira_server/test_ticket_action.py | 2 +- .../msteams/test_message_builder.py | 30 ++++++++++++++ .../pagerduty/test_notification.py | 2 +- .../test_slack_notify_service_action.py | 2 +- .../integrations/vsts/test_notify_action.py | 2 +- .../test_issue_alert_registry_handlers.py | 39 ++++++++++--------- .../platform/msteams/renderers/test_issue.py | 4 +- .../msteams/renderers/test_issue_parity.py | 11 +++++- .../notifications/utils/test_participants.py | 1 - .../sentry_apps/tasks/test_sentry_apps.py | 2 +- 41 files changed, 206 insertions(+), 118 deletions(-) diff --git a/src/sentry/digests/notifications.py b/src/sentry/digests/notifications.py index c12eebe5531c..ccec069b2414 100644 --- a/src/sentry/digests/notifications.py +++ b/src/sentry/digests/notifications.py @@ -12,7 +12,7 @@ from sentry.models.group import Group, GroupStatus from sentry.models.project import Project from sentry.models.rule import Rule -from sentry.notifications.types import ActionTargetType, FallthroughChoiceType +from sentry.notifications.types import ActionTargetType, FallthroughChoiceType, NotificationRule from sentry.notifications.utils.rules import get_rule_or_workflow_id from sentry.services.eventstore.models import Event, GroupEvent from sentry.tsdb.base import TSDBModel @@ -78,7 +78,7 @@ def unsplit_key( def event_to_record( event: Event | GroupEvent, - rules: Sequence[Rule], + rules: Sequence[NotificationRule], notification_uuid: str | None = None, identifier_key: IdentifierKey = IdentifierKey.RULE, ) -> Record: diff --git a/src/sentry/integrations/discord/message_builder/issues.py b/src/sentry/integrations/discord/message_builder/issues.py index 452f963907ce..b8550a7af301 100644 --- a/src/sentry/integrations/discord/message_builder/issues.py +++ b/src/sentry/integrations/discord/message_builder/issues.py @@ -23,8 +23,8 @@ from sentry.integrations.types import ExternalProviders from sentry.models.group import Group, GroupStatus from sentry.models.project import Project -from sentry.models.rule import Rule from sentry.notifications.notifications.base import ProjectNotification +from sentry.notifications.types import NotificationRule from sentry.notifications.utils.rules import RuleIdType, get_rule_or_workflow_id from sentry.services.eventstore.models import GroupEvent @@ -37,7 +37,7 @@ def __init__( group: Group, event: GroupEvent | None = None, tags: set[str] | None = None, - rules: list[Rule] | None = None, + rules: list[NotificationRule] | None = None, link_to_event: bool = False, issue_details: bool = False, notification: ProjectNotification | None = None, diff --git a/src/sentry/integrations/messaging/message_builder.py b/src/sentry/integrations/messaging/message_builder.py index 2d2fad7e921d..c366dcbe228d 100644 --- a/src/sentry/integrations/messaging/message_builder.py +++ b/src/sentry/integrations/messaging/message_builder.py @@ -13,6 +13,7 @@ from sentry.models.team import Team from sentry.notifications.notifications.base import BaseNotification from sentry.notifications.notifications.rules import AlertRuleNotification +from sentry.notifications.types import NotificationRule from sentry.notifications.utils.links import create_link_to_workflow from sentry.notifications.utils.rules import get_key_from_rule_data, get_rule_or_workflow_id from sentry.services.eventstore.models import Event, GroupEvent @@ -263,7 +264,7 @@ def build_footer( group: Group, project: Project, url_format: str, - rules: Sequence[Rule] | None = None, + rules: Sequence[Rule | NotificationRule] | None = None, ) -> str: footer = f"{group.qualified_short_id}" if rules: diff --git a/src/sentry/integrations/msteams/actions/notification.py b/src/sentry/integrations/msteams/actions/notification.py index a437aaa2df46..8008e2d90479 100644 --- a/src/sentry/integrations/msteams/actions/notification.py +++ b/src/sentry/integrations/msteams/actions/notification.py @@ -1,5 +1,7 @@ from __future__ import annotations +from collections.abc import Generator, Sequence + from sentry.integrations.messaging.metrics import ( MessagingInteractionEvent, MessagingInteractionType, @@ -11,7 +13,9 @@ from sentry.integrations.msteams.spec import MsTeamsMessagingSpec from sentry.integrations.services.integration import RpcIntegration from sentry.integrations.types import IntegrationProviderSlug +from sentry.notifications.types import RuleFuture from sentry.rules.actions import IntegrationEventAction +from sentry.rules.base import CallbackFuture from sentry.services.eventstore.models import GroupEvent from sentry.shared_integrations.exceptions import ApiError, IntegrationError from sentry.utils import metrics @@ -41,14 +45,16 @@ def get_integrations(self) -> list[RpcIntegration]: a for a in super().get_integrations() if a.metadata.get("installation_type") != "tenant" ] - def after(self, event: GroupEvent, notification_uuid: str | None = None): + def after( + self, event: GroupEvent, notification_uuid: str | None = None + ) -> Generator[CallbackFuture]: channel = self.get_option("channel_id") integration = self.get_integration() if not integration: return - def send_notification(event, futures): + def send_notification(event: GroupEvent, futures: Sequence[RuleFuture]) -> None: rules = [f.rule for f in futures] card = MSTeamsIssueMessageBuilder( event.group, event, rules, integration diff --git a/src/sentry/integrations/msteams/card_builder/issues.py b/src/sentry/integrations/msteams/card_builder/issues.py index 0f1e28f15705..e8bbabf85c77 100644 --- a/src/sentry/integrations/msteams/card_builder/issues.py +++ b/src/sentry/integrations/msteams/card_builder/issues.py @@ -30,6 +30,8 @@ from sentry.models.group import Group, GroupStatus from sentry.models.project import Project from sentry.models.rule import Rule +from sentry.notifications.types import NotificationRule +from sentry.notifications.utils.rules import get_legacy_rule_id from sentry.services.eventstore.models import Event, GroupEvent from .base import MSTeamsMessageBuilder @@ -52,7 +54,7 @@ logger = logging.getLogger(__name__) -def get_workflow_ids(rules: Sequence[Rule]) -> list[int]: +def get_workflow_ids(rules: Sequence[Rule | NotificationRule]) -> list[int]: workflow_ids = [] for rule in rules: action = rule.data.get("actions", [{}])[0] @@ -69,7 +71,7 @@ def __init__( self, group: Group, event: Event | GroupEvent | None, - rules: Sequence[Rule], + rules: Sequence[Rule | NotificationRule], integration: RpcIntegration, workflow_ids: Sequence[int] = (), ): @@ -87,7 +89,11 @@ def generate_action_payload(self, action_type: ACTION_TYPE) -> Any: "actionType": action_type, "groupId": self.group.id, "eventId": self.event.event_id if self.event else None, - "rules": [rule.id for rule in self.rules], + "rules": [ + rule_id + for rule in self.rules + if (rule_id := get_legacy_rule_id(rule)) is not None + ], "workflows": list(dict.fromkeys([*workflow_ids, *self.workflow_ids])), "integrationId": self.integration.id, } diff --git a/src/sentry/integrations/opsgenie/actions/notification.py b/src/sentry/integrations/opsgenie/actions/notification.py index fde38da45ce4..686d650153e5 100644 --- a/src/sentry/integrations/opsgenie/actions/notification.py +++ b/src/sentry/integrations/opsgenie/actions/notification.py @@ -1,6 +1,7 @@ from __future__ import annotations import logging +from collections.abc import Generator, Sequence from typing import cast import sentry_sdk @@ -14,7 +15,10 @@ from sentry.integrations.opsgenie.utils import get_team from sentry.integrations.services.integration import integration_service from sentry.integrations.types import IntegrationProviderSlug +from sentry.notifications.types import RuleFuture from sentry.rules.actions import IntegrationEventAction +from sentry.rules.base import CallbackFuture +from sentry.services.eventstore.models import GroupEvent from sentry.shared_integrations.exceptions import ApiError logger = logging.getLogger("sentry.integrations.opsgenie") @@ -43,7 +47,9 @@ def __init__(self, *args, **kwargs): }, } - def after(self, event, notification_uuid: str | None = None): + def after( + self, event: GroupEvent, notification_uuid: str | None = None + ) -> Generator[CallbackFuture]: integration = self.get_integration() if not integration: logger.warning("Integration removed, but the rule still refers to it") @@ -66,7 +72,7 @@ def after(self, event, notification_uuid: str | None = None): ) return - def send_notification(event, futures): + def send_notification(event: GroupEvent, futures: Sequence[RuleFuture]) -> None: installation = integration.get_installation(self.project.organization_id) try: client: OpsgenieClient = installation.get_keyring_client(self.get_option("team")) diff --git a/src/sentry/integrations/pagerduty/actions/notification.py b/src/sentry/integrations/pagerduty/actions/notification.py index ffbe449773ef..d1c144faef1a 100644 --- a/src/sentry/integrations/pagerduty/actions/notification.py +++ b/src/sentry/integrations/pagerduty/actions/notification.py @@ -14,8 +14,7 @@ build_pagerduty_event_payload, ) from sentry.integrations.types import IntegrationProviderSlug -from sentry.models.rule import Rule -from sentry.notifications.types import RuleFuture +from sentry.notifications.types import NotificationRule, RuleFuture from sentry.rules.actions import IntegrationEventAction from sentry.rules.base import CallbackFuture from sentry.services.eventstore.models import GroupEvent @@ -106,7 +105,7 @@ def send_notification(event: GroupEvent, futures: Sequence[RuleFuture]) -> None: severity=severity, ) - rules: list[Rule] = [f.rule for f in futures] + rules: list[NotificationRule] = [f.rule for f in futures] rule = rules[0] if rules else None if rule and rule.label: diff --git a/src/sentry/integrations/slack/actions/notification.py b/src/sentry/integrations/slack/actions/notification.py index a97c05e28a42..ce2784682674 100644 --- a/src/sentry/integrations/slack/actions/notification.py +++ b/src/sentry/integrations/slack/actions/notification.py @@ -29,9 +29,8 @@ from sentry.integrations.slack.utils.threads import NotificationActionThreadUtils from sentry.integrations.types import IntegrationProviderSlug from sentry.integrations.utils.metrics import EventLifecycle -from sentry.models.rule import Rule from sentry.notifications.additional_attachment_manager import get_additional_attachment -from sentry.notifications.types import RuleFuture +from sentry.notifications.types import NotificationRule, RuleFuture from sentry.notifications.utils.open_period import open_period_start_for_group from sentry.rules.actions import IntegrationEventAction from sentry.rules.base import CallbackFuture @@ -77,7 +76,7 @@ def _should_send_nudge(self, channel_id: str | None) -> bool: def _build_notification_blocks( self, event: GroupEvent, - rules: Sequence[Rule], + rules: Sequence[NotificationRule], tags: set, integration: RpcIntegration, notification_uuid: str | None = None, diff --git a/src/sentry/integrations/slack/message_builder/issues.py b/src/sentry/integrations/slack/message_builder/issues.py index a219941ae9d1..9743f2f1bf63 100644 --- a/src/sentry/integrations/slack/message_builder/issues.py +++ b/src/sentry/integrations/slack/message_builder/issues.py @@ -52,6 +52,7 @@ from sentry.models.team import Team from sentry.notifications.notifications.base import ProjectNotification from sentry.notifications.platform.slack.renderers.seer import SeerSlackRenderer +from sentry.notifications.types import NotificationRule from sentry.notifications.utils.actions import BlockKitMessageAction, MessageAction from sentry.notifications.utils.participants import ( dedupe_suggested_assignees, @@ -195,7 +196,7 @@ def get_tags( return fields -def get_context(group: Group, rules: list[Rule] | None = None) -> str: +def get_context(group: Group, rules: list[Rule | NotificationRule] | None = None) -> str: context_text = "" context = group.issue_type.notification_config.context.copy() @@ -416,7 +417,7 @@ def __init__( tags: set[str] | None = None, identity: RpcIdentity | None = None, actions: Sequence[MessageAction | BlockKitMessageAction] | None = None, - rules: list[Rule] | None = None, + rules: list[Rule | NotificationRule] | None = None, link_to_event: bool = False, issue_details: bool = False, notification: ProjectNotification | None = None, diff --git a/src/sentry/integrations/slack/message_builder/util.py b/src/sentry/integrations/slack/message_builder/util.py index 2c302ecb4914..47146e501572 100644 --- a/src/sentry/integrations/slack/message_builder/util.py +++ b/src/sentry/integrations/slack/message_builder/util.py @@ -5,6 +5,7 @@ from sentry.models.group import Group from sentry.models.project import Project from sentry.models.rule import Rule +from sentry.notifications.types import NotificationRule from sentry.notifications.utils.links import create_link_to_workflow from sentry.notifications.utils.rules import get_rule_or_workflow_id from sentry.utils.http import absolute_uri @@ -13,7 +14,7 @@ def build_slack_footer( group: Group, project: Project, - rules: Sequence[Rule] | None = None, + rules: Sequence[Rule | NotificationRule] | None = None, ) -> str: footer = f"{group.qualified_short_id}" diff --git a/src/sentry/mail/adapter.py b/src/sentry/mail/adapter.py index dfeb6e731737..530c06d1df7b 100644 --- a/src/sentry/mail/adapter.py +++ b/src/sentry/mail/adapter.py @@ -18,6 +18,7 @@ ActionTargetType, FallthroughChoiceType, NotificationSettingEnum, + RuleFuture, ) from sentry.notifications.types import ( RuleFuture as RuleFuture, diff --git a/src/sentry/notifications/notification_action/types.py b/src/sentry/notifications/notification_action/types.py index 2500403bb124..2229ca9ea6f4 100644 --- a/src/sentry/notifications/notification_action/types.py +++ b/src/sentry/notifications/notification_action/types.py @@ -23,8 +23,8 @@ from sentry.models.activity import Activity from sentry.models.organization import Organization from sentry.models.project import Project -from sentry.models.rule import Rule, RuleSource -from sentry.notifications.types import TEST_NOTIFICATION_ID, RuleFuture +from sentry.models.rule import Rule +from sentry.notifications.types import TEST_NOTIFICATION_ID, NotificationRule, RuleFuture from sentry.notifications.utils.issue_notification_context import IssueNotificationContext from sentry.rules.processing.processor import activate_downstream_actions from sentry.services.eventstore.models import GroupEvent @@ -206,7 +206,7 @@ def create_rule_instance_from_action( detector: Detector, event_data: WorkflowEventData, workflow_id: WorkflowId, - ) -> Rule: + ) -> NotificationRule: """ Creates a Rule instance from the Action model. :param action: Action @@ -271,14 +271,12 @@ def create_rule_instance_from_action( # mail action needs to have skipDigests set to True data["actions"][0]["skipDigests"] = True - rule = Rule( + rule = NotificationRule( id=action.id, project=detector.linked_project, environment_id=environment_id, label=label, data=dict(data), - status=ObjectStatus.ACTIVE, - source=RuleSource.ISSUE, ) return rule @@ -286,7 +284,7 @@ def create_rule_instance_from_action( @staticmethod def get_rule_futures( event_data: WorkflowEventData, - rule: Rule, + rule: NotificationRule, notification_uuid: str, ) -> Collection[tuple[Callable[[GroupEvent, Sequence[RuleFuture]], None], list[RuleFuture]]]: """ diff --git a/src/sentry/notifications/platform/discord/renderers/issue.py b/src/sentry/notifications/platform/discord/renderers/issue.py index e1dfe0a31aa7..70fc2a1ff3a9 100644 --- a/src/sentry/notifications/platform/discord/renderers/issue.py +++ b/src/sentry/notifications/platform/discord/renderers/issue.py @@ -50,7 +50,7 @@ def render[DataT: NotificationData]( except Exception: raise NotificationRenderError(f"Failed to retrieve event {data.event_id}") - rules = [data.rule.to_rule()] if data.rule else [] + rules = [data.rule.to_notification_rule(group.project)] if data.rule else [] return DiscordIssuesMessageBuilder( group=group, diff --git a/src/sentry/notifications/platform/msteams/renderers/issue.py b/src/sentry/notifications/platform/msteams/renderers/issue.py index 29a3974e207b..bd3643e35a3b 100644 --- a/src/sentry/notifications/platform/msteams/renderers/issue.py +++ b/src/sentry/notifications/platform/msteams/renderers/issue.py @@ -8,7 +8,6 @@ from sentry.integrations.types import IntegrationProviderSlug from sentry.models.group import Group, GroupStatus from sentry.models.project import Project -from sentry.models.rule import Rule from sentry.notifications.platform.msteams.provider import MSTeamsRenderable from sentry.notifications.platform.registry import renderer_registry from sentry.notifications.platform.renderer import NotificationRenderer @@ -20,6 +19,8 @@ NotificationRenderedTemplate, NotificationSource, ) +from sentry.notifications.types import NotificationRule +from sentry.notifications.utils.rules import get_legacy_rule_id from sentry.services.eventstore.models import Event, GroupEvent from sentry.types.actor import Actor @@ -65,7 +66,7 @@ def render[DataT: NotificationData]( except Exception: raise NotificationRenderError(f"Failed to retrieve event {data.event_id}") - rules = [data.rule.to_rule()] if data.rule else [] + rules = [data.rule.to_notification_rule(group.project)] if data.rule else [] issue_url = cls.build_issue_url(group=group, notification_uuid=data.notification_uuid) fields: list[Block | None] = [ @@ -125,7 +126,7 @@ def build_footer( *, group: Group, event: Event | GroupEvent | None, - rules: Sequence[Rule], + rules: Sequence[NotificationRule], ) -> ColumnSetBlock: from sentry.integrations.messaging.message_builder import build_footer from sentry.integrations.msteams.card_builder import MSTEAMS_URL_FORMAT @@ -186,7 +187,11 @@ def build_assignee_note(cls, group: Group) -> TextBlock | None: @classmethod def build_action_payload( - cls, *, action_type: ACTION_TYPE, data: IssueNotificationData, rules: Sequence[Rule] + cls, + *, + action_type: ACTION_TYPE, + data: IssueNotificationData, + rules: Sequence[NotificationRule], ) -> dict[str, Any]: # Keep this lazy to avoid initializing the msteams package during notifications app startup. from sentry.integrations.msteams.card_builder.issues import get_workflow_ids @@ -198,7 +203,9 @@ def build_action_payload( "actionType": action_type, "groupId": data.group_id, "eventId": data.event_id, - "rules": [rule.id for rule in rules], + "rules": [ + rule_id for rule in rules if (rule_id := get_legacy_rule_id(rule)) is not None + ], "workflows": get_workflow_ids(rules), } } @@ -213,7 +220,7 @@ def build_action( reverse_action: ACTION_TYPE, reverse_action_title: str, data: IssueNotificationData, - rules: Sequence[Rule], + rules: Sequence[NotificationRule], **card_kwargs: Any, ) -> Action: """ @@ -253,7 +260,7 @@ def build_assignee_choices(cls, group: Group) -> Sequence[tuple[str, str]]: @classmethod def build_actions( - cls, *, group: Group, data: IssueNotificationData, rules: Sequence[Rule] + cls, *, group: Group, data: IssueNotificationData, rules: Sequence[NotificationRule] ) -> ContainerBlock: from sentry.integrations.msteams.card_builder import ME from sentry.integrations.msteams.card_builder.block import ( diff --git a/src/sentry/notifications/platform/slack/renderers/issue.py b/src/sentry/notifications/platform/slack/renderers/issue.py index 76642c29fb1f..1447ba55c90b 100644 --- a/src/sentry/notifications/platform/slack/renderers/issue.py +++ b/src/sentry/notifications/platform/slack/renderers/issue.py @@ -34,7 +34,7 @@ def render[DataT: NotificationData]( group=group, event=event, tags=set(data.tags) if data.tags else None, - rules=[data.rule.to_rule()] if data.rule else None, + rules=[data.rule.to_notification_rule(group.project)] if data.rule else None, notes=data.notes, link_to_event=True, ).build(notification_uuid=data.notification_uuid) diff --git a/src/sentry/notifications/platform/templates/issue.py b/src/sentry/notifications/platform/templates/issue.py index 1873bd6c9a3e..08a6e2764cb3 100644 --- a/src/sentry/notifications/platform/templates/issue.py +++ b/src/sentry/notifications/platform/templates/issue.py @@ -4,7 +4,7 @@ from pydantic import BaseModel, ConfigDict -from sentry.models.rule import Rule +from sentry.models.project import Project from sentry.notifications.platform.registry import template_registry from sentry.notifications.platform.types import ( NotificationCategory, @@ -13,6 +13,7 @@ NotificationSource, NotificationTemplate, ) +from sentry.notifications.types import NotificationRule class SerializableRuleProxy(BaseModel): @@ -29,11 +30,8 @@ class SerializableRuleProxy(BaseModel): project_id: int @classmethod - def from_rule(cls, rule: Rule) -> SerializableRuleProxy: - """ - Temporary method to convert a Rule to a NotificationRuleInfo. This will - be removed once we no longer rely on the Rule ORM model. - """ + def from_rule(cls, rule: NotificationRule) -> SerializableRuleProxy: + """Create a serializable representation of a notification rule.""" return cls( id=rule.id, label=rule.label, @@ -42,17 +40,13 @@ def from_rule(cls, rule: Rule) -> SerializableRuleProxy: project_id=rule.project.id, ) - def to_rule(self) -> Rule: - """ - Temporary method to convert a NotificationRuleInfo to a Rule. This will - be removed once we no longer rely on the Rule ORM model. - """ - return Rule( + def to_notification_rule(self, project: Project) -> NotificationRule: + return NotificationRule( id=self.id, label=self.label, data=self.data, environment_id=self.environment_id, - project_id=self.project_id, + project=project, ) diff --git a/src/sentry/notifications/types.py b/src/sentry/notifications/types.py index c76f0f41f638..f4eaa91e860b 100644 --- a/src/sentry/notifications/types.py +++ b/src/sentry/notifications/types.py @@ -8,11 +8,24 @@ if TYPE_CHECKING: from sentry.models.organization import Organization - from sentry.models.rule import Rule + from sentry.models.project import Project + + +@dataclass(eq=False) +class NotificationRule: + id: int + label: str + data: dict[str, Any] + project: Project + environment_id: int | None + + @property + def project_id(self) -> int: + return self.project.id class RuleFuture(NamedTuple): - rule: Rule + rule: NotificationRule kwargs: dict[str, Any] diff --git a/src/sentry/notifications/utils/participants.py b/src/sentry/notifications/utils/participants.py index 1b7c354fb7be..ce9b87cce4d8 100644 --- a/src/sentry/notifications/utils/participants.py +++ b/src/sentry/notifications/utils/participants.py @@ -371,7 +371,6 @@ def get_send_to( notification_uuid, ) - def get_fallthrough_recipients( project: Project, fallthrough_choice: FallthroughChoiceType | None ) -> Iterable[RpcUser]: diff --git a/src/sentry/notifications/utils/rules.py b/src/sentry/notifications/utils/rules.py index 39de8a922488..7d5b51fec3df 100644 --- a/src/sentry/notifications/utils/rules.py +++ b/src/sentry/notifications/utils/rules.py @@ -3,11 +3,20 @@ from typing import Literal from sentry.models.rule import Rule +from sentry.notifications.types import NotificationRule RuleIdType = Literal["workflow_id", "legacy_rule_id"] -def get_key_from_rule_data(rule: Rule, key: str) -> str: +def get_legacy_rule_id(rule: Rule | NotificationRule) -> int | None: + if isinstance(rule, Rule): + return rule.id + + value = rule.data.get("actions", [{}])[0].get("legacy_rule_id") + return int(value) if value is not None else None + + +def get_key_from_rule_data(rule: Rule | NotificationRule, key: str) -> str: value = rule.data.get("actions", [{}])[0].get(key) assert value is not None return value @@ -15,11 +24,11 @@ def get_key_from_rule_data(rule: Rule, key: str) -> str: @dataclass class RulesAndWorkflows: - rules: list[Rule] - workflow_rules: list[Rule] # workflows as fake Rules + rules: list[NotificationRule] + workflow_rules: list[NotificationRule] -def split_rules_by_rule_workflow_id(rules: Sequence[Rule]) -> RulesAndWorkflows: +def split_rules_by_rule_workflow_id(rules: Sequence[NotificationRule]) -> RulesAndWorkflows: parsed_rules = [] workflow_rules = [] for rule in rules: @@ -33,7 +42,7 @@ def split_rules_by_rule_workflow_id(rules: Sequence[Rule]) -> RulesAndWorkflows: def get_rule_or_workflow_id( - rule: Rule, *, prefer: RuleIdType = "legacy_rule_id" + rule: Rule | NotificationRule, *, prefer: RuleIdType = "legacy_rule_id" ) -> tuple[RuleIdType, str]: """ Returns which id the rule data carries, and its value. When both a legacy diff --git a/src/sentry/rules/actions/base.py b/src/sentry/rules/actions/base.py index 4ac119e9c094..0b6ad05ecc30 100644 --- a/src/sentry/rules/actions/base.py +++ b/src/sentry/rules/actions/base.py @@ -2,16 +2,19 @@ import abc import logging -from collections.abc import Generator +from collections.abc import Generator, MutableMapping +from typing import Any -from sentry.models.rule import Rule +from sentry.notifications.types import NotificationRule from sentry.rules.base import CallbackFuture, RuleBase from sentry.services.eventstore.models import GroupEvent logger = logging.getLogger("sentry.rules") -def instantiate_action(rule: Rule, action): +def instantiate_action( + rule: NotificationRule, action: MutableMapping[str, Any] +) -> EventAction | None: from sentry.rules import rules action_id = action["id"] diff --git a/src/sentry/rules/actions/integrations/base.py b/src/sentry/rules/actions/integrations/base.py index b88b38de1b42..31e392ec162a 100644 --- a/src/sentry/rules/actions/integrations/base.py +++ b/src/sentry/rules/actions/integrations/base.py @@ -15,8 +15,7 @@ ) from sentry.mail.analytics import EmailNotificationSent from sentry.models.organization import OrganizationStatus -from sentry.models.rule import Rule -from sentry.notifications.types import RuleFuture +from sentry.notifications.types import NotificationRule, RuleFuture from sentry.rules.actions import EventAction from sentry.rules.base import CallbackFuture from sentry.services.eventstore.models import GroupEvent @@ -110,7 +109,7 @@ def record_notification_sent( self, event: GroupEvent, external_id: str, - rule: Rule | None = None, + rule: NotificationRule | None = None, notification_uuid: str | None = None, ) -> None: from sentry.integrations.discord.analytics import DiscordIntegrationNotificationSent diff --git a/src/sentry/rules/actions/integrations/create_ticket/base.py b/src/sentry/rules/actions/integrations/create_ticket/base.py index 5a5a630a2f3b..b9a7c79b8f08 100644 --- a/src/sentry/rules/actions/integrations/create_ticket/base.py +++ b/src/sentry/rules/actions/integrations/create_ticket/base.py @@ -5,7 +5,7 @@ from typing import Any from sentry.integrations.services.integration import RpcIntegration -from sentry.models.rule import Rule +from sentry.notifications.types import NotificationRule from sentry.rules.actions.integrations.base import IntegrationEventAction from sentry.rules.actions.integrations.create_ticket.form import IntegrationNotifyServiceForm from sentry.rules.actions.integrations.create_ticket.utils import create_issue @@ -18,7 +18,7 @@ class TicketEventAction(IntegrationEventAction, abc.ABC): integration_key = "integration" link: str | None - rule: Rule + rule: NotificationRule def __init__(self, *args: Any, **kwargs: Any) -> None: super(IntegrationEventAction, self).__init__(*args, **kwargs) diff --git a/src/sentry/rules/actions/integrations/create_ticket/utils.py b/src/sentry/rules/actions/integrations/create_ticket/utils.py index d0f385a73f9b..944342afbd91 100644 --- a/src/sentry/rules/actions/integrations/create_ticket/utils.py +++ b/src/sentry/rules/actions/integrations/create_ticket/utils.py @@ -138,7 +138,7 @@ def create_issue(event: GroupEvent, futures: Sequence[RuleFuture]) -> None: organization = event.group.project.organization for future in futures: - rule_id = future.rule.id + action_id = future.rule.id data: dict[str, Any] = future.kwargs["data"] provider = future.kwargs.get("provider") integration_id = future.kwargs.get("integration_id") @@ -146,8 +146,7 @@ def create_issue(event: GroupEvent, futures: Sequence[RuleFuture]) -> None: # If we invoked this handler from the notification action, we need to replace the rule_id with the legacy_rule_id, so we link notifications correctly # In the Notification Action, we store the rule_id in the action_id field - action_id = rule_id - rule_id = data.get("legacy_rule_id", rule_id) + legacy_rule_id = data.get("legacy_rule_id") integration = integration_service.get_integration( integration_id=integration_id, @@ -172,7 +171,10 @@ def create_issue(event: GroupEvent, futures: Sequence[RuleFuture]) -> None: event, workflow_id, installation, generate_footer ) else: - data["description"] = build_description(event, rule_id, installation, generate_footer) + assert isinstance(legacy_rule_id, int) + data["description"] = build_description( + event, legacy_rule_id, installation, generate_footer + ) if data.get("dynamic_form_fields"): del data["dynamic_form_fields"] @@ -182,7 +184,7 @@ def create_issue(event: GroupEvent, futures: Sequence[RuleFuture]) -> None: "%s.rule_trigger.link_already_exists", provider, extra={ - "rule_id": rule_id, + "rule_id": legacy_rule_id, "project_id": event.group.project.id, "group_id": event.group.id, }, @@ -195,7 +197,7 @@ def create_issue(event: GroupEvent, futures: Sequence[RuleFuture]) -> None: ).capture() as lifecycle: lifecycle.add_extra("provider", provider) lifecycle.add_extra("integration_id", integration.id) - lifecycle.add_extra("rule_id", rule_id) + lifecycle.add_extra("rule_id", legacy_rule_id) if action_id: lifecycle.add_extra("action_id", action_id) diff --git a/src/sentry/rules/base.py b/src/sentry/rules/base.py index 97ecbdf94aad..8a2bb333dde0 100644 --- a/src/sentry/rules/base.py +++ b/src/sentry/rules/base.py @@ -2,14 +2,13 @@ import abc import logging -from collections import namedtuple from collections.abc import Callable, MutableMapping, Sequence -from typing import TYPE_CHECKING, Any, ClassVar +from typing import TYPE_CHECKING, Any, ClassVar, NamedTuple from django import forms from sentry.models.project import Project -from sentry.notifications.types import RuleFuture +from sentry.notifications.types import NotificationRule, RuleFuture from sentry.services.eventstore.models import GroupEvent if TYPE_CHECKING: @@ -45,10 +44,14 @@ - [ACTION:I want to group events when] [RULE:an event matches [FORM]] """ + # Encapsulates a reference to the callback, including arguments. The `key` # attribute may be specifically used to key the callbacks when they are # collated during rule processing. -CallbackFuture = namedtuple("CallbackFuture", ["callback", "kwargs", "key"]) +class CallbackFuture(NamedTuple): + callback: Callable[[GroupEvent, Sequence[RuleFuture]], None] + kwargs: dict[str, Any] + key: str | None class RuleBase(abc.ABC): @@ -58,7 +61,7 @@ def __init__( self, project: Project, data: MutableMapping[str, Any] | None = None, - rule: Rule | None = None, + rule: Rule | NotificationRule | None = None, ) -> None: self.project = project self.data = data or {} diff --git a/src/sentry/rules/processing/processor.py b/src/sentry/rules/processing/processor.py index ce43c1f8b625..ca978af417eb 100644 --- a/src/sentry/rules/processing/processor.py +++ b/src/sentry/rules/processing/processor.py @@ -4,8 +4,7 @@ from collections.abc import Callable, Mapping, MutableMapping, Sequence from typing import Any -from sentry.models.rule import Rule -from sentry.notifications.types import RuleFuture +from sentry.notifications.types import NotificationRule, RuleFuture from sentry.rules import rules from sentry.rules.actions.base import instantiate_action from sentry.services.eventstore.models import GroupEvent @@ -64,14 +63,16 @@ def split_conditions_and_filters( def activate_downstream_actions( - rule: Rule, + rule: NotificationRule, event: GroupEvent, notification_uuid: str | None = None, ) -> MutableMapping[ - str, tuple[Callable[[GroupEvent, Sequence[RuleFuture]], None], list[RuleFuture]] + str | Callable[[GroupEvent, Sequence[RuleFuture]], None], + tuple[Callable[[GroupEvent, Sequence[RuleFuture]], None], list[RuleFuture]], ]: grouped_futures: MutableMapping[ - str, tuple[Callable[[GroupEvent, Sequence[RuleFuture]], None], list[RuleFuture]] + str | Callable[[GroupEvent, Sequence[RuleFuture]], None], + tuple[Callable[[GroupEvent, Sequence[RuleFuture]], None], list[RuleFuture]], ] = {} instantiated_actions = 0 diff --git a/src/sentry/sentry_apps/tasks/sentry_apps.py b/src/sentry/sentry_apps/tasks/sentry_apps.py index 059299a46842..456f62005d86 100644 --- a/src/sentry/sentry_apps/tasks/sentry_apps.py +++ b/src/sentry/sentry_apps/tasks/sentry_apps.py @@ -808,18 +808,18 @@ def notify_sentry_app(event: GroupEvent, futures: Sequence[RuleFuture]) -> None: # If the future comes from a rule with a UI component form in the schema, append the issue alert payload # TODO(ecosystem): We need to change this payload format after alerts create issues - id = f.rule.id + rule_or_workflow_id: int | str = f.rule.id # if we are using the new workflow engine, we need to use the legacy rule id # Ignore test notifications - if int(id) != -1: - _, id = get_rule_or_workflow_id(f.rule) + if int(rule_or_workflow_id) != -1: + _, rule_or_workflow_id = get_rule_or_workflow_id(f.rule) settings = f.kwargs.get("schema_defined_settings") if settings: extra_kwargs["additional_payload_key"] = "issue_alert" extra_kwargs["additional_payload"] = { - "id": int(id), + "id": int(rule_or_workflow_id), "title": f.rule.label, "sentry_app_id": f.kwargs["sentry_app"].id, "settings": settings, diff --git a/src/sentry/types/rules.py b/src/sentry/types/rules.py index 216c104276d2..3c3b993db77d 100644 --- a/src/sentry/types/rules.py +++ b/src/sentry/types/rules.py @@ -2,7 +2,6 @@ from sentry.notifications.types import RuleFuture as RuleFuture - @dataclass class NotificationRuleDetails: """ diff --git a/tests/sentry/integrations/github/test_ticket_action.py b/tests/sentry/integrations/github/test_ticket_action.py index 95db76d8dce5..cce777d2e025 100644 --- a/tests/sentry/integrations/github/test_ticket_action.py +++ b/tests/sentry/integrations/github/test_ticket_action.py @@ -14,6 +14,7 @@ from sentry.issues.action_log.types import CreateExternalIssueAction from sentry.models.activity import Activity from sentry.models.repository import Repository +from sentry.notifications.types import RuleFuture from sentry.services.eventstore.models import GroupEvent from sentry.silo.base import SiloMode from sentry.testutils.cases import RuleTestCase @@ -22,7 +23,6 @@ from sentry.testutils.silo import assume_test_silo_mode from sentry.testutils.skips import requires_snuba from sentry.types.activity import ActivityType -from sentry.types.rules import RuleFuture pytestmark = [requires_snuba] diff --git a/tests/sentry/integrations/github_enterprise/test_ticket_action.py b/tests/sentry/integrations/github_enterprise/test_ticket_action.py index 3ee2f5836c5e..7583f7add094 100644 --- a/tests/sentry/integrations/github_enterprise/test_ticket_action.py +++ b/tests/sentry/integrations/github_enterprise/test_ticket_action.py @@ -14,6 +14,7 @@ from sentry.integrations.models.external_issue import ExternalIssue from sentry.models.activity import Activity from sentry.models.repository import Repository +from sentry.notifications.types import RuleFuture from sentry.rules import rules from sentry.services.eventstore.models import GroupEvent from sentry.silo.base import SiloMode @@ -22,7 +23,6 @@ from sentry.testutils.silo import assume_test_silo_mode from sentry.testutils.skips import requires_snuba from sentry.types.activity import ActivityType -from sentry.types.rules import RuleFuture pytestmark = [requires_snuba] diff --git a/tests/sentry/integrations/jira/test_notify_action.py b/tests/sentry/integrations/jira/test_notify_action.py index 21b4636cd66a..91416c4125c3 100644 --- a/tests/sentry/integrations/jira/test_notify_action.py +++ b/tests/sentry/integrations/jira/test_notify_action.py @@ -4,12 +4,12 @@ from sentry.integrations.jira import JiraCreateTicketAction from sentry.integrations.models.external_issue import ExternalIssue from sentry.models.grouplink import GroupLink +from sentry.notifications.types import RuleFuture from sentry.silo.base import SiloMode from sentry.testutils.cases import PerformanceIssueTestCase, RuleTestCase from sentry.testutils.helpers.notifications import TEST_ISSUE_OCCURRENCE from sentry.testutils.silo import assume_test_silo_mode from sentry.testutils.skips import requires_snuba -from sentry.types.rules import RuleFuture from sentry.utils import json pytestmark = [requires_snuba] diff --git a/tests/sentry/integrations/jira/test_ticket_action.py b/tests/sentry/integrations/jira/test_ticket_action.py index 1643eda5fa2b..7d8f8e931dd6 100644 --- a/tests/sentry/integrations/jira/test_ticket_action.py +++ b/tests/sentry/integrations/jira/test_ticket_action.py @@ -10,6 +10,7 @@ from sentry.integrations.models.external_issue import ExternalIssue from sentry.integrations.types import EventLifecycleOutcome from sentry.models.activity import Activity +from sentry.notifications.types import RuleFuture from sentry.services.eventstore.models import GroupEvent from sentry.shared_integrations.exceptions import ( ApiInvalidRequestError, @@ -20,7 +21,6 @@ from sentry.testutils.cases import RuleTestCase from sentry.testutils.skips import requires_snuba from sentry.types.activity import ActivityType -from sentry.types.rules import RuleFuture from sentry.utils import json pytestmark = [requires_snuba] diff --git a/tests/sentry/integrations/jira_server/test_ticket_action.py b/tests/sentry/integrations/jira_server/test_ticket_action.py index 022fe880efcb..3191929ddeb4 100644 --- a/tests/sentry/integrations/jira_server/test_ticket_action.py +++ b/tests/sentry/integrations/jira_server/test_ticket_action.py @@ -7,12 +7,12 @@ from sentry.integrations.jira_server import JiraServerCreateTicketAction, JiraServerIntegration from sentry.integrations.models.external_issue import ExternalIssue from sentry.models.rule import Rule +from sentry.notifications.types import RuleFuture from sentry.services.eventstore.models import GroupEvent from sentry.silo.base import SiloMode from sentry.testutils.cases import RuleTestCase from sentry.testutils.silo import assume_test_silo_mode from sentry.testutils.skips import requires_snuba -from sentry.types.rules import RuleFuture from tests.sentry.integrations.jira_server import EXAMPLE_PRIVATE_KEY pytestmark = [requires_snuba] diff --git a/tests/sentry/integrations/msteams/test_message_builder.py b/tests/sentry/integrations/msteams/test_message_builder.py index 9f736e01d6ec..54aeb60fbca0 100644 --- a/tests/sentry/integrations/msteams/test_message_builder.py +++ b/tests/sentry/integrations/msteams/test_message_builder.py @@ -49,6 +49,7 @@ from sentry.models.group import GroupStatus from sentry.models.groupassignee import GroupAssignee from sentry.models.organization import Organization +from sentry.notifications.types import NotificationRule from sentry.testutils.cases import TestCase from sentry.testutils.helpers.notifications import ( DummyNotification, @@ -443,6 +444,35 @@ def test_issue_without_description(self) -> None: assert 3 == len(issue_card["body"]) + def test_action_payload_uses_only_legacy_rule_ids(self) -> None: + legacy_rule = self.rules[0] + rules = [ + NotificationRule( + id=legacy_rule.id + 1000, + label="Workflow with legacy rule", + data={"actions": [{"legacy_rule_id": legacy_rule.id}]}, + project=self.project1, + environment_id=None, + ), + NotificationRule( + id=legacy_rule.id + 2000, + label="Workflow only", + data={"actions": [{"workflow_id": 123}]}, + project=self.project1, + environment_id=None, + ), + ] + builder = MSTeamsIssueMessageBuilder( + group=self.group1, + event=self.event1, + rules=rules, + integration=self.integration, + ) + + payload = builder.generate_action_payload(ACTION_TYPE.RESOLVE) + + assert payload["payload"]["rules"] == [legacy_rule.id] + def test_issue_with_only_one_rule(self) -> None: one_rule = self.rules[:1] issue_card = MSTeamsIssueMessageBuilder( diff --git a/tests/sentry/integrations/pagerduty/test_notification.py b/tests/sentry/integrations/pagerduty/test_notification.py index dfe2a4c22809..0b4691199872 100644 --- a/tests/sentry/integrations/pagerduty/test_notification.py +++ b/tests/sentry/integrations/pagerduty/test_notification.py @@ -11,6 +11,7 @@ from sentry.integrations.pagerduty.client import PAGERDUTY_SUMMARY_MAX_LENGTH from sentry.integrations.pagerduty.utils import add_service from sentry.integrations.types import EventLifecycleOutcome +from sentry.notifications.types import RuleFuture from sentry.silo.base import SiloMode from sentry.testutils.asserts import assert_halt_metric, assert_slo_metric from sentry.testutils.cases import PerformanceIssueTestCase, RuleTestCase @@ -22,7 +23,6 @@ from sentry.testutils.helpers.notifications import TEST_ISSUE_OCCURRENCE from sentry.testutils.silo import assume_test_silo_mode from sentry.testutils.skips import requires_snuba -from sentry.types.rules import RuleFuture pytestmark = [requires_snuba] diff --git a/tests/sentry/integrations/slack/actions/notification/test_slack_notify_service_action.py b/tests/sentry/integrations/slack/actions/notification/test_slack_notify_service_action.py index fed5f6d9e6d9..1f5f1be8a45f 100644 --- a/tests/sentry/integrations/slack/actions/notification/test_slack_notify_service_action.py +++ b/tests/sentry/integrations/slack/actions/notification/test_slack_notify_service_action.py @@ -8,12 +8,12 @@ from sentry.integrations.slack import SlackNotifyServiceAction from sentry.integrations.types import EventLifecycleOutcome from sentry.notifications.models.notificationmessage import NotificationMessage +from sentry.notifications.types import RuleFuture from sentry.shared_integrations.exceptions import IntegrationError from sentry.silo.base import SiloMode from sentry.testutils.asserts import assert_failure_metric from sentry.testutils.cases import RuleTestCase from sentry.testutils.silo import assume_test_silo_mode -from sentry.types.rules import RuleFuture class TestInit(RuleTestCase): diff --git a/tests/sentry/integrations/vsts/test_notify_action.py b/tests/sentry/integrations/vsts/test_notify_action.py index 4e72b40ef11e..b15c912788d9 100644 --- a/tests/sentry/integrations/vsts/test_notify_action.py +++ b/tests/sentry/integrations/vsts/test_notify_action.py @@ -8,12 +8,12 @@ from sentry.integrations.vsts import AzureDevopsCreateTicketAction from sentry.integrations.vsts.integration import VstsIntegration from sentry.models.grouplink import GroupLink +from sentry.notifications.types import RuleFuture from sentry.silo.base import SiloMode from sentry.testutils.cases import RuleTestCase from sentry.testutils.helpers.datetime import freeze_time from sentry.testutils.silo import assume_test_silo_mode from sentry.testutils.skips import requires_snuba -from sentry.types.rules import RuleFuture from .test_issues import VstsIssueBase diff --git a/tests/sentry/notifications/notification_action/test_issue_alert_registry_handlers.py b/tests/sentry/notifications/notification_action/test_issue_alert_registry_handlers.py index 91cb71725df1..12194015d743 100644 --- a/tests/sentry/notifications/notification_action/test_issue_alert_registry_handlers.py +++ b/tests/sentry/notifications/notification_action/test_issue_alert_registry_handlers.py @@ -3,8 +3,6 @@ import pytest -from sentry.constants import ObjectStatus -from sentry.models.rule import Rule, RuleSource from sentry.notifications.models.notificationaction import ActionTarget from sentry.notifications.notification_action.issue_alert_registry import ( AzureDevopsIssueAlertHandler, @@ -25,7 +23,12 @@ BaseIssueAlertHandler, TicketingIssueAlertHandler, ) -from sentry.notifications.types import TEST_NOTIFICATION_ID, ActionTargetType, FallthroughChoiceType +from sentry.notifications.types import ( + TEST_NOTIFICATION_ID, + ActionTargetType, + FallthroughChoiceType, + NotificationRule, +) from sentry.testutils.helpers.data_blobs import ( AZURE_DEVOPS_ACTION_DATA_BLOBS, EMAIL_ACTION_DATA_BLOBS, @@ -112,12 +115,12 @@ def get_additional_fields(cls, action: Action, mapping: ActionFieldMapping): ) def test_create_rule_instance_from_action(self) -> None: - """Test that create_rule_instance_from_action creates a Rule with correct attributes""" + """Test that create_rule_instance_from_action creates a notification rule.""" rule = self.handler.create_rule_instance_from_action( self.action, self.detector, self.event_data, workflow_id=self.workflow.id ) - assert isinstance(rule, Rule) + assert isinstance(rule, NotificationRule) assert rule.id == self.action.id assert rule.project == self.detector.project assert rule.environment_id is not None @@ -136,17 +139,21 @@ def test_create_rule_instance_from_action(self) -> None: } ], } - assert rule.status == ObjectStatus.ACTIVE - assert rule.source == RuleSource.ISSUE + assert rule.project_id == self.detector.project.id + + other_rule = self.handler.create_rule_instance_from_action( + self.action, self.detector, self.event_data, workflow_id=self.workflow.id + ) + assert len({rule: "first", other_rule: "second"}) == 2 def test_create_rule_instance_from_action_with_workflow_only(self) -> None: - """Test that create_rule_instance_from_action creates a Rule with correct attributes""" + """Test that create_rule_instance_from_action creates a notification rule.""" self.rule.delete() rule = self.handler.create_rule_instance_from_action( self.action, self.detector, self.event_data, workflow_id=self.workflow.id ) - assert isinstance(rule, Rule) + assert isinstance(rule, NotificationRule) assert rule.id == self.action.id assert rule.project == self.detector.project assert rule.environment_id is not None @@ -164,8 +171,6 @@ def test_create_rule_instance_from_action_with_workflow_only(self) -> None: } ] } - assert rule.status == ObjectStatus.ACTIVE - assert rule.source == RuleSource.ISSUE def test_create_rule_instance_from_action_deleted_workflow_falls_back_to_detector_name( self, @@ -177,7 +182,7 @@ def test_create_rule_instance_from_action_deleted_workflow_falls_back_to_detecto self.action, self.detector, self.event_data, workflow_id=workflow_id ) - assert isinstance(rule, Rule) + assert isinstance(rule, NotificationRule) assert rule.label == self.detector.name assert rule.data == { "actions": [ @@ -198,7 +203,7 @@ def test_rule_instance_from_action_uses_workflow_name_not_stale_rule_label( rule = self.handler.create_rule_instance_from_action( self.action, self.detector, self.event_data, workflow_id=self.workflow.id ) - assert isinstance(rule, Rule) + assert isinstance(rule, NotificationRule) assert rule.label == "Renamed Alert Name" assert rule.label != self.rule.label # legacy rule label is still "Test Alert" @@ -208,7 +213,7 @@ def test_create_rule_instance_from_action_with_test_notification_id(self) -> Non self.action, self.detector, self.event_data, workflow_id=TEST_NOTIFICATION_ID ) - assert isinstance(rule, Rule) + assert isinstance(rule, NotificationRule) assert rule.label == self.detector.name assert rule.data == { "actions": [ @@ -223,14 +228,14 @@ def test_create_rule_instance_from_action_with_test_notification_id(self) -> Non } def test_create_rule_instance_from_action_no_environment(self) -> None: - """Test that create_rule_instance_from_action creates a Rule with correct attributes""" + """Test that create_rule_instance_from_action creates a notification rule.""" self.create_workflow() job = WorkflowEventData(event=self.group_event, workflow_env=None, group=self.group) rule = self.handler.create_rule_instance_from_action( self.action, self.detector, job, workflow_id=self.workflow.id ) - assert isinstance(rule, Rule) + assert isinstance(rule, NotificationRule) assert rule.id == self.action.id assert rule.project == self.detector.project assert rule.environment_id is None @@ -247,8 +252,6 @@ def test_create_rule_instance_from_action_no_environment(self) -> None: } ], } - assert rule.status == ObjectStatus.ACTIVE - assert rule.source == RuleSource.ISSUE @mock.patch("sentry.notifications.notification_action.types.invoke_future_with_error_handling") @mock.patch("sentry.notifications.notification_action.types.activate_downstream_actions") diff --git a/tests/sentry/notifications/platform/msteams/renderers/test_issue.py b/tests/sentry/notifications/platform/msteams/renderers/test_issue.py index 79ab27fc6926..a4163057bb99 100644 --- a/tests/sentry/notifications/platform/msteams/renderers/test_issue.py +++ b/tests/sentry/notifications/platform/msteams/renderers/test_issue.py @@ -115,7 +115,7 @@ def _build_expected_card( label="Test Detector", data={"actions": [{"workflow_id": 1}]}, project_id=self.project.id, - ).to_rule() + ).to_notification_rule(project) ] footer_text = build_footer( group=group, project=project, url_format=MSTEAMS_URL_FORMAT, rules=rules @@ -148,7 +148,7 @@ def payload(action_type: ACTION_TYPE) -> dict[str, Any]: "actionType": action_type, "groupId": group.id, "eventId": event.event_id, - "rules": [1], + "rules": [], "workflows": [1], } } diff --git a/tests/sentry/notifications/platform/msteams/renderers/test_issue_parity.py b/tests/sentry/notifications/platform/msteams/renderers/test_issue_parity.py index abb0e75bbf87..205c75d9f42c 100644 --- a/tests/sentry/notifications/platform/msteams/renderers/test_issue_parity.py +++ b/tests/sentry/notifications/platform/msteams/renderers/test_issue_parity.py @@ -13,6 +13,7 @@ SerializableRuleProxy, ) from sentry.notifications.platform.types import NotificationRenderedTemplate +from sentry.notifications.types import NotificationRule from sentry.testutils.cases import TestCase @@ -64,7 +65,15 @@ def platform_card(self) -> AdaptiveCard: group_id=self.issue_group.id, event_id=self.event.event_id, notification_uuid="", - rule=SerializableRuleProxy.from_rule(self.rule), + rule=SerializableRuleProxy.from_rule( + NotificationRule( + id=self.rule.id, + label=self.rule.label, + data=self.rule.data, + project=self.project, + environment_id=self.rule.environment_id, + ) + ), ) return IssueMSTeamsRenderer.render( data=data, diff --git a/tests/sentry/notifications/utils/test_participants.py b/tests/sentry/notifications/utils/test_participants.py index e2031bab02d6..c15670e01505 100644 --- a/tests/sentry/notifications/utils/test_participants.py +++ b/tests/sentry/notifications/utils/test_participants.py @@ -130,7 +130,6 @@ def test_no_project_access(self) -> None: ) assert self.get_send_to_member(self.project, user_3.id) == {} - class GetSendToTeamTest(_ParticipantsTest): def setUp(self) -> None: super().setUp() diff --git a/tests/sentry/sentry_apps/tasks/test_sentry_apps.py b/tests/sentry/sentry_apps/tasks/test_sentry_apps.py index 42f66d26d498..1518fd3a49d5 100644 --- a/tests/sentry/sentry_apps/tasks/test_sentry_apps.py +++ b/tests/sentry/sentry_apps/tasks/test_sentry_apps.py @@ -22,6 +22,7 @@ from sentry.issues.grouptype import FeedbackGroup from sentry.issues.ingest import save_issue_occurrence from sentry.models.activity import Activity +from sentry.notifications.types import RuleFuture from sentry.sentry_apps.metrics import SentryAppWebhookFailureReason, SentryAppWebhookHaltReason from sentry.sentry_apps.models.sentry_app import SentryApp from sentry.sentry_apps.models.sentry_app_installation import SentryAppInstallation @@ -56,7 +57,6 @@ from sentry.testutils.silo import assume_test_silo_mode, assume_test_silo_mode_of, control_silo_test from sentry.testutils.skips import requires_snuba from sentry.types.activity import ActivityType -from sentry.types.rules import RuleFuture from sentry.users.services.user.service import user_service from sentry.utils import json from sentry.utils.http import absolute_uri From 676218aff1983f321492597455269dea68f5a9e9 Mon Sep 17 00:00:00 2001 From: Kyle Consalus Date: Wed, 30 Sep 2026 16:41:38 -0700 Subject: [PATCH 02/12] stricter --- src/sentry/digests/notifications.py | 13 ++++- .../discord/message_builder/issues.py | 55 ++++++++++--------- src/sentry/integrations/opsgenie/client.py | 27 +++++---- .../notification_action/types.py | 22 ++++---- .../notifications/platform/templates/issue.py | 14 +++-- src/sentry/notifications/types.py | 39 ++++++++++++- src/sentry/notifications/utils/rules.py | 28 +++++++--- .../integrations/create_ticket/utils.py | 4 +- src/sentry/sentry_apps/tasks/sentry_apps.py | 11 ++-- .../integrations/github/test_ticket_action.py | 13 ++++- .../github_enterprise/test_ticket_action.py | 13 ++++- .../integrations/jira/test_ticket_action.py | 13 ++++- .../jira_server/test_ticket_action.py | 13 ++++- .../msteams/test_message_builder.py | 4 ++ .../integrations/vsts/test_notify_action.py | 17 +++++- .../test_issue_alert_registry_handlers.py | 50 +++++++++++++++++ .../platform/discord/renderers/test_issue.py | 16 +++++- .../platform/msteams/renderers/test_issue.py | 25 ++++++++- .../msteams/renderers/test_issue_parity.py | 2 + .../platform/slack/renderers/test_issue.py | 16 +++++- .../sentry_apps/tasks/test_sentry_apps.py | 41 +++++++++++--- 21 files changed, 337 insertions(+), 99 deletions(-) diff --git a/src/sentry/digests/notifications.py b/src/sentry/digests/notifications.py index ccec069b2414..d88474553ecd 100644 --- a/src/sentry/digests/notifications.py +++ b/src/sentry/digests/notifications.py @@ -78,7 +78,7 @@ def unsplit_key( def event_to_record( event: Event | GroupEvent, - rules: Sequence[NotificationRule], + rules: Sequence[Rule | NotificationRule], notification_uuid: str | None = None, identifier_key: IdentifierKey = IdentifierKey.RULE, ) -> Record: @@ -88,7 +88,16 @@ def event_to_record( # TODO(iamrajjoshi): The typing on this function is wrong, the type should be GroupEvent # TODO(iamrajjoshi): Creating a PR to fix this assert event.group is not None - rule_ids = [int(get_rule_or_workflow_id(rule)[1]) for rule in rules] + rule_ids = [] + for rule in rules: + if isinstance(rule, NotificationRule): + rule_id = ( + rule.legacy_rule_id if identifier_key == IdentifierKey.RULE else rule.workflow_id + ) + assert rule_id is not None + else: + rule_id = int(get_rule_or_workflow_id(rule)[1]) + rule_ids.append(rule_id) return Record( event.event_id, Notification(event, rule_ids, notification_uuid, identifier_key), diff --git a/src/sentry/integrations/discord/message_builder/issues.py b/src/sentry/integrations/discord/message_builder/issues.py index b8550a7af301..430610fea907 100644 --- a/src/sentry/integrations/discord/message_builder/issues.py +++ b/src/sentry/integrations/discord/message_builder/issues.py @@ -25,7 +25,7 @@ from sentry.models.project import Project from sentry.notifications.notifications.base import ProjectNotification from sentry.notifications.types import NotificationRule -from sentry.notifications.utils.rules import RuleIdType, get_rule_or_workflow_id +from sentry.notifications.utils.rules import get_rule_or_workflow_id from sentry.services.eventstore.models import GroupEvent from ..message_builder.base.component import DiscordComponentCustomIds as CustomIds @@ -59,37 +59,38 @@ def build(self, notification_uuid: str | None = None) -> DiscordMessage: obj: Group | GroupEvent = self.event if self.event is not None else self.group rule_id = None rule_environment_id = None - key: RuleIdType = "legacy_rule_id" + is_workflow = False if self.rules: rule_environment_id = self.rules[0].environment_id key, rule_id = get_rule_or_workflow_id(self.rules[0], prefer="workflow_id") + is_workflow = key == "workflow_id" + rule_id = int(rule_id) url = None - match key: - case "workflow_id": - url = get_title_link_workflow_engine_ui( - self.group, - self.event, - self.link_to_event, - self.issue_details, - self.notification, - ExternalProviders.DISCORD, - int(rule_id) if rule_id else None, - rule_environment_id, - notification_uuid=notification_uuid, - ) - case "legacy_rule_id": - url = get_title_link( - self.group, - self.event, - self.link_to_event, - self.issue_details, - self.notification, - ExternalProviders.DISCORD, - int(rule_id) if rule_id else None, - rule_environment_id, - notification_uuid=notification_uuid, - ) + if is_workflow: + url = get_title_link_workflow_engine_ui( + self.group, + self.event, + self.link_to_event, + self.issue_details, + self.notification, + ExternalProviders.DISCORD, + rule_id, + rule_environment_id, + notification_uuid=notification_uuid, + ) + else: + url = get_title_link( + self.group, + self.event, + self.link_to_event, + self.issue_details, + self.notification, + ExternalProviders.DISCORD, + rule_id, + rule_environment_id, + notification_uuid=notification_uuid, + ) embeds = [ DiscordMessageEmbed( diff --git a/src/sentry/integrations/opsgenie/client.py b/src/sentry/integrations/opsgenie/client.py index 937c47b2e91c..d397d1e7fb58 100644 --- a/src/sentry/integrations/opsgenie/client.py +++ b/src/sentry/integrations/opsgenie/client.py @@ -1,6 +1,7 @@ from __future__ import annotations -from typing import Literal +from collections.abc import Sequence +from typing import Any, Literal from sentry.integrations.client import ApiClient from sentry.integrations.models.integration import Integration @@ -9,8 +10,9 @@ from sentry.integrations.services.integration.model import RpcIntegration from sentry.integrations.types import IntegrationProviderSlug from sentry.models.group import Group +from sentry.notifications.types import NotificationRule from sentry.notifications.utils.links import create_link_to_workflow -from sentry.notifications.utils.rules import get_key_from_rule_data, split_rules_by_rule_workflow_id +from sentry.notifications.utils.rules import split_rules_by_rule_workflow_id from sentry.services.eventstore.models import Event, GroupEvent from sentry.shared_integrations.exceptions import ApiError @@ -41,35 +43,38 @@ def get_alerts(self, limit: int | None = 1) -> object | None: path = f"/alerts?limit={limit}" return self.get(path=path, headers=self._get_auth_headers()) - def _get_workflow_urls(self, group, rules): + def _get_workflow_urls(self, group: Group, rules: Sequence[NotificationRule]) -> list[str]: organization = group.project.organization workflow_urls = [] for rule in rules: - # fetch the workflow_id from the rule.data - workflow_id = get_key_from_rule_data(rule, "workflow_id") + workflow_id = rule.workflow_id + assert workflow_id is not None workflow_urls.append( - organization.absolute_url(create_link_to_workflow(organization.slug, workflow_id)) + organization.absolute_url( + create_link_to_workflow(organization.slug, str(workflow_id)) + ) ) return workflow_urls - def _get_rule_urls(self, group, rules): + def _get_rule_urls(self, group: Group, rules: Sequence[NotificationRule]) -> list[str]: organization = group.project.organization rule_urls = [] for rule in rules: - rule_id = get_key_from_rule_data(rule, "legacy_rule_id") + rule_id = rule.legacy_rule_id + assert rule_id is not None path = f"/organizations/{organization.slug}/issues/alerts/rules/{group.project.slug}/{rule_id}/details/" rule_urls.append(organization.absolute_url(path)) return rule_urls def build_issue_alert_payload( self, - data, - rules, + data: Any, + rules: Sequence[NotificationRule], event: Event | GroupEvent, group: Group | None, priority: OpsgeniePriority | None = "P3", notification_uuid: str | None = None, - ): + ) -> dict[str, Any]: payload = { "message": event.message or event.title, "source": "Sentry", diff --git a/src/sentry/notifications/notification_action/types.py b/src/sentry/notifications/notification_action/types.py index 2229ca9ea6f4..a2a93231b2ba 100644 --- a/src/sentry/notifications/notification_action/types.py +++ b/src/sentry/notifications/notification_action/types.py @@ -2,7 +2,7 @@ from abc import ABC, abstractmethod from collections.abc import Callable, Collection, Sequence from dataclasses import asdict -from typing import Any, ClassVar, NotRequired, Protocol, TypedDict +from typing import Any, ClassVar, Protocol from django.core.exceptions import ValidationError from taskbroker_client.retry import RetryTaskError @@ -24,7 +24,12 @@ from sentry.models.organization import Organization from sentry.models.project import Project from sentry.models.rule import Rule -from sentry.notifications.types import TEST_NOTIFICATION_ID, NotificationRule, RuleFuture +from sentry.notifications.types import ( + TEST_NOTIFICATION_ID, + NotificationRule, + NotificationRuleData, + RuleFuture, +) from sentry.notifications.utils.issue_notification_context import IssueNotificationContext from sentry.rules.processing.processor import activate_downstream_actions from sentry.services.eventstore.models import GroupEvent @@ -52,11 +57,6 @@ FutureCallback = Callable[[GroupEvent, Sequence[RuleFuture]], Any] -class RuleData(TypedDict): - actions: list[dict[str, Any]] - legacy_rule_id: NotRequired[int] - - class LegacyRegistryHandler(ABC): """ Abstract base class that defines the interface for notification handlers. @@ -217,7 +217,7 @@ def create_rule_instance_from_action( """ environment_id = event_data.workflow_env.id if event_data.workflow_env else None - data: RuleData = { + data: NotificationRuleData = { "actions": [ cls.build_rule_action_blob(action, detector.linked_project.organization.id) ], @@ -276,7 +276,9 @@ def create_rule_instance_from_action( project=detector.linked_project, environment_id=environment_id, label=label, - data=dict(data), + data=data, + workflow_id=(None if workflow_id == TEST_NOTIFICATION_ID else workflow_id), + legacy_rule_id=(data["actions"][0].get("legacy_rule_id")), ) return rule @@ -370,7 +372,7 @@ def invoke_legacy_registry(cls, invocation: ActionInvocation) -> None: # Execute the futures # If the rule id is -1, we are sending a test notification - if rule.id == TEST_NOTIFICATION_ID: + if rule.is_test_notification: cls.send_test_notification(invocation.event_data, futures) else: cls.execute_futures(invocation.event_data, futures) diff --git a/src/sentry/notifications/platform/templates/issue.py b/src/sentry/notifications/platform/templates/issue.py index 08a6e2764cb3..77d3dd304ace 100644 --- a/src/sentry/notifications/platform/templates/issue.py +++ b/src/sentry/notifications/platform/templates/issue.py @@ -1,7 +1,5 @@ from __future__ import annotations -from typing import Any - from pydantic import BaseModel, ConfigDict from sentry.models.project import Project @@ -13,7 +11,7 @@ NotificationSource, NotificationTemplate, ) -from sentry.notifications.types import NotificationRule +from sentry.notifications.types import NotificationRule, NotificationRuleData class SerializableRuleProxy(BaseModel): @@ -25,9 +23,11 @@ class SerializableRuleProxy(BaseModel): id: int label: str - data: dict[str, Any] + data: NotificationRuleData environment_id: int | None = None project_id: int + workflow_id: int | None + legacy_rule_id: int | None @classmethod def from_rule(cls, rule: NotificationRule) -> SerializableRuleProxy: @@ -38,6 +38,8 @@ def from_rule(cls, rule: NotificationRule) -> SerializableRuleProxy: data=rule.data, environment_id=rule.environment_id, project_id=rule.project.id, + workflow_id=rule.workflow_id, + legacy_rule_id=rule.legacy_rule_id, ) def to_notification_rule(self, project: Project) -> NotificationRule: @@ -47,6 +49,8 @@ def to_notification_rule(self, project: Project) -> NotificationRule: data=self.data, environment_id=self.environment_id, project=project, + workflow_id=self.workflow_id, + legacy_rule_id=self.legacy_rule_id, ) @@ -77,6 +81,8 @@ class IssueNotificationTemplate(NotificationTemplate[IssueNotificationData]): data={ "actions": [{"workflow_id": 3}], }, + workflow_id=3, + legacy_rule_id=None, ), ) hide_from_debugger = True diff --git a/src/sentry/notifications/types.py b/src/sentry/notifications/types.py index f4eaa91e860b..3811e6eb3157 100644 --- a/src/sentry/notifications/types.py +++ b/src/sentry/notifications/types.py @@ -2,7 +2,7 @@ from dataclasses import dataclass from enum import Enum, StrEnum -from typing import TYPE_CHECKING, Any, NamedTuple +from typing import TYPE_CHECKING, Any, NamedTuple, TypedDict from sentry.hybridcloud.rpc import ValueEqualityEnum @@ -11,13 +11,48 @@ from sentry.models.project import Project +class NotificationRuleData(TypedDict): + actions: list[dict[str, Any]] + + @dataclass(eq=False) class NotificationRule: + """Rule-like notification context for the legacy action registry. + + ``id`` identifies the source of this delivery. It is usually a workflow-engine + Action ID, but its domain is not stable across every notification path. It must + not be used to look up a persisted Rule; use ``legacy_rule_id`` explicitly. + """ + id: int label: str - data: dict[str, Any] + data: NotificationRuleData project: Project environment_id: int | None + workflow_id: int | None + legacy_rule_id: int | None + + def __post_init__(self) -> None: + if not self.data["actions"]: + raise ValueError("NotificationRule requires at least one action") + + if self.legacy_rule_id == TEST_NOTIFICATION_ID: + if self.workflow_id is not None: + raise ValueError("Test notification cannot have a workflow ID") + elif self.workflow_id is None or self.workflow_id == TEST_NOTIFICATION_ID: + raise ValueError("NotificationRule requires a workflow ID") + + @property + def is_test_notification(self) -> bool: + return self.legacy_rule_id == TEST_NOTIFICATION_ID + + @property + def is_workflow_only(self) -> bool: + return self.workflow_id is not None and self.legacy_rule_id is None + + @property + def is_workflow_with_legacy_rule(self) -> bool: + return self.workflow_id is not None and self.legacy_rule_id is not None @property def project_id(self) -> int: diff --git a/src/sentry/notifications/utils/rules.py b/src/sentry/notifications/utils/rules.py index 7d5b51fec3df..c10ca7b7a961 100644 --- a/src/sentry/notifications/utils/rules.py +++ b/src/sentry/notifications/utils/rules.py @@ -11,24 +11,34 @@ def get_legacy_rule_id(rule: Rule | NotificationRule) -> int | None: if isinstance(rule, Rule): return rule.id - - value = rule.data.get("actions", [{}])[0].get("legacy_rule_id") - return int(value) if value is not None else None + return rule.legacy_rule_id def get_key_from_rule_data(rule: Rule | NotificationRule, key: str) -> str: + if isinstance(rule, NotificationRule): + if key == "legacy_rule_id": + value = rule.legacy_rule_id + elif key == "workflow_id": + value = rule.workflow_id + else: + raise KeyError(key) + assert value is not None + return str(value) + value = rule.data.get("actions", [{}])[0].get(key) assert value is not None return value @dataclass -class RulesAndWorkflows: - rules: list[NotificationRule] - workflow_rules: list[NotificationRule] +class RulesAndWorkflows[RuleT: Rule | NotificationRule]: + rules: list[RuleT] + workflow_rules: list[RuleT] -def split_rules_by_rule_workflow_id(rules: Sequence[NotificationRule]) -> RulesAndWorkflows: +def split_rules_by_rule_workflow_id[RuleT: Rule | NotificationRule]( + rules: Sequence[RuleT], +) -> RulesAndWorkflows[RuleT]: parsed_rules = [] workflow_rules = [] for rule in rules: @@ -58,4 +68,6 @@ def get_rule_or_workflow_id( return (key, get_key_from_rule_data(rule, key)) except AssertionError: pass - return ("legacy_rule_id", str(rule.id)) + if isinstance(rule, Rule): + return ("legacy_rule_id", str(rule.id)) + raise AssertionError("NotificationRule must have a workflow or legacy rule ID") diff --git a/src/sentry/rules/actions/integrations/create_ticket/utils.py b/src/sentry/rules/actions/integrations/create_ticket/utils.py index 944342afbd91..0e9dd7ec39c0 100644 --- a/src/sentry/rules/actions/integrations/create_ticket/utils.py +++ b/src/sentry/rules/actions/integrations/create_ticket/utils.py @@ -146,7 +146,7 @@ def create_issue(event: GroupEvent, futures: Sequence[RuleFuture]) -> None: # If we invoked this handler from the notification action, we need to replace the rule_id with the legacy_rule_id, so we link notifications correctly # In the Notification Action, we store the rule_id in the action_id field - legacy_rule_id = data.get("legacy_rule_id") + legacy_rule_id = future.rule.legacy_rule_id integration = integration_service.get_integration( integration_id=integration_id, @@ -165,7 +165,7 @@ def create_issue(event: GroupEvent, futures: Sequence[RuleFuture]) -> None: ) data["title"] = installation.get_group_title(event.group, event) - workflow_id = data.get("workflow_id") + workflow_id = future.rule.workflow_id if workflow_id is not None: data["description"] = build_description_workflow_engine_ui( event, workflow_id, installation, generate_footer diff --git a/src/sentry/sentry_apps/tasks/sentry_apps.py b/src/sentry/sentry_apps/tasks/sentry_apps.py index 456f62005d86..f8742488ff2d 100644 --- a/src/sentry/sentry_apps/tasks/sentry_apps.py +++ b/src/sentry/sentry_apps/tasks/sentry_apps.py @@ -46,7 +46,6 @@ from sentry.models.organizationmapping import OrganizationMapping from sentry.models.project import Project from sentry.notifications.types import RuleFuture -from sentry.notifications.utils.rules import get_rule_or_workflow_id from sentry.sentry_apps.api.serializers.app_platform_event import AppPlatformEvent from sentry.sentry_apps.event_types import SentryAppEventType from sentry.sentry_apps.metrics import ( @@ -808,12 +807,10 @@ def notify_sentry_app(event: GroupEvent, futures: Sequence[RuleFuture]) -> None: # If the future comes from a rule with a UI component form in the schema, append the issue alert payload # TODO(ecosystem): We need to change this payload format after alerts create issues - rule_or_workflow_id: int | str = f.rule.id - - # if we are using the new workflow engine, we need to use the legacy rule id - # Ignore test notifications - if int(rule_or_workflow_id) != -1: - _, rule_or_workflow_id = get_rule_or_workflow_id(f.rule) + rule_or_workflow_id = ( + f.rule.legacy_rule_id if f.rule.legacy_rule_id is not None else f.rule.workflow_id + ) + assert rule_or_workflow_id is not None settings = f.kwargs.get("schema_defined_settings") if settings: diff --git a/tests/sentry/integrations/github/test_ticket_action.py b/tests/sentry/integrations/github/test_ticket_action.py index cce777d2e025..1c03c3b720cb 100644 --- a/tests/sentry/integrations/github/test_ticket_action.py +++ b/tests/sentry/integrations/github/test_ticket_action.py @@ -14,7 +14,7 @@ from sentry.issues.action_log.types import CreateExternalIssueAction from sentry.models.activity import Activity from sentry.models.repository import Repository -from sentry.notifications.types import RuleFuture +from sentry.notifications.types import NotificationRule, RuleFuture from sentry.services.eventstore.models import GroupEvent from sentry.silo.base import SiloMode from sentry.testutils.cases import RuleTestCase @@ -71,7 +71,16 @@ def trigger(self, event, rule_object): results = list(action_inst.after(event=event)) assert len(results) == 1 - rule_future = RuleFuture(rule=rule_object, kwargs=results[0].kwargs) + notification_rule = NotificationRule( + id=rule_object.id, + label=rule_object.label, + data={"actions": [action]}, + project=rule_object.project, + environment_id=rule_object.environment_id, + workflow_id=123, + legacy_rule_id=rule_object.id, + ) + rule_future = RuleFuture(rule=notification_rule, kwargs=results[0].kwargs) return results[0].callback(event, futures=[rule_future]) def get_key(self, event: GroupEvent): diff --git a/tests/sentry/integrations/github_enterprise/test_ticket_action.py b/tests/sentry/integrations/github_enterprise/test_ticket_action.py index 7583f7add094..c3da8b4c1656 100644 --- a/tests/sentry/integrations/github_enterprise/test_ticket_action.py +++ b/tests/sentry/integrations/github_enterprise/test_ticket_action.py @@ -14,7 +14,7 @@ from sentry.integrations.models.external_issue import ExternalIssue from sentry.models.activity import Activity from sentry.models.repository import Repository -from sentry.notifications.types import RuleFuture +from sentry.notifications.types import NotificationRule, RuleFuture from sentry.rules import rules from sentry.services.eventstore.models import GroupEvent from sentry.silo.base import SiloMode @@ -82,7 +82,16 @@ def trigger(self, event, rule_object): results = list(action_inst.after(event=event)) assert len(results) == 1 - rule_future = RuleFuture(rule=rule_object, kwargs=results[0].kwargs) + notification_rule = NotificationRule( + id=rule_object.id, + label=rule_object.label, + data={"actions": [action]}, + project=rule_object.project, + environment_id=rule_object.environment_id, + workflow_id=123, + legacy_rule_id=rule_object.id, + ) + rule_future = RuleFuture(rule=notification_rule, kwargs=results[0].kwargs) return results[0].callback(event, futures=[rule_future]) def get_key(self, event: GroupEvent): diff --git a/tests/sentry/integrations/jira/test_ticket_action.py b/tests/sentry/integrations/jira/test_ticket_action.py index 7d8f8e931dd6..e74f28e4bbdf 100644 --- a/tests/sentry/integrations/jira/test_ticket_action.py +++ b/tests/sentry/integrations/jira/test_ticket_action.py @@ -10,7 +10,7 @@ from sentry.integrations.models.external_issue import ExternalIssue from sentry.integrations.types import EventLifecycleOutcome from sentry.models.activity import Activity -from sentry.notifications.types import RuleFuture +from sentry.notifications.types import NotificationRule, RuleFuture from sentry.services.eventstore.models import GroupEvent from sentry.shared_integrations.exceptions import ( ApiInvalidRequestError, @@ -54,7 +54,16 @@ def trigger(self, event, rule_object): results = list(action_inst.after(event=event)) assert len(results) == 1 - rule_future = RuleFuture(rule=rule_object, kwargs=results[0].kwargs) + notification_rule = NotificationRule( + id=rule_object.id, + label=rule_object.label, + data={"actions": [action]}, + project=rule_object.project, + environment_id=rule_object.environment_id, + workflow_id=123, + legacy_rule_id=rule_object.id, + ) + rule_future = RuleFuture(rule=notification_rule, kwargs=results[0].kwargs) return results[0].callback(event, futures=[rule_future]) def get_key(self, event: GroupEvent): diff --git a/tests/sentry/integrations/jira_server/test_ticket_action.py b/tests/sentry/integrations/jira_server/test_ticket_action.py index 3191929ddeb4..efada004ef99 100644 --- a/tests/sentry/integrations/jira_server/test_ticket_action.py +++ b/tests/sentry/integrations/jira_server/test_ticket_action.py @@ -7,7 +7,7 @@ from sentry.integrations.jira_server import JiraServerCreateTicketAction, JiraServerIntegration from sentry.integrations.models.external_issue import ExternalIssue from sentry.models.rule import Rule -from sentry.notifications.types import RuleFuture +from sentry.notifications.types import NotificationRule, RuleFuture from sentry.services.eventstore.models import GroupEvent from sentry.silo.base import SiloMode from sentry.testutils.cases import RuleTestCase @@ -54,7 +54,16 @@ def trigger(self, event: GroupEvent, rule_object: Rule) -> object: results = list(action_inst.after(event=event)) assert len(results) == 1 - rule_future = RuleFuture(rule=rule_object, kwargs=results[0].kwargs) + notification_rule = NotificationRule( + id=rule_object.id, + label=rule_object.label, + data={"actions": [action]}, + project=rule_object.project, + environment_id=rule_object.environment_id, + workflow_id=123, + legacy_rule_id=rule_object.id, + ) + rule_future = RuleFuture(rule=notification_rule, kwargs=results[0].kwargs) return results[0].callback(event, futures=[rule_future]) def get_key(self, event: GroupEvent) -> str: diff --git a/tests/sentry/integrations/msteams/test_message_builder.py b/tests/sentry/integrations/msteams/test_message_builder.py index 54aeb60fbca0..f881be4e4e9d 100644 --- a/tests/sentry/integrations/msteams/test_message_builder.py +++ b/tests/sentry/integrations/msteams/test_message_builder.py @@ -453,6 +453,8 @@ def test_action_payload_uses_only_legacy_rule_ids(self) -> None: data={"actions": [{"legacy_rule_id": legacy_rule.id}]}, project=self.project1, environment_id=None, + workflow_id=123, + legacy_rule_id=legacy_rule.id, ), NotificationRule( id=legacy_rule.id + 2000, @@ -460,6 +462,8 @@ def test_action_payload_uses_only_legacy_rule_ids(self) -> None: data={"actions": [{"workflow_id": 123}]}, project=self.project1, environment_id=None, + workflow_id=123, + legacy_rule_id=None, ), ] builder = MSTeamsIssueMessageBuilder( diff --git a/tests/sentry/integrations/vsts/test_notify_action.py b/tests/sentry/integrations/vsts/test_notify_action.py index b15c912788d9..ab29fd9b870b 100644 --- a/tests/sentry/integrations/vsts/test_notify_action.py +++ b/tests/sentry/integrations/vsts/test_notify_action.py @@ -8,7 +8,7 @@ from sentry.integrations.vsts import AzureDevopsCreateTicketAction from sentry.integrations.vsts.integration import VstsIntegration from sentry.models.grouplink import GroupLink -from sentry.notifications.types import RuleFuture +from sentry.notifications.types import NotificationRule, RuleFuture from sentry.silo.base import SiloMode from sentry.testutils.cases import RuleTestCase from sentry.testutils.helpers.datetime import freeze_time @@ -71,7 +71,20 @@ def test_create_issue(self) -> None: assert len(results) == 1 # Trigger rule callback - rule_future = RuleFuture(rule=azuredevops_rule, kwargs=results[0].kwargs) + persisted_rule = azuredevops_rule.rule + assert persisted_rule is not None + rule_future = RuleFuture( + rule=NotificationRule( + id=persisted_rule.id, + label=persisted_rule.label, + data={"actions": [azuredevops_rule.data]}, + project=persisted_rule.project, + environment_id=persisted_rule.environment_id, + workflow_id=123, + legacy_rule_id=persisted_rule.id, + ), + kwargs=results[0].kwargs, + ) results[0].callback(event, futures=[rule_future]) data = orjson.loads(responses.calls[0].response.text) diff --git a/tests/sentry/notifications/notification_action/test_issue_alert_registry_handlers.py b/tests/sentry/notifications/notification_action/test_issue_alert_registry_handlers.py index 12194015d743..3e87b8af0564 100644 --- a/tests/sentry/notifications/notification_action/test_issue_alert_registry_handlers.py +++ b/tests/sentry/notifications/notification_action/test_issue_alert_registry_handlers.py @@ -127,6 +127,11 @@ def test_create_rule_instance_from_action(self) -> None: assert self.workflow.environment is not None assert rule.environment_id == self.workflow.environment.id assert rule.label == self.workflow.name + assert rule.workflow_id == self.workflow.id + assert rule.legacy_rule_id == self.rule.id + assert rule.is_workflow_with_legacy_rule + assert not rule.is_workflow_only + assert not rule.is_test_notification assert rule.data == { "actions": [ { @@ -160,6 +165,9 @@ def test_create_rule_instance_from_action_with_workflow_only(self) -> None: assert self.workflow.environment is not None assert rule.environment_id == self.workflow.environment.id assert rule.label == self.workflow.name + assert rule.workflow_id == self.workflow.id + assert rule.legacy_rule_id is None + assert rule.is_workflow_only assert rule.data == { "actions": [ { @@ -184,6 +192,9 @@ def test_create_rule_instance_from_action_deleted_workflow_falls_back_to_detecto assert isinstance(rule, NotificationRule) assert rule.label == self.detector.name + assert rule.workflow_id == workflow_id + assert rule.legacy_rule_id is None + assert rule.is_workflow_only assert rule.data == { "actions": [ { @@ -215,6 +226,9 @@ def test_create_rule_instance_from_action_with_test_notification_id(self) -> Non assert isinstance(rule, NotificationRule) assert rule.label == self.detector.name + assert rule.workflow_id is None + assert rule.legacy_rule_id == TEST_NOTIFICATION_ID + assert rule.is_test_notification assert rule.data == { "actions": [ { @@ -227,6 +241,42 @@ def test_create_rule_instance_from_action_with_test_notification_id(self) -> Non ], } + def test_notification_rule_rejects_invalid_identity(self) -> None: + data = {"actions": [{"id": "test-action"}]} + + with pytest.raises(ValueError, match="requires at least one action"): + NotificationRule( + id=self.action.id, + label="Invalid", + data={"actions": []}, + project=self.project, + environment_id=None, + workflow_id=self.workflow.id, + legacy_rule_id=None, + ) + + with pytest.raises(ValueError, match="requires a workflow ID"): + NotificationRule( + id=self.action.id, + label="Invalid", + data=data, + project=self.project, + environment_id=None, + workflow_id=None, + legacy_rule_id=None, + ) + + with pytest.raises(ValueError, match="cannot have a workflow ID"): + NotificationRule( + id=self.action.id, + label="Invalid", + data=data, + project=self.project, + environment_id=None, + workflow_id=self.workflow.id, + legacy_rule_id=TEST_NOTIFICATION_ID, + ) + def test_create_rule_instance_from_action_no_environment(self) -> None: """Test that create_rule_instance_from_action creates a notification rule.""" self.create_workflow() diff --git a/tests/sentry/notifications/platform/discord/renderers/test_issue.py b/tests/sentry/notifications/platform/discord/renderers/test_issue.py index 3fea480a9d8d..850c8b9c53bc 100644 --- a/tests/sentry/notifications/platform/discord/renderers/test_issue.py +++ b/tests/sentry/notifications/platform/discord/renderers/test_issue.py @@ -49,6 +49,8 @@ def _create_data( "actions": [{"workflow_id": 1}], }, project_id=self.project.id, + workflow_id=1, + legacy_rule_id=None, ), ) @@ -119,7 +121,12 @@ def test_source(self) -> None: data = IssueNotificationData( group_id=self.group.id, rule=SerializableRuleProxy( - id=1, label="Test Detector", data={}, project_id=self.project.id + id=1, + label="Test Detector", + data={"actions": [{"workflow_id": 1}]}, + project_id=self.project.id, + workflow_id=1, + legacy_rule_id=None, ), ) assert data.source == NotificationSource.ISSUE @@ -130,7 +137,12 @@ def test_provider_returns_issue_renderer(self) -> None: data = IssueNotificationData( group_id=self.group.id, rule=SerializableRuleProxy( - id=1, label="Test Detector", data={}, project_id=self.project.id + id=1, + label="Test Detector", + data={"actions": [{"workflow_id": 1}]}, + project_id=self.project.id, + workflow_id=1, + legacy_rule_id=None, ), ) renderer = DiscordNotificationProvider.get_renderer(data=data) diff --git a/tests/sentry/notifications/platform/msteams/renderers/test_issue.py b/tests/sentry/notifications/platform/msteams/renderers/test_issue.py index a4163057bb99..cdbbfadcb84e 100644 --- a/tests/sentry/notifications/platform/msteams/renderers/test_issue.py +++ b/tests/sentry/notifications/platform/msteams/renderers/test_issue.py @@ -83,6 +83,8 @@ def _create_data( "actions": [{"workflow_id": 1}], }, project_id=self.project.id, + workflow_id=1, + legacy_rule_id=None, ), ) @@ -115,6 +117,8 @@ def _build_expected_card( label="Test Detector", data={"actions": [{"workflow_id": 1}]}, project_id=self.project.id, + workflow_id=1, + legacy_rule_id=None, ).to_notification_rule(project) ] footer_text = build_footer( @@ -420,7 +424,12 @@ def test_render_group_not_found(self) -> None: group_id=999999999, notification_uuid="test-uuid", rule=SerializableRuleProxy( - id=1, label="Test Detector", data={}, project_id=self.project.id + id=1, + label="Test Detector", + data={"actions": [{"workflow_id": 1}]}, + project_id=self.project.id, + workflow_id=1, + legacy_rule_id=None, ), ) rendered_template = NotificationRenderedTemplate(subject="Issue Alert", body=[]) @@ -435,7 +444,12 @@ def test_source(self) -> None: data = IssueNotificationData( group_id=self.group.id, rule=SerializableRuleProxy( - id=1, label="Test Detector", data={}, project_id=self.project.id + id=1, + label="Test Detector", + data={"actions": [{"workflow_id": 1}]}, + project_id=self.project.id, + workflow_id=1, + legacy_rule_id=None, ), ) assert data.source == NotificationSource.ISSUE @@ -446,7 +460,12 @@ def test_provider_returns_issue_renderer(self) -> None: data = IssueNotificationData( group_id=self.group.id, rule=SerializableRuleProxy( - id=1, label="Test Detector", data={}, project_id=self.project.id + id=1, + label="Test Detector", + data={"actions": [{"workflow_id": 1}]}, + project_id=self.project.id, + workflow_id=1, + legacy_rule_id=None, ), ) renderer = MSTeamsNotificationProvider.get_renderer(data=data) diff --git a/tests/sentry/notifications/platform/msteams/renderers/test_issue_parity.py b/tests/sentry/notifications/platform/msteams/renderers/test_issue_parity.py index 205c75d9f42c..42844f85d3e0 100644 --- a/tests/sentry/notifications/platform/msteams/renderers/test_issue_parity.py +++ b/tests/sentry/notifications/platform/msteams/renderers/test_issue_parity.py @@ -72,6 +72,8 @@ def platform_card(self) -> AdaptiveCard: data=self.rule.data, project=self.project, environment_id=self.rule.environment_id, + workflow_id=123, + legacy_rule_id=self.rule.id, ) ), ) diff --git a/tests/sentry/notifications/platform/slack/renderers/test_issue.py b/tests/sentry/notifications/platform/slack/renderers/test_issue.py index be01e9e4fa55..666c1bb5b397 100644 --- a/tests/sentry/notifications/platform/slack/renderers/test_issue.py +++ b/tests/sentry/notifications/platform/slack/renderers/test_issue.py @@ -79,7 +79,12 @@ def test_source(self) -> None: data = IssueNotificationData( group_id=self.group.id, rule=SerializableRuleProxy( - id=1, label="Test Detector", data={}, project_id=self.project.id + id=1, + label="Test Detector", + data={"actions": [{"workflow_id": 1}]}, + project_id=self.project.id, + workflow_id=1, + legacy_rule_id=None, ), ) assert data.source == NotificationSource.ISSUE @@ -99,6 +104,8 @@ def test_from_action_invocation(self) -> None: assert isinstance(result.rule, SerializableRuleProxy) assert result.rule.id == invocation.action.id assert result.rule.label == "Test Workflow" + assert result.rule.workflow_id == invocation.workflow_id + assert result.rule.legacy_rule_id is None assert result.tags == ["environment", "level"] assert result.notes == "test note" assert len(result.rule.data["actions"]) == 1 @@ -337,7 +344,12 @@ def test_provider_returns_issue_renderer(self) -> None: data = IssueNotificationData( group_id=self.group.id, rule=SerializableRuleProxy( - id=1, label="Test Detector", data={}, project_id=self.project.id + id=1, + label="Test Detector", + data={"actions": [{"workflow_id": 1}]}, + project_id=self.project.id, + workflow_id=1, + legacy_rule_id=None, ), ) renderer = SlackNotificationProvider.get_renderer(data=data) diff --git a/tests/sentry/sentry_apps/tasks/test_sentry_apps.py b/tests/sentry/sentry_apps/tasks/test_sentry_apps.py index 1518fd3a49d5..fc587b28db4c 100644 --- a/tests/sentry/sentry_apps/tasks/test_sentry_apps.py +++ b/tests/sentry/sentry_apps/tasks/test_sentry_apps.py @@ -22,7 +22,7 @@ from sentry.issues.grouptype import FeedbackGroup from sentry.issues.ingest import save_issue_occurrence from sentry.models.activity import Activity -from sentry.notifications.types import RuleFuture +from sentry.notifications.types import NotificationRule, RuleFuture from sentry.sentry_apps.metrics import SentryAppWebhookFailureReason, SentryAppWebhookHaltReason from sentry.sentry_apps.models.sentry_app import SentryApp from sentry.sentry_apps.models.sentry_app_installation import SentryAppInstallation @@ -123,6 +123,15 @@ class TestSendAlertEvent(TestCase, OccurrenceTestMixin): def setUp(self) -> None: self.sentry_app = self.create_sentry_app(organization=self.organization) self.rule = self.create_project_rule(name="Issa Rule") + self.notification_rule = NotificationRule( + id=self.rule.id, + label=self.rule.label, + data={"actions": self.rule.data["actions"]}, + project=self.rule.project, + environment_id=self.rule.environment_id, + workflow_id=123, + legacy_rule_id=self.rule.id, + ) self.install = self.create_sentry_app_installation( organization=self.organization, slug=self.sentry_app.slug ) @@ -195,7 +204,7 @@ def test_no_sentry_app_in_future(self, safe_urlopen: MagicMock) -> None: event = self.store_event(data={}, project_id=self.project.id) assert event.group is not None group_event = GroupEvent.from_event(event, event.group) - rule_future = RuleFuture(rule=self.rule, kwargs={}) + rule_future = RuleFuture(rule=self.notification_rule, kwargs={}) with self.tasks(): notify_sentry_app(group_event, [rule_future]) @@ -209,7 +218,7 @@ def test_no_installation(self, mock_record: MagicMock, safe_urlopen: MagicMock) event = self.store_event(data={}, project_id=self.project.id) assert event.group is not None group_event = GroupEvent.from_event(event, event.group) - rule_future = RuleFuture(rule=self.rule, kwargs={"sentry_app": sentry_app}) + rule_future = RuleFuture(rule=self.notification_rule, kwargs={"sentry_app": sentry_app}) with self.tasks(): notify_sentry_app(group_event, [rule_future]) @@ -236,7 +245,9 @@ def test_send_alert_event(self, mock_record: MagicMock, safe_urlopen: MagicMock) assert event.group is not None group = event.group group_event = GroupEvent.from_event(event, group) - rule_future = RuleFuture(rule=self.rule, kwargs={"sentry_app": self.sentry_app}) + rule_future = RuleFuture( + rule=self.notification_rule, kwargs={"sentry_app": self.sentry_app} + ) with self.tasks(): notify_sentry_app(group_event, [rule_future]) @@ -314,7 +325,7 @@ def test_send_alert_event_with_additional_payload( ] rule_future = RuleFuture( - rule=self.rule, + rule=self.notification_rule, kwargs={"sentry_app": self.sentry_app, "schema_defined_settings": settings}, ) @@ -375,7 +386,15 @@ def test_send_alert_event_with_additional_payload_legacy_rule_id( ] rule_future = RuleFuture( - rule=rule, + rule=NotificationRule( + id=rule.id, + label=rule.label, + data={"actions": rule.data["actions"]}, + project=rule.project, + environment_id=rule.environment_id, + workflow_id=123, + legacy_rule_id=rule.id, + ), kwargs={"sentry_app": self.sentry_app, "schema_defined_settings": settings}, ) @@ -426,7 +445,9 @@ def test_send_alert_event_with_groupevent( group_event = event.for_group(group_info.group) group_event.occurrence = occurrence - rule_future = RuleFuture(rule=self.rule, kwargs={"sentry_app": self.sentry_app}) + rule_future = RuleFuture( + rule=self.notification_rule, kwargs={"sentry_app": self.sentry_app} + ) with self.tasks(): notify_sentry_app(group_event, [rule_future]) @@ -515,7 +536,9 @@ def test_feedback_alert_webhook_includes_message_in_metadata_value( group_event = event.for_group(group_info.group) group_event.occurrence = occurrence - rule_future = RuleFuture(rule=self.rule, kwargs={"sentry_app": self.sentry_app}) + rule_future = RuleFuture( + rule=self.notification_rule, kwargs={"sentry_app": self.sentry_app} + ) with self.tasks(): notify_sentry_app(group_event, [rule_future]) @@ -554,7 +577,7 @@ def test_send_alert_event_with_3p_failure( ] rule_future = RuleFuture( - rule=self.rule, + rule=self.notification_rule, kwargs={"sentry_app": self.sentry_app, "schema_defined_settings": settings}, ) From 126c651fa6bdcf549123646fffdd386a78338afb Mon Sep 17 00:00:00 2001 From: Kyle Consalus Date: Wed, 30 Sep 2026 17:24:31 -0700 Subject: [PATCH 03/12] even more --- src/sentry/digests/notifications.py | 71 ++++++-------- src/sentry/digests/types.py | 8 +- src/sentry/digests/utils.py | 5 +- .../integrations/messaging/message_builder.py | 10 +- .../msteams/card_builder/issues.py | 20 +--- src/sentry/integrations/msteams/webhook.py | 12 ++- .../slack/message_builder/issues.py | 20 +--- .../slack/message_builder/util.py | 3 +- .../integrations/slack/webhooks/action.py | 25 ++--- .../notifications/notifications/rules.py | 8 +- .../platform/msteams/renderers/issue.py | 3 +- src/sentry/notifications/types.py | 46 ++++++++- src/sentry/notifications/utils/links.py | 35 +++---- .../notifications/utils/participants.py | 1 + src/sentry/notifications/utils/rules.py | 97 +++++++++++++------ .../msteams/test_message_builder.py | 10 +- .../slack/test_message_builder.py | 6 +- .../test_issue_alert_registry_handlers.py | 11 ++- .../msteams/renderers/test_issue_parity.py | 5 +- .../notifications/utils/test_participants.py | 1 + 20 files changed, 232 insertions(+), 165 deletions(-) diff --git a/src/sentry/digests/notifications.py b/src/sentry/digests/notifications.py index d88474553ecd..27c4aeddbe6d 100644 --- a/src/sentry/digests/notifications.py +++ b/src/sentry/digests/notifications.py @@ -3,17 +3,15 @@ import logging from collections import defaultdict from collections.abc import Mapping, Sequence +from dataclasses import replace from typing import Any, NamedTuple, TypeAlias -import sentry_sdk - from sentry import tsdb from sentry.digests.types import IdentifierKey, Notification, Record, RecordWithRuleObjects from sentry.models.group import Group, GroupStatus from sentry.models.project import Project from sentry.models.rule import Rule from sentry.notifications.types import ActionTargetType, FallthroughChoiceType, NotificationRule -from sentry.notifications.utils.rules import get_rule_or_workflow_id from sentry.services.eventstore.models import Event, GroupEvent from sentry.tsdb.base import TSDBModel from sentry.workflow_engine.models import Workflow @@ -21,7 +19,7 @@ logger = logging.getLogger("sentry.digests") -Digest: TypeAlias = dict[Rule, dict[Group, list[RecordWithRuleObjects]]] +Digest: TypeAlias = dict[NotificationRule, dict[Group, list[RecordWithRuleObjects]]] class DigestInfo(NamedTuple): @@ -90,13 +88,10 @@ def event_to_record( assert event.group is not None rule_ids = [] for rule in rules: - if isinstance(rule, NotificationRule): - rule_id = ( - rule.legacy_rule_id if identifier_key == IdentifierKey.RULE else rule.workflow_id - ) - assert rule_id is not None - else: - rule_id = int(get_rule_or_workflow_id(rule)[1]) + if isinstance(rule, Rule): + rule = NotificationRule.from_deprecated_legacy_rule(rule) + rule_id = rule.legacy_rule_id if identifier_key == IdentifierKey.RULE else rule.workflow_id + assert rule_id is not None rule_ids.append(rule_id) return Record( event.event_id, @@ -106,7 +101,7 @@ def event_to_record( def _bind_records( - records: Sequence[Record], groups: dict[int, Group], rules: dict[int, Rule] + records: Sequence[Record], groups: dict[int, Group], rules: dict[int, NotificationRule] ) -> list[RecordWithRuleObjects]: ret = [] for record in records: @@ -132,7 +127,9 @@ def _bind_records( def _group_records( - records: Sequence[RecordWithRuleObjects], groups: dict[int, Group], rules: dict[int, Rule] + records: Sequence[RecordWithRuleObjects], + groups: dict[int, Group], + rules: dict[int, NotificationRule], ) -> Digest: grouped: Digest = defaultdict(lambda: defaultdict(list)) for record in records: @@ -170,7 +167,7 @@ def _sort_digest( def _build_digest_impl( records: Sequence[Record], groups: dict[int, Group], - rules: dict[int, Rule], + rules: dict[int, NotificationRule], event_counts: dict[int, int], user_counts: Mapping[Any, int], ) -> Digest: @@ -180,8 +177,10 @@ def _build_digest_impl( return _sort_digest(grouped, event_counts=event_counts, user_counts=user_counts) -def get_rules_from_workflows(project: Project, workflow_ids: set[int]) -> dict[int, Rule]: - rules: dict[int, Rule] = {} +def get_rules_from_workflows( + project: Project, workflow_ids: set[int] +) -> dict[int, NotificationRule]: + rules: dict[int, NotificationRule] = {} if not workflow_ids: return rules @@ -203,27 +202,22 @@ def get_rules_from_workflows(project: Project, workflow_ids: set[int]) -> dict[i if alert_workflow: if rule := bulk_rules.get(alert_workflow.rule_id): assert rule.project_id == project.id, "Rule must belong to Project" - rule.environment_id = workflow.environment_id - try: - rule.data["actions"][0]["legacy_rule_id"] = rule.id - rule.data["actions"][0]["workflow_id"] = workflow_id - except KeyError: - # This shouldn't happen, but isn't a deal breaker if it does - sentry_sdk.capture_exception( - Exception(f"Rule {rule.id} does not have a legacy_rule_id"), - level="warning", - ) - rules[workflow_id] = rule + rules[workflow_id] = replace( + NotificationRule.from_deprecated_legacy_rule( + rule, workflow_id=workflow_id + ), + environment_id=workflow.environment_id, + ) continue - # Create synthetic Rule when no AlertRuleWorkflow or no Rule found - rules[workflow_id] = Rule( + rules[workflow_id] = NotificationRule( label=workflow.name, id=workflow_id, - project_id=project.id, + project=project, environment_id=workflow.environment_id, - # We need to do this so that the links are built correctly downstream data={"actions": [{"workflow_id": workflow_id}]}, + workflow_id=workflow_id, + legacy_rule_id=None, ) return rules @@ -254,17 +248,10 @@ def build_digest(project: Project, records: Sequence[Record]) -> DigestInfo: groups = Group.objects.in_bulk(record.value.event.group_id for record in records) group_ids = list(groups) - rules = Rule.objects.in_bulk(rule_ids) - - for rule in rules.values(): - try: - rule.data["actions"][0]["legacy_rule_id"] = rule.id - except KeyError: - # This shouldn't happen, but isn't a deal breaker if it does - sentry_sdk.capture_exception( - Exception(f"Rule {rule.id} does not have a legacy_rule_id"), - level="warning", - ) + rules = { + rule_id: NotificationRule.from_deprecated_legacy_rule(rule) + for rule_id, rule in Rule.objects.in_bulk(rule_ids).items() + } rules.update(get_rules_from_workflows(project, workflow_ids)) diff --git a/src/sentry/digests/types.py b/src/sentry/digests/types.py index c1c1aa5c0d56..dc3ed112cd66 100644 --- a/src/sentry/digests/types.py +++ b/src/sentry/digests/types.py @@ -8,7 +8,7 @@ from sentry.utils.dates import to_datetime if TYPE_CHECKING: - from sentry.models.rule import Rule + from sentry.notifications.types import NotificationRule from sentry.services.eventstore.models import Event, GroupEvent @@ -23,7 +23,7 @@ class Notification(NamedTuple): notification_uuid: str | None = None identifier_key: IdentifierKey = IdentifierKey.RULE - def with_rules(self, rules: list[Rule]) -> NotificationWithRuleObjects: + def with_rules(self, rules: list[NotificationRule]) -> NotificationWithRuleObjects: return NotificationWithRuleObjects( event=self.event, rules=rules, @@ -41,7 +41,7 @@ class Record(NamedTuple): def datetime(self) -> datetime_mod.datetime: return to_datetime(self.timestamp) - def with_rules(self, rules: list[Rule]) -> RecordWithRuleObjects: + def with_rules(self, rules: list[NotificationRule]) -> RecordWithRuleObjects: return RecordWithRuleObjects( key=self.key, value=self.value.with_rules(rules), @@ -51,7 +51,7 @@ def with_rules(self, rules: list[Rule]) -> RecordWithRuleObjects: class NotificationWithRuleObjects(NamedTuple): event: Event | GroupEvent - rules: list[Rule] + rules: list[NotificationRule] notification_uuid: str | None diff --git a/src/sentry/digests/utils.py b/src/sentry/digests/utils.py index 811358ccfcd4..4d44265daddd 100644 --- a/src/sentry/digests/utils.py +++ b/src/sentry/digests/utils.py @@ -11,8 +11,7 @@ from sentry.models.group import Group from sentry.models.project import Project from sentry.models.projectownership import ProjectOwnership -from sentry.models.rule import Rule -from sentry.notifications.types import ActionTargetType, FallthroughChoiceType +from sentry.notifications.types import ActionTargetType, FallthroughChoiceType, NotificationRule from sentry.notifications.utils.participants import get_send_to from sentry.services.eventstore.models import Event, GroupEvent from sentry.types.actor import Actor @@ -166,7 +165,7 @@ def sort_func(record: Record) -> datetime: return sorted(records, key=sort_func, reverse=True) -def get_groups(digest: Digest) -> Sequence[tuple[Rule, Group, Event | GroupEvent]]: +def get_groups(digest: Digest) -> Sequence[tuple[NotificationRule, Group, Event | GroupEvent]]: """ Split a digest into groups and return it as a tuple of: the applicable rule, the group, and the group's first event. diff --git a/src/sentry/integrations/messaging/message_builder.py b/src/sentry/integrations/messaging/message_builder.py index c366dcbe228d..21cae0889c66 100644 --- a/src/sentry/integrations/messaging/message_builder.py +++ b/src/sentry/integrations/messaging/message_builder.py @@ -9,13 +9,12 @@ from sentry.models.environment import Environment from sentry.models.group import Group from sentry.models.project import Project -from sentry.models.rule import Rule from sentry.models.team import Team from sentry.notifications.notifications.base import BaseNotification from sentry.notifications.notifications.rules import AlertRuleNotification from sentry.notifications.types import NotificationRule from sentry.notifications.utils.links import create_link_to_workflow -from sentry.notifications.utils.rules import get_key_from_rule_data, get_rule_or_workflow_id +from sentry.notifications.utils.rules import get_rule_or_workflow_id from sentry.services.eventstore.models import Event, GroupEvent from sentry.users.services.user import RpcUser from sentry.utils.http import absolute_uri @@ -251,10 +250,11 @@ def build_attachment_replay_link( return None -def build_rule_url(rule: Any, group: Group, project: Project) -> str: +def build_rule_url(rule: NotificationRule, group: Group, project: Project) -> str: org_slug = group.organization.slug project_slug = project.slug - rule_id = get_key_from_rule_data(rule, "legacy_rule_id") + rule_id = rule.legacy_rule_id + assert rule_id is not None rule_url = f"/organizations/{org_slug}/issues/alerts/rules/{project_slug}/{rule_id}/details/" return absolute_uri(rule_url) @@ -264,7 +264,7 @@ def build_footer( group: Group, project: Project, url_format: str, - rules: Sequence[Rule | NotificationRule] | None = None, + rules: Sequence[NotificationRule] | None = None, ) -> str: footer = f"{group.qualified_short_id}" if rules: diff --git a/src/sentry/integrations/msteams/card_builder/issues.py b/src/sentry/integrations/msteams/card_builder/issues.py index e8bbabf85c77..c3ba26eed134 100644 --- a/src/sentry/integrations/msteams/card_builder/issues.py +++ b/src/sentry/integrations/msteams/card_builder/issues.py @@ -29,9 +29,7 @@ from sentry.integrations.types import IntegrationProviderSlug from sentry.models.group import Group, GroupStatus from sentry.models.project import Project -from sentry.models.rule import Rule from sentry.notifications.types import NotificationRule -from sentry.notifications.utils.rules import get_legacy_rule_id from sentry.services.eventstore.models import Event, GroupEvent from .base import MSTeamsMessageBuilder @@ -54,16 +52,8 @@ logger = logging.getLogger(__name__) -def get_workflow_ids(rules: Sequence[Rule | NotificationRule]) -> list[int]: - workflow_ids = [] - for rule in rules: - action = rule.data.get("actions", [{}])[0] - workflow_id = action.get("workflow_id") - - if workflow_id is not None: - workflow_ids.append(int(workflow_id)) - - return workflow_ids +def get_workflow_ids(rules: Sequence[NotificationRule]) -> list[int]: + return [rule.workflow_id for rule in rules if rule.workflow_id is not None] class MSTeamsIssueMessageBuilder(MSTeamsMessageBuilder): @@ -71,7 +61,7 @@ def __init__( self, group: Group, event: Event | GroupEvent | None, - rules: Sequence[Rule | NotificationRule], + rules: Sequence[NotificationRule], integration: RpcIntegration, workflow_ids: Sequence[int] = (), ): @@ -90,9 +80,7 @@ def generate_action_payload(self, action_type: ACTION_TYPE) -> Any: "groupId": self.group.id, "eventId": self.event.event_id if self.event else None, "rules": [ - rule_id - for rule in self.rules - if (rule_id := get_legacy_rule_id(rule)) is not None + rule.legacy_rule_id for rule in self.rules if rule.legacy_rule_id is not None ], "workflows": list(dict.fromkeys([*workflow_ids, *self.workflow_ids])), "integrationId": self.integration.id, diff --git a/src/sentry/integrations/msteams/webhook.py b/src/sentry/integrations/msteams/webhook.py index 777d0ea8ca3b..cc6fa04d47c4 100644 --- a/src/sentry/integrations/msteams/webhook.py +++ b/src/sentry/integrations/msteams/webhook.py @@ -50,7 +50,7 @@ from sentry.models.activity import ActivityIntegration from sentry.models.apikey import ApiKey from sentry.models.group import Group -from sentry.models.rule import Rule +from sentry.notifications.utils.rules import get_notification_rules from sentry.services import eventstore from sentry.silo.base import SiloMode from sentry.users.services.user.service import user_service @@ -653,13 +653,19 @@ def _handle_action_submitted(self, request: Request) -> Response: # get the rules from the payload rule_ids = payload.get("rules", []) workflow_ids = payload.get("workflows", []) - rules = tuple(Rule.objects.filter(id__in=rule_ids, project_id=group.project_id)) + rules = tuple( + get_notification_rules( + group.project, + legacy_rule_ids=rule_ids, + workflow_ids=workflow_ids, + ) + ) metrics.incr( "integrations.msteams.action.rule_lookup", tags={ "has_rule": bool(rule_ids), "has_workflow_ids": bool(workflow_ids), - "lookup_succeeded": bool(rule_ids) and len(rules) == len(set(rule_ids)), + "lookup_succeeded": bool(rules), }, sample_rate=1.0, ) diff --git a/src/sentry/integrations/slack/message_builder/issues.py b/src/sentry/integrations/slack/message_builder/issues.py index 9743f2f1bf63..046f90960ec5 100644 --- a/src/sentry/integrations/slack/message_builder/issues.py +++ b/src/sentry/integrations/slack/message_builder/issues.py @@ -48,7 +48,6 @@ from sentry.models.project import Project from sentry.models.projectownership import ProjectOwnership from sentry.models.release import Release -from sentry.models.rule import Rule from sentry.models.team import Team from sentry.notifications.notifications.base import ProjectNotification from sentry.notifications.platform.slack.renderers.seer import SeerSlackRenderer @@ -66,7 +65,6 @@ from sentry.types.actor import Actor from sentry.types.group import SUBSTATUS_TO_STR from sentry.users.services.user.model import RpcUser -from sentry.workflow_engine.models import Workflow STATUSES = {"resolved": "resolved", "ignored": "ignored", "unresolved": "re-opened"} MAX_BLOCK_TEXT_LENGTH = 256 @@ -74,7 +72,7 @@ MAX_SUGGESTED_ASSIGNEES = 3 -def get_group_users_count(group: Group, rules: list[Rule] | None = None) -> int: +def get_group_users_count(group: Group, rules: list[NotificationRule] | None = None) -> int: environment_ids: list[int] | None = None if rules: environment_ids = [rule.environment_id for rule in rules if rule.environment_id is not None] @@ -196,7 +194,7 @@ def get_tags( return fields -def get_context(group: Group, rules: list[Rule | NotificationRule] | None = None) -> str: +def get_context(group: Group, rules: list[NotificationRule] | None = None) -> str: context_text = "" context = group.issue_type.notification_config.context.copy() @@ -417,7 +415,7 @@ def __init__( tags: set[str] | None = None, identity: RpcIdentity | None = None, actions: Sequence[MessageAction | BlockKitMessageAction] | None = None, - rules: list[Rule | NotificationRule] | None = None, + rules: list[NotificationRule] | None = None, link_to_event: bool = False, issue_details: bool = False, notification: ProjectNotification | None = None, @@ -614,18 +612,10 @@ def build(self, notification_uuid: str | None = None) -> SlackBlock: # handler, so it keeps preferring the legacy rule id. _, value = get_rule_or_workflow_id(self.rules[0]) rule_id = int(value) - action = self.rules[0].data.get("actions", [{}])[0] - if action.get("workflow_id") is not None: - workflow_id = int(action["workflow_id"]) - + workflow_id = self.rules[0].workflow_id or workflow_id link_key, link_value = get_rule_or_workflow_id(self.rules[0], prefer="workflow_id") link_id = int(link_value) - match link_key: - case "workflow_id": - workflow = Workflow.objects.filter(id=link_id).first() - rule_environment_id = workflow.environment_id if workflow else None - case "legacy_rule_id": - rule_environment_id = self.rules[0].environment_id + rule_environment_id = self.rules[0].environment_id # build up actions text if self.actions and self.identity and not action_text: diff --git a/src/sentry/integrations/slack/message_builder/util.py b/src/sentry/integrations/slack/message_builder/util.py index 47146e501572..9ba5d49dbcd9 100644 --- a/src/sentry/integrations/slack/message_builder/util.py +++ b/src/sentry/integrations/slack/message_builder/util.py @@ -4,7 +4,6 @@ from sentry.integrations.slack.message_builder.types import SLACK_URL_FORMAT from sentry.models.group import Group from sentry.models.project import Project -from sentry.models.rule import Rule from sentry.notifications.types import NotificationRule from sentry.notifications.utils.links import create_link_to_workflow from sentry.notifications.utils.rules import get_rule_or_workflow_id @@ -14,7 +13,7 @@ def build_slack_footer( group: Group, project: Project, - rules: Sequence[Rule | NotificationRule] | None = None, + rules: Sequence[NotificationRule] | None = None, ) -> str: footer = f"{group.qualified_short_id}" diff --git a/src/sentry/integrations/slack/webhooks/action.py b/src/sentry/integrations/slack/webhooks/action.py index d3f8c1c8a8f7..a8e3533318ff 100644 --- a/src/sentry/integrations/slack/webhooks/action.py +++ b/src/sentry/integrations/slack/webhooks/action.py @@ -57,9 +57,10 @@ from sentry.models.activity import ActivityIntegration from sentry.models.group import Group from sentry.models.organizationmember import InviteStatus, OrganizationMember -from sentry.models.rule import Rule from sentry.notifications.services import notifications_service +from sentry.notifications.types import NotificationRule from sentry.notifications.utils.actions import BlockKitMessageAction, MessageAction +from sentry.notifications.utils.rules import get_notification_rules from sentry.seer.entrypoints.operator import SeerAutofixOperator from sentry.seer.entrypoints.slack.entrypoint import SlackAutofixEntrypoint from sentry.seer.entrypoints.slack.messaging import send_not_org_member_message @@ -136,18 +137,18 @@ def update_group( return resp -def get_rule(rule_id: int | None, organization_id: int) -> Rule | None: +def get_rule( + rule_id: int | None, workflow_id: int | None, group: Group +) -> NotificationRule | None: """Get the rule that fired""" - if not rule_id: + if not rule_id and not workflow_id: return None - try: - # Scope the callback-provided rule ID to the integration-validated organization - rule = Rule.objects.get(id=rule_id, project__organization_id=organization_id) - # We need to add the legacy_rule_id field to the rule data since the message builder will use it to build the link to the rule - rule.data["actions"][0]["legacy_rule_id"] = rule.id - except Rule.DoesNotExist: - return None - return rule + rules = get_notification_rules( + group.project, + legacy_rule_ids=[rule_id] if rule_id else [], + workflow_ids=[workflow_id] if workflow_id else [], + ) + return rules[0] if rules else None def get_group(slack_request: SlackActionRequest) -> Group | None: @@ -365,7 +366,7 @@ def _handle_group_actions( rule_id = slack_request.callback_data.get("rule") workflow_id = slack_request.callback_data.get("workflow") - rule = get_rule(rule_id, group.project.organization_id) + rule = get_rule(rule_id, workflow_id, group) metrics.incr( "integrations.slack.action.rule_lookup", tags={ diff --git a/src/sentry/notifications/notifications/rules.py b/src/sentry/notifications/notifications/rules.py index 5606c67321a1..cb55a8d8f7db 100644 --- a/src/sentry/notifications/notifications/rules.py +++ b/src/sentry/notifications/notifications/rules.py @@ -28,6 +28,7 @@ from sentry.notifications.types import ( ActionTargetType, FallthroughChoiceType, + NotificationRule, NotificationSettingEnum, ) from sentry.notifications.utils import ( @@ -97,7 +98,12 @@ def __init__( self.target_type = target_type self.target_identifier = target_identifier self.fallthrough_choice = fallthrough_choice - self.rules = notification.rules + self.rules = [ + rule + if isinstance(rule, NotificationRule) + else NotificationRule.from_deprecated_legacy_rule(rule) + for rule in notification.rules + ] if ( event.group.issue_category in GROUP_CATEGORIES_CUSTOM_EMAIL diff --git a/src/sentry/notifications/platform/msteams/renderers/issue.py b/src/sentry/notifications/platform/msteams/renderers/issue.py index bd3643e35a3b..f55e7aa72cbb 100644 --- a/src/sentry/notifications/platform/msteams/renderers/issue.py +++ b/src/sentry/notifications/platform/msteams/renderers/issue.py @@ -20,7 +20,6 @@ NotificationSource, ) from sentry.notifications.types import NotificationRule -from sentry.notifications.utils.rules import get_legacy_rule_id from sentry.services.eventstore.models import Event, GroupEvent from sentry.types.actor import Actor @@ -204,7 +203,7 @@ def build_action_payload( "groupId": data.group_id, "eventId": data.event_id, "rules": [ - rule_id for rule in rules if (rule_id := get_legacy_rule_id(rule)) is not None + rule.legacy_rule_id for rule in rules if rule.legacy_rule_id is not None ], "workflows": get_workflow_ids(rules), } diff --git a/src/sentry/notifications/types.py b/src/sentry/notifications/types.py index 3811e6eb3157..d646131ce918 100644 --- a/src/sentry/notifications/types.py +++ b/src/sentry/notifications/types.py @@ -9,6 +9,7 @@ if TYPE_CHECKING: from sentry.models.organization import Organization from sentry.models.project import Project + from sentry.models.rule import Rule class NotificationRuleData(TypedDict): @@ -32,6 +33,38 @@ class NotificationRule: workflow_id: int | None legacy_rule_id: int | None + @classmethod + def from_deprecated_legacy_rule( + cls, rule: Rule, *, workflow_id: int | None = None + ) -> NotificationRule: + actions = rule.data.get("actions") + if ( + not isinstance(actions, list) + or not actions + or not all(isinstance(action, dict) for action in actions) + ): + raise ValueError("Legacy Rule requires at least one notification action") + + first_action = actions[0] + embedded_workflow_id = first_action.get("workflow_id") + embedded_legacy_rule_id = first_action.get("legacy_rule_id") + effective_workflow_id = workflow_id or embedded_workflow_id + legacy_rule_id = ( + None + if effective_workflow_id is not None and embedded_legacy_rule_id is None + else rule.id + ) + + return cls( + id=rule.id, + label=rule.label, + data={"actions": [dict(action) for action in actions]}, + project=rule.project, + environment_id=rule.environment_id, + workflow_id=effective_workflow_id, + legacy_rule_id=legacy_rule_id, + ) + def __post_init__(self) -> None: if not self.data["actions"]: raise ValueError("NotificationRule requires at least one action") @@ -39,8 +72,10 @@ def __post_init__(self) -> None: if self.legacy_rule_id == TEST_NOTIFICATION_ID: if self.workflow_id is not None: raise ValueError("Test notification cannot have a workflow ID") - elif self.workflow_id is None or self.workflow_id == TEST_NOTIFICATION_ID: - raise ValueError("NotificationRule requires a workflow ID") + elif self.workflow_id == TEST_NOTIFICATION_ID: + raise ValueError("Workflow ID cannot be the test notification ID") + elif self.workflow_id is None and self.legacy_rule_id is None: + raise ValueError("NotificationRule requires a workflow or legacy rule ID") @property def is_test_notification(self) -> bool: @@ -54,6 +89,13 @@ def is_workflow_only(self) -> bool: def is_workflow_with_legacy_rule(self) -> bool: return self.workflow_id is not None and self.legacy_rule_id is not None + @property + def is_legacy_rule_only(self) -> bool: + return self.workflow_id is None and self.legacy_rule_id not in ( + None, + TEST_NOTIFICATION_ID, + ) + @property def project_id(self) -> int: return self.project.id diff --git a/src/sentry/notifications/utils/links.py b/src/sentry/notifications/utils/links.py index ce3785371ce1..c014759b922c 100644 --- a/src/sentry/notifications/utils/links.py +++ b/src/sentry/notifications/utils/links.py @@ -8,11 +8,8 @@ from sentry.models.group import Group from sentry.models.organization import Organization from sentry.models.project import Project -from sentry.models.rule import Rule -from sentry.notifications.utils.rules import ( - get_key_from_rule_data, - split_rules_by_rule_workflow_id, -) +from sentry.notifications.types import NotificationRule +from sentry.notifications.utils.rules import split_rules_by_rule_workflow_id from sentry.types.rules import NotificationRuleDetails """ @@ -106,7 +103,10 @@ def get_issue_replay_link(group: Group, sentry_query_params: str = "") -> str: def get_rules( - rules: Sequence[Rule], organization: Organization, project: Project, type_id: int | None = None + rules: Sequence[NotificationRule], + organization: Organization, + project: Project, + type_id: int | None = None, ) -> list[NotificationRuleDetails]: rules_and_workflows = split_rules_by_rule_workflow_id(rules) @@ -115,21 +115,13 @@ def get_rules( ) + get_rules_with_legacy_ids(rules_and_workflows.rules, organization, project) -def _fetch_rule_id(rule: Rule, type_id: int | None = None) -> int: - # Try to fetch the legacy rule id, if it fails, return the rule id - # This allows us to support both legacy and new rule ids - try: - return int(get_key_from_rule_data(rule, "legacy_rule_id")) - except AssertionError: - return rule.id - - def get_rules_with_legacy_ids( - rules: Sequence[Rule], organization: Organization, project: Project + rules: Sequence[NotificationRule], organization: Organization, project: Project ) -> list[NotificationRuleDetails]: rules_with_legacy_ids = [] for rule in rules: - rule_id = _fetch_rule_id(rule) + rule_id = rule.legacy_rule_id + assert rule_id is not None rules_with_legacy_ids.append( NotificationRuleDetails( rule_id, @@ -141,16 +133,17 @@ def get_rules_with_legacy_ids( def get_workflow_links( - rules: Sequence[Rule], organization: Organization, project: Project + rules: Sequence[NotificationRule], organization: Organization, project: Project ) -> list[NotificationRuleDetails]: workflow_links = [] for rule in rules: - workflow_id = get_key_from_rule_data(rule, "workflow_id") + workflow_id = rule.workflow_id + assert workflow_id is not None workflow_links.append( NotificationRuleDetails( - int(workflow_id), + workflow_id, rule.label, - create_link_to_workflow(organization.slug, workflow_id), + create_link_to_workflow(organization.slug, str(workflow_id)), ) ) return workflow_links diff --git a/src/sentry/notifications/utils/participants.py b/src/sentry/notifications/utils/participants.py index ce9b87cce4d8..1b7c354fb7be 100644 --- a/src/sentry/notifications/utils/participants.py +++ b/src/sentry/notifications/utils/participants.py @@ -371,6 +371,7 @@ def get_send_to( notification_uuid, ) + def get_fallthrough_recipients( project: Project, fallthrough_choice: FallthroughChoiceType | None ) -> Iterable[RpcUser]: diff --git a/src/sentry/notifications/utils/rules.py b/src/sentry/notifications/utils/rules.py index c10ca7b7a961..cd0752370dfd 100644 --- a/src/sentry/notifications/utils/rules.py +++ b/src/sentry/notifications/utils/rules.py @@ -1,47 +1,87 @@ -from collections.abc import Sequence +from collections.abc import Iterable, Sequence from dataclasses import dataclass from typing import Literal +from sentry.models.project import Project from sentry.models.rule import Rule from sentry.notifications.types import NotificationRule +from sentry.workflow_engine.models import AlertRuleWorkflow, Workflow RuleIdType = Literal["workflow_id", "legacy_rule_id"] -def get_legacy_rule_id(rule: Rule | NotificationRule) -> int | None: - if isinstance(rule, Rule): - return rule.id - return rule.legacy_rule_id +def get_notification_rules( + project: Project, + *, + legacy_rule_ids: Iterable[int] = (), + workflow_ids: Iterable[int] = (), +) -> list[NotificationRule]: + workflow_ids = list(dict.fromkeys(workflow_ids)) + legacy_rule_ids = list(dict.fromkeys(legacy_rule_ids)) + workflows = Workflow.objects.filter(organization_id=project.organization_id).in_bulk( + workflow_ids + ) + workflow_links = { + link.workflow_id: link.rule_id + for link in AlertRuleWorkflow.objects.filter( + workflow_id__in=workflows, rule_id__isnull=False + ) + } + rules = Rule.objects.filter(project_id=project.id).in_bulk( + {*legacy_rule_ids, *workflow_links.values()} + ) + notification_rules = [] + linked_rule_ids = set() + for workflow_id in workflow_ids: + workflow = workflows.get(workflow_id) + if workflow is None: + continue -def get_key_from_rule_data(rule: Rule | NotificationRule, key: str) -> str: - if isinstance(rule, NotificationRule): - if key == "legacy_rule_id": - value = rule.legacy_rule_id - elif key == "workflow_id": - value = rule.workflow_id + legacy_rule_id = workflow_links.get(workflow_id) + legacy_rule = rules.get(legacy_rule_id) if legacy_rule_id is not None else None + if legacy_rule is not None: + linked_rule_ids.add(legacy_rule.id) + notification_rules.append( + NotificationRule.from_deprecated_legacy_rule( + legacy_rule, workflow_id=workflow_id + ) + ) else: - raise KeyError(key) - assert value is not None - return str(value) + notification_rules.append( + NotificationRule( + id=workflow_id, + label=workflow.name, + data={"actions": [{"workflow_id": workflow_id}]}, + project=project, + environment_id=workflow.environment_id, + workflow_id=workflow_id, + legacy_rule_id=None, + ) + ) - value = rule.data.get("actions", [{}])[0].get(key) - assert value is not None - return value + notification_rules.extend( + NotificationRule.from_deprecated_legacy_rule(rule) + for rule_id in legacy_rule_ids + if rule_id not in linked_rule_ids and (rule := rules.get(rule_id)) is not None + ) + return notification_rules @dataclass -class RulesAndWorkflows[RuleT: Rule | NotificationRule]: - rules: list[RuleT] - workflow_rules: list[RuleT] +class RulesAndWorkflows: + rules: list[NotificationRule] + workflow_rules: list[NotificationRule] -def split_rules_by_rule_workflow_id[RuleT: Rule | NotificationRule]( - rules: Sequence[RuleT], -) -> RulesAndWorkflows[RuleT]: +def split_rules_by_rule_workflow_id( + rules: Sequence[Rule | NotificationRule], +) -> RulesAndWorkflows: parsed_rules = [] workflow_rules = [] for rule in rules: + if isinstance(rule, Rule): + rule = NotificationRule.from_deprecated_legacy_rule(rule) key, _ = get_rule_or_workflow_id(rule) match key: case "workflow_id": @@ -52,7 +92,7 @@ def split_rules_by_rule_workflow_id[RuleT: Rule | NotificationRule]( def get_rule_or_workflow_id( - rule: Rule | NotificationRule, *, prefer: RuleIdType = "legacy_rule_id" + rule: NotificationRule, *, prefer: RuleIdType = "legacy_rule_id" ) -> tuple[RuleIdType, str]: """ Returns which id the rule data carries, and its value. When both a legacy @@ -64,10 +104,7 @@ def get_rule_or_workflow_id( else ("legacy_rule_id", "workflow_id") ) for key in keys: - try: - return (key, get_key_from_rule_data(rule, key)) - except AssertionError: - pass - if isinstance(rule, Rule): - return ("legacy_rule_id", str(rule.id)) + value = rule.workflow_id if key == "workflow_id" else rule.legacy_rule_id + if value is not None: + return (key, str(value)) raise AssertionError("NotificationRule must have a workflow or legacy rule ID") diff --git a/tests/sentry/integrations/msteams/test_message_builder.py b/tests/sentry/integrations/msteams/test_message_builder.py index f881be4e4e9d..6284e9146cde 100644 --- a/tests/sentry/integrations/msteams/test_message_builder.py +++ b/tests/sentry/integrations/msteams/test_message_builder.py @@ -118,10 +118,13 @@ def setUp(self) -> None: assert self.event1.group is not None self.group1 = self.event1.group - self.rules = [ + persisted_rules = [ self.create_project_rule(name="rule1"), self.create_project_rule(name="rule2"), ] + self.rules = [ + NotificationRule.from_deprecated_legacy_rule(rule) for rule in persisted_rules + ] def test_simple(self) -> None: card = MSTeamsMessageBuilder().build( @@ -423,9 +426,8 @@ def test_issue_message_builder(self) -> None: assert card_json[0] == "{" and card_json[-1] == "}" def test_issue_action_payload_includes_rule_and_workflow_ids(self) -> None: - self.rules[0].data["actions"][0].update( - {"legacy_rule_id": self.rules[0].id, "workflow_id": 123} - ) + self.rules[0].legacy_rule_id = self.rules[0].id + self.rules[0].workflow_id = 123 payload = MSTeamsIssueMessageBuilder( group=self.group1, diff --git a/tests/sentry/integrations/slack/test_message_builder.py b/tests/sentry/integrations/slack/test_message_builder.py index 00c80ca60577..729bb48171b0 100644 --- a/tests/sentry/integrations/slack/test_message_builder.py +++ b/tests/sentry/integrations/slack/test_message_builder.py @@ -38,6 +38,7 @@ from sentry.models.rule import Rule as IssueAlertRule from sentry.models.team import Team from sentry.monitors.grouptype import MonitorIncidentType +from sentry.notifications.types import NotificationRule from sentry.notifications.utils.actions import MessageAction from sentry.services.eventstore.models import Event from sentry.silo.base import SiloMode @@ -388,7 +389,10 @@ def test_build_group_block_noa(self) -> None: more_tags = {"escape": "`room`", "foo": "bar", "release": release.version} notes = "hey @colleen fix it" - assert SlackIssuesMessageBuilder(group, rules=[rule]).build() == build_test_message_blocks( + notification_rule = NotificationRule.from_deprecated_legacy_rule(rule) + assert SlackIssuesMessageBuilder( + group, rules=[notification_rule] + ).build() == build_test_message_blocks( teams={self.team}, users={self.user}, group=group, diff --git a/tests/sentry/notifications/notification_action/test_issue_alert_registry_handlers.py b/tests/sentry/notifications/notification_action/test_issue_alert_registry_handlers.py index 3e87b8af0564..9cd6855148e0 100644 --- a/tests/sentry/notifications/notification_action/test_issue_alert_registry_handlers.py +++ b/tests/sentry/notifications/notification_action/test_issue_alert_registry_handlers.py @@ -255,7 +255,7 @@ def test_notification_rule_rejects_invalid_identity(self) -> None: legacy_rule_id=None, ) - with pytest.raises(ValueError, match="requires a workflow ID"): + with pytest.raises(ValueError, match="requires a workflow or legacy rule ID"): NotificationRule( id=self.action.id, label="Invalid", @@ -277,6 +277,15 @@ def test_notification_rule_rejects_invalid_identity(self) -> None: legacy_rule_id=TEST_NOTIFICATION_ID, ) + def test_from_deprecated_legacy_rule(self) -> None: + notification_rule = NotificationRule.from_deprecated_legacy_rule(self.rule) + + assert notification_rule.id == self.rule.id + assert notification_rule.legacy_rule_id == self.rule.id + assert notification_rule.workflow_id is None + assert notification_rule.is_legacy_rule_only + assert notification_rule.data == {"actions": self.rule.data["actions"]} + def test_create_rule_instance_from_action_no_environment(self) -> None: """Test that create_rule_instance_from_action creates a notification rule.""" self.create_workflow() diff --git a/tests/sentry/notifications/platform/msteams/renderers/test_issue_parity.py b/tests/sentry/notifications/platform/msteams/renderers/test_issue_parity.py index 42844f85d3e0..433eb95dd204 100644 --- a/tests/sentry/notifications/platform/msteams/renderers/test_issue_parity.py +++ b/tests/sentry/notifications/platform/msteams/renderers/test_issue_parity.py @@ -57,7 +57,10 @@ def legacy_card(self) -> AdaptiveCard: rpc_integration = integration_service.get_integration(integration_id=self.integration.id) assert rpc_integration is not None return MSTeamsIssueMessageBuilder( - self.issue_group, self.event, [self.rule], rpc_integration + self.issue_group, + self.event, + [NotificationRule.from_deprecated_legacy_rule(self.rule)], + rpc_integration, ).build_group_card() def platform_card(self) -> AdaptiveCard: diff --git a/tests/sentry/notifications/utils/test_participants.py b/tests/sentry/notifications/utils/test_participants.py index c15670e01505..e2031bab02d6 100644 --- a/tests/sentry/notifications/utils/test_participants.py +++ b/tests/sentry/notifications/utils/test_participants.py @@ -130,6 +130,7 @@ def test_no_project_access(self) -> None: ) assert self.get_send_to_member(self.project, user_3.id) == {} + class GetSendToTeamTest(_ParticipantsTest): def setUp(self) -> None: super().setUp() From 293139e39156e3b0aa331f3190dd9b80460b7da1 Mon Sep 17 00:00:00 2001 From: Kyle Consalus Date: Wed, 30 Sep 2026 17:36:16 -0700 Subject: [PATCH 04/12] fix tests --- src/sentry/notifications/types.py | 23 ++++++--- tests/sentry/digests/test_notifications.py | 50 ++++++++++-------- .../integrations/opsgenie/test_client.py | 9 ++-- .../pagerduty/test_notification.py | 22 ++++++-- .../test_slack_notify_service_action.py | 51 ++++++++++++++++--- .../slack/test_message_builder.py | 4 +- tests/sentry/mail/test_adapter.py | 9 ++-- .../test_issue_alert_registry_handlers.py | 12 +++-- 8 files changed, 126 insertions(+), 54 deletions(-) diff --git a/src/sentry/notifications/types.py b/src/sentry/notifications/types.py index d646131ce918..2e5e26ed1aef 100644 --- a/src/sentry/notifications/types.py +++ b/src/sentry/notifications/types.py @@ -48,12 +48,23 @@ def from_deprecated_legacy_rule( first_action = actions[0] embedded_workflow_id = first_action.get("workflow_id") embedded_legacy_rule_id = first_action.get("legacy_rule_id") - effective_workflow_id = workflow_id or embedded_workflow_id - legacy_rule_id = ( - None - if effective_workflow_id is not None and embedded_legacy_rule_id is None - else rule.id - ) + if embedded_legacy_rule_id == TEST_NOTIFICATION_ID: + effective_workflow_id = None + legacy_rule_id = TEST_NOTIFICATION_ID + elif workflow_id is not None: + effective_workflow_id = workflow_id + legacy_rule_id = ( + embedded_legacy_rule_id if embedded_legacy_rule_id is not None else rule.id + ) + elif embedded_legacy_rule_id is not None: + effective_workflow_id = embedded_workflow_id + legacy_rule_id = embedded_legacy_rule_id + elif embedded_workflow_id is not None: + effective_workflow_id = embedded_workflow_id + legacy_rule_id = None + else: + effective_workflow_id = None + legacy_rule_id = rule.id return cls( id=rule.id, diff --git a/tests/sentry/digests/test_notifications.py b/tests/sentry/digests/test_notifications.py index 92e5c505aa19..733219fbdcee 100644 --- a/tests/sentry/digests/test_notifications.py +++ b/tests/sentry/digests/test_notifications.py @@ -16,7 +16,7 @@ from sentry.models.group import Group from sentry.models.project import Project from sentry.models.rule import Rule -from sentry.notifications.types import ActionTargetType, FallthroughChoiceType +from sentry.notifications.types import ActionTargetType, FallthroughChoiceType, NotificationRule from sentry.testutils.cases import TestCase from sentry.testutils.skips import requires_snuba @@ -40,17 +40,21 @@ def rule(self) -> Rule: def record(self) -> Record: return event_to_record(self.event, (self.rule,), self.notification_uuid) + @cached_property + def notification_rule(self) -> NotificationRule: + return NotificationRule.from_deprecated_legacy_rule(self.rule) + @property def group_mapping(self) -> dict[int, Group]: return {self.event.group.id: self.event.group} @property - def rule_mapping(self) -> dict[int, Rule]: - return {self.rule.id: self.rule} + def rule_mapping(self) -> dict[int, NotificationRule]: + return {self.rule.id: self.notification_rule} def test_success(self) -> None: (record,) = _bind_records([self.record], self.group_mapping, self.rule_mapping) - assert record == self.record.with_rules([self.rule]) + assert record == self.record.with_rules([self.notification_rule]) def test_without_group(self) -> None: # If the record can't be associated with a group, it should be dropped @@ -70,8 +74,10 @@ def project(self) -> Project: return self.create_project(fire_project_created=True) @cached_property - def rule(self) -> Rule: - return self.create_project_rule(project=self.project) + def rule(self) -> NotificationRule: + return NotificationRule.from_deprecated_legacy_rule( + self.create_project_rule(project=self.project) + ) def test_success(self) -> None: events = [ @@ -98,21 +104,25 @@ def project(self) -> Project: return self.create_project(fire_project_created=True) def test_success(self) -> None: - first_rule = self.create_project_rule( - project=self.project, - name="Send a notification for new issues", - condition_data=[ - {"id": "sentry.rules.conditions.first_seen_event.FirstSeenEventCondition"} - ], - action_data=[{"id": "sentry.rules.actions.notify_event.NotifyEventAction"}], + first_rule = NotificationRule.from_deprecated_legacy_rule( + self.create_project_rule( + project=self.project, + name="Send a notification for new issues", + condition_data=[ + {"id": "sentry.rules.conditions.first_seen_event.FirstSeenEventCondition"} + ], + action_data=[{"id": "sentry.rules.actions.notify_event.NotifyEventAction"}], + ) ) - second_rule = self.create_project_rule( - project=self.project, - name="Send a notification for regressions", - condition_data=[ - {"id": "sentry.rules.conditions.regression_event.RegressionEventCondition"} - ], - action_data=[{"id": "sentry.rules.actions.notify_event.NotifyEventAction"}], + second_rule = NotificationRule.from_deprecated_legacy_rule( + self.create_project_rule( + project=self.project, + name="Send a notification for regressions", + condition_data=[ + {"id": "sentry.rules.conditions.regression_event.RegressionEventCondition"} + ], + action_data=[{"id": "sentry.rules.actions.notify_event.NotifyEventAction"}], + ) ) rules = [first_rule, second_rule] diff --git a/tests/sentry/integrations/opsgenie/test_client.py b/tests/sentry/integrations/opsgenie/test_client.py index 761bf858d714..edf22f04e880 100644 --- a/tests/sentry/integrations/opsgenie/test_client.py +++ b/tests/sentry/integrations/opsgenie/test_client.py @@ -6,6 +6,7 @@ from sentry.integrations.opsgenie.client import OpsgenieClient from sentry.integrations.types import EventLifecycleOutcome +from sentry.notifications.types import NotificationRule from sentry.shared_integrations.exceptions import ApiError, ApiUnauthorized from sentry.testutils.asserts import ( assert_count_of_metric, @@ -82,7 +83,7 @@ def test_send_notification(self, mock_record: MagicMock) -> None: with self.options({"system.url-prefix": "http://example.com"}): payload = client.build_issue_alert_payload( data=event, - rules=[rule], + rules=[NotificationRule.from_deprecated_legacy_rule(rule)], event=event, group=group, priority="P2", @@ -149,7 +150,7 @@ def test_send_notification_with_workflow_engine_trigger_actions( with self.options({"system.url-prefix": "http://example.com"}): payload = client.build_issue_alert_payload( data=event, - rules=[rule], + rules=[NotificationRule.from_deprecated_legacy_rule(rule)], event=event, group=group, priority="P2", @@ -215,7 +216,7 @@ def test_send_notification_with_workflow_engine_ui_links(self, mock_record: Magi with self.options({"system.url-prefix": "http://example.com"}): payload = client.build_issue_alert_payload( data=event, - rules=[rule], + rules=[NotificationRule.from_deprecated_legacy_rule(rule)], event=event, group=group, priority="P2", @@ -282,7 +283,7 @@ def test_send_notification_unauthorized_errors(self, mock_record: MagicMock) -> with self.options({"system.url-prefix": "http://example.com"}): payload = client.build_issue_alert_payload( data=event, - rules=[rule], + rules=[NotificationRule.from_deprecated_legacy_rule(rule)], event=event, group=group, priority="P2", diff --git a/tests/sentry/integrations/pagerduty/test_notification.py b/tests/sentry/integrations/pagerduty/test_notification.py index 0b4691199872..180b01435e0a 100644 --- a/tests/sentry/integrations/pagerduty/test_notification.py +++ b/tests/sentry/integrations/pagerduty/test_notification.py @@ -11,7 +11,7 @@ from sentry.integrations.pagerduty.client import PAGERDUTY_SUMMARY_MAX_LENGTH from sentry.integrations.pagerduty.utils import add_service from sentry.integrations.types import EventLifecycleOutcome -from sentry.notifications.types import RuleFuture +from sentry.notifications.types import NotificationRule, RuleFuture from sentry.silo.base import SiloMode from sentry.testutils.asserts import assert_halt_metric, assert_slo_metric from sentry.testutils.cases import PerformanceIssueTestCase, RuleTestCase @@ -101,7 +101,10 @@ def test_applies_correctly(self, mock_record: MagicMock) -> None: ) # Trigger rule callback - rule_future = RuleFuture(rule=self.project_rule, kwargs=results[0].kwargs) + rule_future = RuleFuture( + rule=NotificationRule.from_deprecated_legacy_rule(self.project_rule), + kwargs=results[0].kwargs, + ) results[0].callback(event, futures=[rule_future]) data = orjson.loads(responses.calls[0].request.body) @@ -152,7 +155,10 @@ def test_applies_correctly_performance_issue(self) -> None: ) # Trigger rule callback - rule_future = RuleFuture(rule=self.project_rule, kwargs=results[0].kwargs) + rule_future = RuleFuture( + rule=NotificationRule.from_deprecated_legacy_rule(self.project_rule), + kwargs=results[0].kwargs, + ) results[0].callback(event, futures=[rule_future]) data = orjson.loads(responses.calls[0].request.body) @@ -188,7 +194,10 @@ def test_applies_correctly_generic_issue(self) -> None: ) # Trigger rule callback - rule_future = RuleFuture(rule=self.project_rule, kwargs=results[0].kwargs) + rule_future = RuleFuture( + rule=NotificationRule.from_deprecated_legacy_rule(self.project_rule), + kwargs=results[0].kwargs, + ) results[0].callback(group_event, futures=[rule_future]) data = orjson.loads(responses.calls[0].request.body) @@ -228,7 +237,10 @@ def test_truncates_summary(self) -> None: ) # Trigger rule callback - rule_future = RuleFuture(rule=self.project_rule, kwargs=results[0].kwargs) + rule_future = RuleFuture( + rule=NotificationRule.from_deprecated_legacy_rule(self.project_rule), + kwargs=results[0].kwargs, + ) results[0].callback(event, futures=[rule_future]) data = orjson.loads(responses.calls[0].request.body) diff --git a/tests/sentry/integrations/slack/actions/notification/test_slack_notify_service_action.py b/tests/sentry/integrations/slack/actions/notification/test_slack_notify_service_action.py index 1f5f1be8a45f..8b179bd0109f 100644 --- a/tests/sentry/integrations/slack/actions/notification/test_slack_notify_service_action.py +++ b/tests/sentry/integrations/slack/actions/notification/test_slack_notify_service_action.py @@ -8,7 +8,7 @@ from sentry.integrations.slack import SlackNotifyServiceAction from sentry.integrations.types import EventLifecycleOutcome from sentry.notifications.models.notificationmessage import NotificationMessage -from sentry.notifications.types import RuleFuture +from sentry.notifications.types import NotificationRule, RuleFuture from sentry.shared_integrations.exceptions import IntegrationError from sentry.silo.base import SiloMode from sentry.testutils.asserts import assert_failure_metric @@ -78,7 +78,12 @@ def test_after_noa( results = list(rule_cls_instance.after(event=self.event)) assert len(results) == 1 - results[0].callback(self.event, futures=[RuleFuture(rule=rule, kwargs={})]) + results[0].callback( + self.event, + futures=[ + RuleFuture(rule=NotificationRule.from_deprecated_legacy_rule(rule), kwargs={}) + ], + ) blocks = mock_post.call_args.kwargs["blocks"] blocks = orjson.loads(blocks) @@ -123,7 +128,12 @@ def test_after_noa_slo_halt(self, mock_post: MagicMock, mock_record: MagicMock) results = list(rule_cls_instance.after(event=self.event)) assert len(results) == 1 - results[0].callback(self.event, futures=[RuleFuture(rule=rule, kwargs={})]) + results[0].callback( + self.event, + futures=[ + RuleFuture(rule=NotificationRule.from_deprecated_legacy_rule(rule), kwargs={}) + ], + ) blocks = mock_post.call_args.kwargs["blocks"] blocks = orjson.loads(blocks) @@ -174,7 +184,12 @@ def test_after_noa_error( results = list(rule_cls_instance.after(event=self.event)) assert len(results) == 1 - results[0].callback(self.event, futures=[RuleFuture(rule=rule, kwargs={})]) + results[0].callback( + self.event, + futures=[ + RuleFuture(rule=NotificationRule.from_deprecated_legacy_rule(rule), kwargs={}) + ], + ) assert NotificationMessage.objects.all().count() == 1 @@ -210,7 +225,12 @@ def test_after_noa_test_action( results = list(rule_cls_instance.after(event=self.event)) assert len(results) == 1 - results[0].callback(self.event, futures=[RuleFuture(rule=rule, kwargs={})]) + results[0].callback( + self.event, + futures=[ + RuleFuture(rule=NotificationRule.from_deprecated_legacy_rule(rule), kwargs={}) + ], + ) blocks = mock_post.call_args.kwargs["blocks"] blocks = orjson.loads(blocks) @@ -251,7 +271,12 @@ def test_after_noa_new_ui( results = list(rule_cls_instance.after(event=self.event)) assert len(results) == 1 - results[0].callback(self.event, futures=[RuleFuture(rule=rule, kwargs={})]) + results[0].callback( + self.event, + futures=[ + RuleFuture(rule=NotificationRule.from_deprecated_legacy_rule(rule), kwargs={}) + ], + ) blocks = mock_post.call_args.kwargs["blocks"] blocks = orjson.loads(blocks) @@ -292,7 +317,12 @@ def test_after_with_threads_noa( rule.id = self.action.id rule.environment_id = None - results[0].callback(self.event, futures=[RuleFuture(rule=rule, kwargs={})]) + results[0].callback( + self.event, + futures=[ + RuleFuture(rule=NotificationRule.from_deprecated_legacy_rule(rule), kwargs={}) + ], + ) blocks = mock_post.call_args.kwargs["blocks"] blocks = orjson.loads(blocks) @@ -349,7 +379,12 @@ def test_after_reply_in_thread_noa( rule.id = self.action.id rule.environment_id = None - results[0].callback(self.event, futures=[RuleFuture(rule=rule, kwargs={})]) + results[0].callback( + self.event, + futures=[ + RuleFuture(rule=NotificationRule.from_deprecated_legacy_rule(rule), kwargs={}) + ], + ) blocks = mock_post.call_args.kwargs["blocks"] blocks = orjson.loads(blocks) diff --git a/tests/sentry/integrations/slack/test_message_builder.py b/tests/sentry/integrations/slack/test_message_builder.py index 729bb48171b0..cdc4486ca9aa 100644 --- a/tests/sentry/integrations/slack/test_message_builder.py +++ b/tests/sentry/integrations/slack/test_message_builder.py @@ -1321,7 +1321,7 @@ def test_get_context_users_affected(self) -> None: assert group group.update(type=1, substatus=GroupSubStatus.ONGOING, times_seen=3) - context = get_context(group, [rule]) + context = get_context(group, [NotificationRule.from_deprecated_legacy_rule(rule)]) assert ( context == f"Events: *3* Users Affected: *7* State: *Ongoing* First Seen: *{time_since(group.first_seen)}*" @@ -1329,7 +1329,7 @@ def test_get_context_users_affected(self) -> None: # filter users affected by env rule.update(environment_id=env.id) - context = get_context(group, [rule]) + context = get_context(group, [NotificationRule.from_deprecated_legacy_rule(rule)]) assert ( context == f"Events: *3* Users Affected: *5* State: *Ongoing* First Seen: *{time_since(group.first_seen)}*" diff --git a/tests/sentry/mail/test_adapter.py b/tests/sentry/mail/test_adapter.py index f0d9e4129eb0..07eaf76f46d6 100644 --- a/tests/sentry/mail/test_adapter.py +++ b/tests/sentry/mail/test_adapter.py @@ -44,6 +44,7 @@ from sentry.notifications.types import ( ActionTargetType, FallthroughChoiceType, + NotificationRule, RuleFuture, ) from sentry.notifications.utils.digest import get_digest_subject @@ -1567,7 +1568,7 @@ class MailAdapterRuleNotifyTest(BaseMailAdapterTest): def test_normal(self, mock_logger: MagicMock) -> None: event = self.store_event(data={}, project_id=self.project.id) rule = self.create_project_rule(name="my rule") - futures = [RuleFuture(rule, {})] + futures = [RuleFuture(NotificationRule.from_deprecated_legacy_rule(rule), {})] with mock.patch.object(self.adapter, "notify") as notify: self.adapter.rule_notify(event, futures, ActionTargetType.ISSUE_OWNERS) assert notify.call_count == 1 @@ -1596,7 +1597,7 @@ def test_digest(self, mock_logger: MagicMock, digests: MagicMock) -> None: event = self.store_event(data={}, project_id=self.project.id) rule = self.create_project_rule(project=self.project) - futures = [RuleFuture(rule, {})] + futures = [RuleFuture(NotificationRule.from_deprecated_legacy_rule(rule), {})] self.adapter.rule_notify(event, futures, ActionTargetType.ISSUE_OWNERS) assert digests.backend.add.call_count == 1 assert event.group @@ -1623,14 +1624,14 @@ def test_digest_with_perf_issue(self, digests: MagicMock) -> None: event = self.create_performance_issue() rule = self.create_project_rule(project=self.project) - futures = [RuleFuture(rule, {})] + futures = [RuleFuture(NotificationRule.from_deprecated_legacy_rule(rule), {})] self.adapter.rule_notify(event, futures, ActionTargetType.ISSUE_OWNERS) assert digests.backend.add.call_count == 1 def test_notify_includes_uuid(self) -> None: event = self.store_event(data={}, project_id=self.project.id) rule = self.create_project_rule(name="my rule") - futures = [RuleFuture(rule, {})] + futures = [RuleFuture(NotificationRule.from_deprecated_legacy_rule(rule), {})] notification_uuid = str(uuid.uuid4()) with mock.patch.object(self.adapter, "notify") as notify: self.adapter.rule_notify( diff --git a/tests/sentry/notifications/notification_action/test_issue_alert_registry_handlers.py b/tests/sentry/notifications/notification_action/test_issue_alert_registry_handlers.py index 9cd6855148e0..05e791f8ce40 100644 --- a/tests/sentry/notifications/notification_action/test_issue_alert_registry_handlers.py +++ b/tests/sentry/notifications/notification_action/test_issue_alert_registry_handlers.py @@ -28,6 +28,7 @@ ActionTargetType, FallthroughChoiceType, NotificationRule, + NotificationRuleData, ) from sentry.testutils.helpers.data_blobs import ( AZURE_DEVOPS_ACTION_DATA_BLOBS, @@ -242,7 +243,7 @@ def test_create_rule_instance_from_action_with_test_notification_id(self) -> Non } def test_notification_rule_rejects_invalid_identity(self) -> None: - data = {"actions": [{"id": "test-action"}]} + data: NotificationRuleData = {"actions": [{"id": "test-action"}]} with pytest.raises(ValueError, match="requires at least one action"): NotificationRule( @@ -278,13 +279,14 @@ def test_notification_rule_rejects_invalid_identity(self) -> None: ) def test_from_deprecated_legacy_rule(self) -> None: - notification_rule = NotificationRule.from_deprecated_legacy_rule(self.rule) + legacy_rule = self.create_project_rule(project=self.project, include_workflow_id=False) + notification_rule = NotificationRule.from_deprecated_legacy_rule(legacy_rule) - assert notification_rule.id == self.rule.id - assert notification_rule.legacy_rule_id == self.rule.id + assert notification_rule.id == legacy_rule.id + assert notification_rule.legacy_rule_id == legacy_rule.id assert notification_rule.workflow_id is None assert notification_rule.is_legacy_rule_only - assert notification_rule.data == {"actions": self.rule.data["actions"]} + assert notification_rule.data == {"actions": legacy_rule.data["actions"]} def test_create_rule_instance_from_action_no_environment(self) -> None: """Test that create_rule_instance_from_action creates a notification rule.""" From 1f0ee71758a4d2fd02387e4c28a9f4f348808e3a Mon Sep 17 00:00:00 2001 From: Kyle Consalus Date: Wed, 30 Sep 2026 17:50:48 -0700 Subject: [PATCH 05/12] dict key --- src/sentry/notifications/types.py | 17 ++++++++++++++++- tests/sentry/digests/test_notifications.py | 11 +++++++++-- .../msteams/test_message_builder.py | 11 +++++++---- .../test_issue_alert_registry_handlers.py | 17 ++++++++++++++++- 4 files changed, 48 insertions(+), 8 deletions(-) diff --git a/src/sentry/notifications/types.py b/src/sentry/notifications/types.py index 2e5e26ed1aef..f1839a28d6c9 100644 --- a/src/sentry/notifications/types.py +++ b/src/sentry/notifications/types.py @@ -16,7 +16,7 @@ class NotificationRuleData(TypedDict): actions: list[dict[str, Any]] -@dataclass(eq=False) +@dataclass(eq=False, frozen=True) class NotificationRule: """Rule-like notification context for the legacy action registry. @@ -88,6 +88,21 @@ def __post_init__(self) -> None: elif self.workflow_id is None and self.legacy_rule_id is None: raise ValueError("NotificationRule requires a workflow or legacy rule ID") + @property + def identifier(self) -> str: + if self.workflow_id is not None: + return f"workflow:{self.workflow_id}" + assert self.legacy_rule_id is not None + return f"legacy:{self.legacy_rule_id}" + + def __eq__(self, other: object) -> bool: + if not isinstance(other, NotificationRule): + return NotImplemented + return self.identifier == other.identifier + + def __hash__(self) -> int: + return hash(self.identifier) + @property def is_test_notification(self) -> bool: return self.legacy_rule_id == TEST_NOTIFICATION_ID diff --git a/tests/sentry/digests/test_notifications.py b/tests/sentry/digests/test_notifications.py index 733219fbdcee..a5d90f26b4ca 100644 --- a/tests/sentry/digests/test_notifications.py +++ b/tests/sentry/digests/test_notifications.py @@ -1,6 +1,7 @@ from __future__ import annotations import uuid +from dataclasses import replace from functools import cached_property from sentry.digests.notifications import ( @@ -86,13 +87,19 @@ def test_success(self) -> None: ] group = events[0].group assert group is not None + equivalent_rule = replace(self.rule) + assert equivalent_rule is not self.rule records = [ RecordWithRuleObjects( event.event_id, - NotificationWithRuleObjects(event, [self.rule], self.notification_uuid), + NotificationWithRuleObjects( + event, + [equivalent_rule if index == 1 else self.rule], + self.notification_uuid, + ), event.datetime.timestamp(), ) - for event in events + for index, event in enumerate(events) ] ret = _group_records(records, {group.id: group}, {self.rule.id: self.rule}) assert ret == {self.rule: {group: records}} diff --git a/tests/sentry/integrations/msteams/test_message_builder.py b/tests/sentry/integrations/msteams/test_message_builder.py index 6284e9146cde..c01559024fe7 100644 --- a/tests/sentry/integrations/msteams/test_message_builder.py +++ b/tests/sentry/integrations/msteams/test_message_builder.py @@ -1,6 +1,7 @@ from __future__ import annotations import re +from dataclasses import replace from typing import TypeGuard import orjson @@ -426,13 +427,14 @@ def test_issue_message_builder(self) -> None: assert card_json[0] == "{" and card_json[-1] == "}" def test_issue_action_payload_includes_rule_and_workflow_ids(self) -> None: - self.rules[0].legacy_rule_id = self.rules[0].id - self.rules[0].workflow_id = 123 + rule = replace( + self.rules[0], legacy_rule_id=self.rules[0].id, workflow_id=123 + ) payload = MSTeamsIssueMessageBuilder( group=self.group1, event=self.event1, - rules=[self.rules[0]], + rules=[rule], integration=self.integration, ).generate_action_payload(ACTION_TYPE.RESOLVE)["payload"] @@ -446,7 +448,7 @@ def test_issue_without_description(self) -> None: assert 3 == len(issue_card["body"]) - def test_action_payload_uses_only_legacy_rule_ids(self) -> None: + def test_action_payload_uses_explicit_rule_and_workflow_ids(self) -> None: legacy_rule = self.rules[0] rules = [ NotificationRule( @@ -478,6 +480,7 @@ def test_action_payload_uses_only_legacy_rule_ids(self) -> None: payload = builder.generate_action_payload(ACTION_TYPE.RESOLVE) assert payload["payload"]["rules"] == [legacy_rule.id] + assert payload["payload"]["workflows"] == [123] def test_issue_with_only_one_rule(self) -> None: one_rule = self.rules[:1] diff --git a/tests/sentry/notifications/notification_action/test_issue_alert_registry_handlers.py b/tests/sentry/notifications/notification_action/test_issue_alert_registry_handlers.py index 05e791f8ce40..93d08f2beae8 100644 --- a/tests/sentry/notifications/notification_action/test_issue_alert_registry_handlers.py +++ b/tests/sentry/notifications/notification_action/test_issue_alert_registry_handlers.py @@ -150,7 +150,22 @@ def test_create_rule_instance_from_action(self) -> None: other_rule = self.handler.create_rule_instance_from_action( self.action, self.detector, self.event_data, workflow_id=self.workflow.id ) - assert len({rule: "first", other_rule: "second"}) == 2 + assert rule.identifier == f"workflow:{self.workflow.id}" + assert rule == other_rule + assert hash(rule) == hash(other_rule) + assert {rule: "first", other_rule: "second"} == {rule: "second"} + + same_numeric_legacy_rule = NotificationRule( + id=self.workflow.id, + label="Legacy rule", + data={"actions": [{"id": "test-action"}]}, + project=self.project, + environment_id=None, + workflow_id=None, + legacy_rule_id=self.workflow.id, + ) + assert same_numeric_legacy_rule.identifier == f"legacy:{self.workflow.id}" + assert same_numeric_legacy_rule != rule def test_create_rule_instance_from_action_with_workflow_only(self) -> None: """Test that create_rule_instance_from_action creates a notification rule.""" From 9440e625d867855b2c3b228ecd9a3f5612905915 Mon Sep 17 00:00:00 2001 From: Kyle Consalus Date: Thu, 1 Oct 2026 09:13:14 -0700 Subject: [PATCH 06/12] address issues --- src/sentry/digests/notifications.py | 6 ++--- .../notifications/notifications/rules.py | 2 +- .../notifications/platform/templates/issue.py | 24 +++++++++++++++---- src/sentry/notifications/types.py | 19 ++++++++++++--- src/sentry/notifications/utils/rules.py | 4 ++-- .../integrations/jira/test_notify_action.py | 9 +++++-- .../test_issue_alert_registry_handlers.py | 14 +++++++++++ .../platform/slack/renderers/test_issue.py | 15 ++++++++++++ 8 files changed, 77 insertions(+), 16 deletions(-) diff --git a/src/sentry/digests/notifications.py b/src/sentry/digests/notifications.py index 27c4aeddbe6d..43564c6a7193 100644 --- a/src/sentry/digests/notifications.py +++ b/src/sentry/digests/notifications.py @@ -89,7 +89,7 @@ def event_to_record( rule_ids = [] for rule in rules: if isinstance(rule, Rule): - rule = NotificationRule.from_deprecated_legacy_rule(rule) + rule = NotificationRule.from_deprecated_legacy_rule(rule, project=event.group.project) rule_id = rule.legacy_rule_id if identifier_key == IdentifierKey.RULE else rule.workflow_id assert rule_id is not None rule_ids.append(rule_id) @@ -204,7 +204,7 @@ def get_rules_from_workflows( assert rule.project_id == project.id, "Rule must belong to Project" rules[workflow_id] = replace( NotificationRule.from_deprecated_legacy_rule( - rule, workflow_id=workflow_id + rule, project=project, workflow_id=workflow_id ), environment_id=workflow.environment_id, ) @@ -249,7 +249,7 @@ def build_digest(project: Project, records: Sequence[Record]) -> DigestInfo: groups = Group.objects.in_bulk(record.value.event.group_id for record in records) group_ids = list(groups) rules = { - rule_id: NotificationRule.from_deprecated_legacy_rule(rule) + rule_id: NotificationRule.from_deprecated_legacy_rule(rule, project=project) for rule_id, rule in Rule.objects.in_bulk(rule_ids).items() } diff --git a/src/sentry/notifications/notifications/rules.py b/src/sentry/notifications/notifications/rules.py index cb55a8d8f7db..ff4aa3ae020a 100644 --- a/src/sentry/notifications/notifications/rules.py +++ b/src/sentry/notifications/notifications/rules.py @@ -101,7 +101,7 @@ def __init__( self.rules = [ rule if isinstance(rule, NotificationRule) - else NotificationRule.from_deprecated_legacy_rule(rule) + else NotificationRule.from_deprecated_legacy_rule(rule, project=project) for rule in notification.rules ] diff --git a/src/sentry/notifications/platform/templates/issue.py b/src/sentry/notifications/platform/templates/issue.py index 77d3dd304ace..d4896b8522c8 100644 --- a/src/sentry/notifications/platform/templates/issue.py +++ b/src/sentry/notifications/platform/templates/issue.py @@ -11,7 +11,7 @@ NotificationSource, NotificationTemplate, ) -from sentry.notifications.types import NotificationRule, NotificationRuleData +from sentry.notifications.types import TEST_NOTIFICATION_ID, NotificationRule, NotificationRuleData class SerializableRuleProxy(BaseModel): @@ -26,8 +26,8 @@ class SerializableRuleProxy(BaseModel): data: NotificationRuleData environment_id: int | None = None project_id: int - workflow_id: int | None - legacy_rule_id: int | None + workflow_id: int | None = None + legacy_rule_id: int | None = None @classmethod def from_rule(cls, rule: NotificationRule) -> SerializableRuleProxy: @@ -43,14 +43,28 @@ def from_rule(cls, rule: NotificationRule) -> SerializableRuleProxy: ) def to_notification_rule(self, project: Project) -> NotificationRule: + workflow_id = self.workflow_id + legacy_rule_id = self.legacy_rule_id + if workflow_id is None and legacy_rule_id is None: + # Compatibility for payloads serialized before identities became top-level fields. + actions = self.data["actions"] + action = actions[0] if actions else {} + workflow_id = action.get("workflow_id") + legacy_rule_id = action.get("legacy_rule_id") + if workflow_id == TEST_NOTIFICATION_ID or legacy_rule_id == TEST_NOTIFICATION_ID: + workflow_id = None + legacy_rule_id = TEST_NOTIFICATION_ID + elif workflow_id is None and legacy_rule_id is None: + legacy_rule_id = self.id + return NotificationRule( id=self.id, label=self.label, data=self.data, environment_id=self.environment_id, project=project, - workflow_id=self.workflow_id, - legacy_rule_id=self.legacy_rule_id, + workflow_id=workflow_id, + legacy_rule_id=legacy_rule_id, ) diff --git a/src/sentry/notifications/types.py b/src/sentry/notifications/types.py index f1839a28d6c9..79a9ae59f126 100644 --- a/src/sentry/notifications/types.py +++ b/src/sentry/notifications/types.py @@ -13,6 +13,8 @@ class NotificationRuleData(TypedDict): + """Configuration for legacy action instantiation, not notification identity.""" + actions: list[dict[str, Any]] @@ -23,6 +25,12 @@ class NotificationRule: ``id`` identifies the source of this delivery. It is usually a workflow-engine Action ID, but its domain is not stable across every notification path. It must not be used to look up a persisted Rule; use ``legacy_rule_id`` explicitly. + + ``workflow_id`` and ``legacy_rule_id`` are the canonical notification identities. + Notification code must not recover identity from ``data["actions"]``. Action data + exists only to configure the legacy action registry. Parsing identity from action + data is restricted to explicit compatibility boundaries for deprecated Rule rows + and payloads serialized before these top-level fields existed. """ id: int @@ -35,7 +43,11 @@ class NotificationRule: @classmethod def from_deprecated_legacy_rule( - cls, rule: Rule, *, workflow_id: int | None = None + cls, + rule: Rule, + *, + project: Project | None = None, + workflow_id: int | None = None, ) -> NotificationRule: actions = rule.data.get("actions") if ( @@ -43,7 +55,8 @@ def from_deprecated_legacy_rule( or not actions or not all(isinstance(action, dict) for action in actions) ): - raise ValueError("Legacy Rule requires at least one notification action") + # Deprecated rules can reach render-only paths without action data. + actions = [{}] first_action = actions[0] embedded_workflow_id = first_action.get("workflow_id") @@ -70,7 +83,7 @@ def from_deprecated_legacy_rule( id=rule.id, label=rule.label, data={"actions": [dict(action) for action in actions]}, - project=rule.project, + project=project or rule.project, environment_id=rule.environment_id, workflow_id=effective_workflow_id, legacy_rule_id=legacy_rule_id, diff --git a/src/sentry/notifications/utils/rules.py b/src/sentry/notifications/utils/rules.py index cd0752370dfd..de9ccc9a8acd 100644 --- a/src/sentry/notifications/utils/rules.py +++ b/src/sentry/notifications/utils/rules.py @@ -44,7 +44,7 @@ def get_notification_rules( linked_rule_ids.add(legacy_rule.id) notification_rules.append( NotificationRule.from_deprecated_legacy_rule( - legacy_rule, workflow_id=workflow_id + legacy_rule, project=project, workflow_id=workflow_id ) ) else: @@ -61,7 +61,7 @@ def get_notification_rules( ) notification_rules.extend( - NotificationRule.from_deprecated_legacy_rule(rule) + NotificationRule.from_deprecated_legacy_rule(rule, project=project) for rule_id in legacy_rule_ids if rule_id not in linked_rule_ids and (rule := rules.get(rule_id)) is not None ) diff --git a/tests/sentry/integrations/jira/test_notify_action.py b/tests/sentry/integrations/jira/test_notify_action.py index 91416c4125c3..6c476eee1ab7 100644 --- a/tests/sentry/integrations/jira/test_notify_action.py +++ b/tests/sentry/integrations/jira/test_notify_action.py @@ -4,7 +4,7 @@ from sentry.integrations.jira import JiraCreateTicketAction from sentry.integrations.models.external_issue import ExternalIssue from sentry.models.grouplink import GroupLink -from sentry.notifications.types import RuleFuture +from sentry.notifications.types import NotificationRule, RuleFuture from sentry.silo.base import SiloMode from sentry.testutils.cases import PerformanceIssueTestCase, RuleTestCase from sentry.testutils.helpers.notifications import TEST_ISSUE_OCCURRENCE @@ -84,7 +84,12 @@ def create_issue_base(self, event): assert len(results) == 1 # Trigger rule callback - rule_future = RuleFuture(rule=self.jira_rule, kwargs=results[0].kwargs) + persisted_rule = self.jira_rule.rule + assert persisted_rule is not None + rule_future = RuleFuture( + rule=NotificationRule.from_deprecated_legacy_rule(persisted_rule), + kwargs=results[0].kwargs, + ) results[0].callback(event, futures=[rule_future]) return json.loads(responses.calls[1].request.body) diff --git a/tests/sentry/notifications/notification_action/test_issue_alert_registry_handlers.py b/tests/sentry/notifications/notification_action/test_issue_alert_registry_handlers.py index 93d08f2beae8..a826932d01fa 100644 --- a/tests/sentry/notifications/notification_action/test_issue_alert_registry_handlers.py +++ b/tests/sentry/notifications/notification_action/test_issue_alert_registry_handlers.py @@ -3,6 +3,7 @@ import pytest +from sentry.models.rule import Rule from sentry.notifications.models.notificationaction import ActionTarget from sentry.notifications.notification_action.issue_alert_registry import ( AzureDevopsIssueAlertHandler, @@ -303,6 +304,19 @@ def test_from_deprecated_legacy_rule(self) -> None: assert notification_rule.is_legacy_rule_only assert notification_rule.data == {"actions": legacy_rule.data["actions"]} + def test_from_deprecated_legacy_rule_without_actions(self) -> None: + legacy_rule = self.create_project_rule(project=self.project) + legacy_rule.update(data={}) + legacy_rule = Rule.objects.get(id=legacy_rule.id) + + with self.assertNumQueries(0): + notification_rule = NotificationRule.from_deprecated_legacy_rule( + legacy_rule, project=self.project + ) + + assert notification_rule.is_legacy_rule_only + assert notification_rule.data == {"actions": [{}]} + def test_create_rule_instance_from_action_no_environment(self) -> None: """Test that create_rule_instance_from_action creates a notification rule.""" self.create_workflow() diff --git a/tests/sentry/notifications/platform/slack/renderers/test_issue.py b/tests/sentry/notifications/platform/slack/renderers/test_issue.py index 666c1bb5b397..ec30d14ed8b0 100644 --- a/tests/sentry/notifications/platform/slack/renderers/test_issue.py +++ b/tests/sentry/notifications/platform/slack/renderers/test_issue.py @@ -75,6 +75,21 @@ def _create_invocation( class IssueNotificationDataTest(IssueAlertInvocationMixin): + def test_deserializes_legacy_rule_proxy(self) -> None: + proxy = SerializableRuleProxy.parse_obj( + { + "id": 1, + "label": "Legacy payload", + "data": {"actions": [{"workflow_id": 2}]}, + "project_id": self.project.id, + } + ) + + rule = proxy.to_notification_rule(self.project) + + assert rule.workflow_id == 2 + assert rule.legacy_rule_id is None + def test_source(self) -> None: data = IssueNotificationData( group_id=self.group.id, From 890b2221871747105aa807070bb21e4cc7418bd7 Mon Sep 17 00:00:00 2001 From: Kyle Consalus Date: Thu, 1 Oct 2026 10:27:13 -0700 Subject: [PATCH 07/12] fix test --- .../frontend/debug/debug_feedback_issue.py | 6 ++++-- .../web/frontend/debug/debug_generic_issue.py | 6 ++++-- src/sentry/web/frontend/debug/mail.py | 16 +++++++++++++--- tests/sentry/notifications/test_helpers.py | 19 ++++++++++++++++--- 4 files changed, 37 insertions(+), 10 deletions(-) diff --git a/src/sentry/web/frontend/debug/debug_feedback_issue.py b/src/sentry/web/frontend/debug/debug_feedback_issue.py index a481156357d6..d0e259296501 100644 --- a/src/sentry/web/frontend/debug/debug_feedback_issue.py +++ b/src/sentry/web/frontend/debug/debug_feedback_issue.py @@ -6,6 +6,7 @@ from sentry.models.organization import Organization from sentry.models.project import Project from sentry.models.rule import Rule +from sentry.notifications.types import NotificationRule from sentry.notifications.utils import get_generic_data from sentry.notifications.utils.links import get_group_settings_link, get_rules from sentry.utils import json @@ -25,6 +26,7 @@ def get(self, request: HttpRequest) -> HttpResponse: group = event.group rule = Rule(id=1, label="An example rule") + notification_rule = NotificationRule.from_deprecated_legacy_rule(rule, project=project) generic_issue_data_html = get_generic_data(event) section_header = "Issue Data" if generic_issue_data_html else "" @@ -33,14 +35,14 @@ def get(self, request: HttpRequest) -> HttpResponse: text_template="sentry/emails/feedback.txt", context={ "rule": rule, - "rules": get_rules([rule], org, project, group.type), + "rules": get_rules([notification_rule], org, project, group.type), "group": group, "event": event, "timezone": settings.SENTRY_DEFAULT_TIME_ZONE, "link": get_group_settings_link( group, None, - get_rules([rule], org, project, group.type), + get_rules([notification_rule], org, project, group.type), ), "generic_issue_data": [(section_header, mark_safe(generic_issue_data_html), None)], "tags": event.tags, diff --git a/src/sentry/web/frontend/debug/debug_generic_issue.py b/src/sentry/web/frontend/debug/debug_generic_issue.py index f29747f956d8..daf0ffa14039 100644 --- a/src/sentry/web/frontend/debug/debug_generic_issue.py +++ b/src/sentry/web/frontend/debug/debug_generic_issue.py @@ -7,6 +7,7 @@ from sentry.models.organization import Organization from sentry.models.project import Project from sentry.models.rule import Rule +from sentry.notifications.types import NotificationRule from sentry.notifications.utils import get_generic_data from sentry.notifications.utils.links import get_group_settings_link, get_rules from sentry.utils import json @@ -26,6 +27,7 @@ def get(self, request: HttpRequest) -> HttpResponse: group = event.group rule = Rule(id=1, label="An example rule") + notification_rule = NotificationRule.from_deprecated_legacy_rule(rule, project=project) generic_issue_data_html = get_generic_data(event) section_header = "Issue Data" if generic_issue_data_html else "" @@ -35,7 +37,7 @@ def get(self, request: HttpRequest) -> HttpResponse: text_template="sentry/emails/generic.txt", context={ "rule": rule, - "rules": get_rules([rule], org, project, group.type), + "rules": get_rules([notification_rule], org, project, group.type), "group": group, "event": event, "timezone": zoneinfo.ZoneInfo("Europe/Vienna"), @@ -44,7 +46,7 @@ def get(self, request: HttpRequest) -> HttpResponse: "link": get_group_settings_link( group, None, - get_rules([rule], org, project, group.type), + get_rules([notification_rule], org, project, group.type), ), "generic_issue_data": [(section_header, mark_safe(generic_issue_data_html), None)], "tags": event.tags, diff --git a/src/sentry/web/frontend/debug/mail.py b/src/sentry/web/frontend/debug/mail.py index c4bc9c6ae7dc..518a434417df 100644 --- a/src/sentry/web/frontend/debug/mail.py +++ b/src/sentry/web/frontend/debug/mail.py @@ -44,7 +44,7 @@ from sentry.notifications.notifications.base import BaseNotification from sentry.notifications.notifications.digest import DigestNotification from sentry.notifications.notifications.rules import get_group_substatus_text -from sentry.notifications.types import GroupSubscriptionReason +from sentry.notifications.types import GroupSubscriptionReason, NotificationRule from sentry.notifications.utils import get_interface_list from sentry.notifications.utils.links import ( get_group_settings_link, @@ -282,7 +282,8 @@ def make_feedback_issue(project: Project) -> GroupEvent: def get_shared_context( rule: Rule, org: Organization, project: Project, group: Group, event: BaseEvent ) -> dict[str, Any]: - rules = get_rules([rule], org, project, group.type) + notification_rule = NotificationRule.from_deprecated_legacy_rule(rule, project=project) + rules = get_rules([notification_rule], org, project, group.type) snooze_alert = len(rules) > 0 snooze_alert_url = rules[0].status_url + urlencode({"mute": "1"}) if snooze_alert else "" return { @@ -517,7 +518,16 @@ def digest(request: HttpRequest) -> HttpResponse: org = Organization(id=1, slug="example", name="Example Organization") project = Project(id=1, slug="example", name="Example Project", organization=org) rules = { - i: Rule(id=i, project=project, label=f"Rule #{i}") for i in range(1, random.randint(2, 4)) + i: NotificationRule( + id=i, + label=f"Rule #{i}", + data={"actions": [{}]}, + project=project, + environment_id=None, + workflow_id=None, + legacy_rule_id=i, + ) + for i in range(1, random.randint(2, 4)) } groups = {} event_counts = {} diff --git a/tests/sentry/notifications/test_helpers.py b/tests/sentry/notifications/test_helpers.py index 4974531e1b2d..0f2fe6fc5de4 100644 --- a/tests/sentry/notifications/test_helpers.py +++ b/tests/sentry/notifications/test_helpers.py @@ -7,7 +7,11 @@ validate, ) from sentry.notifications.models.notificationsettingoption import NotificationSettingOption -from sentry.notifications.types import NotificationSettingEnum, NotificationSettingsOptionEnum +from sentry.notifications.types import ( + NotificationRule, + NotificationSettingEnum, + NotificationSettingsOptionEnum, +) from sentry.notifications.utils.links import ( get_email_link_extra_params, get_group_settings_link, @@ -87,7 +91,10 @@ def test_collect_groups_by_project(self) -> None: def test_get_group_settings_link(self) -> None: rule: Rule = self.create_project_rule(self.project) - rule_details = get_rules([rule], self.organization, self.project, self.group.type) + notification_rule = NotificationRule.from_deprecated_legacy_rule(rule) + rule_details = get_rules( + [notification_rule], self.organization, self.project, self.group.type + ) link = get_group_settings_link( self.group, self.environment.name, rule_details, 1337, extra="123" ) @@ -109,7 +116,13 @@ def test_get_email_link_extra_params(self) -> None: project2 = self.create_project() rule2 = self.create_project_rule(project2) - rule_details = get_rules([rule, rule2], self.organization, self.project, self.group.type) + notification_rules = [ + NotificationRule.from_deprecated_legacy_rule(rule), + NotificationRule.from_deprecated_legacy_rule(rule2), + ] + rule_details = get_rules( + notification_rules, self.organization, self.project, self.group.type + ) extra_params = { k: dict(map(lambda x: (x[0], x[1][0]), parse_qs(v.strip("?")).items())) for k, v in get_email_link_extra_params( From 2fa4a68c872f770d760b77f18264f956c3946c37 Mon Sep 17 00:00:00 2001 From: Kyle Consalus Date: Thu, 1 Oct 2026 13:41:26 -0700 Subject: [PATCH 08/12] simplify --- src/sentry/digests/notifications.py | 4 +-- src/sentry/notifications/utils/rules.py | 4 +-- tests/sentry/digests/test_notifications.py | 2 +- tests/sentry/digests/test_utilities.py | 22 +++++++++---- .../slack/notifications/test_issue_alert.py | 17 ++++++++-- tests/sentry/mail/test_adapter.py | 31 ++++++++++++++++--- .../notifications/test_digests.py | 15 ++++++--- tests/sentry/tasks/test_digests.py | 6 ++-- 8 files changed, 74 insertions(+), 27 deletions(-) diff --git a/src/sentry/digests/notifications.py b/src/sentry/digests/notifications.py index 43564c6a7193..c77dc717f874 100644 --- a/src/sentry/digests/notifications.py +++ b/src/sentry/digests/notifications.py @@ -76,7 +76,7 @@ def unsplit_key( def event_to_record( event: Event | GroupEvent, - rules: Sequence[Rule | NotificationRule], + rules: Sequence[NotificationRule], notification_uuid: str | None = None, identifier_key: IdentifierKey = IdentifierKey.RULE, ) -> Record: @@ -88,8 +88,6 @@ def event_to_record( assert event.group is not None rule_ids = [] for rule in rules: - if isinstance(rule, Rule): - rule = NotificationRule.from_deprecated_legacy_rule(rule, project=event.group.project) rule_id = rule.legacy_rule_id if identifier_key == IdentifierKey.RULE else rule.workflow_id assert rule_id is not None rule_ids.append(rule_id) diff --git a/src/sentry/notifications/utils/rules.py b/src/sentry/notifications/utils/rules.py index de9ccc9a8acd..c30fd18b8816 100644 --- a/src/sentry/notifications/utils/rules.py +++ b/src/sentry/notifications/utils/rules.py @@ -75,13 +75,11 @@ class RulesAndWorkflows: def split_rules_by_rule_workflow_id( - rules: Sequence[Rule | NotificationRule], + rules: Sequence[NotificationRule], ) -> RulesAndWorkflows: parsed_rules = [] workflow_rules = [] for rule in rules: - if isinstance(rule, Rule): - rule = NotificationRule.from_deprecated_legacy_rule(rule) key, _ = get_rule_or_workflow_id(rule) match key: case "workflow_id": diff --git a/tests/sentry/digests/test_notifications.py b/tests/sentry/digests/test_notifications.py index a5d90f26b4ca..48e87e64b387 100644 --- a/tests/sentry/digests/test_notifications.py +++ b/tests/sentry/digests/test_notifications.py @@ -39,7 +39,7 @@ def rule(self) -> Rule: @cached_property def record(self) -> Record: - return event_to_record(self.event, (self.rule,), self.notification_uuid) + return event_to_record(self.event, (self.notification_rule,), self.notification_uuid) @cached_property def notification_rule(self) -> NotificationRule: diff --git a/tests/sentry/digests/test_utilities.py b/tests/sentry/digests/test_utilities.py index 8398948fb440..2c76e48f49e2 100644 --- a/tests/sentry/digests/test_utilities.py +++ b/tests/sentry/digests/test_utilities.py @@ -20,7 +20,7 @@ from sentry.models.project import Project from sentry.models.projectownership import ProjectOwnership from sentry.models.rule import Rule as RuleModel -from sentry.notifications.types import ActionTargetType, FallthroughChoiceType +from sentry.notifications.types import ActionTargetType, FallthroughChoiceType, NotificationRule from sentry.notifications.utils.rules import split_rules_by_rule_workflow_id from sentry.services.eventstore.models import Event from sentry.testutils.cases import SnubaTestCase, TestCase @@ -30,7 +30,10 @@ def _get_records(project: Project, rules: Collection[RuleModel], event: Event) -> list[Record]: - rules_and_workflows = split_rules_by_rule_workflow_id(list(rules)) + notification_rules = [ + NotificationRule.from_deprecated_legacy_rule(rule, project=project) for rule in rules + ] + rules_and_workflows = split_rules_by_rule_workflow_id(notification_rules) rules_by_identifier_key = { IdentifierKey.RULE: rules_and_workflows.rules, IdentifierKey.WORKFLOW: rules_and_workflows.workflow_rules, @@ -74,7 +77,8 @@ def test_get_event_from_groups_in_digest(self) -> None: ), ] - records = [event_to_record(event, (rule,)) for event in events] + notification_rule = NotificationRule.from_deprecated_legacy_rule(rule, project=project) + records = [event_to_record(event, (notification_rule,)) for event in events] digest = build_digest(project, sort_records(records))[0] @@ -246,6 +250,9 @@ def setUp(self) -> None: ) self.rule = self.create_project_rule() + self.notification_rule = NotificationRule.from_deprecated_legacy_rule( + self.rule, project=self.project + ) self.rule_with_workflow_id = self.create_project_rule(include_legacy_rule_id=False) self.rule_with_legacy_rule_id = self.create_project_rule(include_workflow_id=False) @@ -268,7 +275,7 @@ def create_events_from_filenames( def test_simple(self) -> None: records = [ - event_to_record(event, (self.rule,)) + event_to_record(event, (self.notification_rule,)) for event in self.team1_events + self.team2_events + self.user4_events ] digest = build_digest(self.project, sort_records(records))[0] @@ -317,7 +324,7 @@ def test_simple_with_legacy_rule_id(self) -> None: def test_direct_email(self) -> None: """When the action type is not Issue Owners, then the target actor gets a digest.""" self.project_ownership.update(fallthrough=False) - records = [event_to_record(event, (self.rule,)) for event in self.team1_events] + records = [event_to_record(event, (self.notification_rule,)) for event in self.team1_events] digest = build_digest(self.project, sort_records(records))[0] expected_result = {self.user1.id: set(self.team1_events)} @@ -443,7 +450,10 @@ def test_everyone_with_owners(self) -> None: events = self.create_events_from_filenames( self.project, ["hello.moz", "goodbye.moz", "hola.moz", "adios.moz"] ) - records = [event_to_record(event, (self.rule,)) for event in events + self.team1_events] + records = [ + event_to_record(event, (self.notification_rule,)) + for event in events + self.team1_events + ] digest = build_digest(self.project, sort_records(records))[0] expected_result = { self.user1.id: set(events), diff --git a/tests/sentry/integrations/slack/notifications/test_issue_alert.py b/tests/sentry/integrations/slack/notifications/test_issue_alert.py index e2371eb3e263..94190ae8d620 100644 --- a/tests/sentry/integrations/slack/notifications/test_issue_alert.py +++ b/tests/sentry/integrations/slack/notifications/test_issue_alert.py @@ -24,7 +24,12 @@ from sentry.notifications.models.notificationsettingoption import NotificationSettingOption from sentry.notifications.models.notificationsettingprovider import NotificationSettingProvider from sentry.notifications.notifications.rules import AlertRuleNotification -from sentry.notifications.types import ActionTargetType, FallthroughChoiceType, FineTuningAPIKey +from sentry.notifications.types import ( + ActionTargetType, + FallthroughChoiceType, + FineTuningAPIKey, + NotificationRule, +) from sentry.plugins.base import Notification from sentry.silo.base import SiloMode from sentry.tasks.digests import deliver_digest @@ -649,9 +654,12 @@ def test_issue_alert_team_issue_owners_user_settings_off_digests( name="ja rule", action_data=[action_data], ) + notification_rule = NotificationRule.from_deprecated_legacy_rule(rule) key = f"mail:p:{self.project.id}" - backend.add(key, event_to_record(event, [rule]), increment_delay=0, maximum_delay=0) + backend.add( + key, event_to_record(event, [notification_rule]), increment_delay=0, maximum_delay=0 + ) with self.tasks(): deliver_digest(key) @@ -951,13 +959,16 @@ def test_digest_enabled_block(self, digests: MagicMock) -> None: digests.enabled.return_value = True rule = self.create_project_rule(project=self.project) + notification_rule = NotificationRule.from_deprecated_legacy_rule(rule) ProjectOwnership.objects.create(project_id=self.project.id) event = self.store_event( data={"message": "Hello world", "level": "error"}, project_id=self.project.id ) key = f"mail:p:{self.project.id}:IssueOwners::AllMembers" - backend.add(key, event_to_record(event, [rule]), increment_delay=0, maximum_delay=0) + backend.add( + key, event_to_record(event, [notification_rule]), increment_delay=0, maximum_delay=0 + ) with self.tasks(): deliver_digest(key) diff --git a/tests/sentry/mail/test_adapter.py b/tests/sentry/mail/test_adapter.py index 07eaf76f46d6..7834ae179f7f 100644 --- a/tests/sentry/mail/test_adapter.py +++ b/tests/sentry/mail/test_adapter.py @@ -1399,9 +1399,14 @@ def test_notify_digest(self, notify: MagicMock) -> None: ) rule = self.create_project_rule(project=project) + notification_rule = NotificationRule.from_deprecated_legacy_rule(rule) ProjectOwnership.objects.create(project_id=self.project.id, fallthrough=True) digest = build_digest( - project, (event_to_record(event, (rule,)), event_to_record(event2, (rule,))) + project, + ( + event_to_record(event, (notification_rule,)), + event_to_record(event2, (notification_rule,)), + ), ) with self.tasks(): @@ -1455,9 +1460,14 @@ def test_notify_digest_replay_id(self, notify: MagicMock) -> None: ) rule = self.create_project_rule(project=project) + notification_rule = NotificationRule.from_deprecated_legacy_rule(rule) ProjectOwnership.objects.create(project_id=self.project.id, fallthrough=True) digest = build_digest( - project, (event_to_record(event, (rule,)), event_to_record(event2, (rule,))) + project, + ( + event_to_record(event, (notification_rule,)), + event_to_record(event2, (notification_rule,)), + ), ) features = ["organizations:session-replay"] @@ -1483,8 +1493,9 @@ def test_notify_digest_replay_id(self, notify: MagicMock) -> None: def test_notify_digest_single_record(self, send_async: MagicMock, notify: MagicMock) -> None: event = self.store_event(data={}, project_id=self.project.id) rule = self.create_project_rule(project=self.project) + notification_rule = NotificationRule.from_deprecated_legacy_rule(rule) ProjectOwnership.objects.create(project_id=self.project.id, fallthrough=True) - digest = build_digest(self.project, (event_to_record(event, (rule,)),)) + digest = build_digest(self.project, (event_to_record(event, (notification_rule,)),)) self.adapter.notify_digest( self.project, digest, @@ -1510,9 +1521,14 @@ def test_notify_digest_subject_prefix(self) -> None: ) rule = self.create_project_rule(project=self.project) + notification_rule = NotificationRule.from_deprecated_legacy_rule(rule) digest = build_digest( - self.project, (event_to_record(event, (rule,)), event_to_record(event2, (rule,))) + self.project, + ( + event_to_record(event, (notification_rule,)), + event_to_record(event2, (notification_rule,)), + ), ) with self.tasks(): @@ -1551,9 +1567,14 @@ def test_notify_digest_user_does_not_exist(self, notify: MagicMock) -> None: "targetIdentifier": str(444), } rule = self.create_project_rule(name="a rule", action_data=[action_data]) + notification_rule = NotificationRule.from_deprecated_legacy_rule(rule) digest = build_digest( - project, (event_to_record(event, (rule,)), event_to_record(event2, (rule,))) + project, + ( + event_to_record(event, (notification_rule,)), + event_to_record(event2, (notification_rule,)), + ), ) with self.tasks(): diff --git a/tests/sentry/notifications/notifications/test_digests.py b/tests/sentry/notifications/notifications/test_digests.py index e50538402a6a..303d2d56b46e 100644 --- a/tests/sentry/notifications/notifications/test_digests.py +++ b/tests/sentry/notifications/notifications/test_digests.py @@ -14,6 +14,7 @@ from sentry.digests.notifications import event_to_record from sentry.mail.analytics import EmailNotificationSent from sentry.models.projectownership import ProjectOwnership +from sentry.notifications.types import NotificationRule from sentry.services.eventstore.models import Event, GroupEvent from sentry.tasks.digests import deliver_digest from sentry.testutils.cases import PerformanceIssueTestCase, SlackActivityNotificationTest, TestCase @@ -61,7 +62,10 @@ def add_event(self, fingerprint: str, backend: Backend, event_type: str = "error assert event is not None backend.add( - self.key, event_to_record(event, [self.rule]), increment_delay=0, maximum_delay=0 + self.key, + event_to_record(event, [self.notification_rule]), + increment_delay=0, + maximum_delay=0, ) def run_test( @@ -91,6 +95,7 @@ def run_test( def setUp(self) -> None: super().setUp() self.rule = self.create_project_rule(project=self.project) + self.notification_rule = NotificationRule.from_deprecated_legacy_rule(self.rule) self.key = f"mail:p:{self.project.id}:IssueOwners::AllMembers" ProjectOwnership.objects.create(project_id=self.project.id, fallthrough=True) for i in range(USER_COUNT - 1): @@ -269,6 +274,7 @@ def test_slack_digest_notification_block(self, digests: MagicMock) -> None: timestamp = timestamp_raw.isoformat() key = f"slack:p:{self.project.id}:IssueOwners::AllMembers" rule = self.create_project_rule(project=self.project) + notification_rule = NotificationRule.from_deprecated_legacy_rule(rule) event1 = self.store_event( data={ "timestamp": timestamp, @@ -291,13 +297,13 @@ def test_slack_digest_notification_block(self, digests: MagicMock) -> None: notification_uuid = str(uuid.uuid4()) backend.add( key, - event_to_record(event1, [rule], notification_uuid), + event_to_record(event1, [notification_rule], notification_uuid), increment_delay=0, maximum_delay=0, ) backend.add( key, - event_to_record(event2, [rule], notification_uuid), + event_to_record(event2, [notification_rule], notification_uuid), increment_delay=0, maximum_delay=0, ) @@ -350,6 +356,7 @@ def test_slack_digest_notification_truncates_at_48_blocks(self, digests: MagicMo timestamp = before_now(days=1).isoformat() key = f"slack:p:{self.project.id}:IssueOwners::AllMembers" rule = self.create_project_rule(project=self.project) + notification_rule = NotificationRule.from_deprecated_legacy_rule(rule) notification_uuid = str(uuid.uuid4()) # Create 17 events to exceed 48 blocks (each event generates ~3 blocks) @@ -365,7 +372,7 @@ def test_slack_digest_notification_truncates_at_48_blocks(self, digests: MagicMo ) backend.add( key, - event_to_record(event, [rule], notification_uuid), + event_to_record(event, [notification_rule], notification_uuid), increment_delay=0, maximum_delay=0, ) diff --git a/tests/sentry/tasks/test_digests.py b/tests/sentry/tasks/test_digests.py index ad0f473e5905..c70f98c4c822 100644 --- a/tests/sentry/tasks/test_digests.py +++ b/tests/sentry/tasks/test_digests.py @@ -10,6 +10,7 @@ from sentry.digests.backends.redis import RedisBackend from sentry.digests.notifications import event_to_record from sentry.models.projectownership import ProjectOwnership +from sentry.notifications.types import NotificationRule from sentry.tasks.digests import deliver_digest from sentry.testutils.cases import TestCase from sentry.testutils.helpers.datetime import before_now @@ -27,6 +28,7 @@ def run_test(self, key: str) -> None: digests.backend.digest = backend.digest rule = self.create_project_rule(project=self.project) + notification_rule = NotificationRule.from_deprecated_legacy_rule(rule) ProjectOwnership.objects.create(project_id=self.project.id, fallthrough=True) event = self.store_event( data={"timestamp": before_now(days=1).isoformat(), "fingerprint": ["group-1"]}, @@ -39,13 +41,13 @@ def run_test(self, key: str) -> None: notification_uuid = str(uuid.uuid4()) backend.add( key, - event_to_record(event, [rule], notification_uuid), + event_to_record(event, [notification_rule], notification_uuid), increment_delay=0, maximum_delay=0, ) backend.add( key, - event_to_record(event_2, [rule], notification_uuid), + event_to_record(event_2, [notification_rule], notification_uuid), increment_delay=0, maximum_delay=0, ) From cc9d56020982be53fa953cf93c5e72432c24e86f Mon Sep 17 00:00:00 2001 From: Kyle Consalus Date: Thu, 1 Oct 2026 13:57:29 -0700 Subject: [PATCH 09/12] fix --- .../integrations/github/actions/create_ticket.py | 2 +- .../github_enterprise/actions/create_ticket.py | 2 +- .../integrations/jira/actions/create_ticket.py | 2 +- .../jira_server/actions/create_ticket.py | 2 +- .../integrations/vsts/actions/create_ticket.py | 2 +- src/sentry/mail/adapter.py | 1 - .../rules/actions/integrations/create_ticket/base.py | 12 ++++++++++-- src/sentry/types/rules.py | 1 + .../platform/msteams/renderers/test_issue_parity.py | 10 +--------- 9 files changed, 17 insertions(+), 17 deletions(-) diff --git a/src/sentry/integrations/github/actions/create_ticket.py b/src/sentry/integrations/github/actions/create_ticket.py index 585c1334a20c..578b5aa2ade1 100644 --- a/src/sentry/integrations/github/actions/create_ticket.py +++ b/src/sentry/integrations/github/actions/create_ticket.py @@ -14,4 +14,4 @@ class GitHubCreateTicketAction(TicketEventAction): provider = IntegrationProviderSlug.GITHUB.value def generate_footer(self, rule_url: str) -> str: - return f"\nThis issue was automatically created by Sentry via [{self.rule.label}]({absolute_uri(rule_url)})" + return f"\nThis issue was automatically created by Sentry via [{self.rule_context.label}]({absolute_uri(rule_url)})" diff --git a/src/sentry/integrations/github_enterprise/actions/create_ticket.py b/src/sentry/integrations/github_enterprise/actions/create_ticket.py index b3848dee32e9..ca7fe0a342a1 100644 --- a/src/sentry/integrations/github_enterprise/actions/create_ticket.py +++ b/src/sentry/integrations/github_enterprise/actions/create_ticket.py @@ -14,4 +14,4 @@ class GitHubEnterpriseCreateTicketAction(TicketEventAction): provider = IntegrationProviderSlug.GITHUB_ENTERPRISE.value def generate_footer(self, rule_url: str) -> str: - return f"\nThis issue was automatically created by Sentry via [{self.rule.label}]({absolute_uri(rule_url)})" + return f"\nThis issue was automatically created by Sentry via [{self.rule_context.label}]({absolute_uri(rule_url)})" diff --git a/src/sentry/integrations/jira/actions/create_ticket.py b/src/sentry/integrations/jira/actions/create_ticket.py index 68170054e498..c43dd1acf7f1 100644 --- a/src/sentry/integrations/jira/actions/create_ticket.py +++ b/src/sentry/integrations/jira/actions/create_ticket.py @@ -24,7 +24,7 @@ def __init__(self, *args: Any, **kwargs: Any) -> None: self.data["fixVersions"] = [fix_versions] def generate_footer(self, rule_url: str) -> str: - return f"This ticket was automatically created by Sentry via [{self.rule.label}|{absolute_uri(rule_url)}]" + return f"This ticket was automatically created by Sentry via [{self.rule_context.label}|{absolute_uri(rule_url)}]" def translate_integration(self, integration: RpcIntegration) -> str: name = integration.metadata.get("domain_name", integration.name) diff --git a/src/sentry/integrations/jira_server/actions/create_ticket.py b/src/sentry/integrations/jira_server/actions/create_ticket.py index 5a7b87448658..0b436a36ec52 100644 --- a/src/sentry/integrations/jira_server/actions/create_ticket.py +++ b/src/sentry/integrations/jira_server/actions/create_ticket.py @@ -24,7 +24,7 @@ def __init__(self, *args: Any, **kwargs: Any) -> None: self.data["fixVersions"] = [fix_versions] def generate_footer(self, rule_url: str) -> str: - return f"This ticket was automatically created by Sentry via [{self.rule.label}|{absolute_uri(rule_url)}]" + return f"This ticket was automatically created by Sentry via [{self.rule_context.label}|{absolute_uri(rule_url)}]" def translate_integration(self, integration: RpcIntegration) -> str: return integration.metadata.get("domain_name", integration.name) diff --git a/src/sentry/integrations/vsts/actions/create_ticket.py b/src/sentry/integrations/vsts/actions/create_ticket.py index 100fd6d3ce9d..4b5e7c96829c 100644 --- a/src/sentry/integrations/vsts/actions/create_ticket.py +++ b/src/sentry/integrations/vsts/actions/create_ticket.py @@ -15,4 +15,4 @@ class AzureDevopsCreateTicketAction(TicketEventAction): provider = IntegrationProviderSlug.AZURE_DEVOPS.value def generate_footer(self, rule_url: str) -> str: - return f"\nThis work item was automatically created by Sentry via [{self.rule.label}]({absolute_uri(rule_url)})" + return f"\nThis work item was automatically created by Sentry via [{self.rule_context.label}]({absolute_uri(rule_url)})" diff --git a/src/sentry/mail/adapter.py b/src/sentry/mail/adapter.py index 530c06d1df7b..dfeb6e731737 100644 --- a/src/sentry/mail/adapter.py +++ b/src/sentry/mail/adapter.py @@ -18,7 +18,6 @@ ActionTargetType, FallthroughChoiceType, NotificationSettingEnum, - RuleFuture, ) from sentry.notifications.types import ( RuleFuture as RuleFuture, diff --git a/src/sentry/rules/actions/integrations/create_ticket/base.py b/src/sentry/rules/actions/integrations/create_ticket/base.py index b9a7c79b8f08..6970021b9f01 100644 --- a/src/sentry/rules/actions/integrations/create_ticket/base.py +++ b/src/sentry/rules/actions/integrations/create_ticket/base.py @@ -2,7 +2,7 @@ import abc from collections.abc import Generator, Mapping -from typing import Any +from typing import TYPE_CHECKING, Any from sentry.integrations.services.integration import RpcIntegration from sentry.notifications.types import NotificationRule @@ -12,13 +12,15 @@ from sentry.rules.base import CallbackFuture from sentry.services.eventstore.models import GroupEvent +if TYPE_CHECKING: + from sentry.models.rule import Rule + class TicketEventAction(IntegrationEventAction, abc.ABC): """Shared ticket actions""" integration_key = "integration" link: str | None - rule: NotificationRule def __init__(self, *args: Any, **kwargs: Any) -> None: super(IntegrationEventAction, self).__init__(*args, **kwargs) @@ -47,6 +49,12 @@ def render_label(self) -> str: label: str = self.label.format(integration=self.get_integration_name()) return label + @property + def rule_context(self) -> Rule | NotificationRule: + if self.rule is None: + raise TypeError("Ticket delivery requires a rule context") + return self.rule + @property @abc.abstractmethod def ticket_type(self) -> str: diff --git a/src/sentry/types/rules.py b/src/sentry/types/rules.py index 3c3b993db77d..216c104276d2 100644 --- a/src/sentry/types/rules.py +++ b/src/sentry/types/rules.py @@ -2,6 +2,7 @@ from sentry.notifications.types import RuleFuture as RuleFuture + @dataclass class NotificationRuleDetails: """ diff --git a/tests/sentry/notifications/platform/msteams/renderers/test_issue_parity.py b/tests/sentry/notifications/platform/msteams/renderers/test_issue_parity.py index 433eb95dd204..1e1c2069e358 100644 --- a/tests/sentry/notifications/platform/msteams/renderers/test_issue_parity.py +++ b/tests/sentry/notifications/platform/msteams/renderers/test_issue_parity.py @@ -69,15 +69,7 @@ def platform_card(self) -> AdaptiveCard: event_id=self.event.event_id, notification_uuid="", rule=SerializableRuleProxy.from_rule( - NotificationRule( - id=self.rule.id, - label=self.rule.label, - data=self.rule.data, - project=self.project, - environment_id=self.rule.environment_id, - workflow_id=123, - legacy_rule_id=self.rule.id, - ) + NotificationRule.from_deprecated_legacy_rule(self.rule) ), ) return IssueMSTeamsRenderer.render( From 506423626c14bf0985a525ced38e4203cb8b0b62 Mon Sep 17 00:00:00 2001 From: Kyle Consalus Date: Thu, 1 Oct 2026 17:01:30 -0700 Subject: [PATCH 10/12] All NotificationRule, no NotificationRule.id --- src/sentry/digests/notifications.py | 2 +- .../slack/actions/notification.py | 8 +++- src/sentry/mail/adapter.py | 3 +- .../notification_action/types.py | 5 ++- .../notifications/notifications/digest.py | 3 +- .../notifications/notifications/rules.py | 5 ++- .../notifications/platform/templates/issue.py | 6 ++- src/sentry/notifications/types.py | 18 ++++++--- src/sentry/notifications/utils/rules.py | 2 +- src/sentry/rules/actions/integrations/base.py | 8 +++- .../integrations/create_ticket/base.py | 7 +--- .../integrations/create_ticket/utils.py | 2 +- src/sentry/rules/actions/notify_event.py | 6 ++- src/sentry/rules/base.py | 7 +--- .../templates/sentry/emails/_group.html | 8 ++-- .../templates/sentry/emails/digests/body.html | 6 ++- src/sentry/web/frontend/debug/mail.py | 2 +- tests/sentry/digests/test_notifications.py | 2 +- .../integrations/github/test_ticket_action.py | 10 ++--- .../github_enterprise/test_ticket_action.py | 10 ++--- .../integrations/jira/test_ticket_action.py | 10 ++--- .../jira_server/test_ticket_action.py | 10 ++--- .../msteams/test_message_builder.py | 20 +++++----- .../test_slack_notify_service_action.py | 37 ++++++++----------- .../integrations/vsts/test_notify_action.py | 2 +- tests/sentry/mail/test_adapter.py | 14 ++++--- .../test_issue_alert_registry_handlers.py | 17 +++++---- .../platform/slack/renderers/test_issue.py | 19 +++++++++- .../sentry_apps/tasks/test_sentry_apps.py | 4 +- 29 files changed, 147 insertions(+), 106 deletions(-) diff --git a/src/sentry/digests/notifications.py b/src/sentry/digests/notifications.py index c77dc717f874..0568b708ed83 100644 --- a/src/sentry/digests/notifications.py +++ b/src/sentry/digests/notifications.py @@ -210,7 +210,7 @@ def get_rules_from_workflows( rules[workflow_id] = NotificationRule( label=workflow.name, - id=workflow_id, + action_id=None, project=project, environment_id=workflow.environment_id, data={"actions": [{"workflow_id": workflow_id}]}, diff --git a/src/sentry/integrations/slack/actions/notification.py b/src/sentry/integrations/slack/actions/notification.py index ce2784682674..b51d09a307c6 100644 --- a/src/sentry/integrations/slack/actions/notification.py +++ b/src/sentry/integrations/slack/actions/notification.py @@ -240,7 +240,13 @@ def _send_notification_action_notification( rule = rules[0] if rules else None rule_to_use = self.rule if self.rule else rule # In the NOA, we will store the action id in the rule id field - action_id = rule_to_use.id if rule_to_use else None + action_id = ( + rule_to_use.action_id + if isinstance(rule_to_use, NotificationRule) + else rule_to_use.id + if rule_to_use + else None + ) if not action_id: # We are logging because this should never happen, all actions should have an uuid diff --git a/src/sentry/mail/adapter.py b/src/sentry/mail/adapter.py index dfeb6e731737..097bcaca5de3 100644 --- a/src/sentry/mail/adapter.py +++ b/src/sentry/mail/adapter.py @@ -65,7 +65,8 @@ def rule_notify( log_event = "dispatched" for future in futures: rules.append(future.rule) - extra["rule_id"] = future.rule.id + extra["workflow_id"] = future.rule.workflow_id + extra["legacy_rule_id"] = future.rule.legacy_rule_id if not future.kwargs: continue raise NotImplementedError( diff --git a/src/sentry/notifications/notification_action/types.py b/src/sentry/notifications/notification_action/types.py index a2a93231b2ba..6a0a8b8869da 100644 --- a/src/sentry/notifications/notification_action/types.py +++ b/src/sentry/notifications/notification_action/types.py @@ -272,7 +272,7 @@ def create_rule_instance_from_action( data["actions"][0]["skipDigests"] = True rule = NotificationRule( - id=action.id, + action_id=action.id, project=detector.linked_project, environment_id=environment_id, label=label, @@ -360,7 +360,8 @@ def invoke_legacy_registry(cls, invocation: ActionInvocation) -> None: "action_id": invocation.action.id, "detector_id": invocation.detector.id, "event_data": asdict(invocation.event_data), - "rule_id": rule.id, + "legacy_rule_id": rule.legacy_rule_id, + "workflow_id": rule.workflow_id, "rule_project_id": rule.project.id, "rule_environment_id": rule.environment_id, "rule_label": rule.label, diff --git a/src/sentry/notifications/notifications/digest.py b/src/sentry/notifications/notifications/digest.py index 1cb14dfc3e03..2ad38b0fc0fa 100644 --- a/src/sentry/notifications/notifications/digest.py +++ b/src/sentry/notifications/notifications/digest.py @@ -265,7 +265,8 @@ def send(self) -> None: def get_log_params(self, recipient: Actor) -> Mapping[str, Any]: try: - alert_id = list(self.digest.digest)[0].id + rule = list(self.digest.digest)[0] + alert_id = rule.workflow_id or rule.legacy_rule_id except Exception: alert_id = None diff --git a/src/sentry/notifications/notifications/rules.py b/src/sentry/notifications/notifications/rules.py index ff4aa3ae020a..f283eeeb5fba 100644 --- a/src/sentry/notifications/notifications/rules.py +++ b/src/sentry/notifications/notifications/rules.py @@ -358,10 +358,13 @@ def send(self) -> None: notify(provider, self, participants, shared_context) def get_log_params(self, recipient: Actor) -> Mapping[str, Any]: + alert_id = None + if self.rules: + alert_id = self.rules[0].workflow_id or self.rules[0].legacy_rule_id return { "target_type": self.target_type, "target_identifier": self.target_identifier, - "alert_id": self.rules[0].id if self.rules else None, + "alert_id": alert_id, **super().get_log_params(recipient), } diff --git a/src/sentry/notifications/platform/templates/issue.py b/src/sentry/notifications/platform/templates/issue.py index d4896b8522c8..124be7372f95 100644 --- a/src/sentry/notifications/platform/templates/issue.py +++ b/src/sentry/notifications/platform/templates/issue.py @@ -22,6 +22,7 @@ class SerializableRuleProxy(BaseModel): model_config = ConfigDict(frozen=True) id: int + action_id: int | None = None label: str data: NotificationRuleData environment_id: int | None = None @@ -33,7 +34,8 @@ class SerializableRuleProxy(BaseModel): def from_rule(cls, rule: NotificationRule) -> SerializableRuleProxy: """Create a serializable representation of a notification rule.""" return cls( - id=rule.id, + id=rule.broken_rule_id, + action_id=rule.action_id, label=rule.label, data=rule.data, environment_id=rule.environment_id, @@ -58,7 +60,7 @@ def to_notification_rule(self, project: Project) -> NotificationRule: legacy_rule_id = self.id return NotificationRule( - id=self.id, + action_id=self.action_id if "action_id" in self.__fields_set__ else self.id, label=self.label, data=self.data, environment_id=self.environment_id, diff --git a/src/sentry/notifications/types.py b/src/sentry/notifications/types.py index 79a9ae59f126..eeafd4f57fdd 100644 --- a/src/sentry/notifications/types.py +++ b/src/sentry/notifications/types.py @@ -22,10 +22,6 @@ class NotificationRuleData(TypedDict): class NotificationRule: """Rule-like notification context for the legacy action registry. - ``id`` identifies the source of this delivery. It is usually a workflow-engine - Action ID, but its domain is not stable across every notification path. It must - not be used to look up a persisted Rule; use ``legacy_rule_id`` explicitly. - ``workflow_id`` and ``legacy_rule_id`` are the canonical notification identities. Notification code must not recover identity from ``data["actions"]``. Action data exists only to configure the legacy action registry. Parsing identity from action @@ -33,7 +29,7 @@ class NotificationRule: and payloads serialized before these top-level fields existed. """ - id: int + action_id: int | None label: str data: NotificationRuleData project: Project @@ -80,7 +76,7 @@ def from_deprecated_legacy_rule( legacy_rule_id = rule.id return cls( - id=rule.id, + action_id=None, label=rule.label, data={"actions": [dict(action) for action in actions]}, project=project or rule.project, @@ -108,6 +104,16 @@ def identifier(self) -> str: assert self.legacy_rule_id is not None return f"legacy:{self.legacy_rule_id}" + @property + def broken_rule_id(self) -> int: + """Preserve callers that historically treated several ID domains as Rule.id.""" + if self.action_id is not None: + return self.action_id + if self.legacy_rule_id is not None: + return self.legacy_rule_id + assert self.workflow_id is not None + return self.workflow_id + def __eq__(self, other: object) -> bool: if not isinstance(other, NotificationRule): return NotImplemented diff --git a/src/sentry/notifications/utils/rules.py b/src/sentry/notifications/utils/rules.py index c30fd18b8816..a0fe95943b8e 100644 --- a/src/sentry/notifications/utils/rules.py +++ b/src/sentry/notifications/utils/rules.py @@ -50,7 +50,7 @@ def get_notification_rules( else: notification_rules.append( NotificationRule( - id=workflow_id, + action_id=None, label=workflow.name, data={"actions": [{"workflow_id": workflow_id}]}, project=project, diff --git a/src/sentry/rules/actions/integrations/base.py b/src/sentry/rules/actions/integrations/base.py index 31e392ec162a..ee0a0aadbd89 100644 --- a/src/sentry/rules/actions/integrations/base.py +++ b/src/sentry/rules/actions/integrations/base.py @@ -128,6 +128,10 @@ def record_notification_sent( "slack": SlackIntegrationNotificationSent, "email": EmailNotificationSent, } + alert_id = None + if rule is not None: + alert_id = rule.broken_rule_id + try: if event_class := PROVIDER_TO_EVENT_CLASS.get(self.provider): analytics.record( @@ -136,7 +140,7 @@ def record_notification_sent( project_id=event.project_id, group_id=event.group_id, notification_uuid=notification_uuid if notification_uuid else "", - alert_id=rule.id if rule else None, + alert_id=alert_id, category="issue_alert", ) ) @@ -147,7 +151,7 @@ def record_notification_sent( analytics.record( AlertSentEvent( provider=self.provider, - alert_id=rule.id if rule else "", + alert_id=alert_id if alert_id is not None else "", alert_type="issue_alert", organization_id=event.organization.id, project_id=event.project_id, diff --git a/src/sentry/rules/actions/integrations/create_ticket/base.py b/src/sentry/rules/actions/integrations/create_ticket/base.py index 6970021b9f01..a303de710311 100644 --- a/src/sentry/rules/actions/integrations/create_ticket/base.py +++ b/src/sentry/rules/actions/integrations/create_ticket/base.py @@ -2,7 +2,7 @@ import abc from collections.abc import Generator, Mapping -from typing import TYPE_CHECKING, Any +from typing import Any from sentry.integrations.services.integration import RpcIntegration from sentry.notifications.types import NotificationRule @@ -12,9 +12,6 @@ from sentry.rules.base import CallbackFuture from sentry.services.eventstore.models import GroupEvent -if TYPE_CHECKING: - from sentry.models.rule import Rule - class TicketEventAction(IntegrationEventAction, abc.ABC): """Shared ticket actions""" @@ -50,7 +47,7 @@ def render_label(self) -> str: return label @property - def rule_context(self) -> Rule | NotificationRule: + def rule_context(self) -> NotificationRule: if self.rule is None: raise TypeError("Ticket delivery requires a rule context") return self.rule diff --git a/src/sentry/rules/actions/integrations/create_ticket/utils.py b/src/sentry/rules/actions/integrations/create_ticket/utils.py index 0e9dd7ec39c0..b24055278d1f 100644 --- a/src/sentry/rules/actions/integrations/create_ticket/utils.py +++ b/src/sentry/rules/actions/integrations/create_ticket/utils.py @@ -138,7 +138,7 @@ def create_issue(event: GroupEvent, futures: Sequence[RuleFuture]) -> None: organization = event.group.project.organization for future in futures: - action_id = future.rule.id + action_id = future.rule.action_id data: dict[str, Any] = future.kwargs["data"] provider = future.kwargs.get("provider") integration_id = future.kwargs.get("integration_id") diff --git a/src/sentry/rules/actions/notify_event.py b/src/sentry/rules/actions/notify_event.py index 650a81ab67aa..dca5b998e109 100644 --- a/src/sentry/rules/actions/notify_event.py +++ b/src/sentry/rules/actions/notify_event.py @@ -23,10 +23,14 @@ class NotifyEventAction(EventAction): def after( self, event: GroupEvent, notification_uuid: str | None = None ) -> Generator[CallbackFuture]: + rule_id = None + if self.rule is not None: + rule_id = self.rule.broken_rule_id + logger.info( "notify_event.legacy_plugin_action_noop", extra={ - "rule_id": self.rule.id if self.rule else None, + "rule_id": rule_id, "event_id": event.event_id, "action": self.id, }, diff --git a/src/sentry/rules/base.py b/src/sentry/rules/base.py index 8a2bb333dde0..8137ae93eaa5 100644 --- a/src/sentry/rules/base.py +++ b/src/sentry/rules/base.py @@ -3,7 +3,7 @@ import abc import logging from collections.abc import Callable, MutableMapping, Sequence -from typing import TYPE_CHECKING, Any, ClassVar, NamedTuple +from typing import Any, ClassVar, NamedTuple from django import forms @@ -11,9 +11,6 @@ from sentry.notifications.types import NotificationRule, RuleFuture from sentry.services.eventstore.models import GroupEvent -if TYPE_CHECKING: - from sentry.models.rule import Rule - """ Rules apply either before an event gets stored, or immediately after. @@ -61,7 +58,7 @@ def __init__( self, project: Project, data: MutableMapping[str, Any] | None = None, - rule: Rule | NotificationRule | None = None, + rule: NotificationRule | None = None, ) -> None: self.project = project self.data = data or {} diff --git a/src/sentry/templates/sentry/emails/_group.html b/src/sentry/templates/sentry/emails/_group.html index c8cebdc850a1..4c3102141a20 100644 --- a/src/sentry/templates/sentry/emails/_group.html +++ b/src/sentry/templates/sentry/emails/_group.html @@ -6,7 +6,7 @@

{% if metadata.type %} {%if rule %} - {{metadata.type|truncatechars:40}} + {% if rule_id %}{{metadata.type|truncatechars:40}}{% else %}{{metadata.type|truncatechars:40}}{% endif %} {% else %} {{metadata.type|truncatechars:40}} {% endif %} @@ -19,7 +19,7 @@

{% endif %} {% else %} {%if rule %} - {{metadata.value|truncatechars:40}} + {% if rule_id %}{{metadata.value|truncatechars:40}}{% else %}{{metadata.value|truncatechars:40}}{% endif %} {% else %} {{metadata.value|truncatechars:40}} {% endif %} @@ -34,7 +34,7 @@

{%if rule %} - {{metadata.directive|truncatechars:40}} + {% if rule_id %}{{metadata.directive|truncatechars:40}}{% else %}{{metadata.directive|truncatechars:40}}{% endif %} {% else %} {{metadata.directive|truncatechars:40}} {% endif %} @@ -48,7 +48,7 @@

{%if rule %} - {{group.title|truncatechars:40}} + {% if rule_id %}{{group.title|truncatechars:40}}{% else %}{{group.title|truncatechars:40}}{% endif %} {% else %} {{group.title|truncatechars:40}} {% endif %} diff --git a/src/sentry/templates/sentry/emails/digests/body.html b/src/sentry/templates/sentry/emails/digests/body.html index 6cb5f84b11c3..25c7babff217 100644 --- a/src/sentry/templates/sentry/emails/digests/body.html +++ b/src/sentry/templates/sentry/emails/digests/body.html @@ -48,10 +48,11 @@

{{ counts|length }} new alert{{ counts|pluralize }} from
- {% with rule_details=rules_details|get_item:rule.id snooze_alert_url=snooze_alert_urls|get_item:rule.id %} + {% with rule_details=rules_details|get_item:rule_context_id snooze_alert_url=snooze_alert_urls|get_item:rule_context_id %} {% if snooze_alert %} Mute this alert {% endif %} @@ -72,7 +73,7 @@

{{ counts|length }} new alert{{ counts|pluralize }} from {{ group.get_level_display }} - {% include "sentry/emails/_group.html" %} + {% include "sentry/emails/_group.html" with rule_id=rule_context_id %}
{% if clock_24_hours %} @@ -106,6 +107,7 @@

{{ counts|length }} new alert{{ counts|pluralize }} from None: ) for index, event in enumerate(events) ] - ret = _group_records(records, {group.id: group}, {self.rule.id: self.rule}) + ret = _group_records(records, {group.id: group}, {self.rule.legacy_rule_id: self.rule}) assert ret == {self.rule: {group: records}} diff --git a/tests/sentry/integrations/github/test_ticket_action.py b/tests/sentry/integrations/github/test_ticket_action.py index 1c03c3b720cb..9c9edb2a1c07 100644 --- a/tests/sentry/integrations/github/test_ticket_action.py +++ b/tests/sentry/integrations/github/test_ticket_action.py @@ -67,12 +67,8 @@ def stub_get_jwt(self): def trigger(self, event, rule_object): action = rule_object.data.get("actions", ())[0] - action_inst = self.get_rule(data=action, rule=rule_object) - results = list(action_inst.after(event=event)) - assert len(results) == 1 - notification_rule = NotificationRule( - id=rule_object.id, + action_id=rule_object.id, label=rule_object.label, data={"actions": [action]}, project=rule_object.project, @@ -80,6 +76,10 @@ def trigger(self, event, rule_object): workflow_id=123, legacy_rule_id=rule_object.id, ) + action_inst = self.get_rule(data=action, rule=notification_rule) + results = list(action_inst.after(event=event)) + assert len(results) == 1 + rule_future = RuleFuture(rule=notification_rule, kwargs=results[0].kwargs) return results[0].callback(event, futures=[rule_future]) diff --git a/tests/sentry/integrations/github_enterprise/test_ticket_action.py b/tests/sentry/integrations/github_enterprise/test_ticket_action.py index c3da8b4c1656..299a24bfa633 100644 --- a/tests/sentry/integrations/github_enterprise/test_ticket_action.py +++ b/tests/sentry/integrations/github_enterprise/test_ticket_action.py @@ -78,12 +78,8 @@ def stub_get_jwt(self): def trigger(self, event, rule_object): action = rule_object.data.get("actions", ())[0] - action_inst = self.get_rule(data=action, rule=rule_object) - results = list(action_inst.after(event=event)) - assert len(results) == 1 - notification_rule = NotificationRule( - id=rule_object.id, + action_id=rule_object.id, label=rule_object.label, data={"actions": [action]}, project=rule_object.project, @@ -91,6 +87,10 @@ def trigger(self, event, rule_object): workflow_id=123, legacy_rule_id=rule_object.id, ) + action_inst = self.get_rule(data=action, rule=notification_rule) + results = list(action_inst.after(event=event)) + assert len(results) == 1 + rule_future = RuleFuture(rule=notification_rule, kwargs=results[0].kwargs) return results[0].callback(event, futures=[rule_future]) diff --git a/tests/sentry/integrations/jira/test_ticket_action.py b/tests/sentry/integrations/jira/test_ticket_action.py index e74f28e4bbdf..5e9d10a598ae 100644 --- a/tests/sentry/integrations/jira/test_ticket_action.py +++ b/tests/sentry/integrations/jira/test_ticket_action.py @@ -50,12 +50,8 @@ def setUp(self) -> None: def trigger(self, event, rule_object): action = rule_object.data.get("actions", ())[0] - action_inst = self.get_rule(data=action, rule=rule_object) - results = list(action_inst.after(event=event)) - assert len(results) == 1 - notification_rule = NotificationRule( - id=rule_object.id, + action_id=rule_object.id, label=rule_object.label, data={"actions": [action]}, project=rule_object.project, @@ -63,6 +59,10 @@ def trigger(self, event, rule_object): workflow_id=123, legacy_rule_id=rule_object.id, ) + action_inst = self.get_rule(data=action, rule=notification_rule) + results = list(action_inst.after(event=event)) + assert len(results) == 1 + rule_future = RuleFuture(rule=notification_rule, kwargs=results[0].kwargs) return results[0].callback(event, futures=[rule_future]) diff --git a/tests/sentry/integrations/jira_server/test_ticket_action.py b/tests/sentry/integrations/jira_server/test_ticket_action.py index efada004ef99..85ca3aec6820 100644 --- a/tests/sentry/integrations/jira_server/test_ticket_action.py +++ b/tests/sentry/integrations/jira_server/test_ticket_action.py @@ -50,12 +50,8 @@ def setUp(self) -> None: def trigger(self, event: GroupEvent, rule_object: Rule) -> object: action = rule_object.data.get("actions", ())[0] - action_inst = self.get_rule(data=action, rule=rule_object) - results = list(action_inst.after(event=event)) - assert len(results) == 1 - notification_rule = NotificationRule( - id=rule_object.id, + action_id=rule_object.id, label=rule_object.label, data={"actions": [action]}, project=rule_object.project, @@ -63,6 +59,10 @@ def trigger(self, event: GroupEvent, rule_object: Rule) -> object: workflow_id=123, legacy_rule_id=rule_object.id, ) + action_inst = self.get_rule(data=action, rule=notification_rule) + results = list(action_inst.after(event=event)) + assert len(results) == 1 + rule_future = RuleFuture(rule=notification_rule, kwargs=results[0].kwargs) return results[0].callback(event, futures=[rule_future]) diff --git a/tests/sentry/integrations/msteams/test_message_builder.py b/tests/sentry/integrations/msteams/test_message_builder.py index c01559024fe7..bb2531420989 100644 --- a/tests/sentry/integrations/msteams/test_message_builder.py +++ b/tests/sentry/integrations/msteams/test_message_builder.py @@ -427,9 +427,9 @@ def test_issue_message_builder(self) -> None: assert card_json[0] == "{" and card_json[-1] == "}" def test_issue_action_payload_includes_rule_and_workflow_ids(self) -> None: - rule = replace( - self.rules[0], legacy_rule_id=self.rules[0].id, workflow_id=123 - ) + legacy_rule_id = self.rules[0].legacy_rule_id + assert legacy_rule_id is not None + rule = replace(self.rules[0], legacy_rule_id=legacy_rule_id, workflow_id=123) payload = MSTeamsIssueMessageBuilder( group=self.group1, @@ -438,7 +438,7 @@ def test_issue_action_payload_includes_rule_and_workflow_ids(self) -> None: integration=self.integration, ).generate_action_payload(ACTION_TYPE.RESOLVE)["payload"] - assert payload["rules"] == [self.rules[0].id] + assert payload["rules"] == [legacy_rule_id] assert payload["workflows"] == [123] def test_issue_without_description(self) -> None: @@ -450,18 +450,20 @@ def test_issue_without_description(self) -> None: def test_action_payload_uses_explicit_rule_and_workflow_ids(self) -> None: legacy_rule = self.rules[0] + legacy_rule_id = legacy_rule.legacy_rule_id + assert legacy_rule_id is not None rules = [ NotificationRule( - id=legacy_rule.id + 1000, + action_id=legacy_rule.action_id, label="Workflow with legacy rule", - data={"actions": [{"legacy_rule_id": legacy_rule.id}]}, + data={"actions": [{"legacy_rule_id": legacy_rule_id}]}, project=self.project1, environment_id=None, workflow_id=123, - legacy_rule_id=legacy_rule.id, + legacy_rule_id=legacy_rule_id, ), NotificationRule( - id=legacy_rule.id + 2000, + action_id=None, label="Workflow only", data={"actions": [{"workflow_id": 123}]}, project=self.project1, @@ -479,7 +481,7 @@ def test_action_payload_uses_explicit_rule_and_workflow_ids(self) -> None: payload = builder.generate_action_payload(ACTION_TYPE.RESOLVE) - assert payload["payload"]["rules"] == [legacy_rule.id] + assert payload["payload"]["rules"] == [legacy_rule_id] assert payload["payload"]["workflows"] == [123] def test_issue_with_only_one_rule(self) -> None: diff --git a/tests/sentry/integrations/slack/actions/notification/test_slack_notify_service_action.py b/tests/sentry/integrations/slack/actions/notification/test_slack_notify_service_action.py index 8b179bd0109f..996e1be021b9 100644 --- a/tests/sentry/integrations/slack/actions/notification/test_slack_notify_service_action.py +++ b/tests/sentry/integrations/slack/actions/notification/test_slack_notify_service_action.py @@ -1,4 +1,5 @@ from copy import deepcopy +from dataclasses import replace from unittest.mock import MagicMock, patch import orjson @@ -7,6 +8,7 @@ from sentry.integrations.slack import SlackNotifyServiceAction from sentry.integrations.types import EventLifecycleOutcome +from sentry.models.rule import Rule from sentry.notifications.models.notificationmessage import NotificationMessage from sentry.notifications.types import NotificationRule, RuleFuture from sentry.shared_integrations.exceptions import IntegrationError @@ -16,6 +18,10 @@ from sentry.testutils.silo import assume_test_silo_mode +def notification_rule_for_action(rule: Rule) -> NotificationRule: + return replace(NotificationRule.from_deprecated_legacy_rule(rule), action_id=rule.id) + + class TestInit(RuleTestCase): rule_cls = SlackNotifyServiceAction @@ -80,9 +86,7 @@ def test_after_noa( results[0].callback( self.event, - futures=[ - RuleFuture(rule=NotificationRule.from_deprecated_legacy_rule(rule), kwargs={}) - ], + futures=[RuleFuture(rule=notification_rule_for_action(rule), kwargs={})], ) blocks = mock_post.call_args.kwargs["blocks"] blocks = orjson.loads(blocks) @@ -130,9 +134,7 @@ def test_after_noa_slo_halt(self, mock_post: MagicMock, mock_record: MagicMock) results[0].callback( self.event, - futures=[ - RuleFuture(rule=NotificationRule.from_deprecated_legacy_rule(rule), kwargs={}) - ], + futures=[RuleFuture(rule=notification_rule_for_action(rule), kwargs={})], ) blocks = mock_post.call_args.kwargs["blocks"] blocks = orjson.loads(blocks) @@ -186,9 +188,7 @@ def test_after_noa_error( results[0].callback( self.event, - futures=[ - RuleFuture(rule=NotificationRule.from_deprecated_legacy_rule(rule), kwargs={}) - ], + futures=[RuleFuture(rule=notification_rule_for_action(rule), kwargs={})], ) assert NotificationMessage.objects.all().count() == 1 @@ -227,9 +227,7 @@ def test_after_noa_test_action( results[0].callback( self.event, - futures=[ - RuleFuture(rule=NotificationRule.from_deprecated_legacy_rule(rule), kwargs={}) - ], + futures=[RuleFuture(rule=notification_rule_for_action(rule), kwargs={})], ) blocks = mock_post.call_args.kwargs["blocks"] blocks = orjson.loads(blocks) @@ -266,16 +264,15 @@ def test_after_noa_new_ui( rule.id = self.action.id rule.environment_id = None - rule_cls_instance = self.get_rule(data=rule.data["actions"][0], rule=rule) + notification_rule = notification_rule_for_action(rule) + rule_cls_instance = self.get_rule(data=rule.data["actions"][0], rule=notification_rule) results = list(rule_cls_instance.after(event=self.event)) assert len(results) == 1 results[0].callback( self.event, - futures=[ - RuleFuture(rule=NotificationRule.from_deprecated_legacy_rule(rule), kwargs={}) - ], + futures=[RuleFuture(rule=notification_rule, kwargs={})], ) blocks = mock_post.call_args.kwargs["blocks"] blocks = orjson.loads(blocks) @@ -319,9 +316,7 @@ def test_after_with_threads_noa( results[0].callback( self.event, - futures=[ - RuleFuture(rule=NotificationRule.from_deprecated_legacy_rule(rule), kwargs={}) - ], + futures=[RuleFuture(rule=notification_rule_for_action(rule), kwargs={})], ) blocks = mock_post.call_args.kwargs["blocks"] blocks = orjson.loads(blocks) @@ -381,9 +376,7 @@ def test_after_reply_in_thread_noa( results[0].callback( self.event, - futures=[ - RuleFuture(rule=NotificationRule.from_deprecated_legacy_rule(rule), kwargs={}) - ], + futures=[RuleFuture(rule=notification_rule_for_action(rule), kwargs={})], ) blocks = mock_post.call_args.kwargs["blocks"] blocks = orjson.loads(blocks) diff --git a/tests/sentry/integrations/vsts/test_notify_action.py b/tests/sentry/integrations/vsts/test_notify_action.py index ab29fd9b870b..9ec7c030404e 100644 --- a/tests/sentry/integrations/vsts/test_notify_action.py +++ b/tests/sentry/integrations/vsts/test_notify_action.py @@ -75,7 +75,7 @@ def test_create_issue(self) -> None: assert persisted_rule is not None rule_future = RuleFuture( rule=NotificationRule( - id=persisted_rule.id, + action_id=persisted_rule.id, label=persisted_rule.label, data={"actions": [azuredevops_rule.data]}, project=persisted_rule.project, diff --git a/tests/sentry/mail/test_adapter.py b/tests/sentry/mail/test_adapter.py index 7834ae179f7f..e459fd21e681 100644 --- a/tests/sentry/mail/test_adapter.py +++ b/tests/sentry/mail/test_adapter.py @@ -243,7 +243,7 @@ def test_simple_notification(self, mock_record: MagicMock) -> None: organization_id=self.organization.id, project_id=self.project.id, provider="email", - alert_id=rule.id, + alert_id=rule.data["actions"][0]["workflow_id"], alert_type="issue_alert", external_id="ANY", notification_uuid="ANY", @@ -1589,7 +1589,8 @@ class MailAdapterRuleNotifyTest(BaseMailAdapterTest): def test_normal(self, mock_logger: MagicMock) -> None: event = self.store_event(data={}, project_id=self.project.id) rule = self.create_project_rule(name="my rule") - futures = [RuleFuture(NotificationRule.from_deprecated_legacy_rule(rule), {})] + notification_rule = NotificationRule.from_deprecated_legacy_rule(rule) + futures = [RuleFuture(notification_rule, {})] with mock.patch.object(self.adapter, "notify") as notify: self.adapter.rule_notify(event, futures, ActionTargetType.ISSUE_OWNERS) assert notify.call_count == 1 @@ -1605,7 +1606,8 @@ def test_normal(self, mock_logger: MagicMock) -> None: "target_identifier": None, "fallthrough_choice": None, "notification_uuid": mock.ANY, - "rule_id": rule.id, + "workflow_id": notification_rule.workflow_id, + "legacy_rule_id": notification_rule.legacy_rule_id, "project_id": event.group.project.id, }, ) @@ -1618,7 +1620,8 @@ def test_digest(self, mock_logger: MagicMock, digests: MagicMock) -> None: event = self.store_event(data={}, project_id=self.project.id) rule = self.create_project_rule(project=self.project) - futures = [RuleFuture(NotificationRule.from_deprecated_legacy_rule(rule), {})] + notification_rule = NotificationRule.from_deprecated_legacy_rule(rule) + futures = [RuleFuture(notification_rule, {})] self.adapter.rule_notify(event, futures, ActionTargetType.ISSUE_OWNERS) assert digests.backend.add.call_count == 1 assert event.group @@ -1633,7 +1636,8 @@ def test_digest(self, mock_logger: MagicMock, digests: MagicMock) -> None: "target_identifier": None, "fallthrough_choice": None, "notification_uuid": mock.ANY, - "rule_id": rule.id, + "workflow_id": notification_rule.workflow_id, + "legacy_rule_id": notification_rule.legacy_rule_id, "project_id": event.group.project.id, "digest_key": mock.ANY, }, diff --git a/tests/sentry/notifications/notification_action/test_issue_alert_registry_handlers.py b/tests/sentry/notifications/notification_action/test_issue_alert_registry_handlers.py index a826932d01fa..c543e72cf6ac 100644 --- a/tests/sentry/notifications/notification_action/test_issue_alert_registry_handlers.py +++ b/tests/sentry/notifications/notification_action/test_issue_alert_registry_handlers.py @@ -123,7 +123,7 @@ def test_create_rule_instance_from_action(self) -> None: ) assert isinstance(rule, NotificationRule) - assert rule.id == self.action.id + assert rule.action_id == self.action.id assert rule.project == self.detector.project assert rule.environment_id is not None assert self.workflow.environment is not None @@ -157,7 +157,7 @@ def test_create_rule_instance_from_action(self) -> None: assert {rule: "first", other_rule: "second"} == {rule: "second"} same_numeric_legacy_rule = NotificationRule( - id=self.workflow.id, + action_id=None, label="Legacy rule", data={"actions": [{"id": "test-action"}]}, project=self.project, @@ -176,7 +176,7 @@ def test_create_rule_instance_from_action_with_workflow_only(self) -> None: ) assert isinstance(rule, NotificationRule) - assert rule.id == self.action.id + assert rule.action_id == self.action.id assert rule.project == self.detector.project assert rule.environment_id is not None assert self.workflow.environment is not None @@ -263,7 +263,7 @@ def test_notification_rule_rejects_invalid_identity(self) -> None: with pytest.raises(ValueError, match="requires at least one action"): NotificationRule( - id=self.action.id, + action_id=self.action.id, label="Invalid", data={"actions": []}, project=self.project, @@ -274,7 +274,7 @@ def test_notification_rule_rejects_invalid_identity(self) -> None: with pytest.raises(ValueError, match="requires a workflow or legacy rule ID"): NotificationRule( - id=self.action.id, + action_id=self.action.id, label="Invalid", data=data, project=self.project, @@ -285,7 +285,7 @@ def test_notification_rule_rejects_invalid_identity(self) -> None: with pytest.raises(ValueError, match="cannot have a workflow ID"): NotificationRule( - id=self.action.id, + action_id=self.action.id, label="Invalid", data=data, project=self.project, @@ -298,7 +298,8 @@ def test_from_deprecated_legacy_rule(self) -> None: legacy_rule = self.create_project_rule(project=self.project, include_workflow_id=False) notification_rule = NotificationRule.from_deprecated_legacy_rule(legacy_rule) - assert notification_rule.id == legacy_rule.id + assert notification_rule.action_id is None + assert notification_rule.legacy_rule_id == legacy_rule.id assert notification_rule.legacy_rule_id == legacy_rule.id assert notification_rule.workflow_id is None assert notification_rule.is_legacy_rule_only @@ -326,7 +327,7 @@ def test_create_rule_instance_from_action_no_environment(self) -> None: ) assert isinstance(rule, NotificationRule) - assert rule.id == self.action.id + assert rule.action_id == self.action.id assert rule.project == self.detector.project assert rule.environment_id is None assert rule.label == self.workflow.name diff --git a/tests/sentry/notifications/platform/slack/renderers/test_issue.py b/tests/sentry/notifications/platform/slack/renderers/test_issue.py index ec30d14ed8b0..afa2f28c5601 100644 --- a/tests/sentry/notifications/platform/slack/renderers/test_issue.py +++ b/tests/sentry/notifications/platform/slack/renderers/test_issue.py @@ -87,9 +87,26 @@ def test_deserializes_legacy_rule_proxy(self) -> None: rule = proxy.to_notification_rule(self.project) + assert rule.action_id == 1 assert rule.workflow_id == 2 assert rule.legacy_rule_id is None + def test_deserializes_workflow_rule_without_action(self) -> None: + proxy = SerializableRuleProxy( + id=2, + action_id=None, + label="Workflow payload", + data={"actions": [{"workflow_id": 2}]}, + project_id=self.project.id, + workflow_id=2, + legacy_rule_id=None, + ) + + rule = proxy.to_notification_rule(self.project) + + assert rule.action_id is None + assert rule.workflow_id == 2 + def test_source(self) -> None: data = IssueNotificationData( group_id=self.group.id, @@ -117,7 +134,7 @@ def test_from_action_invocation(self) -> None: assert result.event_id == invocation.event_data.event.event_id assert result.notification_uuid == "test-uuid-123" assert isinstance(result.rule, SerializableRuleProxy) - assert result.rule.id == invocation.action.id + assert result.rule.action_id == invocation.action.id assert result.rule.label == "Test Workflow" assert result.rule.workflow_id == invocation.workflow_id assert result.rule.legacy_rule_id is None diff --git a/tests/sentry/sentry_apps/tasks/test_sentry_apps.py b/tests/sentry/sentry_apps/tasks/test_sentry_apps.py index fc587b28db4c..47e7686be609 100644 --- a/tests/sentry/sentry_apps/tasks/test_sentry_apps.py +++ b/tests/sentry/sentry_apps/tasks/test_sentry_apps.py @@ -124,7 +124,7 @@ def setUp(self) -> None: self.sentry_app = self.create_sentry_app(organization=self.organization) self.rule = self.create_project_rule(name="Issa Rule") self.notification_rule = NotificationRule( - id=self.rule.id, + action_id=self.rule.id, label=self.rule.label, data={"actions": self.rule.data["actions"]}, project=self.rule.project, @@ -387,7 +387,7 @@ def test_send_alert_event_with_additional_payload_legacy_rule_id( rule_future = RuleFuture( rule=NotificationRule( - id=rule.id, + action_id=rule.id, label=rule.label, data={"actions": rule.data["actions"]}, project=rule.project, From 28f27c67b9628905317597c6ad461ed335284123 Mon Sep 17 00:00:00 2001 From: Kyle Consalus Date: Fri, 2 Oct 2026 14:49:26 -0700 Subject: [PATCH 11/12] test fixes --- .../slack/test_message_builder.py | 3 +- .../sentry/notifications/utils/test_rules.py | 32 ++++++++++++------- 2 files changed, 23 insertions(+), 12 deletions(-) diff --git a/tests/sentry/integrations/slack/test_message_builder.py b/tests/sentry/integrations/slack/test_message_builder.py index cdc4486ca9aa..be72b1c291fe 100644 --- a/tests/sentry/integrations/slack/test_message_builder.py +++ b/tests/sentry/integrations/slack/test_message_builder.py @@ -465,8 +465,9 @@ def test_build_group_block_with_workflow_only(self) -> None: rule = self.create_project_rule(project=self.project) workflow_id = rule.data["actions"][0]["workflow_id"] rule.data["actions"][0].pop("legacy_rule_id") + notification_rule = NotificationRule.from_deprecated_legacy_rule(rule) - blocks = SlackIssuesMessageBuilder(self.group, rules=[rule]).build()["blocks"] + blocks = SlackIssuesMessageBuilder(self.group, rules=[notification_rule]).build()["blocks"] assert orjson.loads(blocks[0]["block_id"]) == { "issue": self.group.id, diff --git a/tests/sentry/notifications/utils/test_rules.py b/tests/sentry/notifications/utils/test_rules.py index f85f26d03136..397fdab3b844 100644 --- a/tests/sentry/notifications/utils/test_rules.py +++ b/tests/sentry/notifications/utils/test_rules.py @@ -1,28 +1,38 @@ -from sentry.models.rule import Rule +from sentry.models.project import Project +from sentry.notifications.types import NotificationRule from sentry.notifications.utils.rules import get_rule_or_workflow_id -def _rule(action: dict[str, str]) -> Rule: - return Rule(id=99, data={"actions": [action]}) +def _rule(*, legacy_rule_id: int | None, workflow_id: int | None) -> NotificationRule: + return NotificationRule( + action_id=None, + label="Test rule", + data={"actions": [{}]}, + project=Project(id=1), + environment_id=None, + workflow_id=workflow_id, + legacy_rule_id=legacy_rule_id, + ) def test_get_rule_or_workflow_id_prefers_legacy_rule_id_by_default() -> None: - rule = _rule({"legacy_rule_id": "1", "workflow_id": "2"}) + rule = _rule(legacy_rule_id=1, workflow_id=2) assert get_rule_or_workflow_id(rule) == ("legacy_rule_id", "1") def test_get_rule_or_workflow_id_prefer_workflow() -> None: - rule = _rule({"legacy_rule_id": "1", "workflow_id": "2"}) + rule = _rule(legacy_rule_id=1, workflow_id=2) assert get_rule_or_workflow_id(rule, prefer="workflow_id") == ("workflow_id", "2") def test_get_rule_or_workflow_id_falls_back_to_available_id() -> None: - assert get_rule_or_workflow_id(_rule({"legacy_rule_id": "1"}), prefer="workflow_id") == ( + assert get_rule_or_workflow_id( + _rule(legacy_rule_id=1, workflow_id=None), prefer="workflow_id" + ) == ( "legacy_rule_id", "1", ) - assert get_rule_or_workflow_id(_rule({"workflow_id": "2"})) == ("workflow_id", "2") - - -def test_get_rule_or_workflow_id_falls_back_to_rule_id() -> None: - assert get_rule_or_workflow_id(_rule({}), prefer="workflow_id") == ("legacy_rule_id", "99") + assert get_rule_or_workflow_id(_rule(legacy_rule_id=None, workflow_id=2)) == ( + "workflow_id", + "2", + ) From ff13202dce9209901521231bfa675fd2ffe690ea Mon Sep 17 00:00:00 2001 From: Kyle Consalus Date: Fri, 2 Oct 2026 15:29:40 -0700 Subject: [PATCH 12/12] restore --- .../discord/message_builder/issues.py | 6 +- .../msteams/card_builder/issues.py | 4 +- src/sentry/integrations/msteams/webhook.py | 12 ++-- .../slack/actions/notification.py | 2 +- .../integrations/slack/webhooks/action.py | 24 +++---- src/sentry/mail/adapter.py | 3 +- .../notification_action/types.py | 3 +- .../notifications/notifications/digest.py | 3 +- .../notifications/notifications/rules.py | 5 +- .../platform/msteams/renderers/issue.py | 4 +- .../notifications/platform/templates/issue.py | 20 ++++-- src/sentry/notifications/types.py | 10 ++- src/sentry/notifications/utils/rules.py | 63 +------------------ .../integrations/create_ticket/utils.py | 2 +- .../templates/sentry/emails/_group.html | 8 +-- .../templates/sentry/emails/digests/body.html | 6 +- tests/sentry/digests/test_notifications.py | 4 +- tests/sentry/digests/test_utilities.py | 8 ++- .../msteams/test_message_builder.py | 4 +- .../slack/notifications/test_issue_alert.py | 14 ++++- tests/sentry/mail/test_adapter.py | 8 +-- .../test_issue_alert_registry_handlers.py | 44 +++++++++++++ .../platform/msteams/renderers/test_issue.py | 2 +- .../platform/slack/renderers/test_issue.py | 32 ++++++++++ 24 files changed, 158 insertions(+), 133 deletions(-) diff --git a/src/sentry/integrations/discord/message_builder/issues.py b/src/sentry/integrations/discord/message_builder/issues.py index 430610fea907..e06191aec11c 100644 --- a/src/sentry/integrations/discord/message_builder/issues.py +++ b/src/sentry/integrations/discord/message_builder/issues.py @@ -57,14 +57,14 @@ def build(self, notification_uuid: str | None = None) -> DiscordMessage: max(self.group.last_seen, self.event.datetime) if self.event else self.group.last_seen ) obj: Group | GroupEvent = self.event if self.event is not None else self.group - rule_id = None + rule_id: int | None = None rule_environment_id = None is_workflow = False if self.rules: rule_environment_id = self.rules[0].environment_id - key, rule_id = get_rule_or_workflow_id(self.rules[0], prefer="workflow_id") + key, rule_id_value = get_rule_or_workflow_id(self.rules[0], prefer="workflow_id") is_workflow = key == "workflow_id" - rule_id = int(rule_id) + rule_id = int(rule_id_value) url = None if is_workflow: diff --git a/src/sentry/integrations/msteams/card_builder/issues.py b/src/sentry/integrations/msteams/card_builder/issues.py index c3ba26eed134..b9bc43d762f7 100644 --- a/src/sentry/integrations/msteams/card_builder/issues.py +++ b/src/sentry/integrations/msteams/card_builder/issues.py @@ -79,9 +79,7 @@ def generate_action_payload(self, action_type: ACTION_TYPE) -> Any: "actionType": action_type, "groupId": self.group.id, "eventId": self.event.event_id if self.event else None, - "rules": [ - rule.legacy_rule_id for rule in self.rules if rule.legacy_rule_id is not None - ], + "rules": [rule.broken_rule_id for rule in self.rules], "workflows": list(dict.fromkeys([*workflow_ids, *self.workflow_ids])), "integrationId": self.integration.id, } diff --git a/src/sentry/integrations/msteams/webhook.py b/src/sentry/integrations/msteams/webhook.py index cc6fa04d47c4..301e7ab01fc8 100644 --- a/src/sentry/integrations/msteams/webhook.py +++ b/src/sentry/integrations/msteams/webhook.py @@ -50,7 +50,8 @@ from sentry.models.activity import ActivityIntegration from sentry.models.apikey import ApiKey from sentry.models.group import Group -from sentry.notifications.utils.rules import get_notification_rules +from sentry.models.rule import Rule +from sentry.notifications.types import NotificationRule from sentry.services import eventstore from sentry.silo.base import SiloMode from sentry.users.services.user.service import user_service @@ -654,18 +655,15 @@ def _handle_action_submitted(self, request: Request) -> Response: rule_ids = payload.get("rules", []) workflow_ids = payload.get("workflows", []) rules = tuple( - get_notification_rules( - group.project, - legacy_rule_ids=rule_ids, - workflow_ids=workflow_ids, - ) + NotificationRule.from_deprecated_legacy_rule(rule, project=group.project) + for rule in Rule.objects.filter(id__in=rule_ids, project_id=group.project_id) ) metrics.incr( "integrations.msteams.action.rule_lookup", tags={ "has_rule": bool(rule_ids), "has_workflow_ids": bool(workflow_ids), - "lookup_succeeded": bool(rules), + "lookup_succeeded": bool(rule_ids) and len(rules) == len(set(rule_ids)), }, sample_rate=1.0, ) diff --git a/src/sentry/integrations/slack/actions/notification.py b/src/sentry/integrations/slack/actions/notification.py index b51d09a307c6..6aa4446aadae 100644 --- a/src/sentry/integrations/slack/actions/notification.py +++ b/src/sentry/integrations/slack/actions/notification.py @@ -241,7 +241,7 @@ def _send_notification_action_notification( rule_to_use = self.rule if self.rule else rule # In the NOA, we will store the action id in the rule id field action_id = ( - rule_to_use.action_id + rule_to_use.broken_rule_id if isinstance(rule_to_use, NotificationRule) else rule_to_use.id if rule_to_use diff --git a/src/sentry/integrations/slack/webhooks/action.py b/src/sentry/integrations/slack/webhooks/action.py index a8e3533318ff..a9ba7986aba5 100644 --- a/src/sentry/integrations/slack/webhooks/action.py +++ b/src/sentry/integrations/slack/webhooks/action.py @@ -57,10 +57,10 @@ from sentry.models.activity import ActivityIntegration from sentry.models.group import Group from sentry.models.organizationmember import InviteStatus, OrganizationMember +from sentry.models.rule import Rule from sentry.notifications.services import notifications_service from sentry.notifications.types import NotificationRule from sentry.notifications.utils.actions import BlockKitMessageAction, MessageAction -from sentry.notifications.utils.rules import get_notification_rules from sentry.seer.entrypoints.operator import SeerAutofixOperator from sentry.seer.entrypoints.slack.entrypoint import SlackAutofixEntrypoint from sentry.seer.entrypoints.slack.messaging import send_not_org_member_message @@ -137,18 +137,18 @@ def update_group( return resp -def get_rule( - rule_id: int | None, workflow_id: int | None, group: Group -) -> NotificationRule | None: +def get_rule(rule_id: int | None, organization_id: int) -> NotificationRule | None: """Get the rule that fired""" - if not rule_id and not workflow_id: + if not rule_id: return None - rules = get_notification_rules( - group.project, - legacy_rule_ids=[rule_id] if rule_id else [], - workflow_ids=[workflow_id] if workflow_id else [], - ) - return rules[0] if rules else None + try: + # Scope the callback-provided rule ID to the integration-validated organization. + rule = Rule.objects.get(id=rule_id, project__organization_id=organization_id) + # The callback contract puns Rule and Workflow IDs, so preserve Rule.id here. + rule.data["actions"][0]["legacy_rule_id"] = rule.id + except Rule.DoesNotExist: + return None + return NotificationRule.from_deprecated_legacy_rule(rule) def get_group(slack_request: SlackActionRequest) -> Group | None: @@ -366,7 +366,7 @@ def _handle_group_actions( rule_id = slack_request.callback_data.get("rule") workflow_id = slack_request.callback_data.get("workflow") - rule = get_rule(rule_id, workflow_id, group) + rule = get_rule(rule_id, group.project.organization_id) metrics.incr( "integrations.slack.action.rule_lookup", tags={ diff --git a/src/sentry/mail/adapter.py b/src/sentry/mail/adapter.py index 097bcaca5de3..999aa965fab8 100644 --- a/src/sentry/mail/adapter.py +++ b/src/sentry/mail/adapter.py @@ -65,8 +65,7 @@ def rule_notify( log_event = "dispatched" for future in futures: rules.append(future.rule) - extra["workflow_id"] = future.rule.workflow_id - extra["legacy_rule_id"] = future.rule.legacy_rule_id + extra["rule_id"] = future.rule.broken_rule_id if not future.kwargs: continue raise NotImplementedError( diff --git a/src/sentry/notifications/notification_action/types.py b/src/sentry/notifications/notification_action/types.py index 6a0a8b8869da..6aca62dd26ec 100644 --- a/src/sentry/notifications/notification_action/types.py +++ b/src/sentry/notifications/notification_action/types.py @@ -360,8 +360,7 @@ def invoke_legacy_registry(cls, invocation: ActionInvocation) -> None: "action_id": invocation.action.id, "detector_id": invocation.detector.id, "event_data": asdict(invocation.event_data), - "legacy_rule_id": rule.legacy_rule_id, - "workflow_id": rule.workflow_id, + "rule_id": rule.broken_rule_id, "rule_project_id": rule.project.id, "rule_environment_id": rule.environment_id, "rule_label": rule.label, diff --git a/src/sentry/notifications/notifications/digest.py b/src/sentry/notifications/notifications/digest.py index 2ad38b0fc0fa..85ddf3eaa4b8 100644 --- a/src/sentry/notifications/notifications/digest.py +++ b/src/sentry/notifications/notifications/digest.py @@ -265,8 +265,7 @@ def send(self) -> None: def get_log_params(self, recipient: Actor) -> Mapping[str, Any]: try: - rule = list(self.digest.digest)[0] - alert_id = rule.workflow_id or rule.legacy_rule_id + alert_id = list(self.digest.digest)[0].broken_rule_id except Exception: alert_id = None diff --git a/src/sentry/notifications/notifications/rules.py b/src/sentry/notifications/notifications/rules.py index f283eeeb5fba..63f03b82399e 100644 --- a/src/sentry/notifications/notifications/rules.py +++ b/src/sentry/notifications/notifications/rules.py @@ -358,13 +358,10 @@ def send(self) -> None: notify(provider, self, participants, shared_context) def get_log_params(self, recipient: Actor) -> Mapping[str, Any]: - alert_id = None - if self.rules: - alert_id = self.rules[0].workflow_id or self.rules[0].legacy_rule_id return { "target_type": self.target_type, "target_identifier": self.target_identifier, - "alert_id": alert_id, + "alert_id": self.rules[0].broken_rule_id if self.rules else None, **super().get_log_params(recipient), } diff --git a/src/sentry/notifications/platform/msteams/renderers/issue.py b/src/sentry/notifications/platform/msteams/renderers/issue.py index f55e7aa72cbb..9e92eec0c33a 100644 --- a/src/sentry/notifications/platform/msteams/renderers/issue.py +++ b/src/sentry/notifications/platform/msteams/renderers/issue.py @@ -202,9 +202,7 @@ def build_action_payload( "actionType": action_type, "groupId": data.group_id, "eventId": data.event_id, - "rules": [ - rule.legacy_rule_id for rule in rules if rule.legacy_rule_id is not None - ], + "rules": [rule.broken_rule_id for rule in rules], "workflows": get_workflow_ids(rules), } } diff --git a/src/sentry/notifications/platform/templates/issue.py b/src/sentry/notifications/platform/templates/issue.py index 124be7372f95..af3de9d4e7a3 100644 --- a/src/sentry/notifications/platform/templates/issue.py +++ b/src/sentry/notifications/platform/templates/issue.py @@ -1,5 +1,7 @@ from __future__ import annotations +from typing import Any + from pydantic import BaseModel, ConfigDict from sentry.models.project import Project @@ -24,7 +26,7 @@ class SerializableRuleProxy(BaseModel): id: int action_id: int | None = None label: str - data: NotificationRuleData + data: dict[str, Any] environment_id: int | None = None project_id: int workflow_id: int | None = None @@ -47,22 +49,30 @@ def from_rule(cls, rule: NotificationRule) -> SerializableRuleProxy: def to_notification_rule(self, project: Project) -> NotificationRule: workflow_id = self.workflow_id legacy_rule_id = self.legacy_rule_id + actions = self.data.get("actions") + if ( + not isinstance(actions, list) + or not actions + or not all(isinstance(action, dict) for action in actions) + ): + actions = [{}] + data: NotificationRuleData = {"actions": [dict(action) for action in actions]} if workflow_id is None and legacy_rule_id is None: # Compatibility for payloads serialized before identities became top-level fields. - actions = self.data["actions"] - action = actions[0] if actions else {} + action = actions[0] workflow_id = action.get("workflow_id") legacy_rule_id = action.get("legacy_rule_id") + workflow_id = int(workflow_id) if workflow_id is not None else None + legacy_rule_id = int(legacy_rule_id) if legacy_rule_id is not None else None if workflow_id == TEST_NOTIFICATION_ID or legacy_rule_id == TEST_NOTIFICATION_ID: workflow_id = None legacy_rule_id = TEST_NOTIFICATION_ID elif workflow_id is None and legacy_rule_id is None: legacy_rule_id = self.id - return NotificationRule( action_id=self.action_id if "action_id" in self.__fields_set__ else self.id, label=self.label, - data=self.data, + data=data, environment_id=self.environment_id, project=project, workflow_id=workflow_id, diff --git a/src/sentry/notifications/types.py b/src/sentry/notifications/types.py index eeafd4f57fdd..56ea8b547e54 100644 --- a/src/sentry/notifications/types.py +++ b/src/sentry/notifications/types.py @@ -57,14 +57,16 @@ def from_deprecated_legacy_rule( first_action = actions[0] embedded_workflow_id = first_action.get("workflow_id") embedded_legacy_rule_id = first_action.get("legacy_rule_id") + if embedded_workflow_id is not None: + embedded_workflow_id = int(embedded_workflow_id) + if embedded_legacy_rule_id is not None: + embedded_legacy_rule_id = int(embedded_legacy_rule_id) if embedded_legacy_rule_id == TEST_NOTIFICATION_ID: effective_workflow_id = None legacy_rule_id = TEST_NOTIFICATION_ID elif workflow_id is not None: effective_workflow_id = workflow_id - legacy_rule_id = ( - embedded_legacy_rule_id if embedded_legacy_rule_id is not None else rule.id - ) + legacy_rule_id = rule.id elif embedded_legacy_rule_id is not None: effective_workflow_id = embedded_workflow_id legacy_rule_id = embedded_legacy_rule_id @@ -99,6 +101,8 @@ def __post_init__(self) -> None: @property def identifier(self) -> str: + if self.is_test_notification and self.action_id is not None: + return f"test:{self.action_id}" if self.workflow_id is not None: return f"workflow:{self.workflow_id}" assert self.legacy_rule_id is not None diff --git a/src/sentry/notifications/utils/rules.py b/src/sentry/notifications/utils/rules.py index a0fe95943b8e..93e39dba09fc 100644 --- a/src/sentry/notifications/utils/rules.py +++ b/src/sentry/notifications/utils/rules.py @@ -1,73 +1,12 @@ -from collections.abc import Iterable, Sequence +from collections.abc import Sequence from dataclasses import dataclass from typing import Literal -from sentry.models.project import Project -from sentry.models.rule import Rule from sentry.notifications.types import NotificationRule -from sentry.workflow_engine.models import AlertRuleWorkflow, Workflow RuleIdType = Literal["workflow_id", "legacy_rule_id"] -def get_notification_rules( - project: Project, - *, - legacy_rule_ids: Iterable[int] = (), - workflow_ids: Iterable[int] = (), -) -> list[NotificationRule]: - workflow_ids = list(dict.fromkeys(workflow_ids)) - legacy_rule_ids = list(dict.fromkeys(legacy_rule_ids)) - workflows = Workflow.objects.filter(organization_id=project.organization_id).in_bulk( - workflow_ids - ) - workflow_links = { - link.workflow_id: link.rule_id - for link in AlertRuleWorkflow.objects.filter( - workflow_id__in=workflows, rule_id__isnull=False - ) - } - rules = Rule.objects.filter(project_id=project.id).in_bulk( - {*legacy_rule_ids, *workflow_links.values()} - ) - - notification_rules = [] - linked_rule_ids = set() - for workflow_id in workflow_ids: - workflow = workflows.get(workflow_id) - if workflow is None: - continue - - legacy_rule_id = workflow_links.get(workflow_id) - legacy_rule = rules.get(legacy_rule_id) if legacy_rule_id is not None else None - if legacy_rule is not None: - linked_rule_ids.add(legacy_rule.id) - notification_rules.append( - NotificationRule.from_deprecated_legacy_rule( - legacy_rule, project=project, workflow_id=workflow_id - ) - ) - else: - notification_rules.append( - NotificationRule( - action_id=None, - label=workflow.name, - data={"actions": [{"workflow_id": workflow_id}]}, - project=project, - environment_id=workflow.environment_id, - workflow_id=workflow_id, - legacy_rule_id=None, - ) - ) - - notification_rules.extend( - NotificationRule.from_deprecated_legacy_rule(rule, project=project) - for rule_id in legacy_rule_ids - if rule_id not in linked_rule_ids and (rule := rules.get(rule_id)) is not None - ) - return notification_rules - - @dataclass class RulesAndWorkflows: rules: list[NotificationRule] diff --git a/src/sentry/rules/actions/integrations/create_ticket/utils.py b/src/sentry/rules/actions/integrations/create_ticket/utils.py index b24055278d1f..5a2345edae54 100644 --- a/src/sentry/rules/actions/integrations/create_ticket/utils.py +++ b/src/sentry/rules/actions/integrations/create_ticket/utils.py @@ -138,7 +138,7 @@ def create_issue(event: GroupEvent, futures: Sequence[RuleFuture]) -> None: organization = event.group.project.organization for future in futures: - action_id = future.rule.action_id + action_id = future.rule.broken_rule_id data: dict[str, Any] = future.kwargs["data"] provider = future.kwargs.get("provider") integration_id = future.kwargs.get("integration_id") diff --git a/src/sentry/templates/sentry/emails/_group.html b/src/sentry/templates/sentry/emails/_group.html index 4c3102141a20..651760e1d251 100644 --- a/src/sentry/templates/sentry/emails/_group.html +++ b/src/sentry/templates/sentry/emails/_group.html @@ -6,7 +6,7 @@

{% if metadata.type %} {%if rule %} - {% if rule_id %}{{metadata.type|truncatechars:40}}{% else %}{{metadata.type|truncatechars:40}}{% endif %} + {{metadata.type|truncatechars:40}} {% else %} {{metadata.type|truncatechars:40}} {% endif %} @@ -19,7 +19,7 @@

{% endif %} {% else %} {%if rule %} - {% if rule_id %}{{metadata.value|truncatechars:40}}{% else %}{{metadata.value|truncatechars:40}}{% endif %} + {{metadata.value|truncatechars:40}} {% else %} {{metadata.value|truncatechars:40}} {% endif %} @@ -34,7 +34,7 @@

{%if rule %} - {% if rule_id %}{{metadata.directive|truncatechars:40}}{% else %}{{metadata.directive|truncatechars:40}}{% endif %} + {{metadata.directive|truncatechars:40}} {% else %} {{metadata.directive|truncatechars:40}} {% endif %} @@ -48,7 +48,7 @@

{%if rule %} - {% if rule_id %}{{group.title|truncatechars:40}}{% else %}{{group.title|truncatechars:40}}{% endif %} + {{group.title|truncatechars:40}} {% else %} {{group.title|truncatechars:40}} {% endif %} diff --git a/src/sentry/templates/sentry/emails/digests/body.html b/src/sentry/templates/sentry/emails/digests/body.html index 25c7babff217..4358707a5763 100644 --- a/src/sentry/templates/sentry/emails/digests/body.html +++ b/src/sentry/templates/sentry/emails/digests/body.html @@ -48,11 +48,10 @@

{{ counts|length }} new alert{{ counts|pluralize }} from
- {% with rule_details=rules_details|get_item:rule_context_id snooze_alert_url=snooze_alert_urls|get_item:rule_context_id %} + {% with rule_details=rules_details|get_item:rule.broken_rule_id snooze_alert_url=snooze_alert_urls|get_item:rule.broken_rule_id %} {% if snooze_alert %} Mute this alert {% endif %} @@ -73,7 +72,7 @@

{{ counts|length }} new alert{{ counts|pluralize }} from {{ group.get_level_display }} - {% include "sentry/emails/_group.html" with rule_id=rule_context_id %} + {% include "sentry/emails/_group.html" %}
{% if clock_24_hours %} @@ -107,7 +106,6 @@

{{ counts|length }} new alert{{ counts|pluralize }} from None: + def test_action_payload_preserves_rule_id_contract(self) -> None: legacy_rule = self.rules[0] legacy_rule_id = legacy_rule.legacy_rule_id assert legacy_rule_id is not None @@ -481,7 +481,7 @@ def test_action_payload_uses_explicit_rule_and_workflow_ids(self) -> None: payload = builder.generate_action_payload(ACTION_TYPE.RESOLVE) - assert payload["payload"]["rules"] == [legacy_rule_id] + assert payload["payload"]["rules"] == [legacy_rule_id, 123] assert payload["payload"]["workflows"] == [123] def test_issue_with_only_one_rule(self) -> None: diff --git a/tests/sentry/integrations/slack/notifications/test_issue_alert.py b/tests/sentry/integrations/slack/notifications/test_issue_alert.py index 94190ae8d620..51523c6c4c5d 100644 --- a/tests/sentry/integrations/slack/notifications/test_issue_alert.py +++ b/tests/sentry/integrations/slack/notifications/test_issue_alert.py @@ -334,7 +334,15 @@ def test_issue_alert_issue_owners_block(self) -> None: == f"{event.project.slug} | " ) - def _assert_issue_owners_env_block(self, rule: Rule, environment: Environment) -> None: + def _assert_issue_owners_env_block( + self, rule: Rule | NotificationRule, environment: Environment + ) -> None: + workflow_id = ( + rule.workflow_id + if isinstance(rule, NotificationRule) + else rule.data["actions"][0]["workflow_id"] + ) + assert workflow_id is not None event = self.store_event( data={"message": "Hello world", "level": "error", "environment": environment.name}, project_id=self.project.id, @@ -355,13 +363,13 @@ def _assert_issue_owners_env_block(self, rule: Rule, environment: Environment) - notification_uuid = notification.notification_uuid assert ( fallback_text - == f"Alert triggered " + == f"Alert triggered " ) assert blocks[0]["text"]["text"] == fallback_text assert event.group assert ( blocks[1]["text"]["text"] - == f":red_circle: " + == f":red_circle: " ) assert ( blocks[4]["elements"][0]["text"] diff --git a/tests/sentry/mail/test_adapter.py b/tests/sentry/mail/test_adapter.py index e459fd21e681..f84649e46811 100644 --- a/tests/sentry/mail/test_adapter.py +++ b/tests/sentry/mail/test_adapter.py @@ -243,7 +243,7 @@ def test_simple_notification(self, mock_record: MagicMock) -> None: organization_id=self.organization.id, project_id=self.project.id, provider="email", - alert_id=rule.data["actions"][0]["workflow_id"], + alert_id=rule.id, alert_type="issue_alert", external_id="ANY", notification_uuid="ANY", @@ -1606,8 +1606,7 @@ def test_normal(self, mock_logger: MagicMock) -> None: "target_identifier": None, "fallthrough_choice": None, "notification_uuid": mock.ANY, - "workflow_id": notification_rule.workflow_id, - "legacy_rule_id": notification_rule.legacy_rule_id, + "rule_id": notification_rule.broken_rule_id, "project_id": event.group.project.id, }, ) @@ -1636,8 +1635,7 @@ def test_digest(self, mock_logger: MagicMock, digests: MagicMock) -> None: "target_identifier": None, "fallthrough_choice": None, "notification_uuid": mock.ANY, - "workflow_id": notification_rule.workflow_id, - "legacy_rule_id": notification_rule.legacy_rule_id, + "rule_id": notification_rule.broken_rule_id, "project_id": event.group.project.id, "digest_key": mock.ANY, }, diff --git a/tests/sentry/notifications/notification_action/test_issue_alert_registry_handlers.py b/tests/sentry/notifications/notification_action/test_issue_alert_registry_handlers.py index c543e72cf6ac..bce1edcd76f2 100644 --- a/tests/sentry/notifications/notification_action/test_issue_alert_registry_handlers.py +++ b/tests/sentry/notifications/notification_action/test_issue_alert_registry_handlers.py @@ -305,6 +305,50 @@ def test_from_deprecated_legacy_rule(self) -> None: assert notification_rule.is_legacy_rule_only assert notification_rule.data == {"actions": legacy_rule.data["actions"]} + def test_from_deprecated_legacy_rule_normalizes_string_identity(self) -> None: + legacy_rule = self.create_project_rule(project=self.project) + legacy_rule.data["actions"][0]["legacy_rule_id"] = str(legacy_rule.id) + legacy_rule.data["actions"][0]["workflow_id"] = str(self.workflow.id) + + notification_rule = NotificationRule.from_deprecated_legacy_rule(legacy_rule) + + assert notification_rule.legacy_rule_id == legacy_rule.id + assert notification_rule.workflow_id == self.workflow.id + + def test_explicit_workflow_uses_authoritative_legacy_rule_id(self) -> None: + legacy_rule = self.create_project_rule(project=self.project) + legacy_rule.data["actions"][0]["legacy_rule_id"] = legacy_rule.id + 1 + + notification_rule = NotificationRule.from_deprecated_legacy_rule( + legacy_rule, workflow_id=self.workflow.id + ) + + assert notification_rule.legacy_rule_id == legacy_rule.id + assert notification_rule.workflow_id == self.workflow.id + + def test_test_notification_identity_uses_action_id(self) -> None: + data: NotificationRuleData = {"actions": [{"id": "test-action"}]} + first = NotificationRule( + action_id=1, + label="First", + data=data, + project=self.project, + environment_id=None, + workflow_id=None, + legacy_rule_id=TEST_NOTIFICATION_ID, + ) + second = NotificationRule( + action_id=2, + label="Second", + data=data, + project=self.project, + environment_id=None, + workflow_id=None, + legacy_rule_id=TEST_NOTIFICATION_ID, + ) + + assert first != second + def test_from_deprecated_legacy_rule_without_actions(self) -> None: legacy_rule = self.create_project_rule(project=self.project) legacy_rule.update(data={}) diff --git a/tests/sentry/notifications/platform/msteams/renderers/test_issue.py b/tests/sentry/notifications/platform/msteams/renderers/test_issue.py index cdbbfadcb84e..9baa2218e7b0 100644 --- a/tests/sentry/notifications/platform/msteams/renderers/test_issue.py +++ b/tests/sentry/notifications/platform/msteams/renderers/test_issue.py @@ -152,7 +152,7 @@ def payload(action_type: ACTION_TYPE) -> dict[str, Any]: "actionType": action_type, "groupId": group.id, "eventId": event.event_id, - "rules": [], + "rules": [1], "workflows": [1], } } diff --git a/tests/sentry/notifications/platform/slack/renderers/test_issue.py b/tests/sentry/notifications/platform/slack/renderers/test_issue.py index afa2f28c5601..84c60af5b2a4 100644 --- a/tests/sentry/notifications/platform/slack/renderers/test_issue.py +++ b/tests/sentry/notifications/platform/slack/renderers/test_issue.py @@ -91,6 +91,38 @@ def test_deserializes_legacy_rule_proxy(self) -> None: assert rule.workflow_id == 2 assert rule.legacy_rule_id is None + def test_deserializes_legacy_rule_proxy_without_action_data(self) -> None: + proxy = SerializableRuleProxy.parse_obj( + { + "id": 1, + "label": "Legacy payload", + "data": {}, + "project_id": self.project.id, + } + ) + + rule = proxy.to_notification_rule(self.project) + + assert rule.action_id == 1 + assert rule.workflow_id is None + assert rule.legacy_rule_id == 1 + assert rule.data == {"actions": [{}]} + + def test_deserializes_string_identity(self) -> None: + proxy = SerializableRuleProxy.parse_obj( + { + "id": 1, + "label": "Legacy payload", + "data": {"actions": [{"workflow_id": "2"}]}, + "project_id": self.project.id, + } + ) + + rule = proxy.to_notification_rule(self.project) + + assert rule.workflow_id == 2 + assert rule.legacy_rule_id is None + def test_deserializes_workflow_rule_without_action(self) -> None: proxy = SerializableRuleProxy( id=2,