ReceiptFetcher: added retry mechanism
#1945
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
NachoSoto
force-pushed
the
receipt-fetch-retries
branch
6 times, most recently
from
e0eb66b
to
ba64e84
Compare
NachoSoto
changed the title
ReceiptFetcher: added retry mechanismReceiptFetcher: added retry mechanism
NachoSoto
commented
Sep 28, 2022
| @@ -740,6 +741,15 @@ private extension PurchasesOrchestrator { | |||
| } | |||
| } | |||
|
|
|||
| private func refreshRequestPolicy(forProductIdentifier productIdentifier: String) -> ReceiptRefreshPolicy { | |||
| if self.systemInfo.isSandbox { | |||
There was a problem hiding this comment.
Enabled only in sandbox for now
NachoSoto
force-pushed
the
receipt-fetch-retries
branch
2 times, most recently
from
576f825
to
4f405cc
Compare
NachoSoto
force-pushed
the
receipt-fetch-retries
branch
2 times, most recently
from
4517a42
to
65a0562
Compare
vegaro
reviewed
Sep 29, 2022
|
|
||
| var isActiveSubscription: Bool { | ||
| guard self.productType?.isSubscription == true else { return false } | ||
| guard let expiration = self.expiresDate else { return true } |
There was a problem hiding this comment.
A purchase without expiration?
There was a problem hiding this comment.
non subscriptions won't have expiration dates
There was a problem hiding this comment.
so in theory you'd never get here, but better safe than sorry
|
|
||
| func containsActivePurchase(forProductIdentifier identifier: String) -> Bool { | ||
| return (self.inAppPurchases.contains { $0.isActiveSubscription } || | ||
| self.inAppPurchases.contains { $0.productId == identifier }) |
There was a problem hiding this comment.
Since you're doing ||... Wouldn't this statement return true if there's an active subscription for a product identifier? It would also return true if there's an inactive subscription for that productId.
There was a problem hiding this comment.
Yeah, I think this is the logic we ended up agreeing on to avoid false negatives during a presubscription. @aboedo did I do this right?
There was a problem hiding this comment.
ohhh I see what @vegaro means, though - if you made any purchases for a past subscription this will now be true.
it should be either of:
- you have any active subscriptions at all (this is just in case of upgrades, where product id won't be the same as the one you purchased)
- you have a non-subscription purchase for that product identifier
But if you only have an expired subscription for that product identifier this should be false
There was a problem hiding this comment.
Okay I'll update this logic 👍🏻
Once that's finished, are we okay merging this PR enabled only for sandbox then? At the very least it will help integration tests. We could also only enable it for tests.
There was a problem hiding this comment.
we can start by having it for tests only and figure out next steps once we have a clearer picture of how to tackle triage-82
There was a problem hiding this comment.
That would be risk-free, since the only difference is a separate fetch policy that for now is unused :)
There was a problem hiding this comment.
Okay logic updated.
NachoSoto
added a commit
that referenced
this pull request
Sep 29, 2022
…xpires` to fix flakiness (#1952) Fixes [CSDK-479]. ### Changes: - Removed extra `verifyEntitlementWentThrough`. #1880 introduced a change so that weekly subscriptions aren't verified, because they expire within a second. This make the test flaky due to that race condition. - Removed `assertNoActiveSubscription`: I tried calling `SKTestSession.disableAutoRenewForTransaction`, but sometimes the server still thinks the subscription has auto-renewed. This was making the test flaky. Turns out that even if it's not active, the eligibility test still passes as expected. - Removed call to `syncPurchases`. It doesn't matter what state the server is in, as long as locally `SKTestSession` has an expired subscription. This, together with #1945, should fix the last of the issues causing flaky integration tests 🤞🏻 [CSDK-479]: https://revenuecats.atlassian.net/browse/CSDK-479?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
NachoSoto
force-pushed
the
receipt-fetch-retries
branch
from
9805380
to
1033914
Compare
NachoSoto
force-pushed
the
receipt-fetch-retries
branch
2 times, most recently
from
cd63ddd
to
cc52ff1
Compare
NachoSoto
commented
Sep 30, 2022
| Purchases.logLevel = .debug | ||
| Purchases.shared.delegate = self.purchasesDelegate | ||
| } | ||
|
|
||
| private var dangerousSettings: DangerousSettings { | ||
| return .init(autoSyncPurchases: true, | ||
| internalSettings: .init(enableReceiptFetchRetry: true)) |
There was a problem hiding this comment.
New DangerousSettings API to be able to set options without exposing them as public.
vegaro
approved these changes
Oct 7, 2022
Co-authored-by: Cesar de la Vega <cesarvegaro@gmail.com>
NachoSoto
force-pushed
the
receipt-fetch-retries
branch
from
cc52ff1
to
0936f4e
Compare
NachoSoto
added a commit
that referenced
this pull request
Nov 4, 2022
…ons` This changes the default back to `StoreKit 1`. We decided to do this for the following reasons: - Purchasing with `PromotionalOffer`s does not work with StoreKit 2 due to an Apple bug (see #2020 (comment)) - `checkTrialOrIntroDiscountEligibility` is significantly slower with StoreKit 2 (#1893). We're adding optimizations to help with that (#2007), but the underlying logic will still be slow. - A rare race-condition where `StoreKit 2` does not have transactions after a purchase ([TRIAGE-82]). We have some workarounds (#1945), but it's still being investigated. _ Note: This effectively reverts 0ee540a. That commit made it easier to only change the default in one place which is why this PR is basically just one line._
NachoSoto
added a commit
that referenced
this pull request
Nov 4, 2022
… "invalid" receipt See #1945. To make this retry mechanism less risky when eventually enabled, this change makes the "worst case scenario" (where retrying didn't help, or `AppleReceipt.containsActivePurchase` has issues) equivalent to `ReceiptFetcher.onlyIfEmpty`.
NachoSoto
added a commit
that referenced
this pull request
Nov 4, 2022
…ons` (#2022) This changes the default back to `StoreKit 1`. We decided to do this for the following reasons: - Purchasing with `PromotionalOffer`s does not work with StoreKit 2 due to an Apple bug (see #2020 (comment)) - `checkTrialOrIntroDiscountEligibility` is significantly slower with `StoreKit 2` (#1893). We're adding optimizations to help with that (#2007), but the underlying logic will still be slow. - A rare race-condition where `StoreKit 2` does not have transactions after a purchase ([TRIAGE-82]). We have some workarounds (#1945), but it's still being investigated. _Note: This effectively reverts 0ee540a. That commit made it easier to only change the default in one place which is why this PR is basically just one line._ [TRIAGE-82]: https://revenuecats.atlassian.net/browse/TRIAGE-82?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
NachoSoto
added a commit
that referenced
this pull request
Nov 9, 2022
… "invalid" receipt (#2024) See #1945. To make this retry mechanism less risky when eventually enabled, this change makes the "worst case scenario" (where retrying didn't help, or `AppleReceipt.containsActivePurchase` has issues) equivalent to `ReceiptFetcher.onlyIfEmpty`. _Note: this is currently not enabled in production, only in Integration Tests._
NachoSoto
added a commit
to RevenueCat/purchases-hybrid-common
that referenced
this pull request
Dec 19, 2022
See RevenueCat/purchases-ios#1945. Hopefully this will fail the tests in #278.
NachoSoto
added a commit
to RevenueCat/purchases-hybrid-common
that referenced
this pull request
Dec 19, 2022
See RevenueCat/purchases-ios#1945. Hopefully this will fail the tests in #278.
NachoSoto
added a commit
that referenced
this pull request
Feb 15, 2023
Fixes #2260 and [SDKONCALL-216]. Follow up to #2245. The change in #2245 was meant to avoid receipt refresh throttling errors (#2116). However, that was a regression when using StoreKit config files because those don't get refreshed in the backend. Unfortunately the workaround introduced in #1945 prevented us from noticing this, because we use `DangerousSettings.InternalSettings.enableReceiptFetchRetry` in our own integration tests. I added an integration test that explicitly covers this scenario. That fails without this change if disabling that dangerous setting. I thought about making that test run without the setting, but that might lead to flaky failures in CI, which is why we introduced that workaround in the first place. But at least now we have an explicit integration test for this, as well as 2 unit tests.
NachoSoto
added a commit
that referenced
this pull request
Feb 15, 2023
…2280) Fixes #2260 and [SDKONCALL-216]. Follow up to #2245. The change in #2245 was meant to avoid receipt refresh throttling errors (#2116). However, that was a regression when using StoreKit config files because those don't get refreshed in the backend. Unfortunately the workaround introduced in #1945 prevented us from noticing this, because we use `DangerousSettings.InternalSettings.enableReceiptFetchRetry` in our own integration tests. I added an integration test that explicitly covers this scenario. That fails without this change if disabling that dangerous setting. I thought about making that test run without the setting, but that might lead to flaky failures in CI, which is why we introduced that workaround in the first place. But at least now we have an explicit integration test for this, as well as 2 unit tests. [SDKONCALL-216]: https://revenuecats.atlassian.net/browse/SDKONCALL-216?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes CSDK-478
Depends on #1941, #1942, and #1943.Example:
Log of a successful purchase after retrying 🎉