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 79b153de2..2f145de2b 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 94d295b5e..889428ab2 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 be02d6b55..360995005 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 bfe8391b2..ae39d183b 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 { + + + + {t('Saved metric')} + + + + @@ -356,27 +394,6 @@ export default class AdhocMetricEditPopover extends React.Component { - - - - {t('Saved metric')} - - - - {title.label || defaultLabel} + ); + } + + return this.state.isEditMode ? ( - {title.hasCustomLabel ? title.label : 'My Metric'} + {title.label || defaultLabel}   + this.setState({ + isTitleEditDisabled: tab === SAVED_TAB_KEY, + }) + } /> ); @@ -113,6 +123,7 @@ class AdhocMetricPopoverTrigger extends React.PureComponent< ); diff --git a/superset-frontend/src/explore/components/controls/MetricControl/MetricDefinitionValue.jsx b/superset-frontend/src/explore/components/controls/MetricControl/MetricDefinitionValue.jsx index 5bae01d5b..30acfdaa5 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 83b5c1661..7b7233fe4 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 (