From a45f94add7a0c770bd1e596432e74351e7a6393b Mon Sep 17 00:00:00 2001 From: MohitMaliFtechiz Date: Wed, 30 Oct 2024 23:16:00 +0530 Subject: [PATCH] Implemented error handling for cases when the selected file is not a ZIM file. * We retrieve the file name from the incoming Uri using `DocumentFile` (the official way to handle Uris) and check if it contains `.zim` or `.zimma` before proceeding with the copy/move operation. If the file name is null, we copy/move the file first and then validate it as a ZIM file using `libkiwix`. If valid, the file opens in our reader; otherwise, we revert the copy/move operation. * Added test cases for this new functionality. --- .../library/CopyMoveFileHandler.kt | 109 +++++++++++++----- .../library/LocalLibraryFragment.kt | 12 +- .../localLibrary/CopyMoveFileHandlerTest.kt | 96 +++++++++++++++ 3 files changed, 189 insertions(+), 28 deletions(-) diff --git a/app/src/main/java/org/kiwix/kiwixmobile/nav/destination/library/CopyMoveFileHandler.kt b/app/src/main/java/org/kiwix/kiwixmobile/nav/destination/library/CopyMoveFileHandler.kt index 0a4f8b4f9..37aae9b35 100644 --- a/app/src/main/java/org/kiwix/kiwixmobile/nav/destination/library/CopyMoveFileHandler.kt +++ b/app/src/main/java/org/kiwix/kiwixmobile/nav/destination/library/CopyMoveFileHandler.kt @@ -28,6 +28,7 @@ import android.view.View import android.widget.ProgressBar import android.widget.TextView import androidx.appcompat.app.AlertDialog +import androidx.core.net.toUri import androidx.documentfile.provider.DocumentFile import io.reactivex.android.schedulers.AndroidSchedulers import io.reactivex.disposables.Disposable @@ -40,6 +41,7 @@ import org.kiwix.kiwixmobile.R.layout import org.kiwix.kiwixmobile.core.R import org.kiwix.kiwixmobile.core.extensions.deleteFile import org.kiwix.kiwixmobile.core.extensions.isFileExist +import org.kiwix.kiwixmobile.core.reader.ZimReaderSource import org.kiwix.kiwixmobile.core.settings.StorageCalculator import org.kiwix.kiwixmobile.core.utils.SharedPreferenceUtil import org.kiwix.kiwixmobile.core.utils.dialog.AlertDialogShower @@ -69,7 +71,8 @@ class CopyMoveFileHandler @Inject constructor( private var lifecycleScope: CoroutineScope? = null private var progressBar: ProgressBar? = null private var progressBarTextView: TextView? = null - private var isMoveOperation = false + var isMoveOperation = false + private var shouldValidateZimFile: Boolean = false private var fileSystemDisposable: Disposable? = null private val copyMoveTitle: String by lazy { @@ -87,7 +90,12 @@ class CopyMoveFileHandler @Inject constructor( } } - fun showMoveFileToPublicDirectoryDialog(uri: Uri? = null, documentFile: DocumentFile? = null) { + fun showMoveFileToPublicDirectoryDialog( + uri: Uri? = null, + documentFile: DocumentFile? = null, + shouldValidateZimFile: Boolean = false + ) { + this.shouldValidateZimFile = shouldValidateZimFile setSelectedFileAndUri(uri, documentFile) if (!sharedPreferenceUtil.copyMoveZimFilePermissionDialog) { showMoveToPublicDirectoryPermissionDialog() @@ -210,50 +218,63 @@ class CopyMoveFileHandler @Inject constructor( showProgressDialog() copyFile(sourceUri, destinationFile) withContext(Dispatchers.Main) { - notifyFileOperationSuccess(destinationFile) + notifyFileOperationSuccess(destinationFile, sourceUri) } } catch (ignore: Exception) { ignore.printStackTrace() - handleFileOperationError(ignore.message, destinationFile) + handleFileOperationError( + activity.getString(R.string.copy_file_error_message, ignore.message), + destinationFile + ) } } } + @Suppress("UnsafeCallOnNullableType") private fun moveZimFileToPublicAppDirectory() { lifecycleScope?.launch { val destinationFile = getDestinationFile() try { val sourceUri = selectedFileUri ?: throw FileNotFoundException("Selected file not found") showProgressDialog() - var moveSuccess = false - if (tryMoveWithDocumentContract(sourceUri)) { - moveSuccess = true - } else { - moveSuccess = true + val moveSuccess = selectedFile?.parentFile?.uri?.let { parentUri -> + tryMoveWithDocumentContract( + sourceUri, + parentUri, + DocumentFile.fromFile(File(sharedPreferenceUtil.prefStorage)).uri + ) + } ?: run { copyFile(sourceUri, destinationFile) - deleteSourceFile(sourceUri) + true } withContext(Dispatchers.Main) { if (moveSuccess) { - notifyFileOperationSuccess(destinationFile) + notifyFileOperationSuccess(destinationFile, sourceUri) } else { - handleFileOperationError("File move failed", destinationFile) + handleFileOperationError( + activity.getString(R.string.move_file_error_message, "File move failed"), + destinationFile + ) } } } catch (ignore: Exception) { ignore.printStackTrace() - handleFileOperationError(ignore.message, destinationFile) + handleFileOperationError( + activity.getString(R.string.move_file_error_message, ignore.message), + destinationFile + ) } } } - @Suppress("UnsafeCallOnNullableType") - private fun tryMoveWithDocumentContract(selectedUri: Uri): Boolean { + fun tryMoveWithDocumentContract( + selectedUri: Uri, + sourceParentFolderUri: Uri, + destinationFolderUri: Uri + ): Boolean { return try { val contentResolver = activity.contentResolver if (documentCanMove(selectedUri, contentResolver)) { - val sourceParentFolderUri = selectedFile?.parentFile!!.uri - val destinationFolderUri = DocumentFile.fromFile(File(sharedPreferenceUtil.prefStorage)).uri DocumentsContract.moveDocument( contentResolver, @@ -289,17 +310,12 @@ class CopyMoveFileHandler @Inject constructor( return flags and DocumentsContract.Document.FLAG_SUPPORTS_MOVE != 0 } - private fun handleFileOperationError( + fun handleFileOperationError( errorMessage: String?, destinationFile: File ) { dismissProgressDialog() - val userFriendlyMessage = if (isMoveOperation) { - activity.getString(R.string.move_file_error_message, errorMessage) - } else { - activity.getString(R.string.copy_file_error_message, errorMessage) - } - fileCopyMoveCallback?.onError(userFriendlyMessage).also { + fileCopyMoveCallback?.onError("$errorMessage").also { // Clean up the destination file if an error occurs lifecycleScope?.launch { destinationFile.deleteFile() @@ -307,16 +323,55 @@ class CopyMoveFileHandler @Inject constructor( } } - private fun notifyFileOperationSuccess(destinationFile: File) { + suspend fun notifyFileOperationSuccess(destinationFile: File, sourceUri: Uri) { + if (shouldValidateZimFile && !isValidZimFile(destinationFile)) { + handleInvalidZimFile(destinationFile, sourceUri) + return + } dismissProgressDialog() if (isMoveOperation) { + deleteSourceFile(sourceUri) fileCopyMoveCallback?.onFileMoved(destinationFile) } else { fileCopyMoveCallback?.onFileCopied(destinationFile) } } - private fun deleteSourceFile(uri: Uri) { + fun handleInvalidZimFile(destinationFile: File, sourceUri: Uri) { + val errorMessage = activity.getString(R.string.error_file_invalid) + if (isMoveOperation) { + val moveSuccessful = tryMoveWithDocumentContract( + destinationFile.toUri(), + destinationFile.parentFile.toUri(), + sourceUri + ) + + if (moveSuccessful) { + // If files is moved back using the documentContract then show the error message to user + dismissProgressDialog() + fileCopyMoveCallback?.onError(errorMessage) + } else { + // Show error message and delete the moved file if move failed. + handleFileOperationError(errorMessage, destinationFile) + } + } else { + // For copy operation, show error message and delete the copied file. + handleFileOperationError(errorMessage, destinationFile) + } + } + + suspend fun isValidZimFile(destinationFile: File): Boolean { + return try { + // create archive object, and check if it has the mainEntry or not to validate the ZIM file. + val archive = ZimReaderSource(destinationFile).createArchive() + archive?.hasMainEntry() == true + } catch (ignore: Exception) { + // if it is a invalid ZIM file + false + } + } + + fun deleteSourceFile(uri: Uri) { try { DocumentsContract.deleteDocument(activity.applicationContext.contentResolver, uri) } catch (ignore: Exception) { @@ -413,7 +468,7 @@ class CopyMoveFileHandler @Inject constructor( progressBarDialog?.show() } - private fun dismissProgressDialog() { + fun dismissProgressDialog() { if (progressBarDialog?.isShowing == true) { progressBarDialog?.dismiss() } diff --git a/app/src/main/java/org/kiwix/kiwixmobile/nav/destination/library/LocalLibraryFragment.kt b/app/src/main/java/org/kiwix/kiwixmobile/nav/destination/library/LocalLibraryFragment.kt index 360e57f8b..37d2bb266 100644 --- a/app/src/main/java/org/kiwix/kiwixmobile/nav/destination/library/LocalLibraryFragment.kt +++ b/app/src/main/java/org/kiwix/kiwixmobile/nav/destination/library/LocalLibraryFragment.kt @@ -415,9 +415,19 @@ class LocalLibraryFragment : BaseFragment(), CopyMoveFileHandler.FileCopyMoveCal DocumentFile.fromSingleUri(requireActivity(), uri) } } + // If the file is not valid, it shows an error message and stops further processing. + // If the file name is not found, then let them to copy the file + // and we will handle this later. + val fileName = documentFile?.name + if (fileName != null && !FileUtils.isValidZimFile(fileName)) { + activity.toast(string.error_file_invalid) + return + } copyMoveFileHandler?.showMoveFileToPublicDirectoryDialog( uri, - documentFile + documentFile, + // pass if fileName is null then we will validate it after copying/moving + fileName == null ) } else { getZimFileFromUri(uri)?.let(::navigateToReaderFragment) diff --git a/app/src/test/java/org/kiwix/kiwixmobile/localLibrary/CopyMoveFileHandlerTest.kt b/app/src/test/java/org/kiwix/kiwixmobile/localLibrary/CopyMoveFileHandlerTest.kt index 95b825867..b4f10c879 100644 --- a/app/src/test/java/org/kiwix/kiwixmobile/localLibrary/CopyMoveFileHandlerTest.kt +++ b/app/src/test/java/org/kiwix/kiwixmobile/localLibrary/CopyMoveFileHandlerTest.kt @@ -19,17 +19,22 @@ package org.kiwix.kiwixmobile.localLibrary import android.app.Activity +import android.net.Uri import androidx.documentfile.provider.DocumentFile import io.mockk.Runs import io.mockk.clearAllMocks +import io.mockk.coEvery +import io.mockk.coVerify import io.mockk.every import io.mockk.just import io.mockk.mockk +import io.mockk.mockkConstructor import io.mockk.slot import io.mockk.spyk import io.mockk.verify import kotlinx.coroutines.test.StandardTestDispatcher import kotlinx.coroutines.test.TestScope +import kotlinx.coroutines.test.runTest import org.junit.jupiter.api.AfterEach import org.junit.jupiter.api.Assertions.assertFalse import org.junit.jupiter.api.Assertions.assertTrue @@ -46,6 +51,9 @@ import org.kiwix.kiwixmobile.zimManager.Fat32Checker.FileSystemState.CanWrite4Gb import org.kiwix.kiwixmobile.zimManager.Fat32Checker.FileSystemState.CannotWrite4GbFile import org.kiwix.kiwixmobile.zimManager.Fat32Checker.FileSystemState.DetectingFileSystem import java.io.File +import org.kiwix.kiwixmobile.core.R +import org.kiwix.kiwixmobile.core.reader.ZimReaderSource +import org.kiwix.libzim.Archive class CopyMoveFileHandlerTest { private lateinit var fileHandler: CopyMoveFileHandler @@ -61,6 +69,8 @@ class CopyMoveFileHandlerTest { private val storageFile: File = mockk(relaxed = true) private val selectedFile: DocumentFile = mockk(relaxed = true) private val storagePath = "storage/0/emulated/Android/media/org.kiwix.kiwixmobile" + private val destinationFile = mockk() + private val sourceUri = mockk() @BeforeEach fun setup() { @@ -76,6 +86,7 @@ class CopyMoveFileHandlerTest { setLifeCycleScope(testScope) setFileCopyMoveCallback(this@CopyMoveFileHandlerTest.fileCopyMoveCallback) } + every { destinationFile.canRead() } returns true } @Test @@ -292,6 +303,91 @@ class CopyMoveFileHandlerTest { every { fat32Checker.fileSystemStates.value } returns fileSystemState } + @Test + fun notifyFileOperationSuccessShouldCallOnFileMovedIfValidZIMFileAndIsMoveOperationIsTrue() = + runTest { + fileHandler = spyk(fileHandler) + coEvery { fileHandler.isValidZimFile(destinationFile) } returns true + fileHandler.isMoveOperation = true + + fileHandler.notifyFileOperationSuccess(destinationFile, sourceUri) + + verify { fileCopyMoveCallback.onFileMoved(destinationFile) } + verify { fileHandler.dismissProgressDialog() } + coVerify { fileHandler.deleteSourceFile(sourceUri) } + } + + @Test + fun notifyFileOperationSuccessShouldCallOnFileCopiedIfValidZIMFileAndIsMoveOperationIsFalse() = + runTest { + fileHandler = spyk(fileHandler) + coEvery { fileHandler.isValidZimFile(destinationFile) } returns true + fileHandler.isMoveOperation = false + + fileHandler.notifyFileOperationSuccess(destinationFile, sourceUri) + + verify { fileCopyMoveCallback.onFileCopied(destinationFile) } + verify { fileHandler.dismissProgressDialog() } + } + + @Test + fun `notifyFileOperationSuccess should handle invalid ZIM file`() = runTest { + mockkConstructor(Archive::class) + val archiveMock = mockk(relaxed = true) + every { constructedWith(any()) } returns archiveMock + coEvery { fileHandler.isValidZimFile(destinationFile) } returns false + + fileHandler.notifyFileOperationSuccess(destinationFile, sourceUri) + + verify { fileHandler.handleInvalidZimFile(destinationFile, sourceUri) } + } + + @Test + fun `handleInvalidZimFile should call onError if move is successful`() { + every { fileHandler.tryMoveWithDocumentContract(any(), any(), any()) } returns true + fileHandler.isMoveOperation = true + + fileHandler.handleInvalidZimFile(destinationFile, sourceUri) + + verify { fileHandler.dismissProgressDialog() } + verify { fileCopyMoveCallback.onError(activity.getString(R.string.error_file_invalid)) } + } + + @Test + fun `handleInvalidZimFile should delete file and show error if move fails`() { + every { fileHandler.tryMoveWithDocumentContract(any(), any(), any()) } returns false + fileHandler.isMoveOperation = true + + fileHandler.handleInvalidZimFile(destinationFile, sourceUri) + + verify { + fileHandler.handleFileOperationError( + activity.getString(R.string.error_file_invalid), + destinationFile + ) + } + } + + @Test + fun `isValidZimFile should return true if ZIM file has main entry`() = runTest { + val archive: Archive = mockk() + every { archive.hasMainEntry() } returns true + coEvery { ZimReaderSource(destinationFile).createArchive() } returns archive + + val result = fileHandler.isValidZimFile(destinationFile) + + assertTrue(result) + } + + @Test + fun `isValidZimFile should return false if ZIM file creation fails`() = runTest { + coEvery { ZimReaderSource(destinationFile).createArchive() } throws Exception() + + val result = fileHandler.isValidZimFile(destinationFile) + + assertFalse(result) + } + @AfterEach fun dispose() { fileHandler.dispose()