fix(dashboard import): Importing existing dashboard via UI won't update charts and datasets

[adolfotcar]

SUMMARY

This commit fixes a bug when importing a dashboard via the UI it wouldn't update the charts or datasets within that dashboard.

The fix involved in using the overwrite parameter to the sub-imports instead of having a hard coded False value.

A unit test was also added to test/unit_tests/commands/dashboard/importers/v1/assets_test.py to run the import procedure and compare the changed_on fields of dashboard, charts and datasets and ensure they are updating.

Fixes: #34879

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

Same Environment

• Create a dashboard containing chartA and datasetA
• Export the dashboard using the UI
• Make changes either to the chartA or datasetA
• Import the Zip exported
• You'll notice that the chartA and datasetA were reverted back

Multiple Environments

• Create a dashboardA in the environmentA containing chartA and datasetA
• Export the dashboardA
• Import it into the envornmentB
• You'll notice that both: dashboard, chart and datasets are now present in environmentB
• Go back to environmentA and make changes to chartA or datasetA
• Export dashboardA again from environmentA
• Import into environmentB
• You'll notice that the chart and dashboard now are updated in environmentB

ADDITIONAL INFORMATION

• Has associated issue: Importing existing dashboard via UI won't update charts and datasets #34879
• 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

[korbit-ai]

I've completed my review and didn't find any issues.

Files scanned

​

File Path	Reviewed
superset/commands/dashboard/importers/v1/init.py	✅

Explore our documentation to understand the languages and file types we support and the files we ignore.

Check out our docs on how you can make Korbit work best for you and your team.

Loving Korbit!? Share us on LinkedIn Reddit and X

[sadpandajoe]

@adolfotcar do we want to actually revert back by default? Let's say you exported a chart or dashboard a week ago. Since then we've updated a dataset by adding more columns or added an extra dimension or even created new charts off this updated dashboard. Someone goes in and imports an old dashboard, if the columns are now missing that other charts use, wouldn't this then lead to broken charts?

I think @rusackas and I have had discussions before about this problem and it might be if there is a change we should ask the person if they want to overwrite or create new. Such that your imported dataset and chart might just be a new chart or a duplicate to prevent breaking changes.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.36%. Comparing base (54af1cb) to head (670a16b).
⚠️ Report is 20 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff             @@
##           master   #34880       +/-   ##
===========================================
+ Coverage        0   72.36%   +72.36%     
===========================================
  Files           0      584      +584     
  Lines           0    42512    +42512     
  Branches        0     4482     +4482     
===========================================
+ Hits            0    30764    +30764     
- Misses          0    10566    +10566     
- Partials        0     1182     +1182     

Flag	Coverage Δ
hive	46.85% <0.00%> (?)
mysql	71.38% <100.00%> (?)
postgres	71.45% <100.00%> (?)
presto	50.53% <0.00%> (?)
python	72.33% <100.00%> (?)
sqlite	71.00% <100.00%> (?)
unit	100.00% <ø> (?)

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.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[rusackas]

Do we need this file?

[adolfotcar]

Nope, not needed...

[rusackas]

Same here... not sure this was intended/needed.

[adolfotcar]

same....not needed, my bad

[adolfotcar]

@sadpandajoe I think that not reverting leads to more issues. Imagine you have a dashboard that has chart's column as a filter and you export it. Over the next few days you remove that column from the dataset and chart and also remove the filter from the dashboard but then you want to revert your job back to the way it was before the changes, it will import the dashboard back again with that column as filter but now the chart and datasets won't have that column anymore.

When you have multiple environments as well it makes much easier for the development to have everything being overwritten: you do all the changes in dashboards, charts and dataset in a test environment and when moving to production you export only the dashboard and import it rather than having to keep track of all your changes and export one by one.

In the end, the user is asked whether they want to overwrite and their answer is what defines if the assets will be overwritten or not. Maybe there should be more explicit message about what happens when overwriting?

Another solution I guess would be to have an option to "overwrite only dashboard" or "overwrite all assets". A simpler solution to this would be to have an option in the config.py file this way the UI won't require any changes.

[rusackas]

Thanks for the PR! Looks quite approvable, just a couple questions about empty files :)

[rusackas]

Looks like there's also this to contend with:

tests/unit_tests/commands/dashboard/importers/v1/assets_test.py:101:89: E501 Line too long (95 > 88)
    |
 99 |     imported_dataset = db.session.query(SqlaTable).filter_by(uuid=dataset_uuid).one()
100 |
101 |     # extracting changed_on field to compare with the previous retrieved, ignoring milliseconds
    |                                                                                         ^^^^^^^ E501
102 |     final_dashboard_changed_on = imported_dashboard.changed_on.strftime(
103 |         "%Y-%m-%d %H:%M:%S"
    |

Superset uses Git pre-commit hooks courtesy of pre-commit. To install run the following:

pip3 install -r requirements/development.txt
pre-commit install

A series of checks will now run when you make a git commit.

Alternatively it is possible to run pre-commit by running pre-commit manually:

pre-commit run --all-files

[sadpandajoe]

@adolfotcar I think this is good, but I just would like to see a confirmation of some sort and the user acknowledges that it can break or undo existing work. I just don't want to see this and then a couple of months later someone else comes in and say "hey whenever i import a dashboard, it breaks my dataset and charts". So I guess what I'm proposing is either some kind of message to let people know, that if you upload this it'll overwrite all charts and datasets in this dashboard or maybe a checkbox that when checks lets them know it overwrites all assets.

[adolfotcar]

@sadpandajoe I just added a checkbox there.
This checkbox only appears for dashboards importing and only when needsOverwrite flag is true.
Also changed the unit test to test both overwritting only dashboard and overwritting everything.