[SecuritySolution] Retrieve onboarding steps by angorayc · Pull Request #181442 · elastic/kibana

angorayc · 2024-04-23T13:58:34Z

Summary

Fixes #181190
Relevant PRs: #143598 | #163739

Steps to Verify:

Entering these in your kibana.dev.yml
Execute this command to set the Guided onboarding steps to alertsCases

curl --location --request PUT 'http://localhost:5601/internal/guided_onboarding/state' \
--header 'kbn-xsrf: cypress-creds' \
--header 'x-elastic-internal-origin: security-solution' \
--header 'Content-Type: application/json' \
--header 'Authorization: Basic {YOUR_AUTH_TOKEN}' \.    // If you are using Postman just fill in the Auth tab
--data '{
  "status": "in_progress",
  "guide": {
    "isActive": true,
    "status": "in_progress",
    "steps": [
      { "id": "add_data", "status": "complete" },
      { "id": "rules", "status": "complete" },
      { "id": "alertsCases", "status": "active" }
    ],
    "guideId": "siem"
  }
}'

Make sure you have alerts available.
To test the old flyout with Guided onboarding tour, please go to Stack Management > Advanced settings > Expandable flyout OFF

It compatible with the new expandable flyout:

It shows expandable flyout tour when the guided onboarding tour is not enabled.
The first two steps should be hidden when the left expandable is opened.
Most of the guided onboarding tour steps should be hidden when Add to new case flyout or Add to existing case modal opened.
Once the test case is created, the insight section and correlation tab should be opened automatically to fetch cases.
expandable flyout tour should be visible again after the guided onboarding tour is finished.

Screen.Recording.2024-05-03.at.14.30.03.mov

It compatible with the old flyout:

Screen.Recording.2024-05-01.at.21.19.29.mov

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Unit or functional tests were updated or added to match the most common scenarios

angorayc · 2024-05-01T21:53:24Z

