-
Notifications
You must be signed in to change notification settings - Fork 82
Add APIs for hybrid SDKs to set presentedOfferingContext #2610
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 APIs for hybrid SDKs to set presentedOfferingContext #2610
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2610 +/- ##
=======================================
Coverage 78.38% 78.38%
=======================================
Files 301 301
Lines 11229 11229
Branches 1561 1561
=======================================
Hits 8802 8802
Misses 1741 1741
Partials 686 686 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Thanks for taking care of this! Just a question on whether we need to introduce the new "OfferingInfo" concept for this.
data class OfferingId(val offeringId: String) : OfferingSelection() | ||
data class OfferingInfo(val offeringInfo: OfferingPresentationInfo) : OfferingSelection() |
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 need a new class (OfferingPresentationInfo
) here? Can we not add the PresentedOfferingContext
parameter to OfferingId
? Like so:
data class OfferingId(
val offeringId: String,
val presentedOfferingContext: PresentedOfferingContext?,
) : OfferingSelection()
Reasoning also being that it's not super clear (at first glance) how setOfferingInfo()
is different from setOffering()
. That could also remain setOfferingId()
in this case:
internal fun setOfferingId(
offeringId: String?,
presentedOfferingContext: PresentedOfferingContext?,
)
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... we could add it as a new parameter, the main reason I joined them into a new class was because both types of data should move together through all the plumbing, and creating a new entity ensured that better than separating into different parameters and moving both around. That could lead to easily forgetting passing/setting one of them IMO. It's true it's a bit confusing with the other setOffering
APIs though... not sure if we could come up with a better naming...
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 about IdAndPresentedOfferingContext
? 😅 (It's already namespaced to OfferingSelection
, so maybe we can get away with omitting "Offering" in 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.
Hmm it's a bit of a longer name... certainly more explicit, but not sure if we win much that much clarity TBH... But I guess it could improve on the readability for the setOfferingId method... So will do the rename.
@@ -86,6 +94,15 @@ class PaywallActivityLauncher(resultCaller: ActivityResultCaller, resultHandler: | |||
* @param edgeToEdge Whether to display the paywall in edge-to-edge mode. | |||
* Default is true for Android 15+, false otherwise. | |||
*/ | |||
@Deprecated( | |||
message = "Use launch with presented offering context instead", |
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.
Is the PresentedOfferingContext
always available? Should we default to null
?
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.
For an offering, it should always be available yes. I had to make it nullable for backwards compatibility reasons, but otherwise, it would be good to assume that it should always be there.
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, but what is the purpose of the "offering ID" API, if devs will always need to have a full Offering
(because they now need the PresentedOfferingContext
)? That makes the "offering ID" API obsolete no, or am I misunderstanding something?
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 main reason is that it's tricky to create a full Offering
native object from the hybrids, since it has a lot of data and even some non-parcelable objects like the Store's product entities. So we can't easily recreate the offerings from the data in the hybrids, that's the reason we were only passing the offering id.
This PR adds the data we were missing for proper traceability purposes. Otherwise, any placement/targeting data would be lost.
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 got it, but this is a public API too. Before this PR, it had its utility. You could launch a paywall with just an Offering ID, without having to get Offerings first. But now the utility of this public API is gone, as devs will need to get Offerings to be able to pass a PresentedOfferingContext
.
I was gonna suggest:
- we deprecate
launch(offeringIdentifier: String)
in favor oflaunch(offering: Offering)
instead, and - we make
launch(offeringIdentifier: String, presentedOfferingContext: PresentedOfferingContext)
an@InternalRevenueCatAPI
.
But I see you already did 2 😄 So in that case I think we should still do 1, as the current deprecation message encourages the use of an internal API.
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... our docs did say not to use this method, but yeah, we probably should have made it internal from the beginning... In any case, I do agre that the ReplaceWith should point to the method taking an offering instead of the internal API. Good catch!
ui/revenuecatui/src/main/kotlin/com/revenuecat/purchases/ui/revenuecatui/views/PaywallView.kt
Outdated
Show resolved
Hide resolved
In this PR I wasn't using the newly provided presented offering context yet. So I added that in a separate PR: #2612 |
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! Just the OriginalTemplatePaywallFooterView
deprecation message mostly.
this.offeringSelection = offeringId?.let { OfferingSelection.OfferingId(it) } | ||
?: OfferingSelection.None | ||
internal fun setOfferingIdAndPresentedOfferingContext( | ||
offeringInfo: OfferingSelection.IdAndPresentedOfferingContext?, |
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.
nit:
offeringInfo: OfferingSelection.IdAndPresentedOfferingContext?, | |
idAndPresentedOfferingContext: OfferingSelection.IdAndPresentedOfferingContext?, |
@Deprecated( | ||
"Use setOfferingInfo instead.", | ||
ReplaceWith( | ||
"setOfferingInfo(offeringId, presentedOfferingContext)", | ||
), | ||
) |
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.
setOfferingInfo
does not exist anymore, and setOfferingIdAndPresentedOfferingContext
is internal. Should we add a setOffering
function here? If not, we should remove the ReplaceWith
from the deprecation message.
// WIP: We do not support presentedOfferingContext when using the view in XML layouts. | ||
presentedOfferingContext = null, |
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.
Not for this PR, just thinking out loud: what if we support passing a placement in XML layouts?
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 we could... TBH, I'm not sure how many people are using paywalls through XMLs... but it's certainly something we could add if needed.
@JvmOverloads | ||
fun setOfferingId(offeringId: String?, presentedOfferingContext: PresentedOfferingContext? = null) { |
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 need a setOffering(Offering)
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 believe we needed this for RN if I remember correctly, that required us to set the offering AFTER actually creating the paywall, which was pretty convoluted. We didn't need the setOffering
API until now, and in general, I think it's better to just pass it in the constructor.
### Description Based on #2610 This actually makes use of the presented offering context to pass a modified offering to the paywall. This data will be sent as part of the post receipt when a purchase is made.
**This is an automatic release.** ## RevenueCat SDK ### 🐞 Bugfixes * Use `Block store` to backup anonymous user ids across installations (#2595) via Toni Rico (@tonidero) ## RevenueCatUI SDK ### Paywallv2 #### 🐞 Bugfixes * Fixes price formatting discrepancies on Paywalls for `{{ product.price_per_[day|week|month|year] }}` (#2604) via JayShortway (@JayShortway) ### 🔄 Other Changes * Revert dokka 2 and gradle 9 update (#2618) via Toni Rico (@tonidero) * Introduce runtime annotations library and add stability annotations for increasing UI performances (#2608) via Jaewoong Eum (@skydoves) * Override presented offering context paywalls without offering (#2612) via Toni Rico (@tonidero) * Add APIs for hybrid SDKs to set presentedOfferingContext (#2610) via Toni Rico (@tonidero) * Bump Baseline Profiles to 1.4.0 and update profiles (#2611) via Jaewoong Eum (@skydoves) * Migrate deprecated kotlinOptions to compilerOptions (#2607) via Jaewoong Eum (@skydoves) * [AUTOMATIC][Paywalls V2] Updates paywall-preview-resources submodule (#2613) via RevenueCat Git Bot (@RCGitBot) * Migrate amazon & debugview modules to KTS (#2327) via Jaewoong Eum (@skydoves) * Update to Dokka 2.0.0 (#2609) via Toni Rico (@tonidero) * Add log when restoring purchases finds no purchases with some troubleshooting (#2599) via Toni Rico (@tonidero) Co-authored-by: revenuecat-ops <ops@revenuecat.com>
This adds some new internal APIs to be used by our hybrid SDKs that allow to pass a `PresentedOfferingContext` when launching paywalls and passing a specific offering. - [ ] Actually use presented offering context data
Based on #2610 This actually makes use of the presented offering context to pass a modified offering to the paywall. This data will be sent as part of the post receipt when a purchase is made.
Description
This adds some new internal APIs to be used by our hybrid SDKs that allow to pass a
PresentedOfferingContext
when launching paywalls and passing a specific offering.