Skip to content

Fix import dashboard id #8203

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

Closed
gbrian wants to merge 2 commits into from
Closed

Fix import dashboard id #8203

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
66884b7
Fix import dashboard id
gbrian Sep 9, 2019
b8e71db
deal
gbrian Sep 10, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion superset/models/core.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -611,10 +611,11 @@ def alter_positions(dashboard, old_to_new_slc_id_dict):

# override the dashboard
existing_dashboard = None
remote_id = dashboard_to_import.params_dict.get("remote_id") or dashboard_to_import.id
for dash in session.query(Dashboard).all():
if (
"remote_id" in dash.params_dict
and dash.params_dict["remote_id"] == dashboard_to_import.id
and dash.params_dict["remote_id"] == remote_id
Copy link
Member

@bkyryliuk bkyryliuk Sep 9, 2019

Choose a reason for hiding this comment

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

it would be nice to add a unit test

Copy link
Contributor Author

@gbrian gbrian Sep 9, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Yes. Looked at the tests and they seem to be testing a different approach than the one implemented but still I'm missing many parts.

This test should have cough the error: test_import_override_dashboard_2_slices but executes a different logic than the current one implemented.
Do you think we cab move on without the test? Ofc, is working on our project 😅 "Trust me I'm engineer!"

Copy link
Member

@bkyryliuk bkyryliuk Sep 9, 2019

Choose a reason for hiding this comment

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

I was working on this part of the codebase a while ago and this one is quite tricky and easy to introduce a bug.
In addition to that if somebody would refactor this file in the future it would be nice to provide them some guardrails. This is not a blocker but just a suggestion.

A good test case would be importing:
dash1 {id: 1, remote_id: 101}
dash2 {id:2, remote_id: 101}
and making sure that override will happen

):
existing_dashboard = dash

Expand Down