-
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] All Cases Refactor & getAllCasesSelectorModal introduced #97149
[Security Solution] [Cases] All Cases Refactor & getAllCasesSelectorModal introduced #97149
Conversation
@@ -58,4 +58,4 @@ export const MAX_GENERATED_ALERTS_PER_SUB_CASE = 50; | |||
/** | |||
* This flag governs enabling the case as a connector feature. It is disabled by default as the feature is not complete. | |||
*/ | |||
export const ENABLE_CASE_CONNECTOR = false; | |||
export const ENABLE_CASE_CONNECTOR = true; |
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.
switch to false before merging!
@@ -16,13 +16,11 @@ import * as i18n from './translations'; | |||
import { isIndividual } from './helpers'; | |||
|
|||
interface GetActions { | |||
caseStatus: string; |
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.
unused
}: GetCasesColumn): CasesColumns[] => { | ||
const columns = [ | ||
const { loading: isLoadingCases, dispatchUpdateCaseProperty } = useGetCases(); |
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.
move actions for deleting/updating a single case into columns, along with the confirmDeleteModal
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.
dope
Pinging @elastic/security-solution (Team: SecuritySolution) |
Pinging @elastic/security-threat-hunting (Team:Threat Hunting) |
@@ -18,7 +18,7 @@ import * as i18n from './translations'; | |||
|
|||
export interface CasesNavigation<T = React.MouseEvent | MouseEvent, K = null> { | |||
href: K extends 'configurable' ? (arg: T) => string : string; | |||
onClick?: (arg: T) => void; | |||
onClick: (arg: T) => void; |
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.
made this required, href triggers app reload. we need href for right click > open in new tab
@@ -45,8 +45,8 @@ const CaseDetailsLinkComponent: React.FC<{ | |||
const { href: getHref, onClick } = caseDetailsNavigation; | |||
const goToCaseDetails = useCallback( | |||
(ev) => { | |||
ev.preventDefault(); |
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.
yikes, this made href only unclickable
const ProgressLoader = styled(EuiProgress)` | ||
${({ $isShow }: { $isShow: boolean }) => | ||
$isShow | ||
? css` |
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.
👍🏾 Nice, didn't know about this
x-pack/plugins/cases/public/components/all_cases/all_cases_generic.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/cases/public/components/all_cases/all_cases_generic.tsx
Outdated
Show resolved
Hide resolved
[dispatchUpdateCaseProperty, refreshCases] | ||
); | ||
|
||
const actions = useMemo( |
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 don't think this needs a useMemo
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 not? otherwise it is called each render
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.
You can ignore this. I dug into the actions call a bit deeper. What I was thinking was that since all it's doing is just setting properties and returning an object, the actual call isn't that computationally expensive, but I missed maintaining the referential equality when it's being passed to the CasesTable
and expandedRowMap
.
<span>{theCase.title}</span> | ||
); | ||
const caseDetailsLinkComponent = | ||
caseDetailsNavigation != null ? ( |
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.
Maybe just caseDetailsNavigation ?
to handle any situation where it may be undefined
as well
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.
!= null
checks that the value is not null
and that the value is not undefined
. if it was !== null
, it would only check against null
and allow undefined
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.
Ahh sorry! Didn't realize it was single equals 👍🏾
<AllCasesSelectorModal {...fullProps} /> | ||
</TestProviders> | ||
); | ||
// @ts-ignore idk what this mock style is but it works ¯\_(ツ)_/¯ |
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.
lol, love the comment. This just to check the props?
@@ -0,0 +1,55 @@ | |||
/* | |||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one |
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 so much cleaner!
x-pack/plugins/cases/public/components/all_cases/utility_bar.tsx
Outdated
Show resolved
Hide resolved
import { useGetCasesStatus } from '../../containers/use_get_cases_status'; | ||
|
||
interface CountProps { | ||
refresh?: number; |
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 refresh is a number?
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 way the parent can trigger an update in the child. When the number changes, a useEffect runs the re-fetch function
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.
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 we can do better now, we are smarter than a year ago
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.
Speak for yourself 😂
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.
Lol, I know we did something similar in resolver (the code may have been removed now), but it was to make sure that if multiple requests are made we could abort an earlier one so we didn't get out of sync data. @kqualters-elastic worked on it I believe.
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. Smart!
x-pack/plugins/cases/public/components/configure_cases/index.tsx
Outdated
Show resolved
Hide resolved
I really like this PR and the effort you put to make the I see that the
All other code inside the
@stephmilovic @michaelolo24 @XavierM What do you think? |
@cnasikas about your last comment TBH, I do not see a big difference between the hook and the generic component (new vs old, you are younger @cnasikas ❤️ ) . I think that with the hook we will have some duplication functionality in these two components. I do not mind either way but I do like that we were able to make our API for these components simpler for our users and not having branch logic on props to show one or the other components. I do like the generic component for now, the hook did not come in my mind when we discussed the problem. To conclude, I think we can keep the way it is. However if @stephmilovic see 👀 to 😍 with you, I will support your decision. |
Responding to @cnasikas without copy/pasting his comment. I mean... we could. What value does it add? It seems like you're just reorganizing the code. The end user would still get the same casesUi API, same methods |
Thank you @stephmilovic @XavierM for your answers. I like this type of discussions because I learn a lot from them. I agree that both approaches have their merits and neither is better than the other one. @michaelolo24 made me realise that the view level is so similar as we only have a single boolean check. If these component diverge more we can then think about implementing the hook. Let's the old live a bit longer 😋 |
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 rock this LGTM!
💚 Build SucceededMetrics [docs]Module Count
Async chunks
Page load bundle
Unknown metric groupsAPI count
API count missing comments
async chunk count
Non-exported public API item count
History
To update your PR or re-run it, just comment with: |
I'm going to merge cuz i got that ✅ . I will pair with @XavierM on whatever smart refresh child method he has in store for me. Look for a quick follow up PR 🎢 |
AllCases
broken up into smaller componentsCasesTableHeader
,Count
,NavButtons
, andCasesTableUtilityBar
, able to removeuseGetCasesStatus
,useDeleteCases
anduseUpdateCases
fromAllCases
getCasesColumns
upgraded to a hook,useCasesColumns
move actions logic intouseCasesColumns
, able to removeuseDeleteCases
anddispatchUpdateCaseProperty
fromAllCases
Refactor inDon't do this, see this slack thread for official recommendationAllCases
to move anything in jsx markup contained within a{}
to a useCallback. See this commit: 9e0b6a6New AllCases components to manage props
<AllCases />
(props are same asgetAllCases
below)<AllCasesSelectorModal />
(props are same asgetAllCasesSelectorModal
below)<AllCasesGeneric />
- this is formerly justAllCases
. This is the management component. These props are a conditional combo of the 2 above and not exposed to the consumer.New
casesUi
method,getAllCasesSelectorModal
. Change underlyinggetAllCases
component to have more specific props. Here is there result:getAllCases
Arguments:
CasesNavigation<CaseDetailsHrefSchema, 'configurable'>
route configuration to generate the case details url for the case details pageCasesNavigation
route configuration for configure cases pageCasesNavigation
route configuration for create cases pageboolean;
user permissions to crudgetAllCasesSelectorModal
Arguments:
Omit<CommentRequestAlertType, 'type'>;
alert data to post to caseCasesNavigation
route configuration for create cases pageCaseStatuses[];
array of disabled statuses(theCase?: Case ! SubCase) => void;
callback for row click, passing case in row(theCase: Case ! SubCase) => void;
callback after case has been updatedboolean;
user permissions to crudUI component: