geido commented on code in PR #43719:
URL: https://github.com/apache/superset/pull/43719#discussion_r3915019806
##########
superset/datasets/api.py:
##########
@@ -1757,6 +1757,8 @@ def get_drill_info(self, pk: int, **kwargs: Any) ->
Response:
"columns.column_name",
"columns.verbose_name",
"columns.groupby",
+ "metrics.metric_name",
Review Comment:
Correct, and this was the more important half of the bug — the backend
change alone did nothing for the users who actually hit it. Fixed.
`SliceHeaderControls/index.tsx` now fetches whenever either permission can
use the verbose map, guarded by `can_get_drill_info` so a role that lacks
dataset read access doesn't fire a request that can only 403:
```ts
const datasetResource = useDatasetDrillInfo(
props.slice.datasource,
props.dashboardId,
props.formData,
!canGetDrillInfo || !(canDrillToDetail || canViewTable),
);
```
`usePermissions` now returns `canGetDrillInfo` (it was already computed
there, just not exposed). `ChartContextMenu` keeps its `!canDrillToDetail &&
!canDrillBy` gate — it uses the dataset for the drill-by picker and
drill-to-detail, not for labels.
Three tests in `SliceHeaderControls.test.tsx`:
- `Dataset drill info API call is made when user can only view chart as
table`
- `Dataset drill info API call is not made without \`can_get_drill_info\``
- `Results grid receives verbose names for a view-as-table-only user` —
asserts the map that reaches `ResultsPaneOnDashboard` is `{region: 'Region',
sum__num: 'Yearly Total'}`
The first and third fail against the pre-fix code (verified by reverting the
change: 2 failed / 5 passed → 61 passed).
##########
superset/datasets/api.py:
##########
@@ -1757,6 +1757,8 @@ def get_drill_info(self, pk: int, **kwargs: Any) ->
Response:
"columns.column_name",
"columns.verbose_name",
"columns.groupby",
+ "metrics.metric_name",
Review Comment:
Done — label loading is decoupled from the drill permission:
```ts
!canGetDrillInfo || !(canDrillToDetail || canViewTable)
```
`usePermissions` now exposes `canGetDrillInfo`, which was already computed
there. The extra guard is because the endpoint itself is `@protect()`-ed as
`can_get_drill_info` on Dataset, so a custom role holding only
`can_view_chart_as_table` would otherwise fire a request that can only 403.
On the default Gamma case you raised: Gamma does get `can_get_drill_info`
(it's in `READ_ONLY_PERMISSION`, and `Dataset` is in
`GAMMA_READ_ONLY_MODEL_VIEWS`), so Gamma now resolves Labels. What it was
missing before was `can_samples` on Datasource / `can_drill`, which is why
`canDrillToDetail` was false and the map came back empty.
Tests as you suggested, in `SliceHeaderControls.test.tsx`:
- fetch happens for a `can_view_chart_as_table` + `can_get_drill_info` user
- no fetch for `can_view_chart_as_table` alone
- the results grid actually receives `{region: 'Region', sum__num: 'Yearly
Total'}` for a view-as-table-only user, so this covers the end of the path and
not just the request
--
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]
