aminghadersohi commented on PR #41606: URL: https://github.com/apache/superset/pull/41606#issuecomment-4987963403
@rebenitez1802 thank you for such a careful, well-calibrated review — the severity framing (and the explicit "reviewed & cleared" notes) made this round easy to act on. Everything is addressed in 366767f9fdf954dfd4ada8ebef39cb2859f979d1: **🟡 Roles tool missing the no-op short-circuit** — Added, mirroring the owners tool: right after the mypy-narrowing asserts, an effective no-op now returns an **empty** `roles` list with a "No effective change" warning and skips the DB write entirely. The docstring's privacy paragraph now states the invariant explicitly (same wording as the owners tool), and `test_add_already_assigned_role_is_noop_without_disclosure` asserts `payload["roles"] == []`, no added/removed deltas, and `commit` never called. **🟢 Owners redundant write on caller self-removal** — Went with the comment option. Hoisting the detection would mean predicting `ensure_no_lockout`'s outcome (admin status, `EXTRA_EDITORS_RESOLVER`) outside `populate_subject_list`, which owns those semantics — duplicating that felt more fragile than the rare state-preserving write. The comment at the first no-op check now documents the path and why it's deferred to the post-resolution check. **🟢 Valid role rejected as "does not exist"** — Fixed properly rather than just re-worded: added `get_or_create_role_subject` to `superset/subjects/utils.py` (mirroring `get_or_create_user_subject` — lookup, `sync_role_subject`, flush, re-fetch), and the roles tool now resolves per-role through it. A pre-sync role gets its Subject row created on demand, so the "do not exist" error is now only ever true, and the two tools are symmetric. Covered by three new unit tests in `tests/unit_tests/subjects/test_utils.py`. **🟢 Helper duplication** — Extracted `find_and_authorize_dashboard` (parameterized by response class) and `dashboard_url` into `dashboard/tool/governance_utils.py`; the three governance tools now share one copy. `update_dashboard` keeps its variant since its not-found contract differs (`DashboardError` with `error_type` vs. the tool's own response schema) — folding that in is the follow-up. **🟢 `List[...]` → `list[...]`** — Done for all 20 occurrences this PR added (requests, responses, validators, and local annotations in the governance schemas). Pre-existing `List[...]` elsewhere in the file left untouched to keep the diff scoped. **🟢 Defensive-branch coverage** — All four gaps now tested: refresh-fails-after-commit in each of the three tools (assert captured values still returned; the certification one also asserts the advisory warning), commit-fails-then-rollback-also-fails (structured error, no unhandled exception), `dashboard.viewers` lazy-load `SQLAlchemyError` (raising-property fixture), and the `SubjectsNotFoundValidationError` handler in the owners tool. **🟢 Stale PR description** — Updated: `viewers_enabled` / `raise_for_editorship`, plus the equally-stale `populate_owner_list` reference and the roles bullet's DASHBOARD_RBAC wording (now describes the #38831 Subject/`ENABLE_VIEWERS` model). Also refreshed the test-plan counts. **🟢 (informational) `removed_owner_ids=[]` on auto-reverted self-removal** — Appreciated the flag-and-clear; agreed it's by design, no change made. Also picked up Bito run #21bb9b's suggestion (refresh-failure test for the certification tool) — it's part of the coverage batch above. Validation: `pytest tests/unit_tests/mcp_service/ tests/unit_tests/subjects/` — 2,885 passed (dashboard MCP suite now 340); pre-commit (mypy/ruff/ruff-format/pylint) clean on all changed files. -- 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]
