[IAP] Check if the payment processor is available by mklim · Pull Request #1057 · flutter/plugins

mklim · 2019-01-09T22:16:50Z

The actual functionality here is extremely minimal. Most of this patch
is establishing the structure of the rest of the plugin code to follow.

Add a generic IAP plugin interface with platform-specific
implementations (*_connection.dart.)
Add platform specific wrappers (*_wrappers.dart).
Add dart end to end tests covering the sample app and unit tests
covering the native platform code. These all need to be run manually
currently.

Fixes flutter/flutter#26321
flutter/flutter#25987

mklim · 2019-01-09T22:18:13Z

I'm happy to break this up into smaller PRs if that would make it easier to review, let me know. I think there's a couple ways this could be split and it's a bit on the large side right now.

packages/in_app_purchase/lib/src/in_app_purchase_connection.dart

amirh · 2019-01-10T17:58:01Z

packages/in_app_purchase/example/lib/main.dart

    // If the widget was removed from the tree while the asynchronous platform
    // message was in flight, we want to discard the reply rather than calling
    // setState to update our non-existent appearance.
    if (!mounted) return;


WDYT about using a FutureBuilder instead of having this kind of logic explicitly here? (it will also makes it more clear where to to handle future error).

[Actually looking at this I'm thinking we might want to update the plugin template to use a FutureBuilder]

Fixed here, filed flutter/flutter#26404 for the more broad issue.

amirh · 2019-01-10T18:00:23Z

packages/in_app_purchase/example/lib/main.dart

+    final Widget storeHeader = buildListCard(ListTile(
+        leading: Icon(_storeReady ? Icons.check : Icons.block),
+        title:
+            Text('The store is ' + (_storeReady ? 'open' : 'closed') + '.')));


On a first look this made me think like this is some state we're getting from the IAP server (e.g user can configure their store to be "close")

Changed to "available."

packages/in_app_purchase/example/lib/main.dart

amirh · 2019-01-10T18:21:33Z

packages/in_app_purchase/lib/src/in_app_purchase_connection.dart

+  ///
+  /// Throws an [UnsupportedError] when accessed on a platform other than
+  /// Android or iOS.
+  InAppPurchaseConnection connection = _createConnection();


Is this connection something an app might want to keep open only part of the time? Does it ever makes sense to close it? Is there a scenario where it would make sense to have multiple different connections?

packages/in_app_purchase/ios/Classes/InAppPurchasePlugin.m

amirh · 2019-01-10T19:17:34Z

...app_purchase/android/src/main/java/io/flutter/plugins/inapppurchase/InAppPurchasePlugin.java

+                  public void onPurchasesUpdated(
+                      int responseCode, @Nullable List<Purchase> purchases) {}
+                })
+            .build();


Just making sure - this have no side effects right?

Not that I'm aware of, besides creating the instance of the class in memory.

...app_purchase/android/src/main/java/io/flutter/plugins/inapppurchase/InAppPurchasePlugin.java

...mple/android/app/src/test/java/io/flutter/plugins/inapppurchase/InAppPurchasePluginTest.java

packages/in_app_purchase/example/ios/in_app_purchase_pluginTests/in_app_purchase_pluginTests.m

mklim

Is this connection something an app might want to keep open only part of the time? Does it ever makes sense to close it? Is there a scenario where it would make sense to have multiple different connections?

Good catch, I missed this. There's some larger changes in the latest commit related to this. I added handlers to close the connection. Then on second thought I was thinking that the start/stop connection was an Android specific thing that didn't really need to be in the main interface. I'm thinking that feasibly, the Android implementation of the interface could just handle those calls internally. I changed it over in the latest patch but I'd appreciate a second opinion. I think it may be a little bit too much "magic," even for the top layer that's supposed to be really simple to use.

mklim · 2019-01-10T23:00:30Z

...app_purchase/android/src/main/java/io/flutter/plugins/inapppurchase/InAppPurchasePlugin.java

+                  public void onPurchasesUpdated(
+                      int responseCode, @Nullable List<Purchase> purchases) {}
+                })
+            .build();


Not that I'm aware of, besides creating the instance of the class in memory.

