You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@superset.apache.org by GitBox <gi...@apache.org> on 2018/04/23 05:40:13 UTC
[GitHub] mistercrunch closed pull request #2818: add validation rule for
visualize
mistercrunch closed pull request #2818: add validation rule for visualize
URL: https://github.com/apache/incubator-superset/pull/2818
This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:
As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):
diff --git a/superset/assets/javascripts/SqlLab/components/VisualizeModal.jsx b/superset/assets/javascripts/SqlLab/components/VisualizeModal.jsx
index 2ebfef9318..65a12a7091 100644
--- a/superset/assets/javascripts/SqlLab/components/VisualizeModal.jsx
+++ b/superset/assets/javascripts/SqlLab/components/VisualizeModal.jsx
@@ -10,16 +10,14 @@ import Select from 'react-select';
import { Table } from 'reactable';
import shortid from 'shortid';
import { getExploreUrl } from '../../explore/exploreUtils';
+import { visTypes } from '../../explore/stores/visTypes';
import * as actions from '../actions';
import { VISUALIZE_VALIDATION_ERRORS } from '../constants';
import { QUERY_TIMEOUT_THRESHOLD } from '../../constants';
-const CHART_TYPES = [
- { value: 'dist_bar', label: 'Distribution - Bar Chart', requiresTime: false },
- { value: 'pie', label: 'Pie Chart', requiresTime: false },
- { value: 'line', label: 'Time Series - Line Chart', requiresTime: true },
- { value: 'bar', label: 'Time Series - Bar Chart', requiresTime: true },
-];
+const CHART_TYPES = Object.keys(visTypes)
+ .filter(key => ['bar', 'line', 'pie', 'dist_bar'].indexOf(key) > -1)
+ .map(key => Object.assign({}, visTypes[key], { value: key }));
const propTypes = {
actions: PropTypes.object.isRequired,
@@ -89,20 +87,28 @@ class VisualizeModal extends React.PureComponent {
});
if (this.state.chartType === null) {
hints.push(VISUALIZE_VALIDATION_ERRORS.REQUIRE_CHART_TYPE);
- } else if (this.state.chartType.requiresTime) {
- let hasTime = false;
- for (const colName in cols) {
- const col = cols[colName];
- if (col.hasOwnProperty('is_date') && col.is_date) {
- hasTime = true;
- }
- }
- if (!hasTime) {
+ } else {
+ if (this.state.chartType.requiresTime && !this.hasColumnType(cols, 'is_date')) {
hints.push(VISUALIZE_VALIDATION_ERRORS.REQUIRE_TIME);
}
+ if (this.state.chartType.requiresDimension && !this.hasColumnType(cols, 'is_dim')) {
+ hints.push(VISUALIZE_VALIDATION_ERRORS.REQUIRE_DIMENSION);
+ }
+ if (this.state.chartType.requiresAggregationFn && !this.hasColumnType(cols, 'agg')) {
+ hints.push(VISUALIZE_VALIDATION_ERRORS.REQUIRE_AGGREGATION_FUNCTION);
+ }
}
this.setState({ hints });
}
+ hasColumnType(cols, propName) {
+ if (!cols || !propName) {
+ return false;
+ }
+
+ return Object.keys(cols)
+ .filter(key => cols[key].hasOwnProperty(propName) && cols[key][propName])
+ .length > 0;
+ }
changeChartType(option) {
this.setState({ chartType: option }, this.validate);
}
diff --git a/superset/assets/javascripts/explore/stores/visTypes.js b/superset/assets/javascripts/explore/stores/visTypes.js
index 1e2eec2317..d93f920af5 100644
--- a/superset/assets/javascripts/explore/stores/visTypes.js
+++ b/superset/assets/javascripts/explore/stores/visTypes.js
@@ -74,9 +74,10 @@ export const sections = {
],
};
-const visTypes = {
+export const visTypes = {
dist_bar: {
label: 'Distribution - Bar Chart',
+ requiresDimension: true,
controlPanelSections: [
{
label: 'Chart Options',
@@ -107,6 +108,7 @@ const visTypes = {
pie: {
label: 'Pie Chart',
+ requiresDimension: true,
controlPanelSections: [
{
label: null,
@@ -194,6 +196,7 @@ const visTypes = {
bar: {
label: 'Time Series - Bar Chart',
requiresTime: true,
+ requiresDimension: true,
controlPanelSections: [
sections.NVD3TimeSeries[0],
{
diff --git a/superset/assets/spec/javascripts/sqllab/VisualizeModal_spec.jsx b/superset/assets/spec/javascripts/sqllab/VisualizeModal_spec.jsx
index eab4608889..0d3de321e2 100644
--- a/superset/assets/spec/javascripts/sqllab/VisualizeModal_spec.jsx
+++ b/superset/assets/spec/javascripts/sqllab/VisualizeModal_spec.jsx
@@ -3,6 +3,7 @@ import configureStore from 'redux-mock-store';
import thunk from 'redux-thunk';
import { Modal } from 'react-bootstrap';
+import Select from 'react-select';
import { shallow } from 'enzyme';
import { describe, it } from 'mocha';
import { expect } from 'chai';
@@ -16,6 +17,7 @@ import * as actions from '../../../javascripts/SqlLab/actions';
import { VISUALIZE_VALIDATION_ERRORS } from '../../../javascripts/SqlLab/constants';
import VisualizeModal from '../../../javascripts/SqlLab/components/VisualizeModal';
import * as exploreUtils from '../../../javascripts/explore/exploreUtils';
+import { visTypes } from '../../../javascripts/explore/stores/visTypes';
global.notify = {
info: () => {},
@@ -45,16 +47,6 @@ describe('VisualizeModal', () => {
type: 'STRING',
},
};
- const mockChartTypeBarChart = {
- label: 'Distribution - Bar Chart',
- requiresTime: false,
- value: 'dist_bar',
- };
- const mockChartTypeTB = {
- label: 'Time Series - Bar Chart',
- requiresTime: true,
- value: 'bar',
- };
const mockEvent = {
target: {
value: 'mock event value',
@@ -143,10 +135,6 @@ describe('VisualizeModal', () => {
describe('mergedColumns', () => {
const wrapper = getVisualizeModalWrapper();
- const oldColumns = {
- ds: 1,
- gender: 2,
- };
it('should merge by column name', () => {
wrapper.setState({ columns: {} });
@@ -154,6 +142,10 @@ describe('VisualizeModal', () => {
expect(mc).to.deep.equal(mockColumns);
});
it('should not override current state', () => {
+ const oldColumns = {
+ ds: 1,
+ gender: 2,
+ };
wrapper.setState({ columns: oldColumns });
const mc = wrapper.instance().mergedColumns();
@@ -174,17 +166,16 @@ describe('VisualizeModal', () => {
it('should validate column name', () => {
columnsStub.returns(mockColumns);
+ wrapper.setState({ chartType: visTypes.dist_bar });
wrapper.instance().validate();
expect(wrapper.state().hints).to.have.length(0);
- wrapper.instance().mergedColumns.restore();
});
it('should hint invalid column name', () => {
columnsStub.returns({
'&': 1,
});
wrapper.instance().validate();
- expect(wrapper.state().hints).to.have.length(1);
- wrapper.instance().mergedColumns.restore();
+ expect(wrapper.state().hints).to.have.length.above(1);
});
it('should hint empty chartType', () => {
columnsStub.returns(mockColumns);
@@ -196,7 +187,7 @@ describe('VisualizeModal', () => {
});
it('should check time series', () => {
columnsStub.returns(mockColumns);
- wrapper.setState({ chartType: mockChartTypeTB });
+ wrapper.setState({ chartType: visTypes.line });
wrapper.instance().validate();
expect(wrapper.state().hints).to.have.length(0);
@@ -215,11 +206,61 @@ describe('VisualizeModal', () => {
type: 'STRING',
},
});
- wrapper.setState({ chartType: mockChartTypeTB });
+ wrapper.setState({ chartType: visTypes.line });
wrapper.instance().validate();
expect(wrapper.state().hints).to.have.length(1);
expect(wrapper.state().hints[0]).to.have.string(VISUALIZE_VALIDATION_ERRORS.REQUIRE_TIME);
});
+ it('should check dimension', () => {
+ // no is_dim
+ columnsStub.returns({
+ ds: {
+ is_date: true,
+ is_dim: false,
+ name: 'ds',
+ type: 'STRING',
+ },
+ gender: {
+ is_date: false,
+ is_dim: false,
+ name: 'gender',
+ type: 'STRING',
+ },
+ });
+ wrapper.setState({ chartType: visTypes.bar });
+ wrapper.instance().validate();
+ expect(wrapper.state().hints).to.have.length(1);
+ expect(wrapper.state().hints[0])
+ .to.have.string(VISUALIZE_VALIDATION_ERRORS.REQUIRE_DIMENSION);
+ });
+ it('should check aggregation function', () => {
+ // no agg fn
+ columnsStub.returns({
+ ds: {
+ is_date: true,
+ is_dim: false,
+ name: 'ds',
+ type: 'STRING',
+ },
+ gender: {
+ is_date: false,
+ is_dim: false,
+ name: 'gender',
+ type: 'STRING',
+ agg: null,
+ },
+ });
+ const mockCharType = {
+ label: 'Aggregation Chart',
+ requiresAggregationFn: true,
+ controlPanelSections: [],
+ };
+ wrapper.setState({ chartType: mockCharType });
+ wrapper.instance().validate();
+ expect(wrapper.state().hints).to.have.length(1);
+ expect(wrapper.state().hints[0])
+ .to.have.string(VISUALIZE_VALIDATION_ERRORS.REQUIRE_AGGREGATION_FUNCTION);
+ });
it('should validate after change checkbox', () => {
const spy = sinon.spy(wrapper.instance(), 'validate');
columnsStub.returns(mockColumns);
@@ -231,21 +272,26 @@ describe('VisualizeModal', () => {
it('should validate after change Agg function', () => {
const spy = sinon.spy(wrapper.instance(), 'validate');
columnsStub.returns(mockColumns);
-
+ // set agg fn
wrapper.instance().changeAggFunction('num', { label: 'MIN(x)', value: 'min' });
+ expect(wrapper.state().columns.num).to.deep.equal({ agg: 'min' });
expect(spy.callCount).to.equal(1);
+ // clear agg fn
+ wrapper.instance().changeAggFunction('num');
+ expect(wrapper.state().columns.num).to.deep.equal({ agg: null });
+ expect(spy.callCount).to.equal(2);
spy.restore();
});
});
it('should validate after change chart type', () => {
const wrapper = getVisualizeModalWrapper();
- wrapper.setState({ chartType: mockChartTypeTB });
+ wrapper.setState({ chartType: visTypes.line });
const spy = sinon.spy(wrapper.instance(), 'validate');
- wrapper.instance().changeChartType(mockChartTypeBarChart);
+ wrapper.instance().changeChartType(visTypes.bar);
expect(spy.callCount).to.equal(1);
- expect(wrapper.state().chartType).to.equal(mockChartTypeBarChart);
+ expect(wrapper.state().chartType).to.equal(visTypes.bar);
});
it('should validate after change datasource name', () => {
@@ -259,7 +305,7 @@ describe('VisualizeModal', () => {
it('should build viz options', () => {
const wrapper = getVisualizeModalWrapper();
- wrapper.setState({ chartType: mockChartTypeTB });
+ wrapper.setState({ chartType: visTypes.line });
const spy = sinon.spy(wrapper.instance(), 'buildVizOptions');
wrapper.instance().buildVizOptions();
expect(spy.returnValues[0]).to.deep.equal({
@@ -295,7 +341,7 @@ describe('VisualizeModal', () => {
const wrapper = getVisualizeModalWrapper();
const mockOptions = { attr: 'mockOptions' };
wrapper.setState({
- chartType: mockChartTypeBarChart,
+ chartType: visTypes.bar,
columns: mockColumns,
datasourceName: 'mockDatasourceName',
});
@@ -353,4 +399,21 @@ describe('VisualizeModal', () => {
expect(notify.error.callCount).to.equal(1);
});
});
+
+ describe('render', () => {
+ it('should have 4 chart types', () => {
+ const wrapper = getVisualizeModalWrapper();
+ expect(wrapper.find(Modal)).to.have.length(1);
+
+ const selectorOptions = wrapper.find(Modal).dive()
+ .find(Modal.Body).dive()
+ .find(Select)
+ .props().options;
+ expect(selectorOptions).to.have.length(4);
+
+ const selectorOptionsValues =
+ Object.keys(selectorOptions).map(key => selectorOptions[key].value);
+ expect(selectorOptionsValues).to.have.same.members(['bar', 'line', 'pie', 'dist_bar']);
+ });
+ });
});
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services