diff --git a/src/sentry/workflow_engine/models/action.py b/src/sentry/workflow_engine/models/action.py index 89637d4fa5e071..2cf4f08a3ada76 100644 --- a/src/sentry/workflow_engine/models/action.py +++ b/src/sentry/workflow_engine/models/action.py @@ -4,7 +4,7 @@ import logging from dataclasses import asdict from enum import StrEnum -from typing import TYPE_CHECKING +from typing import TYPE_CHECKING, ClassVar from django.db import models from django.db.models.signals import pre_save @@ -16,6 +16,8 @@ from sentry.db.models import DefaultFieldsModel, region_silo_model, sane_repr from sentry.db.models.fields.bounded import BoundedPositiveIntegerField from sentry.db.models.fields.hybrid_cloud_foreign_key import HybridCloudForeignKey +from sentry.db.models.manager.base import BaseManager +from sentry.db.models.manager.base_query_set import BaseQuerySet from sentry.utils import metrics from sentry.workflow_engine.models.json_config import JSONConfigBase from sentry.workflow_engine.registry import action_handler_registry @@ -28,6 +30,15 @@ logger = logging.getLogger(__name__) +class ActionManager(BaseManager["Action"]): + def get_queryset(self) -> BaseQuerySet[Action]: + return ( + super() + .get_queryset() + .exclude(status__in=(ObjectStatus.PENDING_DELETION, ObjectStatus.DELETION_IN_PROGRESS)) + ) + + @region_silo_model class Action(DefaultFieldsModel, JSONConfigBase): """ @@ -41,6 +52,9 @@ class Action(DefaultFieldsModel, JSONConfigBase): __relocation_scope__ = RelocationScope.Excluded __repr__ = sane_repr("id", "type") + objects: ClassVar[ActionManager] = ActionManager() + objects_for_deletion: ClassVar[BaseManager] = BaseManager() + class Type(StrEnum): SLACK = "slack" MSTEAMS = "msteams" diff --git a/src/sentry/workflow_engine/processors/action.py b/src/sentry/workflow_engine/processors/action.py index fdbba8f73be7bc..41da1081289444 100644 --- a/src/sentry/workflow_engine/processors/action.py +++ b/src/sentry/workflow_engine/processors/action.py @@ -123,16 +123,19 @@ def update_workflow_action_group_statuses( ) -def deduplicate_actions( +def get_unique_active_actions( actions_queryset: BaseQuerySet[Action], # decorated with the workflow_ids ) -> BaseQuerySet[Action]: """ - Deduplicates actions based on their handler's dedup_key method. - Returns a de-duplicated queryset of actions. + Returns a queryset of unique active actions based on their handler's dedup_key method. """ dedup_key_to_action_id: dict[str, int] = {} for action in actions_queryset: + # We only want to fire active actions + if action.status != ObjectStatus.ACTIVE: + continue + # workflow_id is annotated in the queryset workflow_id = getattr(action, "workflow_id") dedup_key = action.get_dedup_key(workflow_id) @@ -144,7 +147,8 @@ def deduplicate_actions( def fire_actions( actions: BaseQuerySet[Action], detector: Detector, event_data: WorkflowEventData ) -> None: - deduped_actions = deduplicate_actions(actions) + deduped_actions = get_unique_active_actions(actions) + for action in deduped_actions: task_params = build_trigger_action_task_params(action, detector, event_data) trigger_action.apply_async(kwargs=task_params, headers={"sentry-propagate-traces": False}) diff --git a/src/sentry/workflow_engine/processors/workflow.py b/src/sentry/workflow_engine/processors/workflow.py index 955bae743fd358..e80b5ed4aaa12f 100644 --- a/src/sentry/workflow_engine/processors/workflow.py +++ b/src/sentry/workflow_engine/processors/workflow.py @@ -465,6 +465,7 @@ def process_workflows( create_workflow_fire_histories( detector, actions, event_data, should_trigger_actions, is_delayed=False ) + fire_actions(actions, detector, event_data) return triggered_workflows diff --git a/tests/sentry/workflow_engine/processors/test_action_deduplication.py b/tests/sentry/workflow_engine/processors/test_action_deduplication.py index 4b89110fa0457f..3ca88236711e16 100644 --- a/tests/sentry/workflow_engine/processors/test_action_deduplication.py +++ b/tests/sentry/workflow_engine/processors/test_action_deduplication.py @@ -1,11 +1,12 @@ from django.db import models from django.db.models import Value +from sentry.constants import ObjectStatus from sentry.notifications.models.notificationaction import ActionTarget from sentry.testutils.cases import TestCase from sentry.testutils.silo import region_silo_test from sentry.workflow_engine.models import Action -from sentry.workflow_engine.processors.action import deduplicate_actions +from sentry.workflow_engine.processors.action import get_unique_active_actions from sentry.workflow_engine.typings.notification_action import SentryAppIdentifier @@ -68,7 +69,7 @@ def test_deduplicate_actions_different_types(self) -> None: id__in=[self.slack_action.id, email_action.id] ).annotate(workflow_id=Value(1, output_field=models.IntegerField())) - result = deduplicate_actions(actions_queryset) + result = get_unique_active_actions(actions_queryset) # Both actions should remain since they're different types result_ids = list(result.values_list("id", flat=True)) @@ -76,6 +77,29 @@ def test_deduplicate_actions_different_types(self) -> None: assert self.slack_action.id in result_ids assert email_action.id in result_ids + def test_deduplicate_actions_inactive_actions(self) -> None: + """Test that inactive actions are not deduplicated.""" + email_action = self.create_action( + type=Action.Type.EMAIL, + config={ + "target_type": ActionTarget.SPECIFIC, + "target_identifier": "test@example.com", + }, + status=ObjectStatus.DISABLED, + ) + + actions_queryset = Action.objects.filter( + id__in=[self.slack_action.id, email_action.id] + ).annotate(workflow_id=Value(1, output_field=models.IntegerField())) + + result = get_unique_active_actions(actions_queryset) + + # Only one action should remain + # The inactive action should be filtered out + result_ids = list(result.values_list("id", flat=True)) + assert len(result_ids) == 1 + assert self.slack_action.id in result_ids + def test_deduplicate_actions_same_slack_channels(self) -> None: """Test that Slack actions to the same channel are deduplicated.""" slack_action_1 = self.slack_action @@ -93,7 +117,7 @@ def test_deduplicate_actions_same_slack_channels(self) -> None: id__in=[slack_action_1.id, slack_action_2.id] ).annotate(workflow_id=Value(1, output_field=models.IntegerField())) - result = deduplicate_actions(actions_queryset) + result = get_unique_active_actions(actions_queryset) # Only one action should remain result_ids = list(result.values_list("id", flat=True)) @@ -122,7 +146,7 @@ def test_deduplicate_actions_different_slack_channels(self) -> None: id__in=[slack_action_1.id, slack_action_2.id] ).annotate(workflow_id=Value(1, output_field=models.IntegerField())) - result = deduplicate_actions(actions_queryset) + result = get_unique_active_actions(actions_queryset) # Both actions should remain since they target different channels result_ids = list(result.values_list("id", flat=True)) @@ -148,7 +172,7 @@ def test_deduplicate_multiple_slack_actions_same_channel_different_name(self) -> id__in=[slack_action_1.id, slack_action_2.id] ).annotate(workflow_id=Value(1, output_field=models.IntegerField())) - result = deduplicate_actions(actions_queryset) + result = get_unique_active_actions(actions_queryset) # Only one action should remain result_ids = list(result.values_list("id", flat=True)) @@ -182,7 +206,7 @@ def test_deduplicate_actions_same_slack_different_data(self) -> None: id__in=[slack_action_1.id, slack_action_2.id] ).annotate(workflow_id=Value(1, output_field=models.IntegerField())) - result = deduplicate_actions(actions_queryset) + result = get_unique_active_actions(actions_queryset) # Both actions should remain since they have different data result_ids = list(result.values_list("id", flat=True)) @@ -219,7 +243,7 @@ def test_deduplicate_actions_different_slack_integrations(self) -> None: id__in=[slack_action_1.id, slack_action_2.id] ).annotate(workflow_id=Value(1, output_field=models.IntegerField())) - result = deduplicate_actions(actions_queryset) + result = get_unique_active_actions(actions_queryset) # Both actions should remain since they have different data result_ids = list(result.values_list("id", flat=True)) @@ -250,7 +274,7 @@ def test_deduplicate_actions_email_same_target(self) -> None: id__in=[email_action_1.id, email_action_2.id] ).annotate(workflow_id=Value(1, output_field=models.IntegerField())) - result = deduplicate_actions(actions_queryset) + result = get_unique_active_actions(actions_queryset) # Only one action should remain result_ids = list(result.values_list("id", flat=True)) @@ -278,7 +302,7 @@ def test_deduplicate_actions_email_different_target_identifier(self) -> None: id__in=[email_action_1.id, email_action_2.id] ).annotate(workflow_id=Value(1, output_field=models.IntegerField())) - result = deduplicate_actions(actions_queryset) + result = get_unique_active_actions(actions_queryset) # Both actions should remain since they have different targets result_ids = list(result.values_list("id", flat=True)) @@ -308,7 +332,7 @@ def test_deduplicate_actions_email_different_target_type(self) -> None: id__in=[email_action_1.id, email_action_2.id] ).annotate(workflow_id=Value(1, output_field=models.IntegerField())) - result = deduplicate_actions(actions_queryset) + result = get_unique_active_actions(actions_queryset) # Both actions should remain since they have different targets result_ids = list(result.values_list("id", flat=True)) @@ -344,7 +368,7 @@ def test_deduplicate_actions_email_different_fallthrough_type(self) -> None: id__in=[email_action_1.id, email_action_2.id] ).annotate(workflow_id=Value(1, output_field=models.IntegerField())) - result = deduplicate_actions(actions_queryset) + result = get_unique_active_actions(actions_queryset) # Both actions should remain since they have different targets result_ids = list(result.values_list("id", flat=True)) @@ -379,7 +403,7 @@ def test_deduplicate_actions_email_everything_is_same(self) -> None: id__in=[email_action_1.id, email_action_2.id] ).annotate(workflow_id=Value(1, output_field=models.IntegerField())) - result = deduplicate_actions(actions_queryset) + result = get_unique_active_actions(actions_queryset) # Only one action should remain result_ids = list(result.values_list("id", flat=True)) @@ -410,7 +434,7 @@ def test_deduplicate_actions_sentry_app_same_identifier(self) -> None: id__in=[sentry_app_action_1.id, sentry_app_action_2.id] ).annotate(workflow_id=Value(1, output_field=models.IntegerField())) - result = deduplicate_actions(actions_queryset) + result = get_unique_active_actions(actions_queryset) # Only one action should remain result_ids = list(result.values_list("id", flat=True)) @@ -437,7 +461,7 @@ def test_deduplicate_actions_webhook_same_target_identifier(self) -> None: id__in=[webhook_action_1.id, webhook_action_2.id] ).annotate(workflow_id=Value(1, output_field=models.IntegerField())) - result = deduplicate_actions(actions_queryset) + result = get_unique_active_actions(actions_queryset) # Only one action should remain result_ids = list(result.values_list("id", flat=True)) @@ -452,7 +476,7 @@ def test_deduplicate_actions_plugin_actions(self) -> None: id__in=[plugin_action_1.id, plugin_action_2.id] ).annotate(workflow_id=Value(1, output_field=models.IntegerField())) - result = deduplicate_actions(actions_queryset) + result = get_unique_active_actions(actions_queryset) # One action should remain since its a plugin action result_ids = list(result.values_list("id", flat=True)) @@ -484,7 +508,7 @@ def test_deduplicate_actions_mixed_types_integration_bucket(self) -> None: id__in=[slack_action.id, pagerduty_action.id] ).annotate(workflow_id=Value(1, output_field=models.IntegerField())) - result = deduplicate_actions(actions_queryset) + result = get_unique_active_actions(actions_queryset) # Both actions should remain since they're for different integrations result_ids = list(result.values_list("id", flat=True)) @@ -521,7 +545,7 @@ def test_deduplicate_actions_ticketing_actions(self) -> None: id__in=[jira_action_1.id, jira_action_2.id] ).annotate(workflow_id=Value(1, output_field=models.IntegerField())) - result = deduplicate_actions(actions_queryset) + result = get_unique_active_actions(actions_queryset) # Both actions should remain since ticketing actions are deduplicated by integration_id and dynamic form field data result_ids = list(result.values_list("id", flat=True)) @@ -557,7 +581,7 @@ def test_deduplicate_actions_ticketing_actions_same_integration_and_data(self) - id__in=[jira_action_1.id, jira_action_2.id] ).annotate(workflow_id=Value(1, output_field=models.IntegerField())) - result = deduplicate_actions(actions_queryset) + result = get_unique_active_actions(actions_queryset) # Only 1 action should remain result_ids = list(result.values_list("id", flat=True)) @@ -567,7 +591,7 @@ def test_deduplicate_actions_empty_queryset(self) -> None: """Test deduplication with empty queryset.""" actions_queryset = Action.objects.none() - result = deduplicate_actions(actions_queryset) + result = get_unique_active_actions(actions_queryset) # Should return empty queryset assert list(result) == [] @@ -580,7 +604,7 @@ def test_deduplicate_actions_single_action(self) -> None: workflow_id=Value(1, output_field=models.IntegerField()) ) - result = deduplicate_actions(actions_queryset) + result = get_unique_active_actions(actions_queryset) # Should return the single action result_ids = list(result.values_list("id", flat=True)) @@ -621,7 +645,7 @@ def test_deduplicate_actions_same_actions_different_workflows(self) -> None: ) ) - result = deduplicate_actions(actions_queryset) + result = get_unique_active_actions(actions_queryset) # Both actions should remain since they're from different workflows result_ids = list(result.values_list("id", flat=True))