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

fix(ui): fixed dashboard cells and check query builder update bug #17202

Merged
merged 3 commits into from
Mar 11, 2020

Conversation

asalem1
Copy link
Contributor

@asalem1 asalem1 commented Mar 11, 2020

Closes #17130

Problem

  1. Updates made to existing Checks were not registering the changes
  2. Creating cells with Group as the aggregate function type were not maintaining the aggregate type

Solution

Updated the BuilderConfig tag struct to account for the AggregateFunctionType that had been passed in by the UI but was not being stored on the API. The consequence of this action allows the UI to have access to the aggregateFunctionType as a reference for making changes in the query builder. Put simply, the existing query builder is built around the fact that an aggregateFunctionType will be passed in as a tag property, but it was not.

Check Update Fix:
check_update_fix

Dashboard Cell Fix:
dashboard_cell_fix

  • CHANGELOG.md updated with a link to the PR (not the Issue)

@asalem1 asalem1 requested review from gavincabbage and a team March 11, 2020 16:34
@asalem1 asalem1 requested a review from a team as a code owner March 11, 2020 16:34
@asalem1 asalem1 requested review from hoorayimhelping and imogenkinsman and removed request for a team March 11, 2020 16:34
@asalem1 asalem1 requested review from a team and drdelambre and removed request for hoorayimhelping and a team March 11, 2020 17:04
@asalem1 asalem1 force-pushed the fix/check-update-bug branch from fbc67dd to b5ea518 Compare March 11, 2020 17:30
@@ -2638,7 +2638,7 @@ func (q queries) influxDashQueries() []influxdb.DashboardQuery {
EditMode: "advanced",
}
// TODO: axe this builder configs when issue https://github.com/influxdata/influxdb/issues/15708 is fixed up
Copy link
Contributor

Choose a reason for hiding this comment

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

anyone else curious about this foreboding line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, definitely shows it to @gavincabbage this morning, especially since the issue was closed a couple days ago. I'd love to see @desa 's opinion on this

Copy link
Contributor

@drdelambre drdelambre left a comment

Choose a reason for hiding this comment

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

js lgtm

@asalem1 asalem1 force-pushed the fix/check-update-bug branch from b5ea518 to 1970b74 Compare March 11, 2020 20:49
@asalem1 asalem1 merged commit d2a49b8 into master Mar 11, 2020
@asalem1 asalem1 deleted the fix/check-update-bug branch March 11, 2020 21:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can't change selected field in existing monitor and alerting check on Cloud 2
5 participants