From 61f487d114843aabcbd80b9db5ace27edc71fa83 Mon Sep 17 00:00:00 2001 From: Brandon Sneed Date: Thu, 25 Jun 2020 12:51:18 -0700 Subject: [PATCH] Payload Info & Traits Fixes (#912) * Moved some fields from SEGContext to more appropriate SEGPayload. * Fix trait storage/init issue. * Fixed traits usage w/ tests. Co-authored-by: Brandon Sneed --- Analytics/Classes/SEGAnalytics.m | 16 +++- Analytics/Classes/SEGContext.h | 4 - Analytics/Classes/SEGContext.m | 2 - Analytics/Classes/SEGIdentifyPayload.h | 4 - Analytics/Classes/SEGIdentifyPayload.m | 8 +- Analytics/Classes/SEGPayload.h | 2 + Analytics/Classes/SEGPayload.m | 10 ++- Analytics/Classes/SEGSegmentIntegration.m | 89 +++++---------------- Analytics/Internal/SEGIntegrationsManager.m | 9 ++- AnalyticsTests/AnalyticsTests.swift | 44 ++++++++++ AnalyticsTests/ContextTest.swift | 21 ++--- Podfile.lock | 2 +- 12 files changed, 108 insertions(+), 103 deletions(-) diff --git a/Analytics/Classes/SEGAnalytics.m b/Analytics/Classes/SEGAnalytics.m index 2bd9a8730..fd5e345e6 100644 --- a/Analytics/Classes/SEGAnalytics.m +++ b/Analytics/Classes/SEGAnalytics.m @@ -224,11 +224,19 @@ - (void)identify:(NSString *)userId traits:(NSDictionary *)traits options:(NSDic } // configure traits to match what is seen on android. NSMutableDictionary *newTraits = [traits mutableCopy]; + // if no traits were passed in, need to create. + if (newTraits == nil) { + newTraits = [[NSMutableDictionary alloc] init]; + } newTraits[@"anonymousId"] = anonId; if (userId != nil) { newTraits[@"userId"] = userId; [SEGState sharedInstance].userInfo.userId = userId; } + // merge w/ existing traits and set them. + NSDictionary *existingTraits = [SEGState sharedInstance].userInfo.traits; + [newTraits addEntriesFromDictionary:existingTraits]; + [SEGState sharedInstance].userInfo.traits = newTraits; [self run:SEGEventTypeIdentify payload: [[SEGIdentifyPayload alloc] initWithUserId:userId @@ -481,8 +489,12 @@ - (void)run:(SEGEventType)eventType payload:(SEGPayload *)payload ctx.eventType = eventType; ctx.payload = payload; ctx.payload.messageId = GenerateUUIDString(); - ctx.anonymousId = [SEGState sharedInstance].userInfo.anonymousId; - ctx.userId = [SEGState sharedInstance].userInfo.userId; + if (ctx.payload.userId == nil) { + ctx.payload.userId = [SEGState sharedInstance].userInfo.userId; + } + if (ctx.payload.anonymousId == nil) { + ctx.payload.anonymousId = [SEGState sharedInstance].userInfo.anonymousId; + } }]; // Could probably do more things with callback later, but we don't use it yet. diff --git a/Analytics/Classes/SEGContext.h b/Analytics/Classes/SEGContext.h index bbfc84433..48af05526 100644 --- a/Analytics/Classes/SEGContext.h +++ b/Analytics/Classes/SEGContext.h @@ -59,8 +59,6 @@ NS_SWIFT_NAME(Context) @property (nonatomic, readonly, nonnull) SEGAnalytics *_analytics; @property (nonatomic, readonly) SEGEventType eventType; -@property (nonatomic, readonly, nullable) NSString *userId; -@property (nonatomic, readonly, nullable) NSString *anonymousId; @property (nonatomic, readonly, nullable) NSError *error; @property (nonatomic, readonly, nullable) SEGPayload *payload; @property (nonatomic, readonly) BOOL debug; @@ -74,8 +72,6 @@ NS_SWIFT_NAME(Context) @protocol SEGMutableContext @property (nonatomic) SEGEventType eventType; -@property (nonatomic, nullable) NSString *userId; -@property (nonatomic, nullable) NSString *anonymousId; @property (nonatomic, nullable) SEGPayload *payload; @property (nonatomic, nullable) NSError *error; @property (nonatomic) BOOL debug; diff --git a/Analytics/Classes/SEGContext.m b/Analytics/Classes/SEGContext.m index a87804793..292883be7 100644 --- a/Analytics/Classes/SEGContext.m +++ b/Analytics/Classes/SEGContext.m @@ -73,8 +73,6 @@ - (id)copyWithZone:(NSZone *)zone { SEGContext *ctx = [[SEGContext allocWithZone:zone] initWithAnalytics:self._analytics]; ctx.eventType = self.eventType; - ctx.userId = self.userId; - ctx.anonymousId = self.anonymousId; ctx.payload = self.payload; ctx.error = self.error; ctx.debug = self.debug; diff --git a/Analytics/Classes/SEGIdentifyPayload.h b/Analytics/Classes/SEGIdentifyPayload.h index 47440b4ab..b4bf3355c 100644 --- a/Analytics/Classes/SEGIdentifyPayload.h +++ b/Analytics/Classes/SEGIdentifyPayload.h @@ -6,10 +6,6 @@ NS_ASSUME_NONNULL_BEGIN NS_SWIFT_NAME(IdentifyPayload) @interface SEGIdentifyPayload : SEGPayload -@property (nonatomic, readonly, nullable) NSString *userId; - -@property (nonatomic, readonly, nullable) NSString *anonymousId; - @property (nonatomic, readonly, nullable) JSON_DICT traits; - (instancetype)initWithUserId:(NSString *)userId diff --git a/Analytics/Classes/SEGIdentifyPayload.m b/Analytics/Classes/SEGIdentifyPayload.m index 40862b3ea..418f71f90 100644 --- a/Analytics/Classes/SEGIdentifyPayload.m +++ b/Analytics/Classes/SEGIdentifyPayload.m @@ -1,9 +1,5 @@ #import "SEGIdentifyPayload.h" -@interface SEGIdentifyPayload () -@property (nonatomic, readwrite, nullable) NSString *anonymousId; -@end - @implementation SEGIdentifyPayload - (instancetype)initWithUserId:(NSString *)userId @@ -13,9 +9,9 @@ - (instancetype)initWithUserId:(NSString *)userId integrations:(NSDictionary *)integrations { if (self = [super initWithContext:context integrations:integrations]) { - _userId = [userId copy]; - _anonymousId = [anonymousId copy]; _traits = [traits copy]; + self.anonymousId = [anonymousId copy]; + self.userId = [userId copy]; } return self; } diff --git a/Analytics/Classes/SEGPayload.h b/Analytics/Classes/SEGPayload.h index 277dc049c..6368cc45f 100644 --- a/Analytics/Classes/SEGPayload.h +++ b/Analytics/Classes/SEGPayload.h @@ -10,6 +10,8 @@ NS_SWIFT_NAME(Payload) @property (nonatomic, readonly) JSON_DICT integrations; @property (nonatomic, strong) NSString *timestamp; @property (nonatomic, strong) NSString *messageId; +@property (nonatomic, strong) NSString *anonymousId; +@property (nonatomic, strong) NSString *userId; - (instancetype)initWithContext:(JSON_DICT)context integrations:(JSON_DICT)integrations; diff --git a/Analytics/Classes/SEGPayload.m b/Analytics/Classes/SEGPayload.m index 469718761..6fa9bf64a 100644 --- a/Analytics/Classes/SEGPayload.m +++ b/Analytics/Classes/SEGPayload.m @@ -3,6 +3,9 @@ @implementation SEGPayload +@synthesize userId = _userId; +@synthesize anonymousId = _anonymousId; + - (instancetype)initWithContext:(NSDictionary *)context integrations:(NSDictionary *)integrations { if (self = [super init]) { @@ -15,6 +18,9 @@ - (instancetype)initWithContext:(NSDictionary *)context integrations:(NSDictiona _context = [combinedContext copy]; _integrations = [integrations copy]; + _messageId = nil; + _userId = nil; + _anonymousId = nil; } return self; } @@ -23,20 +29,16 @@ - (instancetype)initWithContext:(NSDictionary *)context integrations:(NSDictiona @implementation SEGApplicationLifecyclePayload - @end @implementation SEGRemoteNotificationPayload - @end @implementation SEGContinueUserActivityPayload - @end @implementation SEGOpenURLPayload - @end diff --git a/Analytics/Classes/SEGSegmentIntegration.m b/Analytics/Classes/SEGSegmentIntegration.m index 61c36aa90..272caa4a5 100644 --- a/Analytics/Classes/SEGSegmentIntegration.m +++ b/Analytics/Classes/SEGSegmentIntegration.m @@ -36,7 +36,7 @@ @interface SEGSegmentIntegration () @property (nonatomic, strong) NSTimer *flushTimer; @property (nonatomic, strong) dispatch_queue_t serialQueue; @property (nonatomic, strong) dispatch_queue_t backgroundTaskQueue; -@property (nonatomic, strong) NSMutableDictionary *traits; +@property (nonatomic, strong) NSDictionary *traits; @property (nonatomic, assign) SEGAnalytics *analytics; @property (nonatomic, assign) SEGAnalyticsConfiguration *configuration; @property (atomic, copy) NSDictionary *referrer; @@ -68,6 +68,9 @@ - (id)initWithAnalytics:(SEGAnalytics *)analytics httpClient:(SEGHTTPClient *)ht self.serialQueue = seg_dispatch_queue_create_specific("io.segment.analytics.segmentio", DISPATCH_QUEUE_SERIAL); self.backgroundTaskQueue = seg_dispatch_queue_create_specific("io.segment.analytics.backgroundTask", DISPATCH_QUEUE_SERIAL); self.flushTaskID = UIBackgroundTaskInvalid; + + // load traits from disk. + [self loadTraits]; [self dispatchBackground:^{ // Check for previous queue data in NSUserDefaults and remove if present. @@ -97,63 +100,6 @@ - (id)initWithAnalytics:(SEGAnalytics *)analytics httpClient:(SEGHTTPClient *)ht return self; } -/* - * There is an iOS bug that causes instances of the CTTelephonyNetworkInfo class to - * sometimes get notifications after they have been deallocated. - * Instead of instantiating, using, and releasing instances you * must instead retain - * and never release them to work around the bug. - * - * Ref: http://stackoverflow.com/questions/14238586/coretelephony-crash - */ - -#if TARGET_OS_IOS -static CTTelephonyNetworkInfo *_telephonyNetworkInfo; -#endif - -- (NSDictionary *)liveContext -{ - NSMutableDictionary *context = [[NSMutableDictionary alloc] init]; - context[@"locale"] = [NSString stringWithFormat: - @"%@-%@", - [NSLocale.currentLocale objectForKey:NSLocaleLanguageCode], - [NSLocale.currentLocale objectForKey:NSLocaleCountryCode]]; - - context[@"timezone"] = [[NSTimeZone localTimeZone] name]; - - context[@"network"] = ({ - NSMutableDictionary *network = [[NSMutableDictionary alloc] init]; - - if (self.reachability.isReachable) { - network[@"wifi"] = @(self.reachability.isReachableViaWiFi); - network[@"cellular"] = @(self.reachability.isReachableViaWWAN); - } - -#if TARGET_OS_IOS - static dispatch_once_t networkInfoOnceToken; - dispatch_once(&networkInfoOnceToken, ^{ - _telephonyNetworkInfo = [[CTTelephonyNetworkInfo alloc] init]; - }); - - CTCarrier *carrier = [_telephonyNetworkInfo subscriberCellularProvider]; - if (carrier.carrierName.length) - network[@"carrier"] = carrier.carrierName; -#endif - - network; - }); - - context[@"traits"] = ({ - NSMutableDictionary *traits = [[NSMutableDictionary alloc] initWithDictionary:[self traits]]; - traits; - }); - - if (self.referrer) { - context[@"referrer"] = [self.referrer copy]; - } - - return [context copy]; -} - - (void)dispatchBackground:(void (^)(void))block { seg_dispatch_specific_async(_serialQueue, block); @@ -217,10 +163,19 @@ - (void)saveUserId:(NSString *)userId }]; } -- (void)addTraits:(NSDictionary *)traits +- (NSDictionary *)traits +{ + return [SEGState sharedInstance].userInfo.traits; +} + +- (void)setTraits:(NSDictionary *)traits +{ + [self saveTraits:traits]; +} + +- (void)saveTraits:(NSDictionary *)traits { [self dispatchBackground:^{ - [self.traits addEntriesFromDictionary:traits]; [SEGState sharedInstance].userInfo.traits = traits; #if TARGET_OS_TV [self.userDefaultsStorage setDictionary:[self.traits copy] forKey:SEGTraitsKey]; @@ -236,7 +191,7 @@ - (void)identify:(SEGIdentifyPayload *)payload { [self dispatchBackground:^{ [self saveUserId:payload.userId]; - [self addTraits:payload.traits]; + [self saveTraits:payload.traits]; }]; NSMutableDictionary *dictionary = [NSMutableDictionary dictionary]; @@ -474,17 +429,17 @@ - (NSMutableArray *)queue return _queue; } -- (NSMutableDictionary *)traits +- (void)loadTraits { - if (!_traits) { + if (![SEGState sharedInstance].userInfo.traits) { + NSDictionary *traits = nil; #if TARGET_OS_TV - _traits = [[self.userDefaultsStorage dictionaryForKey:SEGTraitsKey] ?: @{} mutableCopy]; + traits = [[self.userDefaultsStorage dictionaryForKey:SEGTraitsKey] ?: @{} mutableCopy]; #else - _traits = [[self.fileStorage dictionaryForKey:kSEGTraitsFilename] ?: @{} mutableCopy]; + traits = [[self.fileStorage dictionaryForKey:kSEGTraitsFilename] ?: @{} mutableCopy]; #endif + [SEGState sharedInstance].userInfo.traits = traits; } - [SEGState sharedInstance].userInfo.traits = _traits; - return _traits; } - (NSUInteger)maxBatchSize diff --git a/Analytics/Internal/SEGIntegrationsManager.m b/Analytics/Internal/SEGIntegrationsManager.m index db2242760..6fa67be96 100644 --- a/Analytics/Internal/SEGIntegrationsManager.m +++ b/Analytics/Internal/SEGIntegrationsManager.m @@ -191,12 +191,13 @@ - (void)identify:(SEGIdentifyPayload *)payload NSCAssert2(payload.userId.length > 0 || payload.traits.count > 0, @"either userId (%@) or traits (%@) must be provided.", payload.userId, payload.traits); NSString *anonymousId = payload.anonymousId; - if (anonymousId) { + NSString *existingAnonymousId = self.cachedAnonymousId; + + if (anonymousId == nil) { + payload.anonymousId = anonymousId; + } else if (![anonymousId isEqualToString:existingAnonymousId]) { [self saveAnonymousId:anonymousId]; - } else { - anonymousId = self.cachedAnonymousId; } - payload.anonymousId = anonymousId; [self callIntegrationsWithSelector:NSSelectorFromString(@"identify:") arguments:@[ payload ] diff --git a/AnalyticsTests/AnalyticsTests.swift b/AnalyticsTests/AnalyticsTests.swift index baa657089..2cb3cf48b 100644 --- a/AnalyticsTests/AnalyticsTests.swift +++ b/AnalyticsTests/AnalyticsTests.swift @@ -88,6 +88,50 @@ class AnalyticsTests: XCTestCase { XCTAssertEqual(analytics2.test_integrationsManager()?.test_segmentIntegration()?.test_userId(), "testUserId1") } + func testPersistsTraits() { + analytics.identify("testUserId1", traits: ["trait1": "someTrait"]) + + let analytics2 = Analytics(configuration: config) + analytics2.test_integrationsManager()?.test_setCachedSettings(settings: cachedSettings) + + XCTAssertEqual(analytics.test_integrationsManager()?.test_segmentIntegration()?.test_userId(), "testUserId1") + XCTAssertEqual(analytics2.test_integrationsManager()?.test_segmentIntegration()?.test_userId(), "testUserId1") + + var traits = analytics.test_integrationsManager()?.test_segmentIntegration()?.test_traits() + var storedTraits = analytics2.test_integrationsManager()?.test_segmentIntegration()?.test_traits() + + if let trait1 = traits?["trait1"] as? String { + XCTAssertEqual(trait1, "someTrait") + } else { + XCTAssert(false, "Traits are nil!") + } + + if let storedTrait1 = storedTraits?["trait1"] as? String { + XCTAssertEqual(storedTrait1, "someTrait") + } else { + XCTAssert(false, "Traits were not stored!") + } + + analytics.identify("testUserId1", traits: ["trait2": "someOtherTrait"]) + + traits = analytics.test_integrationsManager()?.test_segmentIntegration()?.test_traits() + storedTraits = analytics2.test_integrationsManager()?.test_segmentIntegration()?.test_traits() + + if let trait1 = traits?["trait2"] as? String { + XCTAssertEqual(trait1, "someOtherTrait") + } else { + XCTAssert(false, "Traits are nil!") + } + + if let storedTrait1 = storedTraits?["trait2"] as? String { + XCTAssertEqual(storedTrait1, "someOtherTrait") + } else { + XCTAssert(false, "Traits were not stored!") + } + + + } + func testClearsUserData() { analytics.identify("testUserId1", traits: [ "Test trait key" : "Test trait value"]) analytics.reset() diff --git a/AnalyticsTests/ContextTest.swift b/AnalyticsTests/ContextTest.swift index 950e2326a..559c96d89 100644 --- a/AnalyticsTests/ContextTest.swift +++ b/AnalyticsTests/ContextTest.swift @@ -44,38 +44,41 @@ class ContextTests: XCTestCase { let context = Context(analytics: analytics) let newContext = context.modify { context in - context.userId = "sloth" - context.eventType = .track; + context.payload = TrackPayload() + context.payload?.userId = "sloth" + context.eventType = .track } - XCTAssertEqual(newContext.userId, "sloth") + XCTAssertEqual(newContext.payload?.userId, "sloth") XCTAssertEqual(newContext.eventType, EventType.track) } func testModifiesCopyInDebugMode() { let context = Context(analytics: analytics).modify { context in context.debug = true + context.eventType = .track } XCTAssertEqual(context.debug, true) let newContext = context.modify { context in - context.userId = "123" + context.eventType = .identify } XCTAssertNotEqual(context, newContext) - XCTAssertEqual(newContext.userId, "123") - XCTAssertNil(context.userId) + XCTAssertEqual(newContext.eventType, .identify) + XCTAssertEqual(context.eventType, .track) } func testModifiesSelfInNonDebug() { let context = Context(analytics: analytics).modify { context in context.debug = false + context.eventType = .track } XCTAssertFalse(context.debug) let newContext = context.modify { context in - context.userId = "123" + context.eventType = .identify } XCTAssertEqual(context, newContext) - XCTAssertEqual(newContext.userId, "123") - XCTAssertEqual(context.userId, "123") + XCTAssertEqual(newContext.eventType, .identify) + XCTAssertEqual(context.eventType, .identify) } } diff --git a/Podfile.lock b/Podfile.lock index 74fdd16eb..1952e4c7e 100644 --- a/Podfile.lock +++ b/Podfile.lock @@ -25,4 +25,4 @@ SPEC CHECKSUMS: PODFILE CHECKSUM: 0317236deb6b78344dafed0871776d4342f80caa -COCOAPODS: 1.9.3 +COCOAPODS: 1.9.1