Wenxi/intercom destination #40
Conversation
|
don't review yet. unit tests coming up |
| lateinit var intercom: Intercom | ||
| private set | ||
|
|
||
| // Intercom common specced attributes |
There was a problem hiding this comment.
do u think moving this out into a companion object and using const val is better practice?
There was a problem hiding this comment.
yeah. I think you're right. I was misled by the first and second answers here. put it into companion indeed creates an unnecessary getter. but I forgot that if they are not const then each object will have a copy of them. definitely suboptimal. I'll make the change
| analytics.log("Intercom.client().updateUser(userAttributes)") | ||
| } | ||
|
|
||
| private fun setCompany(traits: JsonObject): Company { |
There was a problem hiding this comment.
is traits the correct name here?
There was a problem hiding this comment.
it seems more like its related to the company
There was a problem hiding this comment.
good catch. company is better.
| import io.intercom.android.sdk.identity.Registration | ||
| import kotlinx.serialization.json.* | ||
|
|
||
| /* |
| else { | ||
| val registration = Registration.create().withUserId(userId) | ||
| intercom.registerIdentifiedUser(registration) | ||
| analytics.log("Intercom.client().registerIdentifiedUser(registration)") |
There was a problem hiding this comment.
This might need to be
"Intercom.client().registerIdentifiedUser($registration)"
with the idea being that we are logging the value of registration
There was a problem hiding this comment.
yeah. this is something I wasn't sure of. the original java version logs it this way. I was wondering if registration has sensitive data, so the java version is doing it on purpose?
There was a problem hiding this comment.
oh interesting. lets keep it as is then for now
|
|
||
| properties.forEach { (key, value) -> | ||
| if (key !in setOf("products", REVENUE, TOTAL, CURRENCY) | ||
| && value is JsonPrimitive) { |
There was a problem hiding this comment.
why are we checking the type here?
There was a problem hiding this comment.
this is a translation from the original java code, where it excludes all the map and list values. check these lines.
There was a problem hiding this comment.
oh i see. lets add a comment here stating that we are only interested in primitive values and not maps or collections
|
|
||
| // mock intercom | ||
| mockkStatic(Intercom::class) | ||
| intercom = spyk<Intercom>(StubIntercom()) |
There was a problem hiding this comment.
any reason why we are using a spy of a StubIntercom vs a direct mock?
There was a problem hiding this comment.
I had tests failed because of direct mock, so I changed it to spy. I just tested mock with relaxUnitFun = true that works well. will remove the stub
| } | ||
| } | ||
|
|
||
| val event = buildJsonObject { |
There was a problem hiding this comment.
maybe eventProperties is a more apt name here
Uh oh!
There was an error while loading. Please reload this page.