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
13 changes: 8 additions & 5 deletions x-pack/plugins/cases/common/api/user.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,14 @@

import * as rt from 'io-ts';

export const UserRT = rt.type({
email: rt.union([rt.undefined, rt.null, rt.string]),
full_name: rt.union([rt.undefined, rt.null, rt.string]),
username: rt.union([rt.undefined, rt.null, rt.string]),
});
export const UserRT = rt.intersection([
rt.type({
email: rt.union([rt.undefined, rt.null, rt.string]),
full_name: rt.union([rt.undefined, rt.null, rt.string]),
username: rt.union([rt.undefined, rt.null, rt.string]),
}),
rt.partial({ profile_uid: rt.string }),
Copy link
Member

Choose a reason for hiding this comment

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

What is the difference betweenrt.partial({ profile_uid: rt.string }) and profile_uid: rt.union([rt.undefined, rt.null, rt.string]) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By using a partial we don't have to set the profile_uid wherever a user object is defined. That way we can avoid updating all the locations in the code where we create a user object. I believe we can also avoid writing a migration.

Copy link
Member

Choose a reason for hiding this comment

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

Make sense. Thanks!

]);

export const UsersRt = rt.array(UserRT);

Expand Down
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
169 changes: 169 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,169 @@
/*
* 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 { elasticUser, 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, {
createdBy: {
// damaged_raccoon uid
Copy link
Member

Choose a reason for hiding this comment

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

😄

profileUid: 'u_J41Oh6L9ki-Vo2tOogS8WRTENzhHurGtRc87NgEAlkc_0',
},
});
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, {
createdBy: {
// damaged_raccoon uid
profileUid: 'u_J41Oh6L9ki-Vo2tOogS8WRTENzhHurGtRc87NgEAlkc_0',
},
});
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 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(
<TestProviders>
<EuiCommentList comments={createdUserAction} />
</TestProviders>
);

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(
<TestProviders>
<EuiCommentList comments={createdUserAction} />
</TestProviders>
);

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