From 009136ce1ec2e7976e9f692c6b1a27efcac1b41e Mon Sep 17 00:00:00 2001 From: David Perez Date: Wed, 28 Jan 2026 08:59:22 -0600 Subject: [PATCH] Minor cleanup of the MigrateToMyItemsScreen (#6421) --- .../vaultunlocked/VaultUnlockedNavigation.kt | 1 - .../MigrateToMyItemsNavigation.kt | 2 - .../MigrateToMyItemsScreen.kt | 16 +- .../MigrateToMyItemsViewModel.kt | 50 ++-- .../MigrateToMyItemsScreenTest.kt | 8 - .../MigrateToMyItemsViewModelTest.kt | 243 ++++++++---------- 6 files changed, 141 insertions(+), 179 deletions(-) diff --git a/app/src/main/kotlin/com/x8bit/bitwarden/ui/platform/feature/vaultunlocked/VaultUnlockedNavigation.kt b/app/src/main/kotlin/com/x8bit/bitwarden/ui/platform/feature/vaultunlocked/VaultUnlockedNavigation.kt index d13636317d..2eab9040f6 100644 --- a/app/src/main/kotlin/com/x8bit/bitwarden/ui/platform/feature/vaultunlocked/VaultUnlockedNavigation.kt +++ b/app/src/main/kotlin/com/x8bit/bitwarden/ui/platform/feature/vaultunlocked/VaultUnlockedNavigation.kt @@ -268,7 +268,6 @@ fun NavGraphBuilder.vaultUnlockedGraph( ) migrateToMyItemsDestination( - onNavigateToVault = { navController.popBackStack() }, onNavigateToLeaveOrganization = { organizationId, organizationName -> navController.navigateToLeaveOrganization( organizationId = organizationId, diff --git a/app/src/main/kotlin/com/x8bit/bitwarden/ui/vault/feature/migratetomyitems/MigrateToMyItemsNavigation.kt b/app/src/main/kotlin/com/x8bit/bitwarden/ui/vault/feature/migratetomyitems/MigrateToMyItemsNavigation.kt index 193344dcc6..677b96fbe1 100644 --- a/app/src/main/kotlin/com/x8bit/bitwarden/ui/vault/feature/migratetomyitems/MigrateToMyItemsNavigation.kt +++ b/app/src/main/kotlin/com/x8bit/bitwarden/ui/vault/feature/migratetomyitems/MigrateToMyItemsNavigation.kt @@ -70,12 +70,10 @@ fun NavController.navigateToMigrateToMyItems( * Add the migrate to my items screen to the nav graph. */ fun NavGraphBuilder.migrateToMyItemsDestination( - onNavigateToVault: () -> Unit, onNavigateToLeaveOrganization: (organizationId: String, organizationName: String) -> Unit, ) { composableWithSlideTransitions { MigrateToMyItemsScreen( - onNavigateToVault = onNavigateToVault, onNavigateToLeaveOrganization = onNavigateToLeaveOrganization, ) } diff --git a/app/src/main/kotlin/com/x8bit/bitwarden/ui/vault/feature/migratetomyitems/MigrateToMyItemsScreen.kt b/app/src/main/kotlin/com/x8bit/bitwarden/ui/vault/feature/migratetomyitems/MigrateToMyItemsScreen.kt index ddaf3c6340..0f1265df8d 100644 --- a/app/src/main/kotlin/com/x8bit/bitwarden/ui/vault/feature/migratetomyitems/MigrateToMyItemsScreen.kt +++ b/app/src/main/kotlin/com/x8bit/bitwarden/ui/vault/feature/migratetomyitems/MigrateToMyItemsScreen.kt @@ -3,13 +3,19 @@ package com.x8bit.bitwarden.ui.vault.feature.migratetomyitems import androidx.compose.foundation.Image import androidx.compose.foundation.layout.Column import androidx.compose.foundation.layout.Spacer +import androidx.compose.foundation.layout.WindowInsets +import androidx.compose.foundation.layout.WindowInsetsSides +import androidx.compose.foundation.layout.displayCutout import androidx.compose.foundation.layout.fillMaxSize import androidx.compose.foundation.layout.fillMaxWidth import androidx.compose.foundation.layout.height import androidx.compose.foundation.layout.navigationBarsPadding +import androidx.compose.foundation.layout.only import androidx.compose.foundation.layout.size +import androidx.compose.foundation.layout.union import androidx.compose.foundation.rememberScrollState import androidx.compose.foundation.verticalScroll +import androidx.compose.material3.ScaffoldDefaults import androidx.compose.material3.Text import androidx.compose.runtime.Composable import androidx.compose.runtime.getValue @@ -44,7 +50,6 @@ import com.x8bit.bitwarden.ui.vault.feature.migratetomyitems.handler.rememberMig */ @Composable fun MigrateToMyItemsScreen( - onNavigateToVault: () -> Unit, onNavigateToLeaveOrganization: (organizationId: String, organizationName: String) -> Unit, viewModel: MigrateToMyItemsViewModel = hiltViewModel(), intentManager: IntentManager = LocalIntentManager.current, @@ -54,10 +59,10 @@ fun MigrateToMyItemsScreen( EventsEffect(viewModel = viewModel) { event -> when (event) { - MigrateToMyItemsEvent.NavigateToVault -> onNavigateToVault() is MigrateToMyItemsEvent.NavigateToLeaveOrganization -> { onNavigateToLeaveOrganization(event.organizationId, event.organizationName) } + is MigrateToMyItemsEvent.LaunchUri -> intentManager.launchUri(event.uri.toUri()) } } @@ -68,7 +73,12 @@ fun MigrateToMyItemsScreen( onDismissNoNetworkRequest = handlers.onDismissNoNetworkDialog, ) - BitwardenScaffold { + BitwardenScaffold( + contentWindowInsets = ScaffoldDefaults + .contentWindowInsets + .union(WindowInsets.displayCutout) + .only(WindowInsetsSides.Horizontal + WindowInsetsSides.Top), + ) { MigrateToMyItemsContent( state = state, onAcceptClick = handlers.onAcceptClick, diff --git a/app/src/main/kotlin/com/x8bit/bitwarden/ui/vault/feature/migratetomyitems/MigrateToMyItemsViewModel.kt b/app/src/main/kotlin/com/x8bit/bitwarden/ui/vault/feature/migratetomyitems/MigrateToMyItemsViewModel.kt index c0edad3e9d..16c42e2191 100644 --- a/app/src/main/kotlin/com/x8bit/bitwarden/ui/vault/feature/migratetomyitems/MigrateToMyItemsViewModel.kt +++ b/app/src/main/kotlin/com/x8bit/bitwarden/ui/vault/feature/migratetomyitems/MigrateToMyItemsViewModel.kt @@ -3,7 +3,6 @@ package com.x8bit.bitwarden.ui.vault.feature.migratetomyitems import android.os.Parcelable import androidx.lifecycle.SavedStateHandle import androidx.lifecycle.viewModelScope -import com.bitwarden.core.data.repository.error.MissingPropertyException import com.bitwarden.network.util.isNoConnectionError import com.bitwarden.network.util.isTimeoutError import com.bitwarden.ui.platform.base.BaseViewModel @@ -13,6 +12,7 @@ import com.bitwarden.ui.platform.resource.BitwardenString import com.bitwarden.ui.util.Text import com.bitwarden.ui.util.asText import com.x8bit.bitwarden.data.auth.repository.AuthRepository +import com.x8bit.bitwarden.data.platform.error.NoActiveUserException import com.x8bit.bitwarden.data.platform.manager.event.OrganizationEventManager import com.x8bit.bitwarden.data.platform.manager.model.OrganizationEvent import com.x8bit.bitwarden.data.vault.manager.VaultMigrationManager @@ -91,9 +91,7 @@ class MigrateToMyItemsViewModel @Inject constructor( trySendAction( MigrateToMyItemsAction.Internal.MigrateToMyItemsResultReceived( result = MigratePersonalVaultResult.Failure( - error = MissingPropertyException( - propertyName = "UserId", - ), + error = NoActiveUserException(), ), ), ) @@ -164,7 +162,7 @@ class MigrateToMyItemsViewModel @Inject constructor( ), ) clearDialog() - sendEvent(MigrateToMyItemsEvent.NavigateToVault) + // Navigation to vault is handled by state-based navigation. } is MigratePersonalVaultResult.Failure -> { @@ -174,26 +172,23 @@ class MigrateToMyItemsViewModel @Inject constructor( mutableStateFlow.update { it.copy( - dialog = - if (isNetworkOrTimeoutError) { - MigrateToMyItemsState.DialogState.NoNetwork( - title = BitwardenString - .internet_connection_required_title - .asText(), - message = BitwardenString - .internet_connection_required_message - .asText(), - throwable = result.error, - ) - } else { - MigrateToMyItemsState.DialogState.Error( - title = BitwardenString.an_error_has_occurred.asText(), - message = BitwardenString.failed_to_migrate_items_to_x.asText( - it.organizationName, - ), - throwable = result.error, - ) - }, + dialog = if (isNetworkOrTimeoutError) { + MigrateToMyItemsState.DialogState.NoNetwork( + title = BitwardenString.internet_connection_required_title.asText(), + message = BitwardenString + .internet_connection_required_message + .asText(), + throwable = result.error, + ) + } else { + MigrateToMyItemsState.DialogState.Error( + title = BitwardenString.an_error_has_occurred.asText(), + message = BitwardenString.failed_to_migrate_items_to_x.asText( + it.organizationName, + ), + throwable = result.error, + ) + }, ) } } @@ -256,11 +251,6 @@ data class MigrateToMyItemsState( * Models the events that can be sent from the [MigrateToMyItemsViewModel]. */ sealed class MigrateToMyItemsEvent { - /** - * Navigate to the vault screen after accepting migration. - */ - data object NavigateToVault : MigrateToMyItemsEvent() - /** * Navigate to the leave organization flow after declining. */ diff --git a/app/src/test/kotlin/com/x8bit/bitwarden/ui/vault/feature/migratetomyitems/MigrateToMyItemsScreenTest.kt b/app/src/test/kotlin/com/x8bit/bitwarden/ui/vault/feature/migratetomyitems/MigrateToMyItemsScreenTest.kt index f46bb704b8..2fb8abdd70 100644 --- a/app/src/test/kotlin/com/x8bit/bitwarden/ui/vault/feature/migratetomyitems/MigrateToMyItemsScreenTest.kt +++ b/app/src/test/kotlin/com/x8bit/bitwarden/ui/vault/feature/migratetomyitems/MigrateToMyItemsScreenTest.kt @@ -20,7 +20,6 @@ import org.junit.Test import org.junit.jupiter.api.Assertions.assertTrue class MigrateToMyItemsScreenTest : BitwardenComposeTest() { - private var onNavigateToVaultCalled = false private var onNavigateToLeaveOrganizationCalled = false private val intentManager: IntentManager = mockk { @@ -47,7 +46,6 @@ class MigrateToMyItemsScreenTest : BitwardenComposeTest() { setContent(intentManager = intentManager) { MigrateToMyItemsScreen( viewModel = viewModel, - onNavigateToVault = { onNavigateToVaultCalled = true }, onNavigateToLeaveOrganization = { _, _ -> onNavigateToLeaveOrganizationCalled = true }, @@ -101,12 +99,6 @@ class MigrateToMyItemsScreenTest : BitwardenComposeTest() { } } - @Test - fun `NavigateToVault event should trigger navigation callback`() { - mutableEventFlow.tryEmit(MigrateToMyItemsEvent.NavigateToVault) - assertTrue(onNavigateToVaultCalled) - } - @Test fun `NavigateToLeaveOrganization event should trigger navigation callback`() { mutableEventFlow.tryEmit( diff --git a/app/src/test/kotlin/com/x8bit/bitwarden/ui/vault/feature/migratetomyitems/MigrateToMyItemsViewModelTest.kt b/app/src/test/kotlin/com/x8bit/bitwarden/ui/vault/feature/migratetomyitems/MigrateToMyItemsViewModelTest.kt index 4d5b30bad4..f837cda4d7 100644 --- a/app/src/test/kotlin/com/x8bit/bitwarden/ui/vault/feature/migratetomyitems/MigrateToMyItemsViewModelTest.kt +++ b/app/src/test/kotlin/com/x8bit/bitwarden/ui/vault/feature/migratetomyitems/MigrateToMyItemsViewModelTest.kt @@ -2,7 +2,6 @@ package com.x8bit.bitwarden.ui.vault.feature.migratetomyitems import androidx.lifecycle.SavedStateHandle import app.cash.turbine.test -import com.bitwarden.core.data.repository.error.MissingPropertyException import com.bitwarden.ui.platform.base.BaseViewModelTest import com.bitwarden.ui.platform.components.snackbar.model.BitwardenSnackbarData import com.bitwarden.ui.platform.manager.snackbar.SnackbarRelayManager @@ -10,6 +9,7 @@ import com.bitwarden.ui.platform.resource.BitwardenString import com.bitwarden.ui.util.asText import com.x8bit.bitwarden.data.auth.repository.AuthRepository import com.x8bit.bitwarden.data.auth.repository.model.UserState +import com.x8bit.bitwarden.data.platform.error.NoActiveUserException import com.x8bit.bitwarden.data.platform.manager.event.OrganizationEventManager import com.x8bit.bitwarden.data.platform.manager.model.OrganizationEvent import com.x8bit.bitwarden.data.vault.manager.VaultMigrationManager @@ -21,15 +21,16 @@ import io.mockk.coVerify import io.mockk.every import io.mockk.just import io.mockk.mockk +import io.mockk.mockkConstructor import io.mockk.mockkStatic import io.mockk.runs +import io.mockk.unmockkConstructor import io.mockk.unmockkStatic import io.mockk.verify import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.test.runTest import org.junit.jupiter.api.AfterEach import org.junit.jupiter.api.Assertions.assertEquals -import org.junit.jupiter.api.Assertions.assertNull import org.junit.jupiter.api.BeforeEach import org.junit.jupiter.api.Test import java.net.SocketTimeoutException @@ -62,19 +63,25 @@ class MigrateToMyItemsViewModelTest : BaseViewModelTest() { @BeforeEach fun setup() { mockkStatic(SavedStateHandle::toMigrateToMyItemsArgs) + mockkConstructor(NoActiveUserException::class) + every { + anyConstructed() == any() + } returns true } @AfterEach fun tearDown() { unmockkStatic(SavedStateHandle::toMigrateToMyItemsArgs) + unmockkConstructor(NoActiveUserException::class) } @Test fun `initial state should be set from organization data`() { val viewModel = createViewModel() - assertEquals(ORGANIZATION_NAME, viewModel.stateFlow.value.organizationName) - assertEquals(ORGANIZATION_ID, viewModel.stateFlow.value.organizationId) - assertNull(viewModel.stateFlow.value.dialog) + assertEquals( + DEFAULT_STATE, + viewModel.stateFlow.value, + ) } @Test @@ -102,77 +109,23 @@ class MigrateToMyItemsViewModelTest : BaseViewModelTest() { } } - @Test - fun `AcceptClicked should navigate to vault on success`() = runTest { - val viewModel = createViewModel() - viewModel.eventFlow.test { - viewModel.trySendAction(MigrateToMyItemsAction.AcceptClicked) - assertEquals(MigrateToMyItemsEvent.NavigateToVault, awaitItem()) - } - } - - @Test - fun `AcceptClicked should show error dialog when userId is null`() = runTest { - mutableUserStateFlow.value = null - val viewModel = createViewModel() - viewModel.trySendAction(MigrateToMyItemsAction.AcceptClicked) - - val dialog = viewModel.stateFlow.value.dialog as MigrateToMyItemsState.DialogState.Error - val throwableReference = dialog.throwable - - assert(throwableReference is MissingPropertyException) - assertEquals( - "Missing the required UserId property", - throwableReference?.message, - ) - - assertEquals( - MigrateToMyItemsState.DialogState.Error( - title = BitwardenString.an_error_has_occurred.asText(), - message = BitwardenString.failed_to_migrate_items_to_x.asText( - ORGANIZATION_NAME, - ), - throwable = throwableReference, - ), - viewModel.stateFlow.value.dialog, - ) - - coVerify(exactly = 0) { - mockVaultMigrationManager.migratePersonalVault(any(), any()) - } - } - @Suppress("MaxLineLength") @Test - fun `MigrateToMyItemsResultReceived with success should track ItemOrganizationAccepted event, clear dialog, and navigate to vault`() = + fun `AcceptClicked with valid user and migration should show success snackbar and track the event`() = runTest { val viewModel = createViewModel() - viewModel.eventFlow.test { - // First show the loading dialog - viewModel.trySendAction(MigrateToMyItemsAction.AcceptClicked) + viewModel.trySendAction(MigrateToMyItemsAction.AcceptClicked) - // Migration completes and sends NavigateToVault event - assertEquals(MigrateToMyItemsEvent.NavigateToVault, awaitItem()) - - viewModel.trySendAction( - MigrateToMyItemsAction.Internal.MigrateToMyItemsResultReceived( - result = MigratePersonalVaultResult.Success, - ), + coVerify(exactly = 1) { + mockVaultMigrationManager.migratePersonalVault( + userId = "test-user-id", + organizationId = ORGANIZATION_ID, + ) + mockSnackbarRelayManager.sendSnackbarData( + relay = SnackbarRelay.VAULT_MIGRATED_TO_MY_ITEMS, + data = BitwardenSnackbarData(message = BitwardenString.items_transferred.asText()), ) - assertEquals(MigrateToMyItemsEvent.NavigateToVault, awaitItem()) - } - - assertNull(viewModel.stateFlow.value.dialog) - - mockSnackbarRelayManager.sendSnackbarData( - relay = SnackbarRelay.LEFT_ORGANIZATION, - data = BitwardenSnackbarData( - message = BitwardenString.you_left_the_organization.asText(), - ), - ) - - verify { mockOrganizationEventManager.trackEvent( event = OrganizationEvent.ItemOrganizationAccepted( organizationId = ORGANIZATION_ID, @@ -182,29 +135,70 @@ class MigrateToMyItemsViewModelTest : BaseViewModelTest() { } @Test - fun `MigrateToMyItemsResultReceived with failure should show error dialog`() = runTest { + fun `AcceptClicked should show error dialog when userId is null`() = runTest { + mutableUserStateFlow.value = null val viewModel = createViewModel() - viewModel.stateFlow.test { - awaitItem() // Initial state + viewModel.trySendAction(MigrateToMyItemsAction.AcceptClicked) - viewModel.trySendAction( - MigrateToMyItemsAction.Internal.MigrateToMyItemsResultReceived( - result = MigratePersonalVaultResult.Failure(null), + assertEquals( + DEFAULT_STATE.copy( + dialog = MigrateToMyItemsState.DialogState.Error( + title = BitwardenString.an_error_has_occurred.asText(), + message = BitwardenString.failed_to_migrate_items_to_x.asText( + ORGANIZATION_NAME, + ), + throwable = NoActiveUserException(), ), - ) + ), + viewModel.stateFlow.value, + ) - val errorState = awaitItem() - assert(errorState.dialog is MigrateToMyItemsState.DialogState.Error) - val errorDialog = errorState.dialog as MigrateToMyItemsState.DialogState.Error - assertEquals(BitwardenString.an_error_has_occurred.asText(), errorDialog.title) - assertEquals( - BitwardenString.failed_to_migrate_items_to_x.asText(ORGANIZATION_NAME), - errorDialog.message, - ) + coVerify(exactly = 0) { + mockVaultMigrationManager.migratePersonalVault(any(), any()) } } + @Suppress("MaxLineLength") + @Test + fun `MigrateToMyItemsResultReceived with valid user ID and failed migration should show error dialog`() = + runTest { + val viewModel = createViewModel() + + val error = Throwable("Fail") + coEvery { + mockVaultMigrationManager.migratePersonalVault(any(), any()) + } returns MigratePersonalVaultResult.Failure(error) + + viewModel.stateFlow.test { + awaitItem() // Initial state + + viewModel.trySendAction(MigrateToMyItemsAction.AcceptClicked) + + assertEquals( + DEFAULT_STATE.copy( + dialog = MigrateToMyItemsState.DialogState.Loading( + message = BitwardenString.migrating_items_to_x.asText(ORGANIZATION_NAME), + ), + ), + awaitItem(), + ) + + assertEquals( + DEFAULT_STATE.copy( + dialog = MigrateToMyItemsState.DialogState.Error( + title = BitwardenString.an_error_has_occurred.asText(), + message = BitwardenString.failed_to_migrate_items_to_x.asText( + ORGANIZATION_NAME, + ), + throwable = error, + ), + ), + awaitItem(), + ) + } + } + @Suppress("MaxLineLength") @Test fun `MigrateToMyItemsResultReceived with timeout error should show DialogState NoNetwork`() = @@ -228,7 +222,8 @@ class MigrateToMyItemsViewModelTest : BaseViewModelTest() { message = BitwardenString.internet_connection_required_message.asText(), throwable = error, ), - ), awaitItem(), + ), + awaitItem(), ) } } @@ -253,45 +248,33 @@ class MigrateToMyItemsViewModelTest : BaseViewModelTest() { val viewModel = createViewModel() viewModel.eventFlow.test { viewModel.trySendAction(MigrateToMyItemsAction.HelpLinkClicked) - val event = awaitItem() - assert(event is MigrateToMyItemsEvent.LaunchUri) assertEquals( - "https://bitwarden.com/help/transfer-ownership/", - (event as MigrateToMyItemsEvent.LaunchUri).uri, + MigrateToMyItemsEvent.LaunchUri("https://bitwarden.com/help/transfer-ownership/"), + awaitItem(), ) } } @Test fun `DismissDialogClicked should clear dialog`() = runTest { - val viewModel = createViewModel() + val initialState = DEFAULT_STATE.copy( + dialog = MigrateToMyItemsState.DialogState.Error( + title = BitwardenString.an_error_has_occurred.asText(), + message = BitwardenString.failed_to_migrate_items_to_x.asText( + ORGANIZATION_NAME, + ), + throwable = null, + ), + ) + val viewModel = createViewModel(state = initialState) viewModel.stateFlow.test { - awaitItem() // Initial state - - // First show an error dialog - viewModel.trySendAction( - MigrateToMyItemsAction.Internal.MigrateToMyItemsResultReceived( - result = MigratePersonalVaultResult.Failure(null), - ), - ) - - assertEquals( - DEFAULT_STATE.copy( - dialog = MigrateToMyItemsState.DialogState.Error( - title = BitwardenString.an_error_has_occurred.asText(), - message = BitwardenString.failed_to_migrate_items_to_x.asText( - ORGANIZATION_NAME, - ), - throwable = null, - ), - ), awaitItem(), - ) - + assertEquals(initialState, awaitItem()) // Dismiss the dialog viewModel.trySendAction(MigrateToMyItemsAction.DismissDialogClicked) assertEquals( - DEFAULT_STATE, awaitItem(), + DEFAULT_STATE, + awaitItem(), ) } } @@ -299,35 +282,25 @@ class MigrateToMyItemsViewModelTest : BaseViewModelTest() { @Suppress("MaxLineLength") @Test fun `NoNetworkDismissDialogClicked should clear dialog and clear migration state`() = runTest { - val viewModel = createViewModel() val error = SocketTimeoutException("Timeout") + val initialState = DEFAULT_STATE.copy( + dialog = MigrateToMyItemsState.DialogState.NoNetwork( + title = BitwardenString.internet_connection_required_title.asText(), + message = BitwardenString.internet_connection_required_message.asText(), + throwable = error, + ), + ) + val viewModel = createViewModel(state = initialState) viewModel.stateFlow.test { - awaitItem() // Initial state - - // First show an error dialog - viewModel.trySendAction( - MigrateToMyItemsAction.Internal.MigrateToMyItemsResultReceived( - result = MigratePersonalVaultResult.Failure(error = error), - ), - ) - - assertEquals( - DEFAULT_STATE.copy( - dialog = MigrateToMyItemsState.DialogState.NoNetwork( - title = BitwardenString.internet_connection_required_title.asText(), - message = BitwardenString.internet_connection_required_message.asText(), - throwable = error, - ), - ), awaitItem(), - ) + assertEquals(initialState, awaitItem()) // Dismiss the dialog viewModel.trySendAction(MigrateToMyItemsAction.NoNetworkDismissDialogClicked) - verify { mockVaultMigrationManager.clearMigrationState() } - assertEquals( - DEFAULT_STATE, awaitItem(), - ) + assertEquals(DEFAULT_STATE, awaitItem()) + } + verify(exactly = 1) { + mockVaultMigrationManager.clearMigrationState() } }