fix(explore): Save button incorrectly disabled when adding new metric…

… with dnd (#23000)
apache · Feb 6, 2023 · 7d5c86b · 7d5c86b
1 parent 2dff000
commit 7d5c86b
Show file tree

Hide file tree

Showing 9 changed files with 47 additions and 26 deletions.
diff --git 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
       >
         <div />
       </AdhocMetricPopoverTrigger>

diff --git 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
@@ -39,15 +39,13 @@ describe('AdhocMetric', () => {
       hasCustomLabel: false,
       optionName: adhocMetric.optionName,
       sqlExpression: null,
-      isNew: false,
     });
   });
 
   it('can create altered duplicates', () => {
     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/.../components/controls/MetricControl/AdhocMetricEditPopover/AdhocMetricEditPopover.test.tsx b/.../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(<AdhocMetricEditPopover {...props} isNewMetric />);
+  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/...t-frontend/src/explore/components/controls/MetricControl/AdhocMetricEditPopover/index.jsx b/...t-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,14 +102,14 @@ 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,
       savedMetric: this.props.savedMetric,
       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/...rset-frontend/src/explore/components/controls/MetricControl/AdhocMetricPopoverTrigger.tsx b/...rset-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}
         />
       </ExplorePopoverContent>
     );

diff --git 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
@@ -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}
         </AdhocMetricPopoverTrigger>

diff --git 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,
         },
       ]);
     });