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.

Sign up for GitHub

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

Jump to bottom

[Cases] Refactor attach alert to new case flyout #125505

Merged
merged 28 commits into from
Feb 22, 2022
Merged

[Cases] Refactor attach alert to new case flyout #125505

merged 28 commits into from
Feb 22, 2022

Conversation

academo
Copy link
Contributor

@academo academo commented Feb 14, 2022 •
edited
Loading

Summary

Closes #124524 see there for more details.

Refactors the existing method to open the "attach to new case" flyout in timelines. This PR itself doesn't change any user behaviour or UI.

This PR introduces these changes:

  • Introduces a store, actions, reducer and related hooks inside the CasesContext to be used for plugins integrations: e.g: create case flyout, select case modal, etc...
  • Introduces a the useCasesAddToNewCaseFlyout hook to invoke the add to new case flyout from any third party plugin. To use this hook, plugins must wrap their code in a <CasesContext>
  • Refactor the existing code for "Add to new case" functionality that previously used timelines internal logic to show and hide the flyout to use the new hook.
  • Adjusts tests and mocks to support the new required CasesContext.

Missing in this PR:

A follow up PR will address Refactor plugin cases "Attach to existing case"

Diagrams

In a visual way, the code is coming from this:

image

to this:

image

Checklist

Delete any items that are not applicable to this PR.

