Skip to content

ReceiptRefreshPolicy.retryUntilProductIsFound: default to returning "invalid" receipt#2024

Merged
NachoSoto merged 2 commits into
mainfrom
receipt-fetcher-retry-return-invalid
Nov 9, 2022
Merged

ReceiptRefreshPolicy.retryUntilProductIsFound: default to returning "invalid" receipt#2024
NachoSoto merged 2 commits into
mainfrom
receipt-fetcher-retry-return-invalid

Conversation

@NachoSoto

@NachoSoto NachoSoto commented Nov 4, 2022

Copy link
Copy Markdown
Contributor

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.

… "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 NachoSoto requested a review from a team November 4, 2022 17:30

@tonidero tonidero left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just a question, if that's ok, this looks good.

repeat {
retries += 1
let data = await self.refreshReceipt()
data = await self.refreshReceipt()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

When the receipt can't be parsed on the last retry, we would return that receipt. Is that correct?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We could add a couple of tests for these returning an unparseable receipt and an empty receipt if that's the case.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

When the receipt can't be parsed on the last retry, we would return that receipt. Is that correct?

Correct, as explained in the description:

To make this retry mechanism less risky when eventually enabled, this change makes the "worst case scenario" [...] equivalent to ReceiptFetcher.onlyIfEmpty.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We could add a couple of tests for these returning an unparseable receipt and an empty receipt if that's the case.

The current tests already cover that though?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The current tests already cover that though?

Well I think we have tests for receipts without a purchase but not for unparseable receipts? (I might be missing something since I'm not that familiar with the parsing logic)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Is testStopsRetryingEvenIfDataIsInvalid what you're describing?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

    func testStopsRetryingEvenIfDataIsInvalid() async {
        self.mock(receipt: Self.receiptWithoutPurchases)
        let invalidData = Self.receiptWithoutPurchases.asData

        let data = await self.fetch(productIdentifier: Self.productID, retries: 2)
        expect(data) == Data()
        expect(data) == invalidData

        expect(self.mockReceiptParser.invokedParseParametersList) == [
            invalidData,
            invalidData,
            invalidData
        ]
        expect(self.mockRequestFetcher.refreshReceiptCalledCount) == 3
    }

That seems to be using a receiptWithoutPurchases. I was thinking more along the lines of a receipt that is missing a critical field or something like that. So basically a test where we make self.receiptParser.parse throw an exception.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Oooh gotcha, yeah there's no test coverage for that catch. Good catch!
I added that test.

@NachoSoto NachoSoto merged commit 6ba90d0 into main Nov 9, 2022
@NachoSoto NachoSoto deleted the receipt-fetcher-retry-return-invalid branch November 9, 2022 00:09
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

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants