-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
fix(aci): filter out non-alertable and broken sentry apps in available actions endpoint #101866
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -206,6 +206,32 @@ class SentryAppActionHandler(ActionHandler): | |
| slug=self.sentry_app.slug, organization=self.organization | ||
| ) | ||
|
|
||
| # should not return sentry apps that are not alertable | ||
| self.not_alertable_sentry_app = self.create_sentry_app( | ||
| name="Not Alertable Sentry App", | ||
| organization=self.organization, | ||
| is_alertable=False, | ||
| ) | ||
| self.not_alertable_sentry_app_installation = self.create_sentry_app_installation( | ||
| slug=self.not_alertable_sentry_app.slug, organization=self.organization | ||
| ) | ||
|
|
||
| self.not_alertable_sentry_app = self.create_sentry_app( | ||
| name="Not Alertable Sentry App With Component", | ||
| organization=self.organization, | ||
| schema={ | ||
| "elements": [ | ||
| self.sentry_app_settings_schema, | ||
| ] | ||
| }, | ||
| is_alertable=False, | ||
| ) | ||
| self.not_alertable_sentry_app_with_component_installation = ( | ||
| self.create_sentry_app_installation( | ||
| slug=self.not_alertable_sentry_app.slug, organization=self.organization | ||
| ) | ||
| ) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: Sentry App Overwrite Creates Orphan InstallationThe |
||
|
|
||
| # should not return sentry apps that are not installed | ||
| self.create_sentry_app( | ||
| name="Bad Sentry App", | ||
|
|
@@ -388,6 +414,28 @@ def test_sentry_apps(self, mock_sentry_app_component_preparer: MagicMock) -> Non | |
| }, | ||
| ] | ||
|
|
||
| @patch( | ||
| "sentry.workflow_engine.endpoints.organization_available_action_index.prepare_ui_component" | ||
| ) | ||
| def test_sentry_apps_filters_failed_component_preparation( | ||
| self, mock_prepare_ui_component: MagicMock | ||
| ) -> None: | ||
| """Test that sentry apps whose components fail to prepare are filtered out""" | ||
| self.setup_sentry_apps() | ||
|
|
||
| # make prepare_ui_component return None to simulate a broken app | ||
| mock_prepare_ui_component.return_value = None | ||
|
|
||
| response = self.get_success_response( | ||
| self.organization.slug, | ||
| status_code=200, | ||
| ) | ||
|
|
||
| # verify prepare_ui_component was called | ||
| assert mock_prepare_ui_component.called | ||
| # should return no sentry apps since component preparation failed | ||
| assert len(response.data) == 0 | ||
|
|
||
| def test_webhooks(self) -> None: | ||
| self.setup_webhooks() | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this component preparation code make any network requests at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Summarizing a conversation we had in Slack:
This potentially calls the downstream Sentry App's endpoint, which can add a ton of additional latency, or fallibility.
This is ok for now, but we should consider moving this call into its own API, and letting the UI make this call on-demand to retrieve just the UI component that it needs when the user selects the Sentry app as a target and has to configure it.
Additionally, this call is repeated in the serializer below, so we're removing that other call and hoisting it here for now while @ameliahsu works on the API and UI changes.