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

Purchases: don't clear intro eligibility / purchased products cache on first launch #3067

Merged
merged 2 commits into from
Aug 30, 2023

Conversation

NachoSoto
Copy link
Contributor

These caches are important, especially for RevenueCatUI.
Without this fix, launching the app was doing this:

  • Pre-warming cache
  • Pre-warming cache a second time (to be fixed by a separate PR)
  • Clearing cache

Which meant that the cache wasn't really warm when launching paywalls.

To fix that, this only clears the cache after receiving an actual change.

I've also removed CustomerInfoManager.sendCachedCustomerInfoIfAvailable because it wasn't used.

@NachoSoto NachoSoto added the perf A code change that improves performance label Aug 23, 2023
@NachoSoto NachoSoto requested a review from a team August 23, 2023 20:34
@NachoSoto NachoSoto force-pushed the invalidate-caches-only-if-old-customer-info branch 2 times, most recently from f3fecaa to c92bf68 Compare August 24, 2023 18:26
@@ -100,14 +98,6 @@ class CustomerInfoManager {
}
}

func sendCachedCustomerInfoIfAvailable(appUserID: String) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This wasn't being used by anything other than in tests.

Copy link
Member

Choose a reason for hiding this comment

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

interesting! I guess it's a holder from this: #1828

