feat(explore): Remove default for time range filter and Metrics (#14661)

* feat(explore): Remove default for time range filter and Metrics * Merge errors with same messages * Fix e2e test * Rename a variable * Bump packages * Fix unit tests
2021-05-25 15:55:41 +02:00 · 2021-05-25 15:55:41 +02:00 · 63dc035d6a
parent add35f9fe4
commit 63dc035d6a
9 changed files with 398 additions and 381 deletions
--- a/superset-frontend/cypress-base/cypress/integration/explore/visualizations/line.test.ts
+++ b/superset-frontend/cypress-base/cypress/integration/explore/visualizations/line.test.ts
@ -29,7 +29,7 @@ describe('Visualization > Line', () => {
  it('should show validator error when no metric', () => {
    const formData = { ...LINE_CHART_DEFAULTS, metrics: [] };
    cy.visitChartByParams(JSON.stringify(formData));
-    cy.get('.ant-alert-warning').contains(`"Metrics" cannot be empty`);
+    cy.get('.ant-alert-warning').contains(`Metrics: cannot be empty`);
  });

  it('should preload mathjs', () => {
@ -43,7 +43,7 @@ describe('Visualization > Line', () => {
  it('should not show validator error when metric added', () => {
    const formData = { ...LINE_CHART_DEFAULTS, metrics: [] };
    cy.visitChartByParams(JSON.stringify(formData));
-    cy.get('.ant-alert-warning').contains(`"Metrics" cannot be empty`);
+    cy.get('.ant-alert-warning').contains(`Metrics: cannot be empty`);
    cy.get('.text-danger').contains('Metrics');

    cy.get('[data-test=metrics]')
@ -62,6 +62,8 @@ describe('Visualization > Line', () => {
  });

  it('should allow negative values in Y bounds', () => {
+    const formData = { ...LINE_CHART_DEFAULTS, metrics: [NUM_METRIC] };
+    cy.visitChartByParams(JSON.stringify(formData));
    cy.get('#controlSections-tab-display').click();
    cy.get('span').contains('Y Axis Bounds').scrollIntoView();
    cy.get('input[placeholder="Min"]').type('-0.1', { delay: 100 });
--- a/superset-frontend/package-lock.json
+++ b/superset-frontend/package-lock.json
--- a/superset-frontend/package.json
+++ b/superset-frontend/package.json
@ -67,35 +67,35 @@
    "@emotion/babel-preset-css-prop": "^11.2.0",
    "@emotion/cache": "^11.1.3",
    "@emotion/react": "^11.1.5",
-    "@superset-ui/chart-controls": "^0.17.50",
-    "@superset-ui/core": "^0.17.50",
-    "@superset-ui/legacy-plugin-chart-calendar": "^0.17.50",
-    "@superset-ui/legacy-plugin-chart-chord": "^0.17.50",
-    "@superset-ui/legacy-plugin-chart-country-map": "^0.17.50",
-    "@superset-ui/legacy-plugin-chart-event-flow": "^0.17.50",
-    "@superset-ui/legacy-plugin-chart-force-directed": "^0.17.50",
-    "@superset-ui/legacy-plugin-chart-heatmap": "^0.17.50",
-    "@superset-ui/legacy-plugin-chart-histogram": "^0.17.50",
-    "@superset-ui/legacy-plugin-chart-horizon": "^0.17.50",
-    "@superset-ui/legacy-plugin-chart-map-box": "^0.17.50",
-    "@superset-ui/legacy-plugin-chart-paired-t-test": "^0.17.50",
-    "@superset-ui/legacy-plugin-chart-parallel-coordinates": "^0.17.50",
-    "@superset-ui/legacy-plugin-chart-partition": "^0.17.50",
-    "@superset-ui/legacy-plugin-chart-pivot-table": "^0.17.50",
-    "@superset-ui/legacy-plugin-chart-rose": "^0.17.50",
-    "@superset-ui/legacy-plugin-chart-sankey": "^0.17.50",
-    "@superset-ui/legacy-plugin-chart-sankey-loop": "^0.17.50",
-    "@superset-ui/legacy-plugin-chart-sunburst": "^0.17.50",
-    "@superset-ui/legacy-plugin-chart-treemap": "^0.17.50",
-    "@superset-ui/legacy-plugin-chart-world-map": "^0.17.50",
-    "@superset-ui/legacy-preset-chart-big-number": "^0.17.50",
+    "@superset-ui/chart-controls": "^0.17.52",
+    "@superset-ui/core": "^0.17.52",
+    "@superset-ui/legacy-plugin-chart-calendar": "^0.17.52",
+    "@superset-ui/legacy-plugin-chart-chord": "^0.17.52",
+    "@superset-ui/legacy-plugin-chart-country-map": "^0.17.52",
+    "@superset-ui/legacy-plugin-chart-event-flow": "^0.17.52",
+    "@superset-ui/legacy-plugin-chart-force-directed": "^0.17.52",
+    "@superset-ui/legacy-plugin-chart-heatmap": "^0.17.52",
+    "@superset-ui/legacy-plugin-chart-histogram": "^0.17.52",
+    "@superset-ui/legacy-plugin-chart-horizon": "^0.17.52",
+    "@superset-ui/legacy-plugin-chart-map-box": "^0.17.52",
+    "@superset-ui/legacy-plugin-chart-paired-t-test": "^0.17.52",
+    "@superset-ui/legacy-plugin-chart-parallel-coordinates": "^0.17.52",
+    "@superset-ui/legacy-plugin-chart-partition": "^0.17.52",
+    "@superset-ui/legacy-plugin-chart-pivot-table": "^0.17.52",
+    "@superset-ui/legacy-plugin-chart-rose": "^0.17.52",
+    "@superset-ui/legacy-plugin-chart-sankey": "^0.17.52",
+    "@superset-ui/legacy-plugin-chart-sankey-loop": "^0.17.52",
+    "@superset-ui/legacy-plugin-chart-sunburst": "^0.17.52",
+    "@superset-ui/legacy-plugin-chart-treemap": "^0.17.52",
+    "@superset-ui/legacy-plugin-chart-world-map": "^0.17.52",
+    "@superset-ui/legacy-preset-chart-big-number": "^0.17.52",
    "@superset-ui/legacy-preset-chart-deckgl": "^0.4.6",
-    "@superset-ui/legacy-preset-chart-nvd3": "^0.17.50",
-    "@superset-ui/plugin-chart-echarts": "^0.17.50",
-    "@superset-ui/plugin-chart-pivot-table": "^0.17.50",
-    "@superset-ui/plugin-chart-table": "^0.17.50",
-    "@superset-ui/plugin-chart-word-cloud": "^0.17.50",
-    "@superset-ui/preset-chart-xy": "^0.17.50",
+    "@superset-ui/legacy-preset-chart-nvd3": "^0.17.52",
+    "@superset-ui/plugin-chart-echarts": "^0.17.52",
+    "@superset-ui/plugin-chart-pivot-table": "^0.17.52",
+    "@superset-ui/plugin-chart-table": "^0.17.52",
+    "@superset-ui/plugin-chart-word-cloud": "^0.17.52",
+    "@superset-ui/preset-chart-xy": "^0.17.52",
    "@vx/responsive": "^0.0.195",
    "abortcontroller-polyfill": "^1.1.9",
    "antd": "^4.9.4",
--- a/superset-frontend/spec/javascripts/explore/controlUtils_spec.tsx
+++ b/superset-frontend/spec/javascripts/explore/controlUtils_spec.tsx
@ -149,30 +149,14 @@ describe('controlUtils', () => {
      expect(control).toBeNull();
    });

-    it('applies the default function for metrics', () => {
+    it('metrics control should be empty by default', () => {
      const control = getControlState('metrics', 'table', state);
-      expect(control?.default).toEqual(['first']);
+      expect(control?.default).toBeUndefined();
    });

-    it('applies the default function for metric', () => {
+    it('metric control should be empty by default', () => {
      const control = getControlState('metric', 'table', state);
-      expect(control?.default).toEqual('first');
-    });
-
-    it('applies the default function, prefers count if it exists', () => {
-      const stateWithCount = {
-        ...state,
-        datasource: {
-          ...(state.datasource as DatasourceMeta),
-          metrics: [
-            { metric_name: 'first' },
-            { metric_name: 'second' },
-            { metric_name: 'count' },
-          ],
-        },
-      };
-      const control = getControlState('metrics', 'table', stateWithCount);
-      expect(control?.default).toEqual(['count']);
+      expect(control?.default).toBeUndefined();
    });

    it('should not apply mapStateToProps when initializing', () => {
@ -180,7 +164,6 @@ describe('controlUtils', () => {
        ...state,
        controls: undefined,
      });
-      expect(typeof control?.default).toBe('function');
      expect(control?.value).toBe(undefined);
    });
  });
--- a/superset-frontend/src/explore/components/ExploreViewContainer.jsx
+++ b/superset-frontend/src/explore/components/ExploreViewContainer.jsx
@ -370,18 +370,34 @@ function ExploreViewContainer(props) {

  function renderErrorMessage() {
    // Returns an error message as a node if any errors are in the store
-    const errors = Object.entries(props.controls)
-      .filter(
-        ([, control]) =>
-          control.validationErrors && control.validationErrors.length > 0,
-      )
-      .map(([key, control]) => (
-        <div key={key}>
-          {t('Control labeled ')}
-          <strong>{` "${control.label}" `}</strong>
-          {control.validationErrors.join('. ')}
+    const controlsWithErrors = Object.values(props.controls).filter(
+      control =>
+        control.validationErrors && control.validationErrors.length > 0,
+    );
+    if (controlsWithErrors.length === 0) {
+      return null;
+    }
+
+    const errorMessages = controlsWithErrors.map(
+      control => control.validationErrors,
+    );
+    const uniqueErrorMessages = [...new Set(errorMessages.flat())];
+
+    const errors = uniqueErrorMessages
+      .map(message => {
+        const matchingLabels = controlsWithErrors
+          .filter(control => control.validationErrors?.includes(message))
+          .map(control => control.label);
+        return [matchingLabels, message];
+      })
+      .map(([labels, message]) => (
+        <div key={message}>
+          {labels.length > 1 ? t('Controls labeled ') : t('Control labeled ')}
+          <strong>{` ${labels.join(', ')}`}</strong>
+          <span>: {message}</span>
        </div>
      ));
+
    let errorMessage;
    if (errors.length > 0) {
      errorMessage = <div style={{ textAlign: 'left' }}>{errors}</div>;
--- a/superset-frontend/src/explore/constants.ts
+++ b/superset-frontend/src/explore/constants.ts
@ -106,4 +106,4 @@ export const TIME_FILTER_MAP = {
 };

 // TODO: make this configurable per Superset installation
-export const DEFAULT_TIME_RANGE = 'Last week';
+export const DEFAULT_TIME_RANGE = 'No filter';
--- a/superset-frontend/src/explore/controlUtils/getControlState.ts
+++ b/superset-frontend/src/explore/controlUtils/getControlState.ts
@ -40,15 +40,17 @@ function execControlValidator<T = ControlType>(
  processedState: ControlState<T>,
 ) {
  const validators = control.validators as ControlValueValidator[] | undefined;
-  const validationErrors: ValidationError[] = [];
+  const { externalValidationErrors = [] } = control;
+  const errors: ValidationError[] = [];
  if (validators && validators.length > 0) {
    validators.forEach(validator => {
      const error = validator.call(control, control.value, processedState);
      if (error) {
-        validationErrors.push(error);
+        errors.push(error);
      }
    });
  }
+  const validationErrors = [...errors, ...externalValidationErrors];
  // always reset validation errors even when there is no validator
  return { ...control, validationErrors };
 }
--- a/superset-frontend/src/explore/controls.jsx
+++ b/superset-frontend/src/explore/controls.jsx
@ -64,7 +64,7 @@ import {
  legacyValidateInteger,
  validateNonEmpty,
 } from '@superset-ui/core';
-import { formatSelectOptions, mainMetric } from 'src/modules/utils';
+import { formatSelectOptions } from 'src/modules/utils';
 import { TIME_FILTER_LABELS } from './constants';
 import { StyledColumnOption } from './components/optionRenderers';

@ -152,10 +152,6 @@ const metrics = {
  multi: true,
  label: t('Metrics'),
  validators: [validateNonEmpty],
-  default: c => {
-    const metric = mainMetric(c.savedMetrics);
-    return metric ? [metric] : null;
-  },
  mapStateToProps: state => {
    const { datasource } = state;
    return {
@ -171,7 +167,6 @@ const metric = {
  multi: false,
  label: t('Metric'),
  description: t('Metric'),
-  default: props => mainMetric(props.savedMetrics),
 };

 export function columnChoices(datasource) {
@ -346,7 +341,7 @@ export const controls = {
    type: 'DateFilterControl',
    freeForm: true,
    label: TIME_FILTER_LABELS.time_range,
-    default: t('Last week'), // this value is translated, but the backend wouldn't understand a translated value?
+    default: t('No filter'), // this value is translated, but the backend wouldn't understand a translated value?
    description: t(
      'The time range for the visualization. All relative times, e.g. "Last month", ' +
        '"Last 7 days", "now", etc. are evaluated on the server using the server\'s ' +
--- a/superset-frontend/src/explore/reducers/exploreReducer.js
+++ b/superset-frontend/src/explore/reducers/exploreReducer.js
@ -135,6 +135,24 @@ export default function exploreReducer(state = {}, action) {
        ...getControlStateFromControlConfig(controlConfig, state, action.value),
      };

+      const newState = {
+        ...state,
+        controls: { ...state.controls, [action.controlName]: control },
+      };
+
+      const rerenderedControls = {};
+      if (Array.isArray(control.rerender)) {
+        control.rerender.forEach(controlName => {
+          rerenderedControls[controlName] = {
+            ...getControlStateFromControlConfig(
+              newState.controls[controlName],
+              newState,
+              newState.controls[controlName].value,
+            ),
+          };
+        });
+      }
+
      // combine newly detected errors with errors from `onChange` event of
      // each control component (passed via reducer action).
      const errors = control.validationErrors || [];
@ -169,6 +187,7 @@ export default function exploreReducer(state = {}, action) {
            ...control,
            validationErrors: errors,
          },
+          ...rerenderedControls,
        },
      };
    },