From 68ce4446e9a7000634aa07b01cdda1b3a38b8dae Mon Sep 17 00:00:00 2001 From: Kamil Gabryjelski Date: Thu, 28 Oct 2021 18:07:11 +0200 Subject: [PATCH] Make adhoc metrics pick up changes from dataset columns --- .../DndMetricSelect.test.tsx | 70 +++++++++++++++--- .../DndMetricSelect.tsx | 74 +++++++++++++------ .../controls/MetricControl/AdhocMetric.js | 13 ++-- 3 files changed, 120 insertions(+), 37 deletions(-) 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 bd89cd684bf4f..b859aa09e1e63 100644 --- a/superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndMetricSelect.test.tsx +++ b/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( + , + { + 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 488a863f1f7b8..7a7af372ed66b 100644 --- a/superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndMetricSelect.tsx +++ b/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) => ( { ) { 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) {