[Cases] Improve bulk actions #142150

cnasikas · 2022-09-28T19:07:44Z

Summary

This PR refactors the current bulk actions and row actions of the cases table. Specifically:

Introduces actions that can be used across cases
Move modal & popover responsibilities outside of components
Restructure code files
Status bulk actions are available for all statuses
Disables changing the status of a case from the status column
Adds the ability to change statuses from the row actions
Changes bulk actions and row actions to be a context menu with panels.

Remarks:

Status is disabled if all selected case has the same status
Permissions are applied for each action

User flow

bulk_actions.mp4

Screenshots

Bulk actions

Row actions

Table

Disabled status

Permissions

Hide bulk actions

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Unit or functional tests were updated or added to match the most common scenarios
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

For maintainers

This was checked for breaking API changes and was labeled appropriately

jonathan-buttner

Refactor is looking great! I left a few comments and questions.

One other thing I noticed while testing is that we used to disable the row actions (was only delete) when a row was selected but we're not doing that anymore. I think that's fine just wanted to make sure that was intended.

The new way:

Old way:

jonathan-buttner · 2022-10-03T19:42:40Z

x-pack/plugins/cases/public/components/all_cases/use_actions.tsx

+    selectedStatus: theCase.status,
+  });
+
+  const getPanels = useCallback(


Should this be a useMemo?

I refactor it as you suggested in the comment below. For that reason, it needs to be a callback.

jonathan-buttner · 2022-10-03T19:47:21Z

x-pack/plugins/cases/public/components/all_cases/use_actions.tsx

+  });
+
+  const getPanels = useCallback(
+    (): EuiContextMenuPanelDescriptor[] => [


nit: I don't know if this is more elegant, but it avoids the multiple checks on canUpdate and canDelete:
Pseudo code:

const actionItems = []; const panels = [{id: 0, items: actionItems, ...}]; if (canUpdate) { actionItems.push() panels.push({id: 1, title: 'status', ...}) } const needsSeparator = actionItems.length > 0 if (canDelete) { if (needsSeparator) { actionItems.push({ separator... }) } actionItems.push({ delete... }) }

I agree with you. It is much better as you suggested.

jonathan-buttner · 2022-10-03T20:26:24Z

x-pack/plugins/cases/public/components/actions/status/use_status_action.tsx

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


Could we change the name to shouldDisableStatus?

I think we could simplify it since we don't need to create an array of cases:

const shouldDisableStatus = (...) => cases.some((theCase) => theCase.status !== status);

jonathan-buttner · 2022-10-03T20:29:26Z

x-pack/plugins/cases/public/components/actions/delete/use_delete_action.tsx

+const getDeleteActionTitle = (totalCases: number): string =>
+  totalCases > 1 ? i18n.BULK_ACTION_DELETE_LABEL : i18n.DELETE_ACTION_LABEL;
+
+export const useDeleteAction = ({ onAction, onActionSuccess, isDisabled }: UseActionProps) => {


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.

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.

jonathan-buttner · 2022-10-03T20:32:54Z

x-pack/plugins/cases/public/components/actions/delete/use_delete_action.tsx

+    return {
+      name: <EuiTextColor color={color}>{getDeleteActionTitle(selectedCases.length)}</EuiTextColor>,
+      onClick: () => openModal(selectedCases),
+      disabled: isDisabled,


I think this might be out of scope of this PR but I think we could probably get rid of this prop if we removed the bulk actions button when not rows are selected. Then we'd only hide this action if the user didn't have deletion permissions.

Gmail also displays additional "bulk" options after you've selected at least 1 row.

I agree with you. We should hide the "bulk" options if the user has not selected any cases. @mdefazio What do you think?

I am thinking of leaving the prop in case we need it in the future. I think it is nice for consumers to be able to disable the actions if needed. What do you think?

jonathan-buttner · 2022-10-03T20:34:26Z

x-pack/plugins/cases/public/components/actions/status/use_status_action.tsx

+        name: statuses[CaseStatuses.open].label,
+        icon: getStatusIcon(CaseStatuses.open),
+        onClick: () => handleUpdateCaseStatus(selectedCases, CaseStatuses.open),
+        disabled: isDisabled || disableStatus(selectedCases, CaseStatuses.open),


Is it possible to add a hover text explaining why a status is disabled?

jonathan-buttner · 2022-10-03T20:38:21Z

x-pack/plugins/cases/public/components/all_cases/use_actions.tsx

+                {
+                  name: (
+                    <FormattedMessage
+                      defaultMessage="Status: {status}"


Do you think we need the current status listed? I feel like it's a bit redundant 🤷‍♂️

Maybe we change it to Change Status. The deletion action is phrased as Delete Case(s) so maybe it'd be more consistent to keep with the verbs.

This was a suggestion by @mdefazio. I will the decision to him 🙂

jonathan-buttner · 2022-10-03T20:41:55Z

x-pack/plugins/cases/public/components/all_cases/use_bulk_actions.tsx

+    onActionSuccess,
+  });
+
+  const panels: EuiContextMenuPanelDescriptor[] = [


Should we memoize this?

This looks similar to the one in use_actions: https://github.com/elastic/kibana/pull/142150/files#diff-42bdf17228f1685e18338f8651455291062e9007acaebd184986950f2ec82ebdR53

Should we place it in a hook?

I change it to a useCallback and refactored it as you suggested here #142150 (comment)

jonathan-buttner · 2022-10-03T20:50:48Z

x-pack/plugins/cases/public/components/all_cases/use_cases_columns.tsx

-          },
-        ]
-      : []),
+    ...(!isSelectorView && (permissions.update || permissions.delete) && actions ? [actions] : []),


Do you think it'd be cleaner to have the use_actions hook manage whether a renderable component is returned? Since use_actions has to do permissions checks anyway maybe we could remove the checks here and have the hook return { actions: null } if it has no actions to display. So this line would look like:

...(!isSelectorView && actions ? [actions] : []),

Done! Btw, I refactored the columns as you suggested here #142150 (comment)

jonathan-buttner · 2022-10-03T20:55:08Z

x-pack/plugins/cases/public/components/all_cases/utility_bar.tsx

+     * Granular permission check for each action is performed
+     * in useBulkActions
+     */
+    const showBulkActions = permissions.update || permissions.delete;


The comment mentions that read permissions is required but we're not checking for here. Should this be const showBulkActions = permssions.read;

Would the user even be able to get to this component if they don't have read permissions though? Maybe we don't need to hide them?

This const showBulkActions = permssions.read; will show the bulk actions if they have read permissions. We only want to show the bulk actions if they can update or delete and not if they can read. If fixed the comment, it was wrong 🙂

cnasikas · 2022-10-04T09:56:44Z

Refactor is looking great! I left a few comments and questions.

One other thing I noticed while testing is that we used to disable the row actions (was only delete) when a row was selected but we're not doing that anymore. I think that's fine just wanted to make sure that was intended.

The new way:

Old way:

Thanks for the amazing feedback @jonathan-buttner! Regarding disabling the row actions, it was not intentional 🙂. Some tables in Kibana disable the actions some do not. The Rules table for example. @mdefazio What do you think? Should we disable the row actions when selecting multiple cases?

mdefazio · 2022-10-04T10:47:07Z

What do you think? Should we disable the row actions when selecting multiple cases?

After going through many of our list/table screens, it seems most of the time, the actions are disabled when selecting a row. This is also the pattern shown on the EUI examples.

So I would suggest we follow the common pattern here and disable the row actions. As we update our Rules page, I will be sure to note that there as well.

One other note that I had was to remove the 'bulk actions' when there is nothing selected. Since this menu is disabled anyway, there's no need to show this button/menu. (Also similar to the eui pattern linked above where the 'delete' button appears only after you've selected a row). I am reaching out to the other designers in Sec and O11y to make sure they are ok with this change, so we can all be consistent. (Is this a shared component that would update all case pages?)

@jonathan-buttner Thanks for catching this one.

cnasikas · 2022-10-04T12:12:58Z

What do you think? Should we disable the row actions when selecting multiple cases?

After going through many of our list/table screens, it seems most of the time, the actions are disabled when selecting a row. This is also the pattern shown on the EUI examples.

So I would suggest we follow the common pattern here and disable the row actions. As we update our Rules page, I will be sure to note that there as well.

One other note that I had was to remove the 'bulk actions' when there is nothing selected. Since this menu is disabled anyway, there's no need to show this button/menu. (Also similar to the eui pattern linked above where the 'delete' button appears only after you've selected a row). I am reaching out to the other designers in Sec and O11y to make sure they are ok with this change, so we can all be consistent. (Is this a shared component that would update all case pages?)

