PLAT-13612: Adjust how CLI is tracked in Amplitude #1092
Conversation
robester0403
left a comment
robester0403
left a comment
There was a problem hiding this comment.
Looks good with small optional comments
|
|
||
| class AmplitudeWrapper { | ||
| get userType() { | ||
| // TODO: Implement once we have the data available in the UserSerializer |
There was a problem hiding this comment.
Just doubling checking that there's a ticket for this further down the road.
There was a problem hiding this comment.
I don't think there's one yet, but this was defined as a mandatory property in the spreadsheet, so I added this to not forget. I will ask Bryan, probably should create the ticket now
|
|
||
| const identifyEvent = new Identify(); | ||
| identifyEvent.set("Django Id", currentUser.get("id")); | ||
| identify(identifyEvent, { user_id: currentUser.get("email") }); |
There was a problem hiding this comment.
Not really sure but are we guaranteed to get an email from the user response? otherwise ignore this comment
There was a problem hiding this comment.
If the user object is returned, then that's a guarantee, since it is a required field. But I think to safeguard, I should probably change if (!currentUser.get("id")) return; to if (!currentUser.get("email")) return;, since email is more important than id for amplitude.
I'll change it!
Ticket
PLAT-13612
Related PRs: https://github.com/crowdbotics/vue-dash-5104/pull/4663
Type of PR
Did you make changes to modules or create a new module?
Changes introduced
Updated amplitude tracking to respect the send the same properties as the Vue dashboard. The following events are being tracked:
Notes:
currentUserglobal object, I did not replace otherget /user/API calls with this global object yet (I wanted to keep the changes scoped to amplitude-related code only)Test and review
Test and see if the events mentioned above are being sent to Amplitude.