-
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
chore: Add a migration that removes filter_bar_orientation from dashboard's json_metadata #22248
chore: Add a migration that removes filter_bar_orientation from dashboard's json_metadata #22248
Conversation
…oard's json_metadata
Codecov Report
@@ Coverage Diff @@
## master #22248 +/- ##
===========================================
+ Coverage 55.46% 65.48% +10.01%
===========================================
Files 1841 1841
Lines 70220 70220
Branches 7670 7670
===========================================
+ Hits 38947 45981 +7034
+ Misses 29291 22257 -7034
Partials 1982 1982
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
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. I'm still on the fence about maybe placing
class Meta:
unknown = EXCLUDE
on all these schemas that are known to evolve over time to avoid getting 400s when upgrading/downgrading and having old leftover cruft on the metadata. But maybe leave that for a follow-up discussion.
json_meta.pop("filter_bar_orientation", None) | ||
dashboard.json_metadata = json.dumps(json_meta) |
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.
nit: To avoid unnecessarily updating the metadata if someone happens to have the text filter_bar_orientation
in one of the titles, descriptions or similar (highly likely 😄), maybe add this as a precaution:
json_meta.pop("filter_bar_orientation", None) | |
dashboard.json_metadata = json.dumps(json_meta) | |
filter_bar_orientation = json_meta.pop("filter_bar_orientation", None) | |
if filter_bar_orientation: | |
dashboard.json_metadata = json.dumps(json_meta) |
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.
Good point, didn't think of that 😄
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.
Reapproving most recent changes
SUMMARY
This PR adds a downgrade migration that removes
filter_bar_orientation
field from dashboard'sjson_metadata
.It's related to the horizontal filter bar feature that's currently in development and is locked behind a
HORIZONTAL_FILTER_BAR
feature flag.It's a required cleanup step when downgrading Superset version, because the horizontal filter bar project adds
filter_bar_orientation
field toDashboardJSONMetadataSchema
schema.Upgrade is not required - we simply default to
VERTICAL
iffilter_bar_orientation
is not presentBEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION
Current: 0.26 s 10+: 0.40 s 100+: 0.41 s 1000+: 0.25 s