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: [Explore] Altered Slice Tag (#3668)
Date Sat, 11 Nov 2017 05:33:35 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 4d48d5d  [Explore] Altered Slice Tag (#3668)
4d48d5d is described below

commit 4d48d5d854e23d469c80105a6e384e3c4db8942d
Author: Jeff Niu <jeffniu22@gmail.com>
AuthorDate: Fri Nov 10 21:33:31 2017 -0800

    [Explore] Altered Slice Tag (#3668)
    
    * Added altered tag to explore slice view and fixes #3616
    
    * unit tests
    
    * Moved getDiffs logic into AlteredSliceTag
    
    * code style fixs
---
 .../javascripts/components/AlteredSliceTag.jsx     | 145 +++++++++++++
 .../explore/components/ExploreChartHeader.jsx      |  10 +
 superset/assets/package.json                       |   1 +
 .../components/AlteredSliceTag_spec.jsx            | 235 +++++++++++++++++++++
 superset/utils.py                                  |  36 +++-
 tests/utils_tests.py                               |  87 +++++++-
 6 files changed, 510 insertions(+), 4 deletions(-)

diff --git a/superset/assets/javascripts/components/AlteredSliceTag.jsx b/superset/assets/javascripts/components/AlteredSliceTag.jsx
new file mode 100644
index 0000000..eb24424
--- /dev/null
+++ b/superset/assets/javascripts/components/AlteredSliceTag.jsx
@@ -0,0 +1,145 @@
+import React from 'react';
+import PropTypes from 'prop-types';
+import { Table, Tr, Td, Thead, Th } from 'reactable';
+import { isEqual, isEmpty } from 'underscore';
+
+import TooltipWrapper from './TooltipWrapper';
+import { controls } from '../explore/stores/controls';
+import ModalTrigger from './ModalTrigger';
+import { t } from '../locales';
+
+const propTypes = {
+  origFormData: PropTypes.object.isRequired,
+  currentFormData: PropTypes.object.isRequired,
+};
+
+export default class AlteredSliceTag extends React.Component {
+
+  constructor(props) {
+    super(props);
+    const diffs = this.getDiffs(props);
+    this.state = { diffs, hasDiffs: !isEmpty(diffs) };
+  }
+
+  componentWillReceiveProps(newProps) {
+    // Update differences if need be
+    if (isEqual(this.props, newProps)) {
+      return;
+    }
+    const diffs = this.getDiffs(newProps);
+    this.setState({ diffs, hasDiffs: !isEmpty(diffs) });
+  }
+
+  getDiffs(props) {
+    // Returns all properties that differ in the
+    // current form data and the saved form data
+    const ofd = props.origFormData;
+    const cfd = props.currentFormData;
+    const fdKeys = Object.keys(cfd);
+    const diffs = {};
+    for (const fdKey of fdKeys) {
+      // Ignore values that are undefined/nonexisting in either
+      if (!ofd[fdKey] && !cfd[fdKey]) {
+        continue;
+      }
+      if (!isEqual(ofd[fdKey], cfd[fdKey])) {
+        diffs[fdKey] = { before: ofd[fdKey], after: cfd[fdKey] };
+      }
+    }
+    return diffs;
+  }
+
+  formatValue(value, key) {
+    // Format display value based on the control type
+    // or the value type
+    if (value === undefined) {
+      return 'N/A';
+    } else if (value === null) {
+      return 'null';
+    } else if (controls[key] && controls[key].type === 'FilterControl') {
+      if (!value.length) {
+        return '[]';
+      }
+      return value.map((v) => {
+        const filterVal = v.val.constructor === Array ? `[${v.val.join(', ')}]` : v.val;
+        return `${v.col} ${v.op} ${filterVal}`;
+      }).join(', ');
+    } else if (controls[key] && controls[key].type === 'BoundsControl') {
+      return `Min: ${value[0]}, Max: ${value[1]}`;
+    } else if (controls[key] && controls[key].type === 'CollectionControl') {
+      return value.map(v => JSON.stringify(v)).join(', ');
+    } else if (typeof value === 'boolean') {
+      return value ? 'true' : 'false';
+    } else if (value.constructor === Array) {
+      return value.length ? value.join(', ') : '[]';
+    } else if (typeof value === 'string' || typeof value === 'number') {
+      return value;
+    }
+    return JSON.stringify(value);
+  }
+
+  renderRows() {
+    const diffs = this.state.diffs;
+    const rows = [];
+    for (const key in diffs) {
+      rows.push(
+        <Tr key={key}>
+          <Td column="control" data={(controls[key] && controls[key].label) ||
key} />
+          <Td column="before">{this.formatValue(diffs[key].before, key)}</Td>
+          <Td column="after">{this.formatValue(diffs[key].after, key)}</Td>
+        </Tr>,
+      );
+    }
+    return rows;
+  }
+
+  renderModalBody() {
+    return (
+      <Table className="table" sortable>
+        <Thead>
+          <Th column="control">Control</Th>
+          <Th column="before">Before</Th>
+          <Th column="after">After</Th>
+        </Thead>
+        {this.renderRows()}
+      </Table>
+    );
+  }
+
+  renderTriggerNode() {
+    return (
+      <TooltipWrapper
+        label="difference"
+        tooltip={t('Click to see difference')}
+      >
+        <span
+          className="label label-warning m-l-5"
+          style={{ fontSize: '12px' }}
+        >
+          {t('Altered')}
+        </span>
+      </TooltipWrapper>
+    );
+  }
+
+  render() {
+    // Return nothing if there are no differences
+    if (!this.state.hasDiffs) {
+      return null;
+    }
+    // Render the label-warning 'Altered' tag which the user may
+    // click to open a modal containing a table summarizing the
+    // differences in the slice
+    return (
+      <ModalTrigger
+        animation
+        triggerNode={this.renderTriggerNode()}
+        modalTitle={t('Slice changes')}
+        bsSize="large"
+        modalBody={this.renderModalBody()}
+      />
+    );
+  }
+}
+
+AlteredSliceTag.propTypes = propTypes;
diff --git a/superset/assets/javascripts/explore/components/ExploreChartHeader.jsx b/superset/assets/javascripts/explore/components/ExploreChartHeader.jsx
index 8c2e1f3..3750fc0 100644
--- a/superset/assets/javascripts/explore/components/ExploreChartHeader.jsx
+++ b/superset/assets/javascripts/explore/components/ExploreChartHeader.jsx
@@ -4,6 +4,7 @@ import PropTypes from 'prop-types';
 import { chartPropType } from '../../chart/chartReducer';
 import ExploreActionButtons from './ExploreActionButtons';
 import EditableTitle from '../../components/EditableTitle';
+import AlteredSliceTag from '../../components/AlteredSliceTag';
 import FaveStar from '../../components/FaveStar';
 import TooltipWrapper from '../../components/TooltipWrapper';
 import Timer from '../../components/Timer';
@@ -54,6 +55,13 @@ class ExploreChartHeader extends React.PureComponent {
       });
   }
 
+  renderAlteredTag() {
+    const origFormData = (this.props.slice && this.props.slice.form_data) || {};
+    const currentFormData = this.props.form_data;
+    const tagProps = { origFormData, currentFormData };
+    return (<AlteredSliceTag {...tagProps} />);
+  }
+
   renderChartTitle() {
     let title;
     if (this.props.slice) {
@@ -106,6 +114,8 @@ class ExploreChartHeader extends React.PureComponent {
         </span>
         }
 
+        {this.renderAlteredTag()}
+
         <div className="pull-right">
           {this.props.chart.chartStatus === 'success' &&
           queryResponse &&
diff --git a/superset/assets/package.json b/superset/assets/package.json
index 43aa268..99f8afc 100644
--- a/superset/assets/package.json
+++ b/superset/assets/package.json
@@ -96,6 +96,7 @@
     "srcdoc-polyfill": "^1.0.0",
     "supercluster": "https://github.com/georgeke/supercluster/tarball/ac3492737e7ce98e07af679623aad452373bbc40",
     "urijs": "^1.18.10",
+    "underscore": "^1.8.3",
     "viewport-mercator-project": "^2.1.0"
   },
   "devDependencies": {
diff --git a/superset/assets/spec/javascripts/components/AlteredSliceTag_spec.jsx b/superset/assets/spec/javascripts/components/AlteredSliceTag_spec.jsx
new file mode 100644
index 0000000..f3b746b
--- /dev/null
+++ b/superset/assets/spec/javascripts/components/AlteredSliceTag_spec.jsx
@@ -0,0 +1,235 @@
+import React from 'react';
+import { shallow } from 'enzyme';
+import { describe, it } from 'mocha';
+import { expect } from 'chai';
+
+import { Table, Thead, Td, Th, Tr } from 'reactable';
+
+import AlteredSliceTag from '../../../javascripts/components/AlteredSliceTag';
+import ModalTrigger from '../../../javascripts/components/ModalTrigger';
+import TooltipWrapper from '../../../javascripts/components/TooltipWrapper';
+
+const defaultProps = {
+  origFormData: {
+    filters: [{ col: 'a', op: '==', val: 'hello' }],
+    y_axis_bounds: [10, 20],
+    column_collection: [{ 1: 'a', b: ['6', 'g'] }],
+    bool: false,
+    alpha: undefined,
+    gucci: [1, 2, 3, 4],
+    never: 5,
+    ever: { a: 'b', c: 'd' },
+  },
+  currentFormData: {
+    filters: [{ col: 'b', op: 'in', val: ['hello', 'my', 'name'] }],
+    y_axis_bounds: [15, 16],
+    column_collection: [{ 1: 'a', b: [9, '15'], t: 'gggg' }],
+    bool: true,
+    alpha: null,
+    gucci: ['a', 'b', 'c', 'd'],
+    never: 10,
+    ever: { x: 'y', z: 'z' },
+  },
+};
+
+const expectedDiffs = {
+  filters: {
+    before: [{ col: 'a', op: '==', val: 'hello' }],
+    after: [{ col: 'b', op: 'in', val: ['hello', 'my', 'name'] }],
+  },
+  y_axis_bounds: {
+    before: [10, 20],
+    after: [15, 16],
+  },
+  column_collection: {
+    before: [{ 1: 'a', b: ['6', 'g'] }],
+    after: [{ 1: 'a', b: [9, '15'], t: 'gggg' }],
+  },
+  bool: {
+    before: false,
+    after: true,
+  },
+  gucci: {
+    before: [1, 2, 3, 4],
+    after: ['a', 'b', 'c', 'd'],
+  },
+  never: {
+    before: 5,
+    after: 10,
+  },
+  ever: {
+    before: { a: 'b', c: 'd' },
+    after: { x: 'y', z: 'z' },
+  },
+};
+
+describe('AlteredSliceTag', () => {
+  let wrapper;
+  let props;
+
+  beforeEach(() => {
+    props = Object.assign({}, defaultProps);
+    wrapper = shallow(<AlteredSliceTag {...props} />);
+  });
+
+  it('correctly determines form data differences', () => {
+    const diffs = wrapper.instance().getDiffs(props);
+    expect(diffs).to.deep.equal(expectedDiffs);
+    expect(wrapper.instance().state.diffs).to.deep.equal(expectedDiffs);
+    expect(wrapper.instance().state.hasDiffs).to.equal(true);
+  });
+
+  it('does not run when there are no differences', () => {
+    props = {
+      origFormData: props.origFormData,
+      currentFormData: props.origFormData,
+    };
+    wrapper = shallow(<AlteredSliceTag {...props} />);
+    expect(wrapper.instance().state.diffs).to.deep.equal({});
+    expect(wrapper.instance().state.hasDiffs).to.equal(false);
+    expect(wrapper.instance().render()).to.equal(null);
+  });
+
+  it('sets new diffs when receiving new props', () => {
+    const newProps = {
+      currentFormData: Object.assign({}, props.currentFormData),
+      origFormData: Object.assign({}, props.origFormData),
+    };
+    newProps.currentFormData.beta = 10;
+    wrapper = shallow(<AlteredSliceTag {...props} />);
+    wrapper.instance().componentWillReceiveProps(newProps);
+    const newDiffs = wrapper.instance().state.diffs;
+    const expectedBeta = { before: undefined, after: 10 };
+    expect(newDiffs.beta).to.deep.equal(expectedBeta);
+  });
+
+  it('does not set new state when props are the same', () => {
+    const currentDiff = wrapper.instance().state.diffs;
+    wrapper.instance().componentWillReceiveProps(props);
+    // Check equal references
+    expect(wrapper.instance().state.diffs).to.equal(currentDiff);
+  });
+
+  it('renders a ModalTrigger', () => {
+    expect(wrapper.find(ModalTrigger)).to.have.lengthOf(1);
+  });
+
+  describe('renderTriggerNode', () => {
+    it('renders a TooltipWrapper', () => {
+      const triggerNode = shallow(<div>{wrapper.instance().renderTriggerNode()}</div>);
+      expect(triggerNode.find(TooltipWrapper)).to.have.lengthOf(1);
+    });
+  });
+
+  describe('renderModalBody', () => {
+    it('renders a Table', () => {
+      const modalBody = shallow(<div>{wrapper.instance().renderModalBody()}</div>);
+      expect(modalBody.find(Table)).to.have.lengthOf(1);
+    });
+
+    it('renders a Thead', () => {
+      const modalBody = shallow(<div>{wrapper.instance().renderModalBody()}</div>);
+      expect(modalBody.find(Thead)).to.have.lengthOf(1);
+    });
+
+    it('renders Th', () => {
+      const modalBody = shallow(<div>{wrapper.instance().renderModalBody()}</div>);
+      const th = modalBody.find(Th);
+      expect(th).to.have.lengthOf(3);
+      ['control', 'before', 'after'].forEach((v, i) => {
+        expect(th.get(i).props.column).to.equal(v);
+      });
+    });
+
+    it('renders the correct number of Tr', () => {
+      const modalBody = shallow(<div>{wrapper.instance().renderModalBody()}</div>);
+      const tr = modalBody.find(Tr);
+      expect(tr).to.have.lengthOf(7);
+    });
+
+    it('renders the correct number of Td', () => {
+      const modalBody = shallow(<div>{wrapper.instance().renderModalBody()}</div>);
+      const td = modalBody.find(Td);
+      expect(td).to.have.lengthOf(21);
+      ['control', 'before', 'after'].forEach((v, i) => {
+        expect(td.get(i).props.column).to.equal(v);
+      });
+    });
+  });
+
+  describe('renderRows', () => {
+    it('returns an array of rows with one Tr and three Td', () => {
+      const rows = wrapper.instance().renderRows();
+      expect(rows).to.have.lengthOf(7);
+      const fakeRow = shallow(<div>{rows[0]}</div>);
+      expect(fakeRow.find(Tr)).to.have.lengthOf(1);
+      expect(fakeRow.find(Td)).to.have.lengthOf(3);
+    });
+  });
+
+  describe('formatValue', () => {
+    it('returns "N/A" for undefined values', () => {
+      expect(wrapper.instance().formatValue(undefined, 'b')).to.equal('N/A');
+    });
+
+    it('returns "null" for null values', () => {
+      expect(wrapper.instance().formatValue(null, 'b')).to.equal('null');
+    });
+
+    it('returns "Max" and "Min" for BoundsControl', () => {
+      expect(wrapper.instance().formatValue([5, 6], 'y_axis_bounds')).to.equal(
+        'Min: 5, Max: 6',
+      );
+    });
+
+    it('returns stringified objects for CollectionControl', () => {
+      const value = [{ 1: 2, alpha: 'bravo' }, { sent: 'imental', w0ke: 5 }];
+      const expected = '{"1":2,"alpha":"bravo"}, {"sent":"imental","w0ke":5}';
+      expect(wrapper.instance().formatValue(value, 'column_collection')).to.equal(expected);
+    });
+
+    it('returns boolean values as string', () => {
+      expect(wrapper.instance().formatValue(true, 'b')).to.equal('true');
+      expect(wrapper.instance().formatValue(false, 'b')).to.equal('false');
+    });
+
+    it('returns Array joined by commas', () => {
+      const value = [5, 6, 7, 8, 'hello', 'goodbye'];
+      const expected = '5, 6, 7, 8, hello, goodbye';
+      expect(wrapper.instance().formatValue(value)).to.equal(expected);
+    });
+
+    it('stringifies objects', () => {
+      const value = { 1: 2, alpha: 'bravo' };
+      const expected = '{"1":2,"alpha":"bravo"}';
+      expect(wrapper.instance().formatValue(value)).to.equal(expected);
+    });
+
+    it('does nothing to strings and numbers', () => {
+      expect(wrapper.instance().formatValue(5)).to.equal(5);
+      expect(wrapper.instance().formatValue('hello')).to.equal('hello');
+    });
+
+    it('returns "[]" for empty filters', () => {
+      expect(wrapper.instance().formatValue([], 'filters')).to.equal('[]');
+    });
+
+    it('correctly formats filters with array values', () => {
+      const filters = [
+        { col: 'a', op: 'in', val: ['1', 'g', '7', 'ho'] },
+        { col: 'b', op: 'not in', val: ['hu', 'ho', 'ha'] },
+      ];
+      const expected = 'a in [1, g, 7, ho], b not in [hu, ho, ha]';
+      expect(wrapper.instance().formatValue(filters, 'filters')).to.equal(expected);
+    });
+
+    it('correctly formats filters with string values', () => {
+      const filters = [
+        { col: 'a', op: '==', val: 'gucci' },
+        { col: 'b', op: 'LIKE', val: 'moshi moshi' },
+      ];
+      const expected = 'a == gucci, b LIKE moshi moshi';
+      expect(wrapper.instance().formatValue(filters, 'filters')).to.equal(expected);
+    });
+  });
+});
diff --git a/superset/utils.py b/superset/utils.py
index 74d8dfb..f3f1a60 100644
--- a/superset/utils.py
+++ b/superset/utils.py
@@ -684,10 +684,40 @@ def merge_extra_filters(form_data):
             '__time_origin': 'druid_time_origin',
             '__granularity': 'granularity',
         }
+        # Grab list of existing filters 'keyed' on the column and operator
+
+        def get_filter_key(f):
+            return f['col'] + '__' + f['op']
+        existing_filters = {}
+        for existing in form_data['filters']:
+            existing_filters[get_filter_key(existing)] = existing['val']
         for filtr in form_data['extra_filters']:
-            if date_options.get(filtr['col']):  # merge date options
+            # Pull out time filters/options and merge into form data
+            if date_options.get(filtr['col']):
                 if filtr.get('val'):
                     form_data[date_options[filtr['col']]] = filtr['val']
-            else:
-                form_data['filters'] += [filtr]  # merge col filters
+            elif filtr['val'] and len(filtr['val']):
+                # Merge column filters
+                filter_key = get_filter_key(filtr)
+                if filter_key in existing_filters:
+                    # Check if the filter already exists
+                    if isinstance(filtr['val'], list):
+                        if isinstance(existing_filters[filter_key], list):
+                            # Add filters for unequal lists
+                            # order doesn't matter
+                            if (
+                                sorted(existing_filters[filter_key]) !=
+                                sorted(filtr['val'])
+                            ):
+                                form_data['filters'] += [filtr]
+                        else:
+                            form_data['filters'] += [filtr]
+                    else:
+                        # Do not add filter if same value already exists
+                        if filtr['val'] != existing_filters[filter_key]:
+                            form_data['filters'] += [filtr]
+                else:
+                    # Filter not found, add it
+                    form_data['filters'] += [filtr]
+        # Remove extra filters from the form data since no longer needed
         del form_data['extra_filters']
diff --git a/tests/utils_tests.py b/tests/utils_tests.py
index 5096f80..dc6b454 100644
--- a/tests/utils_tests.py
+++ b/tests/utils_tests.py
@@ -110,6 +110,89 @@ class UtilsTestCase(unittest.TestCase):
         merge_extra_filters(form_data)
         self.assertEquals(form_data, expected)
 
+    def test_merge_extra_filters_ignores_empty_filters(self):
+        form_data = {'extra_filters': [
+            {'col': 'a', 'op': 'in', 'val': ''},
+            {'col': 'B', 'op': '==', 'val': []},
+        ]}
+        expected = {'filters': []}
+        merge_extra_filters(form_data)
+        self.assertEquals(form_data, expected)
+
+    def test_merge_extra_filters_ignores_equal_filters(self):
+        form_data = {
+            'extra_filters': [
+                {'col': 'a', 'op': 'in', 'val': 'someval'},
+                {'col': 'B', 'op': '==', 'val': ['c1', 'c2']},
+            ],
+            'filters': [
+                {'col': 'a', 'op': 'in', 'val': 'someval'},
+                {'col': 'B', 'op': '==', 'val': ['c1', 'c2']},
+            ],
+        }
+        expected = {'filters': [
+            {'col': 'a', 'op': 'in', 'val': 'someval'},
+            {'col': 'B', 'op': '==', 'val': ['c1', 'c2']},
+        ]}
+        merge_extra_filters(form_data)
+        self.assertEquals(form_data, expected)
+
+    def test_merge_extra_filters_merges_different_val_types(self):
+        form_data = {
+            'extra_filters': [
+                {'col': 'a', 'op': 'in', 'val': ['g1', 'g2']},
+                {'col': 'B', 'op': '==', 'val': ['c1', 'c2']},
+            ],
+            'filters': [
+                {'col': 'a', 'op': 'in', 'val': 'someval'},
+                {'col': 'B', 'op': '==', 'val': ['c1', 'c2']},
+            ],
+        }
+        expected = {'filters': [
+            {'col': 'a', 'op': 'in', 'val': 'someval'},
+            {'col': 'B', 'op': '==', 'val': ['c1', 'c2']},
+            {'col': 'a', 'op': 'in', 'val': ['g1', 'g2']},
+        ]}
+        merge_extra_filters(form_data)
+        self.assertEquals(form_data, expected)
+        form_data = {
+            'extra_filters': [
+                {'col': 'a', 'op': 'in', 'val': 'someval'},
+                {'col': 'B', 'op': '==', 'val': ['c1', 'c2']},
+            ],
+            'filters': [
+                {'col': 'a', 'op': 'in', 'val': ['g1', 'g2']},
+                {'col': 'B', 'op': '==', 'val': ['c1', 'c2']},
+            ],
+        }
+        expected = {'filters': [
+            {'col': 'a', 'op': 'in', 'val': ['g1', 'g2']},
+            {'col': 'B', 'op': '==', 'val': ['c1', 'c2']},
+            {'col': 'a', 'op': 'in', 'val': 'someval'},
+        ]}
+        merge_extra_filters(form_data)
+        self.assertEquals(form_data, expected)
+
+    def test_merge_extra_filters_adds_unequal_lists(self):
+        form_data = {
+            'extra_filters': [
+                {'col': 'a', 'op': 'in', 'val': ['g1', 'g2', 'g3']},
+                {'col': 'B', 'op': '==', 'val': ['c1', 'c2', 'c3']},
+            ],
+            'filters': [
+                {'col': 'a', 'op': 'in', 'val': ['g1', 'g2']},
+                {'col': 'B', 'op': '==', 'val': ['c1', 'c2']},
+            ],
+        }
+        expected = {'filters': [
+            {'col': 'a', 'op': 'in', 'val': ['g1', 'g2']},
+            {'col': 'B', 'op': '==', 'val': ['c1', 'c2']},
+            {'col': 'a', 'op': 'in', 'val': ['g1', 'g2', 'g3']},
+            {'col': 'B', 'op': '==', 'val': ['c1', 'c2', 'c3']},
+        ]}
+        merge_extra_filters(form_data)
+        self.assertEquals(form_data, expected)
+
     def test_datetime_f(self):
         self.assertEquals(
             datetime_f(datetime(1990, 9, 21, 19, 11, 19, 626096)),
@@ -119,7 +202,9 @@ class UtilsTestCase(unittest.TestCase):
         self.assertEquals(datetime_f(None), '<nobr>None</nobr>')
         iso = datetime.now().isoformat()[:10].split('-')
         [a, b, c] = [int(v) for v in iso]
-        self.assertEquals(datetime_f(datetime(a, b, c)), '<nobr>00:00:00</nobr>')
+        self.assertEquals(
+            datetime_f(datetime(a, b, c)), '<nobr>00:00:00</nobr>',
+        )
 
     def test_json_encoded_obj(self):
         obj = {'a': 5, 'b': ['a', 'g', 5]}

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

Mime
View raw message