From c14cfe81436902807632ccca8087dc7ce6145378 Mon Sep 17 00:00:00 2001 From: Tim Roes Date: Fri, 6 Jul 2018 16:47:39 +0200 Subject: [PATCH 01/12] Add possibility to specify base for histogram interval --- .../agg_types/__tests__/buckets/_histogram.js | 54 +++++++++++++++++++ src/ui/public/agg_types/buckets/histogram.js | 20 ++++++- 2 files changed, 73 insertions(+), 1 deletion(-) diff --git a/src/ui/public/agg_types/__tests__/buckets/_histogram.js b/src/ui/public/agg_types/__tests__/buckets/_histogram.js index b4589ac51ea20..e1828916eccff 100644 --- a/src/ui/public/agg_types/__tests__/buckets/_histogram.js +++ b/src/ui/public/agg_types/__tests__/buckets/_histogram.js @@ -18,10 +18,13 @@ */ import expect from 'expect.js'; +import sinon from 'sinon'; import ngMock from 'ng_mock'; import { aggTypes } from '../..'; +import chrome from '../../../chrome'; import AggParamWriterProvider from '../agg_param_writer'; +const config = chrome.getUiSettingsClient(); const histogram = aggTypes.byName.histogram; describe('Histogram Agg', function () { @@ -74,6 +77,57 @@ describe('Histogram Agg', function () { const output = paramWriter.write({ interval: [] }); expect(isNaN(output.params.interval)).to.be.ok(); }); + + describe('interval scaling', () => { + + beforeEach(() => { + sinon.stub(config, 'get'); + }); + + it('will respect the histogram:maxBars setting', () => { + config.get.withArgs('histogram:maxBars').returns(5); + const output = paramWriter.write({ interval: 5 }, + aggConfig => aggConfig.setAutoBounds({ min: 0, max: 10000 })); + expect(output.params).to.have.property('interval', 2000); + }); + + it('will return specified interval, if bars are below histogram:maxBars config', () => { + config.get.withArgs('histogram:maxBars').returns(10000); + const output = paramWriter.write({ interval: 5 }, + aggConfig => aggConfig.setAutoBounds({ min: 0, max: 10000 })); + expect(output.params).to.have.property('interval', 5); + }); + + it('will set to intervalBase if interval is below base', () => { + const output = paramWriter.write({ interval: 3, intervalBase: 8 }); + expect(output.params).to.have.property('interval', 8); + }); + + it('will round to nearest intervalBase multiple if interval is above base', () => { + const roundUp = paramWriter.write({ interval: 46, intervalBase: 10 }); + expect(roundUp.params).to.have.property('interval', 50); + const roundDown = paramWriter.write({ interval: 43, intervalBase: 10 }); + expect(roundDown.params).to.have.property('interval', 40); + }); + + it('will not change interval if it is a multiple of base', () => { + const output = paramWriter.write({ interval: 35, intervalBase: 5 }); + expect(output.params).to.have.property('interval', 35); + }); + + it('will round to intervalBase after scaling histogram:maxBars', () => { + config.get.withArgs('histogram:maxBars').returns(100); + const output = paramWriter.write({ interval: 5, intervalBase: 6 }, + aggConfig => aggConfig.setAutoBounds({ min: 0, max: 1000 })); + // 100 buckets in 0 to 1000 would result in an interval of 10, so we should + // round to the next multiple of 6 -> 12 + expect(output.params).to.have.property('interval', 12); + }); + + afterEach(() => { + config.get.restore(); + }); + }); }); describe('min_doc_count', function () { diff --git a/src/ui/public/agg_types/buckets/histogram.js b/src/ui/public/agg_types/buckets/histogram.js index 7fda6537de79e..6da9c30908fdf 100644 --- a/src/ui/public/agg_types/buckets/histogram.js +++ b/src/ui/public/agg_types/buckets/histogram.js @@ -59,7 +59,14 @@ export const histogramBucketAgg = new BucketAggType({ name: 'field', filterFieldTypes: 'number' }, - + { + /* + * This parameter can be set if you want the auto scaled interval to always + * be a multiple of a specific base. + */ + name: 'intervalBase', + default: null, + }, { name: 'interval', editor: intervalTemplate, @@ -111,6 +118,17 @@ export const histogramBucketAgg = new BucketAggType({ } } + const base = aggConfig.params.intervalBase; + if (base) { + if (interval < base) { + // In case the specified interval is below the base, just increase it to it's base + interval = base; + } else if (interval % base !== 0) { + // In case the interval is not a multiple of the base round it to the next base + interval = Math.round(interval / base) * base; + } + } + output.params.interval = interval; } }, From c8d2e30a77cd5e27dacd55fd021ece532acf7ae3 Mon Sep 17 00:00:00 2001 From: Tim Roes Date: Fri, 6 Jul 2018 16:47:54 +0200 Subject: [PATCH 02/12] Move AggParamsWriter to ES6 --- .../agg_types/__tests__/agg_param_writer.js | 83 ++++++++++--------- 1 file changed, 44 insertions(+), 39 deletions(-) diff --git a/src/ui/public/agg_types/__tests__/agg_param_writer.js b/src/ui/public/agg_types/__tests__/agg_param_writer.js index bddb4a9a7f3dc..dcab1bea91771 100644 --- a/src/ui/public/agg_types/__tests__/agg_param_writer.js +++ b/src/ui/public/agg_types/__tests__/agg_param_writer.js @@ -46,55 +46,60 @@ export default function AggParamWriterHelper(Private) { * @param {object} opts - describe the properties of this paramWriter * @param {string} opts.aggType - the name of the aggType we want to test. ('histogram', 'filter', etc.) */ - function AggParamWriter(opts) { - const self = this; + class AggParamWriter { - self.aggType = opts.aggType; - if (_.isString(self.aggType)) { - self.aggType = aggTypes.byName[self.aggType]; - } + constructor(opts) { + const self = this; - // not configurable right now, but totally required - self.indexPattern = stubbedLogstashIndexPattern; + self.aggType = opts.aggType; + if (_.isString(self.aggType)) { + self.aggType = aggTypes.byName[self.aggType]; + } - // the schema that the aggType satisfies - self.visAggSchema = null; + // not configurable right now, but totally required + self.indexPattern = stubbedLogstashIndexPattern; - self.vis = new Vis(self.indexPattern, { - type: 'histogram', - aggs: [{ - id: 1, - type: self.aggType.name, - params: {} - }] - }); - } + // the schema that the aggType satisfies + self.visAggSchema = null; + + self.vis = new Vis(self.indexPattern, { + type: 'histogram', + aggs: [{ + id: 1, + type: self.aggType.name, + params: {} + }] + }); + } - AggParamWriter.prototype.write = function (paramValues) { - const self = this; - paramValues = _.clone(paramValues); + write(paramValues, modifyAggConfig = null) { + const self = this; + paramValues = _.clone(paramValues); - if (self.aggType.params.byName.field && !paramValues.field) { - // pick a field rather than force a field to be specified everywhere - if (self.aggType.type === 'metrics') { - paramValues.field = _.sample(self.indexPattern.fields.byType.number); - } else { - const type = self.aggType.params.byName.field.filterFieldTypes || 'string'; - let field; - do { - field = _.sample(self.indexPattern.fields.byType[type]); - } while (!field.aggregatable); - paramValues.field = field.name; + if (self.aggType.params.byName.field && !paramValues.field) { + // pick a field rather than force a field to be specified everywhere + if (self.aggType.type === 'metrics') { + paramValues.field = _.sample(self.indexPattern.fields.byType.number); + } else { + const type = self.aggType.params.byName.field.filterFieldTypes || 'string'; + let field; + do { + field = _.sample(self.indexPattern.fields.byType[type]); + } while (!field.aggregatable); + paramValues.field = field.name; + } } - } + const aggConfig = self.vis.aggs[0]; + aggConfig.setParams(paramValues); - const aggConfig = self.vis.aggs[0]; - aggConfig.setParams(paramValues); + if (modifyAggConfig) { + modifyAggConfig(aggConfig); + } - return aggConfig.write(self.vis.aggs); - }; + return aggConfig.write(self.vis.aggs); + } + } return AggParamWriter; - } From a54e820c20a571c0fe7716957c7ee008bb4d559f Mon Sep 17 00:00:00 2001 From: Tim Roes Date: Fri, 6 Jul 2018 16:48:15 +0200 Subject: [PATCH 03/12] Add param for time_zone in date_histogram --- .../buckets/date_histogram/_params.js | 28 +++++++++++++++++++ .../agg_types/buckets/date_histogram.js | 16 +++++------ 2 files changed, 35 insertions(+), 9 deletions(-) diff --git a/src/ui/public/agg_types/__tests__/buckets/date_histogram/_params.js b/src/ui/public/agg_types/__tests__/buckets/date_histogram/_params.js index a66b27d5493f8..ba4528110546e 100644 --- a/src/ui/public/agg_types/__tests__/buckets/date_histogram/_params.js +++ b/src/ui/public/agg_types/__tests__/buckets/date_histogram/_params.js @@ -20,13 +20,17 @@ import _ from 'lodash'; import moment from 'moment'; import expect from 'expect.js'; +import sinon from 'sinon'; import ngMock from 'ng_mock'; import AggParamWriterProvider from '../../agg_param_writer'; import FixturesStubbedLogstashIndexPatternProvider from 'fixtures/stubbed_logstash_index_pattern'; +import chrome from '../../../../chrome'; import { aggTypes } from '../../..'; import { AggConfig } from '../../../../vis/agg_config'; import { timefilter } from 'ui/timefilter'; +const config = chrome.getUiSettingsClient(); + describe('params', function () { let paramWriter; @@ -132,6 +136,30 @@ describe('params', function () { }); }); + describe('time_zone', () => { + beforeEach(() => { + sinon.stub(config, 'get'); + sinon.stub(config, 'isDefault'); + }); + + it('should use the specified time_zone', () => { + const output = paramWriter.write({ time_zone: 'Europe/Kiev' }); + expect(output.params).to.have.property('time_zone', 'Europe/Kiev'); + }); + + it('should use the Kibana time_zone if no parameter specified', () => { + config.isDefault.withArgs('dateFormat:tz').returns(false); + config.get.withArgs('dateFormat:tz').returns('Europe/Riga'); + const output = paramWriter.write({}); + expect(output.params).to.have.property('time_zone', 'Europe/Riga'); + }); + + afterEach(() => { + config.get.restore(); + config.isDefault.restore(); + }); + }); + describe('extended_bounds', function () { it('should write a long value if a moment passed in', function () { const then = moment(0); diff --git a/src/ui/public/agg_types/buckets/date_histogram.js b/src/ui/public/agg_types/buckets/date_histogram.js index 528f73fa3904e..a75304ce6e09a 100644 --- a/src/ui/public/agg_types/buckets/date_histogram.js +++ b/src/ui/public/agg_types/buckets/date_histogram.js @@ -128,13 +128,6 @@ export const dateHistogramBucketAgg = new BucketAggType({ output.bucketInterval = interval; output.params.interval = interval.expression; - const isDefaultTimezone = config.isDefault('dateFormat:tz'); - if (isDefaultTimezone) { - output.params.time_zone = detectedTimezone || tzOffset; - } else { - output.params.time_zone = config.get('dateFormat:tz'); - } - const scaleMetrics = interval.scaled && interval.scale < 1; if (scaleMetrics && aggs) { const all = _.every(aggs.bySchemaGroup.metrics, function (agg) { @@ -147,13 +140,18 @@ export const dateHistogramBucketAgg = new BucketAggType({ } } }, - + { + name: 'time_zone', + default: () => { + const isDefaultTimezone = config.isDefault('dateFormat:tz'); + return isDefaultTimezone ? detectedTimezone || tzOffset : config.get('dateFormat:tz'); + }, + }, { name: 'customInterval', default: '2h', write: _.noop }, - { name: 'format' }, From 6b45b7030e3a8cb14aa57377741923e23702beca Mon Sep 17 00:00:00 2001 From: Tim Roes Date: Wed, 11 Jul 2018 14:25:03 +0200 Subject: [PATCH 04/12] Prevent writing intervalBase to DSL --- src/ui/public/agg_types/buckets/histogram.js | 1 + 1 file changed, 1 insertion(+) diff --git a/src/ui/public/agg_types/buckets/histogram.js b/src/ui/public/agg_types/buckets/histogram.js index 6da9c30908fdf..e301f3a59a265 100644 --- a/src/ui/public/agg_types/buckets/histogram.js +++ b/src/ui/public/agg_types/buckets/histogram.js @@ -66,6 +66,7 @@ export const histogramBucketAgg = new BucketAggType({ */ name: 'intervalBase', default: null, + write: () => {}, }, { name: 'interval', From a3c4c8e0c1d159372b8579b75b5c642715a8ca06 Mon Sep 17 00:00:00 2001 From: Tim Roes Date: Wed, 11 Jul 2018 14:28:11 +0200 Subject: [PATCH 05/12] Add basic EditorConfig providers --- .../agg_types/controls/number_interval.html | 3 +- .../editors/config/editor_config_providers.ts | 53 +++++++++++++++++++ src/ui/public/vis/editors/config/types.ts | 27 ++++++++++ .../public/vis/editors/default/agg_params.js | 46 ++++++++++++++++ 4 files changed, 128 insertions(+), 1 deletion(-) create mode 100644 src/ui/public/vis/editors/config/editor_config_providers.ts create mode 100644 src/ui/public/vis/editors/config/types.ts diff --git a/src/ui/public/agg_types/controls/number_interval.html b/src/ui/public/agg_types/controls/number_interval.html index 2b9730a7f7d75..809d7f4e40c43 100644 --- a/src/ui/public/agg_types/controls/number_interval.html +++ b/src/ui/public/agg_types/controls/number_interval.html @@ -14,7 +14,8 @@ type="number" class="form-control" name="interval" - min="0" + min="{{editorConfig.interval.base || 0}}" + step="{{editorConfig.interval.base}}" input-number > diff --git a/src/ui/public/vis/editors/config/editor_config_providers.ts b/src/ui/public/vis/editors/config/editor_config_providers.ts new file mode 100644 index 0000000000000..9b5909f0668d2 --- /dev/null +++ b/src/ui/public/vis/editors/config/editor_config_providers.ts @@ -0,0 +1,53 @@ +/* + * 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 { AggConfig } from '../..'; +import { AggType } from '../../../agg_types'; +import { IndexPattern } from '../../../index_patterns'; +import { EditorConfig } from './types'; + +type EditorConfigProvider = ( + aggType: AggType, + indexPattern: IndexPattern, + aggConfig: AggConfig +) => EditorConfig; + +class EditorConfigProviderRegistry { + private providers: Set = new Set(); + + public register(configProvider: EditorConfigProvider): void { + this.providers.add(configProvider); + } + + public getConfigForAgg( + aggType: AggType, + indexPattern: IndexPattern, + aggConfig: AggConfig + ): EditorConfig { + const configs = Array.from(this.providers).map(provider => + provider(aggType, indexPattern, aggConfig) + ); + // TODO: merge configs in a reasonable way + return configs[0]; + } +} + +const editorConfigProviders = new EditorConfigProviderRegistry(); + +export { editorConfigProviders }; diff --git a/src/ui/public/vis/editors/config/types.ts b/src/ui/public/vis/editors/config/types.ts new file mode 100644 index 0000000000000..fb8041a6b15e7 --- /dev/null +++ b/src/ui/public/vis/editors/config/types.ts @@ -0,0 +1,27 @@ +/* + * 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. + */ + +interface EditorParamConfig { + fixedValue?: any; + hidden?: boolean; +} + +export interface EditorConfig { + [paramName: string]: EditorParamConfig; +} diff --git a/src/ui/public/vis/editors/default/agg_params.js b/src/ui/public/vis/editors/default/agg_params.js index 7314a37ced0dd..753925ad4bc01 100644 --- a/src/ui/public/vis/editors/default/agg_params.js +++ b/src/ui/public/vis/editors/default/agg_params.js @@ -28,6 +28,29 @@ import { uiModules } from '../../../modules'; import { documentationLinks } from '../../../documentation_links/documentation_links'; import aggParamsTemplate from './agg_params.html'; import { aggTypeFilters } from '../../../agg_types/filter'; +import { editorConfigProviders } from '../config/editor_config_providers'; + +// TODO: This is just an example how to use this, and will be removed later. +// editorConfigProviders.register((aggType, indexPattern, aggConfig) => { +// if (aggConfig.params.field && aggConfig.params.field.name === 'bytes') { +// return { +// 'intervalBase': { +// fixedValue: 50 +// }, +// 'interval': { +// base: 50 +// } +// }; +// } +// if (aggConfig.type && aggConfig.type.name === 'date_histogram') { +// return { +// 'time_zone': { +// fixedValue: 'UTC' +// } +// }; +// } +// return {}; +// }); uiModules .get('app/visualize') @@ -51,6 +74,28 @@ uiModules // there is a possibility that the agg type can be automatically selected (if there is only one) $scope.$watch('agg.type', updateAggParamEditor); + function updateEditorConfig() { + $scope.editorConfig = editorConfigProviders.getConfigForAgg( + aggTypes.byType[$scope.groupName], + $scope.indexPattern, + $scope.agg + ); + + Object.keys($scope.editorConfig).forEach(param => { + const config = $scope.editorConfig[param]; + // If the parameter has a fixed value in the config, set this value. + // Also for all supported configs we should freeze the editor for this param. + if (config.hasOwnProperty('fixedValue')) { + $scope.agg.params[param] = config.fixedValue; + } + // TODO: process more editor config? + }); + } + + $scope.$watchCollection('agg.params', updateEditorConfig); + + updateEditorConfig(); + // this will contain the controls for the schema (rows or columns?), which are unrelated to // controls for the agg, which is why they are first addSchemaEditor(); @@ -77,6 +122,7 @@ uiModules let $aggParamEditorsScope; function updateAggParamEditor() { + updateEditorConfig(); $scope.aggHelpLink = null; if (_.has($scope, 'agg.type.name')) { $scope.aggHelpLink = _.get(documentationLinks, ['aggs', $scope.agg.type.name]); From 31bd9e3ea8493920205e8c2adea45a60726e2a03 Mon Sep 17 00:00:00 2001 From: Tim Roes Date: Thu, 12 Jul 2018 10:11:24 +0200 Subject: [PATCH 06/12] Merge multiple configs together --- .../editors/config/editor_config_providers.ts | 41 ++++++++++++++++++- src/ui/public/vis/editors/config/types.ts | 1 + 2 files changed, 40 insertions(+), 2 deletions(-) diff --git a/src/ui/public/vis/editors/config/editor_config_providers.ts b/src/ui/public/vis/editors/config/editor_config_providers.ts index 9b5909f0668d2..62be12208cea5 100644 --- a/src/ui/public/vis/editors/config/editor_config_providers.ts +++ b/src/ui/public/vis/editors/config/editor_config_providers.ts @@ -28,6 +28,14 @@ type EditorConfigProvider = ( aggConfig: AggConfig ) => EditorConfig; +function greatestCommonDivisor(a: number, b: number): number { + return a === 0 ? b : greatestCommonDivisor(b % a, a); +} + +function leastCommonMultiple(a: number, b: number) { + return a * b / greatestCommonDivisor(a, b); +} + class EditorConfigProviderRegistry { private providers: Set = new Set(); @@ -43,8 +51,37 @@ class EditorConfigProviderRegistry { const configs = Array.from(this.providers).map(provider => provider(aggType, indexPattern, aggConfig) ); - // TODO: merge configs in a reasonable way - return configs[0]; + return this.mergeConfigs(configs); + } + + private mergeConfigs(configs: EditorConfig[]): EditorConfig { + return configs.reduce((output, conf) => { + Object.entries(conf).forEach(([paramName, paramConfig]) => { + if (!output[paramName]) { + // No other config had anything configured for that param, just + // use the whole config for that param as it is. + output[paramName] = paramConfig; + } else { + // Another config already had already configured something, so let's merge that + // If one config set it to hidden, we'll hide the param. + output[paramName].hidden = output[paramName].hidden || paramConfig.hidden; + + // In case a base is defined either set it (if no previous base) + // has been configured for that param or otherwise find the least common multiple + if (paramConfig.base) { + const previousBase = output[paramName].base; + output[paramName].base = + previousBase !== undefined + ? leastCommonMultiple(previousBase, paramConfig.base) + : output[paramName].base; + } + + // TODO: What to do for multiple fixedValues + output[paramName].fixedValue = paramConfig.fixedValue; + } + }); + return output; + }, {}); } } diff --git a/src/ui/public/vis/editors/config/types.ts b/src/ui/public/vis/editors/config/types.ts index fb8041a6b15e7..aa01e1682cb5e 100644 --- a/src/ui/public/vis/editors/config/types.ts +++ b/src/ui/public/vis/editors/config/types.ts @@ -20,6 +20,7 @@ interface EditorParamConfig { fixedValue?: any; hidden?: boolean; + base?: number; } export interface EditorConfig { From 5b3eeed8bd279a73430de96348bb56d768958020 Mon Sep 17 00:00:00 2001 From: Tim Roes Date: Thu, 12 Jul 2018 10:22:38 +0200 Subject: [PATCH 07/12] Allow hiding parameters --- .../public/vis/editors/default/agg_params.js | 56 ++++++++++--------- 1 file changed, 29 insertions(+), 27 deletions(-) diff --git a/src/ui/public/vis/editors/default/agg_params.js b/src/ui/public/vis/editors/default/agg_params.js index 753925ad4bc01..6a3dc6bea940f 100644 --- a/src/ui/public/vis/editors/default/agg_params.js +++ b/src/ui/public/vis/editors/default/agg_params.js @@ -18,7 +18,7 @@ */ import $ from 'jquery'; -import _ from 'lodash'; +import { has, get } from 'lodash'; import aggSelectHtml from './agg_select.html'; import advancedToggleHtml from './advanced_toggle.html'; import '../../../filters/match_any'; @@ -124,8 +124,8 @@ uiModules function updateAggParamEditor() { updateEditorConfig(); $scope.aggHelpLink = null; - if (_.has($scope, 'agg.type.name')) { - $scope.aggHelpLink = _.get(documentationLinks, ['aggs', $scope.agg.type.name]); + if (has($scope, 'agg.type.name')) { + $scope.aggHelpLink = get(documentationLinks, ['aggs', $scope.agg.type.name]); } if ($aggParamEditors) { @@ -152,36 +152,38 @@ uiModules }; // build collection of agg params html - $scope.agg.type.params.forEach(function (param, i) { - let aggParam; - let fields; - if ($scope.agg.schema.hideCustomLabel && param.name === 'customLabel') { - return; - } - // if field param exists, compute allowed fields - if (param.name === 'field') { - fields = $aggParamEditorsScope.indexedFields; - } else if (param.type === 'field') { - fields = $aggParamEditorsScope[`${param.name}Options`] = param.getFieldOptions($scope.agg); - } - - if (fields) { - const hasIndexedFields = fields.length > 0; - const isExtraParam = i > 0; - if (!hasIndexedFields && isExtraParam) { // don't draw the rest of the options if there are no indexed fields. + $scope.agg.type.params + .filter(param => !get($scope, ['editorConfig', param.name, 'hidden'], false)) + .forEach(function (param, i) { + let aggParam; + let fields; + if ($scope.agg.schema.hideCustomLabel && param.name === 'customLabel') { return; } - } + // if field param exists, compute allowed fields + if (param.name === 'field') { + fields = $aggParamEditorsScope.indexedFields; + } else if (param.type === 'field') { + fields = $aggParamEditorsScope[`${param.name}Options`] = param.getFieldOptions($scope.agg); + } + if (fields) { + const hasIndexedFields = fields.length > 0; + const isExtraParam = i > 0; + if (!hasIndexedFields && isExtraParam) { // don't draw the rest of the options if there are no indexed fields. + return; + } + } - let type = 'basic'; - if (param.advanced) type = 'advanced'; - if (aggParam = getAggParamHTML(param, i)) { - aggParamHTML[type].push(aggParam); - } + let type = 'basic'; + if (param.advanced) type = 'advanced'; - }); + if (aggParam = getAggParamHTML(param, i)) { + aggParamHTML[type].push(aggParam); + } + + }); // compile the paramEditors html elements let paramEditors = aggParamHTML.basic; From a44b1b51e1de189a05baacd97e9f30f27a166c69 Mon Sep 17 00:00:00 2001 From: Tim Roes Date: Mon, 6 Aug 2018 18:18:40 +0200 Subject: [PATCH 08/12] Improve config merging and add tests --- .../config/editor_config_providers.test.ts | 134 ++++++++++++++++++ .../editors/config/editor_config_providers.ts | 85 ++++++++--- src/ui/public/vis/editors/config/types.ts | 27 +++- 3 files changed, 221 insertions(+), 25 deletions(-) create mode 100644 src/ui/public/vis/editors/config/editor_config_providers.test.ts diff --git a/src/ui/public/vis/editors/config/editor_config_providers.test.ts b/src/ui/public/vis/editors/config/editor_config_providers.test.ts new file mode 100644 index 0000000000000..7b0c60c2a3125 --- /dev/null +++ b/src/ui/public/vis/editors/config/editor_config_providers.test.ts @@ -0,0 +1,134 @@ +/* + * 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 { EditorConfigProviderRegistry } from './editor_config_providers'; +import { EditorParamConfig, FixedParam, NumericIntervalParam } from './types'; + +describe('EditorConfigProvider', () => { + let registry: EditorConfigProviderRegistry; + + beforeEach(() => { + registry = new EditorConfigProviderRegistry(); + }); + + it('should call registered providers with given parameters', () => { + const provider = jest.fn(() => ({})); + registry.register(provider); + expect(provider).not.toHaveBeenCalled(); + const aggType = {}; + const indexPattern = {}; + const aggConfig = {}; + registry.getConfigForAgg(aggType, indexPattern, aggConfig); + expect(provider).toHaveBeenCalledWith(aggType, indexPattern, aggConfig); + }); + + it('should call all registered providers with given parameters', () => { + const provider = jest.fn(() => ({})); + const provider2 = jest.fn(() => ({})); + registry.register(provider); + registry.register(provider2); + expect(provider).not.toHaveBeenCalled(); + expect(provider2).not.toHaveBeenCalled(); + const aggType = {}; + const indexPattern = {}; + const aggConfig = {}; + registry.getConfigForAgg(aggType, indexPattern, aggConfig); + expect(provider).toHaveBeenCalledWith(aggType, indexPattern, aggConfig); + expect(provider2).toHaveBeenCalledWith(aggType, indexPattern, aggConfig); + }); + + describe('merging configs', () => { + function singleConfig(paramConfig: EditorParamConfig) { + return () => ({ singleParam: paramConfig }); + } + + function getOutputConfig(reg: EditorConfigProviderRegistry) { + return reg.getConfigForAgg({}, {}, {}).singleParam; + } + + it('should have hidden true if at least one config was hidden true', () => { + registry.register(singleConfig({ hidden: false })); + registry.register(singleConfig({ hidden: true })); + registry.register(singleConfig({ hidden: false })); + const config = getOutputConfig(registry); + expect(config.hidden).toBe(true); + }); + + it('should merge the same fixed values', () => { + registry.register(singleConfig({ fixedValue: 'foo' })); + registry.register(singleConfig({ fixedValue: 'foo' })); + const config = getOutputConfig(registry) as FixedParam; + expect(config).toHaveProperty('fixedValue'); + expect(config.fixedValue).toBe('foo'); + }); + + it('should throw having different fixed values', () => { + registry.register(singleConfig({ fixedValue: 'foo' })); + registry.register(singleConfig({ fixedValue: 'bar' })); + expect(() => { + getOutputConfig(registry); + }).toThrowError(); + }); + + it('should allow same base values', () => { + registry.register(singleConfig({ base: 5 })); + registry.register(singleConfig({ base: 5 })); + const config = getOutputConfig(registry) as NumericIntervalParam; + expect(config).toHaveProperty('base'); + expect(config.base).toBe(5); + }); + + it('should merge multiple base values, using least common multiple', () => { + registry.register(singleConfig({ base: 2 })); + registry.register(singleConfig({ base: 5 })); + registry.register(singleConfig({ base: 8 })); + const config = getOutputConfig(registry) as NumericIntervalParam; + expect(config).toHaveProperty('base'); + expect(config.base).toBe(40); + }); + + it('should throw on combinding fixedValue with base', () => { + registry.register(singleConfig({ fixedValue: 'foo' })); + registry.register(singleConfig({ base: 5 })); + expect(() => { + getOutputConfig(registry); + }).toThrowError(); + }); + + it('should merge hidden together with fixedValue', () => { + registry.register(singleConfig({ fixedValue: 'foo', hidden: true })); + registry.register(singleConfig({ fixedValue: 'foo', hidden: false })); + const config = getOutputConfig(registry) as FixedParam; + expect(config).toHaveProperty('fixedValue'); + expect(config).toHaveProperty('hidden'); + expect(config.fixedValue).toBe('foo'); + expect(config.hidden).toBe(true); + }); + + it('should merge hidden together with base', () => { + registry.register(singleConfig({ base: 2, hidden: false })); + registry.register(singleConfig({ base: 13, hidden: false })); + const config = getOutputConfig(registry) as NumericIntervalParam; + expect(config).toHaveProperty('base'); + expect(config).toHaveProperty('hidden'); + expect(config.base).toBe(26); + expect(config.hidden).toBe(false); + }); + }); +}); diff --git a/src/ui/public/vis/editors/config/editor_config_providers.ts b/src/ui/public/vis/editors/config/editor_config_providers.ts index 62be12208cea5..9d53393d57272 100644 --- a/src/ui/public/vis/editors/config/editor_config_providers.ts +++ b/src/ui/public/vis/editors/config/editor_config_providers.ts @@ -20,7 +20,7 @@ import { AggConfig } from '../..'; import { AggType } from '../../../agg_types'; import { IndexPattern } from '../../../index_patterns'; -import { EditorConfig } from './types'; +import { EditorConfig, EditorParamConfig, FixedParam, NumericIntervalParam } from './types'; type EditorConfigProvider = ( aggType: AggType, @@ -33,7 +33,7 @@ function greatestCommonDivisor(a: number, b: number): number { } function leastCommonMultiple(a: number, b: number) { - return a * b / greatestCommonDivisor(a, b); + return (a * b) / greatestCommonDivisor(a, b); } class EditorConfigProviderRegistry { @@ -54,30 +54,73 @@ class EditorConfigProviderRegistry { return this.mergeConfigs(configs); } + private isBaseParam(config: EditorParamConfig): config is NumericIntervalParam { + return config.hasOwnProperty('base'); + } + + private isFixedParam(config: EditorParamConfig): config is FixedParam { + return config.hasOwnProperty('fixedValue'); + } + + private mergeHidden(current: EditorParamConfig, merged: EditorParamConfig): boolean { + return Boolean(current.hidden || merged.hidden); + } + + private mergeFixedAndBase( + current: EditorParamConfig, + merged: EditorParamConfig, + paramName: string + ): { fixedValue?: any; base?: number } { + if ( + this.isFixedParam(current) && + this.isFixedParam(merged) && + current.fixedValue !== merged.fixedValue + ) { + // In case multiple configurations provided a fixedValue, these must all be the same. + // If not we'll throw an error. + throw new Error(`Two EditorConfigProviders provided different fixed values for field ${paramName}: + ${merged.fixedValue} !== ${current.fixedValue}`); + } else if ( + (this.isFixedParam(current) && this.isBaseParam(merged)) || + (this.isBaseParam(current) && this.isFixedParam(merged)) + ) { + // In case one config tries to set a fixed value and another setting a base value, + // we'll throw an error. This could be solved more elegantly, by allowing fixedValues + // that are the multiple of the specific base value, but since there is no use-case for that + // right now, this isn't implemented. + throw new Error(`Tried to provide a fixedValue and a base for param ${paramName}.`); + } else if (this.isBaseParam(current) && this.isBaseParam(merged)) { + // In case both had where interval values, just use the least common multiple between both interval + return { + base: leastCommonMultiple(current.base, merged.base), + }; + } else { + // In this case we haven't had a fixed value of base for that param yet, we use the one specified + // in the current config + if (this.isFixedParam(current)) { + return { + fixedValue: current.fixedValue, + }; + } else if (this.isBaseParam(current)) { + return { + base: current.base, + }; + } + + return {}; + } + } + private mergeConfigs(configs: EditorConfig[]): EditorConfig { return configs.reduce((output, conf) => { Object.entries(conf).forEach(([paramName, paramConfig]) => { if (!output[paramName]) { - // No other config had anything configured for that param, just - // use the whole config for that param as it is. output[paramName] = paramConfig; } else { - // Another config already had already configured something, so let's merge that - // If one config set it to hidden, we'll hide the param. - output[paramName].hidden = output[paramName].hidden || paramConfig.hidden; - - // In case a base is defined either set it (if no previous base) - // has been configured for that param or otherwise find the least common multiple - if (paramConfig.base) { - const previousBase = output[paramName].base; - output[paramName].base = - previousBase !== undefined - ? leastCommonMultiple(previousBase, paramConfig.base) - : output[paramName].base; - } - - // TODO: What to do for multiple fixedValues - output[paramName].fixedValue = paramConfig.fixedValue; + output[paramName] = { + hidden: this.mergeHidden(paramConfig, output[paramName]), + ...this.mergeFixedAndBase(paramConfig, output[paramName], paramName), + }; } }); return output; @@ -87,4 +130,4 @@ class EditorConfigProviderRegistry { const editorConfigProviders = new EditorConfigProviderRegistry(); -export { editorConfigProviders }; +export { editorConfigProviders, EditorConfigProviderRegistry }; diff --git a/src/ui/public/vis/editors/config/types.ts b/src/ui/public/vis/editors/config/types.ts index aa01e1682cb5e..f25c93ebab105 100644 --- a/src/ui/public/vis/editors/config/types.ts +++ b/src/ui/public/vis/editors/config/types.ts @@ -17,12 +17,31 @@ * under the License. */ -interface EditorParamConfig { - fixedValue?: any; - hidden?: boolean; - base?: number; +/** + * A hidden parameter can be hidden from the UI completely. + */ +interface HiddenParam { + hidden: boolean; } +/** + * A fixed parameter has a fixed value for a specific field. + * It can optionally also be hidden. + */ +export type FixedParam = Partial & { + fixedValue: any; +}; + +/** + * Numeric interval parameters must always be set in the editor to a multiple of + * the specified base. It can optionally also be hidden. + */ +export type NumericIntervalParam = Partial & { + base: number; +}; + +export type EditorParamConfig = NumericIntervalParam | FixedParam | HiddenParam; + export interface EditorConfig { [paramName: string]: EditorParamConfig; } From 7302f58ae4c86ae99b99b1fee1c09d0835f44cce Mon Sep 17 00:00:00 2001 From: Tim Roes Date: Mon, 6 Aug 2018 18:29:12 +0200 Subject: [PATCH 09/12] Remove TODO --- .../public/vis/editors/default/agg_params.js | 22 ------------------- 1 file changed, 22 deletions(-) diff --git a/src/ui/public/vis/editors/default/agg_params.js b/src/ui/public/vis/editors/default/agg_params.js index 6a3dc6bea940f..8d9fd8ea59d43 100644 --- a/src/ui/public/vis/editors/default/agg_params.js +++ b/src/ui/public/vis/editors/default/agg_params.js @@ -30,28 +30,6 @@ import aggParamsTemplate from './agg_params.html'; import { aggTypeFilters } from '../../../agg_types/filter'; import { editorConfigProviders } from '../config/editor_config_providers'; -// TODO: This is just an example how to use this, and will be removed later. -// editorConfigProviders.register((aggType, indexPattern, aggConfig) => { -// if (aggConfig.params.field && aggConfig.params.field.name === 'bytes') { -// return { -// 'intervalBase': { -// fixedValue: 50 -// }, -// 'interval': { -// base: 50 -// } -// }; -// } -// if (aggConfig.type && aggConfig.type.name === 'date_histogram') { -// return { -// 'time_zone': { -// fixedValue: 'UTC' -// } -// }; -// } -// return {}; -// }); - uiModules .get('app/visualize') .directive('visEditorAggParams', function ($compile) { From f2356856d600ea9d3bbfd9b5ca38caaebb91b0fe Mon Sep 17 00:00:00 2001 From: Tim Roes Date: Tue, 7 Aug 2018 12:35:32 +0200 Subject: [PATCH 10/12] Implement review feedback --- .../agg_types/__tests__/buckets/_histogram.js | 7 ++ src/ui/public/utils/math.test.ts | 66 +++++++++++++++++++ src/ui/public/utils/math.ts | 42 ++++++++++++ .../config/editor_config_providers.test.ts | 2 +- .../editors/config/editor_config_providers.ts | 49 +++++++------- .../public/vis/editors/default/agg_params.js | 2 +- 6 files changed, 141 insertions(+), 27 deletions(-) create mode 100644 src/ui/public/utils/math.test.ts create mode 100644 src/ui/public/utils/math.ts diff --git a/src/ui/public/agg_types/__tests__/buckets/_histogram.js b/src/ui/public/agg_types/__tests__/buckets/_histogram.js index e1828916eccff..02b33cf7059eb 100644 --- a/src/ui/public/agg_types/__tests__/buckets/_histogram.js +++ b/src/ui/public/agg_types/__tests__/buckets/_histogram.js @@ -49,6 +49,13 @@ describe('Histogram Agg', function () { paramWriter = new AggParamWriter({ aggType: 'histogram' }); })); + describe('intervalBase', () => { + it('should not be written to the DSL', () => { + const output = paramWriter.write({ intervalBase: 100 }); + expect(output.params).not.to.have.property('intervalBase'); + }); + }); + describe('interval', function () { // reads aggConfig.params.interval, writes to dsl.interval diff --git a/src/ui/public/utils/math.test.ts b/src/ui/public/utils/math.test.ts new file mode 100644 index 0000000000000..13f090e77647b --- /dev/null +++ b/src/ui/public/utils/math.test.ts @@ -0,0 +1,66 @@ +/* + * 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 { greatestCommonDivisor, leastCommonMultiple } from './math'; + +describe('math utils', () => { + describe('greatestCommonDivisor', () => { + const tests: Array<[number, number, number]> = [ + [3, 5, 1], + [30, 36, 6], + [5, 1, 1], + [9, 9, 9], + [40, 20, 20], + [3, 0, 3], + [0, 5, 5], + [0, 0, 0], + [-9, -3, 3], + [-24, 8, 8], + [22, -7, 1], + ]; + + tests.map(([a, b, expected]) => { + it(`should return ${expected} for greatestCommonDivisor(${a}, ${b})`, () => { + expect(greatestCommonDivisor(a, b)).toBe(expected); + }); + }); + }); + + describe('leastCommonMultiple', () => { + const tests: Array<[number, number, number]> = [ + [3, 5, 15], + [1, 1, 1], + [5, 6, 30], + [3, 9, 9], + [8, 20, 40], + [5, 5, 5], + [0, 5, 0], + [-4, -5, 20], + [-2, -3, 6], + [-8, 2, 8], + [-8, 5, 40], + ]; + + tests.map(([a, b, expected]) => { + it(`should return ${expected} for leastCommonMultiple(${a}, ${b})`, () => { + expect(leastCommonMultiple(a, b)).toBe(expected); + }); + }); + }); +}); diff --git a/src/ui/public/utils/math.ts b/src/ui/public/utils/math.ts new file mode 100644 index 0000000000000..e0cab4236e2b8 --- /dev/null +++ b/src/ui/public/utils/math.ts @@ -0,0 +1,42 @@ +/* + * 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. + */ + +/** + * Calculates the greates common divisor of two numbers. This will be the + * greatest positive integer number, that both input values share as a divisor. + * + * This method does not properly work for fractional (non integer) numbers. If you + * pass in fractional numbers there usually will be an output, but that's not necessarily + * the greatest common divisor of those two numbers. + */ +export function greatestCommonDivisor(a: number, b: number): number { + return a === 0 ? Math.abs(b) : greatestCommonDivisor(b % a, a); +} + +/** + * Calculates the least common multiple of two numbers. The least common multiple + * is the smallest positive integer number, that is divisible by both input parameters. + * + * Since this calculation suffers from rounding issues in decimal values, this method + * won't work for passing in fractional (non integer) numbers. It will return a value, + * but that value won't necessarily be the mathematical correct least common multiple. + */ +export function leastCommonMultiple(a: number, b: number): number { + return Math.abs((a * b) / greatestCommonDivisor(a, b)); +} diff --git a/src/ui/public/vis/editors/config/editor_config_providers.test.ts b/src/ui/public/vis/editors/config/editor_config_providers.test.ts index 7b0c60c2a3125..c0d1bafba4659 100644 --- a/src/ui/public/vis/editors/config/editor_config_providers.test.ts +++ b/src/ui/public/vis/editors/config/editor_config_providers.test.ts @@ -103,7 +103,7 @@ describe('EditorConfigProvider', () => { expect(config.base).toBe(40); }); - it('should throw on combinding fixedValue with base', () => { + it('should throw on combining fixedValue with base', () => { registry.register(singleConfig({ fixedValue: 'foo' })); registry.register(singleConfig({ base: 5 })); expect(() => { diff --git a/src/ui/public/vis/editors/config/editor_config_providers.ts b/src/ui/public/vis/editors/config/editor_config_providers.ts index 9d53393d57272..4c8623d34b1f1 100644 --- a/src/ui/public/vis/editors/config/editor_config_providers.ts +++ b/src/ui/public/vis/editors/config/editor_config_providers.ts @@ -20,6 +20,7 @@ import { AggConfig } from '../..'; import { AggType } from '../../../agg_types'; import { IndexPattern } from '../../../index_patterns'; +import { leastCommonMultiple } from '../../../utils/math'; import { EditorConfig, EditorParamConfig, FixedParam, NumericIntervalParam } from './types'; type EditorConfigProvider = ( @@ -28,14 +29,6 @@ type EditorConfigProvider = ( aggConfig: AggConfig ) => EditorConfig; -function greatestCommonDivisor(a: number, b: number): number { - return a === 0 ? b : greatestCommonDivisor(b % a, a); -} - -function leastCommonMultiple(a: number, b: number) { - return (a * b) / greatestCommonDivisor(a, b); -} - class EditorConfigProviderRegistry { private providers: Set = new Set(); @@ -80,7 +73,9 @@ class EditorConfigProviderRegistry { // If not we'll throw an error. throw new Error(`Two EditorConfigProviders provided different fixed values for field ${paramName}: ${merged.fixedValue} !== ${current.fixedValue}`); - } else if ( + } + + if ( (this.isFixedParam(current) && this.isBaseParam(merged)) || (this.isBaseParam(current) && this.isFixedParam(merged)) ) { @@ -89,33 +84,37 @@ class EditorConfigProviderRegistry { // that are the multiple of the specific base value, but since there is no use-case for that // right now, this isn't implemented. throw new Error(`Tried to provide a fixedValue and a base for param ${paramName}.`); - } else if (this.isBaseParam(current) && this.isBaseParam(merged)) { + } + + if (this.isBaseParam(current) && this.isBaseParam(merged)) { // In case both had where interval values, just use the least common multiple between both interval return { base: leastCommonMultiple(current.base, merged.base), }; - } else { - // In this case we haven't had a fixed value of base for that param yet, we use the one specified - // in the current config - if (this.isFixedParam(current)) { - return { - fixedValue: current.fixedValue, - }; - } else if (this.isBaseParam(current)) { - return { - base: current.base, - }; - } - - return {}; } + + // In this case we haven't had a fixed value of base for that param yet, we use the one specified + // in the current config + if (this.isFixedParam(current)) { + return { + fixedValue: current.fixedValue, + }; + } + + if (this.isBaseParam(current)) { + return { + base: current.base, + }; + } + + return {}; } private mergeConfigs(configs: EditorConfig[]): EditorConfig { return configs.reduce((output, conf) => { Object.entries(conf).forEach(([paramName, paramConfig]) => { if (!output[paramName]) { - output[paramName] = paramConfig; + output[paramName] = { ...paramConfig }; } else { output[paramName] = { hidden: this.mergeHidden(paramConfig, output[paramName]), diff --git a/src/ui/public/vis/editors/default/agg_params.js b/src/ui/public/vis/editors/default/agg_params.js index 8d9fd8ea59d43..d89e7e3944818 100644 --- a/src/ui/public/vis/editors/default/agg_params.js +++ b/src/ui/public/vis/editors/default/agg_params.js @@ -66,7 +66,6 @@ uiModules if (config.hasOwnProperty('fixedValue')) { $scope.agg.params[param] = config.fixedValue; } - // TODO: process more editor config? }); } @@ -131,6 +130,7 @@ uiModules // build collection of agg params html $scope.agg.type.params + // Filter out, i.e. don't render, any parameter that is hidden via the editor config. .filter(param => !get($scope, ['editorConfig', param.name, 'hidden'], false)) .forEach(function (param, i) { let aggParam; From d8137b19310535bfcd92ca978d88e69c1548ff88 Mon Sep 17 00:00:00 2001 From: Tim Roes Date: Wed, 8 Aug 2018 13:36:04 +0200 Subject: [PATCH 11/12] Add warning to parameter --- src/ui/public/agg_types/controls/number_interval.html | 9 +++++++++ src/ui/public/react_components.js | 2 +- .../editors/config/editor_config_providers.test.ts | 8 ++++++++ .../vis/editors/config/editor_config_providers.ts | 9 +++++++++ src/ui/public/vis/editors/config/types.ts | 11 ++++++----- 5 files changed, 33 insertions(+), 6 deletions(-) diff --git a/src/ui/public/agg_types/controls/number_interval.html b/src/ui/public/agg_types/controls/number_interval.html index 809d7f4e40c43..f0283d614cf16 100644 --- a/src/ui/public/agg_types/controls/number_interval.html +++ b/src/ui/public/agg_types/controls/number_interval.html @@ -6,6 +6,15 @@ position="'right'" content="'Interval will be automatically scaled in the event that the provided value creates more buckets than specified by Advanced Setting\'s histogram:maxBars'" > + + reactDirective(EuiIcon)); app.directive('colorPicker', reactDirective => reactDirective(EuiColorPicker)); -app.directive('iconTip', reactDirective => reactDirective(EuiIconTip, ['content', 'type', 'position', 'title'])); +app.directive('iconTip', reactDirective => reactDirective(EuiIconTip, ['content', 'type', 'position', 'title', 'color'])); app.directive('callOut', reactDirective => reactDirective(EuiCallOut, ['title', 'color', 'size', 'iconType', 'children'])); diff --git a/src/ui/public/vis/editors/config/editor_config_providers.test.ts b/src/ui/public/vis/editors/config/editor_config_providers.test.ts index c0d1bafba4659..ab0b8b10718b1 100644 --- a/src/ui/public/vis/editors/config/editor_config_providers.test.ts +++ b/src/ui/public/vis/editors/config/editor_config_providers.test.ts @@ -130,5 +130,13 @@ describe('EditorConfigProvider', () => { expect(config.base).toBe(26); expect(config.hidden).toBe(false); }); + + it('should merge warnings together into one string', () => { + registry.register(singleConfig({ warning: 'Warning' })); + registry.register(singleConfig({ warning: 'Another warning' })); + const config = getOutputConfig(registry); + expect(config).toHaveProperty('warning'); + expect(config.warning).toBe('Warning\n\nAnother warning'); + }); }); }); diff --git a/src/ui/public/vis/editors/config/editor_config_providers.ts b/src/ui/public/vis/editors/config/editor_config_providers.ts index 4c8623d34b1f1..d11144ab0ae61 100644 --- a/src/ui/public/vis/editors/config/editor_config_providers.ts +++ b/src/ui/public/vis/editors/config/editor_config_providers.ts @@ -59,6 +59,14 @@ class EditorConfigProviderRegistry { return Boolean(current.hidden || merged.hidden); } + private mergeWarning(current: EditorParamConfig, merged: EditorParamConfig): string | undefined { + if (!current.warning) { + return merged.warning; + } + + return merged.warning ? `${merged.warning}\n\n${current.warning}` : current.warning; + } + private mergeFixedAndBase( current: EditorParamConfig, merged: EditorParamConfig, @@ -118,6 +126,7 @@ class EditorConfigProviderRegistry { } else { output[paramName] = { hidden: this.mergeHidden(paramConfig, output[paramName]), + warning: this.mergeWarning(paramConfig, output[paramName]), ...this.mergeFixedAndBase(paramConfig, output[paramName], paramName), }; } diff --git a/src/ui/public/vis/editors/config/types.ts b/src/ui/public/vis/editors/config/types.ts index f25c93ebab105..e8c49232c878f 100644 --- a/src/ui/public/vis/editors/config/types.ts +++ b/src/ui/public/vis/editors/config/types.ts @@ -20,15 +20,16 @@ /** * A hidden parameter can be hidden from the UI completely. */ -interface HiddenParam { - hidden: boolean; +interface Param { + hidden?: boolean; + warning?: string; } /** * A fixed parameter has a fixed value for a specific field. * It can optionally also be hidden. */ -export type FixedParam = Partial & { +export type FixedParam = Partial & { fixedValue: any; }; @@ -36,11 +37,11 @@ export type FixedParam = Partial & { * Numeric interval parameters must always be set in the editor to a multiple of * the specified base. It can optionally also be hidden. */ -export type NumericIntervalParam = Partial & { +export type NumericIntervalParam = Partial & { base: number; }; -export type EditorParamConfig = NumericIntervalParam | FixedParam | HiddenParam; +export type EditorParamConfig = NumericIntervalParam | FixedParam | Param; export interface EditorConfig { [paramName: string]: EditorParamConfig; From 0bef613e25ee6c090890180c28cd7d365b94096a Mon Sep 17 00:00:00 2001 From: Tim Roes Date: Fri, 10 Aug 2018 18:01:20 +0200 Subject: [PATCH 12/12] Remove unneeded self --- .../agg_types/__tests__/agg_param_writer.js | 31 +++++++++---------- 1 file changed, 14 insertions(+), 17 deletions(-) diff --git a/src/ui/public/agg_types/__tests__/agg_param_writer.js b/src/ui/public/agg_types/__tests__/agg_param_writer.js index dcab1bea91771..c4c619730092c 100644 --- a/src/ui/public/agg_types/__tests__/agg_param_writer.js +++ b/src/ui/public/agg_types/__tests__/agg_param_writer.js @@ -49,55 +49,52 @@ export default function AggParamWriterHelper(Private) { class AggParamWriter { constructor(opts) { - const self = this; - - self.aggType = opts.aggType; - if (_.isString(self.aggType)) { - self.aggType = aggTypes.byName[self.aggType]; + this.aggType = opts.aggType; + if (_.isString(this.aggType)) { + this.aggType = aggTypes.byName[this.aggType]; } // not configurable right now, but totally required - self.indexPattern = stubbedLogstashIndexPattern; + this.indexPattern = stubbedLogstashIndexPattern; // the schema that the aggType satisfies - self.visAggSchema = null; + this.visAggSchema = null; - self.vis = new Vis(self.indexPattern, { + this.vis = new Vis(this.indexPattern, { type: 'histogram', aggs: [{ id: 1, - type: self.aggType.name, + type: this.aggType.name, params: {} }] }); } write(paramValues, modifyAggConfig = null) { - const self = this; paramValues = _.clone(paramValues); - if (self.aggType.params.byName.field && !paramValues.field) { + if (this.aggType.params.byName.field && !paramValues.field) { // pick a field rather than force a field to be specified everywhere - if (self.aggType.type === 'metrics') { - paramValues.field = _.sample(self.indexPattern.fields.byType.number); + if (this.aggType.type === 'metrics') { + paramValues.field = _.sample(this.indexPattern.fields.byType.number); } else { - const type = self.aggType.params.byName.field.filterFieldTypes || 'string'; + const type = this.aggType.params.byName.field.filterFieldTypes || 'string'; let field; do { - field = _.sample(self.indexPattern.fields.byType[type]); + field = _.sample(this.indexPattern.fields.byType[type]); } while (!field.aggregatable); paramValues.field = field.name; } } - const aggConfig = self.vis.aggs[0]; + const aggConfig = this.vis.aggs[0]; aggConfig.setParams(paramValues); if (modifyAggConfig) { modifyAggConfig(aggConfig); } - return aggConfig.write(self.vis.aggs); + return aggConfig.write(this.vis.aggs); } }