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
[Bug Fix] Returning timeseries_limit_metric in table viz get_data #9196
[Bug Fix] Returning timeseries_limit_metric in table viz get_data #9196
Conversation
Codecov Report
@@ Coverage Diff @@
## master #9196 +/- ##
==========================================
- Coverage 58.93% 58.91% -0.03%
==========================================
Files 372 372
Lines 11987 11992 +5
Branches 2936 2936
==========================================
Hits 7065 7065
- Misses 4742 4749 +7
+ Partials 180 178 -2
Continue to review full report at Codecov.
|
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.
@michellethomas apologies for the regression. Do you think it would be possible to augment the test case to add the timeseries limit metric?
@john-bodley also needed to fix a similar issue with the include_time field too. I added a test for the timeseries_limit_metric. I'll add a test for include_time in a follow up PR. |
|
||
non_percent_metric_columns.extend( | ||
utils.get_metric_names(self.form_data.get("metrics") or []) | ||
) |
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.
How are lines 588 - 594 different to what was previously on lines 584 - 586?
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.
I'm just extending them separately because it feels a bit more readable, rather than extending list + list.
CATEGORY
Choose one
SUMMARY
This change update the table viz's get_data, but was missing returning the timeseries_limit_metric (sort_by). If a metric has a custom label and a sort by field, not returning the sort_by field doesn't show the metric.
TEST PLAN
Tested a metric with a label and a sort by field
ADDITIONAL INFORMATION
REVIEWERS
@john-bodley