Amazon Offering Parsing by beylmk · Pull Request #824 · RevenueCat/purchases-android

beylmk · 2023-02-27T22:24:18Z

Amazon only has a single productId (the termSku) returned in offerings, while google has both the subId and the basePlanID. So now the way we parse offerings must split between the stores.

This PR introduces an OfferingParser and OfferingParserFactory, instantiating the correct parser from the factory upon initialization. I'd love to get some feedback on this approach before writing tests, as I think those will be a lot of work...

My thoughts on other potential implementations and why i chose this one:

simply pass the store as a parameter into the OfferingFactories methods, and if/else split how we do parsing on that

this is the easiest, but it breaks our general "don't have store-specific stuff in purchases" rule. see this commit as an example

make OfferingFactory an abstract class and instantiate a store-specific implementation upon purchases initialization, somewhat like BillingFactory works. see this commit for example (it's not complete/perfect, i just committed for easy reviewing of the overall architecture idea)

this feels a bit like over-architecting to me... but i could see more complex offering parsing in the future -- for instance, if we implement sorted SubscriptionOptions. This is the route I took in this PR.

pull the offering parsing into Billing implementation. There is really only one line of code that differs, so we could only pull line out into a function in the Billing classes for now (and keep the rest in the base billing class).

I went this route here, but it doesn't feel totally right since offering parsing doesn't communicate with the billing libraries and isn't really a "billing" thing.

…ng for amazon vs play

# Conflicts: # common/src/test/java/com/revenuecat/purchases/common/OfferingsTest.kt # purchases/src/main/kotlin/com/revenuecat/purchases/Purchases.kt

swehner

Looks good - I like the approach. Small comment re: the interface

swehner · 2023-02-28T10:13:40Z

+            "offering",
+            Store.PLAY_STORE


All these changes are completely unrelated, right?

oh yeah, the test changes are left from a different approach. was waiting to update them to this one until i get sign off

swehner · 2023-02-28T10:18:29Z

+        val productIdentifier = packageJson.getString("platform_product_identifier")
+
+        val planIdentifier = packageJson.optString("platform_product_plan_identifier").takeIf { it.isNotEmpty() }
+        val product = findMatchingProduct(productsById, productIdentifier, planIdentifier)


I think this interface is a bit strange - one difference between Amazon and Google is that Amazon just needs platform_product_identifier and that Google also needs the platform_product_plan_identifier. I would move this difference into the findMatchingProduct method.
Otherwise for any new piece of information we'll have to change the API again.

Maybe we can do findMatchingProduct(productsById, packageJson)?

fair point... i was just trying to reuse as much code as possible but it's less future-proof.

Yeah, since we are trying to avoid having store-specific logic here, that makes sense.

# Conflicts: # common/src/main/java/com/revenuecat/purchases/common/OfferingFactories.kt # feature/amazon/src/main/java/com/revenuecat/purchases/amazon/storeProductConversions.kt

tonidero

Sorry for the delay! The approach makes sense to me 👍

tonidero · 2023-03-08T15:03:33Z

+
+abstract class OfferingParser {
+
+    abstract fun findMatchingProduct(


This could be protected I think?

tonidero · 2023-03-08T15:05:08Z

+    }
+}
+
+fun String.toPackageType(): PackageType =


And this could be private I think?

tonidero · 2023-03-08T15:08:04Z

+        val productIdentifier = packageJson.getString("platform_product_identifier")
+
+        val planIdentifier = packageJson.optString("platform_product_plan_identifier").takeIf { it.isNotEmpty() }
+        val product = findMatchingProduct(productsById, productIdentifier, planIdentifier)


Yeah, since we are trying to avoid having store-specific logic here, that makes sense.

beylmk · 2023-03-08T15:12:17Z

@tonidero thanks for the review :)

# Conflicts: # feature/amazon/src/main/java/com/revenuecat/purchases/amazon/storeProductConversions.kt

…c values

vegaro

Approach also makes sense to me. Sorry for the delay

joshdholtz · 2023-03-07T17:03:49Z

