[Security Solution][Investigations] - fix opening flyout, and clear filters by michaelolo24 · Pull Request #156702 · elastic/kibana

michaelolo24 · 2023-05-04T14:56:28Z

Summary

This PR fixes the following issue #156590 where a users configuration for their alert page filters can prevent them from seeing the alert in the table. In this PR we clear the filters to only show the required status filter, with no selection, to rely primarily on the kql query filter for the alert id.

Demo:

Screen.Recording.2023-05-04.at.5.13.43.PM.mov

It also fixes a bug where the flyout when opened wasn't unmounted when navigating away from a page and back to it as seen here:

The bug:

Screen.Recording.2023-05-04.at.5.25.05.PM.mov

With the fix:

Screen.Recording.2023-05-04.at.5.28.04.PM.mov

Checklist

Delete any items that are not applicable to this PR.

Unit or functional tests were updated or added to match the most common scenarios

elasticmachine · 2023-05-05T12:46:48Z

Pinging @elastic/security-threat-hunting (Team:Threat Hunting)

elasticmachine · 2023-05-05T12:46:49Z

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

janmonschke · 2023-05-05T12:51:24Z

x-pack/plugins/security_solution/cypress/e2e/detection_alerts/alerts_details.cy.ts

+    it('should remove the flyout details from local storage when closed', () => {
+      cy.get(OVERVIEW_RULE).should('be.visible');
+      closeAlertFlyout();
+      cy.wait(500); // Wait for the localStorage update to take place


Did the tests show that it needs 500ms before the localstorage write has finished? I would expect that 100ms should be enough, plus a little buffer.

No, it's because of how we handle the localStorage updates:

kibana/x-pack/plugins/security_solution/public/common/store/data_table/epic_local_storage.ts

Line 58 in 8c336b2

delay(500),

should add timeout/pipe on assertion instead of delay. Because if someone changes the delay, test will start failing.

Thanks for the help @logeekal! a70dd0c

janmonschke · 2023-05-05T12:52:39Z

x-pack/plugins/security_solution/cypress/e2e/detection_alerts/alerts_details.cy.ts

+    it('should not remove the flyout state from localstorage when navigating away without closing the flyout', () => {
+      cy.get(OVERVIEW_RULE).should('be.visible');
+      goToRuleDetails();
+      cy.wait(500); // Wait for the localStorage update to take place


kibana/x-pack/plugins/security_solution/public/common/store/data_table/epic_local_storage.ts

Line 58 in 8c336b2

delay(500),

Was able to get help from Jatin to refactor this to not explicitly rely on specific timerange 💪🏾 Thanks for the review!

janmonschke · 2023-05-05T12:53:56Z

x-pack/plugins/security_solution/cypress/helpers/common.ts

 export const getClassSelector = (className: string) => `.${className}`;
+
+export const getLocalstorageEntryAsObject = (storage: Cypress.StorageByOrigin, field: string) => {
+  const envLocalstorage = storage?.['http://localhost:5620'];


Is the test env url always 'http://localhost:5620 ?

Yep!

kibana/x-pack/plugins/security_solution/public/management/cypress_endpoint.config.ts

Line 38 in 8c336b2

baseUrl: 'http://localhost:5620',

Gotcha. Somehow I feel like this should be a const, though :D

janmonschke · 2023-05-05T12:56:59Z

x-pack/plugins/security_solution/public/detections/pages/alerts/alert_details_redirect.tsx


-  const url = `${ALERTS_PATH}?${URL_PARAM_KEY.appQuery}=${kqlAppQuery}&${URL_PARAM_KEY.timerange}=${timerange}&${URL_PARAM_KEY.eventFlyout}=${flyoutString}`;
+  const statusPageFilter: FilterItemObj = {
+    fieldName: 'kibana.alert.workflow_status',


Wdyt about using this const instead?

import { ALERT_WORKFLOW_STATUS } from '@kbn/rule-data-utils';

janmonschke

🎉

…fix'

kibana-ci · 2023-05-05T15:59:15Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: 4dc719f

Failed CI Steps

FTR Configs #52

Test Failures

[job] [logs] FTR Configs #52 / expressions explorer emits an action and navigates

Metrics [docs]

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	+213.0B

Unknown metric groups

ESLint disabled line counts

id	before	after	diff
`enterpriseSearch`	19	21	+2
`securitySolution`	398	401	+3
total			+5

Total ESLint disabled count

id	before	after	diff
`enterpriseSearch`	20	22	+2
`securitySolution`	478	481	+3
total			+5

History

💔 Build #125859 failed a70dd0c
💔 Build #125659 failed 094b677
💔 Build #125553 failed d90d2dae5ced84d2444f420093f32c2cc68c96f2

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

cc @michaelolo24

banderror

Rules area changes LGTM!

e40pud

Alerts area changes LGTM!

…ilters (elastic#156702) ## Summary This PR fixes the following issue elastic#156590 where a users configuration for their alert page filters can prevent them from seeing the alert in the table. In this PR we clear the filters to only show the required status filter, with no selection, to rely primarily on the kql query filter for the alert id. Demo: https://user-images.githubusercontent.com/17211684/236332095-ac5583ec-6b5f-4ee2-9c23-9391b54263ba.mov It also fixes a bug where the flyout when opened wasn't unmounted when navigating away from a page and back to it as seen here: The bug: https://user-images.githubusercontent.com/17211684/236333550-b41925f9-a06c-4aa9-931b-beecf4c9ae9b.mov With the fix: https://user-images.githubusercontent.com/17211684/236334123-a0e3169e-2ec8-4754-92d2-65f2f369fa4f.mov ### Checklist Delete any items that are not applicable to this PR. - [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 --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com> (cherry picked from commit 1b40216)

kibanamachine · 2023-05-08T12:35:27Z

💚 All backports created successfully

Status	Branch	Result
✅	8.8

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

Questions ?

Please refer to the Backport tool documentation

…lear filters (#156702) (#156984) # Backport This will backport the following commits from `main` to `8.8`: - [[Security Solution][Investigations] - fix opening flyout, and clear filters (#156702)](#156702)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  Co-authored-by: Michael Olorunnisola <michael.olorunnisola@elastic.co>

fix opening flyout, and clear filters

eb678f4

michaelolo24 force-pushed the url-flyout-fixes branch from d90d2da to 7f20d1d Compare May 4, 2023 21:07

add tests

094b677

michaelolo24 force-pushed the url-flyout-fixes branch from 7f20d1d to 094b677 Compare May 4, 2023 21:09

michaelolo24 added the impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. label May 4, 2023

michaelolo24 self-assigned this May 4, 2023

michaelolo24 added 2 commits May 5, 2023 07:37

cleanup cypress tests

ab49fbc

remove unnecessary return

edd78e9

michaelolo24 marked this pull request as ready for review May 5, 2023 12:46

michaelolo24 requested review from a team as code owners May 5, 2023 12:46

michaelolo24 added the release_note:skip Skip the PR/issue when compiling release notes label May 5, 2023

janmonschke reviewed May 5, 2023

View reviewed changes

janmonschke approved these changes May 5, 2023

View reviewed changes

michaelolo24 and others added 3 commits May 5, 2023 10:03

update tests and order more sensibly

a70dd0c

remove accidental only

c5365ef

[CI] Auto-commit changed files from 'node scripts/lint_ts_projects --…

4dc719f

…fix'

kibanamachine requested a review from a team as a code owner May 5, 2023 14:23

banderror approved these changes May 8, 2023

View reviewed changes

michaelolo24 enabled auto-merge (squash) May 8, 2023 12:26

e40pud approved these changes May 8, 2023

View reviewed changes

michaelolo24 merged commit 1b40216 into elastic:main May 8, 2023

kibanamachine added the v8.9.0 label May 8, 2023

kibanamachine mentioned this pull request May 8, 2023

[8.8] [Security Solution][Investigations] - fix opening flyout, and clear filters (#156702) #156984

Merged

Comments

Conversation

michaelolo24 commented May 4, 2023 • edited by kibanamachine Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

Uh oh!

elasticmachine commented May 5, 2023

Uh oh!

elasticmachine commented May 5, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michaelolo24 May 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

janmonschke left a comment

Choose a reason for hiding this comment

Uh oh!

kibana-ci commented May 5, 2023

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

Metrics [docs]

Async chunks

ESLint disabled line counts

Total ESLint disabled count

History

Uh oh!

banderror left a comment

Choose a reason for hiding this comment

Uh oh!

e40pud left a comment

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented May 8, 2023

💚 All backports created successfully

Questions ?

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

michaelolo24 commented May 4, 2023 •

edited by kibanamachine

Loading

michaelolo24 May 5, 2023 •

edited

Loading