Skip to content

Conversation

@edwardgou-sentry
Copy link
Contributor

Fixed a bug in the Add to Dashboards modal that prevented the user from deleting the last non equation overlay even if there is an existing equation on the Y-Axis
image

@edwardgou-sentry edwardgou-sentry requested a review from a team October 22, 2021 21:58
@edwardgou-sentry edwardgou-sentry requested a review from a team as a code owner October 22, 2021 21:58
@github-actions
Copy link
Contributor

github-actions bot commented Oct 22, 2021

size-limit report

Path Base Size (03af072) Current Size Change
src/sentry/static/sentry/dist/entrypoints/app.js 52.74 KB 52.74 KB -0.01% 🔽
src/sentry/static/sentry/dist/entrypoints/sentry.css 70.89 KB 70.89 KB 0%

@edwardgou-sentry
Copy link
Contributor Author

Updated test_invalid_equation query to equation|count() since the previous equation|count() * 2 is no longer invalid after this change. equation|count() is invalid since it contains only one term

if equations is not None:
try:
resolved_equations, _, _ = resolve_equation_list(equations, fields)
resolved_equations, _, _ = resolve_equation_list(equations, fields, auto_add=True)
Copy link
Member

Choose a reason for hiding this comment

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

We should only do auto_add here if its a chart widget, otherwise we'll break the validation for table widgets

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @wmak, not sure if there is a better way to do this, but I added displayType to the context so I could access it within the Query serializer to conditionally auto_add. Is there a better way for a nested serializer to access properties from parents or siblings?

Comment on lines 77 to 80
auto_add=self.context.get("displayType")
!= DashboardWidgetDisplayTypes.as_text_choices()[
DashboardWidgetDisplayTypes.TABLE
][0],
Copy link
Member

Choose a reason for hiding this comment

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

Let's move this to a helper function, but otherwise this looks good to me 👍
oh, we should set aggregates_only=True as well if its a chart

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 updated!

@edwardgou-sentry edwardgou-sentry merged commit 47cc556 into master Oct 27, 2021
@edwardgou-sentry edwardgou-sentry deleted the fix/discover-add-to-dashboard-delete-overlay branch October 27, 2021 20:02
@github-actions github-actions bot locked and limited conversation to collaborators Nov 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants