Add proration modes to post to backend by swehner · Pull Request #977 · RevenueCat/purchases-android

swehner · 2023-04-19T22:33:18Z

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

Use GoogleProrationMode internally instead of an enum
Add an interface that Amazon could potentially implement in the future

MarkVillacampa

Looks good 👍

greenietea · 2023-04-20T17:39:06Z

I expected to see the proration_mode param to be sent to the backend but it's not there?

swehner · 2023-04-20T20:28:02Z

I expected to see the proration_mode param to be sent to the backend but it's not there?

You're totally right - thanks for double checking it. I must have missed a line in the latest refactor and screwed it up.
Added it back and added a test.

tonidero

Looking good! Left a few comments we should follow through though.

tonidero · 2023-04-21T07:59:45Z

        </value>
      </option>
-      <option name="NAME_COUNT_TO_USE_STAR_IMPORT" value="2147483647" />
-      <option name="NAME_COUNT_TO_USE_STAR_IMPORT_FOR_MEMBERS" value="2147483647" />


Hmm don't see you using star imports in this PR. Any reason for this change?

Nope, that was an accidental add... will remove. Thanks for noticing

tonidero · 2023-04-21T08:00:46Z

+            GoogleProrationMode.IMMEDIATE_WITH_TIME_PRORATION,
+            GoogleProrationMode.DEFERRED,
+            GoogleProrationMode.IMMEDIATE_AND_CHARGE_FULL_PRICE,
+            GoogleProrationMode.IMMEDIATE_AND_CHARGE_PRORATED_PRICE -> {}


We need to add these to the java version of these API tests as well

tonidero · 2023-04-21T08:03:56Z

