fix: Explore popovers issues (apache#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> (cherry picked from commit b2636f0)
preset-io · Oct 31, 2020 · 45e767a · 45e767a
1 parent 5579772
commit 45e767a
Show file tree

Hide file tree

Showing 8 changed files with 118 additions and 109 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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 = (
       <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,26 +115,21 @@ 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>
     );
   }
 }
 
-export default withTheme(AdhocFilterOption);
+export default AdhocFilterOption;
 
 AdhocFilterOption.propTypes = propTypes;