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.

Sign up for GitHub

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

Jump to bottom

Ram 145739 use bulk enable disable in UI #145928

Merged
merged 15 commits into from
Nov 28, 2022
Merged

Ram 145739 use bulk enable disable in UI #145928

merged 15 commits into from
Nov 28, 2022

Conversation

guskovaue
Copy link
Contributor

@guskovaue guskovaue commented Nov 21, 2022 •
edited
Loading

Resolves: #145739

Summary

In this PR I am enabling Enable and Disable in menu when Select all is chosen. And trigger new bulk enable/disable API when Enable/Disable option will be chosen.

Checklist

@guskovaue guskovaue added release_note:enhancement Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v8.7.0 labels Nov 21, 2022
@guskovaue guskovaue self-assigned this Nov 21, 2022
@guskovaue guskovaue force-pushed the RAM-145739-use-bulk-enable-disable-in-ui branch from 024f05b to 222d8e0 Compare November 22, 2022 10:33
@guskovaue guskovaue marked this pull request as ready for review November 23, 2022 08:02
@guskovaue guskovaue requested a review from a team as a code owner November 23, 2022 08:02
@elasticmachine
Copy link
Contributor

Pinging @elastic/response-ops (Team:ResponseOps)

@guskovaue
Copy link
Contributor Author

@elasticmachine merge upstream

JiaweiWu
JiaweiWu reviewed Nov 24, 2022
View reviewed changes
Copy link
Contributor

@JiaweiWu JiaweiWu left a comment

Choose a reason for hiding this comment

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

Just doing an initial review, looks pretty good! I have a few comments.

Also, I know we're currently skipping the rules_list unit tests, but I think we should still try to keep them up to date with new features 🙂

x-pack/plugins/triggers_actions_ui/public/application/hooks/use_bulk_operation_toast.tsx Outdated
Comment on lines 82 to 98
const actionToToastMapping = {
DELETE: {
getSuccessfulNotificationText: getSuccessfulDeletionNotificationText,
getFailedNotificationText: getFailedDeletionNotificationText,
getPartialSuccessNotificationText: getPartialSuccessDeletionNotificationText,
},
ENABLE: {
getSuccessfulNotificationText: getSuccessfulEnablingNotificationText,
getFailedNotificationText: getFailedEnablingNotificationText,
getPartialSuccessNotificationText: getPartialSuccessEnablingNotificationText,
},
DISABLE: {
getSuccessfulNotificationText: getSuccessfulDisablingNotificationText,
getFailedNotificationText: getFailedDisablingNotificationText,
getPartialSuccessNotificationText: getPartialSuccessDisablingNotificationText,
},
};
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: let's move this outside of the component, incase it gets reused

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right. It's more clean. b44c57a
I did not move it in another file, just decided better to do it when we need it.

...plugins/triggers_actions_ui/public/application/sections/rules_list/components/rules_list.tsx Outdated
setIsEnablingRules(false);
showToast({ action: 'ENABLE', errors, total });
await refreshRules();
}, [http, selectedIds, filter, setIsEnablingRules, toasts]);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think memoizing this function is worth exposing filter, I think we should just call getFilter lazily because that function could be fairly expensive.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After your message I've realised that filter and getFilter is almost the same and rewrite this part with getFilter:
e250d9f

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@JiaweiWu I did not get how to call getFilter lazily. Can you, please, write more details?

@guskovaue guskovaue force-pushed the RAM-145739-use-bulk-enable-disable-in-ui branch from c264a5c to e250d9f Compare November 24, 2022 10:36
@JiaweiWu
Copy link
Contributor

JiaweiWu commented Nov 24, 2022 •
edited
Loading

I pulled this locally and did some testing, everything seems to be working as intended. Just to be consistent with the other bulk actions, I think we should unselect the items and close the dropdown when we bulk enable/disable them:

2022-11-24.12-00-44.mp4

Otherwise, it looks great!

JiaweiWu
JiaweiWu reviewed Nov 24, 2022
View reviewed changes
...plugins/triggers_actions_ui/public/application/sections/rules_list/components/rules_list.tsx Outdated
setIsEnablingRules(false);
showToast({ action: 'ENABLE', errors, total });
await refreshRules();
}, [http, selectedIds, getFilter, setIsEnablingRules, toasts]);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think toasts is not being use in the function, so we can probably remove it from the dependency array.

Maybe you meant to have showToast?

JiaweiWu
JiaweiWu approved these changes Nov 24, 2022
View reviewed changes
Copy link
Contributor

@JiaweiWu JiaweiWu left a comment

Choose a reason for hiding this comment

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

LGTM, there are 2 comments remaining but they should be easy changes. Feel free to merge after you've made those changes.

@guskovaue
Copy link
Contributor Author

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
triggersActionsUi 468 470 +2

Async chunks

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

id before after diff
triggersActionsUi 681.4KB 674.6KB -6.7KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
triggersActionsUi 110.3KB 110.9KB +589.0B
Unknown metric groups

async chunk count

id before after diff
triggersActionsUi 52 53 +1

ESLint disabled in files

id before after diff
osquery 1 2 +1

ESLint disabled line counts

id before after diff
enterpriseSearch 19 21 +2
fleet 59 65 +6
osquery 109 115 +6
securitySolution 443 449 +6
triggersActionsUi 132 136 +4
total +24

Total ESLint disabled count

id before after diff
enterpriseSearch 20 22 +2
fleet 68 74 +6
osquery 110 117 +7
securitySolution 520 526 +6
triggersActionsUi 135 139 +4
total +25

History

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

cc @guskovaue

@guskovaue guskovaue merged commit c5f2072 into elastic:main Nov 28, 2022
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Nov 28, 2022
@guskovaue guskovaue mentioned this pull request Dec 20, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JiaweiWu JiaweiWu JiaweiWu approved these changes

Assignees

@guskovaue guskovaue

Labels
backport:skip This commit does not require backporting release_note:enhancement review Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v8.7.0
Projects
No open projects
AppEx: ResponseOps - Rules & Alerts Management
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[RAM] use bulk enable and disable UI
5 participants
@guskovaue @elasticmachine @JiaweiWu @kibana-ci @kibanamachine