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

[Cases] Improve bulk actions #142150

Merged
merged 38 commits into from
Oct 5, 2022
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
38 commits
Select commit Hold shift + click to select a range
95daaa5
Use react query for delete cases
cnasikas Sep 23, 2022
6f2bb32
Convert delete to react query
cnasikas Sep 26, 2022
a425bbc
Convert update to react query
cnasikas Sep 26, 2022
cf19789
Convert use_get_cases_status to react query
cnasikas Sep 26, 2022
10a670d
Convert use_get_cases_metrics to react query
cnasikas Sep 26, 2022
4471cb2
Refresh metrics and statuses
cnasikas Sep 26, 2022
f58b6a8
Show loading when updating cases
cnasikas Sep 26, 2022
a43ea7b
Create query key builder
cnasikas Sep 26, 2022
79beccc
Improve refreshing logic
cnasikas Sep 26, 2022
ad9b7cc
Improve delete messages
cnasikas Sep 26, 2022
d2b5c6c
Fix types and tests
cnasikas Sep 27, 2022
6c3eb0f
Improvements
cnasikas Sep 27, 2022
01fe84f
Merge branch 'main' into update_delete_rquery
cnasikas Sep 27, 2022
c3d9508
PR feedback
cnasikas Sep 28, 2022
b8eae38
Merge branch 'main' into update_delete_rquery
cnasikas Sep 28, 2022
cdbf449
Fix bug
cnasikas Sep 28, 2022
40f2894
Refactor actions
cnasikas Sep 28, 2022
384fe85
Merge branch 'main' into refactor_bulk_actions
cnasikas Sep 28, 2022
753977b
Add status actions
cnasikas Sep 29, 2022
c2a354e
Change status to panel
cnasikas Sep 29, 2022
09e4278
Add status column
cnasikas Sep 29, 2022
09220c6
Improvements
cnasikas Sep 30, 2022
d2a64a6
Fix tests & types
cnasikas Sep 30, 2022
939e4cd
Remove comment
cnasikas Sep 30, 2022
fb14028
Improve e2e tests
cnasikas Oct 2, 2022
e6dcaa2
Add unit tests
cnasikas Oct 2, 2022
c774576
Merge branch 'main' into refactor_bulk_actions
cnasikas Oct 3, 2022
10f2763
Add permissions
cnasikas Oct 3, 2022
8d4421c
Fix delete e2e
cnasikas Oct 3, 2022
6af1146
Disable statuses
cnasikas Oct 3, 2022
727410f
Fix i18n
cnasikas Oct 4, 2022
d96956b
PR feedback
cnasikas Oct 4, 2022
98a2032
Disable actions when cases are selected
cnasikas Oct 4, 2022
243a220
Improve modal tests
cnasikas Oct 4, 2022
0454288
Disables checkbox on read only
cnasikas Oct 4, 2022
76c8eb9
Merge branch 'main' into refactor_bulk_actions
cnasikas Oct 5, 2022
b4da899
PR feedback
cnasikas Oct 5, 2022
0b285e3
Merge branch 'main' into refactor_bulk_actions
cnasikas Oct 5, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -12,14 +12,19 @@ import { useDeleteCases } from '../../../containers/use_delete_cases';

import * as i18n from './translations';
import { UseActionProps } from '../types';
import { useCasesContext } from '../../cases_context/use_cases_context';

const getDeleteActionTitle = (totalCases: number): string =>
totalCases > 1 ? i18n.BULK_ACTION_DELETE_LABEL : i18n.DELETE_ACTION_LABEL;

