[in_app_purchase] Fully migrate to BillingClient V5 #3752
Conversation
JeroenWeener
force-pushed
the
enhancement/v5-migration
branch
from
da666d1 to
2dcc1ed
Compare
JeroenWeener
force-pushed
the
enhancement/v5-migration
branch
from
2dcc1ed to
6c17124
Compare
There was a problem hiding this comment.
I have a few small questions/comments, but overall it looks good! The huge caveat there is that I'm not at all familiar with the underlying Android API, so the review is mostly of the mechanics rather than the overall design (unfortunately the person with the most expertise here is no longer on the team). But since it's mirroring the underlying API fairly closely hopefully that's not a significant issue.
| @@ -1,3 +1,7 @@ | |||
| ## 0.3.0 | |||
| * **BREAKING CHANGE**: Removes `launchPriceChangeConfirmationFlow` from `InAppPurchaseAndroidPlatform` as it is deprecated by Android. | |||
There was a problem hiding this comment.
Generally we like to provide migration tips with breaking changes; is there a replacement we can point people to?
There was a problem hiding this comment.
I will update the CHANGELOG mentioning that price flow changes are now handled by Google Play. Can I link developers to a relevant url or do we stray away from using those?
There was a problem hiding this comment.
A link to relevant docs sounds great!
| @@ -26,7 +26,7 @@ class GooglePlayPurchaseDetails extends PurchaseDetails { | |||
| factory GooglePlayPurchaseDetails.fromPurchase(PurchaseWrapper purchase) { | |||
| final GooglePlayPurchaseDetails purchaseDetails = GooglePlayPurchaseDetails( | |||
| purchaseID: purchase.orderId, | |||
| productID: purchase.sku, | |||
| productID: purchase.products.isNotEmpty ? purchase.products.first : '', | |||
There was a problem hiding this comment.
Could you add a comment here explaining why it's okay to drop the other products?
There was a problem hiding this comment.
Sorry to interrupt. I have gone through this issue and have a simple question!
In my opinion, to use a single class that covers both Google Play Store and App Store like PurchaseDetails and ProductDetails, we have to pretend 1 Subscription must have only 1 Base Plan in Google Play Store.
*Other base plans will be dropped if we only handle the first base plan
I wondered if dropping other base plans in Google Play Store and noticing in package usage docs or sth is okay.
here is the link I previously added comment in issue. thanks
There was a problem hiding this comment.
I made this change to be in line with what we did with sku, as initially a purchase could only hold one. My guess is that this was already causing problems on v4. I updated the code to now return a List<GooglePlayPurchaseDetails> instead.
There was a problem hiding this comment.
Hi @seunghwanly 👋
The approach this PR takes is returning all base plans and offers instead of only a single one, as is requested in 110909. The logic for splitting out the original object into multiple is here:
There was a problem hiding this comment.
Thanks for the reply @JeroenWeener 👍
I really appreciate your consideration.
| oneTimePurchaseOfferDetails.priceAmountMicros / 1000000.0; | ||
| final String currencyCode = oneTimePurchaseOfferDetails.priceCurrencyCode; | ||
| final String currencySymbol = | ||
| formattedPrice.isEmpty ? currencyCode : formattedPrice[0]; |
There was a problem hiding this comment.
Is it guaranteed that the currency symbol is exactly the first character in all cases?
There was a problem hiding this comment.
I checked and it's not, thank you for noticing. I will employ some regex to catch all* cases.
packages/in_app_purchase/in_app_purchase_android/lib/src/types/google_play_product_details.dart
Show resolved
Hide resolved
| /// Generate a [GooglePlayProductDetails] object based on an Android | ||
| /// [ProductDetailsWrapper] object for a subscription product. | ||
| /// Subscriptions can consist of multiple base plans, and base plans in turn | ||
| /// can consist of multiple offers. This method generates a list where every |
There was a problem hiding this comment.
Where is the list? I just see one object.
| final double rawPrice = (firstPricingPhase.priceAmountMicros) / 1000000.0; | ||
| final String currencyCode = firstPricingPhase.priceCurrencyCode; | ||
| final String currencySymbol = | ||
| formattedPrice.isEmpty ? currencyCode : formattedPrice[0]; |
There was a problem hiding this comment.
Same question here; is this actually safe?
Extracting a helper method, with comments about why this is the logic, would be helpful.
| final ProductDetailsWrapper productDetails; | ||
|
|
||
| /// The index pointing to the subscription this [GooglePlayProductDetails] | ||
| /// object was contructed for, or `null` if it was not a subscription. |
There was a problem hiding this comment.
The comment should be specific about what exactly its an index into.
...purchase/in_app_purchase_android/lib/src/billing_client_wrappers/billing_client_wrapper.dart
Show resolved
Hide resolved
|
|
||
| /// The error message shown when the map represents billing result is invalid from method channel. | ||
| /// | ||
| /// This usually indicates a series underlining code issue in the plugin. |
There was a problem hiding this comment.
Should this say "serious underlying"?
There was a problem hiding this comment.
I am not the author of this comment, but that would be my guess as well. I will update it.
JeroenWeener
force-pushed
the
enhancement/v5-migration
branch
from
6c17124 to
c7df4cb
Compare
There was a problem hiding this comment.
LGTM, again with the caveats about unfamiliarity with the underlying APIs. @gmackall could you do a secondary review?
@mvanbeusekom Do you want to review as well since you've worked on IAP?
| (BillingClient client) => client.queryProductDetails( | ||
| productList: identifiers | ||
| .map((String productId) => ProductWrapper( | ||
| productId: productId, productType: ProductType.subs)) |
There was a problem hiding this comment.
You've swapped the order of subs and inapp, is that intentional? (I'm not sure if it matters, it just seemed like a non-obvious change.)
There was a problem hiding this comment.
Good catch. I have swapped them back just to be sure. Now developers receive them in the order they are used to.
JeroenWeener
force-pushed
the
enhancement/v5-migration
branch
from
256b4f0 to
01a1428
Compare
Will take a look this afternoon! |
|
@JeroenWeener Great job in this PR! Now I have access to offer details and Billing V5! I see that you automatically apply the offer if it exists, is valid, but if I want to apply the offer conditionally? For example, I want to check on my backend if the user should see the offer or if I have multiple offers apply one of other. I tried to recreate a GooglePlayProductDetails using ProductDetailsWrapper but no success because subscriptionOfferDetails and subscriptionIndex seems always to be set. Sorry if this is not the best place to ask this question. |
...hase_android/android/src/main/java/io/flutter/plugins/inapppurchase/InAppPurchasePlugin.java
Outdated
Show resolved
Hide resolved
Implement v5 calls
|
Hello @JeroenWeener if latest commits/changes returns product with and without offers, that would be perfect for my use case, thank you! |
|
@gmackall Could you take a final look at this? It looks like your review bit got cleared. |
|
It looks like there is a failing test that needs to be resolved, but the changes to the method strings look good to me so re-adding my approval. |
@gmackall I'm not too familiar with LUCI yet. Is this failing test caused by the PR? I do not understand how my changes would affect a Windows test. |
|
It's not related to your PR, you just happened to have pulled in an unrelated issue the last time you sync'd |
stuartmorgan-g
added
the
autosubmit
|
Once this lands and is published, we'll need to update |
flutter/packages@b971830...5c69914 2023-05-17 49699333+dependabot[bot]@users.noreply.github.com [video_player]: Bump exoplayer_version from 2.18.5 to 2.18.6 in /packages/video_player/video_player_android/android (flutter/packages#3770) 2023-05-17 [email protected] [in_app_purchase] Fully migrate to BillingClient V5 (flutter/packages#3752) 2023-05-17 [email protected] [ci] Re-enable Windows repo tool tests (flutter/packages#4007) 2023-05-16 [email protected] [ci] Allow simctl shutdown to fail (flutter/packages#4024) 2023-05-16 [email protected] [ci] Move Linux repo tool tests to LUCI (flutter/packages#4006) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages-flutter-autoroll Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
In the upgrade to billing client v5 in #3752, some method signature strings on the Java side were updated. Unfortunately, as the Dart side was not updated. This mismatch broke the interaction between the Dart and native Android code through the method channels. This PR updates the strings on the Dart side so the method invocation work correctly again. This is a follow-up to - flutter/flutter#110909 - flutter/flutter#107370 - flutter/flutter#114265
flutter/packages@b971830...5c69914 2023-05-17 49699333+dependabot[bot]@users.noreply.github.com [video_player]: Bump exoplayer_version from 2.18.5 to 2.18.6 in /packages/video_player/video_player_android/android (flutter/packages#3770) 2023-05-17 [email protected] [in_app_purchase] Fully migrate to BillingClient V5 (flutter/packages#3752) 2023-05-17 [email protected] [ci] Re-enable Windows repo tool tests (flutter/packages#4007) 2023-05-16 [email protected] [ci] Allow simctl shutdown to fail (flutter/packages#4024) 2023-05-16 [email protected] [ci] Move Linux repo tool tests to LUCI (flutter/packages#4006) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages-flutter-autoroll Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
|
I think you forgot to update CHANGELOG.md? There are a lot more changes than it mentions now. I would have expected to have a migration guide too, required changes are all but trivial. |
|
@kinex This PR did update the CHANGELOG. If you'd like to see more details about migration in the docs, please file an issue with details. |
Sunsets BillingClient v4 calls in favor of the new v5 calls. Changes mostly follow the [Migration guide](https://developer.android.com/google/play/billing/migrate-gpblv5). Besides solving several issues, this change is also required as [billing client v4 will be obsolete by August 2nd, 2023](https://developer.android.com/google/play/billing/deprecation-faq). `getProductDetails()` will now return both base plans and their offers for subscriptions. Before, it would only return subscriptions that were backwards compatible with billing client v4. Price changes for subscriptions seem to be handled by the Play Store now instead. Therefore, `launchPriceChangeConfirmationFlow()` has been removed, making this PR a breaking change. Context: * [Billing Client API](https://developer.android.com/reference/com/android/billingclient/api/BillingClient#launchPriceChangeConfirmationFlow(android.app.Activity,%20com.android.billingclient.api.PriceChangeFlowParams,%20com.android.billingclient.api.PriceChangeConfirmationListener)) * [Billing Client docs](https://developer.android.com/google/play/billing/subscriptions#price-change) This PR fixes the following issues: * Fixes [#110909](flutter/flutter#110909) * Fixes [#107370](flutter/flutter#107370) * Fixes [#114265](flutter/flutter#114265)
…4040) In the upgrade to billing client v5 in flutter#3752, some method signature strings on the Java side were updated. Unfortunately, as the Dart side was not updated. This mismatch broke the interaction between the Dart and native Android code through the method channels. This PR updates the strings on the Dart side so the method invocation work correctly again. This is a follow-up to - flutter/flutter#110909 - flutter/flutter#107370 - flutter/flutter#114265
Sunsets BillingClient v4 calls in favor of the new v5 calls. Changes mostly follow the Migration guide.
Besides solving several issues, this change is also required as billing client v4 will be obsolete by August 2nd, 2023.
getProductDetails()will now return both base plans and their offers for subscriptions. Before, it would only return subscriptions that were backwards compatible with billing client v4.Price changes for subscriptions seem to be handled by the Play Store now instead. Therefore,
launchPriceChangeConfirmationFlow()has been removed, making this PR a breaking change. Context:This PR fixes the following issues:
Pre-launch Checklist
dart format.)[shared_preferences]pubspec.yamlwith an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.CHANGELOG.mdto add a description of the change, following repository CHANGELOG style.///).If you need help, consider asking for advice on the #hackers-new channel on Discord.