Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Cases] Refactor: Move cases action buttons out of timelines #126265

Merged

Conversation

academo
Copy link
Contributor

@academo academo commented Feb 23, 2022

Summary

See epic: #123183

Follow up to #125782 and #125782
Closes #126069 and #126070

After #125782 and #125782 it is now required to remove the cases "add to new case" and "add to existing case" out of timelines.

No visual or ux changes.

Changes:

  1. Removes the usage of the timelines hook useAddToCase and replace them for the new cases hooks
  2. Implements the button UI inside each cases consumer plugin (security and observability)

Missing in this PR for follow up PRs:

Visually this is how the new flow for timelines action buttons related to cases work.
image

Checklist

Delete any items that are not applicable to this PR.

Esteban Beltran and others added 30 commits February 22, 2022 10:59
…5931)

* showing agent policy creation error message on UI

* mapping the error instead of showing from the backend

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Copy link
Contributor

@peteharverson peteharverson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested and LGTM. Just left a question about a TODO comment.

Esteban Beltran added 2 commits March 1, 2022 16:41
Copy link
Member

@cnasikas cnasikas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job 🚀 ! Amazing as always. I tested and everything is working as expected.

Copy link
Contributor

@kqualters-elastic kqualters-elastic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

regardless of the ongoing context and plugin api discussion, this is still another step in the right direction. once @cnasikas 's suggestion about moving toaster usage into cases is done, lgtm 👍

@academo
Copy link
Contributor Author

academo commented Mar 2, 2022

regardless of the ongoing context and plugin api discussion, this is still another step in the right direction. once @cnasikas 's suggestion about moving toaster usage into cases is done, lgtm +1

The toast had been moved :)

@mgiota mgiota self-requested a review March 2, 2022 11:34
Copy link
Contributor

@mgiota mgiota left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good in the Observability Alerts page. Recently we integrated Alerts in the Overview page as well. Currently Overview page is broken
Screenshot 2022-03-02 at 15 33 51

@mgiota
Copy link
Contributor

mgiota commented Mar 2, 2022

@academo Also make sure to add proper labelling to the these tickets #126069 and #126070 so that they are properly linked with this PR

const ruleId = alert.fields['kibana.alert.rule.uuid'] ?? null;
const linkToRule = ruleId ? prepend(paths.management.ruleDetails(ruleId)) : null;

const caseAttachments: CaseAttachments = useMemo(() => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@academo As discussed I will create a separate ticket to move all these in a separate file once this PR is merged

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mgiota of course I can help you move that code out when you start working on it. We can make it a custom hook.

@academo academo requested a review from a team as a code owner March 2, 2022 15:23
@academo
Copy link
Contributor Author

academo commented Mar 2, 2022

Changes look good in the Observability Alerts page. Recently we integrated Alerts in the Overview page as well. Currently Overview page is broken Screenshot 2022-03-02 at 15 33 51

Thanks for the report @mgiota This is now fixed.

@mgiota mgiota self-requested a review March 2, 2022 16:10
Copy link
Contributor

@mgiota mgiota left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@academo I confirm that Overview page is fixed after your latest changes!

@academo academo enabled auto-merge (squash) March 2, 2022 16:15
Copy link
Contributor

@peteharverson peteharverson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested latest changes and confirmed the actions work on the Observability Overview page for me too.

rangeTo={relativeTime.end}
indexNames={indexNames}
/>
<CasesContext
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@academo Thanks for the fix. We will make sure to create a wrapper component that contains cases context so that both alerts and overview page can share

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mgiota I was thinking on putting the CasesContext all the way up in the observability plugin. To have it available everywhere. What do you think?

Copy link
Contributor

@mgiota mgiota Mar 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep exactly that was my thought as well. This will avoid us having to repeat the same code in multiple places

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not take this line of reasoning 1 step further? 😬

@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Test Failures

  • [job] [logs] Security Solution Tests / Detections > Callouts indicating read-only access to resources On Detections home page We show one primary callout
  • [job] [logs] Security Solution Tests / Detections > Callouts indicating read-only access to resources On Detections home page When a user clicks Dismiss on the callout We hide it and persist the dismissal
  • [job] [logs] Security Solution Tests / Detections > Need Admin Callouts indicating an admin is needed to migrate the alert data set The users index_mapping_outdated is "false" and their admin callouts should not show up On Rule Details page "before each" hook for "We show 1 primary callout"
  • [job] [logs] Security Solution Tests / Detections > Need Admin Callouts indicating an admin is needed to migrate the alert data set The users index_mapping_outdated is "true" and their admin callouts should show up On Detections home page We show the need admin primary callout
  • [job] [logs] Security Solution Tests / Row renderers Suricata Signature tooltips do not overlap

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
cases 391 393 +2
observability 360 361 +1
total +3

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
cases 59 61 +2

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
cases 295.6KB 289.7KB -6.0KB
observability 394.7KB 395.6KB +896.0B
securitySolution 4.7MB 4.7MB +693.0B
total -4.4KB

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id before after diff
cases 20 19 -1

Page load bundle

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

id before after diff
cases 74.1KB 82.6KB +8.5KB
observability 87.3KB 87.5KB +135.0B
total +8.6KB
Unknown metric groups

API count

id before after diff
cases 82 84 +2

History

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

cc @academo

@academo academo merged commit 83105bd into elastic:main Mar 3, 2022
@cnasikas cnasikas added the backport:skip This commit does not require backporting label Mar 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Cases Cases feature release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v8.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor "Add to new case" timeline action button to move it out of timelines.