From b4db76952318b4c4284b5014b2c792f3144dec84 Mon Sep 17 00:00:00 2001 From: Jeff Niu Date: Thu, 12 Oct 2017 20:01:40 -0700 Subject: [PATCH 1/4] Added altered tag to explore slice view and fixes #3616 --- .../components/AlteredSliceTag.jsx | 100 ++++++++++++++++++ .../explore/components/ChartContainer.jsx | 28 +++++ .../components/ExploreViewContainer.jsx | 5 + superset/utils.py | 31 +++++- 4 files changed, 161 insertions(+), 3 deletions(-) create mode 100644 superset/assets/javascripts/components/AlteredSliceTag.jsx diff --git a/superset/assets/javascripts/components/AlteredSliceTag.jsx b/superset/assets/javascripts/components/AlteredSliceTag.jsx new file mode 100644 index 0000000000000..db64857c602a1 --- /dev/null +++ b/superset/assets/javascripts/components/AlteredSliceTag.jsx @@ -0,0 +1,100 @@ +import React from 'react'; +import PropTypes from 'prop-types'; +import { Table, Tr, Td, Thead, Th } from 'reactable'; +import TooltipWrapper from './TooltipWrapper'; +import { controls } from '../explore/stores/controls'; +import Button from './Button'; +import ModalTrigger from './ModalTrigger'; +import { t } from '../locales'; + +export default class AlteredSliceTag extends React.Component { + + formatValue(value, key) { + 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 (value.constructor === Object) { + return JSON.stringify(value); + } else { + return value; + } + } + + renderRows() { + const altered = this.props.altered; + const rows = []; + for (const key in altered) { + rows.push( + + + {this.formatValue(altered[key].before, key)} + {this.formatValue(altered[key].after, key)} + + ); + } + return rows; + } + + renderModalBody() { + const alteredRows = this.renderRows(); + return ( + + + + + + + {this.renderRows()} +
ControlBeforeAfter
+ ); + } + + renderTriggerNode() { + return ( + + + {t('Altered')} + + + ); + } + + render() { + return ( + + ); + } +} + +AlteredSliceTag.propTypes = { + altered: PropTypes.object.isRequired, +} diff --git a/superset/assets/javascripts/explore/components/ChartContainer.jsx b/superset/assets/javascripts/explore/components/ChartContainer.jsx index f3c660adf66bc..c3526f1d74e4b 100644 --- a/superset/assets/javascripts/explore/components/ChartContainer.jsx +++ b/superset/assets/javascripts/explore/components/ChartContainer.jsx @@ -8,6 +8,7 @@ import visMap from '../../../visualizations/main'; import { d3format } from '../../modules/utils'; 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'; @@ -74,6 +75,25 @@ class ChartContainer extends React.PureComponent { } } + isAltered() { + // Returns all properties that differ in the + // current form data and the base form data + const fd = this.props.formData || {}; + const bfd = (this.props.slice && this.props.slice.form_data) || {}; + const fdKeys = new Set(Object.keys(fd).concat(Object.keys(bfd))); + const differing = {}; + for (const fdKey of fdKeys) { + // Ignore values that are undefined/nonexisting in either + if (!fd[fdKey] && !bfd[fdKey]) { + continue; + } + if (JSON.stringify(fd[fdKey]) !== JSON.stringify(bfd[fdKey])) { + differing[fdKey] = { before: bfd[fdKey], after: fd[fdKey] }; + } + } + return differing; + } + getMockedSliceObject() { const props = this.props; const getHeight = () => { @@ -225,6 +245,12 @@ class ChartContainer extends React.PureComponent { ); } + renderAlteredTag() { + const altered = this.isAltered(); + return Object.keys(altered).length ? + : null; + } + renderChart() { if (this.props.alert) { return this.renderAlert(); @@ -296,6 +322,8 @@ class ChartContainer extends React.PureComponent { } + {this.renderAlteredTag()} +
{this.props.chartStatus === 'success' && this.props.queryResponse && diff --git a/superset/assets/javascripts/explore/components/ExploreViewContainer.jsx b/superset/assets/javascripts/explore/components/ExploreViewContainer.jsx index f696ed6445e0f..e3c6893a9f375 100644 --- a/superset/assets/javascripts/explore/components/ExploreViewContainer.jsx +++ b/superset/assets/javascripts/explore/components/ExploreViewContainer.jsx @@ -21,6 +21,7 @@ const propTypes = { controls: PropTypes.object.isRequired, forcedHeight: PropTypes.string, form_data: PropTypes.object.isRequired, + baseFormData: PropTypes.object, standalone: PropTypes.bool.isRequired, triggerQuery: PropTypes.bool.isRequired, queryRequest: PropTypes.object, @@ -28,6 +29,7 @@ const propTypes = { }; class ExploreViewContainer extends React.Component { + constructor(props) { super(props); this.state = { @@ -105,11 +107,13 @@ class ExploreViewContainer extends React.Component { toggleModal() { this.setState({ showModal: !this.state.showModal }); } + hasErrors() { const ctrls = this.props.controls; return Object.keys(ctrls).some( k => ctrls[k].validationErrors && ctrls[k].validationErrors.length > 0); } + renderErrorMessage() { // Returns an error message as a node if any errors are in the store const errors = []; @@ -132,6 +136,7 @@ class ExploreViewContainer extends React.Component { } return errorMessage; } + renderChartContainer() { return ( Date: Fri, 13 Oct 2017 00:14:53 -0700 Subject: [PATCH 2/4] unit tests --- .../components/AlteredSliceTag.jsx | 18 +- .../explore/components/ChartContainer.jsx | 38 ++-- .../components/ExploreViewContainer.jsx | 5 - .../components/AlteredSliceTag_spec.jsx | 180 ++++++++++++++++++ superset/utils.py | 9 +- tests/utils_tests.py | 94 ++++++++- 6 files changed, 307 insertions(+), 37 deletions(-) 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 index db64857c602a1..cc8bab2a230ac 100644 --- a/superset/assets/javascripts/components/AlteredSliceTag.jsx +++ b/superset/assets/javascripts/components/AlteredSliceTag.jsx @@ -3,13 +3,14 @@ import PropTypes from 'prop-types'; import { Table, Tr, Td, Thead, Th } from 'reactable'; import TooltipWrapper from './TooltipWrapper'; import { controls } from '../explore/stores/controls'; -import Button from './Button'; import ModalTrigger from './ModalTrigger'; import { t } from '../locales'; export default class AlteredSliceTag extends React.Component { 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) { @@ -23,7 +24,7 @@ export default class AlteredSliceTag extends React.Component { return `${v.col} ${v.op} ${filterVal}`; }).join(', '); } else if (controls[key] && controls[key].type === 'BoundsControl') { - return `Min: ${value[0]}, Max; ${value[1]}`; + 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') { @@ -32,9 +33,8 @@ export default class AlteredSliceTag extends React.Component { return value.length ? value.join(', ') : '[]'; } else if (value.constructor === Object) { return JSON.stringify(value); - } else { - return value; } + return value; } renderRows() { @@ -46,14 +46,13 @@ export default class AlteredSliceTag extends React.Component { {this.formatValue(altered[key].before, key)} {this.formatValue(altered[key].after, key)} - + , ); } return rows; } renderModalBody() { - const alteredRows = this.renderRows(); return ( @@ -83,9 +82,12 @@ export default class AlteredSliceTag extends React.Component { } render() { + // 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 ( { @@ -165,6 +146,25 @@ class ChartContainer extends React.PureComponent { }; } + isAltered() { + // Returns all properties that differ in the + // current form data and the base form data + const fd = this.props.formData || {}; + const bfd = (this.props.slice && this.props.slice.form_data) || {}; + const fdKeys = new Set(Object.keys(fd).concat(Object.keys(bfd))); + const differing = {}; + for (const fdKey of fdKeys) { + // Ignore values that are undefined/nonexisting in either + if (!fd[fdKey] && !bfd[fdKey]) { + continue; + } + if (JSON.stringify(fd[fdKey]) !== JSON.stringify(bfd[fdKey])) { + differing[fdKey] = { before: bfd[fdKey], after: fd[fdKey] }; + } + } + return differing; + } + removeAlert() { this.props.actions.removeChartAlert(); } diff --git a/superset/assets/javascripts/explore/components/ExploreViewContainer.jsx b/superset/assets/javascripts/explore/components/ExploreViewContainer.jsx index e3c6893a9f375..f696ed6445e0f 100644 --- a/superset/assets/javascripts/explore/components/ExploreViewContainer.jsx +++ b/superset/assets/javascripts/explore/components/ExploreViewContainer.jsx @@ -21,7 +21,6 @@ const propTypes = { controls: PropTypes.object.isRequired, forcedHeight: PropTypes.string, form_data: PropTypes.object.isRequired, - baseFormData: PropTypes.object, standalone: PropTypes.bool.isRequired, triggerQuery: PropTypes.bool.isRequired, queryRequest: PropTypes.object, @@ -29,7 +28,6 @@ const propTypes = { }; class ExploreViewContainer extends React.Component { - constructor(props) { super(props); this.state = { @@ -107,13 +105,11 @@ class ExploreViewContainer extends React.Component { toggleModal() { this.setState({ showModal: !this.state.showModal }); } - hasErrors() { const ctrls = this.props.controls; return Object.keys(ctrls).some( k => ctrls[k].validationErrors && ctrls[k].validationErrors.length > 0); } - renderErrorMessage() { // Returns an error message as a node if any errors are in the store const errors = []; @@ -136,7 +132,6 @@ class ExploreViewContainer extends React.Component { } return errorMessage; } - renderChartContainer() { return ( { + let wrapper; + let props; + + beforeEach(() => { + props = Object.assign({}, defaultProps); + wrapper = shallow(); + }); + + 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(8); + }); + + it('renders the correct number of Td', () => { + const modalBody = shallow(
{wrapper.instance().renderModalBody()}
); + const td = modalBody.find(Td); + expect(td).to.have.lengthOf(24); + ['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(8); + 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 7ef572b653c05..2a68d02bbce5f 100644 --- a/superset/utils.py +++ b/superset/utils.py @@ -687,9 +687,12 @@ def merge_extra_filters(form_data): '__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[existing['col'] + '__' + existing['op']] = existing['val'] + existing_filters[get_filter_key(existing)] = existing['val'] for filtr in form_data['extra_filters']: # Pull out time filters/options and merge into form data if date_options.get(filtr['col']): @@ -697,11 +700,13 @@ def merge_extra_filters(form_data): form_data[date_options[filtr['col']]] = filtr['val'] elif filtr['val'] and len(filtr['val']): # Merge column filters - filter_key = filtr['col'] + '__' + filtr['op'] + 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']) diff --git a/tests/utils_tests.py b/tests/utils_tests.py index b642c4374a75c..3a9add30e2028 100644 --- a/tests/utils_tests.py +++ b/tests/utils_tests.py @@ -117,14 +117,102 @@ 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)), - '1990-09-21T19:11:19.626096') + self.assertEquals( + datetime_f(datetime(1990, 9, 21, 19, 11, 19, 626096)), + '1990-09-21T19:11:19.626096' + ) self.assertEquals(len(datetime_f(datetime.now())), 28) 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]} From 2e83588da6af7d149838d90746758a30c6859747 Mon Sep 17 00:00:00 2001 From: Jeff Niu Date: Tue, 17 Oct 2017 10:45:53 -0700 Subject: [PATCH 3/4] Moved getDiffs logic into AlteredSliceTag --- .../components/AlteredSliceTag.jsx | 63 +++++++-- .../explore/components/ChartContainer.jsx | 26 +--- superset/assets/package.json | 1 + .../components/AlteredSliceTag_spec.jsx | 127 +++++++++++++----- 4 files changed, 149 insertions(+), 68 deletions(-) diff --git a/superset/assets/javascripts/components/AlteredSliceTag.jsx b/superset/assets/javascripts/components/AlteredSliceTag.jsx index cc8bab2a230ac..eb24424e8d41f 100644 --- a/superset/assets/javascripts/components/AlteredSliceTag.jsx +++ b/superset/assets/javascripts/components/AlteredSliceTag.jsx @@ -1,13 +1,54 @@ 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 @@ -31,21 +72,21 @@ export default class AlteredSliceTag extends React.Component { return value ? 'true' : 'false'; } else if (value.constructor === Array) { return value.length ? value.join(', ') : '[]'; - } else if (value.constructor === Object) { - return JSON.stringify(value); + } else if (typeof value === 'string' || typeof value === 'number') { + return value; } - return value; + return JSON.stringify(value); } renderRows() { - const altered = this.props.altered; + const diffs = this.state.diffs; const rows = []; - for (const key in altered) { + for (const key in diffs) { rows.push( - + + , ); } @@ -82,6 +123,10 @@ export default class AlteredSliceTag extends React.Component { } 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 @@ -97,6 +142,4 @@ export default class AlteredSliceTag extends React.Component { } } -AlteredSliceTag.propTypes = { - altered: PropTypes.object.isRequired, -}; +AlteredSliceTag.propTypes = propTypes; diff --git a/superset/assets/javascripts/explore/components/ChartContainer.jsx b/superset/assets/javascripts/explore/components/ChartContainer.jsx index a7eec2e99538a..f74a263af7781 100644 --- a/superset/assets/javascripts/explore/components/ChartContainer.jsx +++ b/superset/assets/javascripts/explore/components/ChartContainer.jsx @@ -146,25 +146,6 @@ class ChartContainer extends React.PureComponent { }; } - isAltered() { - // Returns all properties that differ in the - // current form data and the base form data - const fd = this.props.formData || {}; - const bfd = (this.props.slice && this.props.slice.form_data) || {}; - const fdKeys = new Set(Object.keys(fd).concat(Object.keys(bfd))); - const differing = {}; - for (const fdKey of fdKeys) { - // Ignore values that are undefined/nonexisting in either - if (!fd[fdKey] && !bfd[fdKey]) { - continue; - } - if (JSON.stringify(fd[fdKey]) !== JSON.stringify(bfd[fdKey])) { - differing[fdKey] = { before: bfd[fdKey], after: fd[fdKey] }; - } - } - return differing; - } - removeAlert() { this.props.actions.removeChartAlert(); } @@ -246,9 +227,10 @@ class ChartContainer extends React.PureComponent { } renderAlteredTag() { - const altered = this.isAltered(); - return Object.keys(altered).length ? - : null; + const origFormData = (this.props.slice && this.props.slice.form_data) || {}; + const currentFormData = this.props.formData; + const tagProps = { origFormData, currentFormData }; + return (); } renderChart() { diff --git a/superset/assets/package.json b/superset/assets/package.json index 3dfdb78240f9b..952d494445e6d 100644 --- a/superset/assets/package.json +++ b/superset/assets/package.json @@ -93,6 +93,7 @@ "sprintf-js": "^1.1.1", "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 index d9aeaa25f74f3..f3b746bbb361c 100644 --- a/superset/assets/spec/javascripts/components/AlteredSliceTag_spec.jsx +++ b/superset/assets/spec/javascripts/components/AlteredSliceTag_spec.jsx @@ -10,39 +10,56 @@ import ModalTrigger from '../../../javascripts/components/ModalTrigger'; import TooltipWrapper from '../../../javascripts/components/TooltipWrapper'; const defaultProps = { - altered: { - 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, - }, - alpha: { - before: undefined, - after: null, - }, - 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' }, - }, + 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' }, }, }; @@ -55,6 +72,44 @@ describe('AlteredSliceTag', () => { 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); }); @@ -89,13 +144,13 @@ describe('AlteredSliceTag', () => { it('renders the correct number of Tr', () => { const modalBody = shallow(
{wrapper.instance().renderModalBody()}
); const tr = modalBody.find(Tr); - expect(tr).to.have.lengthOf(8); + 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(24); + expect(td).to.have.lengthOf(21); ['control', 'before', 'after'].forEach((v, i) => { expect(td.get(i).props.column).to.equal(v); }); @@ -105,7 +160,7 @@ describe('AlteredSliceTag', () => { describe('renderRows', () => { it('returns an array of rows with one Tr and three Td', () => { const rows = wrapper.instance().renderRows(); - expect(rows).to.have.lengthOf(8); + 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); From c7a3c496298f745e50417ad2e8898eef5a70749a Mon Sep 17 00:00:00 2001 From: Jeff Niu Date: Thu, 9 Nov 2017 23:13:14 -0800 Subject: [PATCH 4/4] code style fixs --- tests/utils_tests.py | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/tests/utils_tests.py b/tests/utils_tests.py index 9ce16a13479c0..98aa085839573 100644 --- a/tests/utils_tests.py +++ b/tests/utils_tests.py @@ -112,7 +112,7 @@ def test_merge_extra_filters(self): def test_merge_extra_filters_ignores_empty_filters(self): form_data = {'extra_filters': [ {'col': 'a', 'op': 'in', 'val': ''}, - {'col': 'B', 'op': '==', 'val': []} + {'col': 'B', 'op': '==', 'val': []}, ]} expected = {'filters': []} merge_extra_filters(form_data) @@ -122,16 +122,16 @@ 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']} + {'col': 'B', 'op': '==', 'val': ['c1', 'c2']}, ], 'filters': [ {'col': 'a', 'op': 'in', 'val': 'someval'}, - {'col': 'B', 'op': '==', 'val': ['c1', 'c2']} + {'col': 'B', 'op': '==', 'val': ['c1', 'c2']}, ], } expected = {'filters': [ {'col': 'a', 'op': 'in', 'val': 'someval'}, - {'col': 'B', 'op': '==', 'val': ['c1', 'c2']} + {'col': 'B', 'op': '==', 'val': ['c1', 'c2']}, ]} merge_extra_filters(form_data) self.assertEquals(form_data, expected) @@ -195,15 +195,14 @@ def test_merge_extra_filters_adds_unequal_lists(self): def test_datetime_f(self): self.assertEquals( datetime_f(datetime(1990, 9, 21, 19, 11, 19, 626096)), - '1990-09-21T19:11:19.626096' + '1990-09-21T19:11:19.626096', ) self.assertEquals(len(datetime_f(datetime.now())), 28) 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' + datetime_f(datetime(a, b, c)), '00:00:00', ) def test_json_encoded_obj(self):
- {this.formatValue(altered[key].before, key)}{this.formatValue(altered[key].after, key)}{this.formatValue(diffs[key].before, key)}{this.formatValue(diffs[key].after, key)}