Fix import dashboard id #8203
Fix import dashboard id #8203
Conversation
| 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 |
There was a problem hiding this comment.
it would be nice to add a unit test
There was a problem hiding this comment.
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!"
There was a problem hiding this comment.
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
|
@bkyryliuk , @mistercrunch can we merge this in the meantime? Moved to other issues and the fix is so small so does not introduce more complexity and current import is not updating existing dashboards. Thanks in advance |
superset/models/core.py
Outdated
|
|
||
| # override the dashboard | ||
| existing_dashboard = None | ||
| remote_id = dashboard_to_import.params_dict.get("remote_id") |
There was a problem hiding this comment.
can we do remote_id = dashboard_to_import.params_dict.get("remote_id") or dashboard_to_import.id to be prudent here
|
I'd also like to see a test that fails before the fix and succeed after. Maybe it's just a matter of adding a new assertion counting the number of dashboards (?) to check if it gets added instead of updated (?) |
|
Saw you hit a |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. For admin, please label this issue |
3 participants
CATEGORY
Choose one
SUMMARY
#8199
REVIEWERS