mikebridge commented on code in PR #45146:
URL: https://github.com/apache/superset/pull/45146#discussion_r4252592403
##########
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:
Codex fixed the reducer in a71d9301d5280c1eb3cdd84e1bca854e2b7d9eb2; Claude
independently reviewed the final change. You were right that the header-only
guard did not repair the Redux value. `SLICE_UPDATED` now normalizes API `{id,
label}` editors, select `{value, label}` editors and numeric subject IDs before
any store consumer sees them.
- The regression drives the real reducer into the real `canOverwriteSlice`
predicate for a non-admin subject. Before the fix, the API shape throws on
`undefined.id`; afterwards it retains `[8]` and editorship. The select and
numeric forms remain supported.
- This fixes the stored shape read by SaveModal, ExploreVersionHistory and
ChartVersionPreview. The header guard remains defensive; its invalid-ID case is
renamed so it no longer describes the normal properties-save response.
- All 153 focused tests across seven suites and the branch-file hooks,
including TypeScript, pass.
A separate pre-existing issue remains on the version-restore actions: they
check editorship without Chart write permission. That is not the editor-ID
crash and is not changed here; read-only history visibility is preserved.
##########
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:
Codex addressed the SaveModal asymmetry in
a71d9301d5280c1eb3cdd84e1bca854e2b7d9eb2; Claude independently reviewed the
final change. Overwrite now requires `explore.can_add` as well as
`canOverwriteSlice`, matching the title and properties-save controls. The
shared predicate remains editorship-only because it also governs read-only
version-history visibility.
- Explicit and extra editors with write permission retain enabled Overwrite;
both read-only cases now get disabled Overwrite and the appropriate permission
message. These cases failed before the fix.
- The current PR table already had the Chart-write qualifiers when I
rechecked it; the description and test instructions now also explain the
reducer normalization and SaveModal gate.
- Final checks: 153 tests across seven suites, frontend type checking and
branch-file hooks passed.
The pre-existing version-restore action gates still need a separate
write-permission follow-up; this change does not claim to fix those actions.
--
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]
