diff --git a/app/src/main/java/com/x8bit/bitwarden/data/vault/datasource/network/di/VaultNetworkModule.kt b/app/src/main/java/com/x8bit/bitwarden/data/vault/datasource/network/di/VaultNetworkModule.kt index f7054c4fa..beffb9802 100644 --- a/app/src/main/java/com/x8bit/bitwarden/data/vault/datasource/network/di/VaultNetworkModule.kt +++ b/app/src/main/java/com/x8bit/bitwarden/data/vault/datasource/network/di/VaultNetworkModule.kt @@ -9,6 +9,7 @@ import dagger.Module import dagger.Provides import dagger.hilt.InstallIn import dagger.hilt.components.SingletonComponent +import kotlinx.serialization.json.Json import retrofit2.create import javax.inject.Singleton @@ -23,8 +24,10 @@ object VaultNetworkModule { @Singleton fun provideCiphersService( retrofits: Retrofits, + json: Json, ): CiphersService = CiphersServiceImpl( ciphersApi = retrofits.authenticatedApiRetrofit.create(), + json = json, ) @Provides diff --git a/app/src/main/java/com/x8bit/bitwarden/data/vault/datasource/network/model/UpdateCipherResponseJson.kt b/app/src/main/java/com/x8bit/bitwarden/data/vault/datasource/network/model/UpdateCipherResponseJson.kt new file mode 100644 index 000000000..32cd41b5a --- /dev/null +++ b/app/src/main/java/com/x8bit/bitwarden/data/vault/datasource/network/model/UpdateCipherResponseJson.kt @@ -0,0 +1,33 @@ +package com.x8bit.bitwarden.data.vault.datasource.network.model + +import kotlinx.serialization.SerialName +import kotlinx.serialization.Serializable + +/** + * Models the response from the update cipher request. + */ +sealed class UpdateCipherResponseJson { + /** + * The request completed successfully and returned the updated [cipher]. + */ + data class Success( + val cipher: SyncResponseJson.Cipher, + ) : UpdateCipherResponseJson() + + /** + * Represents the json body of an invalid update request. + * + * @param message A general, user-displayable error message. + * @param validationErrors a map where each value is a list of error messages for each key. + * The values in the array should be used for display to the user, since the keys tend to come + * back as nonsense. (eg: empty string key) + */ + @Serializable + data class Invalid( + @SerialName("message") + val message: String?, + + @SerialName("validationErrors") + val validationErrors: Map>?, + ) : UpdateCipherResponseJson() +} diff --git a/app/src/main/java/com/x8bit/bitwarden/data/vault/datasource/network/service/CiphersService.kt b/app/src/main/java/com/x8bit/bitwarden/data/vault/datasource/network/service/CiphersService.kt index c67468b0b..68c181c27 100644 --- a/app/src/main/java/com/x8bit/bitwarden/data/vault/datasource/network/service/CiphersService.kt +++ b/app/src/main/java/com/x8bit/bitwarden/data/vault/datasource/network/service/CiphersService.kt @@ -2,6 +2,7 @@ package com.x8bit.bitwarden.data.vault.datasource.network.service import com.x8bit.bitwarden.data.vault.datasource.network.model.CipherJsonRequest import com.x8bit.bitwarden.data.vault.datasource.network.model.SyncResponseJson +import com.x8bit.bitwarden.data.vault.datasource.network.model.UpdateCipherResponseJson /** * Provides an API for querying ciphers endpoints. @@ -18,5 +19,5 @@ interface CiphersService { suspend fun updateCipher( cipherId: String, body: CipherJsonRequest, - ): Result + ): Result } diff --git a/app/src/main/java/com/x8bit/bitwarden/data/vault/datasource/network/service/CiphersServiceImpl.kt b/app/src/main/java/com/x8bit/bitwarden/data/vault/datasource/network/service/CiphersServiceImpl.kt index 5ee36c713..272f82c6e 100644 --- a/app/src/main/java/com/x8bit/bitwarden/data/vault/datasource/network/service/CiphersServiceImpl.kt +++ b/app/src/main/java/com/x8bit/bitwarden/data/vault/datasource/network/service/CiphersServiceImpl.kt @@ -1,11 +1,16 @@ package com.x8bit.bitwarden.data.vault.datasource.network.service +import com.x8bit.bitwarden.data.platform.datasource.network.model.toBitwardenError +import com.x8bit.bitwarden.data.platform.datasource.network.util.parseErrorBodyOrNull import com.x8bit.bitwarden.data.vault.datasource.network.api.CiphersApi import com.x8bit.bitwarden.data.vault.datasource.network.model.CipherJsonRequest import com.x8bit.bitwarden.data.vault.datasource.network.model.SyncResponseJson +import com.x8bit.bitwarden.data.vault.datasource.network.model.UpdateCipherResponseJson +import kotlinx.serialization.json.Json class CiphersServiceImpl constructor( private val ciphersApi: CiphersApi, + private val json: Json, ) : CiphersService { override suspend fun createCipher(body: CipherJsonRequest): Result = ciphersApi.createCipher(body = body) @@ -13,9 +18,20 @@ class CiphersServiceImpl constructor( override suspend fun updateCipher( cipherId: String, body: CipherJsonRequest, - ): Result = - ciphersApi.updateCipher( - cipherId = cipherId, - body = body, - ) + ): Result = + ciphersApi + .updateCipher( + cipherId = cipherId, + body = body, + ) + .map { UpdateCipherResponseJson.Success(cipher = it) } + .recoverCatching { throwable -> + throwable + .toBitwardenError() + .parseErrorBodyOrNull( + code = 400, + json = json, + ) + ?: throw throwable + } } 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 e0282c85e..7e19337fd 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 @@ -22,6 +22,7 @@ import com.x8bit.bitwarden.data.platform.util.asSuccess import com.x8bit.bitwarden.data.platform.util.flatMap import com.x8bit.bitwarden.data.vault.datasource.disk.VaultDiskSource import com.x8bit.bitwarden.data.vault.datasource.network.model.SyncResponseJson +import com.x8bit.bitwarden.data.vault.datasource.network.model.UpdateCipherResponseJson import com.x8bit.bitwarden.data.vault.datasource.network.service.CiphersService import com.x8bit.bitwarden.data.vault.datasource.network.service.SyncService import com.x8bit.bitwarden.data.vault.datasource.sdk.VaultSdkSource @@ -387,10 +388,18 @@ class VaultRepositoryImpl( ) } .fold( - onFailure = { UpdateCipherResult.Error }, - onSuccess = { - sync() - UpdateCipherResult.Success + onFailure = { UpdateCipherResult.Error(errorMessage = null) }, + onSuccess = { response -> + when (response) { + is UpdateCipherResponseJson.Invalid -> { + UpdateCipherResult.Error(errorMessage = response.message) + } + + is UpdateCipherResponseJson.Success -> { + sync() + UpdateCipherResult.Success + } + } }, ) diff --git a/app/src/main/java/com/x8bit/bitwarden/data/vault/repository/model/UpdateCipherResult.kt b/app/src/main/java/com/x8bit/bitwarden/data/vault/repository/model/UpdateCipherResult.kt index 9e8f99ee8..9fe61ffe3 100644 --- a/app/src/main/java/com/x8bit/bitwarden/data/vault/repository/model/UpdateCipherResult.kt +++ b/app/src/main/java/com/x8bit/bitwarden/data/vault/repository/model/UpdateCipherResult.kt @@ -11,7 +11,8 @@ sealed class UpdateCipherResult { data object Success : UpdateCipherResult() /** - * Generic error while updating cipher. + * Generic error while updating cipher. The optional [errorMessage] may be displayed directly in + * the UI when present. */ - data object Error : UpdateCipherResult() + data class Error(val errorMessage: String?) : UpdateCipherResult() } diff --git a/app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/addedit/VaultAddEditViewModel.kt b/app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/addedit/VaultAddEditViewModel.kt index 57370035e..f6f4b19dd 100644 --- a/app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/addedit/VaultAddEditViewModel.kt +++ b/app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/addedit/VaultAddEditViewModel.kt @@ -690,10 +690,18 @@ class VaultAddEditViewModel @Inject constructor( action: VaultAddEditAction.Internal.UpdateCipherResultReceive, ) { mutableStateFlow.update { it.copy(dialog = null) } - when (action.updateCipherResult) { + when (val result = action.updateCipherResult) { is UpdateCipherResult.Error -> { - // TODO Display error dialog BIT-501 - sendEvent(VaultAddEditEvent.ShowToast(message = "Save Item Failure".asText())) + mutableStateFlow.update { + it.copy( + dialog = VaultAddEditState.DialogState.Error( + message = result + .errorMessage + ?.asText() + ?: R.string.generic_error_message.asText(), + ), + ) + } } is UpdateCipherResult.Success -> { diff --git a/app/src/test/java/com/x8bit/bitwarden/data/platform/base/BaseServiceTest.kt b/app/src/test/java/com/x8bit/bitwarden/data/platform/base/BaseServiceTest.kt index beadd4220..cb38b6ebf 100644 --- a/app/src/test/java/com/x8bit/bitwarden/data/platform/base/BaseServiceTest.kt +++ b/app/src/test/java/com/x8bit/bitwarden/data/platform/base/BaseServiceTest.kt @@ -13,7 +13,7 @@ import retrofit2.Retrofit */ abstract class BaseServiceTest { - private val json = PlatformNetworkModule.providesJson() + protected val json = PlatformNetworkModule.providesJson() protected val server = MockWebServer().apply { start() } diff --git a/app/src/test/java/com/x8bit/bitwarden/data/vault/datasource/network/service/CiphersServiceTest.kt b/app/src/test/java/com/x8bit/bitwarden/data/vault/datasource/network/service/CiphersServiceTest.kt index 5e0a2e4a9..f5e690ab8 100644 --- a/app/src/test/java/com/x8bit/bitwarden/data/vault/datasource/network/service/CiphersServiceTest.kt +++ b/app/src/test/java/com/x8bit/bitwarden/data/vault/datasource/network/service/CiphersServiceTest.kt @@ -2,6 +2,7 @@ package com.x8bit.bitwarden.data.vault.datasource.network.service import com.x8bit.bitwarden.data.platform.base.BaseServiceTest import com.x8bit.bitwarden.data.vault.datasource.network.api.CiphersApi +import com.x8bit.bitwarden.data.vault.datasource.network.model.UpdateCipherResponseJson import com.x8bit.bitwarden.data.vault.datasource.network.model.createMockCipher import com.x8bit.bitwarden.data.vault.datasource.network.model.createMockCipherJsonRequest import kotlinx.coroutines.test.runTest @@ -15,6 +16,7 @@ class CiphersServiceTest : BaseServiceTest() { private val ciphersService: CiphersService = CiphersServiceImpl( ciphersApi = ciphersApi, + json = json, ) @Test @@ -30,17 +32,37 @@ class CiphersServiceTest : BaseServiceTest() { } @Test - fun `updateCipher should return the correct response`() = runTest { - server.enqueue(MockResponse().setBody(CREATE_UPDATE_CIPHER_SUCCESS_JSON)) - val result = ciphersService.updateCipher( - cipherId = "cipher-id-1", - body = createMockCipherJsonRequest(number = 1), - ) - assertEquals( - createMockCipher(number = 1), - result.getOrThrow(), - ) - } + fun `updateCipher with success response should return a Success with the correct cipher`() = + runTest { + server.enqueue(MockResponse().setBody(CREATE_UPDATE_CIPHER_SUCCESS_JSON)) + val result = ciphersService.updateCipher( + cipherId = "cipher-id-1", + body = createMockCipherJsonRequest(number = 1), + ) + assertEquals( + UpdateCipherResponseJson.Success( + cipher = createMockCipher(number = 1), + ), + result.getOrThrow(), + ) + } + + @Test + fun `updateCipher with an invalid response should return an Invalid with the correct data`() = + runTest { + server.enqueue(MockResponse().setResponseCode(400).setBody(UPDATE_CIPHER_INVALID_JSON)) + val result = ciphersService.updateCipher( + cipherId = "cipher-id-1", + body = createMockCipherJsonRequest(number = 1), + ) + assertEquals( + UpdateCipherResponseJson.Invalid( + message = "You do not have permission to edit this.", + validationErrors = null, + ), + result.getOrThrow(), + ) + } } private const val CREATE_UPDATE_CIPHER_SUCCESS_JSON = """ @@ -134,3 +156,10 @@ private const val CREATE_UPDATE_CIPHER_SUCCESS_JSON = """ "key": "mockKey-1" } """ + +private const val UPDATE_CIPHER_INVALID_JSON = """ +{ + "message": "You do not have permission to edit this.", + "validationErrors": null +} +""" 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 e3490c67b..5d0710356 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 @@ -21,6 +21,7 @@ import com.x8bit.bitwarden.data.platform.util.asFailure import com.x8bit.bitwarden.data.platform.util.asSuccess import com.x8bit.bitwarden.data.vault.datasource.disk.VaultDiskSource import com.x8bit.bitwarden.data.vault.datasource.network.model.SyncResponseJson +import com.x8bit.bitwarden.data.vault.datasource.network.model.UpdateCipherResponseJson import com.x8bit.bitwarden.data.vault.datasource.network.model.createMockCipher import com.x8bit.bitwarden.data.vault.datasource.network.model.createMockCipherJsonRequest import com.x8bit.bitwarden.data.vault.datasource.network.model.createMockCollection @@ -1746,12 +1747,12 @@ class VaultRepositoryTest { cipherView = mockCipherView, ) - assertEquals(UpdateCipherResult.Error, result) + assertEquals(UpdateCipherResult.Error(errorMessage = null), result) } @Test @Suppress("MaxLineLength") - fun `updateCipher with ciphersService updateCipher failure should return UpdateCipherResult failure`() = + fun `updateCipher with ciphersService updateCipher failure should return UpdateCipherResult Error with a null message`() = runTest { val cipherId = "cipherId1234" val mockCipherView = createMockCipherView(number = 1) @@ -1770,12 +1771,12 @@ class VaultRepositoryTest { cipherView = mockCipherView, ) - assertEquals(UpdateCipherResult.Error, result) + assertEquals(UpdateCipherResult.Error(errorMessage = null), result) } @Test @Suppress("MaxLineLength") - fun `updateCipher with ciphersService updateCipher success should return UpdateCipherResult success`() = + fun `updateCipher with ciphersService updateCipher Invalid response should return UpdateCipherResult Error with a non-null message`() = runTest { val cipherId = "cipherId1234" val mockCipherView = createMockCipherView(number = 1) @@ -1787,7 +1788,43 @@ class VaultRepositoryTest { cipherId = cipherId, body = createMockCipherJsonRequest(number = 1), ) - } returns createMockCipher(number = 1).asSuccess() + } returns UpdateCipherResponseJson + .Invalid( + message = "You do not have permission to edit this.", + validationErrors = null, + ) + .asSuccess() + + val result = vaultRepository.updateCipher( + cipherId = cipherId, + cipherView = mockCipherView, + ) + + assertEquals( + UpdateCipherResult.Error( + errorMessage = "You do not have permission to edit this.", + ), + result, + ) + } + + @Test + @Suppress("MaxLineLength") + fun `updateCipher with ciphersService updateCipher Success response should return UpdateCipherResult success`() = + runTest { + val cipherId = "cipherId1234" + val mockCipherView = createMockCipherView(number = 1) + coEvery { + vaultSdkSource.encryptCipher(cipherView = mockCipherView) + } returns createMockSdkCipher(number = 1).asSuccess() + coEvery { + ciphersService.updateCipher( + cipherId = cipherId, + body = createMockCipherJsonRequest(number = 1), + ) + } returns UpdateCipherResponseJson + .Success(cipher = createMockCipher(number = 1)) + .asSuccess() coEvery { syncService.sync() } returns Result.success(createMockSyncResponse(1)) diff --git a/app/src/test/java/com/x8bit/bitwarden/ui/vault/feature/addedit/VaultAddEditViewModelTest.kt b/app/src/test/java/com/x8bit/bitwarden/ui/vault/feature/addedit/VaultAddEditViewModelTest.kt index f8ab948e7..9916aab95 100644 --- a/app/src/test/java/com/x8bit/bitwarden/ui/vault/feature/addedit/VaultAddEditViewModelTest.kt +++ b/app/src/test/java/com/x8bit/bitwarden/ui/vault/feature/addedit/VaultAddEditViewModelTest.kt @@ -266,39 +266,88 @@ class VaultAddEditViewModelTest : BaseViewModelTest() { } } + @Suppress("MaxLineLength") @Test - fun `in edit mode, SaveClick createCipher error should emit ShowToast`() = runTest { - val cipherView = mockk() - val vaultAddEditType = VaultAddEditType.EditItem(DEFAULT_EDIT_ITEM_ID) - val stateWithName = createVaultAddItemState( - vaultAddEditType = vaultAddEditType, - commonContentViewState = createCommonContentViewState( - name = "mockName", - ), - ) - - every { cipherView.toViewState() } returns stateWithName.viewState - coEvery { - vaultRepository.updateCipher(DEFAULT_EDIT_ITEM_ID, any()) - } returns UpdateCipherResult.Error - mutableVaultItemFlow.value = DataState.Loaded(cipherView) - - val viewModel = createAddVaultItemViewModel( - createSavedStateHandleWithState( - state = stateWithName, + fun `in edit mode, SaveClick updateCipher error with a null message should show an error dialog with a generic message`() = + runTest { + val cipherView = mockk() + val vaultAddEditType = VaultAddEditType.EditItem(DEFAULT_EDIT_ITEM_ID) + val stateWithName = createVaultAddItemState( vaultAddEditType = vaultAddEditType, - ), - ) + commonContentViewState = createCommonContentViewState( + name = "mockName", + ), + ) + + every { cipherView.toViewState() } returns stateWithName.viewState + coEvery { + vaultRepository.updateCipher(DEFAULT_EDIT_ITEM_ID, any()) + } returns UpdateCipherResult.Error(errorMessage = null) + mutableVaultItemFlow.value = DataState.Loaded(cipherView) + + val viewModel = createAddVaultItemViewModel( + createSavedStateHandleWithState( + state = stateWithName, + vaultAddEditType = vaultAddEditType, + ), + ) - viewModel.eventFlow.test { viewModel.actionChannel.trySend(VaultAddEditAction.Common.SaveClick) - assertEquals(VaultAddEditEvent.ShowToast("Save Item Failure".asText()), awaitItem()) + + assertEquals( + stateWithName.copy( + dialog = VaultAddEditState.DialogState.Error( + message = R.string.generic_error_message.asText(), + ), + ), + viewModel.stateFlow.value, + ) + coVerify(exactly = 1) { + vaultRepository.updateCipher(DEFAULT_EDIT_ITEM_ID, any()) + } } - coVerify(exactly = 1) { - vaultRepository.updateCipher(DEFAULT_EDIT_ITEM_ID, any()) + @Suppress("MaxLineLength") + @Test + fun `in edit mode, SaveClick updateCipher error with a non-null message should show an error dialog with that message`() = + runTest { + val cipherView = mockk() + val vaultAddEditType = VaultAddEditType.EditItem(DEFAULT_EDIT_ITEM_ID) + val stateWithName = createVaultAddItemState( + vaultAddEditType = vaultAddEditType, + commonContentViewState = createCommonContentViewState( + name = "mockName", + ), + ) + val errorMessage = "You do not have permission to edit this." + + every { cipherView.toViewState() } returns stateWithName.viewState + coEvery { + vaultRepository.updateCipher(DEFAULT_EDIT_ITEM_ID, any()) + } returns UpdateCipherResult.Error(errorMessage = errorMessage) + mutableVaultItemFlow.value = DataState.Loaded(cipherView) + + val viewModel = createAddVaultItemViewModel( + createSavedStateHandleWithState( + state = stateWithName, + vaultAddEditType = vaultAddEditType, + ), + ) + + viewModel.actionChannel.trySend(VaultAddEditAction.Common.SaveClick) + + assertEquals( + stateWithName.copy( + dialog = VaultAddEditState.DialogState.Error( + message = errorMessage.asText(), + ), + ), + viewModel.stateFlow.value, + ) + coVerify(exactly = 1) { + vaultRepository.updateCipher(DEFAULT_EDIT_ITEM_ID, any()) + } } - } @Test fun `Saving item with an empty name field will cause a dialog to show up`() = runTest {