From 85e67d7b26ba040c53302f169952fdb575d4aff3 Mon Sep 17 00:00:00 2001 From: Tim Roes Date: Thu, 25 Oct 2018 15:04:22 +0200 Subject: [PATCH] Preserve nested tables in table vis (#24377) (#24571) * 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 --- .../public/controls/rows_or_columns.html | 2 + .../__tests__/_legacy_response_handler.js | 154 ++++++++++++++++++ .../table_vis/public/__tests__/index.js | 2 + .../public/legacy_response_handler.js | 99 +++++++++++ .../table_vis/public/table_vis.js | 4 +- test/functional/apps/visualize/_data_table.js | 16 ++ .../functional/page_objects/visualize_page.js | 23 +++ 7 files changed, 299 insertions(+), 1 deletion(-) create mode 100644 src/core_plugins/table_vis/public/__tests__/_legacy_response_handler.js create mode 100644 src/core_plugins/table_vis/public/legacy_response_handler.js diff --git a/src/core_plugins/kbn_vislib_vis_types/public/controls/rows_or_columns.html b/src/core_plugins/kbn_vislib_vis_types/public/controls/rows_or_columns.html index 4ef19a4b29a1..5db6862c592f 100644 --- a/src/core_plugins/kbn_vislib_vis_types/public/controls/rows_or_columns.html +++ b/src/core_plugins/kbn_vislib_vis_types/public/controls/rows_or_columns.html @@ -4,6 +4,7 @@ type="button" class="kuiButton kuiButton--basic kuiButton--small" ng-model="agg.params.row" + data-test-subj="splitBy-row" btn-radio="true"> Rows @@ -11,6 +12,7 @@ type="button" class="kuiButton kuiButton--basic kuiButton--small" ng-model="agg.params.row" + data-test-subj="splitBy-column" btn-radio="false"> Columns diff --git a/src/core_plugins/table_vis/public/__tests__/_legacy_response_handler.js b/src/core_plugins/table_vis/public/__tests__/_legacy_response_handler.js new file mode 100644 index 000000000000..07fc069de53d --- /dev/null +++ b/src/core_plugins/table_vis/public/__tests__/_legacy_response_handler.js @@ -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); + }); + }); + +}); diff --git a/src/core_plugins/table_vis/public/__tests__/index.js b/src/core_plugins/table_vis/public/__tests__/index.js index aa5320faf880..2508ba5764be 100644 --- a/src/core_plugins/table_vis/public/__tests__/index.js +++ b/src/core_plugins/table_vis/public/__tests__/index.js @@ -18,5 +18,7 @@ */ import './_table_vis_controller'; +import './_legacy_response_handler'; + describe('Table Vis', function () { }); diff --git a/src/core_plugins/table_vis/public/legacy_response_handler.js b/src/core_plugins/table_vis/public/legacy_response_handler.js new file mode 100644 index 000000000000..b8f12f00c4bb --- /dev/null +++ b/src/core_plugins/table_vis/public/legacy_response_handler.js @@ -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) }; +} diff --git a/src/core_plugins/table_vis/public/table_vis.js b/src/core_plugins/table_vis/public/table_vis.js index 625c309bf4c0..dfa28c34968c 100644 --- a/src/core_plugins/table_vis/public/table_vis.js +++ b/src/core_plugins/table_vis/public/table_vis.js @@ -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 @@ -94,7 +96,7 @@ function TableVisTypeProvider(Private) { } ]) }, - responseHandler: 'legacy', + responseHandler: legacyTableResponseHandler, responseHandlerConfig: { asAggConfigResults: true }, diff --git a/test/functional/apps/visualize/_data_table.js b/test/functional/apps/visualize/_data_table.js index 7e0fe6fcbd95..10664b5c3526 100644 --- a/test/functional/apps/visualize/_data_table.js +++ b/test/functional/apps/visualize/_data_table.js @@ -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(); }); @@ -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 ]); + }); }); }); diff --git a/test/functional/page_objects/visualize_page.js b/test/functional/page_objects/visualize_page.js index 7b597a2ab320..9e742f129593 100644 --- a/test/functional/page_objects/visualize_page.js +++ b/test/functional/page_objects/visualize_page.js @@ -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();