[Cases] Bulk edit tags: Prevent error when updating cases with the same tags (#145608)

## Summary

This PR fixes a bug where an error occurs when updating the case with
the same tags. To fix it, we filter cases without any changes.

Fixes: https://github.com/elastic/kibana/issues/145217

### Checklist

Delete any items that are not applicable to this PR.

- [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

### For maintainers

- [x] This was checked for breaking API changes and was [labeled
appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)
This commit is contained in:
Christos Nasikas 2022-11-21 17:51:00 +02:00 committed by GitHub
parent 62d9dffbb2
commit b1fb286d84
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
6 changed files with 237 additions and 61 deletions

View file

@ -193,4 +193,154 @@ describe('useTagsAction', () => {
);
});
});
it('do not update cases with no changes', async () => {
const updateSpy = jest.spyOn(api, 'updateCases');
const { result, waitFor } = renderHook(
() => useTagsAction({ onAction, onActionSuccess, isDisabled: false }),
{
wrapper: appMockRender.AppWrapper,
}
);
const action = result.current.getAction([{ ...basicCase, tags: [] }]);
act(() => {
action.onClick();
});
expect(onAction).toHaveBeenCalled();
expect(result.current.isFlyoutOpen).toBe(true);
act(() => {
result.current.onSaveTags({ selectedTags: [], unSelectedTags: ['pepsi'] });
});
await waitFor(() => {
expect(result.current.isFlyoutOpen).toBe(false);
expect(onActionSuccess).not.toHaveBeenCalled();
expect(updateSpy).not.toHaveBeenCalled();
});
});
it('do not update if the selected tags are the same but with different order', async () => {
const updateSpy = jest.spyOn(api, 'updateCases');
const { result, waitFor } = renderHook(
() => useTagsAction({ onAction, onActionSuccess, isDisabled: false }),
{
wrapper: appMockRender.AppWrapper,
}
);
const action = result.current.getAction([{ ...basicCase, tags: ['1', '2'] }]);
act(() => {
action.onClick();
});
expect(onAction).toHaveBeenCalled();
expect(result.current.isFlyoutOpen).toBe(true);
act(() => {
result.current.onSaveTags({ selectedTags: ['2', '1'], unSelectedTags: [] });
});
await waitFor(() => {
expect(result.current.isFlyoutOpen).toBe(false);
expect(onActionSuccess).not.toHaveBeenCalled();
expect(updateSpy).not.toHaveBeenCalled();
});
});
it('do not update if the selected tags are the same', async () => {
const updateSpy = jest.spyOn(api, 'updateCases');
const { result, waitFor } = renderHook(
() => useTagsAction({ onAction, onActionSuccess, isDisabled: false }),
{
wrapper: appMockRender.AppWrapper,
}
);
const action = result.current.getAction([{ ...basicCase, tags: ['1'] }]);
act(() => {
action.onClick();
});
expect(onAction).toHaveBeenCalled();
expect(result.current.isFlyoutOpen).toBe(true);
act(() => {
result.current.onSaveTags({ selectedTags: ['1'], unSelectedTags: [] });
});
await waitFor(() => {
expect(result.current.isFlyoutOpen).toBe(false);
expect(onActionSuccess).not.toHaveBeenCalled();
expect(updateSpy).not.toHaveBeenCalled();
});
});
it('do not update if selecting and unselecting the same tag', async () => {
const updateSpy = jest.spyOn(api, 'updateCases');
const { result, waitFor } = renderHook(
() => useTagsAction({ onAction, onActionSuccess, isDisabled: false }),
{
wrapper: appMockRender.AppWrapper,
}
);
const action = result.current.getAction([{ ...basicCase, tags: ['1'] }]);
act(() => {
action.onClick();
});
expect(onAction).toHaveBeenCalled();
expect(result.current.isFlyoutOpen).toBe(true);
act(() => {
result.current.onSaveTags({ selectedTags: ['1'], unSelectedTags: ['1'] });
});
await waitFor(() => {
expect(result.current.isFlyoutOpen).toBe(false);
expect(onActionSuccess).not.toHaveBeenCalled();
expect(updateSpy).not.toHaveBeenCalled();
});
});
it('do not update with empty tags and no selection', async () => {
const updateSpy = jest.spyOn(api, 'updateCases');
const { result, waitFor } = renderHook(
() => useTagsAction({ onAction, onActionSuccess, isDisabled: false }),
{
wrapper: appMockRender.AppWrapper,
}
);
const action = result.current.getAction([{ ...basicCase, tags: [] }]);
act(() => {
action.onClick();
});
expect(onAction).toHaveBeenCalled();
expect(result.current.isFlyoutOpen).toBe(true);
act(() => {
result.current.onSaveTags({ selectedTags: [], unSelectedTags: [] });
});
await waitFor(() => {
expect(result.current.isFlyoutOpen).toBe(false);
expect(onActionSuccess).not.toHaveBeenCalled();
expect(updateSpy).not.toHaveBeenCalled();
});
});
});

