diff --git a/superset-frontend/cypress-base/cypress/integration/explore/AdhocFilters.test.ts b/superset-frontend/cypress-base/cypress/integration/explore/AdhocFilters.test.ts index 1880f7fc08645..9b7c5ae61c473 100644 --- a/superset-frontend/cypress-base/cypress/integration/explore/AdhocFilters.test.ts +++ b/superset-frontend/cypress-base/cypress/integration/explore/AdhocFilters.test.ts @@ -50,13 +50,17 @@ describe('AdhocFilters', () => { }); it('Set simple adhoc filter', () => { - cy.get('#filter-edit-popover').within(() => { - cy.get('[data-test=adhoc-filter-simple-value]').within(() => { - cy.get('.Select__control').click(); - cy.get('input[type=text]').focus().type('Any{enter}'); - }); - cy.get('button').contains('Save').click(); - }); + cy.get('[data-test=adhoc-filter-simple-value] .Select__control').click(); + cy.get('[data-test=adhoc-filter-simple-value] input[type=text]') + .focus() + .type('Jack{enter}', { delay: 20 }); + + cy.get('[data-test="adhoc-filter-edit-popover-save-button"]').click(); + + cy.get( + '[data-test=adhoc_filters] .Select__control span.option-label', + ).contains('name = Jack'); + cy.get('button[data-test="run-query-button"]').click(); cy.verifySliceSuccess({ waitAlias: '@postJson', @@ -65,25 +69,35 @@ describe('AdhocFilters', () => { }); it('Set custom adhoc filter', () => { - cy.visitChartByName('Num Births Trend'); - cy.verifySliceSuccess({ waitAlias: '@postJson' }); + const filterType = 'name'; + const filterContent = "'Amy' OR name = 'Donald'"; cy.get('[data-test=adhoc_filters] .Select__control') .scrollIntoView() .click(); + + // remove previous input cy.get('[data-test=adhoc_filters] input[type=text]') .focus() - .type('name{enter}'); - cy.get('.adhoc-filter-option').click(); + .type('{backspace}'); + + cy.get('[data-test=adhoc_filters] input[type=text]') + .focus() + .type(`${filterType}{enter}`); cy.wait('@filterValues'); + // selecting a new filter should auto-open the popup, + // so the tabshould be visible by now cy.get('#filter-edit-popover #adhoc-filter-edit-tabs-tab-SQL').click(); cy.get('#filter-edit-popover .ace_content').click(); - cy.get('#filter-edit-popover .ace_text-input').type( - "'Amy' OR name = 'Bob'", - ); - cy.get('#filter-edit-popover button').contains('Save').click(); + cy.get('#filter-edit-popover .ace_text-input').type(filterContent); + cy.get('[data-test="adhoc-filter-edit-popover-save-button"]').click(); + + // check if the filter was saved correctly + cy.get( + '[data-test=adhoc_filters] .Select__control span.option-label', + ).contains(`${filterType} = ${filterContent}`); cy.get('button[data-test="run-query-button"]').click(); cy.verifySliceSuccess({ 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 f3a19ec7562cb..d673af02a8e17 100644 --- a/superset-frontend/cypress-base/cypress/integration/explore/AdhocMetrics.test.ts +++ b/superset-frontend/cypress-base/cypress/integration/explore/AdhocMetrics.test.ts @@ -42,7 +42,6 @@ describe('AdhocMetrics', () => { .trigger('mousedown') .click(); - cy.get('[data-test="option-label"]').first().click(); cy.get('[data-test="AdhocMetricEditTitle#trigger"]').click(); cy.get('[data-test="AdhocMetricEditTitle#input"]').type(metricName); cy.get('[data-test="AdhocMetricEdit#save"]').contains('Save').click(); diff --git a/superset-frontend/spec/javascripts/explore/components/AdhocFilterOption_spec.jsx b/superset-frontend/spec/javascripts/explore/components/AdhocFilterOption_spec.jsx index efdd7c14a47d8..1c47af8c3e20a 100644 --- a/superset-frontend/spec/javascripts/explore/components/AdhocFilterOption_spec.jsx +++ b/superset-frontend/spec/javascripts/explore/components/AdhocFilterOption_spec.jsx @@ -19,7 +19,7 @@ /* eslint-disable no-unused-expressions */ import React from 'react'; import sinon from 'sinon'; -import { styledShallow as shallow } from 'spec/helpers/theming'; +import { shallow } from 'enzyme'; import Popover from 'src/common/components/Popover'; import Label from 'src/components/Label'; @@ -46,7 +46,7 @@ function setup(overrides) { datasource: {}, ...overrides, }; - const wrapper = shallow().dive(); + const wrapper = shallow(); return { wrapper }; } diff --git a/superset-frontend/spec/javascripts/explore/components/AdhocMetricOption_spec.jsx b/superset-frontend/spec/javascripts/explore/components/AdhocMetricOption_spec.jsx index c2e6629a60778..9d358a0c66d46 100644 --- a/superset-frontend/spec/javascripts/explore/components/AdhocMetricOption_spec.jsx +++ b/superset-frontend/spec/javascripts/explore/components/AdhocMetricOption_spec.jsx @@ -19,7 +19,7 @@ /* eslint-disable no-unused-expressions */ import React from 'react'; import sinon from 'sinon'; -import { styledShallow as shallow } from 'spec/helpers/theming'; +import { shallow } from 'enzyme'; import Popover from 'src/common/components/Popover'; import Label from 'src/components/Label'; @@ -46,7 +46,7 @@ function setup(overrides) { columns, ...overrides, }; - const wrapper = shallow().dive(); + const wrapper = shallow(); return { wrapper, onMetricEdit }; } @@ -73,11 +73,13 @@ describe('AdhocMetricOption', () => { it('returns to default labels when the custom label is cleared', () => { const { wrapper } = setup(); + expect(wrapper.state('title').label).toBe('SUM(value)'); + wrapper.instance().onLabelChange({ target: { value: 'new label' } }); + expect(wrapper.state('title').label).toBe('new label'); + wrapper.instance().onLabelChange({ target: { value: '' } }); - // close and open the popover - wrapper.instance().closeMetricEditOverlay(); - wrapper.instance().onOverlayEntered(); + expect(wrapper.state('title').label).toBe('SUM(value)'); expect(wrapper.state('title').hasCustomLabel).toBe(false); }); diff --git a/superset-frontend/src/common/components/Popover.tsx b/superset-frontend/src/common/components/Popover.tsx index 1f8442ca86527..b2682c458de9d 100644 --- a/superset-frontend/src/common/components/Popover.tsx +++ b/superset-frontend/src/common/components/Popover.tsx @@ -17,8 +17,7 @@ * under the License. */ import { Popover as AntdPopover } from 'src/common/components'; -import { styled } from '@superset-ui/core'; -const SupersetPopover = styled(AntdPopover)``; +const SupersetPopover = AntdPopover; export default SupersetPopover; diff --git a/superset-frontend/src/explore/components/AdhocFilterOption.jsx b/superset-frontend/src/explore/components/AdhocFilterOption.jsx index 29240e42fb4bc..42ba81dab528d 100644 --- a/superset-frontend/src/explore/components/AdhocFilterOption.jsx +++ b/superset-frontend/src/explore/components/AdhocFilterOption.jsx @@ -18,10 +18,10 @@ */ import React from 'react'; import PropTypes from 'prop-types'; -import Popover from 'src/common/components/Popover'; -import { t, withTheme } from '@superset-ui/core'; +import { t } from '@superset-ui/core'; import { InfoTooltipWithTrigger } from '@superset-ui/chart-controls'; +import Popover from 'src/common/components/Popover'; import Label from 'src/components/Label'; import AdhocFilterEditPopover from './AdhocFilterEditPopover'; import AdhocFilter from '../AdhocFilter'; @@ -44,57 +44,63 @@ const propTypes = { class AdhocFilterOption extends React.PureComponent { constructor(props) { super(props); - this.closeFilterEditOverlay = this.closeFilterEditOverlay.bind(this); this.onPopoverResize = this.onPopoverResize.bind(this); - this.onOverlayEntered = this.onOverlayEntered.bind(this); - this.onOverlayExited = this.onOverlayExited.bind(this); - this.handleVisibleChange = this.handleVisibleChange.bind(this); - this.state = { overlayShown: false }; + this.closePopover = this.closePopover.bind(this); + this.togglePopover = this.togglePopover.bind(this); + this.state = { + // automatically open the popover the the metric is new + popoverVisible: !!props.adhocFilter.isNew, + }; } - onPopoverResize() { - this.forceUpdate(); - } - - onOverlayEntered() { - // isNew is used to indicate whether to automatically open the overlay - // once the overlay has been opened, the metric/filter will never be - // considered new again. - this.props.adhocFilter.isNew = false; - this.setState({ overlayShown: true }); + componentDidMount() { + const { adhocFilter } = this.props; + // isNew is used to auto-open the popup. Once popup is opened, it's not + // considered new anymore. + // put behind setTimeout so in case consequetive re-renderings are triggered + // for some reason, the popup can still show up. + setTimeout(() => { + adhocFilter.isNew = false; + }); } - onOverlayExited() { - this.setState({ overlayShown: false }); + onPopoverResize() { + this.forceUpdate(); } - closeFilterEditOverlay() { - this.setState({ overlayShown: false }); + closePopover() { + this.setState({ popoverVisible: false }); } - handleVisibleChange(visible) { - if (visible) { - this.onOverlayEntered(); - } else { - this.onOverlayExited(); - } + togglePopover(visible) { + this.setState(({ popoverVisible }) => { + return { + popoverVisible: visible === undefined ? !popoverVisible : visible, + }; + }); } render() { const { adhocFilter } = this.props; - const content = ( + const overlayContent = ( ); + return ( -
e.stopPropagation()}> +
e.stopPropagation()} + onKeyDown={e => e.stopPropagation()} + > {adhocFilter.isExtra && (
@@ -129,6 +130,6 @@ class AdhocFilterOption extends React.PureComponent { } } -export default withTheme(AdhocFilterOption); +export default AdhocFilterOption; AdhocFilterOption.propTypes = propTypes; diff --git a/superset-frontend/src/explore/components/AdhocMetricOption.jsx b/superset-frontend/src/explore/components/AdhocMetricOption.jsx index 4c47e22d86b0d..d030e670e2373 100644 --- a/superset-frontend/src/explore/components/AdhocMetricOption.jsx +++ b/superset-frontend/src/explore/components/AdhocMetricOption.jsx @@ -18,7 +18,6 @@ */ import React from 'react'; import PropTypes from 'prop-types'; -import { withTheme } from '@superset-ui/core'; import Popover from 'src/common/components/Popover'; import Label from 'src/components/Label'; @@ -38,13 +37,12 @@ const propTypes = { class AdhocMetricOption extends React.PureComponent { constructor(props) { super(props); - this.closeMetricEditOverlay = this.closeMetricEditOverlay.bind(this); - this.onOverlayEntered = this.onOverlayEntered.bind(this); this.onPopoverResize = this.onPopoverResize.bind(this); - this.handleVisibleChange = this.handleVisibleChange.bind(this); this.onLabelChange = this.onLabelChange.bind(this); + this.closePopover = this.closePopover.bind(this); + this.togglePopover = this.togglePopover.bind(this); this.state = { - overlayShown: false, + popoverVisible: undefined, title: { label: props.adhocMetric.label, hasCustomLabel: props.adhocMetric.hasCustomLabel, @@ -52,12 +50,23 @@ class AdhocMetricOption extends React.PureComponent { }; } + componentDidMount() { + const { adhocMetric } = this.props; + // isNew is used to auto-open the popup. Once popup is opened, it's not + // considered new anymore. + // put behind setTimeout so in case consequetive re-renderings are triggered + // for some reason, the popup can still show up. + setTimeout(() => { + adhocMetric.isNew = false; + }); + } + onLabelChange(e) { const label = e.target.value; this.setState({ title: { - label, - hasCustomLabel: true, + label: label || this.props.adhocMetric.label, + hasCustomLabel: !!label, }, }); } @@ -66,43 +75,30 @@ class AdhocMetricOption extends React.PureComponent { this.forceUpdate(); } - onOverlayEntered() { - // isNew is used to indicate whether to automatically open the overlay - // once the overlay has been opened, the metric/filter will never be - // considered new again. - this.props.adhocMetric.isNew = false; - this.setState({ - overlayShown: true, - title: { - label: this.props.adhocMetric.label, - hasCustomLabel: this.props.adhocMetric.hasCustomLabel, - }, - }); - } - - closeMetricEditOverlay() { - this.setState({ overlayShown: false }); + closePopover() { + this.setState({ popoverVisible: false }); } - handleVisibleChange(visible) { - if (visible) { - this.onOverlayEntered(); - } else { - this.closeMetricEditOverlay(); - } + togglePopover(visible) { + this.setState(({ popoverVisible }) => { + return { + popoverVisible: visible === undefined ? !popoverVisible : visible, + }; + }); } render() { const { adhocMetric } = this.props; + const overlayContent = ( ); @@ -129,17 +125,13 @@ class AdhocMetricOption extends React.PureComponent { disabled content={overlayContent} defaultVisible={adhocMetric.isNew} - onVisibleChange={this.handleVisibleChange} - visible={this.state.overlayShown} + visible={this.state.popoverVisible} + onVisibleChange={this.togglePopover} title={popoverTitle} >
@@ -147,6 +139,6 @@ class AdhocMetricOption extends React.PureComponent { } } -export default withTheme(AdhocMetricOption); +export default AdhocMetricOption; AdhocMetricOption.propTypes = propTypes; diff --git a/superset-frontend/src/explore/components/controls/AdhocFilterControl.jsx b/superset-frontend/src/explore/components/controls/AdhocFilterControl.jsx index 13f8d87a5faec..28e6b42275fb7 100644 --- a/superset-frontend/src/explore/components/controls/AdhocFilterControl.jsx +++ b/superset-frontend/src/explore/components/controls/AdhocFilterControl.jsx @@ -180,9 +180,10 @@ export default class AdhocFilterControl extends React.Component { operator: OPERATORS['>'], comparator: 0, clause: CLAUSES.HAVING, + isNew: true, }); } - // has a custom label + // has a custom label, meaning it's custom column if (option.label) { return new AdhocFilter({ expressionType: @@ -196,6 +197,7 @@ export default class AdhocFilterControl extends React.Component { operator: OPERATORS['>'], comparator: 0, clause: CLAUSES.HAVING, + isNew: true, }); } // add a new filter item @@ -262,7 +264,7 @@ export default class AdhocFilterControl extends React.Component { render() { return ( -
+