Purchase cancellations: unify behavior between SK1 and SK2#1841
Merged
Conversation
Fixes [CSDK-113] and RevenueCat/purchases-flutter#403 ### Changes: - SK2 purchase now forwards an error (`ErrorCode.purchaseCancelledError`) if the purchase is cancelled. This matches the behavior in SK1. - In order to retain the behavior on `async` APIs, this error is ignored if `userCancelled` is `true`, to ensure that `PurchaseResultData` is returned instead of an error being thrown. Unfortunately, as explained in [CSDK-113], we can't test this because there's no way to fake cancellations.
aboedo
approved these changes
Aug 16, 2022
aboedo
left a comment
Member
There was a problem hiding this comment.
looks good! sucks to not be able to test this programmatically.
I assume it works fine locally right?
Have you tried pointing a hybrid sdk to this branch to verify that they don't crash now? lmk if you need hand setting that up
| purchase(product: product, | ||
| promotionalOffer: promotionalOffer) { transaction, customerInfo, error, userCancelled in | ||
| continuation.resume(with: Result(customerInfo, error) | ||
| continuation.resume(with: Result(customerInfo, error?.ignoreIfPurchaseCancelled(userCancelled)) |
Member
There was a problem hiding this comment.
feels like we're duplicating this code a lot, should we be moving this up the stack?
Contributor
Author
There was a problem hiding this comment.
We can't, we do want to send the error in the completion block based APIs (that's what broke PHC, that SK2 wasn't doing that), but we don't want to throw an error in the async APIs. Does that make sense?
Contributor
Author
There was a problem hiding this comment.
So to summarize:
- Completion block APIs need both the error and the cancellation parameter. This is consistent with SK1 and SK2, users can choose how to read cancellations
asyncAPIs can't have an error and a value, so throwing in case of cancellation seems like a bad API, especially because the returned value has auserCancelledboolean.
So we can't push this up because we only want to remove the error in async APIs. Does that make sense?
Contributor
Author
Yup, I tried this in Flutter, but I just tried it on iOS through the |
aboedo
approved these changes
Aug 17, 2022
Merged
NachoSoto
added a commit
that referenced
this pull request
Aug 19, 2022
### Bugfixes * `ErrorResponse`: don't add attribute errors to message if empty (#1844) via NachoSoto (@NachoSoto) * Purchase cancellations: unify behavior between SK1 and SK2 (#1841) via NachoSoto (@NachoSoto) * StoreKit 2: `PurchasesOrchestrator`: don't log "purchased product" if it was cancelled (#1840) via NachoSoto (@NachoSoto) * `Backend`: fixed potential race conditions introduced by `OperationDispatcher.dispatchOnWorkerThread(withRandomDelay:)` (#1827) via NachoSoto (@NachoSoto) * `DeviceCache`: `Sendable` conformance and fixed thread-safety (#1823) via NachoSoto (@NachoSoto) * Directly send delegate customer info when delegate is set (always sends cached CustomerInfo value) (#1828) via Josh Holtz (@joshdholtz) * `SystemInfo.finishTransactions`: made thread-safe (#1807) via NachoSoto (@NachoSoto) * `Purchases.shared` and `Purchases.isConfigured` are now thread-safe (#1813) via NachoSoto (@NachoSoto) * `PriceFormatterProvider: Sendable` conformance and fixed thread-safety (#1818) via NachoSoto (@NachoSoto) * `StoreKitConfigTestCase.changeStorefront`: re-enabled on iOS 16 (#1811) via NachoSoto (@NachoSoto) ### Other Changes * `DeviceCache`: no longer set cache timestamp before beginning request (#1839) via NachoSoto (@NachoSoto) * `MagicWeatherSwiftUI`: updated to use `async` APIs (#1843) via NachoSoto (@NachoSoto) * Release train (#1842) via Cesar de la Vega (@vegaro) * Adds hotfixes section to RELEASING doc (#1837) via Cesar de la Vega (@vegaro) * Update fastlane plugin (#1838) via Toni Rico (@tonidero) * Update migration doc from didReceiveUpdatedCustomerInfo to receivedUpdatedCustomerInfo (#1836) via Josh Holtz (@joshdholtz) * `PurchasesDelegate`: added test for latest cached customer info always being sent (#1830) via NachoSoto (@NachoSoto) * `CallbackCache: Sendable` conformance (#1835) via NachoSoto (@NachoSoto) * `CallbackCache`: simplified implementation using `Atomic` (#1834) via NachoSoto (@NachoSoto) * `PurchasesLogInTests`: added test to verify `logIn` updates offerings cache (#1833) via NachoSoto (@NachoSoto) * Created `PurchasesLoginTests` (#1832) via NachoSoto (@NachoSoto) * `SwiftLint`: cleaned up output (#1821) via NachoSoto (@NachoSoto) * Link to sdk reference (#1831) via aboedo (@aboedo) * `Atomic: ExpressibleByBooleanLiteral` (#1822) via NachoSoto (@NachoSoto) * `SwiftLint`: fixed build warning (#1820) via NachoSoto (@NachoSoto) * Adds an approval job that will tag the release (#1815) via Cesar de la Vega (@vegaro) * `Atomic: ExpressibleByNilLiteral` (#1804) via NachoSoto (@NachoSoto) * `PurchasesAttributionDataTests`: fixed potential race condition in flaky test (#1805) via NachoSoto (@NachoSoto) * Fixed warnings for unnecessary `try` (#1816) via NachoSoto (@NachoSoto) * Moved `AttributionFetcherError` inside `AttributionFetcher` (#1808) via NachoSoto (@NachoSoto) * Update documentation for presentCodeRedemptionSheet (#1817) via Joshua Liebowitz (@taquitos) * `Dangerfile`: added "next_release" as supported label (#1810) via NachoSoto (@NachoSoto) * PurchaseTester- Update Podfile.lock (#1814) via Joshua Liebowitz (@taquitos) * Update to latest fastlane plugin (#1802) via Toni Rico (@tonidero) * Clean up: moved `BackendIntegrationTests.xctestplan` to `TestPlans` folder (#1812) via NachoSoto (@NachoSoto) * `SK2StoreProduct`: conditionally removed `@available` workaround (#1794) via NachoSoto (@NachoSoto) * `SwiftLint`: fixed deprecation warning (#1809) via NachoSoto (@NachoSoto) * Update gems (#1791) via Joshua Liebowitz (@taquitos) * Replace usages of replace_in with replace_text_in_files action (#1803) via Toni Rico (@tonidero)
Closed
5 tasks
NachoSoto
added a commit
that referenced
this pull request
Aug 29, 2022
Fixes #1868. See #1868 (comment) for a summary of the changes in #1841. ### Before #1841: - SK1 / completion-block: no `CustomerInfo` + cancelled + `ErrorCode.purchaseCancelledError` - SK1 / `async`: thrown `ErrorCode.purchaseCancelledError` - SK2 / completion-block: `CustomerInfo` + cancelled + no error - SK2 / `async`: `CustomerInfo` + cancelled (not error) ### After #1841: - SK1 / completion-block: no `CustomerInfo` + cancelled + `ErrorCode.purchaseCancelledError` - SK1 / `async`: no `CustomerInfo` + ignored error -> crash! 🚨 - SK2 / completion-block: `CustomerInfo` + cancelled + `ErrorCode.purchaseCancelledError` - SK2 / `async`: `CustomerInfo` + cancelled (not error) ### After this PR: - SK1 / completion-block: no `CustomerInfo` + cancelled + `ErrorCode.purchaseCancelledError` - **SK1 / `async`: thrown `ErrorCode.purchaseCancelledError`**⚠️ - SK2 / completion-block: `CustomerInfo` + cancelled + `ErrorCode.purchaseCancelledError` - SK2 / `async`: `CustomerInfo` + cancelled (not error) The change in #1841 was slightly incorrect. The `ignoreIfPurchaseCancelled` didn't make a lot of sense because `Result(value:error:)` already ignored the error if the value wasn't `nil`. This change still doesn't get us to a fully consistent behavior across SK1 and SK2 but it adds coverage for this crash and temporarily fixes it.
NachoSoto
added a commit
that referenced
this pull request
Aug 29, 2022
Fixes #1868. See #1868 (comment) for a summary of the changes in #1841. ### Before #1841: - SK1 / completion-block: no `CustomerInfo` + cancelled + `ErrorCode.purchaseCancelledError` - SK1 / `async`: thrown `ErrorCode.purchaseCancelledError` - SK2 / completion-block: `CustomerInfo` + cancelled + no error - SK2 / `async`: `CustomerInfo` + cancelled (not error) ### After #1841: - SK1 / completion-block: no `CustomerInfo` + cancelled + `ErrorCode.purchaseCancelledError` - SK1 / `async`: no `CustomerInfo` + ignored error -> crash! 🚨 - SK2 / completion-block: `CustomerInfo` + cancelled + `ErrorCode.purchaseCancelledError` - SK2 / `async`: `CustomerInfo` + cancelled (not error) ### After this PR: - SK1 / completion-block: no `CustomerInfo` + cancelled + `ErrorCode.purchaseCancelledError` - **SK1 / `async`: thrown `ErrorCode.purchaseCancelledError`**⚠️ - SK2 / completion-block: `CustomerInfo` + cancelled + `ErrorCode.purchaseCancelledError` - SK2 / `async`: `CustomerInfo` + cancelled (not error) The change in #1841 was slightly incorrect. The `ignoreIfPurchaseCancelled` didn't make a lot of sense because `Result(value:error:)` already ignored the error if the value wasn't `nil`. This change still doesn't get us to a fully consistent behavior across SK1 and SK2 but it adds coverage for this crash and temporarily fixes it.
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
2 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-113 and RevenueCat/purchases-flutter#403
Changes:
ErrorCode.purchaseCancelledError) if the purchase is cancelled. This matches the behavior in SK1.asyncAPIs, this error is ignored ifuserCancelledistrue, to ensure thatPurchaseResultDatais returned instead of an error being thrown.Unfortunately, as explained in CSDK-113, we can't test this because there's no way to fake cancellations (FB10300403).