Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ResponseOps][Cases] Add assignee user actions #139392

Merged
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import { useGetCaseUserActions } from '../../containers/use_get_case_user_action
import { useGetTags } from '../../containers/use_get_tags';
import { usePostPushToService } from '../../containers/use_post_push_to_service';
import { useUpdateCase } from '../../containers/use_update_case';
import { useBulkGetUserProfiles } from '../../containers/user_profiles/use_bulk_get_user_profiles';
import { CaseViewPage } from './case_view_page';
import {
caseData,
Expand All @@ -40,6 +41,7 @@ jest.mock('../../containers/use_get_tags');
jest.mock('../../containers/use_get_case');
jest.mock('../../containers/configure/use_connectors');
jest.mock('../../containers/use_post_push_to_service');
jest.mock('../../containers/user_profiles/use_bulk_get_user_profiles');
jest.mock('../user_actions/timestamp', () => ({
UserActionTimestamp: () => <></>,
}));
Expand All @@ -56,6 +58,7 @@ const useGetConnectorsMock = useGetConnectors as jest.Mock;
const usePostPushToServiceMock = usePostPushToService as jest.Mock;
const useGetCaseMetricsMock = useGetCaseMetrics as jest.Mock;
const useGetTagsMock = useGetTags as jest.Mock;
const useBulkGetUserProfilesMock = useBulkGetUserProfiles as jest.Mock;

const mockGetCase = (props: Partial<UseGetCase> = {}) => {
const data = {
Expand Down Expand Up @@ -96,6 +99,7 @@ describe('CaseViewPage', () => {
usePostPushToServiceMock.mockReturnValue({ isLoading: false, pushCaseToExternalService });
useGetConnectorsMock.mockReturnValue({ data: connectorsMock, isLoading: false });
useGetTagsMock.mockReturnValue({ data: [], isLoading: false });
useBulkGetUserProfilesMock.mockReturnValue({ data: new Map(), isLoading: false });

appMockRenderer = createAppMockRenderer();
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@
*/

import { EuiFlexGroup, EuiFlexItem, EuiLoadingContent } from '@elastic/eui';
import React, { useCallback, useMemo } from 'react';
import { isEqual } from 'lodash';
import React, { useCallback, useEffect, useMemo, useState } from 'react';
import { isEqual, uniq } from 'lodash';
import { useGetCurrentUserProfile } from '../../../containers/user_profiles/use_get_current_user_profile';
import { useBulkGetUserProfiles } from '../../../containers/user_profiles/use_bulk_get_user_profiles';
import { useGetConnectors } from '../../../containers/configure/use_connectors';
Expand Down Expand Up @@ -58,10 +58,19 @@ export const CaseViewActivity = ({
[caseData.assignees]
);

const [uidsToRetrieve, setUidsToRetrieve] = useState<string[]>([]);

const { data: userProfiles, isLoading: isLoadingUserProfiles } = useBulkGetUserProfiles({
uids: assignees,
uids: uidsToRetrieve,
});

useEffect(() => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no need to use a useEffect to derive the uids. You can do:

const uidsToRetrieve = uniq([...userActionsData?.profileUids ?? [], ...assignees]);

const { data: userProfiles, isLoading: isLoadingUserProfiles } = useBulkGetUserProfiles({
    uids: uidsToRetrieve,
  });

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah ok, just to make sure I understand, let's say useGetCaseUserActions initially returns undefined because it's loading, we'll call useBulkGetUserProfiles with only the assignees. Once useGetCaseUserActions is finished loading, the component will rerender and it'll automatically call useBulkGetUserProfiles with the updated uidsToRetrieve if we don't include a useEffect?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly! Usually, you do need a useEffect to derive the state from props or functions. These articles are great to read about the topic: https://tkdodo.eu/blog/dont-over-use-state and https://reactjs.org/blog/2018/06/07/you-probably-dont-need-derived-state.html

if (userActionsData?.profileUids !== undefined) {
const uids = uniq([...userActionsData.profileUids, ...assignees]);
setUidsToRetrieve(uids);
}
}, [assignees, userActionsData?.profileUids]);

const { data: currentUserProfile, isLoading: isLoadingCurrentUserProfile } =
useGetCurrentUserProfile();

Expand Down Expand Up @@ -151,10 +160,12 @@ export const CaseViewActivity = ({
{isLoadingUserActions && (
<EuiLoadingContent lines={8} data-test-subj="case-view-loading-content" />
)}
{!isLoadingUserActions && userActionsData && (
{!isLoadingUserActions && userActionsData && userProfiles && (
<EuiFlexGroup direction="column" responsive={false} data-test-subj="case-view-activity">
<EuiFlexItem>
<UserActions
userProfiles={userProfiles}
currentUserProfile={currentUserProfile}
getRuleDetailsHref={ruleDetailsNavigation?.href}
onRuleDetailsClick={ruleDetailsNavigation?.onClick}
caseServices={userActionsData.caseServices}
Expand Down
125 changes: 125 additions & 0 deletions x-pack/plugins/cases/public/components/user_actions/assignees.test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,125 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

import React from 'react';
import { EuiCommentList } from '@elastic/eui';
import { render, screen } from '@testing-library/react';

import { Actions } from '../../../common/api';
import { getUserAction } from '../../containers/mock';
import { TestProviders } from '../../common/mock';
import { createAssigneesUserActionBuilder } 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);
const builder = createAssigneesUserActionBuilder({
...builderArgs,
userAction,
});

const createdUserAction = builder.build();
render(
<TestProviders>
<EuiCommentList comments={createdUserAction} />
</TestProviders>
);

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);
const builder = createAssigneesUserActionBuilder({
...builderArgs,
userAction,
});

const createdUserAction = builder.build();
render(
<TestProviders>
<EuiCommentList comments={createdUserAction} />
</TestProviders>
);

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' },
],
},
});
const builder = createAssigneesUserActionBuilder({
...builderArgs,
userAction,
});

const createdUserAction = builder.build();
render(
<TestProviders>
<EuiCommentList comments={createdUserAction} />
</TestProviders>
);

expect(screen.getByText('Physical Dinosaur')).toBeInTheDocument();
expect(screen.queryByText('themselves,')).not.toBeInTheDocument();
expect(screen.queryByText('and')).not.toBeInTheDocument();
});

it('renders a single assigned user that is themselves', () => {
const userAction = getUserAction('assignees', Actions.add, {
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(
<TestProviders>
<EuiCommentList comments={createdUserAction} />
</TestProviders>
);

expect(screen.getByText('themselves')).toBeInTheDocument();
expect(screen.queryByText('Physical Dinosaur')).not.toBeInTheDocument();
expect(screen.queryByText('and')).not.toBeInTheDocument();
});
});
150 changes: 148 additions & 2 deletions x-pack/plugins/cases/public/components/user_actions/assignees.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,159 @@
* 2.0.
*/

import type { UserActionBuilder } from './types';
import { EuiFlexGroup, EuiFlexItem, EuiText } from '@elastic/eui';
import { UserProfileWithAvatar } from '@kbn/user-profile-components';
import React, { memo } from 'react';
import { Actions, AssigneesUserAction } from '../../../common/api';
import { getName } from '../user_profiles/display_name';
import { Assignee } from '../user_profiles/types';
import { UserToolTip } from '../user_profiles/user_tooltip';
import { createCommonUpdateUserActionBuilder } from './common';
import type { UserActionBuilder, UserActionResponse } from './types';
import * as i18n from './translations';
import { getUsernameDataTestSubj } from '../user_profiles/data_test_subject';

const FormatListItem: React.FC<{
Copy link
Member

@cnasikas cnasikas Aug 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A comma is placed if there are two people assigned. I think it should not have a comma. Like: <name1> and <name2> and not <name1>, and <name2>. Example:

Screenshot 2022-08-30 at 5 58 08 PM

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great catch!

children: React.ReactElement;
index: number;
listSize: number;
}> = ({ children, index, listSize }) => {
if (shouldAddAnd(index, listSize)) {
return (
<>
{i18n.AND_SPACE}
{children}
</>
);
} else if (shouldAddComma(index, listSize)) {
return (
<>
{children}
{','}
</>
);
}

return children;
};
FormatListItem.displayName = 'FormatListItem';

const shouldAddComma = (index: number, arrayLength: number) => {
return arrayLength > 0 && index !== arrayLength - 1;
};

const shouldAddAnd = (index: number, arrayLength: number) => {
return arrayLength > 1 && index === arrayLength - 1;
};

const Themselves: React.FC<{
index: number;
numOfAssigness: number;
}> = ({ index, numOfAssigness }) => (
<FormatListItem index={index} listSize={numOfAssigness}>
<>{i18n.THEMSELVES}</>
</FormatListItem>
);
Themselves.displayName = 'Themselves';

const AssigneeComponent: React.FC<{
assignee: Assignee;
index: number;
numOfAssigness: number;
}> = ({ assignee, index, numOfAssigness }) => (
<FormatListItem index={index} listSize={numOfAssigness}>
<UserToolTip profile={assignee.profile}>
<strong>{getName(assignee.profile?.user)}</strong>
</UserToolTip>
</FormatListItem>
);
AssigneeComponent.displayName = 'Assignee';

interface AssigneesProps {
assignees: Assignee[];
currentUserProfile?: UserProfileWithAvatar;
}

const AssigneesComponent = ({ assignees, currentUserProfile }: AssigneesProps) => (
<>
{assignees.length > 0 && (
<EuiFlexGroup alignItems="center" gutterSize="xs">
peteharverson marked this conversation as resolved.
Show resolved Hide resolved
{assignees.map((assignee, index) => {
const usernameDataTestSubj = getUsernameDataTestSubj(assignee);

return (
<EuiFlexItem
data-test-subj={`ua-assignee-${usernameDataTestSubj}`}
grow={false}
key={assignee.uid}
>
<EuiText size="s" className="eui-textBreakWord">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here I logged in as Bad Hawk, and assigned that user as well as some other users. In the comment it lists Bad Hawk and themselves - should only add an item for themselves:

image

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't reproduce this one a second time! Maybe some stale state?

{isCurrentUser(assignee, currentUserProfile) ? (
<Themselves index={index} numOfAssigness={assignees.length} />
) : (
<AssigneeComponent
assignee={assignee}
index={index}
numOfAssigness={assignees.length}
/>
)}
</EuiText>
</EuiFlexItem>
);
})}
</EuiFlexGroup>
)}
</>
);
AssigneesComponent.displayName = 'Assignees';
const Assignees = memo(AssigneesComponent);

const isCurrentUser = (assignee: Assignee, currentUserProfile?: UserProfileWithAvatar) => {
return assignee.uid === currentUserProfile?.uid;
};

const getLabelTitle = (
userAction: UserActionResponse<AssigneesUserAction>,
userProfiles?: Map<string, UserProfileWithAvatar>,
currentUserProfile?: UserProfileWithAvatar
) => {
const assignees = userAction.payload.assignees.map((assignee) => {
const profile = userProfiles?.get(assignee.uid);
return {
uid: assignee.uid,
profile,
};
});

return (
<EuiFlexGroup alignItems="baseline" gutterSize="xs" component="span" responsive={false}>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just checking this is the desired behavior - the first part of the label uses the user ID, and the rest uses the full names.

image

GitHub by comparison only uses user IDs:

image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah the mocks are showing the display_name, we don't have that within elasticsearch yet so the fallback order is full name -> email -> username

Once we have support for display name the user can customize the name that is displayed.

<EuiFlexItem data-test-subj="ua-assignees-label" grow={false}>
{userAction.action === Actions.add && i18n.ASSIGNED}
{userAction.action === Actions.delete && i18n.UNASSIGNED}
</EuiFlexItem>
<EuiFlexItem grow={false}>
<Assignees assignees={assignees} currentUserProfile={currentUserProfile} />
peteharverson marked this conversation as resolved.
Show resolved Hide resolved
</EuiFlexItem>
</EuiFlexGroup>
);
};

export const createAssigneesUserActionBuilder: UserActionBuilder = ({
userAction,
handleOutlineComment,
userProfiles,
currentUserProfile,
}) => ({
build: () => {
return [];
const assigneesUserAction = userAction as UserActionResponse<AssigneesUserAction>;
const label = getLabelTitle(assigneesUserAction, userProfiles, currentUserProfile);
const commonBuilder = createCommonUpdateUserActionBuilder({
userAction,
handleOutlineComment,
label,
icon: 'userAvatar',
});

return commonBuilder.build();
},
});
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,12 @@ const getCreateCommentUserAction = ({
comment: Comment;
} & Omit<
UserActionBuilderArgs,
'caseServices' | 'comments' | 'index' | 'handleOutlineComment'
| 'caseServices'
| 'comments'
| 'index'
| 'handleOutlineComment'
| 'userProfiles'
| 'currentUserProfile'
>): EuiCommentProps[] => {
switch (comment.type) {
case CommentType.user:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ const onShowAlertDetails = jest.fn();
const defaultProps = {
caseServices: {},
caseUserActions: [],
userProfiles: new Map(),
connectors: [],
actionsNavigation: { href: jest.fn(), onClick: jest.fn() },
getRuleDetailsHref: jest.fn(),
Expand Down Expand Up @@ -440,6 +441,7 @@ describe(`UserActions`, () => {
).toBe('lock');
});
});

it('shows a lockOpen icon if the action is unisolate/release', async () => {
const isolateAction = [getHostIsolationUserAction()];
const props = {
Expand Down
Loading