-
Notifications
You must be signed in to change notification settings - Fork 159
[Draft][EMT-2431] - Google Play Billing Library Modularization #1323
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
base: master
Are you sure you want to change the base?
Conversation
ReferenceSDK-2431 -- Emt 2431 - Draft PR. DescriptionSummary By MatterAI
🔄 What ChangedThis pull request executes the complete removal of the legacy 🔍 Impact of the ChangeBy removing the legacy V6 module, the project's technical debt is significantly reduced, and the build graph is simplified. This ensures that the SDK exclusively uses the newer Billing V8/V7 paths, preventing accidental usage of deprecated APIs. Note: The previously identified issues in the V8 implementation (empty listeners and instance creation bugs) still require attention as they are not addressed by this cleanup PR. 📁 Total Files ChangedClick to Expand
🧪 Test Added/RecommendedAddedN/A (This PR focuses on code removal). Recommended
🔒Security VulnerabilitiesN/A Testing InstructionsPerform a clean build of the project and verify that no references to the Risk Assessment [
|
|
Important PR Review SkippedPR review skipped as per the configuration setting. Run a manually review by commenting /matter review 💡Tips to use MatterAICommand List
|
| @@ -0,0 +1,40 @@ | |||
| plugins { | |||
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.
Are these gradle files necessary to get the library version?
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.
You should be able to do this with
String billingClientVersion = com.android.billingclient.BuildConfig.VERSION_NAME;
in Java (or Kotlin) instead
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.
You mean having that line in the main gradle file instead? So I'll remove the product flavors.
Doe that replace this? :compileOnly("com.android.billingclient:billing:6.0.1")
Branch-SDK/build.gradle.kts
Outdated
| isRequired = isReleaseBuild() | ||
| } | ||
|
|
||
| flavorDimensions.add("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.
Let's keep things simple and just check the buildconfig version string and conditionally execute the right library implementation.
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.
Got it. Removing flavor dimensions
settings.gradle.kts
Outdated
| @@ -1,5 +1,7 @@ | |||
| include(":Branch-SDK") | |||
| include(":Branch-SDK-TestBed") | |||
| include (":BillingGooglePlayModules:BillingV6V7", ":BillingGooglePlayModules:BillingV8") | |||
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.
Same as above, revert 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.
Removed this as well
| public class BillingGooglePlayReflection { | ||
| public static BillingGooglePlayInterface getBillingLibraryVersion() { | ||
| try { | ||
| // Check for a class added in version 8.0 or higher |
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 can be hopefully be simplified with my suggestion. Just parse the major version
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.
There isn't a specific method to grab the version so the checks look for what's available in the version that is being used.
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.
Just realizing that was for the reflection section. Replacing it!
|
Important PR Review SkippedPR review skipped as per the configuration setting. Run a manually review by commenting /matter review 💡Tips to use MatterAICommand List
|
|
|
||
| public class BillingGooglePlayReflection { | ||
| public static BillingGooglePlayInterface getBillingLibraryVersion() { | ||
| String billingClient = com.android.billingclient.BuildConfig.VERSION_NAME; |
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.
Make the variable name a bit more relevant like billingClientVersion
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.
Renamed to billingClientVersionString
|
|
||
| try { | ||
| int majorIndex = billingClient.indexOf("."); | ||
| String majorVersion = billingClient.substring(0, majorIndex); |
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.
Might be worth making this version extraction bit into a utility function since this could be used in other places as well.
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.
Created function called dependencyMajorVersionFinder and added it to DependencyUtils.kt
Function takes in a string with the depenency name and pulls out the major version number.
Kept the Version Name extract itself in the BillingGooglePlayReflection class so that the function can take in any method of calling the version name depending on what the class offers.
| } | ||
|
|
||
| } catch (Exception e) { | ||
| System.err.println("Error parsing billing client version: " + e.getMessage()); |
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.
Use BranchLogger.e instead of System.err.println
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.
Changed to BranchLogger.e
Branch-SDK/build.gradle.kts
Outdated
| toolVersion = "0.8.10" | ||
| } | ||
|
|
||
| //configurations.all { |
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.
Delete 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.
Deleted
|
Important PR Review SkippedPR review skipped as per the configuration setting. Run a manually review by commenting /matter review 💡Tips to use MatterAICommand List
|
1 similar comment
|
Important PR Review SkippedPR review skipped as per the configuration setting. Run a manually review by commenting /matter review 💡Tips to use MatterAICommand List
|
|
Hey @gdeluna-branch I updated all of your comments and also added the V8 logic. Is there a better way to reference them in the BillingGooglePlayReflection.java file? Also, I'm hesitant to delete the BillingGooglePlay.kt file until everything is done and working. Also still need to build the gradle to reference each version properly. |
dfcdcf2 to
c4e85a7
Compare
|
Important PR Review SkippedPR review skipped as per the configuration setting. Run a manually review by commenting /matter review 💡Tips to use MatterAICommand List
|
Reference
EMT 2431 -- Google Play Billing Library Modularization.
Description
Added in the main structure of the potential update for Android Billing Library using Reflection
Testing Instructions
Risk Assessment [
HIGH||MEDIUM||LOW]High Risk as this may break android billing entirely if not done correctly.
Reviewer Checklist (To be checked off by the reviewer only)
cc @BranchMetrics/saas-sdk-devs for visibility.