[Security Solution][Detections] Add Bulk Scheduling for rules #140166

jpdjere · 2022-09-07T09:19:04Z

Addresses #2127 (internal)

Summary

Adds feature to bulk edit schedule of rules (interval -runs every- and lookback time)

Screen.Recording.2022-09-07.at.11.35.41.mov

Checklist

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)

jpdjere · 2022-09-07T09:21:08Z

.../security_solution/server/lib/detection_engine/rules/bulk_actions/split_bulk_edit_actions.ts

@@ -29,6 +29,10 @@ export const splitBulkEditActions = (actions: BulkActionEditPayload[]) => {

  return actions.reduce((acc, action) => {
    switch (action.type) {
+      case BulkActionEditType.set_schedule:


updating the schedule needs to update both attributes (interval) and params (the meta.from and from params)

...ck/plugins/security_solution/public/detections/components/rules/schedule_item_form/index.tsx

banderror

Did the first pass of review (checked only some of the changes, not all of them) and tested locally. Looking good so far! Left a few suggestions.

...gins/security_solution/common/detection_engine/schemas/request/perform_bulk_action_schema.ts

...ck/plugins/security_solution/public/detections/components/rules/schedule_item_form/index.tsx

...ic/detections/pages/detection_engine/rules/all/bulk_actions/utils/compute_dry_run_payload.ts

banderror · 2022-09-07T13:41:53Z

@jpdjere when the test plan is finalized for this feature, could you please open a doc ticket similar to elastic/security-docs#2441 and refer to this PR and the plan from it?

elasticmachine · 2022-09-13T08:55:40Z

Pinging @elastic/security-detections-response (Team:Detections and Resp)

elasticmachine · 2022-09-13T08:55:41Z

Pinging @elastic/security-solution (Team: SecuritySolution)

jpdjere · 2022-09-13T08:57:21Z

@banderror @vitaliidm Added tests according to the test plan and marked the PR as ready for review.

Also here is the ticket for the docs.

banderror · 2022-09-19T11:12:25Z

@elastic/security-docs Could you please check the new bulk editing rule schedule form for the wording used in there? We have a bunch of new labels and callouts again :)

We'll deploy this PR to Cloud. If we end up merging it before you have a chance to review it, please ping us and we'll open a new one for you. Also, let us know if you need help with getting credentials for the cloud deployment.

@jpdjere Could you please rebase once again to trigger the cloud deployment?

kibana-ci · 2022-09-19T17:52:59Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`lists`	309	310	+1
`securitySolution`	3076	3093	+17
total			+18

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`@kbn/securitysolution-io-ts-types`	32	33	+1

Async chunks

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

id	before	after	diff
`lists`	149.0KB	149.3KB	+332.0B
`securitySolution`	6.4MB	6.4MB	+11.8KB
total			+12.2KB

Page load bundle

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

id	before	after	diff
`securitySolution`	261.2KB	262.6KB	+1.4KB

Unknown metric groups

API count

id	before	after	diff
`@kbn/securitysolution-io-ts-types`	61	63	+2

ESLint disabled in files

id	before	after	diff
`securitySolution`	71	72	+1

Total ESLint disabled count

id	before	after	diff
`securitySolution`	478	479	+1

History

💚 Build #73316 succeeded a0a529ed26a142f88558288793f110792848da95
💚 Build #73256 succeeded e5f89ec6b83753fe0aa161adb5181fbe9e104eef
💚 Build #72133 succeeded bf2a07a26c942c696aa67c35aed3fa568ab91cfe
💚 Build #71597 succeeded 1e3f0d2e4e9d62082ace9b682594b14cfb0ca89d
💚 Build #71423 succeeded f6cca0cc505702bcbd9b398230377325c0883c7f

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

cc @jpdjere

yctercero · 2022-09-19T19:14:40Z

...ty_solution/public/detections/pages/detection_engine/rules/all/bulk_actions/translations.tsx

+  warningCalloutMessage: (rulesCount: number): JSX.Element => (
+    <FormattedMessage
+      id="xpack.securitySolution.detectionEngine.rules.allRules.bulkActions.edit.setSchedule.warningCalloutMessage"
+      defaultMessage="You're about to apply changes to {rulesCount, plural, one {# selected rule} other {# selected rules}}. The changes you made will be overwritten to the existing Rule schedules and additional look-back time (if any)."


Suggested change

defaultMessage="You're about to apply changes to {rulesCount, plural, one {# selected rule} other {# selected rules}}. The changes you made will be overwritten to the existing Rule schedules and additional look-back time (if any)."

defaultMessage="You're about to apply changes to {rulesCount, plural, one {# selected rule} other {# selected rules}}. The changes you make will overwrite the existing Rule schedules and additional look-back time (if any)."

Something felt a bit confusing with the wording. Maybe @elastic/security-docs has thoughts?

Agree with @yctercero, plus one other edit:

Suggested change

defaultMessage="You're about to apply changes to {rulesCount, plural, one {# selected rule} other {# selected rules}}. The changes you made will be overwritten to the existing Rule schedules and additional look-back time (if any)."

defaultMessage="You're about to apply changes to {rulesCount, plural, one {# selected rule} other {# selected rules}}. The changes you make will overwrite the existing rule schedules and additional look-back time (if any)."

yctercero

Code changes for security-solution-platform LGTM.

joepeeples

Added a couple edits for follow-up PR. Thanks!

joepeeples · 2022-09-19T20:48:15Z

...ty_solution/public/detections/pages/detection_engine/rules/all/bulk_actions/translations.tsx

+  warningCalloutMessage: (rulesCount: number): JSX.Element => (
+    <FormattedMessage
+      id="xpack.securitySolution.detectionEngine.rules.allRules.bulkActions.edit.setSchedule.warningCalloutMessage"
+      defaultMessage="You're about to apply changes to {rulesCount, plural, one {# selected rule} other {# selected rules}}. The changes you made will be overwritten to the existing Rule schedules and additional look-back time (if any)."


Agree with @yctercero, plus one other edit:

Suggested change

defaultMessage="You're about to apply changes to {rulesCount, plural, one {# selected rule} other {# selected rules}}. The changes you made will be overwritten to the existing Rule schedules and additional look-back time (if any)."

defaultMessage="You're about to apply changes to {rulesCount, plural, one {# selected rule} other {# selected rules}}. The changes you make will overwrite the existing rule schedules and additional look-back time (if any)."

joepeeples · 2022-09-19T20:49:50Z

x-pack/plugins/security_solution/cypress/tasks/rules_bulk_edit.ts

+export const assertUpdateScheduleWarningExists = (expectedNumberOfNotMLRules: number) => {
+  cy.get(RULES_BULK_EDIT_SCHEDULES_WARNING).should(
+    'have.text',
+    `You're about to apply changes to ${expectedNumberOfNotMLRules} selected rules. The changes you made will be overwritten to the existing Rule schedules and additional look-back time (if any).`


I think this is just test UI copy, but to match actual UI copy, edit the second sentence to "The changes you make will overwrite the existing rule schedules and additional look-back time (if any)."

…ion (#141604) ## Summary Fixes issues, nits and [expands test coverage](https://docs.google.com/document/d/116x7ITTTJQ6cTiwaGK831_f6Ox7XB3qyLiHxC3Cmf8w/edit#) for PR: #140166 - Extends definition of `TimeUnit` type and its tests - Adds e2e test to test default values of Bulk Schedule flyout - Corrects copy as reported by @elastic/security-docs - Corrects validation for Interval field when editing rule schedule individually ### Checklist Delete any items that are not applicable to this PR. - [ ] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md) - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [ ] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [ ] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/)) - [ ] Any UI touched in this PR does not create any new axe failures (run axe in browser: [FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/), [Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US)) - [ ] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the [docker list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker) - [ ] This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server)) - [ ] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers) ### For maintainers - [ ] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)

…ion (elastic#141604) ## Summary Fixes issues, nits and [expands test coverage](https://docs.google.com/document/d/116x7ITTTJQ6cTiwaGK831_f6Ox7XB3qyLiHxC3Cmf8w/edit#) for PR: elastic#140166 - Extends definition of `TimeUnit` type and its tests - Adds e2e test to test default values of Bulk Schedule flyout - Corrects copy as reported by @elastic/security-docs - Corrects validation for Interval field when editing rule schedule individually ### Checklist Delete any items that are not applicable to this PR. - [ ] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md) - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [ ] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [ ] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/)) - [ ] Any UI touched in this PR does not create any new axe failures (run axe in browser: [FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/), [Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US)) - [ ] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the [docker list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker) - [ ] This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server)) - [ ] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers) ### For maintainers - [ ] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) (cherry picked from commit 9738b04)

…ion (#141604) (#141946) ## Summary Fixes issues, nits and [expands test coverage](https://docs.google.com/document/d/116x7ITTTJQ6cTiwaGK831_f6Ox7XB3qyLiHxC3Cmf8w/edit#) for PR: #140166 - Extends definition of `TimeUnit` type and its tests - Adds e2e test to test default values of Bulk Schedule flyout - Corrects copy as reported by @elastic/security-docs - Corrects validation for Interval field when editing rule schedule individually ### Checklist Delete any items that are not applicable to this PR. - [ ] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md) - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [ ] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [ ] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/)) - [ ] Any UI touched in this PR does not create any new axe failures (run axe in browser: [FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/), [Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US)) - [ ] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the [docker list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker) - [ ] This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server)) - [ ] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers) ### For maintainers - [ ] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) (cherry picked from commit 9738b04) Co-authored-by: Juan Pablo Djeredjian <[email protected]>

jpdjere commented Sep 7, 2022

View reviewed changes

...ck/plugins/security_solution/public/detections/components/rules/schedule_item_form/index.tsx Show resolved Hide resolved

jpdjere commented Sep 7, 2022

View reviewed changes

...ck/plugins/security_solution/public/detections/components/rules/schedule_item_form/index.tsx Outdated Show resolved Hide resolved

jpdjere self-assigned this Sep 7, 2022

banderror reviewed Sep 7, 2022

View reviewed changes

banderror added the Feature:Rule Management Security Solution Detection Rule Management area label Sep 7, 2022

jpdjere force-pushed the 2172-v2-cp-bulk-scheduling-for-rules branch 4 times, most recently from f6cca0c to 1e3f0d2 Compare September 13, 2022 08:31

jpdjere mentioned this pull request Sep 13, 2022

[DOCS] Bulk edit rule schedules elastic/security-docs#2453

Closed

jpdjere marked this pull request as ready for review September 13, 2022 08:55

jpdjere requested review from a team as code owners September 13, 2022 08:55

jpdjere requested a review from banderror September 13, 2022 08:55

jpdjere force-pushed the 2172-v2-cp-bulk-scheduling-for-rules branch 2 times, most recently from bf2a07a to e5f89ec Compare September 19, 2022 09:46

jpdjere requested a review from a team as a code owner September 19, 2022 09:46

banderror added the ci:cloud-deploy Create or update a Cloud deployment label Sep 19, 2022

jpdjere added 18 commits September 19, 2022 19:25

Flyout design

171a22a

Fix label

baba0a3

Update CSS of ScheduleItem

3b1aea8

Type fixes

36cd38b

Refactored types and added unit tests to perform bulk actions

1a64d8e

Add bulkEditActionToRulesClientOperation test

5fee389

Ad test to rulesParamModifier

b715f64

Add validation to form

4f07e29

Make ScheduleItem default to color version

797c5ea

Update type

3ecfc6b

Added TimeLength io type

c4ca544

Fix payload

fa14e5b

Fix type

dbbd4d4

Update tests

3fc9237

Updated TimeDuration io-ts type

5208840

Add integration and e2e tests according to test plan

fb0b855

Add warning callout assertion

985b86c

Fix Schedule container width conflict

ab26845

jpdjere force-pushed the 2172-v2-cp-bulk-scheduling-for-rules branch from a0a529e to ab26845 Compare September 19, 2022 17:26

yctercero reviewed Sep 19, 2022

View reviewed changes

yctercero approved these changes Sep 19, 2022

View reviewed changes

jpdjere merged commit 672bdd2 into elastic:main Sep 19, 2022

kibanamachine added the backport:skip This commit does not require backporting label Sep 19, 2022

joepeeples reviewed Sep 19, 2022

View reviewed changes

jpdjere mentioned this pull request Sep 23, 2022

[Security Solution] Rule bulk schedule fixes and test coverage expansion #141604

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution][Detections] Add Bulk Scheduling for rules #140166

[Security Solution][Detections] Add Bulk Scheduling for rules #140166

jpdjere commented Sep 7, 2022 •

edited by banderror

Loading

jpdjere Sep 7, 2022 •

edited

Loading

banderror left a comment

banderror commented Sep 7, 2022

elasticmachine commented Sep 13, 2022

elasticmachine commented Sep 13, 2022

jpdjere commented Sep 13, 2022

banderror commented Sep 19, 2022 •

edited

Loading

kibana-ci commented Sep 19, 2022 •

edited

Loading

API count

ESLint disabled in files

Total ESLint disabled count

yctercero Sep 19, 2022 •

edited

Loading

joepeeples Sep 19, 2022

yctercero left a comment

joepeeples left a comment

joepeeples Sep 19, 2022

joepeeples Sep 19, 2022

	defaultMessage="You're about to apply changes to {rulesCount, plural, one {# selected rule} other {# selected rules}}. The changes you made will be overwritten to the existing Rule schedules and additional look-back time (if any)."
	defaultMessage="You're about to apply changes to {rulesCount, plural, one {# selected rule} other {# selected rules}}. The changes you make will overwrite the existing Rule schedules and additional look-back time (if any)."

[Security Solution][Detections] Add Bulk Scheduling for rules #140166

[Security Solution][Detections] Add Bulk Scheduling for rules #140166

Conversation

jpdjere commented Sep 7, 2022 • edited by banderror Loading

Summary

Checklist

jpdjere Sep 7, 2022 • edited Loading

Choose a reason for hiding this comment

banderror left a comment

Choose a reason for hiding this comment

banderror commented Sep 7, 2022

elasticmachine commented Sep 13, 2022

elasticmachine commented Sep 13, 2022

jpdjere commented Sep 13, 2022

banderror commented Sep 19, 2022 • edited Loading

kibana-ci commented Sep 19, 2022 • edited Loading

💚 Build Succeeded

Metrics [docs]

Module Count

Public APIs missing comments

Async chunks

Page load bundle

API count

ESLint disabled in files

Total ESLint disabled count

History

yctercero Sep 19, 2022 • edited Loading

Choose a reason for hiding this comment

joepeeples Sep 19, 2022

Choose a reason for hiding this comment

yctercero left a comment

Choose a reason for hiding this comment

joepeeples left a comment

Choose a reason for hiding this comment

joepeeples Sep 19, 2022

Choose a reason for hiding this comment

joepeeples Sep 19, 2022

Choose a reason for hiding this comment

jpdjere commented Sep 7, 2022 •

edited by banderror

Loading

jpdjere Sep 7, 2022 •

edited

Loading

banderror commented Sep 19, 2022 •

edited

Loading

kibana-ci commented Sep 19, 2022 •

edited

Loading

yctercero Sep 19, 2022 •

edited

Loading