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.
This commit is contained in:
MohitMaliFtechiz 2024-10-30 23:16:00 +05:30 committed by Kelson
parent ab3d8ffd75
commit a45f94add7
3 changed files with 189 additions and 28 deletions

View File

@ -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()
}

View File

@ -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)

View File

@ -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<File>()
private val sourceUri = mockk<Uri>()
@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<Archive>(relaxed = true)
every { constructedWith<Archive>(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()