-
Notifications
You must be signed in to change notification settings - Fork 9.8k
[In_app_purchases] migrate to Play Billing Library 2.0. #2287
Conversation
@@ -14,6 +14,7 @@ | |||
@Override | |||
public BillingClient createBillingClient(Context context, MethodChannel channel) { | |||
return BillingClient.newBuilder(context) | |||
.enablePendingPurchases() |
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.
As far as I can tell this API exists on the PBL2 SDK just to make sure that the developer has really read the new API docs and is aware of the need to register pending purchase changes. I think we should also gate this breaking change behind something similar, though I'm not totally sure as to what. Some people use package: any
dependencies in their pubspec and will just automatically get this. I think it may be good to have some kind of obvious hard failure that makes it basically impossible to miss the new changes. I'm not sure if requiring people to effectively set a param or call this is the right way to go here, but I'm considering it. WDYT?
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.
We can guard this by asking BillingClient
on dart to call an API with the same name enablePendingPurchases ()
, otherwise do an assertion failure. Although we can fail it on dart side, I think we should always call it on the JAVA side.
On the unified layer, we can either do the same, or hide it from the user. WDYT?
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.
I'm going to reach out offline to get some second opinions on this.
packages/in_app_purchase/lib/src/billing_client_wrappers/billing_client_wrapper.dart
Outdated
Show resolved
Hide resolved
packages/in_app_purchase/lib/src/billing_client_wrappers/billing_client_wrapper.dart
Outdated
Show resolved
Hide resolved
packages/in_app_purchase/lib/src/billing_client_wrappers/billing_client_wrapper.dart
Outdated
Show resolved
Hide resolved
packages/in_app_purchase/lib/src/in_app_purchase/in_app_purchase_connection.dart
Outdated
Show resolved
Hide resolved
if (status == PurchaseStatus.purchased || | ||
status == PurchaseStatus.error) { | ||
pendingCompletePurchase = true; | ||
} | ||
} | ||
if (_platform == _kPlatformAndroid) { | ||
if (status == PurchaseStatus.purchased) { |
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.
So does the user need to special case whether or not they call completePurchase
based on what the status is and whether they're on iOS or Android now, where with Android they should avoid calling it for PurchaseStatus.error
? I think this should probably be mentioned in the completePurchase
docs.
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.
see the response on https://github.com/flutter/plugins/pull/2287/files#r349738652
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.
Sorry, that link doesn't work anymore. I think the PR may be too big. Do you mind copy/pasting it?
packages/in_app_purchase/lib/src/in_app_purchase/purchase_details.dart
Outdated
Show resolved
Hide resolved
packages/in_app_purchase/lib/src/in_app_purchase/purchase_details.dart
Outdated
Show resolved
Hide resolved
this.billingClientPurchase = null { | ||
this.status = SKTransactionStatusConverter() | ||
.toPurchaseStatus(transaction.transactionState); | ||
_platform = _kPlatformIOS; |
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.
nit: I think this can go outside of the method block with the other variables that are set above it. Same with the Android one below.
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.
done
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.
nit: I think _platform
can also be set the same way here and below.
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.
done
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.
Updated the most of the comments.
For some documentation changes, will need to confirm and update it in another patch.
...xample/android/app/src/test/java/io/flutter/plugins/inapppurchase/MethodCallHandlerTest.java
Outdated
Show resolved
Hide resolved
...chase/example/android/app/src/test/java/io/flutter/plugins/inapppurchase/TranslatorTest.java
Show resolved
Hide resolved
packages/in_app_purchase/lib/src/billing_client_wrappers/purchase_wrapper.dart
Show resolved
Hide resolved
packages/in_app_purchase/lib/src/billing_client_wrappers/purchase_wrapper.dart
Outdated
Show resolved
Hide resolved
packages/in_app_purchase/lib/src/in_app_purchase/in_app_purchase_connection.dart
Outdated
Show resolved
Hide resolved
if (status == PurchaseStatus.purchased || | ||
status == PurchaseStatus.error) { | ||
pendingCompletePurchase = true; | ||
} | ||
} | ||
if (_platform == _kPlatformAndroid) { | ||
if (status == PurchaseStatus.purchased) { |
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.
see the response on https://github.com/flutter/plugins/pull/2287/files#r349738652
packages/in_app_purchase/lib/src/in_app_purchase/purchase_details.dart
Outdated
Show resolved
Hide resolved
packages/in_app_purchase/lib/src/in_app_purchase/purchase_details.dart
Outdated
Show resolved
Hide resolved
this.billingClientPurchase = null { | ||
this.status = SKTransactionStatusConverter() | ||
.toPurchaseStatus(transaction.transactionState); | ||
_platform = _kPlatformIOS; |
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.
done
@@ -158,6 +158,16 @@ public void fromBillingResult() throws JSONException { | |||
assertEquals(billingResultMap.get("debugMessage"), newBillingResult.getDebugMessage()); | |||
} | |||
|
|||
@Test | |||
public void fromBillingResult_dubugMessageNull() throws JSONException { |
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.
nit:
public void fromBillingResult_dubugMessageNull() throws JSONException { | |
public void fromBillingResult_debugMessageNull() throws JSONException { |
packages/in_app_purchase/lib/src/billing_client_wrappers/billing_client_wrapper.dart
Show resolved
Hide resolved
/// | ||
/// Use these in `@JsonSerializable()` classes by annotating them with | ||
/// `@BillingResponseConverter()`. | ||
// Use these in `@JsonSerializable()` classes by annotating them with |
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.
nit: Why deleting the paragraph breaks here and below? I think they probably made sense as-is.
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.
done
if (purchase.billingClientPurchase.isAcknowledged) { | ||
return BillingResultWrapper(responseCode: BillingResponse.ok); | ||
} | ||
return await billingClient.acknowledgePurchase( | ||
purchase.verificationData.serverVerificationData, | ||
developerPayload: developerPayload); |
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 is a nice thing to have for the developer in case they accidentally call this twice, but I would rather just defer to the underlying SDK at all times here instead of trying to special case based off of what we think the state of this purchase really is. I don't want to risk introducing any bugs based on our state being somehow out of sync with the reality on the platform.
packages/in_app_purchase/lib/src/in_app_purchase/google_play_connection.dart
Show resolved
Hide resolved
/// | ||
/// Warning!Fail to call this method within 3 days of the purchase will result a refund on Android. | ||
/// Warning!Failure to call this method and get a successful response within 3 days of the purchase will result a refund on Android. |
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.
nit:
/// Warning!Failure to call this method and get a successful response within 3 days of the purchase will result a refund on Android. | |
/// Warning! Failure to call this method and get a successful response within 3 days of the purchase will result a refund on Android. |
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.
done
this.billingClientPurchase = null { | ||
this.status = SKTransactionStatusConverter() | ||
.toPurchaseStatus(transaction.transactionState); | ||
_platform = _kPlatformIOS; |
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.
nit: I think _platform
can also be set the same way here and below.
packages/in_app_purchase/lib/src/billing_client_wrappers/purchase_wrapper.dart
Show resolved
Hide resolved
if (status == PurchaseStatus.purchased || | ||
status == PurchaseStatus.error) { | ||
pendingCompletePurchase = true; | ||
} | ||
} | ||
if (_platform == _kPlatformAndroid) { | ||
if (status == PurchaseStatus.purchased) { |
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.
Sorry, that link doesn't work anymore. I think the PR may be too big. Do you mind copy/pasting it?
@@ -14,6 +14,7 @@ | |||
@Override | |||
public BillingClient createBillingClient(Context context, MethodChannel channel) { | |||
return BillingClient.newBuilder(context) | |||
.enablePendingPurchases() |
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.
I'm going to reach out offline to get some second opinions on this.
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.
Updated with suggestions. Waiting on enablePendingPurchases
API decision.
packages/in_app_purchase/lib/src/billing_client_wrappers/billing_client_wrapper.dart
Outdated
Show resolved
Hide resolved
packages/in_app_purchase/lib/src/billing_client_wrappers/billing_client_wrapper.dart
Outdated
Show resolved
Hide resolved
packages/in_app_purchase/lib/src/billing_client_wrappers/billing_client_wrapper.dart
Outdated
Show resolved
Hide resolved
packages/in_app_purchase/lib/src/in_app_purchase/in_app_purchase_connection.dart
Show resolved
Hide resolved
packages/in_app_purchase/lib/src/billing_client_wrappers/billing_client_wrapper.dart
Show resolved
Hide resolved
/// | ||
/// Use these in `@JsonSerializable()` classes by annotating them with | ||
/// `@BillingResponseConverter()`. | ||
// Use these in `@JsonSerializable()` classes by annotating them with |
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.
done
/// | ||
/// Warning!Fail to call this method within 3 days of the purchase will result a refund on Android. | ||
/// Warning!Failure to call this method and get a successful response within 3 days of the purchase will result a refund on Android. |
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.
done
packages/in_app_purchase/lib/src/in_app_purchase/google_play_connection.dart
Show resolved
Hide resolved
this.billingClientPurchase = null { | ||
this.status = SKTransactionStatusConverter() | ||
.toPurchaseStatus(transaction.transactionState); | ||
_platform = _kPlatformIOS; |
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.
done
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.
Everything here LG pending the enablePendingPurchases
question.
Small nit, looks like some new analyzer failures:
info - Don't explicitly initialize variables to null at lib/src/in_app_purchase/in_app_purchase_connection.dart:193:8 - (avoid_init_to_null)
info - Don't explicitly initialize variables to null at lib/src/in_app_purchase/in_app_purchase_connection.dart:205:8 - (avoid_init_to_null)
packages/in_app_purchase/lib/src/billing_client_wrappers/billing_client_wrapper.dart
Show resolved
Hide resolved
Came to a decision offline. We'd like to expose the enablePendingPurchase API in both the Play and unified API layers in the plugin. However we'll make the iOS instance of the unified implementation of the API a no-op and not cause any issues if it's not called when running on iOS. |
@mklim Added a draft of the new pendingPurchaseAPI, do you want to take a look and see how you like it? |
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.
LGTM!
nit: it looks like the unit tests are failing and need to be updated.
@@ -1,6 +1,8 @@ | |||
## 0.3.0 | |||
|
|||
* Migrate the `Google Play Library` to 2.0.3. | |||
* **[Breaking Change]:** Added `enablePendingPurchases` in `InAppPurchaseConnection`. The application has |
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.
nit: I think a link to the Play documentation on enablePendingPurchases
would be useful here.
@@ -9,6 +9,7 @@ import 'package:in_app_purchase/in_app_purchase.dart'; | |||
import 'consumable_store.dart'; | |||
|
|||
void main() { | |||
InAppPurchaseConnection.enablePendingPurchases(); |
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.
nit: Comment here explaining what this is and linking to the Play documentation would be useful.
@@ -71,6 +73,17 @@ class BillingClient { | |||
Future<bool> isReady() async => | |||
await channel.invokeMethod<bool>('BillingClient#isReady()'); | |||
|
|||
/// Enable the [BillingClientWrapper] to handle pending purchases. | |||
/// | |||
/// This method is required to be called when initialize the application. |
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.
Small optional wording nit:
/// This method is required to be called when initialize the application. | |
/// Play requires that you call this method when initializing your application. |
/// This method is required to be called when initialize the application. | ||
/// It is to acknowledge your application has been updated to support pending purchases. | ||
/// See [Support pending transactions](https://developer.android.com/google/play/billing/billing_library_overview#pending) | ||
/// for more details. |
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.
nit: paragraph break below would make sense, I think.
Offline discussion with @amirh. Because the overhead of a 2-phase* release for this breaking change is very big and the usage of the plugin is low. We've decided to let this change to be a 1-phase* update. The breaking changes will be described in the CHANGELOG. 2-phase: introduce a minor update while deprecating the old methods, then follow up with a major update which delete the deprecated methods. In this particular case, it is hard to do since we will need to mock many functionalities without updating Android's Play Billing Library to 2.0, which can result unwanted behaviors and confusions. 1-phase: directly introduce a major update with all the breakages. |
LGTM to land this without a deprecation phase, since the platform library didn't land their breaking change with a deprecation doing so in our side is complex and potentially error prone, and as the plugin is still relatively new we expect the ecosystem impact to not be too wide. |
…acheing-01-08 * flutterPlugin/master: (30 commits) Update Gradle version (flutter#2448) [image_picker] support android V2 embedding (flutter#2430) [webview_flutter] Setup XCTests (flutter#2445) [video_player] Fixes video initialization future stall. (flutter#2134) [ci] Upgrade to Xcode 11.3 (flutter#2435) [In_app_purchases] migrate to Play Billing Library 2.0. (flutter#2287) Migrate away from deprecated `BinaryMessages` (flutter#2444) [google_sign_in]Update google_sign_in_example name in pubspec.yaml (flutter#2335) [ios_platform_images] Removed android support from the pubspec. (flutter#2432) [google_sign_in] Expose network error (flutter#2398) [battery] cleanup for Android embedding post 1.12 (flutter#2400) [flutter_webview] Raise min Flutter SDK to stable (flutter#2425) re-enable stable CI (flutter#2402) [in_app_purchase]Change a comment. (flutter#2329) [google_sign_in] Pass the client id to the platform interface. (flutter#2427) [ios_platform_images] Made ios_platform_images set the correct image scale. (flutter#2414) [url_launcher_platform_interface] use non static token for platform interface (flutter#2418) [plugin_platform_interface] Don't use const Object as a token (flutter#2417) Update endorsed macos plugins readme and update others (flutter#2407) [webview_flutter] add gesture navigation for iOS (flutter#2339) ... # Conflicts: # packages/video_player/video_player/CHANGELOG.md # packages/video_player/video_player/pubspec.yaml
@@ -361,28 +365,32 @@ class _MyAppState extends State<MyApp> { | |||
|
|||
void _listenToPurchaseUpdated(List<PurchaseDetails> purchaseDetailsList) { | |||
purchaseDetailsList.forEach((PurchaseDetails purchaseDetails) async { | |||
await InAppPurchaseConnection.instance.consumePurchase(purchaseDetails); |
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.
consumePurchase is only for Play Store. This line should not be here when running on iOS or during error case as you won't be able to consume the purchaseDetails
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.
Good catch! This was added for a manual testing. It shouldn't be here.
if (purchaseDetails.status == PurchaseStatus.pending) { | ||
showPendingUI(); | ||
} else { | ||
if (purchaseDetails.status == PurchaseStatus.error) { | ||
handleError(purchaseDetails.error); | ||
return; |
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.
Also on iOS completePurchase() needs to be called for status == PurchaseStatus.error. With this change this will be broken.
Please refer to completePurchase documentation.
plugins/packages/in_app_purchase/lib/src/in_app_purchase/in_app_purchase_connection.dart
Line 197 in ed7620c
/// the purchase needs to be completed if the [PurchaseDetails.status] is [PurchaseStatus.error]. |
} else if (purchaseDetails.status == PurchaseStatus.purchased) { | ||
bool valid = await _verifyPurchase(purchaseDetails); | ||
if (valid) { | ||
deliverProduct(purchaseDetails); | ||
} else { | ||
_handleInvalidPurchase(purchaseDetails); | ||
return; |
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.
I am guessing this will have the same impact as line 374.
@malsabbagh Thanks for the review. Do you mind putting up a PR to fix those? :) |
@cyanglaz sure I am happy to do it :) |
@malsabbagh That's great! Please ping me in the PR once you open it, I will be happy to review it! |
Description
This PR migrates the IAP plugin to BillingClient 2.0 on Android.
It contains changes described below:
This PR also contains some documentation updates based on the BillingClient v2 changes.
Link to the BillingClient v2 release note: https://developer.android.com/google/play/billing/billing_library_releases_notes#release-2_0
IMPORTANT a breaking change announcement following the breaking change protocol has to be made prior to landing this PR
The desgin doc including all the public API change details can be found here:
https://docs.google.com/document/d/1XM16UsLE_aPWoZnheE9waO06mhxLkkWjpPf9jtI1AdY/edit?usp=sharing
Related Issues
flutter/flutter#37021
flutter/flutter#37821 and flutter/flutter#40956 should also be fixed since
enablePendingPurchases()
is called by default after this change.Checklist
Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes (
[x]
). This will ensure a smooth and quick review process.///
).flutter analyze
) does not report any problems on my PR.Breaking Change
Does your PR require plugin users to manually update their apps to accommodate your change?