[Security Solution][Detections] Update rules count indicator in the Rules table to show pagination info #138902

jpdjere · 2022-08-16T12:26:59Z

Fixes: #121754

Summary

Changes rules count pagination count to format:

"Showing 1-10 of 596 rules" -> when page 1, rules per page is 10 and total rules is 596
"Showing 6-6 of 6 rules" -> when page 2, rules per page is 5 and total rules is 6
See other cases in tests.

Before changes

After changes

Codebase changes

Refactors AllRulesUtilityBar component to separate usage of the utility bar between the all-rules table use-case and the Exceptions table use-case, by creating a new ExceptionsTableUtilityBar component.

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)

For maintainers

This was checked for breaking API changes and was labeled appropriately

elasticmachine · 2022-08-18T13:01:27Z

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

elasticmachine · 2022-08-18T13:01:28Z

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

...plugins/security_solution/public/detections/pages/detection_engine/rules/all/utility_bar.tsx

xcrzx

Thanks for the fix, @jpdjere 👍 Verified locally, rule counts work as expected. Left a couple of minor comments, but other than that, code changes LGTM 🎉

xcrzx · 2022-08-22T10:03:39Z

...solution/public/detections/pages/detection_engine/rules/all/rules_table_utility_bar.test.tsx

+    it.each([
+      [0, 0, 0, 'Showing 0-0 of 0 rules'],
+      [1, 1, 1, 'Showing 1-1 of 1 rule'],
+      [1, 10, 21, 'Showing 1-10 of 21 rules'],
+      [1, 10, 8, 'Showing 1-8 of 8 rules'],
+      [2, 10, 31, 'Showing 11-20 of 31 rules'],
+      [4, 10, 31, 'Showing 31-31 of 31 rules'],
+      [1, 5, 4, 'Showing 1-4 of 4 rules'],
+      [1, 100, 100, 'Showing 1-100 of 100 rules'],
+      [2, 100, 101, 'Showing 101-101 of 101 rules'],
+    ])(


I wonder if we need all of these test cases, do they all contribute to the coverage? Maybe it would be possible to ensure coverage with fewer cases.

True. But I still consider the following cases to be necessary:

Showing 0-0 of 0 rules -----> When there are 0 rules to show Showing 1-1 of 1 rule -----> Special case for a single rule (singular instead of plural) Showing 1-10 of 21 rules -----> Displaying first page, with rules per page being less than total rules Showing 1-8 of 8 rules -----> Displaying first page, with rules per page being more than total rules Showing 11-20 of 31 rules -----> Displaying any other page, with rules per page being less than total rules Showing 101-101 of 101 rules -----> Displaying the last page, which shows the remaining rules, which are less than the rules per page (in this case, just 1)

Okay, I think the fewer tests we have, the better (without harming coverage). So if we could get rid of three extra test cases, that's already something.

Also, if you decide to stick with test.each, it would be great to add some comments to the cases (as you did in the comment above) to clarify what they are supposed to test. Without comments, it's a bit hard to figure out the meaning of tests by looking at the input args only.

...solution/public/detections/pages/detection_engine/rules/all/rules_table_utility_bar.test.tsx

x-pack/plugins/security_solution/public/detections/pages/detection_engine/rules/types.ts

...rity_solution/public/detections/pages/detection_engine/rules/all/rules_table_utility_bar.tsx

banderror

Reviewed the changes, checked out, and tested locally -- all good and works as expected, including the Exception Lists page! 👍

Thank you for splitting the utility bar into two components and adding test coverage. The code is nice and clean - love it!

Great PR overall @jpdjere. Left a few minor comments.

x-pack/plugins/security_solution/cypress/integration/detection_rules/prebuilt_rules.spec.ts

...blic/detections/pages/detection_engine/rules/all/exceptions/exceptions_table_utility_bar.tsx

...solution/public/detections/pages/detection_engine/rules/all/rules_table_utility_bar.test.tsx

...detections/pages/detection_engine/rules/all/exceptions/exceptions_table_utility_bar.test.tsx

...rity_solution/public/detections/pages/detection_engine/rules/all/rules_table_utility_bar.tsx

x-pack/plugins/security_solution/public/detections/pages/detection_engine/rules/types.ts

jpdjere · 2022-08-22T13:09:51Z

...solution/public/detections/pages/detection_engine/rules/all/rules_table_utility_bar.test.tsx

+  });
+});
+
+describe('getShowingRulesParams creates correct label when', () => {


I moved getShowingRulesParams to the same (and only) file in which it is used (rules_table_utility_bar.tsx) and moved its tests to x-pack/plugins/security_solution/public/detections/pages/detection_engine/rules/all/rules_table_utility_bar.test.tsx.

Also, changed style of tests and improved description of test cases. What do you think? @banderror @xcrzx

Looks perfect!

x-pack/plugins/security_solution/cypress/integration/detection_rules/prebuilt_rules.spec.ts

kibana-ci · 2022-08-23T08:47:00Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`securitySolution`	2993	2994	+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`	5.4MB	5.4MB	+167.0B

Unknown metric groups

ESLint disabled in files

id	before	after	diff
`securitySolution`	76	75	-1

Total ESLint disabled count

id	before	after	diff
`securitySolution`	476	475	-1

History

💚 Build #66457 succeeded a27ed0ea7f88a18abcc8d7973c06cc6ac4592479
💔 Build #66419 failed 1f4005889bae6e5875e4e039308d7b7d907255b6
💔 Build #66399 failed 4292a4287e354445ebcc47a5a18c8671e86066d2
💚 Build #66315 succeeded 52c90b3e0d54d63c703f02dbc3134b657f045ec8
💔 Build #66305 failed 89845f74e81bbb70505cfcf4b241ff07616f5729
💚 Build #66034 succeeded 6681359a35726025f199148b9d20f91e84233fe1

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

cc @jpdjere

banderror · 2022-08-23T10:17:24Z

x-pack/plugins/security_solution/cypress/integration/detection_rules/custom_query_rule.spec.ts

@@ -218,18 +217,21 @@ describe('Custom query rules', () => {
            const initialNumberOfRules = rules.length;
            const expectedNumberOfRulesAfterDeletion = initialNumberOfRules - 1;

-            cy.get(SHOWING_RULES_TEXT).should('have.text', `Showing ${initialNumberOfRules} rules`);
+            cy.request({ url: '/api/detection_engine/rules/_find' }).then(({ body }) => {


Nit: we could introduce a constant for this path.

The constant would be defined here:

kibana/x-pack/plugins/security_solution/common/constants.ts

Lines 254 to 263 in 0212338

/**

* Detection engine routes

*/

export const DETECTION_ENGINE_URL = '/api/detection_engine' as const;

export const DETECTION_ENGINE_RULES_URL = `${DETECTION_ENGINE_URL}/rules` as const;

export const DETECTION_ENGINE_PREPACKAGED_URL =

`${DETECTION_ENGINE_RULES_URL}/prepackaged` as const;

export const DETECTION_ENGINE_PRIVILEGES_URL = `${DETECTION_ENGINE_URL}/privileges` as const;

export const DETECTION_ENGINE_INDEX_URL = `${DETECTION_ENGINE_URL}/index` as const;

We would use it in the tests and in the route handler:

kibana/x-pack/plugins/security_solution/server/lib/detection_engine/routes/rules/find_rules_route.ts

Line 26 in 0212338

path: `${DETECTION_ENGINE_RULES_URL}/_find`,

x-pack/plugins/security_solution/cypress/integration/detection_rules/prebuilt_rules.spec.ts

banderror · 2022-08-23T10:22:35Z

x-pack/plugins/security_solution/cypress/integration/detection_rules/prebuilt_rules.spec.ts

+        // Assert the table was refreshed with the rules returned by the API request
+        const ruleNames = rawRules.map((rule) => rule.name);
+        cy.get(RULE_NAME).each(($item) => {
+          expect($item.text()).to.be.oneOf(ruleNames);
+        });


Thank you for adding the comments. They are very useful 🙏

banderror

@jpdjere Alright, I think you addressed all my annoying comments! 😄 The PR looks great. Thank you for your efforts in improving the reliability of tests, much appreciated 🙏

Left a small nit for your consideration.

When you're ready for merging it, let's chat about how to do that and what will be the next steps after the merge.

…ules table to show pagination info (elastic#138902) **Fixes:** elastic#121754 ## Summary Changes rules count pagination count to format: **"Showing 1-10 of 596 rules"** -> when page 1, rules per page is 10 and total rules is 596 **"Showing 6-6 of 6 rules"** -> when page 2, rules per page is 5 and total rules is 6 See other cases in tests. **Before changes** ![image](https://user-images.githubusercontent.com/5354282/184883106-118f0041-5567-4cf8-bfd5-8383e8146018.png) ![image](https://user-images.githubusercontent.com/5354282/184884145-22d30482-cc2d-4796-8f84-d809a8ca6d8a.png) **After changes** ![image](https://user-images.githubusercontent.com/5354282/184882149-d6c55dff-39a0-4a72-94a9-217b2e5ca7ac.png) ![image](https://user-images.githubusercontent.com/5354282/184886334-a107a2c5-31dc-4a7b-9e70-54c87c1630e0.png) ## Codebase changes - Refactors `AllRulesUtilityBar` component to separate usage of the utility bar between the all-rules table use-case and the Exceptions table use-case, by creating a new `ExceptionsTableUtilityBar` component. ### Checklist Delete any items that are not applicable to this PR. - [x] 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) - [x] [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 - [x] 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)) ### 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 147e414)

kibanamachine · 2022-08-23T11:39:34Z

💚 All backports created successfully

Status	Branch	Result
✅	8.4

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

jpdjere · 2022-08-23T12:03:32Z

💚 All backports created successfully

Status	Branch	Result
✅	8.4

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

…ules table to show pagination info (elastic#138902) **Fixes:** elastic#121754 ## Summary Changes rules count pagination count to format: **"Showing 1-10 of 596 rules"** -> when page 1, rules per page is 10 and total rules is 596 **"Showing 6-6 of 6 rules"** -> when page 2, rules per page is 5 and total rules is 6 See other cases in tests. **Before changes** ![image](https://user-images.githubusercontent.com/5354282/184883106-118f0041-5567-4cf8-bfd5-8383e8146018.png) ![image](https://user-images.githubusercontent.com/5354282/184884145-22d30482-cc2d-4796-8f84-d809a8ca6d8a.png) **After changes** ![image](https://user-images.githubusercontent.com/5354282/184882149-d6c55dff-39a0-4a72-94a9-217b2e5ca7ac.png) ![image](https://user-images.githubusercontent.com/5354282/184886334-a107a2c5-31dc-4a7b-9e70-54c87c1630e0.png) ## Codebase changes - Refactors `AllRulesUtilityBar` component to separate usage of the utility bar between the all-rules table use-case and the Exceptions table use-case, by creating a new `ExceptionsTableUtilityBar` component. ### Checklist Delete any items that are not applicable to this PR. - [x] 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) - [x] [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 - [x] 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)) ### 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 147e414)

…ules table to show pagination info (#138902) (#139288) **Fixes:** #121754 ## Summary Changes rules count pagination count to format: **"Showing 1-10 of 596 rules"** -> when page 1, rules per page is 10 and total rules is 596 **"Showing 6-6 of 6 rules"** -> when page 2, rules per page is 5 and total rules is 6 See other cases in tests. **Before changes** ![image](https://user-images.githubusercontent.com/5354282/184883106-118f0041-5567-4cf8-bfd5-8383e8146018.png) ![image](https://user-images.githubusercontent.com/5354282/184884145-22d30482-cc2d-4796-8f84-d809a8ca6d8a.png) **After changes** ![image](https://user-images.githubusercontent.com/5354282/184882149-d6c55dff-39a0-4a72-94a9-217b2e5ca7ac.png) ![image](https://user-images.githubusercontent.com/5354282/184886334-a107a2c5-31dc-4a7b-9e70-54c87c1630e0.png) ## Codebase changes - Refactors `AllRulesUtilityBar` component to separate usage of the utility bar between the all-rules table use-case and the Exceptions table use-case, by creating a new `ExceptionsTableUtilityBar` component. ### Checklist Delete any items that are not applicable to this PR. - [x] 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) - [x] [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 - [x] 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)) ### 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 147e414) Co-authored-by: Juan Pablo Djeredjian <[email protected]>

…ules table to show pagination info (elastic#138902) **Fixes:** elastic#121754 ## Summary Changes rules count pagination count to format: **"Showing 1-10 of 596 rules"** -> when page 1, rules per page is 10 and total rules is 596 **"Showing 6-6 of 6 rules"** -> when page 2, rules per page is 5 and total rules is 6 See other cases in tests. **Before changes** ![image](https://user-images.githubusercontent.com/5354282/184883106-118f0041-5567-4cf8-bfd5-8383e8146018.png) ![image](https://user-images.githubusercontent.com/5354282/184884145-22d30482-cc2d-4796-8f84-d809a8ca6d8a.png) **After changes** ![image](https://user-images.githubusercontent.com/5354282/184882149-d6c55dff-39a0-4a72-94a9-217b2e5ca7ac.png) ![image](https://user-images.githubusercontent.com/5354282/184886334-a107a2c5-31dc-4a7b-9e70-54c87c1630e0.png) ## Codebase changes - Refactors `AllRulesUtilityBar` component to separate usage of the utility bar between the all-rules table use-case and the Exceptions table use-case, by creating a new `ExceptionsTableUtilityBar` component. ### Checklist Delete any items that are not applicable to this PR. - [x] 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) - [x] [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 - [x] 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)) ### 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)

jpdjere changed the title ~~2022 08 16 showing rules display~~ [Security Solution][Detections] Fix incorrect pagination rules count Aug 16, 2022

jpdjere added the ci:no-auto-commit Disable auto-committing changes on CI label Aug 17, 2022

jpdjere force-pushed the 2022-08-16-showing-rules-display branch 2 times, most recently from b2febab to 36add26 Compare August 18, 2022 11:25

banderror assigned jpdjere Aug 18, 2022

banderror changed the title ~~[Security Solution][Detections] Fix incorrect pagination rules count~~ [Security Solution][Detections] Update rules count indicator in the Rules table to show pagination info Aug 18, 2022

banderror marked this pull request as ready for review August 18, 2022 13:01

banderror requested a review from a team as a code owner August 18, 2022 13:01

banderror requested a review from xcrzx August 18, 2022 13:01

banderror self-requested a review August 18, 2022 13:04

jpdjere force-pushed the 2022-08-16-showing-rules-display branch from c6e53cc to 6681359 Compare August 18, 2022 13:26

xcrzx reviewed Aug 18, 2022

View reviewed changes

...plugins/security_solution/public/detections/pages/detection_engine/rules/all/utility_bar.tsx Outdated Show resolved Hide resolved

jpdjere force-pushed the 2022-08-16-showing-rules-display branch 2 times, most recently from d6818d9 to 89845f7 Compare August 22, 2022 08:44

jpdjere requested a review from xcrzx August 22, 2022 08:48

jpdjere force-pushed the 2022-08-16-showing-rules-display branch from 89845f7 to 52c90b3 Compare August 22, 2022 09:34

xcrzx approved these changes Aug 22, 2022

View reviewed changes

banderror reviewed Aug 22, 2022

View reviewed changes

jpdjere commented Aug 22, 2022

View reviewed changes

jpdjere force-pushed the 2022-08-16-showing-rules-display branch from 2c80fbd to a27ed0e Compare August 22, 2022 14:22

jpdjere commented Aug 22, 2022

View reviewed changes

x-pack/plugins/security_solution/cypress/integration/detection_rules/prebuilt_rules.spec.ts Outdated Show resolved Hide resolved

jpdjere added 12 commits August 23, 2022 09:50

Removed complexity warning

164fce3

Fix e2e load prebuilt_rules test

b2575c2

Fix linting

7852288

Split UtilityBar component

912194b

Address comments

98a65a7

Moved getShowingRulesParams and updated tests

7b8894e

Simplify test for component

2d7a216

Lint

79de6ce

Delete unused translations

08b2045

Updated Cypress test

e593631

Update Cypress test

54a51ca

Update custom_query_rule e2e test

a5e684d

jpdjere force-pushed the 2022-08-16-showing-rules-display branch from a27ed0e to a5e684d Compare August 23, 2022 07:50

jpdjere requested a review from banderror August 23, 2022 07:55

banderror reviewed Aug 23, 2022

View reviewed changes

banderror approved these changes Aug 23, 2022

View reviewed changes

jpdjere merged commit 147e414 into elastic:main Aug 23, 2022

jpdjere deleted the 2022-08-16-showing-rules-display branch August 23, 2022 11:36

kibanamachine mentioned this pull request Aug 23, 2022

[8.4] [Security Solution][Detections] Update rules count indicator in the Rules table to show pagination info (#138902) #139288

Merged

jpdjere linked an issue Aug 23, 2022 that may be closed by this pull request

[Security Solution] rules count is wrong #121754

Closed

This was referenced Aug 23, 2022

[Security Solution] rules count is wrong #121754

Closed

[8.4] [Security Solution][Detections] Update rules count indicator in the Rules table to show pagination info (#138902) #139293

Closed

kibanamachine added the v8.4.0 label Aug 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution][Detections] Update rules count indicator in the Rules table to show pagination info #138902

[Security Solution][Detections] Update rules count indicator in the Rules table to show pagination info #138902

jpdjere commented Aug 16, 2022 •

edited by kibanamachine

Loading

elasticmachine commented Aug 18, 2022

elasticmachine commented Aug 18, 2022

xcrzx left a comment

xcrzx Aug 22, 2022

jpdjere Aug 22, 2022

xcrzx Aug 22, 2022

banderror left a comment •

edited

Loading

jpdjere Aug 22, 2022

banderror Aug 22, 2022

kibana-ci commented Aug 23, 2022

ESLint disabled in files

Total ESLint disabled count

banderror Aug 23, 2022

banderror Aug 23, 2022

banderror left a comment

kibanamachine commented Aug 23, 2022

jpdjere commented Aug 23, 2022

	/**
	* Detection engine routes
	*/
	export const DETECTION_ENGINE_URL = '/api/detection_engine' as const;
	export const DETECTION_ENGINE_RULES_URL = `${DETECTION_ENGINE_URL}/rules` as const;
	export const DETECTION_ENGINE_PREPACKAGED_URL =
	`${DETECTION_ENGINE_RULES_URL}/prepackaged` as const;
	export const DETECTION_ENGINE_PRIVILEGES_URL = `${DETECTION_ENGINE_URL}/privileges` as const;
	export const DETECTION_ENGINE_INDEX_URL = `${DETECTION_ENGINE_URL}/index` as const;

[Security Solution][Detections] Update rules count indicator in the Rules table to show pagination info #138902

[Security Solution][Detections] Update rules count indicator in the Rules table to show pagination info #138902

Conversation

jpdjere commented Aug 16, 2022 • edited by kibanamachine Loading

Summary

Codebase changes

Checklist

For maintainers

elasticmachine commented Aug 18, 2022

elasticmachine commented Aug 18, 2022

xcrzx left a comment

Choose a reason for hiding this comment

xcrzx Aug 22, 2022

Choose a reason for hiding this comment

jpdjere Aug 22, 2022

Choose a reason for hiding this comment

xcrzx Aug 22, 2022

Choose a reason for hiding this comment

banderror left a comment • edited Loading

Choose a reason for hiding this comment

jpdjere Aug 22, 2022

Choose a reason for hiding this comment

banderror Aug 22, 2022

Choose a reason for hiding this comment

kibana-ci commented Aug 23, 2022

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

ESLint disabled in files

Total ESLint disabled count

History

banderror Aug 23, 2022

Choose a reason for hiding this comment

banderror Aug 23, 2022

Choose a reason for hiding this comment

banderror left a comment

Choose a reason for hiding this comment

kibanamachine commented Aug 23, 2022

💚 All backports created successfully

Questions ?

jpdjere commented Aug 23, 2022

💚 All backports created successfully

Questions ?

jpdjere commented Aug 16, 2022 •

edited by kibanamachine

Loading

banderror left a comment •

edited

Loading