From fe1b00d8f4e50c2aab4a68f456dc01b064285914 Mon Sep 17 00:00:00 2001
From: "Michael S. Molina" <70410625+michael-s-molina@users.noreply.github.com>
Date: Thu, 28 Mar 2024 11:32:00 -0300
Subject: [PATCH] fix: Select onChange is fired when the same item is selected
 in single mode (#27706)

---
 .../src/components/Select/AsyncSelect.test.tsx | 12 ++++++++++++
 .../src/components/Select/AsyncSelect.tsx      | 13 ++++++++++++-
 .../src/components/Select/Select.test.tsx      | 12 ++++++++++++
 .../src/components/Select/Select.tsx           | 12 +++++++++++-
 .../src/components/Select/utils.tsx            | 18 ++++++++++--------
 5 files changed, 57 insertions(+), 10 deletions(-)

diff --git a/superset-frontend/src/components/Select/AsyncSelect.test.tsx b/superset-frontend/src/components/Select/AsyncSelect.test.tsx
index 4a2ba0007c432..8e8e151b6641f 100644
--- a/superset-frontend/src/components/Select/AsyncSelect.test.tsx
+++ b/superset-frontend/src/components/Select/AsyncSelect.test.tsx
@@ -939,6 +939,18 @@ test('pasting an existing option does not duplicate it in multiple mode', async
   expect(await findAllSelectOptions()).toHaveLength(4);
 });
 
+test('does not fire onChange if the same value is selected in single mode', async () => {
+  const onChange = jest.fn();
+  render(<AsyncSelect {...defaultProps} onChange={onChange} />);
+  const optionText = 'Emma';
+  await open();
+  expect(onChange).toHaveBeenCalledTimes(0);
+  userEvent.click(await findSelectOption(optionText));
+  expect(onChange).toHaveBeenCalledTimes(1);
+  userEvent.click(await findSelectOption(optionText));
+  expect(onChange).toHaveBeenCalledTimes(1);
+});
+
 /*
  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
index d41c87f0478ce..3fb8bceaf432f 100644
--- a/superset-frontend/src/components/Select/AsyncSelect.tsx
+++ b/superset-frontend/src/components/Select/AsyncSelect.tsx
@@ -50,10 +50,12 @@ import {
   mapOptions,
   getOption,
   isObject,
+  isEqual as utilsIsEqual,
 } from './utils';
 import {
   AsyncSelectProps,
   AsyncSelectRef,
+  RawValue,
   SelectOptionsPagePromise,
   SelectOptionsType,
   SelectOptionsTypePage,
@@ -220,7 +222,16 @@ const AsyncSelect = forwardRef(
 
     const handleOnSelect: SelectProps['onSelect'] = (selectedItem, option) => {
       if (isSingleMode) {
+        // on select is fired in single value mode if the same value is selected
+        const valueChanged = !utilsIsEqual(
+          selectedItem,
+          selectValue as RawValue | AntdLabeledValue,
+          'value',
+        );
         setSelectValue(selectedItem);
+        if (valueChanged) {
+          fireOnChange();
+        }
       } else {
         setSelectValue(previousState => {
           const array = ensureIsArray(previousState);
@@ -234,8 +245,8 @@ const AsyncSelect = forwardRef(
           }
           return previousState;
         });
+        fireOnChange();
       }
-      fireOnChange();
       onSelect?.(selectedItem, option);
     };
 
diff --git a/superset-frontend/src/components/Select/Select.test.tsx b/superset-frontend/src/components/Select/Select.test.tsx
index 2910353295187..1daff06d4de77 100644
--- a/superset-frontend/src/components/Select/Select.test.tsx
+++ b/superset-frontend/src/components/Select/Select.test.tsx
@@ -1053,6 +1053,18 @@ test('pasting an existing option does not duplicate it in multiple mode', async
   expect(await findAllSelectOptions()).toHaveLength(4);
 });
 
+test('does not fire onChange if the same value is selected in single mode', async () => {
+  const onChange = jest.fn();
+  render(<Select {...defaultProps} onChange={onChange} />);
+  const optionText = 'Emma';
+  await open();
+  expect(onChange).toHaveBeenCalledTimes(0);
+  userEvent.click(await findSelectOption(optionText));
+  expect(onChange).toHaveBeenCalledTimes(1);
+  userEvent.click(await findSelectOption(optionText));
+  expect(onChange).toHaveBeenCalledTimes(1);
+});
+
 /*
  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
index f4f9565abb8a7..3db455cbe2714 100644
--- a/superset-frontend/src/components/Select/Select.tsx
+++ b/superset-frontend/src/components/Select/Select.tsx
@@ -53,6 +53,7 @@ import {
   hasCustomLabels,
   getOption,
   isObject,
+  isEqual as utilsIsEqual,
 } from './utils';
 import { RawValue, SelectOptionsType, SelectProps } from './types';
 import {
@@ -227,7 +228,16 @@ const Select = forwardRef(
 
     const handleOnSelect: SelectProps['onSelect'] = (selectedItem, option) => {
       if (isSingleMode) {
+        // on select is fired in single value mode if the same value is selected
+        const valueChanged = !utilsIsEqual(
+          selectedItem,
+          selectValue as RawValue | AntdLabeledValue,
+          'value',
+        );
         setSelectValue(selectedItem);
+        if (valueChanged) {
+          fireOnChange();
+        }
       } else {
         setSelectValue(previousState => {
           const array = ensureIsArray(previousState);
@@ -259,8 +269,8 @@ const Select = forwardRef(
           }
           return previousState;
         });
+        fireOnChange();
       }
-      fireOnChange();
       onSelect?.(selectedItem, option);
     };
 
diff --git a/superset-frontend/src/components/Select/utils.tsx b/superset-frontend/src/components/Select/utils.tsx
index 0b638f4f0128f..67b2a0191b385 100644
--- a/superset-frontend/src/components/Select/utils.tsx
+++ b/superset-frontend/src/components/Select/utils.tsx
@@ -49,22 +49,24 @@ export function getValue(
   return isLabeledValue(option) ? option.value : option;
 }
 
+export function isEqual(a: V | LabeledValue, b: V | LabeledValue, key: string) {
+  const actualA = isObject(a) && key in a ? a[key] : a;
+  const actualB = isObject(b) && key in b ? b[key] : b;
+  // When comparing the values we use the equality
+  // operator to automatically convert different types
+  // eslint-disable-next-line eqeqeq
+  return actualA == actualB;
+}
+
 export function getOption(
   value: V,
   options?: V | LabeledValue | (V | LabeledValue)[],
   checkLabel = false,
 ): V | LabeledValue {
   const optionsArray = ensureIsArray(options);
-  // When comparing the values we use the equality
-  // operator to automatically convert different types
   return optionsArray.find(
     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))),
+      isEqual(x, value, 'value') || (checkLabel && isEqual(x, value, 'label')),
   );
 }