+                ?.firstOrNull()
+        }
+        return storeProducts?.firstOrNull { storeProduct ->
+            storeProduct.subscriptionOptions?.firstOrNull { it.isBasePlan }?.id == planIdentifier


The SubscriptionOptions has a basePlan property now you can now use 👇

Suggested change

storeProduct.subscriptionOptions?.firstOrNull { it.isBasePlan }?.id == planIdentifier

storeProduct.subscriptionOptions?.basePlan?.id == planIdentifier

ah yes, i hope i didn't miss anything else when merging... it seems like that was the only change to OfferingFactories since this PR...

Yup yup, I think this is the only part of the improvements that can be used here 😇 Pretty small improvement but 🤷‍♂️

beylmk · 2023-03-15T19:45:43Z

+import org.junit.runner.RunWith
+import org.robolectric.annotation.Config
+
+//@RunWith(AndroidJUnit4::class)


i started writing these but felt like it would kinda be wasted time without AmazonStoreProduct work done..

joshdholtz

This looks good to me! Just one question about some commented out tests and I think you have a commented about writing more tests but... LGTM 💪

joshdholtz · 2023-03-16T18:02:42Z

+//    private fun mockOfferingsParser() {
+//        with(mockOfferingParser) {
+//            every {
+//                retrieveCustomerInfo(any(), any(), false, any())
+//            } answers {
+//                val callback = arg<ReceiveCustomerInfoCallback?>(3)
+//                if (errorGettingCustomerInfo == null) {
+//                    callback?.onReceived(mockInfo)
+//                } else {
+//                    callback?.onError(errorGettingCustomerInfo)
+//                }
+//            }
+//            every {
+//                cacheCustomerInfo(any())
+//            } just runs
+//            every {
+//                sendUpdatedCustomerInfoToDelegateIfChanged(any())
+//            } just runs
+//            every {
+//                updatedCustomerInfoListener = any()
+//            } just runs
+//            every {
+//                updatedCustomerInfoListener
+//            } returns null
+//        }
+//    }
+


Do these need to be uncommented or rewritten yet?

was gonna wait until amazonstoreproduct is done!

Maddie Beyl added 10 commits February 17, 2023 10:11

Update OfferingFactories to take store parameter, use different parsi…

00e2260

…ng for amazon vs play

start adding back ProductConverterTests

428fa30

Add back most AmazonBillingTests

fe1a669

Fix tests failing

c8a06e4

POC of offeringfactory on init

ce110fd

Merge branch 'bc5-support' into offering-factory

9849768

# Conflicts: # common/src/test/java/com/revenuecat/purchases/common/OfferingsTest.kt # purchases/src/main/kotlin/com/revenuecat/purchases/Purchases.kt

Make createOfferings a method on OfferingFactory, call from Purchases

662ce0e

Make other functions non-extension functions for consistency

c9c4d0c

use firstOrNull instead of get(0)

7633ac6

Use actual factory pattern

9768a1a

beylmk mentioned this pull request Feb 27, 2023

[WIP] CF-1196 Fix offerings parsing for Amazon #802

Closed

beylmk changed the title ~~Amazon Offering Parsing~~ [WIP] Amazon Offering Parsing Feb 27, 2023

Maddie Beyl added 2 commits February 27, 2023 17:32

Pull parsers out of storeProductConversions and into their own classes

322235d

Make findMatchingProduct a non-extension function

7f882f6

swehner reviewed Feb 28, 2023

View reviewed changes

beylmk requested review from a team and tonidero and removed request for tonidero February 28, 2023 16:47

Merge branch 'bc5-support' into offering-factory

457fe99

# Conflicts: # common/src/main/java/com/revenuecat/purchases/common/OfferingFactories.kt # feature/amazon/src/main/java/com/revenuecat/purchases/amazon/storeProductConversions.kt

beylmk requested review from tonidero and vegaro March 2, 2023 21:11

tonidero reviewed Mar 8, 2023

View reviewed changes

Maddie Beyl added 7 commits March 13, 2023 18:11

Merge branch 'bc5-support' into offering-factory

21b8f6f

Merge branch 'bc5-support' into offering-factory

84c791e

# Conflicts: # feature/amazon/src/main/java/com/revenuecat/purchases/amazon/storeProductConversions.kt

Update offeringParser interface to pass jsonObject instead of specifi…

0e328f4

…c values

Merge branch 'bc5-support' into offering-factory

a0db7d7

fix visibility of OfferingParser functions

3036e34

pass offeringparser to purchases() in unit tests

f6bebb2

Update offeringstest with offeringparser methods

d6fb996

vegaro reviewed Mar 15, 2023

View reviewed changes

joshdholtz reviewed Mar 15, 2023

View reviewed changes

Maddie Beyl added 5 commits March 15, 2023 11:20

update google offeringparser to use new subscriptionOptions.baseplan

94be821

Clean up OfferingsTest for readability

e8ca049

Clean up more offering tests -- clearer names, a few more asserts

ef62c0f

fix multiple packages test, make getOfferingsJson more flexible

396e815

create test file for amazon offerings with todo

3546352

beylmk added the refactor label Mar 15, 2023

Fix product converter tests and deprecation warning

9acd764

beylmk force-pushed the offering-factory branch from ee3c8d1 to 9acd764 Compare March 15, 2023 19:43

beylmk commented Mar 15, 2023

View reviewed changes

beylmk marked this pull request as ready for review March 15, 2023 19:45

beylmk requested a review from a team March 15, 2023 19:47

beylmk changed the title ~~[WIP] Amazon Offering Parsing~~ Amazon Offering Parsing Mar 16, 2023

Merge branch 'bc5-support' into offering-factory

cbc11a0

beylmk force-pushed the offering-factory branch from b49189d to cbc11a0 Compare March 16, 2023 16:20

joshdholtz approved these changes Mar 16, 2023

View reviewed changes

fix bad merge missing commas

fe5f4a7

beylmk force-pushed the offering-factory branch from b9450a1 to fe5f4a7 Compare March 16, 2023 20:22

beylmk merged commit 16a6e1c into bc5-support Mar 16, 2023

beylmk deleted the offering-factory branch March 16, 2023 20:40

joshdholtz mentioned this pull request Mar 23, 2023

Release/6.0.0-beta.3 #895

Merged


		abstract class OfferingParser {

		abstract fun findMatchingProduct(

	storeProduct.subscriptionOptions?.firstOrNull { it.isBasePlan }?.id == planIdentifier
	storeProduct.subscriptionOptions?.basePlan?.id == planIdentifier

Uh oh!

Conversation

beylmk commented Feb 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

swehner 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!

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!

beylmk commented Mar 8, 2023

Uh oh!

vegaro 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!

joshdholtz 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

beylmk commented Feb 27, 2023 •

edited

Loading