-
Notifications
You must be signed in to change notification settings - Fork 3.6k
[in_app_purchase_storekit] Fix consumable repurchase issue in StoreKit2 #10521
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
base: main
Are you sure you want to change the base?
[in_app_purchase_storekit] Fix consumable repurchase issue in StoreKit2 #10521
Conversation
Fixes flutter/flutter#179111 ## Changes - Fixed inish() method to always call the completion handler, even when the transaction is not found - Added fallback to Transaction.unfinished when looking up transactions - Added proper error handling with PigeonError for transaction finish failures - Added tests for non-existent transaction finish and consumable repurchase scenarios ## Problem With StoreKit2, consumable products could only be purchased once because: 1. The inish() method would never call its completion handler if the transaction wasn't found in Transaction.all 2. This caused the Dart side to hang indefinitely 3. Consumable transactions needed to be found in Transaction.unfinished as well ## Solution - inish() now returns success even if the transaction is not found (it's effectively already complete) - etchTransaction() now checks both Transaction.all and Transaction.unfinished - Proper error handling added for unexpected failures
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request effectively addresses a critical issue with repurchasing consumable products in StoreKit2. The changes to the finish and fetchTransaction methods are logical and well-implemented, ensuring that the completion handler is always called and that unfinished transactions are correctly handled. The addition of error handling and corresponding tests significantly improves the robustness of the implementation. I have a couple of suggestions to further enhance code maintainability and test reliability.
| // Finish the transaction | ||
| plugin.finish(id: 0) { result in | ||
| switch result { | ||
| case .success(): | ||
| finishExpectation.fulfill() | ||
| case .failure(let error): | ||
| XCTFail("Finish should NOT fail. Failed with \(error)") | ||
| } | ||
| } | ||
|
|
||
| await fulfillment(of: [finishExpectation], timeout: 5) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using a hardcoded transaction ID 0 makes this test fragile and dependent on the specific behavior of the test environment. A more robust approach would be to dynamically fetch the transaction created by the first purchase and use its actual ID. This ensures the test is more reliable and less likely to break with changes to the test session implementation.
let transactionsExpectation = self.expectation(description: "Get transactions")
var transactionId: Int64?
plugin.transactions { result in
switch result {
case .success(let transactions):
transactionId = transactions.first?.id
transactionsExpectation.fulfill()
case .failure(let error):
XCTFail("Getting transactions should NOT fail. Failed with \(error)")
transactionsExpectation.fulfill()
}
}
await fulfillment(of: [transactionsExpectation], timeout: 5)
guard let idToFinish = transactionId else {
XCTFail("Could not get transaction ID to finish")
return
}
// Finish the transaction
plugin.finish(id: idToFinish) { result in
switch result {
case .success():
finishExpectation.fulfill()
case .failure(let error):
XCTFail("Finish should NOT fail. Failed with \(error)")
}
}
await fulfillment(of: [finishExpectation], timeout: 5)| } | ||
| } | ||
|
|
||
| // If not found in Transaction.all, check unfinished transactions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation says all should include unfinished consumables: https://developer.apple.com/documentation/storekit/transaction/all#Discussion. But if this method is only used by finish, shouldn't we use unfinished in first place instead of all?
| await transaction.finish() | ||
| completion(.success(Void())) | ||
| } else { | ||
| // Transaction not found - this can happen for consumables that have |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels like a case that should be treated as an error since the programmer is trying to finish a non-existent transaction and we should not fail silently? Unless there're legitimate cases where developers can't tell if a transaction is finished or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review! I've addressed both points:
finish() now uses a new fetchUnfinishedTransaction() helper that only checks Transaction.unfinished
finish() now returns an error (storekit2_transaction_not_found) when the transaction is not found, instead of silently succeeding
…r for non-existent transactions Changes based on @LongCatIsLooong's review: - Use Transaction.unfinished instead of Transaction.all for finish() since we're looking for transactions that need to be finished - Return an error when attempting to finish a non-existent transaction instead of silently succeeding - Added fetchUnfinishedTransaction() helper specifically for finish() - Updated tests to expect error for non-existent transaction
Fixes flutter/flutter#179111
Changes
Problem
With StoreKit2, consumable products could only be purchased once because:
Solution
Replace this paragraph with a description of what this PR is changing or adding, and why. Consider including before/after screenshots.
List which issues are fixed by this PR. You must list at least one issue.
Pre-Review Checklist
[shared_preferences]pubspec.yamlwith an appropriate new version according to the pub versioning philosophy, or I have commented below to indicate which version change exemption this PR falls under1.CHANGELOG.mdto add a description of the change, following repository CHANGELOG style, or I have commented below to indicate which CHANGELOG exemption this PR falls under1.///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.Footnotes
Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling. ↩ ↩2 ↩3