superset-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From maximebeauche...@apache.org
Subject [incubator-superset] branch master updated: Fixed filter removal bug (#3458)
Date Fri, 15 Sep 2017 00:10:43 GMT
This is an automated email from the ASF dual-hosted git repository.

maximebeauchemin pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/incubator-superset.git


The following commit(s) were added to refs/heads/master by this push:
     new dd72048  Fixed filter removal bug (#3458)
dd72048 is described below

commit dd72048320edb9527d276a2e5112c8f594bb7600
Author: Jeff Niu <jeffniu22@gmail.com>
AuthorDate: Thu Sep 14 17:10:38 2017 -0700

    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 ++++----
 .../explore/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 bcb0eb9..ffd114c 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 74065ec..9e51601 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) => (
       <div key={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}
         />
       </div>
     ));
diff --git a/superset/assets/spec/javascripts/explore/components/FilterControl_spec.jsx b/superset/assets/spec/javascripts/explore/components/FilterControl_spec.jsx
index 752932a..562cbc6 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(<FilterControl {...defaultProps} />);
+    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: '',
+      },
+    ]);
   });
 });

-- 
To stop receiving notification emails like this one, please contact
['"commits@superset.apache.org" <commits@superset.apache.org>'].

Mime
View raw message