-
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
Adjust click event tracking when connecting, disconnecting, and opening billing setup for Google Ads account #2421
Conversation
… in `ConnectAds`.
… in `DisconnectAccount`.
…racking in `BillingSetupCard`.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## feature/seamlessly-conversion-tracking #2421 +/- ##
========================================================================
+ Coverage 62.6% 63.4% +0.8%
========================================================================
Files 321 321
Lines 5019 5027 +8
Branches 1216 1218 +2
========================================================================
+ Hits 3141 3188 +47
+ Misses 1709 1672 -37
+ Partials 169 167 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
|
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.
Thanks for adding event tracking, I started by testing the behaviour and I've left a question regarding the context setup-ads
and setup-mc
.
onClick={ handleClick } | ||
eventName="gla_ads_set_up_billing_click" | ||
eventProps={ { | ||
context: 'setup-ads', |
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.
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.
Yes, 'setup-mc'
is expected if you were doing extension onboarding. The billing setup card is also used in the Ads onboarding flow, where the value of context is expected to be 'setup-ads'
.
It's true that there's some inconsistency and confusion in the use of these context values, so I've put them down in tracks.js momentarily.
google-listings-and-ads/js/src/utils/tracks.js
Lines 27 to 40 in a63df8d
/* | |
* Please be aware of when to use these context values | |
* - 'setup-mc': Extension onboarding (a.k.a Merchant Center Setup or MC Setup) | |
* - 'setup-ads': Ads onboarding (a.k.a Google Ads Setup) | |
* | |
* Since Google Ads Setup has been added to the extension onboarding, | |
* the 'setup-mc' is no longer an appropriate value to identify it. | |
* The same is the case for 'setup-ads', which represents Google Ads Setup. | |
* | |
* However, as these values are heavily used in codebase and event tracking, | |
* these values continue to be used at present for consistency. | |
*/ | |
export const CONTEXT_EXTENSION_ONBOARDING = 'setup-mc'; | |
export const CONTEXT_ADS_ONBOARDING = 'setup-ads'; |
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.
Thanks for the quick answer. Tested locally and it worked as expected. Just left some comments regarding the tracking JSDoc.
@@ -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 comment
The 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 gla_ads_id
so why not use the same here for consistency, but it's been there before this PR so I think keeping it as is would be better for querying the tracking results.
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 gla_ads_id
is one of the global event properties on the frontend, and the id
of this event exists before the global event property. In order to avoid unnecessary inconsistencies in tracking data, the current preference is to leave it unchanged.
* @property {string} [context] Indicates the place where the button is located. | ||
* @property {string} [step] Indicates the step in the onboarding process. |
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
and gla_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. 0e6b33d
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.
Got it, thanks for clarifying and adding the global event props in the tracking README. 👍
* @property {string} [context] Indicates the place where the button is located. | ||
* @property {string} [step] Indicates the step in the onboarding process. |
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.
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 comment
The reason will be displayed to describe this comment to others. Learn more.
See #2421 (comment)
086366d
into
feature/seamlessly-conversion-tracking
Changes proposed in this Pull Request:
PT: pcTzPl-2iK-p2
The PR adds/adjusts three tracking events for for Google Ads account:
context
andstep
event properties to the click event tracking inConnectAds
.gla_ads_account_disconnect_button_click
event withcontext
andstep
event properties to the click event tracking inDisconnectAccount
.context
and addstep
event properties to the click event tracking inBillingSetupCard
.GoogleAdsAccountSection
.Detailed test instructions:
localStorage.setItem( 'debug', 'wc-admin:*' )
to enable tracking debugging mode.gla_ads_account_connect_button_click
event is sent withid
,context
andstep
.gla_ads_account_disconnect_button_click
event is sent withcontext
andstep
.gla_ads_set_up_billing_click
event is sent withlink_id
,href
,context
andstep
.gla_ads_set_up_billing_click
event is sent withlink_id
,href
,context
andstep
.Changelog entry