Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -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):
Expand Down Expand Up @@ -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
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GroupEvent gate blocks Activity events from old path

High Severity

The early return for non-GroupEvent events prevents Activity events from reaching execute_via_group_type_registry, which explicitly handles Activity events (e.g., calling send_notification() for resolution notifications). Before this change, the old path ran unconditionally. The analogous WebhookActionHandler.execute() correctly places the GroupEvent check only on the new path, not the old one. This silently drops plugin notifications for Activity-based events like issue resolutions.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit b841095. Configure here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

plugins don't support activity alerts since there's no metric handler


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},
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I'd add the same error handling for your new path if you want to compare logging, but if you don't expect errors it might be fine

)
Comment thread
Christinarlong marked this conversation as resolved.

if new_path:
try:
send_legacy_webhooks_for_invocation(invocation)
except Exception:
logger.exception(
"plugin_action_handler.new_path_error",
extra={"invocation": invocation},
)
7 changes: 7 additions & 0 deletions src/sentry/rules/actions/notify_event.py
Original file line number Diff line number Diff line change
@@ -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
Expand All @@ -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
Comment on lines +31 to +32
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can't cut off at the action layer as we still need to alert for the following notificationplugins (that aren't slack, pagerduty, webhooks, and opsgenie)

Image

results.append(LegacyPluginService(plugin))

return results
Expand Down
Original file line number Diff line number Diff line change
@@ -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)
24 changes: 24 additions & 0 deletions tests/sentry/rules/actions/test_notify_event.py
Original file line number Diff line number Diff line change
@@ -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]
Expand All @@ -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
Loading