From e6f30a2ea0dd6d7688583b9b154c99d0745bfbc7 Mon Sep 17 00:00:00 2001
From: Luke Elmers <lukeelmers@gmail.com>
Date: Mon, 17 Dec 2018 17:42:55 -0700
Subject: [PATCH] Ensure extra columns are not shown in table vis when
 showPartialRows:true (#25690)

---
 .../table_vis/public/table_vis_params.html    |  2 +-
 .../__tests__/build_hierarchical_data.js      |  2 +-
 .../tabify/__tests__/_integration.js          | 10 +++++---
 .../tabify/__tests__/_response_writer.js      |  4 +--
 .../agg_response/tabify/_get_columns.js       | 10 ++++++--
 .../agg_response/tabify/_response_writer.js   | 25 +++++++++++--------
 src/ui/public/agg_response/tabify/tabify.js   | 12 ++++++++-
 src/ui/public/agg_table/__tests__/_table.js   |  4 ++-
 src/ui/public/vis/request_handlers/courier.js |  7 +++---
 .../request_handlers/request_handlers.d.ts    |  3 ++-
 .../__tests__/visualizations/pie_chart.js     | 12 ++++++---
 .../visualize/loader/visualize_data_loader.ts | 12 +++++++--
 test/functional/apps/visualize/_data_table.js | 21 +++++++++++++++-
 13 files changed, 93 insertions(+), 31 deletions(-)

diff --git a/src/legacy/core_plugins/table_vis/public/table_vis_params.html b/src/legacy/core_plugins/table_vis/public/table_vis_params.html
index 6e93a05e0027..120a61d8c0a6 100644
--- a/src/legacy/core_plugins/table_vis/public/table_vis_params.html
+++ b/src/legacy/core_plugins/table_vis/public/table_vis_params.html
@@ -21,7 +21,7 @@
 
   <div class="checkbox">
     <label>
-      <input type="checkbox" ng-model="editorState.params.showPartialRows">
+      <input type="checkbox" ng-model="editorState.params.showPartialRows" data-test-subj="showPartialRows">
       <span
         i18n-id="tableVis.params.showPartialRowsLabel"
         i18n-default-message="Show partial rows"
diff --git a/src/ui/public/agg_response/hierarchical/__tests__/build_hierarchical_data.js b/src/ui/public/agg_response/hierarchical/__tests__/build_hierarchical_data.js
index 24d5d78517b1..e3086607d23c 100644
--- a/src/ui/public/agg_response/hierarchical/__tests__/build_hierarchical_data.js
+++ b/src/ui/public/agg_response/hierarchical/__tests__/build_hierarchical_data.js
@@ -42,7 +42,7 @@ describe('buildHierarchicalData', function () {
   const buildHierarchicalData = async (aggs, response) => {
     const vis = new Vis(indexPattern, { type: 'histogram',  aggs: aggs });
     vis.isHierarchical = () => true;
-    const data = tabifyAggResponse(vis.aggs, response, { metricsAtAllLevels: true });
+    const data = tabifyAggResponse(vis.aggs, response, { minimalColumns: false });
     return await responseHandler(data);
   };
 
diff --git a/src/ui/public/agg_response/tabify/__tests__/_integration.js b/src/ui/public/agg_response/tabify/__tests__/_integration.js
index e9a99bc33d20..050b94ccf151 100644
--- a/src/ui/public/agg_response/tabify/__tests__/_integration.js
+++ b/src/ui/public/agg_response/tabify/__tests__/_integration.js
@@ -49,7 +49,7 @@ describe('tabifyAggResponse Integration', function () {
     normalizeIds(vis);
 
     const resp = tabifyAggResponse(vis.getAggConfig(), fixtures.metricOnly, {
-      metricsAtAllLevels: vis.isHierarchical()
+      minimalColumns: !vis.isHierarchical(),
     });
 
     expect(resp).to.have.property('rows').and.property('columns');
@@ -138,7 +138,9 @@ describe('tabifyAggResponse Integration', function () {
       // the default for a non-hierarchical vis is to display
       // only complete rows, and only put the metrics at the end.
 
-      const tabbed = tabifyAggResponse(vis.getAggConfig(), esResp, { minimalColumns: true });
+      const tabbed = tabifyAggResponse(vis.getAggConfig(), esResp, {
+        minimalColumns: true,
+      });
 
       expectColumns(tabbed, [ext, src, os, avg]);
 
@@ -159,7 +161,9 @@ describe('tabifyAggResponse Integration', function () {
       // the existing bucket and it's metric
 
       vis.isHierarchical = _.constant(true);
-      const tabbed = tabifyAggResponse(vis.getAggConfig(), esResp, { metricsAtAllLevels: true });
+      const tabbed = tabifyAggResponse(vis.getAggConfig(), esResp, {
+        minimalColumns: false,
+      });
 
       expectColumns(tabbed, [ext, avg, src, avg, os, avg]);
 
diff --git a/src/ui/public/agg_response/tabify/__tests__/_response_writer.js b/src/ui/public/agg_response/tabify/__tests__/_response_writer.js
index b39546714676..df26d2bcf603 100644
--- a/src/ui/public/agg_response/tabify/__tests__/_response_writer.js
+++ b/src/ui/public/agg_response/tabify/__tests__/_response_writer.js
@@ -74,8 +74,8 @@ describe('TabbedAggResponseWriter class', function () {
       expect(responseWriter.columns.length).to.eql(3);
     });
 
-    it('correctly generates columns with metricsAtAllLevels set to true', () => {
-      const minimalColumnsResponseWriter = createResponseWritter(twoSplitsAggConfig, { metricsAtAllLevels: true });
+    it('correctly generates columns with minimalColumns set to false', () => {
+      const minimalColumnsResponseWriter = createResponseWritter(twoSplitsAggConfig, { minimalColumns: false });
       expect(minimalColumnsResponseWriter.columns.length).to.eql(4);
     });
 
diff --git a/src/ui/public/agg_response/tabify/_get_columns.js b/src/ui/public/agg_response/tabify/_get_columns.js
index 68df7e1a0333..ac7cb3bcac33 100644
--- a/src/ui/public/agg_response/tabify/_get_columns.js
+++ b/src/ui/public/agg_response/tabify/_get_columns.js
@@ -27,10 +27,16 @@ const getColumn = (agg, i) => {
   };
 };
 
-export function tabifyGetColumns(aggs, minimal) {
+/**
+ * Builds tabify columns.
+ *
+ * @param {AggConfigs} aggs - the agg configs object to which the aggregation response correlates
+ * @param {boolean} minimalColumns - setting to true will only return a column for the last bucket/metric instead of one for each level
+ */
+export function tabifyGetColumns(aggs, minimalColumns) {
 
   // pick the columns
-  if (minimal) {
+  if (minimalColumns) {
     return aggs.map((agg, i) => getColumn(agg, i));
   }
 
diff --git a/src/ui/public/agg_response/tabify/_response_writer.js b/src/ui/public/agg_response/tabify/_response_writer.js
index e71fe842e0bc..8463c16e750d 100644
--- a/src/ui/public/agg_response/tabify/_response_writer.js
+++ b/src/ui/public/agg_response/tabify/_response_writer.js
@@ -25,22 +25,27 @@ import { tabifyGetColumns } from './_get_columns';
  * produces a table, or a series of tables.
  *
  * @param {AggConfigs} aggs - the agg configs object to which the aggregation response correlates
- * @param {boolean} metricsAtAllLevels - setting to true will produce metrics for every bucket
- * @param {boolean} partialRows - setting to true will not remove rows with missing values
+ * @param {boolean} minimalColumns - setting to true will only return a column for the last bucket/metric instead of one for each level
+ * @param {boolean} partialRows - vis.params.showPartialRows: determines whether to return rows with incomplete data
+ * @param {Object} timeRange - time range object, if provided
  */
-function TabbedAggResponseWriter(aggs, { metricsAtAllLevels = false, partialRows = false, timeRange } = {}) {
+function TabbedAggResponseWriter(aggs, {
+  minimalColumns = true,
+  partialRows = false,
+  timeRange
+} = {}) {
+  // Private
+  this._removePartialRows = !partialRows;
+
+  // Public
   this.rowBuffer = {};
   this.bucketBuffer = [];
   this.metricBuffer = [];
-
-  this.metricsForAllBuckets = metricsAtAllLevels;
-  this.partialRows = partialRows;
   this.aggs = aggs;
-  this.columns = tabifyGetColumns(aggs.getResponseAggs(), !metricsAtAllLevels);
+  this.partialRows = partialRows;
+  this.columns = tabifyGetColumns(aggs.getResponseAggs(), minimalColumns);
   this.aggStack = [...this.columns];
-
   this.rows = [];
-
   // Extract the time range object if provided
   if (timeRange) {
     const timeRangeKey = Object.keys(timeRange)[0];
@@ -67,7 +72,7 @@ TabbedAggResponseWriter.prototype.row = function () {
     this.rowBuffer[metric.id] = metric.value;
   });
 
-  if (!toArray(this.rowBuffer).length || (!this.partialRows && this.isPartialRow(this.rowBuffer))) {
+  if (!toArray(this.rowBuffer).length || (this._removePartialRows && this.isPartialRow(this.rowBuffer))) {
     return;
   }
 
diff --git a/src/ui/public/agg_response/tabify/tabify.js b/src/ui/public/agg_response/tabify/tabify.js
index 08abe3fbb359..c49adb7f242c 100644
--- a/src/ui/public/agg_response/tabify/tabify.js
+++ b/src/ui/public/agg_response/tabify/tabify.js
@@ -21,6 +21,16 @@ import _ from 'lodash';
 import { TabbedAggResponseWriter } from './_response_writer';
 import { TabifyBuckets } from './_buckets';
 
+/**
+ * Sets up the ResponseWriter and kicks off bucket collection.
+ *
+ * @param {AggConfigs} aggs - the agg configs object to which the aggregation response correlates
+ * @param {Object} esResponse - response that came back from Elasticsearch
+ * @param {Object} respOpts - options object for the ResponseWriter with params set by Courier
+ * @param {boolean} respOpts.minimalColumns - setting to true will only return a column for the last bucket/metric instead of one for each level
+ * @param {boolean} respOpts.partialRows - vis.params.showPartialRows: determines whether to return rows with incomplete data
+ * @param {Object} respOpts.timeRange - time range object, if provided
+ */
 export function tabifyAggResponse(aggs, esResponse, respOpts = {}) {
   const write = new TabbedAggResponseWriter(aggs, respOpts);
 
@@ -34,7 +44,7 @@ export function tabifyAggResponse(aggs, esResponse, respOpts = {}) {
 }
 
 /**
- * read an aggregation from a bucket, which is *might* be found at key (if
+ * read an aggregation from a bucket, which *might* be found at key (if
  * the response came in object form), and will recurse down the aggregation
  * tree and will pass the read values to the ResponseWriter.
  *
diff --git a/src/ui/public/agg_table/__tests__/_table.js b/src/ui/public/agg_table/__tests__/_table.js
index 5ad5133e3564..59a25a2123de 100644
--- a/src/ui/public/agg_table/__tests__/_table.js
+++ b/src/ui/public/agg_table/__tests__/_table.js
@@ -57,7 +57,9 @@ describe('AggTable Directive', function () {
     vis2.aggs.forEach(function (agg, i) {
       agg.id = 'agg_' + (i + 1);
     });
-    tabifiedData.threeTermBuckets = tabifyAggResponse(vis2.aggs, fixtures.threeTermBuckets, { metricsAtAllLevels: true });
+    tabifiedData.threeTermBuckets = tabifyAggResponse(vis2.aggs, fixtures.threeTermBuckets, {
+      minimalColumns: false,
+    });
 
     const vis3 = new Vis(indexPattern, {
       type: 'table',
diff --git a/src/ui/public/vis/request_handlers/courier.js b/src/ui/public/vis/request_handlers/courier.js
index a5a9c81eaaaa..79b28a224959 100644
--- a/src/ui/public/vis/request_handlers/courier.js
+++ b/src/ui/public/vis/request_handlers/courier.js
@@ -38,8 +38,9 @@ const CourierRequestHandlerProvider = function () {
       filters,
       forceFetch,
       partialRows,
-      isHierarchical,
+      metricsAtAllLevels,
       inspectorAdapters,
+      minimalColumns,
       queryFilter
     }) {
 
@@ -67,7 +68,7 @@ const CourierRequestHandlerProvider = function () {
       });
 
       requestSearchSource.setField('aggs', function () {
-        return aggs.toDsl(isHierarchical);
+        return aggs.toDsl(metricsAtAllLevels);
       });
 
       requestSearchSource.onRequestStart((searchSource, searchRequest) => {
@@ -133,7 +134,7 @@ const CourierRequestHandlerProvider = function () {
 
       const parsedTimeRange = timeRange ? getTime(aggs.indexPattern, timeRange) : null;
       const tabifyParams = {
-        metricsAtAllLevels: isHierarchical,
+        minimalColumns,
         partialRows,
         timeRange: parsedTimeRange ? parsedTimeRange.range : undefined,
       };
diff --git a/src/ui/public/vis/request_handlers/request_handlers.d.ts b/src/ui/public/vis/request_handlers/request_handlers.d.ts
index e6f93c5353f9..7b2b5d416b46 100644
--- a/src/ui/public/vis/request_handlers/request_handlers.d.ts
+++ b/src/ui/public/vis/request_handlers/request_handlers.d.ts
@@ -36,7 +36,8 @@ export interface RequestHandlerParams {
   uiState?: PersistedState;
   partialRows?: boolean;
   inspectorAdapters?: Adapters;
-  isHierarchical?: boolean;
+  metricsAtAllLevels?: boolean;
+  minimalColumns?: boolean;
   visParams?: any;
 }
 
diff --git a/src/ui/public/vislib/__tests__/visualizations/pie_chart.js b/src/ui/public/vislib/__tests__/visualizations/pie_chart.js
index db3593f997ca..352413fc5740 100644
--- a/src/ui/public/vislib/__tests__/visualizations/pie_chart.js
+++ b/src/ui/public/vislib/__tests__/visualizations/pie_chart.js
@@ -128,8 +128,12 @@ describe('No global chart settings', function () {
   }));
 
   beforeEach(async () => {
-    const table1 = tabifyAggResponse(stubVis1.aggs, fixtures.threeTermBuckets, { metricsAtAllLevels: true });
-    const table2 = tabifyAggResponse(stubVis2.aggs, fixtures.threeTermBuckets, { metricsAtAllLevels: true });
+    const table1 = tabifyAggResponse(stubVis1.aggs, fixtures.threeTermBuckets, {
+      minimalColumns: false,
+    });
+    const table2 = tabifyAggResponse(stubVis2.aggs, fixtures.threeTermBuckets, {
+      minimalColumns: false,
+    });
     data1 = await responseHandler(table1);
     data2 = await responseHandler(table2);
 
@@ -219,7 +223,9 @@ describe('Vislib PieChart Class Test Suite', function () {
       }));
 
       beforeEach(async () => {
-        const table = tabifyAggResponse(stubVis.aggs, fixtures.threeTermBuckets, { metricsAtAllLevels: true });
+        const table = tabifyAggResponse(stubVis.aggs, fixtures.threeTermBuckets, {
+          minimalColumns: false,
+        });
         data = await responseHandler(table);
         vis.render(data, persistedState);
       });
diff --git a/src/ui/public/visualize/loader/visualize_data_loader.ts b/src/ui/public/visualize/loader/visualize_data_loader.ts
index 52f78e83d39d..ca3cbf649bcb 100644
--- a/src/ui/public/visualize/loader/visualize_data_loader.ts
+++ b/src/ui/public/visualize/loader/visualize_data_loader.ts
@@ -73,10 +73,18 @@ export class VisualizeDataLoader {
     this.vis.showRequestError = false;
 
     try {
+      // Vis types that have a `showMetricsAtAllLevels` param (e.g. data table) should tell
+      // tabify whether to return columns for each bucket based on the param value. Vis types
+      // without this param should default to returning all columns if they are hierarchical.
+      const minimalColumns =
+        typeof this.vis.params.showMetricsAtAllLevels !== 'undefined'
+          ? !this.vis.params.showMetricsAtAllLevels
+          : !this.vis.isHierarchical();
       // searchSource is only there for courier request handler
       const requestHandlerResponse = await this.requestHandler({
-        partialRows: this.vis.params.partialRows || this.vis.type.requiresPartialRows,
-        isHierarchical: this.vis.isHierarchical(),
+        partialRows: this.vis.type.requiresPartialRows || this.vis.params.showPartialRows,
+        minimalColumns,
+        metricsAtAllLevels: this.vis.isHierarchical(),
         visParams: this.vis.params,
         ...params,
         filters: params.filters
diff --git a/test/functional/apps/visualize/_data_table.js b/test/functional/apps/visualize/_data_table.js
index 9bc6bda65b6b..3b82b9ae6bee 100644
--- a/test/functional/apps/visualize/_data_table.js
+++ b/test/functional/apps/visualize/_data_table.js
@@ -267,6 +267,25 @@ export default function ({ getService, getPageObjects }) {
         ]);
       });
 
+      it('should show correct data without showMetricsAtAllLevels even if showPartialRows is selected', async () => {
+        await PageObjects.visualize.clickOptionsTab();
+        await PageObjects.visualize.checkCheckbox('showPartialRows');
+        await PageObjects.visualize.clickGo();
+        const data = await PageObjects.visualize.getTableVisContent();
+        expect(data).to.be.eql([
+          [ 'jpg', 'CN', '1,718' ],
+          [ 'jpg', 'IN', '1,511' ],
+          [ 'jpg', 'US', '770' ],
+          [ 'jpg', 'ID', '314' ],
+          [ 'jpg', 'PK', '244' ],
+          [ 'css', 'CN', '422' ],
+          [ 'css', 'IN', '346' ],
+          [ 'css', 'US', '189' ],
+          [ 'css', 'ID', '68' ],
+          [ 'css', 'BR', '58' ],
+        ]);
+      });
+
       it('should show metrics on each level', async () => {
         await PageObjects.visualize.clickOptionsTab();
         await PageObjects.visualize.checkCheckbox('showMetricsAtAllLevels');
@@ -364,7 +383,7 @@ export default function ({ getService, getPageObjects }) {
         ]);
       });
 
-      it('should not show metrics for split bucket when using showMetricsAtAllLevels', async () => {
+      it('should show metrics for split bucket when using showMetricsAtAllLevels', async () => {
         await PageObjects.visualize.clickOptionsTab();
         await PageObjects.visualize.checkCheckbox('showMetricsAtAllLevels');
         await PageObjects.visualize.clickGo();