From 0c12506aa78dff8795f439ce19a4f3cd41a665f5 Mon Sep 17 00:00:00 2001 From: Devon Thomson Date: Thu, 5 Mar 2020 10:03:59 -0500 Subject: [PATCH] Disallow duplicate percentiles (#57444) (#58299) (#59360) Added an optional validation step to the number_list component to disallow duplicates, Reworked and consolidated number_list component validations into one method and enabled this option in the percentiles editor. Added unit / integration tests --- .../number_list/number_list.test.tsx | 20 +++++ .../components/number_list/number_list.tsx | 39 ++++------ .../components/number_list/utils.test.ts | 69 +++++++++-------- .../controls/components/number_list/utils.ts | 77 +++++++++++++++---- .../components/controls/percentile_ranks.tsx | 1 + .../components/controls/percentiles.test.tsx | 64 +++++++++++++++ .../components/controls/percentiles.tsx | 2 +- 7 files changed, 199 insertions(+), 73 deletions(-) create mode 100644 src/legacy/core_plugins/vis_default_editor/public/components/controls/percentiles.test.tsx diff --git a/src/legacy/core_plugins/vis_default_editor/public/components/controls/components/number_list/number_list.test.tsx b/src/legacy/core_plugins/vis_default_editor/public/components/controls/components/number_list/number_list.test.tsx index 3faf164c365d9..f547f1dee6a39 100644 --- a/src/legacy/core_plugins/vis_default_editor/public/components/controls/components/number_list/number_list.test.tsx +++ b/src/legacy/core_plugins/vis_default_editor/public/components/controls/components/number_list/number_list.test.tsx @@ -63,12 +63,31 @@ describe('NumberList', () => { test('should show an order error', () => { defaultProps.numberArray = [3, 1]; + defaultProps.validateAscendingOrder = true; defaultProps.showValidation = true; const comp = mountWithIntl(); expect(comp.find('EuiFormErrorText').length).toBe(1); }); + test('should show a duplicate error', () => { + defaultProps.numberArray = [3, 1, 3]; + defaultProps.disallowDuplicates = true; + defaultProps.showValidation = true; + const comp = mountWithIntl(); + + expect(comp.find('EuiFormErrorText').length).toBeGreaterThan(0); + }); + + test('should show many duplicate errors', () => { + defaultProps.numberArray = [3, 1, 3, 1, 3, 1, 3, 1]; + defaultProps.disallowDuplicates = true; + defaultProps.showValidation = true; + const comp = mountWithIntl(); + + expect(comp.find('EuiFormErrorText').length).toBe(6); + }); + test('should set validity as true', () => { mountWithIntl(); @@ -77,6 +96,7 @@ describe('NumberList', () => { test('should set validity as false when the order is invalid', () => { defaultProps.numberArray = [3, 2]; + defaultProps.validateAscendingOrder = true; const comp = mountWithIntl(); expect(defaultProps.setValidity).lastCalledWith(false); diff --git a/src/legacy/core_plugins/vis_default_editor/public/components/controls/components/number_list/number_list.tsx b/src/legacy/core_plugins/vis_default_editor/public/components/controls/components/number_list/number_list.tsx index 8e290ceedfeac..a43c66c2e08cc 100644 --- a/src/legacy/core_plugins/vis_default_editor/public/components/controls/components/number_list/number_list.tsx +++ b/src/legacy/core_plugins/vis_default_editor/public/components/controls/components/number_list/number_list.tsx @@ -21,17 +21,14 @@ import React, { Fragment, useState, useEffect, useMemo, useCallback } from 'reac import { EuiSpacer, EuiButtonEmpty, EuiFlexItem, EuiFormErrorText } from '@elastic/eui'; import { FormattedMessage } from '@kbn/i18n/react'; -import { i18n } from '@kbn/i18n'; import { NumberRow, NumberRowModel } from './number_row'; import { parse, EMPTY_STRING, getRange, - validateOrder, - validateValue, getNextModel, getInitModelList, - getUpdatedModels, + getValidatedModels, hasInvalidValues, } from './utils'; import { useValidation } from '../../utils'; @@ -41,6 +38,7 @@ export interface NumberListProps { numberArray: Array; range?: string; showValidation: boolean; + disallowDuplicates?: boolean; unitName: string; validateAscendingOrder?: boolean; onChange(list: Array): void; @@ -54,31 +52,27 @@ function NumberList({ range, showValidation, unitName, - validateAscendingOrder = true, + validateAscendingOrder = false, + disallowDuplicates = false, onChange, setTouched, setValidity, }: NumberListProps) { const numberRange = useMemo(() => getRange(range), [range]); const [models, setModels] = useState(getInitModelList(numberArray)); - const [ascendingError, setAscendingError] = useState(EMPTY_STRING); // set up validity for each model useEffect(() => { - let id: number | undefined; - if (validateAscendingOrder) { - const { isValidOrder, modelIndex } = validateOrder(numberArray); - id = isValidOrder ? undefined : modelIndex; - setAscendingError( - isValidOrder - ? EMPTY_STRING - : i18n.translate('visDefaultEditor.controls.numberList.invalidAscOrderErrorMessage', { - defaultMessage: 'The values should be in ascending order.', - }) - ); - } - setModels(state => getUpdatedModels(numberArray, state, numberRange, id)); - }, [numberArray, numberRange, validateAscendingOrder]); + setModels(state => + getValidatedModels( + numberArray, + state, + numberRange, + validateAscendingOrder, + disallowDuplicates + ) + ); + }, [numberArray, numberRange, validateAscendingOrder, disallowDuplicates]); // responsible for setting up an initial value ([0]) when there is no default value useEffect(() => { @@ -105,12 +99,10 @@ function NumberList({ onUpdate( models.map(model => { if (model.id === id) { - const { isInvalid, error } = validateValue(parsedValue, numberRange); return { id, value: parsedValue, - isInvalid, - error, + isInvalid: false, }; } return model; @@ -155,7 +147,6 @@ function NumberList({ {models.length - 1 !== arrayIndex && } ))} - {showValidation && ascendingError && {ascendingError}} diff --git a/src/legacy/core_plugins/vis_default_editor/public/components/controls/components/number_list/utils.test.ts b/src/legacy/core_plugins/vis_default_editor/public/components/controls/components/number_list/utils.test.ts index 89fb5738db379..9cffaadfc956d 100644 --- a/src/legacy/core_plugins/vis_default_editor/public/components/controls/components/number_list/utils.test.ts +++ b/src/legacy/core_plugins/vis_default_editor/public/components/controls/components/number_list/utils.test.ts @@ -19,13 +19,12 @@ import { getInitModelList, - getUpdatedModels, - validateOrder, hasInvalidValues, parse, validateValue, getNextModel, getRange, + getValidatedModels, } from './utils'; import { NumberListRange } from './range'; import { NumberRowModel } from './number_row'; @@ -33,6 +32,7 @@ import { NumberRowModel } from './number_row'; describe('NumberList utils', () => { let modelList: NumberRowModel[]; let range: NumberListRange; + let invalidEntry: NumberRowModel; beforeEach(() => { modelList = [ @@ -46,6 +46,12 @@ describe('NumberList utils', () => { maxInclusive: true, within: jest.fn(() => true), }; + invalidEntry = { + value: expect.any(Number), + isInvalid: true, + error: expect.any(String), + id: expect.any(String), + }; }); describe('getInitModelList', () => { @@ -65,27 +71,27 @@ describe('NumberList utils', () => { }); }); - describe('getUpdatedModels', () => { + describe('getValidatedModels', () => { test('should return model list when number list is empty', () => { - const updatedModelList = getUpdatedModels([], modelList, range); + const updatedModelList = getValidatedModels([], modelList, range); expect(updatedModelList).toEqual([{ value: 0, id: expect.any(String), isInvalid: false }]); }); test('should not update model list when number list is the same', () => { - const updatedModelList = getUpdatedModels([1, 2], modelList, range); + const updatedModelList = getValidatedModels([1, 2], modelList, range); expect(updatedModelList).toEqual(modelList); }); test('should update model list when number list was changed', () => { - const updatedModelList = getUpdatedModels([1, 3], modelList, range); + const updatedModelList = getValidatedModels([1, 3], modelList, range); modelList[1].value = 3; expect(updatedModelList).toEqual(modelList); }); test('should update model list when number list increased', () => { - const updatedModelList = getUpdatedModels([1, 2, 3], modelList, range); + const updatedModelList = getValidatedModels([1, 2, 3], modelList, range); expect(updatedModelList).toEqual([ ...modelList, { value: 3, id: expect.any(String), isInvalid: false }, @@ -93,45 +99,46 @@ describe('NumberList utils', () => { }); test('should update model list when number list decreased', () => { - const updatedModelList = getUpdatedModels([2], modelList, range); + const updatedModelList = getValidatedModels([2], modelList, range); expect(updatedModelList).toEqual([{ value: 2, id: '1', isInvalid: false }]); }); test('should update model list when number list has undefined value', () => { - const updatedModelList = getUpdatedModels([1, undefined], modelList, range); + const updatedModelList = getValidatedModels([1, undefined], modelList, range); modelList[1].value = ''; modelList[1].isInvalid = true; expect(updatedModelList).toEqual(modelList); }); - test('should update model list when number order is invalid', () => { - const updatedModelList = getUpdatedModels([1, 3, 2], modelList, range, 2); - expect(updatedModelList).toEqual([ - modelList[0], - { ...modelList[1], value: 3 }, - { value: 2, id: expect.any(String), isInvalid: true }, - ]); + test('should identify when a number is out of order', () => { + const updatedModelList = getValidatedModels([1, 3, 2], modelList, range, true); + expect(updatedModelList[2]).toEqual(invalidEntry); }); - }); - describe('validateOrder', () => { - test('should return true when order is valid', () => { - expect(validateOrder([1, 2])).toEqual({ - isValidOrder: true, - }); + test('should identify when many numbers are out of order', () => { + const updatedModelList = getValidatedModels([1, 3, 2, 3, 4, 2], modelList, range, true); + expect(updatedModelList[2]).toEqual(invalidEntry); + expect(updatedModelList[5]).toEqual(invalidEntry); }); - test('should return true when a number is undefined', () => { - expect(validateOrder([1, undefined])).toEqual({ - isValidOrder: true, - }); + test('should identify a duplicate', () => { + const updatedModelList = getValidatedModels([1, 2, 3, 6, 2], modelList, range, false, true); + expect(updatedModelList[4]).toEqual(invalidEntry); }); - test('should return false when order is invalid', () => { - expect(validateOrder([2, 1])).toEqual({ - isValidOrder: false, - modelIndex: 1, - }); + test('should identify many duplicates', () => { + const updatedModelList = getValidatedModels( + [2, 2, 2, 3, 4, 5, 2, 2, 3], + modelList, + range, + false, + true + ); + expect(updatedModelList[1]).toEqual(invalidEntry); + expect(updatedModelList[2]).toEqual(invalidEntry); + expect(updatedModelList[6]).toEqual(invalidEntry); + expect(updatedModelList[7]).toEqual(invalidEntry); + expect(updatedModelList[8]).toEqual(invalidEntry); }); }); diff --git a/src/legacy/core_plugins/vis_default_editor/public/components/controls/components/number_list/utils.ts b/src/legacy/core_plugins/vis_default_editor/public/components/controls/components/number_list/utils.ts index e0f32366fc265..c2ac63c98cbea 100644 --- a/src/legacy/core_plugins/vis_default_editor/public/components/controls/components/number_list/utils.ts +++ b/src/legacy/core_plugins/vis_default_editor/public/components/controls/components/number_list/utils.ts @@ -49,6 +49,7 @@ function validateValue(value: number | '', numberRange: NumberListRange) { if (value === EMPTY_STRING) { result.isInvalid = true; + result.error = EMPTY_STRING; } else if (!numberRange.within(value)) { result.isInvalid = true; result.error = i18n.translate('visDefaultEditor.controls.numberList.invalidRangeErrorMessage', { @@ -60,19 +61,46 @@ function validateValue(value: number | '', numberRange: NumberListRange) { return result; } -function validateOrder(list: Array) { - const result: { isValidOrder: boolean; modelIndex?: number } = { - isValidOrder: true, +function validateValueAscending( + inputValue: number | '', + index: number, + list: Array +) { + const result: { isInvalidOrder: boolean; error?: string } = { + isInvalidOrder: false, }; - list.forEach((inputValue, index, array) => { - const previousModel = array[index - 1]; - if (previousModel !== undefined && inputValue !== undefined && inputValue <= previousModel) { - result.isValidOrder = false; - result.modelIndex = index; - } - }); + const previousModel = list[index - 1]; + if (previousModel !== undefined && inputValue !== undefined && inputValue <= previousModel) { + result.isInvalidOrder = true; + result.error = i18n.translate( + 'visDefaultEditor.controls.numberList.invalidAscOrderErrorMessage', + { + defaultMessage: 'Value is not in ascending order.', + } + ); + } + return result; +} + +function validateValueUnique( + inputValue: number | '', + index: number, + list: Array +) { + const result: { isDuplicate: boolean; error?: string } = { + isDuplicate: false, + }; + if (inputValue && list.indexOf(inputValue) !== index) { + result.isDuplicate = true; + result.error = i18n.translate( + 'visDefaultEditor.controls.numberList.duplicateValueErrorMessage', + { + defaultMessage: 'Duplicate value.', + } + ); + } return result; } @@ -101,11 +129,12 @@ function getInitModelList(list: Array): NumberRowModel[] { : [defaultModel]; } -function getUpdatedModels( +function getValidatedModels( numberList: Array, modelList: NumberRowModel[], numberRange: NumberListRange, - invalidOrderModelIndex?: number + validateAscendingOrder: boolean = false, + disallowDuplicates: boolean = false ): NumberRowModel[] { if (!numberList.length) { return [defaultModel]; @@ -113,12 +142,27 @@ function getUpdatedModels( return numberList.map((number, index) => { const model = modelList[index] || { id: generateId() }; const newValue: NumberRowModel['value'] = number === undefined ? EMPTY_STRING : number; - const { isInvalid, error } = validateValue(newValue, numberRange); + + const valueResult = numberRange ? validateValue(newValue, numberRange) : { isInvalid: false }; + + const ascendingResult = validateAscendingOrder + ? validateValueAscending(newValue, index, numberList) + : { isInvalidOrder: false }; + + const duplicationResult = disallowDuplicates + ? validateValueUnique(newValue, index, numberList) + : { isDuplicate: false }; + + const allErrors = [valueResult.error, ascendingResult.error, duplicationResult.error] + .filter(Boolean) + .join(' '); + return { ...model, value: newValue, - isInvalid: invalidOrderModelIndex === index ? true : isInvalid, - error, + isInvalid: + valueResult.isInvalid || ascendingResult.isInvalidOrder || duplicationResult.isDuplicate, + error: allErrors === EMPTY_STRING ? undefined : allErrors, }; }); } @@ -132,9 +176,8 @@ export { parse, getRange, validateValue, - validateOrder, getNextModel, getInitModelList, - getUpdatedModels, + getValidatedModels, hasInvalidValues, }; diff --git a/src/legacy/core_plugins/vis_default_editor/public/components/controls/percentile_ranks.tsx b/src/legacy/core_plugins/vis_default_editor/public/components/controls/percentile_ranks.tsx index c6057b7ce2a99..fb7d8d78b28e3 100644 --- a/src/legacy/core_plugins/vis_default_editor/public/components/controls/percentile_ranks.tsx +++ b/src/legacy/core_plugins/vis_default_editor/public/components/controls/percentile_ranks.tsx @@ -62,6 +62,7 @@ function PercentileRanksEditor({ unitName={i18n.translate('visDefaultEditor.controls.percentileRanks.valueUnitNameText', { defaultMessage: 'value', })} + validateAscendingOrder={true} showValidation={showValidation} onChange={setValue} setTouched={setTouched} diff --git a/src/legacy/core_plugins/vis_default_editor/public/components/controls/percentiles.test.tsx b/src/legacy/core_plugins/vis_default_editor/public/components/controls/percentiles.test.tsx new file mode 100644 index 0000000000000..020dbb351b497 --- /dev/null +++ b/src/legacy/core_plugins/vis_default_editor/public/components/controls/percentiles.test.tsx @@ -0,0 +1,64 @@ +/* + * 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 React from 'react'; +import { AggParamEditorProps } from '../agg_param_props'; +import { IAggConfig } from '../../legacy_imports'; +import { VisState } from 'src/legacy/core_plugins/visualizations/public'; +import { mount } from 'enzyme'; +import { PercentilesEditor } from './percentiles'; + +describe('PercentilesEditor component', () => { + let setValue: jest.Mock; + let setValidity: jest.Mock; + let setTouched: jest.Mock; + let defaultProps: AggParamEditorProps>; + + beforeEach(() => { + setValue = jest.fn(); + setValidity = jest.fn(); + setTouched = jest.fn(); + + defaultProps = { + agg: {} as IAggConfig, + aggParam: {} as any, + formIsTouched: false, + value: [1, 5, 25, 50, 75, 95, 99], + editorConfig: {}, + showValidation: false, + setValue, + setValidity, + setTouched, + state: {} as VisState, + metricAggs: [] as IAggConfig[], + }; + }); + + it('should set valid state to true after adding a unique percentile', () => { + defaultProps.value = [1, 5, 25, 50, 70]; + mount(); + expect(setValidity).lastCalledWith(true); + }); + + it('should set valid state to false after adding a duplicate percentile', () => { + defaultProps.value = [1, 5, 25, 50, 50]; + mount(); + expect(setValidity).lastCalledWith(false); + }); +}); diff --git a/src/legacy/core_plugins/vis_default_editor/public/components/controls/percentiles.tsx b/src/legacy/core_plugins/vis_default_editor/public/components/controls/percentiles.tsx index 74e7957bc1944..9f1f26fe5446f 100644 --- a/src/legacy/core_plugins/vis_default_editor/public/components/controls/percentiles.tsx +++ b/src/legacy/core_plugins/vis_default_editor/public/components/controls/percentiles.tsx @@ -58,7 +58,7 @@ function PercentilesEditor({ labelledbyId={`visEditorPercentileLabel${agg.id}-legend`} numberArray={value} range="[0,100]" - validateAscendingOrder={false} + disallowDuplicates={true} unitName={i18n.translate('visDefaultEditor.controls.percentileRanks.percentUnitNameText', { defaultMessage: 'percent', })}