fix(rules): Surface errors from Sentry Apps + reformat slack errors (#33571)

See API-2619

This PR cleans up the errors that appear on the rules page for Sentry Apps and Slack.

I've also added a TODO to refactor the function that checks for Alert Rule Actions into a better wrapper, along with renaming one of the constants.

src/sentry/api/endpoints/project_rules.py

@@ -9,6 +9,7 @@ from sentry.api.serializers import serialize
 from sentry.api.serializers.rest_framework import RuleSerializer
 from sentry.integrations.slack import tasks
 from sentry.mediators import alert_rule_actions, project_rules
+from sentry.mediators.external_requests.alert_rule_action_requester import AlertRuleActionResult
 from sentry.models import (
     AuditLogEntryEvent,
     Rule,
@@ -23,6 +24,7 @@ from sentry.signals import alert_rule_created
 from sentry.web.decorators import transaction_start
 
 
+# TODO(Leander): Move this method into a relevant abstract base class
 def trigger_alert_rule_action_creators(
     actions: Sequence[Mapping[str, str]],
 ) -> Optional[str]:
@@ -33,15 +35,13 @@ def trigger_alert_rule_action_creators(
             continue
 
         install = SentryAppInstallation.objects.get(uuid=action.get("sentryAppInstallationUuid"))
-        result = alert_rule_actions.AlertRuleActionCreator.run(
+        result: AlertRuleActionResult = alert_rule_actions.AlertRuleActionCreator.run(
             install=install,
             fields=action.get("settings"),
         )
         # Bubble up errors from Sentry App to the UI
         if not result["success"]:
-            raise serializers.ValidationError(
-                {"sentry_app": f'{install.sentry_app.name}: {result["message"]}'}
-            )
+            raise serializers.ValidationError({"actions": [result["message"]]})
         created = "alert-rule-action"
     return created
 

src/sentry/api/endpoints/project_rules_configuration.py

@@ -3,7 +3,7 @@ from rest_framework.response import Response
 
 from sentry import features
 from sentry.api.bases.project import ProjectEndpoint
-from sentry.constants import MIGRATED_CONDITIONS, SCHEMA_FORM_ACTIONS, TICKET_ACTIONS
+from sentry.constants import MIGRATED_CONDITIONS, SENTRY_APP_ACTIONS, TICKET_ACTIONS
 from sentry.rules import rules
 
 
@@ -32,7 +32,7 @@ class ProjectRulesConfigurationEndpoint(ProjectEndpoint):
             if not can_create_tickets and node.id in TICKET_ACTIONS:
                 continue
 
-            if node.id in SCHEMA_FORM_ACTIONS:
+            if node.id in SENTRY_APP_ACTIONS:
                 custom_actions = node.get_custom_actions(project)
                 if custom_actions:
                     action_list.extend(custom_actions)

src/sentry/api/serializers/rest_framework/rule.py

@@ -3,7 +3,7 @@ from rest_framework import serializers
 from sentry import features
 from sentry.api.fields.actor import ActorField
 from sentry.api.serializers.rest_framework.list import ListField
-from sentry.constants import MIGRATED_CONDITIONS, SCHEMA_FORM_ACTIONS, TICKET_ACTIONS
+from sentry.constants import MIGRATED_CONDITIONS, SENTRY_APP_ACTIONS, TICKET_ACTIONS
 from sentry.models import Environment
 from sentry.rules import rules
 
@@ -34,7 +34,7 @@ class RuleNodeField(serializers.Field):
         node = cls(self.context["project"], data)
 
         # Nodes with user-declared fields will manage their own validation
-        if node.id in SCHEMA_FORM_ACTIONS:
+        if node.id in SENTRY_APP_ACTIONS:
             if not data.get("hasSchemaFormConfig"):
                 raise ValidationError("Please configure your integration settings.")
             node.self_validate()

src/sentry/constants.py

@@ -256,7 +256,7 @@ TICKET_ACTIONS = frozenset(
     ]
 )
 
-SCHEMA_FORM_ACTIONS = frozenset(
+SENTRY_APP_ACTIONS = frozenset(
     ["sentry.rules.actions.notify_event_sentry_app.NotifyEventSentryAppAction"]
 )
 

src/sentry/integrations/slack/notify_action.py

@@ -57,6 +57,9 @@ class SlackNotifyServiceForm(forms.Form):  # type: ignore
         # in the task (integrations/slack/tasks.py) if the channel_id is found.
         self._pending_save = False
 
+    def _format_slack_error_message(self, message: str) -> Any:
+        return _(f"Slack: {message}")
+
     def clean(self) -> dict[str, Any] | None:
         channel_id = (
             self.data.get("inputChannelId")
@@ -73,9 +76,7 @@ class SlackNotifyServiceForm(forms.Form):  # type: ignore
             )
             if not self.data.get("channel"):
                 raise forms.ValidationError(
-                    _(
-                        "Slack channel name is a required field.",
-                    ),
+                    self._format_slack_error_message("Channel name is a required field."),
                     code="invalid",
                 )
             # default to "#" if they have the channel name without the prefix
@@ -95,9 +96,8 @@ class SlackNotifyServiceForm(forms.Form):  # type: ignore
             except ValidationError as e:
                 params = {"channel": self.data.get("channel"), "channel_id": channel_id}
                 raise forms.ValidationError(
-                    _(
-                        str(e),
-                    ),
+                    # ValidationErrors contain a list of error messages, not just one.
+                    self._format_slack_error_message("; ".join(e.messages)),
                     code="invalid",
                     params=params,
                 )
@@ -105,9 +105,7 @@ class SlackNotifyServiceForm(forms.Form):  # type: ignore
             integration = Integration.objects.get(id=workspace)
         except Integration.DoesNotExist:
             raise forms.ValidationError(
-                _(
-                    "Slack workspace is a required field.",
-                ),
+                self._format_slack_error_message("Workspace is a required field."),
                 code="invalid",
             )
 
@@ -129,14 +127,18 @@ class SlackNotifyServiceForm(forms.Form):  # type: ignore
                 params = {"channel": channel, "domain": domain}
 
                 raise forms.ValidationError(
-                    _(
-                        'Multiple users were found with display name "%(channel)s". Please use your username, found at %(domain)s/account/settings#username.',
+                    self._format_slack_error_message(
+                        "Multiple users were found with display name '%(channel)s'. "
+                        "Please use your username, found at %(domain)s/account/settings#username."
                     ),
                     code="invalid",
                     params=params,
                 )
             except ApiRateLimitedError:
-                raise forms.ValidationError(_(SLACK_RATE_LIMITED_MESSAGE))
+                raise forms.ValidationError(
+                    self._format_slack_error_message(SLACK_RATE_LIMITED_MESSAGE),
+                    code="invalid",
+                )
 
         channel = strip_channel_name(channel)
         if channel_id is None and timed_out:
@@ -150,8 +152,8 @@ class SlackNotifyServiceForm(forms.Form):  # type: ignore
                 "workspace": dict(self.fields["workspace"].choices).get(int(workspace)),
             }
             raise forms.ValidationError(
-                _(
-                    'The slack resource "%(channel)s" does not exist or has not been granted access in the %(workspace)s Slack workspace.'
+                self._format_slack_error_message(
+                    'The resource "%(channel)s" does not exist or has not been granted access in the %(workspace)s Slack workspace.'
                 ),
                 code="invalid",
                 params=params,

src/sentry/mediators/alert_rule_actions/creator.py

@@ -1,5 +1,6 @@
 from sentry.coreapi import APIError
 from sentry.mediators import Mediator, Param, external_requests
+from sentry.mediators.external_requests.alert_rule_action_requester import AlertRuleActionResult
 from sentry.models import SentryAppComponent
 from sentry.utils.cache import memoize
 
@@ -8,7 +9,7 @@ class AlertRuleActionCreator(Mediator):
     install = Param("sentry.models.SentryAppInstallation")
     fields = Param(object, default=[])  # array of dicts
 
-    def call(self):
+    def call(self) -> AlertRuleActionResult:
         uri = self._fetch_sentry_app_uri()
         self._make_external_request(uri)
         return self.response

src/sentry/mediators/external_requests/alert_rule_action_requester.py

@@ -1,11 +1,11 @@
 import logging
-from typing import Mapping, Union
+from typing import TypedDict
 from urllib.parse import urlparse, urlunparse
 from uuid import uuid4
 
 from requests import RequestException
+from rest_framework.response import Response
 
-from sentry.http import safe_urlread
 from sentry.mediators import Mediator, Param
 from sentry.mediators.external_requests.util import send_and_save_sentry_app_request
 from sentry.utils import json
@@ -17,6 +17,11 @@ DEFAULT_SUCCESS_MESSAGE = "Success!"
 DEFAULT_ERROR_MESSAGE = "Something went wrong!"
 
 
+class AlertRuleActionResult(TypedDict):
+    success: bool
+    message: str
+
+
 class AlertRuleActionRequester(Mediator):
     """
     Makes a POST request to another service to fetch/update the values for each field in the
@@ -36,9 +41,22 @@ class AlertRuleActionRequester(Mediator):
         urlparts[2] = self.uri
         return urlunparse(urlparts)
 
-    def _make_request(self) -> Mapping[str, Union[bool, str]]:
+    def _get_response_message(self, response: Response, default_message: str) -> str:
+        """
+        Returns the message from the response body, if in the expected location.
+        Used to bubble up info from the Sentry App to the UI.
+        The location should be coordinated with the docs on Alert Rule Action UI Components.
+        """
         try:
-            request = send_and_save_sentry_app_request(
+            message = response.json().get("message", default_message)
+        except Exception:
+            message = default_message
+
+        return f"{self.sentry_app.name}: {message}"
+
+    def _make_request(self) -> AlertRuleActionResult:
+        try:
+            response = send_and_save_sentry_app_request(
                 self._build_url(),
                 self.sentry_app,
                 self.install.organization_id,
@@ -57,16 +75,12 @@ class AlertRuleActionRequester(Mediator):
                     "error_message": str(e),
                 },
             )
-            # NOTE: bool(e.response) is always False because non-2xx responses are Falsey.
-            text = e.response.text if e.response is not None else None
-            message = f"{self.sentry_app.name}: {text or DEFAULT_ERROR_MESSAGE}"
-            # Bubble up error message from Sentry App to the UI for the user.
-            return {"success": False, "message": message}
-
-        body_raw = safe_urlread(request)
-        body = body_raw.decode() if body_raw else None
-        message = f"{self.sentry_app.name}: {body or DEFAULT_SUCCESS_MESSAGE}"
-        return {"success": True, "message": message}
+            return AlertRuleActionResult(
+                success=False, message=self._get_response_message(e.response, DEFAULT_ERROR_MESSAGE)
+            )
+        return AlertRuleActionResult(
+            success=True, message=self._get_response_message(response, DEFAULT_SUCCESS_MESSAGE)
+        )
 
     def _build_headers(self):
         request_uuid = uuid4().hex

tests/sentry/incidents/endpoints/test_serializers.py

@@ -997,14 +997,17 @@ class TestAlertRuleTriggerActionSerializer(TestCase):
 
     @responses.activate
     def test_sentry_app_action_creator_fails(self):
+        error_message = "Invalid channel."
         responses.add(
             method=responses.POST,
             url="https://example.com/sentry/alert-rule",
             status=400,
-            body="Invalid channel.",
+            json={"message": error_message},
         )
-        self.run_fail_validation_test(
-            {
+
+        serializer = AlertRuleTriggerActionSerializer(
+            context=self.context,
+            data={
                 "type": AlertRuleTriggerAction.get_registered_type(
                     AlertRuleTriggerAction.Type.SENTRY_APP
                 ).slug,
@@ -1016,8 +1019,10 @@ class TestAlertRuleTriggerActionSerializer(TestCase):
                 "sentry_app_config": {"channel": "#santry"},
                 "sentry_app_installation_uuid": self.sentry_app_installation.uuid,
             },
-            {"sentryApp": ["Super Awesome App: Invalid channel."]},
         )
+        assert not serializer.is_valid()
+        error = serializer.errors.get("sentryApp")[0]
+        assert str(error) == f"Super Awesome App: {error_message}"
 
     @responses.activate
     def test_create_and_update_sentry_app_action_success(self):

tests/sentry/integrations/slack/test_notify_action.py

@@ -312,7 +312,7 @@ class SlackNotifyActionTest(RuleTestCase):
 
         form = rule.get_form_instance()
         assert not form.is_valid()
-        assert ["Slack workspace is a required field."] in form.errors.values()
+        assert ["Slack: Workspace is a required field."] in form.errors.values()
 
     @responses.activate
     def test_display_name_conflict(self):
@@ -347,7 +347,7 @@ class SlackNotifyActionTest(RuleTestCase):
         form = rule.get_form_instance()
         assert not form.is_valid()
         assert [
-            'Multiple users were found with display name "@morty". Please use your username, found at sentry.slack.com/account/settings#username.'
+            "Slack: Multiple users were found with display name '@morty'. Please use your username, found at sentry.slack.com/account/settings#username."
         ] in form.errors.values()
 
     def test_disabled_org_integration(self):

tests/sentry/mediators/external_requests/test_alert_rule_action_requester.py

@@ -1,6 +1,10 @@
 import responses
 
 from sentry.mediators.external_requests import AlertRuleActionRequester
+from sentry.mediators.external_requests.alert_rule_action_requester import (
+    DEFAULT_ERROR_MESSAGE,
+    DEFAULT_SUCCESS_MESSAGE,
+)
 from sentry.testutils import TestCase
 from sentry.utils import json
 from sentry.utils.sentryappwebhookrequests import SentryAppWebhookRequestsBuffer
@@ -33,6 +37,8 @@ class TestAlertRuleActionRequester(TestCase):
             {"name": "description", "value": "threshold reached"},
             {"name": "assignee_id", "value": "user-1"},
         ]
+        self.error_message = "Channel not found!"
+        self.success_message = "Created alert!"
 
     @responses.activate
     def test_makes_successful_request(self):
@@ -41,7 +47,6 @@ class TestAlertRuleActionRequester(TestCase):
             method=responses.POST,
             url="https://example.com/sentry/alert-rule",
             status=200,
-            json="Saved information",
         )
 
         result = AlertRuleActionRequester.run(
@@ -50,7 +55,7 @@ class TestAlertRuleActionRequester(TestCase):
             fields=self.fields,
         )
         assert result["success"]
-        assert result["message"] == 'foo: "Saved information"'
+        assert result["message"] == f"{self.sentry_app.name}: {DEFAULT_SUCCESS_MESSAGE}"
         request = responses.calls[0].request
 
         data = {
@@ -78,6 +83,39 @@ class TestAlertRuleActionRequester(TestCase):
         assert requests[0]["response_code"] == 200
         assert requests[0]["event_type"] == "alert_rule_action.requested"
 
+    @responses.activate
+    def test_makes_successful_request_with_message(self):
+        responses.add(
+            method=responses.POST,
+            url="https://example.com/sentry/alert-rule",
+            status=200,
+            json={"message": self.success_message},
+        )
+
+        result = AlertRuleActionRequester.run(
+            install=self.install,
+            uri="/sentry/alert-rule",
+            fields=self.fields,
+        )
+        assert result["success"]
+        assert result["message"] == f"{self.sentry_app.name}: {self.success_message}"
+
+    @responses.activate
+    def test_makes_successful_request_with_malformed_message(self):
+        responses.add(
+            method=responses.POST,
+            url="https://example.com/sentry/alert-rule",
+            status=200,
+            body=bytes(self.success_message, encoding="utf-8"),
+        )
+        result = AlertRuleActionRequester.run(
+            install=self.install,
+            uri="/sentry/alert-rule",
+            fields=self.fields,
+        )
+        assert result["success"]
+        assert result["message"] == f"{self.sentry_app.name}: {DEFAULT_SUCCESS_MESSAGE}"
+
     @responses.activate
     def test_makes_failed_request(self):
 
@@ -85,7 +123,6 @@ class TestAlertRuleActionRequester(TestCase):
             method=responses.POST,
             url="https://example.com/sentry/alert-rule",
             status=401,
-            json="Channel not found!",
         )
 
         result = AlertRuleActionRequester.run(
@@ -94,7 +131,7 @@ class TestAlertRuleActionRequester(TestCase):
             fields=self.fields,
         )
         assert not result["success"]
-        assert result["message"] == 'foo: "Channel not found!"'
+        assert result["message"] == f"{self.sentry_app.name}: {DEFAULT_ERROR_MESSAGE}"
         request = responses.calls[0].request
 
         data = {
@@ -123,12 +160,12 @@ class TestAlertRuleActionRequester(TestCase):
         assert requests[0]["event_type"] == "alert_rule_action.requested"
 
     @responses.activate
-    def test_makes_failed_request_returning_only_status(self):
-
+    def test_makes_failed_request_with_message(self):
         responses.add(
             method=responses.POST,
             url="https://example.com/sentry/alert-rule",
             status=401,
+            json={"message": self.error_message},
         )
 
         result = AlertRuleActionRequester.run(
@@ -137,30 +174,20 @@ class TestAlertRuleActionRequester(TestCase):
             fields=self.fields,
         )
         assert not result["success"]
-        assert result["message"] == "foo: Something went wrong!"
-        request = responses.calls[0].request
+        assert result["message"] == f"{self.sentry_app.name}: {self.error_message}"
 
-        data = {
-            "fields": [
-                {"name": "title", "value": "An Alert"},
-                {"name": "description", "value": "threshold reached"},
-                {
-                    "name": "assignee_id",
-                    "value": "user-1",
-                },
-            ],
-            "installationId": self.install.uuid,
-        }
-        payload = json.loads(request.body)
-        assert payload == data
-
-        assert request.headers["Sentry-App-Signature"] == self.sentry_app.build_signature(
-            json.dumps(payload)
+    @responses.activate
+    def test_makes_failed_request_with_malformed_message(self):
+        responses.add(
+            method=responses.POST,
+            url="https://example.com/sentry/alert-rule",
+            status=401,
+            body=bytes(self.error_message, encoding="utf-8"),
         )
-
-        buffer = SentryAppWebhookRequestsBuffer(self.sentry_app)
-        requests = buffer.get_requests()
-
-        assert len(requests) == 1
-        assert requests[0]["response_code"] == 401
-        assert requests[0]["event_type"] == "alert_rule_action.requested"
+        result = AlertRuleActionRequester.run(
+            install=self.install,
+            uri="/sentry/alert-rule",
+            fields=self.fields,
+        )
+        assert not result["success"]
+        assert result["message"] == f"{self.sentry_app.name}: {DEFAULT_ERROR_MESSAGE}"