export const useDeleteAction = ({ onAction, onActionSuccess, isDisabled }: UseActionProps) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think it makes sense to move the permissions check within this hook (and the `use_status_action)? I know we'd still need it in the callers to do various other checks though. I think it'd encapsulate the logic a bit more though.

Copy link
Member Author

Choose a reason for hiding this comment

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

I moved the logic to the hooks as you suggested. To avoid the consumers doing permissions checks, the hooks will return canDelete and canUpdateStatus respectively.

const euiTheme = useEuiTheme();
const { permissions } = useCasesContext();
const [isModalVisible, setIsModalVisible] = useState<boolean>(false);
const [caseToBeDeleted, setCaseToBeDeleted] = useState<Case[]>([]);
const canDelete = permissions.delete;
const isActionDisabled = isDisabled || !canDelete;

const onCloseModal = useCallback(() => setIsModalVisible(false), []);
const openModal = useCallback(
(selectedCases: Case[]) => {
Expand All @@ -43,20 +48,20 @@ export const useDeleteAction = ({ onAction, onActionSuccess, isDisabled }: UseAc
);
}, [deleteCases, onActionSuccess, onCloseModal, caseToBeDeleted]);

const color = isDisabled ? euiTheme.euiTheme.colors.disabled : 'danger';
const color = isActionDisabled ? euiTheme.euiTheme.colors.disabled : 'danger';

const getAction = (selectedCases: Case[]) => {
return {
name: <EuiTextColor color={color}>{getDeleteActionTitle(selectedCases.length)}</EuiTextColor>,
onClick: () => openModal(selectedCases),
disabled: isDisabled,
disabled: isActionDisabled,
'data-test-subj': 'cases-bulk-action-delete',
icon: <EuiIcon type="trash" size="m" color={color} />,
key: 'cases-bulk-action-delete',
};
};

return { getAction, isModalVisible, onConfirmDeletion, onCloseModal };
return { getAction, isModalVisible, onConfirmDeletion, onCloseModal, canDelete };
};

export type UseDeleteAction = ReturnType<typeof useDeleteAction>;
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import { Case, CaseStatuses } from '../../../../common';
import * as i18n from './translations';
import { UseActionProps } from '../types';
import { statuses } from '../../status';
import { useCasesContext } from '../../cases_context/use_cases_context';

const getStatusToasterMessage = (status: CaseStatuses, cases: Case[]): string => {
const totalCases = cases.length;
Expand All @@ -33,11 +34,8 @@ interface UseStatusActionProps extends UseActionProps {
selectedStatus?: CaseStatuses;
}

const getCasesWithChanges = (cases: Case[], status: CaseStatuses): Case[] =>
cases.filter((theCase) => theCase.status !== status);

const disableStatus = (cases: Case[], status: CaseStatuses) =>
getCasesWithChanges(cases, status).length === 0;
const shouldDisableStatus = (cases: Case[], status: CaseStatuses) =>
cases.every((theCase) => theCase.status === status);

export const useStatusAction = ({
onAction,
Expand All @@ -46,6 +44,9 @@ export const useStatusAction = ({
selectedStatus,
}: UseStatusActionProps) => {
const { mutate: updateCases } = useUpdateCases();
const { permissions } = useCasesContext();
const canUpdateStatus = permissions.update;
const isActionDisabled = isDisabled || !canUpdateStatus;

const handleUpdateCaseStatus = useCallback(
(selectedCases: Case[], status: CaseStatuses) => {
Expand Down Expand Up @@ -76,30 +77,31 @@ export const useStatusAction = ({
name: statuses[CaseStatuses.open].label,
icon: getStatusIcon(CaseStatuses.open),
onClick: () => handleUpdateCaseStatus(selectedCases, CaseStatuses.open),
disabled: isDisabled || disableStatus(selectedCases, CaseStatuses.open),
disabled: isActionDisabled || shouldDisableStatus(selectedCases, CaseStatuses.open),
'data-test-subj': 'cases-bulk-action-status-open',
key: 'cases-bulk-action-status-open',
},
{
name: statuses[CaseStatuses['in-progress']].label,
icon: getStatusIcon(CaseStatuses['in-progress']),
onClick: () => handleUpdateCaseStatus(selectedCases, CaseStatuses['in-progress']),
disabled: isDisabled || disableStatus(selectedCases, CaseStatuses['in-progress']),
disabled:
isActionDisabled || shouldDisableStatus(selectedCases, CaseStatuses['in-progress']),
'data-test-subj': 'cases-bulk-action-status-in-progress',
key: 'cases-bulk-action-status-in-progress',
},
{
name: statuses[CaseStatuses.closed].label,
icon: getStatusIcon(CaseStatuses.closed),
onClick: () => handleUpdateCaseStatus(selectedCases, CaseStatuses.closed),
disabled: isDisabled || disableStatus(selectedCases, CaseStatuses.closed),
disabled: isActionDisabled || shouldDisableStatus(selectedCases, CaseStatuses.closed),
'data-test-subj': 'cases-bulk-action-status-closed',
key: 'cases-bulk-status-action',
},
];
};

return { getActions };
return { getActions, canUpdateStatus };
};

export type UseStatusAction = ReturnType<typeof useStatusAction>;
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,13 @@ import {
AppMockRenderer,
createAppMockRenderer,
noDeleteCasesPermissions,
readCasesPermissions,
TestProviders,
} from '../../common/mock';
import { useGetCasesMockState, connectorsMock } from '../../containers/mock';

import { StatusAll } from '../../../common/ui/types';
import { CaseSeverity, CaseStatuses } from '../../../common/api';
import { CaseStatuses } from '../../../common/api';
import { SECURITY_SOLUTION_OWNER } from '../../../common/constants';
import { getEmptyTagValue } from '../empty_value';
import { useKibana } from '../../common/lib/kibana';
Expand Down Expand Up @@ -360,60 +361,21 @@ describe('AllCasesListGeneric', () => {
});

it('should call onRowClick when clicking a case with modal=true', async () => {
const theCase = defaultGetCases.data.cases[0];

const wrapper = mount(
<TestProviders>
<AllCasesList isSelectorView={true} onRowClick={onRowClick} />
</TestProviders>
);

wrapper.find('[data-test-subj="cases-table-row-select-1"]').first().simulate('click');
wrapper
.find(`[data-test-subj="cases-table-row-select-${theCase.id}"]`)
.first()
.simulate('click');

await waitFor(() => {
expect(onRowClick).toHaveBeenCalledWith({
assignees: [{ uid: 'u_J41Oh6L9ki-Vo2tOogS8WRTENzhHurGtRc87NgEAlkc_0' }],
closedAt: null,
closedBy: null,
comments: [],
connector: { fields: null, id: '123', name: 'My Connector', type: '.jira' },
createdAt: '2020-02-19T23:06:33.798Z',
createdBy: {
email: '[email protected]',
fullName: 'Leslie Knope',
username: 'lknope',
},
description: 'Security banana Issue',
severity: CaseSeverity.LOW,
duration: null,
externalService: {
connectorId: '123',
connectorName: 'connector name',
externalId: 'external_id',
externalTitle: 'external title',
externalUrl: 'basicPush.com',
pushedAt: '2020-02-20T15:02:57.995Z',
pushedBy: {
email: '[email protected]',
fullName: 'Leslie Knope',
username: 'lknope',
},
},
id: '1',
owner: SECURITY_SOLUTION_OWNER,
status: 'open',
tags: ['coke', 'pepsi'],
title: 'Another horrible breach!!',
totalAlerts: 0,
totalComment: 0,
updatedAt: '2020-02-20T15:02:57.995Z',
updatedBy: {
email: '[email protected]',
fullName: 'Leslie Knope',
username: 'lknope',
},
version: 'WzQ3LDFd',
settings: {
syncAlerts: true,
},
});
expect(onRowClick).toHaveBeenCalledWith(theCase);
});
});

Expand Down Expand Up @@ -746,6 +708,10 @@ describe('AllCasesListGeneric', () => {
it('Renders bulk action', async () => {
const result = appMockRenderer.render(<AllCasesList />);

act(() => {
userEvent.click(result.getByTestId('checkboxSelectAll'));
});

act(() => {
userEvent.click(result.getByText('Bulk actions'));
});
Expand All @@ -760,10 +726,9 @@ describe('AllCasesListGeneric', () => {
'Bulk update status: %s',
async (status) => {
const result = appMockRenderer.render(<AllCasesList />);
const theCase = useGetCasesMockState.data.cases[0];

act(() => {
userEvent.click(result.getByTestId(`checkboxSelectRow-${theCase.id}`));
userEvent.click(result.getByTestId('checkboxSelectAll'));
});

act(() => {
Expand All @@ -787,7 +752,11 @@ describe('AllCasesListGeneric', () => {
await waitForComponentToUpdate();

expect(updateCasesSpy).toBeCalledWith(
[{ id: theCase.id, version: theCase.version, status }],
useGetCasesMockState.data.cases.map(({ id, version }) => ({
id,
version,
status,
})),
expect.anything()
);
}
Expand Down Expand Up @@ -836,6 +805,19 @@ describe('AllCasesListGeneric', () => {
);
});
});

it('should disable the checkboxes when the user has read only permissions', async () => {
appMockRenderer = createAppMockRenderer({ permissions: readCasesPermissions() });
const res = appMockRenderer.render(<AllCasesList />);

expect(res.getByTestId('checkboxSelectAll')).toBeDisabled();

await waitFor(() => {
for (const theCase of defaultGetCases.data.cases) {
expect(res.getByTestId(`checkboxSelectRow-${theCase.id}`)).toBeDisabled();
}
});
});
});

describe('Row actions', () => {
Expand All @@ -857,7 +839,9 @@ describe('AllCasesListGeneric', () => {

it.each(statusTests)('update the status of a case: %s', async (status) => {
const res = appMockRenderer.render(<AllCasesList />);
const theCase = defaultGetCases.data.cases[0];
const openCase = useGetCasesMockState.data.cases[0];
const inProgressCase = useGetCasesMockState.data.cases[1];
const theCase = status === CaseStatuses.open ? inProgressCase : openCase;

await waitFor(() => {
expect(res.getByTestId(`case-action-popover-button-${theCase.id}`)).toBeInTheDocument();
Expand Down Expand Up @@ -887,7 +871,7 @@ describe('AllCasesListGeneric', () => {

await waitFor(() => {
expect(updateCasesSpy).toHaveBeenCalledWith(
[{ id: 'basic-case-id', status, version: 'WzQ3LDFd' }],
[{ id: theCase.id, status, version: theCase.version }],
expect.anything()
);
});
Expand Down Expand Up @@ -927,6 +911,20 @@ describe('AllCasesListGeneric', () => {
expect(deleteCasesSpy).toHaveBeenCalledWith(['basic-case-id'], expect.anything());
});
});

it('should disable row actions when bulk selecting cases', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we add a test for disabling the row actions when a single checkbox is selected?

const res = appMockRenderer.render(<AllCasesList />);

act(() => {
userEvent.click(res.getByTestId('checkboxSelectAll'));
});

await waitFor(() => {
for (const theCase of defaultGetCases.data.cases) {
expect(res.getByTestId(`case-action-popover-button-${theCase.id}`)).toBeDisabled();
}
});
});
});
});
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ import {
} from '../../containers/use_get_cases';
import { useBulkGetUserProfiles } from '../../containers/user_profiles/use_bulk_get_user_profiles';
import { useGetCurrentUserProfile } from '../../containers/user_profiles/use_get_current_user_profile';
import { getAllPermissionsExceptFrom } from '../../utils/permissions';
import { getAllPermissionsExceptFrom, isReadOnlyPermissions } from '../../utils/permissions';
import { useIsLoadingCases } from './use_is_loading_cases';

const ProgressLoader = styled(EuiProgress)`
Expand All @@ -64,7 +64,7 @@ export interface AllCasesListProps {

export const AllCasesList = React.memo<AllCasesListProps>(
({ hiddenStatuses = [], isSelectorView = false, onRowClick }) => {
const { owner } = useCasesContext();
const { owner, permissions } = useCasesContext();
const availableSolutions = useAvailableCasesOwners(getAllPermissionsExceptFrom('delete'));
const isLoading = useIsLoadingCases();

Expand Down Expand Up @@ -204,6 +204,7 @@ export const AllCasesList = React.memo<AllCasesListProps>(
connectors,
onRowClick,
showSolutionColumn: !hasOwner && availableSolutions.length > 1,
disableActions: selectedCases.length > 0,
});

const pagination = useMemo(
Expand All @@ -220,8 +221,9 @@ export const AllCasesList = React.memo<AllCasesListProps>(
() => ({
onSelectionChange: setSelectedCases,
initialSelected: selectedCases,
selectable: () => !isReadOnlyPermissions(permissions),
}),
[selectedCases, setSelectedCases]
[permissions, selectedCases]
);
const isDataEmpty = useMemo(() => data.total === 0, [data]);

Expand Down
Loading