-
Notifications
You must be signed in to change notification settings - Fork 29
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
fix: Segmentation missing for countly events (WPB-14646) #3816
Conversation
@@ -257,7 +261,12 @@ interface AnalyticsEvent { | |||
} | |||
|
|||
sealed class QrCode : AnalyticsEvent { | |||
data class Click(val isTeam: Boolean) : QrCode() { | |||
data class Click( |
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.
question: why still only some of them get these new fields? Wouldn't be better to add it to all of them? Maybe even inside the sendEvent
method so that they are always added in one place 🤔
class AnonymousAnalyticsRecorderImpl(private val appVersion: String) : AnonymousAnalyticsRecorder {
...
override fun sendEvent(event: AnalyticsEvent) = wrapCountlyRequest {
Countly.sharedInstance()?.events()?.recordEvent(
event.key,
event.toSegmentation().toMutableMap()
.plus("app_version" to appVersion)
.plus("device_model" to Build.MODEL)
.plus("os_version" to Build.VERSION.RELEASE)
)
}
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 am following product specs not all events are having that, but you have a good point better to add those three segmentations to all events.
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 also think we could add these parameters to all events. In case we do not want to do that, maybe we can have another sendWithAppInfo
fun, or new withAppInfo: Boolean = false
parameter in sendEvent
to avoid repeating these parameters every time we call sendEvent
@@ -257,7 +261,12 @@ interface AnalyticsEvent { | |||
} | |||
|
|||
sealed class QrCode : AnalyticsEvent { | |||
data class Click(val isTeam: Boolean) : QrCode() { | |||
data class Click( |
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 also think we could add these parameters to all events. In case we do not want to do that, maybe we can have another sendWithAppInfo
fun, or new withAppInfo: Boolean = false
parameter in sendEvent
to avoid repeating these parameters every time we call sendEvent
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3816 +/- ##
========================================
Coverage 45.43% 45.44%
========================================
Files 490 490
Lines 16895 16900 +5
Branches 2833 2833
========================================
+ Hits 7677 7680 +3
- Misses 8432 8434 +2
Partials 786 786
Continue to review full report in Codecov by Sentry.
|
Quality Gate passedIssues Measures |
PR Submission Checklist for internal contributors
The PR Title
SQPIT-764
The PR Description
What's new in this PR?
Issues
Some events are missing some segmentations:
For example app_version, device_model and os_version are not included in ui.QR-click
Causes (Optional)
Not implemented
Dependencies (Optional)
If there are some other pull requests related to this one (e.g. new releases of frameworks), specify them here.
Needs releases with:
Testing
Test Coverage (Optional)
How to Test
Briefly describe how this change was tested and if applicable the exact steps taken to verify that it works as expected.
Notes (Optional)
Specify here any other facts that you think are important for this issue.
Attachments (Optional)
Attachments like images, videos, etc. (drag and drop in the text box)
PR Post Submission Checklist for internal contributors (Optional)
PR Post Merge Checklist for internal contributors
References
feat(conversation-list): Sort conversations by most emojis in the title #SQPIT-764
.