-
Notifications
You must be signed in to change notification settings - Fork 86
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
Track member approval #1307
Track member approval #1307
Conversation
@iref this is a useful addition in other ways so I want this PR to get merged as-is, though I think it's not quite a fix for #1285. I've had some extensive conversations with a support engineer at Mixpanel who has enlightened me that we may want to track a We likely need to tweak the semantics here, but does the gist of what I'm saying make sense? |
Logger.info "Called track for event #{event_name} for User #{user_id}" | ||
defp log_track(user_id, event_name, properties) do | ||
props = Poison.encode!(properties) | ||
Logger.info "Called track for event #{event_name} for User #{user_id} and properties #{props}" |
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.
Good call logging the properties.
project_id: record.project_id | ||
project_id: record.project_id, | ||
member_id: record.user.id, | ||
member: record.user.username |
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.
Would reverse these to keep with consistency of below: member
and then member_id
.
If I understand it correctly, we would like to emit two events on membership request/approval. One from user perspective (this one already exists) and one from project perspective. Both events have same properties. Can we extract |
I don't think we'd extract if from the |
And to clarify @iref, you're right about wanting to emit multiple events. Without being confusing, here's the message from Mixpanel (modified slightly to fix some errors I spotted):
In our case the user's All that make sense? |
If you want to squash your commits on this PR down to just a single commit, then I'm happy to merge this as-is since this is a good changeset, and we can continue with the bigger change in another PR. |
Added member id and member user name to properties of requesting/approving project membership. This change enables us to compute conversion rate between invited users and users that actually join project. Added properties logging to in memory analytics api
c357eb8
to
5549bdf
Compare
Yes, I think I get it now. I'll start to work on new PR that implements suggested changes. |
What's in this PR?
project_user
,These changes should enable conversion funnel analytics using member_id in queries.
References
Progress on #1285