-
Notifications
You must be signed in to change notification settings - Fork 13.5k
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
feat: Migrates Pivot Table v1 to v2 #23712
feat: Migrates Pivot Table v1 to v2 #23712
Conversation
42296d0
to
fb3a4ec
Compare
Codecov Report
@@ Coverage Diff @@
## master #23712 +/- ##
==========================================
+ Coverage 68.33% 68.37% +0.04%
==========================================
Files 1957 1957
Lines 75628 75549 -79
Branches 8225 8225
==========================================
- Hits 51680 51658 -22
+ Misses 21835 21778 -57
Partials 2113 2113
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 2 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
@michael-s-molina same comment as in #23741; please set 🔥 to |
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 adding this. So happy to see the removal of legacy visualizations.
superset/migrations/versions/2023-04-14_11-10_9ba2ce3086e5_migrate_pivot_table_v1_to_v2.py
Outdated
Show resolved
Hide resolved
f7fb7c8
to
dad6109
Compare
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.
Tested on my local environment, LGTM!
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.
Very solid work, highly appreciate this! ❤️ LGTM!
@@ -24,11 +11,24 @@ | |||
"analysis", | |||
"data" | |||
], | |||
"license": "Apache-2.0", | |||
"homepage": "https://github.com/apache-superset/superset-ui#readme", |
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.
bycatch: since that repo is archived, I wonder if we should just point these to the main repo?
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.
Thank you!!!
9187061
to
0f5a741
Compare
0f5a741
to
98523e6
Compare
8790a85
to
022d554
Compare
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.
Could we update all the translation files in a separated pull request?
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.
It would be possible but the idea was to make the PR atomic by removing the all legacy-related artifacts.
def with_feature_flags(**mock_feature_flags): | ||
""" | ||
Use this decorator to mock feature flags in tests.integration_tests. | ||
|
||
Usage: | ||
|
||
class TestYourFeature(SupersetTestCase): | ||
|
||
@with_feature_flags(YOUR_FEATURE=True) | ||
def test_your_feature_enabled(self): | ||
self.assertEqual(is_feature_enabled("YOUR_FEATURE"), True) | ||
|
||
@with_feature_flags(YOUR_FEATURE=False) | ||
def test_your_feature_disabled(self): | ||
self.assertEqual(is_feature_enabled("YOUR_FEATURE"), False) | ||
""" | ||
|
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.
The decorator just mocked a single FF item, in order to support more than 1 FF item, I suggest to provide a DICT structure as a parameter.
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.
Lets tackle this in a follow-up PR given that this is a copy/paste of the decorator found in the integration tests.
184a21b
to
2c86c6d
Compare
2c86c6d
to
611da97
Compare
SUMMARY
This PR migrates the Pivot Table v1 chart to v2. In detail, it does the following:
form_data
to v2MigrateViz
class to support migrating generic chart axes related fields given that some legacy plugins only work without generic chart axesThe following table summarizes the differences in the
form_data
structure between the versions:* All other keys are the same or exist only in the target version
* The time column type (simple or SQL) affects the
adhoc_filters
valueAFTER SCREENSHOT
TESTING INSTRUCTIONS
1 - Make sure all Pivot Table (legacy) charts were converted to Pivot Table
2 - Make sure Pivot Table (legacy) is not available anymore in the viz picker
3 - Make sure that it's possible to revert the migration by executing
superset db downgrade
and reverting this PRADDITIONAL INFORMATION