From b0885ff60a4a7e8d15a4a93c1157372c092eeb0a Mon Sep 17 00:00:00 2001
From: Dave Severns <149429124+dseverns-livefront@users.noreply.github.com>
Date: Thu, 24 Oct 2024 13:06:30 -0400
Subject: [PATCH] PM-13886 show dialog when no logins were imported (#4139)
---
.../vault/repository/VaultRepositoryImpl.kt | 10 +-
.../repository/model/SyncVaultDataResult.kt | 4 +-
.../importlogins/ImportLoginsScreen.kt | 2 +-
.../importlogins/ImportLoginsViewModel.kt | 32 +++--
app/src/main/res/values/strings.xml | 1 +
.../vault/repository/VaultRepositoryTest.kt | 125 +++++++++++++-----
.../importlogins/ImportLoginsScreenTest.kt | 39 +++++-
.../importlogins/ImportLoginsViewModelTest.kt | 45 ++++++-
8 files changed, 210 insertions(+), 48 deletions(-)
diff --git a/app/src/main/java/com/x8bit/bitwarden/data/vault/repository/VaultRepositoryImpl.kt b/app/src/main/java/com/x8bit/bitwarden/data/vault/repository/VaultRepositoryImpl.kt
index 552ede5353..edc6d259de 100644
--- a/app/src/main/java/com/x8bit/bitwarden/data/vault/repository/VaultRepositoryImpl.kt
+++ b/app/src/main/java/com/x8bit/bitwarden/data/vault/repository/VaultRepositoryImpl.kt
@@ -1336,7 +1336,12 @@ class VaultRepositoryImpl(
userId = userId,
lastSyncTime = clock.instant(),
)
- return SyncVaultDataResult.Success
+ val itemsAvailable = vaultDiskSource
+ .getCiphers(userId)
+ .firstOrNull()
+ ?.isNotEmpty()
+ ?: false
+ return SyncVaultDataResult.Success(itemsAvailable = itemsAvailable)
}
},
onFailure = {
@@ -1381,7 +1386,8 @@ class VaultRepositoryImpl(
lastSyncTime = clock.instant(),
)
vaultDiskSource.replaceVaultData(userId = userId, vault = syncResponse)
- return SyncVaultDataResult.Success
+ val itemsAvailable = syncResponse.ciphers?.isNotEmpty() ?: false
+ return SyncVaultDataResult.Success(itemsAvailable = itemsAvailable)
},
onFailure = { throwable ->
updateVaultStateFlowsToError(throwable)
diff --git a/app/src/main/java/com/x8bit/bitwarden/data/vault/repository/model/SyncVaultDataResult.kt b/app/src/main/java/com/x8bit/bitwarden/data/vault/repository/model/SyncVaultDataResult.kt
index cbccb7db75..d38d9c9af3 100644
--- a/app/src/main/java/com/x8bit/bitwarden/data/vault/repository/model/SyncVaultDataResult.kt
+++ b/app/src/main/java/com/x8bit/bitwarden/data/vault/repository/model/SyncVaultDataResult.kt
@@ -6,8 +6,10 @@ package com.x8bit.bitwarden.data.vault.repository.model
sealed class SyncVaultDataResult {
/**
* Indicates a successful sync operation.
+ *
+ * @property itemsAvailable indicated whether the sync returned any vault items or not.
*/
- data object Success : SyncVaultDataResult()
+ data class Success(val itemsAvailable: Boolean) : SyncVaultDataResult()
/**
* Indicates a failed sync operation.
diff --git a/app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/importlogins/ImportLoginsScreen.kt b/app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/importlogins/ImportLoginsScreen.kt
index d5ff4e4823..e8b43d68f7 100644
--- a/app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/importlogins/ImportLoginsScreen.kt
+++ b/app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/importlogins/ImportLoginsScreen.kt
@@ -222,7 +222,7 @@ private fun ImportLoginsDialogContent(
)
}
- ImportLoginsState.DialogState.Error -> {
+ is ImportLoginsState.DialogState.Error -> {
BitwardenTwoButtonDialog(
title = dialogState.title?.invoke(),
message = dialogState.message(),
diff --git a/app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/importlogins/ImportLoginsViewModel.kt b/app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/importlogins/ImportLoginsViewModel.kt
index 9d00e9148b..04c5731aff 100644
--- a/app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/importlogins/ImportLoginsViewModel.kt
+++ b/app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/importlogins/ImportLoginsViewModel.kt
@@ -82,22 +82,33 @@ class ImportLoginsViewModel @Inject constructor(
private fun handleVaultSyncResultReceived(
action: ImportLoginsAction.Internal.VaultSyncResultReceived,
) {
- when (action.result) {
+ when (val result = action.result) {
is SyncVaultDataResult.Error -> {
mutableStateFlow.update {
it.copy(
isVaultSyncing = false,
- dialogState = ImportLoginsState.DialogState.Error,
+ dialogState = ImportLoginsState.DialogState.Error(),
)
}
}
- SyncVaultDataResult.Success -> {
- mutableStateFlow.update {
- it.copy(
- showBottomSheet = true,
- isVaultSyncing = false,
- )
+ is SyncVaultDataResult.Success -> {
+ if (result.itemsAvailable) {
+ mutableStateFlow.update {
+ it.copy(
+ showBottomSheet = true,
+ isVaultSyncing = false,
+ )
+ }
+ } else {
+ mutableStateFlow.update {
+ it.copy(
+ isVaultSyncing = false,
+ dialogState = ImportLoginsState.DialogState.Error(
+ R.string.no_logins_were_imported.asText(),
+ ),
+ )
+ }
}
}
}
@@ -215,9 +226,10 @@ data class ImportLoginsState(
/**
* Show a dialog with an error message.
*/
- data object Error : DialogState() {
+ data class Error(
+ override val message: Text = R.string.generic_error_message.asText(),
+ ) : DialogState() {
override val title: Text? = null
- override val message: Text = R.string.generic_error_message.asText()
}
}
diff --git a/app/src/main/res/values/strings.xml b/app/src/main/res/values/strings.xml
index 8580c39963..50e9c553b0 100644
--- a/app/src/main/res/values/strings.xml
+++ b/app/src/main/res/values/strings.xml
@@ -1072,4 +1072,5 @@ Do you want to switch to this account?
Manage your logins from anywhere with Bitwarden tools for web and desktop.
Bitwarden Tools
Got it
+ No logins were imported
diff --git a/app/src/test/java/com/x8bit/bitwarden/data/vault/repository/VaultRepositoryTest.kt b/app/src/test/java/com/x8bit/bitwarden/data/vault/repository/VaultRepositoryTest.kt
index 2455bbaf5f..d7678350f1 100644
--- a/app/src/test/java/com/x8bit/bitwarden/data/vault/repository/VaultRepositoryTest.kt
+++ b/app/src/test/java/com/x8bit/bitwarden/data/vault/repository/VaultRepositoryTest.kt
@@ -160,8 +160,11 @@ class VaultRepositoryTest {
private val sendsService: SendsService = mockk()
private val ciphersService: CiphersService = mockk()
private val folderService: FolderService = mockk()
+ private val mutableGetCiphersFlow: MutableStateFlow> =
+ MutableStateFlow(listOf(createMockCipher(1)))
private val vaultDiskSource: VaultDiskSource = mockk {
coEvery { resyncVaultData(any()) } just runs
+ every { getCiphers(any()) } returns mutableGetCiphersFlow
}
private val totpCodeManager: TotpCodeManager = mockk()
private val vaultSdkSource: VaultSdkSource = mockk {
@@ -4393,39 +4396,77 @@ class VaultRepositoryTest {
}
}
+ @Suppress("MaxLineLength")
@Test
- fun `syncForResult should return result`() = runTest {
- fakeAuthDiskSource.userState = MOCK_USER_STATE
- val userId = "mockId-1"
- val mockSyncResponse = createMockSyncResponse(number = 1)
- coEvery {
- syncService.sync()
- } returns mockSyncResponse.asSuccess()
- coEvery {
- vaultSdkSource.initializeOrganizationCrypto(
- userId = userId,
- request = InitOrgCryptoRequest(
- organizationKeys = createMockOrganizationKeys(1),
- ),
- )
- } returns InitializeCryptoResult.Success.asSuccess()
- coEvery {
- vaultDiskSource.replaceVaultData(
- userId = MOCK_USER_STATE.activeUserId,
- vault = mockSyncResponse,
- )
- } just runs
+ fun `syncForResult should return success result with itemsAvailable = true when sync succeeds and ciphers list is not empty`() =
+ runTest {
+ fakeAuthDiskSource.userState = MOCK_USER_STATE
+ val userId = "mockId-1"
+ val mockSyncResponse = createMockSyncResponse(number = 1)
+ coEvery {
+ syncService.sync()
+ } returns mockSyncResponse.asSuccess()
+ coEvery {
+ vaultSdkSource.initializeOrganizationCrypto(
+ userId = userId,
+ request = InitOrgCryptoRequest(
+ organizationKeys = createMockOrganizationKeys(1),
+ ),
+ )
+ } returns InitializeCryptoResult.Success.asSuccess()
+ coEvery {
+ vaultDiskSource.replaceVaultData(
+ userId = MOCK_USER_STATE.activeUserId,
+ vault = mockSyncResponse,
+ )
+ } just runs
- every {
- settingsDiskSource.storeLastSyncTime(
- MOCK_USER_STATE.activeUserId,
- clock.instant(),
- )
- } just runs
+ every {
+ settingsDiskSource.storeLastSyncTime(
+ MOCK_USER_STATE.activeUserId,
+ clock.instant(),
+ )
+ } just runs
- val syncResult = vaultRepository.syncForResult()
- assertEquals(SyncVaultDataResult.Success, syncResult)
- }
+ val syncResult = vaultRepository.syncForResult()
+ assertEquals(SyncVaultDataResult.Success(itemsAvailable = true), syncResult)
+ }
+
+ @Suppress("MaxLineLength")
+ @Test
+ fun `syncForResult should return success result with itemsAvailable = false when sync succeeds and ciphers list is empty`() =
+ runTest {
+ fakeAuthDiskSource.userState = MOCK_USER_STATE
+ val userId = "mockId-1"
+ val mockSyncResponse = createMockSyncResponse(number = 1).copy(ciphers = emptyList())
+ coEvery {
+ syncService.sync()
+ } returns mockSyncResponse.asSuccess()
+ coEvery {
+ vaultSdkSource.initializeOrganizationCrypto(
+ userId = userId,
+ request = InitOrgCryptoRequest(
+ organizationKeys = createMockOrganizationKeys(1),
+ ),
+ )
+ } returns InitializeCryptoResult.Success.asSuccess()
+ coEvery {
+ vaultDiskSource.replaceVaultData(
+ userId = MOCK_USER_STATE.activeUserId,
+ vault = mockSyncResponse,
+ )
+ } just runs
+
+ every {
+ settingsDiskSource.storeLastSyncTime(
+ MOCK_USER_STATE.activeUserId,
+ clock.instant(),
+ )
+ } just runs
+
+ val syncResult = vaultRepository.syncForResult()
+ assertEquals(SyncVaultDataResult.Success(itemsAvailable = false), syncResult)
+ }
@Test
fun `syncForResult should return error when getAccountRevisionDateMillis fails`() =
@@ -4456,6 +4497,30 @@ class VaultRepositoryTest {
)
}
+ @Suppress("MaxLineLength")
+ @Test
+ fun `syncForResult when the last sync time is more recent than the revision date should return result from disk source data`() =
+ runTest {
+ val userId = "mockId-1"
+ fakeAuthDiskSource.userState = MOCK_USER_STATE
+ every {
+ settingsDiskSource.getLastSyncTime(userId = userId)
+ } returns clock.instant().plus(2, ChronoUnit.MINUTES)
+ mutableGetCiphersFlow.update { emptyList() }
+ val result = vaultRepository.syncForResult()
+ assertEquals(
+ SyncVaultDataResult.Success(itemsAvailable = false),
+ result,
+ )
+ verify(exactly = 1) {
+ settingsDiskSource.storeLastSyncTime(
+ userId = userId,
+ lastSyncTime = clock.instant(),
+ )
+ }
+ coVerify(exactly = 0) { syncService.sync() }
+ }
+
//region Helper functions
/**
diff --git a/app/src/test/java/com/x8bit/bitwarden/ui/vault/feature/importlogins/ImportLoginsScreenTest.kt b/app/src/test/java/com/x8bit/bitwarden/ui/vault/feature/importlogins/ImportLoginsScreenTest.kt
index dca296a327..5b1fd9cb19 100644
--- a/app/src/test/java/com/x8bit/bitwarden/ui/vault/feature/importlogins/ImportLoginsScreenTest.kt
+++ b/app/src/test/java/com/x8bit/bitwarden/ui/vault/feature/importlogins/ImportLoginsScreenTest.kt
@@ -17,9 +17,11 @@ import androidx.compose.ui.test.performScrollTo
import androidx.compose.ui.test.performSemanticsAction
import androidx.compose.ui.test.printToLog
import androidx.core.net.toUri
+import com.x8bit.bitwarden.R
import com.x8bit.bitwarden.data.platform.repository.util.bufferedMutableSharedFlow
import com.x8bit.bitwarden.data.util.advanceTimeByAndRunCurrent
import com.x8bit.bitwarden.ui.platform.base.BaseComposeTest
+import com.x8bit.bitwarden.ui.platform.base.util.asText
import com.x8bit.bitwarden.ui.platform.manager.intent.IntentManager
import com.x8bit.bitwarden.ui.util.assertNoDialogExists
import io.mockk.every
@@ -342,7 +344,7 @@ class ImportLoginsScreenTest : BaseComposeTest() {
fun `Error dialog is displayed when dialog state is Error`() {
mutableImportLoginsStateFlow.update {
it.copy(
- dialogState = ImportLoginsState.DialogState.Error,
+ dialogState = ImportLoginsState.DialogState.Error(),
)
}
composeTestRule
@@ -371,6 +373,41 @@ class ImportLoginsScreenTest : BaseComposeTest() {
verifyActionSent(ImportLoginsAction.FailedSyncAcknowledged)
}
+ @Test
+ fun `Error dialog is displayed when dialog state is Error for no logins`() {
+ mutableImportLoginsStateFlow.tryEmit(
+ DEFAULT_STATE.copy(
+ dialogState = ImportLoginsState.DialogState.Error(
+ message = R.string.no_logins_were_imported.asText(),
+ ),
+ ),
+ )
+ composeTestRule
+ .onNode(isDialog())
+ .assertIsDisplayed()
+ composeTestRule
+ .onAllNodesWithText(
+ text = "No logins were imported",
+ useUnmergedTree = true,
+ )
+ .filterToOne(hasAnyAncestor(isDialog()))
+ .assertIsDisplayed()
+
+ composeTestRule
+ .onAllNodesWithText("Try again")
+ .filterToOne(hasAnyAncestor(isDialog()))
+ .assertIsDisplayed()
+ .performClick()
+ verifyActionSent(ImportLoginsAction.RetryVaultSync)
+
+ composeTestRule
+ .onAllNodesWithText("Ok")
+ .filterToOne(hasAnyAncestor(isDialog()))
+ .assertIsDisplayed()
+ .performClick()
+ verifyActionSent(ImportLoginsAction.FailedSyncAcknowledged)
+ }
+
@Test
fun `Success bottom sheet is shown when state is updated`() {
mutableImportLoginsStateFlow.update {
diff --git a/app/src/test/java/com/x8bit/bitwarden/ui/vault/feature/importlogins/ImportLoginsViewModelTest.kt b/app/src/test/java/com/x8bit/bitwarden/ui/vault/feature/importlogins/ImportLoginsViewModelTest.kt
index 30f8858693..6885b65444 100644
--- a/app/src/test/java/com/x8bit/bitwarden/ui/vault/feature/importlogins/ImportLoginsViewModelTest.kt
+++ b/app/src/test/java/com/x8bit/bitwarden/ui/vault/feature/importlogins/ImportLoginsViewModelTest.kt
@@ -2,9 +2,11 @@ package com.x8bit.bitwarden.ui.vault.feature.importlogins
import app.cash.turbine.test
import app.cash.turbine.turbineScope
+import com.x8bit.bitwarden.R
import com.x8bit.bitwarden.data.vault.repository.VaultRepository
import com.x8bit.bitwarden.data.vault.repository.model.SyncVaultDataResult
import com.x8bit.bitwarden.ui.platform.base.BaseViewModelTest
+import com.x8bit.bitwarden.ui.platform.base.util.asText
import io.mockk.coEvery
import io.mockk.coVerify
import io.mockk.mockk
@@ -17,7 +19,7 @@ import org.junit.jupiter.api.Test
class ImportLoginsViewModelTest : BaseViewModelTest() {
private val vaultRepository: VaultRepository = mockk() {
- coEvery { syncForResult() } returns SyncVaultDataResult.Success
+ coEvery { syncForResult() } returns SyncVaultDataResult.Success(itemsAvailable = true)
}
@Test
@@ -278,7 +280,9 @@ class ImportLoginsViewModelTest : BaseViewModelTest() {
viewModel.trySendAction(ImportLoginsAction.MoveToSyncInProgress)
viewModel.stateFlow.test {
assertNotNull(awaitItem().dialogState)
- coEvery { vaultRepository.syncForResult() } returns SyncVaultDataResult.Success
+ coEvery { vaultRepository.syncForResult() } returns SyncVaultDataResult.Success(
+ itemsAvailable = true,
+ )
viewModel.trySendAction(ImportLoginsAction.RetryVaultSync)
assertEquals(
ImportLoginsState(
@@ -309,6 +313,41 @@ class ImportLoginsViewModelTest : BaseViewModelTest() {
)
}
+ @Suppress("MaxLineLength")
+ @Test
+ fun `MoveToSyncInProgress should set no items imported error dialog state when sync succeeds but no items are available`() =
+ runTest {
+ coEvery {
+ vaultRepository.syncForResult()
+ } returns SyncVaultDataResult.Success(itemsAvailable = false)
+ val viewModel = createViewModel()
+ viewModel.stateFlow.test {
+ assertEquals(
+ DEFAULT_STATE,
+ awaitItem(),
+ )
+ viewModel.trySendAction(ImportLoginsAction.MoveToSyncInProgress)
+ assertEquals(
+ ImportLoginsState(
+ dialogState = null,
+ viewState = ImportLoginsState.ViewState.InitialContent,
+ isVaultSyncing = true,
+ showBottomSheet = false,
+ ),
+ awaitItem(),
+ )
+ assertEquals(
+ ImportLoginsState(
+ dialogState = ImportLoginsState.DialogState.Error(R.string.no_logins_were_imported.asText()),
+ viewState = ImportLoginsState.ViewState.InitialContent,
+ isVaultSyncing = false,
+ showBottomSheet = false,
+ ),
+ awaitItem(),
+ )
+ }
+ }
+
@Test
fun `SyncVaultDataResult Error should remove loading state and show error dialog`() = runTest {
coEvery { vaultRepository.syncForResult() } returns SyncVaultDataResult.Error(Exception())
@@ -316,7 +355,7 @@ class ImportLoginsViewModelTest : BaseViewModelTest() {
viewModel.trySendAction(ImportLoginsAction.MoveToSyncInProgress)
assertEquals(
ImportLoginsState(
- dialogState = ImportLoginsState.DialogState.Error,
+ dialogState = ImportLoginsState.DialogState.Error(),
viewState = ImportLoginsState.ViewState.InitialContent,
isVaultSyncing = false,
showBottomSheet = false,