@@ -19,8 +19,6 @@ class CustomerInfoManager {

typealias CustomerInfoCompletion = @MainActor @Sendable (Result<CustomerInfo, BackendError>) -> Void

var lastSentCustomerInfo: CustomerInfo? { return self.data.value.lastSentCustomerInfo }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was only visible for tests so I moved it below.

@NachoSoto NachoSoto force-pushed the invalidate-caches-only-if-old-customer-info branch 2 times, most recently from f5705fa to 5ebd497 Compare August 24, 2023 18:28
self.purchasedProductsFetcher?.clearCache()
self.delegate?.purchases?(self, receivedUpdated: customerInfo)
func handleCustomerInfoChanged(from old: CustomerInfo?, to new: CustomerInfo) {
if old != nil {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the core of this change. We no longer clear the caches for the first time we receive a customer info change.

@@ -196,51 +207,10 @@ class CustomerInfoManagerTests: BaseCustomerInfoManagerTests {
expect(self.mockBackend.invokedGetSubscriberDataCount) == 1
}

func testSendCachedCustomerInfoIfAvailableForAppUserIDSendsIfNeverSent() throws {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed all these tests for the unused method.

expect(self.customerInfoManagerLastCustomerInfoChange) == (old: nil, new: self.mockCustomerInfo)
}

func testCacheCustomerInfoSendsMultipleUpdatesIfChange() throws {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We didn't have this test.

expect(self.customerInfoManagerLastCustomerInfoChange) == (old: nil, new: info)
}

func testCacheCustomerInfoSendsToDelegateAfterCachingComputedOnDevice() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The first implementation broke integration tests in a way that this test now captures.

@@ -215,7 +205,12 @@ class CustomerInfoManager {
func clearCustomerInfoCache(forAppUserID appUserID: String) {
self.modifyData {
$0.deviceCache.clearCustomerInfoCache(appUserID: appUserID)
$0.lastSentCustomerInfo = nil
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This didn't make sense. It didn't matter because the property wasn't used by anything, but now this is used for sending the "old" value. Resetting this broke an integration test, which is what I captured in the new testCacheCustomerInfoSendsToDelegateAfterCachingComputedOnDevice test.

Comment on lines +227 to +240
func testFirstInitializationDoesNotClearIntroEligibilityCache() {
self.setupPurchases()
expect(self.purchasesDelegate.customerInfoReceivedCount).toEventually(equal(1))

expect(self.cachingTrialOrIntroPriceEligibilityChecker.invokedClearCache) == false
}

func testFirstInitializationDoesNotClearPurchasedProductsCache() {
self.setupPurchases()
expect(self.purchasesDelegate.customerInfoReceivedCount).toEventually(equal(1))

expect(self.mockPurchasedProductsFetcher.invokedClearCache) == false
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tests to capture the new behavior.

@NachoSoto NachoSoto requested a review from aboedo August 24, 2023 20:28
@NachoSoto NachoSoto assigned NachoSoto and aboedo and unassigned NachoSoto Aug 24, 2023
… on first launch

These caches are important, especially for `RevenueCatUI`.
Without this fix, launching the app was doing this:
- Pre-warming cache
- Pre-warming cache a second time (to be fixed by a separate PR)
- Clearing cache

Which meant that the cache wasn't really warm when launching paywalls.

To fix that, this only clears the cache after receiving an actual change.

I've also removed `CustomerInfoManager.sendCachedCustomerInfoIfAvailable` because it wasn't used.
@NachoSoto NachoSoto force-pushed the invalidate-caches-only-if-old-customer-info branch from 5ebd497 to 5392d52 Compare August 24, 2023 22:30
@codecov
Copy link

codecov bot commented Aug 24, 2023

Codecov Report

Merging #3067 (5392d52) into main (d4145e6) will increase coverage by 0.05%.
The diff coverage is 96.00%.

@@            Coverage Diff             @@
##             main    #3067      +/-   ##
==========================================
+ Coverage   86.58%   86.64%   +0.05%     
==========================================
  Files         219      219              
  Lines       15674    15676       +2     
==========================================
+ Hits        13572    13582      +10     
+ Misses       2102     2094       -8     
Files Changed Coverage Δ
Sources/Identity/CustomerInfoManager.swift 95.04% <93.33%> (+0.27%) ⬆️
Sources/Purchasing/Purchases/Purchases.swift 75.93% <100.00%> (+0.11%) ⬆️

... and 3 files with indirect coverage changes

Copy link
Member

@aboedo aboedo left a comment

Choose a reason for hiding this comment

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

this reminds me of this old idea: https://linear.app/revenuecat/issue/SDK-2871/new-customerinfo-listener-with-diff, feels like this change would make that pretty much trivial?

@NachoSoto
Copy link
Contributor Author

Oh that's right 😇 this paves the way for that.

@NachoSoto NachoSoto merged commit cdcd1a0 into main Aug 30, 2023
17 checks passed
@NachoSoto NachoSoto deleted the invalidate-caches-only-if-old-customer-info branch August 30, 2023 22:38
NachoSoto added a commit that referenced this pull request Aug 31, 2023
… on first launch (#3067)

These caches are important, especially for `RevenueCatUI`.
Without this fix, launching the app was doing this:
- Pre-warming cache
- Pre-warming cache a second time (to be fixed by a separate PR)
- Clearing cache

Which meant that the cache wasn't really warm when launching paywalls.

To fix that, this only clears the cache after receiving an actual
change.

I've also removed
`CustomerInfoManager.sendCachedCustomerInfoIfAvailable` because it
wasn't used.
NachoSoto pushed a commit that referenced this pull request Sep 5, 2023
**This is an automatic release.**

### Bugfixes
* `DebugViewModel`: fixed runtime crash on iOS < 16 (#3139) via
NachoSoto (@NachoSoto)
### Performance Improvements
* `PurchasesOrchestrator`: return early if receipt has no transactions
when checking for promo offers (#3123) via Mark Villacampa
(@MarkVillacampa)
* `Purchases`: don't clear intro eligibility / purchased products cache
on first launch (#3067) via NachoSoto (@NachoSoto)
### Dependency Updates
* `SPM`: update `Package.resolved` (#3130) via NachoSoto (@NachoSoto)
### Other Changes
* `ReceiptParser`: fixed SPM build (#3144) via NachoSoto (@NachoSoto)
* `carthage_installation_tests`: optimize SPM package loading (#3129)
via NachoSoto (@NachoSoto)
* `CI`: add workaround for `Carthage` timing out (#3119) via NachoSoto
(@NachoSoto)
* `Integration Tests`: workaround to not lose debug logs (#3108) via
NachoSoto (@NachoSoto)
MarkVillacampa pushed a commit that referenced this pull request Sep 6, 2023
… on first launch (#3067)

These caches are important, especially for `RevenueCatUI`.
Without this fix, launching the app was doing this:
- Pre-warming cache
- Pre-warming cache a second time (to be fixed by a separate PR)
- Clearing cache

Which meant that the cache wasn't really warm when launching paywalls.

To fix that, this only clears the cache after receiving an actual
change.

I've also removed
`CustomerInfoManager.sendCachedCustomerInfoIfAvailable` because it
wasn't used.
MarkVillacampa pushed a commit that referenced this pull request Sep 6, 2023
**This is an automatic release.**

### Bugfixes
* `DebugViewModel`: fixed runtime crash on iOS < 16 (#3139) via
NachoSoto (@NachoSoto)
### Performance Improvements
* `PurchasesOrchestrator`: return early if receipt has no transactions
when checking for promo offers (#3123) via Mark Villacampa
(@MarkVillacampa)
* `Purchases`: don't clear intro eligibility / purchased products cache
on first launch (#3067) via NachoSoto (@NachoSoto)
### Dependency Updates
* `SPM`: update `Package.resolved` (#3130) via NachoSoto (@NachoSoto)
### Other Changes
* `ReceiptParser`: fixed SPM build (#3144) via NachoSoto (@NachoSoto)
* `carthage_installation_tests`: optimize SPM package loading (#3129)
via NachoSoto (@NachoSoto)
* `CI`: add workaround for `Carthage` timing out (#3119) via NachoSoto
(@NachoSoto)
* `Integration Tests`: workaround to not lose debug logs (#3108) via
NachoSoto (@NachoSoto)
@rafaelnobrekz
Copy link

rafaelnobrekz commented Nov 2, 2023

Hey @NachoSoto , is there even a way to clear these caches from the user side at all ? I'm having stale cache issues that persist even across a cold app start

edit: for eligibility I mean

@NachoSoto
Copy link
Contributor Author

There is not currently. What are you observing? Could you share some logs so we can take a look?

@rafaelnobrekz
Copy link

We're hoping to add some trial introductory offers a few weeks from now, and that would go along with some UI updates on the paywall to showcase it to the users. But it takes a long time for the SDK to catch up with any updates we do on App Store Connect, it comes back from disk cache it seems:

2023-11-01 22:20:23.887923-0300 Kinzoo[92535:24609323] [offering] DEBUG: ℹ️ Skipping products request for these products because they were already cached: ["com.kinzoo.app.staging.club.yearly", "com.kinzoo.app.staging.club.monthly"]
2023-11-01 22:20:23.921121-0300 Kinzoo[92535:24609323] [customer] DEBUG: ℹ️ Local intro eligibility computed locally. Result: ["com.kinzoo.app.staging.club.monthly": IntroEligibilityStatus.eligible, "com.kinzoo.app.staging.club.yearly": IntroEligibilityStatus.noIntroOfferExists]
2023-11-01 22:20:23.921359-0300 Kinzoo[92535:24609323] [eligibility] DEBUG: ℹ️ Caching trial or intro eligibility for products: ["com.kinzoo.app.staging.club.yearly", "com.kinzoo.app.staging.club.monthly"]

Could you share with me what is the cache window duration if any? I believe it would have cleared if I killed the app, but apparently it does not

@rafaelnobrekz
Copy link

rafaelnobrekz commented Nov 2, 2023

The test I'm doing is actually adding/removing a free trial on App Store Connect, so product caching might also play a factor (using the default offering for this)

@NachoSoto
Copy link
Contributor Author

it comes back from disk cache it seems:

Intro eligibility is never cached on disk though, only in memory (so re-computed when launching the app). The cache is cleared whenever the CustomerInfo changes (after a purchase, or when fetching an updated value from the server.

But it takes a long time for the SDK to catch up with any updates we do on App Store Connect

Interesting. What types of changes are you making and what do you observe? Also can confirm if you're using SK1 or SK2?

@rafaelnobrekz
Copy link

rafaelnobrekz commented Nov 2, 2023

Intro eligibility is never cached on disk though, only in memory (so re-computed when launching the app). The cache is cleared whenever the CustomerInfo changes (after a purchase, or when fetching an updated value from the server.

Cool, thanks for the assurance. I did notice the clearCache() call in handleCustomerChanged, would calling Purchases.shared.getCustomerInfo(fetchPolicy: .fetchCurrent) have any effect when the customer data does not change in an attempt to clear the cache through that means? Not that it would be any efficient. Anyway, the changes surviving restart suggest this is not my issue, possibly RC product cache (is it also memory-only?) or even StoreKit product caching.

Interesting. What types of changes are you making and what do you observe? Also can confirm if you're using SK1 or SK2?

I am removing an existing Introductory Offer from App Store Connect (or adding one if it did not exist). I assume it wouldn't be a very common scenario, but it was caught by our QA team and we're trying to understand exactly what is the criteria for this to update or nudge it in the right way to refresh.
Regarding StoreKit, we're using the defaults for latest RC 4.29.0 so I assume it's SK2 for eligibility checking and SK1 for product listing and purchasing?

@NachoSoto
Copy link
Contributor Author

Anyway, the changes surviving restart suggest this is not my issue, possibly RC product cache (is it also memory-only?) or even StoreKit product caching.

The SDK also doesn't have any on-disk product cache.

Regarding StoreKit, we're using the defaults for latest RC 4.29.0 so I assume it's SK2 for eligibility checking and SK1 for product listing and purchasing?

The default is SK1 for everything.

we're trying to understand exactly what is the criteria for this to update or nudge it in the right way to refresh.

Our current guess is that there's some propagation lag by Apple when you update offers in ASC.
For testing this behavior I'd recommend using SK config files where you can simulate these changes more effectively than relying on ASC servers catching up 👍🏻

@rafaelnobrekz
Copy link

When using a SK file to introduce / remove the trial offers it reflects as soon as I run the new code as expected. Simulating realtime updates won't be possible and those would be cached by RC's eligibility checker anyway, so I think that's good enough as Apple is the main bottleneck here!
Thanks for all the information @NachoSoto !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
perf A code change that improves performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants