Skip to content

Commit

Permalink
Preserve nested tables in table vis (#24377)
Browse files Browse the repository at this point in the history
* Add legacy response handler for table vis.

The new legacy response handler introduced a regression in how nested
tables were handled within table vis. This adds a new table-specific
response handler to ensure splitting is preserved.

This is a short term solution and will be removed once we are able to
update table splitting to be consistent with other vis types.

* Ensure formatted dates are preserved in table titles.

* Update legacy table response handler based on feedback.

* Ensure AggConfigResult.rawData is preserved in legacy table response handler.

* Move legacy table response handler to core_plugins.

* Legacy table response handler - style cleanup.

* Remove unneeded aggConfigResult.rawData from legacy table response handler.

* Add basic unit tests for legacy table response handler.

* In table vis, exclude split columns when showing metrics at all levels.

* Add functional tests
  • Loading branch information
lukeelmers authored and timroes committed Oct 25, 2018
1 parent 567576a commit 40c232b
Show file tree
Hide file tree
Showing 7 changed files with 299 additions and 1 deletion.
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,15 @@
type="button"
class="kuiButton kuiButton--basic kuiButton--small"
ng-model="agg.params.row"
data-test-subj="splitBy-row"
btn-radio="true">
Rows
</button>
<button
type="button"
class="kuiButton kuiButton--basic kuiButton--small"
ng-model="agg.params.row"
data-test-subj="splitBy-column"
btn-radio="false">
Columns
</button>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,154 @@
/*
* Licensed to Elasticsearch B.V. under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch B.V. licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

import expect from 'expect.js';
import sinon from 'sinon';
import ngMock from 'ng_mock';
import { AggConfig } from '../../../../ui/public/vis/agg_config';
import AggConfigResult from '../../../../ui/public/vis/agg_config_result';
import { VisProvider } from '../../../../ui/public/vis';
import FixturesStubbedLogstashIndexPatternProvider from 'fixtures/stubbed_logstash_index_pattern';
import { splitRowsOnColumn, splitTable, legacyTableResponseHandler } from '../legacy_response_handler';

const rows = [
{ 'col-0-2': 'A', 'col-1-3': 100, 'col-2-1': 'Jim' },
{ 'col-0-2': 'A', 'col-1-3': 0, 'col-2-1': 'Dwight' },
{ 'col-0-2': 'B', 'col-1-3': 24, 'col-2-1': 'Angela' },
{ 'col-0-2': 'C', 'col-1-3': 1, 'col-2-1': 'Angela' },
{ 'col-0-2': 'C', 'col-1-3': 7, 'col-2-1': 'Angela' },
{ 'col-0-2': 'C', 'col-1-3': -30, 'col-2-1': 'Jim' },
];

describe('Table Vis Legacy Response Handler', () => {

let Vis;
let indexPattern;
let columns;
let mockAggConfig;
let mockSplitAggConfig;

beforeEach(ngMock.module('kibana'));
beforeEach(ngMock.inject(function (Private) {
Vis = Private(VisProvider);
indexPattern = Private(FixturesStubbedLogstashIndexPatternProvider);
const vis = new Vis(indexPattern, { type: 'table', aggs: [] });

mockAggConfig = new AggConfig(vis.aggs, { type: 'terms', schema: 'metric' });
mockSplitAggConfig = new AggConfig(vis.aggs, { type: 'terms', schema: 'split' });

sinon.stub(mockSplitAggConfig, 'fieldFormatter').returns(val => val);
sinon.stub(mockSplitAggConfig, 'makeLabel').returns('some label');

columns = [
{ id: 'col-0-2', name: 'Team', aggConfig: mockSplitAggConfig },
{ id: 'col-1-3', name: 'Score', aggConfig: mockAggConfig },
{ id: 'col-2-1', name: 'Leader', aggConfig: mockAggConfig },
];
}));

describe('#splitRowsOnColumn', () => {
it('should be a function', () => {
expect(typeof splitRowsOnColumn).to.be('function');
});

it('.results should return an array with each unique value for the column id', () => {
const expected = ['A', 'B', 'C'];
const actual = splitRowsOnColumn(rows, 'col-0-2');
expect(actual.results).to.eql(expected);
});

it('.results should preserve types in case a result is not a string', () => {
const expected = [0, 1, 7, 24, 100, -30];
const actual = splitRowsOnColumn(rows, 'col-1-3');
expect(actual.results).to.eql(expected);
actual.results.forEach(result => expect(typeof result).to.eql('number'));
});

it('.rowsGroupedByResult should return an object with rows grouped by value for the column id', () => {
const expected = {
A: [
{ 'col-1-3': 100, 'col-2-1': 'Jim' },
{ 'col-1-3': 0, 'col-2-1': 'Dwight' },
],
B: [
{ 'col-1-3': 24, 'col-2-1': 'Angela' },
],
C: [
{ 'col-1-3': 1, 'col-2-1': 'Angela' },
{ 'col-1-3': 7, 'col-2-1': 'Angela' },
{ 'col-1-3': -30, 'col-2-1': 'Jim' },
],
};
const actual = splitRowsOnColumn(rows, 'col-0-2');
expect(actual.rowsGroupedByResult).to.eql(expected);
});
});

describe('#splitTable', () => {
it('should be a function', () => {
expect(typeof splitTable).to.be('function');
});

it('should return an array of objects with the expected keys', () => {
const expected = ['$parent', 'aggConfig', 'title', 'key', 'tables'];
const actual = splitTable(columns, rows, null);
expect(Object.keys(actual[0])).to.eql(expected);
});

it('should return a reference to the parent AggConfigResult', () => {
const actual = splitTable(columns, rows, null);
expect(actual[0].$parent).to.be.a(AggConfigResult);
});

it('should return the correct split values', () => {
const expected = ['A', 'B', 'C'];
const actual = splitTable(columns, rows, null);
expect(actual.map(i => i.key)).to.eql(expected);
});

it('should return the correct titles', () => {
const expected = ['A: some label', 'B: some label', 'C: some label'];
const actual = splitTable(columns, rows, null);
expect(actual.map(i => i.title)).to.eql(expected);
});

it('should return nested split tables with the correct number of entries', () => {
const expected = [2, 1, 3];
const actual = splitTable(columns, rows, null);
expect(actual.map(i => i.tables[0].rows.length)).to.eql(expected);
});

it('should return nested split tables with rows of the correct type', () => {
const actual = splitTable(columns, rows, null);
expect(actual[0].tables[0].rows[0][0]).to.be.a(AggConfigResult);
});
});

describe('#legacyTableResponseHandler', () => {
it('should be a function', () => {
expect(typeof legacyTableResponseHandler).to.be('function');
});

it('should return the correct number of tables', async () => {
const actual = await legacyTableResponseHandler({ columns, rows });
expect(actual.tables).to.have.length(3);
});
});

});
2 changes: 2 additions & 0 deletions src/core_plugins/table_vis/public/__tests__/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,5 +18,7 @@
*/

import './_table_vis_controller';
import './_legacy_response_handler';

describe('Table Vis', function () {
});
99 changes: 99 additions & 0 deletions src/core_plugins/table_vis/public/legacy_response_handler.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
/*
* Licensed to Elasticsearch B.V. under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch B.V. licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

import { get, findLastIndex } from 'lodash';
import AggConfigResult from 'ui/vis/agg_config_result';

/**
* Takes an array of tabified rows and splits them by column value:
*
* const rows = [
* { col-1: 'foo', col-2: 'X' },
* { col-1: 'bar', col-2: 50 },
* { col-1: 'baz', col-2: 'X' },
* ];
* const splitRows = splitRowsOnColumn(rows, 'col-2');
* splitRows.results; // ['X', 50];
* splitRows.rowsGroupedByResult; // { X: [{ col-1: 'foo' }, { col-1: 'baz' }], 50: [{ col-1: 'bar' }] }
*/
export function splitRowsOnColumn(rows, columnId) {
const resultsMap = {}; // Used to preserve types, since object keys are always converted to strings.
return {
rowsGroupedByResult: rows.reduce((acc, row) => {
const { [columnId]: splitValue, ...rest } = row;
resultsMap[splitValue] = splitValue;
acc[splitValue] = [...(acc[splitValue] || []), rest];
return acc;
}, {}),
results: Object.values(resultsMap),
};
}

export function splitTable(columns, rows, $parent) {
const splitColumn = columns.find(column => get(column, 'aggConfig.schema.name') === 'split');

if (!splitColumn) {
return [{
$parent,
columns: columns.map(column => ({ title: column.name, ...column })),
rows: rows.map(row => {
return columns.map(column => {
return new AggConfigResult(column.aggConfig, $parent, row[column.id], row[column.id]);
});
})
}];
}

const splitColumnIndex = columns.findIndex(column => column.id === splitColumn.id);
const splitRows = splitRowsOnColumn(rows, splitColumn.id);

// Check if there are buckets after the first metric.
const firstMetricsColumnIndex = columns.findIndex(column => get(column, 'aggConfig.type.type') === 'metrics');
const lastBucketsColumnIndex = findLastIndex(columns, column => get(column, 'aggConfig.type.type') === 'buckets');
const metricsAtAllLevels = firstMetricsColumnIndex < lastBucketsColumnIndex;

// Calculate metrics:bucket ratio.
const numberOfMetrics = columns.filter(column => get(column, 'aggConfig.type.type') === 'metrics').length;
const numberOfBuckets = columns.filter(column => get(column, 'aggConfig.type.type') === 'buckets').length;
const metricsPerBucket = numberOfMetrics / numberOfBuckets;

const filteredColumns = columns
.filter((column, i) => {
const isSplitColumn = i === splitColumnIndex;
const isSplitMetric = metricsAtAllLevels && i > splitColumnIndex && i <= splitColumnIndex + metricsPerBucket;
return !isSplitColumn && !isSplitMetric;
})
.map(column => ({ title: column.name, ...column }));

return splitRows.results.map(splitValue => {
const $newParent = new AggConfigResult(splitColumn.aggConfig, $parent, splitValue, splitValue);
return {
$parent: $newParent,
aggConfig: splitColumn.aggConfig,
title: `${splitColumn.aggConfig.fieldFormatter()(splitValue)}: ${splitColumn.aggConfig.makeLabel()}`,
key: splitValue,
// Recurse with filtered data to continue the search for additional split columns.
tables: splitTable(filteredColumns, splitRows.rowsGroupedByResult[splitValue], $newParent),
};
});
}

export async function legacyTableResponseHandler(table) {
return { tables: splitTable(table.columns, table.rows, null) };
}
4 changes: 3 additions & 1 deletion src/core_plugins/table_vis/public/table_vis.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ import { CATEGORY } from 'ui/vis/vis_category';
import { Schemas } from 'ui/vis/editors/default/schemas';
import tableVisTemplate from './table_vis.html';
import { VisTypesRegistryProvider } from 'ui/registry/vis_types';
import { legacyTableResponseHandler } from './legacy_response_handler';

// we need to load the css ourselves

// we also need to load the controller and used by the template
Expand Down Expand Up @@ -94,7 +96,7 @@ function TableVisTypeProvider(Private) {
}
])
},
responseHandler: 'legacy',
responseHandler: legacyTableResponseHandler,
responseHandlerConfig: {
asAggConfigResults: true
},
Expand Down
16 changes: 16 additions & 0 deletions test/functional/apps/visualize/_data_table.js
Original file line number Diff line number Diff line change
Expand Up @@ -293,6 +293,7 @@ export default function ({ getService, getPageObjects }) {
await PageObjects.visualize.selectAggregation('Terms');
await PageObjects.visualize.selectField('geo.src');
await PageObjects.visualize.setSize(3);
await PageObjects.visualize.toggleOpenEditor(4, 'false');
await PageObjects.visualize.clickGo();
});

