fix(dashboard): prevent "undefined undefined" owner names in properties modal (#40528)

Co-authored-by: Claude Code <noreply@anthropic.com>
(cherry picked from commit aba6ea536c)

superset-frontend/src/dashboard/components/PropertiesModal/index.tsx

@@ -38,10 +38,6 @@ import {
 } from '@superset-ui/core';
 
 import withToasts from 'src/components/MessageToasts/withToasts';
-import {
-  OWNER_TEXT_LABEL_PROP,
-  OWNER_EMAIL_PROP,
-} from 'src/features/owners/OwnerSelectLabel';
 import { fetchTags, OBJECT_TYPES } from 'src/features/tags/tags';
 import {
   applyColors,
@@ -65,6 +61,7 @@ import {
   CertificationSection,
   AdvancedSection,
 } from './sections';
+import { parseSelectedOwners, type OwnerOption } from './utils';
 
 type PropertiesModalProps = {
   dashboardId: number;
@@ -254,17 +251,10 @@ const PropertiesModal = ({
   };
 
   const handleOnChangeOwners = (
-    owners: { value: number; label: string }[],
-    options: Record<string, unknown>[],
+    selectedOwners: OwnerOption[],
+    options: OwnerOption[],
   ) => {
-    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) || '',
-    }));
-    setOwners(parsedOwners);
+    setOwners(parseSelectedOwners(selectedOwners, options, owners));
   };
 
   const handleOnChangeRoles = (roles: { value: number; label: string }[]) => {

superset-frontend/src/dashboard/components/PropertiesModal/sections/AccessSection.tsx

@@ -32,6 +32,7 @@ import {
   OWNER_OPTION_FILTER_PROPS,
 } from 'src/features/owners/OwnerSelectLabel';
 import { useAccessOptions } from '../hooks/useAccessOptions';
+import { type OwnerOption } from '../utils';
 
 type Roles = { id: number; name: string }[];
 type Owners = {
@@ -47,10 +48,7 @@ interface AccessSectionProps {
   owners: Owners;
   roles: Roles;
   tags: TagType[];
-  onChangeOwners: (
-    owners: { value: number; label: string }[],
-    options: Record<string, unknown>[],
-  ) => void;
+  onChangeOwners: (owners: OwnerOption[], options: OwnerOption[]) => void;
   onChangeRoles: (roles: { value: number; label: string }[]) => void;
   onChangeTags: (tags: { label: string; value: number }[]) => void;
   onClearTags: () => void;

superset-frontend/src/dashboard/components/PropertiesModal/utils.test.ts

@@ -0,0 +1,72 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import {
+  OWNER_TEXT_LABEL_PROP,
+  OWNER_EMAIL_PROP,
+} from 'src/features/owners/OwnerSelectLabel';
+import { parseSelectedOwners } from './utils';
+
+test('preserves a remaining owner from state when the option cache is partial', () => {
+  // Owners A(1) and B(2) were loaded from the dashboard, so their full data
+  // lives only in component state (the controlled `value`), not in the
+  // AsyncSelect option cache.
+  const existingOwners = [
+    { id: 1, full_name: 'Alice Adams', email: 'alice@example.com' },
+    { id: 2, full_name: 'Bob Brown', email: 'bob@example.com' },
+  ];
+  // The user removes A; onChange fires with only B, and `options` does not
+  // contain B (it was never searched/loaded).
+  const selectedOwners = [{ value: 2, label: 'Bob Brown' }];
+  const options: never[] = [];
+
+  // Regression: B must keep its real name/email rather than collapsing into a
+  // nameless ("undefined undefined") owner.
+  expect(parseSelectedOwners(selectedOwners, options, existingOwners)).toEqual([
+    { id: 2, full_name: 'Bob Brown', email: 'bob@example.com' },
+  ]);
+});
+
+test('builds a new owner from the option text label when not already in state', () => {
+  const options = [
+    {
+      value: 3,
+      // Real labels are OwnerSelectLabel React elements; a number is used here
+      // simply as a non-string ReactNode for the test.
+      label: 1,
+      [OWNER_TEXT_LABEL_PROP]: 'Carol Clark',
+      [OWNER_EMAIL_PROP]: 'carol@example.com',
+    },
+  ];
+
+  expect(parseSelectedOwners([{ value: 3, label: 1 }], options, [])).toEqual([
+    { id: 3, full_name: 'Carol Clark', email: 'carol@example.com' },
+  ]);
+});
+
+test('falls back to a string label when the option has no text label', () => {
+  expect(
+    parseSelectedOwners([{ value: 4, label: 'Plain Name' }], [], []),
+  ).toEqual([{ id: 4, full_name: 'Plain Name', email: '' }]);
+});
+
+test('yields an empty name for a non-string label with no text label', () => {
+  expect(parseSelectedOwners([{ value: 5, label: 1 }], [], [])).toEqual([
+    { id: 5, full_name: '', email: '' },
+  ]);
+});

superset-frontend/src/dashboard/components/PropertiesModal/utils.ts

@@ -0,0 +1,79 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { type ReactNode } from 'react';
+import { ensureIsArray } from '@superset-ui/core';
+import {
+  OWNER_TEXT_LABEL_PROP,
+  OWNER_EMAIL_PROP,
+} from 'src/features/owners/OwnerSelectLabel';
+
+/**
+ * An owners AsyncSelect option. The `label` is the rendered `OwnerSelectLabel`
+ * React element (not a string), so the plain-text name is carried separately on
+ * `OWNER_TEXT_LABEL_PROP` for the options the component constructs.
+ */
+export type OwnerOption = {
+  value: number;
+  label: ReactNode;
+  [OWNER_TEXT_LABEL_PROP]?: string;
+  [OWNER_EMAIL_PROP]?: string;
+};
+
+export type ParsedOwner = {
+  id: number;
+  full_name?: string;
+  first_name?: string;
+  last_name?: string;
+  email?: string;
+};
+
+/**
+ * Resolve the owner objects to persist when the owners AsyncSelect changes.
+ *
+ * AsyncSelect only caches the options the user has actually loaded or searched,
+ * so `options` can be a partial set that is missing owners which only ever
+ * existed in the controlled `value` prop. We therefore prefer the full owner
+ * object already in component state (it carries the real name/email from the
+ * API) and only fall back to the option cache — and finally a string label —
+ * for genuinely new owners. This prevents a removed owner from collapsing the
+ * remaining owners into nameless entries.
+ */
+export function parseSelectedOwners(
+  selectedOwners: OwnerOption[],
+  options: OwnerOption[],
+  existingOwners: ParsedOwner[],
+): ParsedOwner[] {
+  const optionsById = new Map(options.map(opt => [opt.value, opt]));
+  return ensureIsArray(selectedOwners).map(o => {
+    const existingOwner = existingOwners.find(ow => ow.id === o.value);
+    if (existingOwner) {
+      return existingOwner;
+    }
+    const opt = optionsById.get(o.value);
+    return {
+      id: o.value,
+      full_name:
+        opt?.[OWNER_TEXT_LABEL_PROP] ||
+        // `label` is a React element unless the option came from a plain-text
+        // source, so only use it as a name when it is actually a string.
+        (typeof o.label === 'string' ? o.label : ''),
+      email: opt?.[OWNER_EMAIL_PROP] || '',
+    };
+  });
+}

superset-frontend/src/utils/getOwnerName.test.ts

@@ -29,3 +29,15 @@ test('render owner name correctly', () => {
 test('return empty string for undefined owner', () => {
   expect(getOwnerName(undefined)).toEqual('');
 });
+
+test('return empty string when no name fields are set', () => {
+  expect(getOwnerName({ id: 1 })).toEqual('');
+});
+
+test('handle only first_name set', () => {
+  expect(getOwnerName({ id: 1, first_name: 'Foo' })).toEqual('Foo');
+});
+
+test('handle only last_name set', () => {
+  expect(getOwnerName({ id: 1, last_name: 'Bar' })).toEqual('Bar');
+});

superset-frontend/src/utils/getOwnerName.ts

@@ -22,5 +22,8 @@ export default function getOwnerName(owner?: Owner): string {
   if (!owner) {
     return '';
   }
-  return owner.full_name || `${owner.first_name} ${owner.last_name}`;
+  return (
+    owner.full_name ||
+    [owner.first_name, owner.last_name].filter(Boolean).join(' ')
+  );
 }