-
Notifications
You must be signed in to change notification settings - Fork 13.8k
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(migrations): Fix the time comparison migration #30029
fix(migrations): Fix the time comparison migration #30029
Conversation
…on can be upgraded properly - Add extra tests to cover or new cases
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #30029 +/- ##
===========================================
+ Coverage 60.48% 83.68% +23.20%
===========================================
Files 1931 528 -1403
Lines 76236 38207 -38029
Branches 8568 0 -8568
===========================================
- Hits 46114 31975 -14139
+ Misses 28017 6232 -21785
+ Partials 2105 0 -2105
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
Thanks for fixing this!
@@ -161,6 +161,70 @@ | |||
} | |||
], | |||
} | |||
params_v1_other_than_custom_false: dict[str, Any] = { |
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.
Would you mind extracting the common parts between params_v1_with_custom
, params_v1_other_than_custom_false
, params_v2_with_custom
, params_v2_other_than_custom
, params_v2_other_than_custom_false
and just override the necessary properties for each variable to avoid code duplication and increase readability?
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.
Thanks for addressing the comments @Antonio-RiveroMartnez!
(cherry picked from commit d80f23e)
SUMMARY
After the initial version of the time comparison feature for Table and Big Number we introduced a migration to upgrade old charts to the new comparison controls, however, during that upgrade a table that never had comparison in it was wrongly showing Time Comparison Data.
This PR is fixing the migration so we can properly upgrade such old tables if needed, also we extend our tests to not only consider charts that have the enabled_time_comparison flag set to True.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
BEFORE
Old version of Chart using Time Comparison once but then removing it from the controls:
The old Chart is upgraded with the previous version of the migration:
The chart start showing comparison data:
AFTER
Old version of Chart using Time Comparison once but then removing it from the controls:
The old Chart is upgraded with the new version of the migration:
The chart doesn't show comparison data:
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION