-
Notifications
You must be signed in to change notification settings - Fork 316
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
identity v3 #438
identity v3 #438
Conversation
68d9a08
to
7f76486
Compare
a96f970
to
3f1c121
Compare
Purchases/Caching/RCDeviceCache.m
Outdated
|
||
[self clearLatestNetworkAndAdvertisingIdsSentForAppUserID:oldAppUserID]; |
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 did this because I figured if we're clearing caches, we should probably delete everything about the old user, but... this might be a problem because we call this method even if the user gets aliased, so if the user doesn't change, we might still be deleting this
#import <Foundation/Foundation.h> | ||
#import "RCDeviceCache.h" | ||
|
||
@class RCPurchaserInfo, RCPurchaserInfoManager, RCBackend; |
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 class is mostly just the same as before, it was moved into the Identity
group
- (void)logInAppUserID:(NSString *)newAppUserID | ||
completionBlock:(void (^)(RCPurchaserInfo * _Nullable purchaserInfo, | ||
BOOL created, | ||
NSError *error))completion; | ||
|
||
- (void)logOutWithCompletionBlock:(void (^)(NSError * _Nullable error))completion; |
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.
new methods: logIn
, logOut
#pragma MARK - deprecated methods | ||
|
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.
added the deprecated pragma, but since these are the internal methods, I didn't actually mark them deprecated otherwise we'd get a compiler warning ourselves
@@ -51,10 +55,10 @@ - (void)configureWithAppUserID:(nullable NSString *)appUserID { | |||
[self.deviceCache cleanupSubscriberAttributes]; | |||
} | |||
|
|||
- (void)identifyAppUserID:(NSString *)appUserID withCompletionBlock:(void (^)(NSError *_Nullable error))completion { | |||
- (void)identifyAppUserID:(NSString *)appUserID completionBlock:(void (^)(NSError *_Nullable error))completion { |
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.
mild rename because you're only supposed to reference the first param in objc, after that you can just use the name
Purchases/Public/RCPurchasesErrors.h
Outdated
@@ -52,6 +52,7 @@ typedef NS_ERROR_ENUM(RCPurchasesErrorDomain, RCPurchasesErrorCode) { | |||
RCInsufficientPermissionsError, | |||
RCPaymentPendingError, | |||
RCInvalidSubscriberAttributesError, | |||
RCLogOutAnonymousUserError, |
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.
will update this to add a number after the #452 stuff gets merged
@@ -1,37 +0,0 @@ | |||
// |
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.
again, this wasn't removed, it was moved to the Identity
group / folder
@@ -0,0 +1,420 @@ | |||
// |
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.
tests start here
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.
the ones in this class aren't fully new, the file was moved into the Identity
group. I'll point out the new ones.
assertCorrectlyIdentified(expectedAppUserID: "cesar") | ||
} | ||
|
||
func testLogInFailsIfEmptyAppUserID() { |
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.
new tests start here
- (void)cachePurchaserInfo:(RCPurchaserInfo *)info forAppUserID:(NSString *)appUserID { | ||
if (info) { | ||
NSDictionary *infoAsJSONDict = info.JSONObject; | ||
if ([NSJSONSerialization isValidJSONObject:infoAsJSONDict]) { |
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 check is new. previously, if there was an error parsing the JSON (for example if the json has a value like infinity
), the app would crash.
the jsonError
bit only helps for errors like incomplete stream and stuff like that, but it for some reason doesn't actually cover what would happen with an invalid json.
35de297
to
c662650
Compare
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.
Haven't finished yet but I have some comments
NSString *currentAppUserID = self.currentAppUserID; | ||
if (!currentAppUserID) { | ||
RCWarnLog(@"%@", RCStrings.identity.logging_in_with_initial_appuserid_nil); | ||
} |
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.
Not that I don't like this, but this should technically not happen right? Just checking
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.
yeah, this should never happen. In other cases like this, we would just crash. But this is the one instance where if it did happen, this method would actually make the SDK recover, so I figured I might as well continue execution if it does happen.
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.
come to think about this again, we had issues in the past with aliasing to nil #255, so I'll give this some more thought
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.
updated so that it errors out now, like with identity v2
|
||
NSString *currentAppUserID = self.currentAppUserID; | ||
if (!currentAppUserID) { | ||
RCWarnLog(@"%@", RCStrings.identity.logging_in_with_initial_appuserid_nil); |
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 this string should be creating_alias_failed_null_currentappuserid
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 is just the identityV3 version of the same string
d34e785
to
c3a6f58
Compare
c3a6f58
to
c6d18fa
Compare
@vegaro bump |
} | ||
|
||
[self.systemInfo isApplicationBackgroundedWithCompletion:^(BOOL isAppBackgrounded) { | ||
BOOL shouldCallCompletionAfterFetching = infoFromCache == nil && completion != nil; |
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 had some trouble understanding this part. I didn't understand why you were not just passing completion to fetchAndCachePurchaserInfoIfStaleWithAppUserID
even if it's nil. I now get that it's to avoid the completion being called again if it was called already in line 167, but it took me a bit.
I was thinking maybe the check for [self.systemInfo isApplicationBackgroundedWithCompletion
could be done in fetchAndCachePurchaserInfoIfStaleWithAppUserID
. Then calling fetchAndCachePurchaserInfoIfStaleWithAppUserID
in line 169 passing a nil completion.
This is just a suggestion including my personal preference, it is not necessarily the way I think this method should be written. If you think your current implementation is clear, leave it as is :)
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.
Agreed, this looks pretty confusing
I did some cleanup here: I added an explicit boolean to store whether we've already called completion, and added a comment. lmk if it feels better! Doing the isAppBackgrounded check in fetchAndCachePurchaserInfoIfStaleWithAppUserID
is a slightly larger refactor, but I'm happy to do it if the new logic is still confusing
45c9647
to
547efd2
Compare
} | ||
|
||
// prevent calling completion twice | ||
RCReceivePurchaserInfoBlock _Nullable fetchPurchaserInfoCompletion = completionCalled ? nil : completion; |
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.
Thanks for changing this!
…ods on the main thread
…t now errors out instead of continuing
… returning from cache
updated purchaserInfo data key in login: "subscriber_data" -> "subscriber"
1196b7c
to
404edf4
Compare
merging this since it's approved, but I'm going to follow up to update the location of the appUserID param for login calls |
Introduces a new take on identity.
Public changes overview
New methods
logIn
, a new way of identifying users, which also returns whether a new user has been registered in the system.logIn
uses a new backend endpoint.logOut
, a replacement forreset
.Deprecations / removals
createAlias
identify
in favor oflogIn
reset
in favor oflogOut
allowSharingAppStoreAccount
in favor of dashboard-side configurationReview notes
RCPurchaserInfoManager
, which takes on the responsibility of sending information to the delegate, knowing what has last been sent, serializing purchaserInfo for caching, and everything else purchaser-info related.develop
before the backend is ready.