Refactor LDClient updateUser (#99)

CHANGELOG.md

@@ -2,6 +2,10 @@
 
 All notable changes to the LaunchDarkly iOS SDK will be documented in this file. This project adheres to [Semantic Versioning](http://semver.org).
 
+## [2.13.6] - 2018-10-04
+### Fixed
+- LDClient's `updateUser` did not attempt to retrieve the new user's cached flag values.
+
 ## [2.13.5] - 2018-09-23
 ### Changed
 - Repairs Carthage build errors caused by higher fidelity checks in Xcode 10's new build engine.

Darkly/DarklyConstants.m

@@ -4,7 +4,7 @@
 
 #import "DarklyConstants.h"
 
-NSString * const kClientVersion = @"2.13.5";
+NSString * const kClientVersion = @"2.13.6";
 NSString * const kBaseUrl = @"https://app.launchdarkly.com";
 NSString * const kEventsUrl = @"https://mobile.launchdarkly.com";
 NSString * const kStreamUrl = @"https://clientstream.launchdarkly.com";

Darkly/LDClient.m

@@ -96,7 +96,13 @@ - (BOOL)updateUser:(LDUserBuilder *)builder {
         return NO;
     }
 
-    self.ldUser = [LDUserBuilder compareNewBuilder:builder withUser:self.ldUser];
+    if (builder.key.length > 0 && [builder.key isEqualToString:self.ldUser.key]) {
+        self.ldUser = [LDUserBuilder compareNewBuilder:builder withUser:self.ldUser];
+        [[LDDataManager sharedManager] saveUser:self.ldUser];
+    } else {
+        self.ldUser = [builder build];
+    }
+
     [[LDClientManager sharedInstance] updateUser];
     return YES;
 }

Darkly/LDDataManager.h

@@ -41,7 +41,6 @@ extern int const kUserCacheSize;
                   defaultFlagValue:(id)defaultFlagValue
                               user:(LDUserModel*)user
                             config:(LDConfig*)config;
--(void)purgeOldUser: (NSMutableDictionary *)dictionary;
 -(void)saveUser: (LDUserModel *) user;
 -(void)saveUserDeprecated:(LDUserModel *)user __deprecated_msg("Use saveUser: instead");
 -(void)deleteProcessedEvents: (NSArray *) processedJsonArray;

Darkly/LDDataManager.m

@@ -11,6 +11,7 @@
 #import "LDEventTrackingContext.h"
 #import "LDFlagConfigTracker.h"
 #import "NSDate+ReferencedDate.h"
+#import "NSThread+MainExecutable.h"
 
 int const kUserCacheSize = 5;
 
@@ -24,6 +25,7 @@ @interface LDDataManager()
 
 @implementation LDDataManager
 
+dispatch_queue_t saveUserQueue;
 dispatch_queue_t eventsQueue;
 
 + (id)sharedManager {
@@ -32,7 +34,8 @@ + (id)sharedManager {
     dispatch_once(&onceToken, ^{
         sharedDataManager = [[self alloc] init];
         sharedDataManager.eventsArray = [[NSMutableArray alloc] init];
-        eventsQueue = dispatch_queue_create("com.launchdarkly.EventQueue", NULL);
+        saveUserQueue = dispatch_queue_create("com.launchdarkly.dataManager.saveUserQueue", DISPATCH_QUEUE_SERIAL);
+        eventsQueue = dispatch_queue_create("com.launchdarkly.dataManager.eventQueue", DISPATCH_QUEUE_SERIAL);
     });
     return sharedDataManager;
 }
@@ -50,43 +53,51 @@ -(void) purgeOldUser: (NSMutableDictionary *)dictionary {
 }
 
 -(void) saveUser: (LDUserModel *) user {
-    [self saveUser:user asDict:YES];
+    [self saveUser:user asDict:YES completion:nil];
 }
 
 -(void) saveUserDeprecated:(LDUserModel *)user {
-    [self saveUser:user asDict:NO];
+    [self saveUser:user asDict:NO completion:nil];
 }
 
--(void) saveUser:(LDUserModel *)user asDict:(BOOL)asDict {
-    NSMutableDictionary *userDictionary = [self retrieveUserDictionary];
-    if (userDictionary) {
-        LDUserModel *resultUser = [userDictionary objectForKey:user.key];
-        if (resultUser) {
-            // User is found
-            [self compareConfigForUser:resultUser withNewUser:user];
-            resultUser = user;
-            resultUser.updatedAt = [NSDate date];
+-(void) saveUser:(LDUserModel *)user asDict:(BOOL)asDict completion:(void (^)(void))completion {
+    LDUserModel *userCopy = [[LDUserModel alloc] initWithDictionary:[user dictionaryValueWithPrivateAttributesAndFlagConfig:YES]];      //Preserve the user while waiting to save on the saveQueue
+    dispatch_async(saveUserQueue, ^{
+        NSMutableDictionary *userDictionary = [self retrieveUserDictionary];
+        if (userDictionary) {
+            LDUserModel *resultUser = [userDictionary objectForKey:userCopy.key];
+            if (resultUser) {
+                // User is found
+                [self compareConfigForUser:resultUser withNewUser:userCopy];
+                userCopy.updatedAt = [NSDate date];
+                userDictionary[userCopy.key] = userCopy;
+            } else {
+                // User is not found so need to create and purge old users
+                [self compareConfigForUser:nil withNewUser:userCopy];
+                [self purgeOldUser: userDictionary];
+                userCopy.updatedAt = [NSDate date];
+                userDictionary[userCopy.key] = userCopy;
+            }
         } else {
-            // User is not found so need to create and purge old users
-            [self compareConfigForUser:nil withNewUser:user];
-            [self purgeOldUser: userDictionary];
-            user.updatedAt = [NSDate date];
-            [userDictionary setObject:user forKey:user.key];
+            // No Dictionary exists so create
+            [self compareConfigForUser:nil withNewUser:userCopy];
+            userDictionary = [[NSMutableDictionary alloc] init];
+            userDictionary[userCopy.key] = userCopy;
         }
-    } else {
-        // No Dictionary exists so create
-        [self compareConfigForUser:nil withNewUser:user];
-        userDictionary = [[NSMutableDictionary alloc] init];
-        [userDictionary setObject:user forKey:user.key];
-    }
-    [userDictionary setObject:user forKey:user.key];
-    if (asDict) {
-        [self storeUserDictionary:userDictionary];
-    }
-    else{
-        [self deprecatedStoreUserDictionary:userDictionary];
-    }
-
+        userDictionary[userCopy.key] = userCopy;
+        if (asDict) {
+            [self storeUserDictionary:userDictionary];
+        }
+        else{
+            [self deprecatedStoreUserDictionary:userDictionary];
+        }
+        DEBUG_LOG(@"LDDataManager saved user:%@ %@", userCopy.key, userCopy);
+        if (completion != nil) {
+            [NSThread performOnMainThread:^{
+                completion();
+            }];
+        }
+    });
 }
 
 -(LDUserModel *)findUserWithkey: (NSString *)key {
@@ -95,6 +106,7 @@ -(LDUserModel *)findUserWithkey: (NSString *)key {
     if (userDictionary) {
         resultUser = [userDictionary objectForKey:key];
         if (resultUser) {
+            DEBUG_LOG(@"LDDataManager found cached user:%@ %@", resultUser.key, resultUser);
             resultUser.updatedAt = [NSDate date];
         }
     }

DarklyTests/LDClientTest.m

@@ -96,7 +96,6 @@ - (void)setUp {
     OCMStub([self.throttlerMock runThrottled:[OCMArg invokeBlock]]);
     [LDClient sharedInstance].throttler = self.throttlerMock;
 
-
     self.user = [LDUserModel stubWithKey:nil];
 
     self.userBuilderMock = OCMClassMock([LDUserBuilder class]);
@@ -782,31 +781,102 @@ - (void)testStopClient {
     OCMVerifyAll(self.clientManagerMock);
 }
 
-- (void)testUpdateUserWithoutStart {
+- (void)testUpdateUser_withoutStart {
     [[self.clientManagerMock reject] updateUser];
     [[self.dataManagerMock reject] createIdentifyEventWithUser:[OCMArg any] config:[OCMArg any]];
+
     XCTAssertFalse([[LDClient sharedInstance] updateUser:[[LDUserBuilder alloc] init]]);
+
     [self.clientManagerMock verify];
     [self.dataManagerMock verify];
+}
+
+- (void)testUpdateUser_withoutBuilder {
+    [[LDClient sharedInstance] start:self.config withUserBuilder:self.userBuilderMock];
+    [[self.clientManagerMock reject] updateUser];
+    [[self.dataManagerMock reject] createIdentifyEventWithUser:[OCMArg any] config:[OCMArg any]];
+
+    XCTAssertFalse([[LDClient sharedInstance] updateUser:nil]);
+
     [self.clientManagerMock verify];
+    [self.dataManagerMock verify];
 }
 
--(void)testUpdateUserWithStart {
-    LDConfig *config = [[LDConfig alloc] initWithMobileKey:kTestMobileKey];
+-(void)testUpdateUser_sameUser {
+    [[LDClient sharedInstance] start:self.config withUserBuilder:self.userBuilderMock];
+    //Configure the builder with the same user key, and an updated name
+    [[[self.userBuilderMock stub] andReturn:self.user.key] key];
+    NSString *newUserName = @"George Gershwin";
+    [[[self.userBuilderMock stub] andReturn:newUserName] name];
+    [[[self.userBuilderMock expect] andReturn:self.user] compareNewBuilder:[OCMArg checkWithBlock:^BOOL(id obj) {
+        if (![obj isKindOfClass:[LDUserBuilder class]]) {
+            return NO;
+        }
+        LDUserBuilder *userBuilder = obj;
+        self.user.name = userBuilder.name;  //Simulates the compare copying builder properties into the existing user
+        return [userBuilder isEqual:self.userBuilderMock];
+    }] withUser:self.user];
+    [[self.dataManagerMock expect] saveUser:self.user];
     [[self.clientManagerMock expect] updateUser];
-    LDUserBuilder *userBuilder = [[LDUserBuilder alloc] init];
-    userBuilder.key = [[NSUUID UUID] UUIDString];
     [[self.dataManagerMock expect] createIdentifyEventWithUser:[OCMArg checkWithBlock:^BOOL(id obj) {
-        if (![obj isKindOfClass:[LDUserModel class]]) { return NO; }
-        return [((LDUserModel*)obj).key isEqualToString:userBuilder.key];
-    }] config:[OCMArg checkWithBlock:^BOOL(id obj) {
-        if (![obj isKindOfClass:[LDConfig class]]) { return NO; }
-        return [((LDConfig*)obj).mobileKey isEqualToString:config.mobileKey];
-    }]];
-    [[LDClient sharedInstance] start:config withUserBuilder:nil];
+        if (![obj isKindOfClass:[LDUserModel class]]) {
+            return NO;
+        }
+        LDUserModel *user = obj;
+        return [user.key isEqualToString:self.user.key] && [user.name isEqualToString:newUserName];
+    }] config:self.config];
+
+    XCTAssertTrue([[LDClient sharedInstance] updateUser:self.userBuilderMock]);
+
+    [self.userBuilderMock verify];
+    XCTAssertEqualObjects([LDClient sharedInstance].ldUser, self.user);
+    [self.clientManagerMock verify];
+    [self.dataManagerMock verify];
+}
+
+-(void)testUpdateUser_differentUser {
+    [[LDClient sharedInstance] start:self.config withUserBuilder:self.userBuilderMock];
+    LDUserModel *newUser = [LDUserModel stubWithKey:[[NSUUID UUID] UUIDString]];
+    self.userBuilderMock = nil; //Stop the originally set up builder from mocking because it's set to build into the existing user
+    id newUserBuilderMock = [OCMockObject niceMockForClass:[LDUserBuilder class]];
+    [[[newUserBuilderMock expect] andReturn:newUser] build];
+    [[[newUserBuilderMock stub] andReturn:newUser.key] key];    //Give the builder the key for the newUser to trigger the 'different user' part of updateUser
+    [[self.clientManagerMock expect] updateUser];
+    [[self.dataManagerMock expect] createIdentifyEventWithUser:[OCMArg checkWithBlock:^BOOL(id obj) {
+        if (![obj isKindOfClass:[LDUserModel class]]) {
+            return NO;
+        }
+        LDUserModel *user = obj;
+        return [user.key isEqualToString:newUser.key];
+    }] config:self.config];
+
+    XCTAssertTrue([[LDClient sharedInstance] updateUser:newUserBuilderMock]);
+
+    [newUserBuilderMock verify];
+    XCTAssertEqualObjects([LDClient sharedInstance].ldUser, newUser);
+    [self.clientManagerMock verify];
+    [self.dataManagerMock verify];
+}
+
+-(void)testUpdateUser_differentUser_builderKeyOmitted {
+    [[LDClient sharedInstance] start:self.config withUserBuilder:self.userBuilderMock];
+    LDUserModel *newUser = [LDUserModel stubWithKey:[[NSUUID UUID] UUIDString]];
+    self.userBuilderMock = nil; //Stop the originally set up builder from mocking because it's set to build into the existing user. Omit the key to trigger the 'different user' part of updateUser
+    id newUserBuilderMock = [OCMockObject niceMockForClass:[LDUserBuilder class]];
+    [[[newUserBuilderMock expect] andReturn:newUser] build];
+    [[self.clientManagerMock expect] updateUser];
+    [[self.dataManagerMock expect] createIdentifyEventWithUser:[OCMArg checkWithBlock:^BOOL(id obj) {
+        if (![obj isKindOfClass:[LDUserModel class]]) {
+            return NO;
+        }
+        LDUserModel *user = obj;
+        return [user.key isEqualToString:newUser.key];
+    }] config:self.config];
 
-    XCTAssertTrue([[LDClient sharedInstance] updateUser:userBuilder]);
+    XCTAssertTrue([[LDClient sharedInstance] updateUser:newUserBuilderMock]);
 
+    [newUserBuilderMock verify];
+    XCTAssertEqualObjects([LDClient sharedInstance].ldUser, newUser);
     [self.clientManagerMock verify];
     [self.dataManagerMock verify];
 }

DarklyTests/Models/LDDataManagerTest.m

@@ -27,6 +27,10 @@
 
 NSString * const kMobileKeyMock = @"LDDataManagerTest.mobileKeyMock";
 
+@interface LDDataManager (LDDataManagerTest)
+-(void)saveUser:(LDUserModel*)user asDict:(BOOL)asDict completion:(void (^)(void))completion;
+@end
+
 @interface LDDataManagerTest : DarklyXCTestCase
 @property (nonatomic, strong) id clientMock;
 @property (nonatomic, strong) id eventModelMock;
@@ -412,50 +416,60 @@ -(void)testAllEventsDictionaryArray {
     [self waitForExpectations:@[expectation] timeout:1];
 }
 
--(void)testFindOrCreateUser {
-    NSString *userKey = @"thisisgus";
-    LDUserModel *aUser = [[LDUserModel alloc] init];
-    aUser.key = userKey;
-    aUser.email = @"gus@anemail.com";
-    aUser.updatedAt = [NSDate date];
-    aUser.flagConfig = user.flagConfig;
-    [[LDDataManager sharedManager] saveUser: aUser];
-
-    LDUserModel *foundAgainUser = [[LDDataManager sharedManager] findUserWithkey: userKey];
+-(void)testSaveAndFindUser {
+    XCTestExpectation *userSavedExpectation = [self expectationWithDescription:@"LDDataManagerTest.testSaveAndFindUser.userSavedExpectation"];
+
+    [[LDDataManager sharedManager] saveUser:self.user asDict:YES completion:^{
+        [userSavedExpectation fulfill];
+    }];
+    [self waitForExpectations:@[userSavedExpectation] timeout:1.0];
+    LDUserModel *foundUser = [[LDDataManager sharedManager] findUserWithkey:self.user.key];
 
-    XCTAssertNotNil(foundAgainUser);
-    XCTAssertEqualObjects(aUser.email, foundAgainUser.email);
+    XCTAssertNotNil(foundUser);
+    XCTAssertTrue([foundUser isEqual:self.user ignoringAttributes:@[kUserAttributeUpdatedAt]]);
 }
 
--(void) testPurgeUsers {
-    NSString *baseUserKey = @"gus";
-    NSString *baseUserEmail = @"gus@email.com";
-
+-(void)testSaveAndFindUser_backwardCompatibility {
+    XCTestExpectation *userSavedExpectation = [self expectationWithDescription:@"LDDataManagerTest.testSaveAndFindUser.userSavedExpectation"];
+
+    [[LDDataManager sharedManager] saveUser:self.user asDict:NO completion:^{
+        [userSavedExpectation fulfill];
+    }];
+    [self waitForExpectations:@[userSavedExpectation] timeout:1.0];
+    LDUserModel *foundUser = [[LDDataManager sharedManager] findUserWithkey:self.user.key];
+
+    XCTAssertNotNil(foundUser);
+    XCTAssertTrue([foundUser isEqual:self.user ignoringAttributes:@[kUserAttributeUpdatedAt]]);
+}
+
+-(void)testSaveAndFindUsers_overCapacity {
+    NSMutableArray *users = [NSMutableArray arrayWithCapacity:kUserCacheSize + 3];
     for(int index = 0; index < kUserCacheSize + 3; index++) {
-        LDUserModel *aUser = [[LDUserModel alloc] init];
-        aUser.key = [NSString stringWithFormat: @"%@%d", baseUserKey, index];
-        aUser.email = [NSString stringWithFormat: @"%@%d", baseUserEmail, index];;
-
-        NSTimeInterval secondsInXHours = (index+1) * 60 * 60 * 24;
-        NSDate *dateInXHours = [[NSDate date] dateByAddingTimeInterval:secondsInXHours];
-        aUser.updatedAt = dateInXHours;
-
-        [[LDDataManager sharedManager] saveUser: aUser];
+        LDUserModel *user = [LDUserModel stubWithKey:[[NSUUID UUID] UUIDString]];
+        //Keep users ordered most recent first
+        if (users.count == 0) {
+            [users addObject:user];
+        } else {
+            [users insertObject:user atIndex:0];
+        }
+        XCTestExpectation *userSavedExpectation = [self expectationWithDescription:[NSString stringWithFormat:@"LDDataManagerTest.testSaveAndFindUser.userSavedExpectation.%d", index]];
+
+        [[LDDataManager sharedManager] saveUser:user asDict:YES completion:^{
+            [userSavedExpectation fulfill];
+        }];
+
+        [self waitForExpectations:@[userSavedExpectation] timeout:1.0];
+        for(int usersIndex = 0; usersIndex < users.count; usersIndex++) {
+            LDUserModel *targetUser = users[usersIndex];
+            LDUserModel *foundUser = [[LDDataManager sharedManager] findUserWithkey:targetUser.key];
+            if (usersIndex < kUserCacheSize) {
+                XCTAssertNotNil(foundUser);
+                XCTAssertTrue([foundUser isEqual:targetUser ignoringAttributes:@[kUserAttributeUpdatedAt]]);
+            } else {
+                XCTAssertNil(foundUser);
+            }
+        }
     }
-
-    XCTAssertEqual([[[LDDataManager sharedManager] retrieveUserDictionary] count],kUserCacheSize);
-    NSString *firstCreatedKey = [NSString stringWithFormat: @"%@%d", baseUserKey, 0];
-    LDUserModel *firstCreatedUser = [[LDDataManager sharedManager] findUserWithkey:firstCreatedKey];
-    XCTAssertNil(firstCreatedUser);
-    NSString *secondCreatedKey = [NSString stringWithFormat: @"%@%d", baseUserKey, 1];
-    LDUserModel *secondCreatedUser = [[LDDataManager sharedManager] findUserWithkey:secondCreatedKey];
-    XCTAssertNil(secondCreatedUser);
-    NSString *thirdCreatedKey = [NSString stringWithFormat: @"%@%d", baseUserKey, 2];
-    LDUserModel *thirdCreatedUser = [[LDDataManager sharedManager] findUserWithkey:thirdCreatedKey];
-    XCTAssertNil(thirdCreatedUser);
-    NSString *fourthCreatedKey = [NSString stringWithFormat: @"%@%d", baseUserKey, 3];
-    LDUserModel *fourthCreatedUser = [[LDDataManager sharedManager] findUserWithkey:fourthCreatedKey];
-    XCTAssertNotNil(fourthCreatedUser);
 }
 
 -(void)testCreateEventAfterCapacityReached {