@academo academo added Feature:Cases Cases feature Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v8.2.0 labels Feb 14, 2022
@academo academo self-assigned this Feb 14, 2022
@academo academo requested review from cnasikas and XavierM February 14, 2022 13:31
academo
academo commented Feb 15, 2022
View reviewed changes
x-pack/plugins/cases/common/ui/types.ts
@@ -26,15 +26,6 @@ export interface CasesContextFeatures {

export type CasesFeatures = Partial<CasesContextFeatures>;

export interface CasesContextValue {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was moved out of here as it is not a common type for BE and FE but only FE.

Copy link
Contributor

Choose a reason for hiding this comment

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

@academo Out of curiosity what does BE and FE stand for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

BE = Backend, FE = Frontend.

academo
academo commented Feb 15, 2022
View reviewed changes
x-pack/plugins/cases/public/components/cases_context/cases_context_reducer.ts
@@ -0,0 +1,50 @@
/*
Copy link
Contributor Author

@academo academo Feb 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This reducer will be only to handle the state for the CasesContext which will handle the flyout, modals and in general third integrations. It is not intended to be used for all cases plugin purposes.

academo
academo commented Feb 15, 2022
View reviewed changes
x-pack/plugins/cases/public/components/cases_context/cases_context_ui.tsx Outdated
@@ -0,0 +1,21 @@
/*
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the component that decides to show or not the flyout based on the state and eventually the select case modal too and any other integration the cases plugin would offer.

academo
academo commented Feb 15, 2022
View reviewed changes
x-pack/plugins/cases/public/components/cases_context/index.tsx

export const CasesContext = React.createContext<CasesContextValue | undefined>(undefined);
export interface CasesContextValue {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Brought from common types.

academo
academo commented Feb 15, 2022
View reviewed changes
x-pack/plugins/cases/public/components/create/flyout/create_case_flyout.tsx
@@ -16,8 +16,8 @@ import { UsePostComment } from '../../../containers/use_post_comment';

export interface CreateCaseFlyoutProps {
afterCaseCreated?: (theCase: Case, postComment: UsePostComment['postComment']) => Promise<void>;
onClose: () => void;
onSuccess: (theCase: Case) => Promise<void>;
onClose?: () => void;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These attributes are now optional because third party plugins might decide to not need them. The idea is they could simply say: "Open the modal, attach this info to a case" and might or not care about the outcome of the operation.

academo
academo commented Feb 15, 2022
View reviewed changes
x-pack/plugins/cases/public/components/create/flyout/use_cases_add_to_new_case_flyout.tsx
@@ -0,0 +1,45 @@
/*
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This hook is the direct integration with third party plugins. Plugins will load this hook and use it to open or close the flyout as required.

academo
academo commented Feb 15, 2022
View reviewed changes
x-pack/plugins/cases/public/components/create/form.tsx
@@ -57,6 +57,7 @@ const MySpinner = styled(EuiLoadingSpinner)`
`;
export type SupportedCreateCaseAttachment = CommentRequestAlertType | CommentRequestUserType;
export type CreateCaseAttachment = SupportedCreateCaseAttachment[];
export type CaseAttachments = SupportedCreateCaseAttachment[];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a type to replace CreateCaseAttachment. That will come in cleanup PRs to make this PR smaller.

academo
academo commented Feb 15, 2022
View reviewed changes
x-pack/plugins/cases/public/methods/get_cases_context.tsx
@@ -0,0 +1,34 @@
/*
Copy link
Contributor Author

Choose a reason for hiding this comment

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

lazy component to load the CasesContext.

The CasesContext is required to render the cases flyout and eventually the modals. You use the useCreateCaseFlyout hook to interact with this context.

Plugins should wrap the components that will show the flyout or modal on this.

@YulNaumenko YulNaumenko self-requested a review February 16, 2022 17:56
academo
academo commented Feb 17, 2022
View reviewed changes
x-pack/plugins/timelines/public/components/actions/timeline/cases/add_to_new_case_button.tsx
onSuccess: onCaseSuccess,
});

const handleClick = () => {
Copy link
Contributor Author

@academo academo Feb 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This code will be moved out in following PRs. see #125945

cnasikas
cnasikas approved these changes Feb 17, 2022
View reviewed changes
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.

LGTM! Thank you for doing this! 🚀

kqualters-elastic
kqualters-elastic approved these changes Feb 17, 2022
View reviewed changes
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.

👍 changes lgtm, on the right track to removing from timelines

@cnasikas
Copy link
Member

@elasticmachine merge upstream

@academo academo enabled auto-merge (squash) February 21, 2022 08:51
@academo academo mentioned this pull request Feb 21, 2022
Merged
1 task
@academo
Copy link
Contributor Author

academo commented Feb 21, 2022

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Test Failures

  • [job] [logs] Security Solution Tests / Export rules Exports a custom rule

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
cases 386 390 +4

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 51 56 +5

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.0KB 295.0KB +30.0B
observability 394.7KB 394.9KB +200.0B
securitySolution 4.7MB 4.7MB +363.0B
timelines 226.0KB 227.2KB +1.2KB
total +1.8KB

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 19 20 +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 70.3KB 72.5KB +2.1KB
securitySolution 245.3KB 245.4KB +64.0B
total +2.2KB
Unknown metric groups

API count

id before after diff
cases 74 79 +5

ESLint disabled in files

id before after diff
cases 15 16 +1

ESLint disabled line counts

id before after diff
cases 76 77 +1

Total ESLint disabled count

id before after diff
cases 91 93 +2

History

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

cc @academo

@mgiota mgiota self-requested a review February 22, 2022 07:27
mgiota
mgiota approved these changes Feb 22, 2022
View reviewed changes
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.

LGTM! I confirm that add to case works well in Obervability

@academo academo merged commit 1b2f9a4 into elastic:main Feb 22, 2022
mgiota
mgiota reviewed Feb 22, 2022
View reviewed changes
x-pack/plugins/timelines/public/hooks/use_add_to_case.ts
@@ -140,6 +159,7 @@ export const useAddToCase = ({
}
}, [onClose, closePopover, dispatch, eventId]);
return {
caseAttachments,
addNewCaseClick,
Copy link
Contributor

Choose a reason for hiding this comment

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

@academo I was wondering if we can get rid of addNewCaseClick. This is currently used here https://github.com/elastic/kibana/blob/main/x-pack/plugins/timelines/public/components/actions/timeline/cases/add_to_case_action_button.tsx#L49, which actually comes down to https://github.com/elastic/kibana/blob/main/x-pack/plugins/timelines/public/plugin.ts#L100, but I don't see getAddToCasePopover being used anywhere.

My understanding is that add_to_new_case_button.tsx is currently in use, where as add_to_case_action_button.tsx is a leftover. If that's the case, could you get rid of unused stuff in the follow up PRs you are working on?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @mgiota that it is indeed part of the plan. Please check the epic about this chain of PRs. #123183

@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Feb 24, 2022
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 125505 or prevent reminders by adding the backport:skip label.

@cnasikas cnasikas added backport:skip This commit does not require backporting and removed release_note:skip Skip the PR/issue when compiling release notes labels Feb 24, 2022
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Feb 24, 2022
@cnasikas cnasikas added the release_note:skip Skip the PR/issue when compiling release notes label Feb 24, 2022
lucasfcosta pushed a commit to lucasfcosta/kibana that referenced this pull request Mar 2, 2022
@academo @kibanamachine @lucasfcosta
[Cases] Refactor attach alert to new case flyout (elastic#125505)
2c15756 
Co-authored-by: Kibana Machine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mgiota mgiota mgiota approved these changes

@kqualters-elastic kqualters-elastic kqualters-elastic approved these changes

@semd semd semd approved these changes

@cnasikas cnasikas cnasikas approved these changes

@XavierM XavierM Awaiting requested review from XavierM

@peteharverson peteharverson Awaiting requested review from peteharverson

@YulNaumenko YulNaumenko Awaiting requested review from YulNaumenko

Assignees

@academo academo

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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Cases] Refactor plugin cases "Add to new case" flyout show and hide logic out of timelines
8 participants
@academo @elasticmachine @cnasikas @kibana-ci @kibanamachine @mgiota @semd @kqualters-elastic