-
Notifications
You must be signed in to change notification settings - Fork 6
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
Add UUID to each video event #73
Conversation
To identify `videostart` events and corresponding `vheartbeat` events.
to video-related events.
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.
👋 @wzieba !
I have reviewed and tested this PR as per the instructions, everything works as expected, good job! 🌟
I have left a suggestion (💡) and a minor (🔍) comment for you to consider. I am going to approve this PR anyway, since none is blocking. I am NOT going to merge this PR yet to give you some time to apply any of my suggestions. However, feel free to ignore them and merge the PR yourself.
|
||
// Enqueue the videostart | ||
enqueueEvent(buildEvent(url, urlRef, "videostart", videoMetadata, extraData)); | ||
@NonNull final Map<String, Object> videostartEvent = buildEvent(url, urlRef, "videostart", videoMetadata, extraData); | ||
videostartEvent.put(VIDEO_START_ID_KEY, uuid); |
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.
Suggestion (💡): Instead of explicitly using an extra .put(VIDEO_START_ID_KEY, uuid)
call after the event is constructed and returned, wherever that might be required, and thus letting the caller decide the event's state, which can be prone to misusage, how about moving that logic with the buildEvent(...)
method itself and avoid any such misusage?
@NonNull
private Map<String, Object> buildEvent(
String url,
String urlRef,
String action,
ParselyMetadata metadata,
Map<String, Object> extraData,
@Nullable String uuid) {
...
if (action.equals("videostart") || action.equals("vheartbeat")) {
event.put(VIDEO_START_ID_KEY, uuid);
}
return event;
}
FYI: I also think that doing this makes it more visible and explicit and only videostart
and vheartbeat
type of events can have an associated vsid
attached to them.
PS: I understand that this is adding an extra parameter to the method, and that would have been better if a builder pattern would to be used, but I still think it is better than letting callers decide that after the fact, when the even is already created and returned by the buildEvent(...)
method. 🤔
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.
Sure, I can do that 👍
I understand that this is adding an extra parameter to the method
That was my concern when deciding on approach here, but keeping all "bulding blocks" in buildEvent
method is probably indeed better!
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.
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.
Yes, thank you! 🙏
Partially addresses #28
Description
This PR adds a new parameter:
vsid
(Video Start ID) to each newvideostart
event and the same ID to associatedvheartbeat
events.Requirements
Internal source: p1696261199218709/1696235256.764549-slack-C0533SEJ82H
New payload
Click
Test
Track Video
[Parsely] POST Data {"events": (...)
log. Copy the JSON part.[Parsely] POST Data {"events": (...)
log, copy it as well."action": "videostart"
event, rest of them should be"action": "vheartbeat"
"vsid"
field with the same value