EnxDev commented on PR #38584: URL: https://github.com/apache/superset/pull/38584#issuecomment-4952993024
## EnxDev's Review Agent — apache/superset#38584 · HEAD 8915de0 **lgtm** — the two prior blockers are resolved; core fix is correct and tested. Only test-hygiene items remain. Re-review superseding my [earlier pass on `dd1308b`](https://github.com/apache/superset/pull/38584#issuecomment-4809685227). Since then the branch was rebased clean onto master and the dead loop was collapsed. **Verified fixed since last review:** - **Dead ownership loop** (my prior finding + @rusackas's note) — `update.py` now has a single merged check `if dash.is_managed_externally or not security_manager.is_owner(dash): raise DashboardsForbiddenError()`; the redundant "Additional ownership check" loop and its stale comment are gone. - **Info-leak (403 vs 422)** — `_validate_new_dashboard_access` appends `DashboardsNotFoundValidationError()` and `return`s before the managed/owner check, so inaccessible dashboards consistently read as not-found rather than leaking managed-vs-forbidden. - **Filter validity** — `is_managed_externally` is in `search_columns` (`dashboards/api.py:389`), so the frontend `{col: 'is_managed_externally', opr: 'eq', value: false}` filter is accepted; the on-mount guard reads a real value (field is in `BASE_LIST_COLUMNS` and the single-dashboard GET schema). ### 🟡 Should-fix - **`SaveModal.test.tsx:330-335`** — `does not preselect...` reconfigures `fetchMock` routes at line 286 and restores them only at the end of the test body. There's no `beforeEach`/`afterEach` (only `beforeAll`/`afterAll`), so if any assertion between 286 and 330 throws, the modified routes leak into later tests → order-dependent flakiness. Move the restore into `afterEach` (or `try/finally`). ### 🔵 Nits - `SaveModal.test.tsx:41` — module-level `mockLoadDashboards` is never reset between tests; `loadDashboards includes is_managed_externally filter` only waits for it to be defined, so a stale loader from a prior render could mask a mount regression. Reset it in `afterEach`. ### 🙌 Praise - `SaveModal.test.tsx` — the no-preselect test asserts the behavioral side effect (`/tabs` not requested) rather than the mock input's `value`, exactly the stronger assertion raised in earlier threads; the filter test invokes the captured loader and checks the request URL. Both backend rejection paths (create + update) are covered too. <!-- enxdev-review-agent:8915de0 --> _Reviewed by EnxDev's Review Agent — @EnxDev · HEAD 8915de0._ -- 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]
