-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Add GTM event tracking #51599
Add GTM event tracking #51599
Conversation
Hey, I noticed you changed some webpack configuration files. This can break production builds. Did you remember to run a production build locally to verify they still work? |
A preview of your ExpensifyHelp changes have been deployed to https://1f93192b.helpdot.pages.dev ⚡️ |
…osiclair-gtm-events
8bd621e
to
b05efd9
Compare
78e2a82
to
e6fc1f0
Compare
Reviewer Checklist
Screenshots/VideosAndroid: mWeb ChromeN/a iOS: mWeb SafariN/a |
@arosiclair is the workspace_created event recorded only the first time? I am seeing the logs the first time only. Screen.Recording.2024-11-06.at.9.36.51.at.night.mov |
Yes we only publish |
src/libs/Navigation/AppNavigator/Navigators/OnboardingModalNavigator.tsx
Outdated
Show resolved
Hide resolved
Lint is failing |
Yes, we just assume the command succeeded in this case. It's fine if it fails and the event gets published.
Done |
This comment was marked as duplicate.
This comment was marked as duplicate.
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.
LGTM, the comments I made are NAB
// Publish a sign_up event when we start the onboarding flow. This should track basic sign ups | ||
// as well as Google and Apple SSO. | ||
useEffect(() => { | ||
if (!accountID) { | ||
return; | ||
} | ||
|
||
GoogleTagManager.publishEvent(CONST.ANALYTICS.EVENT.SIGN_UP, accountID); | ||
}, [accountID]); |
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.
Are there cases when this effect could run multiple times for the user? ie they will get to this point and close the app. They will come back to it later and we will again send the sign_up event. Not sure if its a problem though, just thought about 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.
Yeah if they restart/refresh the app during onboarding, this would get called again. Since it's kind of an edge case we're just ignoring that for now. Also it's not too big of a deal as long as it stays consistent between A/B tests.
@@ -1838,6 +1839,11 @@ function createWorkspace(policyOwnerEmail = '', makeMeAdmin = false, policyName | |||
const {optimisticData, failureData, successData, params} = buildPolicyData(policyOwnerEmail, makeMeAdmin, policyName, policyID, undefined, engagementChoice); | |||
API.write(WRITE_COMMANDS.CREATE_WORKSPACE, params, {optimisticData, successData, failureData}); | |||
|
|||
// Publish a workspace created event if this is their first policy |
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.
NAB, I wonder if the events and should be called workspace or policy since we try to keep everything in the BE as policy. Also its weird to see both of them in the comment on one line :D
// Publish a workspace created event if this is their first policy | |
// Publish a workspace created event if this is their first workspace |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/mountiny in version: 9.0.60-0 🚀
|
🚀 Deployed to production by https://github.com/francoisl in version: 9.0.60-3 🚀
|
Details
Uses our new
GoogleTagManager
lib to tracksign_up
,workspace_created
andpaid_adoption
events.Fixed Issues
$ #50940
Tests
USE_THIRD_PARTY_SCRIPTS=true
to your .envQA Steps
Note: QA for web only
Sign Up
Create Workspace - Onboarding Flow
Create Workspace - Global create
Create Workspace - Workspaces page
Create Workspace - Categorize tracked expense
Paid Adoption
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps./** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)Screenshots/Videos
Android: Native
iOS: Native
MacOS: Chrome / Safari
MacOS: Desktop