Refactor `presentedOfferingIdentifier` into `presentedOfferingContext` object by tonidero · Pull Request #1612 · RevenueCat/purchases-android

tonidero · 2024-02-14T11:32:44Z

Description

With some new incoming features, we will need to pass more context information about where the package/product was obtained from. This PR prepares for that by adding a new class PresentedOfferingContext which right now only holds the presentedOfferingIdentifier but that we can add more info later on that allows to pass more information to our servers.

tonidero · 2024-02-14T11:38:18Z

+        ReplaceWith("presentedOfferingContext.offeringIdentifier"),
+    )
+    val offering: String
+        get() = presentedOfferingContext.offeringIdentifier ?: ""


I made the presentedOfferingContext have a nullable offeringIdentifier instead of making the presentedOfferingContext nullable in StoreProduct/SubscriptionOption in case we could have non-offering related context, which is probably not the case right now... But this forces us to default to something here as a fallback. It should never be null right now though in the Package.

I like this! I think this makes sense 👍

hmm I think it makes sense for StoreProduct/SubscriptionOption, but with this there's no way to indicate that a package will always have an offering and a package is always going to have an offering. Do we need to replace offering in Package with presentedOfferingContext? I think we can leave this as is offering: String and only replace presentedOfferingIdentifier in StoreProduct and SubscriptionOption? We access the presentedOfferingContext through the products, and not the packages.

Hmm it's a good point... the only difference is going to be that the offering is not nullable in the Package... However, It feels nicer to me if we have the same format in all 3 places... If we don't have this data at the Package level we will need to access it through the StoreProduct when purchasing, which seems slightly less clean...

What do you think about leaving the offering property undeprecated here in the Package? It would be duplicated with the one in presentedOfferingContext but that way we can keep the non-nullability here

I guess my question is if we need a presentedOfferingContext at all in Package. Is it ever going to be different than the offering value? I guess we can add a different way of getting Packages in the future, which would indeed make offering not nullable and create the need of a presentedOfferingContext, but maybe we can add that property whenever we need. I just think adding it right now makes the API confusing and doesn't add value. Maybe I am missing context

Actually... What if we make it a sealed class that only has an implementation right now? That way we can keep the original nullabilities. Can you remind me what other type of data we are adding to the PresentedOfferingContext?

@joshdholtz I was chatting live with @vegaro about this one... In the end, we believe it would be better to make the offeringIdentifier not nullable inside the PresentedOfferingContext so we can make the PresentedOfferingContext not nullable in Package but nullable in StoreProduct/SubscriptionOption. Do you think we will (now or in the future) have data like this without having an offering id?

My original reason to make it nullable was to provide more flexibility in the future in case we have some of this data without an offering id, but I don't think that would happen for now at least, so it makes sense to better reflect the current model.

Let us know what you think!

Addressed in 9eaf8a4

