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/src/sentry/notifications/notification_action/action_validation.py b/src/sentry/notifications/notification_action/action_validation.py index 6aa42b883df3d5..90ab6454435080 100644 --- a/src/sentry/notifications/notification_action/action_validation.py +++ b/src/sentry/notifications/notification_action/action_validation.py @@ -15,11 +15,10 @@ 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.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 @@ -206,18 +205,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) return self.validated_data 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) 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/endpoints/test_organization_workflow_details.py b/tests/sentry/workflow_engine/endpoints/test_organization_workflow_details.py index af001f4af86742..8b699b6d4bf628 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 @@ -13,8 +15,20 @@ 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 sentry.workflow_engine.typings.notification_action import ( + ActionTarget, + ActionType, + SentryAppIdentifier, +) from tests.sentry.workflow_engine.test_base import BaseWorkflowTest @@ -54,7 +68,7 @@ def setUp(self) -> None: "enabled": True, "config": {}, "triggers": {"logicType": "any", "conditions": []}, - "action_filters": [], + "actionFilters": [], } validator = WorkflowValidator( data=self.valid_workflow, @@ -73,6 +87,64 @@ def test_simple(self) -> None: assert response.status_code == 200 assert updated_workflow.name == "Updated Workflow" + @responses.activate + def test_update_add_sentry_app_action(self) -> None: + """ + Test that adding a sentry app action to a workflow works as expected + """ + responses.add( + method=responses.POST, + 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", + "conditions": [ + { + "type": Condition.EQUAL.value, + "comparison": 1, + "conditionResult": True, + } + ], + "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, 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_ID, + "target_identifier": str(self.sentry_app.id), + "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""" # 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 58c2cad1d4fe04..ac9dc661ab8f63 100644 --- a/tests/sentry/workflow_engine/endpoints/test_organization_workflow_index.py +++ b/tests/sentry/workflow_engine/endpoints/test_organization_workflow_index.py @@ -2,6 +2,8 @@ from typing import Any from unittest import mock +import responses + from sentry import audit_log from sentry.api.serializers import serialize from sentry.constants import ObjectStatus @@ -9,19 +11,24 @@ 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.testutils.asserts import assert_org_audit_log_exists from sentry.testutils.cases import APITestCase from sentry.testutils.outbox import outbox_runner from sentry.testutils.silo import region_silo_test from sentry.workflow_engine.models import ( Action, + DataConditionGroupAction, DetectorWorkflow, Workflow, WorkflowDataConditionGroup, WorkflowFireHistory, ) -from tests.sentry.workflow_engine.test_base import MockActionValidatorTranslator +from sentry.workflow_engine.typings.notification_action import ( + ActionTarget, + ActionType, + SentryAppIdentifier, +) +from tests.sentry.workflow_engine.test_base import BaseWorkflowTest, MockActionValidatorTranslator class OrganizationWorkflowAPITestCase(APITestCase): @@ -372,7 +379,7 @@ def test_compound_query(self) -> None: @region_silo_test -class OrganizationWorkflowCreateTest(OrganizationWorkflowAPITestCase): +class OrganizationWorkflowCreateTest(OrganizationWorkflowAPITestCase, BaseWorkflowTest): method = "POST" def setUp(self) -> None: @@ -394,6 +401,14 @@ def setUp(self) -> None: role="member", organization=self.organization, ) + 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: @@ -492,6 +507,168 @@ def test_create_workflow__with_actions(self, mock_action_validator: mock.MagicMo "actionFilters", [] )[0].get("id") + @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": [ + { + "type": "eq", + "comparison": 1, + "conditionResult": True, + } + ], + "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 be 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": [ + { + "type": "eq", + "comparison": 1, + "conditionResult": True, + } + ], + "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 + """ + + # currently it's failing trying to look up the sentryappcomponent in SentryAppAlertRuleActionCreator.run()'s _fetch_sentry_app_uri' + # but these types of apps don't have any components. look into how we create these for alert rules + # then I can go through and remove the settings from all the tests I added them to + 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": [ + { + "type": "eq", + "comparison": 1, + "conditionResult": True, + } + ], + "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, + "target_identifier": str(self.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( 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 90ea747db6b8dd..b2a3d2141953ea 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( 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", diff --git a/tests/sentry/workflow_engine/test_base.py b/tests/sentry/workflow_engine/test_base.py index 917329517bba0a..ad64f0c3a50134 100644 --- a/tests/sentry/workflow_engine/test_base.py +++ b/tests/sentry/workflow_engine/test_base.py @@ -364,3 +364,18 @@ def create_group_event( ) return group, event, group_event + + 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", + organization=self.organization, + schema={ + "elements": [ + sentry_app_settings_schema, + ] + }, + is_alertable=True, + ) + self.create_sentry_app_installation(slug=sentry_app.slug, organization=self.organization) + return sentry_app