From ea9482128c7002bc5544d8bacb3fe47e9dba65ea Mon Sep 17 00:00:00 2001
From: czgu <cgu@pinterest.com>
Date: Tue, 8 Dec 2020 12:21:51 -0500
Subject: [PATCH] Fix selection behavior (#341)

https://github.com/pinterest/datahub/pull/341
---
 datahub/webapp/__tests__/ui/Select.test.js    | 43 ++++++++++++++++++-
 .../components/AppAdmin/AdminQueryEngine.tsx  |  8 +++-
 .../DataTableTags/TableTagSelect.tsx          |  1 +
 datahub/webapp/ui/Select/Select.tsx           | 14 ++----
 .../SimpleReactSelect/SimpleReactSelect.tsx   | 17 +++++++-
 5 files changed, 69 insertions(+), 14 deletions(-)

diff --git a/datahub/webapp/__tests__/ui/Select.test.js b/datahub/webapp/__tests__/ui/Select.test.js
index 27e5e79e6..bbdeeb068 100644
--- a/datahub/webapp/__tests__/ui/Select.test.js
+++ b/datahub/webapp/__tests__/ui/Select.test.js
@@ -1,13 +1,52 @@
 import React from 'react';
-import { shallow } from 'enzyme';
+import { shallow, mount } from 'enzyme';
 import toJson from 'enzyme-to-json';
 
-import { Select } from '../../ui/Select/Select';
+import { Select, makeSelectOptions } from '../../ui/Select/Select';
 
 it('renders without crashing', () => {
     shallow(<Select value="test" onChange={() => null} />);
 });
 
+describe('Select behavior', () => {
+    it('selects the correct field', () => {
+        const onChangeMock = jest.fn();
+        const wrapper = mount(
+            <Select
+                value="test"
+                onChange={(event) => onChangeMock(event.target.value)}
+            >
+                {makeSelectOptions(['test', 'test1', 'test2'])}
+            </Select>
+        );
+        wrapper.find('select').instance().selectedIndex = 2;
+        wrapper.find('select').simulate('change');
+
+        expect(wrapper.find('option')).toHaveLength(3);
+        expect(onChangeMock).toHaveBeenCalledTimes(1);
+        expect(onChangeMock).toHaveBeenCalledWith('test2');
+    });
+
+    it('deselects correctly', () => {
+        const onChangeMock = jest.fn();
+        const wrapper = mount(
+            <Select
+                value="test"
+                onChange={(event) => onChangeMock(event.target.value)}
+                withDeselect
+            >
+                {makeSelectOptions(['test', 'test1', 'test2'])}
+            </Select>
+        );
+        wrapper.find('select').instance().selectedIndex = 0;
+        wrapper.find('select').simulate('change');
+
+        expect(wrapper.find('option')).toHaveLength(4);
+        expect(onChangeMock).toHaveBeenCalledTimes(1);
+        expect(onChangeMock).toHaveBeenCalledWith('');
+    });
+});
+
 describe('matches enzyme snapshots', () => {
     it('matches snapshot', () => {
         const wrapper = shallow(<Select value="test" onChange={() => null} />);
diff --git a/datahub/webapp/components/AppAdmin/AdminQueryEngine.tsx b/datahub/webapp/components/AppAdmin/AdminQueryEngine.tsx
index d5eba0a1b..0b85137ba 100644
--- a/datahub/webapp/components/AppAdmin/AdminQueryEngine.tsx
+++ b/datahub/webapp/components/AppAdmin/AdminQueryEngine.tsx
@@ -121,7 +121,7 @@ export const AdminQueryEngine: React.FunctionComponent<IProps> = ({
                 language: queryEngine.language,
                 executor: queryEngine.executor,
                 executor_params: queryEngine.executor_params,
-                metastore_id: queryEngine.metastore_id,
+                metastore_id: queryEngine.metastore_id ?? null,
                 status_checker: queryEngine.status_checker,
             });
 
@@ -290,6 +290,12 @@ export const AdminQueryEngine: React.FunctionComponent<IProps> = ({
                                         value: metastore.name,
                                     })
                                 )}
+                                onChange={(value) => {
+                                    onChange(
+                                        'metastore_id',
+                                        value !== '' ? value : null
+                                    );
+                                }}
                                 withDeselect
                             />
                             <SimpleField
diff --git a/datahub/webapp/components/DataTableTags/TableTagSelect.tsx b/datahub/webapp/components/DataTableTags/TableTagSelect.tsx
index 6d85d1f7c..bf8dd8b56 100644
--- a/datahub/webapp/components/DataTableTags/TableTagSelect.tsx
+++ b/datahub/webapp/components/DataTableTags/TableTagSelect.tsx
@@ -84,6 +84,7 @@ export const TableTagSelect: React.FunctionComponent<IProps> = ({
                         }
                     },
                 }}
+                clearAfterSelect
             />
         </div>
     );