x-pack/plugins/security_solution/public/common/components/event_details/event_details.tsx

  );

  return (
-    <GuidedOnboardingTourStep


We should only wrap only the target tab with the GuidedOnboardingTourStep to avoid adding extra attributes to other tabs in their DOM elements and cause the tour anchor locate incorrectly.

angorayc · 2024-05-01T21:55:15Z

x-pack/plugins/security_solution/public/common/components/guided_onboarding_tour/tour_step.tsx

+  const isAddToNewCaseFlyoutOpen = cases.hooks.useIsAddToNewCaseFlyoutOpen();
+  const isAddToExistingCaseModalOpen = cases.hooks.useAddToExistingCaseModalOpen();
+
+  const hiddenWhenCasesModalFlyoutExpanded = useMemo(


To avoid this scenario:

angorayc · 2024-05-01T21:57:29Z

x-pack/plugins/cases/public/plugin.ts

      hooks: {
        useCasesAddToNewCaseFlyout,
        useCasesAddToExistingCaseModal,
+        useIsAddToNewCaseFlyoutOpen,


Please find our use cases here

angorayc · 2024-05-01T21:59:04Z

...k/plugins/security_solution/public/flyout/document_details/left/components/related_cases.tsx

-      </CellTooltipWrapper>
-    ),
+    render: (value: string, caseData: RelatedCase) => {
+      const index = data.findIndex((d) => d.id === caseData.id);


In the existing guided onboarding tour's logic, it only shows the tour step if index equals to 0

angorayc · 2024-05-01T22:00:28Z

...ugins/security_solution/public/flyout/document_details/right/components/insights_section.tsx

+  const { activeStep, isTourShown } = useTourContext();
+  const isGuidedOnboardingTourShown =
+    isTourShown(SecurityStepId.alertsCases) && activeStep === AlertsCasesTourSteps.viewCase;
+  const expanded =


Expand the insight section automatically after the test case created, so the relevant cases can be fetched.

angorayc · 2024-05-01T22:02:10Z

x-pack/plugins/security_solution/public/flyout/document_details/right/header.tsx

+    );
+
+    const renderTabs = tabs.map((tab, index) =>
+      isAlert && tab.id === 'overview' ? (


Same reason as this one: https://github.com/elastic/kibana/pull/181442/files#r1586866840

angorayc · 2024-05-02T13:44:25Z

...tion/public/detections/components/alerts_table/timeline_actions/use_add_exception_flyout.tsx

+  const onAddExceptionTypeClick = useCallback(
+    (exceptionListType?: ExceptionListTypeEnum): void => {
+      setExceptionFlyoutType(exceptionListType ?? null);
+      setAllTourStepsHidden(true);


Hide the tour when flyout open to avoid this situation:

angorayc · 2024-05-02T13:46:50Z

buildkite test this

angorayc · 2024-05-07T09:45:46Z

/ci

angorayc · 2024-05-07T11:48:15Z

buildkite test this

angorayc · 2024-05-07T15:41:42Z

buildkite test this

semd · 2024-05-21T09:48:17Z

@angorayc I added React.memo to the SecurityTourStep component, it prevents some unnecessary updates from happening: 05784f5

semd · 2024-05-21T10:18:56Z

Hi @PhilippeOberti, I've been checking the re-renders you mentioned. It's an interesting point, the reason we see the table cells highlighted is not the DefaultCellRenderer component being re-rendered, it's the useHiddenByFlyout hook that receives an update from the useLocation() call, due to the flyout URL state change.

This hook is the only code that gets re-executed, it needs to check if the flyout "expanded" value has changed, there's no way around that if we want to have a tour anchor inside a table cells.

The good news is that the only component that updates is the GuidedOnboardingTourStep wrapper. The DefaultCellRenderer children component never receives any update when the flyout state changes. On top of that, I made a small change (2534c07) so the only GuidedOnboardingTourStep cell that updates is the one that has the anchor, the rest of the cells are rendered directly:

tour_updates_only_anchor.mov

angorayc · 2024-05-22T08:34:58Z

/ci

kqualters-elastic

this seems like a lot of changes to many different cross cutting concerns to fix something that I think could/should be done in the onboarding code, but the re-renders are no longer as bad as they were, lgtm 👍

cnasikas

I understand the reasoning behind adding the casesContextState$ in the cases context provider value but I think it breaks the encapsulation between the internal state and the value of the provider. Instead of exposing the state this way, what do you think about passing a callback to the provider? Example:

export const CasesProvider: FC<
  PropsWithChildren<{
    value: CasesContextProps;
    queryClient?: QueryClient;
  }>
> = ({
  children,
  value: {
    externalReferenceAttachmentTypeRegistry,
    persistableStateAttachmentTypeRegistry,
    owner,
    permissions,
    basePath = DEFAULT_BASE_PATH,
    features = {},
    releasePhase = 'ga',
    getFilesClient,
  },
  queryClient = casesQueryClient,
  onToggleFlyout,
}) => {
  const [state, dispatch] = useReducer(casesContextReducer, getInitialCasesContextState());

  const onDispatch = (args) => {
    if (args.type === CasesContextStoreActionsList.OPEN_CREATE_CASE_FLYOUT ) {
      onToggleFlyout({isFlyoutOpen: true})
    }

    onDispatch(args)
  };
  
  // rest of code
  
  };

semd · 2024-05-24T07:08:33Z

Hey @cnasikas yes, using the observable inside the context is not a conventional approach, I understand why you prefer the onToggleFlyout callback, but I don't think this is a very good approach either, to get what we need it would look like this:

  const [state, _dispatch] = useReducer(casesContextReducer, getInitialCasesContextState());

  const dispatch: CasesContextValueDispatch = useCallback(
    (args) => {
      _dispatch(args);
      if (
        args.type === CasesContextStoreActionsList.OPEN_CREATE_CASE_FLYOUT ||
        args.type === CasesContextStoreActionsList.OPEN_ADD_TO_CASE_MODAL
      ) {
        onToggleAddToCase({ isOpen: true });
      } else if (
        args.type === CasesContextStoreActionsList.CLOSE_CREATE_CASE_FLYOUT ||
        args.type === CasesContextStoreActionsList.CLOSE_ADD_TO_CASE_MODAL
      ) {
        onToggleAddToCase({ isOpen: false });
      }
    },
    [onToggleAddToCase]
  );

  // add `dispatch` function to the context

This is a very specific implementation for what we need right now, it'd need to be extracted to a new "side-effects" module to make it extensible, and the onToggleAddToCase prop could cause context re-renders if it gets updated from the consumer. Also, from our side, it's much harder to use, we'd need to keep this value in a new state (isAddToCaseOpen) at the CasesContext level (applications top level), and somehow make it available to be read from the GuidedTour component, which would have some other complexities.

As an alternative, I have a third proposal:
Leave the current context alone and create a separate context to store only the state so we don't interfere with the Cases components that currently use the existing CasesContext for static information. The new state context will only be used by the new useIsAddToCaseOpen hook that is exposed from the cases plugin (more hooks like this one can be added later), so only the components that use this hook (our security GuidedTour components) will be re-rendered when the state changes. I made the changes here: 96f5924

This approach is explained in the react documentation docs. I think this is cleaner than the other 2 options, more reusable, does not mess up with the current context, and is very easy to use for us.
What do you think?

cnasikas · 2024-05-24T07:18:55Z

@semd I really like the new proposal. I knew there was something better but I could not figure it out. Thanks! Does the order of context providers matter? Also should we add the new context provider here x-pack/plugins/cases/public/common/mock/test_providers.tsx? I think we should not but I am curious to here your opinion.

semd · 2024-05-24T07:29:30Z

@cnasikas

I really like the new proposal. I knew there was something better but I could not figure it out

Great! Yes, Angela and I have been discussing this code for quite a while, and the observable was my idea lol, but none of us were convinced about it. Creating a separate context for the state makes a lot more sense.

Does the order of context providers matter?

Nope. The only thing that matters is that the provider wraps the consumer at some level.

Also should we add the new context provider here x-pack/plugins/cases/public/common/mock/test_providers.tsx? I think we should not but I am curious to here your opinion.

Exactly, since the CasesProvider already renders the CasesStateContext, there's no need to do anything else.

cnasikas · 2024-05-24T07:37:15Z

@semd @angorayc Thank you both! I will review it again today.

cnasikas

Thank you for your willingness to help and find a new solution! LGMT!

x-pack/plugins/cases/public/components/cases_context/state/use_is_add_to_case_open.test.tsx

semd · 2024-05-24T12:56:00Z

@elasticmachine merge upstream

semd · 2024-05-27T12:19:17Z

@elasticmachine merge upstream

kibana-ci · 2024-05-27T13:37:38Z

💚 Build Succeeded

Buildkite Build
Commit: fc02346

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`cases`	753	755	+2
`securitySolution`	5483	5484	+1
total			+3

Async chunks

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

id	before	after	diff
`securitySolution`	15.1MB	15.2MB	+113.7KB

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`	27	28	+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`	150.9KB	151.5KB	+586.0B
`securitySolution`	83.6KB	83.7KB	+55.0B
total			+641.0B

History

💛 Build #212021 was flaky 1e6f738
💔 Build #211948 failed c80bdc8
💔 Build #211905 failed a5e9085
💚 Build #211499 succeeded 39e1468
💚 Build #211425 succeeded 690f59b
💔 Build #211176 failed 2534c07

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

angorayc added 2 commits April 23, 2024 14:58

Retrieve onboarding steps

6d4b37e

compatible with legacy flyout

7366052

angorayc mentioned this pull request Apr 24, 2024

[Security solution] Explore cypress, fix more flakes #173539

Closed

angorayc added 2 commits April 30, 2024 15:17

Merge branch 'main' of github.com:elastic/kibana into issues/181190

f2afbaa

hide onboarding steps when cases opened

6e15bde

angorayc commented May 1, 2024

View reviewed changes

remove debugger

91e636a

angorayc commented May 1, 2024

View reviewed changes

hide all tours when irrelevant flyout open

5675be7

angorayc commented May 2, 2024

View reviewed changes

angorayc added 2 commits May 3, 2024 14:36

open correlation tab after case submission

2d503bf

fix types

818ffa2

Merge branch 'main' of github.com:elastic/kibana into issues/181190

da45950

angorayc changed the title ~~Retrieve onboarding steps~~ [SecuritySolution] Retrieve onboarding steps May 7, 2024

angorayc added 2 commits May 7, 2024 12:18

unit tests

2b1ad24

unit tests

31d997b

fix unit tests

c6c8a8d

prevent cells without anchor to update

2534c07

angorayc requested a review from PhilippeOberti May 21, 2024 10:23

type failure

690f59b

Merge branch 'main' into issues/181190

39e1468

kqualters-elastic approved these changes May 22, 2024

View reviewed changes

PhilippeOberti approved these changes May 22, 2024

View reviewed changes

cnasikas requested changes May 23, 2024

View reviewed changes

use separate cases state context

96f5924

remove type not used

e3433d9

clean tests

a5e9085

cnasikas approved these changes May 24, 2024

View reviewed changes

semd added 2 commits May 24, 2024 11:36

improve test

5337e47

Merge remote-tracking branch 'upstream/main' into issues/181190

c80bdc8

Merge branch 'main' into issues/181190

1e6f738

Merge branch 'main' into issues/181190

fc02346

angorayc merged commit 5d0f09a into elastic:main May 28, 2024

kibanamachine added the backport:skip This PR does not require backporting label May 28, 2024

christineweng mentioned this pull request May 30, 2024

[Security Solution][Expandable flyout] - replace advanced settings with feature flag #184169

Merged

2 tasks

Conversation

angorayc commented Apr 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Steps to Verify:

It compatible with the new expandable flyout:

It compatible with the old flyout:

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

angorayc commented May 2, 2024

Uh oh!

angorayc commented May 7, 2024

Uh oh!

angorayc commented May 7, 2024

Uh oh!

angorayc commented May 7, 2024

Uh oh!

semd commented May 21, 2024

Uh oh!

semd commented May 21, 2024

Uh oh!

angorayc commented May 22, 2024

Uh oh!

kqualters-elastic left a comment

Choose a reason for hiding this comment

Uh oh!

cnasikas left a comment

Choose a reason for hiding this comment

Uh oh!

semd commented May 24, 2024

Uh oh!

cnasikas commented May 24, 2024

Uh oh!

semd commented May 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cnasikas commented May 24, 2024

Uh oh!

cnasikas left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

semd commented May 24, 2024

Uh oh!

semd commented May 27, 2024

Uh oh!

kibana-ci commented May 27, 2024

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

Public APIs missing exports

Page load bundle

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

angorayc commented Apr 23, 2024 •

edited

Loading

semd commented May 24, 2024 •

edited

Loading

cnasikas left a comment •

edited

Loading