Add support for bulk track/engage APIs#3
Conversation
- `Mixpanel.engage` function added to update user profile
- `Mixpanel.track` and `Mixpanel.engage` will both support a list of maps
- When given a list, these functions expect the maps to conform to the structure defined by
the mixpanel HTTP api documentation (linked from README)
michihuber
left a comment
michihuber
left a comment
There was a problem hiding this comment.
Hey Mike!
Thank you very much for the pull request!
I added some comments, mostly cosmetics, but I also think we should add some tests using ExVcr (which I should have done from the beginning, my bad).
If you want me to help out with the testing part, please let me know.
Thank you very much!
| :ok | ||
| end | ||
|
|
||
| def engage(event) when is_map(event) do |
There was a problem hiding this comment.
Both engage fns have the same body, could you collapse them into a single fn, please?
| GenServer.cast(:mixpanel_client, {:track, events}) | ||
| end | ||
|
|
||
| def engage(event) when is_map(event) do |
There was a problem hiding this comment.
These could also be the same fn.
| properties = Dict.put(properties, :token, state.token) | ||
| {:ok, json} = JSX.encode([event: event, properties: properties]) | ||
| body = String.to_char_list("data=#{ :base64.encode(json) }") | ||
| {:ok, json} = JSX.encode(event: event, properties: properties) |
There was a problem hiding this comment.
Does bulk posting a non-array work?
We should probably add some ex-vcr specs at this point.
|
|
||
| def handle_cast({:track, events}, state) when is_list(events) do | ||
| events = Enum.map(events, &put_in(&1, [:properties, :token], state.token)) | ||
| {:ok, json} = JSX.encode(events) |
There was a problem hiding this comment.
could you dry up the encode/post/reply dance that's happening in all the handle_casts, please?
- Mixpanel.Client now only accepts lists of events - Mixpanel facade normalizes single events to lists of maps
- Note that these don't actually contain any asserts - And the ExVCR integration isn't working -- AFAICT the HTTPC adapter doesn't work with a custom :inets profile -- Even when using the default profile, the Mixpanel API is async, so need to wait in tests before asserting somehow
|
Hey Michi, thanks for considering this PR. I've tried to DRY up the implementation as per your feedback. I didn't have much luck with ExVCR. I've added the mix dependency and left the test module in there, but it just exercises the code without actually asserting on anything right now. |
|
@michihuber Any suggestions on getting the VCRs on this one working? |
2 participants
Mixpanel.engagefunction added to update user profileMixpanel.trackandMixpanel.engagewill both support a list of mapsthe mixpanel HTTP api documentation (linked from README)