Skip to content
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

Update offerings cache when switchUser(to:) is called #2455

Merged
merged 7 commits into from
Apr 27, 2023

Conversation

aboedo
Copy link
Member

@aboedo aboedo commented Apr 26, 2023

Added updating of offerings cache when switchUser(to:) is called.
Also added checking that the new appUserID != the current one and no-op if it is.

@aboedo aboedo added the perf label Apr 26, 2023
@aboedo aboedo requested a review from a team April 26, 2023 22:19
@aboedo aboedo self-assigned this Apr 26, 2023
Logger.debug(Strings.identity.switching_user(newUserId: newAppUserID))
Logger.debug(Strings.identity.switching_user(newUserID: newAppUserID))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

renamed param for consistency with others

Comment on lines 698 to 707
guard self.identityManager.currentAppUserID != newAppUserID else {
self.logger.warn(Strings.identity.switching_user_same_app_user_id(newUserID: newAppUserID))
return
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we missed this in the original implementation but it becomes a lot more important when a cache update is added to the mix

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏻

func testRefreshesOfferingsCache() {
expect(self.mockOfferingsManager.invokedUpdateOfferingsCacheCount) == 0

self.systemInfo = MockSystemInfo(finishTransactions: true, customEntitlementsComputation: true)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This initialization is duplicated in several tests now, might be worth extracting.

}

func testSwitchUserNoOpIfAppUserIDIsSameAsCurrent() {
self.systemInfo = MockSystemInfo(finishTransactions: true, customEntitlementsComputation: true)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto duplicated initialization.

@aboedo
Copy link
Member Author

aboedo commented Apr 27, 2023

@NachoSoto I ended up going with a slightly simpler path. And... I think we might wanna do this a bit more tbh, it feels a lot simpler to manage.
Just have everything in internal methods, use compiler directive to decide whether to include a public version of the same method.
Thoughts?

@aboedo aboedo enabled auto-merge (squash) April 27, 2023 17:37
Copy link
Contributor

@NachoSoto NachoSoto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great idea! I like the simple approach. I think tests should catch that this is backwards though :)


#if !ENABLE_CUSTOM_ENTITLEMENT_COMPUTATION
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is backwards?

Copy link
Member Author

@aboedo aboedo Apr 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LOL I shouldn't be allowed to code. Or at least not automerge

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@NachoSoto
Copy link
Contributor

Oh they don't yet because APITesters aren't merged.

@NachoSoto NachoSoto disabled auto-merge April 27, 2023 17:44
@NachoSoto
Copy link
Contributor

I disabled auto merge just in case.

///
/// Updates the current appUserID to a new one, without associating the two.
/// - Important: This method is **only available** in Custom Entitlements Computation mode.
/// Receipts posted by the SDK to the RevenueCat backend after calling this method will be sent
/// with the newAppUserID.
///
@objc(switchUserToNewAppUserID:)
func switchUser(to newAppUserID: String) {
public func switchUser(to newAppUserID: String) {
internalSwitchUser(to: newAppUserID)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit:

Suggested change
internalSwitchUser(to: newAppUserID)
self.internalSwitchUser(to: newAppUserID)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, woops, forgot

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added

@aboedo
Copy link
Member Author

aboedo commented Apr 27, 2023

@NachoSoto thanks for the catches! Updated

@aboedo aboedo force-pushed the andy/sdk-3089-refresh-offerings-cache-after branch from 440123a to 6e41333 Compare April 27, 2023 17:49
@aboedo aboedo merged commit 9e373c3 into main Apr 27, 2023
@aboedo aboedo deleted the andy/sdk-3089-refresh-offerings-cache-after branch April 27, 2023 18:19
NachoSoto pushed a commit that referenced this pull request Apr 27, 2023
Added updating of offerings cache when `switchUser(to:)` is called.
Also added checking that the new appUserID != the current one and no-op
if it is.
@vegaro vegaro added pr:other and removed pr:perf labels Sep 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants