diff --git a/superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndMetricSelect.test.tsx b/superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndMetricSelect.test.tsx
index 71cf4bf5480b3..b859aa09e1e63 100644
--- a/superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndMetricSelect.test.tsx
+++ b/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',
- expression: 'Expression A',
+ metric_name: 'metric_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(, { useDnd: true });
expect(screen.getByText('Drop columns or metrics here')).toBeInTheDocument();
});
+
+test('render selected metrics correctly', () => {
+ const metricValues = ['metric_a', 'metric_b', adhocMetricB];
+ render(, {
+ 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(
+ ,
+ {
+ useDnd: true,
+ },
+ );
+
+ const newPropsWithRemovedMetric = {
+ ...defaultProps,
+ savedMetrics: [
+ {
+ metric_name: 'metric_a',
+ expression: 'expression_a',
+ },
+ ],
+ };
+ rerender(
+ ,
+ );
+ 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(
+ ,
+ {
+ useDnd: true,
+ },
+ );
+
+ const newPropsWithRemovedColumn = {
+ ...defaultProps,
+ columns: [
+ {
+ column_name: 'column_a',
+ },
+ ],
+ };
+
+ // rerender twice - first to update columns, second to update value
+ rerender(
+ ,
+ );
+ rerender(
+ ,
+ );
+
+ 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(
+ ,
+ {
+ 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(
+ ,
+ );
+ rerender(
+ ,
+ );
+
+ 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();
+});
diff --git a/superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndMetricSelect.tsx b/superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndMetricSelect.tsx
index a200768d82da7..c733ad69b10c2 100644
--- 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,39 +80,48 @@ const getOptionsForSavedMetrics = (
type ValueType = Metric | AdhocMetric | QueryFormMetric;
-const columnsContainAllMetrics = (
- value: ValueType | ValueType[] | null | undefined,
+// TODO: use typeguards to distinguish saved metrics from adhoc metrics
+const getMetricsMatchingCurrentDataset = (
+ values: ValueType[],
columns: ColumnMeta[],
savedMetrics: (savedMetricType | Metric)[],
+ prevColumns: ColumnMeta[],
+ prevSavedMetrics: (savedMetricType | Metric)[],
) => {
- const columnNames = new Set(
- [...(columns || []), ...(savedMetrics || [])]
- // eslint-disable-next-line camelcase
- .map(
- item =>
- (item as ColumnMeta).column_name ||
- (item as savedMetricType).metric_name,
- ),
- );
+ const areSavedMetricsEqual =
+ !prevSavedMetrics || isEqual(prevSavedMetrics, savedMetrics);
+ const areColsEqual = !prevColumns || isEqual(prevColumns, columns);
- return (
- ensureIsArray(value)
- .filter(metric => metric)
- // find column names
- .map(metric =>
- (metric as AdhocMetric).column
- ? (metric as AdhocMetric).column.column_name
- : (metric as ColumnMeta).column_name || metric,
- )
- .filter(name => name && typeof name === 'string')
- .every(name => columnNames.has(name))
- );
-};
+ if (areColsEqual && areSavedMetricsEqual) {
+ return values;
+ }
+ return values.reduce((acc: ValueType[], metric) => {
+ if (
+ (typeof metric === 'string' || (metric as Metric).metric_name) &&
+ (areSavedMetricsEqual ||
+ savedMetrics?.some(
+ savedMetric =>
+ savedMetric.metric_name === metric ||
+ savedMetric.metric_name === (metric as Metric).metric_name,
+ ))
+ ) {
+ acc.push(metric);
+ return acc;
+ }
-export type DndMetricSelectProps = DndControlProps & {
- savedMetrics: savedMetricType[];
- columns: ColumnMeta[];
- datasourceType?: DatasourceType;
+ 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 acc;
+ }, []);
};
export const DndMetricSelect = (props: any) => {
@@ -158,25 +165,25 @@ export const DndMetricSelect = (props: any) => {
}, [JSON.stringify(props.value)]);
useEffect(() => {
- if (
- !isEqual(prevColumns, columns) ||
- !isEqual(prevSavedMetrics, savedMetrics)
- ) {
- // 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(props.value, columns, savedMetrics)) {
- onChange([]);
- }
+ // 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
+ // Sync adhoc metrics with dataset columns when they are modified by the user
+ if (!props.value) {
+ return;
+ }
+ const propsValues = ensureIsArray(props.value);
+ const matchingMetrics = getMetricsMatchingCurrentDataset(
+ propsValues,
+ columns,
+ savedMetrics,
+ prevColumns,
+ prevSavedMetrics,
+ );
+
+ if (!isEqual(propsValues, matchingMetrics)) {
+ handleChange(matchingMetrics);
}
- }, [
- prevColumns,
- columns,
- prevSavedMetrics,
- savedMetrics,
- props.value,
- onChange,
- ]);
+ }, [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 = {
- column: { column_name: itemValue?.column_name },
+ column: itemValue,
};
if (isFeatureEnabled(FeatureFlag.UX_BETA)) {
if (itemValue.type_generic === GenericDataType.NUMERIC) {
diff --git a/superset-frontend/src/explore/components/controls/MetricControl/AdhocMetric.js b/superset-frontend/src/explore/components/controls/MetricControl/AdhocMetric.js
index f5a488557a386..497de79f03eff 100644
--- a/superset-frontend/src/explore/components/controls/MetricControl/AdhocMetric.js
+++ b/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) {
diff --git a/superset-frontend/src/explore/components/controls/MetricControl/MetricsControl.jsx b/superset-frontend/src/explore/components/controls/MetricControl/MetricsControl.jsx
index 4e4925b9cd202..5df7d755b9e3b 100644
--- 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,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
- // 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([]);
+ // 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
+ if (
+ propsValue &&
+ (!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));