[Security Solution][Endpoint][Response Actions] Fix table navigation when trays are expanded #157777
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ashokaditya
added
release_note:skip
ashokaditya
force-pushed
the
fix/dw-response-actions-history-tray-back-page-6603
branch
6 times, most recently
from
f68530b to
f340441
Compare
1 task
ashokaditya
force-pushed
the
fix/dw-response-actions-history-tray-back-page-6603
branch
from
f340441 to
1853938
Compare
...public/management/components/endpoint_response_actions_list/components/actions_log_table.tsx
Outdated
Show resolved
Hide resolved
...public/management/components/endpoint_response_actions_list/components/actions_log_table.tsx
Outdated
Show resolved
Hide resolved
...public/management/components/endpoint_response_actions_list/components/actions_log_table.tsx
Outdated
Show resolved
Hide resolved
kevinlog
pushed a commit
that referenced
this pull request
May 16, 2023
…ay (#157896) ## Summary Fixes the bug when sometimes the expanding tray breaks the page on the response actions history page/flyout It doesn't fix the other issues where when a tray is expanded and paging is used the page breaks. That is being worked on in /pull/157777 ### Checklist - [ ] [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
ashokaditya
changed the title
kibanamachine
pushed a commit
to kibanamachine/kibana
that referenced
this pull request
May 16, 2023
…ay (elastic#157896) ## Summary Fixes the bug when sometimes the expanding tray breaks the page on the response actions history page/flyout It doesn't fix the other issues where when a tray is expanded and paging is used the page breaks. That is being worked on in elastic/pull/157777 ### Checklist - [ ] [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 (cherry picked from commit 8210806)
kibanamachine
added a commit
that referenced
this pull request
May 16, 2023
…ils tray (#157896) (#157910) # Backport This will backport the following commits from `main` to `8.8`: - [[Security Solution][Endpoint][Response Actions] Fix expand details tray (#157896)](#157896) <!--- Backport version: 8.9.7 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Ashokaditya","email":"[email protected]"},"sourceCommit":{"committedDate":"2023-05-16T17:36:54Z","message":"[Security Solution][Endpoint][Response Actions] Fix expand details tray (#157896)\n\n## Summary\r\n\r\nFixes the bug when sometimes the expanding tray breaks the page on the\r\nresponse actions history page/flyout\r\n\r\nIt doesn't fix the other issues where when a tray is expanded and paging\r\nis used the page breaks.\r\nThat is being worked on in /pull/157777\r\n\r\n### Checklist\r\n\r\n- [ ] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios","sha":"82108060e7c3794add5687761cf65890dc39d573","branchLabelMapping":{"^v8.9.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","Team:Defend Workflows","OLM Sprint","v8.8.0","v8.9.0"],"number":157896,"url":"https://github.com/elastic/kibana/pull/157896","mergeCommit":{"message":"[Security Solution][Endpoint][Response Actions] Fix expand details tray (#157896)\n\n## Summary\r\n\r\nFixes the bug when sometimes the expanding tray breaks the page on the\r\nresponse actions history page/flyout\r\n\r\nIt doesn't fix the other issues where when a tray is expanded and paging\r\nis used the page breaks.\r\nThat is being worked on in /pull/157777\r\n\r\n### Checklist\r\n\r\n- [ ] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios","sha":"82108060e7c3794add5687761cf65890dc39d573"}},"sourceBranch":"main","suggestedTargetBranches":["8.8"],"targetPullRequestStates":[{"branch":"8.8","label":"v8.8.0","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.9.0","labelRegex":"^v8.9.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/157896","number":157896,"mergeCommit":{"message":"[Security Solution][Endpoint][Response Actions] Fix expand details tray (#157896)\n\n## Summary\r\n\r\nFixes the bug when sometimes the expanding tray breaks the page on the\r\nresponse actions history page/flyout\r\n\r\nIt doesn't fix the other issues where when a tray is expanded and paging\r\nis used the page breaks.\r\nThat is being worked on in /pull/157777\r\n\r\n### Checklist\r\n\r\n- [ ] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios","sha":"82108060e7c3794add5687761cf65890dc39d573"}}]}] BACKPORT--> Co-authored-by: Ashokaditya <[email protected]>
ashokaditya
force-pushed
the
fix/dw-response-actions-history-tray-back-page-6603
branch
from
465f7dc to
890eb1d
Compare
jasonrhodes
pushed a commit
that referenced
this pull request
May 17, 2023
…ay (#157896) ## Summary Fixes the bug when sometimes the expanding tray breaks the page on the response actions history page/flyout It doesn't fix the other issues where when a tray is expanded and paging is used the page breaks. That is being worked on in /pull/157777 ### Checklist - [ ] [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
ashokaditya
force-pushed
the
fix/dw-response-actions-history-tray-back-page-6603
branch
from
d777167 to
3bc094b
Compare
ashokaditya
commented
May 17, 2023
Comment on lines
+316
to
+318
| if (!actionItem) { | ||
| idToRowMap[actionId] = <EuiSkeletonText size="relative" lines={8} />; | ||
| } else { |
There was a problem hiding this comment.
This case was not handled earlier and adding this change fixes the issue.
There was a problem hiding this comment.
Good fix 👍
Just a detail that I was thinking about and maybe would make sense to you - it's personal preference, I can't justify which approach is better :P Would you consider wrapping openDetails in useMemo and use it with setExpandedRowMap in useEffect instead of using redoOpenTrans wrapper in useCallback?
tomsonpl
approved these changes
May 23, 2023
Comment on lines
+316
to
+318
| if (!actionItem) { | ||
| idToRowMap[actionId] = <EuiSkeletonText size="relative" lines={8} />; | ||
| } else { |
There was a problem hiding this comment.
Good fix 👍
Just a detail that I was thinking about and maybe would make sense to you - it's personal preference, I can't justify which approach is better :P Would you consider wrapping openDetails in useMemo and use it with setExpandedRowMap in useEffect instead of using redoOpenTrans wrapper in useCallback?
Good suggestion. Yeah, that will also do the same. I'll leave it as it is for now. With your suggestion I might end up stating dependencies twice (once in the useMemo and then in the useEffect). For now, I like that all the open tray logic is wrapped up in one function and I just call that with |
...public/management/components/endpoint_response_actions_list/components/actions_log_table.tsx
Show resolved
Hide resolved
patrykkopycinski
approved these changes
May 23, 2023
patrykkopycinski
approved these changes
May 23, 2023
joeypoon
approved these changes
May 23, 2023
paul-tavares
approved these changes
May 23, 2023
| @@ -47,7 +47,7 @@ export const indexEndpointAndFleetActionsForHost = async ( | |||
| ): Promise<IndexedEndpointAndFleetActionsForHostResponse> => { | |||
| const ES_INDEX_OPTIONS = { headers: { 'X-elastic-product-origin': 'fleet' } }; | |||
| const agentId = endpointHost.elastic.agent.id; | |||
| const total = fleetActionGenerator.randomN(5) + 1; // generate at least one | |||
| const total = fleetActionGenerator.randomN(5) + 11; // generate at least 11 actions | |||
There was a problem hiding this comment.
Do we really want 11 actions to be index every time the generator runs? Remember that this is used in other use cases outside of your test.
Suggestion: consider adding an options argument to this function and introduce a count parameter that drives how many to create and then use that from your test.
There was a problem hiding this comment.
Yeah, adding options is a good idea.
| } | ||
| }); | ||
|
|
||
| it('retains expanded action details on page navigation', () => { |
There was a problem hiding this comment.
I'm ok if you leave this as is, but curious: aren't these tests already covered with Jest Unit?
There was a problem hiding this comment.
Yeah, you're right! I also covered this in jest. I'll remove this one.
| }, []); | ||
|
|
||
| // verify 5 rows are expanded | ||
| expect(traysOnPage1).toBeTruthy(); |
There was a problem hiding this comment.
I think this is always thruthy because getAllbyTestId() always returns an array. I think it not necessary because the next assertion below is checking the expected length
There was a problem hiding this comment.
Oh indeed. I'll update it
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]Async chunks
Unknown metric groupsESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: cc @ashokaditya |
kibanamachine
pushed a commit
to kibanamachine/kibana
that referenced
this pull request
May 23, 2023
…when trays are expanded (elastic#157777) ## Summary Fixes an issue where when an action detail is shown via an expanded tray item and the total number of items goes beyond the first page on the response actions history page/flyout, switching between pages while the tray is open breaks the page. - [x] fix paging with trays expanded on a flyout - [x] fix paging with trays expanded on a page - [x] ensure when the page is loaded with `?withOutputs=` with action ids from different sets of pages, table paging doesn't break when paged, and trays show open for the action ids in `?withOutputs=` URL param - tests: - [x] page navigation flyout/page view - [x] page reload with URL params (cypress) **flyout**  **page**  *page with URL load* - three actions are open on two different pages - we re-load page 2 with two open trays and then navigate to page 1 to see the third one open - also re-load page 1; we see the tray open, then navigate to page 2 to see the other two trays open.  ### Checklist - [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 (cherry picked from commit 9e01dc8)
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
ashokaditya
deleted the
fix/dw-response-actions-history-tray-back-page-6603
branch
delanni
pushed a commit
to delanni/kibana
that referenced
this pull request
May 25, 2023
…when trays are expanded (elastic#157777) ## Summary Fixes an issue where when an action detail is shown via an expanded tray item and the total number of items goes beyond the first page on the response actions history page/flyout, switching between pages while the tray is open breaks the page. - [x] fix paging with trays expanded on a flyout - [x] fix paging with trays expanded on a page - [x] ensure when the page is loaded with `?withOutputs=` with action ids from different sets of pages, table paging doesn't break when paged, and trays show open for the action ids in `?withOutputs=` URL param - tests: - [x] page navigation flyout/page view - [x] page reload with URL params (cypress) **flyout**  **page**  *page with URL load* - three actions are open on two different pages - we re-load page 2 with two open trays and then navigate to page 1 to see the third one open - also re-load page 1; we see the tray open, then navigate to page 2 to see the other two trays open.  ### Checklist - [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
ashokaditya
added a commit
that referenced
this pull request
May 26, 2023
…ation when trays are expanded (#157777) (#158306) # Backport This will backport the following commits from `main` to `8.8`: - [[Security Solution][Endpoint][Response Actions] Fix table navigation when trays are expanded (#157777)](#157777) <!--- Backport version: 8.9.7 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Ashokaditya","email":"[email protected]"},"sourceCommit":{"committedDate":"2023-05-23T18:49:02Z","message":"[Security Solution][Endpoint][Response Actions] Fix table navigation when trays are expanded (#157777)\n\n## Summary\r\n\r\nFixes an issue where when an action detail is shown via an expanded tray\r\nitem and the total number of items goes beyond the first page on the\r\nresponse actions history page/flyout, switching between pages while the\r\ntray is open breaks the page.\r\n\r\n- [x] fix paging with trays expanded on a flyout \r\n- [x] fix paging with trays expanded on a page\r\n- [x] ensure when the page is loaded with `?withOutputs=` with action\r\nids from different sets of pages, table paging doesn't break when paged,\r\nand trays show open for the action ids in `?withOutputs=` URL param\r\n- tests:\r\n - [x] page navigation flyout/page view\r\n - [x] page reload with URL params (cypress)\r\n \r\n**flyout**\r\n\r\n\r\n\r\n**page**\r\n\r\n\r\n\r\n*page with URL load*\r\n- three actions are open on two different pages\r\n- we re-load page 2 with two open trays and then navigate to page 1 to\r\nsee the third one open\r\n- also re-load page 1; we see the tray open, then navigate to page 2 to\r\nsee the other two trays open.\r\n\r\n\r\n\r\n### Checklist\r\n\r\n- [x] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios","sha":"9e01dc815f87ccb70f961852f6d7c014e3e43c0b","branchLabelMapping":{"^v8.9.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","Team:Defend Workflows","OLM Sprint","v8.9.0","v8.8.1"],"number":157777,"url":"https://github.com/elastic/kibana/pull/157777","mergeCommit":{"message":"[Security Solution][Endpoint][Response Actions] Fix table navigation when trays are expanded (#157777)\n\n## Summary\r\n\r\nFixes an issue where when an action detail is shown via an expanded tray\r\nitem and the total number of items goes beyond the first page on the\r\nresponse actions history page/flyout, switching between pages while the\r\ntray is open breaks the page.\r\n\r\n- [x] fix paging with trays expanded on a flyout \r\n- [x] fix paging with trays expanded on a page\r\n- [x] ensure when the page is loaded with `?withOutputs=` with action\r\nids from different sets of pages, table paging doesn't break when paged,\r\nand trays show open for the action ids in `?withOutputs=` URL param\r\n- tests:\r\n - [x] page navigation flyout/page view\r\n - [x] page reload with URL params (cypress)\r\n \r\n**flyout**\r\n\r\n\r\n\r\n**page**\r\n\r\n\r\n\r\n*page with URL load*\r\n- three actions are open on two different pages\r\n- we re-load page 2 with two open trays and then navigate to page 1 to\r\nsee the third one open\r\n- also re-load page 1; we see the tray open, then navigate to page 2 to\r\nsee the other two trays open.\r\n\r\n\r\n\r\n### Checklist\r\n\r\n- [x] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios","sha":"9e01dc815f87ccb70f961852f6d7c014e3e43c0b"}},"sourceBranch":"main","suggestedTargetBranches":["8.8"],"targetPullRequestStates":[{"branch":"main","label":"v8.9.0","labelRegex":"^v8.9.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/157777","number":157777,"mergeCommit":{"message":"[Security Solution][Endpoint][Response Actions] Fix table navigation when trays are expanded (#157777)\n\n## Summary\r\n\r\nFixes an issue where when an action detail is shown via an expanded tray\r\nitem and the total number of items goes beyond the first page on the\r\nresponse actions history page/flyout, switching between pages while the\r\ntray is open breaks the page.\r\n\r\n- [x] fix paging with trays expanded on a flyout \r\n- [x] fix paging with trays expanded on a page\r\n- [x] ensure when the page is loaded with `?withOutputs=` with action\r\nids from different sets of pages, table paging doesn't break when paged,\r\nand trays show open for the action ids in `?withOutputs=` URL param\r\n- tests:\r\n - [x] page navigation flyout/page view\r\n - [x] page reload with URL params (cypress)\r\n \r\n**flyout**\r\n\r\n\r\n\r\n**page**\r\n\r\n\r\n\r\n*page with URL load*\r\n- three actions are open on two different pages\r\n- we re-load page 2 with two open trays and then navigate to page 1 to\r\nsee the third one open\r\n- also re-load page 1; we see the tray open, then navigate to page 2 to\r\nsee the other two trays open.\r\n\r\n\r\n\r\n### Checklist\r\n\r\n- [x] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios","sha":"9e01dc815f87ccb70f961852f6d7c014e3e43c0b"}},{"branch":"8.8","label":"v8.8.1","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"}]}] BACKPORT--> Co-authored-by: Ashokaditya <[email protected]>
ashokaditya
added a commit
that referenced
this pull request
Jul 11, 2023
…cypress test (#161533) ## Summary Unskip test that was timing out at indexing test hosts. The timeout has since been increased in /pull/159518 It was added in /pull/157777 and skipped in /pull/156933 thus should be backported to `8.8.1` and `8.9.0`
ashokaditya
added a commit
that referenced
this pull request
Jul 12, 2023
…cypress test (#161533) ## Summary Unskip test that was timing out at indexing test hosts. The timeout has since been increased in /pull/159518 It was added in /pull/157777 and skipped in /pull/156933 thus should be backported to `8.8.1` and `8.9.0` (cherry picked from commit f6f5986) # Conflicts: # x-pack/plugins/security_solution/public/management/cypress/e2e/mocked_data/reponse_actions_history.cy.ts
ashokaditya
added a commit
to ashokaditya/kibana
that referenced
this pull request
Jul 12, 2023
…cypress test (elastic#161533) ## Summary Unskip test that was timing out at indexing test hosts. The timeout has since been increased in elastic/pull/159518 It was added in elastic/pull/157777 and skipped in elastic/pull/156933 thus should be backported to `8.8.1` and `8.9.0` (cherry picked from commit f6f5986) # Conflicts: # x-pack/plugins/security_solution/public/management/cypress/e2e/mocked_data/reponse_actions_history.cy.ts
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Fixes an issue where when an action detail is shown via an expanded tray item and the total number of items goes beyond the first page on the response actions history page/flyout, switching between pages while the tray is open breaks the page.
?withOutputs=with action ids from different sets of pages, table paging doesn't break when paged, and trays show open for the action ids in?withOutputs=URL paramflyout
page
page with URL load
Checklist