-
Notifications
You must be signed in to change notification settings - Fork 6
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
Support siteId
per tracking request
#116
Changes from 8 commits
3fd2009
329735f
3c1e4d0
e4d96e7
7f2782e
2273370
306635d
f4bb8a2
68eec00
9af20d9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,12 +1,10 @@ | ||
package com.parsely.parselyandroid | ||
|
||
import com.parsely.parselyandroid.Logging.log | ||
import java.util.Calendar | ||
import java.util.TimeZone | ||
|
||
internal class EventsBuilder( | ||
private val deviceInfoRepository: DeviceInfoRepository, | ||
private val siteId: String, | ||
private val initializationSiteId: String, | ||
private val clock: Clock, | ||
) { | ||
/** | ||
|
@@ -24,15 +22,19 @@ internal class EventsBuilder( | |
action: String, | ||
metadata: ParselyMetadata?, | ||
extraData: Map<String, Any>?, | ||
uuid: String | ||
uuid: String, | ||
siteIdSource: SiteIdSource, | ||
Comment on lines
+27
to
+28
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Parameter docs for these two are missing. |
||
): Map<String, Any> { | ||
log("buildEvent called for %s/%s", action, url) | ||
|
||
// Main event info | ||
val event: MutableMap<String, Any> = HashMap() | ||
event["url"] = url | ||
event["urlref"] = urlRef | ||
event["idsite"] = siteId | ||
event["idsite"] = when (siteIdSource) { | ||
is SiteIdSource.Default -> initializationSiteId | ||
is SiteIdSource.Custom -> siteIdSource.siteId | ||
} | ||
event["action"] = action | ||
|
||
// Make a copy of extraData and add some things. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps it is worth it to add / repeat / emphasize a bit more info on siteId and what's the default behavior? |
||
*/ | ||
public fun trackPageview( | ||
url: String, | ||
urlRef: String = "", | ||
urlMetadata: ParselyMetadata? = null, | ||
extraData: Map<String, Any>? = null, | ||
siteIdSource: SiteIdSource = SiteIdSource.Default, | ||
) | ||
|
||
/** | ||
|
@@ -59,11 +61,13 @@ public interface ParselyTracker { | |
* @param url The URL of the tracked article (eg: “http://example.com/some-old/article.html“) | ||
* @param urlRef The url of the page that linked to the page being engaged with. Analogous to HTTP referer | ||
* @param extraData A map of additional information to send with the generated `heartbeat` events | ||
* @param siteIdSource The source of the site ID to use for the event. | ||
*/ | ||
public fun startEngagement( | ||
url: String, | ||
urlRef: String = "", | ||
extraData: Map<String, Any>? = null | ||
extraData: Map<String, Any>? = null, | ||
siteIdSource: SiteIdSource = SiteIdSource.Default, | ||
) | ||
|
||
/** | ||
|
@@ -96,12 +100,14 @@ public interface ParselyTracker { | |
* @param urlRef The url of the page that linked to the page being engaged with. Analogous to HTTP referer | ||
* @param videoMetadata Metadata about the tracked video | ||
* @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. | ||
*/ | ||
public fun trackPlay( | ||
url: String, | ||
urlRef: String = "", | ||
videoMetadata: ParselyVideoMetadata, | ||
extraData: Map<String, Any>? = null, | ||
siteIdSource: SiteIdSource = SiteIdSource.Default, | ||
) | ||
|
||
/** | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
package com.parsely.parselyandroid | ||
|
||
/** | ||
* Configuration for the site ID to be used for an event. | ||
*/ | ||
public sealed class SiteIdSource { | ||
/** | ||
* Instruct SDK to use site ID provided during [ParselyTracker.init]. | ||
*/ | ||
public data object Default : SiteIdSource() | ||
|
||
/** | ||
* Instruct SDK to override the site ID for the event. | ||
* | ||
* @param siteId The Parsely public site ID (e.g. "example.com") to use for the event. | ||
*/ | ||
public data class Custom(val siteId: String) : SiteIdSource() | ||
} |
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 wonder if
ParselyTracker.init()
shouldn't also receive aSiteIdSource
🤔Are there reasons not to?
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.
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.trackPageview
it's "custom" to the value provided in initialization, so it makes sense 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.
Got it! Makes sense 👍