Support siteId per tracking request
#116
Conversation
…`siteId` per event
This change was making PR review more difficult, better to do in a separate PR
|
hi @iangmaia 👋 would you be ok reviewing this PR? |
| uuid: String, | ||
| siteIdSource: SiteIdSource, |
There was a problem hiding this comment.
Parameter docs for these two are missing.
Perhaps adding more details specially on how siteIdSource works with its default can also be helpful.
| @@ -36,12 +36,14 @@ public interface ParselyTracker { | |||
| * content). Do not use this for **content also hosted on** URLs Parse.ly | |||
| * would normally crawl. | |||
| * @param extraData A Map of additional information to send with the event. | |||
| * @param siteIdSource The source of the site ID to use for the event. | |||
There was a problem hiding this comment.
Perhaps it is worth it to add / repeat / emphasize a bit more info on siteId and what's the default behavior?
|
Thanks @iangmaia ! I've added and extended comments, ready for another round 👍 |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #116 +/- ##
==========================================
- Coverage 70.54% 70.46% -0.09%
==========================================
Files 21 22 +1
Lines 404 413 +9
Branches 49 51 +2
==========================================
+ Hits 285 291 +6
- Misses 104 107 +3
Partials 15 15 ☔ View full report in Codecov by Sentry. |
| @@ -31,7 +35,7 @@ protected void onCreate(Bundle savedInstanceState) { | |||
| setContentView(R.layout.activity_main); | |||
|
|
|||
| // initialize the Parsely tracker with your site id and the current Context | |||
| ParselyTracker.init("example.com", 30, this, true); | |||
| ParselyTracker.init(DEFAULT_SITE_ID, 30, this, true); | |||
There was a problem hiding this comment.
I wonder if ParselyTracker.init() shouldn't also receive a SiteIdSource 🤔
Are there reasons not to?
There was a problem hiding this comment.
Because we need some site id to be defined. If ParselyTracker.init() received SiteIdSource, then API consumer could pass SiteIdSource.Default, which would have to be supported and it's not a valid state.
Alternatively, we could make ParselyTracker.init() receive SiteIdSource.Custom, but I think it only shadows implementation and causes confusion as it can raise a question: "custom" to what? In ParselyTracker.trackPageview it's "custom" to the value provided in initialization, so it makes sense there.
There was a problem hiding this comment.
Because we need some site id to be defined. If
ParselyTracker.init()receivedSiteIdSource, then API consumer could passSiteIdSource.Default, which would have to be supported and it's not a valid state.Alternatively, we could make
ParselyTracker.init()receiveSiteIdSource.Custom, but I think it only shadows implementation and causes confusion as it can raise a question: "custom" to what? InParselyTracker.trackPageviewit's "custom" to the value provided in initialization, so it makes sense there.
Got it! Makes sense 👍
Closes: #115
Description
This PR introduces a possibility for SDK users to provide a custom
siteIdwhenever they record a pixel/track event.This will also align to iOS, which supports this feature already
https://github.com/Parsely/AnalyticsSDK-iOS/blob/9b9db5478996582fdbd07d5aa4a2da0134f9ffe7/Sources/ParselyTracker.swift#L47-L48
Design
I've decided to propose more verbose API and introduce
SiteIdSourceinstead of aStringwhich is empty by default. I think this way it's easier for SDK users to understand the behavior.Backwards compatibility
Because we cannot introduce
@JvmOverloadson interface methods, this change will be breaking for Java users. But I think this is acceptable - I plan to release4.1.0after this (not bumping major version). In the case of Java users, addressing breaking changes will be as easy as providingSiteIdSource.Default.INSTANCEin tracking methods.Test
This PR introduces unit tests and functional test case for engagement manager case. We can also test this with example app:
idsiteequal to the custom id you provided in step 2.