Make adhoc metrics pick up changes from dataset columns

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

@@ -48,17 +48,13 @@ const defaultProps = {
 
 const adhocMetricA = {
   expressionType: EXPRESSION_TYPES.SIMPLE,
-  column: {
-    column_name: 'column_a',
-  },
+  column: defaultProps.columns[0],
   aggregate: AGGREGATES.SUM,
   optionName: 'abc',
 };
 const adhocMetricB = {
   expressionType: EXPRESSION_TYPES.SIMPLE,
-  column: {
-    column_name: 'column_b',
-  },
+  column: defaultProps.columns[1],
   aggregate: AGGREGATES.SUM,
   optionName: 'def',
 };
@@ -80,7 +76,7 @@ test('render selected metrics correctly', () => {
   });
   expect(screen.getByText('metric_a')).toBeVisible();
   expect(screen.getByText('Metric B')).toBeVisible();
-  expect(screen.getByText('SUM(column_b)')).toBeVisible();
+  expect(screen.getByText('SUM(Column B)')).toBeVisible();
 });
 
 test('remove selected custom metric when metric gets removed from dataset', () => {
@@ -121,7 +117,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.getByText('SUM(column_a)')).toBeVisible();
-  expect(screen.getByText('SUM(column_b)')).toBeVisible();
+  expect(screen.getByText('SUM(Column B)')).toBeVisible();
 });
 
 test('remove selected adhoc metric when column gets removed from dataset', async () => {
@@ -172,5 +168,61 @@ test('remove selected adhoc metric when column gets removed from dataset', async
   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();
+  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

@@ -82,23 +82,46 @@ type ValueType = Metric | AdhocMetric | QueryFormMetric;
 
 // TODO: use typeguards to distinguish saved metrics from adhoc metrics
 const getMetricsMatchingCurrentDataset = (
-  value: ValueType | ValueType[] | null | undefined,
+  values: ValueType[],
   columns: ColumnMeta[],
   savedMetrics: (savedMetricType | Metric)[],
-) =>
-  ensureIsArray(value).filter(metric => {
-    if (typeof metric === 'string' || (metric as Metric).metric_name) {
-      return savedMetrics?.some(
-        savedMetric =>
-          savedMetric.metric_name === metric ||
-          savedMetric.metric_name === (metric as Metric).metric_name,
+  prevColumns: ColumnMeta[],
+  prevSavedMetrics: (savedMetricType | Metric)[],
+) => {
+  const areMetricsEqual = isEqual(prevSavedMetrics, savedMetrics);
+  const areColsEqual = isEqual(prevColumns, columns);
+
+  if (areColsEqual && areMetricsEqual) {
+    return values;
+  }
+  return values.reduce((acc: ValueType[], metric) => {
+    if (
+      (typeof metric === 'string' || (metric as Metric).metric_name) &&
+      (areMetricsEqual ||
+        savedMetrics?.some(
+          savedMetric =>
+            savedMetric.metric_name === metric ||
+            savedMetric.metric_name === (metric as Metric).metric_name,
+        ))
+    ) {
+      acc.push(metric);
+      return acc;
+    }
+
+    if (!areColsEqual) {
+      const newCol = columns?.find(
+        column =>
+          (metric as AdhocMetric).column?.column_name === column.column_name,
       );
+      if (newCol) {
+        acc.push({ ...(metric as AdhocMetric), column: newCol });
+      }
+    } else {
+      acc.push(metric);
     }
-    return columns?.some(
-      column =>
-        (metric as AdhocMetric).column?.column_name === column.column_name,
-    );
-  });
+    return acc;
+  }, []);
+};
 
 export const DndMetricSelect = (props: any) => {
   const { onChange, multi, columns, savedMetrics } = props;
@@ -141,15 +164,19 @@ export const DndMetricSelect = (props: any) => {
   }, [JSON.stringify(props.value)]);
 
   useEffect(() => {
-    if (
-      !isEqual(prevColumns, columns) ||
-      !isEqual(prevSavedMetrics, savedMetrics)
-    ) {
-      // 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
-      handleChange(
-        getMetricsMatchingCurrentDataset(props.value, columns, savedMetrics),
-      );
+    // 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
+    const propsValues = ensureIsArray(props.value);
+    const matchingMetrics = getMetricsMatchingCurrentDataset(
+      propsValues,
+      columns,
+      savedMetrics,
+      prevColumns,
+      prevSavedMetrics,
+    );
+
+    if (!isEqual(propsValues, matchingMetrics)) {
+      handleChange(matchingMetrics);
     }
   }, [
     prevColumns,
@@ -248,6 +275,7 @@ export const DndMetricSelect = (props: any) => {
     [multi, onChange, value],
   );
 
+  console.log('value', value);
   const valueRenderer = useCallback(
     (option: ValueType, index: number) => (
       <MetricDefinitionValue
@@ -319,7 +347,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
-        ? `(${this.column.column_name})`
-        : '';
+      const column =
+        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) {