From fa44325a368dab45f24204c2f8241a1c7afa3d86 Mon Sep 17 00:00:00 2001 From: Kamil Gabryjelski Date: Fri, 29 Oct 2021 16:15:19 +0200 Subject: [PATCH] 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 --- .../DndMetricSelect.test.tsx | 192 +++++++++++++++++- .../DndMetricSelect.tsx | 105 +++++----- .../controls/MetricControl/AdhocMetric.js | 13 +- .../controls/MetricControl/MetricsControl.jsx | 63 +++--- 4 files changed, 290 insertions(+), 83 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 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));