-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[Security Solution] [Cases] Deleting files from security solution and removing feature flag #95176
[Security Solution] [Cases] Deleting files from security solution and removing feature flag #95176
Conversation
@@ -5,4 +5,4 @@ | |||
* 2.0. | |||
*/ | |||
|
|||
export * from '../../translations'; | |||
export * from '../../public/containers/types'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is weird. this is a brand new file, but github thinks it comes from one of my deleted files?
const onTableRowClick = memoize(() => { | ||
const onTableRowClick = memoize(async () => { | ||
if (alertData != null) { | ||
await postComment({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this was previously in security_solution/public/cases/components/timeline_actions/add_to_case_action.tsx
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Imo, this shouldn't be inside the component. It is better to be implemented inside the onRowClick
where the consumer of the component pass it as a prop. If security solution, needs access to the postComment
function we can provided it through our casesClient
(backend).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i didnt want the ui components to rely on any api methods since actions dont, they should just work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see what you mean. I talk about it with @XavierM and we think that we should break the AllCases
components into smaller components and remove the isModal
logic. This way we can move the onTableRowClick
and alertData
logic inside a new modal called, for example, AttachToCaseModal
which can be exported through the CasesUiClient
.
Example:
// AllCases component
interface Props {
configureCasesHref: string;
createCaseHref: string;
disabledStatuses?: CaseStatuses[];
getCaseDetailsHref: (caseDetails: CaseDetailsHrefSchema) => string;
onCaseDetailsNavClick: (caseDetails: CaseDetailsHrefSchema) => void;
onConfigureCasesNavClick?: (ev: React.MouseEvent) => void;
onCreateCaseNavClick?: (ev: React.MouseEvent) => void;
userCanCrud: boolean;
}
<>
<AllCasesHeader>
<AllCasesFilters>
<AllCasesTable>
</>
// AttachToCase modal
interface Props {
onAttachment: () => void;
}
<>
<AllCasesFilters>
<AllCasesTable>
</>
I think is a good opportunity to do it in this PR. It will be much cleaner. The AllCases
is too compact, does a lot of things without reason.
@@ -36,8 +36,6 @@ const CaseParamsFields: React.FunctionComponent<ActionParamsProps<CaseActionPara | |||
actionParams, | |||
editAction, | |||
index, | |||
errors, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these were unused
@@ -26,21 +27,36 @@ const Container = styled.div` | |||
|
|||
export interface CreateCaseProps { | |||
afterCaseCreated?: (theCase: Case) => Promise<void>; | |||
caseType?: CaseType; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i missed a few properties from create case from modal/flyout
perPage: number; | ||
} | ||
|
||
const RecentCases = ({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -131,15 +131,15 @@ export interface UseGetCases extends UseGetCasesState { | |||
} | |||
|
|||
export const useGetCases = ( | |||
initialQueryParams?: QueryParams, | |||
initialFilterOptions?: FilterOptions | |||
initialQueryParams: Partial<QueryParams> = {}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed this so components passing arguments don't need to import DEFAULT_FILTER_OPTIONS
or DEFAULT_QUERY_PARAMS
, only need to pass the params that are different from default
useInsertTimeline(formData[descriptionFieldName] ?? '', onTimelineAttached); | ||
return null; | ||
}; | ||
// TO DO: Cases RAC UI, reimplement this shiz |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😂
userCanCrud={userPermissions?.crud ?? false} | ||
/> | ||
)} | ||
<CaseView |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gracias 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Way to crush it!
@@ -7,3 +7,4 @@ | |||
|
|||
export * from './constants'; | |||
export * from './api'; | |||
export * from './ui/types'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the ui types are being exported? Will they be used by another plugin?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, security solution uses the types on the UI
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see! Thanks!
|
||
UI component: | ||
![Recent Cases Component][recent-cases-img] | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know is out of the scope of this PR but could you add a note that the case action type is disabled by default and the connector is not supported?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -5,13 +5,16 @@ | |||
* 2.0. | |||
*/ | |||
|
|||
import { CoreStart, Plugin, PluginInitializerContext } from 'src/core/public'; | |||
import { CoreSetup, CoreStart, Plugin, PluginInitializerContext } from 'src/core/public'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very clean file. I love it!
x-pack/plugins/security_solution/public/overview/components/recent_cases/index.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/cases/public/components/recent_cases/loading_placeholders.tsx
Show resolved
Hide resolved
x-pack/plugins/cases/public/components/recent_cases/no_cases/index.test.tsx
Outdated
Show resolved
Hide resolved
const ref = useRef(); | ||
useEffect(() => { | ||
(ref.current as unknown) = value; | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will run on every render, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think so, copy/pasted over from @andrew-goldstein
}; | ||
|
||
// eslint-disable-next-line import/no-default-export | ||
export { RecentCases as default }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why don't we export React.memo(RecentCases);
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why tho? what benefit does wrapping it in React.memo
give?
...ck/plugins/security_solution/public/cases/components/timeline_actions/add_to_case_action.tsx
Show resolved
Hide resolved
...ck/plugins/security_solution/public/cases/components/timeline_actions/add_to_case_action.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very good PR. I left some comments. The most important one is the one about the alertData
we are passing to the AllCases
component.
I did the following tests:
- Recent cases widget:
- Create case button 🟢
- Switch between filters 🟢
- View all cases 🟢
- All cases:
- KPI 🟢
- Bulk actions 🟢
- Change status from actions 🟢
- Filter by status, tags, reporters 🟢
- Search bar 🟢
- Pagination 🟢
- Correct data on the table 🟢
- Sort by alerts or comments 🔴 (I think this is a know issue)
- Configuration page:
- Create connectors 🟢
- Select connector 🟢
- Go back to all cases 🟢
- Change closure options 🟢
- Creation form:
- Create case 🟢
- Cancel 🟢
- Default connector 🟢
- Connectors' fields 🟢
- View case
- Add comment 🟢
- Edit tags, title, description, connector 🟢
- User actions 🟢
- Push case to all supported connectors 🟢
- Delete case 🟢
- View external service incident 🟢
- Alerts:
- See attach alerts on a case 🟢
- Show alerts details inside a case 🟢
- Navigate to alert's rule from a case 🟢
- Alert status is changed when changing the status of a case and sync is on.
- Attach alert to a new case (flyout) 🟢
- Attach alert to an existing case (modal) 🟢
- Timeline
- Cannot attach a timeline to a new case 🔴 (I think this is a know issue)
- Cannot attach a timeline to an existing case 🔴 (I think this is a know issue)
- Timeline closes after pressing attach to an existing case 🔴 (even thought the application is being navigated to the case view page).
Bugs:
- When you change the status of a case the push button appears. This issue in unrelated to this PR.
- The title of the case is not being shown on the path
- Comments tooltip in the recent cases widget is not being shown correctly
💚 Build SucceededMetrics [docs]
History
To update your PR or re-run it, just comment with: |
Deleting files from security solution and removing feature flag. Security Solution now fully relies upon Cases for all UI.
More moved:
plugins.triggersActionsUi.actionTypeRegistry.register(getCaseConnectorUi());
is now called fromcases
rather thansecurity_solution
x-pack/plugins/security_solution/public/cases/components/create/flyout.tsx
x-pack/plugins/security_solution/public/overview/components/recent_cases/
was moved tox-pack/plugins/cases/public/components/recent_cases/
. theindex.tsx
insecurity_solution
now calls a new method,cases.getRecentCases
New method:
getRecentCases
Arguments:
string;
href of all cases pagestring;
(caseDetails: CaseDetailsHrefSchema) => string;
(ev: React.MouseEvent) => void;
callback for all cases link click(caseDetails: CaseDetailsHrefSchema) => void;
callback for case details nav clicknumber;
number of cases to show in widgetUI component: