fix(explore): Metrics disappearing after removing metric from dataset (#17201)

* fix(explore): Metrics disappearing after removing metric from dataset

* fix test

* Apply fix to non-dnd controls

* Make adhoc metrics pick up changes from dataset columns

* Remove console log

* Fix bug in nondnd controls

superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndMetricSelect.test.tsx

@@ -19,14 +19,44 @@
 import React from 'react';
 import { render, screen } from 'spec/helpers/testing-library';
 import { DndMetricSelect } from 'src/explore/components/controls/DndColumnSelectControl/DndMetricSelect';
+import { AGGREGATES } from 'src/explore/constants';
+import { EXPRESSION_TYPES } from '../MetricControl/AdhocMetric';
 
 const defaultProps = {
   savedMetrics: [
     {
-      metric_name: 'Metric A',
+      metric_name: 'metric_a',
-      expression: 'Expression A',
+      expression: 'expression_a',
+    },
+    {
+      metric_name: 'metric_b',
+      expression: 'expression_b',
+      verbose_name: 'Metric B',
     },
   ],
+  columns: [
+    {
+      column_name: 'column_a',
+    },
+    {
+      column_name: 'column_b',
+      verbose_name: 'Column B',
+    },
+  ],
+  onChange: () => {},
+};
+
+const adhocMetricA = {
+  expressionType: EXPRESSION_TYPES.SIMPLE,
+  column: defaultProps.columns[0],
+  aggregate: AGGREGATES.SUM,
+  optionName: 'abc',
+};
+const adhocMetricB = {
+  expressionType: EXPRESSION_TYPES.SIMPLE,
+  column: defaultProps.columns[1],
+  aggregate: AGGREGATES.SUM,
+  optionName: 'def',
 };
 
 test('renders with default props', () => {
@@ -38,3 +68,161 @@ test('renders with default props and multi = true', () => {
   render(<DndMetricSelect {...defaultProps} multi />, { useDnd: true });
   expect(screen.getByText('Drop columns or metrics here')).toBeInTheDocument();
 });
+
+test('render selected metrics correctly', () => {
+  const metricValues = ['metric_a', 'metric_b', adhocMetricB];
+  render(<DndMetricSelect {...defaultProps} value={metricValues} multi />, {
+    useDnd: true,
+  });
+  expect(screen.getByText('metric_a')).toBeVisible();
+  expect(screen.getByText('Metric B')).toBeVisible();
+  expect(screen.getByText('SUM(Column B)')).toBeVisible();
+});
+
+test('remove selected custom metric when metric gets removed from dataset', () => {
+  let metricValues = ['metric_a', 'metric_b', adhocMetricA, adhocMetricB];
+  const onChange = (val: any[]) => {
+    metricValues = val;
+  };
+
+  const { rerender } = render(
+    <DndMetricSelect
+      {...defaultProps}
+      value={metricValues}
+      onChange={onChange}
+      multi
+    />,
+    {
+      useDnd: true,
+    },
+  );
+
+  const newPropsWithRemovedMetric = {
+    ...defaultProps,
+    savedMetrics: [
+      {
+        metric_name: 'metric_a',
+        expression: 'expression_a',
+      },
+    ],
+  };
+  rerender(
+    <DndMetricSelect
+      {...newPropsWithRemovedMetric}
+      value={metricValues}
+      onChange={onChange}
+      multi
+    />,
+  );
+  expect(screen.getByText('metric_a')).toBeVisible();
+  expect(screen.queryByText('Metric B')).not.toBeInTheDocument();
+  expect(screen.getByText('SUM(column_a)')).toBeVisible();
+  expect(screen.getByText('SUM(Column B)')).toBeVisible();
+});
+
+test('remove selected adhoc metric when column gets removed from dataset', async () => {
+  let metricValues = ['metric_a', 'metric_b', adhocMetricA, adhocMetricB];
+  const onChange = (val: any[]) => {
+    metricValues = val;
+  };
+
+  const { rerender } = render(
+    <DndMetricSelect
+      {...defaultProps}
+      value={metricValues}
+      onChange={onChange}
+      multi
+    />,
+    {
+      useDnd: true,
+    },
+  );
+
+  const newPropsWithRemovedColumn = {
+    ...defaultProps,
+    columns: [
+      {
+        column_name: 'column_a',
+      },
+    ],
+  };
+
+  // rerender twice - first to update columns, second to update value
+  rerender(
+    <DndMetricSelect
+      {...newPropsWithRemovedColumn}
+      value={metricValues}
+      onChange={onChange}
+      multi
+    />,
+  );
+  rerender(
+    <DndMetricSelect
+      {...newPropsWithRemovedColumn}
+      value={metricValues}
+      onChange={onChange}
+      multi
+    />,
+  );
+
+  expect(screen.getByText('metric_a')).toBeVisible();
+  expect(screen.getByText('Metric B')).toBeVisible();
+  expect(screen.getByText('SUM(column_a)')).toBeVisible();
+  expect(screen.queryByText('SUM(Column B)')).not.toBeInTheDocument();
+});
+
+test('update adhoc metric name when column label in dataset changes', () => {
+  let metricValues = ['metric_a', 'metric_b', adhocMetricA, adhocMetricB];
+  const onChange = (val: any[]) => {
+    metricValues = val;
+  };
+
+  const { rerender } = render(
+    <DndMetricSelect
+      {...defaultProps}
+      value={metricValues}
+      onChange={onChange}
+      multi
+    />,
+    {
+      useDnd: true,
+    },
+  );
+
+  const newPropsWithUpdatedColNames = {
+    ...defaultProps,
+    columns: [
+      {
+        column_name: 'column_a',
+        verbose_name: 'new col A name',
+      },
+      {
+        column_name: 'column_b',
+        verbose_name: 'new col B name',
+      },
+    ],
+  };
+
+  // rerender twice - first to update columns, second to update value
+  rerender(
+    <DndMetricSelect
+      {...newPropsWithUpdatedColNames}
+      value={metricValues}
+      onChange={onChange}
+      multi
+    />,
+  );
+  rerender(
+    <DndMetricSelect
+      {...newPropsWithUpdatedColNames}
+      value={metricValues}
+      onChange={onChange}
+      multi
+    />,
+  );
+
+  expect(screen.getByText('metric_a')).toBeVisible();
+  expect(screen.getByText('Metric B')).toBeVisible();
+  expect(screen.getByText('SUM(new col A name)')).toBeVisible();
+  expect(screen.getByText('SUM(new col B name)')).toBeVisible();
+});

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,39 +80,48 @@ const getOptionsForSavedMetrics = (
 
 type ValueType = Metric | AdhocMetric | QueryFormMetric;
 
-const columnsContainAllMetrics = (
+// TODO: use typeguards to distinguish saved metrics from adhoc metrics
-  value: ValueType | ValueType[] | null | undefined,
+const getMetricsMatchingCurrentDataset = (
+  values: ValueType[],
   columns: ColumnMeta[],
   savedMetrics: (savedMetricType | Metric)[],
+  prevColumns: ColumnMeta[],
+  prevSavedMetrics: (savedMetricType | Metric)[],
 ) => {
-  const columnNames = new Set(
+  const areSavedMetricsEqual =
-    [...(columns || []), ...(savedMetrics || [])]
+    !prevSavedMetrics || isEqual(prevSavedMetrics, savedMetrics);
-      // eslint-disable-next-line camelcase
+  const areColsEqual = !prevColumns || isEqual(prevColumns, columns);
-      .map(
-        item =>
-          (item as ColumnMeta).column_name ||
-          (item as savedMetricType).metric_name,
-      ),
-  );
 
-  return (
+  if (areColsEqual && areSavedMetricsEqual) {
-    ensureIsArray(value)
+    return values;
-      .filter(metric => metric)
+  }
-      // find column names
+  return values.reduce((acc: ValueType[], metric) => {
-      .map(metric =>
+    if (
-        (metric as AdhocMetric).column
+      (typeof metric === 'string' || (metric as Metric).metric_name) &&
-          ? (metric as AdhocMetric).column.column_name
+      (areSavedMetricsEqual ||
-          : (metric as ColumnMeta).column_name || metric,
+        savedMetrics?.some(
-      )
+          savedMetric =>
-      .filter(name => name && typeof name === 'string')
+            savedMetric.metric_name === metric ||
-      .every(name => columnNames.has(name))
+            savedMetric.metric_name === (metric as Metric).metric_name,
-  );
+        ))
-};
+    ) {
+      acc.push(metric);
+      return acc;
+    }
 
-export type DndMetricSelectProps = DndControlProps<ValueType> & {
+    if (!areColsEqual) {
-  savedMetrics: savedMetricType[];
+      const newCol = columns?.find(
-  columns: ColumnMeta[];
+        column =>
-  datasourceType?: DatasourceType;
+          (metric as AdhocMetric).column?.column_name === column.column_name,
+      );
+      if (newCol) {
+        acc.push({ ...(metric as AdhocMetric), column: newCol });
+      }
+    } else {
+      acc.push(metric);
+    }
+    return acc;
+  }, []);
 };
 
 export const DndMetricSelect = (props: any) => {
@@ -158,25 +165,25 @@ export const DndMetricSelect = (props: any) => {
   }, [JSON.stringify(props.value)]);
 
   useEffect(() => {
-    if (
+    // Remove selected custom metrics that do not exist in the dataset anymore
-      !isEqual(prevColumns, columns) ||
+    // Remove selected adhoc metrics that use columns which do not exist in the dataset anymore
-      !isEqual(prevSavedMetrics, savedMetrics)
+    // Sync adhoc metrics with dataset columns when they are modified by the user
-    ) {
+    if (!props.value) {
-      // Remove all metrics if selected value no longer a valid column
+      return;
-      // in the dataset. Must use `nextProps` here because Redux reducers may
-      // have already updated the value for this control.
-      if (!columnsContainAllMetrics(props.value, columns, savedMetrics)) {
-        onChange([]);
-      }
     }
-  }, [
+    const propsValues = ensureIsArray(props.value);
-    prevColumns,
+    const matchingMetrics = getMetricsMatchingCurrentDataset(
-    columns,
+      propsValues,
-    prevSavedMetrics,
+      columns,
-    savedMetrics,
+      savedMetrics,
-    props.value,
+      prevColumns,
-    onChange,
+      prevSavedMetrics,
-  ]);
+    );
+
+    if (!isEqual(propsValues, matchingMetrics)) {
+      handleChange(matchingMetrics);
+    }
+  }, [columns, savedMetrics, handleChange]);
 
   const canDrop = useCallback(
     (item: DatasourcePanelDndItem) => {
@@ -337,7 +344,7 @@ export const DndMetricSelect = (props: any) => {
     ) {
       const itemValue = droppedItem.value as ColumnMeta;
       const config: Partial<AdhocMetric> = {
-        column: { column_name: itemValue?.column_name },
+        column: itemValue,
       };
       if (isFeatureEnabled(FeatureFlag.UX_BETA)) {
         if (itemValue.type_generic === GenericDataType.NUMERIC) {

superset-frontend/src/explore/components/controls/MetricControl/AdhocMetric.js

@@ -86,17 +86,20 @@ export default class AdhocMetric {
   }
 
   getDefaultLabel() {
-    const label = this.translateToSql();
+    const label = this.translateToSql(true);
     return label.length < 43 ? label : `${label.substring(0, 40)}...`;
   }
 
-  translateToSql() {
+  translateToSql(useVerboseName = false) {
     if (this.expressionType === EXPRESSION_TYPES.SIMPLE) {
       const aggregate = this.aggregate || '';
       // eslint-disable-next-line camelcase
-      const column = this.column?.column_name
+      const column =
-        ? `(${this.column.column_name})`
+        useVerboseName && this.column?.verbose_name
-        : '';
+          ? `(${this.column.verbose_name})`
+          : this.column?.column_name
+          ? `(${this.column.column_name})`
+          : '';
       return aggregate + column;
     }
     if (this.expressionType === EXPRESSION_TYPES.SQL) {

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,22 @@ 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 || metric.column.column_name === column.column_name,
+    );
+  });
+
 const MetricsControl = ({
   onChange,
   multi,
@@ -130,6 +127,8 @@ const MetricsControl = ({
 }) => {
   const [value, setValue] = useState(coerceAdhocMetrics(propsValue));
   const theme = useTheme();
+  const prevColumns = usePrevious(columns);
+  const prevSavedMetrics = usePrevious(savedMetrics);
 
   const handleChange = useCallback(
     opts => {
@@ -253,13 +252,23 @@ const MetricsControl = ({
   );
 
   useEffect(() => {
-    // Remove all metrics if selected value no longer a valid column
+    // Remove selected custom metrics that do not exist in the dataset anymore
-    // in the dataset. Must use `nextProps` here because Redux reducers may
+    // Remove selected adhoc metrics that use columns which do not exist in the dataset anymore
-    // have already updated the value for this control.
+    if (
-    if (!columnsContainAllMetrics(propsValue, columns, savedMetrics)) {
+      propsValue &&
-      handleChange([]);
+      (!isEqual(prevColumns, columns) ||
+        !isEqual(prevSavedMetrics, savedMetrics))
+    ) {
+      const matchingMetrics = getMetricsMatchingCurrentDataset(
+        propsValue,
+        columns,
+        savedMetrics,
+      );
+      if (!isEqual(matchingMetrics, propsValue)) {
+        handleChange(matchingMetrics);
+      }
     }
-  }, [columns, savedMetrics]);
+  }, [columns, handleChange, savedMetrics]);
 
   useEffect(() => {
     setValue(coerceAdhocMetrics(propsValue));