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

Added BackendErrorCode.purchasedProductMissingInAppleReceipt #2033

Merged
merged 2 commits into from
Nov 8, 2022

Conversation

NachoSoto
Copy link
Contributor

See https://github.com/RevenueCat/khepri/pull/4716
This makes that error ErrorCode.invalidReceiptError instead of .unknownBackendError.

I've also added #2032 to improve error messages when these errors are unknown in the future.

See https://github.com/RevenueCat/khepri/pull/4716
This makes that error `ErrorCode.invalidReceiptError` instead of `.unknownBackendError`.

I've also added #2032 to improve error messages when these errors are unknown in the future.
@NachoSoto NachoSoto added the refactor A code change that neither fixes a bug nor adds a feature label Nov 7, 2022
@NachoSoto NachoSoto requested review from antoniobg and a team November 7, 2022 22:08
@@ -89,7 +90,7 @@ extension BackendErrorCode {
return .storeProblemError
case .cannotTransferPurchase:
return .receiptAlreadyInUseError
case .invalidReceiptToken:
case .invalidReceiptToken, .purchasedProductMissingInAppleReceipt:
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick, these multi-cases seem to be set in multiple lines here. Just to be consistent:

Suggested change
case .invalidReceiptToken, .purchasedProductMissingInAppleReceipt:
case .invalidReceiptToken,
.purchasedProductMissingInAppleReceipt:

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, not sure if we have tests for these... If we do, it might be good to add one, if not, I guess something to think about.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What type of test would you write, other than simply duplicating this logic?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I think it would be good to have a test that checks the mapping from backend code to purchases code for each of these values. As you said, it's basically duplicating the logic, so not a huge benefit, but might catch some issues if someone changes the mapping at some point and moves one of the error codes accidentally. That said, I don't think this is needed for this PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Like you said yourself, that would just be duplicating the logic. This is purely data-driven, so the data is the test as well.

for each of these values.

In order to do this cleanly, we would need to have a mapping for each of the errors, instead of duplicating each test. Then iterate the mapping and assert it's correct... but wait, that mapping is the data we're testing, so there would be no valid in that duplication.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well I agree that it's redundant, but there is some security in redundancy. In any case, I agree the chances of this being changed mistakenly are minimal and if we did this, we would probably have to duplicate logic for tests in a bunch of places so I'm ok with leaving this.

@NachoSoto NachoSoto merged commit a69cb94 into main Nov 8, 2022
@NachoSoto NachoSoto deleted the backend-error-missing-product-receipt branch November 8, 2022 17:14
NachoSoto pushed a commit that referenced this pull request Nov 9, 2022
**This is an automatic release.**

### Bugfixes
* `ISO8601DateFormatter.withMilliseconds`: fixed iOS 11 crash (#2037)
via NachoSoto (@NachoSoto)
* Changed `StoreKit2Setting.default` back to
`.enabledOnlyForOptimizations` (#2022) via NachoSoto (@NachoSoto)
### Other Changes
* `Integration Tests`: changed weekly to monthly subscriptions to work
around 0-second subscriptions (#2042) via NachoSoto (@NachoSoto)
* `Integration Tests`: fixed `testPurchaseWithAskToBuyPostsReceipt`
(#2040) via NachoSoto (@NachoSoto)
* `ReceiptRefreshPolicy.retryUntilProductIsFound`: default to returning
"invalid" receipt (#2024) via NachoSoto (@NachoSoto)
* `CachingProductsManager`: use partial cached products (#2014) via
NachoSoto (@NachoSoto)
* Added `BackendErrorCode.purchasedProductMissingInAppleReceipt` (#2033)
via NachoSoto (@NachoSoto)
* `PurchaseTesterSwiftUI`: replaced `Purchases` dependency with `SPM`
(#2027) via NachoSoto (@NachoSoto)
* `Integration Tests`: changed log output to `raw` (#2031) via NachoSoto
(@NachoSoto)
* `Integration Tests`: run on iOS 16 (#2035) via NachoSoto (@NachoSoto)
* CI: fixed `iOS 14` tests Xcode version (#2030) via NachoSoto
(@NachoSoto)
* `Async.call`: added non-throwing overload (#2006) via NachoSoto
(@NachoSoto)
* Documentation: Fixed references in `V4_API_Migration_guide.md` (#2018)
via NachoSoto (@NachoSoto)
* `eligiblePromotionalOffers`: don't log error if response is ineligible
(#2019) via NachoSoto (@NachoSoto)
* Runs push-pods after make-release (#2025) via Cesar de la Vega
(@vegaro)
* Some updates on notify-on-non-patch-release-branches: (#2026) via
Cesar de la Vega (@vegaro)
* Deploy `PurchaseTesterSwiftUI` to TestFlight (#2003) via NachoSoto
(@NachoSoto)
* `PurchaseTesterSwiftUI`: added "logs" screen (#2012) via NachoSoto
(@NachoSoto)
* `PurchaseTesterSwiftUI`: allow configuring API key at runtime (#1999)
via NachoSoto (@NachoSoto)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor A code change that neither fixes a bug nor adds a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants