geido commented on PR #43719: URL: https://github.com/apache/superset/pull/43719#issuecomment-5510925644
Pushed an update addressing all review threads (rebased onto master, so #43390's schema hardening is now on the branch). **What changed since the last review** The bigger half of the fix turned out to be on the frontend, per @msyavuz's and @EnxDev's comments: `SliceHeaderControls` passed `!canDrillToDetail` as `useDatasetDrillInfo`'s `skip`, so a `can_view_chart_as_table`-only user — including default Gamma — never fetched `drill_info` at all and the backend change did nothing for them. Label loading is now decoupled from the drill permission, guarded by `can_get_drill_info` so a role without dataset read access doesn't fire a guaranteed 403. The PR description has been rewritten to describe both causes instead of claiming no frontend change was needed. Also added tests pinning the deliberate `columns`/`metrics` filtering asymmetry (@amaannawab923), including a fixture metric that column-style filtering would drop, plus schema-level unit tests that don't need a DB. **On the Bito note about `DatasetMetricDrillInfoSchema` duplicating `DatasetColumnDrillInfoSchema`** — leaving as is. The two share only `verbose_name`; each has a different required identifier (`column_name` vs `metric_name`), which is the field that would actually diverge. A base class holding a single optional string adds indirection without preventing that. **Verification** | | Fix reverted | With fix | |---|---|---| | `SliceHeaderControls.test.tsx` (3 new) | 2 failed, 5 passed | 61 passed | | `schema_tests.py` (2 new) | 2 failed | 16 passed | | `test_get_drill_info_admin_user` | `KeyError: 'metrics'` | passed | Full local runs: 145 frontend tests across 17 suites (`SliceHeaderControls`, `usePermissions`, `ChartContextMenu`, `apiResources`), 116 Python unit tests in `tests/unit_tests/datasets/`, and all pre-commit hooks green over the branch diff. -- 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]
