Skip to content

Commit

Permalink
feat(explore): Remove default for time range filter and Metrics (apac…
Browse files Browse the repository at this point in the history 
…he#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
  • Loading branch information
@kgabryje
kgabryje authored and cccs-RyanS committed Dec 17, 2021
1 parent ea18859 commit 828a301
Show file tree
Hide file tree
Showing 9 changed files with 398 additions and 381 deletions.
6 changes: 4 additions & 2 deletions superset-frontend/cypress-base/cypress/integration/explore/visualizations/line.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand All @@ -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]')
Expand All @@ -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 });
Expand Down
620 changes: 310 additions & 310 deletions superset-frontend/package-lock.json
View file

Large diffs are not rendered by default.

56 changes: 28 additions & 28 deletions superset-frontend/package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -75,35 +75,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",
Expand Down
25 changes: 4 additions & 21 deletions superset-frontend/spec/javascripts/explore/controlUtils_spec.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -149,38 +149,21 @@ 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', () => {
const control = getControlState('metrics', 'table', {
...state,
controls: undefined,
});
expect(typeof control?.default).toBe('function');
expect(control?.value).toBe(undefined);
});
});
Expand Down
36 changes: 26 additions & 10 deletions superset-frontend/src/explore/components/ExploreViewContainer.jsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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>;
Expand Down
2 changes: 1 addition & 1 deletion superset-frontend/src/explore/constants.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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';
6 changes: 4 additions & 2 deletions superset-frontend/src/explore/controlUtils/getControlState.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 };
}
Expand Down
9 changes: 2 additions & 7 deletions superset-frontend/src/explore/controls.jsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand Down Expand Up @@ -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 {
Expand All @@ -171,7 +167,6 @@ const metric = {
multi: false,
label: t('Metric'),
description: t('Metric'),
default: props => mainMetric(props.savedMetrics),
};

export function columnChoices(datasource) {
Expand Down Expand Up @@ -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 ' +
Expand Down
19 changes: 19 additions & 0 deletions superset-frontend/src/explore/reducers/exploreReducer.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 || [];
Expand Down Expand Up @@ -169,6 +187,7 @@ export default function exploreReducer(state = {}, action) {
...control,
validationErrors: errors,
},
...rerenderedControls,
},
};
},
Expand Down

0 comments on commit 828a301

Please sign in to comment.