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

feat(dashboards): POC for sharing dashboards #82455

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

harshithadurai
Copy link
Contributor

@harshithadurai harshithadurai commented Dec 20, 2024

DRAFT: POC for sharing dashboards

@github-actions github-actions bot added Scope: Frontend Automatically applied to PRs that change frontend components Scope: Backend Automatically applied to PRs that change backend components labels Dec 20, 2024
Copy link
Contributor

🚨 Warning: This pull request contains Frontend and Backend changes!

It's discouraged to make changes to Sentry's Frontend and Backend in a single pull request. The Frontend and Backend are not atomically deployed. If the changes are interdependent of each other, they must be separated into two pull requests and be made forward or backwards compatible, such that the Backend or Frontend can be safely deployed independently.

Have questions? Please ask in the #discuss-dev-infra channel.

except Organization.DoesNotExist:
return Response({"detail": "Organization not found"}, status=404)
except Exception as e:
return Response({"error": str(e)}, status=400)

Check warning

Code scanning / CodeQL

Information exposure through an exception Medium

Stack trace information
flows to this location and may be exposed to an external user.

Copilot Autofix AI 22 days ago

To fix the problem, we should log the detailed exception message on the server and return a generic error message to the user. This approach ensures that sensitive information is not exposed to the user while still allowing developers to access the necessary details for debugging.

  • Modify the exception handling block to log the exception message and stack trace.
  • Return a generic error message to the user instead of the actual exception message.
Suggested changeset 1
src/sentry/api/endpoints/shared_widget_details.py

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/src/sentry/api/endpoints/shared_widget_details.py b/src/sentry/api/endpoints/shared_widget_details.py
--- a/src/sentry/api/endpoints/shared_widget_details.py
+++ b/src/sentry/api/endpoints/shared_widget_details.py
@@ -94,3 +94,4 @@
         except Exception as e:
-            return Response({"error": str(e)}, status=400)
+            self.logger.error("An error occurred: %s", str(e), exc_info=True)
+            return Response({"error": "An internal error has occurred."}, status=400)
 
EOF
@@ -94,3 +94,4 @@
except Exception as e:
return Response({"error": str(e)}, status=400)
self.logger.error("An error occurred: %s", str(e), exc_info=True)
return Response({"error": "An internal error has occurred."}, status=400)

Copilot is powered by AI and may make mistakes. Always verify output.
Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
Copy link

codecov bot commented Dec 20, 2024

❌ 8 Tests Failed:

Tests completed Failed Passed Skipped
31777 8 31769 213
View the top 3 failed tests by shortest run time
buildRoutes() orgId routes also have domain routes
Stack Traces | 0.064s run time
Error: Unable to find matching URLs for the following 1 routes:

- slug: /organizations/:orgId/shared/dashboard/:shareId/
  domain: /shared/dashboard/:shareId/

Each route with the :orgId parameter is expected to have corresponding domain based route as well. If you need help with this drop by #proj-hybrid-cloud.
    at Object.<anonymous> (.../static/app/routes.spec.tsx:102:13)
    at Promise.then.completed (.../jest-circus/build/utils.js:298:28)
    at new Promise (<anonymous>)
    at callAsyncCircusFn (.../jest-circus/build/utils.js:231:10)
    at _callCircusTest (.../jest-circus/build/run.js:316:40)
    at processTicksAndRejections (node:internal/process/task_queues:105:5)
    at _runTest (.../jest-circus/build/run.js:252:3)
    at _runTestsForDescribeBlock (.../jest-circus/build/run.js:126:9)
    at _runTestsForDescribeBlock (.../jest-circus/build/run.js:121:9)
    at run (.../jest-circus/build/run.js:71:3)
    at runAndTransformResultsToJestFormat (.../build/legacy-code-todo-rewrite/jestAdapterInit.js:122:21)
    at jestAdapter (.../build/legacy-code-todo-rewrite/jestAdapter.js:79:19)
    at runTestInternal (.../jest-runner/build/runTest.js:367:16)
    at runTest (.../jest-runner/build/runTest.js:444:34)
    at Object.worker (.../jest-runner/build/testWorker.js:106:12)
Dashboards > Detail custom dashboards disables edit dashboard and add widget button if user cannot edit dashboard
Stack Traces | 1.23s run time
Error: Found multiple elements with the text: Edit Access:

Here are the matching elements:

Ignored nodes: comments, script, style
...

Ignored nodes: comments, script, style
...

(If this is intentional, then use the `*AllBy*` variant of the query (like `queryAllByText`, `getAllByText`, or `findAllByText`)).

