diff --git a/client/landing/stepper/declarative-flow/internals/analytics/record-step-complete.ts b/client/landing/stepper/declarative-flow/internals/analytics/record-step-complete.ts index 1a09977b9ce45..0df86153eb723 100644 --- a/client/landing/stepper/declarative-flow/internals/analytics/record-step-complete.ts +++ b/client/landing/stepper/declarative-flow/internals/analytics/record-step-complete.ts @@ -5,7 +5,7 @@ import { STEPPER_TRACKS_EVENT_STEP_COMPLETE } from 'calypso/landing/stepper/cons export interface RecordStepCompleteProps { flow: string; step: string; - optionalProps?: Record< string, string | number | null >; + optionalProps?: Record< string, string | number | null | boolean >; } const recordStepComplete = ( { flow, step, optionalProps }: RecordStepCompleteProps ) => { diff --git a/client/landing/stepper/declarative-flow/internals/components/step-route/hooks/use-step-route-tracking/index.tsx b/client/landing/stepper/declarative-flow/internals/components/step-route/hooks/use-step-route-tracking/index.tsx index 91571b0bf6fa4..de6df5e2fd78e 100644 --- a/client/landing/stepper/declarative-flow/internals/components/step-route/hooks/use-step-route-tracking/index.tsx +++ b/client/landing/stepper/declarative-flow/internals/components/step-route/hooks/use-step-route-tracking/index.tsx @@ -1,6 +1,5 @@ //* This hook is used to track the step route in the declarative flow. -import { SiteDetails } from '@automattic/data-stores'; import { isAnyHostingFlow } from '@automattic/onboarding'; import { useEffect, useRef } from '@wordpress/element'; import { getStepOldSlug } from 'calypso/landing/stepper/declarative-flow/helpers/get-step-old-slug'; @@ -21,18 +20,25 @@ import { import { useSelector } from 'calypso/state'; import { isRequestingSite } from 'calypso/state/sites/selectors'; -// Ensure that the selected site is fetched, if available. This is used for event tracking purposes. -// See https://github.com/Automattic/wp-calypso/pull/82981. -const useIsRequestingSelectedSite = ( siteSlugOrId: string | number, site: SiteDetails | null ) => { - return useSelector( ( state ) => site && isRequestingSite( state, siteSlugOrId ) ); +/** + * We wait for the site to be fetched before tracking the step route when a site ID/slug are defined in the params. + * This is to ensure that the site data is available for event tracking purposes. + * See `superProps`, `site_plan_id`, and https://github.com/Automattic/wp-calypso/pull/82981. + */ +const useHasRequestedSelectedSite = () => { + const { site, siteSlugOrId } = useSiteData(); + const isRequestingSelectedSite = useSelector( + ( state ) => site && isRequestingSite( state, siteSlugOrId ) + ); + + return siteSlugOrId ? !! site && ! isRequestingSelectedSite : true; }; interface Props { flowName: string; stepSlug: string; - // If true, the tracking event will not be recorded - skipTracking: boolean; flowVariantSlug?: string; + skipStepRender?: boolean; } /** @@ -41,15 +47,14 @@ interface Props { export const useStepRouteTracking = ( { flowName, stepSlug, - skipTracking, flowVariantSlug, + skipStepRender, }: Props ) => { const intent = useIntent(); const design = useSelectedDesign(); - const { site, siteSlugOrId } = useSiteData(); - const isRequestingSelectedSite = useIsRequestingSelectedSite( siteSlugOrId, site ); - const hasRequestedSelectedSite = siteSlugOrId ? !! site && ! isRequestingSelectedSite : true; + const hasRequestedSelectedSite = useHasRequestedSelectedSite(); const stepCompleteEventPropsRef = useRef< RecordStepCompleteProps | null >( null ); + const pathname = window.location.pathname; /** * Cleanup effect to record step-complete event when `StepRoute` unmounts. @@ -66,8 +71,8 @@ export const useStepRouteTracking = ( { }, [] ); useEffect( () => { - // We record the event only when the step is not empty. Additionally, we should not fire this event whenever the intent is changed - if ( ! hasRequestedSelectedSite || skipTracking ) { + // We wait for the site to be fetched before tracking the step route. + if ( ! hasRequestedSelectedSite ) { return; } @@ -83,13 +88,14 @@ export const useStepRouteTracking = ( { is_in_hosting_flow: isAnyHostingFlow( flowName ), ...( design && { assembler_source: getAssemblerSource( design ) } ), ...( flowVariantSlug && { flow_variant: flowVariantSlug } ), + ...( skipStepRender && { skip_step_render: skipStepRender } ), } ); // Apply the props to record in the exit/step-complete event. We only record this if start event gets recorded. stepCompleteEventPropsRef.current = { flow: flowName, step: stepSlug, - optionalProps: { intent }, + optionalProps: { intent, ...( skipStepRender && { skip_step_render: skipStepRender } ) }, }; const stepOldSlug = getStepOldSlug( stepSlug ); @@ -100,20 +106,23 @@ export const useStepRouteTracking = ( { is_in_hosting_flow: isAnyHostingFlow( flowName ), ...( design && { assembler_source: getAssemblerSource( design ) } ), ...( flowVariantSlug && { flow_variant: flowVariantSlug } ), + ...( skipStepRender && { skip_step_render: skipStepRender } ), } ); } } // Also record page view for data and analytics - const pathname = window.location.pathname; const pageTitle = `Setup > ${ flowName } > ${ stepSlug }`; const params = { flow: flowName, + ...( skipStepRender && { skip_step_render: skipStepRender } ), }; recordPageView( pathname, pageTitle, params ); // We leave out intent and design from the dependency list, due to the ONBOARD_STORE being reset in the exit flow. // The store reset causes these values to become empty, and may trigger this event again. + // We also leave out pathname. The respective event (calypso_page_view) is recorded behind a timeout and we don't want to trigger it again. + // - window.location.pathname called inside the effect keeps referring to the previous path on a redirect. So we moved it outside. // eslint-disable-next-line react-hooks/exhaustive-deps - }, [ flowName, hasRequestedSelectedSite, stepSlug, skipTracking ] ); + }, [ flowName, hasRequestedSelectedSite, stepSlug, skipStepRender ] ); }; diff --git a/client/landing/stepper/declarative-flow/internals/components/step-route/index.tsx b/client/landing/stepper/declarative-flow/internals/components/step-route/index.tsx index 958b481476a0b..38f8302d9b651 100644 --- a/client/landing/stepper/declarative-flow/internals/components/step-route/index.tsx +++ b/client/landing/stepper/declarative-flow/internals/components/step-route/index.tsx @@ -25,15 +25,14 @@ const StepRoute = ( { step, flow, showWooLogo, renderStep, navigate }: StepRoute const loginUrl = useLoginUrlForFlow( { flow } ); const shouldAuthUser = step.requiresLoggedInUser && ! userIsLoggedIn; const shouldSkipRender = shouldAuthUser || ! stepContent; - const skipTracking = shouldAuthUser || ! stepContent; const useBuiltItInAuth = flow.__experimentalUseBuiltinAuth; useStepRouteTracking( { flowName: flow.name, stepSlug: step.slug, - skipTracking, flowVariantSlug: flow.variantSlug, + skipStepRender: shouldSkipRender, } ); useEffect( () => { diff --git a/client/landing/stepper/declarative-flow/internals/components/step-route/test/index.tsx b/client/landing/stepper/declarative-flow/internals/components/step-route/test/index.tsx index d6ad94515fbb2..354c80dbed7ab 100644 --- a/client/landing/stepper/declarative-flow/internals/components/step-route/test/index.tsx +++ b/client/landing/stepper/declarative-flow/internals/components/step-route/test/index.tsx @@ -152,7 +152,7 @@ describe( 'StepRoute', () => { } ); describe( 'tracking', () => { - it( 'records a page view when the step is rendered', async () => { + it( 'records a page view', async () => { render( { step: regularStep } ); expect( recordPageView ).toHaveBeenCalledWith( '/', 'Setup > some-flow > some-step-slug', { @@ -160,7 +160,7 @@ describe( 'StepRoute', () => { } ); } ); - it( 'records recordStepStart when the step is rendered', async () => { + it( 'records recordStepStart', async () => { render( { step: regularStep } ); expect( recordStepStart ).toHaveBeenCalledWith( 'some-flow', 'some-step-slug', { @@ -170,14 +170,6 @@ describe( 'StepRoute', () => { } ); } ); - it( 'does not record start and page view when the login is required and the user is not logged in', async () => { - ( isUserLoggedIn as jest.Mock ).mockReturnValue( false ); - render( { step: requiresLoginStep } ); - - expect( recordStepStart ).not.toHaveBeenCalled(); - expect( recordPageView ).not.toHaveBeenCalled(); - } ); - it( 'skips tracking when the step is re-entered', () => { ( getSignupCompleteFlowNameAndClear as jest.Mock ).mockReturnValue( 'some-flow' ); ( getSignupCompleteStepNameAndClear as jest.Mock ).mockReturnValue( 'some-step-slug' ); @@ -187,15 +179,7 @@ describe( 'StepRoute', () => { expect( recordStepStart ).not.toHaveBeenCalled(); } ); - it( 'skips trackings when the renderStep returns null', () => { - render( { step: regularStep, renderStep: () => null } ); - - expect( recordStepStart ).not.toHaveBeenCalled(); - expect( recordPageView ).not.toHaveBeenCalled(); - } ); - - it( 'tracks step-complete when the step is unmounted and step-start was previously recorded', () => { - ( isUserLoggedIn as jest.Mock ).mockReturnValue( true ); + it( 'records step-complete when the step is unmounted and step-start was previously recorded', () => { ( getSignupCompleteFlowNameAndClear as jest.Mock ).mockReturnValue( 'some-other-flow' ); ( getSignupCompleteStepNameAndClear as jest.Mock ).mockReturnValue( 'some-other-step-slug' ); const { unmount } = render( { step: regularStep } ); @@ -211,7 +195,7 @@ describe( 'StepRoute', () => { } ); } ); - it( 'skips tracking step-complete when the step is unmounted and step-start was not recorded', () => { + it( 'skips recording step-complete when the step is unmounted and step-start was not recorded', () => { ( getSignupCompleteFlowNameAndClear as jest.Mock ).mockReturnValue( 'some-flow' ); ( getSignupCompleteStepNameAndClear as jest.Mock ).mockReturnValue( 'some-step-slug' ); const { unmount } = render( { step: regularStep } ); @@ -220,5 +204,63 @@ describe( 'StepRoute', () => { unmount(); expect( recordStepComplete ).not.toHaveBeenCalled(); } ); + + it( 'records skip_step_render on start, complete and page view when the login is required and the user is not logged in', async () => { + ( isUserLoggedIn as jest.Mock ).mockReturnValue( false ); + ( getSignupCompleteFlowNameAndClear as jest.Mock ).mockReturnValue( 'some-other-flow' ); + ( getSignupCompleteStepNameAndClear as jest.Mock ).mockReturnValue( 'some-other-step-slug' ); + + const { unmount } = render( { step: requiresLoginStep } ); + + expect( recordStepStart ).toHaveBeenCalledWith( 'some-flow', 'some-step-slug', { + intent: 'build', + assembler_source: 'premium', + is_in_hosting_flow: false, + skip_step_render: true, + } ); + expect( recordPageView ).toHaveBeenCalledWith( '/', 'Setup > some-flow > some-step-slug', { + flow: 'some-flow', + skip_step_render: true, + } ); + + unmount(); + + expect( recordStepComplete ).toHaveBeenCalledWith( { + step: 'some-step-slug', + flow: 'some-flow', + optionalProps: { + intent: 'build', + skip_step_render: true, + }, + } ); + } ); + + it( 'records skip_step_render on start, complete and page view when renderStep returns null', async () => { + ( getSignupCompleteFlowNameAndClear as jest.Mock ).mockReturnValue( 'some-other-flow' ); + ( getSignupCompleteStepNameAndClear as jest.Mock ).mockReturnValue( 'some-other-step-slug' ); + const { unmount } = render( { step: regularStep, renderStep: () => null } ); + + expect( recordStepStart ).toHaveBeenCalledWith( 'some-flow', 'some-step-slug', { + intent: 'build', + assembler_source: 'premium', + is_in_hosting_flow: false, + skip_step_render: true, + } ); + expect( recordPageView ).toHaveBeenCalledWith( '/', 'Setup > some-flow > some-step-slug', { + flow: 'some-flow', + skip_step_render: true, + } ); + + unmount(); + + expect( recordStepComplete ).toHaveBeenCalledWith( { + step: 'some-step-slug', + flow: 'some-flow', + optionalProps: { + intent: 'build', + skip_step_render: true, + }, + } ); + } ); } ); } );