+class PurchaseContext(
+    val productType: ProductType,
+    val presentedOffering: String?,
+    val subscriptionOptionSelected: String?,


Should we rename these two to presentedOfferingId and selectedSubscriptionOptionId?

tonidero · 2023-04-21T08:04:47Z

-    private val productTypes = mutableMapOf<String, ProductType>()
-    private val presentedOfferingsByProductIdentifier = mutableMapOf<String, String?>()
-    private val subscriptionOptionSelectedByProductIdentifier = mutableMapOf<String, String?>()
+    private val purchaseContext = mutableMapOf<String, PurchaseContext>()


tonidero · 2023-04-21T08:15:00Z

        setOldPurchaseToken(replaceProductInfo.oldPurchase.purchaseToken)
        replaceProductInfo.prorationMode?.let {
-            setReplaceProrationMode(it)
+            setReplaceProrationMode((it as GoogleProrationMode).playBillingClientMode)


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

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.

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.

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.

tonidero · 2023-04-21T08:20:26Z

+                        context.productType,
+                        context.presentedOffering,
+                        context.subscriptionOptionSelected,
+                        context.prorationMode,


Wonder if we could just make this toStoreTransaction receive a PurchaseContext object?

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.

tonidero · 2023-04-21T08:26:57Z

+    val subscriptionOptionId: String?,
+
+    /**
+     * The proration_mode string to be sent to the backend.


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."

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~~

tonidero · 2023-04-21T08:27:17Z

+    /**
+     * The proration_mode string to be sent to the backend.
+     */
+    val prorationMode: String?


Considering we are exposing the ProrationMode and GoogleProrationMode classes as well, any reason we don't use them here?

Right... initially I didn't because of where I had put the ProrationMode interface. But I'll just change it.

tonidero · 2023-04-21T08:34:16Z

+package com.revenuecat.purchases
+
+interface ProrationMode {
+    val name: String


Since this is part of the public API, it should have docs as well. Both for the interface and the name.

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

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

The PurchaseParams interface already uses GoogleProrationMode directly... But well, I think it's ok

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.

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.

tonidero

Nothing blocking, looks good!

tonidero · 2023-04-21T15:14:21Z

+     * 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);


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.

We're working on supporting them in the BE

tonidero · 2023-04-21T15:15:48Z

            val su1: String? = storeUserID
            val purchaseType: PurchaseType = purchaseType
            val subscriptionOptionId = subscriptionOptionId
+            val prorationMode = prorationMode


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

tonidero · 2023-04-21T15:19:12Z

+package com.revenuecat.purchases
+
+interface ProrationMode {
+    val name: String


The PurchaseParams interface already uses GoogleProrationMode directly... But well, I think it's ok

tonidero · 2023-04-21T15:20:57Z

-enum class GoogleProrationMode(@BillingFlowParams.ProrationMode val playBillingClientMode: Int) {
+enum class GoogleProrationMode(
+    @BillingFlowParams.ProrationMode val playBillingClientMode: Int
+) : com.revenuecat.purchases.ProrationMode {


We could import this proration mode with an alias like

import com.revenuecat.purchases.ProrationMode as RCProrationMode

NABD though.

tonidero · 2023-04-21T15:21:53Z

+    val subscriptionOptionId: String?,
+
+    /**
+     * The name of the prorationMode used to perform the upgrade/downgrade of this purchase.


Now it's not the name, so this should be The prorationMode used to...

tonidero · 2023-04-21T15:25:37Z

Oh looks like tests failed now.

…onMode in StoreTransactionFactory#createStoreTransaction - regression from RevenueCat#977

@dalshamaa

**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>

swehner added the pr:feat A new feature label Apr 19, 2023

swehner changed the base branch from main to v6 April 20, 2023 12:37

First version

149bec8

swehner force-pushed the add_proration_modes branch from ac1f182 to 149bec8 Compare April 20, 2023 13:46

swehner added 2 commits April 20, 2023 15:48

add pricing phases back

6077266

fix tests

af48fb7

swehner changed the base branch from v6 to main April 20, 2023 14:28

swehner added 3 commits April 20, 2023 16:31

Fix detekt

d6b26ce

Create prorationmode superclass

f64633a

Fix casts

60b2b43

swehner changed the title ~~[DRAFT] Add proration modes~~ Add proration modes to post to backend Apr 20, 2023

swehner requested review from a team, greenietea and tonidero April 20, 2023 16:32

Add documentation

6a1c985

MarkVillacampa approved these changes Apr 20, 2023

View reviewed changes

Comment thread public/src/main/java/com/revenuecat/purchases/models/StoreTransaction.kt Outdated

Add doc and fix detekt

8003d10

Add missing parameter to ReceiptInfo

bc03779

greenietea approved these changes Apr 20, 2023

View reviewed changes

tonidero reviewed Apr 21, 2023

View reviewed changes

swehner added 2 commits April 21, 2023 16:30

Comments

5bdacff

Fix test

0914dab

swehner force-pushed the add_proration_modes branch from 5b23a2b to 0914dab Compare April 21, 2023 15:01

remove accidental changes

92ff1d3

swehner requested a review from tonidero April 21, 2023 15:03

tonidero approved these changes Apr 21, 2023

View reviewed changes

swehner added 3 commits April 21, 2023 18:19

Fix test

a0dae41

Fix comments

6b491da

remove deferred

88cf88c

swehner merged commit acf47ef into main Apr 26, 2023

swehner deleted the add_proration_modes branch April 26, 2023 16:59

RCGitBot mentioned this pull request May 3, 2023

[AUTOMATIC] Release/6.2.0 #992

Closed

pablo-guardiola mentioned this pull request May 4, 2023

[External] Fix PurchasesIntegrationTest building issue #996

Merged

2 tasks

pablo-guardiola added a commit to pablo-guardiola/purchases-android that referenced this pull request May 4, 2023

fix PurchasesIntegrationTest building issue caused by missing prorati…

0f271b1

…onMode in StoreTransactionFactory#createStoreTransaction - regression from RevenueCat#977

RCGitBot mentioned this pull request May 17, 2023

[AUTOMATIC] Release/6.2.0 #1007

Merged

Uh oh!

Conversation

swehner commented Apr 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Description

Uh oh!

MarkVillacampa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

greenietea commented Apr 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

swehner commented Apr 20, 2023

Uh oh!

tonidero left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

swehner Apr 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tonidero Apr 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tonidero left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

swehner commented Apr 19, 2023 •

edited

Loading

greenietea commented Apr 20, 2023 •

edited

Loading

swehner Apr 21, 2023 •

edited

Loading

tonidero Apr 21, 2023 •

edited

Loading