[Security Solution] Add rule snooze settings on the rule details page #155407

maximpn · 2023-04-20T13:51:27Z

Addresses: #155406

Summary

This PR adds rule snoozing support on the Rule Details page.

Screen.Recording.2023-04-20.at.15.49.31.mov

Checklist

Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios

elasticmachine · 2023-04-20T15:14:16Z

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

elasticmachine · 2023-04-20T15:14:17Z

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

xcrzx

Thank you, @maximpn, for working on this feature! I have tested the PR locally, and it works as expected. Overall, the implementation looks good. I have left a few comments for further improvements; please let me know if you want to discuss them over Zoom.

xcrzx · 2023-04-21T09:24:38Z

.../security_solution/public/detection_engine/rule_details_ui/pages/rule_details/index.test.tsx

+jest.mock('./components/rule_details_snooze_settings', () => ({
+  RuleDetailsSnoozeSettings: () => <></>,
+}));


Wow, it's the 14th mock added to this test file 😅.
Honestly, it might be better just to delete this messy test altogether.

Relying heavily on mocks in a single test isn't great for several reasons:

Bad readability: Too many mocks make reading and understanding the test difficult. In this case, there are ~170 LoC mock definitions, roughly half of the test file.

Fragility: Mocks can cause tests to break easily when the code changes. This is because mocks assume certain things about how the system works, and if those things change, the test will fail even if everything else is still working fine.

Violation of encapsulation: Mocks usually rely on implementation detail. This makes introducing code changes without breaking tests hard, slowing development.

Fake test scenarios: When using too many mocks, you basically test mocks instead of actual code. This can cause tests to pass or fail regardless of the existing system behavior.

I totally agree with the comment though I'd like to abstain from any other changes in the test file in this PR. It more or less outside of scope here so I'd rather address test file refactoring in a subsequent PR if you don't mind.

xcrzx · 2023-04-21T09:27:41Z

...tection_engine/rule_management_ui/components/rules_table/rules_table/rules_table_context.tsx

+  /**
+   * Rule SO's id (not ruleId) to snooze settings map,
+   */
+  data: Record<string, RuleSnoozeSettings>;


Not sure how "Rule SO's id" is related to this structure.

The comment is fixed, it should reflect the data field's essence better.

xcrzx · 2023-04-21T10:11:50Z

...ugins/security_solution/public/detection_engine/rule_details_ui/pages/rule_details/index.tsx

The layout is a bit off on smaller screens:

Unfortunately this problem takes more time to be fixed. I've created a task to address this bug after feature freeze.

xcrzx · 2023-04-21T10:31:33Z

...y_solution/public/detection_engine/rule_management_ui/components/rules_table/use_columns.tsx