mklim · 2019-01-11T02:24:47Z

packages/in_app_purchase/example/lib/main.dart

+    final Widget storeHeader = buildListCard(ListTile(
+        leading: Icon(_storeReady ? Icons.check : Icons.block),
+        title:
+            Text('The store is ' + (_storeReady ? 'open' : 'closed') + '.')));


Changed to "available."

mklim · 2019-01-11T02:28:56Z

packages/in_app_purchase/example/lib/main.dart

    // If the widget was removed from the tree while the asynchronous platform
    // message was in flight, we want to discard the reply rather than calling
    // setState to update our non-existent appearance.
    if (!mounted) return;


Fixed here, filed flutter/flutter#26404 for the more broad issue.

packages/in_app_purchase/example/lib/main.dart

packages/in_app_purchase/ios/Classes/InAppPurchasePlugin.m

mklim · 2019-01-11T02:43:12Z

packages/in_app_purchase/lib/src/billing_client_wrappers.dart

+      MethodChannel('plugins.flutter.io/in_app_purchase');
+  List<Map<String, Function>> _callbacks = <Map<String, Function>>[];
+
+  /// Wraps `BillingClient#isReady()`.


I think the native documentation for the native APIs these wrappers are calling through to is going to be much more useful than me trying to write summary Dart docs here. I'd rather just point people to the underlying class documentation with wrapper specific notes when it's worthwhile.

mklim · 2019-01-11T02:45:17Z

packages/in_app_purchase/lib/src/billing_client_wrappers.dart

