fitzee commented on PR #45530:
URL: https://github.com/apache/superset/pull/45530#issuecomment-6357087761
**Re-checked at `d4b45d97`:** Still LGTM. The only new commit is a master
merge, and the PR's own diff is unchanged.
- **What changed since `38e286ce`:** one merge of master (`aabddf5b`,
#45532). The only conflict was `UPDATING.md`, where both sides added an entry
under "Next". It was resolved by keeping both entries, this PR's first.
Compared against master, the PR's own changes to `superset/daos/dashboard.py`,
the tests and the docs are byte-identical to `38e286ce`; only the surrounding
`UPDATING.md` text differs. #45528 is still at `0099a023`.
- **Merge check vs #45538 (`ebb6ca1c`, still open):** it conflicts at the
same spots we noted before.
- `superset/daos/dashboard.py:1111-1113`: the right resolution keeps this
PR's block (no `dash.params = original_dash.params`) and adds #45538's
`allowed_chart_ids=(set(old_to_new_slice_ids.values()) if old_to_new_slice_ids
else _member_chart_ids(original_dash.id))` to the `set_dash_metadata` call.
`_copy_chart_metadata` then runs after that call as before.
- `tests/integration_tests/dashboards/api_tests.py:~5456`: both sides only
add tests, so keep both. #45538's `test_copy_dashboard_layout_chart_access`
doesn't depend on seeding settings from the source dashboard.
- With that resolution, the unit tests pass (96).
- **Merge check vs #45543 (`e30773a1`, still open):** one conflict in
`superset/daos/dashboard.py`. It's only position: this PR's `_copy_*` helpers
and #45543's `_install_membership_baseline` are added at the same place; keep
both. The two don't affect each other's behaviour:
- A copy's dashboard is new and not yet saved, so
`_install_membership_baseline` returns early and doesn't touch the copy.
- `kept_archived` stays empty for a copy, so `copied_ids` (taken from
`dash.slices`) still covers only the charts in the layout.
- The test file merges cleanly. Unit tests pass (100, including #45543's
`dashboard_test.py`).
- **Tests at head:**
`tests/unit_tests/dao/test_copy_dashboard_legacy_ids.py`,
`tests/unit_tests/subjects/test_creator_group_call_sites.py` and
`tests/unit_tests/dao/dashboard_test.py` all pass (96). Integration tests were
not run locally; CI covers them.
- **CI:** 58 pass, 14 skipped, 0 failing.
- **Prior nits:**
- `superset/daos/dashboard.py:404`: `config["id"] = int(chart_key)` is
unchanged and still not tested directly. The existing tests give each config an
`id` that already matches its key.
- Schema `unknown=RAISE` on old snapshots: unchanged; this predates the PR.
- Merge-order caveat still applies: whichever of this PR and #45538 merges
second must keep `allowed_chart_ids` and must not bring back `dash.params =
original_dash.params`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
