This is an automated email from the ASF dual-hosted git repository. villebro pushed a commit to branch 1.0 in repository https://gitbox.apache.org/repos/asf/superset.git
commit e4453a77f7da32254746f81ac10a7e713ca4fdf3 Author: Kamil Gabryjelski <[email protected]> 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}
