Skip to content

Commit

Permalink
fix(explore): Save button incorrectly disabled when adding new metric…
Browse files Browse the repository at this point in the history
… with dnd (apache#23000)
  • Loading branch information
kgabryje authored and PawankumarES committed Feb 13, 2023
1 parent 7fd24c8 commit 0ea3889
Show file tree
Hide file tree
Showing 9 changed files with 47 additions and 26 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -370,6 +370,7 @@ const DndMetricSelect = (props: any) => {
visible={newMetricPopoverVisible}
togglePopover={togglePopover}
closePopover={closePopover}
isNew
>
<div />
</AdhocMetricPopoverTrigger>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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,
});
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ const createProps = () => ({
expression: 'sum(num)',
},
],
adhocMetric: new AdhocMetric({ isNew: true }),
adhocMetric: new AdhocMetric({}),
datasource: {
extra: '{}',
type: 'table',
Expand Down Expand Up @@ -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 => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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)`
Expand All @@ -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);
Expand All @@ -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);
}

Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -279,6 +298,7 @@ export default class AdhocMetricEditPopover extends React.PureComponent {
onClose,
onResize,
datasource,
isNewMetric,
...popoverProps
} = this.props;
const { adhocMetric, savedMetric } = this.state;
Expand Down Expand Up @@ -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' &&
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ export type AdhocMetricPopoverTriggerProps = {
visible?: boolean;
togglePopover?: (visible: boolean) => void;
closePopover?: () => void;
isNew?: boolean;
};

export type AdhocMetricPopoverTriggerState = {
Expand Down Expand Up @@ -223,6 +224,7 @@ class AdhocMetricPopoverTrigger extends React.PureComponent<
onChange={this.onChange}
getCurrentTab={this.getCurrentTab}
getCurrentLabel={this.getCurrentLabel}
isNewMetric={this.props.isNew}
/>
</ExplorePopoverContent>
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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()) {
Expand All @@ -234,6 +231,7 @@ const MetricsControl = ({
savedMetricsOptions={savedMetricOptions}
savedMetric={emptySavedMetric}
datasource={datasource}
isNew
>
{trigger}
</AdhocMetricPopoverTrigger>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,6 @@ describe.skip('MetricsControl', () => {
hasCustomLabel: false,
optionName: 'blahblahblah',
sqlExpression: null,
isNew: false,
},
]);
});
Expand Down

0 comments on commit 0ea3889

Please sign in to comment.