From 15e276bf14400fc2b1417aababbce70ece1f674b Mon Sep 17 00:00:00 2001 From: Colleen O'Rourke Date: Thu, 20 Nov 2025 15:25:11 -0800 Subject: [PATCH 01/25] fix(ACI): Fix adding sentry app action to a workflow --- .../sentry_app_handler.py | 1 + .../test_organization_workflow_details.py | 106 +++++++++++- .../test_organization_workflow_index.py | 154 +++++++++++++++++- 3 files changed, 257 insertions(+), 4 deletions(-) diff --git a/src/sentry/notifications/notification_action/action_handler_registry/sentry_app_handler.py b/src/sentry/notifications/notification_action/action_handler_registry/sentry_app_handler.py index 4adbed04bb43f6..b3f1deb4e908ed 100644 --- a/src/sentry/notifications/notification_action/action_handler_registry/sentry_app_handler.py +++ b/src/sentry/notifications/notification_action/action_handler_registry/sentry_app_handler.py @@ -33,6 +33,7 @@ class SentryAppActionHandler(ActionHandler): data_schema = { "$schema": "https://json-schema.org/draft/2020-12/schema", + "description": "The data schema for a Sentry App Action", "type": "object", "properties": { "settings": {"type": ["array", "object"]}, diff --git a/tests/sentry/workflow_engine/endpoints/test_organization_workflow_details.py b/tests/sentry/workflow_engine/endpoints/test_organization_workflow_details.py index af001f4af86742..43b419b9d76b47 100644 --- a/tests/sentry/workflow_engine/endpoints/test_organization_workflow_details.py +++ b/tests/sentry/workflow_engine/endpoints/test_organization_workflow_details.py @@ -13,7 +13,14 @@ from sentry.testutils.outbox import outbox_runner from sentry.testutils.silo import assume_test_silo_mode, region_silo_test from sentry.workflow_engine.endpoints.validators.base.workflow import WorkflowValidator -from sentry.workflow_engine.models import Action, DataConditionGroup, Workflow +from sentry.workflow_engine.models import ( + Action, + Condition, + DataConditionGroup, + DataConditionGroupAction, + Workflow, + WorkflowDataConditionGroup, +) from sentry.workflow_engine.models.detector_workflow import DetectorWorkflow from tests.sentry.workflow_engine.test_base import BaseWorkflowTest @@ -54,7 +61,7 @@ def setUp(self) -> None: "enabled": True, "config": {}, "triggers": {"logicType": "any", "conditions": []}, - "action_filters": [], + "actionFilters": [], } validator = WorkflowValidator( data=self.valid_workflow, @@ -73,6 +80,101 @@ def test_simple(self) -> None: assert response.status_code == 200 assert updated_workflow.name == "Updated Workflow" + def test_update_add_email_action(self) -> None: + """ + Test that adding a simple email user action to a workflow works as expected + """ + self.valid_workflow["actionFilters"] = [ + { + "logicType": "any", + "conditions": [ + { + "type": Condition.EQUAL.value, + "comparison": 1, + "conditionResult": True, + } + ], + "actions": [ + { + "type": Action.Type.EMAIL, + "config": { + "targetIdentifier": str(self.user.id), + "targetType": "user", + }, + "data": {}, + }, + ], + } + ] + response = self.get_success_response( + self.organization.slug, self.workflow.id, raw_data=self.valid_workflow + ) + updated_workflow = Workflow.objects.get(id=response.data.get("id")) + action_filter = WorkflowDataConditionGroup.objects.get(workflow=updated_workflow) + dcga = DataConditionGroupAction.objects.get(condition_group=action_filter.condition_group) + action = dcga.action + + assert response.status_code == 200 + assert action.type == Action.Type.EMAIL + assert action.data == {} + assert action.config == {"target_type": "user", "target_identifier": str(self.user.id)} + + def test_update_add_sentry_app_action(self) -> None: + """ + Test that adding a sentry app action to a workflow works as expected + """ + self.sentry_app_settings_schema = self.create_alert_rule_action_schema() + self.sentry_app = self.create_sentry_app( + name="Moo Deng's Fire Sentry App", + organization=self.organization, + schema={ + "elements": [ + self.sentry_app_settings_schema, + ] + }, + is_alertable=True, + ) + self.sentry_app_installation = self.create_sentry_app_installation( + slug=self.sentry_app.slug, organization=self.organization + ) + self.valid_workflow["actionFilters"] = [ + { + "logicType": "any", + "conditions": [ + { + "type": Condition.EQUAL.value, + "comparison": 1, + "conditionResult": True, + } + ], + "actions": [ + { + "type": Action.Type.SENTRY_APP, + "config": { + "sentryAppIdentifier": "sentry_app_id", + "targetIdentifier": str(self.sentry_app.id), + "targetType": "sentry_app", + }, + "data": {}, + "integrationId": None, + "status": "active", + }, + ], + } + ] + response = self.get_success_response( + self.organization.slug, self.workflow.id, raw_data=self.valid_workflow + ) + updated_workflow = Workflow.objects.get(id=response.data.get("id")) + action_filter = WorkflowDataConditionGroup.objects.get(workflow=updated_workflow) + dcga = DataConditionGroupAction.objects.get(condition_group=action_filter.condition_group) + action = dcga.action + + assert response.status_code == 200 + assert action.type == Action.Type.SENTRY_APP + assert action.data == {} + assert action.config == {"target_type": "user", "target_identifier": str(self.user.id)} + def test_update_triggers_with_empty_conditions(self) -> None: """Test that passing an empty list to triggers.conditions clears all conditions""" # Create a workflow with a trigger condition diff --git a/tests/sentry/workflow_engine/endpoints/test_organization_workflow_index.py b/tests/sentry/workflow_engine/endpoints/test_organization_workflow_index.py index 6a93eee22b7724..3a201b07403f17 100644 --- a/tests/sentry/workflow_engine/endpoints/test_organization_workflow_index.py +++ b/tests/sentry/workflow_engine/endpoints/test_organization_workflow_index.py @@ -1,6 +1,8 @@ + from collections.abc import Sequence from typing import Any from unittest import mock +import responses from sentry import audit_log from sentry.api.serializers import serialize @@ -24,7 +26,7 @@ WorkflowFireHistory, ) from sentry.workflow_engine.models.data_condition import Condition -from tests.sentry.workflow_engine.test_base import MockActionValidatorTranslator +from tests.sentry.workflow_engine.test_base import BaseWorkflowTest, MockActionValidatorTranslator class OrganizationWorkflowAPITestCase(APITestCase): @@ -375,7 +377,7 @@ def test_compound_query(self) -> None: @region_silo_test -class OrganizationWorkflowCreateTest(OrganizationWorkflowAPITestCase): +class OrganizationWorkflowCreateTest(OrganizationWorkflowAPITestCase, BaseWorkflowTest): method = "POST" def setUp(self) -> None: @@ -404,6 +406,14 @@ def setUp(self) -> None: "conditionResult": True, } ] + self.sentry_app, _ = self.create_sentry_app_with_schema() + self.sentry_app_settings = [ + {"name": "alert_prefix", "value": "[Not Good]"}, + {"name": "channel", "value": "#ignored-errors"}, + {"name": "best_emoji", "value": ":fire:"}, + {"name": "teamId", "value": 1}, + {"name": "assigneeId", "value": 3}, + ] @mock.patch("sentry.workflow_engine.endpoints.validators.base.workflow.create_audit_entry") def test_create_workflow__basic(self, mock_audit: mock.MagicMock) -> None: @@ -539,6 +549,146 @@ def test_create_workflow__with_fallthrough_type_action( assert dcga.action.type == Action.Type.EMAIL assert dcga.action.data == {"fallthrough_type": "ActiveMembers"} + @responses.activate + def test_create_workflow_with_sentry_app_action(self) -> None: + """ + Test that you can add a sentry app with settings + (e.g. a sentry app that makes a ticket in some 3rd party system as opposed to one without settings) + """ + responses.add( + method=responses.POST, + url="https://example.com/sentry/alert-rule", + status=200, + ) + self.valid_workflow["actionFilters"] = [ + { + "logicType": "any", + "conditions": self.basic_condition, + "actions": [ + { + "config": { + "sentryAppIdentifier": SentryAppIdentifier.SENTRY_APP_ID, + "targetIdentifier": str(self.sentry_app.id), + "targetType": ActionType.SENTRY_APP, + }, + "data": {"settings": self.sentry_app_settings}, + "type": Action.Type.SENTRY_APP, + }, + ], + } + ] + + response = self.get_success_response( + self.organization.slug, + raw_data=self.valid_workflow, + ) + updated_workflow = Workflow.objects.get(id=response.data["id"]) + new_action_filters = WorkflowDataConditionGroup.objects.filter(workflow=updated_workflow) + dcga = DataConditionGroupAction.objects.filter( + condition_group=new_action_filters[0].condition_group + ) + action = dcga[0].action + + assert action.type == Action.Type.SENTRY_APP + assert action.config == { + "sentry_app_identifier": SentryAppIdentifier.SENTRY_APP_ID, + "target_identifier": str(self.sentry_app.id), + "target_type": ActionTarget.SENTRY_APP.value, + } + assert action.data["settings"] == self.sentry_app_settings + + @responses.activate + def test_create_sentry_app_action_missing_settings(self) -> None: + """ + Test that if you forget to pass settings to your sentry app action it will fail and tell you why. + Settings are only required if the sentry app schema is not an empty dict + """ + responses.add( + method=responses.POST, + url="https://example.com/sentry/alert-rule", + status=200, + ) + + self.valid_workflow["actionFilters"] = [ + { + "logicType": "any", + "conditions": self.basic_condition, + "actions": [ + { + "config": { + "sentryAppIdentifier": SentryAppIdentifier.SENTRY_APP_ID, + "targetIdentifier": str(self.sentry_app.id), + "targetType": ActionType.SENTRY_APP, + }, + "data": {}, + "type": Action.Type.SENTRY_APP, + }, + ], + } + ] + response = self.get_response( + self.organization.slug, + raw_data=self.valid_workflow, + ) + + assert response.status_code == 400 + assert "'settings' is a required property" in str(response.data).lower() + + @responses.activate + def test_create_sentry_app_action_no_settings(self) -> None: + """ + Test that if you are creating a sentry app action for a sentry app that has no schema it works as expected when settings are not passed + because settings are not expected + """ + sentry_app = self.create_sentry_app( + name="Moo Deng's Wind Sentry App", + organization=self.organization, + is_alertable=True, + ) + self.create_sentry_app_installation(slug=sentry_app.slug, organization=self.organization) + + responses.add( + method=responses.POST, + url="https://example.com/sentry/alert-rule", + status=200, + ) + + self.valid_workflow["actionFilters"] = [ + { + "logicType": "any", + "conditions": self.basic_condition, + "actions": [ + { + "config": { + "sentryAppIdentifier": SentryAppIdentifier.SENTRY_APP_ID, + "targetIdentifier": str(sentry_app.id), + "targetType": ActionType.SENTRY_APP, + }, + "data": {}, + "type": Action.Type.SENTRY_APP, + }, + ], + } + ] + response = self.get_success_response( + self.organization.slug, + raw_data=self.valid_workflow, + ) + updated_workflow = Workflow.objects.get(id=response.data["id"]) + new_action_filters = WorkflowDataConditionGroup.objects.filter(workflow=updated_workflow) + dcga = DataConditionGroupAction.objects.filter( + condition_group=new_action_filters[0].condition_group + ) + action = dcga[0].action + + assert action.type == Action.Type.SENTRY_APP + assert action.config == { + "sentry_app_identifier": SentryAppIdentifier.SENTRY_APP_ID.value, + "target_identifier": str(sentry_app.id), + "target_type": ActionTarget.SENTRY_APP.value, + } + assert action.data == {} + def test_create_invalid_workflow(self) -> None: self.valid_workflow["name"] = "" response = self.get_response( From 94a664b961720171515f0fe4d775b625023d7c92 Mon Sep 17 00:00:00 2001 From: Colleen O'Rourke Date: Thu, 20 Nov 2025 16:47:53 -0800 Subject: [PATCH 02/25] require settings, add tests --- .../sentry_app_handler.py | 1 + .../test_organization_workflow_details.py | 53 +++++++++++-------- tests/sentry/workflow_engine/test_base.py | 17 ++++++ 3 files changed, 49 insertions(+), 22 deletions(-) diff --git a/src/sentry/notifications/notification_action/action_handler_registry/sentry_app_handler.py b/src/sentry/notifications/notification_action/action_handler_registry/sentry_app_handler.py index b3f1deb4e908ed..7594a368bbbe53 100644 --- a/src/sentry/notifications/notification_action/action_handler_registry/sentry_app_handler.py +++ b/src/sentry/notifications/notification_action/action_handler_registry/sentry_app_handler.py @@ -38,6 +38,7 @@ class SentryAppActionHandler(ActionHandler): "properties": { "settings": {"type": ["array", "object"]}, }, + "required": ["settings"], "additionalProperties": False, } diff --git a/tests/sentry/workflow_engine/endpoints/test_organization_workflow_details.py b/tests/sentry/workflow_engine/endpoints/test_organization_workflow_details.py index 43b419b9d76b47..e85c1126ddc210 100644 --- a/tests/sentry/workflow_engine/endpoints/test_organization_workflow_details.py +++ b/tests/sentry/workflow_engine/endpoints/test_organization_workflow_details.py @@ -1,5 +1,7 @@ from contextlib import AbstractContextManager +import responses + from sentry import audit_log from sentry.api.serializers import serialize from sentry.constants import ObjectStatus @@ -22,6 +24,11 @@ WorkflowDataConditionGroup, ) from sentry.workflow_engine.models.detector_workflow import DetectorWorkflow +from sentry.workflow_engine.typings.notification_action import ( + ActionTarget, + ActionType, + SentryAppIdentifier, +) from tests.sentry.workflow_engine.test_base import BaseWorkflowTest @@ -119,24 +126,24 @@ def test_update_add_email_action(self) -> None: assert action.data == {} assert action.config == {"target_type": "user", "target_identifier": str(self.user.id)} + @responses.activate def test_update_add_sentry_app_action(self) -> None: """ Test that adding a sentry app action to a workflow works as expected """ - self.sentry_app_settings_schema = self.create_alert_rule_action_schema() - self.sentry_app = self.create_sentry_app( - name="Moo Deng's Fire Sentry App", - organization=self.organization, - schema={ - "elements": [ - self.sentry_app_settings_schema, - ] - }, - is_alertable=True, - ) - self.sentry_app_installation = self.create_sentry_app_installation( - slug=self.sentry_app.slug, organization=self.organization + responses.add( + method=responses.POST, + url="https://example.com/sentry/alert-rule", + status=200, ) + self.sentry_app_installation = self.create_sentry_app_with_installation() + self.sentry_app_settings = [ + {"name": "alert_prefix", "value": "[Not Good]"}, + {"name": "channel", "value": "#ignored-errors"}, + {"name": "best_emoji", "value": ":fire:"}, + {"name": "teamId", "value": 1}, + {"name": "assigneeId", "value": 3}, + ] self.valid_workflow["actionFilters"] = [ { "logicType": "any", @@ -149,15 +156,13 @@ def test_update_add_sentry_app_action(self) -> None: ], "actions": [ { - "type": Action.Type.SENTRY_APP, "config": { - "sentryAppIdentifier": "sentry_app_id", - "targetIdentifier": str(self.sentry_app.id), - "targetType": "sentry_app", + "sentryAppIdentifier": SentryAppIdentifier.SENTRY_APP_INSTALLATION_UUID, + "targetIdentifier": self.sentry_app_installation.uuid, + "targetType": ActionType.SENTRY_APP, }, - "data": {}, - "integrationId": None, - "status": "active", + "data": {"settings": self.sentry_app_settings}, + "type": Action.Type.SENTRY_APP, }, ], } @@ -172,8 +177,12 @@ def test_update_add_sentry_app_action(self) -> None: assert response.status_code == 200 assert action.type == Action.Type.SENTRY_APP - assert action.data == {} - assert action.config == {"target_type": "user", "target_identifier": str(self.user.id)} + assert action.config == { + "sentry_app_identifier": SentryAppIdentifier.SENTRY_APP_INSTALLATION_UUID, + "target_identifier": self.sentry_app_installation.uuid, + "target_type": ActionTarget.SENTRY_APP.value, + } + assert action.data["settings"] == self.sentry_app_settings def test_update_triggers_with_empty_conditions(self) -> None: """Test that passing an empty list to triggers.conditions clears all conditions""" diff --git a/tests/sentry/workflow_engine/test_base.py b/tests/sentry/workflow_engine/test_base.py index 917329517bba0a..48c3ae48f68da7 100644 --- a/tests/sentry/workflow_engine/test_base.py +++ b/tests/sentry/workflow_engine/test_base.py @@ -364,3 +364,20 @@ def create_group_event( ) return group, event, group_event + + def create_sentry_app_with_installation(self): + sentry_app_settings_schema = self.create_alert_rule_action_schema() + sentry_app = self.create_sentry_app( + name="Moo Deng's Fire Sentry App", + organization=self.organization, + schema={ + "elements": [ + sentry_app_settings_schema, + ] + }, + is_alertable=True, + ) + sentry_app_installation = self.create_sentry_app_installation( + slug=sentry_app.slug, organization=self.organization + ) + return sentry_app_installation From 1eb048adaeb4b64f76883046ab4427571ee895cf Mon Sep 17 00:00:00 2001 From: Colleen O'Rourke Date: Thu, 20 Nov 2025 17:09:29 -0800 Subject: [PATCH 03/25] Add settings to test --- tests/sentry/deletions/test_sentry_app.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/tests/sentry/deletions/test_sentry_app.py b/tests/sentry/deletions/test_sentry_app.py index 793a18cd3b8a32..6edc64dc2a1511 100644 --- a/tests/sentry/deletions/test_sentry_app.py +++ b/tests/sentry/deletions/test_sentry_app.py @@ -67,6 +67,11 @@ def test_disables_actions(self) -> None: "sentry_app_identifier": SentryAppIdentifier.SENTRY_APP_ID, "target_type": ActionTarget.SENTRY_APP, }, + data={ + "settings": [ + {"name": "best_emoji", "value": ":fire:"}, + ] + }, ) webhook_action = self.create_action( type=Action.Type.WEBHOOK, @@ -81,6 +86,11 @@ def test_disables_actions(self) -> None: "sentry_app_identifier": SentryAppIdentifier.SENTRY_APP_ID, "target_type": ActionTarget.SENTRY_APP, }, + data={ + "settings": [ + {"name": "best_emoji", "value": ":fire:"}, + ] + }, ) deletions.exec_sync(self.sentry_app) From 3e92db0a9942f900c4684d2977eed78e96e75f72 Mon Sep 17 00:00:00 2001 From: Colleen O'Rourke Date: Thu, 20 Nov 2025 17:14:20 -0800 Subject: [PATCH 04/25] update more tests --- .../service/test_action_service.py | 30 +++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/tests/sentry/workflow_engine/service/test_action_service.py b/tests/sentry/workflow_engine/service/test_action_service.py index 0ebe7c8913e318..e5b5e0bccc5871 100644 --- a/tests/sentry/workflow_engine/service/test_action_service.py +++ b/tests/sentry/workflow_engine/service/test_action_service.py @@ -143,6 +143,11 @@ def test_delete_actions_for_organization_integration_mixed_types(self) -> None: "sentry_app_identifier": SentryAppIdentifier.SENTRY_APP_ID, "target_type": ActionTarget.SENTRY_APP, }, + data={ + "settings": [ + {"name": "best_emoji", "value": ":fire:"}, + ] + }, ) self.create_data_condition_group_action( @@ -180,6 +185,11 @@ def test_disable_actions_for_organization_integration_mixed_types(self) -> None: "sentry_app_identifier": SentryAppIdentifier.SENTRY_APP_ID, "target_type": ActionTarget.SENTRY_APP, }, + data={ + "settings": [ + {"name": "best_emoji", "value": ":fire:"}, + ] + }, ) self.create_data_condition_group_action( @@ -225,6 +235,11 @@ def test_enable_actions_for_organization_integration_mixed_types(self) -> None: "sentry_app_identifier": SentryAppIdentifier.SENTRY_APP_ID, "target_type": ActionTarget.SENTRY_APP, }, + data={ + "settings": [ + {"name": "best_emoji", "value": ":fire:"}, + ] + }, status=ObjectStatus.DISABLED, ) @@ -262,6 +277,11 @@ def test_update_action_status_for_sentry_app__installation_uuid(self) -> None: "sentry_app_identifier": SentryAppIdentifier.SENTRY_APP_INSTALLATION_UUID, "target_type": ActionTarget.SENTRY_APP, }, + data={ + "settings": [ + {"name": "best_emoji", "value": ":fire:"}, + ] + }, ) action_service.update_action_status_for_sentry_app_via_uuid( @@ -286,6 +306,11 @@ def test_update_action_status_for_sentry_app__installation_uuid__region(self) -> "sentry_app_identifier": SentryAppIdentifier.SENTRY_APP_INSTALLATION_UUID, "target_type": ActionTarget.SENTRY_APP, }, + data={ + "settings": [ + {"name": "best_emoji", "value": ":fire:"}, + ] + }, ) action_service.update_action_status_for_sentry_app_via_uuid__region( region_name="us", @@ -304,6 +329,11 @@ def test_update_action_status_for_sentry_app__via_sentry_app_id(self) -> None: "sentry_app_identifier": SentryAppIdentifier.SENTRY_APP_ID, "target_type": ActionTarget.SENTRY_APP, }, + data={ + "settings": [ + {"name": "best_emoji", "value": ":fire:"}, + ] + }, ) action_service.update_action_status_for_sentry_app_via_sentry_app_id( region_name="us", From 5d7cae22c4887ad8e18f2cf2a49e227b59b60662 Mon Sep 17 00:00:00 2001 From: Colleen O'Rourke Date: Fri, 21 Nov 2025 13:18:16 -0800 Subject: [PATCH 05/25] fix test --- .../endpoints/test_organization_workflow_details.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/sentry/workflow_engine/endpoints/test_organization_workflow_details.py b/tests/sentry/workflow_engine/endpoints/test_organization_workflow_details.py index e85c1126ddc210..d553e1147a359e 100644 --- a/tests/sentry/workflow_engine/endpoints/test_organization_workflow_details.py +++ b/tests/sentry/workflow_engine/endpoints/test_organization_workflow_details.py @@ -124,7 +124,10 @@ def test_update_add_email_action(self) -> None: assert response.status_code == 200 assert action.type == Action.Type.EMAIL assert action.data == {} - assert action.config == {"target_type": "user", "target_identifier": str(self.user.id)} + assert action.config == { + "target_type": ActionTarget.USER.value, + "target_identifier": str(self.user.id), + } @responses.activate def test_update_add_sentry_app_action(self) -> None: From abd84b9af11a9b912dc7803d03c0edd71a87f466 Mon Sep 17 00:00:00 2001 From: Colleen O'Rourke Date: Fri, 21 Nov 2025 13:49:14 -0800 Subject: [PATCH 06/25] update more tests --- .../sentry/deletions/test_sentry_app_installations.py | 10 ++++++++++ .../test_sentry_app_metric_alert_handler.py | 9 +++++++-- .../api/endpoints/test_sentry_app_details.py | 5 +++++ .../migration_helpers/test_migrate_alert_rule.py | 5 +++++ .../processors/test_action_deduplication.py | 10 ++++++++++ 5 files changed, 37 insertions(+), 2 deletions(-) diff --git a/tests/sentry/deletions/test_sentry_app_installations.py b/tests/sentry/deletions/test_sentry_app_installations.py index a35c33c52aee87..fd27a816445b16 100644 --- a/tests/sentry/deletions/test_sentry_app_installations.py +++ b/tests/sentry/deletions/test_sentry_app_installations.py @@ -119,6 +119,11 @@ def test_disables_actions(self) -> None: "sentry_app_identifier": SentryAppIdentifier.SENTRY_APP_INSTALLATION_UUID, "target_type": ActionTarget.SENTRY_APP, }, + data={ + "settings": [ + {"name": "best_emoji", "value": ":fire:"}, + ] + }, ) other_action = self.create_action( type=Action.Type.SENTRY_APP, @@ -127,6 +132,11 @@ def test_disables_actions(self) -> None: "sentry_app_identifier": SentryAppIdentifier.SENTRY_APP_INSTALLATION_UUID, "target_type": ActionTarget.SENTRY_APP, }, + data={ + "settings": [ + {"name": "best_emoji", "value": ":fire:"}, + ] + }, ) deletions.exec_sync(self.install) with outbox_runner(): diff --git a/tests/sentry/notifications/notification_action/metric_alert_registry/test_sentry_app_metric_alert_handler.py b/tests/sentry/notifications/notification_action/metric_alert_registry/test_sentry_app_metric_alert_handler.py index 73f02b5aa5cfd7..b5f3073a3b495e 100644 --- a/tests/sentry/notifications/notification_action/metric_alert_registry/test_sentry_app_metric_alert_handler.py +++ b/tests/sentry/notifications/notification_action/metric_alert_registry/test_sentry_app_metric_alert_handler.py @@ -45,6 +45,11 @@ def setUp(self) -> None: "target_type": ActionTarget.SENTRY_APP.value, "sentry_app_identifier": SentryAppIdentifier.SENTRY_APP_ID, }, + data={ + "settings": [ + {"name": "best_emoji", "value": ":fire:"}, + ] + }, ) self.handler = SentryAppMetricAlertHandler() @@ -112,7 +117,7 @@ def test_invoke_legacy_registry(self, mock_send_alert: mock.MagicMock) -> None: integration_id=None, target_identifier=None, target_display=None, - sentry_app_config=None, + sentry_app_config=self.action.data.get("settings"), sentry_app_id=str(self.sentry_app.id), ) @@ -188,7 +193,7 @@ def test_invoke_legacy_registry_with_activity(self, mock_send_alert: mock.MagicM integration_id=None, target_identifier=None, target_display=None, - sentry_app_config=None, + sentry_app_config=self.action.data.get("settings"), sentry_app_id=str(self.sentry_app.id), ) diff --git a/tests/sentry/sentry_apps/api/endpoints/test_sentry_app_details.py b/tests/sentry/sentry_apps/api/endpoints/test_sentry_app_details.py index 4b034819926f2e..bb43a9e3f20257 100644 --- a/tests/sentry/sentry_apps/api/endpoints/test_sentry_app_details.py +++ b/tests/sentry/sentry_apps/api/endpoints/test_sentry_app_details.py @@ -858,6 +858,11 @@ def test_disables_actions(self) -> None: "sentry_app_identifier": SentryAppIdentifier.SENTRY_APP_ID, "target_type": ActionTarget.SENTRY_APP, }, + data={ + "settings": [ + {"name": "best_emoji", "value": ":fire:"}, + ] + }, ) webhook_action = self.create_action( type=Action.Type.WEBHOOK, diff --git a/tests/sentry/workflow_engine/migration_helpers/test_migrate_alert_rule.py b/tests/sentry/workflow_engine/migration_helpers/test_migrate_alert_rule.py index c2d581d617d138..bb5cd607d06a48 100644 --- a/tests/sentry/workflow_engine/migration_helpers/test_migrate_alert_rule.py +++ b/tests/sentry/workflow_engine/migration_helpers/test_migrate_alert_rule.py @@ -1310,6 +1310,11 @@ def test_dual_update_trigger_action_data_sentry_app_null_value(self) -> None: target_type=AlertRuleTriggerAction.TargetType.SENTRY_APP, sentry_app=sentry_app, alert_rule_trigger=self.alert_rule_trigger, + sentry_app_config=[ + {"name": "title", "value": "An alert"}, + {"name": "points", "value": "3"}, + {"name": "assignee", "value": "Denny"}, + ], ) action, _, _ = migrate_metric_action(sentry_app_trigger_action) update_alert_rule_trigger_action( diff --git a/tests/sentry/workflow_engine/processors/test_action_deduplication.py b/tests/sentry/workflow_engine/processors/test_action_deduplication.py index 6492c0bb186288..4133e1fa104693 100644 --- a/tests/sentry/workflow_engine/processors/test_action_deduplication.py +++ b/tests/sentry/workflow_engine/processors/test_action_deduplication.py @@ -395,6 +395,11 @@ def test_deduplicate_actions_sentry_app_same_identifier(self) -> None: "target_identifier": "action-123", "sentry_app_identifier": SentryAppIdentifier.SENTRY_APP_ID, }, + data={ + "settings": [ + {"name": "best_emoji", "value": ":fire:"}, + ] + }, ) sentry_app_action_2 = self.create_action( @@ -404,6 +409,11 @@ def test_deduplicate_actions_sentry_app_same_identifier(self) -> None: "target_identifier": "action-123", "sentry_app_identifier": SentryAppIdentifier.SENTRY_APP_ID, }, + data={ + "settings": [ + {"name": "best_emoji", "value": ":fire:"}, + ] + }, ) actions_queryset = Action.objects.filter( From ec96939e51c94d0c7d4c12e1b0b9922c9f3a74f7 Mon Sep 17 00:00:00 2001 From: Colleen O'Rourke Date: Fri, 21 Nov 2025 17:20:48 -0800 Subject: [PATCH 07/25] only check for settings if required, update tests --- .../sentry_app_handler.py | 1 - .../notification_action/action_validation.py | 24 ++++++++++--------- .../test_organization_workflow_details.py | 10 ++++---- tests/sentry/workflow_engine/test_base.py | 8 +++---- 4 files changed, 21 insertions(+), 22 deletions(-) diff --git a/src/sentry/notifications/notification_action/action_handler_registry/sentry_app_handler.py b/src/sentry/notifications/notification_action/action_handler_registry/sentry_app_handler.py index 7594a368bbbe53..b3f1deb4e908ed 100644 --- a/src/sentry/notifications/notification_action/action_handler_registry/sentry_app_handler.py +++ b/src/sentry/notifications/notification_action/action_handler_registry/sentry_app_handler.py @@ -38,7 +38,6 @@ class SentryAppActionHandler(ActionHandler): "properties": { "settings": {"type": ["array", "object"]}, }, - "required": ["settings"], "additionalProperties": False, } diff --git a/src/sentry/notifications/notification_action/action_validation.py b/src/sentry/notifications/notification_action/action_validation.py index 7aa99675cdb6d5..1e6a4dce132ad9 100644 --- a/src/sentry/notifications/notification_action/action_validation.py +++ b/src/sentry/notifications/notification_action/action_validation.py @@ -15,12 +15,11 @@ from sentry.rules.actions.integrations.create_ticket.form import IntegrationNotifyServiceForm from sentry.rules.actions.notify_event_service import NotifyEventServiceForm from sentry.rules.actions.sentry_apps.utils import validate_sentry_app_action -from sentry.sentry_apps.services.app.service import app_service +from sentry.sentry_apps.services.app import app_service from sentry.sentry_apps.utils.errors import SentryAppBaseError from sentry.utils import json from sentry.workflow_engine.models.action import Action from sentry.workflow_engine.processors.action import get_notification_plugins_for_org -from sentry.workflow_engine.typings.notification_action import SentryAppIdentifier from .types import BaseActionValidatorHandler @@ -207,18 +206,21 @@ def __init__(self, validated_data: dict[str, Any], organization: Organization) - self.organization = organization def clean_data(self) -> dict[str, Any]: - is_sentry_app_installation = ( - SentryAppIdentifier(self.validated_data["config"]["sentry_app_identifier"]) - == SentryAppIdentifier.SENTRY_APP_INSTALLATION_UUID + installation = app_service.get_installation_by_id( + id=int(self.validated_data["config"]["target_identifier"]) ) + settings = self.validated_data["data"].get("settings", []) action = { - "settings": self.validated_data["data"]["settings"], - "sentryAppInstallationUuid": ( - self.validated_data["config"]["target_identifier"] - if is_sentry_app_installation - else None - ), + "settings": settings, + "sentryAppInstallationUuid": installation.uuid, } + # XXX: it's only ok to not pass settings if there is no sentry app schema + # this means the app doesn't expect any settings + if not settings: + components = app_service.find_app_components(app_id=installation.sentry_app.id) + if any(component.app_schema for component in components): + raise ValidationError("'settings' is a required property") + try: validate_sentry_app_action(action) diff --git a/tests/sentry/workflow_engine/endpoints/test_organization_workflow_details.py b/tests/sentry/workflow_engine/endpoints/test_organization_workflow_details.py index d553e1147a359e..a5960dfc0a9bab 100644 --- a/tests/sentry/workflow_engine/endpoints/test_organization_workflow_details.py +++ b/tests/sentry/workflow_engine/endpoints/test_organization_workflow_details.py @@ -139,7 +139,7 @@ def test_update_add_sentry_app_action(self) -> None: url="https://example.com/sentry/alert-rule", status=200, ) - self.sentry_app_installation = self.create_sentry_app_with_installation() + self.sentry_app = self.create_sentry_app_with_schema() self.sentry_app_settings = [ {"name": "alert_prefix", "value": "[Not Good]"}, {"name": "channel", "value": "#ignored-errors"}, @@ -160,8 +160,8 @@ def test_update_add_sentry_app_action(self) -> None: "actions": [ { "config": { - "sentryAppIdentifier": SentryAppIdentifier.SENTRY_APP_INSTALLATION_UUID, - "targetIdentifier": self.sentry_app_installation.uuid, + "sentryAppIdentifier": SentryAppIdentifier.SENTRY_APP_ID, + "targetIdentifier": str(self.sentry_app.id), "targetType": ActionType.SENTRY_APP, }, "data": {"settings": self.sentry_app_settings}, @@ -181,8 +181,8 @@ def test_update_add_sentry_app_action(self) -> None: assert response.status_code == 200 assert action.type == Action.Type.SENTRY_APP assert action.config == { - "sentry_app_identifier": SentryAppIdentifier.SENTRY_APP_INSTALLATION_UUID, - "target_identifier": self.sentry_app_installation.uuid, + "sentry_app_identifier": SentryAppIdentifier.SENTRY_APP_ID, + "target_identifier": str(self.sentry_app.id), "target_type": ActionTarget.SENTRY_APP.value, } assert action.data["settings"] == self.sentry_app_settings diff --git a/tests/sentry/workflow_engine/test_base.py b/tests/sentry/workflow_engine/test_base.py index 48c3ae48f68da7..ad64f0c3a50134 100644 --- a/tests/sentry/workflow_engine/test_base.py +++ b/tests/sentry/workflow_engine/test_base.py @@ -365,7 +365,7 @@ def create_group_event( return group, event, group_event - def create_sentry_app_with_installation(self): + def create_sentry_app_with_schema(self): sentry_app_settings_schema = self.create_alert_rule_action_schema() sentry_app = self.create_sentry_app( name="Moo Deng's Fire Sentry App", @@ -377,7 +377,5 @@ def create_sentry_app_with_installation(self): }, is_alertable=True, ) - sentry_app_installation = self.create_sentry_app_installation( - slug=sentry_app.slug, organization=self.organization - ) - return sentry_app_installation + self.create_sentry_app_installation(slug=sentry_app.slug, organization=self.organization) + return sentry_app From afce6e37cfe36d6c76a88933f84ae2a32e816dd7 Mon Sep 17 00:00:00 2001 From: Colleen O'Rourke Date: Fri, 21 Nov 2025 17:30:37 -0800 Subject: [PATCH 08/25] clean up --- .../test_organization_workflow_details.py | 42 ------------------- 1 file changed, 42 deletions(-) diff --git a/tests/sentry/workflow_engine/endpoints/test_organization_workflow_details.py b/tests/sentry/workflow_engine/endpoints/test_organization_workflow_details.py index a5960dfc0a9bab..8b699b6d4bf628 100644 --- a/tests/sentry/workflow_engine/endpoints/test_organization_workflow_details.py +++ b/tests/sentry/workflow_engine/endpoints/test_organization_workflow_details.py @@ -87,48 +87,6 @@ def test_simple(self) -> None: assert response.status_code == 200 assert updated_workflow.name == "Updated Workflow" - def test_update_add_email_action(self) -> None: - """ - Test that adding a simple email user action to a workflow works as expected - """ - self.valid_workflow["actionFilters"] = [ - { - "logicType": "any", - "conditions": [ - { - "type": Condition.EQUAL.value, - "comparison": 1, - "conditionResult": True, - } - ], - "actions": [ - { - "type": Action.Type.EMAIL, - "config": { - "targetIdentifier": str(self.user.id), - "targetType": "user", - }, - "data": {}, - }, - ], - } - ] - response = self.get_success_response( - self.organization.slug, self.workflow.id, raw_data=self.valid_workflow - ) - updated_workflow = Workflow.objects.get(id=response.data.get("id")) - action_filter = WorkflowDataConditionGroup.objects.get(workflow=updated_workflow) - dcga = DataConditionGroupAction.objects.get(condition_group=action_filter.condition_group) - action = dcga.action - - assert response.status_code == 200 - assert action.type == Action.Type.EMAIL - assert action.data == {} - assert action.config == { - "target_type": ActionTarget.USER.value, - "target_identifier": str(self.user.id), - } - @responses.activate def test_update_add_sentry_app_action(self) -> None: """ From 0bc27140fd6163846243099b0b2e3f76e8fa00e2 Mon Sep 17 00:00:00 2001 From: Colleen O'Rourke Date: Mon, 24 Nov 2025 15:33:42 -0800 Subject: [PATCH 09/25] fix installation fetch logic --- .../notification_action/action_validation.py | 26 ++++++++++++++++--- src/sentry/sentry_apps/services/app/impl.py | 13 ++++++++++ .../sentry_apps/services/app/service.py | 7 +++++ 3 files changed, 43 insertions(+), 3 deletions(-) diff --git a/src/sentry/notifications/notification_action/action_validation.py b/src/sentry/notifications/notification_action/action_validation.py index 1e6a4dce132ad9..dbd94b702bd9a3 100644 --- a/src/sentry/notifications/notification_action/action_validation.py +++ b/src/sentry/notifications/notification_action/action_validation.py @@ -15,11 +15,12 @@ from sentry.rules.actions.integrations.create_ticket.form import IntegrationNotifyServiceForm from sentry.rules.actions.notify_event_service import NotifyEventServiceForm from sentry.rules.actions.sentry_apps.utils import validate_sentry_app_action -from sentry.sentry_apps.services.app import app_service +from sentry.sentry_apps.services.app import RpcSentryAppInstallation, app_service from sentry.sentry_apps.utils.errors import SentryAppBaseError from sentry.utils import json from sentry.workflow_engine.models.action import Action from sentry.workflow_engine.processors.action import get_notification_plugins_for_org +from sentry.workflow_engine.typings.notification_action import SentryAppIdentifier from .types import BaseActionValidatorHandler @@ -205,10 +206,29 @@ def __init__(self, validated_data: dict[str, Any], organization: Organization) - self.validated_data = validated_data self.organization = organization + def _get_sentry_app_installation( + self, sentry_app_identifier: SentryAppIdentifier, target_identifier: str + ) -> RpcSentryAppInstallation | None: + """ + Get the sentry app installation based on whether the target identifier is an installation id or sentry app id + We do not want to accept SentryAppIdentifier.SENTRY_APP_INSTALLATION_UUID long term, this is temporary until we migrate the data over + """ + installation = None + + if sentry_app_identifier == SentryAppIdentifier.SENTRY_APP_INSTALLATION_UUID: + installation = app_service.get_installation_by_id(target_identifier) + else: + installation = app_service.get_installation_by_sentry_app_id( + sentry_app_id=int(target_identifier), organization_id=self.organization.id + ) + return installation + def clean_data(self) -> dict[str, Any]: - installation = app_service.get_installation_by_id( - id=int(self.validated_data["config"]["target_identifier"]) + sentry_app_identifier = SentryAppIdentifier( + self.validated_data["config"]["sentry_app_identifier"] ) + target_identifier = self.validated_data["config"]["target_identifier"] + installation = self._get_sentry_app_installation(sentry_app_identifier, target_identifier) settings = self.validated_data["data"].get("settings", []) action = { "settings": settings, diff --git a/src/sentry/sentry_apps/services/app/impl.py b/src/sentry/sentry_apps/services/app/impl.py index a448d01b1e2399..c0281289444e36 100644 --- a/src/sentry/sentry_apps/services/app/impl.py +++ b/src/sentry/sentry_apps/services/app/impl.py @@ -72,6 +72,19 @@ def get_sentry_app_by_id(self, *, id: int) -> RpcSentryApp | None: return None return serialize_sentry_app(sentry_app) + def get_installation_by_sentry_app_id( + self, sentry_app_id: int, organization_id: int + ) -> RpcSentryAppInstallation | None: + try: + install = SentryAppInstallation.objects.select_related("sentry_app").get( + sentry_app_id=sentry_app_id, + organization_id=organization_id, + status=SentryAppInstallationStatus.INSTALLED, + ) + return serialize_sentry_app_installation(install) + except SentryAppInstallation.DoesNotExist: + return None + def get_installation_by_id(self, *, id: int) -> RpcSentryAppInstallation | None: try: install = SentryAppInstallation.objects.select_related("sentry_app").get( diff --git a/src/sentry/sentry_apps/services/app/service.py b/src/sentry/sentry_apps/services/app/service.py index c0c494b3d57541..ea8b275d9785c0 100644 --- a/src/sentry/sentry_apps/services/app/service.py +++ b/src/sentry/sentry_apps/services/app/service.py @@ -82,6 +82,13 @@ def get_installations_for_organization( def get_sentry_app_by_id(self, *, id: int) -> RpcSentryApp | None: pass + @rpc_method + @abc.abstractmethod + def get_installation_by_sentry_app_id( + self, sentry_app_id: int, organization_id: int + ) -> RpcSentryApp | None: + pass + @rpc_method @abc.abstractmethod def get_sentry_app_by_slug(self, *, slug: str) -> RpcSentryApp | None: From 959f0593ec5892c008da8b37f294234d70273ca1 Mon Sep 17 00:00:00 2001 From: Colleen O'Rourke Date: Mon, 24 Nov 2025 15:46:07 -0800 Subject: [PATCH 10/25] fix return type, typo --- src/sentry/sentry_apps/services/app/service.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/sentry/sentry_apps/services/app/service.py b/src/sentry/sentry_apps/services/app/service.py index ea8b275d9785c0..144eb12cc06c61 100644 --- a/src/sentry/sentry_apps/services/app/service.py +++ b/src/sentry/sentry_apps/services/app/service.py @@ -86,7 +86,7 @@ def get_sentry_app_by_id(self, *, id: int) -> RpcSentryApp | None: @abc.abstractmethod def get_installation_by_sentry_app_id( self, sentry_app_id: int, organization_id: int - ) -> RpcSentryApp | None: + ) -> RpcSentryAppInstallation | None: pass @rpc_method From da225d85765cf504b39188664a04fbc149838bf9 Mon Sep 17 00:00:00 2001 From: Colleen O'Rourke Date: Mon, 24 Nov 2025 15:52:38 -0800 Subject: [PATCH 11/25] don't need all this after all, yay --- tests/sentry/deletions/test_sentry_app.py | 10 ------- .../test_sentry_app_installations.py | 10 ------- .../test_sentry_app_metric_alert_handler.py | 9 ++---- .../api/endpoints/test_sentry_app_details.py | 5 ---- .../test_migrate_alert_rule.py | 5 ---- .../processors/test_action_deduplication.py | 10 ------- .../service/test_action_service.py | 30 ------------------- 7 files changed, 2 insertions(+), 77 deletions(-) diff --git a/tests/sentry/deletions/test_sentry_app.py b/tests/sentry/deletions/test_sentry_app.py index 6edc64dc2a1511..793a18cd3b8a32 100644 --- a/tests/sentry/deletions/test_sentry_app.py +++ b/tests/sentry/deletions/test_sentry_app.py @@ -67,11 +67,6 @@ def test_disables_actions(self) -> None: "sentry_app_identifier": SentryAppIdentifier.SENTRY_APP_ID, "target_type": ActionTarget.SENTRY_APP, }, - data={ - "settings": [ - {"name": "best_emoji", "value": ":fire:"}, - ] - }, ) webhook_action = self.create_action( type=Action.Type.WEBHOOK, @@ -86,11 +81,6 @@ def test_disables_actions(self) -> None: "sentry_app_identifier": SentryAppIdentifier.SENTRY_APP_ID, "target_type": ActionTarget.SENTRY_APP, }, - data={ - "settings": [ - {"name": "best_emoji", "value": ":fire:"}, - ] - }, ) deletions.exec_sync(self.sentry_app) diff --git a/tests/sentry/deletions/test_sentry_app_installations.py b/tests/sentry/deletions/test_sentry_app_installations.py index fd27a816445b16..a35c33c52aee87 100644 --- a/tests/sentry/deletions/test_sentry_app_installations.py +++ b/tests/sentry/deletions/test_sentry_app_installations.py @@ -119,11 +119,6 @@ def test_disables_actions(self) -> None: "sentry_app_identifier": SentryAppIdentifier.SENTRY_APP_INSTALLATION_UUID, "target_type": ActionTarget.SENTRY_APP, }, - data={ - "settings": [ - {"name": "best_emoji", "value": ":fire:"}, - ] - }, ) other_action = self.create_action( type=Action.Type.SENTRY_APP, @@ -132,11 +127,6 @@ def test_disables_actions(self) -> None: "sentry_app_identifier": SentryAppIdentifier.SENTRY_APP_INSTALLATION_UUID, "target_type": ActionTarget.SENTRY_APP, }, - data={ - "settings": [ - {"name": "best_emoji", "value": ":fire:"}, - ] - }, ) deletions.exec_sync(self.install) with outbox_runner(): diff --git a/tests/sentry/notifications/notification_action/metric_alert_registry/test_sentry_app_metric_alert_handler.py b/tests/sentry/notifications/notification_action/metric_alert_registry/test_sentry_app_metric_alert_handler.py index b5f3073a3b495e..73f02b5aa5cfd7 100644 --- a/tests/sentry/notifications/notification_action/metric_alert_registry/test_sentry_app_metric_alert_handler.py +++ b/tests/sentry/notifications/notification_action/metric_alert_registry/test_sentry_app_metric_alert_handler.py @@ -45,11 +45,6 @@ def setUp(self) -> None: "target_type": ActionTarget.SENTRY_APP.value, "sentry_app_identifier": SentryAppIdentifier.SENTRY_APP_ID, }, - data={ - "settings": [ - {"name": "best_emoji", "value": ":fire:"}, - ] - }, ) self.handler = SentryAppMetricAlertHandler() @@ -117,7 +112,7 @@ def test_invoke_legacy_registry(self, mock_send_alert: mock.MagicMock) -> None: integration_id=None, target_identifier=None, target_display=None, - sentry_app_config=self.action.data.get("settings"), + sentry_app_config=None, sentry_app_id=str(self.sentry_app.id), ) @@ -193,7 +188,7 @@ def test_invoke_legacy_registry_with_activity(self, mock_send_alert: mock.MagicM integration_id=None, target_identifier=None, target_display=None, - sentry_app_config=self.action.data.get("settings"), + sentry_app_config=None, sentry_app_id=str(self.sentry_app.id), ) diff --git a/tests/sentry/sentry_apps/api/endpoints/test_sentry_app_details.py b/tests/sentry/sentry_apps/api/endpoints/test_sentry_app_details.py index bb43a9e3f20257..4b034819926f2e 100644 --- a/tests/sentry/sentry_apps/api/endpoints/test_sentry_app_details.py +++ b/tests/sentry/sentry_apps/api/endpoints/test_sentry_app_details.py @@ -858,11 +858,6 @@ def test_disables_actions(self) -> None: "sentry_app_identifier": SentryAppIdentifier.SENTRY_APP_ID, "target_type": ActionTarget.SENTRY_APP, }, - data={ - "settings": [ - {"name": "best_emoji", "value": ":fire:"}, - ] - }, ) webhook_action = self.create_action( type=Action.Type.WEBHOOK, diff --git a/tests/sentry/workflow_engine/migration_helpers/test_migrate_alert_rule.py b/tests/sentry/workflow_engine/migration_helpers/test_migrate_alert_rule.py index bb5cd607d06a48..c2d581d617d138 100644 --- a/tests/sentry/workflow_engine/migration_helpers/test_migrate_alert_rule.py +++ b/tests/sentry/workflow_engine/migration_helpers/test_migrate_alert_rule.py @@ -1310,11 +1310,6 @@ def test_dual_update_trigger_action_data_sentry_app_null_value(self) -> None: target_type=AlertRuleTriggerAction.TargetType.SENTRY_APP, sentry_app=sentry_app, alert_rule_trigger=self.alert_rule_trigger, - sentry_app_config=[ - {"name": "title", "value": "An alert"}, - {"name": "points", "value": "3"}, - {"name": "assignee", "value": "Denny"}, - ], ) action, _, _ = migrate_metric_action(sentry_app_trigger_action) update_alert_rule_trigger_action( diff --git a/tests/sentry/workflow_engine/processors/test_action_deduplication.py b/tests/sentry/workflow_engine/processors/test_action_deduplication.py index 4133e1fa104693..6492c0bb186288 100644 --- a/tests/sentry/workflow_engine/processors/test_action_deduplication.py +++ b/tests/sentry/workflow_engine/processors/test_action_deduplication.py @@ -395,11 +395,6 @@ def test_deduplicate_actions_sentry_app_same_identifier(self) -> None: "target_identifier": "action-123", "sentry_app_identifier": SentryAppIdentifier.SENTRY_APP_ID, }, - data={ - "settings": [ - {"name": "best_emoji", "value": ":fire:"}, - ] - }, ) sentry_app_action_2 = self.create_action( @@ -409,11 +404,6 @@ def test_deduplicate_actions_sentry_app_same_identifier(self) -> None: "target_identifier": "action-123", "sentry_app_identifier": SentryAppIdentifier.SENTRY_APP_ID, }, - data={ - "settings": [ - {"name": "best_emoji", "value": ":fire:"}, - ] - }, ) actions_queryset = Action.objects.filter( diff --git a/tests/sentry/workflow_engine/service/test_action_service.py b/tests/sentry/workflow_engine/service/test_action_service.py index e5b5e0bccc5871..0ebe7c8913e318 100644 --- a/tests/sentry/workflow_engine/service/test_action_service.py +++ b/tests/sentry/workflow_engine/service/test_action_service.py @@ -143,11 +143,6 @@ def test_delete_actions_for_organization_integration_mixed_types(self) -> None: "sentry_app_identifier": SentryAppIdentifier.SENTRY_APP_ID, "target_type": ActionTarget.SENTRY_APP, }, - data={ - "settings": [ - {"name": "best_emoji", "value": ":fire:"}, - ] - }, ) self.create_data_condition_group_action( @@ -185,11 +180,6 @@ def test_disable_actions_for_organization_integration_mixed_types(self) -> None: "sentry_app_identifier": SentryAppIdentifier.SENTRY_APP_ID, "target_type": ActionTarget.SENTRY_APP, }, - data={ - "settings": [ - {"name": "best_emoji", "value": ":fire:"}, - ] - }, ) self.create_data_condition_group_action( @@ -235,11 +225,6 @@ def test_enable_actions_for_organization_integration_mixed_types(self) -> None: "sentry_app_identifier": SentryAppIdentifier.SENTRY_APP_ID, "target_type": ActionTarget.SENTRY_APP, }, - data={ - "settings": [ - {"name": "best_emoji", "value": ":fire:"}, - ] - }, status=ObjectStatus.DISABLED, ) @@ -277,11 +262,6 @@ def test_update_action_status_for_sentry_app__installation_uuid(self) -> None: "sentry_app_identifier": SentryAppIdentifier.SENTRY_APP_INSTALLATION_UUID, "target_type": ActionTarget.SENTRY_APP, }, - data={ - "settings": [ - {"name": "best_emoji", "value": ":fire:"}, - ] - }, ) action_service.update_action_status_for_sentry_app_via_uuid( @@ -306,11 +286,6 @@ def test_update_action_status_for_sentry_app__installation_uuid__region(self) -> "sentry_app_identifier": SentryAppIdentifier.SENTRY_APP_INSTALLATION_UUID, "target_type": ActionTarget.SENTRY_APP, }, - data={ - "settings": [ - {"name": "best_emoji", "value": ":fire:"}, - ] - }, ) action_service.update_action_status_for_sentry_app_via_uuid__region( region_name="us", @@ -329,11 +304,6 @@ def test_update_action_status_for_sentry_app__via_sentry_app_id(self) -> None: "sentry_app_identifier": SentryAppIdentifier.SENTRY_APP_ID, "target_type": ActionTarget.SENTRY_APP, }, - data={ - "settings": [ - {"name": "best_emoji", "value": ":fire:"}, - ] - }, ) action_service.update_action_status_for_sentry_app_via_sentry_app_id( region_name="us", From bde144fdbd45e2797174a10454d235d7afdd6bf8 Mon Sep 17 00:00:00 2001 From: Colleen O'Rourke Date: Mon, 24 Nov 2025 16:47:48 -0800 Subject: [PATCH 12/25] fix once again, add tests to make sure sentry app installation uuid still works for updating --- .../notification_action/action_validation.py | 58 +++++++------- .../test_organization_workflow_details.py | 77 +++++++++++++++++-- tests/sentry/workflow_engine/test_base.py | 10 ++- 3 files changed, 105 insertions(+), 40 deletions(-) diff --git a/src/sentry/notifications/notification_action/action_validation.py b/src/sentry/notifications/notification_action/action_validation.py index dbd94b702bd9a3..0a519da3cab6e8 100644 --- a/src/sentry/notifications/notification_action/action_validation.py +++ b/src/sentry/notifications/notification_action/action_validation.py @@ -15,7 +15,7 @@ from sentry.rules.actions.integrations.create_ticket.form import IntegrationNotifyServiceForm from sentry.rules.actions.notify_event_service import NotifyEventServiceForm from sentry.rules.actions.sentry_apps.utils import validate_sentry_app_action -from sentry.sentry_apps.services.app import RpcSentryAppInstallation, app_service +from sentry.sentry_apps.services.app import app_service from sentry.sentry_apps.utils.errors import SentryAppBaseError from sentry.utils import json from sentry.workflow_engine.models.action import Action @@ -206,53 +206,53 @@ def __init__(self, validated_data: dict[str, Any], organization: Organization) - self.validated_data = validated_data self.organization = organization - def _get_sentry_app_installation( - self, sentry_app_identifier: SentryAppIdentifier, target_identifier: str - ) -> RpcSentryAppInstallation | None: - """ - Get the sentry app installation based on whether the target identifier is an installation id or sentry app id - We do not want to accept SentryAppIdentifier.SENTRY_APP_INSTALLATION_UUID long term, this is temporary until we migrate the data over - """ - installation = None - - if sentry_app_identifier == SentryAppIdentifier.SENTRY_APP_INSTALLATION_UUID: - installation = app_service.get_installation_by_id(target_identifier) - else: - installation = app_service.get_installation_by_sentry_app_id( - sentry_app_id=int(target_identifier), organization_id=self.organization.id - ) - return installation - def clean_data(self) -> dict[str, Any]: sentry_app_identifier = SentryAppIdentifier( self.validated_data["config"]["sentry_app_identifier"] ) target_identifier = self.validated_data["config"]["target_identifier"] - installation = self._get_sentry_app_installation(sentry_app_identifier, target_identifier) settings = self.validated_data["data"].get("settings", []) + installation_uuid = None + + if sentry_app_identifier == SentryAppIdentifier.SENTRY_APP_ID: + installation = app_service.get_installation_by_sentry_app_id( + sentry_app_id=int(target_identifier), organization_id=self.organization.id + ) + installation_uuid = installation.uuid + + # XXX: because we only have the installation when the the identifier is SENTRY_APP_ID, we can only validate + # settings for this type. however, this is ok because the front end always sends this identifier for new actions + # and we can assume old actions are correct. we will migrate away from old data soon anyway + if not settings: + # XXX: it's only ok to not pass settings if there is no sentry app schema + # this means the app doesn't expect any settings + components = app_service.find_app_components(app_id=installation.sentry_app.id) + if any(component.app_schema for component in components): + raise ValidationError("'settings' is a required property") + else: + installation_uuid = target_identifier + action = { "settings": settings, - "sentryAppInstallationUuid": installation.uuid, + "sentryAppInstallationUuid": installation_uuid, } - # XXX: it's only ok to not pass settings if there is no sentry app schema - # this means the app doesn't expect any settings - if not settings: - components = app_service.find_app_components(app_id=installation.sentry_app.id) - if any(component.app_schema for component in components): - raise ValidationError("'settings' is a required property") + if settings: + try: + # Only call creator for Sentry Apps with UI Components (settings) for actions + validate_sentry_app_action(action) + except SentryAppBaseError as e: + raise ValidationError(e.message) from e try: validate_sentry_app_action(action) - # Sentry app config blob expects value to be a string - settings = self.validated_data["data"]["settings"] for setting in settings: if setting.get("value") is not None and not isinstance(setting["value"], str): setting["value"] = json.dumps(setting["value"]) - return self.validated_data except SentryAppBaseError as e: raise ValidationError(e.message) from e + return self.validated_data @action_validator_registry.register(Action.Type.WEBHOOK) diff --git a/tests/sentry/workflow_engine/endpoints/test_organization_workflow_details.py b/tests/sentry/workflow_engine/endpoints/test_organization_workflow_details.py index 8b699b6d4bf628..fe7bd7345fb371 100644 --- a/tests/sentry/workflow_engine/endpoints/test_organization_workflow_details.py +++ b/tests/sentry/workflow_engine/endpoints/test_organization_workflow_details.py @@ -77,6 +77,15 @@ def setUp(self) -> None: validator.is_valid(raise_exception=True) self.workflow = validator.create(validator.validated_data) + self.sentry_app, self.sentry_app_installation = self.create_sentry_app_with_schema() + self.sentry_app_settings = [ + {"name": "alert_prefix", "value": "[Not Good]"}, + {"name": "channel", "value": "#ignored-errors"}, + {"name": "best_emoji", "value": ":fire:"}, + {"name": "teamId", "value": 1}, + {"name": "assigneeId", "value": 3}, + ] + def test_simple(self) -> None: self.valid_workflow["name"] = "Updated Workflow" response = self.get_success_response( @@ -97,14 +106,6 @@ def test_update_add_sentry_app_action(self) -> None: url="https://example.com/sentry/alert-rule", status=200, ) - self.sentry_app = self.create_sentry_app_with_schema() - self.sentry_app_settings = [ - {"name": "alert_prefix", "value": "[Not Good]"}, - {"name": "channel", "value": "#ignored-errors"}, - {"name": "best_emoji", "value": ":fire:"}, - {"name": "teamId", "value": 1}, - {"name": "assigneeId", "value": 3}, - ] self.valid_workflow["actionFilters"] = [ { "logicType": "any", @@ -145,6 +146,66 @@ def test_update_add_sentry_app_action(self) -> None: } assert action.data["settings"] == self.sentry_app_settings + @responses.activate + def test_update_add_sentry_app_installation_uuid_action(self) -> None: + """ + Test that adding a sentry app action to a workflow works as expected when we pass the installation UUID instead of the sentry app id. + We do not create new actions like this today, but we have data like this in the DB and need to make sure it still works until we can migrate away from it + to use the sentry app id + """ + responses.add( + method=responses.POST, + url="https://example.com/sentry/alert-rule", + status=200, + ) + # update the settings + updated_sentry_app_settings = [ + {"name": "alert_prefix", "value": "[Very Good]"}, + {"name": "channel", "value": "#pay-attention-to-errors"}, + {"name": "best_emoji", "value": ":ice:"}, + {"name": "teamId", "value": 1}, + {"name": "assigneeId", "value": 3}, + ] + self.valid_workflow["actionFilters"] = [ + { + "logicType": "any", + "conditions": [ + { + "type": Condition.EQUAL.value, + "comparison": 1, + "conditionResult": True, + } + ], + "actions": [ + { + "config": { + "sentryAppIdentifier": SentryAppIdentifier.SENTRY_APP_INSTALLATION_UUID, + "targetIdentifier": self.sentry_app_installation.uuid, + "targetType": ActionType.SENTRY_APP, + }, + "data": {"settings": updated_sentry_app_settings}, + "type": Action.Type.SENTRY_APP, + }, + ], + } + ] + response = self.get_success_response( + self.organization.slug, self.workflow.id, raw_data=self.valid_workflow + ) + updated_workflow = Workflow.objects.get(id=response.data.get("id")) + action_filter = WorkflowDataConditionGroup.objects.get(workflow=updated_workflow) + dcga = DataConditionGroupAction.objects.get(condition_group=action_filter.condition_group) + action = dcga.action + + assert response.status_code == 200 + assert action.type == Action.Type.SENTRY_APP + assert action.config == { + "sentry_app_identifier": SentryAppIdentifier.SENTRY_APP_INSTALLATION_UUID, + "target_identifier": self.sentry_app_installation.uuid, + "target_type": ActionTarget.SENTRY_APP.value, + } + assert action.data["settings"] == updated_sentry_app_settings + def test_update_triggers_with_empty_conditions(self) -> None: """Test that passing an empty list to triggers.conditions clears all conditions""" # Create a workflow with a trigger condition diff --git a/tests/sentry/workflow_engine/test_base.py b/tests/sentry/workflow_engine/test_base.py index ad64f0c3a50134..b77eff1e3cad82 100644 --- a/tests/sentry/workflow_engine/test_base.py +++ b/tests/sentry/workflow_engine/test_base.py @@ -16,6 +16,8 @@ from sentry.models.group import Group from sentry.models.project import Project from sentry.notifications.notification_action.types import BaseActionValidatorHandler +from sentry.sentry_apps.models.sentry_app import SentryApp +from sentry.sentry_apps.models.sentry_app_installation import SentryAppInstallation from sentry.services.eventstore.models import Event, GroupEvent from sentry.snuba.dataset import Dataset from sentry.snuba.models import QuerySubscription, SnubaQuery, SnubaQueryEventType @@ -365,7 +367,7 @@ def create_group_event( return group, event, group_event - def create_sentry_app_with_schema(self): + def create_sentry_app_with_schema(self) -> tuple[SentryApp, SentryAppInstallation]: sentry_app_settings_schema = self.create_alert_rule_action_schema() sentry_app = self.create_sentry_app( name="Moo Deng's Fire Sentry App", @@ -377,5 +379,7 @@ def create_sentry_app_with_schema(self): }, is_alertable=True, ) - self.create_sentry_app_installation(slug=sentry_app.slug, organization=self.organization) - return sentry_app + installation = self.create_sentry_app_installation( + slug=sentry_app.slug, organization=self.organization + ) + return sentry_app, installation From ec0be09e5abbb808ef245b468d8ff51fd0553e05 Mon Sep 17 00:00:00 2001 From: Colleen O'Rourke Date: Mon, 24 Nov 2025 17:04:18 -0800 Subject: [PATCH 13/25] look up installation by uuid and org id --- .../notification_action/action_validation.py | 48 +++++++++++-------- src/sentry/sentry_apps/services/app/impl.py | 13 +++++ .../sentry_apps/services/app/service.py | 7 +++ 3 files changed, 48 insertions(+), 20 deletions(-) diff --git a/src/sentry/notifications/notification_action/action_validation.py b/src/sentry/notifications/notification_action/action_validation.py index 0a519da3cab6e8..c5ef84a9812bfa 100644 --- a/src/sentry/notifications/notification_action/action_validation.py +++ b/src/sentry/notifications/notification_action/action_validation.py @@ -15,7 +15,7 @@ from sentry.rules.actions.integrations.create_ticket.form import IntegrationNotifyServiceForm from sentry.rules.actions.notify_event_service import NotifyEventServiceForm from sentry.rules.actions.sentry_apps.utils import validate_sentry_app_action -from sentry.sentry_apps.services.app import app_service +from sentry.sentry_apps.services.app import RpcSentryAppInstallation, app_service from sentry.sentry_apps.utils.errors import SentryAppBaseError from sentry.utils import json from sentry.workflow_engine.models.action import Action @@ -206,35 +206,43 @@ def __init__(self, validated_data: dict[str, Any], organization: Organization) - self.validated_data = validated_data self.organization = organization + def _get_sentry_app_installation( + self, sentry_app_identifier: SentryAppIdentifier, target_identifier: str + ) -> RpcSentryAppInstallation | None: + """ + Get the sentry app installation based on whether the target identifier is an installation id or sentry app id + We do not want to accept SentryAppIdentifier.SENTRY_APP_INSTALLATION_UUID long term, this is temporary until we migrate the data over + """ + installation = None + + if sentry_app_identifier == SentryAppIdentifier.SENTRY_APP_INSTALLATION_UUID: + installation = app_service.get_installation_by_uuid( + uuid=target_identifier, organization_id=self.organization.id + ) + else: + installation = app_service.get_installation_by_sentry_app_id( + sentry_app_id=int(target_identifier), organization_id=self.organization.id + ) + return installation + def clean_data(self) -> dict[str, Any]: sentry_app_identifier = SentryAppIdentifier( self.validated_data["config"]["sentry_app_identifier"] ) target_identifier = self.validated_data["config"]["target_identifier"] + installation = self._get_sentry_app_installation(sentry_app_identifier, target_identifier) settings = self.validated_data["data"].get("settings", []) - installation_uuid = None - if sentry_app_identifier == SentryAppIdentifier.SENTRY_APP_ID: - installation = app_service.get_installation_by_sentry_app_id( - sentry_app_id=int(target_identifier), organization_id=self.organization.id - ) - installation_uuid = installation.uuid - - # XXX: because we only have the installation when the the identifier is SENTRY_APP_ID, we can only validate - # settings for this type. however, this is ok because the front end always sends this identifier for new actions - # and we can assume old actions are correct. we will migrate away from old data soon anyway - if not settings: - # XXX: it's only ok to not pass settings if there is no sentry app schema - # this means the app doesn't expect any settings - components = app_service.find_app_components(app_id=installation.sentry_app.id) - if any(component.app_schema for component in components): - raise ValidationError("'settings' is a required property") - else: - installation_uuid = target_identifier + if not settings: + # XXX: it's only ok to not pass settings if there is no sentry app schema + # this means the app doesn't expect any settings + components = app_service.find_app_components(app_id=installation.sentry_app.id) + if any(component.app_schema for component in components): + raise ValidationError("'settings' is a required property") action = { "settings": settings, - "sentryAppInstallationUuid": installation_uuid, + "sentryAppInstallationUuid": installation.uuid, } if settings: try: diff --git a/src/sentry/sentry_apps/services/app/impl.py b/src/sentry/sentry_apps/services/app/impl.py index c0281289444e36..c3ae9f0b392346 100644 --- a/src/sentry/sentry_apps/services/app/impl.py +++ b/src/sentry/sentry_apps/services/app/impl.py @@ -85,6 +85,19 @@ def get_installation_by_sentry_app_id( except SentryAppInstallation.DoesNotExist: return None + def get_installation_by_uuid( + self, uuid: str, organization_id: int + ) -> RpcSentryAppInstallation | None: + try: + install = SentryAppInstallation.objects.select_related("sentry_app").get( + uuid=uuid, + organization_id=organization_id, + status=SentryAppInstallationStatus.INSTALLED, + ) + return serialize_sentry_app_installation(install) + except SentryAppInstallation.DoesNotExist: + return None + def get_installation_by_id(self, *, id: int) -> RpcSentryAppInstallation | None: try: install = SentryAppInstallation.objects.select_related("sentry_app").get( diff --git a/src/sentry/sentry_apps/services/app/service.py b/src/sentry/sentry_apps/services/app/service.py index 144eb12cc06c61..f79c1c66ce0b66 100644 --- a/src/sentry/sentry_apps/services/app/service.py +++ b/src/sentry/sentry_apps/services/app/service.py @@ -89,6 +89,13 @@ def get_installation_by_sentry_app_id( ) -> RpcSentryAppInstallation | None: pass + @rpc_method + @abc.abstractmethod + def get_installation_by_uuid( + self, uuid: str, organization_id: int + ) -> RpcSentryAppInstallation | None: + pass + @rpc_method @abc.abstractmethod def get_sentry_app_by_slug(self, *, slug: str) -> RpcSentryApp | None: From 17c717de3203e4d79b32e8781021e4cb2ed147e5 Mon Sep 17 00:00:00 2001 From: Colleen O'Rourke Date: Mon, 24 Nov 2025 17:39:04 -0800 Subject: [PATCH 14/25] move rebased stuff around to make more sense, fix tests --- .../notification_action/action_validation.py | 23 ++++++++----------- .../test_organization_workflow_details.py | 8 +++---- .../test_organization_workflow_index.py | 12 ++++++---- 3 files changed, 21 insertions(+), 22 deletions(-) diff --git a/src/sentry/notifications/notification_action/action_validation.py b/src/sentry/notifications/notification_action/action_validation.py index c5ef84a9812bfa..c0e1be7b503f20 100644 --- a/src/sentry/notifications/notification_action/action_validation.py +++ b/src/sentry/notifications/notification_action/action_validation.py @@ -232,6 +232,10 @@ def clean_data(self) -> dict[str, Any]: target_identifier = self.validated_data["config"]["target_identifier"] installation = self._get_sentry_app_installation(sentry_app_identifier, target_identifier) settings = self.validated_data["data"].get("settings", []) + action = { + "settings": settings, + "sentryAppInstallationUuid": installation.uuid, + } if not settings: # XXX: it's only ok to not pass settings if there is no sentry app schema @@ -240,26 +244,17 @@ def clean_data(self) -> dict[str, Any]: if any(component.app_schema for component in components): raise ValidationError("'settings' is a required property") - action = { - "settings": settings, - "sentryAppInstallationUuid": installation.uuid, - } - if settings: + else: + # Sentry app config blob expects value to be a string + for setting in settings: + if setting.get("value") is not None and not isinstance(setting["value"], str): + setting["value"] = json.dumps(setting["value"]) try: # Only call creator for Sentry Apps with UI Components (settings) for actions validate_sentry_app_action(action) except SentryAppBaseError as e: raise ValidationError(e.message) from e - try: - validate_sentry_app_action(action) - # Sentry app config blob expects value to be a string - for setting in settings: - if setting.get("value") is not None and not isinstance(setting["value"], str): - setting["value"] = json.dumps(setting["value"]) - - except SentryAppBaseError as e: - raise ValidationError(e.message) from e return self.validated_data diff --git a/tests/sentry/workflow_engine/endpoints/test_organization_workflow_details.py b/tests/sentry/workflow_engine/endpoints/test_organization_workflow_details.py index fe7bd7345fb371..d4b63c290ffd93 100644 --- a/tests/sentry/workflow_engine/endpoints/test_organization_workflow_details.py +++ b/tests/sentry/workflow_engine/endpoints/test_organization_workflow_details.py @@ -82,8 +82,8 @@ def setUp(self) -> None: {"name": "alert_prefix", "value": "[Not Good]"}, {"name": "channel", "value": "#ignored-errors"}, {"name": "best_emoji", "value": ":fire:"}, - {"name": "teamId", "value": 1}, - {"name": "assigneeId", "value": 3}, + {"name": "teamId", "value": "1"}, + {"name": "assigneeId", "value": "3"}, ] def test_simple(self) -> None: @@ -163,8 +163,8 @@ def test_update_add_sentry_app_installation_uuid_action(self) -> None: {"name": "alert_prefix", "value": "[Very Good]"}, {"name": "channel", "value": "#pay-attention-to-errors"}, {"name": "best_emoji", "value": ":ice:"}, - {"name": "teamId", "value": 1}, - {"name": "assigneeId", "value": 3}, + {"name": "teamId", "value": "1"}, + {"name": "assigneeId", "value": "3"}, ] self.valid_workflow["actionFilters"] = [ { diff --git a/tests/sentry/workflow_engine/endpoints/test_organization_workflow_index.py b/tests/sentry/workflow_engine/endpoints/test_organization_workflow_index.py index 3a201b07403f17..cf6ae37d332a40 100644 --- a/tests/sentry/workflow_engine/endpoints/test_organization_workflow_index.py +++ b/tests/sentry/workflow_engine/endpoints/test_organization_workflow_index.py @@ -1,7 +1,7 @@ - from collections.abc import Sequence from typing import Any from unittest import mock + import responses from sentry import audit_log @@ -11,7 +11,6 @@ from sentry.deletions.tasks.scheduled import run_scheduled_deletions from sentry.grouping.grouptype import ErrorGroupType from sentry.incidents.grouptype import MetricIssue -from sentry.notifications.models.notificationaction import ActionTarget from sentry.notifications.types import FallthroughChoiceType from sentry.testutils.asserts import assert_org_audit_log_exists from sentry.testutils.cases import APITestCase @@ -26,6 +25,11 @@ WorkflowFireHistory, ) from sentry.workflow_engine.models.data_condition import Condition +from sentry.workflow_engine.typings.notification_action import ( + ActionTarget, + ActionType, + SentryAppIdentifier, +) from tests.sentry.workflow_engine.test_base import BaseWorkflowTest, MockActionValidatorTranslator @@ -411,8 +415,8 @@ def setUp(self) -> None: {"name": "alert_prefix", "value": "[Not Good]"}, {"name": "channel", "value": "#ignored-errors"}, {"name": "best_emoji", "value": ":fire:"}, - {"name": "teamId", "value": 1}, - {"name": "assigneeId", "value": 3}, + {"name": "teamId", "value": "1"}, + {"name": "assigneeId", "value": "3"}, ] @mock.patch("sentry.workflow_engine.endpoints.validators.base.workflow.create_audit_entry") From 2ad3b58c4183008f2d2f6b77a7d40d60601d65bd Mon Sep 17 00:00:00 2001 From: Colleen O'Rourke Date: Mon, 24 Nov 2025 17:40:32 -0800 Subject: [PATCH 15/25] make sure installation exists --- .../notifications/notification_action/action_validation.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/sentry/notifications/notification_action/action_validation.py b/src/sentry/notifications/notification_action/action_validation.py index c0e1be7b503f20..d892b16d870d64 100644 --- a/src/sentry/notifications/notification_action/action_validation.py +++ b/src/sentry/notifications/notification_action/action_validation.py @@ -231,6 +231,8 @@ def clean_data(self) -> dict[str, Any]: ) target_identifier = self.validated_data["config"]["target_identifier"] installation = self._get_sentry_app_installation(sentry_app_identifier, target_identifier) + if not installation: + raise ValidationError("Sentry app installation not found.") settings = self.validated_data["data"].get("settings", []) action = { "settings": settings, From 4fc10d965c1cb8554bd2a97a461d60153410d50f Mon Sep 17 00:00:00 2001 From: Colleen O'Rourke Date: Tue, 25 Nov 2025 09:58:58 -0800 Subject: [PATCH 16/25] update test --- .../validators/actions/test_sentry_app.py | 25 +++++++++++++------ 1 file changed, 17 insertions(+), 8 deletions(-) diff --git a/tests/sentry/workflow_engine/endpoints/validators/actions/test_sentry_app.py b/tests/sentry/workflow_engine/endpoints/validators/actions/test_sentry_app.py index 0eab559ca6c562..ed080855c8ab18 100644 --- a/tests/sentry/workflow_engine/endpoints/validators/actions/test_sentry_app.py +++ b/tests/sentry/workflow_engine/endpoints/validators/actions/test_sentry_app.py @@ -4,24 +4,33 @@ from sentry.sentry_apps.services.app.model import RpcAlertRuleActionResult from sentry.sentry_apps.utils.errors import SentryAppErrorType -from sentry.testutils.cases import TestCase from sentry.workflow_engine.endpoints.validators.base import BaseActionValidator from sentry.workflow_engine.models import Action from sentry.workflow_engine.types import WorkflowEventData +from sentry.workflow_engine.typings.notification_action import ActionType, SentryAppIdentifier +from tests.sentry.workflow_engine.test_base import BaseWorkflowTest -class TestSentryAppActionValidator(TestCase): +class TestSentryAppActionValidator(BaseWorkflowTest): def setUp(self) -> None: super().setUp() + self.sentry_app, self.sentry_app_installation = self.create_sentry_app_with_schema() + self.sentry_app_settings = [ + {"name": "alert_prefix", "value": "[Not Good]"}, + {"name": "channel", "value": "#ignored-errors"}, + {"name": "best_emoji", "value": ":fire:"}, + {"name": "teamId", "value": "1"}, + {"name": "assigneeId", "value": "3"}, + ] self.valid_data = { "type": Action.Type.SENTRY_APP, "config": { - "sentry_app_identifier": "sentry_app_installation_uuid", - "targetType": "sentry_app", - "target_identifier": "123", + "sentry_app_identifier": SentryAppIdentifier.SENTRY_APP_ID.SENTRY_APP_INSTALLATION_UUID, + "targetType": ActionType.SENTRY_APP, + "target_identifier": self.sentry_app_installation.uuid, }, - "data": {"settings": []}, + "data": {"settings": self.sentry_app_settings}, } group_event = self.event.for_group(self.group) @@ -97,8 +106,8 @@ def test_validate_settings_action_trigger( self.valid_data = { "type": Action.Type.SENTRY_APP, "config": { - "sentry_app_identifier": "sentry_app_installation_uuid", - "targetType": "sentry_app", + "sentry_app_identifier": SentryAppIdentifier.SENTRY_APP_ID.SENTRY_APP_INSTALLATION_UUID, + "targetType": ActionType.SENTRY_APP, "target_identifier": install.uuid, }, "data": { From e8d52a3426f2520e9d8c2117826ad080b579af3d Mon Sep 17 00:00:00 2001 From: Colleen O'Rourke Date: Tue, 25 Nov 2025 14:38:40 -0800 Subject: [PATCH 17/25] use get_many --- .../notification_action/action_validation.py | 12 ++++++--- src/sentry/sentry_apps/services/app/impl.py | 26 ------------------- .../sentry_apps/services/app/service.py | 14 ---------- 3 files changed, 8 insertions(+), 44 deletions(-) diff --git a/src/sentry/notifications/notification_action/action_validation.py b/src/sentry/notifications/notification_action/action_validation.py index d892b16d870d64..23ae3536a509fe 100644 --- a/src/sentry/notifications/notification_action/action_validation.py +++ b/src/sentry/notifications/notification_action/action_validation.py @@ -213,16 +213,20 @@ def _get_sentry_app_installation( Get the sentry app installation based on whether the target identifier is an installation id or sentry app id We do not want to accept SentryAppIdentifier.SENTRY_APP_INSTALLATION_UUID long term, this is temporary until we migrate the data over """ + installations = None installation = None if sentry_app_identifier == SentryAppIdentifier.SENTRY_APP_INSTALLATION_UUID: - installation = app_service.get_installation_by_uuid( - uuid=target_identifier, organization_id=self.organization.id + installations = app_service.get_many( + filter=dict(uuids=[target_identifier], organization_ids=[self.organization.id]) ) else: - installation = app_service.get_installation_by_sentry_app_id( - sentry_app_id=int(target_identifier), organization_id=self.organization.id + installations = app_service.get_many( + filter=dict(app_ids=[target_identifier], organization_ids=[self.organization.id]) ) + if installations: + installation = installations[0] + return installation def clean_data(self) -> dict[str, Any]: diff --git a/src/sentry/sentry_apps/services/app/impl.py b/src/sentry/sentry_apps/services/app/impl.py index c3ae9f0b392346..a448d01b1e2399 100644 --- a/src/sentry/sentry_apps/services/app/impl.py +++ b/src/sentry/sentry_apps/services/app/impl.py @@ -72,32 +72,6 @@ def get_sentry_app_by_id(self, *, id: int) -> RpcSentryApp | None: return None return serialize_sentry_app(sentry_app) - def get_installation_by_sentry_app_id( - self, sentry_app_id: int, organization_id: int - ) -> RpcSentryAppInstallation | None: - try: - install = SentryAppInstallation.objects.select_related("sentry_app").get( - sentry_app_id=sentry_app_id, - organization_id=organization_id, - status=SentryAppInstallationStatus.INSTALLED, - ) - return serialize_sentry_app_installation(install) - except SentryAppInstallation.DoesNotExist: - return None - - def get_installation_by_uuid( - self, uuid: str, organization_id: int - ) -> RpcSentryAppInstallation | None: - try: - install = SentryAppInstallation.objects.select_related("sentry_app").get( - uuid=uuid, - organization_id=organization_id, - status=SentryAppInstallationStatus.INSTALLED, - ) - return serialize_sentry_app_installation(install) - except SentryAppInstallation.DoesNotExist: - return None - def get_installation_by_id(self, *, id: int) -> RpcSentryAppInstallation | None: try: install = SentryAppInstallation.objects.select_related("sentry_app").get( diff --git a/src/sentry/sentry_apps/services/app/service.py b/src/sentry/sentry_apps/services/app/service.py index f79c1c66ce0b66..c0c494b3d57541 100644 --- a/src/sentry/sentry_apps/services/app/service.py +++ b/src/sentry/sentry_apps/services/app/service.py @@ -82,20 +82,6 @@ def get_installations_for_organization( def get_sentry_app_by_id(self, *, id: int) -> RpcSentryApp | None: pass - @rpc_method - @abc.abstractmethod - def get_installation_by_sentry_app_id( - self, sentry_app_id: int, organization_id: int - ) -> RpcSentryAppInstallation | None: - pass - - @rpc_method - @abc.abstractmethod - def get_installation_by_uuid( - self, uuid: str, organization_id: int - ) -> RpcSentryAppInstallation | None: - pass - @rpc_method @abc.abstractmethod def get_sentry_app_by_slug(self, *, slug: str) -> RpcSentryApp | None: From 3206278ce33acd97d2daf36533d8a16209ae929d Mon Sep 17 00:00:00 2001 From: Colleen O'Rourke Date: Tue, 25 Nov 2025 14:52:21 -0800 Subject: [PATCH 18/25] update uuid to use app id --- .../notification_action/action_validation.py | 8 ++++++++ .../endpoints/test_organization_workflow_details.py | 4 ++-- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/src/sentry/notifications/notification_action/action_validation.py b/src/sentry/notifications/notification_action/action_validation.py index 23ae3536a509fe..33d6a31426bde9 100644 --- a/src/sentry/notifications/notification_action/action_validation.py +++ b/src/sentry/notifications/notification_action/action_validation.py @@ -237,6 +237,14 @@ def clean_data(self) -> dict[str, Any]: installation = self._get_sentry_app_installation(sentry_app_identifier, target_identifier) if not installation: raise ValidationError("Sentry app installation not found.") + + if sentry_app_identifier == SentryAppIdentifier.SENTRY_APP_INSTALLATION_UUID: + # convert to use sentry_app_id until we can migrate all the data + self.validated_data["config"][ + "sentry_app_identifier" + ] = SentryAppIdentifier.SENTRY_APP_ID + self.validated_data["config"]["target_identifier"] = str(installation.sentry_app.id) + settings = self.validated_data["data"].get("settings", []) action = { "settings": settings, diff --git a/tests/sentry/workflow_engine/endpoints/test_organization_workflow_details.py b/tests/sentry/workflow_engine/endpoints/test_organization_workflow_details.py index d4b63c290ffd93..df02e98782088a 100644 --- a/tests/sentry/workflow_engine/endpoints/test_organization_workflow_details.py +++ b/tests/sentry/workflow_engine/endpoints/test_organization_workflow_details.py @@ -200,8 +200,8 @@ def test_update_add_sentry_app_installation_uuid_action(self) -> None: assert response.status_code == 200 assert action.type == Action.Type.SENTRY_APP assert action.config == { - "sentry_app_identifier": SentryAppIdentifier.SENTRY_APP_INSTALLATION_UUID, - "target_identifier": self.sentry_app_installation.uuid, + "sentry_app_identifier": SentryAppIdentifier.SENTRY_APP_ID, + "target_identifier": str(self.sentry_app.id), "target_type": ActionTarget.SENTRY_APP.value, } assert action.data["settings"] == updated_sentry_app_settings From 4b9d4975af6e2f8793166b4b19c50796b721dff1 Mon Sep 17 00:00:00 2001 From: Colleen O'Rourke Date: Tue, 25 Nov 2025 14:55:03 -0800 Subject: [PATCH 19/25] fix test --- .../endpoints/validators/actions/test_sentry_app.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/sentry/workflow_engine/endpoints/validators/actions/test_sentry_app.py b/tests/sentry/workflow_engine/endpoints/validators/actions/test_sentry_app.py index ed080855c8ab18..efd7256bfd58df 100644 --- a/tests/sentry/workflow_engine/endpoints/validators/actions/test_sentry_app.py +++ b/tests/sentry/workflow_engine/endpoints/validators/actions/test_sentry_app.py @@ -26,7 +26,7 @@ def setUp(self) -> None: self.valid_data = { "type": Action.Type.SENTRY_APP, "config": { - "sentry_app_identifier": SentryAppIdentifier.SENTRY_APP_ID.SENTRY_APP_INSTALLATION_UUID, + "sentry_app_identifier": SentryAppIdentifier.SENTRY_APP_INSTALLATION_UUID, "targetType": ActionType.SENTRY_APP, "target_identifier": self.sentry_app_installation.uuid, }, From 105a194e38cd1c0ac570f10a9c627980adda7595 Mon Sep 17 00:00:00 2001 From: Colleen O'Rourke Date: Tue, 25 Nov 2025 14:56:31 -0800 Subject: [PATCH 20/25] fix get_many usage --- .../notifications/notification_action/action_validation.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/sentry/notifications/notification_action/action_validation.py b/src/sentry/notifications/notification_action/action_validation.py index 33d6a31426bde9..755a6a6f5119aa 100644 --- a/src/sentry/notifications/notification_action/action_validation.py +++ b/src/sentry/notifications/notification_action/action_validation.py @@ -218,11 +218,11 @@ def _get_sentry_app_installation( if sentry_app_identifier == SentryAppIdentifier.SENTRY_APP_INSTALLATION_UUID: installations = app_service.get_many( - filter=dict(uuids=[target_identifier], organization_ids=[self.organization.id]) + filter=dict(uuids=[target_identifier], organization_id=self.organization.id) ) else: installations = app_service.get_many( - filter=dict(app_ids=[target_identifier], organization_ids=[self.organization.id]) + filter=dict(app_ids=[target_identifier], organization_id=self.organization.id) ) if installations: installation = installations[0] From 0c384a5912af14a9aeaa294eb4e1dd08ad199cf2 Mon Sep 17 00:00:00 2001 From: Colleen O'Rourke Date: Tue, 25 Nov 2025 15:54:44 -0800 Subject: [PATCH 21/25] update test --- .../endpoints/test_organization_workflow_index.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/sentry/workflow_engine/endpoints/test_organization_workflow_index.py b/tests/sentry/workflow_engine/endpoints/test_organization_workflow_index.py index cf6ae37d332a40..639f2e0109020a 100644 --- a/tests/sentry/workflow_engine/endpoints/test_organization_workflow_index.py +++ b/tests/sentry/workflow_engine/endpoints/test_organization_workflow_index.py @@ -687,7 +687,7 @@ def test_create_sentry_app_action_no_settings(self) -> None: assert action.type == Action.Type.SENTRY_APP assert action.config == { - "sentry_app_identifier": SentryAppIdentifier.SENTRY_APP_ID.value, + "sentry_app_identifier": SentryAppIdentifier.SENTRY_APP_ID, "target_identifier": str(sentry_app.id), "target_type": ActionTarget.SENTRY_APP.value, } From 975c21622f293126e2f73a63b37f42171cd74db4 Mon Sep 17 00:00:00 2001 From: Colleen O'Rourke Date: Tue, 25 Nov 2025 16:18:40 -0800 Subject: [PATCH 22/25] fix test, add another for sentry app id --- .../validators/actions/test_sentry_app.py | 27 ++++++++++++++++--- 1 file changed, 24 insertions(+), 3 deletions(-) diff --git a/tests/sentry/workflow_engine/endpoints/validators/actions/test_sentry_app.py b/tests/sentry/workflow_engine/endpoints/validators/actions/test_sentry_app.py index efd7256bfd58df..ddc652f90f25e3 100644 --- a/tests/sentry/workflow_engine/endpoints/validators/actions/test_sentry_app.py +++ b/tests/sentry/workflow_engine/endpoints/validators/actions/test_sentry_app.py @@ -26,9 +26,9 @@ def setUp(self) -> None: self.valid_data = { "type": Action.Type.SENTRY_APP, "config": { - "sentry_app_identifier": SentryAppIdentifier.SENTRY_APP_INSTALLATION_UUID, + "sentryAppIdentifier": SentryAppIdentifier.SENTRY_APP_INSTALLATION_UUID, "targetType": ActionType.SENTRY_APP, - "target_identifier": self.sentry_app_installation.uuid, + "targetIdentifier": self.sentry_app_installation.uuid, }, "data": {"settings": self.sentry_app_settings}, } @@ -61,6 +61,27 @@ def test_validate(self, mock_trigger_sentry_app_action_creators: mock.MagicMock) assert result is True validator.save() + @mock.patch( + "sentry.rules.actions.sentry_apps.utils.app_service.trigger_sentry_app_action_creators" + ) + def test_validate_sentry_app_id( + self, mock_trigger_sentry_app_action_creators: mock.MagicMock + ) -> None: + mock_trigger_sentry_app_action_creators.return_value = RpcAlertRuleActionResult( + success=True, message="success" + ) + self.valid_data["config"]["sentryAppIdentifier"] = SentryAppIdentifier.SENTRY_APP_ID + self.valid_data["config"]["targetIdentifier"] = str(self.sentry_app.id) + + validator = BaseActionValidator( + data=self.valid_data, + context={"organization": self.organization}, + ) + + result = validator.is_valid() + assert result is True + validator.save() + @mock.patch( "sentry.rules.actions.sentry_apps.utils.app_service.trigger_sentry_app_action_creators" ) @@ -106,7 +127,7 @@ def test_validate_settings_action_trigger( self.valid_data = { "type": Action.Type.SENTRY_APP, "config": { - "sentry_app_identifier": SentryAppIdentifier.SENTRY_APP_ID.SENTRY_APP_INSTALLATION_UUID, + "sentry_app_identifier": SentryAppIdentifier.SENTRY_APP_INSTALLATION_UUID, "targetType": ActionType.SENTRY_APP, "target_identifier": install.uuid, }, From 51a1ea8d43d203762e0aff400eb070ed0272f3db Mon Sep 17 00:00:00 2001 From: Colleen O'Rourke Date: Tue, 25 Nov 2025 16:22:52 -0800 Subject: [PATCH 23/25] convert to int --- .../notifications/notification_action/action_validation.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/sentry/notifications/notification_action/action_validation.py b/src/sentry/notifications/notification_action/action_validation.py index 755a6a6f5119aa..db0fe1a58fe62e 100644 --- a/src/sentry/notifications/notification_action/action_validation.py +++ b/src/sentry/notifications/notification_action/action_validation.py @@ -222,7 +222,7 @@ def _get_sentry_app_installation( ) else: installations = app_service.get_many( - filter=dict(app_ids=[target_identifier], organization_id=self.organization.id) + filter=dict(app_ids=[int(target_identifier)], organization_id=self.organization.id) ) if installations: installation = installations[0] From 5d9fc723468fe0d94a05abf582f06d952596aa8b Mon Sep 17 00:00:00 2001 From: Colleen O'Rourke Date: Tue, 25 Nov 2025 16:32:34 -0800 Subject: [PATCH 24/25] only check alert-rule-action components --- .../notifications/notification_action/action_validation.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/sentry/notifications/notification_action/action_validation.py b/src/sentry/notifications/notification_action/action_validation.py index db0fe1a58fe62e..e8e9d5e2bbfb76 100644 --- a/src/sentry/notifications/notification_action/action_validation.py +++ b/src/sentry/notifications/notification_action/action_validation.py @@ -255,7 +255,11 @@ def clean_data(self) -> dict[str, Any]: # XXX: it's only ok to not pass settings if there is no sentry app schema # this means the app doesn't expect any settings components = app_service.find_app_components(app_id=installation.sentry_app.id) - if any(component.app_schema for component in components): + if any( + component.app_schema + for component in components + if component.type == "alert-rule-action" + ): raise ValidationError("'settings' is a required property") else: From 99b006729e64f4213b6cf48798b839de47a81fac Mon Sep 17 00:00:00 2001 From: Colleen O'Rourke Date: Tue, 25 Nov 2025 16:46:53 -0800 Subject: [PATCH 25/25] appease typing --- .../endpoints/validators/actions/test_sentry_app.py | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/tests/sentry/workflow_engine/endpoints/validators/actions/test_sentry_app.py b/tests/sentry/workflow_engine/endpoints/validators/actions/test_sentry_app.py index ddc652f90f25e3..aa56068a4b3fe4 100644 --- a/tests/sentry/workflow_engine/endpoints/validators/actions/test_sentry_app.py +++ b/tests/sentry/workflow_engine/endpoints/validators/actions/test_sentry_app.py @@ -70,11 +70,18 @@ def test_validate_sentry_app_id( mock_trigger_sentry_app_action_creators.return_value = RpcAlertRuleActionResult( success=True, message="success" ) - self.valid_data["config"]["sentryAppIdentifier"] = SentryAppIdentifier.SENTRY_APP_ID - self.valid_data["config"]["targetIdentifier"] = str(self.sentry_app.id) + valid_data = { + "type": Action.Type.SENTRY_APP, + "config": { + "sentryAppIdentifier": SentryAppIdentifier.SENTRY_APP_ID, + "targetType": ActionType.SENTRY_APP, + "targetIdentifier": str(self.sentry_app.id), + }, + "data": {"settings": self.sentry_app_settings}, + } validator = BaseActionValidator( - data=self.valid_data, + data=valid_data, context={"organization": self.organization}, )