gabotorresruiz commented on code in PR #45146:
URL: https://github.com/apache/superset/pull/45146#discussion_r4233086484
##########
superset-frontend/src/explore/components/ExploreChartHeader/index.tsx:
##########
@@ -291,19 +291,17 @@ const ExploreChartHeader: FC<ExploreChartHeaderProps> = ({
}
}, [showUnsavedChangesModal, shouldForceCloseModal]);
- const userSubjects = useMemo(
- () => new Set(getBootstrapData()?.common?.user_subjects ?? []),
- [],
- );
-
const editableTitleProps = useMemo(
() => ({
title: sliceName ?? '',
canEdit:
!isVersionPreviewActive &&
(!slice ||
- canOverwrite ||
- Boolean(slice?.editors?.some(editor => userSubjects.has(editor)))),
+ canOverwriteSlice({
Review Comment:
Not a blocker, and not caused by your diff, but `canEdit` now runs through a
predicate that can throw. After **Edit chart properties** saves,
`PropertiesModal` hands `onSave` the raw `GET /api/v1/chart/<pk>` body, where
`editors` is a list of `SubjectResponseSchema` objects keyed `id`.
`exploreReducer`'s `SLICE_UPDATED` maps that list with `editor.value`, so
`explore.slice.editors` becomes `[null]`, and `subjectId` in `permissionUtils`
then reads `.id` off the `null`. Driving the real reducer into the real
predicate gives `Cannot read properties of null (reading 'id')`, and rendering
this header with `editors: [null]` throws the same on your branch and on its
parent, since `canModifySlice` in `useExploreAdditionalActionsMenu` already
calls `canOverwriteSlice` with the same slice. Pre-existing, then, but cheap to
close while you are in here:
```ts
const getEditorId = (editor: EditorItem): number =>
typeof editor === 'number' ? editor : (editor.value ?? editor.id);
```
or a `subject == null` short circuit in `subjectId`. A `SLICE_UPDATED` case
asserting that a `[{ id: 8, label: 'Alice' }]` payload leaves `slice.editors`
as `[8]` would pin it.
##########
docs/docs/using-superset/exploring-data.mdx:
##########
@@ -128,6 +128,10 @@ To save the visualization, click on **Save** in the top
left of the screen. In t
- Select **Add To Dashboard** and enter
Tutorial Dashboard. Finally, select **Save & Go To Dashboard**.
+For an existing chart, chart editors (including extra editors and
administrators)
+can edit its title in Explore and save the change. Externally managed charts
and
Review Comment:
Small question on "and save the change": an extra editor whose role carries
only `can_read` on `Chart` gets an editable title here, but the rename is
persisted by the overwrite `PUT /api/v1/chart/<pk>`, and `put` maps to `write`
in `MODEL_API_RW_METHOD_PERMISSION_MAP`, so it would come back 403.
`canEditProperties` in `useExploreAdditionalActionsMenu` ANDs `can_add` (the
`can_write` on `Chart` lookup) for exactly that reason. Is matching
`SaveModal`, which also gates on `canOverwriteSlice` alone, the intent? I might
be missing something.
--
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]
