Skip to content

Commit

Permalink
Fixes pie charts on empty time window (#24031) (#24517)
Browse files Browse the repository at this point in the history 
* Fix missing check on empty response

* Fix test order and remove applying on each entered filter

* Change quotes on addNewFilterAggregation testsubject

* Rename test hasPieChartError method to expectPieChartError

* Refactor piechart zero-value slices data cleaning.

The previous implementation used to remove zero-value slices by mutating the vis data from the legend logic.
We moved the logic of "cleaning" the zero-value slices before rendering the piechart and/or the legend, so now piechart and legends are rendering themselves with the same data structure.

* Reverting _validatePieData method to the old one
  • Loading branch information
@markov00
markov00 authored Oct 24, 2018
1 parent e400a9e commit 0f677d8
Show file tree
Hide file tree
Showing 6 changed files with 119 additions and 15 deletions.
2 changes: 2 additions & 0 deletions src/ui/public/agg_types/controls/filters.html
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@

<div class="form-group">
<input
data-test-subj="visEditorFilterInput_{{agg.id}}_{{$index}}"
id="visEditorFilterInput{{agg.id}}"
parse-query
ng-model="filter.input.query"
Expand Down Expand Up @@ -58,6 +59,7 @@
</div>

<button
data-test-subj="visEditorAddFilterButton"
click-focus="'filter'+(agg.params.filters.length-1)"
ng-click="agg.params.filters.push({input:{}})"
class="kuiButton kuiButton--primary kuiButton--fullWidth"
Expand Down
44 changes: 34 additions & 10 deletions src/ui/public/vislib/lib/data.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ export function VislibLibDataProvider(Private) {
this.uiState = uiState;
this.data = this.copyDataObj(data);
this.type = this.getDataType();

this._cleanVisData();
this.labels = this._getLabels(this.data);
this.color = this.labels ? color(this.labels, uiState.get('vis.colors')) : undefined;
this._normalizeOrdered();
Expand Down Expand Up @@ -96,7 +96,10 @@ export function VislibLibDataProvider(Private) {
}

_getLabels(data) {
return this.type === 'series' ? getLabels(data) : this.pieNames();
if (this.type === 'series') {
return getLabels(data);
}
return [];
}

getDataType() {
Expand Down Expand Up @@ -345,19 +348,42 @@ export function VislibLibDataProvider(Private) {
}

/**
* Removes zeros from pie chart data
* Clean visualization data from missing/wrong values.
* Currently used only to clean remove zero slices from
* pie chart.
*/
_cleanVisData() {
const visData = this.getVisData();
if (this.type === 'slices') {
this._cleanPieChartData(visData);
}
}

/**
* Mutate the current pie chart vis data to remove slices with
* zero values.
* @param {Array} data
*/
_cleanPieChartData(data) {
_.forEach(data, (obj) => {
obj.slices = this._removeZeroSlices(obj.slices);
});
}

/**
* Removes zeros from pie chart data, mutating the passed values.
* @param slices
* @returns {*}
*/
_removeZeroSlices(slices) {
const self = this;

if (!slices.children) return slices;
if (!slices.children) {
return slices;
}

slices = _.clone(slices);
slices.children = slices.children.reduce(function (children, child) {
slices.children = slices.children.reduce((children, child) => {
if (child.size !== 0) {
children.push(self._removeZeroSlices(child));
return [...children, this._removeZeroSlices(child)];
}
return children;
}, []);
Expand All @@ -378,8 +404,6 @@ export function VislibLibDataProvider(Private) {

_.forEach(data, function (obj) {
const columns = obj.raw ? obj.raw.columns : undefined;
obj.slices = self._removeZeroSlices(obj.slices);

_.forEach(self.getNames(obj, columns), function (name) {
names.push(name);
});
Expand Down
3 changes: 2 additions & 1 deletion src/ui/public/vislib/lib/handler.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,8 @@ export function VisHandlerProvider(Private) {
.append('div')
// class name needs `chart` in it for the polling checkSize function
// to continuously call render on resize
.attr('class', 'visualize-error chart error');
.attr('class', 'visualize-error chart error')
.attr('data-test-subj', 'visLibVisualizeError');

if (message === 'No results found') {
div.append('div')
Expand Down
4 changes: 1 addition & 3 deletions src/ui/public/vislib/visualizations/pie_chart.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,10 +48,8 @@ export function VislibVisualizationsPieChartProvider(Private) {
class PieChart extends Chart {
constructor(handler, chartEl, chartData) {
super(handler, chartEl, chartData);

const charts = this.handler.data.getVisData();
this._validatePieData(charts);

this._attr = _.defaults(handler.visConfig.get('chart', {}), defaults);
}

Expand All @@ -61,7 +59,7 @@ export function VislibVisualizationsPieChartProvider(Private) {
* If so, an error is thrown.
*/
_validatePieData(charts) {
const isAllZeros = charts.every(function (chart) {
const isAllZeros = charts.every((chart) => {
return chart.slices.children.length === 0;
});

Expand Down
59 changes: 58 additions & 1 deletion test/functional/apps/visualize/_pie_chart.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ export default function ({ getService, getPageObjects }) {
describe('pie chart', async function () {
const vizName1 = 'Visualization PieChart';
before(async function () {

log.debug('navigateToApp visualize');
await PageObjects.visualize.navigateToNewVisualization();
log.debug('clickPieChart');
Expand Down Expand Up @@ -170,5 +169,63 @@ export default function ({ getService, getPageObjects }) {
expect(pieData).to.eql(expectedTableData);
});
});

describe('empty time window', () => {
it('should show no data message when no data on selected timerange', async function () {
await PageObjects.visualize.navigateToNewVisualization();
log.debug('clickPieChart');
await PageObjects.visualize.clickPieChart();
await PageObjects.visualize.clickNewSearch();
log.debug('Set absolute time range from \"' + fromTime + '\" to \"' + toTime + '\"');
await PageObjects.header.setAbsoluteRange(fromTime, toTime);
log.debug('select bucket Split Slices');
await PageObjects.visualize.clickBucket('Split Slices');
log.debug('Click aggregation Filters');
await PageObjects.visualize.selectAggregation('Filters');
log.debug('Set the 1st filter value');
await PageObjects.visualize.setFilterAggregationValue('geo.dest:"US"');
log.debug('Add new filter');
await PageObjects.visualize.addNewFilterAggregation();
log.debug('Set the 2nd filter value');
await PageObjects.visualize.setFilterAggregationValue('geo.dest:"CN"', 1);
await PageObjects.visualize.clickGo();
const emptyFromTime = '2016-09-19 06:31:44.000';
const emptyToTime = '2016-09-23 18:31:44.000';
log.debug('Switch to a different time range from \"' + emptyFromTime + '\" to \"' + emptyToTime + '\"');
await PageObjects.header.setAbsoluteRange(emptyFromTime, emptyToTime);
await PageObjects.visualize.waitForVisualization();
await PageObjects.visualize.expectPieChartError();
});
});
describe('multi series slice', () => {
it('should still showing pie chart when a subseries have zero data', async function () {
await PageObjects.visualize.navigateToNewVisualization();
log.debug('clickPieChart');
await PageObjects.visualize.clickPieChart();
await PageObjects.visualize.clickNewSearch();
log.debug('Set absolute time range from \"' + fromTime + '\" to \"' + toTime + '\"');
await PageObjects.header.setAbsoluteRange(fromTime, toTime);
log.debug('select bucket Split Slices');
await PageObjects.visualize.clickBucket('Split Slices');
log.debug('Click aggregation Filters');
await PageObjects.visualize.selectAggregation('Filters');
log.debug('Set the 1st filter value');
await PageObjects.visualize.setFilterAggregationValue('geo.dest:"US"');
log.debug('Toggle previous editor');
await PageObjects.visualize.toggleAggegationEditor(2);
log.debug('Add a new series');
await PageObjects.visualize.clickAddBucket();
log.debug('select bucket Split Slices');
await PageObjects.visualize.clickBucket('Split Slices');
log.debug('Click aggregation Filters');
await PageObjects.visualize.selectAggregation('Filters');
log.debug('Set the 1st filter value of the aggregation id 3');
await PageObjects.visualize.setFilterAggregationValue('geo.dest:"UX"', 0, 3);
await PageObjects.visualize.clickGo();
const legends = await PageObjects.visualize.getLegendEntries();
const expectedLegends = ['geo.dest:"US"', 'geo.dest:"UX"'];
expect(legends).to.eql(expectedLegends);
});
});
});
}
22 changes: 22 additions & 0 deletions test/functional/page_objects/visualize_page.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -509,6 +509,21 @@ export function VisualizePageProvider({ getService, getPageObjects }) {
await PageObjects.common.sleep(500);
}

/**
* Set the test for a filter aggregation.
* @param {*} filterValue the string value of the filter
* @param {*} filterIndex used when multiple filters are configured on the same aggregation
* @param {*} aggregationId the ID if the aggregation. On Tests, it start at from 2
*/
async setFilterAggregationValue(filterValue, filterIndex = 0, aggregationId = 2) {
const inputField = await testSubjects.find(`visEditorFilterInput_${aggregationId}_${filterIndex}`);
await inputField.type(filterValue);
}

async addNewFilterAggregation() {
return await testSubjects.click('visEditorAddFilterButton');
}

async toggleOpenEditor(index, toState = 'true') {
// index, see selectYAxisAggregation
const toggle = await find.byCssSelector(`button[aria-controls="visAggEditorParams${index}"]`);
Expand Down Expand Up @@ -653,6 +668,10 @@ export function VisualizePageProvider({ getService, getPageObjects }) {
await testSubjects.click(`aggregationEditor${agg} disableAggregationBtn`);
await PageObjects.header.waitUntilLoadingHasFinished();
}
async toggleAggegationEditor(agg) {
await testSubjects.click(`aggregationEditor${agg} toggleEditor`);
await PageObjects.header.waitUntilLoadingHasFinished();
}

async toggleOtherBucket() {
return await find.clickByCssSelector('input[name="showOther"]');
Expand Down Expand Up @@ -922,6 +941,9 @@ export function VisualizePageProvider({ getService, getPageObjects }) {
const getChartTypesPromises = chartTypes.map(async chart => await chart.getAttribute('data-label'));
return await Promise.all(getChartTypesPromises);
}
async expectPieChartError() {
return await testSubjects.existOrFail('visLibVisualizeError');
}

async getChartAreaWidth() {
const rect = await retry.try(async () => find.byCssSelector('clipPath rect'));
Expand Down

0 comments on commit 0f677d8

Please sign in to comment.