From 25bbe792304411a4de73049207767b8ed8fe077a Mon Sep 17 00:00:00 2001 From: Oleg Semenenko <146032743+oleg-livefront@users.noreply.github.com> Date: Fri, 8 Dec 2023 12:50:34 -0600 Subject: [PATCH] BIT-890 Adding a check for name when we save an item (#355) --- .../feature/additem/VaultAddItemScreen.kt | 17 +++- .../feature/additem/VaultAddItemViewModel.kt | 93 +++++++++++------- .../feature/additem/VaultAddItemScreenTest.kt | 52 +++++++++- .../additem/VaultAddItemViewModelTest.kt | 94 +++++++++++++++++-- 4 files changed, 212 insertions(+), 44 deletions(-) diff --git a/app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/additem/VaultAddItemScreen.kt b/app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/additem/VaultAddItemScreen.kt index bf5fc86c60..3b4d044037 100644 --- a/app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/additem/VaultAddItemScreen.kt +++ b/app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/additem/VaultAddItemScreen.kt @@ -25,6 +25,9 @@ import androidx.hilt.navigation.compose.hiltViewModel import androidx.lifecycle.compose.collectAsStateWithLifecycle import com.x8bit.bitwarden.R import com.x8bit.bitwarden.ui.platform.base.util.EventsEffect +import com.x8bit.bitwarden.ui.platform.base.util.asText +import com.x8bit.bitwarden.ui.platform.components.BasicDialogState +import com.x8bit.bitwarden.ui.platform.components.BitwardenBasicDialog import com.x8bit.bitwarden.ui.platform.components.BitwardenListHeaderText import com.x8bit.bitwarden.ui.platform.components.BitwardenLoadingDialog import com.x8bit.bitwarden.ui.platform.components.BitwardenMultiSelectButton @@ -72,6 +75,16 @@ fun VaultAddItemScreen( ) } + is VaultAddItemState.DialogState.Error -> BitwardenBasicDialog( + visibilityState = BasicDialogState.Shown( + title = R.string.an_error_has_occurred.asText(), + message = dialogState.message, + ), + onDismissRequest = remember(viewModel) { + { viewModel.trySendAction(VaultAddItemAction.DismissDialog) } + }, + ) + null -> Unit } @@ -139,11 +152,11 @@ fun VaultAddItemScreen( ) } - VaultAddItemState.ItemType.Card -> { + is VaultAddItemState.ItemType.Card -> { // TODO(BIT-507): Create UI for card-type item creation } - VaultAddItemState.ItemType.Identity -> { + is VaultAddItemState.ItemType.Identity -> { // TODO(BIT-667): Create UI for identity-type item creation } diff --git a/app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/additem/VaultAddItemViewModel.kt b/app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/additem/VaultAddItemViewModel.kt index 936125e21d..bea90b66e0 100644 --- a/app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/additem/VaultAddItemViewModel.kt +++ b/app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/additem/VaultAddItemViewModel.kt @@ -9,8 +9,6 @@ import com.x8bit.bitwarden.data.vault.repository.model.CreateCipherResult import com.x8bit.bitwarden.ui.platform.base.BaseViewModel import com.x8bit.bitwarden.ui.platform.base.util.Text import com.x8bit.bitwarden.ui.platform.base.util.asText -import com.x8bit.bitwarden.ui.vault.feature.additem.VaultAddItemState.ItemType.Card.displayStringResId -import com.x8bit.bitwarden.ui.vault.feature.additem.VaultAddItemState.ItemType.Identity.displayStringResId import com.x8bit.bitwarden.ui.vault.feature.vault.util.toCipherView import com.x8bit.bitwarden.ui.vault.model.VaultAddEditType import dagger.hilt.android.lifecycle.HiltViewModel @@ -62,6 +60,10 @@ class VaultAddItemViewModel @Inject constructor( handleCloseClick() } + is VaultAddItemAction.DismissDialog -> { + handleDismissDialog() + } + is VaultAddItemAction.TypeOptionSelect -> { handleTypeOptionSelect(action) } @@ -85,6 +87,18 @@ class VaultAddItemViewModel @Inject constructor( //region Top Level Handlers private fun handleSaveClick() { + if (state.selectedType.name.isBlank()) { + mutableStateFlow.update { + it.copy( + dialog = VaultAddItemState.DialogState.Error( + R.string.validation_field_required + .asText(R.string.name.asText()), + ), + ) + } + return + } + mutableStateFlow.update { it.copy( dialog = VaultAddItemState.DialogState.Loading( @@ -94,27 +108,13 @@ class VaultAddItemViewModel @Inject constructor( } viewModelScope.launch { - when (state.selectedType) { - is VaultAddItemState.ItemType.Login, - is VaultAddItemState.ItemType.SecureNotes, - -> { - sendAction( - action = VaultAddItemAction.Internal.CreateCipherResultReceive( - createCipherResult = vaultRepository.createCipher( - cipherView = stateFlow.value.selectedType.toCipherView(), - ), - ), - ) - } - - VaultAddItemState.ItemType.Card -> { - // TODO Add Saving of Card Type (BIT-668) - } - - VaultAddItemState.ItemType.Identity -> { - // TODO Add Saving of Identity type (BIT-508) - } - } + sendAction( + action = VaultAddItemAction.Internal.CreateCipherResultReceive( + createCipherResult = vaultRepository.createCipher( + cipherView = stateFlow.value.selectedType.toCipherView(), + ), + ), + ) } } @@ -124,6 +124,12 @@ class VaultAddItemViewModel @Inject constructor( ) } + private fun handleDismissDialog() { + mutableStateFlow.update { + it.copy(dialog = null) + } + } + //endregion Top Level Handlers //region Type Option Handlers @@ -156,7 +162,7 @@ class VaultAddItemViewModel @Inject constructor( private fun handleSwitchToAddCardItem() { mutableStateFlow.update { currentState -> currentState.copy( - selectedType = VaultAddItemState.ItemType.Card, + selectedType = VaultAddItemState.ItemType.Card(), ) } } @@ -164,7 +170,7 @@ class VaultAddItemViewModel @Inject constructor( private fun handleSwitchToAddIdentityItem() { mutableStateFlow.update { currentState -> currentState.copy( - selectedType = VaultAddItemState.ItemType.Identity, + selectedType = VaultAddItemState.ItemType.Identity(), ) } } @@ -633,10 +639,15 @@ data class VaultAddItemState( */ abstract val displayStringResId: Int + /** + * Represents the name for the item type. This is an abstract property + * that must be overridden to save the item + */ + abstract val name: String + /** * Represents the login item information. * - * @property name The name associated with the login item. * @property username The username required for the login item. * @property password The password required for the login item. * @property uri The URI associated with the login item. @@ -650,7 +661,7 @@ data class VaultAddItemState( */ @Parcelize data class Login( - val name: String = "", + override val name: String = "", val username: String = "", val password: String = "", val uri: String = "", @@ -679,22 +690,24 @@ data class VaultAddItemState( /** * Represents the `Card` item type. - * - * @property displayStringResId Resource ID for the display string of the card type. */ @Parcelize - data object Card : ItemType() { + data class Card( + // TODO create the Card Item (BIT-509) + override val name: String = "", + ) : ItemType() { override val displayStringResId: Int get() = ItemTypeOption.CARD.labelRes } /** * Represents the `Identity` item type. - * - * @property displayStringResId Resource ID for the display string of the identity type. */ @Parcelize - data object Identity : ItemType() { + data class Identity( + // TODO create the Identity Item (BIT-667) + override val name: String = "", + ) : ItemType() { override val displayStringResId: Int get() = ItemTypeOption.IDENTITY.labelRes } @@ -702,7 +715,6 @@ data class VaultAddItemState( /** * Represents the `SecureNotes` item type. * - * @property name The name associated with the SecureNotes item. * @property folder The folder used for the SecureNotes item * @property favorite Indicates whether this SecureNotes item is marked as a favorite. * @property masterPasswordReprompt Indicates if a master password reprompt is required. @@ -713,7 +725,7 @@ data class VaultAddItemState( */ @Parcelize data class SecureNotes( - val name: String = "", + override val name: String = "", val folderName: Text = DEFAULT_FOLDER, val favorite: Boolean = false, val masterPasswordReprompt: Boolean = false, @@ -747,6 +759,12 @@ data class VaultAddItemState( */ @Parcelize data class Loading(val label: Text) : DialogState() + + /** + * Displays an error dialog to the user. + */ + @Parcelize + data class Error(val message: Text) : DialogState() } } @@ -782,6 +800,11 @@ sealed class VaultAddItemAction { */ data object CloseClick : VaultAddItemAction() + /** + * The user has clicked to dismiss the dialog. + */ + data object DismissDialog : VaultAddItemAction() + /** * Represents the action when a type option is selected. * diff --git a/app/src/test/java/com/x8bit/bitwarden/ui/vault/feature/additem/VaultAddItemScreenTest.kt b/app/src/test/java/com/x8bit/bitwarden/ui/vault/feature/additem/VaultAddItemScreenTest.kt index 464444672d..7d62882ffe 100644 --- a/app/src/test/java/com/x8bit/bitwarden/ui/vault/feature/additem/VaultAddItemScreenTest.kt +++ b/app/src/test/java/com/x8bit/bitwarden/ui/vault/feature/additem/VaultAddItemScreenTest.kt @@ -2,13 +2,16 @@ package com.x8bit.bitwarden.ui.vault.feature.additem import androidx.compose.ui.geometry.Offset import androidx.compose.ui.test.assertIsDisplayed +import androidx.compose.ui.test.assertIsNotDisplayed import androidx.compose.ui.test.assertIsOff import androidx.compose.ui.test.assertIsOn import androidx.compose.ui.test.assertTextContains import androidx.compose.ui.test.click import androidx.compose.ui.test.filterToOne +import androidx.compose.ui.test.hasAnyAncestor import androidx.compose.ui.test.hasContentDescription import androidx.compose.ui.test.hasSetTextAction +import androidx.compose.ui.test.isDialog import androidx.compose.ui.test.onAllNodesWithText import androidx.compose.ui.test.onFirst import androidx.compose.ui.test.onLast @@ -77,6 +80,47 @@ class VaultAddItemScreenTest : BaseComposeTest() { } } + @Test + fun `clicking dismiss dialog button should send DismissDialog action`() { + mutableStateFlow.value = DEFAULT_STATE_LOGIN_DIALOG + + composeTestRule.setContent { + VaultAddItemScreen(viewModel = viewModel, onNavigateBack = {}) + } + + composeTestRule + .onAllNodesWithText("Ok") + .filterToOne(hasAnyAncestor(isDialog())) + .performClick() + + verify { + viewModel.trySendAction( + VaultAddItemAction.DismissDialog, + ) + } + } + + @Test + fun `dialog should display when state is updated to do so`() { + mutableStateFlow.value = DEFAULT_STATE_LOGIN + + composeTestRule.setContent { + VaultAddItemScreen(viewModel = viewModel, onNavigateBack = {}) + } + + composeTestRule + .onAllNodesWithText("Ok") + .filterToOne(hasAnyAncestor(isDialog())) + .assertIsNotDisplayed() + + mutableStateFlow.value = DEFAULT_STATE_LOGIN_DIALOG + + composeTestRule + .onAllNodesWithText("Ok") + .filterToOne(hasAnyAncestor(isDialog())) + .assertIsDisplayed() + } + @Test fun `clicking a Type Option should send TypeOptionSelect action`() { composeTestRule.setContent { @@ -112,7 +156,7 @@ class VaultAddItemScreenTest : BaseComposeTest() { .onNodeWithContentDescriptionAfterScroll(label = "Type, Login") .assertIsDisplayed() - mutableStateFlow.update { it.copy(selectedType = VaultAddItemState.ItemType.Card) } + mutableStateFlow.update { it.copy(selectedType = VaultAddItemState.ItemType.Card()) } composeTestRule .onNodeWithContentDescriptionAfterScroll(label = "Type, Card") @@ -960,6 +1004,12 @@ class VaultAddItemScreenTest : BaseComposeTest() { //endregion Helper functions companion object { + private val DEFAULT_STATE_LOGIN_DIALOG = VaultAddItemState( + selectedType = VaultAddItemState.ItemType.Login(), + dialog = VaultAddItemState.DialogState.Error("test".asText()), + vaultAddEditType = VaultAddEditType.AddItem, + ) + private val DEFAULT_STATE_LOGIN = VaultAddItemState( vaultAddEditType = VaultAddEditType.AddItem, selectedType = VaultAddItemState.ItemType.Login(), diff --git a/app/src/test/java/com/x8bit/bitwarden/ui/vault/feature/additem/VaultAddItemViewModelTest.kt b/app/src/test/java/com/x8bit/bitwarden/ui/vault/feature/additem/VaultAddItemViewModelTest.kt index 955d5b07e7..6874a5fc83 100644 --- a/app/src/test/java/com/x8bit/bitwarden/ui/vault/feature/additem/VaultAddItemViewModelTest.kt +++ b/app/src/test/java/com/x8bit/bitwarden/ui/vault/feature/additem/VaultAddItemViewModelTest.kt @@ -59,26 +59,47 @@ class VaultAddItemViewModelTest : BaseViewModelTest() { @Test fun `SaveClick should show dialog, and remove it once an item is saved`() = runTest { val stateWithDialog = createVaultAddLoginItemState( + name = "tester", dialogState = VaultAddItemState.DialogState.Loading( R.string.saving.asText(), ), ) - val viewModel = createAddVaultItemViewModel() + val stateWithName = createVaultAddLoginItemState( + name = "tester", + ) + + val viewModel = createAddVaultItemViewModel( + createSavedStateHandleWithState( + state = stateWithName, + vaultAddEditType = VaultAddEditType.AddItem, + ), + ) + coEvery { vaultRepository.createCipher(any()) } returns CreateCipherResult.Success viewModel.stateFlow.test { viewModel.actionChannel.trySend(VaultAddItemAction.SaveClick) - assertEquals(initialState, awaitItem()) + assertEquals(stateWithName, awaitItem()) assertEquals(stateWithDialog, awaitItem()) - assertEquals(initialState, awaitItem()) + assertEquals(stateWithName, awaitItem()) } } @Test fun `SaveClick should update value to loading`() = runTest { - val viewModel = createAddVaultItemViewModel() + val stateWithName = createVaultAddLoginItemState( + name = "tester", + ) + + val viewModel = createAddVaultItemViewModel( + createSavedStateHandleWithState( + state = stateWithName, + vaultAddEditType = VaultAddEditType.AddItem, + ), + ) + coEvery { vaultRepository.createCipher(any()) } returns CreateCipherResult.Success @@ -90,7 +111,17 @@ class VaultAddItemViewModelTest : BaseViewModelTest() { @Test fun `SaveClick createCipher error should emit ShowToast`() = runTest { - val viewModel = createAddVaultItemViewModel() + val stateWithName = createVaultAddLoginItemState( + name = "tester", + ) + + val viewModel = createAddVaultItemViewModel( + createSavedStateHandleWithState( + state = stateWithName, + vaultAddEditType = VaultAddEditType.AddItem, + ), + ) + coEvery { vaultRepository.createCipher(any()) } returns CreateCipherResult.Error @@ -100,6 +131,56 @@ class VaultAddItemViewModelTest : BaseViewModelTest() { } } + @Test + fun `Saving item with an empty name field will cause a dialog to show up`() = runTest { + val stateWithNoName = createVaultAddSecureNotesItemState(name = "") + + val stateWithNoNameAndDialog = createVaultAddSecureNotesItemState( + name = "", + dialogState = VaultAddItemState.DialogState.Error( + R.string.validation_field_required + .asText(R.string.name.asText()), + ), + ) + + val viewModel = createAddVaultItemViewModel( + createSavedStateHandleWithState( + state = stateWithNoName, + vaultAddEditType = VaultAddEditType.AddItem, + ), + ) + coEvery { vaultRepository.createCipher(any()) } returns CreateCipherResult.Success + viewModel.stateFlow.test { + viewModel.actionChannel.trySend(VaultAddItemAction.SaveClick) + assertEquals(stateWithNoName, awaitItem()) + assertEquals(stateWithNoNameAndDialog, awaitItem()) + } + } + + @Test + fun `HandleDialogDismiss will remove the current dialog`() = runTest { + val errorState = createVaultAddLoginItemState( + dialogState = VaultAddItemState.DialogState.Error( + R.string.validation_field_required + .asText(R.string.name.asText()), + ), + ) + + val viewModel = createAddVaultItemViewModel( + createSavedStateHandleWithState( + state = errorState, + vaultAddEditType = VaultAddEditType.AddItem, + ), + ) + + coEvery { vaultRepository.createCipher(any()) } returns CreateCipherResult.Success + viewModel.stateFlow.test { + viewModel.actionChannel.trySend(VaultAddItemAction.DismissDialog) + assertEquals(errorState, awaitItem()) + assertEquals(null, awaitItem().dialog) + } + } + @Test fun `TypeOptionSelect LOGIN should switch to LoginItem`() = runTest { val viewModel = createAddVaultItemViewModel() @@ -572,6 +653,7 @@ class VaultAddItemViewModelTest : BaseViewModelTest() { masterPasswordReprompt: Boolean = false, notes: String = "", ownership: String = "placeholder@email.com", + dialogState: VaultAddItemState.DialogState? = null, ): VaultAddItemState = VaultAddItemState( vaultAddEditType = VaultAddEditType.AddItem, @@ -583,7 +665,7 @@ class VaultAddItemViewModelTest : BaseViewModelTest() { notes = notes, ownership = ownership, ), - dialog = null, + dialog = dialogState, ) private fun createSavedStateHandleWithState(