From 8a57a71bed30a781a1d5e5b2ce42ccd08045b3e9 Mon Sep 17 00:00:00 2001
From: Antonio Rivero Martinez
 <38889534+Antonio-RiveroMartnez@users.noreply.github.com>
Date: Fri, 1 Jul 2022 17:40:13 -0300
Subject: [PATCH] fix(sql lab): Save Dataset Modal Autocomplete should display
 list when overwritting (#20512)

* Save Dataset Modal:

- Use our Select component as substitute of the Autocomplete one so options are loaded initially without the user having to trigger a search and we are mosre consistent with the rest of the app
- Changing datasetId to lowercase so when custom props get into the DOM we don't get warning related to invalid formatting
- We extracted the dropdown out of the radio because it causes invalid click handling when an option is selected
- Updated tests

* Save Dataset Modal:

- Update missing test for DatasourceControl

* Save Dataset Modal:

- Remove conditional from load options function since only guest users dont have userId, and if that is the case they wont reach this part of the application

* Save Dataset Modal:

- Remove unused comment

* Save Dataset Modal:

- Add getPopupContainer as prop for Select component

* Save Dataset Modal:

- Add tests for our new getPopupContainer prop in Select component. So if passed it gets called.

* Save Dataset Modal:

- use lowercased property when calling post form data

* Save Dataset Modal:

- Update tests so there is no need to define a null returning func

* Save Dataset Modal:

- Including getPopupContainer from PickedSelectProps instead
- Updating definition in SelectFilterPlugin
---
 .../SaveDatasetModal.test.tsx                 |   2 +-
 .../components/SaveDatasetModal/index.tsx     | 121 +++++++++---------
 .../src/components/Select/Select.test.tsx     |  22 ++++
 .../src/components/Select/Select.tsx          |   6 +-
 .../DatasourceControl.test.tsx                |   4 +-
 .../components/Select/SelectFilterPlugin.tsx  |   5 +-
 6 files changed, 93 insertions(+), 67 deletions(-)

diff --git a/superset-frontend/src/SqlLab/components/SaveDatasetModal/SaveDatasetModal.test.tsx b/superset-frontend/src/SqlLab/components/SaveDatasetModal/SaveDatasetModal.test.tsx
index c35b5eb2b..d0698e3ed 100644
--- a/superset-frontend/src/SqlLab/components/SaveDatasetModal/SaveDatasetModal.test.tsx
+++ b/superset-frontend/src/SqlLab/components/SaveDatasetModal/SaveDatasetModal.test.tsx
@@ -50,7 +50,7 @@ describe('SaveDatasetModal RTL', () => {
     render(<SaveDatasetModal {...mockedProps} />, { useRedux: true });
 
     const overwriteRadioBtn = screen.getByRole('radio', {
-      name: /overwrite existing select or type dataset name/i,
+      name: /overwrite existing/i,
     });
     const fieldLabel = screen.getByText(/overwrite existing/i);
     const inputField = screen.getByRole('combobox');
diff --git a/superset-frontend/src/SqlLab/components/SaveDatasetModal/index.tsx b/superset-frontend/src/SqlLab/components/SaveDatasetModal/index.tsx
index 4a87d52c1..4fbfe11ad 100644
--- a/superset-frontend/src/SqlLab/components/SaveDatasetModal/index.tsx
+++ b/superset-frontend/src/SqlLab/components/SaveDatasetModal/index.tsx
@@ -17,9 +17,9 @@
  * under the License.
  */
 
-import React, { FunctionComponent, useState } from 'react';
+import React, { FunctionComponent, useCallback, useState } from 'react';
 import { Radio } from 'src/components/Radio';
-import { AutoComplete, RadioChangeEvent } from 'src/components';
+import { RadioChangeEvent, Select } from 'src/components';
 import { Input } from 'src/components/Input';
 import StyledModal from 'src/components/Modal';
 import Button from 'src/components/Button';
@@ -27,7 +27,6 @@ import {
   styled,
   t,
   SupersetClient,
-  makeApi,
   JsonResponse,
   JsonObject,
   QueryResponse,
@@ -42,7 +41,6 @@ import {
   DatasetRadioState,
   EXPLORE_CHART_DEFAULT,
   DatasetOwner,
-  DatasetOptionAutocomplete,
   SqlLabExploreRootState,
   getInitialState,
   ExploreDatasource,
@@ -51,6 +49,8 @@ import {
 import { mountExploreUrl } from 'src/explore/exploreUtils';
 import { postFormData } from 'src/explore/exploreUtils/formData';
 import { URL_PARAMS } from 'src/constants';
+import { SelectValue } from 'antd/lib/select';
+import { isEmpty } from 'lodash';
 
 interface SaveDatasetModalProps {
   visible: boolean;
@@ -70,8 +70,8 @@ const Styles = styled.div`
     width: 401px;
   }
   .sdm-autocomplete {
-    margin-left: 8px;
     width: 401px;
+    align-self: center;
   }
   .sdm-radio {
     display: block;
@@ -82,6 +82,10 @@ const Styles = styled.div`
   .sdm-overwrite-msg {
     margin: 7px;
   }
+  .sdm-overwrite-container {
+    flex: 1 1 auto;
+    display: flex;
+  }
 `;
 
 const updateDataset = async (
@@ -129,13 +133,12 @@ export const SaveDatasetModal: FunctionComponent<SaveDatasetModalProps> = ({
     DatasetRadioState.SAVE_NEW,
   );
   const [shouldOverwriteDataset, setShouldOverwriteDataset] = useState(false);
-  const [userDatasetOptions, setUserDatasetOptions] = useState<
-    DatasetOptionAutocomplete[]
-  >([]);
   const [datasetToOverwrite, setDatasetToOverwrite] = useState<
     Record<string, any>
   >({});
-  const [autocompleteValue, setAutocompleteValue] = useState('');
+  const [selectedDatasetToOverwrite, setSelectedDatasetToOverwrite] = useState<
+    SelectValue | undefined
+  >(undefined);
 
   const user = useSelector<SqlLabExploreRootState, User>(user =>
     getInitialState(user),
@@ -146,7 +149,7 @@ export const SaveDatasetModal: FunctionComponent<SaveDatasetModalProps> = ({
     const [, key] = await Promise.all([
       updateDataset(
         query.dbId,
-        datasetToOverwrite.datasetId,
+        datasetToOverwrite.datasetid,
         query.sql,
         query.results.selected_columns.map(
           (d: { name: string; type: string; is_dttm: boolean }) => ({
@@ -158,9 +161,9 @@ export const SaveDatasetModal: FunctionComponent<SaveDatasetModalProps> = ({
         datasetToOverwrite.owners.map((o: DatasetOwner) => o.id),
         true,
       ),
-      postFormData(datasetToOverwrite.datasetId, 'table', {
+      postFormData(datasetToOverwrite.datasetid, 'table', {
         ...EXPLORE_CHART_DEFAULT,
-        datasource: `${datasetToOverwrite.datasetId}__table`,
+        datasource: `${datasetToOverwrite.datasetid}__table`,
         ...(defaultVizType === 'table' && {
           all_columns: query.results.selected_columns.map(
             column => column.name,
@@ -179,17 +182,15 @@ export const SaveDatasetModal: FunctionComponent<SaveDatasetModalProps> = ({
     setDatasetName(getDefaultDatasetName());
   };
 
-  const getUserDatasets = async (searchText = '') => {
-    // Making sure that autocomplete input has a value before rendering the dropdown
-    // Transforming the userDatasetsOwned data for SaveModalComponent)
-    const { userId } = user;
-    if (userId) {
+  const loadDatasetOverwriteOptions = useCallback(
+    async (input = '') => {
+      const { userId } = user;
       const queryParams = rison.encode({
         filters: [
           {
             col: 'table_name',
             opr: 'ct',
-            value: searchText,
+            value: input,
           },
           {
             col: 'owners',
@@ -201,22 +202,22 @@ export const SaveDatasetModal: FunctionComponent<SaveDatasetModalProps> = ({
         order_direction: 'desc',
       });
 
-      const response = await makeApi({
-        method: 'GET',
-        endpoint: '/api/v1/dataset',
-      })(`q=${queryParams}`);
-
-      return response.result.map(
-        (r: { table_name: string; id: number; owners: [DatasetOwner] }) => ({
-          value: r.table_name,
-          datasetId: r.id,
-          owners: r.owners,
-        }),
-      );
-    }
-
-    return null;
-  };
+      return SupersetClient.get({
+        endpoint: `/api/v1/dataset?q=${queryParams}`,
+      }).then(response => ({
+        data: response.json.result.map(
+          (r: { table_name: string; id: number; owners: [DatasetOwner] }) => ({
+            value: r.table_name,
+            label: r.table_name,
+            datasetid: r.id,
+            owners: r.owners,
+          }),
+        ),
+        totalCount: response.json.count,
+      }));
+    },
+    [user],
+  );
 
   const handleSaveInDataset = () => {
     // if user wants to overwrite a dataset we need to prompt them
@@ -274,16 +275,11 @@ export const SaveDatasetModal: FunctionComponent<SaveDatasetModalProps> = ({
     onHide();
   };
 
-  const handleSaveDatasetModalSearch = async (searchText: string) => {
-    const userDatasetsOwned = await getUserDatasets(searchText);
-    setUserDatasetOptions(userDatasetsOwned);
+  const handleOverwriteDatasetOption = (value: SelectValue, option: any) => {
+    setDatasetToOverwrite(option);
+    setSelectedDatasetToOverwrite(value);
   };
 
-  const handleOverwriteDatasetOption = (
-    _data: string,
-    option: Record<string, any>,
-  ) => setDatasetToOverwrite(option);
-
   const handleDatasetNameChange = (e: React.FormEvent<HTMLInputElement>) => {
     // @ts-expect-error
     setDatasetName(e.target.value);
@@ -298,12 +294,11 @@ export const SaveDatasetModal: FunctionComponent<SaveDatasetModalProps> = ({
     (newOrOverwrite === DatasetRadioState.SAVE_NEW &&
       datasetName.length === 0) ||
     (newOrOverwrite === DatasetRadioState.OVERWRITE_DATASET &&
-      Object.keys(datasetToOverwrite).length === 0 &&
-      autocompleteValue.length === 0);
+      isEmpty(selectedDatasetToOverwrite));
 
   const filterAutocompleteOption = (
     inputValue: string,
-    option: { value: string; datasetId: number },
+    option: { value: string; datasetid: number },
   ) => option.value.toLowerCase().includes(inputValue.toLowerCase());
 
   return (
@@ -359,23 +354,25 @@ export const SaveDatasetModal: FunctionComponent<SaveDatasetModalProps> = ({
                   disabled={newOrOverwrite !== 1}
                 />
               </Radio>
-              <Radio className="sdm-radio" value={2}>
-                {t('Overwrite existing')}
-                <AutoComplete
-                  className="sdm-autocomplete"
-                  options={userDatasetOptions}
-                  onSelect={handleOverwriteDatasetOption}
-                  onSearch={handleSaveDatasetModalSearch}
-                  onChange={value => {
-                    setDatasetToOverwrite({});
-                    setAutocompleteValue(value);
-                  }}
-                  placeholder={t('Select or type dataset name')}
-                  filterOption={filterAutocompleteOption}
-                  disabled={newOrOverwrite !== 2}
-                  value={autocompleteValue}
-                />
-              </Radio>
+              <div className="sdm-overwrite-container">
+                <Radio className="sdm-radio" value={2}>
+                  {t('Overwrite existing')}
+                </Radio>
+                <div className="sdm-autocomplete">
+                  <Select
+                    allowClear
+                    showSearch
+                    placeholder={t('Select or type dataset name')}
+                    ariaLabel={t('Existing dataset')}
+                    onChange={handleOverwriteDatasetOption}
+                    options={input => loadDatasetOverwriteOptions(input)}
+                    value={selectedDatasetToOverwrite}
+                    filterOption={filterAutocompleteOption}
+                    disabled={newOrOverwrite !== 2}
+                    getPopupContainer={() => document.body}
+                  />
+                </div>
+              </div>
             </Radio.Group>
           </div>
         )}
diff --git a/superset-frontend/src/components/Select/Select.test.tsx b/superset-frontend/src/components/Select/Select.test.tsx
index 97aec1c08..1515a7c54 100644
--- a/superset-frontend/src/components/Select/Select.test.tsx
+++ b/superset-frontend/src/components/Select/Select.test.tsx
@@ -829,6 +829,28 @@ test('async - requests the options again after clearing the cache', async () =>
   expect(mock).toHaveBeenCalledTimes(2);
 });
 
+test('async - triggers getPopupContainer if passed', async () => {
+  const getPopupContainer = jest.fn();
+  render(
+    <div>
+      <Select
+        {...defaultProps}
+        options={loadOptions}
+        getPopupContainer={getPopupContainer}
+      />
+    </div>,
+  );
+  await open();
+  expect(getPopupContainer).toHaveBeenCalled();
+});
+
+test('static - triggers getPopupContainer if passed', async () => {
+  const getPopupContainer = jest.fn();
+  render(<Select {...defaultProps} getPopupContainer={getPopupContainer} />);
+  await open();
+  expect(getPopupContainer).toHaveBeenCalled();
+});
+
 /*
  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 591fe350c..9cea5a726 100644
--- a/superset-frontend/src/components/Select/Select.tsx
+++ b/superset-frontend/src/components/Select/Select.tsx
@@ -67,6 +67,7 @@ type PickedSelectProps = Pick<
   | 'showSearch'
   | 'tokenSeparators'
   | 'value'
+  | 'getPopupContainer'
 >;
 
 export type OptionsType = Exclude<AntdSelectAllProps['options'], undefined>;
@@ -316,6 +317,7 @@ const Select = (
     sortComparator = DEFAULT_SORT_COMPARATOR,
     tokenSeparators,
     value,
+    getPopupContainer,
     ...props
   }: SelectProps,
   ref: RefObject<SelectRef>,
@@ -701,7 +703,9 @@ const Select = (
         dropdownRender={dropdownRender}
         filterOption={handleFilterOption}
         filterSort={sortComparatorWithSearch}
-        getPopupContainer={triggerNode => triggerNode.parentNode}
+        getPopupContainer={
+          getPopupContainer || (triggerNode => triggerNode.parentNode)
+        }
         labelInValue={isAsync || labelInValue}
         maxTagCount={MAX_TAG_COUNT}
         mode={mappedMode}
diff --git a/superset-frontend/src/explore/components/controls/DatasourceControl/DatasourceControl.test.tsx b/superset-frontend/src/explore/components/controls/DatasourceControl/DatasourceControl.test.tsx
index 8a65a1139..46ecbeae5 100644
--- a/superset-frontend/src/explore/components/controls/DatasourceControl/DatasourceControl.test.tsx
+++ b/superset-frontend/src/explore/components/controls/DatasourceControl/DatasourceControl.test.tsx
@@ -199,10 +199,12 @@ test('Click on Save as dataset', () => {
     name: /save as new undefined/i,
   });
   const overwriteRadioBtn = screen.getByRole('radio', {
-    name: /overwrite existing select or type dataset name/i,
+    name: /overwrite existing/i,
   });
+  const dropdownField = screen.getByText(/select or type dataset name/i);
   expect(saveRadioBtn).toBeVisible();
   expect(overwriteRadioBtn).toBeVisible();
   expect(screen.getByRole('button', { name: /save/i })).toBeVisible();
   expect(screen.getByRole('button', { name: /close/i })).toBeVisible();
+  expect(dropdownField).toBeVisible();
 });
diff --git a/superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx b/superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx
index 9803e4a7e..936c6d548 100644
--- a/superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx
+++ b/superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx
@@ -307,8 +307,9 @@ export default function PluginFilterSelect(props: PluginFilterSelectProps) {
           disabled={isDisabled}
           getPopupContainer={
             showOverflow
-              ? () => parentRef?.current
-              : (trigger: HTMLElement) => trigger?.parentNode
+              ? () => (parentRef?.current as HTMLElement) || document.body
+              : (trigger: HTMLElement) =>
+                  (trigger?.parentNode as HTMLElement) || document.body
           }
           showSearch={showSearch}
           mode={multiSelect ? 'multiple' : 'single'}