From 36d5500a650d7e8bf7fcc686ec83bcfb97e02740 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Wed, 28 Jan 2026 16:09:31 +0100 Subject: [PATCH] Iterate on login error: add a cancel button that fully close the flow. tom --- .../impl/LinkNewDeviceFlowNode.kt | 9 +++++- .../impl/screens/error/ErrorNode.kt | 2 ++ .../impl/screens/error/ErrorView.kt | 26 +++++++++++++---- .../impl/screens/error/ErrorViewTest.kt | 23 +++++++++++---- .../features/login/impl/LoginFlowNode.kt | 8 +++++- .../login/impl/qrcode/QrCodeLoginFlowNode.kt | 16 +++++++++-- .../screens/qrcode/error/QrCodeErrorNode.kt | 2 ++ .../screens/qrcode/error/QrCodeErrorView.kt | 28 +++++++++++++++---- .../impl/qrcode/QrCodeLoginFlowNodeTest.kt | 7 ++++- .../qrcode/error/QrCodeErrorViewTest.kt | 27 +++++++++++++----- 10 files changed, 120 insertions(+), 28 deletions(-) diff --git a/features/linknewdevice/impl/src/main/kotlin/io/element/android/features/linknewdevice/impl/LinkNewDeviceFlowNode.kt b/features/linknewdevice/impl/src/main/kotlin/io/element/android/features/linknewdevice/impl/LinkNewDeviceFlowNode.kt index 23c6b6ab2d..52e93d4992 100644 --- a/features/linknewdevice/impl/src/main/kotlin/io/element/android/features/linknewdevice/impl/LinkNewDeviceFlowNode.kt +++ b/features/linknewdevice/impl/src/main/kotlin/io/element/android/features/linknewdevice/impl/LinkNewDeviceFlowNode.kt @@ -193,7 +193,8 @@ class LinkNewDeviceFlowNode( is ErrorType.Unknown -> ErrorScreenType.UnknownError is ErrorType.UnsupportedProtocol -> ErrorScreenType.UnknownError } - // It is OK to push on backstack, since when user leaves the error screen, a new root will be set + // It is OK to push on backstack, since when user leaves the error screen, a new root will be set, + // or the whole flow will be popped. backstack.push(NavTarget.Error(error)) } @@ -263,6 +264,12 @@ class LinkNewDeviceFlowNode( linkNewDesktopHandler.reset() backstack.newRoot(NavTarget.Root) } + + override fun onCancel() { + linkNewMobileHandler.reset() + linkNewDesktopHandler.reset() + callback.onDone() + } } createNode(buildContext, listOf(callback, navTarget.errorScreenType)) } diff --git a/features/linknewdevice/impl/src/main/kotlin/io/element/android/features/linknewdevice/impl/screens/error/ErrorNode.kt b/features/linknewdevice/impl/src/main/kotlin/io/element/android/features/linknewdevice/impl/screens/error/ErrorNode.kt index 70fd3b49a4..e9ad9b4bc2 100644 --- a/features/linknewdevice/impl/src/main/kotlin/io/element/android/features/linknewdevice/impl/screens/error/ErrorNode.kt +++ b/features/linknewdevice/impl/src/main/kotlin/io/element/android/features/linknewdevice/impl/screens/error/ErrorNode.kt @@ -27,6 +27,7 @@ class ErrorNode( ) : Node(buildContext = buildContext, plugins = plugins) { interface Callback : Plugin { fun onRetry() + fun onCancel() } private val callback: Callback = callback() @@ -38,6 +39,7 @@ class ErrorNode( modifier = modifier, errorScreenType = errorScreenType, onRetry = callback::onRetry, + onCancel = callback::onCancel, ) } } diff --git a/features/linknewdevice/impl/src/main/kotlin/io/element/android/features/linknewdevice/impl/screens/error/ErrorView.kt b/features/linknewdevice/impl/src/main/kotlin/io/element/android/features/linknewdevice/impl/screens/error/ErrorView.kt index 3a77f19f49..9f67e8bc17 100644 --- a/features/linknewdevice/impl/src/main/kotlin/io/element/android/features/linknewdevice/impl/screens/error/ErrorView.kt +++ b/features/linknewdevice/impl/src/main/kotlin/io/element/android/features/linknewdevice/impl/screens/error/ErrorView.kt @@ -33,6 +33,7 @@ import io.element.android.libraries.designsystem.preview.ElementPreview import io.element.android.libraries.designsystem.preview.PreviewsDayNight import io.element.android.libraries.designsystem.theme.LocalBuildMeta import io.element.android.libraries.designsystem.theme.components.Button +import io.element.android.libraries.designsystem.theme.components.OutlinedButton import io.element.android.libraries.designsystem.theme.components.Text import io.element.android.libraries.ui.strings.CommonStrings import kotlinx.collections.immutable.persistentListOf @@ -41,17 +42,23 @@ import kotlinx.collections.immutable.persistentListOf fun ErrorView( errorScreenType: ErrorScreenType, onRetry: () -> Unit, + onCancel: () -> Unit, modifier: Modifier = Modifier, ) { val appName = LocalBuildMeta.current.applicationName - BackHandler(onBack = onRetry) + BackHandler(onBack = onCancel) FlowStepPage( modifier = modifier, iconStyle = BigIcon.Style.AlertSolid, title = titleText(errorScreenType, appName), subTitle = subtitleText(errorScreenType, appName), content = { Content(errorScreenType) }, - buttons = { Buttons(onRetry) }, + buttons = { + Buttons( + onRetry = onRetry, + onCancel = onCancel, + ) + }, ) } @@ -118,11 +125,19 @@ private fun Content(errorScreenType: ErrorScreenType) { } @Composable -private fun Buttons(onRetry: () -> Unit) { +private fun Buttons( + onRetry: () -> Unit, + onCancel: () -> Unit, +) { Button( modifier = Modifier.fillMaxWidth(), - text = stringResource(CommonStrings.action_start_over), - onClick = onRetry + text = stringResource(CommonStrings.action_try_again), + onClick = onRetry, + ) + OutlinedButton( + modifier = Modifier.fillMaxWidth(), + text = stringResource(CommonStrings.action_cancel), + onClick = onCancel, ) } @@ -133,6 +148,7 @@ internal fun ErrorViewPreview(@PreviewParameter(ErrorScreenTypeProvider::class) ErrorView( errorScreenType = errorScreenType, onRetry = {}, + onCancel = {}, ) } } diff --git a/features/linknewdevice/impl/src/test/kotlin/io/element/android/features/linknewdevice/impl/screens/error/ErrorViewTest.kt b/features/linknewdevice/impl/src/test/kotlin/io/element/android/features/linknewdevice/impl/screens/error/ErrorViewTest.kt index 8f44182dd4..aa52a70149 100644 --- a/features/linknewdevice/impl/src/test/kotlin/io/element/android/features/linknewdevice/impl/screens/error/ErrorViewTest.kt +++ b/features/linknewdevice/impl/src/test/kotlin/io/element/android/features/linknewdevice/impl/screens/error/ErrorViewTest.kt @@ -12,6 +12,7 @@ import androidx.compose.ui.test.junit4.AndroidComposeTestRule import androidx.compose.ui.test.junit4.createAndroidComposeRule import androidx.test.ext.junit.runners.AndroidJUnit4 import io.element.android.libraries.ui.strings.CommonStrings +import io.element.android.tests.testutils.EnsureNeverCalled import io.element.android.tests.testutils.clickOn import io.element.android.tests.testutils.ensureCalledOnce import io.element.android.tests.testutils.pressBackKey @@ -26,33 +27,45 @@ class ErrorViewTest { val rule = createAndroidComposeRule() @Test - fun `on back pressed - calls the onRetry callback`() { + fun `on back pressed - calls the onCancel callback`() { ensureCalledOnce { callback -> rule.setErrorView( - onRetry = callback + onCancel = callback, ) rule.pressBackKey() } } @Test - fun `on start over button clicked - calls the expected callback`() { + fun `on try again button clicked - calls the expected callback`() { ensureCalledOnce { callback -> rule.setErrorView( onRetry = callback ) - rule.clickOn(CommonStrings.action_start_over) + rule.clickOn(CommonStrings.action_try_again) + } + } + + @Test + fun `on cancel button clicked - calls the expected callback`() { + ensureCalledOnce { callback -> + rule.setErrorView( + onCancel = callback + ) + rule.clickOn(CommonStrings.action_cancel) } } private fun AndroidComposeTestRule.setErrorView( - onRetry: () -> Unit, + onRetry: () -> Unit = EnsureNeverCalled(), + onCancel: () -> Unit = EnsureNeverCalled(), errorScreenType: ErrorScreenType = ErrorScreenType.UnknownError, ) { setContent { ErrorView( errorScreenType = errorScreenType, onRetry = onRetry, + onCancel = onCancel, ) } } diff --git a/features/login/impl/src/main/kotlin/io/element/android/features/login/impl/LoginFlowNode.kt b/features/login/impl/src/main/kotlin/io/element/android/features/login/impl/LoginFlowNode.kt index a19bb12d86..928d98c244 100644 --- a/features/login/impl/src/main/kotlin/io/element/android/features/login/impl/LoginFlowNode.kt +++ b/features/login/impl/src/main/kotlin/io/element/android/features/login/impl/LoginFlowNode.kt @@ -20,6 +20,7 @@ import com.bumble.appyx.core.modality.BuildContext import com.bumble.appyx.core.node.Node import com.bumble.appyx.core.plugin.Plugin import com.bumble.appyx.navmodel.backstack.BackStack +import com.bumble.appyx.navmodel.backstack.operation.pop import com.bumble.appyx.navmodel.backstack.operation.push import com.bumble.appyx.navmodel.backstack.operation.singleTop import dev.zacsweers.metro.AppScope @@ -196,7 +197,12 @@ class LoginFlowNode( createNode(buildContext, listOf(callback)) } NavTarget.QrCode -> { - createNode(buildContext) + val callback = object : QrCodeLoginFlowNode.Callback { + override fun navigateBack() { + backstack.pop() + } + } + createNode(buildContext, listOf(callback)) } is NavTarget.ConfirmAccountProvider -> { val inputs = ConfirmAccountProviderNode.Inputs( diff --git a/features/login/impl/src/main/kotlin/io/element/android/features/login/impl/qrcode/QrCodeLoginFlowNode.kt b/features/login/impl/src/main/kotlin/io/element/android/features/login/impl/qrcode/QrCodeLoginFlowNode.kt index 7dad95741b..8a5f77a81e 100644 --- a/features/login/impl/src/main/kotlin/io/element/android/features/login/impl/qrcode/QrCodeLoginFlowNode.kt +++ b/features/login/impl/src/main/kotlin/io/element/android/features/login/impl/qrcode/QrCodeLoginFlowNode.kt @@ -37,6 +37,7 @@ import io.element.android.libraries.architecture.BackstackView import io.element.android.libraries.architecture.BaseFlowNode import io.element.android.libraries.architecture.NodeInputs import io.element.android.libraries.architecture.bindings +import io.element.android.libraries.architecture.callback import io.element.android.libraries.architecture.createNode import io.element.android.libraries.core.coroutine.CoroutineDispatchers import io.element.android.libraries.di.DependencyInjectionGraphOwner @@ -64,6 +65,12 @@ class QrCodeLoginFlowNode( buildContext = buildContext, plugins = plugins, ), DependencyInjectionGraphOwner { + interface Callback : Plugin { + fun navigateBack() + } + + private val callback: Callback = callback() + private var authenticationJob: Job? = null override val graph = qrCodeLoginGraphFactory.create() @@ -85,7 +92,6 @@ class QrCodeLoginFlowNode( override fun onBuilt() { super.onBuilt() - observeLoginStep() } @@ -178,7 +184,13 @@ class QrCodeLoginFlowNode( } is NavTarget.Error -> { val callback = object : QrCodeErrorNode.Callback { - override fun onRetry() = reset() + override fun onRetry() { + reset() + } + + override fun onCancel() { + callback.navigateBack() + } } createNode(buildContext, plugins = listOf(navTarget.errorType, callback)) } diff --git a/features/login/impl/src/main/kotlin/io/element/android/features/login/impl/screens/qrcode/error/QrCodeErrorNode.kt b/features/login/impl/src/main/kotlin/io/element/android/features/login/impl/screens/qrcode/error/QrCodeErrorNode.kt index 4dc1e48e51..506335163e 100644 --- a/features/login/impl/src/main/kotlin/io/element/android/features/login/impl/screens/qrcode/error/QrCodeErrorNode.kt +++ b/features/login/impl/src/main/kotlin/io/element/android/features/login/impl/screens/qrcode/error/QrCodeErrorNode.kt @@ -31,6 +31,7 @@ class QrCodeErrorNode( ) : Node(buildContext = buildContext, plugins = plugins) { interface Callback : Plugin { fun onRetry() + fun onCancel() } private val callback: Callback = callback() @@ -43,6 +44,7 @@ class QrCodeErrorNode( errorScreenType = qrCodeErrorScreenType, appName = buildMeta.productionApplicationName, onRetry = callback::onRetry, + onCancel = callback::onCancel, ) } } diff --git a/features/login/impl/src/main/kotlin/io/element/android/features/login/impl/screens/qrcode/error/QrCodeErrorView.kt b/features/login/impl/src/main/kotlin/io/element/android/features/login/impl/screens/qrcode/error/QrCodeErrorView.kt index d2ec6ce192..a6d9da3de2 100644 --- a/features/login/impl/src/main/kotlin/io/element/android/features/login/impl/screens/qrcode/error/QrCodeErrorView.kt +++ b/features/login/impl/src/main/kotlin/io/element/android/features/login/impl/screens/qrcode/error/QrCodeErrorView.kt @@ -35,6 +35,7 @@ import io.element.android.libraries.designsystem.components.BigIcon import io.element.android.libraries.designsystem.preview.ElementPreview import io.element.android.libraries.designsystem.preview.PreviewsDayNight import io.element.android.libraries.designsystem.theme.components.Button +import io.element.android.libraries.designsystem.theme.components.OutlinedButton import io.element.android.libraries.designsystem.theme.components.Text import io.element.android.libraries.ui.strings.CommonStrings import kotlinx.collections.immutable.persistentListOf @@ -44,16 +45,22 @@ fun QrCodeErrorView( errorScreenType: QrCodeErrorScreenType, appName: String, onRetry: () -> Unit, + onCancel: () -> Unit, modifier: Modifier = Modifier, ) { - BackHandler(onBack = onRetry) + BackHandler(onBack = onCancel) FlowStepPage( modifier = modifier, iconStyle = BigIcon.Style.AlertSolid, title = titleText(errorScreenType, appName), subTitle = subtitleText(errorScreenType, appName), content = { Content(errorScreenType) }, - buttons = { Buttons(onRetry) }, + buttons = { + Buttons( + onRetry = onRetry, + onCancel = onCancel, + ) + }, ) } @@ -118,11 +125,19 @@ private fun Content(errorScreenType: QrCodeErrorScreenType) { } @Composable -private fun Buttons(onRetry: () -> Unit) { +private fun Buttons( + onRetry: () -> Unit, + onCancel: () -> Unit, +) { Button( modifier = Modifier.fillMaxWidth(), - text = stringResource(R.string.screen_qr_code_login_start_over_button), - onClick = onRetry + text = stringResource(CommonStrings.action_try_again), + onClick = onRetry, + ) + OutlinedButton( + modifier = Modifier.fillMaxWidth(), + text = stringResource(CommonStrings.action_cancel), + onClick = onCancel, ) } @@ -133,7 +148,8 @@ internal fun QrCodeErrorViewPreview(@PreviewParameter(QrCodeErrorScreenTypeProvi QrCodeErrorView( errorScreenType = errorScreenType, appName = "Element X", - onRetry = {} + onRetry = {}, + onCancel = {}, ) } } diff --git a/features/login/impl/src/test/kotlin/io/element/android/features/login/impl/qrcode/QrCodeLoginFlowNodeTest.kt b/features/login/impl/src/test/kotlin/io/element/android/features/login/impl/qrcode/QrCodeLoginFlowNodeTest.kt index ee99d1170e..32ebe668ef 100644 --- a/features/login/impl/src/test/kotlin/io/element/android/features/login/impl/qrcode/QrCodeLoginFlowNodeTest.kt +++ b/features/login/impl/src/test/kotlin/io/element/android/features/login/impl/qrcode/QrCodeLoginFlowNodeTest.kt @@ -21,6 +21,7 @@ import io.element.android.libraries.matrix.api.auth.qrlogin.QrLoginException import io.element.android.libraries.matrix.test.A_SESSION_ID import io.element.android.libraries.matrix.test.auth.FakeMatrixAuthenticationService import io.element.android.libraries.matrix.test.auth.qrlogin.FakeMatrixQrCodeLoginData +import io.element.android.tests.testutils.lambda.lambdaError import io.element.android.tests.testutils.testCoroutineDispatchers import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.test.TestScope @@ -183,7 +184,11 @@ class QrCodeLoginFlowNodeTest { ) return QrCodeLoginFlowNode( buildContext = buildContext, - plugins = emptyList(), + plugins = listOf( + object : QrCodeLoginFlowNode.Callback { + override fun navigateBack() = lambdaError() + } + ), qrCodeLoginGraphFactory = FakeQrCodeLoginGraph.Builder(qrCodeLoginManager), coroutineDispatchers = coroutineDispatchers, ) diff --git a/features/login/impl/src/test/kotlin/io/element/android/features/login/impl/screens/qrcode/error/QrCodeErrorViewTest.kt b/features/login/impl/src/test/kotlin/io/element/android/features/login/impl/screens/qrcode/error/QrCodeErrorViewTest.kt index c7b6a5e3d6..de0f689220 100644 --- a/features/login/impl/src/test/kotlin/io/element/android/features/login/impl/screens/qrcode/error/QrCodeErrorViewTest.kt +++ b/features/login/impl/src/test/kotlin/io/element/android/features/login/impl/screens/qrcode/error/QrCodeErrorViewTest.kt @@ -12,8 +12,9 @@ import androidx.activity.ComponentActivity import androidx.compose.ui.test.junit4.AndroidComposeTestRule import androidx.compose.ui.test.junit4.createAndroidComposeRule import androidx.test.ext.junit.runners.AndroidJUnit4 -import io.element.android.features.login.impl.R import io.element.android.features.login.impl.qrcode.QrCodeErrorScreenType +import io.element.android.libraries.ui.strings.CommonStrings +import io.element.android.tests.testutils.EnsureNeverCalled import io.element.android.tests.testutils.clickOn import io.element.android.tests.testutils.ensureCalledOnce import io.element.android.tests.testutils.pressBackKey @@ -28,10 +29,10 @@ class QrCodeErrorViewTest { val rule = createAndroidComposeRule() @Test - fun `on back pressed - calls the onRetry callback`() { + fun `on back pressed - calls the onCancel callback`() { ensureCalledOnce { callback -> rule.setQrCodeErrorView( - onRetry = callback + onCancel = callback, ) rule.pressBackKey() } @@ -41,14 +42,25 @@ class QrCodeErrorViewTest { fun `on try again button clicked - calls the expected callback`() { ensureCalledOnce { callback -> rule.setQrCodeErrorView( - onRetry = callback + onRetry = callback, ) - rule.clickOn(R.string.screen_qr_code_login_start_over_button) + rule.clickOn(CommonStrings.action_try_again) + } + } + + @Test + fun `on cancel button clicked - calls the expected callback`() { + ensureCalledOnce { callback -> + rule.setQrCodeErrorView( + onCancel = callback, + ) + rule.clickOn(CommonStrings.action_cancel) } } private fun AndroidComposeTestRule.setQrCodeErrorView( - onRetry: () -> Unit, + onRetry: () -> Unit = EnsureNeverCalled(), + onCancel: () -> Unit = EnsureNeverCalled(), errorScreenType: QrCodeErrorScreenType = QrCodeErrorScreenType.UnknownError, appName: String = "Element X", ) { @@ -56,7 +68,8 @@ class QrCodeErrorViewTest { QrCodeErrorView( errorScreenType = errorScreenType, appName = appName, - onRetry = onRetry + onRetry = onRetry, + onCancel = onCancel, ) } }