rusackas commented on code in PR #40528:
URL: https://github.com/apache/superset/pull/40528#discussion_r3338886701
##########
superset-frontend/src/dashboard/components/PropertiesModal/index.tsx:
##########
@@ -254,16 +254,24 @@ const PropertiesModal = ({
};
const handleOnChangeOwners = (
- owners: { value: number; label: string }[],
+ selectedOwners: { value: number; label: string }[],
options: Record<string, unknown>[],
) => {
- const parsedOwners: Owners = ensureIsArray(owners).map((o, i) => ({
- id: o.value,
- full_name:
- (options?.[i]?.[OWNER_TEXT_LABEL_PROP] as string) ||
- (typeof o.label === 'string' ? o.label : ''),
- email: (options?.[i]?.[OWNER_EMAIL_PROP] as string) || '',
- }));
+ const optionsById = new Map(options.map(opt => [opt.value as number,
opt]));
Review Comment:
Done in bed150cfa5. I typed the owners option `label` as `ReactNode` (it's
the rendered `OwnerSelectLabel` element) in both `handleOnChangeOwners` and
`AccessSection`'s `onChangeOwners` prop via a shared `OwnerOption` type, and
removed the `as number` / `as string` casts around the option lookup. So the
runtime `typeof label === 'string'` guard is no longer redundant — it's exactly
what the honest `ReactNode` type calls for.
##########
superset-frontend/src/dashboard/components/PropertiesModal/index.tsx:
##########
@@ -254,16 +254,24 @@ const PropertiesModal = ({
};
const handleOnChangeOwners = (
- owners: { value: number; label: string }[],
+ selectedOwners: { value: number; label: string }[],
options: Record<string, unknown>[],
) => {
- const parsedOwners: Owners = ensureIsArray(owners).map((o, i) => ({
- id: o.value,
- full_name:
- (options?.[i]?.[OWNER_TEXT_LABEL_PROP] as string) ||
- (typeof o.label === 'string' ? o.label : ''),
- email: (options?.[i]?.[OWNER_EMAIL_PROP] as string) || '',
- }));
+ const optionsById = new Map(options.map(opt => [opt.value as number,
opt]));
+ const parsedOwners: Owners = ensureIsArray(selectedOwners).map(o => {
+ const existingOwner = owners.find(ow => ow.id === o.value);
+ if (existingOwner) {
+ return existingOwner;
+ }
Review Comment:
Added in bed150cfa5. I pulled the parsing out into a pure
`parseSelectedOwners()` helper and unit-tested exactly this scenario: owners A
and B loaded into state, remove A so `onChange` fires with only B while the
option cache is empty → B keeps its name/email from state instead of going
nameless. I also covered building a new owner from the option text label and
the string / non-string label fallbacks.
--
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]
