feat: in app click tracking #187
Conversation
|
Pull request title looks good 👍! If this pull request gets merged, it will cause a new release of the software. Example: If this project's latest release version is All merged pull requests will eventually get deployed. But some types of pull requests will trigger a deployment (such as features and bug fixes) while some pull requests will wait to get deployed until a later time. This project uses a special format for pull requests titles. Expand this section to learn more (expand by clicking the ᐅ symbol on the left side of this sentence)...This project uses a special format for pull requests titles. Don't worry, it's easy! This pull request title should be in this format: If your pull request introduces breaking changes to the code, use this format: where
Examples:Need more examples? Want to learn more about this format? Check out the official docs. Note: If your pull request does multiple things such as adding a feature and makes changes to the CI server and fixes some bugs then you might want to consider splitting this pull request up into multiple smaller pull requests. |
|
Hey, there @Shahroz16 👋🤖. I'm a bot here to help you.
This pull request might still be allowed to be merged. However, you might want to consider make this pull request merge into a different branch other then This project uses a special format for pull requests titles. Expand this section to learn more (expand by clicking the ᐅ symbol on the left side of this sentence)...This project uses a special format for pull requests titles. Don't worry, it's easy! This pull request title should be in this format: If your pull request introduces breaking changes to the code, use this format: where
Examples:Need more examples? Want to learn more about this format? Check out the official docs. Note: If your pull request does multiple things such as adding a feature and makes changes to the CI server and fixes some bugs then you might want to consider splitting this pull request up into multiple smaller pull requests. |
Codecov Report
@@ Coverage Diff @@
## main #187 +/- ##
============================================
- Coverage 63.52% 63.38% -0.15%
Complexity 218 218
============================================
Files 91 91
Lines 2051 2059 +8
Branches 263 263
============================================
+ Hits 1303 1305 +2
- Misses 646 654 +8
+ Partials 102 100 -2
... and 2 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
|
Build available to test |
| @@ -13,7 +13,8 @@ internal enum class DeliveryType { | |||
| internal data class DeliveryPayload( | |||
| @field:Json(name = "delivery_id") val deliveryID: String, | |||
| val event: MetricEvent, | |||
| val timestamp: Date | |||
| val timestamp: Date, | |||
| @field:Json(name = "metadata") val metaData: Map<String, String> | |||
There was a problem hiding this comment.
| @field:Json(name = "metadata") val metaData: Map<String, String> | |
| val metadata: Map<String, String> |
I think metadata is spelled as one word. having it as one word could make this code more simple, too.
There was a problem hiding this comment.
Separately, from the ticket, the metadata payload is defined currently. I would prefer instead of using Map<String, String>, we have a data class with action_name and action_value inside of it.
There was a problem hiding this comment.
I think metadata is spelled as one word
I usually see it as meta-data, in android.content.pm Android class, they use the variable public Bundle metaData;
The metadata payload is defined currently in ticket
I don't think that is the case, that was for illustration purposes only. In Web SDK it's not a class either but a dictionary.
There was a problem hiding this comment.
I think metadata is spelled as one word
Huh. That is the first time I have seen metaData. In this PR, I see metaData and metadata both being used and I would prefer consistency being used in the codebase. Especially with a @Json annotation of metadata. I am not going to block this PR based on this, however.
The metadata payload is defined currently in ticket
If the SDK sends a pre-determined data set like this PR currently does (we send 2 properties, and the keys are hard-coded), then I would strongly prefer we stick with a strongly typed data set. However, such as the feature of profile/device attributes where we allow customers to send an undetermined data set with undetermined keys, that's where I see a Map being helpful. This is the piece of the PR that currently holds me back from approval of the PR.
There was a problem hiding this comment.
If the SDK sends a pre-determined data set
I think that's our assumption that BE would always require these 2 values because the meta-data could be more than that. We only need these two variables, but in the future, we might need more. I don't think it's fixed it's always going to be only these 2 variables forever.
So that is why I am more comfortable, with Map, I think the product shared the same idea as this is also a Map in the web SDK as mentioned earlier. So I would like to stick with the Map, if you feel more strongly about it, we can check with the broader team for you.
There was a problem hiding this comment.
I agree that Map is a great solution when the backend accepts a not pre-determined data set. I also agree that the backend today accepts a pre-determined data set and could change that requirement.
The backend may never make a requirement change. It might make a requirement change to a different endpoint, not this one.
In my code review, I am suggesting to the team that when a payload is pre-determined in the API today, we use a data class. When the payload is not pre-determiend in the API today, we use a Map. When the backend makes a change to it's requirements, we make a modification to the SDKs to conform to that change.
Since we cannot predict the future and the backend may never make a requirements change, I would rather use strongly typed data classes when possible to make our code easier to maintain.
There was a problem hiding this comment.
Thanks for the insights, I agree on the backend may or may not require changes, but metadata for me makes sense as a Map, as it is supposed to not be pre-defined in nature even if it has pre-defined keys. Like the Android class, I mentioned earlier has meta-data as a Bundle which could hold any key and value.
I don't want to add another class that we need to keep and maintain for this use case. It's the same thing as using Pair or any other data structure provided for convenience, all of them could be replaced by a static data class.
I think it comes down to personal preference, and I see Rehan has approved it already, web is also using Map so a lot of members of the team aren't married to the idea of replacing Map with a static class in this case.
So I would like to stick with it as well, I don't feel like it's a blocker because it's not customer-facing nor is its performance related but rather preference.
There was a problem hiding this comment.
To try and make communication easier on this thread, I played with the code and made a refactor for this branch illustrating what I was referring to in my comments above.
I do have a personal preference to use strongly-typed data classes when possible. However, I am open to not use a strong data type for this use case if the rest of the team prefers not to.
To prevent this PR from being a blocker, I'll approve this PR and leave it up to the rest of the team to decide if this PR gets deployed as-is or include the refactor.
Thanks for the work on the PR and the PR review conversations 👍
There was a problem hiding this comment.
Appreciate the PR Levi, it helps unblock and make sure that even if the work refactoring doesn't get merged in this round it can get merged in the second one. Refactor looks good, but I might not be able to test out if the json structuring is coming out correctly and test it manually with RH to see if all cases are working.
If you can do that and confirm, I'll happily merge otherwise, If I manage to test it out myself I'll merge it otherwise I'll go with the current implementation which was tested by me. 👍
## [3.4.0](3.3.2...3.4.0) (2023-04-19) ### Features * in app click tracking ([#187](#187)) ([4ad1f35](4ad1f35))
closes: https://github.com/customerio/issues/issues/8657
Complete each step to get your pull request merged in. Learn more about the workflow this project uses.