+  const {
+    state: { rulesSnoozeSettings },
+  } = useRulesTableContext();
+
  return useMemo(
    () => ({
      field: 'snooze',
      name: i18n.COLUMN_SNOOZE,
-      render: (_, rule: Rule) => <RuleSnoozeBadge id={rule.id} />,
+      render: (_, rule: Rule) => {
+        const snoozeSettings = rulesSnoozeSettings.data[rule.id];
+        const { isFetching, isError } = rulesSnoozeSettings;
+
+        return (
+          <RuleSnoozeBadge
+            snoozeSettings={snoozeSettings}
+            error={
+              isError || (!snoozeSettings && !isFetching)
+                ? rulesTableI18n.UNABLE_TO_FETCH_RULES_SNOOZE_SETTINGS
+                : undefined
+            }
+          />
+        );
+      },


It would be nice to abstract the settings and error retrieval logic into a reusable hook. Something like:

const { snoozingSettings, error } = useRuleSnoozingSettings(rule.id)

Moreover, this logic is duplicated in the RuleDetailsSnoozeSettings component. It would be better to encapsulate it inside the RuleSnoozeBadge component instead of handling the data access part outside. The component might try to get the rule settings from the table context if it's available and fall back to data fetching in other cases.

Good point. I'm gonna address this in the next PR for adding support of rule snooze on the rule editing page.

xcrzx · 2023-04-21T10:56:51Z

...tection_engine/rule_management_ui/components/rules_table/rules_table/rules_table_context.tsx

@@ -40,8 +40,18 @@ import { RuleSource } from './rules_table_saved_state';
 import { useRulesTableSavedState } from './use_rules_table_saved_state';

 interface RulesSnoozeSettings {


Not related to this PR directly.

I was thinking about the snooze state inside the table context, and it seems a bit alien there. If I understood correctly, its purpose is to batch outgoing HTTP requests, i.e., send a single rule snooze request instead of 1 x N table rows. So I think it would be better to create an abstraction just for that. It could be a data hook that doesn't trigger a request immediately but waits some time and accumulates request params if other similar hooks are present on the page. Something similar to the GraphQL dataloader batching functionality but adapted for usage with react query.

Agree, I also had some similar ideas already. My main concern is to make it simple, maintainable and reusable as creating such functionality only for one feature is time consuming. I think we can take the thoughts into account and analyze the possibilities when the snoozing epic is done.

kibana-ci · 2023-04-21T18:38:57Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: caf95b9

Failed CI Steps

Security Solution Tests #1

Test Failures

[job] [logs] Security Solution Tests #1 / Detection rules, bulk edit Timeline templates Apply timeline template to custom rules

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`securitySolution`	3831	3832	+1

Async chunks

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

id	before	after	diff
`securitySolution`	9.1MB	9.1MB	+808.0B

Unknown metric groups

ESLint disabled line counts

id	before	after	diff
`securitySolution`	394	397	+3

Total ESLint disabled count

id	before	after	diff
`securitySolution`	474	477	+3

History

💛 Build #121892 was flaky d8da26b
💔 Build #121883 failed ede1b07
💚 Build #121588 succeeded 543fbe5

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

cc @maximpn

xcrzx

LGTM 👍

add rule snooze settings on the rule details page

543fbe5

maximpn self-assigned this Apr 20, 2023

maximpn marked this pull request as ready for review April 20, 2023 15:14

maximpn requested review from a team as code owners April 20, 2023 15:14

maximpn requested a review from xcrzx April 20, 2023 15:14

banderror removed the Feature:Rule Creation Security Solution Detection Rule Creation workflow label Apr 20, 2023

maximpn added 2 commits April 21, 2023 09:32

take into account deleted rules

ede1b07

fix typings

d8da26b

xcrzx reviewed Apr 21, 2023

View reviewed changes

maximpn added 3 commits April 21, 2023 17:13

make a comment more clear

5ed58d0

rename rule snooze badge component's file

f09f722

do not hide rule snooze settings upon rule status refresh

caf95b9

xcrzx approved these changes Apr 24, 2023

View reviewed changes

maximpn merged commit 84e6e36 into elastic:main Apr 24, 2023

kibanamachine added the backport:skip This commit does not require backporting label Apr 24, 2023

maximpn deleted the support-snooze-settings-on-rule-details-page branch April 24, 2023 09:44

banderror mentioned this pull request Apr 24, 2023

[Security Solution] Support rule snoozing on the Rule Details page #155406

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution] Add rule snooze settings on the rule details page #155407

[Security Solution] Add rule snooze settings on the rule details page #155407

maximpn commented Apr 20, 2023

elasticmachine commented Apr 20, 2023

elasticmachine commented Apr 20, 2023

xcrzx left a comment

xcrzx Apr 21, 2023

maximpn Apr 21, 2023 •

edited

Loading

xcrzx Apr 21, 2023

maximpn Apr 21, 2023

xcrzx Apr 21, 2023

maximpn Apr 24, 2023

xcrzx Apr 21, 2023

maximpn Apr 24, 2023

xcrzx Apr 21, 2023

maximpn Apr 21, 2023

kibana-ci commented Apr 21, 2023

ESLint disabled line counts

Total ESLint disabled count

xcrzx left a comment

		@@ -40,8 +40,18 @@ import { RuleSource } from './rules_table_saved_state';
		import { useRulesTableSavedState } from './use_rules_table_saved_state';

		interface RulesSnoozeSettings {

[Security Solution] Add rule snooze settings on the rule details page #155407

[Security Solution] Add rule snooze settings on the rule details page #155407

Conversation

maximpn commented Apr 20, 2023

Summary

Checklist

elasticmachine commented Apr 20, 2023

elasticmachine commented Apr 20, 2023

xcrzx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maximpn Apr 21, 2023 • 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

kibana-ci commented Apr 21, 2023

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

Metrics [docs]

Module Count

Async chunks

ESLint disabled line counts

Total ESLint disabled count

History

xcrzx left a comment

Choose a reason for hiding this comment

maximpn Apr 21, 2023 •

edited

Loading