Expand Down Expand Up @@ -354,6 +355,21 @@ export default function ({ getService, getPageObjects }) {
]
]);
});

it('should allow nesting multiple splits', async () => {
// This test can be removed as soon as we remove the nested split table
// feature (https://github.com/elastic/kibana/issues/24560).
await PageObjects.visualize.clickData();
await PageObjects.visualize.clickAddBucket();
await PageObjects.visualize.clickBucket('Split Table');
await PageObjects.visualize.selectAggregation('Terms');
await PageObjects.visualize.clickSplitDirection('column');
await PageObjects.visualize.selectField('machine.os.raw');
await PageObjects.visualize.setSize(2);
await PageObjects.visualize.clickGo();
const splitCount = await PageObjects.visualize.countNestedTables();
expect(splitCount).to.be.eql([ 12, 10, 8 ]);
});
});

});
Expand Down
23 changes: 23 additions & 0 deletions test/functional/page_objects/visualize_page.js
Original file line number Diff line number Diff line change
Expand Up @@ -1220,6 +1220,29 @@ export function VisualizePageProvider({ getService, getPageObjects }) {
await this.selectAggregation(metric, 'groupName');
await this.selectField(field, 'groupName');
}

async clickSplitDirection(direction) {
const activeParamPanel = await find.byCssSelector('vis-editor-agg-params[aria-hidden="false"]');
const button = await testSubjects.findDescendant(`splitBy-${direction}`, activeParamPanel);
await button.click();
}

async countNestedTables() {
const vis = await testSubjects.find('tableVis');
const result = [];

for (let i = 1; true; i++) {
const selector = new Array(i).fill('.agg-table-group').join(' ');
const tables = await vis.findAllByCssSelector(selector);
if (tables.length === 0) {
break;
}
result.push(tables.length);
}

return result;
}

}

return new VisualizePage();
Expand Down

0 comments on commit 40c232b

Please sign in to comment.