From ec4f2dcfbfa14dd73dc57dfde350c389a7e14249 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Wed, 4 Sep 2024 16:28:35 +0200 Subject: [PATCH] Properly skip the FTUE verification screen if verification is not needed. --- .../FtueSessionVerificationFlowNode.kt | 7 +- .../api/VerifySessionEntryPoint.kt | 4 + .../impl/DefaultVerifySessionEntryPoint.kt | 5 + .../impl/VerifySelfSessionNode.kt | 7 +- .../impl/VerifySelfSessionPresenter.kt | 41 +++++++-- .../impl/VerifySelfSessionState.kt | 1 + .../impl/VerifySelfSessionStateProvider.kt | 6 ++ .../impl/VerifySelfSessionView.kt | 91 +++++++++++-------- .../impl/VerifySelfSessionPresenterTest.kt | 29 +++++- 9 files changed, 139 insertions(+), 52 deletions(-) diff --git a/features/ftue/impl/src/main/kotlin/io/element/android/features/ftue/impl/sessionverification/FtueSessionVerificationFlowNode.kt b/features/ftue/impl/src/main/kotlin/io/element/android/features/ftue/impl/sessionverification/FtueSessionVerificationFlowNode.kt index 3e4ed218a5..694819a2dc 100644 --- a/features/ftue/impl/src/main/kotlin/io/element/android/features/ftue/impl/sessionverification/FtueSessionVerificationFlowNode.kt +++ b/features/ftue/impl/src/main/kotlin/io/element/android/features/ftue/impl/sessionverification/FtueSessionVerificationFlowNode.kt @@ -46,7 +46,7 @@ class FtueSessionVerificationFlowNode @AssistedInject constructor( private val secureBackupEntryPoint: SecureBackupEntryPoint, ) : BaseFlowNode( backstack = BackStack( - initialElement = NavTarget.Root, + initialElement = NavTarget.Root(showDeviceVerifiedScreen = false), savedStateMap = buildContext.savedStateMap, ), buildContext = buildContext, @@ -54,7 +54,7 @@ class FtueSessionVerificationFlowNode @AssistedInject constructor( ) { sealed interface NavTarget : Parcelable { @Parcelize - data object Root : NavTarget + data class Root(val showDeviceVerifiedScreen: Boolean) : NavTarget @Parcelize data object EnterRecoveryKey : NavTarget @@ -71,7 +71,7 @@ class FtueSessionVerificationFlowNode @AssistedInject constructor( override fun onDone() { lifecycleScope.launch { // Move to the completed state view in the verification flow - backstack.newRoot(NavTarget.Root) + backstack.newRoot(NavTarget.Root(showDeviceVerifiedScreen = true)) } } } @@ -80,6 +80,7 @@ class FtueSessionVerificationFlowNode @AssistedInject constructor( return when (navTarget) { is NavTarget.Root -> { verifySessionEntryPoint.nodeBuilder(this, buildContext) + .params(VerifySessionEntryPoint.Params(navTarget.showDeviceVerifiedScreen)) .callback(object : VerifySessionEntryPoint.Callback { override fun onEnterRecoveryKey() { backstack.push(NavTarget.EnterRecoveryKey) diff --git a/features/verifysession/api/src/main/kotlin/io/element/android/features/verifysession/api/VerifySessionEntryPoint.kt b/features/verifysession/api/src/main/kotlin/io/element/android/features/verifysession/api/VerifySessionEntryPoint.kt index deb5cdf267..46ec8647b6 100644 --- a/features/verifysession/api/src/main/kotlin/io/element/android/features/verifysession/api/VerifySessionEntryPoint.kt +++ b/features/verifysession/api/src/main/kotlin/io/element/android/features/verifysession/api/VerifySessionEntryPoint.kt @@ -20,12 +20,16 @@ import com.bumble.appyx.core.modality.BuildContext import com.bumble.appyx.core.node.Node import com.bumble.appyx.core.plugin.Plugin import io.element.android.libraries.architecture.FeatureEntryPoint +import io.element.android.libraries.architecture.NodeInputs interface VerifySessionEntryPoint : FeatureEntryPoint { + data class Params(val showDeviceVerifiedScreen: Boolean) : NodeInputs + fun nodeBuilder(parentNode: Node, buildContext: BuildContext): NodeBuilder interface NodeBuilder { fun callback(callback: Callback): NodeBuilder + fun params(params: Params): NodeBuilder fun build(): Node } diff --git a/features/verifysession/impl/src/main/kotlin/io/element/android/features/verifysession/impl/DefaultVerifySessionEntryPoint.kt b/features/verifysession/impl/src/main/kotlin/io/element/android/features/verifysession/impl/DefaultVerifySessionEntryPoint.kt index b514742a87..ce9136c760 100644 --- a/features/verifysession/impl/src/main/kotlin/io/element/android/features/verifysession/impl/DefaultVerifySessionEntryPoint.kt +++ b/features/verifysession/impl/src/main/kotlin/io/element/android/features/verifysession/impl/DefaultVerifySessionEntryPoint.kt @@ -36,6 +36,11 @@ class DefaultVerifySessionEntryPoint @Inject constructor() : VerifySessionEntryP return this } + override fun params(params: VerifySessionEntryPoint.Params): VerifySessionEntryPoint.NodeBuilder { + plugins += params + return this + } + override fun build(): Node { return parentNode.createNode(buildContext, plugins) } diff --git a/features/verifysession/impl/src/main/kotlin/io/element/android/features/verifysession/impl/VerifySelfSessionNode.kt b/features/verifysession/impl/src/main/kotlin/io/element/android/features/verifysession/impl/VerifySelfSessionNode.kt index 72640d3dbc..6ca8ecf261 100644 --- a/features/verifysession/impl/src/main/kotlin/io/element/android/features/verifysession/impl/VerifySelfSessionNode.kt +++ b/features/verifysession/impl/src/main/kotlin/io/element/android/features/verifysession/impl/VerifySelfSessionNode.kt @@ -30,16 +30,21 @@ import io.element.android.anvilannotations.ContributesNode import io.element.android.compound.theme.ElementTheme import io.element.android.features.logout.api.util.onSuccessLogout import io.element.android.features.verifysession.api.VerifySessionEntryPoint +import io.element.android.libraries.architecture.inputs import io.element.android.libraries.di.SessionScope @ContributesNode(SessionScope::class) class VerifySelfSessionNode @AssistedInject constructor( @Assisted buildContext: BuildContext, @Assisted plugins: List, - private val presenter: VerifySelfSessionPresenter, + presenterFactory: VerifySelfSessionPresenter.Factory, ) : Node(buildContext, plugins = plugins) { private val callback = plugins().first() + private val presenter = presenterFactory.create( + showDeviceVerifiedScreen = inputs().showDeviceVerifiedScreen, + ) + @Composable override fun View(modifier: Modifier) { val state = presenter.present() diff --git a/features/verifysession/impl/src/main/kotlin/io/element/android/features/verifysession/impl/VerifySelfSessionPresenter.kt b/features/verifysession/impl/src/main/kotlin/io/element/android/features/verifysession/impl/VerifySelfSessionPresenter.kt index 03da02e4b3..54c21e67ee 100644 --- a/features/verifysession/impl/src/main/kotlin/io/element/android/features/verifysession/impl/VerifySelfSessionPresenter.kt +++ b/features/verifysession/impl/src/main/kotlin/io/element/android/features/verifysession/impl/VerifySelfSessionPresenter.kt @@ -28,6 +28,9 @@ import androidx.compose.runtime.mutableStateOf import androidx.compose.runtime.remember import androidx.compose.runtime.rememberCoroutineScope import com.freeletics.flowredux.compose.rememberStateAndDispatch +import dagger.assisted.Assisted +import dagger.assisted.AssistedFactory +import dagger.assisted.AssistedInject import io.element.android.features.logout.api.LogoutUseCase import io.element.android.libraries.architecture.AsyncAction import io.element.android.libraries.architecture.AsyncData @@ -37,6 +40,7 @@ import io.element.android.libraries.core.meta.BuildMeta 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.verification.SessionVerificationService +import io.element.android.libraries.matrix.api.verification.SessionVerifiedStatus import io.element.android.libraries.matrix.api.verification.VerificationFlowState import io.element.android.libraries.preferences.api.store.SessionPreferencesStore import kotlinx.coroutines.CoroutineScope @@ -44,11 +48,11 @@ import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.flow.launchIn import kotlinx.coroutines.flow.onEach import kotlinx.coroutines.launch -import javax.inject.Inject import io.element.android.features.verifysession.impl.VerifySelfSessionStateMachine.Event as StateMachineEvent import io.element.android.features.verifysession.impl.VerifySelfSessionStateMachine.State as StateMachineState -class VerifySelfSessionPresenter @Inject constructor( +class VerifySelfSessionPresenter @AssistedInject constructor( + @Assisted private val showDeviceVerifiedScreen: Boolean, private val sessionVerificationService: SessionVerificationService, private val encryptionService: EncryptionService, private val stateMachine: VerifySelfSessionStateMachine, @@ -56,6 +60,11 @@ class VerifySelfSessionPresenter @Inject constructor( private val sessionPreferencesStore: SessionPreferencesStore, private val logoutUseCase: LogoutUseCase, ) : Presenter { + @AssistedFactory + interface Factory { + fun create(showDeviceVerifiedScreen: Boolean): VerifySelfSessionPresenter + } + @Composable override fun present(): VerifySelfSessionState { val coroutineScope = rememberCoroutineScope() @@ -66,18 +75,32 @@ class VerifySelfSessionPresenter @Inject constructor( val recoveryState by encryptionService.recoveryStateStateFlow.collectAsState() val stateAndDispatch = stateMachine.rememberStateAndDispatch() val skipVerification by sessionPreferencesStore.isSessionVerificationSkipped().collectAsState(initial = false) - val needsVerification by sessionVerificationService.needsSessionVerification.collectAsState(initial = true) + val sessionVerifiedStatus by sessionVerificationService.sessionVerifiedStatus.collectAsState() val signOutAction = remember { mutableStateOf>(AsyncAction.Uninitialized) } val verificationFlowStep by remember { derivedStateOf { - when { - skipVerification -> VerifySelfSessionState.VerificationStep.Skipped - needsVerification -> stateAndDispatch.state.value.toVerificationStep( - canEnterRecoveryKey = recoveryState == RecoveryState.INCOMPLETE - ) - else -> VerifySelfSessionState.VerificationStep.Completed + if (skipVerification) { + VerifySelfSessionState.VerificationStep.Skipped + } else { + when (sessionVerifiedStatus) { + SessionVerifiedStatus.Unknown -> VerifySelfSessionState.VerificationStep.Loading + SessionVerifiedStatus.NotVerified -> { + stateAndDispatch.state.value.toVerificationStep( + canEnterRecoveryKey = recoveryState == RecoveryState.INCOMPLETE + ) + } + SessionVerifiedStatus.Verified -> { + if (stateAndDispatch.state.value != StateMachineState.Initial || showDeviceVerifiedScreen) { + // The user has verified the session, we need to show the success screen + VerifySelfSessionState.VerificationStep.Completed + } else { + // Automatic verification, which can happen on freshly created account, in this case, skip the screen + VerifySelfSessionState.VerificationStep.Skipped + } + } + } } } } diff --git a/features/verifysession/impl/src/main/kotlin/io/element/android/features/verifysession/impl/VerifySelfSessionState.kt b/features/verifysession/impl/src/main/kotlin/io/element/android/features/verifysession/impl/VerifySelfSessionState.kt index aca96eea1c..2fbef1d8ef 100644 --- a/features/verifysession/impl/src/main/kotlin/io/element/android/features/verifysession/impl/VerifySelfSessionState.kt +++ b/features/verifysession/impl/src/main/kotlin/io/element/android/features/verifysession/impl/VerifySelfSessionState.kt @@ -31,6 +31,7 @@ data class VerifySelfSessionState( ) { @Stable sealed interface VerificationStep { + data object Loading : VerificationStep data class Initial(val canEnterRecoveryKey: Boolean, val isLastDevice: Boolean = false) : VerificationStep data object Canceled : VerificationStep data object AwaitingOtherDeviceResponse : VerificationStep diff --git a/features/verifysession/impl/src/main/kotlin/io/element/android/features/verifysession/impl/VerifySelfSessionStateProvider.kt b/features/verifysession/impl/src/main/kotlin/io/element/android/features/verifysession/impl/VerifySelfSessionStateProvider.kt index 379d9b1686..8767fbfbc7 100644 --- a/features/verifysession/impl/src/main/kotlin/io/element/android/features/verifysession/impl/VerifySelfSessionStateProvider.kt +++ b/features/verifysession/impl/src/main/kotlin/io/element/android/features/verifysession/impl/VerifySelfSessionStateProvider.kt @@ -59,6 +59,12 @@ open class VerifySelfSessionStateProvider : PreviewParameterProvider error("Should not happen") is FlowStep.Initial, FlowStep.AwaitingOtherDeviceResponse -> BigIcon.Style.Default(CompoundIcons.LockSolid()) FlowStep.Canceled -> BigIcon.Style.AlertSolid FlowStep.Ready, is FlowStep.Verifying -> BigIcon.Style.Default(CompoundIcons.Reaction()) @@ -164,6 +180,7 @@ private fun HeaderContent(verificationFlowStep: FlowStep) { is FlowStep.Skipped -> return } val titleTextId = when (verificationFlowStep) { + VerifySelfSessionState.VerificationStep.Loading -> error("Should not happen") is FlowStep.Initial, FlowStep.AwaitingOtherDeviceResponse -> R.string.screen_identity_confirmation_title FlowStep.Canceled -> CommonStrings.common_verification_cancelled FlowStep.Ready -> R.string.screen_session_verification_compare_emojis_title @@ -175,6 +192,7 @@ private fun HeaderContent(verificationFlowStep: FlowStep) { is FlowStep.Skipped -> return } val subtitleTextId = when (verificationFlowStep) { + VerifySelfSessionState.VerificationStep.Loading -> error("Should not happen") is FlowStep.Initial, FlowStep.AwaitingOtherDeviceResponse -> R.string.screen_identity_confirmation_subtitle FlowStep.Canceled -> R.string.screen_session_verification_cancelled_subtitle FlowStep.Ready -> R.string.screen_session_verification_ready_subtitle @@ -268,6 +286,7 @@ private fun BottomMenu( val isVerifying = (verificationViewState as? FlowStep.Verifying)?.state is AsyncData.Loading when (verificationViewState) { + VerifySelfSessionState.VerificationStep.Loading -> error("Should not happen") is FlowStep.Initial -> { BottomMenu { if (verificationViewState.isLastDevice) { diff --git a/features/verifysession/impl/src/test/kotlin/io/element/android/features/verifysession/impl/VerifySelfSessionPresenterTest.kt b/features/verifysession/impl/src/test/kotlin/io/element/android/features/verifysession/impl/VerifySelfSessionPresenterTest.kt index c258b9df03..ca8836ac61 100644 --- a/features/verifysession/impl/src/test/kotlin/io/element/android/features/verifysession/impl/VerifySelfSessionPresenterTest.kt +++ b/features/verifysession/impl/src/test/kotlin/io/element/android/features/verifysession/impl/VerifySelfSessionPresenterTest.kt @@ -298,18 +298,39 @@ class VerifySelfSessionPresenterTest { } @Test - fun `present - When verification is not needed, the flow is completed`() = runTest { + fun `present - When verification is done using recovery key, the flow is completed`() = runTest { val service = FakeSessionVerificationService().apply { givenNeedsSessionVerification(false) givenVerifiedStatus(SessionVerifiedStatus.Verified) givenVerificationFlowState(VerificationFlowState.Finished) } - val presenter = createVerifySelfSessionPresenter(service) + val presenter = createVerifySelfSessionPresenter( + service = service, + showDeviceVerifiedScreen = true, + ) + moleculeFlow(RecompositionMode.Immediate) { + presenter.present() + }.test { + assertThat(awaitItem().verificationFlowStep).isEqualTo(VerificationStep.Completed) + } + } + + @Test + fun `present - When verification is not needed, the flow is skipped`() = runTest { + val service = FakeSessionVerificationService().apply { + givenNeedsSessionVerification(false) + givenVerifiedStatus(SessionVerifiedStatus.Verified) + givenVerificationFlowState(VerificationFlowState.Finished) + } + val presenter = createVerifySelfSessionPresenter( + service = service, + showDeviceVerifiedScreen = false, + ) moleculeFlow(RecompositionMode.Immediate) { presenter.present() }.test { skipItems(1) - assertThat(awaitItem().verificationFlowStep).isEqualTo(VerificationStep.Completed) + assertThat(awaitItem().verificationFlowStep).isEqualTo(VerificationStep.Skipped) } } @@ -374,8 +395,10 @@ class VerifySelfSessionPresenterTest { buildMeta: BuildMeta = aBuildMeta(), sessionPreferencesStore: InMemorySessionPreferencesStore = InMemorySessionPreferencesStore(), logoutUseCase: LogoutUseCase = FakeLogoutUseCase(), + showDeviceVerifiedScreen: Boolean = false, ): VerifySelfSessionPresenter { return VerifySelfSessionPresenter( + showDeviceVerifiedScreen = showDeviceVerifiedScreen, sessionVerificationService = service, encryptionService = encryptionService, stateMachine = VerifySelfSessionStateMachine(service, encryptionService),