-
Notifications
You must be signed in to change notification settings - Fork 24
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
(feat): easy event tracking. #359
Conversation
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.
Please update title to be correct. I don't think log messages are an issue.
@@ -27,7 +28,7 @@ | |||
|
|||
// --- Visitor Parameters ---- | |||
NSString * const OPTLYEventParameterKeysSnapshots = @"snapshots"; // nonnull | |||
NSString * const OPTLYEventParameterKeysVisitorId = @"visitor_id"; // nonnull | |||
NSString * const OPTLYEventParameterKeysVisitorId = @"visitor_id"; // nonnull @"enrich_decisions" |
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.
enrich_decisions
?
@@ -1626,7 +1626,7 @@ - (void)testTrackWithAttributesComplexAudienceMismatch { | |||
|
|||
[self.optimizelyTypedAudience track:@"user_signed_up" userId:@"test_user" attributes:userAttributes]; | |||
NSString *logMessage = [NSString stringWithFormat:OPTLYLoggerMessagesConversionFailure, @"user_signed_up"]; | |||
OCMVerify([loggerMock logMessage:logMessage withLevel:OptimizelyLogLevelInfo]); | |||
//OCMVerify([loggerMock logMessage:logMessage withLevel:OptimizelyLogLevelInfo]); |
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.
This can be removed right?
@@ -1386,7 +1386,7 @@ - (void)testTrackExcludesUserFromExperimentWithTypedAudiences { | |||
}]]; | |||
[optimizely track:eventId userId:userId attributes:attributes]; | |||
NSString *logMessage = [NSString stringWithFormat:OPTLYLoggerMessagesConversionFailure, eventId]; | |||
OCMVerify([loggerMock logMessage:logMessage withLevel:OptimizelyLogLevelInfo]); | |||
//OCMVerify([loggerMock logMessage:logMessage withLevel:OptimizelyLogLevelInfo]); |
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.
This can be removed right?
Headers need to be updated. |
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.
I think you can get rid of https://github.com/optimizely/objective-c-sdk/blob/master/OptimizelySDKCore/OptimizelySDKCore/Optimizely.m#L912-L961 and associated unit tests.
Will wait to get resolution for compatibility tests before we can get this in.
@@ -1,5 +1,5 @@ | |||
/**************************************************************************** | |||
* Copyright 2016,2018, Optimizely, Inc. and contributors * | |||
* Copyright 2016-2019, Optimizely, Inc. and contributors * |
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.
nit. This should actually be 2016, 2018-2019 based on what it was previously.
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.
Does that mean we weren't copyrighted in 2017? I will change it, but, seems weird.
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.
Looks good. Also, @msohailhussain confirmed that tests are fine.
Going to merge this change.
Summary
Event builder change.
One byproduct is we do not filter the event anymore so we do not get a log message if the event is not part of any experiment. We also send the event when it is not part of any experiment which is different behaviour.
Test plan
Added a test that makes sure enhance_decisions is set
Issues