mirror of
https://github.com/apache/superset.git
synced 2024-09-18 19:49:37 -04:00
fix(explore): Save button incorrectly disabled when adding new metric with dnd (#23000)
This commit is contained in:
parent
2dff0009e9
commit
7d5c86b44c
@ -328,7 +328,7 @@ const DndMetricSelect = (props: any) => {
|
|||||||
}
|
}
|
||||||
return new AdhocMetric(config);
|
return new AdhocMetric(config);
|
||||||
}
|
}
|
||||||
return new AdhocMetric({ isNew: true });
|
return new AdhocMetric({});
|
||||||
}, [droppedItem]);
|
}, [droppedItem]);
|
||||||
|
|
||||||
const ghostButtonText = isFeatureEnabled(FeatureFlag.ENABLE_DND_WITH_CLICK_UX)
|
const ghostButtonText = isFeatureEnabled(FeatureFlag.ENABLE_DND_WITH_CLICK_UX)
|
||||||
@ -370,6 +370,7 @@ const DndMetricSelect = (props: any) => {
|
|||||||
visible={newMetricPopoverVisible}
|
visible={newMetricPopoverVisible}
|
||||||
togglePopover={togglePopover}
|
togglePopover={togglePopover}
|
||||||
closePopover={closePopover}
|
closePopover={closePopover}
|
||||||
|
isNew
|
||||||
>
|
>
|
||||||
<div />
|
<div />
|
||||||
</AdhocMetricPopoverTrigger>
|
</AdhocMetricPopoverTrigger>
|
||||||
|
@ -75,7 +75,6 @@ export default class AdhocMetric {
|
|||||||
this.column = null;
|
this.column = null;
|
||||||
this.aggregate = null;
|
this.aggregate = null;
|
||||||
}
|
}
|
||||||
this.isNew = !!adhocMetric.isNew;
|
|
||||||
this.datasourceWarning = !!adhocMetric.datasourceWarning;
|
this.datasourceWarning = !!adhocMetric.datasourceWarning;
|
||||||
this.hasCustomLabel = !!(adhocMetric.hasCustomLabel && adhocMetric.label);
|
this.hasCustomLabel = !!(adhocMetric.hasCustomLabel && adhocMetric.label);
|
||||||
this.label = this.hasCustomLabel
|
this.label = this.hasCustomLabel
|
||||||
@ -125,9 +124,6 @@ export default class AdhocMetric {
|
|||||||
duplicateWith(nextFields) {
|
duplicateWith(nextFields) {
|
||||||
return new AdhocMetric({
|
return new AdhocMetric({
|
||||||
...this,
|
...this,
|
||||||
// all duplicate metrics are not considered new by default
|
|
||||||
isNew: false,
|
|
||||||
// but still overridable by nextFields
|
|
||||||
...nextFields,
|
...nextFields,
|
||||||
});
|
});
|
||||||
}
|
}
|
||||||
|
@ -39,7 +39,6 @@ describe('AdhocMetric', () => {
|
|||||||
hasCustomLabel: false,
|
hasCustomLabel: false,
|
||||||
optionName: adhocMetric.optionName,
|
optionName: adhocMetric.optionName,
|
||||||
sqlExpression: null,
|
sqlExpression: null,
|
||||||
isNew: false,
|
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
@ -47,7 +46,6 @@ describe('AdhocMetric', () => {
|
|||||||
const adhocMetric1 = new AdhocMetric({
|
const adhocMetric1 = new AdhocMetric({
|
||||||
column: valueColumn,
|
column: valueColumn,
|
||||||
aggregate: AGGREGATES.SUM,
|
aggregate: AGGREGATES.SUM,
|
||||||
isNew: true,
|
|
||||||
});
|
});
|
||||||
const adhocMetric2 = adhocMetric1.duplicateWith({
|
const adhocMetric2 = adhocMetric1.duplicateWith({
|
||||||
aggregate: AGGREGATES.AVG,
|
aggregate: AGGREGATES.AVG,
|
||||||
@ -58,10 +56,6 @@ describe('AdhocMetric', () => {
|
|||||||
|
|
||||||
expect(adhocMetric1.aggregate).toBe(AGGREGATES.SUM);
|
expect(adhocMetric1.aggregate).toBe(AGGREGATES.SUM);
|
||||||
expect(adhocMetric2.aggregate).toBe(AGGREGATES.AVG);
|
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', () => {
|
it('can verify equality', () => {
|
||||||
|
@ -45,7 +45,7 @@ const createProps = () => ({
|
|||||||
expression: 'sum(num)',
|
expression: 'sum(num)',
|
||||||
},
|
},
|
||||||
],
|
],
|
||||||
adhocMetric: new AdhocMetric({ isNew: true }),
|
adhocMetric: new AdhocMetric({}),
|
||||||
datasource: {
|
datasource: {
|
||||||
extra: '{}',
|
extra: '{}',
|
||||||
type: 'table',
|
type: 'table',
|
||||||
@ -152,6 +152,16 @@ test('Clicking on "Save" should not call onChange and onClose', () => {
|
|||||||
expect(props.onClose).toBeCalledTimes(0);
|
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', () => {
|
test('Should switch to tab:Simple', () => {
|
||||||
const props = createProps();
|
const props = createProps();
|
||||||
props.getCurrentTab.mockImplementation(tab => {
|
props.getCurrentTab.mockImplementation(tab => {
|
||||||
|
@ -19,7 +19,13 @@
|
|||||||
/* eslint-disable camelcase */
|
/* eslint-disable camelcase */
|
||||||
import React from 'react';
|
import React from 'react';
|
||||||
import PropTypes from 'prop-types';
|
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 Tabs from 'src/components/Tabs';
|
||||||
import Button from 'src/components/Button';
|
import Button from 'src/components/Button';
|
||||||
import { Select } from 'src/components';
|
import { Select } from 'src/components';
|
||||||
@ -55,11 +61,13 @@ const propTypes = {
|
|||||||
savedMetricsOptions: PropTypes.arrayOf(savedMetricType),
|
savedMetricsOptions: PropTypes.arrayOf(savedMetricType),
|
||||||
savedMetric: savedMetricType,
|
savedMetric: savedMetricType,
|
||||||
datasource: PropTypes.object,
|
datasource: PropTypes.object,
|
||||||
|
isNewMetric: PropTypes.bool,
|
||||||
};
|
};
|
||||||
|
|
||||||
const defaultProps = {
|
const defaultProps = {
|
||||||
columns: [],
|
columns: [],
|
||||||
getCurrentTab: noOp,
|
getCurrentTab: noOp,
|
||||||
|
isNewMetric: false,
|
||||||
};
|
};
|
||||||
|
|
||||||
const StyledSelect = styled(Select)`
|
const StyledSelect = styled(Select)`
|
||||||
@ -78,12 +86,7 @@ export const SAVED_TAB_KEY = 'SAVED';
|
|||||||
|
|
||||||
export default class AdhocMetricEditPopover extends React.PureComponent {
|
export default class AdhocMetricEditPopover extends React.PureComponent {
|
||||||
// "Saved" is a default tab unless there are no saved metrics for dataset
|
// "Saved" is a default tab unless there are no saved metrics for dataset
|
||||||
defaultActiveTabKey =
|
defaultActiveTabKey = this.getDefaultTab();
|
||||||
(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;
|
|
||||||
|
|
||||||
constructor(props) {
|
constructor(props) {
|
||||||
super(props);
|
super(props);
|
||||||
@ -99,6 +102,7 @@ export default class AdhocMetricEditPopover extends React.PureComponent {
|
|||||||
this.onTabChange = this.onTabChange.bind(this);
|
this.onTabChange = this.onTabChange.bind(this);
|
||||||
this.handleAceEditorRef = this.handleAceEditorRef.bind(this);
|
this.handleAceEditorRef = this.handleAceEditorRef.bind(this);
|
||||||
this.refreshAceEditor = this.refreshAceEditor.bind(this);
|
this.refreshAceEditor = this.refreshAceEditor.bind(this);
|
||||||
|
this.getDefaultTab = this.getDefaultTab.bind(this);
|
||||||
|
|
||||||
this.state = {
|
this.state = {
|
||||||
adhocMetric: this.props.adhocMetric,
|
adhocMetric: this.props.adhocMetric,
|
||||||
@ -106,7 +110,6 @@ export default class AdhocMetricEditPopover extends React.PureComponent {
|
|||||||
width: POPOVER_INITIAL_WIDTH,
|
width: POPOVER_INITIAL_WIDTH,
|
||||||
height: POPOVER_INITIAL_HEIGHT,
|
height: POPOVER_INITIAL_HEIGHT,
|
||||||
};
|
};
|
||||||
|
|
||||||
document.addEventListener('mouseup', this.onMouseUp);
|
document.addEventListener('mouseup', this.onMouseUp);
|
||||||
}
|
}
|
||||||
|
|
||||||
@ -137,6 +140,22 @@ export default class AdhocMetricEditPopover extends React.PureComponent {
|
|||||||
document.removeEventListener('mousemove', this.onMouseMove);
|
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() {
|
onSave() {
|
||||||
const { adhocMetric, savedMetric } = this.state;
|
const { adhocMetric, savedMetric } = this.state;
|
||||||
|
|
||||||
@ -279,6 +298,7 @@ export default class AdhocMetricEditPopover extends React.PureComponent {
|
|||||||
onClose,
|
onClose,
|
||||||
onResize,
|
onResize,
|
||||||
datasource,
|
datasource,
|
||||||
|
isNewMetric,
|
||||||
...popoverProps
|
...popoverProps
|
||||||
} = this.props;
|
} = this.props;
|
||||||
const { adhocMetric, savedMetric } = this.state;
|
const { adhocMetric, savedMetric } = this.state;
|
||||||
@ -325,6 +345,7 @@ export default class AdhocMetricEditPopover extends React.PureComponent {
|
|||||||
|
|
||||||
const stateIsValid = adhocMetric.isValid() || savedMetric?.metric_name;
|
const stateIsValid = adhocMetric.isValid() || savedMetric?.metric_name;
|
||||||
const hasUnsavedChanges =
|
const hasUnsavedChanges =
|
||||||
|
isNewMetric ||
|
||||||
!adhocMetric.equals(propsAdhocMetric) ||
|
!adhocMetric.equals(propsAdhocMetric) ||
|
||||||
(!(
|
(!(
|
||||||
typeof savedMetric?.metric_name === 'undefined' &&
|
typeof savedMetric?.metric_name === 'undefined' &&
|
||||||
|
@ -44,6 +44,7 @@ export type AdhocMetricPopoverTriggerProps = {
|
|||||||
visible?: boolean;
|
visible?: boolean;
|
||||||
togglePopover?: (visible: boolean) => void;
|
togglePopover?: (visible: boolean) => void;
|
||||||
closePopover?: () => void;
|
closePopover?: () => void;
|
||||||
|
isNew?: boolean;
|
||||||
};
|
};
|
||||||
|
|
||||||
export type AdhocMetricPopoverTriggerState = {
|
export type AdhocMetricPopoverTriggerState = {
|
||||||
@ -223,6 +224,7 @@ class AdhocMetricPopoverTrigger extends React.PureComponent<
|
|||||||
onChange={this.onChange}
|
onChange={this.onChange}
|
||||||
getCurrentTab={this.getCurrentTab}
|
getCurrentTab={this.getCurrentTab}
|
||||||
getCurrentLabel={this.getCurrentLabel}
|
getCurrentLabel={this.getCurrentLabel}
|
||||||
|
isNewMetric={this.props.isNew}
|
||||||
/>
|
/>
|
||||||
</ExplorePopoverContent>
|
</ExplorePopoverContent>
|
||||||
);
|
);
|
||||||
|
@ -65,7 +65,7 @@ export default function MetricDefinitionValue({
|
|||||||
|
|
||||||
if (option instanceof AdhocMetric || savedMetric) {
|
if (option instanceof AdhocMetric || savedMetric) {
|
||||||
const adhocMetric =
|
const adhocMetric =
|
||||||
option instanceof AdhocMetric ? option : new AdhocMetric({ isNew: true });
|
option instanceof AdhocMetric ? option : new AdhocMetric({});
|
||||||
|
|
||||||
const metricOptionProps = {
|
const metricOptionProps = {
|
||||||
onMetricEdit,
|
onMetricEdit,
|
||||||
|
@ -217,10 +217,7 @@ const MetricsControl = ({
|
|||||||
[propsValue, savedMetrics],
|
[propsValue, savedMetrics],
|
||||||
);
|
);
|
||||||
|
|
||||||
const newAdhocMetric = useMemo(
|
const newAdhocMetric = useMemo(() => new AdhocMetric({}), [value]);
|
||||||
() => new AdhocMetric({ isNew: true }),
|
|
||||||
[value],
|
|
||||||
);
|
|
||||||
const addNewMetricPopoverTrigger = useCallback(
|
const addNewMetricPopoverTrigger = useCallback(
|
||||||
trigger => {
|
trigger => {
|
||||||
if (isAddNewMetricDisabled()) {
|
if (isAddNewMetricDisabled()) {
|
||||||
@ -234,6 +231,7 @@ const MetricsControl = ({
|
|||||||
savedMetricsOptions={savedMetricOptions}
|
savedMetricsOptions={savedMetricOptions}
|
||||||
savedMetric={emptySavedMetric}
|
savedMetric={emptySavedMetric}
|
||||||
datasource={datasource}
|
datasource={datasource}
|
||||||
|
isNew
|
||||||
>
|
>
|
||||||
{trigger}
|
{trigger}
|
||||||
</AdhocMetricPopoverTrigger>
|
</AdhocMetricPopoverTrigger>
|
||||||
|
@ -100,7 +100,6 @@ describe.skip('MetricsControl', () => {
|
|||||||
hasCustomLabel: false,
|
hasCustomLabel: false,
|
||||||
optionName: 'blahblahblah',
|
optionName: 'blahblahblah',
|
||||||
sqlExpression: null,
|
sqlExpression: null,
|
||||||
isNew: false,
|
|
||||||
},
|
},
|
||||||
]);
|
]);
|
||||||
});
|
});
|
||||||
|
Loading…
Reference in New Issue
Block a user