View file

@ -7,7 +7,8 @@
import { EuiIcon } from '@elastic/eui';
import React, { useCallback, useState } from 'react';
import { difference } from 'lodash';
import { difference, isEqual } from 'lodash';
import type { CaseUpdateRequest } from '../../../../common/ui';
import { useUpdateCases } from '../../../containers/use_bulk_update_case';
import type { Case } from '../../../../common';
import { useCasesContext } from '../../cases_context/use_cases_context';
@ -33,20 +34,32 @@ export const useTagsAction = ({ onAction, onActionSuccess, isDisabled }: UseActi
[onAction]
);
const areTagsEqual = (originalTags: Set<string>, tagsToUpdate: Set<string>): boolean => {
return isEqual(originalTags, tagsToUpdate);
};
const onSaveTags = useCallback(
(tagsSelection: TagsSelectionState) => {
onAction();
onFlyoutClosed();
const casesToUpdate = selectedCasesToEditTags.map((theCase) => {
const tags = difference(theCase.tags, tagsSelection.unSelectedTags);
const uniqueTags = new Set([...tags, ...tagsSelection.selectedTags]);
return {
tags: Array.from(uniqueTags.values()),
id: theCase.id,
version: theCase.version,
};
});
const casesToUpdate = selectedCasesToEditTags.reduce((acc, theCase) => {
const tagsWithoutUnselectedTags = difference(theCase.tags, tagsSelection.unSelectedTags);
const uniqueTags = new Set([...tagsWithoutUnselectedTags, ...tagsSelection.selectedTags]);
if (areTagsEqual(new Set([...theCase.tags]), uniqueTags)) {
return acc;
}
return [
...acc,
{
tags: Array.from(uniqueTags.values()),
id: theCase.id,
version: theCase.version,
},
];
}, [] as CaseUpdateRequest[]);
updateCases(
{

View file

@ -75,7 +75,7 @@ describe('Cases API', () => {
});
const data = ['1', '2'];
test('should be called with correct check url, method, signal', async () => {
it('should be called with correct check url, method, signal', async () => {
await deleteCases(data, abortCtrl.signal);
expect(fetchMock).toHaveBeenCalledWith(`${CASES_URL}`, {
method: 'DELETE',
@ -84,7 +84,7 @@ describe('Cases API', () => {
});
});
test('should return correct response', async () => {
it('should return correct response', async () => {
const resp = await deleteCases(data, abortCtrl.signal);
expect(resp).toEqual('');
});
@ -96,7 +96,7 @@ describe('Cases API', () => {
fetchMock.mockResolvedValue(actionLicenses);
});
test('should be called with correct check url, method, signal', async () => {
it('should be called with correct check url, method, signal', async () => {
await getActionLicense(abortCtrl.signal);
expect(fetchMock).toHaveBeenCalledWith(`/api/actions/connector_types`, {
method: 'GET',
@ -104,7 +104,7 @@ describe('Cases API', () => {
});
});
test('should return correct response', async () => {
it('should return correct response', async () => {
const resp = await getActionLicense(abortCtrl.signal);
expect(resp).toEqual(actionLicenses);
});
@ -117,7 +117,7 @@ describe('Cases API', () => {
});
const data = basicCase.id;
test('should be called with correct check url, method, signal', async () => {
it('should be called with correct check url, method, signal', async () => {
await getCase(data, true, abortCtrl.signal);
expect(fetchMock).toHaveBeenCalledWith(`${CASES_URL}/${basicCase.id}`, {
method: 'GET',
@ -126,12 +126,12 @@ describe('Cases API', () => {
});
});
test('should return correct response', async () => {
it('should return correct response', async () => {
const resp = await getCase(data, true, abortCtrl.signal);
expect(resp).toEqual(basicCase);
});
test('should not covert to camel case registered attachments', async () => {
it('should not covert to camel case registered attachments', async () => {
fetchMock.mockResolvedValue(caseWithRegisteredAttachmentsSnake);
const resp = await getCase(data, true, abortCtrl.signal);
expect(resp).toEqual(caseWithRegisteredAttachments);
@ -151,7 +151,7 @@ describe('Cases API', () => {
fetchMock.mockResolvedValue({ ...basicResolveCase, target_alias_id: targetAliasId });
});
test('should be called with correct check url, method, signal', async () => {
it('should be called with correct check url, method, signal', async () => {
await resolveCase(caseId, true, abortCtrl.signal);
expect(fetchMock).toHaveBeenCalledWith(`${CASES_URL}/${caseId}/resolve`, {
method: 'GET',
@ -160,12 +160,12 @@ describe('Cases API', () => {
});
});
test('should return correct response', async () => {
it('should return correct response', async () => {
const resp = await resolveCase(caseId, true, abortCtrl.signal);
expect(resp).toEqual({ ...basicResolveCase, case: basicCase, targetAliasId });
});
test('should not covert to camel case registered attachments', async () => {
it('should not covert to camel case registered attachments', async () => {
fetchMock.mockResolvedValue({
...basicResolveCase,
case: caseWithRegisteredAttachmentsSnake,
@ -187,7 +187,7 @@ describe('Cases API', () => {
fetchMock.mockResolvedValue(allCasesSnake);
});
test('should be called with correct check url, method, signal with empty defaults', async () => {
it('should be called with correct check url, method, signal with empty defaults', async () => {
await getCases({
filterOptions: DEFAULT_FILTER_OPTIONS,
queryParams: DEFAULT_QUERY_PARAMS,
@ -204,7 +204,7 @@ describe('Cases API', () => {
});
});
test('should applies correct all filters', async () => {
it('should applies correct all filters', async () => {
await getCases({
filterOptions: {
searchFields: DEFAULT_FILTER_OPTIONS.searchFields,
@ -237,7 +237,7 @@ describe('Cases API', () => {
});
});
test('should apply the severity field correctly (with severity value)', async () => {
it('should apply the severity field correctly (with severity value)', async () => {
await getCases({
filterOptions: {
...DEFAULT_FILTER_OPTIONS,
@ -258,7 +258,7 @@ describe('Cases API', () => {
});
});
test('should not send the severity field with "all" severity value', async () => {
it('should not send the severity field with "all" severity value', async () => {
await getCases({
filterOptions: {
...DEFAULT_FILTER_OPTIONS,
@ -278,7 +278,7 @@ describe('Cases API', () => {
});
});
test('should apply the severity field correctly (with status value)', async () => {
it('should apply the severity field correctly (with status value)', async () => {
await getCases({
filterOptions: {
...DEFAULT_FILTER_OPTIONS,
@ -299,7 +299,7 @@ describe('Cases API', () => {
});
});
test('should not send the severity field with "all" status value', async () => {
it('should not send the severity field with "all" status value', async () => {
await getCases({
filterOptions: {
...DEFAULT_FILTER_OPTIONS,
@ -319,7 +319,7 @@ describe('Cases API', () => {
});
});
test('should not send the assignees field if it an empty array', async () => {
it('should not send the assignees field if it an empty array', async () => {
await getCases({
filterOptions: {
...DEFAULT_FILTER_OPTIONS,
@ -339,7 +339,7 @@ describe('Cases API', () => {
});
});
test('should convert a single null value to none', async () => {
it('should convert a single null value to none', async () => {
await getCases({
filterOptions: {
...DEFAULT_FILTER_OPTIONS,
@ -360,7 +360,7 @@ describe('Cases API', () => {
});
});
test('should converts null value in the array to none', async () => {
it('should converts null value in the array to none', async () => {
await getCases({
filterOptions: {
...DEFAULT_FILTER_OPTIONS,
@ -381,7 +381,7 @@ describe('Cases API', () => {
});
});
test('should handle tags with weird chars', async () => {
it('should handle tags with weird chars', async () => {
const weirdTags: string[] = ['(', '"double"'];
await getCases({
@ -414,7 +414,7 @@ describe('Cases API', () => {
});
});
test('should return correct response and not covert to camel case registered attachments', async () => {
it('should return correct response and not covert to camel case registered attachments', async () => {
fetchMock.mockResolvedValue(allCasesSnake);
const resp = await getCases({
filterOptions: { ...DEFAULT_FILTER_OPTIONS, owner: [SECURITY_SOLUTION_OWNER] },
@ -433,7 +433,7 @@ describe('Cases API', () => {
fetchMock.mockClear();
});
test('should be called with correct check url, method, signal', async () => {
it('should be called with correct check url, method, signal', async () => {
await getCasesStatus({
http,
signal: abortCtrl.signal,
@ -446,7 +446,7 @@ describe('Cases API', () => {
});
});
test('should return correct response', async () => {
it('should return correct response', async () => {
const resp = await getCasesStatus({
http,
signal: abortCtrl.signal,
@ -463,7 +463,7 @@ describe('Cases API', () => {
fetchMock.mockResolvedValue(caseUserActionsSnake);
});
test('should be called with correct check url, method, signal', async () => {
it('should be called with correct check url, method, signal', async () => {
await getCaseUserActions(basicCase.id, abortCtrl.signal);
expect(fetchMock).toHaveBeenCalledWith(`${CASES_URL}/${basicCase.id}/user_actions`, {
method: 'GET',
@ -471,12 +471,12 @@ describe('Cases API', () => {
});
});
test('should return correct response', async () => {
it('should return correct response', async () => {
const resp = await getCaseUserActions(basicCase.id, abortCtrl.signal);
expect(resp).toEqual(caseUserActions);
});
test('should not covert to camel case registered attachments', async () => {
it('should not covert to camel case registered attachments', async () => {
fetchMock.mockResolvedValue(caseUserActionsWithRegisteredAttachmentsSnake);
const resp = await getCaseUserActions(basicCase.id, abortCtrl.signal);
expect(resp).toEqual(caseUserActionsWithRegisteredAttachments);
@ -489,7 +489,7 @@ describe('Cases API', () => {
fetchMock.mockResolvedValue(tags);
});
test('should be called with correct check url, method, signal', async () => {
it('should be called with correct check url, method, signal', async () => {
await getTags(abortCtrl.signal, [SECURITY_SOLUTION_OWNER]);
expect(fetchMock).toHaveBeenCalledWith(`${CASES_URL}/tags`, {
method: 'GET',
@ -500,7 +500,7 @@ describe('Cases API', () => {
});
});
test('should return correct response', async () => {
it('should return correct response', async () => {
const resp = await getTags(abortCtrl.signal, [SECURITY_SOLUTION_OWNER]);
expect(resp).toEqual(tags);
});
@ -514,7 +514,7 @@ describe('Cases API', () => {
const data = { description: 'updated description' };
test('should be called with correct check url, method, signal', async () => {
it('should be called with correct check url, method, signal', async () => {
await patchCase(basicCase.id, data, basicCase.version, abortCtrl.signal);
expect(fetchMock).toHaveBeenCalledWith(`${CASES_URL}`, {
@ -526,7 +526,7 @@ describe('Cases API', () => {
});
});
test('should return correct response', async () => {
it('should return correct response', async () => {
const resp = await patchCase(
basicCase.id,
{ description: 'updated description' },
@ -537,7 +537,7 @@ describe('Cases API', () => {
expect(resp).toEqual([basicCase]);
});
test('should not covert to camel case registered attachments', async () => {
it('should not covert to camel case registered attachments', async () => {
fetchMock.mockResolvedValue([caseWithRegisteredAttachmentsSnake]);
const resp = await patchCase(
basicCase.id,
@ -564,7 +564,7 @@ describe('Cases API', () => {
},
];
test('should be called with correct check url, method, signal', async () => {
it('should be called with correct check url, method, signal', async () => {
await updateCases(data, abortCtrl.signal);
expect(fetchMock).toHaveBeenCalledWith(`${CASES_URL}`, {
method: 'PATCH',
@ -573,10 +573,15 @@ describe('Cases API', () => {
});
});
test('should return correct response should not covert to camel case registered attachments', async () => {
it('should return correct response should not covert to camel case registered attachments', async () => {
const resp = await updateCases(data, abortCtrl.signal);
expect(resp).toEqual(cases);
});
it('returns an empty array if the cases are empty', async () => {
const resp = await updateCases([], abortCtrl.signal);
expect(resp).toEqual([]);
});
});
describe('patchComment', () => {
@ -585,7 +590,7 @@ describe('Cases API', () => {
fetchMock.mockResolvedValue(basicCaseSnake);
});
test('should be called with correct check url, method, signal', async () => {
it('should be called with correct check url, method, signal', async () => {
await patchComment({
caseId: basicCase.id,
commentId: basicCase.comments[0].id,
@ -608,7 +613,7 @@ describe('Cases API', () => {
});
});
test('should return correct response', async () => {
it('should return correct response', async () => {
const resp = await patchComment({
caseId: basicCase.id,
commentId: basicCase.comments[0].id,
@ -620,7 +625,7 @@ describe('Cases API', () => {
expect(resp).toEqual(basicCase);
});
test('should not covert to camel case registered attachments', async () => {
it('should not covert to camel case registered attachments', async () => {
fetchMock.mockResolvedValue(caseWithRegisteredAttachmentsSnake);
const resp = await patchComment({
@ -657,7 +662,7 @@ describe('Cases API', () => {
owner: SECURITY_SOLUTION_OWNER,
};
test('should be called with correct check url, method, signal', async () => {
it('should be called with correct check url, method, signal', async () => {
await postCase(data, abortCtrl.signal);
expect(fetchMock).toHaveBeenCalledWith(`${CASES_URL}`, {
method: 'POST',
@ -666,12 +671,12 @@ describe('Cases API', () => {
});
});
test('should return correct response', async () => {
it('should return correct response', async () => {
const resp = await postCase(data, abortCtrl.signal);
expect(resp).toEqual(basicCase);
});
test('should not covert to camel case registered attachments', async () => {
it('should not covert to camel case registered attachments', async () => {
fetchMock.mockResolvedValue(caseWithRegisteredAttachmentsSnake);
const resp = await postCase(data, abortCtrl.signal);
expect(resp).toEqual(caseWithRegisteredAttachments);
@ -701,7 +706,7 @@ describe('Cases API', () => {
},
];
test('should be called with correct check url, method, signal', async () => {
it('should be called with correct check url, method, signal', async () => {
await createAttachments(data, basicCase.id, abortCtrl.signal);
expect(fetchMock).toHaveBeenCalledWith(
INTERNAL_BULK_CREATE_ATTACHMENTS_URL.replace('{case_id}', basicCase.id),
@ -713,12 +718,12 @@ describe('Cases API', () => {
);
});
test('should return correct response', async () => {
it('should return correct response', async () => {
const resp = await createAttachments(data, basicCase.id, abortCtrl.signal);
expect(resp).toEqual(basicCase);
});
test('should not covert to camel case registered attachments', async () => {
it('should not covert to camel case registered attachments', async () => {
fetchMock.mockResolvedValue(caseWithRegisteredAttachmentsSnake);
const resp = await createAttachments(data, basicCase.id, abortCtrl.signal);
expect(resp).toEqual(caseWithRegisteredAttachments);
@ -733,7 +738,7 @@ describe('Cases API', () => {
fetchMock.mockResolvedValue(pushedCaseSnake);
});
test('should be called with correct check url, method, signal', async () => {
it('should be called with correct check url, method, signal', async () => {
await pushCase(basicCase.id, connectorId, abortCtrl.signal);
expect(fetchMock).toHaveBeenCalledWith(
`${CASES_URL}/${basicCase.id}/connector/${connectorId}/_push`,
@ -745,12 +750,12 @@ describe('Cases API', () => {
);
});
test('should return correct response', async () => {
it('should return correct response', async () => {
const resp = await pushCase(basicCase.id, connectorId, abortCtrl.signal);
expect(resp).toEqual(pushedCase);
});
test('should not covert to camel case registered attachments', async () => {
it('should not covert to camel case registered attachments', async () => {
fetchMock.mockResolvedValue(caseWithRegisteredAttachmentsSnake);
const resp = await pushCase(basicCase.id, connectorId, abortCtrl.signal);
expect(resp).toEqual(caseWithRegisteredAttachments);
@ -764,7 +769,7 @@ describe('Cases API', () => {
});
const commentId = 'ab1234';
test('should be called with correct check url, method, signal', async () => {
it('should be called with correct check url, method, signal', async () => {
const resp = await deleteComment({
caseId: basicCaseId,
commentId,
@ -784,7 +789,7 @@ describe('Cases API', () => {
fetchMock.mockResolvedValue(['siem', 'observability']);
});
test('should be called with correct check url, method, signal', async () => {
it('should be called with correct check url, method, signal', async () => {
const resp = await getFeatureIds(
{ registrationContext: ['security', 'observability.logs'] },
abortCtrl.signal
@ -811,7 +816,7 @@ describe('Cases API', () => {
owner: SECURITY_SOLUTION_OWNER,
};
test('should be called with correct check url, method, signal', async () => {
it('should be called with correct check url, method, signal', async () => {
await postComment(data, basicCase.id, abortCtrl.signal);
expect(fetchMock).toHaveBeenCalledWith(`${CASES_URL}/${basicCase.id}/comments`, {
@ -821,12 +826,12 @@ describe('Cases API', () => {
});
});
test('should return correct response', async () => {
it('should return correct response', async () => {
const resp = await postComment(data, basicCase.id, abortCtrl.signal);
expect(resp).toEqual(basicCase);
});
test('should not covert to camel case registered attachments', async () => {
it('should not covert to camel case registered attachments', async () => {
fetchMock.mockResolvedValue(caseWithRegisteredAttachmentsSnake);
const resp = await postComment(data, basicCase.id, abortCtrl.signal);
expect(resp).toEqual(caseWithRegisteredAttachments);

View file

@ -231,6 +231,10 @@ export const updateCases = async (
cases: CaseUpdateRequest[],
signal: AbortSignal
): Promise<Case[]> => {
if (cases.length === 0) {
return [];
}
const response = await KibanaServices.get().http.fetch<CasesResponse>(CASES_URL, {
method: 'PATCH',
body: JSON.stringify({ cases }),

View file

@ -17,6 +17,10 @@ export const ERROR_DELETING = i18n.translate('xpack.cases.containers.errorDeleti
defaultMessage: 'Error deleting data',
});
export const ERROR_UPDATING = i18n.translate('xpack.cases.containers.errorUpdatingTitle', {
defaultMessage: 'Error updating data',
});
export const UPDATED_CASE = (caseTitle: string) =>
i18n.translate('xpack.cases.containers.updatedCase', {
values: { caseTitle },

View file

@ -37,7 +37,7 @@ export const useUpdateCases = () => {
showSuccessToast(successToasterTitle);
},
onError: (error: ServerError) => {
showErrorToast(error, { title: i18n.ERROR_DELETING });
showErrorToast(error, { title: i18n.ERROR_UPDATING });
},
}
);