[Security solution] Guided onboarding unhappy path fixes by stephmilovic · Pull Request #144178 · elastic/kibana

stephmilovic · 2022-10-28T16:05:44Z

Summary

Follow up to #143598
Fixes unhappy path behavior on the SecurityStepId.alertsCases tour. Details on the fixes are in the code!

To test

Add to your dev yml: xpack.cloud.id: test (this is new since the original PR)
Follow instructions from original PR #143598

Known issues left unresolved

If you have a flyout guide step open, close the flyout, open the Setup guide menu, and click "Continue", the user needs to manually open the flyout to get back to the step. Here is an issue to add a callback that will allow us to automate this click. FWIW, this only happens if the user is on the alerts page. If they go to an unrelated page (non-detections), open the Setup guide menu, and click "Continue", they are redirected to the Alerts page and reset to step 1.

stephmilovic · 2022-10-28T16:16:05Z

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

+const StyledTourStep = styled(EuiTourStep)<EuiTourStepProps & { stepId: SecurityStepId }>`
+  &.euiPopover__panel[data-popover-open] {
+    z-index: ${({ step, stepId }) =>
+      isStepExternallyMounted(stepId, step) ? '9000 !important' : '1000 !important'};


I needed to lower the z-index to prevent bugs like this:

But it needs to be high again for the cases step or else:

stephmilovic · 2022-10-28T16:17:16Z

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

  );
+
+  const getTimeline = useMemo(() => timelineSelectors.getTimelineByIdSelector(), []);
+  const showTimeline = useShallowEqualSelector(


stephmilovic · 2022-10-28T16:24:44Z

x-pack/plugins/security_solution/public/timelines/components/timeline/body/actions/index.tsx


  const onExpandEvent = useCallback(() => {
-    if (isTourAnchor) {
+    const isStep2Active = activeStep === 2 && isTourShown(SecurityStepId.alertsCases);


To prevent bugs like this, only allow incrementStep from this action if the alerts tour step is currently 2

use the enum here instead of 2 and maybe isExpandEventStepActive ?

stephmilovic · 2022-10-28T16:25:21Z

...ution/public/detections/components/alerts_table/timeline_actions/use_add_to_case_actions.tsx

      attachments: caseAttachments,
-      ...(isTourShown(SecurityStepId.alertsCases) && activeStep === 4
+      // activeStep will be 4 on first render because not yet incremented
+      // if the user closes the flyout without completing the form and comes back, we will be at step 5


Fixes this issue

stephmilovic · 2022-10-28T16:28:39Z

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

  activeStep: number;
  endTourStep: (stepId: SecurityStepId) => void;
-  incrementStep: (stepId: SecurityStepId, step?: number) => void;
+  incrementStep: (stepId: SecurityStepId) => void;


never used/implemented

Is it going to be used in the future or should we just remove it?

oh ignore me

stephmilovic · 2022-10-28T19:13:21Z

...plugins/security_solution/public/common/components/guided_onboarding_tour/tour_step.test.tsx

 import { GuidedOnboardingTourStep, SecurityTourStep } from './tour_step';
 import { SecurityStepId } from './tour_config';
 import { useTourContext } from './tour';
+import { mockGlobalState, SUB_PLUGINS_REDUCER, TestProviders } from '../../mock';


need this now since we have timeline state in the component

elasticmachine · 2022-10-28T19:26:45Z

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

elasticmachine · 2022-10-28T19:26:47Z

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

semd · 2022-10-31T12:36:00Z

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

  return anchor != null ? (
    <>
-      <EuiTourStep {...commonProps} anchor={anchor} />
+      <StyledTourStep stepId={stepId} {...commonProps} anchor={anchor} />


NIT: In this case where we have the anchor, do we need to render the TourStep if we don't have children?
I am asking because if we don't have to render anything if children is not passed, then we could create an exit case (if !children return null) much earlier in the component, and avoid checking it on every return, and this double ternary condition as well.

Yes, step 5 is only the tour step, no children passed

x-pack/plugins/security_solution/public/timelines/components/timeline/body/actions/index.tsx

semd

Tested locally, All the problems mentioned are solved.
The only change I would do is to use constants in the components for the steps, instead of the sequence number, as discussed. But besides that, it LGTM.

michaelolo24 · 2022-11-01T14:55:35Z

A minor ux thing: just pulling down and testing it looks like the FlyoutHeader width is smaller when step 3 is visible? It takes up the full space once you move to the next step

michaelolo24

Tested locally, just one UX change for the flyout header, but everything else looks good...just some nits. Thanks for doing this work!

stephmilovic · 2022-11-01T15:16:10Z

A minor ux thing: just pulling down and testing it looks like the FlyoutHeader width is smaller when step 3 is visible? It takes up the full space once you move to the next step

@michaelolo24 I have a PR with design fixes coming up next so I will clear this problem up there. thanks!

kibana-ci · 2022-11-01T15:49:28Z

💚 Build Succeeded

Buildkite Build
Commit: 8910e9d

Metrics [docs]

Async chunks

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

id	before	after	diff
`securitySolution`	9.5MB	9.5MB	+3.5KB

Unknown metric groups

ESLint disabled in files

id	before	after	diff
`osquery`	1	2	+1

ESLint disabled line counts

id	before	after	diff
`enterpriseSearch`	19	21	+2
`fleet`	57	63	+6
`osquery`	103	108	+5
`securitySolution`	439	443	+4
total			+17

Total ESLint disabled count

id	before	after	diff
`enterpriseSearch`	20	22	+2
`fleet`	65	71	+6
`osquery`	104	110	+6
`securitySolution`	516	520	+4
total			+18

History

💚 Build #83983 succeeded d6d62fa
💚 Build #83660 succeeded 0f4816e
💔 Build #83613 failed 0c0720a

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

stephmilovic added 4 commits October 27, 2022 15:33

stolen commit

9336692

fixes

cd68352

Merge branch 'main' into guided_onboarding_unhappy

1b05177

more

2e0e15f

stephmilovic commented Oct 28, 2022

View reviewed changes

revert

0c0720a

stephmilovic commented Oct 28, 2022

View reviewed changes

stephmilovic added 2 commits October 28, 2022 13:08

tests

c5203dd

un-only

86c9751

stephmilovic commented Oct 28, 2022

View reviewed changes

un-ignore

0f4816e

stephmilovic added release_note:skip Skip the PR/issue when compiling release notes Team:Threat Hunting Security Solution Threat Hunting Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Threat Hunting:Explore v8.6.0 labels Oct 28, 2022

stephmilovic changed the title ~~Guided onboarding unhappy~~ [Security solution] Guided onboarding unhappy path fixes Oct 28, 2022

stephmilovic marked this pull request as ready for review October 28, 2022 19:26

stephmilovic requested review from a team as code owners October 28, 2022 19:26

semd reviewed Oct 31, 2022

View reviewed changes

x-pack/plugins/security_solution/public/timelines/components/timeline/body/actions/index.tsx Show resolved Hide resolved

Merge branch 'main' into guided_onboarding_unhappy

d6d62fa

semd approved these changes Oct 31, 2022

View reviewed changes

Merge branch 'main' into guided_onboarding_unhappy

d8ace54

make step enum AlertsCasesTourSteps

8910e9d

michaelolo24 approved these changes Nov 1, 2022

View reviewed changes

stephmilovic merged commit 8969009 into elastic:main Nov 1, 2022

kibanamachine added the backport:skip This PR does not require backporting label Nov 1, 2022

stephmilovic deleted the guided_onboarding_unhappy branch November 1, 2022 15:59

Conversation

stephmilovic commented Oct 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

To test

Known issues left unresolved

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!

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Oct 28, 2022

Uh oh!

elasticmachine commented Oct 28, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

semd left a comment

Choose a reason for hiding this comment

Uh oh!

michaelolo24 commented Nov 1, 2022

Uh oh!

michaelolo24 left a comment

Choose a reason for hiding this comment

Uh oh!

stephmilovic commented Nov 1, 2022

Uh oh!

kibana-ci commented Nov 1, 2022

💚 Build Succeeded

Metrics [docs]

Async chunks

ESLint disabled in files

ESLint disabled line counts

Total ESLint disabled count

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

stephmilovic commented Oct 28, 2022 •

edited

Loading