Skip to content

Commit

Permalink
fix local state 'columns'
Browse files Browse the repository at this point in the history
I found a bug in VisualizeModal:
in setStateFromProps(props), we create columns object and set into state.
But setStateFromProps will be called every componentWillReceiveProps, which means when user close Modal, open Modal etc, it will reset columns state to original props. As a result if user changed any checkbox, then close and re-open modal, his previous change will lost. This thing will become more annoying when mixed with redux updating its state.

I think we should keep 'columns' as a local state, and it should remember states even after user close Modal and reopen it. my fix is just calling setStateFromProps from constructor, not from every componentWillReceiveProps.
  • Loading branch information
Grace Guo committed Jun 3, 2017
1 parent 0d3f57a commit 55e01cf
Show file tree
Hide file tree
Showing 2 changed files with 39 additions and 16 deletions.
12 changes: 5 additions & 7 deletions superset/assets/javascripts/SqlLab/components/VisualizeModal.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -41,28 +41,26 @@ class VisualizeModal extends React.PureComponent {
this.state = {
chartType: CHART_TYPES[0],
datasourceName: this.datasourceName(),
columns: {},
columns: this.setStateFromProps(),
hints: [],
};
}
componentDidMount() {
this.validate();
}
componentWillReceiveProps(nextProps) {
this.setStateFromProps(nextProps);
}
setStateFromProps(props) {
setStateFromProps() {
const props = this.props;
if (!props ||
!props.query ||
!props.query.results ||
!props.query.results.columns) {
return;
return {};
}
const columns = {};
props.query.results.columns.forEach((col) => {
columns[col.name] = col;
});
this.setState({ columns });
return columns;
}
datasourceName() {
const { query } = this.props;
Expand Down
43 changes: 34 additions & 9 deletions superset/assets/spec/javascripts/sqllab/VisualizeModal_spec.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -79,18 +79,43 @@ describe('VisualizeModal', () => {
});

describe('setStateFromProps', () => {
const wrapper = getVisualizeModalWrapper();
const sampleQuery = queries[0];

it('should require valid props parameters', () => {
const spy = sinon.spy(wrapper.instance(), 'setState');
wrapper.instance().setStateFromProps();
expect(spy.callCount).to.equal(0);
spy.restore();
it('should require valid query parameter in props', () => {
const emptyQuery = {
show: true,
query: {},
};
const wrapper = shallow(<VisualizeModal {...emptyQuery} />, {
context: { store },
}).dive();
expect(wrapper.state().columns).to.deep.equal({});
});
it('should set columns state', () => {
wrapper.instance().setStateFromProps({ query: sampleQuery });
const wrapper = getVisualizeModalWrapper();
expect(wrapper.state().columns).to.deep.equal(mockColumns);
});
it('should not change columns state when closing Modal', () => {
const wrapper = getVisualizeModalWrapper();
expect(wrapper.state().columns).to.deep.equal(mockColumns);

// first change columns state
const newColumns = {
ds: {
is_date: true,
is_dim: false,
name: 'ds',
type: 'STRING',
},
name: {
is_date: false,
is_dim: true,
name: 'name',
type: 'STRING',
},
};
wrapper.instance().setState({ columns: newColumns });
// then close Modal
wrapper.setProps({ show: false });
expect(wrapper.state().columns).to.deep.equal(newColumns);
});
});

Expand Down

0 comments on commit 55e01cf

Please sign in to comment.