ReceiptFetcher: added retry mechanism#1945
Merged
Merged
Conversation
e0eb66b to
ba64e84
Compare
ReceiptFetcher: added retry mechanismReceiptFetcher: added retry mechanism
NachoSoto
commented
Sep 28, 2022
Contributor
Author
There was a problem hiding this comment.
Enabled only in sandbox for now
576f825 to
4f405cc
Compare
4517a42 to
65a0562
Compare
vegaro
reviewed
Sep 29, 2022
Contributor
Author
There was a problem hiding this comment.
A purchase without expiration?
Member
There was a problem hiding this comment.
non subscriptions won't have expiration dates
Member
There was a problem hiding this comment.
so in theory you'd never get here, but better safe than sorry
Member
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.
Contributor
Author
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?
Member
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
Contributor
Author
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.
Member
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
Contributor
Author
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 :)
Contributor
Author
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
9805380 to
1033914
Compare
cd63ddd to
cc52ff1
Compare
NachoSoto
commented
Sep 30, 2022
|
|
||
| private var dangerousSettings: DangerousSettings { | ||
| return .init(autoSyncPurchases: true, | ||
| internalSettings: .init(enableReceiptFetchRetry: true)) |
Contributor
Author
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>
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
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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 🎉