Skip to content
Merged
35 changes: 24 additions & 11 deletions assets/js/components/setup/ModuleSetup.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ import { deleteItem } from '@/js/googlesitekit/api/cache';
import { trackEvent } from '@/js/util';
import { useFeature } from '@/js/hooks/useFeature';
import useQueryArg from '@/js/hooks/useQueryArg';
import useViewContext from '@/js/hooks/useViewContext';
import HelpMenu from '@/js/components/help/HelpMenu';
import { Cell, Grid, Row } from '@/js/material-components';
import Header from '@/js/components/Header';
Expand All @@ -49,10 +50,16 @@ import ExitSetup from '@/js/components/setup/ExitSetup';
import ProgressIndicator from '@/js/components/ProgressIndicator';

export default function ModuleSetup( { moduleSlug } ) {
const viewContext = useViewContext();
const { navigateTo } = useDispatch( CORE_LOCATION );
const setupFlowRefreshEnabled = useFeature( 'setupFlowRefresh' );
const [ showProgress ] = useQueryArg( 'showProgress' );

const isInitialSetupFlow =
setupFlowRefreshEnabled &&
moduleSlug === MODULE_SLUG_ANALYTICS_4 &&
showProgress === 'true';

const module = useSelect( ( select ) =>
select( CORE_MODULES ).getModule( moduleSlug )
);
Expand All @@ -71,11 +78,13 @@ export default function ModuleSetup( { moduleSlug } ) {
async ( redirectURL ) => {
await deleteItem( 'module_setup' );

await trackEvent(
'moduleSetup',
'complete_module_setup',
moduleSlug
);
const completionEventArgs = isInitialSetupFlow
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use a simple if/else here? It would be more readable in my opinion.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestion @abdelmalekkkkk.

I think we should stick to ternary operator here because of following reasons:

  1. This is fairly simple expression.
  2. If we use if...else statement we may have to either call trackEvent twice or need to use let completionEventArgs which is susceptible to change which is a source of potential side-effect bugs.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @ankitrox, thanks for explaining your rationale here, however, I would agree with @abdelmalekkkkk that it would be more readable to use an if/else. It won't result in calling trackEvent() twice, simply writing it twice, which seems fine when considering the improved legibility:

			if ( isInitialSetupFlow ) {
				await trackEvent(
					`${ viewContext }_setup`,
					'setup_flow_v3_complete_analytics_step'
				);
			} else {
				await trackEvent(
					'moduleSetup',
					'complete_module_setup',
					moduleSlug
				);
			}

? [
`${ viewContext }_setup`,
'setup_flow_v3_complete_analytics_step',
]
: [ 'moduleSetup', 'complete_module_setup', moduleSlug ];
await trackEvent( ...completionEventArgs );

if ( redirectURL ) {
navigateTo( redirectURL );
Expand All @@ -93,7 +102,7 @@ export default function ModuleSetup( { moduleSlug } ) {
);
navigateTo( adminURL );
},
[ registry, navigateTo, moduleSlug ]
[ registry, navigateTo, moduleSlug, viewContext, isInitialSetupFlow ]
);

const onCompleteSetup = module?.onCompleteSetup;
Expand All @@ -107,6 +116,15 @@ export default function ModuleSetup( { moduleSlug } ) {
}, [ moduleSlug ] );

useMount( () => {
if ( isInitialSetupFlow ) {
trackEvent(
`${ viewContext }_setup`,
'setup_flow_v3_view_analytics_step'
);

return;
}

trackEvent( 'moduleSetup', 'view_module_setup', moduleSlug );
} );

Expand All @@ -116,11 +134,6 @@ export default function ModuleSetup( { moduleSlug } ) {

const { SetupComponent } = module;

const isInitialSetupFlow =
setupFlowRefreshEnabled &&
moduleSlug === MODULE_SLUG_ANALYTICS_4 &&
showProgress === 'true';

return (
<Fragment>
<Header
Expand Down
33 changes: 33 additions & 0 deletions assets/js/components/setup/ModuleSetup.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,13 +36,18 @@ import { VIEW_CONTEXT_MODULE_SETUP } from '@/js/googlesitekit/constants';
import { mockLocation } from '../../../../tests/js/mock-browser-utils';
import { CORE_USER } from '@/js/googlesitekit/datastore/user/constants';
import { CORE_MODULES } from '@/js/googlesitekit/modules/datastore/constants';
import * as tracking from '@/js/util/tracking';

const mockTrackEvent = jest.spyOn( tracking, 'trackEvent' );
mockTrackEvent.mockImplementation( () => Promise.resolve() );

describe( 'ModuleSetup', () => {
mockLocation();

let registry;

beforeEach( () => {
mockTrackEvent.mockClear();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As per my other comment, please move this line to an afterEach() callback.

registry = createTestRegistry();
registry.dispatch( CORE_USER ).receiveGetDismissedItems( [] );
registry.dispatch( CORE_USER ).receiveGetDismissedPrompts( {} );
Expand Down Expand Up @@ -175,6 +180,20 @@ describe( 'ModuleSetup', () => {
it( 'should match the snapshot', () => {
expect( container ).toMatchSnapshot();
} );

it( 'tracks only the initial setup analytics view event', () => {
expect( mockTrackEvent ).toHaveBeenCalledWith(
`${ VIEW_CONTEXT_MODULE_SETUP }_setup`,
'setup_flow_v3_view_analytics_step'
);
const genericViewCall = mockTrackEvent.mock.calls.find(
( call ) =>
call[ 0 ] === 'moduleSetup' &&
call[ 1 ] === 'view_module_setup' &&
call[ 2 ] === MODULE_SLUG_ANALYTICS_4
);
expect( genericViewCall ).toBeUndefined();
} );
} );

describe( 'not initial setup flow', () => {
Expand Down Expand Up @@ -230,6 +249,20 @@ describe( 'ModuleSetup', () => {
it( 'should match the snapshot', () => {
expect( container ).toMatchSnapshot();
} );

it( 'tracks only the generic module setup view event', () => {
expect( mockTrackEvent ).toHaveBeenCalledWith(
'moduleSetup',
'view_module_setup',
MODULE_SLUG_ANALYTICS_4
);
const initialAnalyticsCall = mockTrackEvent.mock.calls.find(
( call ) =>
call[ 0 ] === `${ VIEW_CONTEXT_MODULE_SETUP }_setup` &&
call[ 1 ] === 'setup_flow_v3_view_analytics_step'
);
expect( initialAnalyticsCall ).toBeUndefined();
} );
} );
} );

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -621,7 +621,7 @@ exports[`ModuleSetup Analytics 4 not initial setup flow should match the snapsho
>
<label
class="mdc-floating-label mdc-floating-label--float-above"
for="googlesitekit-select-33"
for="googlesitekit-select-36"
style=""
>
Account
Expand Down Expand Up @@ -663,7 +663,7 @@ exports[`ModuleSetup Analytics 4 not initial setup flow should match the snapsho
>
<label
class="mdc-floating-label mdc-floating-label--float-above"
for="googlesitekit-select-34"
for="googlesitekit-select-37"
style=""
>
Property
Expand Down Expand Up @@ -705,7 +705,7 @@ exports[`ModuleSetup Analytics 4 not initial setup flow should match the snapsho
>
<label
class="mdc-floating-label mdc-floating-label--float-above"
for="googlesitekit-select-35"
for="googlesitekit-select-38"
style=""
>
Web data stream
Expand Down Expand Up @@ -741,7 +741,7 @@ exports[`ModuleSetup Analytics 4 not initial setup flow should match the snapsho
<input
checked=""
class="mdc-switch__native-control"
id="googlesitekit-switch-11"
id="googlesitekit-switch-12"
readonly=""
role="switch"
type="checkbox"
Expand All @@ -751,7 +751,7 @@ exports[`ModuleSetup Analytics 4 not initial setup flow should match the snapsho
</div>
<label
class=""
for="googlesitekit-switch-11"
for="googlesitekit-switch-12"
>
Enable enhanced measurement
</label>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,7 @@ export default function AccountCreate( { className } ) {
}, [ hasAccountCreateForm, siteName, siteURL, timezone, setValues ] );

const showProgress = getQueryArg( location.href, 'showProgress' );
const isInitialSetupFlow = !! showProgress && setupFlowRefreshEnabled;

const handleSubmit = useCallback( async () => {
const scopes = [];
Expand Down Expand Up @@ -192,11 +193,14 @@ export default function AccountCreate( { className } ) {
}

setValues( FORM_ACCOUNT_CREATE, { autoSubmit: false } );
await trackEvent(
`${ viewContext }_analytics`,
'create_account',
'proxy'
);

const createAccountEventArgs = isInitialSetupFlow
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My reply above also applies here, please do update this to use an if/else.

? [
`${ viewContext }_setup`,
'setup_flow_v3_create_analytics_account',
]
: [ `${ viewContext }_analytics`, 'create_account', 'proxy' ];
await trackEvent( ...createAccountEventArgs );

const { error } = await createAccount( {
showProgress: showProgress === 'true',
Expand All @@ -210,10 +214,11 @@ export default function AccountCreate( { className } ) {
hasEditScope,
hasGTMScope,
setValues,
viewContext,
isInitialSetupFlow,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason why you added isInitialSetupFlow and viewContext to the dependency array here but not in ModuleSetup.js? Doesn't seem like a big deal, but I'm wondering if we can keep things consistent.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good catch @abdelmalekkkkk , thanks for spotting it.

I've added it now.

createAccount,
showProgress,
setPermissionScopeError,
viewContext,
setConversionTrackingEnabled,
saveConversionTrackingSettings,
] );
Expand Down Expand Up @@ -242,8 +247,6 @@ export default function AccountCreate( { className } ) {
return <ProgressBar />;
}

const isInitialSetupFlow = !! showProgress && setupFlowRefreshEnabled;

return (
<div className={ className }>
<StoreErrorNotices
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
* Internal dependencies
*/
import {
cleanup,
createTestRegistry,
fireEvent,
muteFetch,
Expand All @@ -42,9 +43,14 @@ import { MODULE_SLUG_ANALYTICS_4 } from '@/js/modules/analytics-4/constants';
import { createCacheKey } from '@/js/googlesitekit/api';
import { getKeys, setItem } from '@/js/googlesitekit/api/cache';
import AccountCreate from '.';
import * as tracking from '@/js/util/tracking';
import { VIEW_CONTEXT_MODULE_SETUP } from '@/js/googlesitekit/constants';
import { CORE_SITE } from '@/js/googlesitekit/datastore/site/constants';
import { MODULE_SLUG_SEARCH_CONSOLE } from '@/js/modules/search-console/constants';

const mockTrackEvent = jest.spyOn( tracking, 'trackEvent' );
mockTrackEvent.mockImplementation( () => Promise.resolve() );

const REGEX_REST_CONVERSION_TRACKING_SETTINGS = new RegExp(
'^/google-site-kit/v1/core/site/data/conversion-tracking'
);
Expand Down Expand Up @@ -312,5 +318,77 @@ describe( 'AccountCreate', () => {
REGEX_REST_CONVERSION_TRACKING_SETTINGS
);
} );

describe( 'event tracking', () => {
beforeEach( () => {
mockTrackEvent.mockClear();
cleanup();
} );

it( 'should track create account event during initial setup flow', async () => {
global.location.href =
'http://example.com/wp-admin/admin.php?page=googlesitekit-dashboard&slug=analytics-4&reAuth=true&showProgress=true';

( { getByRole, waitForRegistry, rerender } = render(
<AccountCreate />,
{
registry,
viewContext: VIEW_CONTEXT_MODULE_SETUP,
features: [ 'setupFlowRefresh' ],
}
) );

fireEvent.click(
getByRole( 'button', { name: 'Create Account' } )
);

await waitForRegistry();

expect( mockTrackEvent ).toHaveBeenCalledWith(
`${ VIEW_CONTEXT_MODULE_SETUP }_setup`,
'setup_flow_v3_create_analytics_account'
);

const genericCreateAccountCall = mockTrackEvent.mock.calls.find(
( call ) =>
call[ 0 ]?.endsWith( '_analytics' ) &&
call[ 1 ] === 'create_account'
);
expect( genericCreateAccountCall ).toBeUndefined();
} );

it( 'should track generic create account event during non-initial setup flow', async () => {
global.location.href =
'http://example.com/wp-admin/admin.php?page=googlesitekit-dashboard&slug=analytics-4&reAuth=true';
( { getByRole, waitForRegistry, rerender } = render(
<AccountCreate />,
{
registry,
viewContext: VIEW_CONTEXT_MODULE_SETUP,
}
) );

fireEvent.click(
getByRole( 'button', { name: 'Create Account' } )
);

await waitForRegistry();

const genericCreateAccountCall = mockTrackEvent.mock.calls.find(
( call ) =>
call[ 0 ]?.endsWith( '_analytics' ) &&
call[ 1 ] === 'create_account' &&
call[ 2 ] === 'proxy'
);
expect( genericCreateAccountCall ).toBeDefined();

const initialSetupCall = mockTrackEvent.mock.calls.find(
( call ) =>
call[ 0 ] === `${ VIEW_CONTEXT_MODULE_SETUP }_setup` &&
call[ 1 ] === 'setup_flow_v3_create_analytics_account'
);
expect( initialSetupCall ).toBeUndefined();
} );
} );
} );
} );
Loading