-
Notifications
You must be signed in to change notification settings - Fork 14.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
fix(dashboard): update cross filter scoping chart id references during dashboard import #26239
base: master
Are you sure you want to change the base?
Conversation
during the dashboard import, cross filter scoping is not retained because of not updating chart id references. this commit contains the necessary fix for the same
@john-bodley @michael-s-molina @villebro could you please review this PR. This is kind of blocker to use import/export feature for dashboard, since people need to make the changes manually after importing dashboard as cross filter scoping was not retained. |
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.
@betodealmeida do you think it would make sense to start phasing both the pseudo-ids that are used on the frontend, and the serial ids used on the backend, and replace them all with their respective uuid representations? This way we wouldn't have to do all this complicated remapping during import/export.. |
@chanduapple thanks for the contribution, and I agree with the changes done here. I'll let CI complete before reviewing the code. Also, I'm not sure if our current test suites have tests for similar re-mappings (the native filters may have one), but if it's possible, it would be great if you could add a test to cover this particular fix. |
@villebro is there any guideline/info about adding tests, like how to add test and run, I will go through once, I have tested manually. |
@chanduapple the |
1. List error 2. fixed Too many branches (18/15) 3. fixed trailing white spaces
@villebro i have fixed below earlier CI workflow issues.
Could you please approve the workflow to be rerun and review the code changes? |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #26239 +/- ##
==========================================
+ Coverage 66.97% 69.16% +2.18%
==========================================
Files 1948 1948
Lines 76061 76078 +17
Branches 8493 8493
==========================================
+ Hits 50943 52616 +1673
+ Misses 22938 21282 -1656
Partials 2180 2180
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@villebro @betodealmeida could you please review and approve, let me know if any thing to be modified. please help me out here. |
Looks like this one needs the pre-commit hooks run. Hope we can get it through and close out the old issue! |
Any updates on this folks? We are blocked by this issue to export->import charts with cross-filter scoping. |
during the dashboard import, cross filter scoping is not retained because of not updating chart id references. this commit contains the necessary fix for the same.
this pull request provides the fix for the issue
#19944
slack discussion: https://apache-superset.slack.com/archives/C015WAZL0KH/p1699248398108929
SUMMARY
updated the utils.py to update the chart id references for cross filter scoping also. currently chart id reference updates is present for native filters, position and few others, but not for cross filter scoping.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
dashboard in env1
BEFORE FIX
dashboard after importing to env2

AFTER FIX
dashboard after importing to env2

TESTING INSTRUCTIONS
ADDITIONAL INFORMATION
Fixes "#19944"