[Security Solution] [Cases] All Cases Refactor & getAllCasesSelectorModal introduced #97149

stephmilovic · 2021-04-14T16:36:05Z

AllCases broken up into smaller components
- new components: CasesTableHeader, Count, NavButtons, and CasesTableUtilityBar, able to remove useGetCasesStatus, useDeleteCases and useUpdateCases from AllCases
- getCasesColumns upgraded to a hook, useCasesColumns move actions logic into useCasesColumns, able to remove useDeleteCases and dispatchUpdateCaseProperty from AllCases
~~Refactor in AllCases to move anything in jsx markup contained within a {} to a useCallback. See this commit: 9e0b6a6~~ Don't do this, see this slack thread for official recommendation
New AllCases components to manage props
- <AllCases /> (props are same as getAllCases below)
- <AllCasesSelectorModal />(props are same as getAllCasesSelectorModal below)
- <AllCasesGeneric /> - this is formerly just AllCases. 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 underlying getAllCases component to have more specific props. Here is there result:

`getAllCases`

Arguments:

Property	Description
caseDetailsNavigation	`CasesNavigation<CaseDetailsHrefSchema, 'configurable'>` route configuration to generate the case details url for the case details page
configureCasesNavigation	`CasesNavigation` route configuration for configure cases page
createCaseNavigation	`CasesNavigation` route configuration for create cases page
userCanCrud	`boolean;` user permissions to crud

`getAllCasesSelectorModal`

Arguments:

Property	Description
alertData?	`Omit<CommentRequestAlertType, 'type'>;` alert data to post to case
createCaseNavigation	`CasesNavigation` route configuration for create cases page
disabledStatuses?	`CaseStatuses[];` array of disabled statuses
onRowClick	`(theCase?: Case ! SubCase) => void;` callback for row click, passing case in row
updateCase?	`(theCase: Case ! SubCase) => void;` callback after case has been updated
userCanCrud	`boolean;` user permissions to crud

UI component:

stephmilovic · 2021-04-14T17:02:08Z

x-pack/plugins/cases/common/constants.ts

@@ -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;


switch to false before merging!

stephmilovic · 2021-04-14T17:02:25Z

x-pack/plugins/cases/public/components/all_cases/actions.tsx

