From a5c37d79ac8aec3d26fdd59e0ad080ddff0154b1 Mon Sep 17 00:00:00 2001 From: Jonathan Buttner Date: Tue, 30 Aug 2022 11:50:31 -0400 Subject: [PATCH] Addressing comma and uniq feedback --- .../components/case_view_activity.tsx | 3 +- .../user_actions/assignees.test.tsx | 321 +++++++++++------- .../components/user_actions/assignees.tsx | 6 +- .../use_get_case_user_actions.test.tsx | 36 +- .../containers/use_get_case_user_actions.tsx | 8 +- 5 files changed, 233 insertions(+), 141 deletions(-) diff --git a/x-pack/plugins/cases/public/components/case_view/components/case_view_activity.tsx b/x-pack/plugins/cases/public/components/case_view/components/case_view_activity.tsx index 63be7009f8c15..07cf815555f61 100644 --- a/x-pack/plugins/cases/public/components/case_view/components/case_view_activity.tsx +++ b/x-pack/plugins/cases/public/components/case_view/components/case_view_activity.tsx @@ -58,7 +58,8 @@ export const CaseViewActivity = ({ [caseData.assignees] ); - const uidsToRetrieve = uniq([...(userActionsData?.profileUids ?? []), ...assignees]); + const userActionProfileUids = Array.from(userActionsData?.profileUids.values() ?? []); + const uidsToRetrieve = uniq([...userActionProfileUids, ...assignees]); const { data: userProfiles, isLoading: isLoadingUserProfiles } = useBulkGetUserProfiles({ uids: uidsToRetrieve, diff --git a/x-pack/plugins/cases/public/components/user_actions/assignees.test.tsx b/x-pack/plugins/cases/public/components/user_actions/assignees.test.tsx index f27721fa5c04a..57cde43c9fee6 100644 --- a/x-pack/plugins/cases/public/components/user_actions/assignees.test.tsx +++ b/x-pack/plugins/cases/public/components/user_actions/assignees.test.tsx @@ -12,158 +12,219 @@ import { render, screen } from '@testing-library/react'; import { Actions } from '../../../common/api'; import { elasticUser, getUserAction } from '../../containers/mock'; import { TestProviders } from '../../common/mock'; -import { createAssigneesUserActionBuilder } from './assignees'; +import { createAssigneesUserActionBuilder, shouldAddAnd, shouldAddComma } from './assignees'; import { getMockBuilderArgs } from './mock'; jest.mock('../../common/lib/kibana'); jest.mock('../../common/navigation/hooks'); describe('createAssigneesUserActionBuilder', () => { - const builderArgs = getMockBuilderArgs(); - - beforeEach(() => { - jest.clearAllMocks(); - }); - - it('renders assigned users', () => { - const userAction = getUserAction('assignees', Actions.add, { - createdBy: { - // damaged_raccoon uid - profileUid: 'u_J41Oh6L9ki-Vo2tOogS8WRTENzhHurGtRc87NgEAlkc_0', - }, + describe('shouldAddComma', () => { + it('returns false if there are only 2 items', () => { + expect(shouldAddComma(0, 2)).toBeFalsy(); }); - const builder = createAssigneesUserActionBuilder({ - ...builderArgs, - userAction, - }); - - const createdUserAction = builder.build(); - render( - - - - ); - - expect(screen.getByText('assigned')).toBeInTheDocument(); - expect(screen.getByText('themselves,')).toBeInTheDocument(); - expect(screen.getByText('Physical Dinosaur')).toBeInTheDocument(); - expect(screen.getByTestId('ua-assignee-physical_dinosaur')).toContainElement( - screen.getByText('and') - ); - }); - - it('renders unassigned users', () => { - const userAction = getUserAction('assignees', Actions.delete, { - createdBy: { - // damaged_raccoon uid - profileUid: 'u_J41Oh6L9ki-Vo2tOogS8WRTENzhHurGtRc87NgEAlkc_0', - }, - }); - const builder = createAssigneesUserActionBuilder({ - ...builderArgs, - userAction, + it('returns false it is the last items', () => { + expect(shouldAddComma(2, 3)).toBeFalsy(); }); - - const createdUserAction = builder.build(); - render( - - - - ); - - expect(screen.getByText('unassigned')).toBeInTheDocument(); - expect(screen.getByText('themselves,')).toBeInTheDocument(); - expect(screen.getByText('Physical Dinosaur')).toBeInTheDocument(); - - expect(screen.getByTestId('ua-assignee-physical_dinosaur')).toContainElement( - screen.getByText('and') - ); }); - it('renders a single assigned user', () => { - const userAction = getUserAction('assignees', Actions.add, { - payload: { - assignees: [ - // only render the physical dinosaur - { uid: 'u_A_tM4n0wPkdiQ9smmd8o0Hr_h61XQfu8aRPh9GMoRoc_0' }, - ], - }, + describe('shouldAddAnd', () => { + it('returns false if there is only 1 item', () => { + expect(shouldAddAnd(0, 1)).toBeFalsy(); }); - const builder = createAssigneesUserActionBuilder({ - ...builderArgs, - userAction, - }); - - const createdUserAction = builder.build(); - render( - - - - ); - expect(screen.getByText('Physical Dinosaur')).toBeInTheDocument(); - expect(screen.queryByText('themselves,')).not.toBeInTheDocument(); - expect(screen.queryByText('and')).not.toBeInTheDocument(); + it('returns false it is not the last items', () => { + expect(shouldAddAnd(1, 3)).toBeFalsy(); + }); }); - it('renders a single assigned user that is themselves using matching profile uids', () => { - const userAction = getUserAction('assignees', Actions.add, { - createdBy: { - ...elasticUser, - profileUid: 'u_J41Oh6L9ki-Vo2tOogS8WRTENzhHurGtRc87NgEAlkc_0', - }, - payload: { - assignees: [ - // only render the damaged raccoon which is the current user - { uid: 'u_J41Oh6L9ki-Vo2tOogS8WRTENzhHurGtRc87NgEAlkc_0' }, - ], - }, - }); - const builder = createAssigneesUserActionBuilder({ - ...builderArgs, - userAction, + describe('component', () => { + const builderArgs = getMockBuilderArgs(); + + beforeEach(() => { + jest.clearAllMocks(); }); - const createdUserAction = builder.build(); - render( - - - - ); + it('renders assigned users', () => { + const userAction = getUserAction('assignees', Actions.add, { + createdBy: { + // damaged_raccoon uid + profileUid: 'u_J41Oh6L9ki-Vo2tOogS8WRTENzhHurGtRc87NgEAlkc_0', + }, + }); + const builder = createAssigneesUserActionBuilder({ + ...builderArgs, + userAction, + }); + + const createdUserAction = builder.build(); + render( + + + + ); + + expect(screen.getByText('assigned')).toBeInTheDocument(); + expect(screen.getByText('themselves')).toBeInTheDocument(); + expect(screen.getByText('Physical Dinosaur')).toBeInTheDocument(); + + expect(screen.getByTestId('ua-assignee-physical_dinosaur')).toContainElement( + screen.getByText('and') + ); + }); - expect(screen.getByText('themselves')).toBeInTheDocument(); - expect(screen.queryByText('Physical Dinosaur')).not.toBeInTheDocument(); - expect(screen.queryByText('and')).not.toBeInTheDocument(); - }); + it('renders assigned users with a comma', () => { + const userAction = getUserAction('assignees', Actions.add, { + createdBy: { + // damaged_raccoon uid + profileUid: 'u_J41Oh6L9ki-Vo2tOogS8WRTENzhHurGtRc87NgEAlkc_0', + }, + payload: { + assignees: [ + // These values map to uids in x-pack/plugins/cases/public/containers/user_profiles/api.mock.ts + { uid: 'u_J41Oh6L9ki-Vo2tOogS8WRTENzhHurGtRc87NgEAlkc_0' }, + { uid: 'u_A_tM4n0wPkdiQ9smmd8o0Hr_h61XQfu8aRPh9GMoRoc_0' }, + { uid: 'u_9xDEQqUqoYCnFnPPLq5mIRHKL8gBTo_NiKgOnd5gGk0_0' }, + ], + }, + }); + const builder = createAssigneesUserActionBuilder({ + ...builderArgs, + userAction, + }); + + const createdUserAction = builder.build(); + render( + + + + ); + + expect(screen.getByText('assigned')).toBeInTheDocument(); + expect(screen.getByText('themselves,')).toBeInTheDocument(); + expect(screen.getByText('Physical Dinosaur')).toBeInTheDocument(); + + expect(screen.getByTestId('ua-assignee-physical_dinosaur')).toContainElement( + screen.getByText(',') + ); + + expect(screen.getByText('Wet Dingo')).toBeInTheDocument(); + expect(screen.getByTestId('ua-assignee-wet_dingo')).toContainElement(screen.getByText('and')); + }); - it('renders a single assigned user that is themselves using matching usernames', () => { - const userAction = getUserAction('assignees', Actions.add, { - createdBy: { - ...elasticUser, - username: 'damaged_raccoon', - }, - payload: { - assignees: [ - // only render the damaged raccoon which is the current user - { uid: 'u_J41Oh6L9ki-Vo2tOogS8WRTENzhHurGtRc87NgEAlkc_0' }, - ], - }, + it('renders unassigned users', () => { + const userAction = getUserAction('assignees', Actions.delete, { + createdBy: { + // damaged_raccoon uid + profileUid: 'u_J41Oh6L9ki-Vo2tOogS8WRTENzhHurGtRc87NgEAlkc_0', + }, + }); + const builder = createAssigneesUserActionBuilder({ + ...builderArgs, + userAction, + }); + + const createdUserAction = builder.build(); + render( + + + + ); + + expect(screen.getByText('unassigned')).toBeInTheDocument(); + expect(screen.getByText('themselves')).toBeInTheDocument(); + expect(screen.getByText('Physical Dinosaur')).toBeInTheDocument(); + + expect(screen.getByTestId('ua-assignee-physical_dinosaur')).toContainElement( + screen.getByText('and') + ); }); - const builder = createAssigneesUserActionBuilder({ - ...builderArgs, - userAction, + + it('renders a single assigned user', () => { + const userAction = getUserAction('assignees', Actions.add, { + payload: { + assignees: [ + // only render the physical dinosaur + { uid: 'u_A_tM4n0wPkdiQ9smmd8o0Hr_h61XQfu8aRPh9GMoRoc_0' }, + ], + }, + }); + const builder = createAssigneesUserActionBuilder({ + ...builderArgs, + userAction, + }); + + const createdUserAction = builder.build(); + render( + + + + ); + + expect(screen.getByText('Physical Dinosaur')).toBeInTheDocument(); + expect(screen.queryByText('themselves,')).not.toBeInTheDocument(); + expect(screen.queryByText('and')).not.toBeInTheDocument(); }); - const createdUserAction = builder.build(); - render( - - - - ); + it('renders a single assigned user that is themselves using matching profile uids', () => { + const userAction = getUserAction('assignees', Actions.add, { + createdBy: { + ...elasticUser, + profileUid: 'u_J41Oh6L9ki-Vo2tOogS8WRTENzhHurGtRc87NgEAlkc_0', + }, + payload: { + assignees: [ + // only render the damaged raccoon which is the current user + { uid: 'u_J41Oh6L9ki-Vo2tOogS8WRTENzhHurGtRc87NgEAlkc_0' }, + ], + }, + }); + const builder = createAssigneesUserActionBuilder({ + ...builderArgs, + userAction, + }); + + const createdUserAction = builder.build(); + render( + + + + ); + + expect(screen.getByText('themselves')).toBeInTheDocument(); + expect(screen.queryByText('Physical Dinosaur')).not.toBeInTheDocument(); + expect(screen.queryByText('and')).not.toBeInTheDocument(); + }); - expect(screen.getByText('themselves')).toBeInTheDocument(); - expect(screen.queryByText('Physical Dinosaur')).not.toBeInTheDocument(); - expect(screen.queryByText('and')).not.toBeInTheDocument(); + it('renders a single assigned user that is themselves using matching usernames', () => { + const userAction = getUserAction('assignees', Actions.add, { + createdBy: { + ...elasticUser, + username: 'damaged_raccoon', + }, + payload: { + assignees: [ + // only render the damaged raccoon which is the current user + { uid: 'u_J41Oh6L9ki-Vo2tOogS8WRTENzhHurGtRc87NgEAlkc_0' }, + ], + }, + }); + const builder = createAssigneesUserActionBuilder({ + ...builderArgs, + userAction, + }); + + const createdUserAction = builder.build(); + render( + + + + ); + + expect(screen.getByText('themselves')).toBeInTheDocument(); + expect(screen.queryByText('Physical Dinosaur')).not.toBeInTheDocument(); + expect(screen.queryByText('and')).not.toBeInTheDocument(); + }); }); }); diff --git a/x-pack/plugins/cases/public/components/user_actions/assignees.tsx b/x-pack/plugins/cases/public/components/user_actions/assignees.tsx index 0e16184634a4a..e0a499df05633 100644 --- a/x-pack/plugins/cases/public/components/user_actions/assignees.tsx +++ b/x-pack/plugins/cases/public/components/user_actions/assignees.tsx @@ -42,11 +42,11 @@ const FormatListItem: React.FC<{ }; FormatListItem.displayName = 'FormatListItem'; -const shouldAddComma = (index: number, arrayLength: number) => { - return arrayLength > 0 && index !== arrayLength - 1; +export const shouldAddComma = (index: number, arrayLength: number) => { + return arrayLength > 2 && index !== arrayLength - 1; }; -const shouldAddAnd = (index: number, arrayLength: number) => { +export const shouldAddAnd = (index: number, arrayLength: number) => { return arrayLength > 1 && index === arrayLength - 1; }; diff --git a/x-pack/plugins/cases/public/containers/use_get_case_user_actions.test.tsx b/x-pack/plugins/cases/public/containers/use_get_case_user_actions.test.tsx index cb39b4e7247d5..c97e1a8668b1d 100644 --- a/x-pack/plugins/cases/public/containers/use_get_case_user_actions.test.tsx +++ b/x-pack/plugins/cases/public/containers/use_get_case_user_actions.test.tsx @@ -104,10 +104,38 @@ describe('useGetCaseUserActions', () => { await waitFor(() => { expect(result.current.data?.profileUids).toMatchInlineSnapshot(` - Array [ + Set { "u_J41Oh6L9ki-Vo2tOogS8WRTENzhHurGtRc87NgEAlkc_0", "u_A_tM4n0wPkdiQ9smmd8o0Hr_h61XQfu8aRPh9GMoRoc_0", - ] + } + `); + }); + }); + }); + + it('ignores duplicate uids', async () => { + jest + .spyOn(api, 'getCaseUserActions') + .mockReturnValue( + Promise.resolve([ + ...caseUserActions, + getUserAction('assignees', Actions.add), + getUserAction('assignees', Actions.add), + ]) + ); + + await act(async () => { + const { result } = renderHook( + () => useGetCaseUserActions(basicCase.id, basicCase.connector.id), + { wrapper } + ); + + await waitFor(() => { + expect(result.current.data?.profileUids).toMatchInlineSnapshot(` + Set { + "u_J41Oh6L9ki-Vo2tOogS8WRTENzhHurGtRc87NgEAlkc_0", + "u_A_tM4n0wPkdiQ9smmd8o0Hr_h61XQfu8aRPh9GMoRoc_0", + } `); }); }); @@ -128,10 +156,10 @@ describe('useGetCaseUserActions', () => { await waitFor(() => { expect(result.current.data?.profileUids).toMatchInlineSnapshot(` - Array [ + Set { "u_J41Oh6L9ki-Vo2tOogS8WRTENzhHurGtRc87NgEAlkc_0", "u_A_tM4n0wPkdiQ9smmd8o0Hr_h61XQfu8aRPh9GMoRoc_0", - ] + } `); }); }); diff --git a/x-pack/plugins/cases/public/containers/use_get_case_user_actions.tsx b/x-pack/plugins/cases/public/containers/use_get_case_user_actions.tsx index 04448612ce481..1d36521d0b6f4 100644 --- a/x-pack/plugins/cases/public/containers/use_get_case_user_actions.tsx +++ b/x-pack/plugins/cases/public/containers/use_get_case_user_actions.tsx @@ -209,14 +209,16 @@ export const getPushedInfo = ( }; export const getProfileUids = (userActions: CaseUserActions[]) => { - const uids = userActions.reduce((acc, userAction) => { + const uids = userActions.reduce>((acc, userAction) => { if (userAction.type === ActionTypes.assignees) { const uidsFromPayload = userAction.payload.assignees.map((assignee) => assignee.uid); - acc.push(...uidsFromPayload); + for (const uid of uidsFromPayload) { + acc.add(uid); + } } return acc; - }, []); + }, new Set()); return uids; };