Skip to content

Commit

Permalink
Disallow duplicate percentiles (#57444) (#58299)
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@ThomThomson
ThomThomson authored Mar 2, 2020
1 parent c81e0da commit 4696736
Show file tree
Hide file tree
Showing 7 changed files with 199 additions and 73 deletions.
20 changes: 20 additions & 0 deletions ...vis_default_editor/public/components/controls/components/number_list/number_list.test.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(<NumberList {...defaultProps} />);

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(<NumberList {...defaultProps} />);

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(<NumberList {...defaultProps} />);

expect(comp.find('EuiFormErrorText').length).toBe(6);
});

test('should set validity as true', () => {
mountWithIntl(<NumberList {...defaultProps} />);

Expand All @@ -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(<NumberList {...defaultProps} />);

expect(defaultProps.setValidity).lastCalledWith(false);
Expand Down
39 changes: 15 additions & 24 deletions ...gins/vis_default_editor/public/components/controls/components/number_list/number_list.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -41,6 +38,7 @@ export interface NumberListProps {
numberArray: Array<number | undefined>;
range?: string;
showValidation: boolean;
disallowDuplicates?: boolean;
unitName: string;
validateAscendingOrder?: boolean;
onChange(list: Array<number | undefined>): void;
Expand All @@ -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(() => {
Expand All @@ -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;
Expand Down Expand Up @@ -155,7 +147,6 @@ function NumberList({
{models.length - 1 !== arrayIndex && <EuiSpacer size="s" />}
</Fragment>
))}
{showValidation && ascendingError && <EuiFormErrorText>{ascendingError}</EuiFormErrorText>}
<EuiSpacer size="s" />
<EuiFlexItem>
<EuiButtonEmpty iconType="plusInCircleFilled" onClick={onAdd} size="xs">
Expand Down
69 changes: 38 additions & 31 deletions ...lugins/vis_default_editor/public/components/controls/components/number_list/utils.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,20 +19,20 @@

import {
getInitModelList,
getUpdatedModels,
validateOrder,
hasInvalidValues,
parse,
validateValue,
getNextModel,
getRange,
getValidatedModels,
} from './utils';
import { NumberListRange } from './range';
import { NumberRowModel } from './number_row';

describe('NumberList utils', () => {
let modelList: NumberRowModel[];
let range: NumberListRange;
let invalidEntry: NumberRowModel;

beforeEach(() => {
modelList = [
Expand All @@ -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', () => {
Expand All @@ -65,73 +71,74 @@ 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 },
]);
});

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);
});
});

Expand Down
77 changes: 60 additions & 17 deletions ...ore_plugins/vis_default_editor/public/components/controls/components/number_list/utils.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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', {
Expand All @@ -60,19 +61,46 @@ function validateValue(value: number | '', numberRange: NumberListRange) {
return result;
}

function validateOrder(list: Array<number | undefined>) {
const result: { isValidOrder: boolean; modelIndex?: number } = {
isValidOrder: true,
function validateValueAscending(
inputValue: number | '',
index: number,
list: Array<number | undefined>
) {
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<number | undefined>
) {
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;
}

Expand Down Expand Up @@ -101,24 +129,40 @@ function getInitModelList(list: Array<number | undefined>): NumberRowModel[] {
: [defaultModel];
}

function getUpdatedModels(
function getValidatedModels(
numberList: Array<number | undefined>,
modelList: NumberRowModel[],
numberRange: NumberListRange,
invalidOrderModelIndex?: number
validateAscendingOrder: boolean = false,
disallowDuplicates: boolean = false
): NumberRowModel[] {
if (!numberList.length) {
return [defaultModel];
}
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,
};
});
}
Expand All @@ -132,9 +176,8 @@ export {
parse,
getRange,
validateValue,
validateOrder,
getNextModel,
getInitModelList,
getUpdatedModels,
getValidatedModels,
hasInvalidValues,
};
Loading

0 comments on commit 4696736

Please sign in to comment.