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.

Sign up for GitHub

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

PXD-1514 fix(explorer): use sqon to calculate actual number of selected items #346

Merged
merged 3 commits into from
Aug 17, 2018
Merged

PXD-1514 fix(explorer): use sqon to calculate actual number of selected items #346

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7023f6e
fix(filters): use sqon to calculate actual number of selected items
qingyashu Aug 16, 2018
a86ccb9
fix(explorer): remove similar function, stop returning undefined
qingyashu Aug 17, 2018
37b3033
fix(explorer): add more test cases for rendering charts
qingyashu Aug 17, 2018
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
4 changes: 3 additions & 1 deletion src/DataExplorer/DataExplorerVisualizations.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ class DataExplorerVisualizations extends React.Component {

render() {
const charts = this.props.arrangerData ?
getCharts(this.props.arrangerData, this.props.arrangerConfig)
getCharts(this.props.arrangerData, this.props.arrangerConfig, this.props.sqon)
: null;
return (
<div className='data-explorer__visualizations'>
Expand Down Expand Up @@ -66,11 +66,13 @@ class DataExplorerVisualizations extends React.Component {
DataExplorerVisualizations.propTypes = {
arrangerData: PropTypes.object,
arrangerConfig: PropTypes.object,
sqon: PropTypes.object,
};

DataExplorerVisualizations.defaultProps = {
arrangerData: null,
arrangerConfig: {},
sqon: null,
};

export default DataExplorerVisualizations;
48 changes: 33 additions & 15 deletions src/components/charts/helper.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,29 +47,43 @@ const getCategoryColorFrom2Colors = index => colorsForCharts.categorical2Colors[

const getDataKey = showPercentage => (showPercentage ? 'percentage' : 'value');

const transformArrangerDataToChart = (field) => {
const transformArrangerDataToChart = (field, sqonValues) => {
const chartData = [];
field.buckets.map(bucket =>
chartData.push({
name: bucket.key,
value: bucket.doc_count,
}),
);
field.buckets
.filter(bucket => (sqonValues === null || sqonValues.includes(bucket.key)))
.forEach(bucket =>
chartData.push({
name: bucket.key,
value: bucket.doc_count,
}),
);
return chartData;
};

const transformArrangerDataToSummary = (field, chartType, title) => ({
const transformArrangerDataToSummary = (field, chartType, title, sqonValues) => ({
type: chartType,
title,
data: transformArrangerDataToChart(field),
data: transformArrangerDataToChart(field, sqonValues),
});

const transformDataToCount = (field, label) => ({
const transformDataToCount = (field, label, sqonValues) => ({
label,
value: field.buckets.length,
value: sqonValues ? Math.min(field.buckets.length, sqonValues.length) : field.buckets.length,
});

const getCharts = (data, arrangerConfig) => {
/**
* Return an array of selected values in a given field
* If no value selected, return null
*/
const getSQONValues = (sqon, field) => {
if (!sqon || !sqon.content) return null;
Copy link
Contributor

Choose a reason for hiding this comment

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

you could also return an empty array right? that might be easier so you wouldn't have to check for null values, but I'll leave that up to you, just a suggestion

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I think returning an empty array will just be similar to returning null so I still want to keep it 😛
sqonValues === null || sqonValues.includes(bucket.key).... =>
sqonValues.length === 0 ? || sqonValues.includes(bucket.key)...

const sqonItems = sqon.content.filter(item => item.content.field === field);
if (!sqonItems || sqonItems.length !== 1) return null;
const sqonValues = sqonItems[0].content.value;
Copy link
Contributor

Choose a reason for hiding this comment

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

whats does grabbing the first index give us?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems arranger use sqon for filtering queries, sqon.content stores all selected fields, and since this function only need one specific field, the sqonItems should be always in length of 1 or zero: 1 means there are some values selected on this field, and zero means no values selected on this field (which means every values should be counted for this field in the visualizations)

return sqonValues;
};

const getCharts = (data, arrangerConfig, sqon) => {
const countItems = [];
const summaries = [];
const stackedBarCharts = [];
Expand All @@ -78,26 +92,29 @@ const getCharts = (data, arrangerConfig) => {
const fields = data.subject.aggregations;
Object.keys(fields).forEach((field) => {
const fieldConfig = arrangerConfig.charts[field];
const sqonValues = getSQONValues(sqon, field);
if (fieldConfig) {
switch (fieldConfig.chartType) {
case 'count':
countItems.push(transformDataToCount(fields[field], fieldConfig.title));
countItems.push(transformDataToCount(fields[field], fieldConfig.title, sqonValues));
break;
case 'pie':
case 'bar':
summaries.push(
transformArrangerDataToSummary(
fields[field],
fieldConfig.chartType,
fieldConfig.title),
fieldConfig.title,
sqonValues),
);
break;
case 'stackedBar':
stackedBarCharts.push(
transformArrangerDataToSummary(
fields[field],
fieldConfig.chartType,
fieldConfig.title),
fieldConfig.title,
sqonValues),
);
break;
default:
Expand All @@ -121,4 +138,5 @@ module.exports = {
transformArrangerDataToChart,
transformArrangerDataToSummary,
getCharts,
getSQONValues,
};
29 changes: 26 additions & 3 deletions src/components/charts/helper.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,23 @@ describe('helper', () => {
expect(helper.getDataKey(false)).toBe('value');
});

const noSelectSqonValues = null;
const selectWhiteSqon = {
op: 'and',
content: [
{
op: 'in',
content: {
field: 'ethnicity',
value: [
'White',
],
},
},
],
};
const selectWhiteSqonValues = ['White'];

const ethnicityFieldJSON = {
buckets: [
{ doc_count: 4, key: 'White' },
Expand Down Expand Up @@ -110,15 +127,17 @@ describe('helper', () => {
};

it('returns chart data as expected', () => {
expect(helper.transformArrangerDataToChart(ethnicityFieldJSON)).toEqual(ethnicityChartData);
expect(helper.transformArrangerDataToChart(ethnicityFieldJSON, noSelectSqonValues))
.toEqual(ethnicityChartData);
});

it('returns count data as expected', () => {
expect(helper.transformDataToCount(ethnicityFieldJSON, 'Ethnicity')).toEqual(ethnicityCountData);
expect(helper.transformDataToCount(ethnicityFieldJSON, 'Ethnicity', noSelectSqonValues))
.toEqual(ethnicityCountData);
});

it('returns chart summaries as expected', () => {
expect(helper.transformArrangerDataToSummary(ethnicityFieldJSON, 'pie', 'Ethnicity')).toEqual(summaryData);
expect(helper.transformArrangerDataToSummary(ethnicityFieldJSON, 'pie', 'Ethnicity', noSelectSqonValues)).toEqual(summaryData);
});

it('gets charts', () => {
Expand All @@ -127,4 +146,8 @@ describe('helper', () => {
expect(charts.summaries).toEqual([summaryData]);
expect(charts.stackedBarCharts).toEqual([]);
});

it('return selecetd values from SQON as expected', () => {
expect(helper.getSQONValues(selectWhiteSqon, 'ethnicity')).toEqual(selectWhiteSqonValues);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need some more tests. what if there is a filter with no values selected? does it return the length of the buckets or the sqon count? things like that.

});