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.

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

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

chanduapple
Copy link

@chanduapple chanduapple commented Dec 11, 2023

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

  1. create dashboard with more than 2 charts and apply cross filter scoping. as shown below.
  2. export the dashboard.
  3. if you are importing in the same environment, then delete dashboard and charts.
  4. import the dashboard again.
  5. expectation is cross filter scoping should be retained.
  6. actual is cross filter scoping has been removed and global cross filter scoping changes to all charts.
    dashboard in env1
    image

BEFORE FIX

dashboard after importing to env2
image

AFTER FIX

dashboard after importing to env2
image

TESTING INSTRUCTIONS

  1. create dashboard with cross filter scoping.
  2. export the dashboard.
  3. delete the dashboard and charts used in dashboards.
  4. import the dashboard
  5. check the cross filter scoping

ADDITIONAL INFORMATION

Fixes "#19944"

  • Has associated issue: Dashboard filters scopes are not carried while export/import dashboards #19944
  • 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

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
@chanduapple
Copy link
Author

@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.
i am contributing first time, please let me know if any changes required or to furnish more info.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Congrats on making your first PR and thank you for contributing to Superset! 🎉 ❤️

We hope to see you in our Slack community too! Not signed up? Use our Slack App to self-register.

@villebro
Copy link
Member

@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..

@villebro
Copy link
Member

@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. i am contributing first time, please let me know if any changes required or to furnish more info.

@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.

@chanduapple
Copy link
Author

@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.

@villebro
Copy link
Member

@chanduapple the CONTRIBUTING.md has some directives for running the test suites: https://github.com/apache/superset/blob/master/CONTRIBUTING.md#testing The relevant tests I think you're interested in are in https://github.com/apache/superset/blob/master/tests/integration_tests/import_export_tests.py.

@chanduapple chanduapple changed the title fix (dashboard import): update cross filter scoping chart id references during dashboard import fix(dashboard): update cross filter scoping chart id references during dashboard import Dec 12, 2023
@john-bodley john-bodley requested a review from villebro December 12, 2023 17:49
@pull-request-size pull-request-size bot added size/L and removed size/S labels Jan 1, 2024
added a new function to separate the cross filter scoping fixes
@pull-request-size pull-request-size bot added size/S and removed size/L labels Jan 1, 2024
1. List error
2. fixed Too many branches (18/15)
3. fixed trailing white spaces
@pull-request-size pull-request-size bot added size/M and removed size/S labels Jan 1, 2024
@chanduapple
Copy link
Author

@villebro i have fixed below earlier CI workflow issues.

  1. pull request title has been modified.
  2. removed trailing white spaces.
  3. List error
  4. R0912: Too many branches (18/15) (too-many-branches)

Could you please approve the workflow to be rerun and review the code changes?

@betodealmeida @john-bodley

Copy link

codecov bot commented Jan 10, 2024

Codecov Report

Attention: Patch coverage is 47.05882% with 9 lines in your changes missing coverage. Please review.

Project coverage is 69.16%. Comparing base (3daa038) to head (02b2066).
Report is 1392 commits behind head on master.

Files Patch % Lines
superset/commands/dashboard/importers/v1/utils.py 47.05% 9 Missing ⚠️
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              
Flag Coverage Δ
hive 53.66% <5.88%> (?)
mysql 78.05% <47.05%> (-0.02%) ⬇️
postgres 78.15% <47.05%> (-0.02%) ⬇️
presto 53.61% <5.88%> (?)
python 82.84% <47.05%> (+4.53%) ⬆️
sqlite 77.74% <47.05%> (-0.02%) ⬇️
unit 55.87% <47.05%> (?)

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@chanduapple
Copy link
Author

@villebro @betodealmeida could you please review and approve, let me know if any thing to be modified. please help me out here.

@rusackas
Copy link
Member

Looks like this one needs the pre-commit hooks run. Hope we can get it through and close out the old issue!

@ferschubert
Copy link

Any updates on this folks? We are blocked by this issue to export->import charts with cross-filter scoping.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants