You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by ju...@apache.org on 2021/01/19 20:29:34 UTC
[superset] branch master updated: fix(explore): Disable saved
metric name edit in Metric popover (#12582)
This is an automated email from the ASF dual-hosted git repository.
junlin pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/superset.git
The following commit(s) were added to refs/heads/master by this push:
new 853d34b fix(explore): Disable saved metric name edit in Metric popover (#12582)
853d34b is described below
commit 853d34beba7d9e7927ec75b30666cb95b8889b05
Author: Kamil Gabryjelski <ka...@gmail.com>
AuthorDate: Tue Jan 19 21:28:55 2021 +0100
fix(explore): Disable saved metric name edit in Metric popover (#12582)
---
.../integration/explore/AdhocMetrics.test.ts | 3 +
.../explore/visualizations/line.test.ts | 4 ++
.../AdhocMetricEditPopoverTitle_spec.jsx | 16 ++++-
.../MetricControl/AdhocMetricEditPopover.jsx | 73 +++++++++++++---------
.../MetricControl/AdhocMetricEditPopoverTitle.jsx | 21 +++++--
.../MetricControl/AdhocMetricPopoverTrigger.tsx | 17 ++++-
.../MetricControl/MetricDefinitionValue.jsx | 2 +-
.../controls/MetricControl/MetricsControl.jsx | 6 +-
8 files changed, 99 insertions(+), 43 deletions(-)
diff --git a/superset-frontend/cypress-base/cypress/integration/explore/AdhocMetrics.test.ts b/superset-frontend/cypress-base/cypress/integration/explore/AdhocMetrics.test.ts
index 79b153d..2f145de 100644
--- a/superset-frontend/cypress-base/cypress/integration/explore/AdhocMetrics.test.ts
+++ b/superset-frontend/cypress-base/cypress/integration/explore/AdhocMetrics.test.ts
@@ -37,6 +37,9 @@ describe('AdhocMetrics', () => {
.find('[data-test="add-metric-button"]')
.click();
+ // Title edit for saved metrics is disabled - switch to Simple
+ cy.get('[id="adhoc-metric-edit-tabs-tab-SIMPLE"]').click();
+
cy.get('[data-test="AdhocMetricEditTitle#trigger"]').click();
cy.get('[data-test="AdhocMetricEditTitle#input"]').type(metricName);
diff --git a/superset-frontend/cypress-base/cypress/integration/explore/visualizations/line.test.ts b/superset-frontend/cypress-base/cypress/integration/explore/visualizations/line.test.ts
index 94d295b..889428a 100644
--- a/superset-frontend/cypress-base/cypress/integration/explore/visualizations/line.test.ts
+++ b/superset-frontend/cypress-base/cypress/integration/explore/visualizations/line.test.ts
@@ -50,6 +50,10 @@ describe('Visualization > Line', () => {
cy.get('[data-test=metrics]')
.find('[data-test="add-metric-button"]')
.click();
+
+ // Title edit for saved metrics is disabled - switch to Simple
+ cy.get('[id="adhoc-metric-edit-tabs-tab-SIMPLE"]').click();
+
cy.get('[name="select-column"]').click().type('num{enter}');
cy.get('[name="select-aggregate"]').click().type('sum{enter}');
cy.get('[data-test="AdhocMetricEdit#save"]').contains('Save').click();
diff --git a/superset-frontend/spec/javascripts/explore/components/AdhocMetricEditPopoverTitle_spec.jsx b/superset-frontend/spec/javascripts/explore/components/AdhocMetricEditPopoverTitle_spec.jsx
index be02d6b..3609950 100644
--- a/superset-frontend/spec/javascripts/explore/components/AdhocMetricEditPopoverTitle_spec.jsx
+++ b/superset-frontend/spec/javascripts/explore/components/AdhocMetricEditPopoverTitle_spec.jsx
@@ -46,15 +46,25 @@ describe('AdhocMetricEditPopoverTitle', () => {
expect(wrapper.find(Tooltip)).toExist();
expect(
wrapper.find('[data-test="AdhocMetricEditTitle#trigger"]').text(),
- ).toBe('My Metric\xa0');
+ ).toBe(`${title.label}\xa0`);
});
it('transfers to edit mode when clicked', () => {
const { wrapper } = setup();
- expect(wrapper.state('isEditable')).toBe(false);
+ expect(wrapper.state('isEditMode')).toBe(false);
wrapper
.find('[data-test="AdhocMetricEditTitle#trigger"]')
.simulate('click');
- expect(wrapper.state('isEditable')).toBe(true);
+ expect(wrapper.state('isEditMode')).toBe(true);
+ });
+
+ it('Render non-interactive span with title when edit is disabled', () => {
+ const { wrapper } = setup({ isEditDisabled: true });
+ expect(
+ wrapper.find('[data-test="AdhocMetricTitle"]').exists(),
+ ).toBeTruthy();
+ expect(
+ wrapper.find('[data-test="AdhocMetricEditTitle#trigger"]').exists(),
+ ).toBeFalsy();
});
});
diff --git a/superset-frontend/src/explore/components/controls/MetricControl/AdhocMetricEditPopover.jsx b/superset-frontend/src/explore/components/controls/MetricControl/AdhocMetricEditPopover.jsx
index bfe8391..ae39d18 100644
--- a/superset-frontend/src/explore/components/controls/MetricControl/AdhocMetricEditPopover.jsx
+++ b/superset-frontend/src/explore/components/controls/MetricControl/AdhocMetricEditPopover.jsx
@@ -29,6 +29,7 @@ import { ColumnOption } from '@superset-ui/chart-controls';
import FormLabel from 'src/components/FormLabel';
import { SQLEditor } from 'src/components/AsyncAceEditor';
import sqlKeywords from 'src/SqlLab/utils/sqlKeywords';
+import { noOp } from 'src/utils/common';
import { AGGREGATES_OPTIONS } from 'src/explore/constants';
import columnType from 'src/explore/propTypes/columnType';
@@ -40,6 +41,7 @@ const propTypes = {
onChange: PropTypes.func.isRequired,
onClose: PropTypes.func.isRequired,
onResize: PropTypes.func.isRequired,
+ getCurrentTab: PropTypes.func,
columns: PropTypes.arrayOf(columnType),
savedMetrics: PropTypes.arrayOf(savedMetricType),
savedMetric: savedMetricType,
@@ -52,6 +54,7 @@ const propTypes = {
const defaultProps = {
columns: [],
+ getCurrentTab: noOp,
};
const ResizeIcon = styled.i`
@@ -64,12 +67,20 @@ const ColumnOptionStyle = styled.span`
}
`;
-const SAVED_TAB_KEY = 'SAVED';
+export const SAVED_TAB_KEY = 'SAVED';
const startingWidth = 320;
const startingHeight = 240;
export default class AdhocMetricEditPopover extends React.Component {
+ // "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.savedMetrics) &&
+ this.props.savedMetrics.length > 0
+ ? SAVED_TAB_KEY
+ : this.props.adhocMetric.expressionType;
+
constructor(props) {
super(props);
this.onSave = this.onSave.bind(this);
@@ -81,6 +92,7 @@ export default class AdhocMetricEditPopover extends React.Component {
this.onDragDown = this.onDragDown.bind(this);
this.onMouseMove = this.onMouseMove.bind(this);
this.onMouseUp = this.onMouseUp.bind(this);
+ this.onTabChange = this.onTabChange.bind(this);
this.handleAceEditorRef = this.handleAceEditorRef.bind(this);
this.refreshAceEditor = this.refreshAceEditor.bind(this);
@@ -94,6 +106,10 @@ export default class AdhocMetricEditPopover extends React.Component {
document.addEventListener('mouseup', this.onMouseUp);
}
+ componentDidMount() {
+ this.props.getCurrentTab(this.defaultActiveTabKey);
+ }
+
componentWillUnmount() {
document.removeEventListener('mouseup', this.onMouseUp);
document.removeEventListener('mousemove', this.onMouseMove);
@@ -207,6 +223,11 @@ export default class AdhocMetricEditPopover extends React.Component {
document.removeEventListener('mousemove', this.onMouseMove);
}
+ onTabChange(tab) {
+ this.refreshAceEditor();
+ this.props.getCurrentTab(tab);
+ }
+
handleAceEditorRef(ref) {
if (ref) {
this.aceEditorRef = ref;
@@ -316,16 +337,33 @@ export default class AdhocMetricEditPopover extends React.Component {
<Tabs
id="adhoc-metric-edit-tabs"
data-test="adhoc-metric-edit-tabs"
- defaultActiveKey={
- propsSavedMetric.metric_name
- ? SAVED_TAB_KEY
- : adhocMetric.expressionType
- }
+ defaultActiveKey={this.defaultActiveTabKey}
className="adhoc-metric-edit-tabs"
style={{ height: this.state.height, width: this.state.width }}
- onChange={this.refreshAceEditor}
+ onChange={this.onTabChange}
allowOverflow
>
+ <Tabs.TabPane key={SAVED_TAB_KEY} tab={t('Saved')}>
+ <FormGroup>
+ <FormLabel>
+ <strong>{t('Saved metric')}</strong>
+ </FormLabel>
+ <Select name="select-saved" {...savedSelectProps}>
+ {Array.isArray(savedMetrics) &&
+ savedMetrics.map(savedMetric => (
+ <Select.Option
+ value={savedMetric.id}
+ filterBy={
+ savedMetric.verbose_name || savedMetric.metric_name
+ }
+ key={savedMetric.id}
+ >
+ {this.renderColumnOption(savedMetric)}
+ </Select.Option>
+ ))}
+ </Select>
+ </FormGroup>
+ </Tabs.TabPane>
<Tabs.TabPane key={EXPRESSION_TYPES.SIMPLE} tab={t('Simple')}>
<FormGroup>
<FormLabel>
@@ -356,27 +394,6 @@ export default class AdhocMetricEditPopover extends React.Component {
</Select>
</FormGroup>
</Tabs.TabPane>
- <Tabs.TabPane key={SAVED_TAB_KEY} tab={t('Saved')}>
- <FormGroup>
- <FormLabel>
- <strong>{t('Saved metric')}</strong>
- </FormLabel>
- <Select name="select-saved" {...savedSelectProps}>
- {Array.isArray(savedMetrics) &&
- savedMetrics.map(savedMetric => (
- <Select.Option
- value={savedMetric.id}
- filterBy={
- savedMetric.verbose_name || savedMetric.metric_name
- }
- key={savedMetric.id}
- >
- {this.renderColumnOption(savedMetric)}
- </Select.Option>
- ))}
- </Select>
- </FormGroup>
- </Tabs.TabPane>
<Tabs.TabPane
key={EXPRESSION_TYPES.SQL}
tab={t('Custom SQL')}
diff --git a/superset-frontend/src/explore/components/controls/MetricControl/AdhocMetricEditPopoverTitle.jsx b/superset-frontend/src/explore/components/controls/MetricControl/AdhocMetricEditPopoverTitle.jsx
index 08eb9e8..4c2f3f4 100644
--- a/superset-frontend/src/explore/components/controls/MetricControl/AdhocMetricEditPopoverTitle.jsx
+++ b/superset-frontend/src/explore/components/controls/MetricControl/AdhocMetricEditPopoverTitle.jsx
@@ -17,6 +17,7 @@
* under the License.
*/
import React from 'react';
+import { t } from '@superset-ui/core';
import PropTypes from 'prop-types';
import { FormControl } from 'react-bootstrap';
import { Tooltip } from 'src/common/components/Tooltip';
@@ -27,6 +28,7 @@ const propTypes = {
hasCustomLabel: PropTypes.bool,
}),
onChange: PropTypes.func.isRequired,
+ isEditDisabled: PropTypes.bool,
};
export default class AdhocMetricEditPopoverTitle extends React.Component {
@@ -39,7 +41,7 @@ export default class AdhocMetricEditPopoverTitle extends React.Component {
this.onInputBlur = this.onInputBlur.bind(this);
this.state = {
isHovered: false,
- isEditable: false,
+ isEditMode: false,
};
}
@@ -52,11 +54,11 @@ export default class AdhocMetricEditPopoverTitle extends React.Component {
}
onClick() {
- this.setState({ isEditable: true });
+ this.setState({ isEditMode: true });
}
onBlur() {
- this.setState({ isEditable: false });
+ this.setState({ isEditMode: false });
}
onInputBlur(e) {
@@ -67,9 +69,16 @@ export default class AdhocMetricEditPopoverTitle extends React.Component {
}
render() {
- const { title, onChange } = this.props;
+ const { title, onChange, isEditDisabled } = this.props;
+ const defaultLabel = t('My Metric');
- return this.state.isEditable ? (
+ if (isEditDisabled) {
+ return (
+ <span data-test="AdhocMetricTitle">{title.label || defaultLabel}</span>
+ );
+ }
+
+ return this.state.isEditMode ? (
<FormControl
className="metric-edit-popover-label-input"
type="text"
@@ -92,7 +101,7 @@ export default class AdhocMetricEditPopoverTitle extends React.Component {
role="button"
tabIndex={0}
>
- {title.hasCustomLabel ? title.label : 'My Metric'}
+ {title.label || defaultLabel}
<i
className="fa fa-pencil"
diff --git a/superset-frontend/src/explore/components/controls/MetricControl/AdhocMetricPopoverTrigger.tsx b/superset-frontend/src/explore/components/controls/MetricControl/AdhocMetricPopoverTrigger.tsx
index 649d3fe..c5d0e92 100644
--- a/superset-frontend/src/explore/components/controls/MetricControl/AdhocMetricPopoverTrigger.tsx
+++ b/superset-frontend/src/explore/components/controls/MetricControl/AdhocMetricPopoverTrigger.tsx
@@ -19,7 +19,9 @@
import React, { ReactNode } from 'react';
import Popover from 'src/common/components/Popover';
import AdhocMetricEditPopoverTitle from 'src/explore/components/controls/MetricControl/AdhocMetricEditPopoverTitle';
-import AdhocMetricEditPopover from './AdhocMetricEditPopover';
+import AdhocMetricEditPopover, {
+ SAVED_TAB_KEY,
+} from './AdhocMetricEditPopover';
import AdhocMetric from './AdhocMetric';
import { savedMetricType } from './types';
@@ -38,6 +40,7 @@ export type AdhocMetricPopoverTriggerState = {
popoverVisible: boolean;
title: { label: string; hasCustomLabel: boolean };
labelModified: boolean;
+ isTitleEditDisabled: boolean;
};
class AdhocMetricPopoverTrigger extends React.PureComponent<
@@ -57,6 +60,7 @@ class AdhocMetricPopoverTrigger extends React.PureComponent<
hasCustomLabel: props.adhocMetric.hasCustomLabel,
},
labelModified: false,
+ isTitleEditDisabled: false,
};
}
@@ -89,11 +93,12 @@ class AdhocMetricPopoverTrigger extends React.PureComponent<
}
render() {
- const { adhocMetric } = this.props;
+ const { adhocMetric, savedMetric } = this.props;
+ const { verbose_name, metric_name } = savedMetric;
const { label, hasCustomLabel } = adhocMetric;
const title = this.state.labelModified
? this.state.title
- : { label, hasCustomLabel };
+ : { label: verbose_name || metric_name || label, hasCustomLabel };
const overlayContent = (
<AdhocMetricEditPopover
@@ -106,6 +111,11 @@ class AdhocMetricPopoverTrigger extends React.PureComponent<
onResize={this.onPopoverResize}
onClose={this.closePopover}
onChange={this.props.onMetricEdit}
+ getCurrentTab={(tab: string) =>
+ this.setState({
+ isTitleEditDisabled: tab === SAVED_TAB_KEY,
+ })
+ }
/>
);
@@ -113,6 +123,7 @@ class AdhocMetricPopoverTrigger extends React.PureComponent<
<AdhocMetricEditPopoverTitle
title={title}
onChange={this.onLabelChange}
+ isEditDisabled={this.state.isTitleEditDisabled}
/>
);
diff --git a/superset-frontend/src/explore/components/controls/MetricControl/MetricDefinitionValue.jsx b/superset-frontend/src/explore/components/controls/MetricControl/MetricDefinitionValue.jsx
index 5bae01d..30acfda 100644
--- a/superset-frontend/src/explore/components/controls/MetricControl/MetricDefinitionValue.jsx
+++ b/superset-frontend/src/explore/components/controls/MetricControl/MetricDefinitionValue.jsx
@@ -62,7 +62,7 @@ export default function MetricDefinitionValue({
if (option instanceof AdhocMetric || savedMetric) {
const adhocMetric =
- option instanceof AdhocMetric ? option : new AdhocMetric({});
+ option instanceof AdhocMetric ? option : new AdhocMetric({ isNew: true });
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 83b5c16..7b7233f 100644
--- a/superset-frontend/src/explore/components/controls/MetricControl/MetricsControl.jsx
+++ b/superset-frontend/src/explore/components/controls/MetricControl/MetricsControl.jsx
@@ -191,7 +191,9 @@ class MetricsControl extends React.PureComponent {
// compare saved metrics
value === oldMetric.metric_name ||
// compare adhoc metrics
- value.optionName === oldMetric.optionName
+ typeof value.optionName !== 'undefined'
+ ? value.optionName === oldMetric.optionName
+ : false
) {
return changedMetric;
}
@@ -263,7 +265,7 @@ class MetricsControl extends React.PureComponent {
}
return (
<AdhocMetricPopoverTrigger
- adhocMetric={new AdhocMetric({})}
+ adhocMetric={new AdhocMetric({ isNew: true })}
onMetricEdit={this.onNewMetric}
columns={this.props.columns}
savedMetrics={this.props.savedMetrics}