+  /// [onBillingServiceConnected] has been converted from a callback parameter
+  /// to the Future result returned by this function. This returns the
+  /// `BillingClient.BillingResponse` `responseCode` of the connection result.
+  Future<BillingResponse> startConnection(


Fixed in the latest patch, these are now also linked to BillingClient instance creation and destruction on the native side.

mklim · 2019-01-11T03:04:05Z

packages/in_app_purchase/lib/src/in_app_purchase_connection.dart

+/// Basic generic API for making in app purchases across multiple platforms.
+abstract class InAppPurchaseConnection {
+  /// Returns true if the payment platform is ready and available.
+  Future<bool> isAvailable();


I'd want to set up long polling to make sure it didn't go totally stale on the Android side. Maybe I could add it as a configurable option? I'd rather do this in a followup PR. For iOS this is always stable, so I could cache it.

amirh

We should probably also add Dart unit tests.

amirh · 2019-01-11T23:21:14Z

packages/in_app_purchase/lib/src/in_app_purchase_connection.dart

+/// Basic generic API for making in app purchases across multiple platforms.
+abstract class InAppPurchaseConnection {
+  /// Returns true if the payment platform is ready and available.
+  Future<bool> isAvailable();


Would it be possible rely on the BillingClientStateListener callbacks to notify us when we're ready/not ready or is "billing client setup complete" is different than ready?

amirh · 2019-01-11T23:32:18Z

packages/in_app_purchase/lib/src/google_play_connection.dart

+  void didChangeAppLifecycleState(AppLifecycleState state) {
+    switch (state) {
+      case AppLifecycleState.paused:
+      case AppLifecycleState.suspending:


I believe paused is currently called on Android's onStop, and that suspending is not currently invoked, just making sure that this is what we want here...

Got it, removed.

amirh · 2019-01-11T23:51:33Z

packages/in_app_purchase/lib/src/google_play_connection.dart

+class GooglePlayConnection
+    with WidgetsBindingObserver
+    implements InAppPurchaseConnection {
+  GooglePlayConnection() : _billingClient = BillingClient() {


should we make this a singleton as well? or is there a legit reason to have multiple of these?

No reason, changed.

packages/in_app_purchase/lib/in_app_purchase.dart

packages/in_app_purchase/lib/src/store_kit_wrappers.dart

...app_purchase/android/src/main/java/io/flutter/plugins/inapppurchase/InAppPurchasePlugin.java

amirh · 2019-01-12T00:06:08Z

packages/in_app_purchase/lib/src/billing_client_wrappers.dart

+      MethodChannel('plugins.flutter.io/in_app_purchase');
+  List<Map<String, Function>> _callbacks = <Map<String, Function>>[];
+
+  /// Wraps `BillingClient#isReady()`.


ok.
[nit: lets link to the Javadoc at least to make it easier to jump when you look at these docs]

amirh · 2019-01-12T00:08:56Z

packages/in_app_purchase/lib/src/billing_client_wrappers.dart

+  /// [onBillingServiceConnected] has been converted from a callback parameter
+  /// to the Future result returned by this function. This returns the
+  /// `BillingClient.BillingResponse` `responseCode` of the connection result.
+  Future<BillingResponse> startConnection(


IIUC there is no reason for 2 startConnection's to be in flight simultaneously?
I'm not sure I understand why do we need this list of callback maps and not just callback set for the current connection?

packages/in_app_purchase/example/lib/main.dart

mklim

We should probably also add Dart unit tests.

Yeah, originally the Dart classes weren't really doing anything much but now GooglePlayConnection especially should probably be tested. Do you mind if this is done as a followup though? I'd rather unblock the other IAP work sooner, and this patch is already large enough where it's hard to review.

mklim · 2019-01-12T00:33:48Z

packages/in_app_purchase/lib/src/billing_client_wrappers.dart

+      MethodChannel('plugins.flutter.io/in_app_purchase');
+  List<Map<String, Function>> _callbacks = <Map<String, Function>>[];
+
+  /// Wraps `BillingClient#isReady()`.


mklim · 2019-01-12T00:35:33Z

packages/in_app_purchase/lib/src/billing_client_wrappers.dart

+  /// [onBillingServiceConnected] has been converted from a callback parameter
+  /// to the Future result returned by this function. This returns the
+  /// `BillingClient.BillingResponse` `responseCode` of the connection result.
+  Future<BillingResponse> startConnection(


I think that would work for now, but there's going to be other methods on BillingClient that require callbacks back up into Dart eventually. I figured this way I'd get the structure out of the way to start with.

mklim · 2019-01-12T00:47:08Z

packages/in_app_purchase/lib/src/google_play_connection.dart

+class GooglePlayConnection
+    with WidgetsBindingObserver
+    implements InAppPurchaseConnection {
+  GooglePlayConnection() : _billingClient = BillingClient() {


No reason, changed.

mklim · 2019-01-12T00:48:03Z

packages/in_app_purchase/lib/src/google_play_connection.dart

+  void didChangeAppLifecycleState(AppLifecycleState state) {
+    switch (state) {
+      case AppLifecycleState.paused:
+      case AppLifecycleState.suspending:


Got it, removed.

mklim · 2019-01-12T01:04:25Z

packages/in_app_purchase/lib/src/in_app_purchase_connection.dart

+/// Basic generic API for making in app purchases across multiple platforms.
+abstract class InAppPurchaseConnection {
+  /// Returns true if the payment platform is ready and available.
+  Future<bool> isAvailable();


I don't think so, but I can investigate for a followup. The only guarantee for BillingClientStateListener is that it's talking about setup being complete and successful, specifically. Practically I think it's probably tied to isReady but I'm not totally sure. I'd also still need at least one async call here to set it up.

amirh

LGTM but I still think we should include Dart unit tests as part of this PR as future work will probably need that unit test skeleton anyway, and as in general we try to avoid submitting untested code.

amirh · 2019-01-12T01:43:14Z

packages/in_app_purchase/lib/src/billing_client_wrappers.dart

+  /// [onBillingServiceConnected] has been converted from a callback parameter
+  /// to the Future result returned by this function. This returns the
+  /// `BillingClient.BillingResponse` `responseCode` of the connection result.
+  Future<BillingResponse> startConnection(


amirh · 2019-01-12T01:45:01Z

packages/in_app_purchase/lib/src/in_app_purchase_connection.dart

+/// Basic generic API for making in app purchases across multiple platforms.
+abstract class InAppPurchaseConnection {
+  /// Returns true if the payment platform is ready and available.
+  Future<bool> isAvailable();


ok, lets try to verify that before we first publish (we should be able to return false before we set up the callbacks so this could be synchronous)

The actual functionality here is extremely minimal. Most of this patch is establishing the structure of the rest of the plugin code to follow. 1. Add a generic IAP plugin interface with platform-specific implementations (`*_connection.dart`.) 2. Add platform specific wrappers (`*_wrappers.dart`). 3. Add dart end to end tests covering the sample app and unit tests covering the native platform code. These all need to be run manually currently.

It really only applies to Play, and the wrapper layer can manage this state itself. Adds an `endConnection()` call to `BillingClientWrapper`. Also some minor code readability changes.

mklim · 2019-01-14T21:33:23Z

LGTM but I still think we should include Dart unit tests as part of this PR as future work will probably need that unit test skeleton anyway, and as in general we try to avoid submitting untested code.

OK, done. I was going to rearrange the dart files into a more fine grained structure in a followup PR, but I think with adding in dart unit tests it also became important to make sure the structure here made more sense now. d14bac6 moves the dart files around and the new tests are in 1f61bf2.

amirh · 2019-01-15T19:26:48Z

packages/in_app_purchase/test/fake_platform_views_controller.dart

+import 'dart:async';
+import 'package:flutter/services.dart';
+
+class FakePlatformViewsController {


I'd expect a "FakePlatformViewsController" to be a fake implementation of the engine's PlatformViewsController

amirh · 2019-01-15T19:33:05Z

packages/in_app_purchase/test/billing_client_wrappers/billing_client_wrapper_test.dart

+
+  setUpAll(() {
+    Channel.override = SystemChannels.platform_views;
+    SystemChannels.platform_views.setMockMethodCallHandler(


Why override the platform_views system channel? is it used by this plugin at all?

Can we not just set a mock handler for the plugins.flutter.io/in_app_purchase channel? and avoid the indirection added with the Channel class?

mklim · 2019-01-15T21:47:50Z

Following up in #1082.

The actual functionality here is extremely minimal. Most of this patch is establishing the structure of the rest of the plugin code to follow. 1. Add a generic IAP plugin interface with platform-specific implementations (`*_connection.dart`.) 2. Add platform specific wrappers (`*_wrappers.dart`). 3. Add dart end to end tests covering the sample app and unit tests covering dart and the platform code. These all need to be run manually currently. Fixes flutter/flutter#26321 flutter/flutter#25987

…)" This reverts commit e65f594.

mklim requested review from amirh and cyanglaz January 9, 2019 22:16

googlebot added the cla: yes label Jan 9, 2019

amirh reviewed Jan 10, 2019

View reviewed changes

mklim commented Jan 11, 2019

View reviewed changes

amirh reviewed Jan 12, 2019

View reviewed changes

mklim commented Jan 12, 2019

View reviewed changes

amirh reviewed Jan 12, 2019

View reviewed changes

Michael Klimushyn added 4 commits January 14, 2019 09:58

Fix pub publish errors

c60cbb0

Move connection out of the generic interface

b2db185

It really only applies to Play, and the wrapper layer can manage this state itself. Adds an `endConnection()` call to `BillingClientWrapper`. Also some minor code readability changes.

Review fixup.

d52b57e

mklim mentioned this pull request Jan 14, 2019

Iap productlist ios #1068

Merged

Michael Klimushyn added 2 commits January 14, 2019 10:39

Split dart classes into better directory structure.

d14bac6

Unit test skeleton.

1f61bf2

Review fixup.

1cc85af

mklim merged commit fe03e40 into flutter:master Jan 14, 2019

mklim deleted the iap_store_open branch January 14, 2019 22:30

mklim mentioned this pull request Jan 14, 2019

IAP: Create an example app with getting started documentation flutter/flutter#25987

Closed

8 tasks

amirh reviewed Jan 15, 2019

View reviewed changes

andreidiaconu added a commit to andreidiaconu/plugins that referenced this pull request Feb 17, 2019

Revert "[IAP] Check if the payment processor is available (flutter#1057…

b3b31d3

…)" This reverts commit e65f594.

Conversation

mklim commented Jan 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mklim commented Jan 9, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mklim left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mklim Jan 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amirh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mklim left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amirh left a comment

mklim commented Jan 9, 2019 •

edited

Loading

mklim Jan 11, 2019 •

edited

Loading