From c43069971120485b2e0235a32ae23d63a52ba807 Mon Sep 17 00:00:00 2001 From: Valere Date: Mon, 6 May 2024 19:06:54 +0200 Subject: [PATCH 1/4] Add support for Verification state analytics --- appnav/build.gradle.kts | 1 + .../loggedin/AnalyticsVerificationStateExt.kt | 59 +++++++++ .../appnav/loggedin/LoggedInPresenter.kt | 33 +++++ .../appnav/loggedin/LoggedInPresenterTest.kt | 4 + gradle/libs.versions.toml | 2 +- .../posthog/build.gradle.kts | 6 + .../posthog/PosthogAnalyticsProvider.kt | 33 +++-- .../posthog/PosthogAnalyticsProviderTest.kt | 116 ++++++++++++++++++ 8 files changed, 245 insertions(+), 9 deletions(-) create mode 100644 appnav/src/main/kotlin/io/element/android/appnav/loggedin/AnalyticsVerificationStateExt.kt create mode 100644 services/analyticsproviders/posthog/src/test/kotlin/io/element/android/services/analyticsproviders/posthog/PosthogAnalyticsProviderTest.kt diff --git a/appnav/build.gradle.kts b/appnav/build.gradle.kts index ab215f8394..c24e544240 100644 --- a/appnav/build.gradle.kts +++ b/appnav/build.gradle.kts @@ -72,6 +72,7 @@ dependencies { testImplementation(projects.features.rageshake.test) testImplementation(projects.features.rageshake.impl) testImplementation(projects.services.appnavstate.test) + testImplementation(projects.services.analytics.test) testImplementation(libs.test.appyx.junit) testImplementation(libs.test.arch.core) diff --git a/appnav/src/main/kotlin/io/element/android/appnav/loggedin/AnalyticsVerificationStateExt.kt b/appnav/src/main/kotlin/io/element/android/appnav/loggedin/AnalyticsVerificationStateExt.kt new file mode 100644 index 0000000000..2afa0e40f2 --- /dev/null +++ b/appnav/src/main/kotlin/io/element/android/appnav/loggedin/AnalyticsVerificationStateExt.kt @@ -0,0 +1,59 @@ +/* + * Copyright (c) 2024 New Vector Ltd + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package io.element.android.appnav.loggedin + +import im.vector.app.features.analytics.plan.CryptoSessionStateChange +import im.vector.app.features.analytics.plan.UserProperties +import io.element.android.libraries.matrix.api.encryption.RecoveryState +import io.element.android.libraries.matrix.api.verification.SessionVerifiedStatus + +fun SessionVerifiedStatus.toAnalyticsUserPropertyValue(): UserProperties.VerificationState? { + return when (this) { + // we don't need to report transient states + SessionVerifiedStatus.Unknown -> null + SessionVerifiedStatus.NotVerified -> UserProperties.VerificationState.NotVerified + SessionVerifiedStatus.Verified -> UserProperties.VerificationState.Verified + } +} + +fun RecoveryState.toAnalyticsUserPropertyValue(): UserProperties.RecoveryState? { + return when (this) { + RecoveryState.ENABLED -> UserProperties.RecoveryState.Enabled + RecoveryState.DISABLED -> UserProperties.RecoveryState.Disabled + RecoveryState.INCOMPLETE -> UserProperties.RecoveryState.Incomplete + // we don't need to report transient states + else -> null + } +} +fun SessionVerifiedStatus.toAnalyticsStateChangeValue(): CryptoSessionStateChange.VerificationState? { + return when (this) { + // we don't need to report transient states + SessionVerifiedStatus.Unknown -> null + SessionVerifiedStatus.NotVerified -> CryptoSessionStateChange.VerificationState.NotVerified + SessionVerifiedStatus.Verified -> CryptoSessionStateChange.VerificationState.Verified + } +} + +fun RecoveryState.toAnalyticsStateChangeValue(): CryptoSessionStateChange.RecoveryState? { + return when (this) { + RecoveryState.ENABLED -> CryptoSessionStateChange.RecoveryState.Enabled + RecoveryState.DISABLED -> CryptoSessionStateChange.RecoveryState.Disabled + RecoveryState.INCOMPLETE -> CryptoSessionStateChange.RecoveryState.Incomplete + // we don't need to report transient states + else -> null + } +} diff --git a/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInPresenter.kt b/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInPresenter.kt index 7cb1d634b8..0feaf47029 100644 --- a/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInPresenter.kt +++ b/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInPresenter.kt @@ -22,14 +22,19 @@ import androidx.compose.runtime.collectAsState import androidx.compose.runtime.derivedStateOf import androidx.compose.runtime.getValue import androidx.compose.runtime.remember +import im.vector.app.features.analytics.plan.CryptoSessionStateChange +import im.vector.app.features.analytics.plan.UserProperties import io.element.android.features.networkmonitor.api.NetworkMonitor import io.element.android.features.networkmonitor.api.NetworkStatus import io.element.android.libraries.architecture.Presenter import io.element.android.libraries.matrix.api.MatrixClient +import io.element.android.libraries.matrix.api.encryption.EncryptionService +import io.element.android.libraries.matrix.api.encryption.RecoveryState import io.element.android.libraries.matrix.api.roomlist.RoomListService import io.element.android.libraries.matrix.api.verification.SessionVerificationService import io.element.android.libraries.matrix.api.verification.SessionVerifiedStatus import io.element.android.libraries.push.api.PushService +import io.element.android.services.analytics.api.AnalyticsService import kotlinx.coroutines.flow.map import javax.inject.Inject @@ -38,6 +43,8 @@ class LoggedInPresenter @Inject constructor( private val networkMonitor: NetworkMonitor, private val pushService: PushService, private val sessionVerificationService: SessionVerificationService, + private val analyticsService: AnalyticsService, + private val encryptionService: EncryptionService, ) : Presenter { @Composable override fun present(): LoggedInState { @@ -62,8 +69,34 @@ class LoggedInPresenter @Inject constructor( networkStatus == NetworkStatus.Online && syncIndicator == RoomListService.SyncIndicator.Show } } + val verificationState by sessionVerificationService.sessionVerifiedStatus.collectAsState() + val recoveryState by encryptionService.recoveryStateStateFlow.collectAsState() + reportCryptoStatusToAnalytics(verificationState, recoveryState) + return LoggedInState( showSyncSpinner = showSyncSpinner, ) } + + private fun reportCryptoStatusToAnalytics(verificationState: SessionVerifiedStatus, recoveryState: RecoveryState) { + // Update first the user property, to store the current status for that posthog user + val userVerificationState = verificationState.toAnalyticsUserPropertyValue() + val userRecoveryState = recoveryState.toAnalyticsUserPropertyValue() + if (userRecoveryState != null && userVerificationState != null) { + // we want to report when both value are known (if one is unknown we wait until we have them both) + analyticsService.updateUserProperties( + UserProperties( + verificationState = userVerificationState, + recoveryState = userRecoveryState + ) + ) + } + + // Also report when there is a change in the state, to be able to track the changes + val changeVerificationState = verificationState.toAnalyticsStateChangeValue() + val changeRecoveryState = recoveryState.toAnalyticsStateChangeValue() + if (changeVerificationState != null && changeRecoveryState != null) { + analyticsService.capture(CryptoSessionStateChange(changeRecoveryState, changeVerificationState)) + } + } } diff --git a/appnav/src/test/kotlin/io/element/android/appnav/loggedin/LoggedInPresenterTest.kt b/appnav/src/test/kotlin/io/element/android/appnav/loggedin/LoggedInPresenterTest.kt index df17053512..d1b89fca77 100644 --- a/appnav/src/test/kotlin/io/element/android/appnav/loggedin/LoggedInPresenterTest.kt +++ b/appnav/src/test/kotlin/io/element/android/appnav/loggedin/LoggedInPresenterTest.kt @@ -24,9 +24,11 @@ import io.element.android.features.networkmonitor.api.NetworkStatus import io.element.android.features.networkmonitor.test.FakeNetworkMonitor import io.element.android.libraries.matrix.api.roomlist.RoomListService import io.element.android.libraries.matrix.test.FakeMatrixClient +import io.element.android.libraries.matrix.test.encryption.FakeEncryptionService import io.element.android.libraries.matrix.test.roomlist.FakeRoomListService import io.element.android.libraries.matrix.test.verification.FakeSessionVerificationService import io.element.android.libraries.push.test.FakePushService +import io.element.android.services.analytics.test.FakeAnalyticsService import io.element.android.tests.testutils.WarmUpRule import io.element.android.tests.testutils.consumeItemsUntilPredicate import kotlinx.coroutines.test.runTest @@ -73,6 +75,8 @@ class LoggedInPresenterTest { networkMonitor = FakeNetworkMonitor(networkStatus), pushService = FakePushService(), sessionVerificationService = FakeSessionVerificationService(), + analyticsService = FakeAnalyticsService(), + encryptionService = FakeEncryptionService() ) } } diff --git a/gradle/libs.versions.toml b/gradle/libs.versions.toml index db8cfe6b6a..1d5ee6c65b 100644 --- a/gradle/libs.versions.toml +++ b/gradle/libs.versions.toml @@ -183,7 +183,7 @@ kotlinpoet = "com.squareup:kotlinpoet:1.16.0" posthog = "com.posthog:posthog-android:3.1.18" sentry = "io.sentry:sentry-android:7.8.0" # main branch can be tested replacing the version with main-SNAPSHOT -matrix_analytics_events = "com.github.matrix-org:matrix-analytics-events:0.20.0" +matrix_analytics_events = "com.github.matrix-org:matrix-analytics-events:0.21.0" # Emojibase matrix_emojibase_bindings = "io.element.android:emojibase-bindings:1.1.3" diff --git a/services/analyticsproviders/posthog/build.gradle.kts b/services/analyticsproviders/posthog/build.gradle.kts index c9049af237..8a923d4918 100644 --- a/services/analyticsproviders/posthog/build.gradle.kts +++ b/services/analyticsproviders/posthog/build.gradle.kts @@ -34,4 +34,10 @@ dependencies { implementation(projects.libraries.core) implementation(projects.libraries.di) implementation(projects.services.analyticsproviders.api) + + testImplementation(libs.coroutines.test) + testImplementation(libs.test.truth) + testImplementation(libs.test.junit) + testImplementation(projects.tests.testutils) + testImplementation(libs.test.mockk) } diff --git a/services/analyticsproviders/posthog/src/main/kotlin/io/element/android/services/analyticsproviders/posthog/PosthogAnalyticsProvider.kt b/services/analyticsproviders/posthog/src/main/kotlin/io/element/android/services/analyticsproviders/posthog/PosthogAnalyticsProvider.kt index fb542b7f28..b594dcbee4 100644 --- a/services/analyticsproviders/posthog/src/main/kotlin/io/element/android/services/analyticsproviders/posthog/PosthogAnalyticsProvider.kt +++ b/services/analyticsproviders/posthog/src/main/kotlin/io/element/android/services/analyticsproviders/posthog/PosthogAnalyticsProvider.kt @@ -40,6 +40,10 @@ class PosthogAnalyticsProvider @Inject constructor( private var posthog: PostHogInterface? = null private var analyticsId: String? = null + private var pendingUserProperties: MutableMap? = null + + private val userPropertiesLock = Any() + override fun init() { posthog = createPosthog() posthog?.optIn() @@ -57,10 +61,14 @@ class PosthogAnalyticsProvider @Inject constructor( } override fun capture(event: VectorAnalyticsEvent) { - posthog?.capture( - event = event.getName(), - properties = event.getProperties()?.keepOnlyNonNullValues().withExtraProperties(), - ) + synchronized(userPropertiesLock) { + posthog?.capture( + event = event.getName(), + properties = event.getProperties()?.keepOnlyNonNullValues().withExtraProperties(), + userProperties = pendingUserProperties, + ) + pendingUserProperties = null + } } override fun screen(screen: VectorAnalyticsScreen) { @@ -71,10 +79,19 @@ class PosthogAnalyticsProvider @Inject constructor( } override fun updateUserProperties(userProperties: UserProperties) { -// posthog?.identify( -// REUSE_EXISTING_ID, userProperties.getProperties()?.toPostHogUserProperties(), -// IGNORED_OPTIONS -// ) + synchronized(userPropertiesLock) { + // The pending properties will be sent with the following capture call + if (pendingUserProperties == null) { + pendingUserProperties = HashMap() + } + userProperties.getProperties()?.let { + pendingUserProperties?.putAll(it) + } + // We are not currently using `identify` in EIX, if it was the case + // we could have called identify to update the user properties. + // For now, we have to store them, and they will be updated when the next call + // to capture will happen. + } } override fun trackError(throwable: Throwable) { diff --git a/services/analyticsproviders/posthog/src/test/kotlin/io/element/android/services/analyticsproviders/posthog/PosthogAnalyticsProviderTest.kt b/services/analyticsproviders/posthog/src/test/kotlin/io/element/android/services/analyticsproviders/posthog/PosthogAnalyticsProviderTest.kt new file mode 100644 index 0000000000..9032ea5718 --- /dev/null +++ b/services/analyticsproviders/posthog/src/test/kotlin/io/element/android/services/analyticsproviders/posthog/PosthogAnalyticsProviderTest.kt @@ -0,0 +1,116 @@ +/* + * Copyright (c) 2024 New Vector Ltd + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package io.element.android.services.analyticsproviders.posthog + +import com.google.common.truth.Truth.assertThat +import com.posthog.PostHogInterface +import im.vector.app.features.analytics.itf.VectorAnalyticsEvent +import im.vector.app.features.analytics.plan.UserProperties +import io.element.android.tests.testutils.WarmUpRule +import io.mockk.every +import io.mockk.just +import io.mockk.mockk +import io.mockk.runs +import io.mockk.slot +import io.mockk.verify +import kotlinx.coroutines.test.runTest +import org.junit.Rule +import org.junit.Test + +class PosthogAnalyticsProviderTest { + @get:Rule + val warmUpRule = WarmUpRule() + + @Test + fun `Posthog - Test user properties`() = runTest { + val mockPosthog = mockk().also { + every { it.optIn() } just runs + every { it.capture(any(), any(), any(), any(), any(), any()) } just runs + } + val mockPosthogFactory = mockk() + every { mockPosthogFactory.createPosthog() } returns mockPosthog + + val analyticsProvider = PosthogAnalyticsProvider(mockPosthogFactory) + analyticsProvider.init() + + val testUserProperties = UserProperties( + verificationState = UserProperties.VerificationState.Verified, + recoveryState = UserProperties.RecoveryState.Incomplete, + ) + analyticsProvider.updateUserProperties(testUserProperties) + + // report mock event + val mockEvent = mockk().also { + every { + it.getProperties() + } returns emptyMap() + every { it.getName() } returns "MockEventName" + } + analyticsProvider.capture(mockEvent) + val userPropertiesSlot = slot>() + + verify { mockPosthog.capture(event = "MockEventName", any(), any(), userProperties = capture(userPropertiesSlot)) } + + assertThat(userPropertiesSlot.captured).isNotNull() + assertThat(userPropertiesSlot.captured["verificationState"] as String).isEqualTo(testUserProperties.verificationState?.name) + assertThat(userPropertiesSlot.captured["recoveryState"] as String).isEqualTo(testUserProperties.recoveryState?.name) + + // Should only be reported once when the next event is sent + // Try another capture to check + analyticsProvider.capture(mockEvent) + verify { mockPosthog.capture(any(), any(), any(), userProperties = null) } + } + + @Test + fun `Posthog - Test accumulate user properties until next capture call`() = runTest { + val mockPosthog = mockk().also { + every { it.optIn() } just runs + every { it.capture(any(), any(), any(), any(), any(), any()) } just runs + } + val mockPosthogFactory = mockk() + every { mockPosthogFactory.createPosthog() } returns mockPosthog + + val analyticsProvider = PosthogAnalyticsProvider(mockPosthogFactory) + analyticsProvider.init() + + val testUserProperties = UserProperties( + verificationState = UserProperties.VerificationState.NotVerified, + ) + analyticsProvider.updateUserProperties(testUserProperties) + + // Update again + val testUserPropertiesUpdate = UserProperties( + verificationState = UserProperties.VerificationState.Verified, + ) + analyticsProvider.updateUserProperties(testUserPropertiesUpdate) + + // report mock event + val mockEvent = mockk().also { + every { + it.getProperties() + } returns emptyMap() + every { it.getName() } returns "MockEventName" + } + analyticsProvider.capture(mockEvent) + val userPropertiesSlot = slot>() + + verify { mockPosthog.capture(event = "MockEventName", any(), any(), userProperties = capture(userPropertiesSlot)) } + + assertThat(userPropertiesSlot.captured).isNotNull() + assertThat(userPropertiesSlot.captured["verificationState"] as String).isEqualTo(testUserPropertiesUpdate.verificationState?.name) + } +} From 94bbcbf503565ca5631045dc7251c45a522749b9 Mon Sep 17 00:00:00 2001 From: Valere Date: Tue, 7 May 2024 09:38:26 +0200 Subject: [PATCH 2/4] Code review - more tests --- .../AnalyticsVerificationStateMappingTests.kt | 70 +++++++++++++++++++ .../appnav/loggedin/LoggedInPresenterTest.kt | 46 +++++++++++- .../FakeSessionVerificationService.kt | 4 ++ .../analytics/test/FakeAnalyticsService.kt | 2 + .../posthog/PosthogAnalyticsProvider.kt | 2 +- 5 files changed, 120 insertions(+), 4 deletions(-) create mode 100644 appnav/src/test/kotlin/io/element/android/appnav/loggedin/AnalyticsVerificationStateMappingTests.kt diff --git a/appnav/src/test/kotlin/io/element/android/appnav/loggedin/AnalyticsVerificationStateMappingTests.kt b/appnav/src/test/kotlin/io/element/android/appnav/loggedin/AnalyticsVerificationStateMappingTests.kt new file mode 100644 index 0000000000..cce8879d4f --- /dev/null +++ b/appnav/src/test/kotlin/io/element/android/appnav/loggedin/AnalyticsVerificationStateMappingTests.kt @@ -0,0 +1,70 @@ +/* + * Copyright (c) 2024 New Vector Ltd + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package io.element.android.appnav.loggedin + +import com.google.common.truth.Truth.assertThat +import im.vector.app.features.analytics.plan.CryptoSessionStateChange +import im.vector.app.features.analytics.plan.UserProperties +import io.element.android.libraries.matrix.api.encryption.RecoveryState +import io.element.android.libraries.matrix.api.verification.SessionVerifiedStatus +import io.element.android.tests.testutils.WarmUpRule +import kotlinx.coroutines.test.runTest +import org.junit.Rule +import org.junit.Test + +class AnalyticsVerificationStateMappingTests { + @get:Rule + val warmUpRule = WarmUpRule() + + @Test + fun `Test verification Mappings`() = runTest { + assertThat(SessionVerifiedStatus.Verified.toAnalyticsUserPropertyValue()) + .isEqualTo(UserProperties.VerificationState.Verified) + assertThat(SessionVerifiedStatus.NotVerified.toAnalyticsUserPropertyValue()) + .isEqualTo(UserProperties.VerificationState.NotVerified) + + assertThat(SessionVerifiedStatus.Verified.toAnalyticsStateChangeValue()) + .isEqualTo(CryptoSessionStateChange.VerificationState.Verified) + assertThat(SessionVerifiedStatus.NotVerified.toAnalyticsStateChangeValue()) + .isEqualTo(CryptoSessionStateChange.VerificationState.NotVerified) + } + + @Test + fun `Test recovery state Mappings`() = runTest { + assertThat(RecoveryState.UNKNOWN.toAnalyticsUserPropertyValue()) + .isNull() + assertThat(RecoveryState.WAITING_FOR_SYNC.toAnalyticsUserPropertyValue()) + .isNull() + assertThat(RecoveryState.INCOMPLETE.toAnalyticsUserPropertyValue()) + .isEqualTo(UserProperties.RecoveryState.Incomplete) + assertThat(RecoveryState.ENABLED.toAnalyticsUserPropertyValue()) + .isEqualTo(UserProperties.RecoveryState.Enabled) + assertThat(RecoveryState.DISABLED.toAnalyticsUserPropertyValue()) + .isEqualTo(UserProperties.RecoveryState.Disabled) + + assertThat(RecoveryState.UNKNOWN.toAnalyticsStateChangeValue()) + .isNull() + assertThat(RecoveryState.WAITING_FOR_SYNC.toAnalyticsStateChangeValue()) + .isNull() + assertThat(RecoveryState.INCOMPLETE.toAnalyticsStateChangeValue()) + .isEqualTo(CryptoSessionStateChange.RecoveryState.Incomplete) + assertThat(RecoveryState.ENABLED.toAnalyticsStateChangeValue()) + .isEqualTo(CryptoSessionStateChange.RecoveryState.Enabled) + assertThat(RecoveryState.DISABLED.toAnalyticsStateChangeValue()) + .isEqualTo(CryptoSessionStateChange.RecoveryState.Disabled) + } +} diff --git a/appnav/src/test/kotlin/io/element/android/appnav/loggedin/LoggedInPresenterTest.kt b/appnav/src/test/kotlin/io/element/android/appnav/loggedin/LoggedInPresenterTest.kt index d1b89fca77..2678c125e5 100644 --- a/appnav/src/test/kotlin/io/element/android/appnav/loggedin/LoggedInPresenterTest.kt +++ b/appnav/src/test/kotlin/io/element/android/appnav/loggedin/LoggedInPresenterTest.kt @@ -20,9 +20,13 @@ import app.cash.molecule.RecompositionMode import app.cash.molecule.moleculeFlow import app.cash.turbine.test import com.google.common.truth.Truth.assertThat +import im.vector.app.features.analytics.plan.CryptoSessionStateChange +import im.vector.app.features.analytics.plan.UserProperties import io.element.android.features.networkmonitor.api.NetworkStatus import io.element.android.features.networkmonitor.test.FakeNetworkMonitor +import io.element.android.libraries.matrix.api.encryption.RecoveryState import io.element.android.libraries.matrix.api.roomlist.RoomListService +import io.element.android.libraries.matrix.api.verification.SessionVerifiedStatus import io.element.android.libraries.matrix.test.FakeMatrixClient import io.element.android.libraries.matrix.test.encryption.FakeEncryptionService import io.element.android.libraries.matrix.test.roomlist.FakeRoomListService @@ -66,17 +70,53 @@ class LoggedInPresenterTest { } } + @Test + fun `present - report crypto status analytics`() = runTest { + val analyticsService = FakeAnalyticsService() + val verificationService = FakeSessionVerificationService() + val encryptionService = FakeEncryptionService() + val presenter = LoggedInPresenter( + matrixClient = FakeMatrixClient(encryptionService = encryptionService), + networkMonitor = FakeNetworkMonitor(NetworkStatus.Online), + pushService = FakePushService(), + sessionVerificationService = verificationService, + analyticsService = analyticsService, + encryptionService = encryptionService + ) + moleculeFlow(RecompositionMode.Immediate) { + presenter.present() + }.test { + encryptionService.emitRecoveryState(RecoveryState.UNKNOWN) + encryptionService.emitRecoveryState(RecoveryState.INCOMPLETE) + verificationService.emitVerifiedStatus(SessionVerifiedStatus.Verified) + + // Should only capture once (not report while checking state -like unknown-) + consumeItemsUntilPredicate { + analyticsService.capturedEvents.size == 1 && + analyticsService.capturedEvents[0] is CryptoSessionStateChange + } + consumeItemsUntilPredicate { + analyticsService.capturedUserProperties.size == 1 && + analyticsService.capturedUserProperties[0].recoveryState == UserProperties.RecoveryState.Incomplete && + analyticsService.capturedUserProperties[0].verificationState == UserProperties.VerificationState.Verified + } + cancelAndConsumeRemainingEvents() + } + } + private fun createLoggedInPresenter( roomListService: RoomListService = FakeRoomListService(), - networkStatus: NetworkStatus = NetworkStatus.Offline + networkStatus: NetworkStatus = NetworkStatus.Offline, + analyticsService: FakeAnalyticsService = FakeAnalyticsService(), + encryptionService: FakeEncryptionService = FakeEncryptionService(), ): LoggedInPresenter { return LoggedInPresenter( matrixClient = FakeMatrixClient(roomListService = roomListService), networkMonitor = FakeNetworkMonitor(networkStatus), pushService = FakePushService(), sessionVerificationService = FakeSessionVerificationService(), - analyticsService = FakeAnalyticsService(), - encryptionService = FakeEncryptionService() + analyticsService = analyticsService, + encryptionService = encryptionService ) } } diff --git a/libraries/matrix/test/src/main/kotlin/io/element/android/libraries/matrix/test/verification/FakeSessionVerificationService.kt b/libraries/matrix/test/src/main/kotlin/io/element/android/libraries/matrix/test/verification/FakeSessionVerificationService.kt index 64c7c8ab97..cde77d7c61 100644 --- a/libraries/matrix/test/src/main/kotlin/io/element/android/libraries/matrix/test/verification/FakeSessionVerificationService.kt +++ b/libraries/matrix/test/src/main/kotlin/io/element/android/libraries/matrix/test/verification/FakeSessionVerificationService.kt @@ -77,6 +77,10 @@ class FakeSessionVerificationService : SessionVerificationService { _sessionVerifiedStatus.value = status } + suspend fun emitVerifiedStatus(status: SessionVerifiedStatus) { + _sessionVerifiedStatus.emit(status) + } + fun givenVerificationFlowState(state: VerificationFlowState) { _verificationFlowState.value = state } diff --git a/services/analytics/test/src/main/kotlin/io/element/android/services/analytics/test/FakeAnalyticsService.kt b/services/analytics/test/src/main/kotlin/io/element/android/services/analytics/test/FakeAnalyticsService.kt index 14ef329ae4..2d9f320422 100644 --- a/services/analytics/test/src/main/kotlin/io/element/android/services/analytics/test/FakeAnalyticsService.kt +++ b/services/analytics/test/src/main/kotlin/io/element/android/services/analytics/test/FakeAnalyticsService.kt @@ -33,6 +33,7 @@ class FakeAnalyticsService( val capturedEvents = mutableListOf() val screenEvents = mutableListOf() val trackedErrors = mutableListOf() + val capturedUserProperties = mutableListOf() override fun getAvailableAnalyticsProviders(): Set = emptySet() @@ -65,6 +66,7 @@ class FakeAnalyticsService( } override fun updateUserProperties(userProperties: UserProperties) { + capturedUserProperties += userProperties } override fun trackError(throwable: Throwable) { diff --git a/services/analyticsproviders/posthog/src/main/kotlin/io/element/android/services/analyticsproviders/posthog/PosthogAnalyticsProvider.kt b/services/analyticsproviders/posthog/src/main/kotlin/io/element/android/services/analyticsproviders/posthog/PosthogAnalyticsProvider.kt index b594dcbee4..fb121aeb5e 100644 --- a/services/analyticsproviders/posthog/src/main/kotlin/io/element/android/services/analyticsproviders/posthog/PosthogAnalyticsProvider.kt +++ b/services/analyticsproviders/posthog/src/main/kotlin/io/element/android/services/analyticsproviders/posthog/PosthogAnalyticsProvider.kt @@ -87,7 +87,7 @@ class PosthogAnalyticsProvider @Inject constructor( userProperties.getProperties()?.let { pendingUserProperties?.putAll(it) } - // We are not currently using `identify` in EIX, if it was the case + // We are not currently using `identify` in EAX, if it was the case // we could have called identify to update the user properties. // For now, we have to store them, and they will be updated when the next call // to capture will happen. From ca9937674c7001e001888396bfece64e1e8e73da Mon Sep 17 00:00:00 2001 From: Valere Date: Tue, 7 May 2024 09:40:37 +0200 Subject: [PATCH 3/4] Add change log --- changelog.d/2806.misc | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/2806.misc diff --git a/changelog.d/2806.misc b/changelog.d/2806.misc new file mode 100644 index 0000000000..5f6f53667b --- /dev/null +++ b/changelog.d/2806.misc @@ -0,0 +1 @@ +Analytics: Add support to report current session verification and recovery state From 036c73c31bdaff2a74b51cf86f7e77864230a1d0 Mon Sep 17 00:00:00 2001 From: Valere Date: Tue, 7 May 2024 10:41:08 +0200 Subject: [PATCH 4/4] Use LaunchedEffect and improve test --- .../appnav/loggedin/LoggedInPresenter.kt | 4 ++- .../appnav/loggedin/LoggedInPresenterTest.kt | 27 ++++++++++--------- 2 files changed, 18 insertions(+), 13 deletions(-) diff --git a/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInPresenter.kt b/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInPresenter.kt index 0feaf47029..313f4aafe0 100644 --- a/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInPresenter.kt +++ b/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInPresenter.kt @@ -71,7 +71,9 @@ class LoggedInPresenter @Inject constructor( } val verificationState by sessionVerificationService.sessionVerifiedStatus.collectAsState() val recoveryState by encryptionService.recoveryStateStateFlow.collectAsState() - reportCryptoStatusToAnalytics(verificationState, recoveryState) + LaunchedEffect(verificationState, recoveryState) { + reportCryptoStatusToAnalytics(verificationState, recoveryState) + } return LoggedInState( showSyncSpinner = showSyncSpinner, diff --git a/appnav/src/test/kotlin/io/element/android/appnav/loggedin/LoggedInPresenterTest.kt b/appnav/src/test/kotlin/io/element/android/appnav/loggedin/LoggedInPresenterTest.kt index 2678c125e5..f57f648599 100644 --- a/appnav/src/test/kotlin/io/element/android/appnav/loggedin/LoggedInPresenterTest.kt +++ b/appnav/src/test/kotlin/io/element/android/appnav/loggedin/LoggedInPresenterTest.kt @@ -73,10 +73,11 @@ class LoggedInPresenterTest { @Test fun `present - report crypto status analytics`() = runTest { val analyticsService = FakeAnalyticsService() + val roomListService = FakeRoomListService() val verificationService = FakeSessionVerificationService() val encryptionService = FakeEncryptionService() val presenter = LoggedInPresenter( - matrixClient = FakeMatrixClient(encryptionService = encryptionService), + matrixClient = FakeMatrixClient(roomListService = roomListService, encryptionService = encryptionService), networkMonitor = FakeNetworkMonitor(NetworkStatus.Online), pushService = FakePushService(), sessionVerificationService = verificationService, @@ -90,17 +91,19 @@ class LoggedInPresenterTest { encryptionService.emitRecoveryState(RecoveryState.INCOMPLETE) verificationService.emitVerifiedStatus(SessionVerifiedStatus.Verified) - // Should only capture once (not report while checking state -like unknown-) - consumeItemsUntilPredicate { - analyticsService.capturedEvents.size == 1 && - analyticsService.capturedEvents[0] is CryptoSessionStateChange - } - consumeItemsUntilPredicate { - analyticsService.capturedUserProperties.size == 1 && - analyticsService.capturedUserProperties[0].recoveryState == UserProperties.RecoveryState.Incomplete && - analyticsService.capturedUserProperties[0].verificationState == UserProperties.VerificationState.Verified - } - cancelAndConsumeRemainingEvents() + skipItems(4) + + assertThat(analyticsService.capturedEvents.size).isEqualTo(1) + assertThat(analyticsService.capturedEvents[0]).isInstanceOf(CryptoSessionStateChange::class.java) + + assertThat(analyticsService.capturedUserProperties.size).isEqualTo(1) + assertThat(analyticsService.capturedUserProperties[0].recoveryState).isEqualTo(UserProperties.RecoveryState.Incomplete) + assertThat(analyticsService.capturedUserProperties[0].verificationState).isEqualTo(UserProperties.VerificationState.Verified) + + // ensure a sync status change does not trigger a new capture + roomListService.postSyncIndicator(RoomListService.SyncIndicator.Show) + skipItems(1) + assertThat(analyticsService.capturedEvents.size).isEqualTo(1) } }