Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Explore] Altered Slice Tag #3668

Merged
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
102 changes: 102 additions & 0 deletions superset/assets/javascripts/components/AlteredSliceTag.jsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,102 @@
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 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) {
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);
}
return value;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking the catchall could be a JSON.stringify though I'm not 100 sure of the implications. There's a guarantee that controls' values are always serializable since they make it through the redux store.

}

renderRows() {
const altered = this.props.altered;
const rows = [];
for (const key in altered) {
rows.push(
<Tr key={key}>
<Td column="control" data={(controls[key] && controls[key].label) || key} />
<Td column="before">{this.formatValue(altered[key].before, key)}</Td>
<Td column="after">{this.formatValue(altered[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() {
// 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 = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please put propTypes at the top of the file as in most other modules

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also what about if the component received origFormData and currentFormData instead, and the logic to check the differences can live in this component here.

altered: PropTypes.object.isRequired,
};
28 changes: 28 additions & 0 deletions superset/assets/javascripts/explore/components/ChartContainer.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -145,6 +146,25 @@ class ChartContainer extends React.PureComponent {
};
}

isAltered() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about using a deep equality function like http://underscorejs.org/#isEqual instead?

// 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) || {};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here you may want to use the currentFormData as the driver of which keys should exist. slice.form_data may have unused / deprecated keys that don't affect how the slice is rendered...

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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd expect a isSomething method to return a bool by naming conventions, rename the function to something like getDiffs.

}

removeAlert() {
this.props.actions.removeChartAlert();
}
Expand Down Expand Up @@ -225,6 +245,12 @@ class ChartContainer extends React.PureComponent {
</div>);
}

renderAlteredTag() {
const altered = this.isAltered();
return Object.keys(altered).length ?
<AlteredSliceTag altered={altered} /> : null;
}

renderChart() {
if (this.props.alert) {
return this.renderAlert();
Expand Down Expand Up @@ -296,6 +322,8 @@ class ChartContainer extends React.PureComponent {
</span>
}

{this.renderAlteredTag()}

<div className="pull-right">
{this.props.chartStatus === 'success' &&
this.props.queryResponse &&
Expand Down
180 changes: 180 additions & 0 deletions superset/assets/spec/javascripts/components/AlteredSliceTag_spec.jsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,180 @@
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 = {
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' },
},
},
};

describe('AlteredSliceTag', () => {
let wrapper;
let props;

beforeEach(() => {
props = Object.assign({}, defaultProps);
wrapper = shallow(<AlteredSliceTag {...props} />);
});

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(8);
});

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(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(<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);
});
});
});
36 changes: 33 additions & 3 deletions superset/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -686,10 +686,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']
Loading