diff --git a/datahub/webapp/ui/Select/Select.tsx b/datahub/webapp/ui/Select/Select.tsx
index 47d7983dd..7c40f99ed 100644
--- a/datahub/webapp/ui/Select/Select.tsx
+++ b/datahub/webapp/ui/Select/Select.tsx
@@ -2,6 +2,7 @@ import React from 'react';
 import classNames from 'classnames';
 import './Select.scss';
 
+export const DESELECT_VALUE = '';
 export interface ISelectProps {
     name?: string;
     disabled?: boolean;
@@ -12,7 +13,6 @@ export interface ISelectProps {
     transparent?: boolean;
 
     withDeselect?: boolean;
-    deselectValue?: string;
 }
 
 export const Select: React.FunctionComponent<ISelectProps> = ({
@@ -20,17 +20,16 @@ export const Select: React.FunctionComponent<ISelectProps> = ({
     disabled,
     value,
     onChange,
-    className,
+    className = '',
     children,
 
     fullWidth,
     transparent,
 
     withDeselect,
-    deselectValue,
 }) => {
     const deselectOption = withDeselect ? (
-        <option value={deselectValue} key="">
+        <option value={DESELECT_VALUE} key="">
             {'Deselect'}
         </option>
     ) : null;
@@ -39,7 +38,7 @@ export const Select: React.FunctionComponent<ISelectProps> = ({
         <select
             name={name}
             disabled={disabled || false}
-            value={value ?? deselectValue}
+            value={value ?? DESELECT_VALUE}
             onChange={onChange}
         >
             {deselectOption}
@@ -65,11 +64,6 @@ export const Select: React.FunctionComponent<ISelectProps> = ({
     );
 };
 
-Select.defaultProps = {
-    className: '',
-    deselectValue: '',
-};
-
 export type IOptions = Array<
     | {
           key: any;
diff --git a/datahub/webapp/ui/SimpleReactSelect/SimpleReactSelect.tsx b/datahub/webapp/ui/SimpleReactSelect/SimpleReactSelect.tsx
index 23d720359..0f97ad04e 100644
--- a/datahub/webapp/ui/SimpleReactSelect/SimpleReactSelect.tsx
+++ b/datahub/webapp/ui/SimpleReactSelect/SimpleReactSelect.tsx
@@ -18,6 +18,9 @@ export interface ISimpleReactSelectProps<T> {
     withDeselect?: boolean;
     isDisabled?: boolean;
     selectProps?: Partial<ReactSelectProps<T>>;
+
+    // Clear selection user picks value
+    clearAfterSelect?: boolean;
 }
 
 const reactSelectStyle = makeReactSelectStyle(true);
@@ -26,10 +29,21 @@ export function SimpleReactSelect<T>({
     options,
     value,
     onChange,
-    withDeselect = false,
     isDisabled,
+
     selectProps = {},
+    withDeselect = false,
+    clearAfterSelect = false,
 }: ISimpleReactSelectProps<T>) {
+    const overrideSelectProps = useMemo(() => {
+        const override: Partial<ReactSelectProps<T>> = {};
+        if (clearAfterSelect) {
+            override.value = null;
+        }
+
+        return override;
+    }, [clearAfterSelect]);
+
     const computedOptions = useMemo(
         () =>
             (options || []).map((option) =>
@@ -64,6 +78,7 @@ export function SimpleReactSelect<T>({
             isClearable={withDeselect}
             menuPlacement={'auto'}
             {...selectProps}
+            {...overrideSelectProps}
         />
     );
 }