@@ -16,13 +16,11 @@ import * as i18n from './translations';
 import { isIndividual } from './helpers';

 interface GetActions {
-  caseStatus: string;


stephmilovic · 2021-04-14T17:03:02Z

x-pack/plugins/cases/public/components/all_cases/columns.tsx

 }: GetCasesColumn): CasesColumns[] => {
-  const columns = [
+  const { loading: isLoadingCases, dispatchUpdateCaseProperty } = useGetCases();


move actions for deleting/updating a single case into columns, along with the confirmDeleteModal

x-pack/plugins/cases/public/components/all_cases/index.tsx

elasticmachine · 2021-04-15T17:44:54Z

Pinging @elastic/security-solution (Team: SecuritySolution)

elasticmachine · 2021-04-15T17:44:55Z

Pinging @elastic/security-threat-hunting (Team:Threat Hunting)

stephmilovic · 2021-04-19T20:23:30Z

x-pack/plugins/cases/public/components/links/index.tsx

@@ -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;


made this required, href triggers app reload. we need href for right click > open in new tab

stephmilovic · 2021-04-19T20:23:53Z

x-pack/plugins/cases/public/components/links/index.tsx

@@ -45,8 +45,8 @@ const CaseDetailsLinkComponent: React.FC<{
  const { href: getHref, onClick } = caseDetailsNavigation;
  const goToCaseDetails = useCallback(
    (ev) => {
-      ev.preventDefault();


yikes, this made href only unclickable

michaelolo24 · 2021-04-20T15:44:06Z

x-pack/plugins/cases/public/components/all_cases/all_cases_generic.tsx

+const ProgressLoader = styled(EuiProgress)`
+  ${({ $isShow }: { $isShow: boolean }) =>
+    $isShow
+      ? css`


👍🏾 Nice, didn't know about this

x-pack/plugins/cases/public/components/all_cases/all_cases_generic.tsx

x-pack/plugins/cases/public/components/all_cases/columns.tsx

michaelolo24 · 2021-04-20T16:26:14Z

x-pack/plugins/cases/public/components/all_cases/columns.tsx

+    [dispatchUpdateCaseProperty, refreshCases]
+  );
+
+  const actions = useMemo(


I don't think this needs a useMemo

why not? otherwise it is called each render

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.

michaelolo24 · 2021-04-20T16:30:23Z

x-pack/plugins/cases/public/components/all_cases/columns.tsx

-            <span>{theCase.title}</span>
-          );
+          const caseDetailsLinkComponent =
+            caseDetailsNavigation != null ? (


Maybe just caseDetailsNavigation ? to handle any situation where it may be undefined as well

!= 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

Ahh sorry! Didn't realize it was single equals 👍🏾

michaelolo24 · 2021-04-20T17:21:05Z

x-pack/plugins/cases/public/components/all_cases/selector_modal/index.test.tsx

+        <AllCasesSelectorModal {...fullProps} />
+      </TestProviders>
+    );
+    // @ts-ignore idk what this mock style is but it works ¯\_(ツ)_/¯


lol, love the comment. This just to check the props?

michaelolo24 · 2021-04-20T17:22:41Z

x-pack/plugins/cases/public/components/all_cases/nav_buttons.tsx

@@ -0,0 +1,55 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one


This is so much cleaner!

x-pack/plugins/cases/public/components/all_cases/utility_bar.tsx

cnasikas · 2021-04-21T08:57:59Z

x-pack/plugins/cases/public/components/all_cases/count.tsx

+import { useGetCasesStatus } from '../../containers/use_get_cases_status';
+
+interface CountProps {
+  refresh?: number;


Why refresh is a number?

This way the parent can trigger an update in the child. When the number changes, a useEffect runs the re-fetch function

This is where the update is coming from: https://github.com/elastic/kibana/pull/97149/files/f3a7c5688262ee1f7d79ecf542e09a9d6e12c2c5#diff-49443f63bd02ccea55f5419c52288cbc0f7d6c7859e79dc5ebb75dcf45ff3b31R110-R122

I think we can do better now, we are smarter than a year ago

Speak for yourself 😂

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.

I see. Smart!

x-pack/plugins/cases/public/components/configure_cases/index.tsx

cnasikas · 2021-04-21T13:17:05Z

I really like this PR and the effort you put to make the AllCases component better, scalable, and cleaner. I love the smaller components like CasesTableFilters and CasesTable. Amazing job!

I see that the iseSelectorView is used by:

The goToCreateCase function
showActions variable
TableWrap component
tableRowProps function
In the CasesTable. It is being passed as a prop.

All other code inside the AllCasesGeneric component is the same for both the AllCases component and the AllCasesSelectorModal component. I am thinking, without being of course right, now that we have this beautiful components, maybe it is better to create a hook called useAllCases that will contain all the common logic that is right now inside AllCasesGeneric component. the AllCases component and the AllCasesSelectorModal component can use this hook to get what they need and each one of the component can define the pieces in the code they differ (the numbered list above). Example:

export const AllCases: React.FC<AllCasesProps> = (props) => {
  // The hook exports all needed variables that will be passed as props
  // to the components
  const { actionLicense, columns, data, filterOptions, ... } = useAllCases();

  const goToCreateCase = useCallback(
      (ev) => {
        ev.preventDefault();
        onCreateCaseNavClick(ev);
      },
      [onCreateCaseNavClick]
    );

    const tableRowProps = useCallback(
      (theCase: Case) => {
        return {
          'data-test-subj': `cases-table-row-${theCase.id}`,
          className: classnames({ isDisabled: theCase.type === CaseType.collection }),
        };
      },
      [isSelectorView, alertData, onRowClick, postComment, updateCase]
    );

    // I omitted the props and the logic for readability
    <Callout />
    <CasesTableHeader />
    <ProgressLoader />
    // TableWrap is a Panel for AllCases
    <Panel>
      <CasesTableFilters/>
      <CasesTable isSelectorView={false} showActions={true}/>
    </Panel>

};

export const AllCasesSelectorModal: React.FC<AllCasesSelectorModalProps> = ({
  alertData,
  createCaseNavigation,
  disabledStatuses,
  onRowClick,
  updateCase,
  userCanCrud,
}) => {
  // modal logic omitted for readability

  const { actionLicense, columns, data, filterOptions, ... } = useAllCases();
  const { postComment, isLoading: isCommentUpdating } = usePostComment();

  const tableRowProps = useCallback(
      (theCase: Case) => {
        const onTableRowClick = memoize(async () => {
          if (alertData != null) {
            await postComment({
              caseId: theCase.id,
              data: {
                type: CommentType.alert,
                ...alertData,
              },
              updateCase,
            });
          }
          if (onRowClick) {
            onRowClick(theCase);
          }
        });

        return {
          'data-test-subj': `cases-table-row-${theCase.id}`,
          className: classnames({ isDisabled: theCase.type === CaseType.collection }),
          { onClick: onTableRowClick }
        };
      },
      [isSelectorView, alertData, onRowClick, postComment, updateCase]
    );

  // I omitted the props and the logic for readability
    <Callout />
    // TableWrap is a span for AllCasesSelectorModal
    <span>
      <CasesTableFilters/>
      <CasesTable isSelectorView={true} showActions={false}/>
    </span>
};
// eslint-disable-next-line import/no-default-export
export { AllCasesSelectorModal as default };

@stephmilovic @michaelolo24 @XavierM What do you think?

XavierM · 2021-04-21T14:13:17Z

@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.

stephmilovic · 2021-04-21T14:16:22Z

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 getAllCases and getAllCasesSelectorModal, same arguments passed for each. IMO it doesn't add value, it's just a different way to do it. Neither way is right, neither is wrong. I don't think one is better than the other. If you would like to spend your time on this, go for it, I won't stop you. 😋

XavierM · 2021-04-21T14:28:23Z

That's a good refactor

cnasikas · 2021-04-21T14:58:02Z

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 😋

michaelolo24

Way to rock this LGTM!

kibanamachine · 2021-04-21T18:57:19Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`cases`	243	252	+9
`securitySolution`	1984	1981	-3
total			+6

Async chunks

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

id	before	after	diff
`cases`	497.7KB	491.4KB	-6.3KB
`securitySolution`	6.5MB	6.5MB	-3.4KB
total			-9.8KB

Page load bundle

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

id	before	after	diff
`cases`	124.7KB	126.8KB	+2.0KB

Unknown metric groups

API count

id	before	after	diff
`cases`	348	349	+1

API count missing comments

id	before	after	diff
`cases`	336	337	+1

async chunk count

id	before	after	diff
`cases`	12	14	+2

Non-exported public API item count

id	before	after	diff
`cases`	6	7	+1

History

💚 Build #121392 succeeded 5192797
💔 Build #121229 failed f3a7c56
💔 Build #121195 failed 3f00989
💔 Build #120993 failed a7df6c8
💚 Build #120748 succeeded aeea4ac

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

stephmilovic · 2021-04-21T19:34:25Z

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 🎢

stephmilovic added 5 commits April 13, 2021 11:22

create case modal to use method

e0cfb51

gobbledegook

e91a2e7

ok working

97244cf

pulled second layer filters into ocmponet

e8675f3

cleanup

6b92abc

stephmilovic changed the title ~~[Security Solution] [Cases] [WIP] Break up All Cases into smaller components and make modals nicer~~ [Security Solution] [Cases] [skip-ci] Break up All Cases into smaller components and make modals nicer Apr 14, 2021

more

e6c6956

stephmilovic commented Apr 14, 2021

View reviewed changes

x-pack/plugins/cases/public/components/all_cases/index.tsx Outdated Show resolved Hide resolved

stephmilovic added 6 commits April 14, 2021 12:10

switched to modal from cases

d693aa1

Merge branch 'cases_rac_ui' into modals_cases

895a76e

working, need tests to fix

a4d4abc

switch from hook to component

20971db

pairing sesh

7f812c6

get them tests right

59a8cfd

stephmilovic changed the title ~~[Security Solution] [Cases] [skip-ci] Break up All Cases into smaller components and make modals nicer~~ [Security Solution] [Cases] Break up All Cases into smaller components and make modals nicer Apr 15, 2021

stephmilovic marked this pull request as ready for review April 15, 2021 17:44

stephmilovic requested a review from a team as a code owner April 15, 2021 17:44

stephmilovic commented Apr 19, 2021

View reviewed changes

Merge branch 'cases_rac_ui' into modals_cases

a7df6c8

michaelolo24 reviewed Apr 20, 2021

View reviewed changes

x-pack/plugins/cases/public/components/all_cases/all_cases_generic.tsx Outdated Show resolved Hide resolved

michaelolo24 reviewed Apr 20, 2021

View reviewed changes

x-pack/plugins/cases/public/components/all_cases/all_cases_generic.tsx Outdated Show resolved Hide resolved

michaelolo24 reviewed Apr 20, 2021

View reviewed changes

x-pack/plugins/cases/public/components/all_cases/columns.tsx Outdated Show resolved Hide resolved

michaelolo24 reviewed Apr 20, 2021

View reviewed changes

x-pack/plugins/cases/public/components/all_cases/utility_bar.tsx Outdated Show resolved Hide resolved

stephmilovic added 3 commits April 20, 2021 13:44

Merge branch 'cases_rac_ui' into modals_cases

3f00989

pr changes 1

cefcc6d

Merge branch 'cases_rac_ui' into modals_cases

f3a7c56

cnasikas reviewed Apr 21, 2021

View reviewed changes

x-pack/plugins/cases/public/components/configure_cases/index.tsx Outdated Show resolved Hide resolved

stephmilovic added 2 commits April 21, 2021 08:19

update renamed var in the tests whoops

6989af2

fix type

5192797

michaelolo24 approved these changes Apr 21, 2021

View reviewed changes

stephmilovic mentioned this pull request Apr 21, 2021

[SECURITY SOLUTIONS] [Case everywhere] Remaining cases removal work #96074

Closed

12 tasks

revert to useMemo

9e98bd9

stephmilovic merged commit 54c97d4 into elastic:cases_rac_ui Apr 21, 2021

		@@ -0,0 +1,55 @@
		/*
		* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one

[Security Solution] [Cases] All Cases Refactor & getAllCasesSelectorModal introduced #97149

[Security Solution] [Cases] All Cases Refactor & getAllCasesSelectorModal introduced #97149

Conversation

stephmilovic commented Apr 14, 2021 • edited Loading

getAllCases

getAllCasesSelectorModal

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Apr 15, 2021

elasticmachine commented Apr 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelolo24 Apr 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephmilovic Apr 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XavierM Apr 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cnasikas commented Apr 21, 2021 • edited Loading

XavierM commented Apr 21, 2021 • edited Loading

stephmilovic commented Apr 21, 2021

XavierM commented Apr 21, 2021

cnasikas commented Apr 21, 2021 • edited Loading

michaelolo24 left a comment

Choose a reason for hiding this comment

kibanamachine commented Apr 21, 2021

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

Page load bundle

API count

API count missing comments

async chunk count

Non-exported public API item count

History

stephmilovic commented Apr 21, 2021

stephmilovic commented Apr 14, 2021 •

edited

Loading

`getAllCases`

`getAllCasesSelectorModal`

michaelolo24 Apr 21, 2021 •

edited

Loading

stephmilovic Apr 20, 2021 •

edited

Loading

XavierM Apr 21, 2021 •

edited

Loading

cnasikas commented Apr 21, 2021 •

edited

Loading

XavierM commented Apr 21, 2021 •

edited

Loading

cnasikas commented Apr 21, 2021 •

edited

Loading