gabotorresruiz commented on code in PR #45146:
URL: https://github.com/apache/superset/pull/45146#discussion_r4251017247
##########
superset-frontend/src/explore/components/ExploreChartHeader/index.tsx:
##########
@@ -125,6 +125,21 @@ const ExploreChartHeader: FC<ExploreChartHeaderProps> = ({
metadata,
isSaveModalVisible,
}) => {
+ const slice = useMemo(
+ () =>
+ rawSlice
+ ? {
+ ...rawSlice,
+ editors: rawSlice.editors?.filter(Number.isInteger),
Review Comment:
Thanks for both of these, and the read-only extra editor case is a good
addition. One thing I want to flag before this lands though: the guard is a
locally derived value inside this component, so it never reaches the store.
`state.explore.slice.editors` is still `[undefined]` after a properties save,
and three other callers read it straight from Redux rather than through this
memo: `ExploreVersionHistory.tsx:78` and `ChartVersionPreview.tsx:76` both
`useSelector(state => state.explore?.slice)` before calling
`canOverwriteSlice`, and `SaveModal` gets it via `mapStateToProps` at `:935`
and calls the predicate at `:230`. So the title and the additional actions menu
are covered here (the menu reads the sanitized value at line 225), but those
three still throw on the same shape, and `SaveModal` is the one a user reaches
straight after renaming.
None of that is caused by your diff, so I am not asking you to fix it here.
But it does mean the properties-save crash is not closed, and the deferred
reducer change is the thing that would close it everywhere at once. Could that
one go in first, or alongside? `(editor.value ?? editor.id)` in `getEditorId`
would let this guard become belt and braces rather than the only thing holding
it.
Smaller note: `malformed editors after properties save` asserts `granted:
false`, so an explicit non-admin editor now silently loses the rename until
reload where it used to throw. Better, clearly, but it does bake the degraded
behaviour into the test rather than the fixed behaviour.
##########
superset-frontend/src/explore/components/ExploreChartHeader/index.tsx:
##########
@@ -291,30 +306,30 @@ 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)))),
+ (canWriteChart &&
Review Comment:
This matches `canEditProperties`, which is what I asked about, so the dead
end I described is gone for the title. It does leave the pair inconsistent in
the other direction though: `SaveModal` still gates Overwrite on
`canOverwriteSlice` alone (`:230`, button at `:736`) with no `can_add`, so a
read-only extra editor now gets a read-only title next to an enabled
**Overwrite** button, and that is still the 403 I was worried about, just
reached from a different control.
Worth either ANDing `can_add` into `SaveModal`'s predicate too, or folding
it into `canOverwriteSlice` so there is one answer. Happy either way, I just
would not want the two to drift.
Also the description still lists extra editors as able to edit the title
unconditionally, which this commit changes. The docs are updated but the PR
body table is not.
--
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]