Ignored nodes: comments, script, style
...
    at waitForWrapper (.../sentry/sentry/node_modules/@.../dom/dist/wait-for.js:163:27)
    at .../sentry/sentry/node_modules/@.../dom/dist/query-helpers.js:86:33
    at Object.<anonymous> (.../views/dashboards/detail.spec.tsx:1835:41)
    at Promise.then.completed (.../sentry/sentry/node_modules/jest-circus/build/utils.js:298:28)
    at new Promise (<anonymous>)
    at callAsyncCircusFn (.../sentry/sentry/node_modules/jest-circus/build/utils.js:231:10)
    at _callCircusTest (.../sentry/sentry/node_modules/jest-circus/build/run.js:316:40)
    at _runTest (.../sentry/sentry/node_modules/jest-circus/build/run.js:252:3)
    at _runTestsForDescribeBlock (.../sentry/sentry/node_modules/jest-circus/build/run.js:126:9)
    at _runTestsForDescribeBlock (.../sentry/sentry/node_modules/jest-circus/build/run.js:121:9)
    at _runTestsForDescribeBlock (.../sentry/sentry/node_modules/jest-circus/build/run.js:121:9)
    at run (.../sentry/sentry/node_modules/jest-circus/build/run.js:71:3)
    at runAndTransformResultsToJestFormat (.../sentry/sentry/node_modules/jest-circus/build/legacy-code-todo-rewrite/jestAdapterInit.js:122:21)
    at jestAdapter (.../sentry/sentry/node_modules/jest-circus/build/legacy-code-todo-rewrite/jestAdapter.js:79:19)
    at runTestInternal (.../sentry/sentry/node_modules/jest-runner/build/runTest.js:367:16)
    at runTest (.../sentry/sentry/node_modules/jest-runner/build/runTest.js:444:34)
    at Object.worker (.../sentry/sentry/node_modules/jest-runner/build/testWorker.js:106:12)
Dashboards > Detail custom dashboards creator can update permissions for dashboard
Stack Traces | 1.24s run time
Error: Found multiple elements with the text: Edit Access:

Here are the matching elements:

Ignored nodes: comments, script, style
...

Ignored nodes: comments, script, style
...

(If this is intentional, then use the `*AllBy*` variant of the query (like `queryAllByText`, `getAllByText`, or `findAllByText`)).

Ignored nodes: comments, script, style
...
    at waitForWrapper (.../sentry/sentry/node_modules/@.../dom/dist/wait-for.js:163:27)
    at .../sentry/sentry/node_modules/@.../dom/dist/query-helpers.js:86:33
    at Object.<anonymous> (.../views/dashboards/detail.spec.tsx:1672:84)
    at Promise.then.completed (.../sentry/sentry/node_modules/jest-circus/build/utils.js:298:28)
    at new Promise (<anonymous>)
    at callAsyncCircusFn (.../sentry/sentry/node_modules/jest-circus/build/utils.js:231:10)
    at _callCircusTest (.../sentry/sentry/node_modules/jest-circus/build/run.js:316:40)
    at _runTest (.../sentry/sentry/node_modules/jest-circus/build/run.js:252:3)
    at _runTestsForDescribeBlock (.../sentry/sentry/node_modules/jest-circus/build/run.js:126:9)
    at _runTestsForDescribeBlock (.../sentry/sentry/node_modules/jest-circus/build/run.js:121:9)
    at _runTestsForDescribeBlock (.../sentry/sentry/node_modules/jest-circus/build/run.js:121:9)
    at run (.../sentry/sentry/node_modules/jest-circus/build/run.js:71:3)
    at runAndTransformResultsToJestFormat (.../sentry/sentry/node_modules/jest-circus/build/legacy-code-todo-rewrite/jestAdapterInit.js:122:21)
    at jestAdapter (.../sentry/sentry/node_modules/jest-circus/build/legacy-code-todo-rewrite/jestAdapter.js:79:19)
    at runTestInternal (.../sentry/sentry/node_modules/jest-runner/build/runTest.js:367:16)
    at runTest (.../sentry/sentry/node_modules/jest-runner/build/runTest.js:444:34)
    at Object.worker (.../sentry/sentry/node_modules/jest-runner/build/testWorker.js:106:12)

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

@getsantry
Copy link
Contributor

getsantry bot commented Jan 11, 2025

This pull request has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you add the label WIP, I will leave it alone unless WIP is removed ... forever!


"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

@getsantry getsantry bot added the Stale label Jan 11, 2025
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 Scope: Frontend Automatically applied to PRs that change frontend components Stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant