Skip to content
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

Conversation

reidbaker
Copy link
Contributor

@reidbaker reidbaker commented Feb 19, 2024

Add UserChoiceBilling billing mode option.

Fixes flutter/flutter/issues/143004

Left in draft until:

  • connect, disconnect and set user mode test is added to java
  • dart tests for method channel
  • Callback api added to dart
  • Test for call back api
  • Example using callback api in example app

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

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@reidbaker reidbaker changed the title [in_app_purchase_android] [in_app_purchase_android] Add UserChoiceBilling mode. Feb 19, 2024
@reidbaker reidbaker marked this pull request as ready for review February 22, 2024 20:50
// https://developer.android.com/google/play/billing/alternative/alternative-billing-without-user-choice-in-app
builder.enableAlternativeBillingOnly();
break;
case BillingChoiceMode.USER_CHOICE_BILLING:
Copy link
Contributor

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?

Copy link
Contributor Author

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);
Copy link
Contributor

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?

Copy link
Contributor Author

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()));
Copy link
Contributor

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?

Copy link
Contributor Author

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());
Copy link
Contributor

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)?

Copy link
Contributor Author

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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Play

Copy link
Contributor Author

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.

@reidbaker reidbaker requested a review from a team March 8, 2024 19:22
Copy link
Member

@gmackall gmackall left a 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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit

Suggested change
* Adds UserChoiceBilling API's to platform addition.
* Adds UserChoiceBilling APIs to platform addition.

Copy link
Contributor Author

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
Copy link
Member

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

Copy link
Contributor Author

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",
Copy link
Member

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

Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

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

.

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

Copy link
Contributor Author

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.

builder.enableUserChoiceBilling(userChoiceBillingListener);
} else {
Log.e(
"BillingClientFactoryImpl",
Copy link
Member

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

.

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>
Copy link
Member

@gmackall gmackall left a 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

@reidbaker reidbaker added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 8, 2024
@auto-submit auto-submit bot merged commit d489d84 into flutter:main Mar 8, 2024
78 checks passed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 11, 2024
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Mar 11, 2024
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
@fareesh
Copy link

fareesh commented Mar 18, 2024

LGTM

@reidbaker reidbaker deleted the i143004-in-app-purchase-user-choice-billing-intro branch April 19, 2024 15:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App p: in_app_purchase platform-android
Projects
None yet
4 participants