From dd72048320edb9527d276a2e5112c8f594bb7600 Mon Sep 17 00:00:00 2001 From: Jeff Niu Date: Thu, 14 Sep 2017 17:10:38 -0700 Subject: [PATCH] Fixed filter removal bug (#3458) * Fixed bugs when removing filter, switching operators, and switching columns * Fixed lint errors for code style * Added more unit tests for FilterControl * Code format changes to meet standards --- .../explore/components/controls/Filter.jsx | 66 +++---- .../components/controls/FilterControl.jsx | 68 +++++++ .../explore/components/FilterControl_spec.jsx | 187 +++++++++++++++++- 3 files changed, 277 insertions(+), 44 deletions(-) diff --git a/superset/assets/javascripts/explore/components/controls/Filter.jsx b/superset/assets/javascripts/explore/components/controls/Filter.jsx index bcb0eb924..ffd114ca8 100644 --- a/superset/assets/javascripts/explore/components/controls/Filter.jsx +++ b/superset/assets/javascripts/explore/components/controls/Filter.jsx @@ -4,8 +4,6 @@ import Select from 'react-select'; import { Button, Row, Col } from 'react-bootstrap'; import SelectControl from './SelectControl'; -const $ = window.$ = require('jquery'); - const operatorsArr = [ { val: 'in', type: 'array', useSelect: true, multi: true }, { val: 'not in', type: 'array', useSelect: true, multi: true }, @@ -29,6 +27,8 @@ const propTypes = { filter: PropTypes.object.isRequired, datasource: PropTypes.object, having: PropTypes.bool, + valuesLoading: PropTypes.bool, + valueChoices: PropTypes.array, }; const defaultProps = { @@ -36,61 +36,57 @@ const defaultProps = { removeFilter: () => {}, datasource: null, having: false, + valuesLoading: false, + valueChoices: [], }; export default class Filter extends React.Component { - constructor(props) { - super(props); - this.state = { - valuesLoading: false, - }; - } - componentDidMount() { - this.fetchFilterValues(this.props.filter.col); - } - fetchFilterValues(col) { - const datasource = this.props.datasource; - if (col && this.props.datasource && this.props.datasource.filter_select && !this.props.having) { - this.setState({ valuesLoading: true }); - $.ajax({ - type: 'GET', - url: `/superset/filter/${datasource.type}/${datasource.id}/${col}/`, - success: (data) => { - this.setState({ valuesLoading: false, valueChoices: data }); - }, - }); - } - } + switchFilterValue(prevOp, nextOp) { if (operators[prevOp].type !== operators[nextOp].type) { - const val = this.props.filter.value; + // Switch from array to string or vice versa + const val = this.props.filter.val; let newVal; - // switch from array to string - if (operators[nextOp].type === 'string' && val && val.length > 0) { - newVal = val[0]; - } else if (operators[nextOp].type === 'string' && val) { - newVal = [val]; + if (operators[nextOp].type === 'string') { + if (!val || !val.length) { + newVal = ''; + } else { + newVal = val[0]; + } + } else if (operators[nextOp].type === 'array') { + if (!val || !val.length) { + newVal = []; + } else { + newVal = [val]; + } } - this.props.changeFilter('val', newVal); + this.props.changeFilter(['val', 'op'], [newVal, nextOp]); + } else { + // No value type change + this.props.changeFilter('op', nextOp); } } + changeText(event) { this.props.changeFilter('val', event.target.value); } + changeSelect(value) { this.props.changeFilter('val', value); } + changeColumn(event) { this.props.changeFilter('col', event.value); - this.fetchFilterValues(event.value); } + changeOp(event) { this.switchFilterValue(this.props.filter.op, event.value); - this.props.changeFilter('op', event.value); } + removeFilter(filter) { this.props.removeFilter(filter); } + renderFilterFormControl(filter) { const operator = operators[filter.op]; if (operator.useSelect && !this.props.having) { @@ -101,8 +97,8 @@ export default class Filter extends React.Component { freeForm name="filter-value" value={filter.val} - isLoading={this.state.valuesLoading} - choices={this.state.valueChoices} + isLoading={this.props.valuesLoading} + choices={this.props.valueChoices} onChange={this.changeSelect.bind(this)} showHeader={false} /> diff --git a/superset/assets/javascripts/explore/components/controls/FilterControl.jsx b/superset/assets/javascripts/explore/components/controls/FilterControl.jsx index 74065ecc6..9e516011b 100644 --- a/superset/assets/javascripts/explore/components/controls/FilterControl.jsx +++ b/superset/assets/javascripts/explore/components/controls/FilterControl.jsx @@ -3,6 +3,8 @@ import PropTypes from 'prop-types'; import { Button, Row, Col } from 'react-bootstrap'; import Filter from './Filter'; +const $ = window.$ = require('jquery'); + const propTypes = { name: PropTypes.string, onChange: PropTypes.func, @@ -16,6 +18,46 @@ const defaultProps = { }; export default class FilterControl extends React.Component { + + constructor(props) { + super(props); + const initialFilters = props.value.map(() => ({ + valuesLoading: false, + valueChoices: [], + })); + this.state = { + filters: initialFilters, + }; + } + + componentDidMount() { + this.state.filters.forEach((filter, index) => this.fetchFilterValues(index)); + } + + fetchFilterValues(index, column) { + const datasource = this.props.datasource; + const col = column || this.props.value[index].col; + const having = this.props.name === 'having_filters'; + if (col && this.props.datasource && this.props.datasource.filter_select && !having) { + this.setState((prevState) => { + const newStateFilters = Object.assign([], prevState.filters); + newStateFilters[index].valuesLoading = true; + return { filters: newStateFilters }; + }); + $.ajax({ + type: 'GET', + url: `/superset/filter/${datasource.type}/${datasource.id}/${col}/`, + success: (data) => { + this.setState((prevState) => { + const newStateFilters = Object.assign([], prevState.filters); + newStateFilters[index] = { valuesLoading: false, valueChoices: data }; + return { filters: newStateFilters }; + }); + }, + }); + } + } + addFilter() { const newFilters = Object.assign([], this.props.value); const col = this.props.datasource && this.props.datasource.filterable_cols.length > 0 ? @@ -27,7 +69,15 @@ export default class FilterControl extends React.Component { val: this.props.datasource.filter_select ? [] : '', }); this.props.onChange(newFilters); + const nextIndex = this.state.filters.length; + this.setState((prevState) => { + const newStateFilters = Object.assign([], prevState.filters); + newStateFilters.push({ valuesLoading: false, valueChoices: [] }); + return { filters: newStateFilters }; + }); + this.fetchFilterValues(nextIndex, col); } + changeFilter(index, control, value) { const newFilters = Object.assign([], this.props.value); const modifiedFilter = Object.assign({}, newFilters[index]); @@ -38,12 +88,28 @@ export default class FilterControl extends React.Component { modifiedFilter[c] = value[i]; }); } + // Clear selected values and refresh upon column change + if (control === 'col') { + if (modifiedFilter.val.constructor === Array) { + modifiedFilter.val = []; + } else if (typeof modifiedFilter.val === 'string') { + modifiedFilter.val = ''; + } + this.fetchFilterValues(index, value); + } newFilters.splice(index, 1, modifiedFilter); this.props.onChange(newFilters); } + removeFilter(index) { this.props.onChange(this.props.value.filter((f, i) => i !== index)); + this.setState((prevState) => { + const newStateFilters = Object.assign([], prevState.filters); + newStateFilters.splice(index, 1); + return { filters: newStateFilters }; + }); } + render() { const filters = this.props.value.map((filter, i) => (
@@ -53,6 +119,8 @@ export default class FilterControl extends React.Component { datasource={this.props.datasource} removeFilter={this.removeFilter.bind(this, i)} changeFilter={this.changeFilter.bind(this, i)} + valuesLoading={this.state.filters[i].valuesLoading} + valueChoices={this.state.filters[i].valueChoices} />
)); diff --git a/superset/assets/spec/javascripts/explore/components/FilterControl_spec.jsx b/superset/assets/spec/javascripts/explore/components/FilterControl_spec.jsx index 752932ade..562cbc691 100644 --- a/superset/assets/spec/javascripts/explore/components/FilterControl_spec.jsx +++ b/superset/assets/spec/javascripts/explore/components/FilterControl_spec.jsx @@ -8,16 +8,32 @@ import { shallow } from 'enzyme'; import FilterControl from '../../../../javascripts/explore/components/controls/FilterControl'; import Filter from '../../../../javascripts/explore/components/controls/Filter'; +const $ = window.$ = require('jquery'); + const defaultProps = { - choices: ['country_name'], - prefix: 'flt', + name: 'not_having_filters', onChange: sinon.spy(), - value: [], + value: [ + { + col: 'col1', + op: 'in', + val: ['a', 'b', 'd'], + }, + { + col: 'col2', + op: '==', + val: 'Z', + }, + ], datasource: { id: 1, - type: 'table', - filter_select: false, - filterable_cols: ['country_name'], + type: 'qtable', + filter_select: true, + filterable_cols: [['col1', 'col2']], + metrics_combo: [ + ['m1', 'v1'], + ['m2', 'v2'], + ], }, }; @@ -26,6 +42,23 @@ describe('FilterControl', () => { beforeEach(() => { wrapper = shallow(); + wrapper.setState({ + filters: [ + { + valuesLoading: false, + valueChoices: ['a', 'b', 'c', 'd', 'e', 'f'], + }, + { + valuesLoading: false, + valueChoices: ['X', 'Y', 'Z'], + }, + // Need a duplicate since onChange calls are not changing props + { + valuesLoading: false, + valueChoices: ['X', 'Y', 'Z'], + }, + ], + }); }); it('renders Filters', () => { @@ -34,15 +67,151 @@ describe('FilterControl', () => { ).to.equal(true); }); - it('renders one button', () => { - expect(wrapper.find(Filter)).to.have.lengthOf(0); + it('renders one button and two filters', () => { + expect(wrapper.find(Filter)).to.have.lengthOf(2); expect(wrapper.find(Button)).to.have.lengthOf(1); }); - it('add a filter when Add Filter button is clicked', () => { + it('adds filter when clicking Add Filter', () => { const addButton = wrapper.find('#add-button'); expect(addButton).to.have.lengthOf(1); addButton.simulate('click'); expect(defaultProps.onChange).to.have.property('callCount', 1); + expect(defaultProps.onChange.getCall(0).args[0]).to.deep.equal([ + { + col: 'col1', + op: 'in', + val: ['a', 'b', 'd'], + }, + { + col: 'col2', + op: '==', + val: 'Z', + }, + { + col: 'col1', + op: 'in', + val: [], + }, + ]); + }); + + it('removes a the second filter when its delete button is clicked', () => { + expect(wrapper.find(Filter)).to.have.lengthOf(2); + wrapper.instance().removeFilter(1); + expect(defaultProps.onChange).to.have.property('callCount', 2); + expect(defaultProps.onChange.getCall(1).args[0]).to.deep.equal([ + { + col: 'col1', + op: 'in', + val: ['a', 'b', 'd'], + }, + ]); + }); + + after(() => { + $.ajax.restore(); + }); + + it('makes a GET request to retrieve value choices', () => { + sinon.stub($, 'ajax'); + wrapper.instance().fetchFilterValues(0, 'col1'); + expect($.ajax.getCall(0).args[0].type).to.deep.equal('GET'); + expect($.ajax.getCall(0).args[0].url).to.deep.equal('/superset/filter/qtable/1/col1/'); + }); + + it('changes filter values when one is removed', () => { + wrapper.instance().changeFilter(0, 'val', ['a', 'b']); + expect(defaultProps.onChange).to.have.property('callCount', 3); + expect(defaultProps.onChange.getCall(2).args[0]).to.deep.equal([ + { + col: 'col1', + op: 'in', + val: ['a', 'b'], + }, + { + col: 'col2', + op: '==', + val: 'Z', + }, + ]); + }); + + it('changes filter values when one is added', () => { + wrapper.instance().changeFilter(0, 'val', ['a', 'b', 'd', 'e']); + expect(defaultProps.onChange).to.have.property('callCount', 4); + expect(defaultProps.onChange.getCall(3).args[0]).to.deep.equal([ + { + col: 'col1', + op: 'in', + val: ['a', 'b', 'd', 'e'], + }, + { + col: 'col2', + op: '==', + val: 'Z', + }, + ]); + }); + + it('changes op and transforms values', () => { + wrapper.instance().changeFilter(0, ['val', 'op'], ['a', '==']); + wrapper.instance().changeFilter(1, ['val', 'op'], [['Z'], 'in']); + expect(defaultProps.onChange).to.have.property('callCount', 6); + expect(defaultProps.onChange.getCall(4).args[0]).to.deep.equal([ + { + col: 'col1', + op: '==', + val: 'a', + }, + { + col: 'col2', + op: '==', + val: 'Z', + }, + ]); + expect(defaultProps.onChange.getCall(5).args[0]).to.deep.equal([ + { + col: 'col1', + op: 'in', + val: ['a', 'b', 'd'], + }, + { + col: 'col2', + op: 'in', + val: ['Z'], + }, + ]); + }); + + it('changes column and clears invalid values', () => { + wrapper.instance().changeFilter(0, 'col', 'col2'); + expect(defaultProps.onChange).to.have.property('callCount', 7); + expect(defaultProps.onChange.getCall(6).args[0]).to.deep.equal([ + { + col: 'col2', + op: 'in', + val: [], + }, + { + col: 'col2', + op: '==', + val: 'Z', + }, + ]); + wrapper.instance().changeFilter(1, 'col', 'col1'); + expect(defaultProps.onChange).to.have.property('callCount', 8); + expect(defaultProps.onChange.getCall(7).args[0]).to.deep.equal([ + { + col: 'col1', + op: 'in', + val: ['a', 'b', 'd'], + }, + { + col: 'col1', + op: '==', + val: '', + }, + ]); }); });