From 04d60a50ff938210e06cb9a2397decf3774319f8 Mon Sep 17 00:00:00 2001 From: David Perez Date: Wed, 24 Jan 2024 19:10:22 -0600 Subject: [PATCH] Add support for deleting an attachment (#768) --- .../feature/attachments/AttachmentsScreen.kt | 31 ++++++ .../attachments/AttachmentsViewModel.kt | 94 ++++++++++++++++++- .../handlers/AttachmentsHandlers.kt | 4 + .../attachments/util/CipherViewExtensions.kt | 1 + .../attachments/AttachmentsScreenTest.kt | 54 ++++++++++- .../attachments/AttachmentsViewModelTest.kt | 88 +++++++++++++++-- .../util/CipherViewExtensionsTest.kt | 11 ++- 7 files changed, 266 insertions(+), 17 deletions(-) diff --git a/app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/attachments/AttachmentsScreen.kt b/app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/attachments/AttachmentsScreen.kt index 824e52cee0..2a697a7087 100644 --- a/app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/attachments/AttachmentsScreen.kt +++ b/app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/attachments/AttachmentsScreen.kt @@ -18,11 +18,15 @@ 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.components.BasicDialogState +import com.x8bit.bitwarden.ui.platform.components.BitwardenBasicDialog import com.x8bit.bitwarden.ui.platform.components.BitwardenErrorContent import com.x8bit.bitwarden.ui.platform.components.BitwardenLoadingContent +import com.x8bit.bitwarden.ui.platform.components.BitwardenLoadingDialog import com.x8bit.bitwarden.ui.platform.components.BitwardenScaffold import com.x8bit.bitwarden.ui.platform.components.BitwardenTextButton import com.x8bit.bitwarden.ui.platform.components.BitwardenTopAppBar +import com.x8bit.bitwarden.ui.platform.components.LoadingDialogState import com.x8bit.bitwarden.ui.platform.components.NavigationIcon import com.x8bit.bitwarden.ui.platform.manager.intent.IntentManager import com.x8bit.bitwarden.ui.platform.theme.LocalIntentManager @@ -65,6 +69,11 @@ fun AttachmentsScreen( } } + AttachmentsDialogs( + dialogState = state.dialogState, + onDismissRequest = attachmentsHandlers.onDismissRequest, + ) + val scrollBehavior = TopAppBarDefaults.pinnedScrollBehavior(rememberTopAppBarState()) BitwardenScaffold( modifier = Modifier @@ -109,3 +118,25 @@ fun AttachmentsScreen( } } } + +@Composable +private fun AttachmentsDialogs( + dialogState: AttachmentsState.DialogState?, + onDismissRequest: () -> Unit, +) { + when (dialogState) { + is AttachmentsState.DialogState.Error -> BitwardenBasicDialog( + visibilityState = BasicDialogState.Shown( + title = dialogState.title, + message = dialogState.message, + ), + onDismissRequest = onDismissRequest, + ) + + is AttachmentsState.DialogState.Loading -> BitwardenLoadingDialog( + visibilityState = LoadingDialogState.Shown(dialogState.message), + ) + + null -> Unit + } +} diff --git a/app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/attachments/AttachmentsViewModel.kt b/app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/attachments/AttachmentsViewModel.kt index 967aa15dcb..d07781d9d6 100644 --- a/app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/attachments/AttachmentsViewModel.kt +++ b/app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/attachments/AttachmentsViewModel.kt @@ -7,6 +7,7 @@ import com.bitwarden.core.CipherView import com.x8bit.bitwarden.R import com.x8bit.bitwarden.data.platform.repository.model.DataState import com.x8bit.bitwarden.data.vault.repository.VaultRepository +import com.x8bit.bitwarden.data.vault.repository.model.DeleteAttachmentResult 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 @@ -18,6 +19,8 @@ import kotlinx.coroutines.flow.launchIn import kotlinx.coroutines.flow.map import kotlinx.coroutines.flow.onEach import kotlinx.coroutines.flow.update +import kotlinx.coroutines.launch +import kotlinx.parcelize.IgnoredOnParcel import kotlinx.parcelize.Parcelize import javax.inject.Inject @@ -26,6 +29,7 @@ private const val KEY_STATE = "state" /** * ViewModel responsible for handling user interactions in the attachments screen. */ +@Suppress("TooManyFunctions") @HiltViewModel class AttachmentsViewModel @Inject constructor( private val vaultRepo: VaultRepository, @@ -36,6 +40,7 @@ class AttachmentsViewModel @Inject constructor( ?: AttachmentsState( cipherId = AttachmentsArgs(savedStateHandle).cipherId, viewState = AttachmentsState.ViewState.Loading, + dialogState = null, ), ) { init { @@ -50,6 +55,7 @@ class AttachmentsViewModel @Inject constructor( when (action) { AttachmentsAction.BackClick -> handleBackClick() AttachmentsAction.SaveClick -> handleSaveClick() + AttachmentsAction.DismissDialogClick -> handleDismissDialogClick() AttachmentsAction.ChooseFileClick -> handleChooseFileClick() is AttachmentsAction.FileChoose -> handleFileChoose(action) is AttachmentsAction.DeleteClick -> handleDeleteClick(action) @@ -66,6 +72,10 @@ class AttachmentsViewModel @Inject constructor( // TODO: Handle saving the attachments (BIT-522) } + private fun handleDismissDialogClick() { + mutableStateFlow.update { it.copy(dialogState = null) } + } + private fun handleChooseFileClick() { sendEvent(AttachmentsEvent.ShowChooserSheet) } @@ -76,13 +86,30 @@ class AttachmentsViewModel @Inject constructor( } private fun handleDeleteClick(action: AttachmentsAction.DeleteClick) { - sendEvent(AttachmentsEvent.ShowToast("Not Yet Implemented".asText())) - // TODO: Handle choosing a file the attachments (BIT-522) + onContent { content -> + val cipherView = content.originalCipher ?: return@onContent + mutableStateFlow.update { + it.copy( + dialogState = AttachmentsState.DialogState.Loading( + message = R.string.deleting.asText(), + ), + ) + } + viewModelScope.launch { + val result = vaultRepo.deleteCipherAttachment( + cipherId = state.cipherId, + attachmentId = action.attachmentId, + cipherView = cipherView, + ) + sendAction(AttachmentsAction.Internal.DeleteResultReceive(result)) + } + } } private fun handleInternalAction(action: AttachmentsAction.Internal) { when (action) { is AttachmentsAction.Internal.CipherReceive -> handleCipherReceive(action) + is AttachmentsAction.Internal.DeleteResultReceive -> handleDeleteResultReceive(action) } } @@ -142,6 +169,32 @@ class AttachmentsViewModel @Inject constructor( } } } + + private fun handleDeleteResultReceive(action: AttachmentsAction.Internal.DeleteResultReceive) { + when (action.result) { + DeleteAttachmentResult.Error -> { + mutableStateFlow.update { + it.copy( + dialogState = AttachmentsState.DialogState.Error( + title = R.string.an_error_has_occurred.asText(), + message = R.string.generic_error_message.asText(), + ), + ) + } + } + + DeleteAttachmentResult.Success -> { + mutableStateFlow.update { it.copy(dialogState = null) } + sendEvent(AttachmentsEvent.ShowToast(R.string.attachment_deleted.asText())) + } + } + } + + private inline fun onContent( + crossinline block: (AttachmentsState.ViewState.Content) -> Unit, + ) { + (state.viewState as? AttachmentsState.ViewState.Content)?.let(block) + } } /** @@ -151,6 +204,7 @@ class AttachmentsViewModel @Inject constructor( data class AttachmentsState( val cipherId: String, val viewState: ViewState, + val dialogState: DialogState?, ) : Parcelable { /** * Represents the specific view states for the [AttachmentsScreen]. @@ -174,6 +228,8 @@ data class AttachmentsState( */ @Parcelize data class Content( + @IgnoredOnParcel + val originalCipher: CipherView? = null, val attachments: List, ) : ViewState() } @@ -187,6 +243,28 @@ data class AttachmentsState( val title: String, val displaySize: String, ) : Parcelable + + /** + * Represents the current state of any dialogs on the screen. + */ + sealed class DialogState : Parcelable { + /** + * Represents a dismissible dialog with the given error [message]. + */ + @Parcelize + data class Error( + val title: Text?, + val message: Text, + ) : DialogState() + + /** + * Represents a loading dialog with the given [message]. + */ + @Parcelize + data class Loading( + val message: Text, + ) : DialogState() + } } /** @@ -225,6 +303,11 @@ sealed class AttachmentsAction { */ data object SaveClick : AttachmentsAction() + /** + * User clicked to dismiss a dialog. + */ + data object DismissDialogClick : AttachmentsAction() + /** * User clicked to select a new attachment file. */ @@ -254,5 +337,12 @@ sealed class AttachmentsAction { data class CipherReceive( val cipherDataState: DataState, ) : Internal() + + /** + * The result of deleting the attachment. + */ + data class DeleteResultReceive( + val result: DeleteAttachmentResult, + ) : Internal() } } diff --git a/app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/attachments/handlers/AttachmentsHandlers.kt b/app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/attachments/handlers/AttachmentsHandlers.kt index 892a53dea3..bd372f11b6 100644 --- a/app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/attachments/handlers/AttachmentsHandlers.kt +++ b/app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/attachments/handlers/AttachmentsHandlers.kt @@ -13,6 +13,7 @@ data class AttachmentsHandlers( val onChooseFileClick: () -> Unit, val onFileChoose: (IntentManager.FileData) -> Unit, val onDeleteClick: (attachmentId: String) -> Unit, + val onDismissRequest: () -> Unit, ) { companion object { /** @@ -30,6 +31,9 @@ data class AttachmentsHandlers( onDeleteClick = { viewModel.trySendAction(AttachmentsAction.DeleteClick(it)) }, + onDismissRequest = { + viewModel.trySendAction(AttachmentsAction.DismissDialogClick) + }, ) } } diff --git a/app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/attachments/util/CipherViewExtensions.kt b/app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/attachments/util/CipherViewExtensions.kt index 57f5a2ee3c..ec6b16db4a 100644 --- a/app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/attachments/util/CipherViewExtensions.kt +++ b/app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/attachments/util/CipherViewExtensions.kt @@ -8,6 +8,7 @@ import com.x8bit.bitwarden.ui.vault.feature.attachments.AttachmentsState */ fun CipherView.toViewState(): AttachmentsState.ViewState.Content = AttachmentsState.ViewState.Content( + originalCipher = this, attachments = this .attachments .orEmpty() diff --git a/app/src/test/java/com/x8bit/bitwarden/ui/vault/feature/attachments/AttachmentsScreenTest.kt b/app/src/test/java/com/x8bit/bitwarden/ui/vault/feature/attachments/AttachmentsScreenTest.kt index efe3e31bb9..d64d454322 100644 --- a/app/src/test/java/com/x8bit/bitwarden/ui/vault/feature/attachments/AttachmentsScreenTest.kt +++ b/app/src/test/java/com/x8bit/bitwarden/ui/vault/feature/attachments/AttachmentsScreenTest.kt @@ -11,6 +11,7 @@ import androidx.compose.ui.test.onNodeWithContentDescription import androidx.compose.ui.test.onNodeWithText import androidx.compose.ui.test.performClick import com.x8bit.bitwarden.data.platform.repository.util.bufferedMutableSharedFlow +import com.x8bit.bitwarden.data.vault.datasource.sdk.model.createMockCipherView 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 @@ -76,7 +77,7 @@ class AttachmentsScreenTest : BaseComposeTest() { @Test fun `on choose file click should send ChooseFileClick`() { mutableStateFlow.update { - it.copy(viewState = AttachmentsState.ViewState.Content(emptyList())) + it.copy(viewState = DEFAULT_CONTENT_WITHOUT_ATTACHMENTS) } composeTestRule.onNodeWithTextAfterScroll("Choose file").performClick() @@ -96,7 +97,7 @@ class AttachmentsScreenTest : BaseComposeTest() { composeTestRule.onNode(isProgressBar).assertDoesNotExist() mutableStateFlow.update { - it.copy(viewState = AttachmentsState.ViewState.Content(emptyList())) + it.copy(viewState = DEFAULT_CONTENT_WITHOUT_ATTACHMENTS) } composeTestRule.onNode(isProgressBar).assertDoesNotExist() } @@ -113,7 +114,7 @@ class AttachmentsScreenTest : BaseComposeTest() { composeTestRule.onNodeWithText(errorMessage).assertDoesNotExist() mutableStateFlow.update { - it.copy(viewState = AttachmentsState.ViewState.Content(emptyList())) + it.copy(viewState = DEFAULT_CONTENT_WITHOUT_ATTACHMENTS) } composeTestRule.onNodeWithText(errorMessage).assertDoesNotExist() } @@ -121,7 +122,7 @@ class AttachmentsScreenTest : BaseComposeTest() { @Test fun `content with no items should be displayed according to state`() { mutableStateFlow.update { - it.copy(viewState = AttachmentsState.ViewState.Content(emptyList())) + it.copy(viewState = DEFAULT_CONTENT_WITHOUT_ATTACHMENTS) } composeTestRule .onNodeWithTextAfterScroll("There are no attachments.") @@ -197,15 +198,60 @@ class AttachmentsScreenTest : BaseComposeTest() { viewModel.trySendAction(AttachmentsAction.DeleteClick(cipherId)) } } + + @Test + fun `error dialog should be displayed according to state`() { + val errorMessage = "Fail" + composeTestRule.onNode(isDialog()).assertDoesNotExist() + composeTestRule.onNodeWithText(errorMessage).assertDoesNotExist() + + mutableStateFlow.update { + it.copy( + dialogState = AttachmentsState.DialogState.Error( + title = null, + message = errorMessage.asText(), + ), + ) + } + + composeTestRule + .onNodeWithText(errorMessage) + .assert(hasAnyAncestor(isDialog())) + .assertIsDisplayed() + } + + @Test + fun `loading dialog should be displayed according to state`() { + val loadingMessage = "deleting" + composeTestRule.onNode(isDialog()).assertDoesNotExist() + composeTestRule.onNodeWithText(loadingMessage).assertDoesNotExist() + + mutableStateFlow.update { + it.copy(dialogState = AttachmentsState.DialogState.Loading(loadingMessage.asText())) + } + + composeTestRule + .onNodeWithText(loadingMessage) + .assertIsDisplayed() + .assert(hasAnyAncestor(isDialog())) + } } private val DEFAULT_STATE: AttachmentsState = AttachmentsState( cipherId = "cipherId-1234", viewState = AttachmentsState.ViewState.Loading, + dialogState = null, ) +private val DEFAULT_CONTENT_WITHOUT_ATTACHMENTS: AttachmentsState.ViewState.Content = + AttachmentsState.ViewState.Content( + originalCipher = createMockCipherView(number = 1), + attachments = emptyList(), + ) + private val DEFAULT_CONTENT_WITH_ATTACHMENTS: AttachmentsState.ViewState.Content = AttachmentsState.ViewState.Content( + originalCipher = createMockCipherView(number = 1), attachments = listOf( AttachmentsState.AttachmentItem( id = "cipherId-1234", diff --git a/app/src/test/java/com/x8bit/bitwarden/ui/vault/feature/attachments/AttachmentsViewModelTest.kt b/app/src/test/java/com/x8bit/bitwarden/ui/vault/feature/attachments/AttachmentsViewModelTest.kt index 8f93e8a7d6..c8afd8afe2 100644 --- a/app/src/test/java/com/x8bit/bitwarden/ui/vault/feature/attachments/AttachmentsViewModelTest.kt +++ b/app/src/test/java/com/x8bit/bitwarden/ui/vault/feature/attachments/AttachmentsViewModelTest.kt @@ -7,11 +7,13 @@ import com.x8bit.bitwarden.R import com.x8bit.bitwarden.data.platform.repository.model.DataState import com.x8bit.bitwarden.data.vault.datasource.sdk.model.createMockCipherView import com.x8bit.bitwarden.data.vault.repository.VaultRepository +import com.x8bit.bitwarden.data.vault.repository.model.DeleteAttachmentResult import com.x8bit.bitwarden.ui.platform.base.BaseViewModelTest import com.x8bit.bitwarden.ui.platform.base.util.asText import com.x8bit.bitwarden.ui.platform.base.util.concat import com.x8bit.bitwarden.ui.platform.manager.intent.IntentManager import com.x8bit.bitwarden.ui.vault.feature.attachments.util.toViewState +import io.mockk.coEvery import io.mockk.every import io.mockk.mockk import io.mockk.mockkStatic @@ -71,6 +73,19 @@ class AttachmentsViewModelTest : BaseViewModelTest() { } } + @Test + fun `DismissDialogClick should emit DismissDialogClick`() = runTest { + val initialState = DEFAULT_STATE.copy( + dialogState = AttachmentsState.DialogState.Loading("Loading".asText()), + ) + val viewModel = createViewModel(initialState) + viewModel.stateFlow.test { + assertEquals(initialState, awaitItem()) + viewModel.trySendAction(AttachmentsAction.DismissDialogClick) + assertEquals(initialState.copy(dialogState = null), awaitItem()) + } + } + @Test fun `ChooseFileClick should emit ShowToast`() = runTest { val viewModel = createViewModel() @@ -91,12 +106,65 @@ class AttachmentsViewModelTest : BaseViewModelTest() { } @Test - fun `DeleteClick should emit ShowToast`() = runTest { - val attachmentId = "attachmentId-1234" + fun `DeleteClick with deleteCipherAttachment error should display error dialog`() = runTest { + val cipherId = "mockId-1" + val attachmentId = "mockId-1" + val cipherView = createMockCipherView(number = 1) + val initialState = DEFAULT_STATE.copy(viewState = DEFAULT_CONTENT_WITH_ATTACHMENTS) + coEvery { + vaultRepository.deleteCipherAttachment( + cipherId = cipherId, + attachmentId = attachmentId, + cipherView = cipherView, + ) + } returns DeleteAttachmentResult.Error + mutableVaultItemStateFlow.tryEmit(DataState.Loaded(cipherView)) + + val viewModel = createViewModel() + viewModel.stateFlow.test { + assertEquals(initialState, awaitItem()) + viewModel.actionChannel.trySend(AttachmentsAction.DeleteClick(attachmentId)) + assertEquals( + initialState.copy( + dialogState = AttachmentsState.DialogState.Loading( + message = R.string.deleting.asText(), + ), + ), + awaitItem(), + ) + assertEquals( + initialState.copy( + dialogState = AttachmentsState.DialogState.Error( + title = R.string.an_error_has_occurred.asText(), + message = R.string.generic_error_message.asText(), + ), + ), + awaitItem(), + ) + } + } + + @Test + fun `DeleteClick with deleteCipherAttachment success should emit ShowToast`() = runTest { + val cipherId = "mockId-1" + val attachmentId = "mockId-1" + val cipherView = createMockCipherView(number = 1) + coEvery { + vaultRepository.deleteCipherAttachment( + cipherId = cipherId, + attachmentId = attachmentId, + cipherView = cipherView, + ) + } returns DeleteAttachmentResult.Success + mutableVaultItemStateFlow.tryEmit(DataState.Loaded(cipherView)) + val viewModel = createViewModel() viewModel.eventFlow.test { - viewModel.trySendAction(AttachmentsAction.DeleteClick(attachmentId)) - assertEquals(AttachmentsEvent.ShowToast("Not Yet Implemented".asText()), awaitItem()) + viewModel.actionChannel.trySend(AttachmentsAction.DeleteClick(cipherId)) + assertEquals( + AttachmentsEvent.ShowToast(R.string.attachment_deleted.asText()), + awaitItem(), + ) } } @@ -213,23 +281,25 @@ class AttachmentsViewModelTest : BaseViewModelTest() { vaultRepo = vaultRepository, savedStateHandle = SavedStateHandle().apply { set("state", initialState) - set("cipher_id", initialState?.cipherId ?: "cipherId-1234") + set("cipher_id", initialState?.cipherId ?: "mockId-1") }, ) } private val DEFAULT_STATE: AttachmentsState = AttachmentsState( - cipherId = "cipherId-1234", + cipherId = "mockId-1", viewState = AttachmentsState.ViewState.Loading, + dialogState = null, ) private val DEFAULT_CONTENT_WITH_ATTACHMENTS: AttachmentsState.ViewState.Content = AttachmentsState.ViewState.Content( + originalCipher = createMockCipherView(number = 1), attachments = listOf( AttachmentsState.AttachmentItem( - id = "cipherId-1234", - title = "cool_file.png", - displaySize = "10 MB", + id = "mockId-1", + title = "mockFileName-1", + displaySize = "mockSizeName-1", ), ), ) diff --git a/app/src/test/java/com/x8bit/bitwarden/ui/vault/feature/attachments/util/CipherViewExtensionsTest.kt b/app/src/test/java/com/x8bit/bitwarden/ui/vault/feature/attachments/util/CipherViewExtensionsTest.kt index a76ac9eef3..252fd5d296 100644 --- a/app/src/test/java/com/x8bit/bitwarden/ui/vault/feature/attachments/util/CipherViewExtensionsTest.kt +++ b/app/src/test/java/com/x8bit/bitwarden/ui/vault/feature/attachments/util/CipherViewExtensionsTest.kt @@ -16,6 +16,7 @@ class CipherViewExtensionsTest { assertEquals( AttachmentsState.ViewState.Content( + originalCipher = cipherView, attachments = listOf( AttachmentsState.AttachmentItem( id = "mockId-1", @@ -37,7 +38,10 @@ class CipherViewExtensionsTest { val result = cipherView.toViewState() assertEquals( - AttachmentsState.ViewState.Content(attachments = emptyList()), + AttachmentsState.ViewState.Content( + originalCipher = cipherView, + attachments = emptyList(), + ), result, ) } @@ -55,7 +59,10 @@ class CipherViewExtensionsTest { val result = cipherView.toViewState() assertEquals( - AttachmentsState.ViewState.Content(attachments = emptyList()), + AttachmentsState.ViewState.Content( + originalCipher = cipherView, + attachments = emptyList(), + ), result, ) }