From fef3dd22557990370b9d9f1a43b795dbb7d1ed27 Mon Sep 17 00:00:00 2001 From: Luke Elmers Date: Wed, 17 Oct 2018 15:40:23 -0600 Subject: [PATCH 1/5] Ensure custom set axis titles are preserved when loading a saved visualization. Previously, the visualization editor would lose a custom set axis title every time you opened the editor and loaded a visualization or if you changed the actual aggregation label. This was due to the way that agg labels were overwriting custom axis titles. This change simply checks to ensure the custom title hasn't already been changed before overwriting it with an updated agg label. --- .../controls/point_series/value_axes.js | 5 ++++- .../public/editors/__tests__/point_series.js | 22 +++++++++++++++++++ 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/src/core_plugins/kbn_vislib_vis_types/public/controls/point_series/value_axes.js b/src/core_plugins/kbn_vislib_vis_types/public/controls/point_series/value_axes.js index cabd27767b2cb..ae49d35caa866 100644 --- a/src/core_plugins/kbn_vislib_vis_types/public/controls/point_series/value_axes.js +++ b/src/core_plugins/kbn_vislib_vis_types/public/controls/point_series/value_axes.js @@ -158,8 +158,11 @@ module.directive('vislibValueAxes', function () { label = matchingSeries[0].makeLabel(); } if (lastAxisTitles[axis.id] !== label && label !== '') { + // Only overwrite the custom title with the value axis label if it hasn't been changed + if (lastAxisTitles[axis.id] === axis.title.text) { + axis.title.text = label; + } lastAxisTitles[axis.id] = label; - axis.title.text = label; } }); }; diff --git a/src/core_plugins/kbn_vislib_vis_types/public/editors/__tests__/point_series.js b/src/core_plugins/kbn_vislib_vis_types/public/editors/__tests__/point_series.js index a4384ae2a1fea..31b69506215b6 100644 --- a/src/core_plugins/kbn_vislib_vis_types/public/editors/__tests__/point_series.js +++ b/src/core_plugins/kbn_vislib_vis_types/public/editors/__tests__/point_series.js @@ -136,4 +136,26 @@ describe('point series editor', function () { $parentScope.updateAxisTitle(); expect($parentScope.editorState.params.valueAxes[0].title.text).to.equal('Custom Title'); }); + + it('should set the custom title to match the value axis label when only one agg exists for that axis', function () { + $parentScope.editorState.aggs[0].params.customLabel = 'Custom Label'; + $parentScope.updateAxisTitle(); + expect($parentScope.editorState.params.valueAxes[0].title.text).to.equal('Custom Label'); + }); + + it('should not set the custom title to match the value axis label when more than one agg exists for that axis', function () { + const aggConfig = new AggConfig($parentScope.vis.aggs, { type: 'avg', schema: 'metric', params: { field: 'bytes' } }); + $parentScope.vis.aggs.push(aggConfig); + $parentScope.$digest(); + $parentScope.editorState.aggs[0].params.customLabel = 'Custom Label'; + $parentScope.updateAxisTitle(); + expect($parentScope.editorState.params.valueAxes[0].title.text).to.equal('Count'); + }); + + it('should not overwrite the custom title with the value axis label if the custom title has been changed', function () { + $parentScope.editorState.params.valueAxes[0].title.text = 'Custom Title'; + $parentScope.editorState.aggs[0].params.customLabel = 'Custom Label'; + $parentScope.updateAxisTitle(); + expect($parentScope.editorState.params.valueAxes[0].title.text).to.equal('Custom Title'); + }); }); From f0596eec3a4a4f27ec91f6c71bee90d1d91fe81a Mon Sep 17 00:00:00 2001 From: Luke Elmers Date: Wed, 7 Nov 2018 17:20:22 -0700 Subject: [PATCH 2/5] Reset axis title when agg type changes. --- .../controls/point_series/value_axes.js | 28 +++++++++++++------ 1 file changed, 20 insertions(+), 8 deletions(-) diff --git a/src/core_plugins/kbn_vislib_vis_types/public/controls/point_series/value_axes.js b/src/core_plugins/kbn_vislib_vis_types/public/controls/point_series/value_axes.js index ae49d35caa866..0c843784acf3c 100644 --- a/src/core_plugins/kbn_vislib_vis_types/public/controls/point_series/value_axes.js +++ b/src/core_plugins/kbn_vislib_vis_types/public/controls/point_series/value_axes.js @@ -136,10 +136,11 @@ module.directive('vislibValueAxes', function () { }, 1); }; - const lastAxisTitles = {}; + const lastCustomLabels = {}; + let lastMatchingSeriesAggType = ''; // We track this so we can know when the agg type is changed $scope.updateAxisTitle = function () { $scope.editorState.params.valueAxes.forEach((axis, axisNumber) => { - let label = ''; + let newCustomLabel = ''; const isFirst = axisNumber === 0; const matchingSeries = []; $scope.editorState.params.seriesParams.forEach((series, i) => { @@ -154,16 +155,27 @@ module.directive('vislibValueAxes', function () { }); } }); + if (matchingSeries.length === 1) { - label = matchingSeries[0].makeLabel(); + newCustomLabel = matchingSeries[0].makeLabel(); } - if (lastAxisTitles[axis.id] !== label && label !== '') { - // Only overwrite the custom title with the value axis label if it hasn't been changed - if (lastAxisTitles[axis.id] === axis.title.text) { - axis.title.text = label; + + const matchingSeriesAggType = _.get(matchingSeries, '[0]type.name', ''); + + if (lastCustomLabels[axis.id] !== newCustomLabel && newCustomLabel !== '') { + const isFirstRender = Object.keys(lastCustomLabels).length === 0; + const aggTypeIsChanged = lastMatchingSeriesAggType !== matchingSeriesAggType; + const axisTitleIsEmpty = axis.title.text === ''; + const lastCustomLabelMatchesAxisTitle = lastCustomLabels[axis.id] === axis.title.text; + + if (!isFirstRender && (aggTypeIsChanged || axisTitleIsEmpty || lastCustomLabelMatchesAxisTitle)) { + axis.title.text = newCustomLabel; // Override axis title with new custom label } - lastAxisTitles[axis.id] = label; + + lastCustomLabels[axis.id] = newCustomLabel; } + + lastMatchingSeriesAggType = matchingSeriesAggType; }); }; From fd31e3716603508e7a73a0b58890e82d6afc95ef Mon Sep 17 00:00:00 2001 From: Luke Elmers Date: Thu, 8 Nov 2018 11:07:20 -0700 Subject: [PATCH 3/5] Reset axis title when agg field is changed. --- .../public/controls/point_series/value_axes.js | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/core_plugins/kbn_vislib_vis_types/public/controls/point_series/value_axes.js b/src/core_plugins/kbn_vislib_vis_types/public/controls/point_series/value_axes.js index 0c843784acf3c..3f97601422d76 100644 --- a/src/core_plugins/kbn_vislib_vis_types/public/controls/point_series/value_axes.js +++ b/src/core_plugins/kbn_vislib_vis_types/public/controls/point_series/value_axes.js @@ -137,7 +137,9 @@ module.directive('vislibValueAxes', function () { }; const lastCustomLabels = {}; - let lastMatchingSeriesAggType = ''; // We track this so we can know when the agg type is changed + // We track these so we can know when the agg is changed + let lastMatchingSeriesAggType = ''; + let lastMatchingSeriesAggField = ''; $scope.updateAxisTitle = function () { $scope.editorState.params.valueAxes.forEach((axis, axisNumber) => { let newCustomLabel = ''; @@ -161,14 +163,17 @@ module.directive('vislibValueAxes', function () { } const matchingSeriesAggType = _.get(matchingSeries, '[0]type.name', ''); + const matchingSeriesAggField = _.get(matchingSeries, '[0]params.field.name', ''); if (lastCustomLabels[axis.id] !== newCustomLabel && newCustomLabel !== '') { const isFirstRender = Object.keys(lastCustomLabels).length === 0; const aggTypeIsChanged = lastMatchingSeriesAggType !== matchingSeriesAggType; + const aggFieldIsChanged = lastMatchingSeriesAggField !== matchingSeriesAggField; + const aggIsChanged = aggTypeIsChanged || aggFieldIsChanged; const axisTitleIsEmpty = axis.title.text === ''; const lastCustomLabelMatchesAxisTitle = lastCustomLabels[axis.id] === axis.title.text; - if (!isFirstRender && (aggTypeIsChanged || axisTitleIsEmpty || lastCustomLabelMatchesAxisTitle)) { + if (!isFirstRender && (aggIsChanged || axisTitleIsEmpty || lastCustomLabelMatchesAxisTitle)) { axis.title.text = newCustomLabel; // Override axis title with new custom label } @@ -176,6 +181,7 @@ module.directive('vislibValueAxes', function () { } lastMatchingSeriesAggType = matchingSeriesAggType; + lastMatchingSeriesAggField = matchingSeriesAggField; }); }; From 20b59167bc21c087e3ceeb6ca86cacf7a4f3663f Mon Sep 17 00:00:00 2001 From: Luke Elmers Date: Thu, 8 Nov 2018 11:33:22 -0700 Subject: [PATCH 4/5] Update point series vis type unit tests. --- .../public/editors/__tests__/point_series.js | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/src/core_plugins/kbn_vislib_vis_types/public/editors/__tests__/point_series.js b/src/core_plugins/kbn_vislib_vis_types/public/editors/__tests__/point_series.js index 31b69506215b6..be20e8952492e 100644 --- a/src/core_plugins/kbn_vislib_vis_types/public/editors/__tests__/point_series.js +++ b/src/core_plugins/kbn_vislib_vis_types/public/editors/__tests__/point_series.js @@ -158,4 +158,19 @@ describe('point series editor', function () { $parentScope.updateAxisTitle(); expect($parentScope.editorState.params.valueAxes[0].title.text).to.equal('Custom Title'); }); + + it('should overwrite the custom title when the agg type changes', function () { + const aggConfig = new AggConfig($parentScope.vis.aggs, { type: 'avg', schema: 'metric', params: { field: 'bytes' } }); + + $parentScope.editorState.params.valueAxes[0].title.text = 'Custom Title'; + $parentScope.editorState.aggs[0].params.customLabel = 'Custom Label'; + $parentScope.updateAxisTitle(); + + $parentScope.vis.aggs.push(aggConfig); + $parentScope.vis.aggs.shift(); + $parentScope.$digest(); + $parentScope.updateAxisTitle(); + + expect($parentScope.editorState.params.valueAxes[0].title.text).to.equal('Average bytes'); + }); }); From 6e6d1685ea8a32dbee368eac13dca98e105a3764 Mon Sep 17 00:00:00 2001 From: Luke Elmers Date: Thu, 8 Nov 2018 17:31:58 -0700 Subject: [PATCH 5/5] Add functional tests for axis title bug. --- test/functional/apps/visualize/_pie_chart.js | 2 +- .../apps/visualize/_point_series_options.js | 41 +++++++++++++++++++ .../page_objects/point_series_page.js | 8 ++++ .../functional/page_objects/visualize_page.js | 5 ++- 4 files changed, 53 insertions(+), 3 deletions(-) diff --git a/test/functional/apps/visualize/_pie_chart.js b/test/functional/apps/visualize/_pie_chart.js index ffdde2fbc6e6a..e53f89d9d01e4 100644 --- a/test/functional/apps/visualize/_pie_chart.js +++ b/test/functional/apps/visualize/_pie_chart.js @@ -259,7 +259,7 @@ export default function ({ getService, getPageObjects }) { log.debug('Set the 1st filter value'); await PageObjects.visualize.setFilterAggregationValue('geo.dest:"US"'); log.debug('Toggle previous editor'); - await PageObjects.visualize.toggleAggegationEditor(2); + await PageObjects.visualize.toggleAggregationEditor(2); log.debug('Add a new series'); await PageObjects.visualize.clickAddBucket(); log.debug('select bucket Split Slices'); diff --git a/test/functional/apps/visualize/_point_series_options.js b/test/functional/apps/visualize/_point_series_options.js index 91e9e4e05a2bf..6262963d886ed 100644 --- a/test/functional/apps/visualize/_point_series_options.js +++ b/test/functional/apps/visualize/_point_series_options.js @@ -137,5 +137,46 @@ export default function ({ getService, getPageObjects }) { }); }); + describe('custom labels and axis titles', function () { + const visName = 'Visualization Point Series Test'; + const customLabel = 'myLabel'; + const axisTitle = 'myTitle'; + before(async function () { + await PageObjects.visualize.navigateToNewVisualization(); + await PageObjects.visualize.clickLineChart(); + await PageObjects.visualize.clickNewSearch(); + await PageObjects.visualize.selectYAxisAggregation('Average', 'bytes', customLabel, 1); + await PageObjects.visualize.clickGo(); + await PageObjects.visualize.clickMetricsAndAxes(); + await PageObjects.visualize.clickYAxisOptions('ValueAxis-1'); + }); + + it('should render a custom label when one is set', async function () { + const title = await PageObjects.visualize.getYAxisTitle(); + expect(title).to.be(customLabel); + }); + + it('should render a custom axis title when one is set, overriding the custom label', async function () { + await pointSeriesVis.setAxisTitle(axisTitle); + await PageObjects.visualize.clickGo(); + const title = await PageObjects.visualize.getYAxisTitle(); + expect(title).to.be(axisTitle); + }); + + it('should preserve saved axis titles after a vis is saved and reopened', async function () { + await PageObjects.visualize.saveVisualizationExpectSuccess(visName); + await PageObjects.visualize.loadSavedVisualization(visName); + await PageObjects.visualize.waitForVisualization(); + await PageObjects.visualize.clickData(); + await PageObjects.visualize.toggleOpenEditor(1); + await PageObjects.visualize.setCustomLabel('test', 1); + await PageObjects.visualize.clickGo(); + await PageObjects.visualize.clickMetricsAndAxes(); + await PageObjects.visualize.clickYAxisOptions('ValueAxis-1'); + const title = await PageObjects.visualize.getYAxisTitle(); + expect(title).to.be(axisTitle); + }); + }); + }); } diff --git a/test/functional/page_objects/point_series_page.js b/test/functional/page_objects/point_series_page.js index 70bc3de97c7d2..b4f3461ed19d5 100644 --- a/test/functional/page_objects/point_series_page.js +++ b/test/functional/page_objects/point_series_page.js @@ -44,6 +44,14 @@ export function PointSeriesPageProvider({ getService }) { return await testSubjects.click('visualizeAddYAxisButton'); } + setAxisTitle(title, { index = 0 } = {}) { + return remote + .setFindTimeout(defaultFindTimeout) + .findByCssSelector(`#valueAxisTitle${index}`) + .clearValue() + .type(title); + } + getValueAxesCount() { return remote .setFindTimeout(defaultFindTimeout) diff --git a/test/functional/page_objects/visualize_page.js b/test/functional/page_objects/visualize_page.js index 268932fd08bdf..11baa13e524a8 100644 --- a/test/functional/page_objects/visualize_page.js +++ b/test/functional/page_objects/visualize_page.js @@ -605,7 +605,8 @@ export function VisualizePageProvider({ getService, getPageObjects }) { await testSubjects.click(`aggregationEditor${agg} disableAggregationBtn`); await PageObjects.header.waitUntilLoadingHasFinished(); } - async toggleAggegationEditor(agg) { + + async toggleAggregationEditor(agg) { await testSubjects.click(`aggregationEditor${agg} toggleEditor`); await PageObjects.header.waitUntilLoadingHasFinished(); } @@ -649,7 +650,6 @@ export function VisualizePageProvider({ getService, getPageObjects }) { await PageObjects.header.waitUntilLoadingHasFinished(); } - async changeHeatmapColorNumbers(value = 6) { const input = await testSubjects.find(`heatmapOptionsColorsNumberInput`); await input.clearValue(); @@ -686,6 +686,7 @@ export function VisualizePageProvider({ getService, getPageObjects }) { toCell.clearValue(); toCell.type(`${to}`); } + async clickYAxisOptions(axisId) { await testSubjects.click(`toggleYAxisOptions-${axisId}`); }