EnxDev commented on PR #38584: URL: https://github.com/apache/superset/pull/38584#issuecomment-4809685227
## EnxDev's Review Agent โ apache/superset#38584 ยท HEAD dd1308b **comment** โ Correct, well-tested fix for #32961; one dead loop to drop. The fix is sound. Backend create/update now raise `DashboardsForbiddenError` when a chart would be added to an `is_managed_externally` dashboard; the API exposes the flag for filtering; the Save modal stops preselecting and filters out managed dashboards. The info-leak concern raised in earlier bot review (403 vs 422 for inaccessible dashboards) is handled โ `_validate_new_dashboard_access` returns early with `DashboardsNotFoundValidationError` before the managed/owner checks. Verified `is_managed_externally` is in `search_columns` (so the frontend `eq` filter is valid) and in `DashboardGetResponseSchema` (so the on-mount guard reads a real value, not `undefined`). ### ๐ก Should-fix - **`superset/commands/chart/update.py:104-106`** โ The second loop (`for dash in accessible_dashboards: if not security_manager.is_owner(dash): raise`) is now dead code: the merged check on lines 100-102 (`if dash.is_managed_externally or not security_manager.is_owner(dash)`) already raises on `not is_owner`, so the second loop can never fire. Drop it and its stale `# Additional ownership check - must match CreateChartCommand behavior` comment. ### ๐ Praise - `tests/integration_tests/charts/commands_tests.py` & `SaveModal.test.tsx` โ Both backend (create + update rejection) and frontend (no-preselect, filter-present) paths are covered, and the no-preselect test asserts the behavioral side effect (`/tabs` not called) rather than the mock input's `value` โ exactly the stronger assertion flagged in earlier review threads. <!-- enxdev-review-agent:dd1308b --> _Reviewed by EnxDev's Review Agent โ @EnxDev ยท HEAD dd1308b._ -- 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]
