feat(explore): Don't discard controls with custom sql when changing d…

…atasource (#20934)
apache · Oct 19, 2022 · cddc361 · cddc361
1 parent ec20c01
commit cddc361
Show file tree

Hide file tree

Showing 22 changed files with 190 additions and 180 deletions.
diff --git a/superset-frontend/packages/superset-ui-core/src/query/types/Column.ts b/superset-frontend/packages/superset-ui-core/src/query/types/Column.ts
@@ -29,6 +29,7 @@ export interface AdhocColumn {
   expressionType: 'SQL';
   columnType?: 'BASE_AXIS' | 'SERIES';
   timeGrain?: string;
+  datasourceWarning?: boolean;
 }
 
 /**

diff --git a/superset-frontend/packages/superset-ui-core/src/utils/isDefined.ts b/superset-frontend/packages/superset-ui-core/src/utils/isDefined.ts
@@ -17,6 +17,6 @@
  * under the License.
  */
 
-export default function isDefined(x: unknown) {
+export default function isDefined<T>(x: T): x is NonNullable<T> {
   return x !== null && x !== undefined;
 }
diff --git a/superset-frontend/src/explore/components/ControlPanelsContainer.tsx b/superset-frontend/src/explore/components/ControlPanelsContainer.tsx
@@ -36,6 +36,7 @@ import {
   css,
   SupersetTheme,
   useTheme,
+  isDefined,
 } from '@superset-ui/core';
 import {
   ControlPanelSectionConfig,
@@ -45,6 +46,9 @@ import {
   ExpandedControlItem,
   sections,
 } from '@superset-ui/chart-controls';
+import { useSelector } from 'react-redux';
+import { rgba } from 'emotion-rgba';
+import { kebabCase } from 'lodash';
 
 import Collapse from 'src/components/Collapse';
 import Tabs from 'src/components/Tabs';
@@ -57,9 +61,6 @@ import { ExploreActions } from 'src/explore/actions/exploreActions';
 import { ChartState, ExplorePageState } from 'src/explore/types';
 import { Tooltip } from 'src/components/Tooltip';
 import Icons from 'src/components/Icons';
-
-import { rgba } from 'emotion-rgba';
-import { kebabCase } from 'lodash';
 import ControlRow from './ControlRow';
 import Control from './Control';
 import { ExploreAlert } from './ExploreAlert';
@@ -269,6 +270,36 @@ export const ControlPanelsContainer = (props: ControlPanelsContainerProps) => {
 
   const containerRef = useRef<HTMLDivElement>(null);
 
+  const controlsTransferred = useSelector<
+    ExplorePageState,
+    string[] | undefined
+  >(state => state.explore.controlsTransferred);
+
+  useEffect(() => {
+    if (props.chart.chartStatus === 'success') {
+      controlsTransferred?.forEach(controlName => {
+        const alteredControls = ensureIsArray(
+          props.controls[controlName].value,
+        ).map(value => {
+          if (
+            typeof value === 'object' &&
+            isDefined(value) &&
+            'datasourceWarning' in value
+          ) {
+            return { ...value, datasourceWarning: false };
+          }
+          return value;
+        });
+        props.actions.setControlValue(controlName, alteredControls);
+      });
+    }
+  }, [
+    controlsTransferred,
+    props.actions,
+    props.chart.chartStatus,
+    props.controls,
+  ]);
+
   useEffect(() => {
     if (
       prevDatasource &&
@@ -455,7 +486,7 @@ export const ControlPanelsContainer = (props: ControlPanelsContainerProps) => {
           >
             <Icons.InfoCircleOutlined
               css={css`
-                ${iconStyles}
+                ${iconStyles};
                 color: ${errorColor};
               `}
             />
@@ -591,7 +622,7 @@ export const ControlPanelsContainer = (props: ControlPanelsContainerProps) => {
             >
               <Icons.ExclamationCircleOutlined
                 css={css`
-                  ${iconStyles}
+                  ${iconStyles};
                   color: ${errorColor};
                 `}
               />

diff --git a/superset-frontend/src/explore/components/ExploreViewContainer/index.jsx b/superset-frontend/src/explore/components/ExploreViewContainer/index.jsx
@@ -458,6 +458,7 @@ function ExploreViewContainer(props) {
           !areObjectsEqual(
             props.controls[key].value,
             lastQueriedControls[key].value,
+            { ignoreFields: ['datasourceWarning'] },
           ),
       );
 

diff --git a/...-frontend/src/explore/components/controls/DndColumnSelectControl/DndAdhocFilterOption.tsx b/...-frontend/src/explore/components/controls/DndColumnSelectControl/DndAdhocFilterOption.tsx
@@ -17,6 +17,7 @@
  * under the License.
  */
 import React from 'react';
+import { t } from '@superset-ui/core';
 import { DndItemType } from 'src/explore/components/DndItemType';
 import AdhocFilterPopoverTrigger from 'src/explore/components/controls/FilterControl/AdhocFilterPopoverTrigger';
 import AdhocFilter from 'src/explore/components/controls/FilterControl/AdhocFilter';
@@ -63,6 +64,11 @@ export default function DndAdhocFilterOption({
         type={DndItemType.FilterOption}
         withCaret
         isExtra={adhocFilter.isExtra}
+        datasourceWarningMessage={
+          adhocFilter.datasourceWarning
+            ? t('This filter might be incompatible with current dataset')
+            : undefined
+        }
       />
     </AdhocFilterPopoverTrigger>
   );

diff --git a/superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndColumnSelect.tsx b/superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndColumnSelect.tsx
@@ -23,6 +23,8 @@ import {
   isFeatureEnabled,
   tn,
   QueryFormColumn,
+  t,
+  isAdhocColumn,
 } from '@superset-ui/core';
 import {
   ColumnMeta,
@@ -35,7 +37,6 @@ import OptionWrapper from 'src/explore/components/controls/DndColumnSelectContro
 import { OptionSelector } from 'src/explore/components/controls/DndColumnSelectControl/utils';
 import { DatasourcePanelDndItem } from 'src/explore/components/DatasourcePanel/types';
 import { DndItemType } from 'src/explore/components/DndItemType';
-import { useComponentDidUpdate } from 'src/hooks/useComponentDidUpdate';
 import ColumnSelectPopoverTrigger from './ColumnSelectPopoverTrigger';
 import { DndControlProps } from './types';
 import SelectControl from '../SelectControl';
@@ -68,34 +69,6 @@ function DndColumnSelect(props: DndColumnSelectProps) {
     return new OptionSelector(optionsMap, multi, value);
   }, [multi, options, value]);
 
-  // synchronize values in case of dataset changes
-  const handleOptionsChange = useCallback(() => {
-    const optionSelectorValues = optionSelector.getValues();
-    if (typeof value !== typeof optionSelectorValues) {
-      onChange(optionSelectorValues);
-    }
-    if (
-      typeof value === 'string' &&
-      typeof optionSelectorValues === 'string' &&
-      value !== optionSelectorValues
-    ) {
-      onChange(optionSelectorValues);
-    }
-    if (
-      Array.isArray(optionSelectorValues) &&
-      Array.isArray(value) &&
-      (optionSelectorValues.length !== value.length ||
-        optionSelectorValues.every((val, index) => val === value[index]))
-    ) {
-      onChange(optionSelectorValues);
-    }
-    // eslint-disable-next-line react-hooks/exhaustive-deps
-  }, [JSON.stringify(value), JSON.stringify(optionSelector.getValues())]);
-
-  // useComponentDidUpdate to avoid running this for the first render, to avoid
-  // calling onChange when the initial value is not valid for the dataset
-  useComponentDidUpdate(handleOptionsChange);
-
   const onDrop = useCallback(
     (item: DatasourcePanelDndItem) => {
       const column = item.value as ColumnMeta;
@@ -142,8 +115,12 @@ function DndColumnSelect(props: DndColumnSelectProps) {
 
   const valuesRenderer = useCallback(
     () =>
-      optionSelector.values.map((column, idx) =>
-        clickEnabled ? (
+      optionSelector.values.map((column, idx) => {
+        const datasourceWarningMessage =
+          isAdhocColumn(column) && column.datasourceWarning
+            ? t('This column might be incompatible with current dataset')
+            : undefined;
+        return clickEnabled ? (
           <ColumnSelectPopoverTrigger
             key={idx}
             columns={options}
@@ -166,6 +143,7 @@ function DndColumnSelect(props: DndColumnSelectProps) {
               type={`${DndItemType.ColumnOption}_${name}_${label}`}
               canDelete={canDelete}
               column={column}
+              datasourceWarningMessage={datasourceWarningMessage}
               withCaret
             />
           </ColumnSelectPopoverTrigger>
@@ -178,9 +156,10 @@ function DndColumnSelect(props: DndColumnSelectProps) {
             type={`${DndItemType.ColumnOption}_${name}_${label}`}
             canDelete={canDelete}
             column={column}
+            datasourceWarningMessage={datasourceWarningMessage}
           />
-        ),
-      ),
+        );
+      }),
     [
       canDelete,
       clickEnabled,

diff --git a/...-frontend/src/explore/components/controls/DndColumnSelectControl/DndMetricSelect.test.tsx b/...-frontend/src/explore/components/controls/DndColumnSelectControl/DndMetricSelect.test.tsx
@@ -133,6 +133,7 @@ test('remove selected custom metric when metric gets removed from dataset', () =
   );
   expect(screen.getByText('metric_a')).toBeVisible();
   expect(screen.queryByText('Metric B')).not.toBeInTheDocument();
+  expect(screen.queryByText('metric_b')).not.toBeInTheDocument();
   expect(screen.getByText('SUM(column_a)')).toBeVisible();
   expect(screen.getByText('SUM(Column B)')).toBeVisible();
 });
@@ -171,15 +172,6 @@ test('remove selected custom metric when metric gets removed from dataset for si
     ],
   };
 
-  // rerender twice - first to update columns, second to update value
-  rerender(
-    <DndMetricSelect
-      {...newPropsWithRemovedMetric}
-      value={metricValue}
-      onChange={onChange}
-      multi={false}
-    />,
-  );
   rerender(
     <DndMetricSelect
       {...newPropsWithRemovedMetric}
@@ -220,15 +212,6 @@ test('remove selected adhoc metric when column gets removed from dataset', async
     ],
   };
 
-  // rerender twice - first to update columns, second to update value
-  rerender(
-    <DndMetricSelect
-      {...newPropsWithRemovedColumn}
-      value={metricValues}
-      onChange={onChange}
-      multi
-    />,
-  );
   rerender(
     <DndMetricSelect
       {...newPropsWithRemovedColumn}