[Security Solution][List details page]: Manage rules - Selection cleared on navigation #146121

WafaaNasr · 2022-11-23T10:27:00Z

Summary

This PR, is for adding the new Link column Switch instead of using table selection based on the below bug
Addresses [Security Solution]: User is unable to navigate to shared exception list detail page while user click on view list details option. #145683
Remove Tags duplications and filter by tags

Problem

Manage Rules uses the Add to Rule component and this component under the hood is built on top of the In Memory Table

Currently, this table doesn't preserve the selection when the user navigates through the pages, which means if the user had some rules selected on the first page, then navigated to any other page the selection will be cleared if the user clicked on Save which will cause the linked rules to be unlinked from the list without notifying the user.

This could be solved by:

Removing the pagination but the component won't fit in the Add Exception Flyout
Add another column to the table to toggle (link/unlink) using a Switch component with making the table's row not selectable
Make use of the toolsLeft property given by the In Memory Table to design a new way of linking/unlinking
Control the height of the Add to Rule component

We decided in the Sync meeting to try the second option as it will give us full control over the Table data as well as we found some issues in the selection with the search

Not sure still what could be the quicker solution before the release, could you please advise @peluja1012 @jethr0null

After investigating why the table removes the selected item:
euiOnPageChange
The table under the hood, calls clearSelection on pagination

…ection-issue-pagination

WafaaNasr · 2022-11-28T10:09:51Z

@elasticmachine merge upstream

…ection-issue-pagination

…afaaNasr/kibana into fix-selection-issue-pagination

WafaaNasr · 2022-11-28T16:39:34Z

...ection_engine/rule_exceptions/components/flyout_components/add_to_rules_table/index.test.tsx


 jest.mock(
  '../../../../rule_management_ui/components/rules_table/rules_table/use_find_rules_in_memory'
 );
+// TODO convert this test to RTL


disabling Test for now as anyway I need to convert this test to use react-testing-library

kqualters-elastic · 2022-11-29T07:54:13Z

...c/detection_engine/rule_exceptions/components/flyout_components/add_to_rules_table/index.tsx

-      ]),
-    [initiallySelectedRules, rules]
+  // eslint-disable-next-line react/display-name
+  const LinkRuleSwitch = memo(({ rule }: { rule: Rule }) => {


should either move this out of the functional ExceptionsAddToRulesTableComponent or just make it a useMemo, as it is now this is a functional component that will get redefined on every render

thanks, @kqualters-elastic for the review :)
I already have the component pushed before in a previous commit but wanted the team to test with me while working on refactoring 😄 8bc0c26#diff-a70ea5db87eca5d28626b248345bb344c1527a752fda8504ec21a04bd08169e5R1

Please validate the new commit cfff10d

…-ref HEAD~1..HEAD --fix'

kqualters-elastic · 2022-11-29T15:55:55Z

...e/rule_exceptions/components/flyout_components/add_to_rules_table/use_add_to_rules_table.tsx

+      tags.forEach((tag) => acc.add(tag));
+      return acc;
+    }, new Set());
+    return [...uniqueTags].map((tag) => ({ value: tag, name: tag }));


nit: this could be done in the reduce probably

Good point! I thought about, but was worried about readability, but I will give it a try

We discussed it and we found it we need to implement it in two steps as we need to remove duplicates and map the string (tag) to object of { value: tag, name: tag }

kqualters-elastic · 2022-11-29T15:56:43Z

...e/rule_exceptions/components/flyout_components/add_to_rules_table/use_add_to_rules_table.tsx

+
+  const [linkedRules, setLinkedRules] = useState<Rule[]>(initiallySelectedRules || []);
+  useEffect(() => {
+    if (typeof onRuleSelectionChange === 'function') onRuleSelectionChange(linkedRules);


nit: i see this is typed as function | undefined, but all uses of it seem to be defined as a function, could probably change the type and remove this if

please check 6b7f2e7

kqualters-elastic · 2022-11-29T16:00:13Z

...ection_engine/rule_exceptions/components/flyout_components/add_to_rules_table/index.test.tsx

-        />
-      </TestProviders>
-    );
+  // it('it displays fetched rules', () => {


instead of commenting out the code, consider just it.skip and creating an issue to track unskipping eventually, think that is the kibana standard way of doing so

please check 6b7f2e7

…afaaNasr/kibana into fix-selection-issue-pagination

dhurley14

Tested locally - looks great!!!

kibana-ci · 2022-11-29T16:31:59Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`securitySolution`	3313	3315	+2

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.6MB	9.6MB	+3.4KB

Unknown metric groups

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
total			+20

Total ESLint disabled count

id	before	after	diff
`enterpriseSearch`	20	22	+2
`fleet`	68	74	+6
`osquery`	110	117	+7
`securitySolution`	520	526	+6
total			+21

History

💚 Build #91344 succeeded cfff10d
💛 Build #91124 was flaky e5c1289
💚 Build #91123 succeeded a56cdf1
💛 Build #90979 was flaky 28dce70
💚 Build #90912 succeeded c27f980

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

cc @WafaaNasr

@peluja1012

…red on navigation (elastic#146121) ## Summary - This PR, is for adding the new `Link` column Switch instead of using table selection based on the below bug - Addresses elastic#145683 - Remove `Tags` duplications and filter by tags ## Problem Manage Rules uses the `Add to Rule` component and this component under the hood is built on top of the [In Memory Table](https://elastic.github.io/eui/#/tabular-content/in-memory-tables#in-memory-table-selection) Currently, this table doesn't preserve the selection when the user navigates through the pages, which means if the user had some rules selected on the first page, then navigated to any other page the selection will be cleared if the user clicked on `Save` which will cause the linked rules to be unlinked from the list without notifying the user. This could be solved by: - Removing the `pagination` but the component won't fit in the `Add Exception Flyout` - **Add another column to the table to toggle (link/unlink) using a [Switch component](https://elastic.github.io/eui/#/forms/selection-controls#switch) with making the table's row not selectable** - Make use of the `toolsLeft` property given by the `In Memory Table` to design a new way of linking/unlinking - Control the height of the `Add to Rule` component ### **We decided in the Sync meeting to try the second option as it will give us full control over the Table data as well as we found some issues in the selection with the search** Not sure still what could be the quicker solution before the release, could you please advise @peluja1012 @jethr0null After investigating why the table removes the selected item: [euiOnPageChange](https://github.com/elastic/eui/blob/main/src/components/basic_table/basic_table.tsx#L480) The table under the hood, calls `clearSelection` on pagination <img width="712" alt="image" src="https://user-images.githubusercontent.com/12671903/203592532-6309bdd4-c0fe-41fb-b9c3-f2f6f5766dd9.png"> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> (cherry picked from commit 3cf1162)

kibanamachine · 2022-11-29T18:09:14Z

💚 All backports created successfully

Status	Branch	Result
✅	8.6

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

Questions ?

Please refer to the Backport tool documentation

@peluja1012

…n cleared on navigation (#146121) (#146607) # Backport This will backport the following commits from `main` to `8.6`: - [[Security Solution][List details page]: Manage rules - Selection cleared on navigation (#146121)](#146121)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  Co-authored-by: Wafaa Nasr <wafaa.nasr@elastic.co>

## Summary - Continuing from [PR](#146121) to apply the same changes to the `Add to Shared Lists`. - Fix showing the number of Linked rules correctly => in `route.ts` use the `list.namespaceType` instead of namespaceTypes array - Apply docs comment on the text - Use the HeaderMenu item from the `kbn` package for the `Number of Linked rules` menu - Allow displaying the HeaderMenu without iconType - Update snapshots and add tests in HeaderMenu

## Summary - Continuing from [PR](#146121) to apply the same changes to the `Add to Shared Lists`. - Fix showing the number of Linked rules correctly => in `route.ts` use the `list.namespaceType` instead of namespaceTypes array - Apply docs comment on the text - Use the HeaderMenu item from the `kbn` package for the `Number of Linked rules` menu - Allow displaying the HeaderMenu without iconType - Update snapshots and add tests in HeaderMenu (cherry picked from commit 78b4851)

) (#146887) # Backport This will backport the following commits from `main` to `8.6`: - [[Security Solution][Exception]: Add to shared lists fixes (#146750)](#146750)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  Co-authored-by: Wafaa Nasr <wafaa.nasr@elastic.co>

WafaaNasr added 4 commits November 22, 2022 15:46

sort table based on selection

b5f3a58

fix testing by moving back to id

788de21

remove commented sorting

4d5d59a

disable pagination to fix selection of managed rules

8ed6331

WafaaNasr requested a review from a team as a code owner November 23, 2022 10:27

WafaaNasr self-assigned this Nov 23, 2022

WafaaNasr added release_note:fix Team:Security Solution Platform Security Solution Platform Team backport:prev-minor Backport to (9.0) the previous minor version (i.e. one version back from main) ci:cloud-deploy Create or update a Cloud deployment labels Nov 23, 2022

WafaaNasr added 3 commits November 23, 2022 11:29

Merge branch 'main' of https://github.com/elastic/kibana into fix-sel…

50fac2f

…ection-issue-pagination

merge with main

3082640

add new Link column + add list correct link

0e66017

kibanamachine and others added 6 commits November 28, 2022 05:09

Merge branch 'main' into fix-selection-issue-pagination

c27f980

Merge branch 'main' of https://github.com/elastic/kibana into fix-sel…

02c66eb

…ection-issue-pagination

fix issue with switching then search without saving

8bc0c26

Merge branch 'fix-selection-issue-pagination' of https://github.com/W…

28dce70

…afaaNasr/kibana into fix-selection-issue-pagination

fix tags duplications

3559d4e

add translations for link col

a56cdf1

WafaaNasr commented Nov 28, 2022

View reviewed changes

remove commented code

e5c1289

kqualters-elastic reviewed Nov 29, 2022

View reviewed changes

WafaaNasr and others added 3 commits November 29, 2022 12:02

apply refactoring

cfff10d

apply comments

3a55691

[CI] Auto-commit changed files from 'node scripts/precommit_hook.js -…

a4bf586

…-ref HEAD~1..HEAD --fix'

kqualters-elastic reviewed Nov 29, 2022

View reviewed changes

make onrulechange mandatory and revert test back

6b7f2e7

Merge branch 'fix-selection-issue-pagination' of https://github.com/W…

50ceadc

…afaaNasr/kibana into fix-selection-issue-pagination

dhurley14 approved these changes Nov 29, 2022

View reviewed changes

WafaaNasr added release_note:skip Skip the PR/issue when compiling release notes and removed release_note:fix labels Nov 29, 2022

WafaaNasr merged commit 3cf1162 into elastic:main Nov 29, 2022

WafaaNasr deleted the fix-selection-issue-pagination branch November 29, 2022 18:04

kibanamachine added the v8.7.0 label Nov 29, 2022

kibanamachine mentioned this pull request Nov 29, 2022

[8.6] [Security Solution][List details page]: Manage rules - Selection cleared on navigation (#146121) #146607

Merged

kibanamachine added the v8.6.0 label Nov 29, 2022

WafaaNasr mentioned this pull request Nov 30, 2022

[Security Solution][Exception]: Add to shared lists fixes #146750

Merged

elasticmachine unassigned WafaaNasr Oct 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution][List details page]: Manage rules - Selection cleared on navigation #146121

[Security Solution][List details page]: Manage rules - Selection cleared on navigation #146121

WafaaNasr commented Nov 23, 2022 •

edited by kibanamachine

Loading

WafaaNasr commented Nov 28, 2022

WafaaNasr Nov 28, 2022

kqualters-elastic Nov 29, 2022

WafaaNasr Nov 29, 2022

WafaaNasr Nov 29, 2022

kqualters-elastic Nov 29, 2022

WafaaNasr Nov 29, 2022

WafaaNasr Nov 29, 2022

kqualters-elastic Nov 29, 2022

WafaaNasr Nov 29, 2022

kqualters-elastic Nov 29, 2022

WafaaNasr Nov 29, 2022

dhurley14 left a comment

kibana-ci commented Nov 29, 2022 •

edited

Loading

ESLint disabled in files

ESLint disabled line counts

Total ESLint disabled count

kibanamachine commented Nov 29, 2022

[Security Solution][List details page]: Manage rules - Selection cleared on navigation #146121

[Security Solution][List details page]: Manage rules - Selection cleared on navigation #146121

Conversation

WafaaNasr commented Nov 23, 2022 • edited by kibanamachine Loading

Summary

Problem

We decided in the Sync meeting to try the second option as it will give us full control over the Table data as well as we found some issues in the selection with the search

WafaaNasr commented Nov 28, 2022

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhurley14 left a comment

Choose a reason for hiding this comment

kibana-ci commented Nov 29, 2022 • edited Loading

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

ESLint disabled in files

ESLint disabled line counts

Total ESLint disabled count

History

kibanamachine commented Nov 29, 2022

💚 All backports created successfully

Questions ?

WafaaNasr commented Nov 23, 2022 •

edited by kibanamachine

Loading

kibana-ci commented Nov 29, 2022 •

edited

Loading