Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: track user's wallet type through user properties #2688

Merged
merged 5 commits into from
Oct 30, 2023
Merged

feat: track user's wallet type through user properties #2688

merged 5 commits into from
Oct 30, 2023

Conversation

schmanu
Copy link
Member

@schmanu schmanu commented Oct 24, 2023 •
edited
Loading

What it solves

Track the user's used wallet type as user property.

Checklist

  • I've tested the branch on mobile 📱
  • I've documented how it affects the analytics (if at all) 📊
  • I've written a unit/e2e test for it (if applicable) 🧑‍💻

@github-actions
Copy link

github-actions bot commented Oct 24, 2023 •
edited
Loading

Branch preview

✅ Deploy successful!

https://analytics_user_wallet--walletweb.review-wallet-web.5afe.dev

@github-actions
Copy link

github-actions bot commented Oct 24, 2023 •
edited
Loading

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type Occurrences Fixable
Errors 0 0
Warnings 0 0
Ignored 0 N/A
  • Result: ✅ success
  • Annotations: 0 total
Report generated by eslint-plus-action

@usame-algan usame-algan changed the title feat: track user's wallet through user properties feat: track user's wallet type through user properties Oct 24, 2023
@github-actions
Copy link

github-actions bot commented Oct 24, 2023 •
edited
Loading

Coverage report

St.❔
 Category Percentage Covered / Total
🟡 Statements
74.9% (-0.42% 🔻)
 9011/12030
🔴 Branches
48.73% (-0.7% 🔻)
 1809/3712
🔴 Functions
57.28% (-0.9% 🔻)
 1342/2343
🟡 Lines
76.5% (-0.45% 🔻)
 8146/10648
Show files with reduced coverage 🔻
St.❔
 File Statements Branches Functions Lines
🟢 src/store/index.ts
98% (-0.04% 🔻)
 100% 100% 100%
🟢 src/store/common.ts
88.89% (-11.11% 🔻)
0% (-50% 🔻)
66.67% (-33.33% 🔻)
85.71% (-14.29% 🔻)
🟢
... / formatters.ts
80.77% (-1.92% 🔻)
 63.64% 83.33% 81.58%
🟢
... / addedSafesSlice.ts
83.33% (-5% 🔻)
 71.43%
80% (-6.67% 🔻)
82.35% (-5.88% 🔻)
🟡
... / gtm.ts
58.46% (-3.44% 🔻)
 55%
37.5% (-12.5% 🔻)
64.81% (-1.22% 🔻)
🟡
... / transactions.ts
60.18% (-0.35% 🔻)
31.03% (+2% 🔼)
 28.57% 64.29%
🟢
... / useSafeCreationEffects.ts
86.05% (-0.62% 🔻)
88.89% (-1.11% 🔻)
 80%
85% (-0.71% 🔻)
🔴
... / index.tsx
20% (-4.44% 🔻)
 0% 0%
18.92% (-5.47% 🔻)
🟡
... / safe-messages.ts
 78.95%
61.11% (-3.89% 🔻)
 100% 76.92%
🟢
... / useHighlightHiddenTab.ts
77.78% (+1.31% 🔼)
37.5% (-2.5% 🔻)
71.43% (-6.35% 🔻)
80% (+1.88% 🔼)
🟢
... / index.tsx
83.33% (-16.67% 🔻)
 100% 0% 100%
🟢
... / index.tsx
69.23% (-7.69% 🔻)
 0% 0% 81.82%
🟢
... / index.tsx
83.33% (-16.67% 🔻)
 100% 0% 100%
🟡
... / index.tsx
62.5% (-1.79% 🔻)
 0% 0%
65.79% (-1.71% 🔻)

Test suite run success

962 tests passing in 133 suites.

Report generated by 🧪jest coverage report action from 48fa5de

katspaugh
katspaugh reviewed Oct 24, 2023
View reviewed changes
src/services/analytics/useGtm.ts Outdated
@@ -79,6 +82,10 @@ const useGtm = () => {
gtmTrackPageview(router.pathname)
}, [router.pathname])

useEffect(() => {
gtmSetUserProperty('walletLabel', walletLabel ?? 'NONE')
Copy link
Member

Choose a reason for hiding this comment

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

Please create an enum for possible user properties, similar to how to define custom events.

katspaugh
katspaugh reviewed Oct 24, 2023
View reviewed changes
src/services/analytics/useGtm.ts Outdated
@@ -26,6 +26,12 @@ import { DeviceType } from './types'
import useSafeAddress from '@/hooks/useSafeAddress'
import useWallet from '@/hooks/wallets/useWallet'

enum GA_USER_PROPERTIES {
Copy link
Member

Choose a reason for hiding this comment

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

I wouldn’t put it here. This hook is just a hook to bind GTM to React. The actual analytics service is where it should be, similar to our custom event definitions.

katspaugh
katspaugh approved these changes Oct 25, 2023
View reviewed changes
Copy link
Member

@katspaugh katspaugh left a comment

Choose a reason for hiding this comment

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

👍

@francovenica
Copy link
Contributor

Manu and I checked together how the new property is tracked in ever event. Looks good.
There is a new exploration "instance" that tracks the events with this property, but it updates every 24hs, so I'll check tomorrow

@katspaugh
Copy link
Member

Seems to work.

Screenshot 2023-10-30 at 07 35 05

I'm not sure about NONE though, maybe we should just keep it unset?

katspaugh
katspaugh reviewed Oct 30, 2023
View reviewed changes
src/services/analytics/useGtm.ts Outdated
Comment on lines 85 to 87
useEffect(() => {
gtmSetUserProperty(AnalyticsUserProperties.WALLET_LABEL, walletLabel ?? WALLET_LABEL_NONE)
}, [walletLabel])
Copy link
Member

Choose a reason for hiding this comment

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

Since the wallet is asynchronous, it will inevitably log NONE for the first few events before the wallet (re)connects. I would suggest to not set it at all until the user connects a wallet.

Optionally, we could also unset it when the user disconnects. You migth have to listen to a specific onboard event for that. But to keep things simple I would just keep the last wallet and never unset it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes sense. Especially because the interesting interactions such as signing a message / executing a tx will require a wallet anyways.

@katspaugh katspaugh merged commit aa3ddab into dev Oct 30, 2023
8 of 9 checks passed
@katspaugh katspaugh deleted the analytics-user-wallet branch October 30, 2023 10:54
@github-actions github-actions bot locked and limited conversation to collaborators Oct 30, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@katspaugh katspaugh katspaugh approved these changes

Assignees
No one assigned
Labels
None yet
Projects
Safe{Wallet}
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@schmanu @francovenica @katspaugh