Skip to content
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

Feature/aris/threads analytics #5378

Merged
merged 9 commits into from Mar 16, 2022

Conversation

ariskotsomitopoulos
Copy link
Contributor

@ariskotsomitopoulos ariskotsomitopoulos commented Feb 28, 2022

This PR add analytics support for threads and closes #4910

Note: This PR requires Live Threads PR to be merged first, let's make this PR draft until we merge the prerequisite PR

@ariskotsomitopoulos ariskotsomitopoulos marked this pull request as draft February 28, 2022 15:17
@github-actions
Copy link

github-actions bot commented Feb 28, 2022

Unit Test Results

102 files  ±0  102 suites  ±0   1m 8s ⏱️ -1s
182 tests ±0  182 ✔️ ±0  0 💤 ±0  0 ±0 
598 runs  ±0  598 ✔️ ±0  0 💤 ±0  0 ±0 

Results for commit bcf3f1e. ± Comparison against base commit 99b43fd.

♻️ This comment has been updated with latest results.

…/threads_analytics

# Conflicts:
#	vector/src/main/java/im/vector/app/features/analytics/plan/Interaction.kt
@ariskotsomitopoulos ariskotsomitopoulos marked this pull request as ready for review March 15, 2022 14:21
@@ -188,6 +189,9 @@ class MessageComposerViewModel @AssistedInject constructor(

private fun handleSendMessage(action: MessageComposerAction.SendMessage) {
withState { state ->
analyticsTracker.capture(state.toAnalyticsComposer()).also {
setState { copy(startsThread = false) }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if there's a way to avoid this mutable state but I guess we need to be sure we only track the first message

import im.vector.app.features.home.room.detail.composer.MessageComposerViewState
import im.vector.app.features.home.room.detail.composer.SendMode

fun MessageComposerViewState.toAnalyticsComposer(): Composer =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tiny formatting comment (ideally the formatter would do this for us), the kotlin convention is typically to move the closing ) to a new line

fun MessageComposerViewState.toAnalyticsComposer() = Composer(
    inThread = isInThreadTimeline(),
    isEditing = sendMode is SendMode.Edit,
    isReply = sendMode is SendMode.Reply,
    startsThread = startsThread
)

same for https://github.com/vector-im/element-android/pull/5378/files#diff-5f35fe0e05f4f8d301892d428e93ca87334bd099a0821f068ea5f5ecfc90adcaR21

Copy link
Contributor

@ouchadam ouchadam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! some tiny comments on formatting (until we have a way to enforce a convention or written examples it's not a blocker for me, but would be great to update)

will leave merging up to you @ariskotsomitopoulos

Copy link
Contributor

@ouchadam ouchadam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯

@ariskotsomitopoulos ariskotsomitopoulos merged commit d1a77d2 into develop Mar 16, 2022
@ariskotsomitopoulos ariskotsomitopoulos deleted the feature/aris/threads_analytics branch March 16, 2022 14:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Threads: Analytics
2 participants