Apply fix to non-dnd controls

apache · Oct 28, 2021 · bbc806e · bbc806e
1 parent e071046
commit bbc806e
Show file tree

Hide file tree

Showing 2 changed files with 37 additions and 34 deletions.
diff --git a/superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndMetricSelect.tsx b/superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndMetricSelect.tsx
@@ -19,7 +19,6 @@
 
 import React, { useCallback, useEffect, useMemo, useState } from 'react';
 import {
-  DatasourceType,
   ensureIsArray,
   FeatureFlag,
   GenericDataType,
@@ -42,7 +41,6 @@ import { DndItemType } from 'src/explore/components/DndItemType';
 import DndSelectLabel from 'src/explore/components/controls/DndColumnSelectControl/DndSelectLabel';
 import { savedMetricType } from 'src/explore/components/controls/MetricControl/types';
 import { AGGREGATES } from 'src/explore/constants';
-import { DndControlProps } from './types';
 
 const EMPTY_OBJECT = {};
 const DND_ACCEPTED_TYPES = [DndItemType.Column, DndItemType.Metric];
@@ -82,6 +80,7 @@ const getOptionsForSavedMetrics = (
 
 type ValueType = Metric | AdhocMetric | QueryFormMetric;
 
+// TODO: use typeguards to distinguish saved metrics from adhoc metrics
 const getMetricsMatchingCurrentDataset = (
   value: ValueType | ValueType[] | null | undefined,
   columns: ColumnMeta[],
@@ -101,12 +100,6 @@ const getMetricsMatchingCurrentDataset = (
     );
   });
 
-export type DndMetricSelectProps = DndControlProps<ValueType> & {
-  savedMetrics: savedMetricType[];
-  columns: ColumnMeta[];
-  datasourceType?: DatasourceType;
-};
-
 export const DndMetricSelect = (props: any) => {
   const { onChange, multi, columns, savedMetrics } = props;
 
@@ -154,7 +147,7 @@ export const DndMetricSelect = (props: any) => {
     ) {
       // Remove selected custom metrics that do not exist in the dataset anymore
       // Remove selected adhoc metrics that use columns which do not exist in the dataset anymore
-      onChange(
+      handleChange(
         getMetricsMatchingCurrentDataset(props.value, columns, savedMetrics),
       );
     }
@@ -164,7 +157,7 @@ export const DndMetricSelect = (props: any) => {
     prevSavedMetrics,
     savedMetrics,
     props.value,
-    onChange,
+    handleChange,
   ]);
 
   const canDrop = useCallback(

diff --git a/superset-frontend/src/explore/components/controls/MetricControl/MetricsControl.jsx b/superset-frontend/src/explore/components/controls/MetricControl/MetricsControl.jsx
@@ -19,6 +19,7 @@
 import React, { useCallback, useEffect, useMemo, useState } from 'react';
 import PropTypes from 'prop-types';
 import { ensureIsArray, t, useTheme } from '@superset-ui/core';
+import { isEqual } from 'lodash';
 import ControlHeader from 'src/explore/components/ControlHeader';
 import Icons from 'src/components/Icons';
 import {
@@ -27,6 +28,7 @@ import {
   HeaderContainer,
   LabelsContainer,
 } from 'src/explore/components/controls/OptionControls';
+import { usePrevious } from 'src/common/hooks/usePrevious';
 import columnType from './columnType';
 import MetricDefinitionValue from './MetricDefinitionValue';
 import AdhocMetric from './AdhocMetric';
@@ -75,27 +77,6 @@ function isDictionaryForAdhocMetric(value) {
   return value && !(value instanceof AdhocMetric) && value.expressionType;
 }
 
-function columnsContainAllMetrics(value, columns, savedMetrics) {
-  const columnNames = new Set(
-    [...(columns || []), ...(savedMetrics || [])]
-      // eslint-disable-next-line camelcase
-      .map(({ column_name, metric_name }) => column_name || metric_name),
-  );
-
-  return (
-    (Array.isArray(value) ? value : [value])
-      .filter(metric => metric)
-      // find column names
-      .map(metric =>
-        metric.column
-          ? metric.column.column_name
-          : metric.column_name || metric,
-      )
-      .filter(name => name && typeof name === 'string')
-      .every(name => columnNames.has(name))
-  );
-}
-
 // adhoc metrics are stored as dictionaries in URL params. We convert them back into the
 // AdhocMetric class for typechecking, consistency and instance method access.
 function coerceAdhocMetrics(value) {
@@ -118,6 +99,21 @@ function coerceAdhocMetrics(value) {
 
 const emptySavedMetric = { metric_name: '', expression: '' };
 
+// TODO: use typeguards to distinguish saved metrics from adhoc metrics
+const getMetricsMatchingCurrentDataset = (value, columns, savedMetrics) =>
+  ensureIsArray(value).filter(metric => {
+    if (typeof metric === 'string' || metric.metric_name) {
+      return savedMetrics?.some(
+        savedMetric =>
+          savedMetric.metric_name === metric ||
+          savedMetric.metric_name === metric.metric_name,
+      );
+    }
+    return columns?.some(
+      column => metric.column?.column_name === column.column_name,
+    );
+  });
+
 const MetricsControl = ({
   onChange,
   multi,
@@ -130,6 +126,8 @@ const MetricsControl = ({
 }) => {
   const [value, setValue] = useState(coerceAdhocMetrics(propsValue));
   const theme = useTheme();
+  const prevColumns = usePrevious(columns);
+  const prevSavedMetrics = usePrevious(savedMetrics);
 
   const handleChange = useCallback(
     opts => {
@@ -256,10 +254,22 @@ const MetricsControl = ({
     // Remove all metrics if selected value no longer a valid column
     // in the dataset. Must use `nextProps` here because Redux reducers may
     // have already updated the value for this control.
-    if (!columnsContainAllMetrics(propsValue, columns, savedMetrics)) {
-      handleChange([]);
+    if (
+      !isEqual(prevColumns, columns) ||
+      !isEqual(prevSavedMetrics, savedMetrics)
+    ) {
+      handleChange(
+        getMetricsMatchingCurrentDataset(propsValue, columns, savedMetrics),
+      );
     }
-  }, [columns, savedMetrics]);
+  }, [
+    columns,
+    handleChange,
+    prevColumns,
+    prevSavedMetrics,
+    propsValue,
+    savedMetrics,
+  ]);
 
   useEffect(() => {
     setValue(coerceAdhocMetrics(propsValue));