tien238lnd opened a new pull request, #44633:
URL: https://github.com/apache/superset/pull/44633
### SUMMARY
Explore gates the "Edit dataset" action on `datasource.editors`, but the
Explore datasource payload never carried that field, so the gate always
collapsed to `isUserAdmin(user)`. Non-Admin users who are listed as editors of
a dataset saw "Edit dataset" greyed out.
`superset-frontend/src/explore/components/controls/DatasourceControl/index.tsx`
computes:
```ts
const allowEdit =
datasource.editors?.some(o => {
const subjectId = o.id ?? o.value;
return subjectId !== undefined && userSubjects.includes(subjectId);
}) || isUserAdmin(user);
```
`GET /api/v1/explore/` builds `result["dataset"]` from `SqlaTable.data`, and
neither `SqlaTable.data` nor `BaseDatasource.data` emitted `editors`, so the
optional chain short-circuited to `undefined` for everyone.
This looks like an oversight in #38831, which introduced the Subject model
and entity editors/viewers: it removed `"owners": [owner.id for owner in
self.owners]` from `BaseDatasource.data`, switched the frontend from
`datasource.owners` to `datasource.editors`, and added `editors` to
`Slice.data` — but never added `editors` back to the dataset payload.
Everything else was already in place:
- `superset/explore/schemas.py` declares `editors =
fields.List(fields.Nested(SubjectResponseSchema))` on `DatasetSchema`.
- `ExplorableData` in `superset/superset_typing.py` already declares and
documents the `editors` field, so no typing change is needed.
- `GET /api/v1/dataset/<id>` already returns `editors` as `[{id, label,
type}]`.
This PR adds `editors` to `SqlaTable.data` using that same compact subject
shape, so the Explore payload and the dataset REST API agree and a single
frontend code path consumes both.
**Secondary fix, same root cause**
`DatasourceModal` builds its save payload as `editors:
mapSubjectValuesToIds(datasource.editors || [])`. With `editors` missing it
sent `editors: []`, which `compute_subject_list` treats as an explicit clear
(empty list, not `None`), with the lockout guard skipped for Admins. An Admin
who saved a dataset from Explore therefore wiped its entire editor list,
silently — the modal's `onDatasourceSave` call keeps the pre-save `editors` in
local state, so the UI showed nothing amiss. Populating `editors` fixes that
too: `mapSubjectValuesToIds` already reads `value.value ?? value.id`, so it
maps the nested objects back to the integer IDs `DatasetPutSchema` expects.
**Deliberately out of scope**
`Slice.data` returns `"editors": [s.id for s in self.editors]` — a list of
ints — which diverges from the nested-object shape `SliceSchema` declares. That
is a separate inconsistency with its own frontend consumers
(`ExploreChartHeader` reads plain ints) and is intentionally left untouched
here.
**Note on queries**
`SqlaTable.data` now touches the `editors` relationship, which is a plain
lazy `select`. So is every other relationship the same property already reads —
`columns`, `metrics` and `database` are all declared without an eager loading
strategy — so this adds one more load of the same kind rather than introducing
a new class of cost. `data_for_slices` reuses `data` once per datasource, not
per slice, so a dashboard load grows by one small query per distinct dataset.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before: for a non-Admin user who is an editor of the dataset, the Explore
dataset menu shows "Edit dataset" greyed out with the tooltip "You must be a
dataset editor in order to edit…".
After: the same user sees "Edit dataset" enabled and can open the dataset
editor.
### TESTING INSTRUCTIONS
Automated:
```bash
pytest tests/unit_tests/connectors/sqla/models_test.py -k editors
```
Two new tests cover the populated and the empty case. Both fail on `master`
with `KeyError: 'editors'` and pass with this change.
`tests/unit_tests/connectors/` and `tests/unit_tests/explore/` pass in full
(154 tests), and `pre-commit run --files superset/connectors/sqla/models.py
tests/unit_tests/connectors/sqla/models_test.py` passes including mypy, ruff
and pylint.
Manual:
1. As an Admin, create a dataset and add a non-Admin user to its **Editors**
list.
2. Confirm `GET /api/v1/dataset/<id>` returns that user under
`result.editors`.
3. Call `GET /api/v1/explore/?datasource_id=<id>&datasource_type=table` and
confirm `result.dataset.editors` is now present and matches, with `id`, `label`
and `type` per entry.
4. Log in as that non-Admin user, open a chart on that dataset in Explore,
open the dataset menu and confirm "Edit dataset" is enabled and the modal opens.
5. Confirm a user who is not an editor and not an Admin still sees the
action disabled.
6. As an Admin, open "Edit dataset" from Explore, save without changing
anything, and confirm the dataset's editor list is preserved (on `master` it is
cleared).
### ADDITIONAL INFORMATION
- [x] Has associated issue: Fixes #44632
- [ ] Required feature flags:
- [x] Changes UI
- [ ] Includes DB Migration (follow approval process in
[SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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]
