-
Notifications
You must be signed in to change notification settings - Fork 21
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
Adjust click event tracking when connecting, disconnecting, and opening billing setup for Google Ads account #2421
Changes from all commits
4a2bc0d
2ef11c7
9a997e2
130d843
0e6b33d
2b6a746
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 |
---|---|---|
|
@@ -18,15 +18,19 @@ import Subsection from '.~/wcdl/subsection'; | |
import useApiFetchCallback from '.~/hooks/useApiFetchCallback'; | ||
import useDispatchCoreNotices from '.~/hooks/useDispatchCoreNotices'; | ||
import useGoogleAdsAccount from '.~/hooks/useGoogleAdsAccount'; | ||
import useEventPropertiesFilter from '.~/hooks/useEventPropertiesFilter'; | ||
import AdsAccountSelectControl from './ads-account-select-control'; | ||
import { useAppDispatch } from '.~/data'; | ||
import { FILTER_ONBOARDING } from '.~/utils/tracks'; | ||
import './index.scss'; | ||
|
||
/** | ||
* Clicking on the button to connect an existing Google Ads account. | ||
* | ||
* @event gla_ads_account_connect_button_click | ||
* @property {number} id The account ID to be connected. | ||
* @property {string} [context] Indicates the place where the button is located. | ||
* @property {string} [step] Indicates the step in the onboarding process. | ||
*/ | ||
|
||
/** | ||
|
@@ -45,6 +49,7 @@ const ConnectAds = ( props ) => { | |
data: { id: value }, | ||
} ); | ||
const { refetchGoogleAdsAccount } = useGoogleAdsAccount(); | ||
const getEventProps = useEventPropertiesFilter( FILTER_ONBOARDING ); | ||
const { createNotice } = useDispatchCoreNotices(); | ||
const { fetchGoogleAdsAccountStatus } = useAppDispatch(); | ||
|
||
|
@@ -131,7 +136,9 @@ const ConnectAds = ( props ) => { | |
isSecondary | ||
disabled={ ! value } | ||
eventName="gla_ads_account_connect_button_click" | ||
eventProps={ { id: Number( value ) } } | ||
eventProps={ getEventProps( { | ||
id: Number( value ), | ||
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. Just noting down what I thought while reviewing. I was thinking that the name of the Ads ID property when disconnecting account is 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. The |
||
} ) } | ||
onClick={ handleConnectClick } | ||
> | ||
{ __( 'Connect', 'google-listings-and-ads' ) } | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,229 @@ | ||
/** | ||
* External dependencies | ||
*/ | ||
import '@testing-library/jest-dom'; | ||
import { screen, render, act } from '@testing-library/react'; | ||
import userEvent from '@testing-library/user-event'; | ||
import { recordEvent } from '@woocommerce/tracks'; | ||
|
||
/** | ||
* Internal dependencies | ||
*/ | ||
import ConnectAds from './'; | ||
import useApiFetchCallback from '.~/hooks/useApiFetchCallback'; | ||
import useGoogleAdsAccount from '.~/hooks/useGoogleAdsAccount'; | ||
import { useAppDispatch } from '.~/data'; | ||
import { FILTER_ONBOARDING } from '.~/utils/tracks'; | ||
import expectComponentToRecordEventWithFilteredProperties from '.~/tests/expectComponentToRecordEventWithFilteredProperties'; | ||
|
||
jest.mock( '.~/hooks/useApiFetchCallback', () => | ||
jest.fn().mockName( 'useApiFetchCallback' ) | ||
); | ||
|
||
jest.mock( '.~/hooks/useGoogleAdsAccount', () => | ||
jest.fn().mockName( 'useGoogleAdsAccount' ) | ||
); | ||
|
||
jest.mock( '.~/data', () => ( { | ||
...jest.requireActual( '.~/data' ), | ||
useAppDispatch: jest.fn(), | ||
} ) ); | ||
|
||
jest.mock( '@woocommerce/tracks', () => { | ||
return { | ||
recordEvent: jest.fn().mockName( 'recordEvent' ), | ||
}; | ||
} ); | ||
|
||
describe( 'ConnectAds', () => { | ||
const accounts = [ | ||
{ id: '1', name: 'Account A' }, | ||
{ id: '2', name: 'Account B' }, | ||
]; | ||
|
||
let fetchGoogleAdsAccountStatus; | ||
|
||
function getConnectButton() { | ||
return screen.getByRole( 'button', { name: 'Connect' } ); | ||
} | ||
|
||
function getCreateAccountButton() { | ||
return screen.getByRole( 'button', { | ||
name: 'Or, create a new Google Ads account', | ||
} ); | ||
} | ||
|
||
beforeEach( () => { | ||
useApiFetchCallback.mockReturnValue( [ | ||
jest.fn().mockName( 'fetchConnectAdsAccount' ), | ||
] ); | ||
|
||
useGoogleAdsAccount.mockReturnValue( { | ||
refetchGoogleAdsAccount: jest | ||
.fn() | ||
.mockName( 'refetchGoogleAdsAccount' ), | ||
} ); | ||
|
||
fetchGoogleAdsAccountStatus = jest | ||
.fn() | ||
.mockName( 'fetchGoogleAdsAccountStatus' ); | ||
useAppDispatch.mockReturnValue( { fetchGoogleAdsAccountStatus } ); | ||
} ); | ||
|
||
afterEach( () => { | ||
recordEvent.mockClear(); | ||
} ); | ||
|
||
it( 'should render the given accounts in a selection', () => { | ||
render( <ConnectAds accounts={ accounts } /> ); | ||
|
||
expect( screen.getByRole( 'combobox' ) ).toBeInTheDocument(); | ||
expect( | ||
screen.getByRole( 'option', { name: 'Select one' } ) | ||
).toBeInTheDocument(); | ||
expect( | ||
screen.getByRole( 'option', { name: 'Account A (1)' } ) | ||
).toBeInTheDocument(); | ||
expect( | ||
screen.getByRole( 'option', { name: 'Account B (2)' } ) | ||
).toBeInTheDocument(); | ||
} ); | ||
|
||
it( 'should render a message when there are multiple existing accounts', () => { | ||
const message = | ||
'If you manage multiple sub-accounts in Google Ads, please connect the relevant sub-account, not a manager account.'; | ||
const { rerender } = render( | ||
<ConnectAds accounts={ [ accounts[ 0 ] ] } /> | ||
); | ||
|
||
expect( screen.queryByText( message ) ).not.toBeInTheDocument(); | ||
|
||
rerender( <ConnectAds accounts={ accounts } /> ); | ||
|
||
expect( screen.getByText( message ) ).toBeInTheDocument(); | ||
} ); | ||
|
||
it( 'should call back to onCreateNew when clicking the button to switch to account creation', async () => { | ||
const onCreateNew = jest.fn().mockName( 'onCreateNew' ); | ||
render( <ConnectAds accounts={ [] } onCreateNew={ onCreateNew } /> ); | ||
|
||
expect( onCreateNew ).toHaveBeenCalledTimes( 0 ); | ||
|
||
await userEvent.click( getCreateAccountButton() ); | ||
|
||
expect( onCreateNew ).toHaveBeenCalledTimes( 1 ); | ||
} ); | ||
|
||
it( 'should disable the "Connect" button when no account is selected', async () => { | ||
render( <ConnectAds accounts={ accounts } /> ); | ||
const combobox = screen.getByRole( 'combobox' ); | ||
const button = getConnectButton(); | ||
|
||
expect( button ).toBeDisabled(); | ||
|
||
await userEvent.selectOptions( combobox, '1' ); | ||
|
||
expect( button ).toBeEnabled(); | ||
|
||
await userEvent.selectOptions( combobox, '' ); | ||
|
||
expect( button ).toBeDisabled(); | ||
} ); | ||
|
||
it( 'should render a connecting state and disable the button to switch to account creation after clicking the "Connect" button', async () => { | ||
let resolve; | ||
|
||
fetchGoogleAdsAccountStatus.mockReturnValue( | ||
new Promise( ( _resolve ) => { | ||
resolve = _resolve; | ||
} ) | ||
); | ||
|
||
render( <ConnectAds accounts={ accounts } /> ); | ||
|
||
expect( getCreateAccountButton() ).toBeEnabled(); | ||
|
||
await userEvent.selectOptions( screen.getByRole( 'combobox' ), '1' ); | ||
await userEvent.click( getConnectButton() ); | ||
|
||
expect( getCreateAccountButton() ).toBeDisabled(); | ||
expect( screen.getByText( 'Connecting…' ) ).toBeInTheDocument(); | ||
|
||
await act( async () => resolve() ); | ||
|
||
expect( getCreateAccountButton() ).toBeDisabled(); | ||
expect( screen.getByText( 'Connecting…' ) ).toBeInTheDocument(); | ||
} ); | ||
|
||
it( 'should resume to a state where it can retry the connection after a failed connection', async () => { | ||
let reject; | ||
|
||
fetchGoogleAdsAccountStatus.mockReturnValue( | ||
new Promise( ( _, _reject ) => { | ||
reject = _reject; | ||
} ) | ||
); | ||
|
||
render( <ConnectAds accounts={ accounts } /> ); | ||
|
||
await userEvent.selectOptions( screen.getByRole( 'combobox' ), '1' ); | ||
await act( async () => await userEvent.click( getConnectButton() ) ); | ||
|
||
expect( screen.getByText( 'Connecting…' ) ).toBeInTheDocument(); | ||
expect( getCreateAccountButton() ).toBeDisabled(); | ||
|
||
await act( async () => reject() ); | ||
|
||
expect( getConnectButton() ).toBeEnabled(); | ||
expect( getCreateAccountButton() ).toBeEnabled(); | ||
} ); | ||
|
||
it( 'should record click events and the `id` event property for the "Connect" button', async () => { | ||
// Prevent the component from locking in the connecting state | ||
fetchGoogleAdsAccountStatus.mockRejectedValue(); | ||
render( <ConnectAds accounts={ accounts } /> ); | ||
const combobox = screen.getByRole( 'combobox' ); | ||
|
||
expect( recordEvent ).toHaveBeenCalledTimes( 0 ); | ||
|
||
await userEvent.selectOptions( combobox, '2' ); | ||
await act( async () => await userEvent.click( getConnectButton() ) ); | ||
|
||
expect( recordEvent ).toHaveBeenCalledTimes( 1 ); | ||
expect( recordEvent ).toHaveBeenNthCalledWith( | ||
1, | ||
'gla_ads_account_connect_button_click', | ||
{ id: 2 } | ||
); | ||
|
||
await userEvent.selectOptions( combobox, '1' ); | ||
await act( async () => await userEvent.click( getConnectButton() ) ); | ||
|
||
expect( recordEvent ).toHaveBeenCalledTimes( 2 ); | ||
expect( recordEvent ).toHaveBeenNthCalledWith( | ||
2, | ||
'gla_ads_account_connect_button_click', | ||
{ id: 1 } | ||
); | ||
} ); | ||
|
||
it( 'should record click events for the "Connect" button and be aware of extra event properties from filters', async () => { | ||
// Prevent the component from locking in the connecting state | ||
fetchGoogleAdsAccountStatus.mockRejectedValue(); | ||
|
||
await expectComponentToRecordEventWithFilteredProperties( | ||
() => <ConnectAds accounts={ accounts } />, | ||
FILTER_ONBOARDING, | ||
async () => { | ||
const combobox = screen.getByRole( 'combobox' ); | ||
await userEvent.selectOptions( combobox, '1' ); | ||
await userEvent.click( getConnectButton() ); | ||
}, | ||
'gla_ads_account_connect_button_click', | ||
[ | ||
{ context: 'setup-mc', step: '1' }, | ||
{ context: 'setup-ads', step: '2' }, | ||
] | ||
); | ||
} ); | ||
} ); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,10 +9,26 @@ import { useState, useCallback } from '@wordpress/element'; | |
*/ | ||
import { useAppDispatch } from '.~/data'; | ||
import AppButton from '.~/components/app-button'; | ||
import useEventPropertiesFilter from '.~/hooks/useEventPropertiesFilter'; | ||
import { FILTER_ONBOARDING } from '.~/utils/tracks'; | ||
|
||
/** | ||
* Clicking on the button to disconnect the Google Ads account. | ||
* | ||
* @event gla_ads_account_disconnect_button_click | ||
* @property {string} [context] Indicates the place where the button is located. | ||
* @property {string} [step] Indicates the step in the onboarding process. | ||
Comment on lines
+19
to
+20
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. Same as above, do you think it'd be better to add more props in the doc? 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. See #2421 (comment) |
||
*/ | ||
|
||
/** | ||
* Renders a button to disconnect the Google Ads account. | ||
* | ||
* @fires gla_ads_account_disconnect_button_click When the user clicks on the button to disconnect the Google Ads account. | ||
*/ | ||
const DisconnectAccount = () => { | ||
const { disconnectGoogleAdsAccount } = useAppDispatch(); | ||
const [ isDisconnecting, setDisconnecting ] = useState( false ); | ||
const getEventProps = useEventPropertiesFilter( FILTER_ONBOARDING ); | ||
|
||
const handleSwitch = useCallback( () => { | ||
setDisconnecting( true ); | ||
|
@@ -29,6 +45,8 @@ const DisconnectAccount = () => { | |
'Or, connect to a different Google Ads account', | ||
'google-listings-and-ads' | ||
) } | ||
eventName="gla_ads_account_disconnect_button_click" | ||
eventProps={ getEventProps() } | ||
onClick={ handleSwitch } | ||
/> | ||
); | ||
|
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.
Do you think we should also add
gla_mc_id
andgla_version
props in the JSDoc so the tracking document could be more accurate?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.
The current
doc:tracking
script (woocommerce-grow-jsdoc
) doesn't support global event properties, and I'm not leaning toward duplicating global properties for the docs of all frontend tracking events.It needs more discussion about how the
woocommerce-grow-jsdoc
tool should handle global event properties. I've first listed the global event properties directly at the beginning of the tracking README file. 0e6b33dThere 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.
Got it, thanks for clarifying and adding the global event props in the tracking README. 👍