From 7d5c86b44cb0a80fe81bf4693e1eea13132eb83e Mon Sep 17 00:00:00 2001 From: Kamil Gabryjelski Date: Mon, 6 Feb 2023 22:19:02 +0100 Subject: [PATCH] fix(explore): Save button incorrectly disabled when adding new metric with dnd (#23000) --- .../DndMetricSelect.tsx | 3 +- .../controls/MetricControl/AdhocMetric.js | 4 -- .../MetricControl/AdhocMetric.test.js | 6 --- .../AdhocMetricEditPopover.test.tsx | 12 +++++- .../AdhocMetricEditPopover/index.jsx | 37 +++++++++++++++---- .../AdhocMetricPopoverTrigger.tsx | 2 + .../MetricControl/MetricDefinitionValue.jsx | 2 +- .../controls/MetricControl/MetricsControl.jsx | 6 +-- .../MetricControl/MetricsControl.test.jsx | 1 - 9 files changed, 47 insertions(+), 26 deletions(-) diff --git a/superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndMetricSelect.tsx b/superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndMetricSelect.tsx index d3ab394db92ea..32a9c3756bb18 100644 --- a/superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndMetricSelect.tsx +++ b/superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndMetricSelect.tsx @@ -328,7 +328,7 @@ const DndMetricSelect = (props: any) => { } return new AdhocMetric(config); } - return new AdhocMetric({ isNew: true }); + return new AdhocMetric({}); }, [droppedItem]); const ghostButtonText = isFeatureEnabled(FeatureFlag.ENABLE_DND_WITH_CLICK_UX) @@ -370,6 +370,7 @@ const DndMetricSelect = (props: any) => { visible={newMetricPopoverVisible} togglePopover={togglePopover} closePopover={closePopover} + isNew >
diff --git a/superset-frontend/src/explore/components/controls/MetricControl/AdhocMetric.js b/superset-frontend/src/explore/components/controls/MetricControl/AdhocMetric.js index 8688796593e1e..752fc457b948c 100644 --- a/superset-frontend/src/explore/components/controls/MetricControl/AdhocMetric.js +++ b/superset-frontend/src/explore/components/controls/MetricControl/AdhocMetric.js @@ -75,7 +75,6 @@ export default class AdhocMetric { this.column = null; this.aggregate = null; } - this.isNew = !!adhocMetric.isNew; this.datasourceWarning = !!adhocMetric.datasourceWarning; this.hasCustomLabel = !!(adhocMetric.hasCustomLabel && adhocMetric.label); this.label = this.hasCustomLabel @@ -125,9 +124,6 @@ export default class AdhocMetric { duplicateWith(nextFields) { return new AdhocMetric({ ...this, - // all duplicate metrics are not considered new by default - isNew: false, - // but still overridable by nextFields ...nextFields, }); } diff --git a/superset-frontend/src/explore/components/controls/MetricControl/AdhocMetric.test.js b/superset-frontend/src/explore/components/controls/MetricControl/AdhocMetric.test.js index 5186b1f97dfad..4edc21572f8ee 100644 --- a/superset-frontend/src/explore/components/controls/MetricControl/AdhocMetric.test.js +++ b/superset-frontend/src/explore/components/controls/MetricControl/AdhocMetric.test.js @@ -39,7 +39,6 @@ describe('AdhocMetric', () => { hasCustomLabel: false, optionName: adhocMetric.optionName, sqlExpression: null, - isNew: false, }); }); @@ -47,7 +46,6 @@ describe('AdhocMetric', () => { const adhocMetric1 = new AdhocMetric({ column: valueColumn, aggregate: AGGREGATES.SUM, - isNew: true, }); const adhocMetric2 = adhocMetric1.duplicateWith({ aggregate: AGGREGATES.AVG, @@ -58,10 +56,6 @@ describe('AdhocMetric', () => { expect(adhocMetric1.aggregate).toBe(AGGREGATES.SUM); expect(adhocMetric2.aggregate).toBe(AGGREGATES.AVG); - - // duplicated clone should not be new - expect(adhocMetric1.isNew).toBe(true); - expect(adhocMetric2.isNew).toStrictEqual(false); }); it('can verify equality', () => { diff --git a/superset-frontend/src/explore/components/controls/MetricControl/AdhocMetricEditPopover/AdhocMetricEditPopover.test.tsx b/superset-frontend/src/explore/components/controls/MetricControl/AdhocMetricEditPopover/AdhocMetricEditPopover.test.tsx index 8829d4e283d9f..3614e7222ce2b 100644 --- a/superset-frontend/src/explore/components/controls/MetricControl/AdhocMetricEditPopover/AdhocMetricEditPopover.test.tsx +++ b/superset-frontend/src/explore/components/controls/MetricControl/AdhocMetricEditPopover/AdhocMetricEditPopover.test.tsx @@ -45,7 +45,7 @@ const createProps = () => ({ expression: 'sum(num)', }, ], - adhocMetric: new AdhocMetric({ isNew: true }), + adhocMetric: new AdhocMetric({}), datasource: { extra: '{}', type: 'table', @@ -152,6 +152,16 @@ test('Clicking on "Save" should not call onChange and onClose', () => { expect(props.onClose).toBeCalledTimes(0); }); +test('Clicking on "Save" should call onChange and onClose for new metric', () => { + const props = createProps(); + render(); + expect(props.onChange).toBeCalledTimes(0); + expect(props.onClose).toBeCalledTimes(0); + userEvent.click(screen.getByRole('button', { name: 'Save' })); + expect(props.onChange).toBeCalledTimes(1); + expect(props.onClose).toBeCalledTimes(1); +}); + test('Should switch to tab:Simple', () => { const props = createProps(); props.getCurrentTab.mockImplementation(tab => { diff --git a/superset-frontend/src/explore/components/controls/MetricControl/AdhocMetricEditPopover/index.jsx b/superset-frontend/src/explore/components/controls/MetricControl/AdhocMetricEditPopover/index.jsx index f49eb5b4f750c..c824d1e929790 100644 --- a/superset-frontend/src/explore/components/controls/MetricControl/AdhocMetricEditPopover/index.jsx +++ b/superset-frontend/src/explore/components/controls/MetricControl/AdhocMetricEditPopover/index.jsx @@ -19,7 +19,13 @@ /* eslint-disable camelcase */ import React from 'react'; import PropTypes from 'prop-types'; -import { t, styled, ensureIsArray, DatasourceType } from '@superset-ui/core'; +import { + isDefined, + t, + styled, + ensureIsArray, + DatasourceType, +} from '@superset-ui/core'; import Tabs from 'src/components/Tabs'; import Button from 'src/components/Button'; import { Select } from 'src/components'; @@ -55,11 +61,13 @@ const propTypes = { savedMetricsOptions: PropTypes.arrayOf(savedMetricType), savedMetric: savedMetricType, datasource: PropTypes.object, + isNewMetric: PropTypes.bool, }; const defaultProps = { columns: [], getCurrentTab: noOp, + isNewMetric: false, }; const StyledSelect = styled(Select)` @@ -78,12 +86,7 @@ export const SAVED_TAB_KEY = 'SAVED'; export default class AdhocMetricEditPopover extends React.PureComponent { // "Saved" is a default tab unless there are no saved metrics for dataset - defaultActiveTabKey = - (this.props.savedMetric.metric_name || this.props.adhocMetric.isNew) && - Array.isArray(this.props.savedMetricsOptions) && - this.props.savedMetricsOptions.length > 0 - ? SAVED_TAB_KEY - : this.props.adhocMetric.expressionType; + defaultActiveTabKey = this.getDefaultTab(); constructor(props) { super(props); @@ -99,6 +102,7 @@ export default class AdhocMetricEditPopover extends React.PureComponent { this.onTabChange = this.onTabChange.bind(this); this.handleAceEditorRef = this.handleAceEditorRef.bind(this); this.refreshAceEditor = this.refreshAceEditor.bind(this); + this.getDefaultTab = this.getDefaultTab.bind(this); this.state = { adhocMetric: this.props.adhocMetric, @@ -106,7 +110,6 @@ export default class AdhocMetricEditPopover extends React.PureComponent { width: POPOVER_INITIAL_WIDTH, height: POPOVER_INITIAL_HEIGHT, }; - document.addEventListener('mouseup', this.onMouseUp); } @@ -137,6 +140,22 @@ export default class AdhocMetricEditPopover extends React.PureComponent { document.removeEventListener('mousemove', this.onMouseMove); } + getDefaultTab() { + const { adhocMetric, savedMetric, savedMetricsOptions, isNewMetric } = + this.props; + if (isDefined(adhocMetric.column) || isDefined(adhocMetric.sqlExpression)) { + return adhocMetric.expressionType; + } + if ( + (isNewMetric || savedMetric.metric_name) && + Array.isArray(savedMetricsOptions) && + savedMetricsOptions.length > 0 + ) { + return SAVED_TAB_KEY; + } + return adhocMetric.expressionType; + } + onSave() { const { adhocMetric, savedMetric } = this.state; @@ -279,6 +298,7 @@ export default class AdhocMetricEditPopover extends React.PureComponent { onClose, onResize, datasource, + isNewMetric, ...popoverProps } = this.props; const { adhocMetric, savedMetric } = this.state; @@ -325,6 +345,7 @@ export default class AdhocMetricEditPopover extends React.PureComponent { const stateIsValid = adhocMetric.isValid() || savedMetric?.metric_name; const hasUnsavedChanges = + isNewMetric || !adhocMetric.equals(propsAdhocMetric) || (!( typeof savedMetric?.metric_name === 'undefined' && diff --git a/superset-frontend/src/explore/components/controls/MetricControl/AdhocMetricPopoverTrigger.tsx b/superset-frontend/src/explore/components/controls/MetricControl/AdhocMetricPopoverTrigger.tsx index 0136f0a8aa7d2..c593cc2ee878c 100644 --- a/superset-frontend/src/explore/components/controls/MetricControl/AdhocMetricPopoverTrigger.tsx +++ b/superset-frontend/src/explore/components/controls/MetricControl/AdhocMetricPopoverTrigger.tsx @@ -44,6 +44,7 @@ export type AdhocMetricPopoverTriggerProps = { visible?: boolean; togglePopover?: (visible: boolean) => void; closePopover?: () => void; + isNew?: boolean; }; export type AdhocMetricPopoverTriggerState = { @@ -223,6 +224,7 @@ class AdhocMetricPopoverTrigger extends React.PureComponent< onChange={this.onChange} getCurrentTab={this.getCurrentTab} getCurrentLabel={this.getCurrentLabel} + isNewMetric={this.props.isNew} /> ); diff --git a/superset-frontend/src/explore/components/controls/MetricControl/MetricDefinitionValue.jsx b/superset-frontend/src/explore/components/controls/MetricControl/MetricDefinitionValue.jsx index da5743ec766f6..67d9d6e4d231a 100644 --- a/superset-frontend/src/explore/components/controls/MetricControl/MetricDefinitionValue.jsx +++ b/superset-frontend/src/explore/components/controls/MetricControl/MetricDefinitionValue.jsx @@ -65,7 +65,7 @@ export default function MetricDefinitionValue({ if (option instanceof AdhocMetric || savedMetric) { const adhocMetric = - option instanceof AdhocMetric ? option : new AdhocMetric({ isNew: true }); + option instanceof AdhocMetric ? option : new AdhocMetric({}); const metricOptionProps = { onMetricEdit, diff --git a/superset-frontend/src/explore/components/controls/MetricControl/MetricsControl.jsx b/superset-frontend/src/explore/components/controls/MetricControl/MetricsControl.jsx index 853ca90294b18..617be7112bdf3 100644 --- a/superset-frontend/src/explore/components/controls/MetricControl/MetricsControl.jsx +++ b/superset-frontend/src/explore/components/controls/MetricControl/MetricsControl.jsx @@ -217,10 +217,7 @@ const MetricsControl = ({ [propsValue, savedMetrics], ); - const newAdhocMetric = useMemo( - () => new AdhocMetric({ isNew: true }), - [value], - ); + const newAdhocMetric = useMemo(() => new AdhocMetric({}), [value]); const addNewMetricPopoverTrigger = useCallback( trigger => { if (isAddNewMetricDisabled()) { @@ -234,6 +231,7 @@ const MetricsControl = ({ savedMetricsOptions={savedMetricOptions} savedMetric={emptySavedMetric} datasource={datasource} + isNew > {trigger} diff --git a/superset-frontend/src/explore/components/controls/MetricControl/MetricsControl.test.jsx b/superset-frontend/src/explore/components/controls/MetricControl/MetricsControl.test.jsx index c255e6a62b53a..12a19c5cdf796 100644 --- a/superset-frontend/src/explore/components/controls/MetricControl/MetricsControl.test.jsx +++ b/superset-frontend/src/explore/components/controls/MetricControl/MetricsControl.test.jsx @@ -100,7 +100,6 @@ describe.skip('MetricsControl', () => { hasCustomLabel: false, optionName: 'blahblahblah', sqlExpression: null, - isNew: false, }, ]); });