diff --git a/src/sentry/notifications/notification_action/action_handler_registry/plugin_handler.py b/src/sentry/notifications/notification_action/action_handler_registry/plugin_handler.py index dcd00cdf309490..4e2bf77c6f89f3 100644 --- a/src/sentry/notifications/notification_action/action_handler_registry/plugin_handler.py +++ b/src/sentry/notifications/notification_action/action_handler_registry/plugin_handler.py @@ -1,10 +1,16 @@ +import logging from typing import override +from sentry import features from sentry.notifications.notification_action.utils import execute_via_group_type_registry +from sentry.sentry_apps.services.legacy_webhook.service import send_legacy_webhooks_for_invocation +from sentry.services.eventstore.models import GroupEvent from sentry.workflow_engine.models import Action from sentry.workflow_engine.registry import action_handler_registry from sentry.workflow_engine.types import ActionHandler, ActionInvocation, ConfigTransformer +logger = logging.getLogger(__name__) + @action_handler_registry.register(Action.Type.PLUGIN) class PluginActionHandler(ActionHandler): @@ -36,4 +42,25 @@ def get_config_transformer() -> ConfigTransformer | None: @staticmethod @override def execute(invocation: ActionInvocation) -> None: - execute_via_group_type_registry(invocation) + if not isinstance(invocation.event_data.event, GroupEvent): + return + + organization = invocation.detector.project.organization + new_path = features.has("organizations:legacy-webhook-new-path", organization) + + try: + execute_via_group_type_registry(invocation) + except Exception: + logger.exception( + "plugin_action_handler.old_path_error", + extra={"invocation": invocation}, + ) + + if new_path: + try: + send_legacy_webhooks_for_invocation(invocation) + except Exception: + logger.exception( + "plugin_action_handler.new_path_error", + extra={"invocation": invocation}, + ) diff --git a/src/sentry/rules/actions/notify_event.py b/src/sentry/rules/actions/notify_event.py index 0a73015ea99796..d6698b0d9c9e85 100644 --- a/src/sentry/rules/actions/notify_event.py +++ b/src/sentry/rules/actions/notify_event.py @@ -1,5 +1,6 @@ from collections.abc import Generator, Sequence +from sentry import features from sentry.plugins.base import plugins from sentry.rules.actions.base import EventAction from sentry.rules.actions.services import LegacyPluginService @@ -19,10 +20,16 @@ class NotifyEventAction(EventAction): def get_plugins(self) -> Sequence[LegacyPluginService]: from sentry.plugins.bases.notify import NotificationPlugin + skip_webhooks = features.has( + "organizations:legacy-webhook-disable-old-path", self.project.organization + ) + results = [] for plugin in plugins.for_project(self.project, version=1): if not isinstance(plugin, NotificationPlugin): continue + if skip_webhooks and plugin.slug == "webhooks": + continue results.append(LegacyPluginService(plugin)) return results diff --git a/tests/sentry/notifications/notification_action/action_handler_registry/test_plugin_handler.py b/tests/sentry/notifications/notification_action/action_handler_registry/test_plugin_handler.py new file mode 100644 index 00000000000000..36e78b52612a5d --- /dev/null +++ b/tests/sentry/notifications/notification_action/action_handler_registry/test_plugin_handler.py @@ -0,0 +1,134 @@ +import uuid +from unittest import mock + +import responses + +from sentry.models.activity import Activity +from sentry.models.options.project_option import ProjectOption +from sentry.notifications.notification_action.action_handler_registry.plugin_handler import ( + PluginActionHandler, +) +from sentry.plugins.base import plugins +from sentry.testutils.helpers.features import with_feature +from sentry.types.activity import ActivityType +from sentry.utils import json +from sentry.workflow_engine.models import Action +from sentry.workflow_engine.types import ActionInvocation, WorkflowEventData +from tests.sentry.workflow_engine.test_base import BaseWorkflowTest + + +class TestPluginActionHandlerExecute(BaseWorkflowTest): + def setUp(self) -> None: + super().setUp() + self.detector = self.create_detector(project=self.project) + self.workflow = self.create_workflow(environment=self.environment) + self.action = self.create_action( + type=Action.Type.PLUGIN, + ) + self.group, self.event, self.group_event = self.create_group_event() + self.event_data = WorkflowEventData( + event=self.group_event, workflow_env=self.environment, group=self.group + ) + self.invocation = ActionInvocation( + event_data=self.event_data, + action=self.action, + detector=self.detector, + notification_uuid=str(uuid.uuid4()), + workflow_id=self.workflow.id, + ) + ProjectOption.objects.set_value(self.project, "webhooks:urls", "http://example.com/hook") + webhook_plugin = plugins.get("webhooks") + webhook_plugin.set_option("enabled", True, self.project) + + @responses.activate + def test_default_no_flags_fires_old_path_only(self) -> None: + responses.add(responses.POST, "http://example.com/hook") + + with self.tasks(): + PluginActionHandler.execute(self.invocation) + + assert len(responses.calls) == 1 + + @responses.activate + @with_feature( + { + "organizations:legacy-webhook-new-path": True, + "organizations:legacy-webhook-disable-old-path": True, + } + ) + def test_new_path_skips_webhooks_in_old_path(self) -> None: + """When new path is on and old path disabled, webhooks are sent via the + new task-based service and skipped in the old path.""" + responses.add(responses.POST, "http://example.com/hook") + + with self.tasks(): + PluginActionHandler.execute(self.invocation) + + assert len(responses.calls) == 1 + body = json.loads(responses.calls[0].request.body) + assert body["id"] == str(self.group.id) + + @with_feature("organizations:legacy-webhook-new-path") + @mock.patch( + "sentry.notifications.notification_action.action_handler_registry.plugin_handler.send_legacy_webhooks_for_invocation" + ) + @mock.patch( + "sentry.notifications.notification_action.action_handler_registry.plugin_handler.execute_via_group_type_registry" + ) + def test_old_path_always_runs( + self, mock_old_path: mock.MagicMock, mock_new_path: mock.MagicMock + ) -> None: + """Old path runs regardless of flags to keep non-webhook plugins firing.""" + PluginActionHandler.execute(self.invocation) + + mock_old_path.assert_called_once_with(self.invocation) + mock_new_path.assert_called_once_with(self.invocation) + + @with_feature("organizations:legacy-webhook-new-path") + @mock.patch( + "sentry.notifications.notification_action.action_handler_registry.plugin_handler.send_legacy_webhooks_for_invocation" + ) + @mock.patch( + "sentry.notifications.notification_action.action_handler_registry.plugin_handler.execute_via_group_type_registry" + ) + def test_non_group_event_skips_both_paths( + self, mock_old_path: mock.MagicMock, mock_new_path: mock.MagicMock + ) -> None: + activity = Activity.objects.create( + project=self.project, + group=self.group, + type=ActivityType.SET_RESOLVED.value, + ) + invocation = ActionInvocation( + event_data=WorkflowEventData( + event=activity, workflow_env=self.environment, group=self.group + ), + action=self.action, + detector=self.detector, + notification_uuid=str(uuid.uuid4()), + workflow_id=self.workflow.id, + ) + + PluginActionHandler.execute(invocation) + + mock_old_path.assert_not_called() + mock_new_path.assert_not_called() + + @responses.activate + @with_feature("organizations:legacy-webhook-new-path") + @mock.patch( + "sentry.notifications.notification_action.action_handler_registry.plugin_handler.execute_via_group_type_registry", + side_effect=Exception("legacy path error"), + ) + def test_old_path_exception_does_not_block_new_path( + self, mock_old_path: mock.MagicMock + ) -> None: + responses.add(responses.POST, "http://example.com/hook") + + with self.tasks(): + PluginActionHandler.execute(self.invocation) + + mock_old_path.assert_called_once() + assert len(responses.calls) == 1 + body = json.loads(responses.calls[0].request.body) + assert body["id"] == str(self.group.id) diff --git a/tests/sentry/rules/actions/test_notify_event.py b/tests/sentry/rules/actions/test_notify_event.py index 2403c2de7e5aff..6c558c1b7888e0 100644 --- a/tests/sentry/rules/actions/test_notify_event.py +++ b/tests/sentry/rules/actions/test_notify_event.py @@ -1,8 +1,11 @@ from unittest.mock import MagicMock +from sentry.models.options.project_option import ProjectOption +from sentry.plugins.base import plugins from sentry.rules.actions.notify_event import NotifyEventAction from sentry.rules.actions.services import LegacyPluginService from sentry.testutils.cases import RuleTestCase +from sentry.testutils.helpers.features import with_feature from sentry.testutils.skips import requires_snuba pytestmark = [requires_snuba] @@ -23,3 +26,24 @@ def test_applies_correctly(self) -> None: assert len(results) == 1 assert plugin.should_notify.call_count == 1 assert results[0].callback is plugin.rule_notify + + def test_get_plugins_includes_webhooks_by_default(self) -> None: + ProjectOption.objects.set_value(self.project, "webhooks:urls", "http://example.com/hook") + webhook_plugin = plugins.get("webhooks") + webhook_plugin.set_option("enabled", True, self.project) + + rule = self.get_rule() + result_slugs = [p.service.slug for p in rule.get_plugins()] + + assert "webhooks" in result_slugs + + @with_feature("organizations:legacy-webhook-disable-old-path") + def test_get_plugins_skips_webhooks_when_old_path_disabled(self) -> None: + ProjectOption.objects.set_value(self.project, "webhooks:urls", "http://example.com/hook") + webhook_plugin = plugins.get("webhooks") + webhook_plugin.set_option("enabled", True, self.project) + + rule = self.get_rule() + result_slugs = [p.service.slug for p in rule.get_plugins()] + + assert "webhooks" not in result_slugs