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.

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

Alias user on create #1357

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

Alias user on create #1357

wants to merge 2 commits into from

Conversation

joshsmith
Copy link
Contributor

What's in this PR?

Allows us to alias the user on creation.

This is a WIP and very much incomplete.

A few of the issues we have with this:

  • it's not recommended to alias on the server-side
  • we're sending too many events close to the alias, leading to a race condition
  • it's not clear whether we should be aliasing to the segment ID or the mixpanel ID

@begedin
Copy link
Contributor

begedin commented Jan 2, 2018

Re:

  • we're sending too many events close to the alias, leading to a race condition

We already agreed we should weer of off implicit tracking through Plugs.Segment and instead track explicitly from the controller action.

We did this in other places, but it looks like here, we still implicitly track user created using the plug. This may cause some parallelism, which may be what's causing race conditions.

If we moved tracking to be explicit, we may eliminate this, but I'm not sure.

We would do this by

  • removing
def includes?(:create, %CodeCorps.User{}), do: true

from lib/code_corps/analytics/segment_tracking_support.ex

  • adding
user_id |> SegmentTracker.track("Signed Up", user)

to the create action

I also think we should add a function clause to CodeCorps.Analytics.SegmentTracker which accepts the user struct as the first argument, instead of just the id, but that's a whole other issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants