-
Notifications
You must be signed in to change notification settings - Fork 50
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
Purchase.Builder #825
Purchase.Builder #825
Conversation
…ors, lots of TODOs for rest of impl
import com.revenuecat.purchases.models.StoreProduct | ||
import com.revenuecat.purchases.models.SubscriptionOption | ||
|
||
open class Purchase(builder: Builder) { |
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 don't think we should name it this, too confusing with Purchases. will have to think of a better 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.
PurchaseParams
?
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, I would generate a toString
, equals
and hashCode
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 like that, done!
and just curious - is there a reason the PurchasesConfiguration doesn't need those methods?
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.
actually, i don't think there'd be any custom equality logic here.. unless i'm missing something..
} | ||
|
||
private val successfulUpgradeCallback: (purchase: StoreTransaction?, customerInfo: CustomerInfo) -> Unit = | ||
private val successfulPurchaseCallback: (purchase: StoreTransaction?, customerInfo: CustomerInfo) -> Unit = |
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.
during development/until i figure out the deprecation/renaming strategy, i'm just changing this type.
@@ -156,48 +139,6 @@ class OfferingFragment : Fragment(), PackageCardAdapter.PackageCardAdapterListen | |||
} | |||
} | |||
|
|||
private fun startPurchaseProduct( |
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 file is a cool showcase of how much our API is simplified..though i doubt customers have this kinda code exactly 😅
@@ -38,4 +38,6 @@ object PurchaseStrings { | |||
const val INVALID_PURCHASE_TYPE = "Purchase for a %s purchase must be a %s." | |||
const val INVALID_STORE_PRODUCT_TYPE = "StoreProduct for a %s purchase must be a %s." | |||
const val INVALID_PRODUCT_NO_PRICE = "Error finding a price for %s." | |||
const val NULL_TRANSACTION_ON_PURCHASE_ERROR = "Error purchasing. Null transaction returned from a non-upgrade" + |
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 shouldn't ever happen
…reate helper to translate callback
2d38454
to
3e95730
Compare
@@ -1372,7 +1407,33 @@ class Purchases internal constructor( | |||
} | |||
} | |||
|
|||
private fun getPurchaseCallback(productId: String): PurchaseCallback? { | |||
internal fun purchaseNonUpgradeWithDeprecatedCallback( |
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.
could probably do something like toPurchaseCallback did in the last release?
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 love this name, TBH 🙃
I think making it nullable is a good compromise |
fun purchase( | ||
purchaseConfig: Purchase, | ||
listener: NewPurchaseCallback | ||
) { |
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.
Love 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.
This is going to be so much nicer to maintain 💪
val oldProductId: String? | ||
val googleProrationMode: GoogleProrationMode | ||
internal val purchasingData: PurchasingData | ||
internal val activity: Activity |
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.
Friendly reminder to make sure this class' API looks good in Java. So far I think it should be fine
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.
it's been nice having the API tester, for some reason i held off on pushing that bc i wanted it in it's own commit 😓 but it's looking good so far!
val isPersonalizedPrice: Boolean | ||
val oldProductId: String? | ||
val googleProrationMode: GoogleProrationMode | ||
internal val purchasingData: PurchasingData |
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.
Don't forget to add JVMSynthetic to these internal properties
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 looks so good! I can't wait for this
fun isPersonalizedPrice(isPersonalizedPrice: Boolean) = apply { | ||
this.isPersonalizedPrice = isPersonalizedPrice | ||
} | ||
|
||
fun oldProductId(oldProductId: String?) = apply { | ||
this.oldProductId = oldProductId | ||
} | ||
|
||
fun googleProrationMode(googleProrationMode: GoogleProrationMode) = apply { | ||
this.googleProrationMode = googleProrationMode | ||
} |
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 looks so nice 😍
fun purchase( | ||
purchaseConfig: Purchase, | ||
listener: NewPurchaseCallback | ||
) { |
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 going to be so much nicer to maintain 💪
@@ -1372,7 +1407,33 @@ class Purchases internal constructor( | |||
} | |||
} | |||
|
|||
private fun getPurchaseCallback(productId: String): PurchaseCallback? { | |||
internal fun purchaseNonUpgradeWithDeprecatedCallback( |
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 love this name, TBH 🙃
* Will be called after the purchase has completed | ||
* @param storeTransaction StoreTransaction object for the purchased product. | ||
* @param customerInfo Updated [CustomerInfo]. | ||
*/ | ||
fun onCompleted(storeTransaction: StoreTransaction?, customerInfo: CustomerInfo) |
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.
StoreTransaction
is nullable here because this could be deferred or something, right? Do we need to update the docs here explaining that?
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.
yep 💯 definitely should
final PurchaseCallback makePurchaseListener = new PurchaseCallback() { | ||
@Override | ||
public void onCompleted(@NonNull StoreTransaction storeTransaction, @NonNull CustomerInfo customerInfo) { | ||
} | ||
|
||
@Override | ||
public void onError(@NonNull PurchasesError error, boolean userCancelled) { | ||
} | ||
}; |
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.
anyone have thoughts on how to migrate this? can we just change one of the parameters to be nullable and call it out, and keep the same 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.
// TODO how to best migrate old proration mode to new? | ||
// val rcProrationMode = GoogleProrationMode(googleProrationMode) |
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 an interesting one... should we allow them to pass any proration mode into the old purchase functions, as to not push a breaking change?
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.
Ugh, so torn here I changed this comment like 8 times...
I think we need hardcode but:
- Put a comment for this method explaining that and why
- Also included this as a big warning in the migration docs
Codecov Report
@@ Coverage Diff @@
## bc5-support #825 +/- ##
===============================================
- Coverage 80.31% 79.30% -1.02%
===============================================
Files 131 133 +2
Lines 4622 4696 +74
Branches 649 650 +1
===============================================
+ Hits 3712 3724 +12
- Misses 650 714 +64
+ Partials 260 258 -2
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
// TODO how to best migrate old proration mode to new? | ||
// val rcProrationMode = GoogleProrationMode(googleProrationMode) |
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.
Ugh, so torn here I changed this comment like 8 times...
I think we need hardcode but:
- Put a comment for this method explaining that and why
- Also included this as a big warning in the migration docs
// TODO fix proration | ||
.googleProrationMode(GoogleProrationMode.IMMEDIATE_WITHOUT_PRORATION) |
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 does // TODO fix proration
mode mean here? 🤔 Is like... don't hardcode?
And should we add a comment on this one too saying the mode is hardcoded to IMMEDIATE_WITHOUT_PRORATION
?
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.
means we need to determine whether or not we can use the old prorationmodes moving forward 🤷♀️ but i think this is resolved by your other comment...we should jsut fully utilize the old methods
@@ -11,6 +11,7 @@ import com.revenuecat.purchases.models.StoreTransaction | |||
/** | |||
* Interface to be implemented when upgrading or downgrading a purchase. | |||
*/ | |||
// TODO BC5 deprecate and/or rename |
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 isn't used in anything "new", right? So we should be able to just fully deprecate 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.
well technically this is the proper signature for the new callback. so we could either rename this OR create a completely new one.
// TODO rename, update docs | ||
interface NewPurchaseCallback : PurchaseErrorCallback { |
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.
Do we have any ideas on what we'll rename this 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.
no, that's the open question... it's tricky bc i think PurchaseCallback is perfect but i'm not sure if we are "allowed" to change nullability without renaming
* @param storeTransaction StoreTransaction object for the purchased product. Null for deferred purchases. | ||
* @param customerInfo Updated [CustomerInfo]. | ||
*/ | ||
fun onCompleted(storeTransaction: StoreTransaction?, customerInfo: CustomerInfo) |
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.
Could we add another function here called onDeferred
so that onCompleted
doesn't have a possible null StoreTransacton
? Not sure how well that plays with Kotlin lambdas and everything though 🤷♂️
purchaseProduct(activity, storeProduct, purchaseCompletedCallback(onSuccess, onError)) | ||
val purchase = PurchaseParams.Builder(storeProduct, activity).build() | ||
purchaseNonUpgradeWithDeprecatedCallback( | ||
purchase, | ||
purchaseCompletedCallback(onSuccess, onError) |
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 think it might be nice to have all of these deprecated With
functions call the deprecated version so all of the retrofitting is contained in the Purchases.kt
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.
yeah, good call. then we probably need to use the old prorationmodes, 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.
I say LGTM and then do some followup PRs 💪 I love this and want to start using it 😊
merging this to unblock a bunch of other tickets, will do a follow-up PR with other comments after! |
### Motivation [CF-1167](https://revenuecats.atlassian.net/browse/CF-1167) ### Description This task was supposed to rename `UpgradeInfo` to `ProductChangeInfo` but... `UpgradeInfo` is now deprecated as of #825 and replaced with `PurchaseParams` so... This PR simply just removes the last use of `UpgradeInfo` which was in our Purchase Tester 😇 [CF-1167]: https://revenuecats.atlassian.net/browse/CF-1167?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
### Breaking Changes * New `purchase(PurchaseParams.Builder)` method and removed `purchase(SubscriptionOption)` method (#825) (#850) via beylmk (@beylmk) ### New Features * Add/use productPlanIdentifier to EntitlementInfo and CustomerInfo (#849) via Josh Holtz (@joshdholtz) ### Bugfixes * New API for getting products by type, properly filter deprecated functions, reworked logic (#843) via Josh Holtz (@joshdholtz) ### Other Changes * Hide isPersonalizedPrice (#858) via beylmk (@beylmk) * Always use purchasingData property directly on the StoreProduct when purchasing (#847) via Josh Holtz (@joshdholtz) * Remove use of UpgradeInfo from Purchase Tester (#846) via Josh Holtz (@joshdholtz) * Update MagicWeather and Purchase Tester with 6.0.0-alpha.4 APIs (#834) via Josh Holtz (@joshdholtz)
Adding the
isPersonalizedPrice
parameter to the purchase functions acted as a bit of a forcing function for us to refactor all of the purchase methods. See that conversation here.Ultimately, what was decided is that we should use the builder pattern for purchases. Now, rather than having 8+ different purchase functions, we have 1, with a Builder for options like
oldProductId
,prorationMode
, andisPersonalizedPrice
. The builder can be constructed with either aStoreProduct
,Package
, orSubscriptionOption
, to cover all of the purchaseable entities.Making this change means we need to unify
ProductChangeCallback
andPurchaseCallback
, since the same method could be used for a normal purchase or a product change. This means making a single interface that has a nullableStoreTransaction
.This is a WIP, but would like to get eyes on it early in the process, as it touches a lot of code.
Open Questions:
PurchaseCallback
and just make theStoreTransaction
nullable, but I don't know of an easy way to notate that with annotations... I think in the past we've just fully deprecated and added something new, but I don't want to give up the good name. PerhapsPurchaseListener
works, but we already did a big refactor to make listener -> callback.