Skip to content

Commit

Permalink
[lens] Prevent identical include and exclude values (elastic#197628)
Browse files Browse the repository at this point in the history 
## Summary

Fixes elastic#194639

- This PR adds filtering logic to:
  - `onChangeIncludeExcludeOptions`
  - `onCreateOption`
  - `onIncludeRegexChangeToDebounce` 
  - `onExcludeRegexChangeToDebounce`

These changes prevent the include and exclude fields from having the
same values simultaneously.

### Checklist

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios



https://github.com/user-attachments/assets/8f848c2a-0bea-46c6-b335-b04d62c12aef

---------

Co-authored-by: Marta Bondyra <[email protected]>
Co-authored-by: Elastic Machine <[email protected]>
  • Loading branch information
@VriaA @mbondyra @elasticmachine
3 people authored Nov 11, 2024
1 parent 5331489 commit 750e578
Show file tree
Hide file tree
Showing 2 changed files with 212 additions and 42 deletions.
166 changes: 165 additions & 1 deletion ...blic/datasources/form_based/operations/definitions/terms/include_exclude_options.test.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
*/

import React from 'react';
import { fireEvent, render, screen, within } from '@testing-library/react';
import { fireEvent, render, screen, within, act } from '@testing-library/react';
import userEvent from '@testing-library/user-event';
import { IncludeExcludeRow, IncludeExcludeRowProps } from './include_exclude_options';

Expand Down Expand Up @@ -152,4 +152,168 @@ describe('IncludeExcludeComponent', () => {
});
expect(onUpdateSpy).toHaveBeenCalledTimes(2);
});

it('should prevent identical include and exclude values on change when making single selections', async () => {
renderIncludeExcludeRow({
include: undefined,
exclude: undefined,
isNumberField: false,
tableRows,
});

await userEvent.click(screen.getByRole('combobox', { name: 'Include values' }));
await userEvent.click(screen.getByRole('option', { name: 'ABC' }));
expect(screen.getByTestId('lens-include-terms-combobox')).toHaveTextContent('ABC');

await userEvent.click(screen.getByRole('combobox', { name: 'Exclude values' }));
await userEvent.click(screen.getByRole('option', { name: 'ABC' }));
expect(screen.getByTestId('lens-exclude-terms-combobox')).toHaveTextContent('ABC');

expect(screen.getByTestId('lens-include-terms-combobox')).not.toHaveTextContent('ABC');

expect(onUpdateSpy).toHaveBeenCalledTimes(3);
});

it('should prevent identical include and exclude values on change when making multiple selections', async () => {
renderIncludeExcludeRow({
include: undefined,
exclude: undefined,
isNumberField: false,
tableRows,
});

await userEvent.click(screen.getByRole('combobox', { name: 'Include values' }));
await userEvent.click(screen.getByRole('option', { name: 'ABC' }));
expect(screen.getByTestId('lens-include-terms-combobox')).toHaveTextContent('ABC');

await userEvent.click(screen.getByRole('combobox', { name: 'Include values' }));
await userEvent.click(screen.getByRole('option', { name: 'FEF' }));
expect(screen.getByTestId('lens-include-terms-combobox')).toHaveTextContent('FEF');

await userEvent.click(screen.getByRole('combobox', { name: 'Exclude values' }));
await userEvent.click(screen.getByRole('option', { name: 'ABC' }));
expect(screen.getByTestId('lens-include-terms-combobox')).not.toHaveTextContent('ABC');

expect(screen.getByTestId('lens-exclude-terms-combobox')).toHaveTextContent('ABC');

expect(onUpdateSpy).toHaveBeenCalledTimes(4);
});

it('should prevent identical include and exclude values on create option', async () => {
renderIncludeExcludeRow({
include: undefined,
exclude: undefined,
isNumberField: false,
tableRows,
});

await userEvent.click(screen.getByRole('combobox', { name: 'Include values' }));
await userEvent.type(screen.getByRole('combobox', { name: 'Include values' }), 'test{enter}');
expect(screen.getByTestId('lens-include-terms-combobox')).toHaveTextContent('test');

await userEvent.click(screen.getByRole('combobox', { name: 'Exclude values' }));
await userEvent.type(screen.getByRole('combobox', { name: 'Exclude values' }), 'test{enter}');
expect(screen.getByTestId('lens-exclude-terms-combobox')).toHaveTextContent('test');

expect(screen.getByTestId('lens-include-terms-combobox')).not.toHaveTextContent('test');

expect(onUpdateSpy).toHaveBeenCalledTimes(3);
});

