Adds setAppsFlyerConversionData to conveniently track AppsFlyer conversion data#2931
Conversation
|
Cursor Agent can help with this pull request. Just |
b1a220e to
d575239
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2931 +/- ##
==========================================
+ Coverage 78.82% 78.84% +0.02%
==========================================
Files 337 337
Lines 13129 13163 +34
Branches 1771 1786 +15
==========================================
+ Hits 10349 10379 +30
- Misses 2038 2041 +3
- Partials 742 743 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
tonidero
left a comment
There was a problem hiding this comment.
My only real comment is to try to move this logic out of PurchasesOrchestrator. Other than that, looks great! Amazing job!
| * | ||
| * @param data The conversion data map from AppsFlyer's `onConversionDataSuccess` callback. | ||
| */ | ||
| fun setAppsFlyerAttributionData(data: Map<*, *>?) { |
There was a problem hiding this comment.
Maybe it's simpler this way... but I was wondering whether it would be worth adding a AppslyerAttributionData class that can be constructed with this map, so we perform some validations there and it's more flexible in case we want to change the signature later...
But honestly, don't think it's a blocker. Probably shouldn't throw in case it doesn't pass validations, and we can adapt the signature later if needed, so FFTI!!
There was a problem hiding this comment.
Right, I'm very much pro wrapper classes in the public API to allow us to evolve it in a non-breaking way. However in this case I'm not sure if it's worth it. It's really meant to be called like this:
override fun onConversionDataSuccess(data: MutableMap<String, Any>?) {
Purchases.sharedInstance.setAppsFlyerAttributionData(data)
}If AppsFlyer changes the data type, we would add a new function and deprecate the old one. If we had a wrapper class, we would add a new constructor and deprecate the old one. That's pretty much the same, with a tiny bit of added friction because developers need to understand that they need to construct the wrapper class first.
| log(LogIntent.DEBUG) { AttributionStrings.METHOD_CALLED.format("setAppsFlyerAttributionData") } | ||
|
|
||
| if (data == null) { | ||
| return |
There was a problem hiding this comment.
Same question here as on iOS, is it ok that we're not support unsetting attributes?
There was a problem hiding this comment.
I think that's okay. Conversion attribution is determined once for an install, and never changes. It might also be confusing. For instance, if you've set campaign data manually, and then call this function with a map that doesn't contain campaign data, it would be cleared. That might be unexpected.
I thought about supporting unsetting all attributes at once if the developer passes a nil dictionary. However I also think that would be confusing, if sometimes nil unsets and sometimes it doesn't. I chose to clarify this behavior in the docs instead. (ae39ec7).
6f9e3ab to
5a111dd
Compare
setAppsFlyerConversionData to conveniently track AppsFlyer conversion data
ajpallares
left a comment
There was a problem hiding this comment.
Looks good! Just a couple of minor comments. Not a blocker though!
| internal fun Map<*, *>.getStringValue(key: String): String? { | ||
| return when (val value = this[key]) { | ||
| is String -> value.takeIf { it.isNotBlank() } | ||
| null -> null | ||
| else -> value.toString().takeIf { it.isNotBlank() } | ||
| } | ||
| } |
There was a problem hiding this comment.
Nit: Couldn't this be simplified to
internal fun Map<*, *>.getStringValue(key: String): String? =
this[key]?.toString()?.takeIf { it.isNotBlank() }
There was a problem hiding this comment.
Ah yes, other than calling toString on a String, those should be equivalent indeed! Done here: 23d7411.
| # Kotlin compiler | ||
| .kotlin |
There was a problem hiding this comment.
This change seems unrelated to this PR. I think the change makes sense, but idk if maybe should be done in a separate PR? (for easier tracking mostly)
There was a problem hiding this comment.
Good point, done! Changed the base branch of this PR.
cb89c6d to
23d7411
Compare
23d7411 to
39f700a
Compare
39f700a to
e792cfa
Compare
e792cfa to
2d5e7b1
Compare
## RevenueCat SDK ### ✨ New Features * Adds `setAppsFlyerConversionData` to conveniently track AppsFlyer conversion data (#2931) via JayShortway (@JayShortway) ### 🐞 Bugfixes * Make close() method also clear the shared instance of the SDK (#2940) via Toni Rico (@tonidero) * Fix purchase callback not firing for DEFERRED product changes with baePlanId in oldProductId (#2937) via Facundo Menzella (@facumenzella) ### 🔄 Other Changes * [AUTOMATIC] Update golden test files for backend integration tests (#2949) via RevenueCat Git Bot (@RCGitBot) * [AUTOMATIC] Update golden test files for backend integration tests (#2944) via RevenueCat Git Bot (@RCGitBot) * Adds `.kotlin` to `.gitignore` (#2941) via JayShortway (@JayShortway) * Dont use reflection to instantiate AmazonOfferingParser (#2934) via Will Taylor (@fire-at-will) --------- Co-authored-by: revenuecat-ops <ops@revenuecat.com> Co-authored-by: Toni Rico <toni.rico.diez@revenuecat.com>
Motivation
Mini-spec
Description
Introduces a new public function
fun setAppsFlyerConversionData(data: Map<*, *>?)toPurchasesthat processes AppsFlyer conversion data and sets corresponding RevenueCat subscriber attributes.