-
Notifications
You must be signed in to change notification settings - Fork 8.1k
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] Guided onboarding unhappy path fixes #144178
Changes from 9 commits
9336692
cd68352
1b05177
2e0e15f
0c0720a
c5203dd
86c9751
0f4816e
d6d62fa
d8ace54
8910e9d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,6 +9,12 @@ import { render } from '@testing-library/react'; | |
import { GuidedOnboardingTourStep, SecurityTourStep } from './tour_step'; | ||
import { SecurityStepId } from './tour_config'; | ||
import { useTourContext } from './tour'; | ||
import { mockGlobalState, SUB_PLUGINS_REDUCER, TestProviders } from '../../mock'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. need this now since we have timeline state in the component |
||
import { TimelineId } from '../../../../common/types'; | ||
import { createStore } from '../../store'; | ||
import { tGridReducer } from '@kbn/timelines-plugin/public'; | ||
import { kibanaObservable } from '@kbn/timelines-plugin/public/mock'; | ||
import { createSecuritySolutionStorageMock } from '@kbn/timelines-plugin/public/mock/mock_local_storage'; | ||
|
||
jest.mock('./tour'); | ||
const mockTourStep = jest | ||
|
@@ -43,7 +49,8 @@ describe('GuidedOnboardingTourStep', () => { | |
}); | ||
it('renders as a tour step', () => { | ||
const { getByTestId } = render( | ||
<GuidedOnboardingTourStep {...defaultProps}>{mockChildren}</GuidedOnboardingTourStep> | ||
<GuidedOnboardingTourStep {...defaultProps}>{mockChildren}</GuidedOnboardingTourStep>, | ||
{ wrapper: TestProviders } | ||
); | ||
const tourStep = getByTestId('tourStepMock'); | ||
const header = getByTestId('h1'); | ||
|
@@ -54,7 +61,8 @@ describe('GuidedOnboardingTourStep', () => { | |
const { getByTestId, queryByTestId } = render( | ||
<GuidedOnboardingTourStep {...defaultProps} isTourAnchor={false}> | ||
{mockChildren} | ||
</GuidedOnboardingTourStep> | ||
</GuidedOnboardingTourStep>, | ||
{ wrapper: TestProviders } | ||
); | ||
const tourStep = queryByTestId('tourStepMock'); | ||
const header = getByTestId('h1'); | ||
|
@@ -83,7 +91,8 @@ describe('SecurityTourStep', () => { | |
render( | ||
<SecurityTourStep {...securityTourStepDefaultProps} step={99}> | ||
{mockChildren} | ||
</SecurityTourStep> | ||
</SecurityTourStep>, | ||
{ wrapper: TestProviders } | ||
); | ||
expect(mockTourStep).not.toHaveBeenCalled(); | ||
}); | ||
|
@@ -92,7 +101,8 @@ describe('SecurityTourStep', () => { | |
render( | ||
<SecurityTourStep {...securityTourStepDefaultProps} step={4}> | ||
{mockChildren} | ||
</SecurityTourStep> | ||
</SecurityTourStep>, | ||
{ wrapper: TestProviders } | ||
); | ||
expect(mockTourStep).not.toHaveBeenCalled(); | ||
}); | ||
|
@@ -103,12 +113,16 @@ describe('SecurityTourStep', () => { | |
incrementStep: jest.fn(), | ||
isTourShown: () => false, | ||
}); | ||
render(<SecurityTourStep {...securityTourStepDefaultProps}>{mockChildren}</SecurityTourStep>); | ||
render(<SecurityTourStep {...securityTourStepDefaultProps}>{mockChildren}</SecurityTourStep>, { | ||
wrapper: TestProviders, | ||
}); | ||
expect(mockTourStep).not.toHaveBeenCalled(); | ||
}); | ||
|
||
it('renders tour step with correct number of steppers', () => { | ||
render(<SecurityTourStep {...securityTourStepDefaultProps}>{mockChildren}</SecurityTourStep>); | ||
render(<SecurityTourStep {...securityTourStepDefaultProps}>{mockChildren}</SecurityTourStep>, { | ||
wrapper: TestProviders, | ||
}); | ||
const mockCall = { ...mockTourStep.mock.calls[0][0] }; | ||
expect(mockCall.step).toEqual(1); | ||
expect(mockCall.stepsTotal).toEqual(5); | ||
|
@@ -118,7 +132,8 @@ describe('SecurityTourStep', () => { | |
render( | ||
<SecurityTourStep {...securityTourStepDefaultProps} step={5}> | ||
{mockChildren} | ||
</SecurityTourStep> | ||
</SecurityTourStep>, | ||
{ wrapper: TestProviders } | ||
); | ||
const mockCall = { ...mockTourStep.mock.calls[0][0] }; | ||
expect(mockCall.step).toEqual(5); | ||
|
@@ -134,7 +149,8 @@ describe('SecurityTourStep', () => { | |
render( | ||
<SecurityTourStep {...securityTourStepDefaultProps} step={3}> | ||
{mockChildren} | ||
</SecurityTourStep> | ||
</SecurityTourStep>, | ||
{ wrapper: TestProviders } | ||
); | ||
const mockCall = { ...mockTourStep.mock.calls[0][0] }; | ||
expect(mockCall.footerAction).toMatchInlineSnapshot(` | ||
|
@@ -163,7 +179,8 @@ describe('SecurityTourStep', () => { | |
const { container } = render( | ||
<SecurityTourStep {...securityTourStepDefaultProps} step={3}> | ||
{mockChildren} | ||
</SecurityTourStep> | ||
</SecurityTourStep>, | ||
{ wrapper: TestProviders } | ||
); | ||
const selectParent = container.querySelector( | ||
`[data-test-subj="tourStepMock"] [data-test-subj="h1"]` | ||
|
@@ -184,7 +201,8 @@ describe('SecurityTourStep', () => { | |
const { container } = render( | ||
<SecurityTourStep {...securityTourStepDefaultProps} step={2}> | ||
{mockChildren} | ||
</SecurityTourStep> | ||
</SecurityTourStep>, | ||
{ wrapper: TestProviders } | ||
); | ||
const selectParent = container.querySelector( | ||
`[data-test-subj="tourStepMock"] [data-test-subj="h1"]` | ||
|
@@ -197,13 +215,17 @@ describe('SecurityTourStep', () => { | |
}); | ||
|
||
it('if a tour step does not have children and has anchor, only render tour step', () => { | ||
const { getByTestId } = render(<SecurityTourStep {...securityTourStepDefaultProps} step={5} />); | ||
const { getByTestId } = render( | ||
<SecurityTourStep {...securityTourStepDefaultProps} step={5} />, | ||
{ wrapper: TestProviders } | ||
); | ||
expect(getByTestId('tourStepMock')).toBeInTheDocument(); | ||
}); | ||
|
||
it('if a tour step does not have children and does not have anchor, render nothing', () => { | ||
const { queryByTestId } = render( | ||
<SecurityTourStep {...securityTourStepDefaultProps} step={1} /> | ||
<SecurityTourStep {...securityTourStepDefaultProps} step={1} />, | ||
{ wrapper: TestProviders } | ||
); | ||
expect(queryByTestId('tourStepMock')).not.toBeInTheDocument(); | ||
}); | ||
|
@@ -217,9 +239,40 @@ describe('SecurityTourStep', () => { | |
render( | ||
<SecurityTourStep {...securityTourStepDefaultProps} step={4}> | ||
{mockChildren} | ||
</SecurityTourStep> | ||
</SecurityTourStep>, | ||
{ wrapper: TestProviders } | ||
); | ||
const mockCall = { ...mockTourStep.mock.calls[0][0] }; | ||
expect(mockCall.footerAction).toMatchInlineSnapshot(`<React.Fragment />`); | ||
}); | ||
|
||
it('does not render step if timeline is open', () => { | ||
const mockstate = { | ||
...mockGlobalState, | ||
timeline: { | ||
...mockGlobalState.timeline, | ||
timelineById: { | ||
[TimelineId.active]: { | ||
...mockGlobalState.timeline.timelineById.test, | ||
show: true, | ||
}, | ||
}, | ||
}, | ||
}; | ||
const { storage } = createSecuritySolutionStorageMock(); | ||
const mockStore = createStore( | ||
mockstate, | ||
SUB_PLUGINS_REDUCER, | ||
{ dataTable: tGridReducer }, | ||
kibanaObservable, | ||
storage | ||
); | ||
|
||
render( | ||
<TestProviders store={mockStore}> | ||
<SecurityTourStep {...securityTourStepDefaultProps}>{mockChildren}</SecurityTourStep> | ||
</TestProviders> | ||
); | ||
expect(mockTourStep).not.toHaveBeenCalled(); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,26 +11,54 @@ import type { EuiTourStepProps } from '@elastic/eui'; | |
import { EuiButton, EuiImage, EuiSpacer, EuiText, EuiTourStep } from '@elastic/eui'; | ||
import { FormattedMessage } from '@kbn/i18n-react'; | ||
|
||
import styled from 'styled-components'; | ||
import { useShallowEqualSelector } from '../../hooks/use_selector'; | ||
import { TimelineId } from '../../../../common/types'; | ||
import { timelineDefaults } from '../../../timelines/store/timeline/defaults'; | ||
import { timelineSelectors } from '../../../timelines/store/timeline'; | ||
import { useTourContext } from './tour'; | ||
import { securityTourConfig, SecurityStepId } from './tour_config'; | ||
import { SecurityStepId, securityTourConfig } from './tour_config'; | ||
|
||
interface SecurityTourStep { | ||
children?: React.ReactElement; | ||
step: number; | ||
stepId: SecurityStepId; | ||
} | ||
|
||
const isStepExternallyMounted = (stepId: SecurityStepId, step: number) => | ||
step === 5 && stepId === SecurityStepId.alertsCases; | ||
|
||
const StyledTourStep = styled(EuiTourStep)<EuiTourStepProps & { stepId: SecurityStepId }>` | ||
&.euiPopover__panel[data-popover-open] { | ||
z-index: ${({ step, stepId }) => | ||
isStepExternallyMounted(stepId, step) ? '9000 !important' : '1000 !important'}; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
} | ||
`; | ||
|
||
export const SecurityTourStep = ({ children, step, stepId }: SecurityTourStep) => { | ||
const { activeStep, incrementStep, isTourShown } = useTourContext(); | ||
const tourStep = useMemo( | ||
() => securityTourConfig[stepId].find((config) => config.step === step), | ||
[step, stepId] | ||
); | ||
|
||
const getTimeline = useMemo(() => timelineSelectors.getTimelineByIdSelector(), []); | ||
const showTimeline = useShallowEqualSelector( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
(state) => (getTimeline(state, TimelineId.active) ?? timelineDefaults).show | ||
); | ||
|
||
const onClick = useCallback(() => incrementStep(stepId), [incrementStep, stepId]); | ||
|
||
// step === 5 && stepId === SecurityStepId.alertsCases is in Cases app and out of context. | ||
// If we mount this step, we know we need to render it | ||
// we are also managing the context on the siem end in the background | ||
const overrideContext = step === 5 && stepId === SecurityStepId.alertsCases; | ||
if (tourStep == null || ((step !== activeStep || !isTourShown(stepId)) && !overrideContext)) { | ||
const overrideContext = isStepExternallyMounted(stepId, step); | ||
|
||
if ( | ||
tourStep == null || | ||
((step !== activeStep || !isTourShown(stepId)) && !overrideContext) || | ||
showTimeline | ||
) { | ||
return children ? children : null; | ||
} | ||
|
||
|
@@ -89,11 +117,13 @@ export const SecurityTourStep = ({ children, step, stepId }: SecurityTourStep) = | |
// see type EuiTourStepAnchorProps | ||
return anchor != null ? ( | ||
<> | ||
<EuiTourStep {...commonProps} anchor={anchor} /> | ||
<StyledTourStep stepId={stepId} {...commonProps} anchor={anchor} /> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. NIT: In this case where we have the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, step 5 is only the tour step, no children passed |
||
<>{children}</> | ||
</> | ||
) : children != null ? ( | ||
<EuiTourStep {...commonProps}>{children}</EuiTourStep> | ||
<StyledTourStep stepId={stepId} {...commonProps}> | ||
{children} | ||
</StyledTourStep> | ||
) : null; | ||
}; | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -80,7 +80,9 @@ export const useAddToCaseActions = ({ | |
onMenuItemClick(); | ||
createCaseFlyout.open({ | ||
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
...(isTourShown(SecurityStepId.alertsCases) && (activeStep === 4 || activeStep === 5) | ||
? { | ||
headerContent: ( | ||
// isTourAnchor=true no matter what in order to | ||
|
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.
never used/implemented
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.
Is it going to be used in the future or should we just remove it?
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.
oh ignore me