From 2750835a36d7e0f4694c59d8884df2b48400c9ff Mon Sep 17 00:00:00 2001 From: Jorge Martin Espinosa Date: Fri, 25 Jul 2025 13:36:43 +0200 Subject: [PATCH] Make sure we display errors when we create a recovery key and it fails (#5079) * Make sure we display errors when we create a recovery key and it fails * Add another preview for the error state * Update screenshots --------- Co-authored-by: ElementBot --- .../impl/setup/SecureBackupSetupPresenter.kt | 13 ++++++-- .../impl/setup/SecureBackupSetupState.kt | 1 + .../setup/SecureBackupSetupStateMachine.kt | 13 ++++++-- .../setup/SecureBackupSetupStateProvider.kt | 1 + .../impl/setup/SecureBackupSetupView.kt | 20 ++++++++++-- .../setup/SecureBackupSetupPresenterTest.kt | 32 ++++++++++++++++++- .../test/encryption/FakeEncryptionService.kt | 5 +-- ...p_SecureBackupSetupViewChange_Day_5_en.png | 3 ++ ...SecureBackupSetupViewChange_Night_5_en.png | 3 ++ ...l.setup_SecureBackupSetupView_Day_5_en.png | 3 ++ ...setup_SecureBackupSetupView_Night_5_en.png | 3 ++ 11 files changed, 86 insertions(+), 11 deletions(-) create mode 100644 tests/uitests/src/test/snapshots/images/features.securebackup.impl.setup_SecureBackupSetupViewChange_Day_5_en.png create mode 100644 tests/uitests/src/test/snapshots/images/features.securebackup.impl.setup_SecureBackupSetupViewChange_Night_5_en.png create mode 100644 tests/uitests/src/test/snapshots/images/features.securebackup.impl.setup_SecureBackupSetupView_Day_5_en.png create mode 100644 tests/uitests/src/test/snapshots/images/features.securebackup.impl.setup_SecureBackupSetupView_Night_5_en.png diff --git a/features/securebackup/impl/src/main/kotlin/io/element/android/features/securebackup/impl/setup/SecureBackupSetupPresenter.kt b/features/securebackup/impl/src/main/kotlin/io/element/android/features/securebackup/impl/setup/SecureBackupSetupPresenter.kt index 83f27c8a8a..c71e6edec0 100644 --- a/features/securebackup/impl/src/main/kotlin/io/element/android/features/securebackup/impl/setup/SecureBackupSetupPresenter.kt +++ b/features/securebackup/impl/src/main/kotlin/io/element/android/features/securebackup/impl/setup/SecureBackupSetupPresenter.kt @@ -60,6 +60,7 @@ class SecureBackupSetupPresenter @AssistedInject constructor( stateAndDispatch.dispatchAction(SecureBackupSetupStateMachine.Event.UserSavedKey) SecureBackupSetupEvents.DismissDialog -> { showSaveConfirmationDialog = false + stateAndDispatch.dispatchAction(SecureBackupSetupStateMachine.Event.ClearError) } SecureBackupSetupEvents.Done -> { showSaveConfirmationDialog = true @@ -89,6 +90,7 @@ class SecureBackupSetupPresenter @AssistedInject constructor( SecureBackupSetupStateMachine.State.CreatingKey -> SetupState.Creating is SecureBackupSetupStateMachine.State.KeyCreated -> SetupState.Created(formattedRecoveryKey = key) is SecureBackupSetupStateMachine.State.KeyCreatedAndSaved -> SetupState.CreatedAndSaved(formattedRecoveryKey = key) + is SecureBackupSetupStateMachine.State.Error -> SetupState.Error(exception) } } @@ -103,13 +105,20 @@ class SecureBackupSetupPresenter @AssistedInject constructor( stateAndDispatch.dispatchAction(SecureBackupSetupStateMachine.Event.SdkHasCreatedKey(it)) }, onFailure = { - stateAndDispatch.dispatchAction(SecureBackupSetupStateMachine.Event.SdkError(it)) + if (it is Exception) { + stateAndDispatch.dispatchAction(SecureBackupSetupStateMachine.Event.SdkError(it)) + } } ) } else { observeEncryptionService(stateAndDispatch) Timber.tag(loggerTagSetup.value).d("Calling encryptionService.enableRecovery()") - encryptionService.enableRecovery(waitForBackupsToUpload = false) + encryptionService.enableRecovery(waitForBackupsToUpload = false).onFailure { + Timber.tag(loggerTagSetup.value).e(it, "Failed to enable recovery") + if (it is Exception) { + stateAndDispatch.dispatchAction(SecureBackupSetupStateMachine.Event.SdkError(it)) + } + } } } diff --git a/features/securebackup/impl/src/main/kotlin/io/element/android/features/securebackup/impl/setup/SecureBackupSetupState.kt b/features/securebackup/impl/src/main/kotlin/io/element/android/features/securebackup/impl/setup/SecureBackupSetupState.kt index 430d368c6f..91f710d07d 100644 --- a/features/securebackup/impl/src/main/kotlin/io/element/android/features/securebackup/impl/setup/SecureBackupSetupState.kt +++ b/features/securebackup/impl/src/main/kotlin/io/element/android/features/securebackup/impl/setup/SecureBackupSetupState.kt @@ -23,6 +23,7 @@ sealed interface SetupState { data object Creating : SetupState data class Created(val formattedRecoveryKey: String) : SetupState data class CreatedAndSaved(val formattedRecoveryKey: String) : SetupState + data class Error(val exception: Exception) : SetupState } fun SetupState.recoveryKey(): String? = when (this) { diff --git a/features/securebackup/impl/src/main/kotlin/io/element/android/features/securebackup/impl/setup/SecureBackupSetupStateMachine.kt b/features/securebackup/impl/src/main/kotlin/io/element/android/features/securebackup/impl/setup/SecureBackupSetupStateMachine.kt index 0567f4dba7..4fbf3f3fd2 100644 --- a/features/securebackup/impl/src/main/kotlin/io/element/android/features/securebackup/impl/setup/SecureBackupSetupStateMachine.kt +++ b/features/securebackup/impl/src/main/kotlin/io/element/android/features/securebackup/impl/setup/SecureBackupSetupStateMachine.kt @@ -26,8 +26,8 @@ class SecureBackupSetupStateMachine @Inject constructor() : FlowReduxStateMachin } } inState { - on { _: Event.SdkError, state: MachineState -> - state.override { State.Initial } + on { event: Event.SdkError, state: MachineState -> + state.override { State.Error(event.exception) } } on { event: Event.SdkHasCreatedKey, state: MachineState -> state.override { State.KeyCreated(event.key) } @@ -38,6 +38,11 @@ class SecureBackupSetupStateMachine @Inject constructor() : FlowReduxStateMachin state.override { State.KeyCreatedAndSaved(state.snapshot.key) } } } + inState { + on { _: Event.ClearError, state: MachineState -> + state.override { State.Initial } + } + } inState { } } @@ -48,12 +53,14 @@ class SecureBackupSetupStateMachine @Inject constructor() : FlowReduxStateMachin data object CreatingKey : State data class KeyCreated(val key: String) : State data class KeyCreatedAndSaved(val key: String) : State + data class Error(val exception: Exception) : State } sealed interface Event { data object UserCreatesKey : Event data class SdkHasCreatedKey(val key: String) : Event - data class SdkError(val throwable: Throwable) : Event + data class SdkError(val exception: Exception) : Event data object UserSavedKey : Event + data object ClearError : Event } } diff --git a/features/securebackup/impl/src/main/kotlin/io/element/android/features/securebackup/impl/setup/SecureBackupSetupStateProvider.kt b/features/securebackup/impl/src/main/kotlin/io/element/android/features/securebackup/impl/setup/SecureBackupSetupStateProvider.kt index b371548da2..29c03dbcc8 100644 --- a/features/securebackup/impl/src/main/kotlin/io/element/android/features/securebackup/impl/setup/SecureBackupSetupStateProvider.kt +++ b/features/securebackup/impl/src/main/kotlin/io/element/android/features/securebackup/impl/setup/SecureBackupSetupStateProvider.kt @@ -23,6 +23,7 @@ open class SecureBackupSetupStateProvider : PreviewParameterProvider if (state.isChangeRecoveryKeyUserStory) { + SetupState.Creating, + is SetupState.Error -> if (state.isChangeRecoveryKeyUserStory) { stringResource(id = R.string.screen_recovery_key_change_title) } else { stringResource(id = R.string.screen_recovery_key_setup_title) @@ -85,7 +97,8 @@ private fun title(state: SecureBackupSetupState): String { private fun subtitle(state: SecureBackupSetupState): String { return when (state.setupState) { SetupState.Init, - SetupState.Creating -> if (state.isChangeRecoveryKeyUserStory) { + SetupState.Creating, + is SetupState.Error -> if (state.isChangeRecoveryKeyUserStory) { stringResource(id = R.string.screen_recovery_key_change_description) } else { stringResource(id = R.string.screen_recovery_key_setup_description) @@ -137,7 +150,8 @@ private fun ColumnScope.Buttons( val chooserTitle = stringResource(id = R.string.screen_recovery_key_save_action) when (state.setupState) { SetupState.Init, - SetupState.Creating -> { + SetupState.Creating, + is SetupState.Error -> { Button( text = stringResource(id = CommonStrings.action_done), enabled = false, diff --git a/features/securebackup/impl/src/test/kotlin/io/element/android/features/securebackup/impl/setup/SecureBackupSetupPresenterTest.kt b/features/securebackup/impl/src/test/kotlin/io/element/android/features/securebackup/impl/setup/SecureBackupSetupPresenterTest.kt index 7e8bb156e0..003d7645de 100644 --- a/features/securebackup/impl/src/test/kotlin/io/element/android/features/securebackup/impl/setup/SecureBackupSetupPresenterTest.kt +++ b/features/securebackup/impl/src/test/kotlin/io/element/android/features/securebackup/impl/setup/SecureBackupSetupPresenterTest.kt @@ -109,6 +109,34 @@ class SecureBackupSetupPresenterTest { } } + @Test + fun `present - handle errors`() = runTest { + val encryptionService = FakeEncryptionService( + enableRecoveryLambda = { Result.failure(IllegalStateException("Test error")) } + ) + val presenter = createSecureBackupSetupPresenter( + isChangeRecoveryKeyUserStory = false, + encryptionService = encryptionService + ) + moleculeFlow(RecompositionMode.Immediate) { + presenter.present() + }.test { + val initialState = awaitItem() + assertThat(initialState.isChangeRecoveryKeyUserStory).isFalse() + assertThat(initialState.setupState).isEqualTo(SetupState.Init) + + initialState.eventSink(SecureBackupSetupEvents.CreateRecoveryKey) + val creatingState = awaitItem() + assertThat(creatingState.setupState).isEqualTo(SetupState.Creating) + val failedState = awaitItem() + assertThat(failedState.setupState).isInstanceOf(SetupState.Error::class.java) + failedState.eventSink(SecureBackupSetupEvents.DismissDialog) + + val finalState = awaitItem() + assertThat(finalState.setupState).isEqualTo(SetupState.Init) + } + } + @Test fun `present - change recovery key and save it`() = runTest { val encryptionService = FakeEncryptionService() @@ -153,7 +181,9 @@ class SecureBackupSetupPresenterTest { private fun createSecureBackupSetupPresenter( isChangeRecoveryKeyUserStory: Boolean = false, - encryptionService: EncryptionService = FakeEncryptionService(), + encryptionService: EncryptionService = FakeEncryptionService( + enableRecoveryLambda = { Result.success(Unit) }, + ), ): SecureBackupSetupPresenter { return SecureBackupSetupPresenter( isChangeRecoveryKeyUserStory = isChangeRecoveryKeyUserStory, diff --git a/libraries/matrix/test/src/main/kotlin/io/element/android/libraries/matrix/test/encryption/FakeEncryptionService.kt b/libraries/matrix/test/src/main/kotlin/io/element/android/libraries/matrix/test/encryption/FakeEncryptionService.kt index 25e8a535b0..7226465772 100644 --- a/libraries/matrix/test/src/main/kotlin/io/element/android/libraries/matrix/test/encryption/FakeEncryptionService.kt +++ b/libraries/matrix/test/src/main/kotlin/io/element/android/libraries/matrix/test/encryption/FakeEncryptionService.kt @@ -26,7 +26,8 @@ class FakeEncryptionService( private val pinUserIdentityResult: (UserId) -> Result = { lambdaError() }, private val isUserVerifiedResult: (UserId) -> Result = { lambdaError() }, private val withdrawVerificationResult: (UserId) -> Result = { lambdaError() }, - private val getUserIdentityResult: (UserId) -> Result = { lambdaError() } + private val getUserIdentityResult: (UserId) -> Result = { lambdaError() }, + private val enableRecoveryLambda: (Boolean) -> Result = { lambdaError() }, ) : EncryptionService { private var disableRecoveryFailure: Exception? = null override val backupStateStateFlow: MutableStateFlow = MutableStateFlow(BackupState.UNKNOWN) @@ -87,7 +88,7 @@ class FakeEncryptionService( } override suspend fun enableRecovery(waitForBackupsToUpload: Boolean): Result = simulateLongTask { - return Result.success(Unit) + return enableRecoveryLambda(waitForBackupsToUpload) } fun givenWaitForBackupUploadSteadyStateFlow(flow: Flow) { diff --git a/tests/uitests/src/test/snapshots/images/features.securebackup.impl.setup_SecureBackupSetupViewChange_Day_5_en.png b/tests/uitests/src/test/snapshots/images/features.securebackup.impl.setup_SecureBackupSetupViewChange_Day_5_en.png new file mode 100644 index 0000000000..8974e582a9 --- /dev/null +++ b/tests/uitests/src/test/snapshots/images/features.securebackup.impl.setup_SecureBackupSetupViewChange_Day_5_en.png @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:d55e9236e761726e03742c6292ec3ced7af48b385da5a7be9eccc444a78ec312 +size 35886 diff --git a/tests/uitests/src/test/snapshots/images/features.securebackup.impl.setup_SecureBackupSetupViewChange_Night_5_en.png b/tests/uitests/src/test/snapshots/images/features.securebackup.impl.setup_SecureBackupSetupViewChange_Night_5_en.png new file mode 100644 index 0000000000..dd56483ad5 --- /dev/null +++ b/tests/uitests/src/test/snapshots/images/features.securebackup.impl.setup_SecureBackupSetupViewChange_Night_5_en.png @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:0bc9364efa8aa639f8989d74a5172665ee0fa8f4c9c354b584cfdabfd98d723c +size 32964 diff --git a/tests/uitests/src/test/snapshots/images/features.securebackup.impl.setup_SecureBackupSetupView_Day_5_en.png b/tests/uitests/src/test/snapshots/images/features.securebackup.impl.setup_SecureBackupSetupView_Day_5_en.png new file mode 100644 index 0000000000..bc5dbd592f --- /dev/null +++ b/tests/uitests/src/test/snapshots/images/features.securebackup.impl.setup_SecureBackupSetupView_Day_5_en.png @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:34272a001ad54175280dc251375cee45e2978cba78c7d4b08b10d59ae7093d93 +size 34340 diff --git a/tests/uitests/src/test/snapshots/images/features.securebackup.impl.setup_SecureBackupSetupView_Night_5_en.png b/tests/uitests/src/test/snapshots/images/features.securebackup.impl.setup_SecureBackupSetupView_Night_5_en.png new file mode 100644 index 0000000000..72dcc2cfa9 --- /dev/null +++ b/tests/uitests/src/test/snapshots/images/features.securebackup.impl.setup_SecureBackupSetupView_Night_5_en.png @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:86e9cbfe60551d6d7f052ad0a7f75eda029569d9d51cf63339e223df5332ec63 +size 31674