From 4d179622fa7f11c7ebf28bb5eb5f88069340498a Mon Sep 17 00:00:00 2001 From: Jesse Yang Date: Fri, 10 Jul 2020 12:46:25 -0700 Subject: [PATCH] fix(explore): edit datasource does not update control states (#10284) --- .../{control.test.js => control.test.ts} | 55 ++++++++++++++++++- .../components/DatasourceControl_spec.jsx | 9 ++- .../javascripts/explore/controlUtils_spec.jsx | 1 - .../src/components/TooltipWrapper.jsx | 6 ++ .../components/ExploreViewContainer.jsx | 1 + .../components/controls/DatasourceControl.jsx | 20 +++++-- superset-frontend/src/explore/controlUtils.js | 3 +- superset-frontend/src/explore/controls.jsx | 6 +- 8 files changed, 86 insertions(+), 15 deletions(-) rename superset-frontend/cypress-base/cypress/integration/explore/{control.test.js => control.test.ts} (57%) diff --git a/superset-frontend/cypress-base/cypress/integration/explore/control.test.js b/superset-frontend/cypress-base/cypress/integration/explore/control.test.ts similarity index 57% rename from superset-frontend/cypress-base/cypress/integration/explore/control.test.js rename to superset-frontend/cypress-base/cypress/integration/explore/control.test.ts index e2da3a6f6f..d7a4509525 100644 --- a/superset-frontend/cypress-base/cypress/integration/explore/control.test.js +++ b/superset-frontend/cypress-base/cypress/integration/explore/control.test.ts @@ -21,11 +21,60 @@ // *********************************************** import { FORM_DATA_DEFAULTS, NUM_METRIC } from './visualizations/shared.helper'; -describe('Groupby', () => { +describe('Datasource control', () => { + const newMetricName = `abc${Date.now()}`; + + before(() => { + cy.server(); + cy.login(); + cy.route('GET', '/superset/explore_json/**').as('getJson'); + cy.route('POST', '/superset/explore_json/**').as('postJson'); + }); + + it('should allow edit datasource', () => { + cy.visitChartByName('Num Births Trend'); + cy.verifySliceSuccess({ waitAlias: '@postJson' }); + cy.get('#datasource_menu').click(); + cy.get('a').contains('Edit Datasource').click(); + // create new metric + cy.get('button').contains('Add Item').click(); + cy.get('input[value=""]').click(); + cy.get('input[value=""]') + .focus() + .clear() + .type(`${newMetricName}{enter}`); + cy.get('.modal-footer button').contains('Save').click(); + cy.get('.modal-footer button').contains('OK').click(); + // select new metric + cy.get('.metrics-select:eq(0)').click(); + cy.get('.metrics-select:eq(0) input[type="text"]') + .focus() + .type(newMetricName); + cy.get('.metrics-select:eq(0) .Select__menu .Select__option') + .contains(newMetricName) + .click(); + cy.get('.metrics-select:eq(0) .Select__multi-value__label') + .contains(newMetricName) + .click(); + // delete metric + cy.get('#datasource_menu').click(); + cy.get('a').contains('Edit Datasource').click(); + cy.get(`input[value="${newMetricName}"]`) + .closest('tr') + .find('.fa-close') + .click(); + cy.get('.modal-footer button').contains('Save').click(); + cy.get('.modal-footer button').contains('OK').click(); + cy.get('.Select__multi-value__label') + .contains(newMetricName) + .should('not.exist'); + }); +}); + +describe('Groupby control', () => { it('Set groupby', () => { cy.server(); cy.login(); - cy.route('GET', '/superset/explore_json/**').as('getJson'); cy.route('POST', '/superset/explore_json/**').as('postJson'); cy.visitChartByName('Num Births Trend'); @@ -71,5 +120,7 @@ describe('Time range filter', () => { }); }); }); + cy.get('#filter-popover button').contains('Ok').click(); + cy.get('#filter-popover').should('not.exist'); }); }); diff --git a/superset-frontend/spec/javascripts/explore/components/DatasourceControl_spec.jsx b/superset-frontend/spec/javascripts/explore/components/DatasourceControl_spec.jsx index 60a2308754..2fc0b014a7 100644 --- a/superset-frontend/spec/javascripts/explore/components/DatasourceControl_spec.jsx +++ b/superset-frontend/spec/javascripts/explore/components/DatasourceControl_spec.jsx @@ -41,6 +41,9 @@ const defaultProps = { name: 'main', }, }, + actions: { + setDatasource: sinon.spy(), + }, onChange: sinon.spy(), }; @@ -71,15 +74,15 @@ describe('DatasourceControl', () => { let wrapper = setup(); expect(wrapper.find('#datasource_menu')).toHaveLength(1); expect(wrapper.find('#datasource_menu').dive().find(MenuItem)).toHaveLength( - 2, + 3, ); wrapper = setup({ - onDatasourceSave: () => {}, + isEditable: false, }); expect(wrapper.find('#datasource_menu')).toHaveLength(1); expect(wrapper.find('#datasource_menu').dive().find(MenuItem)).toHaveLength( - 3, + 2, ); }); }); diff --git a/superset-frontend/spec/javascripts/explore/controlUtils_spec.jsx b/superset-frontend/spec/javascripts/explore/controlUtils_spec.jsx index dc879252cf..e7355cb83c 100644 --- a/superset-frontend/spec/javascripts/explore/controlUtils_spec.jsx +++ b/superset-frontend/spec/javascripts/explore/controlUtils_spec.jsx @@ -27,7 +27,6 @@ import { getFormDataFromControls, applyMapStateToPropsToControl, getAllControlsState, - getControlsState, } from 'src/explore/controlUtils'; describe('controlUtils', () => { diff --git a/superset-frontend/src/components/TooltipWrapper.jsx b/superset-frontend/src/components/TooltipWrapper.jsx index ed3b970975..20f58ef114 100644 --- a/superset-frontend/src/components/TooltipWrapper.jsx +++ b/superset-frontend/src/components/TooltipWrapper.jsx @@ -26,6 +26,10 @@ const propTypes = { tooltip: PropTypes.node.isRequired, children: PropTypes.node.isRequired, placement: PropTypes.string, + trigger: PropTypes.oneOfType([ + PropTypes.string, + PropTypes.arrayOf(PropTypes.string), + ]), }; const defaultProps = { @@ -37,11 +41,13 @@ export default function TooltipWrapper({ tooltip, children, placement, + trigger, }) { return ( {tooltip}} + trigger={trigger} > {children} diff --git a/superset-frontend/src/explore/components/ExploreViewContainer.jsx b/superset-frontend/src/explore/components/ExploreViewContainer.jsx index d5c7e4536d..37e5dc08c3 100644 --- a/superset-frontend/src/explore/components/ExploreViewContainer.jsx +++ b/superset-frontend/src/explore/components/ExploreViewContainer.jsx @@ -389,6 +389,7 @@ function mapStateToProps(state) { const form_data = getFormDataFromControls(explore.controls); const chartKey = Object.keys(charts)[0]; const chart = charts[chartKey]; + return { isDatasourceMetaLoading: explore.isDatasourceMetaLoading, datasource: explore.datasource, diff --git a/superset-frontend/src/explore/components/controls/DatasourceControl.jsx b/superset-frontend/src/explore/components/controls/DatasourceControl.jsx index 34eefd9d92..a2f086ddf5 100644 --- a/superset-frontend/src/explore/components/controls/DatasourceControl.jsx +++ b/superset-frontend/src/explore/components/controls/DatasourceControl.jsx @@ -38,9 +38,11 @@ import TooltipWrapper from '../../../components/TooltipWrapper'; import './DatasourceControl.less'; const propTypes = { + actions: PropTypes.object.isRequired, onChange: PropTypes.func, value: PropTypes.string, datasource: PropTypes.object.isRequired, + isEditable: PropTypes.bool, onDatasourceSave: PropTypes.func, }; @@ -48,6 +50,7 @@ const defaultProps = { onChange: () => {}, onDatasourceSave: null, value: null, + isEditable: true, }; class DatasourceControl extends React.PureComponent { @@ -58,6 +61,7 @@ class DatasourceControl extends React.PureComponent { showChangeDatasourceModal: false, menuExpanded: false, }; + this.onDatasourceSave = this.onDatasourceSave.bind(this); this.toggleChangeDatasourceModal = this.toggleChangeDatasourceModal.bind( this, ); @@ -66,6 +70,13 @@ class DatasourceControl extends React.PureComponent { this.renderDatasource = this.renderDatasource.bind(this); } + onDatasourceSave(datasource) { + this.props.actions.setDatasource(datasource); + if (this.props.onDatasourceSave) { + this.props.onDatasourceSave(datasource); + } + } + toggleShowDatasource() { this.setState(({ showDatasource }) => ({ showDatasource: !showDatasource, @@ -120,7 +131,7 @@ class DatasourceControl extends React.PureComponent { render() { const { showChangeDatasourceModal, showEditDatasourceModal } = this.state; - const { datasource, onChange, onDatasourceSave, value } = this.props; + const { datasource, onChange, value } = this.props; return (
@@ -128,6 +139,7 @@ class DatasourceControl extends React.PureComponent { )} - {!!this.props.onDatasourceSave && ( + {this.props.isEditable && ( {t('Edit Datasource')} @@ -179,11 +191,11 @@ class DatasourceControl extends React.PureComponent { ({ - datasource: state.datasource, - onDatasourceSave: actions ? actions.setDatasource : () => {}, + mapStateToProps: ({ datasource }) => ({ + datasource, + isEditable: !!datasource, }), },