Fix verification failed issue, simplify verification logic (#3830)

* Simplify session verification: - Reuse Rust `Client` instances created on the login process so we don't need to restore one right before the session verification. - Remove unnecessary sources of verification state updates. - Add an intermediate FTUE flow step which will display an indeterminate progress indicator instead of a blank screen. * Remove unnecessary workaround: the SDK should already handle this * Add regression tests for noop analytics service usage. * Add `services.analytics.noop` module to the test dependencies --------- Co-authored-by: Benoit Marty <benoit@matrix.org>
2024-11-08 16:42:27 +01:00
parent 979c4faafe
commit 49e1cfed42
11 changed files with 176 additions and 113 deletions
--- a/features/ftue/impl/build.gradle.kts
+++ b/features/ftue/impl/build.gradle.kts
@@ -45,6 +45,7 @@ dependencies {
    testImplementation(libs.test.turbine)
    testImplementation(projects.libraries.matrix.test)
    testImplementation(projects.services.analytics.test)
+    testImplementation(projects.services.analytics.noop)
    testImplementation(projects.libraries.permissions.impl)
    testImplementation(projects.libraries.permissions.test)
    testImplementation(projects.libraries.preferences.test)
--- a/features/ftue/impl/src/main/kotlin/io/element/android/features/ftue/impl/FtueFlowNode.kt
+++ b/features/ftue/impl/src/main/kotlin/io/element/android/features/ftue/impl/FtueFlowNode.kt
@@ -8,7 +8,10 @@
 package io.element.android.features.ftue.impl

 import android.os.Parcelable
+import androidx.compose.foundation.layout.Box
+import androidx.compose.foundation.layout.fillMaxSize
 import androidx.compose.runtime.Composable
+import androidx.compose.ui.Alignment
 import androidx.compose.ui.Modifier
 import androidx.lifecycle.lifecycleScope
 import com.bumble.appyx.core.lifecycle.subscribe
@@ -31,12 +34,14 @@ import io.element.android.features.lockscreen.api.LockScreenEntryPoint
 import io.element.android.libraries.architecture.BackstackView
 import io.element.android.libraries.architecture.BaseFlowNode
 import io.element.android.libraries.architecture.createNode
+import io.element.android.libraries.designsystem.theme.components.CircularProgressIndicator
 import io.element.android.libraries.di.AppScope
 import io.element.android.libraries.di.SessionScope
 import io.element.android.services.analytics.api.AnalyticsService
 import kotlinx.coroutines.flow.MutableStateFlow
 import kotlinx.coroutines.flow.StateFlow
 import kotlinx.coroutines.flow.distinctUntilChanged
+import kotlinx.coroutines.flow.filter
 import kotlinx.coroutines.flow.launchIn
 import kotlinx.coroutines.flow.onEach
 import kotlinx.coroutines.launch
@@ -80,14 +85,17 @@ class FtueFlowNode @AssistedInject constructor(
        super.onBuilt()

        lifecycle.subscribe(onCreate = {
-            lifecycleScope.launch { moveToNextStep() }
+            moveToNextStepIfNeeded()
        })

        analyticsService.didAskUserConsent()
            .distinctUntilChanged()
-            .onEach {
-                lifecycleScope.launch { moveToNextStep() }
-            }
+            .onEach { moveToNextStepIfNeeded() }
+            .launchIn(lifecycleScope)
+
+        ftueState.isVerificationStatusKnown
+            .filter { it }
+            .onEach { moveToNextStepIfNeeded() }
            .launchIn(lifecycleScope)
    }

@@ -99,7 +107,7 @@ class FtueFlowNode @AssistedInject constructor(
            NavTarget.SessionVerification -> {
                val callback = object : FtueSessionVerificationFlowNode.Callback {
                    override fun onDone() {
-                        lifecycleScope.launch { moveToNextStep() }
+                        moveToNextStepIfNeeded()
                    }
                }
                createNode<FtueSessionVerificationFlowNode>(buildContext, listOf(callback))
@@ -107,7 +115,7 @@ class FtueFlowNode @AssistedInject constructor(
            NavTarget.NotificationsOptIn -> {
                val callback = object : NotificationsOptInNode.Callback {
                    override fun onNotificationsOptInFinished() {
-                        lifecycleScope.launch { moveToNextStep() }
+                        moveToNextStepIfNeeded()
                    }
                }
                createNode<NotificationsOptInNode>(buildContext, listOf(callback))
@@ -118,7 +126,7 @@ class FtueFlowNode @AssistedInject constructor(
            NavTarget.LockScreenSetup -> {
                val callback = object : LockScreenEntryPoint.Callback {
                    override fun onSetupDone() {
-                        lifecycleScope.launch { moveToNextStep() }
+                        moveToNextStepIfNeeded()
                    }
                }
                lockScreenEntryPoint.nodeBuilder(this, buildContext, LockScreenEntryPoint.Target.Setup)
@@ -128,8 +136,11 @@ class FtueFlowNode @AssistedInject constructor(
        }
    }

-    private fun moveToNextStep() = lifecycleScope.launch {
+    private fun moveToNextStepIfNeeded() = lifecycleScope.launch {
        when (ftueState.getNextStep()) {
+            FtueStep.WaitingForInitialState -> {
+                backstack.newRoot(NavTarget.Placeholder)
+            }
            FtueStep.SessionVerification -> {
                backstack.newRoot(NavTarget.SessionVerification)
            }
@@ -155,7 +166,14 @@ class FtueFlowNode @AssistedInject constructor(
    class PlaceholderNode @AssistedInject constructor(
        @Assisted buildContext: BuildContext,
        @Assisted plugins: List<Plugin>,
-    ) : Node(buildContext, plugins = plugins)
+    ) : Node(buildContext, plugins = plugins) {
+        @Composable
+        override fun View(modifier: Modifier) {
+            Box(modifier = modifier.fillMaxSize(), contentAlignment = Alignment.Center) {
+                CircularProgressIndicator()
+            }
+        }
+    }
 }

 private class NoOpBackstackHandlerStrategy<NavTarget : Any> : BaseBackPressHandlerStrategy<NavTarget, BackStack.State>() {
--- a/features/ftue/impl/src/main/kotlin/io/element/android/features/ftue/impl/state/DefaultFtueService.kt
+++ b/features/ftue/impl/src/main/kotlin/io/element/android/features/ftue/impl/state/DefaultFtueService.kt
@@ -24,18 +24,14 @@ import io.element.android.libraries.preferences.api.store.SessionPreferencesStor
 import io.element.android.services.analytics.api.AnalyticsService
 import io.element.android.services.toolbox.api.sdk.BuildVersionSdkIntProvider
 import kotlinx.coroutines.CoroutineScope
-import kotlinx.coroutines.FlowPreview
 import kotlinx.coroutines.flow.MutableStateFlow
-import kotlinx.coroutines.flow.catch
 import kotlinx.coroutines.flow.distinctUntilChanged
 import kotlinx.coroutines.flow.filter
 import kotlinx.coroutines.flow.first
 import kotlinx.coroutines.flow.launchIn
+import kotlinx.coroutines.flow.map
 import kotlinx.coroutines.flow.onEach
-import kotlinx.coroutines.flow.timeout
-import timber.log.Timber
 import javax.inject.Inject
-import kotlin.time.Duration.Companion.seconds

@ContributesBinding(SessionScope::class)
@SingleIn(SessionScope::class)
@@ -50,6 +46,14 @@ class DefaultFtueService @Inject constructor(
 ) : FtueService {
    override val state = MutableStateFlow<FtueState>(FtueState.Unknown)

+    /**
+     * This flow emits true when the FTUE flow is ready to be displayed.
+     * In this case, the FTUE flow is ready when the session verification status is known.
+     */
+    val isVerificationStatusKnown = sessionVerificationService.sessionVerifiedStatus
+        .map { it != SessionVerifiedStatus.Unknown }
+        .distinctUntilChanged()
+
    override suspend fun reset() {
        analyticsService.reset()
        if (sdkVersionProvider.isAtLeast(Build.VERSION_CODES.TIRAMISU)) {
@@ -70,7 +74,12 @@ class DefaultFtueService @Inject constructor(

    suspend fun getNextStep(currentStep: FtueStep? = null): FtueStep? =
        when (currentStep) {
-            null -> if (isSessionNotVerified()) {
+            null -> if (!isSessionVerificationStateReady()) {
+                FtueStep.WaitingForInitialState
+            } else {
+                getNextStep(FtueStep.WaitingForInitialState)
+            }
+            FtueStep.WaitingForInitialState -> if (isSessionNotVerified()) {
                FtueStep.SessionVerification
            } else {
                getNextStep(FtueStep.SessionVerification)
@@ -90,34 +99,18 @@ class DefaultFtueService @Inject constructor(
            } else {
                getNextStep(FtueStep.AnalyticsOptIn)
            }
-            FtueStep.AnalyticsOptIn -> {
-                updateState()
-                null
-            }
+            FtueStep.AnalyticsOptIn -> null
        }

-    private suspend fun isAnyStepIncomplete(): Boolean {
-        return listOf<suspend () -> Boolean>(
-            { isSessionNotVerified() },
-            { shouldAskNotificationPermissions() },
-            { needsAnalyticsOptIn() },
-            { shouldDisplayLockscreenSetup() },
-        ).any { it() }
+    private fun isSessionVerificationStateReady(): Boolean {
+        return sessionVerificationService.sessionVerifiedStatus.value != SessionVerifiedStatus.Unknown
    }

-    @OptIn(FlowPreview::class)
    private suspend fun isSessionNotVerified(): Boolean {
-        // Wait for the first known (or ready) verification status
-        val readyVerifiedSessionStatus = sessionVerificationService.sessionVerifiedStatus
-            .filter { it != SessionVerifiedStatus.Unknown }
-            // This is not ideal, but there are some very rare cases when reading the flow seems to get stuck
-            .timeout(5.seconds)
-            .catch {
-                Timber.e(it, "Failed to get session verification status, assume it's not verified")
-                emit(SessionVerifiedStatus.NotVerified)
-            }
-            .first()
-        return readyVerifiedSessionStatus == SessionVerifiedStatus.NotVerified && !canSkipVerification()
+        // Wait until the session verification status is known
+        isVerificationStatusKnown.filter { it }.first()
+
+        return sessionVerificationService.sessionVerifiedStatus.value == SessionVerifiedStatus.NotVerified && !canSkipVerification()
    }

    private suspend fun canSkipVerification(): Boolean {
@@ -145,14 +138,17 @@ class DefaultFtueService @Inject constructor(

    @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)
    internal suspend fun updateState() {
+        val nextStep = getNextStep()
        state.value = when {
-            isAnyStepIncomplete() -> FtueState.Incomplete
-            else -> FtueState.Complete
+            // Final state, there aren't any more next steps
+            nextStep == null -> FtueState.Complete
+            else -> FtueState.Incomplete
        }
    }
 }

 sealed interface FtueStep {
+    data object WaitingForInitialState : FtueStep
    data object SessionVerification : FtueStep
    data object NotificationsOptIn : FtueStep
    data object AnalyticsOptIn : FtueStep
--- a/features/ftue/impl/src/test/kotlin/io/element/android/features/ftue/impl/DefaultFtueServiceTest.kt
+++ b/features/ftue/impl/src/test/kotlin/io/element/android/features/ftue/impl/DefaultFtueServiceTest.kt
@@ -23,6 +23,7 @@ import io.element.android.libraries.permissions.impl.FakePermissionStateProvider
 import io.element.android.libraries.preferences.api.store.SessionPreferencesStore
 import io.element.android.libraries.preferences.test.InMemorySessionPreferencesStore
 import io.element.android.services.analytics.api.AnalyticsService
+import io.element.android.services.analytics.noop.NoopAnalyticsService
 import io.element.android.services.analytics.test.FakeAnalyticsService
 import io.element.android.services.toolbox.test.sdk.FakeBuildVersionSdkIntProvider
 import io.element.android.tests.testutils.lambda.lambdaRecorder
@@ -73,6 +74,27 @@ class DefaultFtueServiceTest {
        assertThat(service.state.value).isEqualTo(FtueState.Complete)
    }

+    @Test
+    fun `given all checks being true with no analytics, FtueState is Complete`() = runTest {
+        val analyticsService = NoopAnalyticsService()
+        val sessionVerificationService = FakeSessionVerificationService()
+        val permissionStateProvider = FakePermissionStateProvider(permissionGranted = true)
+        val lockScreenService = FakeLockScreenService()
+        val service = createDefaultFtueService(
+            sessionVerificationService = sessionVerificationService,
+            analyticsService = analyticsService,
+            permissionStateProvider = permissionStateProvider,
+            lockScreenService = lockScreenService,
+        )
+
+        sessionVerificationService.emitVerifiedStatus(SessionVerifiedStatus.Verified)
+        permissionStateProvider.setPermissionGranted()
+        lockScreenService.setIsPinSetup(true)
+        service.updateState()
+
+        assertThat(service.state.value).isEqualTo(FtueState.Complete)
+    }
+
    @Test
    fun `traverse flow`() = runTest {
        val sessionVerificationService = FakeSessionVerificationService().apply {