-
Notifications
You must be signed in to change notification settings - Fork 13.6k
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
[pie-chart] Restricting query to single metric #5203
[pie-chart] Restricting query to single metric #5203
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5203 +/- ##
==========================================
+ Coverage 63.39% 63.45% +0.06%
==========================================
Files 261 261
Lines 19838 19838
Branches 1995 1995
==========================================
+ Hits 12577 12589 +12
+ Misses 7252 7240 -12
Partials 9 9
Continue to review full report at Codecov.
|
LGTM 🚢 |
I think it used to allow multiple metrics or one metric along with a groupby. People may have metrics like |
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.
Also I'm pretty sure this will break existing charts. I think this would require a db migration moving the form_data from metrics
to metric
and selecting the first position of the array
@mistercrunch the pie-chart simply uses the first metric (so it works for It seems there are multiple metric-keys supported, including Note @mistercrunch I'm not sure what the policy is around form-data attributes but for the migration I simply remove the attribute (to prevent bloat) rather than storing |
eba98f5
to
1751fa5
Compare
1751fa5
to
11bfdbc
Compare
LGTM, sorry about the confusion. About how to deal with empty form_data or form_data key that match their default value is we should save all of the keys defined for that chart type at that point in time. That way if the default value changes the chart would still look the same. In some cases, In this case here there should always be one metric so the db migration is fine. |
(cherry picked from commit 8cdc9ca)
The pie chart should only allow specifying one metric in the UI. It always uses the first metric if multiple are specified, though this PR simply enforces the single metric UI component.
to: @williaster @GabeLoins @mistercrunch