@jonathan-buttner Thanks for catching this one.

Thank you for the feedback @mdefazio! I removed the "bulk actions" when there is nothing selected in d96956b and disabled the row actions in 98a2032

(Is this a shared component that would update all case pages?)

Correct! The component is shared to all solutions.

peteharverson · 2022-10-04T13:50:06Z

x-pack/plugins/cases/public/components/all_cases/all_cases_list.tsx

-    const showActions = permissions.delete && !isSelectorView;
-
-    const columns = useCasesColumns({
+    const { columns } = useCasesColumns({


As discussed, the vertical alignment of these controls is not quite right:

Happy for this to be addressed separately as this hasn't been introduced in this PR.

Thank you @peteharverson! Let's do it on another PR.

peteharverson · 2022-10-04T13:59:31Z

For a user with read only permissions for cases, we still show the checkboxes but no bulk actions are available. Is it possible to hide the selection checkboxes as they don't serve any purpose do they?

mdefazio · 2022-10-04T14:24:57Z

For a user with read only permissions for cases, we still show the checkboxes but no bulk actions are available. Is it possible to hide the selection checkboxes as they don't serve any purpose do they?

Perhaps we instead disable the checkboxes rather than remove? We do this for other read-only content.

cnasikas · 2022-10-04T16:16:39Z

@peteharverson @mdefazio Done! I disabled the checkboxes for a user with read-only permissions.

jonathan-buttner

Mostly nit, but another clarifying question about useCallback

jonathan-buttner · 2022-10-04T17:15:05Z

x-pack/plugins/cases/public/components/all_cases/all_cases_list.test.tsx

@@ -927,6 +911,20 @@ describe('AllCasesListGeneric', () => {
          expect(deleteCasesSpy).toHaveBeenCalledWith(['basic-case-id'], expect.anything());
        });
      });
+
+      it('should disable row actions when bulk selecting cases', async () => {


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

jonathan-buttner · 2022-10-04T17:19:20Z

...k/plugins/cases/public/components/all_cases/selector_modal/all_cases_selector_modal.test.tsx

+    expect(res.queryByTestId('all-cases-modal')).toBeFalsy();
+  });
+
+  it('should hide bulk actions and row actions', async () => {


Is it hiding them by default because nothing was selected? If so, could we update the name to say why they are hidden, maybe something like:

should hide bulk actions and row actions when no rows are selected

Might be nice to have for the tests below too.

Bulk actions and row actions should not be shown in the modal (on selector view). The user should not perform bulk actions when selecting a case from a modal. I changed the title to make it more clear.

jonathan-buttner · 2022-10-04T17:43:59Z

x-pack/plugins/cases/public/components/all_cases/use_actions.tsx

+  const canDelete = deleteAction.canDelete;
+  const canUpdate = statusAction.canUpdateStatus;
+
+  const getPanels = useCallback((): EuiContextMenuPanelDescriptor[] => {


I might be misunderstanding how useCallback works, but I think use callback returns a memoized function. So on each rerender the memory address of the function still stays the same but not necessarily the value it returns. That's helpful when we pass the function itself to another component. We're passing the value to another component because we're calling getPanels

panels={getPanels()}

I believe it will call getPanels on each rendering of the component and the array will get recreated each time.

I think if we were passing the function panels={getPanels} then we'd want a useCallback. I think we want useMemo since we ultimately want an array EuiContextMenuPanelDescriptor[] and not a function that returns an array right?

oh, you are right!! My mistake 🙂! Fixed.

jonathan-buttner

Great work!

kibana-ci · 2022-10-05T13:58:01Z

💚 Build Succeeded

Buildkite Build
Commit: 0b285e3

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`cases`	498	502	+4

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`cases`	371.0KB	372.7KB	+1.7KB

History

💚 Build #78276 succeeded b4da899
💚 Build #78028 succeeded 0454288
💛 Build #77912 was flaky 243a220
💔 Build #77624 failed 8702b89b9654d7cfd0332a1975f7acf1696efaae
💔 Build #77591 failed 6af1146
💔 Build #77422 failed c774576

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @cnasikas

* Use react query for delete cases * Convert delete to react query * Convert update to react query * Convert use_get_cases_status to react query * Convert use_get_cases_metrics to react query * Refresh metrics and statuses * Show loading when updating cases * Create query key builder * Improve refreshing logic * Improve delete messages * Fix types and tests * Improvements * PR feedback * Fix bug * Refactor actions * Add status actions * Change status to panel * Add status column * Improvements * Fix tests & types * Remove comment * Improve e2e tests * Add unit tests * Add permissions * Fix delete e2e * Disable statuses * Fix i18n * PR feedback * Disable actions when cases are selected * Improve modal tests * Disables checkbox on read only * PR feedback

cnasikas added 18 commits September 23, 2022 17:30

Use react query for delete cases

95daaa5

Convert delete to react query

6f2bb32

Convert update to react query

a425bbc

Convert use_get_cases_status to react query

cf19789

Convert use_get_cases_metrics to react query

10a670d

Refresh metrics and statuses

4471cb2

Show loading when updating cases

f58b6a8

Create query key builder

a43ea7b

Improve refreshing logic

79beccc

Improve delete messages

ad9b7cc

Fix types and tests

d2b5c6c

Improvements

6c3eb0f

Merge branch 'main' into update_delete_rquery

01fe84f

PR feedback

c3d9508

Merge branch 'main' into update_delete_rquery

b8eae38

Fix bug

cdbf449

Refactor actions

40f2894

Merge branch 'main' into refactor_bulk_actions

384fe85

cnasikas added release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) Feature:Cases Cases feature v8.6.0 labels Sep 28, 2022

cnasikas self-assigned this Sep 28, 2022

cnasikas mentioned this pull request Sep 28, 2022

[Cases] All cases table: bulk actions & single actions #141371

Closed

9 tasks

cnasikas added 5 commits September 29, 2022 16:42

Add status actions

753977b

Change status to panel

c2a354e

Add status column

09e4278

Improvements

09220c6

Fix tests & types

d2a64a6

jonathan-buttner reviewed Oct 3, 2022

View reviewed changes

cnasikas added 2 commits October 4, 2022 14:26

Fix i18n

727410f

PR feedback

d96956b

cnasikas force-pushed the refactor_bulk_actions branch from 8702b89 to d96956b Compare October 4, 2022 11:26

Disable actions when cases are selected

98a2032

Improve modal tests

243a220

cnasikas requested a review from jonathan-buttner October 4, 2022 12:56

peteharverson reviewed Oct 4, 2022

View reviewed changes

Disables checkbox on read only

0454288

jonathan-buttner reviewed Oct 4, 2022

View reviewed changes

cnasikas added 2 commits October 5, 2022 10:09

Merge branch 'main' into refactor_bulk_actions

76c8eb9

PR feedback

b4da899

cnasikas requested review from jonathan-buttner and peteharverson October 5, 2022 12:54

Merge branch 'main' into refactor_bulk_actions

0b285e3

jonathan-buttner approved these changes Oct 5, 2022

View reviewed changes

cnasikas enabled auto-merge (squash) October 5, 2022 13:57

cnasikas merged commit 3469d64 into elastic:main Oct 5, 2022

cnasikas deleted the refactor_bulk_actions branch October 5, 2022 13:58

cnasikas mentioned this pull request Oct 13, 2022

[Security Solution]: Change status dropdown gets stuck under cases tab if user click multiple times. #141381

Closed

cnasikas mentioned this pull request Nov 17, 2022

[Feature Request][Security Cases] Bulk Status Updates #132235

Closed

[Cases] Improve bulk actions #142150

[Cases] Improve bulk actions #142150

Conversation

cnasikas commented Sep 28, 2022 • edited Loading

Summary

User flow

Screenshots

Bulk actions

Row actions

Table

Disabled status

Permissions

Hide bulk actions

Checklist

For maintainers

jonathan-buttner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cnasikas Oct 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cnasikas commented Oct 4, 2022

mdefazio commented Oct 4, 2022

cnasikas commented Oct 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peteharverson commented Oct 4, 2022

mdefazio commented Oct 4, 2022

cnasikas commented Oct 4, 2022

jonathan-buttner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cnasikas Oct 5, 2022 • edited Loading

Choose a reason for hiding this comment

jonathan-buttner left a comment

Choose a reason for hiding this comment

kibana-ci commented Oct 5, 2022

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

History

cnasikas commented Sep 28, 2022 •

edited

Loading

cnasikas Oct 4, 2022 •

edited

Loading

cnasikas commented Oct 4, 2022 •

edited

Loading

cnasikas Oct 5, 2022 •

edited

Loading