it('should prevent identical include and exclude values when creating multiple options', async () => {
renderIncludeExcludeRow({
include: undefined,
exclude: undefined,
isNumberField: false,
tableRows,
});

await userEvent.click(screen.getByRole('combobox', { name: 'Include values' }));
await userEvent.type(screen.getByRole('combobox', { name: 'Include values' }), 'test{enter}');
expect(screen.getByTestId('lens-include-terms-combobox')).toHaveTextContent('test');

await userEvent.type(screen.getByRole('combobox', { name: 'Include values' }), 'test1{enter}');
expect(screen.getByTestId('lens-include-terms-combobox')).toHaveTextContent('test1');

await userEvent.click(screen.getByRole('combobox', { name: 'Exclude values' }));
await userEvent.type(screen.getByRole('combobox', { name: 'Exclude values' }), 'test1{enter}');
expect(screen.getByTestId('lens-exclude-terms-combobox')).toHaveTextContent('test1');

expect(screen.getByTestId('lens-include-terms-combobox')).not.toHaveTextContent('test1');

expect(onUpdateSpy).toHaveBeenCalledTimes(4);
});

it('should prevent identical include value on exclude regex value change', async () => {
jest.useFakeTimers();

renderIncludeExcludeRow({
include: [''],
exclude: [''],
includeIsRegex: true,
excludeIsRegex: true,
tableRows,
});

const includeRegexInput = screen.getByTestId('lens-include-terms-regex-input');
const excludeRegexInput = screen.getByTestId('lens-exclude-terms-regex-input');
const user = userEvent.setup({ advanceTimers: jest.advanceTimersByTime });

await user.type(includeRegexInput, 'test.*');
act(() => {
jest.advanceTimersByTime(256);
});
expect(includeRegexInput).toHaveValue('test.*');
expect(onUpdateSpy).toHaveBeenCalledWith('include', ['test.*'], 'includeIsRegex', true);

await user.type(excludeRegexInput, 'test.*');
act(() => {
jest.advanceTimersByTime(256);
});
expect(excludeRegexInput).toHaveValue('test.*');
expect(onUpdateSpy).toHaveBeenCalledWith('exclude', ['test.*'], 'excludeIsRegex', true);

expect(includeRegexInput).toHaveValue('');
expect(onUpdateSpy).toHaveBeenCalledWith('include', [''], 'includeIsRegex', true);

expect(onUpdateSpy).toHaveBeenCalledTimes(3);

jest.useRealTimers();
});

