OfferingsManager: don't clear offerings cache timestamp when request fails#2359
Conversation
|
Holding on this until after the next release. |
tonidero
left a comment
There was a problem hiding this comment.
Looks like this was also done in Android. I believe the reasoning is that (in Android at least), we set the cache timestamp right before we make the backend call. So if it fails, we want to retry immediately, and not wait for the non-existent cached timestamp to remain. If it's the same in iOS, we should keep this.
|
Oh that's wrong then, we should only update the timestamp after receiving the response. But I guess I see why... that's to avoid making multiple concurrent requests while stale. But the new callback cache fixes that now? |
Hmm yeah I agree... Can't think of any other reasons to have that so I think we could just remove it. Let us know if you have any concerns @vegaro |
|
You two are right. This was there before to prevent making another request (the timestamp would indicate the cache is not stale). But I think as @NachoSoto says, the new cache callback fixes that and makes this more clear, so I think it's safe to remove |
|
Cool, thanks! But again, let's not do this until after the next release, no reason to introduce more risk. I'll leave this open in the meantime and we can revisit it later. |
8c5abe4 to
b9f8e32
Compare
|
@RevenueCat/coresdk this is rebased and ready to review again. |
There was a problem hiding this comment.
Hmm I was thinking, did we also change where we call this to only call it on the success case? Otherwise, if there is an error and we are calling this BEFORE the request happens, this change could cause some unintended behavior.
There was a problem hiding this comment.
Yeah this is only called by preventPurchasePopupCallFromTriggeringCacheRefresh.
There was a problem hiding this comment.
Which doesn't make a lot of sense btw, I filed SDK-3172 to address that.
There was a problem hiding this comment.
I removed that in #2623. Any other concerns here?
b9f8e32 to
5e0eee6
Compare
Codecov Report
@@ Coverage Diff @@
## main #2359 +/- ##
==========================================
- Coverage 86.43% 86.30% -0.14%
==========================================
Files 207 205 -2
Lines 14518 14311 -207
==========================================
- Hits 12549 12351 -198
+ Misses 1969 1960 -9
|
tonidero
left a comment
There was a problem hiding this comment.
After changing it in the other PR this seems safe now 👍
…t fails There is no reason to clear this and make the cache immediately stale. Instead, failed requests would eventually make the cache stale after a certain duration.
5e0eee6 to
7bd5fc8
Compare
**This is an automatic release.** ### Bugfixes * `PurchasesOrchestrator`: update `CustomerInfoManager` cache after processing transactions (#2676) via NachoSoto (@NachoSoto) * `ErrorResponse`: drastically improved error messages, no more "unknown error"s (#2660) via NachoSoto (@NachoSoto) * `PaywallExtensions`: post purchases with `Offering` identifier (#2645) via NachoSoto (@NachoSoto) * Support `product_plan_identifier` for purchased subscriptions from `Google Play` (#2654) via Josh Holtz (@joshdholtz) ### Performance Improvements * `copy(with: VerificationResult)`: optimization to avoid copies (#2639) via NachoSoto (@NachoSoto) ### Other Changes * `ETagManager`: refactored e-tag creation and tests (#2671) via NachoSoto (@NachoSoto) * `getPromotionalOffer`: return `ErrorCode.ineligibleError` if receipt is not found (#2678) via NachoSoto (@NachoSoto) * `TimingUtil`: removed slow purchase logs (#2677) via NachoSoto (@NachoSoto) * `CI`: changed `Codecov` to `informational` (#2670) via NachoSoto (@NachoSoto) * `LoadShedderIntegrationTests`: verify requests are actually handled by load shedder (#2663) via NachoSoto (@NachoSoto) * `ETagManager.httpResultFromCacheOrBackend`: return response headers (#2666) via NachoSoto (@NachoSoto) * `Integration Tests`: added tests to verify 304 behavior (#2659) via NachoSoto (@NachoSoto) * `HTTPClient`: disable `URLSession` cache (#2668) via NachoSoto (@NachoSoto) * Documented `HTTPStatusCode.isSuccessfullySynced` (#2661) via NachoSoto (@NachoSoto) * `NetworkError.signatureVerificationFailed`: added status code to error `userInfo` (#2657) via NachoSoto (@NachoSoto) * `HTTPClient`: improved log for failed requests (#2669) via NachoSoto (@NachoSoto) * `ETagManager`: added new verbose logs (#2656) via NachoSoto (@NachoSoto) * `Signature Verification`: added test-only log for debugging invalid signatures (#2658) via NachoSoto (@NachoSoto) * Fixed `HTTPResponse.description` (#2664) via NachoSoto (@NachoSoto) * Changed `Logger` to use `os_log` (#2608) via NachoSoto (@NachoSoto) * `MainThreadMonitor`: increased threshold (#2662) via NachoSoto (@NachoSoto) * `debugRevenueCatOverlay`: display `receiptURL` (#2652) via NachoSoto (@NachoSoto) * `PurchaseTester`: added ability to display `debugRevenueCatOverlay` (#2653) via NachoSoto (@NachoSoto) * `debugRevenueCatOverlay`: ability to close on `macOS`/`Catalyst` (#2649) via NachoSoto (@NachoSoto) * `debugRevenueCatOverlay`: added support for `macOS` (#2648) via NachoSoto (@NachoSoto) * `LoadShedderIntegrationTests`: enable signature verification (#2655) via NachoSoto (@NachoSoto) * `ImageSnapshot`: fixed Xcode 15 compilation (#2651) via NachoSoto (@NachoSoto) * `OfferingsManager`: don't clear offerings cache timestamp when request fails (#2359) via NachoSoto (@NachoSoto) * `StoreKitObserverModeIntegrationTests`: added test for posting renewals (#2590) via NachoSoto (@NachoSoto) * Always initialize `StoreKit2TransactionListener` even on SK1 mode (#2612) via NachoSoto (@NachoSoto) * `ErrorUtils.missingReceiptFileError`: added receipt URL `userInfo` context (#2650) via NachoSoto (@NachoSoto) * Added `.xcprivacy` for Xcode 15 (#2619) via NachoSoto (@NachoSoto) * `Trusted Entitlements`: added debug log with `ResponseVerificationMode` (#2647) via NachoSoto (@NachoSoto) * `debugRevenueCatOverlay`: simplified title (#2641) via NachoSoto (@NachoSoto) * Simplified `Purchases.updateAllCachesIfNeeded` (#2626) via NachoSoto (@NachoSoto) * `HTTPResponseTests`: fixed disabled test (#2643) via NachoSoto (@NachoSoto) * Add `InternalDangerousSettings.forceSignatureFailures` (#2635) via NachoSoto (@NachoSoto) * `IntegrationTests`: explicit `StoreKit 1` mode (#2636) via NachoSoto (@NachoSoto) * `Signing`: removed API for loading key from a file (#2638) via NachoSoto (@NachoSoto)
There is no reason to clear this and make the cache immediately stale. Instead, failed requests would eventually make the cache stale after a certain duration.