This is an automated email from the ASF dual-hosted git repository.
villebro pushed a commit to branch 0.37
in repository https://gitbox.apache.org/repos/asf/incubator-superset.git
commit 1bc26797add82e60469afccff81e2d9eaa38552e
Author: Jesse Yang <jesse.y...@airbnb.com>
AuthorDate: Mon Jul 13 23:44:57 2020 -0700
fix(table-viz): table chart time column should use default (#10293)
---
.../visualizations/{line.test.js => line.test.ts} | 23 ++----
.../explore/visualizations/table.test.ts | 9 +++
.../spec/javascripts/explore/controlUtils_spec.jsx | 15 +++-
.../explore/components/ControlPanelsContainer.jsx | 5 +-
superset-frontend/src/explore/controlUtils.js | 82 ++++++++++------------
.../src/explore/reducers/getInitialState.js | 2 -
6 files changed, 70 insertions(+), 66 deletions(-)
diff --git
a/superset-frontend/cypress-base/cypress/integration/explore/visualizations/line.test.js
b/superset-frontend/cypress-base/cypress/integration/explore/visualizations/line.test.ts
similarity index 96%
rename from
superset-frontend/cypress-base/cypress/integration/explore/visualizations/line.test.js
rename to
superset-frontend/cypress-base/cypress/integration/explore/visualizations/line.test.ts
index 229c7f4..76a9086 100644
---
a/superset-frontend/cypress-base/cypress/integration/explore/visualizations/line.test.js
+++
b/superset-frontend/cypress-base/cypress/integration/explore/visualizations/line.test.ts
@@ -45,6 +45,13 @@ describe('Visualization > Line', () => {
cy.get('.alert-warning').should('not.exist');
});
+ it('should allow negative values in Y bounds', () => {
+ cy.get('#controlSections-tab-display').click();
+ cy.get('span').contains('Y Axis Bounds').scrollIntoView();
+ cy.get('input[placeholder="Min"]').type('-0.1', { delay: 100 });
+ cy.get('.alert-warning').should('not.exist');
+ });
+
it('should work with adhoc metric', () => {
const formData = { ...LINE_CHART_DEFAULTS, metrics: [NUM_METRIC] };
cy.visitChartByParams(JSON.stringify(formData));
@@ -54,9 +61,7 @@ describe('Visualization > Line', () => {
it('should work with groupby', () => {
const metrics = ['count'];
const groupby = ['gender'];
-
const formData = { ...LINE_CHART_DEFAULTS, metrics, groupby };
-
cy.visitChartByParams(JSON.stringify(formData));
cy.verifySliceSuccess({ waitAlias: '@getJson', chartSelector: 'svg' });
});
@@ -64,13 +69,11 @@ describe('Visualization > Line', () => {
it('should work with simple filter', () => {
const metrics = ['count'];
const filters = [SIMPLE_FILTER];
-
const formData = {
...LINE_CHART_DEFAULTS,
metrics,
adhoc_filters: filters,
};
-
cy.visitChartByParams(JSON.stringify(formData));
cy.verifySliceSuccess({ waitAlias: '@getJson', chartSelector: 'svg' });
});
@@ -83,7 +86,6 @@ describe('Visualization > Line', () => {
groupby: ['name'],
timeseries_limit_metric: NUM_METRIC,
};
-
cy.visitChartByParams(JSON.stringify(formData));
cy.verifySliceSuccess({ waitAlias: '@getJson', chartSelector: 'svg' });
});
@@ -97,28 +99,24 @@ describe('Visualization > Line', () => {
timeseries_limit_metric: NUM_METRIC,
order_desc: true,
};
-
cy.visitChartByParams(JSON.stringify(formData));
cy.verifySliceSuccess({ waitAlias: '@getJson', chartSelector: 'svg' });
});
it('should work with rolling avg', () => {
const metrics = [NUM_METRIC];
-
const formData = {
...LINE_CHART_DEFAULTS,
metrics,
rolling_type: 'mean',
rolling_periods: 10,
};
-
cy.visitChartByParams(JSON.stringify(formData));
cy.verifySliceSuccess({ waitAlias: '@getJson', chartSelector: 'svg' });
});
it('should work with time shift 1 year', () => {
const metrics = [NUM_METRIC];
-
const formData = {
...LINE_CHART_DEFAULTS,
metrics,
@@ -162,28 +160,24 @@ describe('Visualization > Line', () => {
it('should work with time shift yoy', () => {
const metrics = [NUM_METRIC];
-
const formData = {
...LINE_CHART_DEFAULTS,
metrics,
time_compare: ['1+year'],
comparison_type: 'ratio',
};
-
cy.visitChartByParams(JSON.stringify(formData));
cy.verifySliceSuccess({ waitAlias: '@getJson', chartSelector: 'svg' });
});
it('should work with time shift percentage change', () => {
const metrics = [NUM_METRIC];
-
const formData = {
...LINE_CHART_DEFAULTS,
metrics,
time_compare: ['1+year'],
comparison_type: 'percentage',
};
-
cy.visitChartByParams(JSON.stringify(formData));
cy.verifySliceSuccess({ waitAlias: '@getJson', chartSelector: 'svg' });
});
@@ -193,7 +187,6 @@ describe('Visualization > Line', () => {
...LINE_CHART_DEFAULTS,
metrics: ['count'],
};
-
cy.visitChartByParams(JSON.stringify(formData));
cy.verifySliceSuccess({ waitAlias: '@getJson', chartSelector: 'svg' });
cy.get('text.nv-legend-text').contains('COUNT(*)');
@@ -224,7 +217,6 @@ describe('Visualization > Line', () => {
},
],
};
-
cy.visitChartByParams(JSON.stringify(formData));
cy.verifySliceSuccess({ waitAlias: '@getJson', chartSelector: 'svg' });
cy.get('.slice_container').within(() => {
@@ -263,7 +255,6 @@ describe('Visualization > Line', () => {
cy.visitChartByParams(JSON.stringify(formData));
},
);
-
cy.verifySliceSuccess({ waitAlias: '@getJson', chartSelector: 'svg' });
cy.get('.slice_container').within(() => {
cy.get('.nv-event-annotation-layer-0')
diff --git
a/superset-frontend/cypress-base/cypress/integration/explore/visualizations/table.test.ts
b/superset-frontend/cypress-base/cypress/integration/explore/visualizations/table.test.ts
index 20ba9bb..f01e1c7 100644
---
a/superset-frontend/cypress-base/cypress/integration/explore/visualizations/table.test.ts
+++
b/superset-frontend/cypress-base/cypress/integration/explore/visualizations/table.test.ts
@@ -35,6 +35,15 @@ describe('Visualization > Table', () => {
cy.route('POST', '/superset/explore_json/**').as('getJson');
});
+ it('Use default time column', () => {
+ cy.visitChartByParams({
+ ...VIZ_DEFAULTS,
+ granularity_sqla: undefined,
+ metrics: ['count'],
+ });
+ cy.get('input[name="select-granularity_sqla"]').should('have.value', 'ds');
+ });
+
it('Format non-numeric metrics correctly', () => {
cy.visitChartByParams({
...VIZ_DEFAULTS,
diff --git a/superset-frontend/spec/javascripts/explore/controlUtils_spec.jsx
b/superset-frontend/spec/javascripts/explore/controlUtils_spec.jsx
index e7355cb..ad87021 100644
--- a/superset-frontend/spec/javascripts/explore/controlUtils_spec.jsx
+++ b/superset-frontend/spec/javascripts/explore/controlUtils_spec.jsx
@@ -35,6 +35,7 @@ describe('controlUtils', () => {
columns: ['a', 'b', 'c'],
metrics: [{ metric_name: 'first' }, { metric_name: 'second' }],
},
+ controls: {},
};
beforeAll(() => {
@@ -250,9 +251,10 @@ describe('controlUtils', () => {
it('should not apply mapStateToProps when initializing', () => {
const control = getControlState('metrics', 'table', {
...state,
- isInitializing: true,
+ controls: undefined,
});
- expect(control.default).toEqual(null);
+ expect(typeof control.default).toBe('function');
+ expect(control.value).toBe(undefined);
});
});
@@ -261,6 +263,15 @@ describe('controlUtils', () => {
const control = getControlState('metric', 'table', state, null);
expect(control.validationErrors).toEqual(['cannot be empty']);
});
+ it('should not validate if control panel is initializing', () => {
+ const control = getControlState(
+ 'metric',
+ 'table',
+ { ...state, controls: undefined },
+ undefined,
+ );
+ expect(control.validationErrors).toBeUndefined();
+ });
});
describe('queryFields', () => {
diff --git
a/superset-frontend/src/explore/components/ControlPanelsContainer.jsx
b/superset-frontend/src/explore/components/ControlPanelsContainer.jsx
index 77aa5a7..861d301 100644
--- a/superset-frontend/src/explore/components/ControlPanelsContainer.jsx
+++ b/superset-frontend/src/explore/components/ControlPanelsContainer.jsx
@@ -66,15 +66,14 @@ class ControlPanelsContainer extends React.Component {
renderControl({ name, config }) {
const { actions, controls, exploreState, form_data: formData } =
this.props;
const { visibility } = config;
+
// If the control item is not an object, we have to look up the control
data from
// the centralized controls file.
// When it is an object we read control data straight from `config` instead
const controlData = {
- ...controls[name],
...config,
+ ...controls[name],
name,
- // apply current value in formData
- value: formData[name],
};
const {
validationErrors,
diff --git a/superset-frontend/src/explore/controlUtils.js
b/superset-frontend/src/explore/controlUtils.js
index 1219ff7..ce3022e 100644
--- a/superset-frontend/src/explore/controlUtils.js
+++ b/superset-frontend/src/explore/controlUtils.js
@@ -36,18 +36,17 @@ export function getFormDataFromControls(controlsState) {
export function validateControl(control, processedState) {
const validators = control.validators;
+ const validationErrors = [];
if (validators && validators.length > 0) {
- const validatedControl = { ...control };
- const validationErrors = [];
validators.forEach(f => {
const v = f.call(control, control.value, processedState);
if (v) {
validationErrors.push(v);
}
});
- return { ...validatedControl, validationErrors };
}
- return control;
+ // always reset validation errors even when there is no validator
+ return { ...control, validationErrors };
}
/**
@@ -88,17 +87,6 @@ export const getControlConfig = memoizeOne(function
getControlConfig(
return control?.config || control;
});
-export function applyMapStateToPropsToControl(controlState, controlPanelState)
{
- const { mapStateToProps } = controlState;
- if (mapStateToProps && controlPanelState) {
- return {
- ...controlState,
- ...mapStateToProps(controlPanelState, controlState),
- };
- }
- return controlState;
-}
-
function handleMissingChoice(control) {
// If the value is not valid anymore based on choices, clear it
const value = control.value;
@@ -121,6 +109,36 @@ function handleMissingChoice(control) {
return control;
}
+export function applyMapStateToPropsToControl(controlState, controlPanelState)
{
+ const { mapStateToProps } = controlState;
+ let { value } = controlState;
+ let state = { ...controlState };
+ if (mapStateToProps && controlPanelState) {
+ state = {
+ ...controlState,
+ ...mapStateToProps(controlPanelState, controlState),
+ };
+ }
+ // If default is a function, evaluate it
+ if (typeof state.default === 'function') {
+ state.default = state.default(state, controlPanelState);
+ // if default is still a function, discard
+ if (typeof state.default === 'function') {
+ delete state.default;
+ }
+ }
+ // If no current value, set it as default
+ if (state.default && value === undefined) {
+ value = state.default;
+ }
+ // If a choice control went from multi=false to true, wrap value in array
+ if (value && state.multi && !Array.isArray(value)) {
+ value = [value];
+ }
+ state.value = value;
+ return validateControl(handleMissingChoice(state), state);
+}
+
export function getControlStateFromControlConfig(
controlConfig,
controlPanelState,
@@ -130,38 +148,16 @@ export function getControlStateFromControlConfig(
if (!controlConfig) {
return null;
}
- let controlState = { ...controlConfig };
+ const controlState = { ...controlConfig, value };
// only apply mapStateToProps when control states have been initialized
+ // or when explicitly didn't provide control panel state (mostly for testing)
if (
- controlPanelState &&
- (controlPanelState.controls || !controlPanelState.isInitializing)
+ (controlPanelState && controlPanelState.controls) ||
+ controlPanelState === null
) {
- controlState = applyMapStateToPropsToControl(
- controlState,
- controlPanelState,
- );
- }
-
- // If default is a function, evaluate it
- if (typeof controlState.default === 'function') {
- controlState.default = controlState.default(
- controlState,
- controlPanelState,
- );
- // if default is still a function, discard
- if (typeof controlState.default === 'function') {
- delete controlState.default;
- }
+ return applyMapStateToPropsToControl(controlState, controlPanelState);
}
-
- // If a choice control went from multi=false to true, wrap value in array
- const controlValue =
- controlConfig.multi && value && !Array.isArray(value) ? [value] : value;
-
- controlState.value =
- typeof controlValue === 'undefined' ? controlState.default : controlValue;
-
- return validateControl(handleMissingChoice(controlState), controlState);
+ return controlState;
}
export function getControlState(controlKey, vizType, state, value) {
diff --git a/superset-frontend/src/explore/reducers/getInitialState.js
b/superset-frontend/src/explore/reducers/getInitialState.js
index 4efa18d..9f1a672 100644
--- a/superset-frontend/src/explore/reducers/getInitialState.js
+++ b/superset-frontend/src/explore/reducers/getInitialState.js
@@ -41,7 +41,6 @@ export default function getInitialState(bootstrapData) {
filterColumnOpts: [],
isDatasourceMetaLoading: false,
isStarred: false,
- isInitializing: true,
};
const controls = getControlsState(bootstrappedState, rawFormData);
bootstrappedState.controls = controls;
@@ -55,7 +54,6 @@ export default function getInitialState(bootstrapData) {
bootstrappedState,
);
});
- bootstrappedState.isInitializing = false;
const sliceFormData = slice
? getFormDataFromControls(getControlsState(bootstrapData, slice.form_data))