tonidero · 2024-02-14T11:39:14Z

 ) : PurchaseResponseListener {

    private val productTypes = mutableMapOf<String, ProductType>()
-    private val presentedOfferingsByProductIdentifier = mutableMapOf<String, String?>()


I removed this because it seems it wasn't used. Seems we were passing the offering id in the AmazonBilling class instead.

tonidero · 2024-02-14T11:41:16Z

+     * Context of the offering that was presented when making the purchase.
     */
-    val presentedOfferingIdentifier: String?,
+    val presentedOfferingContext: PresentedOfferingContext?,


I had doubts on this... Note that it's nullable here but not nullable in the Package/StoreProduct/SubscriptionOption. It will be null here when it can't find any information about the context (usually restores and such) vs having a context with empty data when the origin of the purchase is through a product/we don't have offering data.

Lmk if you think we should standardize this somehow and we can chat, but I thought it was ok like this for now.

I think this is fine to be nullable here! Forcing an object before the transaction makes it a cleaner (especially when copying the object down into subscription options. I probably would have done the same here in making it nullable in the transaction 👍

joshdholtz

This is looking good! Thank you so much for jumping on this so quickly 👍

joshdholtz · 2024-02-14T13:17:23Z

+        ReplaceWith("presentedOfferingContext.offeringIdentifier"),
+    )
+    val offering: String
+        get() = presentedOfferingContext.offeringIdentifier ?: ""


I like this! I think this makes sense 👍

joshdholtz · 2024-02-14T13:21:17Z

+     * Context of the offering that was presented when making the purchase.
     */
-    val presentedOfferingIdentifier: String?,
+    val presentedOfferingContext: PresentedOfferingContext?,


I think this is fine to be nullable here! Forcing an object before the transaction makes it a cleaner (especially when copying the object down into subscription options. I probably would have done the same here in making it nullable in the transaction 👍

codecov · 2024-02-14T16:46:47Z

Codecov Report

Attention: Patch coverage is 76.87075% with 34 lines in your changes missing coverage. Please review.

Project coverage is 83.67%. Comparing base (663112b) to head (9b6e897).
Report is 167 commits behind head on main.

Files with missing lines	Patch %	Lines
...uecat/purchases/models/GoogleSubscriptionOption.kt	23.07%	9 Missing and 1 partial ⚠️
...om/revenuecat/purchases/models/TestStoreProduct.kt	0.00%	5 Missing ⚠️
.../com/revenuecat/purchases/PurchasesOrchestrator.kt	66.66%	1 Missing and 3 partials ⚠️
.../revenuecat/purchases/amazon/AmazonStoreProduct.kt	83.33%	2 Missing and 2 partials ⚠️
.../revenuecat/purchases/models/GoogleStoreProduct.kt	85.71%	1 Missing and 3 partials ⚠️
...n/com/revenuecat/purchases/amazon/AmazonBilling.kt	25.00%	3 Missing ⚠️
...om/revenuecat/purchases/models/StoreTransaction.kt	90.47%	0 Missing and 2 partials ⚠️
...rc/main/kotlin/com/revenuecat/purchases/Package.kt	85.71%	0 Missing and 1 partial ⚠️
...lin/com/revenuecat/purchases/common/ReceiptInfo.kt	75.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1612      +/-   ##
==========================================
- Coverage   83.78%   83.67%   -0.12%     
==========================================
  Files         218      219       +1     
  Lines        7266     7343      +77     
  Branches     1011     1023      +12     
==========================================
+ Hits         6088     6144      +56     
- Misses        786      799      +13     
- Partials      392      400       +8

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…lable in storeproduct and subscriptionOption

tonidero · 2024-02-15T13:08:54Z

-data class GoogleStoreProduct(
+data class GoogleStoreProduct
+@JvmOverloads
+@Deprecated(


Quick note about this deprecation. I was running into a lot of issues trying to add the new parameter while trying to keep retrocompatibility... In the end, I'm deprecating the main constructor and adding a new one which is internal only.

Reason was that there were a lot of overload conflicts, specially in Java, where String can be null and it was making the API tests fail.

IMO, we should make all these constructors internal in the future. If we want to allow devs to create them, then I would suggest using the Builder pattern instead... Much easier to add/deprecate things and allows us to keep flexibility for the constructor.

tonidero · 2024-02-15T13:22:04Z

Ok this is ready for another review @joshdholtz @vegaro

joshdholtz

This looks good to me! Two minor questions but otherwise LGTM 💪 Thanks so much for doing this!

@vegaro

**This is an automatic release.** ### New Features * Add setDisplayDismissButton to PaywallView (#1608) via Cesar de la Vega (@vegaro) ### Bugfixes * Fix runtime crash when using amazon and targetting android 14 (#1622) via Toni Rico (@tonidero) * Paywalls: No-op on all view model methods in the Loading paywall screen (#1617) via Toni Rico (@tonidero) * Fix safe insets on full screen (#1613) via Cesar de la Vega (@vegaro) ### Other Changes * Change default paywall background with a lower quality image (#1623) via Toni Rico (@tonidero) * Fix load shedder integration tests (#1618) via Cesar de la Vega (@vegaro) * Refactor `presentedOfferingIdentifier` into `presentedOfferingContext` object (#1612) via Toni Rico (@tonidero) * [External] Update russian and kazakh translations (#1577 by @janbolat) (#1616) via Toni Rico (@tonidero) Co-authored-by: revenuecat-ops <ops@revenuecat.com>

Refacor presentedOfferingIdentifier into presentedOfferingContext object

953a39d

tonidero added the refactor label Feb 14, 2024

tonidero commented Feb 14, 2024

View reviewed changes

joshdholtz reviewed Feb 14, 2024

View reviewed changes

tonidero added 2 commits February 14, 2024 16:18

Add API tests

4f00efe

Fix tests

b5a2653

tonidero marked this pull request as ready for review February 14, 2024 16:30

tonidero requested review from a team, joshdholtz and vegaro February 14, 2024 16:30

Make PresentedOfferingContext offeringId not nullable and make it nul…

9eaf8a4

…lable in storeproduct and subscriptionOption

tonidero commented Feb 15, 2024

View reviewed changes

joshdholtz approved these changes Feb 15, 2024

View reviewed changes

Comment thread purchases/src/main/kotlin/com/revenuecat/purchases/amazon/handler/PurchaseHandler.kt Outdated

Comment thread purchases/src/main/kotlin/com/revenuecat/purchases/google/BillingWrapper.kt

vegaro reviewed Feb 15, 2024

View reviewed changes

tonidero added 3 commits February 15, 2024 16:11

Remove unused parameter

ed49688

Fix replace with

a38dff4

Comments from review

6855a71

tonidero requested a review from vegaro February 15, 2024 15:33

Fix tests

9b6e897

vegaro approved these changes Feb 15, 2024

View reviewed changes

tonidero enabled auto-merge (squash) February 15, 2024 15:55

tonidero merged commit 8784d7f into main Feb 15, 2024

tonidero deleted the refactor-presented-offering-id-into-presented-offering-context branch February 15, 2024 16:10

RCGitBot mentioned this pull request Feb 21, 2024

[AUTOMATIC] Release/7.6.0 #1624

Merged

vegaro added pr:other and removed pr:refactor labels Sep 17, 2024

Uh oh!

Conversation

tonidero commented Feb 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

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!

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!

codecov Bot commented Feb 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tonidero commented Feb 15, 2024

Uh oh!

joshdholtz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tonidero commented Feb 14, 2024 •

edited

Loading

codecov Bot commented Feb 14, 2024 •

edited

Loading