-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[Security Solution][Detections] -Fixes rule edit flow bug with max_signals (#92748) #92748
Conversation
@@ -34,11 +34,6 @@ export const activatesRule = () => { | |||
}); | |||
}; | |||
|
|||
export const deactivatesRule = () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not being used anywhere.
Pinging @elastic/security-solution (Team: SecuritySolution) |
Pinging @elastic/security-detections-response (Team:Detections and Resp) |
@@ -251,6 +251,7 @@ const EditRulePageComponent: FC = () => { | |||
rule | |||
), | |||
...(ruleId ? { id: ruleId } : {}), | |||
...(rule != null ? { max_signals: rule.max_signals } : {}), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😎
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checked out, tested locally and all scenarios worked! Thanks for the fix and test coverage to boot! 🙂 LGTM! 👍
💛 Build succeeded, but was flaky
Test FailuresKibana Pipeline / general / "before all" hook for "should contain notes".Timeline notes tab "before all" hook for "should contain notes"Stack Trace
Kibana Pipeline / general / "after all" hook for "should contain notes".Timeline notes tab "after all" hook for "should contain notes"Stack Trace
Kibana Pipeline / general / "before all" hook for "should contain the right query".Timeline query tab Query tab "before all" hook for "should contain the right query"Stack Trace
Metrics [docs]Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: cc @yctercero |
…gnals (elastic#92748) ### Summary Fixes a bug where max_signals was being reverted to it's default value when the rule was edited via the UI.
…gnals (elastic#92748) ### Summary Fixes a bug where max_signals was being reverted to it's default value when the rule was edited via the UI.
…gnals (elastic#92748) ### Summary Fixes a bug where max_signals was being reverted to it's default value when the rule was edited via the UI. # Conflicts: # x-pack/plugins/security_solution/cypress/integration/detection_rules/custom_query_rule.spec.ts
* master: (42 commits) [Lens] Introduces new chart switcher (elastic#91844) [Lens] fix selection when dragging (elastic#93034) Converts usage collection README to .mdx (elastic#92982) Fix expanding document when using saved search data grid (elastic#92999) [SECURITY SOLUTIONS] Bug case connector (elastic#93104) [Security Solution] [Timeline] Bugfix to include unmapped fields in the timeline event details JSON (elastic#92025) [Alerting][Docs] Changed alerting documentation to point to a single source of explaining the configurations. (elastic#92942) [APM] Fix hidden search bar in error pages while loading (elastic#84476) (elastic#93139) [DOCS] Fixes links for machine learning alerts (elastic#92744) [Security Solution][Detections] -Fixes rule edit flow bug with max_signals (elastic#92748) [SecuritySolution][Case] Disable cases on detections in read-only mode (elastic#93010) [Security Solution][Case][Bug] Prevent closing collection when pushing (elastic#93095) [Security Solution][Detections][7.12] Critical Threshold Rule Fixes (elastic#92667) Bump ems landing page to 7.12 (elastic#93065) [App Search] Implement various Relevance Tuning states and form actions (elastic#92644) [actions] for simplistic email servers, set rejectUnauthorized to false (elastic#91760) [Security Solution][Case] Migrate category & subcategory fields of ServiceNow ITSM connector (elastic#93092) Hide instances latency distribution chart (elastic#92869) [Maps] fix MapboxDraw import from pointing to dist just pointing to folder (elastic#93087) [Maps] fix results trimmed tooltip message doubles feature count for line and polygon features (elastic#92932) ...
… playwright-ftr-e2e * 'playwright-ftr-e2e' of github.com:shahzad31/kibana: (38 commits) [chore] Enable core's eslint rule: `@ts-expect-error` (elastic#93086) [Lens] Introduces new chart switcher (elastic#91844) [Lens] fix selection when dragging (elastic#93034) Converts usage collection README to .mdx (elastic#92982) Fix expanding document when using saved search data grid (elastic#92999) [SECURITY SOLUTIONS] Bug case connector (elastic#93104) [Security Solution] [Timeline] Bugfix to include unmapped fields in the timeline event details JSON (elastic#92025) [Alerting][Docs] Changed alerting documentation to point to a single source of explaining the configurations. (elastic#92942) [APM] Fix hidden search bar in error pages while loading (elastic#84476) (elastic#93139) [DOCS] Fixes links for machine learning alerts (elastic#92744) [Security Solution][Detections] -Fixes rule edit flow bug with max_signals (elastic#92748) [SecuritySolution][Case] Disable cases on detections in read-only mode (elastic#93010) [Security Solution][Case][Bug] Prevent closing collection when pushing (elastic#93095) [Security Solution][Detections][7.12] Critical Threshold Rule Fixes (elastic#92667) Bump ems landing page to 7.12 (elastic#93065) [App Search] Implement various Relevance Tuning states and form actions (elastic#92644) [actions] for simplistic email servers, set rejectUnauthorized to false (elastic#91760) [Security Solution][Case] Migrate category & subcategory fields of ServiceNow ITSM connector (elastic#93092) Hide instances latency distribution chart (elastic#92869) [Maps] fix MapboxDraw import from pointing to dist just pointing to folder (elastic#93087) ...
… max_signals (#92748) (#93169) * [Security Solution][Detections] -Fixes rule edit flow bug with max_signals (#92748) ### Summary Fixes a bug where max_signals was being reverted to it's default value when the rule was edited via the UI. # Conflicts: # x-pack/plugins/security_solution/cypress/integration/detection_rules/custom_query_rule.spec.ts * fixing lint issue * Delete bin * Delete kibana * Delete out * Delete testlogs
… ilm/rollup-v2-action * 'ilm/rollup-v2-action' of github.com:elastic/kibana: (30 commits) Fix expanding document when using saved search data grid (#92999) [SECURITY SOLUTIONS] Bug case connector (#93104) [Security Solution] [Timeline] Bugfix to include unmapped fields in the timeline event details JSON (#92025) [Alerting][Docs] Changed alerting documentation to point to a single source of explaining the configurations. (#92942) [APM] Fix hidden search bar in error pages while loading (#84476) (#93139) [DOCS] Fixes links for machine learning alerts (#92744) [Security Solution][Detections] -Fixes rule edit flow bug with max_signals (#92748) [SecuritySolution][Case] Disable cases on detections in read-only mode (#93010) [Security Solution][Case][Bug] Prevent closing collection when pushing (#93095) [Security Solution][Detections][7.12] Critical Threshold Rule Fixes (#92667) Bump ems landing page to 7.12 (#93065) [App Search] Implement various Relevance Tuning states and form actions (#92644) [actions] for simplistic email servers, set rejectUnauthorized to false (#91760) [Security Solution][Case] Migrate category & subcategory fields of ServiceNow ITSM connector (#93092) Hide instances latency distribution chart (#92869) [Maps] fix MapboxDraw import from pointing to dist just pointing to folder (#93087) [Maps] fix results trimmed tooltip message doubles feature count for line and polygon features (#92932) [Security Solution][Detecttions] Indicator enrichment tweaks (#92989) [Maps] fix fit to data on heatmap not working (#92697) [Security Solution][Endpoint][Admin] Fixes policy sticky footer save test (#92919) ...
Summary
Fixes a bug where max_signals was being reverted to it's default value when the rule was edited via the UI.
This PR fixes that and adds tests around the various scenarios.
To test, follow the below steps:
cd x-pack/plugins/security_solution/server/lib/detection_engine/scripts
./post_rule.sh ./rules/queries/query_with_max_signals.json
(creates a rule with max_signals of 500)./get_rules.sh
- check to see that rule continues to havemax_signals: 500
./get_rules.sh
- check to see that rule continues to havemax_signals: 500
./get_rules.sh
- check to see that rule continues to havemax_signals: 500
./get_rules.sh
- check to see that rule continues to havemax_signals: 500
NOTE: The exceptions flow was not previously breaking
max_signals
but I added it as something to test as it had previously caused some issues on thePATCH
.Checklist