-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
[in_app_purchase_android] Add UserChoiceBilling mode. #6162
[in_app_purchase_android] Add UserChoiceBilling mode. #6162
Conversation
…e billing info, define callbacks
// https://developer.android.com/google/play/billing/alternative/alternative-billing-without-user-choice-in-app | ||
builder.enableAlternativeBillingOnly(); | ||
break; | ||
case BillingChoiceMode.USER_CHOICE_BILLING: |
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.
Should this have a docs link too, like the above?
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
// Do nothing | ||
break; | ||
default: | ||
Log.w("BillingClientFactoryImpl", "Unknown BillingChoiceMode " + billingChoiceMode); |
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.
Shouldn't this be .e
, and also mention that it's doing fallback to Play-only?
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
HashMap<String, Object> info = new HashMap<>(); | ||
info.put("externalTransactionToken", userChoiceDetails.getExternalTransactionToken()); | ||
info.put("originalExternalTransactionId", userChoiceDetails.getOriginalExternalTransactionId()); | ||
info.put("productsList", fromProductsList(userChoiceDetails.getProducts())); |
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 Dart object's field is just called products
; is List
added automagically by JSON serialization, or is this a mismatch?
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.
Confirmed mismatch and added test.
static HashMap<String, Object> fromProduct(Product product) { | ||
HashMap<String, Object> info = new HashMap<>(); | ||
info.put("productId", product.getId()); | ||
info.put("productOfferToken", product.getOfferToken()); |
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.
Why do these two have an extra "product" prefix relative to Dart (but the type below matches directly)?
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.
Mismatch, test added.
@@ -507,6 +532,10 @@ enum BillingChoiceMode { | |||
/// Billing through app provided flow. | |||
@JsonValue(1) | |||
alternativeBillingOnly, | |||
|
|||
/// Users can choose play billing or alternative billing. |
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: Play
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 and fixed some other locations in this file.
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.
partial (java side)
|
||
* Adds UserChoiceBilling API's to platform addition. |
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
* Adds UserChoiceBilling API's to platform addition. | |
* Adds UserChoiceBilling APIs to platform addition. |
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
} | ||
break; | ||
case BillingChoiceMode.PLAY_BILLING_ONLY: | ||
// Do nothing |
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: end comment with period
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
builder.enableUserChoiceBilling(userChoiceBillingListener); | ||
} else { | ||
Log.e( | ||
"BillingClientFactoryImpl", |
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.
What do you think about enforcing this being an error case, instead of defaulting? I think I might be confusing for developers to default to PLAY_BILLING_ONLY
, even if they have made a mistake in how they configured for USER_CHOICE_BILLING
.
Not a strong preference, though, as long as the messaging is clear
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.
After reading further, we should never reach this case right? We define the listener, and we basically do it as a pass through where we pass the UserChoiceDetails
in a copied hashmap to the dart side?
And the only time that listener is null is when the BillingChoiceMode
is not USER_CHOICE_BILLING
?
Can't we instead move that logic where we construct the listener (lines 547-557 in MethodCallHandlerImpl
) inside this class and only invoke it when we actually need to make the listener, so it is never null?
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 agree that we should not every hit this case as the code is currently written but I added the additional fallaback logic to protect against future bugs and give us a way to know that we were in a bad state.
I didnt want to create the listener in this class because it needed to invoke methodChannel and I thought the methodchannelimpl class would be where you would look for that logic.
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.
Part of why I suggested this is that we have existing code where we do something similar to construct a listener in this plugin, see
Line 39 in a10b360
channel.invokeMethod(ON_PURCHASES_UPDATED, callbackArgs); |
I still lean towards moving the listener construction, so we can eliminate the bad case here, but if you have a strong preference to keep the structure as is I'm fine that way too
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.
How about a compromise. I am worried about the march 13th deadline for apps to
migrate to this api. I have filed flutter/flutter#144851 and assigned it to myself along with some other cleanup bugs that I can do without impacting the public facing api.
..._app_purchase_android/android/src/main/java/io/flutter/plugins/inapppurchase/Translator.java
Show resolved
Hide resolved
...purchase/in_app_purchase_android/lib/src/billing_client_wrappers/billing_client_wrapper.dart
Outdated
Show resolved
Hide resolved
builder.enableUserChoiceBilling(userChoiceBillingListener); | ||
} else { | ||
Log.e( | ||
"BillingClientFactoryImpl", |
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.
Part of why I suggested this is that we have existing code where we do something similar to construct a listener in this plugin, see
Line 39 in a10b360
channel.invokeMethod(ON_PURCHASES_UPDATED, callbackArgs); |
I still lean towards moving the listener construction, so we can eliminate the bad case here, but if you have a strong preference to keep the structure as is I'm fine that way too
…ng_client_wrappers/billing_client_wrapper.dart Co-authored-by: Gray Mackall <34871572+gmackall@users.noreply.github.com>
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 with flutter/flutter#144851 as a follow up, considering deadline for developers to adopt these new apis is soon
flutter/packages@0badb43...d489d84 2024-03-08 reidbaker@google.com [in_app_purchase_android] Add UserChoiceBilling mode. (flutter/packages#6162) 2024-03-08 engine-flutter-autoroll@skia.org Roll Flutter from 471a828 to 7c89ec8 (15 revisions) (flutter/packages#6288) 2024-03-08 engine-flutter-autoroll@skia.org Roll Flutter (stable) from 7482962 to ba39319 (2 revisions) (flutter/packages#6287) 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 flutter-ecosystem@google.com,rmistry@google.com 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://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
LGTM |
Add UserChoiceBilling billing mode option. Fixes flutter/flutter/issues/143004 Left in draft until: This does not have an End to end working example with play integration. I am currently stuck at the server side play integration part.
Add UserChoiceBilling billing mode option.
Fixes flutter/flutter/issues/143004
Left in draft until:
This does not have an End to end working example with play integration. I am currently stuck at the server side play integration part.
Pre-launch Checklist
dart format
.)[shared_preferences]
pubspec.yaml
with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.CHANGELOG.md
to 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.