[Security Solution][Detection Alerts] Fixes follow-up alert refresh bugs #112169

dplumlee · 2021-09-14T19:32:26Z

Summary

Follow-up to #111042

Addresses #112011

Fixes bug where closing alert via the add exception modal when opening modal from the take action flyout menu wouldn't properly refresh the alerts page
Adds tests
Fixes timeline sync

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

For maintainers

This was checked for breaking API changes and was labeled appropriately

dplumlee · 2021-09-22T20:46:47Z

@elasticmachine merge upstream

elasticmachine · 2021-09-23T19:43:50Z

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

spong

Checked out, desk tested locally and code-reviewed -- Loooks GREEEEAT To Me! 👍 Thanks for following through with the thoroughness in fixes here @dplumlee! Everything I tested refreshed as expected and I identified no stale data when performing actions 🙂

Testing notes:

Updating alert status from Alert Details closes flyout? I think this is desired UX, but was unexpected to me (I think this I mentioned this in the last PR 😅) .
Closing alert from Alert Details Add Exception UI keeps flyout open, so slightly different UX from above with same action outcome.
Updating alert status from Alert Details -> Investigate in Timeline -> Close Alert -> Close Timeline, still shows close action in Alert Details flyout actions (Quite the corner case 😅 -- no resolution expected here).

spong · 2021-10-05T20:25:22Z

x-pack/plugins/security_solution/cypress/integration/detection_alerts/acknowledged.spec.ts

+        cy.get(ALERT_COUNT_TABLE_FIRST_ROW_COUNT).should('have.text', `${expectedNumberOfAlerts}`);

        goToAcknowledgedAlerts();
        waitForAlerts();

        cy.get(ALERTS_COUNT).should('have.text', `${numberOfAlertsToBeMarkedAcknowledged} alert`);
+        cy.get(ALERT_COUNT_TABLE_FIRST_ROW_COUNT).should(
+          'have.text',
+          `${numberOfAlertsToBeMarkedAcknowledged}`
+        );


Is there a case for verifying the Trend histogram has refreshed as well? No explicit counts visible in that component, so we'd need a separate method for verification here -- perhaps setup test to only have one alert and then verify Rule Name is no longer in legend? Or could hover on a bar and verify counts in tooltip (though may be tough to sort out the flake)?

Same comment goes for opening/closing specs below.

it doesn't verify the trend histogram yet, I was looking at some possible ways to do that since the data isn't as neat from a css selector standpoint for cypress. I think having the one alert would be a good way to do that, though, I'll add it to the test case here and in the other relevant files 👍

spong · 2021-10-05T20:27:28Z

...y_solution/public/detections/components/alerts_table/timeline_actions/alert_context_menu.tsx

+      if (routeProps.pageName === 'alerts') {
+        refetchQuery(globalQuery);
+      }


Nice! I had explicitly tried closing an alert from within timeline elsewhere in the app to see if we'd still refetch, haha! 😅

🚀 🥮

nit: you could use https://github.com/elastic/kibana/blob/master/x-pack/plugins/security_solution/common/constants.ts#L72 instead of this string JUST IN CASE it ever changes

spong · 2021-10-05T20:39:34Z

x-pack/plugins/timelines/public/container/use_update_alerts.ts

@@ -37,7 +37,7 @@ export const useUpdateAlertsStatus = (
  const { http } = useKibana<CoreStart>().services;
  return {
    updateAlertStatus: async ({ status, index, query }) => {
-      if (['detections-page', 'detections-rules-details-page'].includes(timelineId)) {
+      if (useDetectionEngine) {


May want to rename -- wasn't immediately clear how the detection engine wasn't involved with updating alert status, but clicking through to see the conditional showed this is just a feature flag check between legacy impl and RAC RBAC routes. I guess this makes sense since the route ends up not being a DE route, but RAC, so don't mind me....

Do need some JSDocs though, haha 😉

spong · 2021-10-05T20:41:01Z

...s/security_solution/public/timelines/components/side_panel/__snapshots__/index.test.tsx.snap

@@ -1054,7 +1054,7 @@ Array [
            </div>
          </EuiFlyoutBody>
        </Styled(EuiFlyoutBody)>
-        <Memo()
+        <Connect(Component)


Not for you to resolve, so commenting for posterity, but it's probably time we revisit some of these larger snapshot tests and determine if they can be broken up into smaller more understandable/maintainable chunks. We've discussed before how they lose their utility when reaching thousands of lines, and I think this is a reasonable example of this. (I identified the structural changes that resulted from this PR, but tough to make sense if that's all that changed that should have.)

kqualters-elastic

code lgtm 👍 , but might be a good idea to get @angorayc to sign off on this before merging, i know there were some strange bugs with refetch in very specific circumstances that were recently fixed

dplumlee · 2021-10-11T16:22:08Z

@elasticmachine merge upstream

dplumlee · 2021-10-11T19:42:55Z

@elasticmachine merge upstream

dplumlee · 2021-10-11T21:07:27Z

@elasticmachine merge upstream

dplumlee · 2021-10-12T00:55:44Z

@elasticmachine merge upstream

dplumlee · 2021-10-12T12:40:42Z

@elasticmachine merge upstream

kibanamachine · 2021-10-12T14:40:50Z

💚 Build Succeeded

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`	4.3MB	4.3MB	+19.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`timelines`	156.7KB	156.6KB	-62.0B

History

💔 Build #159793 failed 9239483
💔 Build #159726 failed 374c023
💔 Build #159681 failed ebd6140
💔 Build #159601 failed 8c03277
💚 Build #158620 succeeded b570bb0

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

cc @dplumlee

…ugs (elastic#112169)

kibanamachine · 2021-10-12T14:44:53Z

💚 Backport successful

Status	Branch	Result
✅	7.x
✅	7.15

The backport PRs will be merged automatically after passing CI.

…ugs (#112169) (#114650) Co-authored-by: Davis Plumlee <[email protected]>

…ugs (#112169) (#114649) Co-authored-by: Davis Plumlee <[email protected]>

dplumlee self-assigned this Sep 14, 2021

dplumlee added the v7.15.1 label Sep 15, 2021

rylnd mentioned this pull request Sep 15, 2021

[Security Solution][Detection] Not all Alert components refresh upon change alert status action #108244

Closed

dplumlee force-pushed the alert-status-refresh-pt2 branch from 9ce2833 to e844acc Compare September 22, 2021 19:55

dplumlee force-pushed the alert-status-refresh-pt2 branch from a9e0ccc to b92715f Compare September 23, 2021 19:24

dplumlee marked this pull request as ready for review September 23, 2021 19:43

dplumlee requested review from a team as code owners September 23, 2021 19:43

dplumlee requested a review from XavierM September 23, 2021 19:44

dplumlee force-pushed the alert-status-refresh-pt2 branch from b92715f to 55010df Compare October 4, 2021 17:47

spong approved these changes Oct 5, 2021

View reviewed changes

dplumlee added 6 commits October 6, 2021 13:37

fixes take action exception options

e7941de

updates snapshots

92ced82

fixes timeline bugs

0878902

fixes prettier stuff

62a905f

updates tests

68bb7c4

adds trend test

acbe509

dplumlee force-pushed the alert-status-refresh-pt2 branch from 55010df to acbe509 Compare October 6, 2021 18:53

adds comments

b570bb0

kqualters-elastic approved these changes Oct 7, 2021

View reviewed changes

Merge branch 'master' into alert-status-refresh-pt2

8c03277

dplumlee enabled auto-merge (squash) October 11, 2021 16:22

Merge branch 'master' into alert-status-refresh-pt2

ebd6140

Merge branch 'master' into alert-status-refresh-pt2

374c023

Merge branch 'master' into alert-status-refresh-pt2

9239483

Merge branch 'master' into alert-status-refresh-pt2

8cf6f6d

dplumlee merged commit 0b46bb1 into elastic:master Oct 12, 2021

kibanamachine mentioned this pull request Oct 12, 2021

[7.x] [Security Solution][Detection Alerts] Fixes follow-up alert refresh bugs (#112169) #114649

Merged

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Oct 12, 2021

[Security Solution][Detection Alerts] Fixes follow-up alert refresh b…

225e898

…ugs (elastic#112169)

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Oct 12, 2021

[Security Solution][Detection Alerts] Fixes follow-up alert refresh b…

2be4f19

…ugs (elastic#112169)

kibanamachine mentioned this pull request Oct 12, 2021

[7.15] [Security Solution][Detection Alerts] Fixes follow-up alert refresh bugs (#112169) #114650

Merged

kibanamachine added a commit that referenced this pull request Oct 12, 2021

[Security Solution][Detection Alerts] Fixes follow-up alert refresh b…

53b3e6b

…ugs (#112169) (#114650) Co-authored-by: Davis Plumlee <[email protected]>

dplumlee deleted the alert-status-refresh-pt2 branch October 13, 2021 04:24

kibanamachine added a commit that referenced this pull request Oct 13, 2021

[Security Solution][Detection Alerts] Fixes follow-up alert refresh b…

dd0e05e

…ugs (#112169) (#114649) Co-authored-by: Davis Plumlee <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution][Detection Alerts] Fixes follow-up alert refresh bugs #112169

[Security Solution][Detection Alerts] Fixes follow-up alert refresh bugs #112169

dplumlee commented Sep 14, 2021 •

edited

Loading

dplumlee commented Sep 22, 2021

elasticmachine commented Sep 23, 2021

spong left a comment

spong Oct 5, 2021

spong Oct 5, 2021

dplumlee Oct 5, 2021

spong Oct 5, 2021

kqualters-elastic Oct 7, 2021

spong Oct 5, 2021

spong Oct 5, 2021

kqualters-elastic left a comment

dplumlee commented Oct 11, 2021

dplumlee commented Oct 11, 2021

dplumlee commented Oct 11, 2021

dplumlee commented Oct 12, 2021

dplumlee commented Oct 12, 2021

kibanamachine commented Oct 12, 2021

kibanamachine commented Oct 12, 2021

[Security Solution][Detection Alerts] Fixes follow-up alert refresh bugs #112169

[Security Solution][Detection Alerts] Fixes follow-up alert refresh bugs #112169

Conversation

dplumlee commented Sep 14, 2021 • edited Loading

Summary

Checklist

For maintainers

dplumlee commented Sep 22, 2021

elasticmachine commented Sep 23, 2021

spong left a comment

Choose a reason for hiding this comment

spong Oct 5, 2021

Choose a reason for hiding this comment

spong Oct 5, 2021

Choose a reason for hiding this comment

dplumlee Oct 5, 2021

Choose a reason for hiding this comment

spong Oct 5, 2021

Choose a reason for hiding this comment

kqualters-elastic Oct 7, 2021

Choose a reason for hiding this comment

spong Oct 5, 2021

Choose a reason for hiding this comment

spong Oct 5, 2021

Choose a reason for hiding this comment

kqualters-elastic left a comment

Choose a reason for hiding this comment

dplumlee commented Oct 11, 2021

dplumlee commented Oct 11, 2021

dplumlee commented Oct 11, 2021

dplumlee commented Oct 12, 2021

dplumlee commented Oct 12, 2021

kibanamachine commented Oct 12, 2021

💚 Build Succeeded

Metrics [docs]

Async chunks

Page load bundle

History

kibanamachine commented Oct 12, 2021

💚 Backport successful

dplumlee commented Sep 14, 2021 •

edited

Loading