Skip to content
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.

Sign up for GitHub

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

Jump to bottom

chore: Refine native dashboard cleanup logic #24864

Merged
merged 1 commit into from
Aug 9, 2023
Merged

chore: Refine native dashboard cleanup logic #24864

merged 1 commit into from
Aug 9, 2023

Conversation

john-bodley
Copy link
Member

SUMMARY

Making a few minor refinements on the native dashboard filter cleanup logic.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

CI.

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@codecov
Copy link

codecov bot commented Aug 2, 2023 •
edited
Loading

Codecov Report

Merging #24864 (407e5f1) into master (44e4fb1) will decrease coverage by 0.01%.
Report is 14 commits behind head on master.
The diff coverage is 52.29%.

❗ Current head 407e5f1 differs from pull request most recent head 70e01f5. Consider uploading reports for the commit 70e01f5 to get more accurate results

@@            Coverage Diff             @@
##           master   #24864      +/-   ##
==========================================
- Coverage   69.00%   69.00%   -0.01%     
==========================================
  Files        1904     1906       +2     
  Lines       74107    74143      +36     
  Branches     8194     8208      +14     
==========================================
+ Hits        51140    51160      +20     
- Misses      20848    20860      +12     
- Partials     2119     2123       +4
Flag Coverage Δ
hive 54.13% <0.00%> (-0.01%) ⬇️
mysql 79.21% <0.00%> (-0.01%) ⬇️
postgres 79.31% <0.00%> (-0.01%) ⬇️
presto 54.03% <0.00%> (-0.01%) ⬇️
python 83.37% <0.00%> (-0.01%) ⬇️
sqlite 77.89% <0.00%> (-0.01%) ⬇️
unit 54.99% <0.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
...d/packages/superset-ui-chart-controls/src/types.ts 100.00% <ø> (ø)
...s/legacy-plugin-chart-country-map/src/countries.ts 100.00% <ø> (ø)
...s/legacy-plugin-chart-heatmap/src/controlPanel.tsx 57.14% <ø> (ø)
.../legacy-plugin-chart-heatmap/src/transformProps.js 0.00% <ø> (ø)
.../legacy-plugin-chart-world-map/src/controlPanel.ts 25.00% <ø> (ø)
...egacy-plugin-chart-world-map/src/transformProps.js 0.00% <ø> (ø)
...harts/src/BigNumber/BigNumberTotal/controlPanel.ts 30.00% <ø> (ø)
...rts/src/BigNumber/BigNumberTotal/transformProps.ts 0.00% <ø> (ø)
.../BigNumber/BigNumberWithTrendline/controlPanel.tsx 16.66% <ø> (ø)
...BigNumber/BigNumberWithTrendline/transformProps.ts 48.57% <ø> (ø)
... and 55 more

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

john-bodley
john-bodley commented Aug 2, 2023
View reviewed changes
superset/cli/native_filters.py
@@ -368,17 +368,15 @@ def cleanup(
json_metadata = json.loads(dashboard.json_metadata or "{}")
position_json = json.loads(dashboard.position_json or "{}")

if "native_filter_migration" not in json_metadata:
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If a dashboard has no filters then this key wouldn't exit and thus it's a misnomer to say that the dashboard hadn't been upgraded.

superset/cli/native_filters.py

for value in position_json.values():
if (
isinstance(value, dict)
and value["type"] == "MARKDOWN"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not all values in the the position_json dictionary contain the meta key.

michael-s-molina
michael-s-molina reviewed Aug 2, 2023
View reviewed changes
superset/cli/native_filters.py

for value in position_json.values():
if (
isinstance(value, dict)
and value["type"] == "MARKDOWN"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
and value["type"] == "MARKDOWN"
and "meta" in value

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@michael-s-molina I've updated the PR to use a variant of this (in the form of the walrus operator) throughout for consistency.

@john-bodley john-bodley force-pushed the john-bodley--refine-native-dashboard-cleanup branch from 3237bb9 to 70e01f5 Compare August 3, 2023 22:09
@pull-request-size pull-request-size bot added size/M and removed size/S labels Aug 3, 2023
@michael-s-molina michael-s-molina added the v3.0 Label added by the release manager to track PRs to be included in the 3.0 branch label Aug 9, 2023
michael-s-molina
michael-s-molina approved these changes Aug 9, 2023
View reviewed changes
Copy link
Member

@michael-s-molina michael-s-molina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@john-bodley john-bodley merged commit 3458664 into master Aug 9, 2023
michael-s-molina pushed a commit that referenced this pull request Aug 10, 2023
@john-bodley @michael-s-molina
chore: Refine native dashboard cleanup logic (#24864)
dba72c4 
(cherry picked from commit 3458664)
@mistercrunch mistercrunch added 🍒 3.0.0 🍒 3.0.1 🍒 3.0.2 🍒 3.0.3 🍒 3.0.4 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 3.1.0 labels Mar 8, 2024
@mistercrunch mistercrunch deleted the john-bodley--refine-native-dashboard-cleanup branch March 26, 2024 17:59
vinothkumar66 pushed a commit to vinothkumar66/superset that referenced this pull request Nov 11, 2024
@john-bodley
chore: Refine native dashboard cleanup logic (apache#24864)
bbddbb6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michael-s-molina michael-s-molina michael-s-molina approved these changes

@justinpark justinpark Awaiting requested review from justinpark

Assignees
No one assigned
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/M v3.0 Label added by the release manager to track PRs to be included in the 3.0 branch 🍒 3.0.0 🍒 3.0.1 🍒 3.0.2 🍒 3.0.3 🍒 3.0.4 🚢 3.1.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@john-bodley @michael-s-molina @mistercrunch