Tracking events domain objects

[dimamo5]

Depends on #3

This PR adds tracking events objects. Now the tracking function uses these objects instead of just a JSONObject. To allow the user to send their own types I created a CustomTrackingEvent that accepts a JSONObject and only validates its type, clientId and siteId.

[codecov]

Codecov Report

Merging #4 into master will increase coverage by 1.59%.
The diff coverage is 83.33%.

@@             Coverage Diff              @@
##             master       #4      +/-   ##
============================================
+ Coverage     67.58%   69.17%   +1.59%     
- Complexity        0        1       +1     
============================================
  Files             8        8              
  Lines           145      146       +1     
  Branches         19       19              
============================================
+ Hits             98      101       +3     
+ Misses           39       37       -2     
  Partials          8        8

Impacted Files	Coverage Δ	Complexity Δ
...ocidi-sdk/src/main/kotlin/com/velocidi/Velocidi.kt	75.86% <83.33%> (ø)	0 <0> (ø)	⬇️
...idi-sdk/src/main/kotlin/com/velocidi/HttpClient.kt	84.61% <0%> (-1.75%)	1% <0%> (+1%)
...c/main/kotlin/com/velocidi/GetAdvertisingIdTask.kt	7.14% <0%> (+1.26%)	0% <0%> (ø)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b98ddc1...9c89eb0. Read the comment docs.

[duartepinto]

Apart from what I left as comments, I also think that siteId and clientId should not be optional when instantiating an instance of TrackingEvent. All subclasses should require those fields in the constructor. Also, personal opinion, don't really like that we have all the possible properties declared in the class constructor. This leads to really big constructors. But this is more of a personal thing, don't have a really strong opinion about this

[dimamo5]

I changed all the product to products in 6305e05 and also upgraded the kotlinx-serialization to v0.11. Now, optional fields no longer need the optional annotation.

[dimamo5]

Apart from what I left as comments, I also think that siteId and clientId should not be optional when instantiating an instance of TrackingEvent. All subclasses should require those fields in the constructor.

Improved in 38af754

[duartepinto]

Also, personal opinion, don't really like that we have all the possible properties declared in the class constructor. This leads to really big constructors. But this is more of a personal thing, don't have a really strong opinion about this.

About this, due to the offline conversation that we had, If in the future we drop the siteId and the clientId, in order to keep backward compatibility, we will still have to provide a constructor with those parameters, leading us to have a really big constructor just for backwards compatibility. Having such big constructor is not easy to manage in the long run. I would prefer, for now, to only have the constructor with siteId, typeId and other required properties for each model class so that it is easier to drop them or change them in the future, should the necessity arise.

[dimamo5]

I've been doing some testing and kotlinx.serialization doesn't like having properties defined in the class body. It would serialize them but to ones in the parent class.

For example:

data class AddToCart(
    override val siteId: String,
    override val clientId: String
) : TrackingEvent("addToCart") {
    var products: List<Product> = emptyList()

    override fun serialize(): String =
        jsonSerilizer().stringify(serializer(), this)
}

In this case, it would only serialize siteId, clientId and products but not type.

I also tried changing TrackingEvent to an interface or making type an abstract val, but neither of them worked.

I think the best solution is leaving it like it is.

[duartepinto]

I've been doing some testing and kotlinx.serialization doesn't like having properties defined in the class body. It would serialize them but to ones in the parent class.

What about replacing kotlinx.serialization with something like gson which is a bit more stable? Performance wise, gson is also quite fast and is a bit more mature than kotlinx.serialization (which still hasn't reached version 1.X)

[dimamo5]

I think with Gson we couldn't have inheritance and we could only generate JSON from data class.
We picked kotlinx.serialization because it's officially supported by Kotlin and is already shipped with the Kotlin compiler.

If you think it's worth I can explore gson.

[duartepinto]

If you think it's worth I can explore gson.

Yes, please. Just give it a quick look and see if it is feasible.

[dimamo5]

After some R&D I found that Gson works really well with Kotlin. I decided to migrate in d9c1bcb and applied your suggestion regarding "big constructors" in faedd38. I also prefer the way custom serializers are built in Gson.

[duartepinto]

applied your suggestion regarding "big constructors" in faedd38

Sorry for nitpicking on this, but can you do the same for Product ProductCustomization and Transaction?

[dimamo5]

Improved in 9c89eb0

[duartepinto]

LGTM!