[final] fix: subscriber attributes deleted when aliasing #222

aboedo · 2020-04-09T18:34:08Z

Fixes a bug where subscriber attributes are deleted when an alias is created.
This was happening because we clear everything in user defaults when creating an alias.

This PR ensures that instead of deleting subscriber attributes, we transfer them over to the new user, if any exist.

I tested this locally against the production server by:

Creating subscriber attributes
calling create alias
checking the shared preferences .plist file to see its contents.

Before	After
The key for subscriber attributes was deleted, along with its contents	There's a new key for subscriber attributes with the alias, its contents are the same as the ones for the old user

This also means changing the format in which subscriber attributes are stored.

Before:

com.revenuecat.subscriberAttributes.<user_id>: {
    attribute_1:
        value: something,
        synced: true
        ...
}

After:

com.revenuecat.subscriberAttributes: {
    <user_id>:
        attribute_1:
            value: something,
            synced: true
            ...
}

To do:

Write unit tests that repro the bug and check for the fix
Update existing unit tests to reflect new behavior

vegaro · 2020-04-09T18:42:22Z

Two thoughts:

Don't we have the same issue with just an identify (no alias created). It is possible we identify the new user but the attributes for the previous user have not been sent and get deleted.
How is transfering different to not using appUserID as part of the user defaults key for the subscriber attributes? Isn't the outcome the same?

…he ones for other users if they're completely synced

…kground

…d tests

…ts own file

…ng returned

aboedo · 2020-04-20T22:54:21Z

PurchasesTests/Caching/DeviceCacheTests.swift

-
-    // MARK: Subscriber Attributes


moved these to a new file

aboedo · 2020-04-20T22:57:41Z

@vegaro this is ready for another pass

aboedo · 2020-04-21T19:34:40Z

Purchases/Caching/RCDeviceCache.m

@@ -104,6 +115,10 @@ - (void)setPurchaserInfoCacheTimestampToNow {
    self.purchaserInfoCachesLastUpdated = [NSDate date];
 }

+- (NSString *)purchaserInfoUserDefaultCacheKeyForAppUserID:(NSString *)appUserID {


this one and clearOfferingsCache were just moved for organizational purposes

aboedo · 2020-04-21T20:17:08Z

found a bug, moving back to WIP

vegaro · 2020-04-21T19:33:47Z

Purchases/Caching/RCDeviceCache.m

+    [self.offeringsCachedObject clearCache];
+}
+
+#pragma mark - subscriber attributes

 - (void)storeSubscriberAttribute:(RCSubscriberAttribute *)attribute appUserID:(NSString *)appUserID {


Maybe put appUserID parameter in next line for consistency with the next function

vegaro · 2020-04-21T19:43:54Z

Purchases/Caching/RCDeviceCache.m

+        subscriberAttributesForAppUserID[attribute.key] = attribute.asDictionary;
+        groupedSubscriberAttributes[appUserID] = subscriberAttributesForAppUserID;
+        [self.userDefaults setObject:groupedSubscriberAttributes
+                              forKey:RCSubscriberAttributesKey];


I think you could remove the logic in this fucntion and reuse the other one by doing something like:

Suggested change

forKey:RCSubscriberAttributesKey];

[self.storeSubscriberAttributes:@{attribute.key: attribute} appUserID: appUserID

vegaro · 2020-04-21T19:46:09Z

Purchases/Caching/RCDeviceCache.m

-                                                                ? allSubscriberAttributesByKey.mutableCopy
-                                                                : [[NSMutableDictionary alloc] init];
+        NSMutableDictionary *groupedSubscriberAttributes = self.unsyncedAttributesForAllUsersAsDict.mutableCopy;
+        NSMutableDictionary *subscriberAttributesForAppUserID = ((NSDictionary *)groupedSubscriberAttributes[appUserID] ?: @{})


You can reuse subscriberAttributesForAppUserID. Although it would call self. unsyncedAttributesForAllUsersAsDict again

Suggested change

NSMutableDictionary *subscriberAttributesForAppUserID = ((NSDictionary *)groupedSubscriberAttributes[appUserID] ?: @{})

NSMutableDictionary *subscriberAttributesForAppUserID = [self subscriberAttributesForAppUserID].mutableCopy;

I kinda wanted to avoid an extra read, if possible

…ynced attributes

… correctly

vegaro · 2020-04-22T03:52:08Z

Purchases/SubscriberAttributes/RCSubscriberAttributesManager.m

 #pragma MARK - Private methods

 - (void)setAttributeWithKey:(NSString *)key value:(nullable NSString *)value appUserID:(NSString *)appUserID {
    [self storeAttributeLocallyIfNeededWithKey:key value:value appUserID:appUserID];
 }

- (void)syncAttributesWithCompletion:(void (^)(NSError *_Nullable error))completion appUserID:(NSString *)appUserID {
+


vegaro · 2020-04-22T03:56:33Z

Purchases/SubscriberAttributes/RCSubscriberAttributesManager.m

    } else {
        completion(nil);
    }
 }

+- (void)syncAttributesForAllUsersWithCurrentAppUserID:(NSString *)currentAppUserID {
+    NSDictionary < NSString * , RCSubscriberAttributeDict > *unsyncedAttributesForAllUsers =


Suggested change

NSDictionary < NSString * , RCSubscriberAttributeDict > *unsyncedAttributesForAllUsers =

NSDictionary <NSString *, RCSubscriberAttributeDict> *unsyncedAttributesForAllUsers =

good catch, no clue what happened with formatting there

vegaro · 2020-04-22T04:00:33Z

Purchases/SubscriberAttributes/RCSubscriberAttributesManager.m

+                          [self.deviceCache deleteAttributesIfSyncedForAppUserID:syncingAppUserID];
+                          RCLog(@"Subscriber attributes synced successfully for appUserID: %@", syncingAppUserID);
+                      } else {
+                          RCErrorLog(@"error when syncing subscriber attributes. Details: %@\n UserInfo:%@",


If error is nil but syncingAppUserID == currentAppUserID it will also print this error right?

good catch. there was also a problem if where we wouldn't log success if the current app user id was the one being synced. fixed and extracted to a separate method

vegaro · 2020-04-22T04:05:57Z

Purchases/Caching/RCDeviceCache.m

+        }
+        NSMutableDictionary <NSString *, NSDictionary *>
+            *groupedAttributes = self.storedAttributesForAllUsers.mutableCopy;
+        [groupedAttributes removeObjectForKey:appUserID];


We are removing all attributes for that user id. We can be sure they have been synced right?

they should be - we check that numberOfUnsyncedAttributesForAppUserID == 0 a couple of lines before doing this

oohhh got it

aboedo · 2020-04-22T21:19:21Z

@vegaro ready for another pass

vegaro · 2020-04-23T00:47:26Z

Purchases/Caching/RCDeviceCache.m

+        NSMutableDictionary *groupedSubscriberAttributes = self.storedAttributesForAllUsers.mutableCopy;
+        [groupedSubscriberAttributes removeObjectForKey:appUserID];
+        [self.userDefaults setObject:groupedSubscriberAttributes forKey:RCSubscriberAttributesKey];
+    }


We should probably synchronize this right?

yep! great catch

vegaro · 2020-04-23T00:50:47Z

Purchases/Caching/RCDeviceCache.m

+        }
+        NSMutableDictionary <NSString *, NSDictionary *>
+            *groupedAttributes = self.storedAttributesForAllUsers.mutableCopy;
+        [groupedAttributes removeObjectForKey:appUserID];


oohhh got it

vegaro

Not sure if this was waiting for another review to get shipped :)

aboedo added the WIP label Apr 9, 2020

aboedo requested review from vegaro and rkotzy April 9, 2020 18:34

aboedo self-assigned this Apr 9, 2020

aboedo added 3 commits April 9, 2020 18:15

added code to migrate to the new subscriber attributes format

a82d5f5

added migration step, small updates to header file

6d050d7

updated subscriber attributes to use the new format

05aa509

aboedo force-pushed the fix/subscriber_attributes_deleted_after_aliasing branch from b6cee53 to 05aa509 Compare April 9, 2020 22:57

aboedo added 4 commits April 10, 2020 18:49

fixed some tests, fixed typing of NSDictionary

0770b26

added method to post subscriber attributes for all users and delete t…

0a096da

…he ones for other users if they're completely synced

added syncing subscriber attributes for all users on foreground / bac…

298238c

…kground

added better logging

5771057

aboedo marked this pull request as draft April 13, 2020 14:43

aboedo added 9 commits April 15, 2020 17:41

added deletion of the old data after the migration

e2d30c8

fixed subscriber attributes manager tests

1fe5f51

fixed device cache tests

42a56ba

updated identity manager tests

a8e49ce

fixed a couple of bugs in subscriberAttributesManager's syncing, adde…

0db50dd

…d tests

separated subscriber-attributes-related tests for device cache into i…

2b86531

…ts own file

added more tests and fixed a bug where the wrong type of dict was bei…

2f00b98

…ng returned

small cleanup

94545e5

NSLog -> RCLog

a1dfd17

aboedo commented Apr 20, 2020

View reviewed changes

aboedo added final and removed WIP labels Apr 20, 2020

aboedo changed the title ~~[WIP] fix: subscriber attributes deleted when aliasing~~ [final] fix: subscriber attributes deleted when aliasing Apr 20, 2020

aboedo marked this pull request as ready for review April 21, 2020 14:28

aboedo commented Apr 21, 2020

View reviewed changes

aboedo added WIP and removed final labels Apr 21, 2020

aboedo marked this pull request as draft April 21, 2020 20:17

vegaro reviewed Apr 21, 2020

View reviewed changes

aboedo added 2 commits April 21, 2020 17:31

fixed a bug where unsyncedAttributes wasn't correctly filtering out s…

6aaf9cf

…ynced attributes

added extra test to make sure that synced attributes are filtered out…

57ac6b5

… correctly

aboedo added final and removed WIP labels Apr 21, 2020

aboedo marked this pull request as ready for review April 21, 2020 20:46

vegaro reviewed Apr 22, 2020

View reviewed changes

improvements from PR comments

c9f2c1f

vegaro approved these changes Apr 23, 2020

View reviewed changes

aboedo added 4 commits April 23, 2020 12:40

added @synced to cache deletion methods, minor re-shuffling

f3576b8

removed redundant method

df20ac6

small cleanup

56abbc9

fix test method name

6fb9c8a

vegaro approved these changes Apr 27, 2020

View reviewed changes

aboedo merged commit 2f15db5 into master Apr 29, 2020

aboedo deleted the fix/subscriber_attributes_deleted_after_aliasing branch April 29, 2020 23:06

aboedo mentioned this pull request May 1, 2020

bump to version 3.2.3 #229

Closed

aboedo mentioned this pull request May 13, 2020

version 3.3.0 #243

Merged

aboedo mentioned this pull request Jul 13, 2021

Release: 3.12.0 #648

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[final] fix: subscriber attributes deleted when aliasing #222

[final] fix: subscriber attributes deleted when aliasing #222

aboedo commented Apr 9, 2020 •

edited

vegaro commented Apr 9, 2020

aboedo Apr 20, 2020

aboedo commented Apr 20, 2020

aboedo Apr 21, 2020

aboedo commented Apr 21, 2020

vegaro Apr 21, 2020

vegaro Apr 21, 2020

aboedo Apr 22, 2020

vegaro Apr 21, 2020

aboedo Apr 22, 2020

vegaro Apr 22, 2020

vegaro Apr 22, 2020

aboedo Apr 22, 2020

vegaro Apr 22, 2020

aboedo Apr 22, 2020

vegaro Apr 22, 2020

aboedo Apr 22, 2020

vegaro Apr 23, 2020

aboedo commented Apr 22, 2020

vegaro Apr 23, 2020

aboedo Apr 23, 2020

aboedo Apr 23, 2020

vegaro Apr 23, 2020

vegaro left a comment

	forKey:RCSubscriberAttributesKey];
	[self.storeSubscriberAttributes:@{attribute.key: attribute} appUserID: appUserID

	NSMutableDictionary subscriberAttributesForAppUserID = ((NSDictionary )groupedSubscriberAttributes[appUserID] ?: @{})
	NSMutableDictionary *subscriberAttributesForAppUserID = [self subscriberAttributesForAppUserID].mutableCopy;

	NSDictionary < NSString * , RCSubscriberAttributeDict > *unsyncedAttributesForAllUsers =
	NSDictionary <NSString , RCSubscriberAttributeDict> unsyncedAttributesForAllUsers =

[final] fix: subscriber attributes deleted when aliasing #222

[final] fix: subscriber attributes deleted when aliasing #222

Conversation

aboedo commented Apr 9, 2020 • edited

To do:

vegaro commented Apr 9, 2020

Choose a reason for hiding this comment

aboedo commented Apr 20, 2020

Choose a reason for hiding this comment

aboedo commented Apr 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aboedo commented Apr 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vegaro left a comment

Choose a reason for hiding this comment

aboedo commented Apr 9, 2020 •

edited