Skip to content

Conversation

djih
Copy link
Member

@djih djih commented Oct 3, 2018

Adding support for group identify calls. We can reuse identify.js objects to capture the operations. Generates an event that is very similar to $identify events. Currently enforcing only 1 group_name value per groupIdentify

@djih djih requested review from blazzy and damahua October 3, 2018 04:32
var baseSetToString = !_defineProperty ? identity_1 : function(func, string) {
return _defineProperty(func, 'toString', {
var baseSetToString = !_defineProperty$1 ? identity_1 : function(func, string) {
return _defineProperty$1(func, 'toString', {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's this $1 mean?

groups[groupType] = groupName;
var identify = new Identify().set(groupType, groupName);
this._logEvent(constants.IDENTIFY_EVENT, null, null, identify.userPropertiesOperations, groups, null, null);
this._logEvent(constants.IDENTIFY_EVENT, null, null, identify.userPropertiesOperations, groups, null, null, null);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need a GROUP_IDENTIFY_EVENT? and groupPropertiesOpeations?

if (identify_obj instanceof Identify) {
// only send if there are operations
if (Object.keys(identify_obj.userPropertiesOperations).length > 0) {
return this._logEvent(constants.GROUP_IDENTIFY_EVENT, null, null, null, _defineProperty({}, group_type, group_name), identify_obj.userPropertiesOperations, null, opt_callback);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it seems we have the GROUP_IDENTIFY_EVENT, do we need groupPropertiesOpeations?

Copy link

@damahua damahua left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm in terms of the event format.

@blazzy blazzy merged commit f98d4aa into master Oct 24, 2018
@blazzy blazzy deleted the add-group-identify branch October 24, 2018 19:00
@sk-pub
Copy link

sk-pub commented Nov 8, 2018

@blazzy , @djih , @damahua Looks like this change broke the client. Null can be passed to the parameter groupProperties and utils.validateProperties(groupProperties) throws the "invalid properties format" error.

"groupProperties = groupProperties || {}" needs to be added.

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

Successfully merging this pull request may close these issues.

4 participants