From 52f541b427c35229a46cccd85b30e77b17339b10 Mon Sep 17 00:00:00 2001 From: Jeff Niu Date: Fri, 10 Nov 2017 21:33:31 -0800 Subject: [PATCH] [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 --- .../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(-) create mode 100644 superset/assets/javascripts/components/AlteredSliceTag.jsx create mode 100644 superset/assets/spec/javascripts/components/AlteredSliceTag_spec.jsx diff --git a/superset/assets/javascripts/components/AlteredSliceTag.jsx b/superset/assets/javascripts/components/AlteredSliceTag.jsx new file mode 100644 index 0000000000000..eb24424e8d41f --- /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( + + + {this.formatValue(diffs[key].before, key)} + {this.formatValue(diffs[key].after, key)} + , + ); + } + return rows; + } + + renderModalBody() { + return ( + + + + + + + {this.renderRows()} +
ControlBeforeAfter
+ ); + } + + renderTriggerNode() { + return ( + + + {t('Altered')} + + + ); + } + + 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 ( + + ); + } +} + +AlteredSliceTag.propTypes = propTypes; diff --git a/superset/assets/javascripts/explore/components/ExploreChartHeader.jsx b/superset/assets/javascripts/explore/components/ExploreChartHeader.jsx index 8c2e1f3ca442f..3750fc029141f 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 (); + } + renderChartTitle() { let title; if (this.props.slice) { @@ -106,6 +114,8 @@ class ExploreChartHeader extends React.PureComponent { } + {this.renderAlteredTag()} +
{this.props.chart.chartStatus === 'success' && queryResponse && diff --git a/superset/assets/package.json b/superset/assets/package.json index 43aa268f288f4..99f8afc16f66a 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 0000000000000..f3b746bbb361c --- /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(); + }); + + 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(); + 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(); + 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(
{wrapper.instance().renderTriggerNode()}
); + expect(triggerNode.find(TooltipWrapper)).to.have.lengthOf(1); + }); + }); + + describe('renderModalBody', () => { + it('renders a Table', () => { + const modalBody = shallow(
{wrapper.instance().renderModalBody()}
); + expect(modalBody.find(Table)).to.have.lengthOf(1); + }); + + it('renders a Thead', () => { + const modalBody = shallow(
{wrapper.instance().renderModalBody()}
); + expect(modalBody.find(Thead)).to.have.lengthOf(1); + }); + + it('renders Th', () => { + const modalBody = shallow(
{wrapper.instance().renderModalBody()}
); + 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(
{wrapper.instance().renderModalBody()}
); + const tr = modalBody.find(Tr); + expect(tr).to.have.lengthOf(7); + }); + + it('renders the correct number of Td', () => { + const modalBody = shallow(
{wrapper.instance().renderModalBody()}
); + 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(
{rows[0]}
); + 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 74d8dfbb63dc2..f3f1a60f6a491 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 5096f80ad3525..dc6b454a36ff9 100644 --- a/tests/utils_tests.py +++ b/tests/utils_tests.py @@ -110,6 +110,89 @@ def test_merge_extra_filters(self): 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 @@ def test_datetime_f(self): self.assertEquals(datetime_f(None), 'None') iso = datetime.now().isoformat()[:10].split('-') [a, b, c] = [int(v) for v in iso] - self.assertEquals(datetime_f(datetime(a, b, c)), '00:00:00') + self.assertEquals( + datetime_f(datetime(a, b, c)), '00:00:00', + ) def test_json_encoded_obj(self): obj = {'a': 5, 'b': ['a', 'g', 5]}