it('should prevent identical exclude value on include regex value change', async () => {
jest.useFakeTimers();

renderIncludeExcludeRow({
include: [''],
exclude: [''],
includeIsRegex: true,
excludeIsRegex: true,
tableRows,
});

const includeRegexInput = screen.getByTestId('lens-include-terms-regex-input');
const excludeRegexInput = screen.getByTestId('lens-exclude-terms-regex-input');
const user = userEvent.setup({ advanceTimers: jest.advanceTimersByTime });

await user.type(excludeRegexInput, 'test.*');
act(() => {
jest.advanceTimersByTime(256);
});
expect(excludeRegexInput).toHaveValue('test.*');
expect(onUpdateSpy).toHaveBeenCalledWith('exclude', ['test.*'], 'excludeIsRegex', true);

await user.type(includeRegexInput, 'test.*');
act(() => {
jest.advanceTimersByTime(256);
});
expect(includeRegexInput).toHaveValue('test.*');
expect(onUpdateSpy).toHaveBeenCalledWith('include', ['test.*'], 'includeIsRegex', true);

expect(excludeRegexInput).toHaveValue('');
expect(onUpdateSpy).toHaveBeenCalledWith('exclude', [''], 'excludeIsRegex', true);

expect(onUpdateSpy).toHaveBeenCalledTimes(3);
jest.useRealTimers();
});
});
88 changes: 47 additions & 41 deletions ...ns/public/datasources/form_based/operations/definitions/terms/include_exclude_options.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -99,68 +99,77 @@ export const IncludeExcludeRow = ({
selectedOptions: IncludeExcludeOptions[],
operation: 'include' | 'exclude'
) => {
const options = {
...includeExcludeSelectedOptions,
[operation]: selectedOptions,
};
setIncludeExcludeSelectedOptions(options);
const terms = selectedOptions.map((option) => {
if (!Number.isNaN(Number(option.label))) {
return Number(option.label);
}
return option.label;
const otherOperation = operation === 'include' ? 'exclude' : 'include';
const otherSelectedOptions = includeExcludeSelectedOptions[otherOperation] ?? [];
const hasIdenticalOptions = selectedOptions.some((option) => {
return otherSelectedOptions.some((otherOption) => otherOption.label === option.label);
});
const param = `${operation}IsRegex`;
updateParams(operation, terms, param, false);
};

const onCreateOption = (
searchValue: string,
flattenedOptions: IncludeExcludeOptions[] = [],
operation: 'include' | 'exclude'
) => {
const newOption = {
label: searchValue,
};

let includeExcludeOptions = [];
const otherSelectedNonIdenticalOptions = hasIdenticalOptions
? otherSelectedOptions.filter(
(otherOption) => !selectedOptions.some((option) => option.label === otherOption.label)
)
: otherSelectedOptions;

const includeORExcludeSelectedOptions = includeExcludeSelectedOptions[operation] ?? [];
includeExcludeOptions = [...includeORExcludeSelectedOptions, newOption];
const options = {
...includeExcludeSelectedOptions,
[operation]: includeExcludeOptions,
[otherOperation]: otherSelectedNonIdenticalOptions,
[operation]: selectedOptions,
};
setIncludeExcludeSelectedOptions(options);

const terms = includeExcludeOptions.map((option) => {
if (!Number.isNaN(Number(option.label))) {
return Number(option.label);
}
return option.label;
});
const getTerms = (updatedSelectedOptions: IncludeExcludeOptions[]) =>
updatedSelectedOptions.map((option) => {
if (!Number.isNaN(Number(option.label))) {
return Number(option.label);
}
return option.label;
});

const terms = getTerms(selectedOptions);
const param = `${operation}IsRegex`;
updateParams(operation, terms, param, false);

if (hasIdenticalOptions) {
const otherTerms = getTerms(otherSelectedNonIdenticalOptions);
const otherParam = `${otherOperation}IsRegex`;
updateParams(otherOperation, otherTerms, otherParam, false);
}
};

const onCreateOption = (searchValue: string, operation: 'include' | 'exclude') => {
const newOption = { label: searchValue };
const selectedOptions = [...(includeExcludeSelectedOptions[operation] ?? []), newOption];
onChangeIncludeExcludeOptions(selectedOptions, operation);
};

const onIncludeRegexChangeToDebounce = useCallback(
(newIncludeValue: string | number | undefined) => {
const isEqualToExcludeValue = newIncludeValue === regex.exclude;
const excludeValue = isEqualToExcludeValue ? '' : regex.exclude;
setRegex({
...regex,
exclude: excludeValue,
include: newIncludeValue,
});
updateParams('include', [newIncludeValue ?? ''], 'includeIsRegex', true);
if (isEqualToExcludeValue) {
updateParams('exclude', [''], 'excludeIsRegex', true);
}
},
[regex, updateParams]
);

const onExcludeRegexChangeToDebounce = useCallback(
(newExcludeValue: string | number | undefined) => {
const isEqualToIncludeValue = newExcludeValue === regex.include;
const includeValue = isEqualToIncludeValue ? '' : regex.include;
setRegex({
...regex,
include: includeValue,
exclude: newExcludeValue,
});
updateParams('exclude', [newExcludeValue ?? ''], 'excludeIsRegex', true);
if (isEqualToIncludeValue) {
updateParams('include', [''], 'includeIsRegex', true);
}
},
[regex, updateParams]
);
Expand Down Expand Up @@ -247,9 +256,7 @@ export const IncludeExcludeRow = ({
options={termsOptions}
selectedOptions={includeExcludeSelectedOptions.include}
onChange={(options) => onChangeIncludeExcludeOptions(options, 'include')}
onCreateOption={(searchValue, options) =>
onCreateOption(searchValue, options, 'include')
}
onCreateOption={(searchValue) => onCreateOption(searchValue, 'include')}
isClearable={true}
data-test-subj="lens-include-terms-combobox"
autoFocus
Expand Down Expand Up @@ -300,6 +307,7 @@ export const IncludeExcludeRow = ({
defaultMessage: 'Enter a regex to filter values',
}
)}
data-test-subj="lens-exclude-terms-regex-input"
value={excludeRegexValue}
onChange={(e) => {
onExcludeRegexValueChange(e.target.value);
Expand All @@ -322,9 +330,7 @@ export const IncludeExcludeRow = ({
options={termsOptions}
selectedOptions={includeExcludeSelectedOptions.exclude}
onChange={(options) => onChangeIncludeExcludeOptions(options, 'exclude')}
onCreateOption={(searchValue, options) =>
onCreateOption(searchValue, options, 'exclude')
}
onCreateOption={(searchValue) => onCreateOption(searchValue, 'exclude')}
isClearable={true}
data-test-subj="lens-exclude-terms-combobox"
autoFocus
Expand Down

0 comments on commit 750e578

Please sign in to comment.