superset-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From christ...@apache.org
Subject [incubator-superset] branch lyftga updated: Fix control validation handling (#7231)
Date Thu, 18 Apr 2019 18:36:49 GMT
This is an automated email from the ASF dual-hosted git repository.

christine pushed a commit to branch lyftga
in repository https://gitbox.apache.org/repos/asf/incubator-superset.git


The following commit(s) were added to refs/heads/lyftga by this push:
     new 97718da  Fix control validation handling (#7231)
97718da is described below

commit 97718daea36765677ee3e79f66704a20e693e86c
Author: Maxime Beauchemin <maximebeauchemin@gmail.com>
AuthorDate: Thu Apr 18 11:36:38 2019 -0700

    Fix control validation handling (#7231)
    
    Fixes a series of unexpected things around control validation.
    
    * when a chart opens in a state where a control is invalid, it still
      runs the query, and sometimes gets stuck in what appears to be a 'running'
      state. After this change, no query is run, and a warning is displayed
      in the chart panel body, just like any other error would
    * validation used to be done in the <Control> component and alter the
      redux store as it went. Clearly this is not the right approach, now
      validation occurs on loading the initial redux state, as well as in
      the reducer when controls are changed
    * currently, when going from a invalid control state to a valid one
      (user addresses what is needed), it auto-triggers a query which can be
      unexpected. After this change, the error message disappears, and the
      "Run Query" overlay gets displayed
    * when changing viz type, it's common to get new validation
      errors, and currently when that occurs it will still go ahead and run
      a query with invalid inputs, which often results in errors
      that are not well handled, since much of the logic
      assumes control-validated input.
    * prettier control validation messages
    
    (cherry picked from commit a3212eba5df95bca834d8d6d98c11d522d9172f3)
---
 .gitignore                                         |  1 +
 CONTRIBUTING.md                                    |  6 ++++
 .../cypress/integration/explore/control.test.js    | 18 +++++------
 .../cypress/integration/explore/link.test.js       | 20 ++++++------
 superset/assets/src/chart/Chart.jsx                |  6 +++-
 superset/assets/src/chart/ChartRenderer.jsx        |  3 +-
 superset/assets/src/explore/components/Control.jsx | 35 +-------------------
 .../explore/components/ExploreViewContainer.jsx    | 10 +++++-
 .../assets/src/explore/reducers/exploreReducer.js  | 37 ++++++++++++----------
 .../assets/src/explore/reducers/getInitialState.js |  4 +--
 superset/assets/src/explore/store.js               | 20 +++++++++++-
 11 files changed, 84 insertions(+), 76 deletions(-)

diff --git a/.gitignore b/.gitignore
index 929abbd..2c553ad 100644
--- a/.gitignore
+++ b/.gitignore
@@ -40,6 +40,7 @@ dump.rdb
 env
 env_py3
 envpy3
+env36
 local_config.py
 superset_config.py
 superset.egg-info/
diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md
index 5db5e98..36ed573 100644
--- a/CONTRIBUTING.md
+++ b/CONTRIBUTING.md
@@ -520,6 +520,12 @@ Run Cypress tests:
 cd /superset/superset/assets
 npm run build
 npm run cypress run
+
+# run tests from a specific file
+npm run cypress run -- --spec cypress/integration/explore/link.test.js
+
+# run specific file with video capture
+npm run cypress run -- --spec cypress/integration/dashboard/index.test.js --config video=true
 ```
 
 ## Translating
diff --git a/superset/assets/cypress/integration/explore/control.test.js b/superset/assets/cypress/integration/explore/control.test.js
index 711ab78..d20cb46 100644
--- a/superset/assets/cypress/integration/explore/control.test.js
+++ b/superset/assets/cypress/integration/explore/control.test.js
@@ -29,7 +29,7 @@ describe('Groupby', () => {
     cy.route('GET', '/superset/explore_json/**').as('getJson');
     cy.route('POST', '/superset/explore_json/**').as('postJson');
     cy.visitChartByName('Num Births Trend');
-    cy.verifySliceSuccess({ waitAlias: '@getJson' });
+    cy.verifySliceSuccess({ waitAlias: '@postJson' });
 
     cy.get('[data-test=groupby]').within(() => {
       cy.get('.Select-control').click();
@@ -53,7 +53,7 @@ describe('AdhocMetrics', () => {
     const metricName = 'Girl Births';
 
     cy.visitChartByName('Num Births Trend');
-    cy.verifySliceSuccess({ waitAlias: '@getJson' });
+    cy.verifySliceSuccess({ waitAlias: '@postJson' });
 
     cy.get('[data-test=metrics]').within(() => {
       cy.get('.select-clear').click();
@@ -86,7 +86,7 @@ describe('AdhocMetrics', () => {
     const metric = 'SUM(num)/COUNT(DISTINCT name)';
 
     cy.visitChartByName('Num Births Trend');
-    cy.verifySliceSuccess({ waitAlias: '@getJson' });
+    cy.verifySliceSuccess({ waitAlias: '@postJson' });
 
     cy.get('[data-test=metrics]').within(() => {
       cy.get('.select-clear').click();
@@ -115,7 +115,7 @@ describe('AdhocMetrics', () => {
 
   it('Switch between simple and custom sql tabs', () => {
     cy.visitChartByName('Num Births Trend');
-    cy.verifySliceSuccess({ waitAlias: '@getJson' });
+    cy.verifySliceSuccess({ waitAlias: '@postJson' });
 
     cy.get('[data-test=metrics]').within(() => {
       cy.get('.select-clear').click();
@@ -155,7 +155,7 @@ describe('AdhocFilters', () => {
 
   it('Set simple adhoc filter', () => {
     cy.visitChartByName('Num Births Trend');
-    cy.verifySliceSuccess({ waitAlias: '@getJson' });
+    cy.verifySliceSuccess({ waitAlias: '@postJson' });
 
     cy.get('[data-test=adhoc_filters]').within(() => {
       cy.get('.Select-control').click({ force: true });
@@ -187,7 +187,7 @@ describe('AdhocFilters', () => {
 
   it('Set custom adhoc filter', () => {
     cy.visitChartByName('Num Births Trend');
-    cy.verifySliceSuccess({ waitAlias: '@getJson' });
+    cy.verifySliceSuccess({ waitAlias: '@postJson' });
 
     cy.get('[data-test=adhoc_filters]').within(() => {
       cy.get('.Select-control').click({ force: true });
@@ -226,7 +226,7 @@ describe('Advanced analytics', () => {
 
   it('Create custom time compare', () => {
     cy.visitChartByName('Num Births Trend');
-    cy.verifySliceSuccess({ waitAlias: '@getJson' });
+    cy.verifySliceSuccess({ waitAlias: '@postJson' });
 
     cy.get('span')
       .contains('Advanced Analytics')
@@ -247,7 +247,7 @@ describe('Advanced analytics', () => {
     cy.wait('@postJson');
     cy.reload();
     cy.verifySliceSuccess({
-      waitAlias: '@getJson',
+      waitAlias: '@postJson',
       chartSelector: 'svg',
     });
 
@@ -267,7 +267,7 @@ describe('Annotations', () => {
 
   it('Create formula annotation y-axis goal line', () => {
     cy.visitChartByName('Num Births Trend');
-    cy.verifySliceSuccess({ waitAlias: '@getJson' });
+    cy.verifySliceSuccess({ waitAlias: '@postJson' });
 
     cy.get('[data-test=annotation_layers]').within(() => {
       cy.get('button').click();
diff --git a/superset/assets/cypress/integration/explore/link.test.js b/superset/assets/cypress/integration/explore/link.test.js
index 36b56ce..9f5f82d 100644
--- a/superset/assets/cypress/integration/explore/link.test.js
+++ b/superset/assets/cypress/integration/explore/link.test.js
@@ -32,7 +32,7 @@ describe('Test explore links', () => {
 
   it('Open and close view query modal', () => {
     cy.visitChartByName('Growth Rate');
-    cy.verifySliceSuccess({ waitAlias: '@getJson' });
+    cy.verifySliceSuccess({ waitAlias: '@postJson' });
 
     cy.get('button#query').click();
     cy.get('span').contains('View query').parent().click();
@@ -48,7 +48,7 @@ describe('Test explore links', () => {
     cy.route('POST', 'r/shortner/').as('getShortUrl');
 
     cy.visitChartByName('Growth Rate');
-    cy.verifySliceSuccess({ waitAlias: '@getJson' });
+    cy.verifySliceSuccess({ waitAlias: '@postJson' });
 
     cy.get('[data-test=short-link-button]').click();
 
@@ -61,12 +61,12 @@ describe('Test explore links', () => {
       .then((text) => {
         cy.visit(text);
       });
-    cy.verifySliceSuccess({ waitAlias: '@getJson' });
+    cy.verifySliceSuccess({ waitAlias: '@postJson' });
   });
 
   it('Test iframe link', () => {
     cy.visitChartByName('Growth Rate');
-    cy.verifySliceSuccess({ waitAlias: '@getJson' });
+    cy.verifySliceSuccess({ waitAlias: '@postJson' });
 
     cy.get('[data-test=embed-code-button]').click();
     cy.get('#embed-code-popover').within(() => {
@@ -94,14 +94,14 @@ describe('Test explore links', () => {
       cy.url().should('eq', url);
 
       cy.visitChartByName(newChartName);
-      cy.verifySliceSuccess({ waitAlias: '@getJson' });
+      cy.verifySliceSuccess({ waitAlias: '@postJson' });
     });
   });
 
   xit('Test chart save', () => {
     const chartName = 'Test chart';
     cy.visitChartByName(chartName);
-    cy.verifySliceSuccess({ waitAlias: '@getJson' });
+    cy.verifySliceSuccess({ waitAlias: '@postJson' });
 
     cy.get('[data-test=groupby]').within(() => {
       cy.get('span.select-clear-zone').click();
@@ -118,7 +118,7 @@ describe('Test explore links', () => {
 
   it('Test chart save as and add to new dashboard', () => {
     cy.visitChartByName('Growth Rate');
-    cy.verifySliceSuccess({ waitAlias: '@getJson' });
+    cy.verifySliceSuccess({ waitAlias: '@postJson' });
 
     const dashboardTitle = 'Test dashboard';
     cy.get('button[data-target="#save_modal"]').click();
@@ -128,7 +128,7 @@ describe('Test explore links', () => {
       cy.get('input[placeholder="[dashboard name]"]').type(dashboardTitle);
       cy.get('button#btn_modal_save').click();
     });
-    cy.verifySliceSuccess({ waitAlias: '@getJson' });
+    cy.verifySliceSuccess({ waitAlias: '@postJson' });
     cy.request(`/dashboard/api/read?_flt_3_dashboard_title=${dashboardTitle}`).then((response)
=> {
       expect(response.body.pks[0]).not.equals(null);
     });
@@ -136,7 +136,7 @@ describe('Test explore links', () => {
 
   it('Test chart save as and add to existing dashboard', () => {
     cy.visitChartByName('Most Populated Countries');
-    cy.verifySliceSuccess({ waitAlias: '@getJson' });
+    cy.verifySliceSuccess({ waitAlias: '@postJson' });
     const chartName = 'New Most Populated Countries';
     const dashboardTitle = 'Test dashboard';
 
@@ -151,7 +151,7 @@ describe('Test explore links', () => {
       });
       cy.get('button#btn_modal_save').click();
     });
-    cy.verifySliceSuccess({ waitAlias: '@getJson' });
+    cy.verifySliceSuccess({ waitAlias: '@postJson' });
     cy.request(`/chart/api/read?_flt_3_slice_name=${chartName}`).then((response) => {
       cy.request('DELETE', `/chart/api/delete/${response.body.pks[0]}`);
     });
diff --git a/superset/assets/src/chart/Chart.jsx b/superset/assets/src/chart/Chart.jsx
index 8cb6153..c0d3916 100644
--- a/superset/assets/src/chart/Chart.jsx
+++ b/superset/assets/src/chart/Chart.jsx
@@ -18,6 +18,8 @@
  */
 import PropTypes from 'prop-types';
 import React from 'react';
+import { Alert } from 'react-bootstrap';
+
 import { Logger, LOG_ACTIONS_RENDER_CHART_CONTAINER } from '../logger/LogUtils';
 import Loading from '../components/Loading';
 import RefreshChartOverlay from '../components/RefreshChartOverlay';
@@ -133,7 +135,9 @@ class Chart extends React.PureComponent {
     if (chartStatus === 'failed') {
       return this.renderStackTraceMessage();
     }
-
+    if (errorMessage) {
+      return <Alert bsStyle="warning">{errorMessage}</Alert>;
+    }
     return (
       <ErrorBoundary onError={this.handleRenderContainerFailure} showMessage={false}>
         <div
diff --git a/superset/assets/src/chart/ChartRenderer.jsx b/superset/assets/src/chart/ChartRenderer.jsx
index 9f366f5..c83d1c8 100644
--- a/superset/assets/src/chart/ChartRenderer.jsx
+++ b/superset/assets/src/chart/ChartRenderer.jsx
@@ -199,9 +199,8 @@ class ChartRenderer extends React.Component {
 
     const isLoading = chartStatus === 'loading';
 
-    const skipChartRendering = isLoading || !!chartAlert;
+    const skipChartRendering = isLoading || !!chartAlert || chartStatus === null;
     this.renderStartTime = Logger.getTimestamp();
-
     return (
       <React.Fragment>
         {this.renderTooltip()}
diff --git a/superset/assets/src/explore/components/Control.jsx b/superset/assets/src/explore/components/Control.jsx
index 31942e9..b8babc7 100644
--- a/superset/assets/src/explore/components/Control.jsx
+++ b/superset/assets/src/explore/components/Control.jsx
@@ -37,7 +37,6 @@ const propTypes = {
   description: PropTypes.string,
   tooltipOnClick: PropTypes.func,
   places: PropTypes.number,
-  validators: PropTypes.array,
   validationErrors: PropTypes.array,
   renderTrigger: PropTypes.bool,
   rightNode: PropTypes.node,
@@ -54,7 +53,6 @@ const propTypes = {
 
 const defaultProps = {
   renderTrigger: false,
-  validators: [],
   hidden: false,
   validationErrors: [],
 };
@@ -63,45 +61,14 @@ export default class Control extends React.PureComponent {
   constructor(props) {
     super(props);
     this.state = { hovered: false };
-    this.validate = this.validate.bind(this);
     this.onChange = this.onChange.bind(this);
   }
-  componentDidMount() {
-    this.validateAndSetValue(this.props.value, []);
-  }
   onChange(value, errors) {
-    this.validateAndSetValue(value, errors);
+    this.props.actions.setControlValue(this.props.name, value, errors);
   }
   setHover(hovered) {
     this.setState({ hovered });
   }
-  validateAndSetValue(value, errors) {
-    let validationErrors = this.props.validationErrors;
-    let currentErrors = this.validate(value);
-    if (errors && errors.length > 0) {
-      currentErrors = validationErrors.concat(errors);
-    }
-    if (validationErrors.length + currentErrors.length > 0) {
-      validationErrors = currentErrors;
-    }
-
-    if (value !== this.props.value || validationErrors !== this.props.validationErrors) {
-      this.props.actions.setControlValue(this.props.name, value, validationErrors);
-    }
-  }
-  validate(value) {
-    const validators = this.props.validators;
-    const validationErrors = [];
-    if (validators && validators.length > 0) {
-      validators.forEach((f) => {
-        const v = f(value);
-        if (v) {
-          validationErrors.push(v);
-        }
-      });
-    }
-    return validationErrors;
-  }
   render() {
     const ControlType = controlMap[this.props.type];
     const divStyle = this.props.hidden ? { display: 'none' } : null;
diff --git a/superset/assets/src/explore/components/ExploreViewContainer.jsx b/superset/assets/src/explore/components/ExploreViewContainer.jsx
index 91777e4..68d8fe1 100644
--- a/superset/assets/src/explore/components/ExploreViewContainer.jsx
+++ b/superset/assets/src/explore/components/ExploreViewContainer.jsx
@@ -21,6 +21,7 @@ import React from 'react';
 import PropTypes from 'prop-types';
 import { bindActionCreators } from 'redux';
 import { connect } from 'react-redux';
+import { t } from '@superset-ui/translation';
 
 import ExploreChartPanel from './ExploreChartPanel';
 import ControlPanelsContainer from './ControlPanelsContainer';
@@ -100,6 +101,12 @@ class ExploreViewContainer extends React.Component {
     document.addEventListener('keydown', this.handleKeydown);
     this.addHistory({ isReplace: true });
     this.props.actions.logEvent(LOG_ACTIONS_MOUNT_EXPLORER);
+
+    // Trigger the chart if there are no errors
+    const { chart } = this.props;
+    if (!this.hasErrors()) {
+      this.props.actions.triggerQuery(true, this.props.chart.id);
+    }
   }
 
   componentWillReceiveProps(nextProps) {
@@ -272,12 +279,13 @@ class ExploreViewContainer extends React.Component {
   renderErrorMessage() {
     // Returns an error message as a node if any errors are in the store
     const errors = [];
+    const ctrls = this.props.controls;
     for (const controlName in this.props.controls) {
       const control = this.props.controls[controlName];
       if (control.validationErrors && control.validationErrors.length > 0) {
         errors.push(
           <div key={controlName}>
-            <strong>{`[ ${control.label} ] `}</strong>
+            {t('Control labeled ')}<strong>{` "${control.label}" `}</strong>
             {control.validationErrors.join('. ')}
           </div>,
         );
diff --git a/superset/assets/src/explore/reducers/exploreReducer.js b/superset/assets/src/explore/reducers/exploreReducer.js
index 3823888..6f6a1a9 100644
--- a/superset/assets/src/explore/reducers/exploreReducer.js
+++ b/superset/assets/src/explore/reducers/exploreReducer.js
@@ -17,7 +17,8 @@
  * under the License.
  */
 /* eslint camelcase: 0 */
-import { getControlsState, getFormDataFromControls } from '../store';
+import { validateControl, getControlsState, getFormDataFromControls } from '../store';
+import controls from '../controls';
 import * as actions from '../actions/exploreActions';
 
 export default function exploreReducer(state = {}, action) {
@@ -75,24 +76,28 @@ export default function exploreReducer(state = {}, action) {
       };
     },
     [actions.SET_FIELD_VALUE]() {
-      const controls = Object.assign({}, state.controls);
-      const control = Object.assign({}, controls[action.controlName]);
-      control.value = action.value;
-      control.validationErrors = action.validationErrors;
-      controls[action.controlName] = control;
-      const changes = {
-        controls,
+      // These errors are reported from the Control components
+      let errors = action.validationErrors || [];
+      let control = {
+        ...controls[action.controlName],
+        value: action.value,
       };
-      if (control.renderTrigger) {
-        changes.triggerRender = true;
-      } else {
-        changes.triggerRender = false;
-      }
-      const newState = {
+      control = validateControl(control);
+
+      // These errors are based on control config `validators`
+      errors = errors.concat(control.validationErrors || []);
+      const hasErrors = errors && errors.length > 0;
+      return {
         ...state,
-        ...changes,
+        triggerRender: control.renderTrigger && !hasErrors,
+        controls: {
+          ...state.controls,
+          [action.controlName]: {
+            ...control,
+            validationErrors: errors,
+          },
+        },
       };
-      return newState;
     },
     [actions.SET_EXPLORE_CONTROLS]() {
       return {
diff --git a/superset/assets/src/explore/reducers/getInitialState.js b/superset/assets/src/explore/reducers/getInitialState.js
index 48c85c7..98b9799 100644
--- a/superset/assets/src/explore/reducers/getInitialState.js
+++ b/superset/assets/src/explore/reducers/getInitialState.js
@@ -52,14 +52,14 @@ export default function getInitialState(bootstrapData) {
       [chartKey]: {
         id: chartKey,
         chartAlert: null,
-        chartStatus: 'loading',
+        chartStatus: null,
         chartUpdateEndTime: null,
         chartUpdateStartTime: 0,
         latestQueryFormData: getFormDataFromControls(controls),
         sliceFormData,
         queryController: null,
         queryResponse: null,
-        triggerQuery: true,
+        triggerQuery: false,
         lastRendered: 0,
       },
     },
diff --git a/superset/assets/src/explore/store.js b/superset/assets/src/explore/store.js
index be2be7b..df456c2 100644
--- a/superset/assets/src/explore/store.js
+++ b/superset/assets/src/explore/store.js
@@ -29,6 +29,24 @@ export function getFormDataFromControls(controlsState) {
   return formData;
 }
 
+export function validateControl(control) {
+  const validators = control.validators;
+  const validationErrors = [];
+  if (validators && validators.length > 0) {
+    validators.forEach((f) => {
+      const v = f(control.value);
+      if (v) {
+        validationErrors.push(v);
+      }
+    });
+  }
+  if (validationErrors.length > 0) {
+    return { ...control, validationErrors };
+  }
+  return control;
+}
+
+
 export function getControlNames(vizType, datasourceType) {
   const controlNames = [];
   sectionsToRender(vizType, datasourceType).forEach(
@@ -109,7 +127,7 @@ export function getControlsState(state, form_data) {
     ) {
       control.value = formData[k];
     }
-    controlsState[k] = control;
+    controlsState[k] = validateControl(control);
   });
   if (viz.onInit) {
     return viz.onInit(controlsState);


Mime
View raw message