fix: Duplicate items when pasting into Select (apache#25447)

10webio · Sep 28, 2023 · 7cf96cd · 7cf96cd
1 parent 1a759ce
commit 7cf96cd
Show file tree

Hide file tree

Showing 5 changed files with 141 additions and 24 deletions.
diff --git a/superset-frontend/src/components/Select/AsyncSelect.test.tsx b/superset-frontend/src/components/Select/AsyncSelect.test.tsx
@@ -858,6 +858,45 @@ test('does not duplicate options when using numeric values', async () => {
   await waitFor(() => expect(getAllSelectOptions().length).toBe(1));
 });
 
+test('pasting an existing option does not duplicate it', async () => {
+  const options = jest.fn(async () => ({
+    data: [OPTIONS[0]],
+    totalCount: 1,
+  }));
+  render(<AsyncSelect {...defaultProps} options={options} />);
+  await open();
+  const input = getElementByClassName('.ant-select-selection-search-input');
+  const paste = createEvent.paste(input, {
+    clipboardData: {
+      getData: () => OPTIONS[0].label,
+    },
+  });
+  fireEvent(input, paste);
+  expect(await findAllSelectOptions()).toHaveLength(1);
+});
+
+test('pasting an existing option does not duplicate it in multiple mode', async () => {
+  const options = jest.fn(async () => ({
+    data: [
+      { label: 'John', value: 1 },
+      { label: 'Liam', value: 2 },
+      { label: 'Olivia', value: 3 },
+    ],
+    totalCount: 3,
+  }));
+  render(<AsyncSelect {...defaultProps} options={options} mode="multiple" />);
+  await open();
+  const input = getElementByClassName('.ant-select-selection-search-input');
+  const paste = createEvent.paste(input, {
+    clipboardData: {
+      getData: () => 'John,Liam,Peter',
+    },
+  });
+  fireEvent(input, paste);
+  // Only Peter should be added
+  expect(await findAllSelectOptions()).toHaveLength(4);
+});
+
 /*
  TODO: Add tests that require scroll interaction. Needs further investigation.
  - Fetches more data when scrolling and more data is available

diff --git a/superset-frontend/src/components/Select/AsyncSelect.tsx b/superset-frontend/src/components/Select/AsyncSelect.tsx
@@ -49,6 +49,8 @@ import {
   dropDownRenderHelper,
   handleFilterOptionHelper,
   mapOptions,
+  getOption,
+  isObject,
 } from './utils';
 import {
   AsyncSelectProps,
@@ -523,19 +525,33 @@ const AsyncSelect = forwardRef(
       [ref],
     );
 
+    const getPastedTextValue = useCallback(
+      (text: string) => {
+        const option = getOption(text, fullSelectOptions, true);
+        const value: AntdLabeledValue = {
+          label: text,
+          value: text,
+        };
+        if (option) {
+          value.label = isObject(option) ? option.label : option;
+          value.value = isObject(option) ? option.value! : option;
+        }
+        return value;
+      },
+      [fullSelectOptions],
+    );
+
     const onPaste = (e: ClipboardEvent<HTMLInputElement>) => {
       const pastedText = e.clipboardData.getData('text');
       if (isSingleMode) {
-        setSelectValue({ label: pastedText, value: pastedText });
+        setSelectValue(getPastedTextValue(pastedText));
       } else {
         const token = tokenSeparators.find(token => pastedText.includes(token));
         const array = token ? uniq(pastedText.split(token)) : [pastedText];
+        const values = array.map(item => getPastedTextValue(item));
         setSelectValue(previous => [
           ...((previous || []) as AntdLabeledValue[]),
-          ...array.map<AntdLabeledValue>(value => ({
-            label: value,
-            value,
-          })),
+          ...values,
         ]);
       }
     };

diff --git a/superset-frontend/src/components/Select/Select.test.tsx b/superset-frontend/src/components/Select/Select.test.tsx
@@ -972,6 +972,45 @@ test('does not duplicate options when using numeric values', async () => {
   await waitFor(() => expect(getAllSelectOptions().length).toBe(1));
 });
 
+test('pasting an existing option does not duplicate it', async () => {
+  render(<Select {...defaultProps} options={[OPTIONS[0]]} />);
+  await open();
+  const input = getElementByClassName('.ant-select-selection-search-input');
+  const paste = createEvent.paste(input, {
+    clipboardData: {
+      getData: () => OPTIONS[0].label,
+    },
+  });
+  fireEvent(input, paste);
+  expect(await findAllSelectOptions()).toHaveLength(1);
+});
+
+test('pasting an existing option does not duplicate it in multiple mode', async () => {
+  const options = [
+    { label: 'John', value: 1 },
+    { label: 'Liam', value: 2 },
+    { label: 'Olivia', value: 3 },
+  ];
+  render(
+    <Select
+      {...defaultProps}
+      options={options}
+      mode="multiple"
+      allowSelectAll={false}
+    />,
+  );
+  await open();
+  const input = getElementByClassName('.ant-select-selection-search-input');
+  const paste = createEvent.paste(input, {
+    clipboardData: {
+      getData: () => 'John,Liam,Peter',
+    },
+  });
+  fireEvent(input, paste);
+  // Only Peter should be added
+  expect(await findAllSelectOptions()).toHaveLength(4);
+});
+
 /*
  TODO: Add tests that require scroll interaction. Needs further investigation.
  - Fetches more data when scrolling and more data is available

diff --git a/superset-frontend/src/components/Select/Select.tsx b/superset-frontend/src/components/Select/Select.tsx
@@ -51,6 +51,8 @@ import {
   mapValues,
   mapOptions,
   hasCustomLabels,
+  getOption,
+  isObject,
 } from './utils';
 import { RawValue, SelectOptionsType, SelectProps } from './types';
 import {
@@ -530,27 +532,42 @@ const Select = forwardRef(
       actualMaxTagCount -= 1;
     }
 
+    const getPastedTextValue = useCallback(
+      (text: string) => {
+        const option = getOption(text, fullSelectOptions, true);
+        if (labelInValue) {
+          const value: AntdLabeledValue = {
+            label: text,
+            value: text,
+          };
+          if (option) {
+            value.label = isObject(option) ? option.label : option;
+            value.value = isObject(option) ? option.value! : option;
+          }
+          return value;
+        }
+        return option ? (isObject(option) ? option.value! : option) : text;
+      },
+      [fullSelectOptions, labelInValue],
+    );
+
     const onPaste = (e: ClipboardEvent<HTMLInputElement>) => {
       const pastedText = e.clipboardData.getData('text');
       if (isSingleMode) {
-        setSelectValue(
-          labelInValue ? { label: pastedText, value: pastedText } : pastedText,
-        );
+        setSelectValue(getPastedTextValue(pastedText));
       } else {
         const token = tokenSeparators.find(token => pastedText.includes(token));
         const array = token ? uniq(pastedText.split(token)) : [pastedText];
+        const values = array.map(item => getPastedTextValue(item));
         if (labelInValue) {
           setSelectValue(previous => [
             ...((previous || []) as AntdLabeledValue[]),
-            ...array.map<AntdLabeledValue>(value => ({
-              label: value,
-              value,
-            })),
+            ...(values as AntdLabeledValue[]),
           ]);
         } else {
           setSelectValue(previous => [
             ...((previous || []) as string[]),
-            ...array,
+            ...(values as string[]),
           ]);
         }
       }

diff --git a/superset-frontend/src/components/Select/utils.tsx b/superset-frontend/src/components/Select/utils.tsx
@@ -49,27 +49,33 @@ export function getValue(
   return isLabeledValue(option) ? option.value : option;
 }
 
-export function hasOption(
+export function getOption(
   value: V,
   options?: V | LabeledValue | (V | LabeledValue)[],
   checkLabel = false,
-): boolean {
+): V | LabeledValue {
   const optionsArray = ensureIsArray(options);
   // When comparing the values we use the equality
   // operator to automatically convert different types
-  return (
-    optionsArray.find(
-      x =>
+  return optionsArray.find(
+    x =>
+      // eslint-disable-next-line eqeqeq
+      x == value ||
+      (isObject(x) &&
         // eslint-disable-next-line eqeqeq
-        x == value ||
-        (isObject(x) &&
-          // eslint-disable-next-line eqeqeq
-          (('value' in x && x.value == value) ||
-            (checkLabel && 'label' in x && x.label === value))),
-    ) !== undefined
+        (('value' in x && x.value == value) ||
+          (checkLabel && 'label' in x && x.label === value))),
   );
 }
 
+export function hasOption(
+  value: V,
+  options?: V | LabeledValue | (V | LabeledValue)[],
+  checkLabel = false,
+): boolean {
+  return getOption(value, options, checkLabel) !== undefined;
+}
+
 /**
  * It creates a comparator to check for a specific property.
  * Can be used with string and number property values.