Skip to content

Commit

Permalink
[8.x] [lens] Prevent identical include and exclude values (#197628) (#…
Browse files Browse the repository at this point in the history 
…200133)

# Backport

This will backport the following commits from `main` to `8.x`:
- [[lens] Prevent identical include and exclude values
(#197628)](#197628)

<!--- Backport version: 9.4.3 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Oyelola
Victoria","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-11-11T16:48:23Z","message":"[lens]
Prevent identical include and exclude values (#197628)\n\n##
Summary\r\n\r\nFixes #194639\r\n\r\n- This PR adds filtering logic
to:\r\n - `onChangeIncludeExcludeOptions`\r\n - `onCreateOption`\r\n -
`onIncludeRegexChangeToDebounce` \r\n -
`onExcludeRegexChangeToDebounce`\r\n\r\nThese changes prevent the
include and exclude fields from having the\r\nsame values
simultaneously.\r\n\r\n### Checklist\r\n\r\n- [x] [Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common
scenarios\r\n\r\n\r\n\r\nhttps://github.com/user-attachments/assets/8f848c2a-0bea-46c6-b335-b04d62c12aef\r\n\r\n---------\r\n\r\nCo-authored-by:
Marta Bondyra
<[email protected]>\r\nCo-authored-by: Elastic
Machine
<[email protected]>","sha":"750e578f2867d1ad6bf2f1f7506a1d75f2d77d05","branchLabelMapping":{"^v9.0.0$":"main","^v8.17.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:fix","Team:Visualizations","backport
missing","💝community","v9.0.0","backport:current-major"],"title":"[Lens]
Prevent identical include and exclude
values","number":197628,"url":"https://github.com/elastic/kibana/pull/197628","mergeCommit":{"message":"[lens]
Prevent identical include and exclude values (#197628)\n\n##
Summary\r\n\r\nFixes #194639\r\n\r\n- This PR adds filtering logic
to:\r\n - `onChangeIncludeExcludeOptions`\r\n - `onCreateOption`\r\n -
`onIncludeRegexChangeToDebounce` \r\n -
`onExcludeRegexChangeToDebounce`\r\n\r\nThese changes prevent the
include and exclude fields from having the\r\nsame values
simultaneously.\r\n\r\n### Checklist\r\n\r\n- [x] [Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common
scenarios\r\n\r\n\r\n\r\nhttps://github.com/user-attachments/assets/8f848c2a-0bea-46c6-b335-b04d62c12aef\r\n\r\n---------\r\n\r\nCo-authored-by:
Marta Bondyra
<[email protected]>\r\nCo-authored-by: Elastic
Machine
<[email protected]>","sha":"750e578f2867d1ad6bf2f1f7506a1d75f2d77d05"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/197628","number":197628,"mergeCommit":{"message":"[lens]
Prevent identical include and exclude values (#197628)\n\n##
Summary\r\n\r\nFixes #194639\r\n\r\n- This PR adds filtering logic
to:\r\n - `onChangeIncludeExcludeOptions`\r\n - `onCreateOption`\r\n -
`onIncludeRegexChangeToDebounce` \r\n -
`onExcludeRegexChangeToDebounce`\r\n\r\nThese changes prevent the
include and exclude fields from having the\r\nsame values
simultaneously.\r\n\r\n### Checklist\r\n\r\n- [x] [Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common
scenarios\r\n\r\n\r\n\r\nhttps://github.com/user-attachments/assets/8f848c2a-0bea-46c6-b335-b04d62c12aef\r\n\r\n---------\r\n\r\nCo-authored-by:
Marta Bondyra
<[email protected]>\r\nCo-authored-by: Elastic
Machine
<[email protected]>","sha":"750e578f2867d1ad6bf2f1f7506a1d75f2d77d05"}}]}]
BACKPORT-->

Co-authored-by: Oyelola Victoria <[email protected]>
  • Loading branch information
@kibanamachine @VriaA
kibanamachine and VriaA authored Nov 14, 2024
1 parent 0c7424f commit 36df94f
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 36df94f

Please sign in to comment.