From b2636f01bbdb3a20fff85907571cd1b291bc856b Mon Sep 17 00:00:00 2001
From: Kamil Gabryjelski <kamil.gabryjelski@gmail.com>
Date: Wed, 28 Oct 2020 17:38:52 +0100
Subject: [PATCH] fix: Explore popovers issues (#11428)

* Fix spaces and comas not working in filter popover

* Fix popup not opening automatically

* Add e2e test

* Remove only from test

* Remove redundant test, add checking label content

* Add comments to e2e test

* Fix unit test

* Use destructuring

* Always open popup for functions and saved metrics, too

* Fix popover for adhoc metrics too

* Small refactor to consistency

* Refactor for consistency

* Remove redundant functions

* Test fix

Co-authored-by: Jesse Yang <jesse.yang@airbnb.com>
---
 .../integration/explore/AdhocFilters.test.ts  | 47 ++++++----
 .../integration/explore/AdhocMetrics.test.ts  |  1 -
 .../components/AdhocFilterOption_spec.jsx     |  4 +-
 .../components/AdhocMetricOption_spec.jsx     | 12 +--
 .../src/common/components/Popover.tsx         |  3 +-
 .../explore/components/AdhocFilterOption.jsx  | 85 ++++++++++---------
 .../explore/components/AdhocMetricOption.jsx  | 74 +++++++---------
 .../controls/AdhocFilterControl.jsx           |  6 +-
 8 files changed, 120 insertions(+), 112 deletions(-)

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 6522555aca..9b7c5ae61c 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,26 +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}', { delay: 20 });
-    cy.get('[data-test="adhoc_filters"]').within(() => {
-      cy.contains('name = ').should('be.visible').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 85db968b61..b422673f5f 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 efdd7c14a4..1c47af8c3e 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(<AdhocFilterOption {...props} />).dive();
+  const wrapper = shallow(<AdhocFilterOption {...props} />);
   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 c2e6629a60..9d358a0c66 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(<AdhocMetricOption {...props} />).dive();
+  const wrapper = shallow(<AdhocMetricOption {...props} />);
   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 1f8442ca86..b2682c458d 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 29240e42fb..42ba81dab5 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,
+    };
+  }
+
+  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;
+    });
   }
 
   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 });
+  closePopover() {
+    this.setState({ popoverVisible: false });
   }
 
-  onOverlayExited() {
-    this.setState({ overlayShown: false });
-  }
-
-  closeFilterEditOverlay() {
-    this.setState({ overlayShown: 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 = (
       <AdhocFilterEditPopover
-        onResize={this.onPopoverResize}
         adhocFilter={adhocFilter}
-        onChange={this.props.onFilterEdit}
-        onClose={this.closeFilterEditOverlay}
         options={this.props.options}
         datasource={this.props.datasource}
         partitionColumn={this.props.partitionColumn}
+        onResize={this.onPopoverResize}
+        onClose={this.closePopover}
+        onChange={this.props.onFilterEdit}
       />
     );
+
     return (
-      <div role="button" tabIndex={0} onMouseDown={e => e.stopPropagation()}>
+      <div
+        role="button"
+        tabIndex={0}
+        onMouseDown={e => e.stopPropagation()}
+        onKeyDown={e => e.stopPropagation()}
+      >
         {adhocFilter.isExtra && (
           <InfoTooltipWithTrigger
             icon="exclamation-triangle"
@@ -109,19 +115,14 @@ class AdhocFilterOption extends React.PureComponent {
         <Popover
           placement="right"
           trigger="click"
-          disabled
-          content={content}
+          content={overlayContent}
           defaultVisible={adhocFilter.isNew}
-          visible={this.state.overlayShown}
-          onVisibleChange={this.handleVisibleChange}
+          visible={this.state.popoverVisible}
+          onVisibleChange={this.togglePopover}
         >
           <Label className="option-label adhoc-option adhoc-filter-option">
             {adhocFilter.getDefaultLabel()}
-            <i
-              className={`fa fa-caret-${
-                this.state.overlayShown ? 'left' : 'right'
-              } adhoc-label-arrow`}
-            />
+            <i className="fa fa-caret-right adhoc-label-arrow" />
           </Label>
         </Popover>
       </div>
@@ -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 4c47e22d86..d030e670e2 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,
-      },
+  closePopover() {
+    this.setState({ popoverVisible: false });
+  }
+
+  togglePopover(visible) {
+    this.setState(({ popoverVisible }) => {
+      return {
+        popoverVisible: visible === undefined ? !popoverVisible : visible,
+      };
     });
   }
 
-  closeMetricEditOverlay() {
-    this.setState({ overlayShown: false });
-  }
-
-  handleVisibleChange(visible) {
-    if (visible) {
-      this.onOverlayEntered();
-    } else {
-      this.closeMetricEditOverlay();
-    }
-  }
-
   render() {
     const { adhocMetric } = this.props;
+
     const overlayContent = (
       <AdhocMetricEditPopover
-        onResize={this.onPopoverResize}
         adhocMetric={adhocMetric}
         title={this.state.title}
-        onChange={this.props.onMetricEdit}
-        onClose={this.closeMetricEditOverlay}
         columns={this.props.columns}
         datasourceType={this.props.datasourceType}
+        onResize={this.onPopoverResize}
+        onClose={this.closePopover}
+        onChange={this.props.onMetricEdit}
       />
     );
 
@@ -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}
         >
           <Label className="option-label adhoc-option" data-test="option-label">
             {adhocMetric.label}
-            <i
-              className={`fa fa-caret-${
-                this.state.overlayShown ? 'left' : 'right'
-              } adhoc-label-arrow`}
-            />
+            <i className="fa fa-caret-right adhoc-label-arrow" />
           </Label>
         </Popover>
       </div>
@@ -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 13f8d87a5f..28e6b42275 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 (
-      <div className="metrics-select">
+      <div className="metrics-select" data-test="adhoc-filter-control">
         <ControlHeader {...this.props} />
         <OnPasteSelect
           isMulti