-
Notifications
You must be signed in to change notification settings - Fork 42
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
Add proration modes to post to backend #977
Conversation
ac1f182
to
149bec8
Compare
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.
Looks good 👍
public/src/main/java/com/revenuecat/purchases/models/StoreTransaction.kt
Outdated
Show resolved
Hide resolved
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.
Looking good! Left a few comments we should follow through though.
.idea/codeStyles/Project.xml
Outdated
@@ -8,8 +8,6 @@ | |||
<package name="kotlinx.android.synthetic" alias="false" withSubpackages="true" /> | |||
</value> | |||
</option> | |||
<option name="NAME_COUNT_TO_USE_STAR_IMPORT" value="2147483647" /> | |||
<option name="NAME_COUNT_TO_USE_STAR_IMPORT_FOR_MEMBERS" value="2147483647" /> |
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.
Hmm don't see you using star imports in this PR. Any reason for this 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.
Nope, that was an accidental add... will remove. Thanks for noticing
GoogleProrationMode.IMMEDIATE_WITH_TIME_PRORATION, | ||
GoogleProrationMode.DEFERRED, | ||
GoogleProrationMode.IMMEDIATE_AND_CHARGE_FULL_PRICE, | ||
GoogleProrationMode.IMMEDIATE_AND_CHARGE_PRORATED_PRICE -> {} |
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.
We need to add these to the java version of these API tests 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.
Done!
class PurchaseContext( | ||
val productType: ProductType, | ||
val presentedOffering: String?, | ||
val subscriptionOptionSelected: String?, |
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 we rename these two to presentedOfferingId
and selectedSubscriptionOptionId
?
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.
Sure
private val productTypes = mutableMapOf<String, ProductType>() | ||
private val presentedOfferingsByProductIdentifier = mutableMapOf<String, String?>() | ||
private val subscriptionOptionSelectedByProductIdentifier = mutableMapOf<String, String?>() | ||
private val purchaseContext = mutableMapOf<String, PurchaseContext>() |
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.
❤️
|
||
fun BillingFlowParams.Builder.setUpgradeInfo(replaceProductInfo: ReplaceProductInfo) { | ||
val subscriptionUpdateParams = BillingFlowParams.SubscriptionUpdateParams.newBuilder().apply { | ||
setOldPurchaseToken(replaceProductInfo.oldPurchase.purchaseToken) | ||
replaceProductInfo.prorationMode?.let { | ||
setReplaceProrationMode(it) | ||
setReplaceProrationMode((it as GoogleProrationMode).playBillingClientMode) |
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 could potentially crash if for some reason we passed a non-GoogleProrationMode here and try to cast it... We could do something like:
(replaceProductInfo.prorationMode?.let {
val googleProrationMode = it as? GoogleProrationMode
if (googleProrationMode == null) {
errorLog("Got non-Google proration mode")
} else {
setReplaceProrationMode(googleProrationMode.playBillingClientMode)
}
}
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's the issue with actually crashing? This is a programming error... and a pretty bad one. This would mean you're using the Google implementation and suddenly passing in Amazon objects, so I'd be fine with it crashing. IMO this would be even better than just logging an error that could go undetected.
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 kind of agree with you that crashes are much more visible and could help detect issues sooner. However, I believe the policy we've been following in the SDKs is that we really don't want to cause crashes in our SDK so we always program defensively and try to fail gracefully. This could be a discussion for the whole team though. CC @aboedo, In case you have any thoughts.
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.
Note that we are crashing devs apps here, and we're not sure in what situation their app might be, or how they would like to handle. That's the main reason we try to avoid crashing whenever possible.
context.productType, | ||
context.presentedOffering, | ||
context.subscriptionOptionSelected, | ||
context.prorationMode, |
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.
Wonder if we could just make this toStoreTransaction
receive a PurchaseContext
object?
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 added an overload with it, but there are a lot of use cases that don't take a PurchaseContext, or mix from different sources, so I'd still leave the other creator.
val subscriptionOptionId: String?, | ||
|
||
/** | ||
* The proration_mode string to be sent to the backend. |
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.
Hmm this seems not that useful for developers? Not sure if they would need to get it from the SDK for something... Maybe something like:
"The prorationMode used to perform the upgrade/downgrade of this purchase. Null if it was not an upgrade/downgrade or if the purchase was restored. This is not available for Amazon purchases."
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.
Makes sense, I wasn't aware whether this is exposed to the app developers. Added the doc and additionally added the fact that it's the name of GoogleProrationMode enum
/** | ||
* The proration_mode string to be sent to the backend. | ||
*/ | ||
val prorationMode: String? |
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.
Considering we are exposing the ProrationMode
and GoogleProrationMode
classes as well, any reason we don't use them here?
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.
Right... initially I didn't because of where I had put the ProrationMode
interface. But I'll just change it.
package com.revenuecat.purchases | ||
|
||
interface ProrationMode { | ||
val name: String |
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.
Since this is part of the public API, it should have docs as well. Both for the interface and the 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.
Also, I know this interface was added to eventually support Amazon if/when they add this functionality. Do we have any news of whether they are planning to add this in the medium term? Just wondering if we should remove the complexity of the new interface/downcasting until we have some clear news of that functionality being supported there... Even if it's not so future-proof
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'd keep it... if it's part of the public/common interface we shouldn't have anything platform specific. And I think for the devs this is write-only, so there shouldn't be a major problem. All casts are done internally in our backend and should be rather safe. (Although I'll add the checks like you suggested).
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 PurchaseParams
interface already uses GoogleProrationMode
directly... But well, I think it's ok
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.
But that's fine, right? It's easier for the user to just specify the Google type, and if Amazon supports it at some point, we'll add a specific method to set the amazon proration type.
Internally we could either keep it the same, or as it is planned here, we just keep the interface that the backend needs - which is just a string for what to send in.
There are only a few places that make the downcast, and those should be fine because you should always get the right object.
Actually in this case we should check that you can't use the googleProrationMode in the builder if the SDK is configured as Amazon.
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, I think this is fine, and definitely makes the API simpler to use. As for adding the check, we could certainly at least log some error if they try to do that... But not needed for this PR I think.
5b23a2b
to
0914dab
Compare
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.
Nothing blocking, looks good!
* so he is charged the difference of $0.50 for his new subscription. | ||
* On May 1st, Samwise is charged $36 for his new subscription tier and another $36 on May 1 of each year following. | ||
*/ | ||
IMMEDIATE_AND_CHARGE_PRORATED_PRICE(BillingFlowParams.ProrationMode.IMMEDIATE_AND_CHARGE_PRORATED_PRICE); |
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.
So I might be wrong but I think we didn't add these before because they weren't fully supported? (I might be wrong) Just want to make sure we want to allow users to use these.
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.
We're working on supporting them in the BE
@@ -24,6 +24,7 @@ private class StoreTransactionAPI { | |||
val su1: String? = storeUserID | |||
val purchaseType: PurchaseType = purchaseType | |||
val subscriptionOptionId = subscriptionOptionId | |||
val prorationMode = prorationMode |
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.
Oh I just noticed we are not specifying the type here, so this wouldn't actually catch issues with the type. This and the above should have the type, like this:
val prorationMode: ProrationMode? = prorationMode
package com.revenuecat.purchases | ||
|
||
interface ProrationMode { | ||
val name: String |
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 PurchaseParams
interface already uses GoogleProrationMode
directly... But well, I think it's ok
enum class GoogleProrationMode(@BillingFlowParams.ProrationMode val playBillingClientMode: Int) { | ||
enum class GoogleProrationMode( | ||
@BillingFlowParams.ProrationMode val playBillingClientMode: Int | ||
) : com.revenuecat.purchases.ProrationMode { |
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.
We could import this proration mode with an alias like
import com.revenuecat.purchases.ProrationMode as RCProrationMode
NABD though.
val subscriptionOptionId: String?, | ||
|
||
/** | ||
* The name of the prorationMode used to perform the upgrade/downgrade of this purchase. |
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.
Now it's not the name, so this should be The prorationMode used to...
Oh looks like tests failed now. |
…onMode in StoreTransactionFactory#createStoreTransaction - regression from RevenueCat#977
…onMode in StoreTransactionFactory#createStoreTransaction - regression from RevenueCat#977
**This is an automatic release.** ### New Features * CAT-859 Expose whether or not a SubscriptionOption is Prepaid in the SDK (#1005) via Deema AlShamaa (@dalshamaa) ### Bugfixes * [CF-1324] Fix personalizedPrice defaulting to false (#952) via beylmk (@beylmk) ### Performance Improvements * Store and return ETag last refresh time header (#978) via Toni Rico (@tonidero) ### Dependency Updates * Bump fastlane-plugin-revenuecat_internal from `3b03efa` to `fe45299` (#991) via dependabot[bot] (@dependabot[bot]) * Bump danger from 9.2.0 to 9.3.0 (#981) via dependabot[bot] (@dependabot[bot]) * Bump fastlane-plugin-revenuecat_internal from `8482a43` to `3b03efa` (#974) via dependabot[bot] (@dependabot[bot]) * Bump fastlane from 2.212.1 to 2.212.2 (#973) via dependabot[bot] (@dependabot[bot]) * Bump fastlane-plugin-revenuecat_internal from `9255366` to `8482a43` (#961) via dependabot[bot] (@dependabot[bot]) ### Other Changes * Add proration modes to post to backend (#977) via swehner (@swehner) * Added ENTITLEMENTS_COMPUTED_ON_DEVICE (#939) via Cesar de la Vega (@vegaro) * Fix flaky test in OfflineCustomerInfoCalculatorTest (#997) via Cesar de la Vega (@vegaro) * Fix `OfflineCustomerInfoCalculatorTest` `Unresolved reference: ProducType` (#995) via Cesar de la Vega (@vegaro) * Add support for product_plan_identifier for offline customer info (#959) via Cesar de la Vega (@vegaro) * Add non-subscriptions support to offline customer info (#958) via Cesar de la Vega (@vegaro) * Query only active purchases when generating offline entitlements customer info (#1003) via Toni Rico (@tonidero) * Fix `PurchasesIntegrationTest` building issue (#996 into main) (#998) via Cesar de la Vega (@vegaro) * Fail offline entitlements computation if product entitlement mapping not available (#999) via Toni Rico (@tonidero) * Fix build_magic_weather lane (#993) via Cesar de la Vega (@vegaro) * Add backend integration tests and test product entitlement mapping endpoint (#988) via Toni Rico (@tonidero) * Fix purchases integration tests (#980) via Toni Rico (@tonidero) * Disable offline entitlements if active inapp purchases exist (#983) via Toni Rico (@tonidero) * Clear cached customer info upon entering offline entitlements mode (#989) via Toni Rico (@tonidero) * Update product entitlement mapping request to new format (#976) via Toni Rico (@tonidero) * Support enabling/disabling offline entitlements (#964) via Toni Rico (@tonidero) * Add back integration tests automation (#972) via Toni Rico (@tonidero) * Upgrade to AGP 8.0 (#975) via Toni Rico (@tonidero) * Extract post receipt logic to PostReceiptHelper (#967) via Toni Rico (@tonidero) * Add isServerDown to error callback for postReceipt and getCustomerInfo requests (#963) via Toni Rico (@tonidero) * Add back integration test flavors (#962) via Toni Rico (@tonidero) * Fix storing test results (#966) via Cesar de la Vega (@vegaro) * Extract detekt job from test job (#965) via Cesar de la Vega (@vegaro) [CF-1324]: https://revenuecats.atlassian.net/browse/CF-1324?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ --------- Co-authored-by: revenuecat-ops <ops@revenuecat.com> Co-authored-by: Toni Rico <antonio.rico.diez@revenuecat.com>
Add missing proration modes and forward them to the Backend when posting receipts
Motivation
For supporting proration modes we need to send them to the Backend. Added extra parameter in POST receipt.
Added all proration modes in GoogleProrationModes
Description