From 2353248d2c9d78d0d31bd63054cf69c1a616957d Mon Sep 17 00:00:00 2001 From: Grace Guo Date: Sat, 20 May 2017 22:54:10 -0700 Subject: [PATCH 1/9] redux visualize modal --- superset/assets/javascripts/SqlLab/actions.js | 39 +++++++++ .../SqlLab/components/VisualizeModal.jsx | 52 +++++++----- .../assets/javascripts/SqlLab/reducers.js | 19 +++++ superset/assets/package.json | 1 + .../javascripts/sqllab/QueryTable_spec.jsx | 11 +-- .../sqllab/VisualizeModal_spec.jsx | 85 ++++++++++++++++++- superset/assets/stylesheets/superset.css | 4 + 7 files changed, 186 insertions(+), 25 deletions(-) diff --git a/superset/assets/javascripts/SqlLab/actions.js b/superset/assets/javascripts/SqlLab/actions.js index bc8d1d0ae0e8..802cbc3b2d3f 100644 --- a/superset/assets/javascripts/SqlLab/actions.js +++ b/superset/assets/javascripts/SqlLab/actions.js @@ -37,6 +37,10 @@ export const REMOVE_DATA_PREVIEW = 'REMOVE_DATA_PREVIEW'; export const CHANGE_DATA_PREVIEW_ID = 'CHANGE_DATA_PREVIEW_ID'; export const SAVE_QUERY = 'SAVE_QUERY'; +export const CREATE_DATASOURCE_STARTED = 'CREATE_DATASOURCE_STARTED'; +export const CREATE_DATASOURCE_SUCCESS = 'CREATE_DATASOURCE_SUCCESS'; +export const CREATE_DATASOURCE_FAILED = 'CREATE_DATASOURCE_FAILED'; + export function resetState() { return { type: RESET_STATE }; } @@ -382,3 +386,38 @@ export function popSavedQuery(saveQueryId) { }); }; } + +export function createDatasourceStarted() { + return { type: CREATE_DATASOURCE_STARTED }; +} +export function createDatasourceSuccess(response) { + const data = JSON.parse(response); + const datasource = `${data.table_id}__table`; + return { type: CREATE_DATASOURCE_SUCCESS, datasource }; +} +export function createDatasourceFailed(err) { + return { type: CREATE_DATASOURCE_FAILED, err }; +} + +export function createDatasource(vizOptions, context) { + return (dispatch) => { + dispatch(createDatasourceStarted()); + + return $.ajax({ + type: 'POST', + url: '/superset/sqllab_viz/', + async: false, + data: { + data: JSON.stringify(vizOptions), + }, + context, + dataType: 'json', + success: (resp) => { + dispatch(createDatasourceSuccess(resp)); + }, + error: () => { + dispatch(createDatasourceFailed('An error occurred while creating the data source')); + }, + }); + }; +} diff --git a/superset/assets/javascripts/SqlLab/components/VisualizeModal.jsx b/superset/assets/javascripts/SqlLab/components/VisualizeModal.jsx index 7a4edf4c353e..821baa99ae07 100644 --- a/superset/assets/javascripts/SqlLab/components/VisualizeModal.jsx +++ b/superset/assets/javascripts/SqlLab/components/VisualizeModal.jsx @@ -1,13 +1,15 @@ /* global notify */ import React from 'react'; import PropTypes from 'prop-types'; +import { bindActionCreators } from 'redux'; +import { connect } from 'react-redux'; import { Alert, Button, Col, Modal } from 'react-bootstrap'; import Select from 'react-select'; import { Table } from 'reactable'; import shortid from 'shortid'; -import $ from 'jquery'; import { getExploreUrl } from '../../explorev2/exploreUtils'; +import * as actions from '../actions'; const CHART_TYPES = [ { value: 'dist_bar', label: 'Distribution - Bar Chart', requiresTime: false }, @@ -17,9 +19,12 @@ const CHART_TYPES = [ ]; const propTypes = { + actions: PropTypes.object.isRequired, onHide: PropTypes.func, query: PropTypes.object, show: PropTypes.bool, + datasource: PropTypes.string, + errorMessage: PropTypes.string, }; const defaultProps = { show: false, @@ -121,22 +126,14 @@ class VisualizeModal extends React.PureComponent { sql: this.props.query.sql, dbId: this.props.query.dbId, }; - notify.info('Creating a data source and popping a new tab'); - $.ajax({ - type: 'POST', - url: '/superset/sqllab_viz/', - async: false, - data: { - data: JSON.stringify(vizOptions), - }, - dataType: 'json', - success: (resp) => { + + this.props.actions.createDatasource(vizOptions, this) + .done(() => { const columns = Object.keys(this.state.columns).map(k => this.state.columns[k]); - const data = JSON.parse(resp); const mainMetric = columns.filter(d => d.agg)[0]; const mainGroupBy = columns.filter(d => d.is_dim)[0]; const formData = { - datasource: `${data.table_id}__table`, + datasource: this.props.datasource, viz_type: this.state.chartType.value, since: '100 years ago', limit: '0', @@ -148,14 +145,16 @@ class VisualizeModal extends React.PureComponent { if (mainGroupBy) { formData.groupby = [mainGroupBy.name]; } + notify.info('Creating a data source and popping a new tab'); + window.open(getExploreUrl(formData)); - }, - error: () => notify('An error occurred while creating the data source'), - }); + }) + .fail(() => { + notify.error(this.props.errorMessage); + }); } changeDatasourceName(event) { - this.setState({ datasourceName: event.target.value }); - this.validate(); + this.setState({ datasourceName: event.target.value }, this.validate); } changeCheckbox(attr, columnName, event) { let columns = this.mergedColumns(); @@ -271,4 +270,19 @@ class VisualizeModal extends React.PureComponent { VisualizeModal.propTypes = propTypes; VisualizeModal.defaultProps = defaultProps; -export default VisualizeModal; +function mapStateToProps(state) { + return { + datasource: state.datasource, + errorMessage: state.errorMessage, + }; +} + +function mapDispatchToProps(dispatch) { + return { + actions: bindActionCreators(actions, dispatch), + }; +} + +export { VisualizeModal }; +export default connect(mapStateToProps, mapDispatchToProps)(VisualizeModal); + diff --git a/superset/assets/javascripts/SqlLab/reducers.js b/superset/assets/javascripts/SqlLab/reducers.js index 4b721aecbaa8..2f5461ab8b27 100644 --- a/superset/assets/javascripts/SqlLab/reducers.js +++ b/superset/assets/javascripts/SqlLab/reducers.js @@ -251,6 +251,25 @@ export const sqlLabReducer = function (state, action) { } return Object.assign({}, state, { queries: newQueries, queriesLastUpdate }); }, + [actions.CREATE_DATASOURCE_STARTED]() { + return Object.assign({}, state, { + isDatasourceLoading: true, + errorMessage: null, + }); + }, + [actions.CREATE_DATASOURCE_SUCCESS]() { + return Object.assign({}, state, { + isDatasourceLoading: false, + errorMessage: null, + datasource: action.datasource, + }); + }, + [actions.CREATE_DATASOURCE_FAILED]() { + return Object.assign({}, state, { + isDatasourceLoading: false, + errorMessage: action.err, + }); + }, }; if (action.type in actionHandlers) { return actionHandlers[action.type](); diff --git a/superset/assets/package.json b/superset/assets/package.json index cc7bc605c131..17e2510ed131 100644 --- a/superset/assets/package.json +++ b/superset/assets/package.json @@ -126,6 +126,7 @@ "mocha": "^3.2.0", "react-addons-test-utils": "^15.5.1", "react-test-renderer": "^15.5.1", + "redux-mock-store": "^1.2.3", "sinon": "^2.1.0", "style-loader": "^0.16.1", "transform-loader": "^0.2.3", diff --git a/superset/assets/spec/javascripts/sqllab/QueryTable_spec.jsx b/superset/assets/spec/javascripts/sqllab/QueryTable_spec.jsx index 3400d85b9be4..47cf4349ad32 100644 --- a/superset/assets/spec/javascripts/sqllab/QueryTable_spec.jsx +++ b/superset/assets/spec/javascripts/sqllab/QueryTable_spec.jsx @@ -1,11 +1,11 @@ import React from 'react'; -import { mount } from 'enzyme'; +import { shallow } from 'enzyme'; import { describe, it } from 'mocha'; import { expect } from 'chai'; import { queries } from './fixtures'; import QueryTable from '../../../javascripts/SqlLab/components/QueryTable'; - +import { Table } from 'reactable'; describe('QueryTable', () => { const mockedProps = { @@ -20,8 +20,9 @@ describe('QueryTable', () => { ).to.equal(true); }); it('renders a proper table', () => { - const wrapper = mount(); - expect(wrapper.find('table')).to.have.length(1); - expect(wrapper.find('tr')).to.have.length(4); + const wrapper = shallow(); + expect(wrapper.find(Table)).to.have.length(1); + expect(wrapper.find(Table).shallow().find('table')).to.have.length(1); + expect(wrapper.find(Table).shallow().find('table').find('Tr')).to.have.length(2); }); }); diff --git a/superset/assets/spec/javascripts/sqllab/VisualizeModal_spec.jsx b/superset/assets/spec/javascripts/sqllab/VisualizeModal_spec.jsx index c23529d36c58..6dc18b305693 100644 --- a/superset/assets/spec/javascripts/sqllab/VisualizeModal_spec.jsx +++ b/superset/assets/spec/javascripts/sqllab/VisualizeModal_spec.jsx @@ -1,17 +1,58 @@ import React from 'react'; +import configureStore from 'redux-mock-store'; +import thunk from 'redux-thunk'; + import { Modal } from 'react-bootstrap'; import { shallow } from 'enzyme'; import { describe, it } from 'mocha'; import { expect } from 'chai'; +import sinon from 'sinon'; +import $ from 'jquery'; import { queries } from './fixtures'; +import { sqlLabReducer } from '../../../javascripts/SqlLab/reducers'; import VisualizeModal from '../../../javascripts/SqlLab/components/VisualizeModal'; +import * as exploreUtils from '../../../javascripts/explorev2/exploreUtils'; + +global.notify = { + info: () => {}, + error: () => {}, +}; describe('VisualizeModal', () => { + const middlewares = [thunk]; + const mockStore = configureStore(middlewares); + const initialState = sqlLabReducer(undefined, {}); + const store = mockStore(initialState); const mockedProps = { show: true, query: queries[0], }; + const mockColumns = { + ds: { + is_date: true, + is_dim: false, + name: 'ds', + type: 'STRING', + }, + gender: { + is_date: false, + is_dim: true, + name: 'gender', + type: 'STRING', + }, + }; + const mockChartTypeBarChart = { + label: 'Distribution - Bar Chart', + requiresTime: false, + value: 'dist_bar', + }; + + const getVisualizeModalWrapper = () => ( + shallow(, { + context: { store }, + }).dive()); + it('renders', () => { expect(React.isValidElement()).to.equal(true); }); @@ -21,7 +62,49 @@ describe('VisualizeModal', () => { ).to.equal(true); }); it('renders a Modal', () => { - const wrapper = shallow(); + const wrapper = getVisualizeModalWrapper(); expect(wrapper.find(Modal)).to.have.length(1); }); + + describe('visualize', () => { + const wrapper = getVisualizeModalWrapper(); + + wrapper.setState({ + chartType: mockChartTypeBarChart, + columns: mockColumns, + datasourceName: 'mockDatasourceName', + }); + + const vizOptions = { + chartType: wrapper.state().chartType.value, + datasourceName: wrapper.state().datasourceName, + columns: wrapper.state().columns, + sql: wrapper.instance().props.query.sql, + dbId: wrapper.instance().props.query.dbId, + }; + + let spy, server; + beforeEach(() => { + spy = sinon.spy($, 'ajax'); + server = sinon.fakeServer.create(); + sinon.stub(JSON, 'parse').callsFake(() => ({ table_id: 107 })); + sinon.stub(exploreUtils, 'getExploreUrl').callsFake(() => ('mockURL')); + }); + afterEach(() => { + spy.restore(); + server.restore(); + JSON.parse.restore(); + exploreUtils.getExploreUrl.restore(); + }); + + it('should build request', () => { + wrapper.instance().visualize(); + expect(spy.callCount).to.equal(1); + + const spyCall = spy.getCall(0); + expect(spyCall.args[0].type).to.equal('POST'); + expect(spyCall.args[0].url).to.equal('/superset/sqllab_viz/'); + expect(spyCall.args[0].data.data).to.equal(JSON.stringify(vizOptions)); + }); + }); }); diff --git a/superset/assets/stylesheets/superset.css b/superset/assets/stylesheets/superset.css index 61a199040b37..4dafda4221d6 100644 --- a/superset/assets/stylesheets/superset.css +++ b/superset/assets/stylesheets/superset.css @@ -205,3 +205,7 @@ div.widget .slice_container { .table-condensed { font-size: 12px; } + +.table-condensed input[type="checkbox"] { + float: left; +} \ No newline at end of file From 5eec1d66f0bef1e4dcbfb78ebb6ea70857d5dacd Mon Sep 17 00:00:00 2001 From: Grace Guo Date: Sat, 20 May 2017 22:54:10 -0700 Subject: [PATCH 2/9] redux visualize modal - apply redux - add unit test for connect container component - fix lint error --- superset/assets/spec/javascripts/sqllab/QueryTable_spec.jsx | 2 +- .../assets/spec/javascripts/sqllab/VisualizeModal_spec.jsx | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/superset/assets/spec/javascripts/sqllab/QueryTable_spec.jsx b/superset/assets/spec/javascripts/sqllab/QueryTable_spec.jsx index 47cf4349ad32..7c593b76eef1 100644 --- a/superset/assets/spec/javascripts/sqllab/QueryTable_spec.jsx +++ b/superset/assets/spec/javascripts/sqllab/QueryTable_spec.jsx @@ -2,10 +2,10 @@ import React from 'react'; import { shallow } from 'enzyme'; import { describe, it } from 'mocha'; import { expect } from 'chai'; +import { Table } from 'reactable'; import { queries } from './fixtures'; import QueryTable from '../../../javascripts/SqlLab/components/QueryTable'; -import { Table } from 'reactable'; describe('QueryTable', () => { const mockedProps = { diff --git a/superset/assets/spec/javascripts/sqllab/VisualizeModal_spec.jsx b/superset/assets/spec/javascripts/sqllab/VisualizeModal_spec.jsx index 6dc18b305693..75b9929b5dcc 100644 --- a/superset/assets/spec/javascripts/sqllab/VisualizeModal_spec.jsx +++ b/superset/assets/spec/javascripts/sqllab/VisualizeModal_spec.jsx @@ -83,7 +83,9 @@ describe('VisualizeModal', () => { dbId: wrapper.instance().props.query.dbId, }; - let spy, server; + let spy; + let server; + beforeEach(() => { spy = sinon.spy($, 'ajax'); server = sinon.fakeServer.create(); From 31cbba8c4f927a7b901f98f578333446d93ec084 Mon Sep 17 00:00:00 2001 From: Grace Guo Date: Mon, 22 May 2017 16:37:36 -0700 Subject: [PATCH 3/9] add unit tests for VisuaizeModal --- .../SqlLab/components/VisualizeModal.jsx | 11 +- .../javascripts/explorev2/actions_spec.js | 2 +- .../sqllab/VisualizeModal_spec.jsx | 228 ++++++++++++++++-- .../spec/javascripts/sqllab/fixtures.js | 12 +- 4 files changed, 229 insertions(+), 24 deletions(-) diff --git a/superset/assets/javascripts/SqlLab/components/VisualizeModal.jsx b/superset/assets/javascripts/SqlLab/components/VisualizeModal.jsx index 821baa99ae07..fb22c40a157a 100644 --- a/superset/assets/javascripts/SqlLab/components/VisualizeModal.jsx +++ b/superset/assets/javascripts/SqlLab/components/VisualizeModal.jsx @@ -49,7 +49,7 @@ class VisualizeModal extends React.PureComponent { this.setStateFromProps(nextProps); } setStateFromProps(props) { - if ( + if (!props || !props.query || !props.query.results || !props.query.results.columns) { @@ -118,16 +118,17 @@ class VisualizeModal extends React.PureComponent { } return columns; } - visualize() { - const vizOptions = { + buildVizOptions() { + return { chartType: this.state.chartType.value, datasourceName: this.state.datasourceName, columns: this.state.columns, sql: this.props.query.sql, dbId: this.props.query.dbId, }; - - this.props.actions.createDatasource(vizOptions, this) + } + visualize() { + this.props.actions.createDatasource(this.buildVizOptions(), this) .done(() => { const columns = Object.keys(this.state.columns).map(k => this.state.columns[k]); const mainMetric = columns.filter(d => d.agg)[0]; diff --git a/superset/assets/spec/javascripts/explorev2/actions_spec.js b/superset/assets/spec/javascripts/explorev2/actions_spec.js index 8eced723e6a0..9bfbffce113d 100644 --- a/superset/assets/spec/javascripts/explorev2/actions_spec.js +++ b/superset/assets/spec/javascripts/explorev2/actions_spec.js @@ -23,7 +23,7 @@ describe('reducers', () => { describe('runQuery', () => { it('should handle query timeout', () => { const dispatch = sinon.spy(); - const urlStub = sinon.stub(exploreUtils, 'getExploreUrl', () => ('mockURL')); + const urlStub = sinon.stub(exploreUtils, 'getExploreUrl').callsFake(() => ('mockURL')); const ajaxStub = sinon.stub($, 'ajax'); ajaxStub.yieldsTo('error', { statusText: 'timeout' }); diff --git a/superset/assets/spec/javascripts/sqllab/VisualizeModal_spec.jsx b/superset/assets/spec/javascripts/sqllab/VisualizeModal_spec.jsx index 75b9929b5dcc..01b3ef2cc995 100644 --- a/superset/assets/spec/javascripts/sqllab/VisualizeModal_spec.jsx +++ b/superset/assets/spec/javascripts/sqllab/VisualizeModal_spec.jsx @@ -9,8 +9,10 @@ import { expect } from 'chai'; import sinon from 'sinon'; import $ from 'jquery'; +import shortid from 'shortid'; import { queries } from './fixtures'; import { sqlLabReducer } from '../../../javascripts/SqlLab/reducers'; +import * as actions from '../../../javascripts/SqlLab/actions'; import VisualizeModal from '../../../javascripts/SqlLab/components/VisualizeModal'; import * as exploreUtils from '../../../javascripts/explorev2/exploreUtils'; @@ -47,7 +49,16 @@ describe('VisualizeModal', () => { requiresTime: false, value: 'dist_bar', }; - + const mockChartTypeTB = { + label: 'Time Series - Bar Chart', + requiresTime: true, + value: 'bar', + }; + const mockEvent = { + target: { + value: 'mock event value', + }, + }; const getVisualizeModalWrapper = () => ( shallow(, { context: { store }, @@ -66,47 +77,230 @@ describe('VisualizeModal', () => { expect(wrapper.find(Modal)).to.have.length(1); }); - describe('visualize', () => { + describe('setStateFromProps', () => { const wrapper = getVisualizeModalWrapper(); + const sampleQuery = queries[0]; - wrapper.setState({ - chartType: mockChartTypeBarChart, - columns: mockColumns, - datasourceName: 'mockDatasourceName', + it('should have valid query prop', () => { + const spy = sinon.spy(wrapper.instance(), 'setState'); + wrapper.instance().setStateFromProps(); + expect(spy.callCount).to.equal(0); + spy.restore(); + }); + it('should set columns state', () => { + wrapper.instance().setStateFromProps({ query: sampleQuery }); + expect(wrapper.state().columns).to.deep.equal(mockColumns); + }); + }); + + describe('datasourceName', () => { + const wrapper = getVisualizeModalWrapper(); + let stub; + beforeEach(() => { + stub = sinon.stub(shortid, 'generate').returns('abcd'); + }); + afterEach(() => { + stub.restore(); + }); + + it('should generate data source name from query', () => { + const sampleQuery = queries[0]; + const name = wrapper.instance().datasourceName(); + expect(name).to.equal(`${sampleQuery.user}-${sampleQuery.db}-${sampleQuery.tab}-abcd`); + }); + it('should generate data source name with empty query', () => { + wrapper.setProps({ query: {} }); + const name = wrapper.instance().datasourceName(); + expect(name).to.equal('undefined-abcd'); + }); + }); + + describe('mergedColumns', () => { + const wrapper = getVisualizeModalWrapper(); + const oldColumns = { + ds: 1, + gender: 2, + }; + + it('should merge by column name', () => { + wrapper.setState({ columns: {} }); + const mc = wrapper.instance().mergedColumns(); + expect(mc).to.deep.equal(mockColumns); + }); + it('should not override current state', () => { + wrapper.setState({ columns: oldColumns }); + + const mc = wrapper.instance().mergedColumns(); + expect(mc.ds).to.equal(oldColumns.ds); + expect(mc.gender).to.equal(oldColumns.gender); + }); + }); + + describe('validate', () => { + const wrapper = getVisualizeModalWrapper(); + let columnsStub; + beforeEach(() => { + columnsStub = sinon.stub(wrapper.instance(), 'mergedColumns'); + }); + afterEach(() => { + columnsStub.restore(); + }); + + it('should validate column name', () => { + columnsStub.returns(mockColumns); + 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(); }); + it('should hint empty chartType', () => { + columnsStub.returns(mockColumns); + wrapper.setState({ chartType: null }); + wrapper.instance().validate(); + expect(wrapper.state().hints).to.have.length(1); + }); + it('should check time series', () => { + columnsStub.returns(mockColumns); + wrapper.setState({ chartType: mockChartTypeTB }); + wrapper.instance().validate(); + expect(wrapper.state().hints).to.have.length(0); + + // no is_date columns + columnsStub.returns({ + ds: { + is_date: false, + is_dim: false, + name: 'ds', + type: 'STRING', + }, + gender: { + is_date: false, + is_dim: true, + name: 'gender', + type: 'STRING', + }, + }); + wrapper.setState({ chartType: mockChartTypeTB }); + wrapper.instance().validate(); + expect(wrapper.state().hints).to.have.length(1); + }); + it('should validate after change checkbox', () => { + const spy = sinon.spy(wrapper.instance(), 'validate'); + columnsStub.returns(mockColumns); + + wrapper.instance().changeCheckbox('is_dim', 'gender', mockEvent); + expect(spy.callCount).to.equal(1); + spy.restore(); + }); + it('should validate after change Agg function', () => { + const spy = sinon.spy(wrapper.instance(), 'validate'); + columnsStub.returns(mockColumns); + + wrapper.instance().changeAggFunction('num', { label: 'MIN(x)', value: 'min' }); + expect(spy.callCount).to.equal(1); + spy.restore(); + }); + }); + + it('should validate after change chart type', () => { + const wrapper = getVisualizeModalWrapper(); + wrapper.setState({ chartType: mockChartTypeTB }); + const spy = sinon.spy(wrapper.instance(), 'validate'); + + wrapper.instance().changeChartType(mockChartTypeBarChart); + expect(spy.callCount).to.equal(1); + expect(wrapper.state().chartType).to.equal(mockChartTypeBarChart); + }); + + it('should validate after change datasource name', () => { + const wrapper = getVisualizeModalWrapper(); + const spy = sinon.spy(wrapper.instance(), 'validate'); + + wrapper.instance().changeDatasourceName(mockEvent); + expect(spy.callCount).to.equal(1); + expect(wrapper.state().datasourceName).to.equal(mockEvent.target.value); + }); - const vizOptions = { + it('should build viz options', () => { + const wrapper = getVisualizeModalWrapper(); + wrapper.setState({ chartType: mockChartTypeTB }); + const spy = sinon.spy(wrapper.instance(), 'buildVizOptions'); + wrapper.instance().buildVizOptions(); + expect(spy.returnValues[0]).to.deep.equal({ chartType: wrapper.state().chartType.value, datasourceName: wrapper.state().datasourceName, columns: wrapper.state().columns, sql: wrapper.instance().props.query.sql, dbId: wrapper.instance().props.query.dbId, - }; + }); + }); - let spy; - let server; + describe('visualize', () => { + const wrapper = getVisualizeModalWrapper(); + const mockOptions = { attr: 'mockOptions' }; + wrapper.setState({ + chartType: mockChartTypeBarChart, + columns: mockColumns, + datasourceName: 'mockDatasourceName', + }); + let ajaxSpy; beforeEach(() => { - spy = sinon.spy($, 'ajax'); - server = sinon.fakeServer.create(); + ajaxSpy = sinon.spy($, 'ajax'); sinon.stub(JSON, 'parse').callsFake(() => ({ table_id: 107 })); sinon.stub(exploreUtils, 'getExploreUrl').callsFake(() => ('mockURL')); + sinon.stub(wrapper.instance(), 'buildVizOptions').callsFake(() => (mockOptions)); + sinon.spy(window, 'open'); }); afterEach(() => { - spy.restore(); - server.restore(); + ajaxSpy.restore(); JSON.parse.restore(); exploreUtils.getExploreUrl.restore(); + wrapper.instance().buildVizOptions.restore(); + window.open.restore(); }); it('should build request', () => { wrapper.instance().visualize(); - expect(spy.callCount).to.equal(1); + expect(ajaxSpy.callCount).to.equal(1); - const spyCall = spy.getCall(0); + const spyCall = ajaxSpy.getCall(0); expect(spyCall.args[0].type).to.equal('POST'); expect(spyCall.args[0].url).to.equal('/superset/sqllab_viz/'); - expect(spyCall.args[0].data.data).to.equal(JSON.stringify(vizOptions)); + expect(spyCall.args[0].data.data).to.equal(JSON.stringify(mockOptions)); + }); + it('should open new window', () => { + const datasourceSpy = sinon.stub(actions, 'createDatasource').callsFake(() => { + const d = $.Deferred(); + d.resolve('done'); + return d.promise(); + }); + wrapper.setProps({ actions: { createDatasource: datasourceSpy } }); + + wrapper.instance().visualize(); + expect(window.open.callCount).to.equal(1); + datasourceSpy.restore(); + }); + it('should notify error', () => { + const datasourceSpy = sinon.stub(actions, 'createDatasource').callsFake(() => { + const d = $.Deferred(); + d.reject('error message'); + return d.promise(); + }); + wrapper.setProps({ actions: { createDatasource: datasourceSpy } }); + sinon.spy(notify, 'error'); + + wrapper.instance().visualize(); + expect(window.open.callCount).to.equal(0); + expect(notify.error.callCount).to.equal(1); + datasourceSpy.restore(); }); }); }); diff --git a/superset/assets/spec/javascripts/sqllab/fixtures.js b/superset/assets/spec/javascripts/sqllab/fixtures.js index 3e3d9c38fc07..db638ba3c18f 100644 --- a/superset/assets/spec/javascripts/sqllab/fixtures.js +++ b/superset/assets/spec/javascripts/sqllab/fixtures.js @@ -205,7 +205,17 @@ export const queries = [ serverId: 141, resultsKey: null, results: { - columns: ['col1', 'col2'], + columns: [{ + is_date: true, + is_dim: false, + name: 'ds', + type: 'STRING', + }, { + is_date: false, + is_dim: true, + name: 'gender', + type: 'STRING', + }], data: [ { col1: 0, col2: 1 }, { col1: 2, col2: 3 }, From 132e402376ff20cf464fbc34526190afe9f594e8 Mon Sep 17 00:00:00 2001 From: Grace Guo Date: Wed, 24 May 2017 14:29:16 -0700 Subject: [PATCH 4/9] add unit tests for VisualizeModal component --- .../SqlLab/components/VisualizeModal.jsx | 11 +- .../javascripts/explorev2/actions_spec.js | 2 +- .../sqllab/VisualizeModal_spec.jsx | 228 ++++++++++++++++-- .../spec/javascripts/sqllab/fixtures.js | 12 +- 4 files changed, 229 insertions(+), 24 deletions(-) diff --git a/superset/assets/javascripts/SqlLab/components/VisualizeModal.jsx b/superset/assets/javascripts/SqlLab/components/VisualizeModal.jsx index 821baa99ae07..fb22c40a157a 100644 --- a/superset/assets/javascripts/SqlLab/components/VisualizeModal.jsx +++ b/superset/assets/javascripts/SqlLab/components/VisualizeModal.jsx @@ -49,7 +49,7 @@ class VisualizeModal extends React.PureComponent { this.setStateFromProps(nextProps); } setStateFromProps(props) { - if ( + if (!props || !props.query || !props.query.results || !props.query.results.columns) { @@ -118,16 +118,17 @@ class VisualizeModal extends React.PureComponent { } return columns; } - visualize() { - const vizOptions = { + buildVizOptions() { + return { chartType: this.state.chartType.value, datasourceName: this.state.datasourceName, columns: this.state.columns, sql: this.props.query.sql, dbId: this.props.query.dbId, }; - - this.props.actions.createDatasource(vizOptions, this) + } + visualize() { + this.props.actions.createDatasource(this.buildVizOptions(), this) .done(() => { const columns = Object.keys(this.state.columns).map(k => this.state.columns[k]); const mainMetric = columns.filter(d => d.agg)[0]; diff --git a/superset/assets/spec/javascripts/explorev2/actions_spec.js b/superset/assets/spec/javascripts/explorev2/actions_spec.js index 8eced723e6a0..9bfbffce113d 100644 --- a/superset/assets/spec/javascripts/explorev2/actions_spec.js +++ b/superset/assets/spec/javascripts/explorev2/actions_spec.js @@ -23,7 +23,7 @@ describe('reducers', () => { describe('runQuery', () => { it('should handle query timeout', () => { const dispatch = sinon.spy(); - const urlStub = sinon.stub(exploreUtils, 'getExploreUrl', () => ('mockURL')); + const urlStub = sinon.stub(exploreUtils, 'getExploreUrl').callsFake(() => ('mockURL')); const ajaxStub = sinon.stub($, 'ajax'); ajaxStub.yieldsTo('error', { statusText: 'timeout' }); diff --git a/superset/assets/spec/javascripts/sqllab/VisualizeModal_spec.jsx b/superset/assets/spec/javascripts/sqllab/VisualizeModal_spec.jsx index 75b9929b5dcc..01b3ef2cc995 100644 --- a/superset/assets/spec/javascripts/sqllab/VisualizeModal_spec.jsx +++ b/superset/assets/spec/javascripts/sqllab/VisualizeModal_spec.jsx @@ -9,8 +9,10 @@ import { expect } from 'chai'; import sinon from 'sinon'; import $ from 'jquery'; +import shortid from 'shortid'; import { queries } from './fixtures'; import { sqlLabReducer } from '../../../javascripts/SqlLab/reducers'; +import * as actions from '../../../javascripts/SqlLab/actions'; import VisualizeModal from '../../../javascripts/SqlLab/components/VisualizeModal'; import * as exploreUtils from '../../../javascripts/explorev2/exploreUtils'; @@ -47,7 +49,16 @@ describe('VisualizeModal', () => { requiresTime: false, value: 'dist_bar', }; - + const mockChartTypeTB = { + label: 'Time Series - Bar Chart', + requiresTime: true, + value: 'bar', + }; + const mockEvent = { + target: { + value: 'mock event value', + }, + }; const getVisualizeModalWrapper = () => ( shallow(, { context: { store }, @@ -66,47 +77,230 @@ describe('VisualizeModal', () => { expect(wrapper.find(Modal)).to.have.length(1); }); - describe('visualize', () => { + describe('setStateFromProps', () => { const wrapper = getVisualizeModalWrapper(); + const sampleQuery = queries[0]; - wrapper.setState({ - chartType: mockChartTypeBarChart, - columns: mockColumns, - datasourceName: 'mockDatasourceName', + it('should have valid query prop', () => { + const spy = sinon.spy(wrapper.instance(), 'setState'); + wrapper.instance().setStateFromProps(); + expect(spy.callCount).to.equal(0); + spy.restore(); + }); + it('should set columns state', () => { + wrapper.instance().setStateFromProps({ query: sampleQuery }); + expect(wrapper.state().columns).to.deep.equal(mockColumns); + }); + }); + + describe('datasourceName', () => { + const wrapper = getVisualizeModalWrapper(); + let stub; + beforeEach(() => { + stub = sinon.stub(shortid, 'generate').returns('abcd'); + }); + afterEach(() => { + stub.restore(); + }); + + it('should generate data source name from query', () => { + const sampleQuery = queries[0]; + const name = wrapper.instance().datasourceName(); + expect(name).to.equal(`${sampleQuery.user}-${sampleQuery.db}-${sampleQuery.tab}-abcd`); + }); + it('should generate data source name with empty query', () => { + wrapper.setProps({ query: {} }); + const name = wrapper.instance().datasourceName(); + expect(name).to.equal('undefined-abcd'); + }); + }); + + describe('mergedColumns', () => { + const wrapper = getVisualizeModalWrapper(); + const oldColumns = { + ds: 1, + gender: 2, + }; + + it('should merge by column name', () => { + wrapper.setState({ columns: {} }); + const mc = wrapper.instance().mergedColumns(); + expect(mc).to.deep.equal(mockColumns); + }); + it('should not override current state', () => { + wrapper.setState({ columns: oldColumns }); + + const mc = wrapper.instance().mergedColumns(); + expect(mc.ds).to.equal(oldColumns.ds); + expect(mc.gender).to.equal(oldColumns.gender); + }); + }); + + describe('validate', () => { + const wrapper = getVisualizeModalWrapper(); + let columnsStub; + beforeEach(() => { + columnsStub = sinon.stub(wrapper.instance(), 'mergedColumns'); + }); + afterEach(() => { + columnsStub.restore(); + }); + + it('should validate column name', () => { + columnsStub.returns(mockColumns); + 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(); }); + it('should hint empty chartType', () => { + columnsStub.returns(mockColumns); + wrapper.setState({ chartType: null }); + wrapper.instance().validate(); + expect(wrapper.state().hints).to.have.length(1); + }); + it('should check time series', () => { + columnsStub.returns(mockColumns); + wrapper.setState({ chartType: mockChartTypeTB }); + wrapper.instance().validate(); + expect(wrapper.state().hints).to.have.length(0); + + // no is_date columns + columnsStub.returns({ + ds: { + is_date: false, + is_dim: false, + name: 'ds', + type: 'STRING', + }, + gender: { + is_date: false, + is_dim: true, + name: 'gender', + type: 'STRING', + }, + }); + wrapper.setState({ chartType: mockChartTypeTB }); + wrapper.instance().validate(); + expect(wrapper.state().hints).to.have.length(1); + }); + it('should validate after change checkbox', () => { + const spy = sinon.spy(wrapper.instance(), 'validate'); + columnsStub.returns(mockColumns); + + wrapper.instance().changeCheckbox('is_dim', 'gender', mockEvent); + expect(spy.callCount).to.equal(1); + spy.restore(); + }); + it('should validate after change Agg function', () => { + const spy = sinon.spy(wrapper.instance(), 'validate'); + columnsStub.returns(mockColumns); + + wrapper.instance().changeAggFunction('num', { label: 'MIN(x)', value: 'min' }); + expect(spy.callCount).to.equal(1); + spy.restore(); + }); + }); + + it('should validate after change chart type', () => { + const wrapper = getVisualizeModalWrapper(); + wrapper.setState({ chartType: mockChartTypeTB }); + const spy = sinon.spy(wrapper.instance(), 'validate'); + + wrapper.instance().changeChartType(mockChartTypeBarChart); + expect(spy.callCount).to.equal(1); + expect(wrapper.state().chartType).to.equal(mockChartTypeBarChart); + }); + + it('should validate after change datasource name', () => { + const wrapper = getVisualizeModalWrapper(); + const spy = sinon.spy(wrapper.instance(), 'validate'); + + wrapper.instance().changeDatasourceName(mockEvent); + expect(spy.callCount).to.equal(1); + expect(wrapper.state().datasourceName).to.equal(mockEvent.target.value); + }); - const vizOptions = { + it('should build viz options', () => { + const wrapper = getVisualizeModalWrapper(); + wrapper.setState({ chartType: mockChartTypeTB }); + const spy = sinon.spy(wrapper.instance(), 'buildVizOptions'); + wrapper.instance().buildVizOptions(); + expect(spy.returnValues[0]).to.deep.equal({ chartType: wrapper.state().chartType.value, datasourceName: wrapper.state().datasourceName, columns: wrapper.state().columns, sql: wrapper.instance().props.query.sql, dbId: wrapper.instance().props.query.dbId, - }; + }); + }); - let spy; - let server; + describe('visualize', () => { + const wrapper = getVisualizeModalWrapper(); + const mockOptions = { attr: 'mockOptions' }; + wrapper.setState({ + chartType: mockChartTypeBarChart, + columns: mockColumns, + datasourceName: 'mockDatasourceName', + }); + let ajaxSpy; beforeEach(() => { - spy = sinon.spy($, 'ajax'); - server = sinon.fakeServer.create(); + ajaxSpy = sinon.spy($, 'ajax'); sinon.stub(JSON, 'parse').callsFake(() => ({ table_id: 107 })); sinon.stub(exploreUtils, 'getExploreUrl').callsFake(() => ('mockURL')); + sinon.stub(wrapper.instance(), 'buildVizOptions').callsFake(() => (mockOptions)); + sinon.spy(window, 'open'); }); afterEach(() => { - spy.restore(); - server.restore(); + ajaxSpy.restore(); JSON.parse.restore(); exploreUtils.getExploreUrl.restore(); + wrapper.instance().buildVizOptions.restore(); + window.open.restore(); }); it('should build request', () => { wrapper.instance().visualize(); - expect(spy.callCount).to.equal(1); + expect(ajaxSpy.callCount).to.equal(1); - const spyCall = spy.getCall(0); + const spyCall = ajaxSpy.getCall(0); expect(spyCall.args[0].type).to.equal('POST'); expect(spyCall.args[0].url).to.equal('/superset/sqllab_viz/'); - expect(spyCall.args[0].data.data).to.equal(JSON.stringify(vizOptions)); + expect(spyCall.args[0].data.data).to.equal(JSON.stringify(mockOptions)); + }); + it('should open new window', () => { + const datasourceSpy = sinon.stub(actions, 'createDatasource').callsFake(() => { + const d = $.Deferred(); + d.resolve('done'); + return d.promise(); + }); + wrapper.setProps({ actions: { createDatasource: datasourceSpy } }); + + wrapper.instance().visualize(); + expect(window.open.callCount).to.equal(1); + datasourceSpy.restore(); + }); + it('should notify error', () => { + const datasourceSpy = sinon.stub(actions, 'createDatasource').callsFake(() => { + const d = $.Deferred(); + d.reject('error message'); + return d.promise(); + }); + wrapper.setProps({ actions: { createDatasource: datasourceSpy } }); + sinon.spy(notify, 'error'); + + wrapper.instance().visualize(); + expect(window.open.callCount).to.equal(0); + expect(notify.error.callCount).to.equal(1); + datasourceSpy.restore(); }); }); }); diff --git a/superset/assets/spec/javascripts/sqllab/fixtures.js b/superset/assets/spec/javascripts/sqllab/fixtures.js index 3e3d9c38fc07..db638ba3c18f 100644 --- a/superset/assets/spec/javascripts/sqllab/fixtures.js +++ b/superset/assets/spec/javascripts/sqllab/fixtures.js @@ -205,7 +205,17 @@ export const queries = [ serverId: 141, resultsKey: null, results: { - columns: ['col1', 'col2'], + columns: [{ + is_date: true, + is_dim: false, + name: 'ds', + type: 'STRING', + }, { + is_date: false, + is_dim: true, + name: 'gender', + type: 'STRING', + }], data: [ { col1: 0, col2: 1 }, { col1: 2, col2: 3 }, From 8a4f1896469cd1ea8357cf25c13440a9885f4b6f Mon Sep 17 00:00:00 2001 From: Grace Guo Date: Wed, 24 May 2017 17:22:25 -0700 Subject: [PATCH 5/9] remove changes on css file (merge by mistake) --- superset/assets/stylesheets/superset.css | 8 -------- 1 file changed, 8 deletions(-) diff --git a/superset/assets/stylesheets/superset.css b/superset/assets/stylesheets/superset.css index 4dafda4221d6..e29fa4930fe4 100644 --- a/superset/assets/stylesheets/superset.css +++ b/superset/assets/stylesheets/superset.css @@ -201,11 +201,3 @@ div.widget .slice_container { margin-top: 8px; margin-bottom: 0px } - -.table-condensed { - font-size: 12px; -} - -.table-condensed input[type="checkbox"] { - float: left; -} \ No newline at end of file From 532845d19d27c4e5371f1ddf67e77a7d14fcf9f5 Mon Sep 17 00:00:00 2001 From: Grace Guo Date: Wed, 24 May 2017 17:22:25 -0700 Subject: [PATCH 6/9] remove changes on css file (merge by mistake) --- superset/assets/stylesheets/superset.css | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/superset/assets/stylesheets/superset.css b/superset/assets/stylesheets/superset.css index e29fa4930fe4..b87fa6422c3c 100644 --- a/superset/assets/stylesheets/superset.css +++ b/superset/assets/stylesheets/superset.css @@ -201,3 +201,8 @@ div.widget .slice_container { margin-top: 8px; margin-bottom: 0px } + +.table-condensed { + font-size: 12px; +} + From 26441156a0f28eeec7a3b339bf92fdd336f0e915 Mon Sep 17 00:00:00 2001 From: Grace Guo Date: Wed, 24 May 2017 18:20:22 -0700 Subject: [PATCH 7/9] update unit test description setStateFromProps required valid props as parameter, otherwise it should not change state. --- superset/assets/spec/javascripts/sqllab/VisualizeModal_spec.jsx | 2 +- superset/assets/stylesheets/superset.css | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/superset/assets/spec/javascripts/sqllab/VisualizeModal_spec.jsx b/superset/assets/spec/javascripts/sqllab/VisualizeModal_spec.jsx index 01b3ef2cc995..be241638e699 100644 --- a/superset/assets/spec/javascripts/sqllab/VisualizeModal_spec.jsx +++ b/superset/assets/spec/javascripts/sqllab/VisualizeModal_spec.jsx @@ -81,7 +81,7 @@ describe('VisualizeModal', () => { const wrapper = getVisualizeModalWrapper(); const sampleQuery = queries[0]; - it('should have valid query prop', () => { + it('should require valid props parameters', () => { const spy = sinon.spy(wrapper.instance(), 'setState'); wrapper.instance().setStateFromProps(); expect(spy.callCount).to.equal(0); diff --git a/superset/assets/stylesheets/superset.css b/superset/assets/stylesheets/superset.css index b87fa6422c3c..61a199040b37 100644 --- a/superset/assets/stylesheets/superset.css +++ b/superset/assets/stylesheets/superset.css @@ -205,4 +205,3 @@ div.widget .slice_container { .table-condensed { font-size: 12px; } - From bfe2497c368813c6fa9f2190163ed82d635d788b Mon Sep 17 00:00:00 2001 From: Grace Guo Date: Sun, 28 May 2017 23:25:27 -0700 Subject: [PATCH 8/9] fix per code review comments --- .../javascripts/SqlLab/components/VisualizeModal.jsx | 7 +++---- superset/assets/javascripts/SqlLab/constants.js | 7 +++++++ .../spec/javascripts/sqllab/VisualizeModal_spec.jsx | 11 +++++++---- 3 files changed, 17 insertions(+), 8 deletions(-) diff --git a/superset/assets/javascripts/SqlLab/components/VisualizeModal.jsx b/superset/assets/javascripts/SqlLab/components/VisualizeModal.jsx index fb22c40a157a..e3025129e7a0 100644 --- a/superset/assets/javascripts/SqlLab/components/VisualizeModal.jsx +++ b/superset/assets/javascripts/SqlLab/components/VisualizeModal.jsx @@ -10,6 +10,7 @@ import { Table } from 'reactable'; import shortid from 'shortid'; import { getExploreUrl } from '../../explorev2/exploreUtils'; import * as actions from '../actions'; +import { VALIDATION_ERRORS } from '../constants'; const CHART_TYPES = [ { value: 'dist_bar', label: 'Distribution - Bar Chart', requiresTime: false }, @@ -87,7 +88,7 @@ class VisualizeModal extends React.PureComponent { } }); if (this.state.chartType === null) { - hints.push('Pick a chart type!'); + hints.push(VALIDATION_ERRORS.REQUIRE_CHART_TYPE); } else if (this.state.chartType.requiresTime) { let hasTime = false; for (const colName in cols) { @@ -97,9 +98,7 @@ class VisualizeModal extends React.PureComponent { } } if (!hasTime) { - hints.push( - 'To use this chart type you need at least one column ' + - 'flagged as a date'); + hints.push(VALIDATION_ERRORS.REQUIRE_TIME); } } this.setState({ hints }); diff --git a/superset/assets/javascripts/SqlLab/constants.js b/superset/assets/javascripts/SqlLab/constants.js index 37e32f708701..1aeb130c94cb 100644 --- a/superset/assets/javascripts/SqlLab/constants.js +++ b/superset/assets/javascripts/SqlLab/constants.js @@ -22,3 +22,10 @@ export const TIME_OPTIONS = [ '90 days ago', '1 year ago', ]; + +export const VALIDATION_ERRORS = { + REQUIRE_CHART_TYPE: 'Pick a chart type!', + REQUIRE_TIME: 'To use this chart type you need at least one column flagged as a date', + REQUIRE_DIMENSION: 'To use this chart type you need at least one dimension', + REQUIRE_AGGREGATION_FUNCTION: 'To use this chart type you need at least one aggregation function', +}; diff --git a/superset/assets/spec/javascripts/sqllab/VisualizeModal_spec.jsx b/superset/assets/spec/javascripts/sqllab/VisualizeModal_spec.jsx index be241638e699..4ce7cf685fa6 100644 --- a/superset/assets/spec/javascripts/sqllab/VisualizeModal_spec.jsx +++ b/superset/assets/spec/javascripts/sqllab/VisualizeModal_spec.jsx @@ -13,6 +13,7 @@ import shortid from 'shortid'; import { queries } from './fixtures'; import { sqlLabReducer } from '../../../javascripts/SqlLab/reducers'; import * as actions from '../../../javascripts/SqlLab/actions'; +import { VALIDATION_ERRORS } from '../../../javascripts/SqlLab/constants'; import VisualizeModal from '../../../javascripts/SqlLab/components/VisualizeModal'; import * as exploreUtils from '../../../javascripts/explorev2/exploreUtils'; @@ -165,6 +166,7 @@ describe('VisualizeModal', () => { wrapper.setState({ chartType: null }); wrapper.instance().validate(); expect(wrapper.state().hints).to.have.length(1); + expect(wrapper.state().hints[0]).to.have.string(VALIDATION_ERRORS.REQUIRE_CHART_TYPE); }); it('should check time series', () => { columnsStub.returns(mockColumns); @@ -252,12 +254,14 @@ describe('VisualizeModal', () => { }); let ajaxSpy; + let datasourceSpy; beforeEach(() => { ajaxSpy = sinon.spy($, 'ajax'); sinon.stub(JSON, 'parse').callsFake(() => ({ table_id: 107 })); sinon.stub(exploreUtils, 'getExploreUrl').callsFake(() => ('mockURL')); sinon.stub(wrapper.instance(), 'buildVizOptions').callsFake(() => (mockOptions)); sinon.spy(window, 'open'); + datasourceSpy = sinon.stub(actions, 'createDatasource'); }); afterEach(() => { ajaxSpy.restore(); @@ -265,6 +269,7 @@ describe('VisualizeModal', () => { exploreUtils.getExploreUrl.restore(); wrapper.instance().buildVizOptions.restore(); window.open.restore(); + datasourceSpy.restore(); }); it('should build request', () => { @@ -277,7 +282,7 @@ describe('VisualizeModal', () => { expect(spyCall.args[0].data.data).to.equal(JSON.stringify(mockOptions)); }); it('should open new window', () => { - const datasourceSpy = sinon.stub(actions, 'createDatasource').callsFake(() => { + datasourceSpy.callsFake(() => { const d = $.Deferred(); d.resolve('done'); return d.promise(); @@ -286,10 +291,9 @@ describe('VisualizeModal', () => { wrapper.instance().visualize(); expect(window.open.callCount).to.equal(1); - datasourceSpy.restore(); }); it('should notify error', () => { - const datasourceSpy = sinon.stub(actions, 'createDatasource').callsFake(() => { + datasourceSpy.callsFake(() => { const d = $.Deferred(); d.reject('error message'); return d.promise(); @@ -300,7 +304,6 @@ describe('VisualizeModal', () => { wrapper.instance().visualize(); expect(window.open.callCount).to.equal(0); expect(notify.error.callCount).to.equal(1); - datasourceSpy.restore(); }); }); }); From 9483f3689193b349a0e0311673d362773c8c1fa0 Mon Sep 17 00:00:00 2001 From: Grace Guo Date: Sun, 28 May 2017 23:25:27 -0700 Subject: [PATCH 9/9] fix per code review comments --- .../assets/javascripts/SqlLab/components/VisualizeModal.jsx | 6 +++--- superset/assets/javascripts/SqlLab/constants.js | 2 +- .../assets/spec/javascripts/sqllab/VisualizeModal_spec.jsx | 6 ++++-- 3 files changed, 8 insertions(+), 6 deletions(-) diff --git a/superset/assets/javascripts/SqlLab/components/VisualizeModal.jsx b/superset/assets/javascripts/SqlLab/components/VisualizeModal.jsx index e3025129e7a0..639e68ab1eb5 100644 --- a/superset/assets/javascripts/SqlLab/components/VisualizeModal.jsx +++ b/superset/assets/javascripts/SqlLab/components/VisualizeModal.jsx @@ -10,7 +10,7 @@ import { Table } from 'reactable'; import shortid from 'shortid'; import { getExploreUrl } from '../../explorev2/exploreUtils'; import * as actions from '../actions'; -import { VALIDATION_ERRORS } from '../constants'; +import { VISUALIZE_VALIDATION_ERRORS } from '../constants'; const CHART_TYPES = [ { value: 'dist_bar', label: 'Distribution - Bar Chart', requiresTime: false }, @@ -88,7 +88,7 @@ class VisualizeModal extends React.PureComponent { } }); if (this.state.chartType === null) { - hints.push(VALIDATION_ERRORS.REQUIRE_CHART_TYPE); + hints.push(VISUALIZE_VALIDATION_ERRORS.REQUIRE_CHART_TYPE); } else if (this.state.chartType.requiresTime) { let hasTime = false; for (const colName in cols) { @@ -98,7 +98,7 @@ class VisualizeModal extends React.PureComponent { } } if (!hasTime) { - hints.push(VALIDATION_ERRORS.REQUIRE_TIME); + hints.push(VISUALIZE_VALIDATION_ERRORS.REQUIRE_TIME); } } this.setState({ hints }); diff --git a/superset/assets/javascripts/SqlLab/constants.js b/superset/assets/javascripts/SqlLab/constants.js index 1aeb130c94cb..2a9327509e3f 100644 --- a/superset/assets/javascripts/SqlLab/constants.js +++ b/superset/assets/javascripts/SqlLab/constants.js @@ -23,7 +23,7 @@ export const TIME_OPTIONS = [ '1 year ago', ]; -export const VALIDATION_ERRORS = { +export const VISUALIZE_VALIDATION_ERRORS = { REQUIRE_CHART_TYPE: 'Pick a chart type!', REQUIRE_TIME: 'To use this chart type you need at least one column flagged as a date', REQUIRE_DIMENSION: 'To use this chart type you need at least one dimension', diff --git a/superset/assets/spec/javascripts/sqllab/VisualizeModal_spec.jsx b/superset/assets/spec/javascripts/sqllab/VisualizeModal_spec.jsx index 4ce7cf685fa6..787c63d2a916 100644 --- a/superset/assets/spec/javascripts/sqllab/VisualizeModal_spec.jsx +++ b/superset/assets/spec/javascripts/sqllab/VisualizeModal_spec.jsx @@ -13,7 +13,7 @@ import shortid from 'shortid'; import { queries } from './fixtures'; import { sqlLabReducer } from '../../../javascripts/SqlLab/reducers'; import * as actions from '../../../javascripts/SqlLab/actions'; -import { VALIDATION_ERRORS } from '../../../javascripts/SqlLab/constants'; +import { VISUALIZE_VALIDATION_ERRORS } from '../../../javascripts/SqlLab/constants'; import VisualizeModal from '../../../javascripts/SqlLab/components/VisualizeModal'; import * as exploreUtils from '../../../javascripts/explorev2/exploreUtils'; @@ -166,7 +166,8 @@ describe('VisualizeModal', () => { wrapper.setState({ chartType: null }); wrapper.instance().validate(); expect(wrapper.state().hints).to.have.length(1); - expect(wrapper.state().hints[0]).to.have.string(VALIDATION_ERRORS.REQUIRE_CHART_TYPE); + expect(wrapper.state().hints[0]) + .to.have.string(VISUALIZE_VALIDATION_ERRORS.REQUIRE_CHART_TYPE); }); it('should check time series', () => { columnsStub.returns(mockColumns); @@ -192,6 +193,7 @@ describe('VisualizeModal', () => { wrapper.setState({ chartType: mockChartTypeTB }); 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 validate after change checkbox', () => { const spy = sinon.spy(wrapper.instance(), 'validate');