Skip to content
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

chore(sentry apps): Audit & use new errors for external req processes & endpoints #83077

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

Christinarlong
Copy link
Contributor

Title

Basically go through all the errors in external request (i.e when we make a req to the third party app for some data) processes and label SentryAppError or SentryAppIntegratorError.

For endpoints I went through and instead of returning a response for errors, we raise the error again and our base class error handler will deal with the error for us.

@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Jan 8, 2025
Copy link

codecov bot commented Jan 8, 2025

❌ 1 Tests Failed:

Tests completed Failed Passed Skipped
23459 1 23458 250
View the top 1 failed tests by shortest run time
tests.sentry.sentry_apps.api.endpoints.test_sentry_app_installation_external_issue_actions.SentryAppInstallationExternalIssuesEndpointTest::test_external_issue_doesnt_get_created
Stack Traces | 6.16s run time
#x1B[1m#x1B[.../api/endpoints/test_sentry_app_installation_external_issue_actions.py#x1B[0m:79: in test_external_issue_doesnt_get_created
    assert (
#x1B[1m#x1B[31mE   assert b'{"detail":"... link issue"}' == b'{"error":"I... link issue"}'#x1B[0m
#x1B[1m#x1B[31mE     #x1B[0m
#x1B[1m#x1B[31mE     At index 2 diff: b'd' != b'e'#x1B[0m
#x1B[1m#x1B[31mE     #x1B[0m
#x1B[1m#x1B[31mE     Full diff:#x1B[0m
#x1B[1m#x1B[31mE     - (b'{"error":"Issue occured while trying to contact testin to link issue"}')#x1B[0m
#x1B[1m#x1B[31mE     ?       ^^^^#x1B[0m
#x1B[1m#x1B[31mE     + (b'{"detail":"Issue occured while trying to contact testin to link issue"}')#x1B[0m
#x1B[1m#x1B[31mE     ?      + ^^^^#x1B[0m

To view more test analytics, go to the Test Analytics Dashboard
📢 Thoughts on this report? Let us know!

@Christinarlong Christinarlong marked this pull request as ready for review January 8, 2025 23:40
@Christinarlong Christinarlong requested review from a team as code owners January 8, 2025 23:40
Copy link
Member

@iamrajjoshi iamrajjoshi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some nits and a question

@@ -45,7 +44,10 @@ def post(self, request: Request, installation) -> Response:
data = request.data.copy()

if not {"groupId", "action", "uri"}.issubset(data.keys()):
return Response(status=400)
raise SentryAppError(
ValidationError('"groupId", "action", and "uri" must be present in request'),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: i would either use group_id or groupID, same everywhere else

f"Invalid response format from sentry app {self.sentry_app} when linking issue"
raise SentryAppIntegratorError(
ValidationError(
f"Invalid response format from sentry app {self.sentry_app} when linking issue"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lets use integration instead of sentry app, not sure if we mention sentry app externally in docs.

f"Something went wrong while getting SelectFields from {self.sentry_app.slug}"
raise SentryAppIntegratorError(
APIError(
f"Something went wrong while getting SelectFields from {self.sentry_app.slug}"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do integrators know what SelectFields is or is it something inside our code?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe ? The most accurate name is Select FormField so we could use that instead

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Scope: Backend Automatically applied to PRs that change backend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants