-
Notifications
You must be signed in to change notification settings - Fork 13.7k
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(chart): Resolve incorrect column customization when switching metrics in table chart #26393
fix(chart): Resolve incorrect column customization when switching metrics in table chart #26393
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #26393 +/- ##
==========================================
+ Coverage 69.14% 69.15% +0.01%
==========================================
Files 1946 1946
Lines 75989 75991 +2
Branches 8479 8479
==========================================
+ Hits 52544 52553 +9
+ Misses 21266 21257 -9
- Partials 2179 2181 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Thanks @soniagtm for the fix. Would you mind adding a unit test to ensure we don't regress in the future? |
Hi, I've added test cases for switching metric columns and changing their names. I'm not sure if this is the correct way to do so. Please let me know if any corrections or further additions are needed. Thank you in advance! |
@justinpark would you mind reviewing this? |
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.
LGTM. Thanks for adding test specs
…rics in table chart (apache#26393) Co-authored-by: Sonia <sonia.gautam@agoda.com> (cherry picked from commit dfde2ad)
…rics in table chart (apache#26393) Co-authored-by: Sonia <sonia.gautam@agoda.com>
SUMMARY
I have found that when attempting to swap the positions of metrics in the table chart with applied column configuration, the column configuration is switched to another column, which is related to the addition of #19841 for maintaining configurations when metrics are renamed. To address this issue, I have added a condition to first check if the column name is found in the old metrics. If it is found, it will avoid updating the column configuration. Reassignment will only occur if the new column name is not found among the existing ones to maintain configurations when metrics are renamed.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before: Currency prefix is switched to SUM(quantity_ordered)
Before.mov
After: Currency prefix is applied to SUM(sales) as expected
After.mov
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION