feat: jobsubmission abstraction

[JNdhlovu]

Story: https://app.shortcut.com/smileid/story/13730/improve-error-handling-on-android-and-ios-sdk
Improve Error Handling on android and ios sdk.

Summary

Better abstraction for

1. Smartselfie enrollment
2. Smartselfie authentication
3. Document verification
4. Enhanced document verification
5. Biometric KYC

Known Issues

n/a

Test Instructions

1. Test the jobs from the orchestrated screens paths
2. Test offline mode
3. Capture jobs and then use the submitJob method to make sure everything is working fine

Screenshot

n/a

[prfectionist]

PR Reviewer Guide 🔍

(Review updated until commit f4bce0e)

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Error Handling The error handling flow in executeApiSubmission() could be improved by adding more specific error types and recovery strategies beyond just wrapping in SmileIDResult.Error Possible Bug handleSubmissionFiles() is called after handleOfflineJobFailure() but before handleOfflineSuccess(), which could lead to inconsistent state if file operations fail Code Duplication The job submission handling code in submitSmartSelfieJob(), submitBiometricKYCJob(), etc. follows very similar patterns and could be refactored to reduce duplication

[prfectionist]

PR Code Suggestions ✨

Latest suggestions up to f4bce0e
Explore these optional code suggestions:

Category	Suggestion	Score
Possible issue	Implement missing file handling logic to properly update file references after job submission Implement the handleSubmissionFiles() method instead of leaving it as TODO. This method should handle updating the selfie and liveness files after a job submission, similar to other view models. lib/src/main/java/com/smileidentity/viewmodel/BiometricKycViewModel.kt [109-111] override fun handleSubmissionFiles(jobId: String) { - TODO("Not yet implemented") + this.selfieFile = getFileByType(jobId, FileType.SELFIE) + this.livenessFiles = getFilesByType(jobId, FileType.LIVENESS) } Suggestion importance[1-10]: 8 Why: The missing implementation could lead to incorrect file state management after job submission. The suggested fix properly updates file references, maintaining consistency with other view models.	8
	Add validation for required fields to fail fast with clear error messages Consider adding null checks for selfieFile and livenessFiles in createSubmission() to fail fast with descriptive error messages. lib/src/main/java/com/smileidentity/viewmodel/SelfieViewModel.kt [63-71] return SelfieSubmission( isEnroll = isEnroll, userId = userId, jobId = jobId, allowNewEnroll = allowNewEnroll, metadata = metadata.toList(), selfieFile = requireNotNull(selfieFile) { "Selfie file must be set before submission" }, - livenessFiles = livenessFiles.toList(), + livenessFiles = requireNotNull(livenessFiles) { "Liveness files must be set before submission" }.toList(), extraPartnerParams = extraPartnerParams, ) Suggestion importance[1-10]: 6 Why: Adding null check for livenessFiles would prevent potential runtime errors and provide clearer error messages, improving error handling and debugging.	6
Possible bug	Add null check for request parameter to prevent potential null pointer exceptions Add error handling for the case when prepUploadRequest is null in executePrepUploadWithRetry to prevent potential NullPointerException. lib/src/main/java/com/smileidentity/submissions/base/BaseJobSubmission.kt [104-108] private suspend fun executePrepUploadWithRetry( - prepUploadRequest: PrepUploadRequest, + prepUploadRequest: PrepUploadRequest?, isRetry: Boolean = false, ): PrepUploadResponse { return try { - SmileID.api.prepUpload(prepUploadRequest) + prepUploadRequest?.let { SmileID.api.prepUpload(it) } + ?: throw IllegalArgumentException("PrepUploadRequest cannot be null") } catch (e: HttpException) { Suggestion importance[1-10]: 8 Why: This suggestion addresses a potential null pointer vulnerability by adding proper null checks, which could prevent runtime crashes in production.	8
	Initialize result field with default error value to prevent potential null pointer exceptions Consider initializing result with a default error value to avoid potential null pointer exceptions when onFinished() is called before a result is set. lib/src/main/java/com/smileidentity/viewmodel/document/OrchestratedDocumentViewModel.kt [71-73] -// var result: SmileIDResult<T> = SmileIDResult.Error( -// IllegalStateException("Document Capture incomplete"), -// ) +var result: SmileIDResult<T> = SmileIDResult.Error( + IllegalStateException("Document Capture incomplete") +) Suggestion importance[1-10]: 7 Why: Uncommented code would prevent potential null pointer exceptions when onFinished() is called before a result is set, improving application stability.	7
Maintainability	Extract duplicate error handling logic into a reusable function to improve code maintainability Extract the duplicate error handling logic in the job submission methods into a reusable function to avoid code duplication. The same pattern of checking SmileIDResult.Success/Error and handling cleanup is repeated across all submission methods. lib/src/main/java/com/smileidentity/submissions/utils/JobSubmissionUtils.kt [39-51] -when (val result = submission.executeSubmission()) { - is SmileIDResult.Success -> { - if (deleteFilesOnSuccess) { - cleanup(jobId) +private fun handleSubmissionResult(result: SmileIDResult<*>, jobId: String, deleteFilesOnSuccess: Boolean) { + when (result) { + is SmileIDResult.Success -> { + if (deleteFilesOnSuccess) { + cleanup(jobId) + } + Timber.d("Upload finished successfully") } - Timber.d("Upload finished successfully") - } - is SmileIDResult.Error -> { - Timber.e(result.throwable, "Upload failed") - throw result.throwable + is SmileIDResult.Error -> { + Timber.e(result.throwable, "Upload failed") + throw result.throwable + } } } Suggestion importance[1-10]: 7 Why: The suggestion effectively addresses code duplication by extracting common error handling logic into a reusable function, improving maintainability and reducing the chance of inconsistent error handling.	7
Enhancement	Implement specific exception handling for network-related errors to improve error management Consider using a more specific exception handler for network-related errors in submitJob instead of handling all exceptions generically. lib/src/main/java/com/smileidentity/submissions/base/BaseSubmissionViewModel.kt [69-72] private fun getExceptionHandler(onError: (Throwable) -> Unit): CoroutineExceptionHandler = CoroutineExceptionHandler { _, throwable -> - onError(throwable) + when (throwable) { + is java.net.UnknownHostException, + is java.net.SocketException, + is java.io.IOException -> onError(throwable) + else -> throw throwable + } } Suggestion importance[1-10]: 6 Why: The suggestion improves error handling by differentiating between network-related and other exceptions, leading to more precise error management and debugging.	6

---

Previous suggestions

Suggestions up to commit 101831e

Category	Suggestion	Score
Possible issue	Implement missing file handling logic in submission callback method The handleSubmissionFiles() method is marked with TODO but should be implemented to properly handle file management after submission. Consider implementing it similar to other view models by updating selfie and liveness files from the job ID. lib/src/main/java/com/smileidentity/viewmodel/BiometricKycViewModel.kt [109-111] override fun handleSubmissionFiles(jobId: String) { - TODO("Not yet implemented") + this.selfieFile = getFileByType(jobId, FileType.SELFIE) + this.livenessFiles = getFilesByType(jobId, FileType.LIVENESS) } Suggestion importance[1-10]: 8 Why: The missing implementation could cause issues with file management after submission. The suggested implementation properly updates the file references which is critical for the offline mode functionality.	8
	Fix potential memory leak by properly using the tuple result from file operations The createSynchronousRes method has a potential memory leak - it creates a tuple but never uses it. The result of selfieFileResult to livenessFilesResult is discarded. lib/src/main/java/com/smileidentity/submissions/SelfieSubmission.kt [98-115] val copySuccess = moveJobToSubmitted(jobId) -if (copySuccess) { +val (finalSelfieFile, finalLivenessFiles) = if (copySuccess) { val selfieFileResult = getFileByType(jobId, FileType.SELFIE) ?: run { Timber.w("Selfie file not found for job ID: $jobId") throw IllegalStateException("Selfie file not found for job ID: $jobId") } val livenessFilesResult = getFilesByType(jobId, FileType.LIVENESS) selfieFileResult to livenessFilesResult } else { Timber.w("Failed to move job $jobId to complete") SmileIDCrashReporting.hub.addBreadcrumb( Breadcrumb().apply { category = "Offline Mode" message = "Failed to move job $jobId to complete" level = SentryLevel.INFO }, ) selfieFile to livenessFiles } Suggestion importance[1-10]: 8 Why: The suggestion identifies a real issue where the result of file operations is being discarded, which could lead to memory leaks and incorrect behavior. The proposed fix properly handles the tuple result.	8
Possible bug	Initialize nullable result variable properly instead of leaving it commented out The commented out result variable initialization should be properly initialized as a nullable type to match the parent class implementation and avoid potential null pointer exceptions. lib/src/main/java/com/smileidentity/viewmodel/document/OrchestratedDocumentViewModel.kt [71-73] -// var result: SmileIDResult<T> = SmileIDResult.Error( -// IllegalStateException("Document Capture incomplete"), -// ) +var result: SmileIDResult<T>? = SmileIDResult.Error( + IllegalStateException("Document Capture incomplete") +) Suggestion importance[1-10]: 7 Why: Properly initializing the result variable is important to maintain consistency with the parent class and prevent potential null pointer exceptions during result handling.	7
Maintainability	Extract duplicate error handling logic into a reusable function to improve code maintainability and reduce duplication Extract the duplicate error handling logic in the submit*Job functions into a reusable function to avoid code duplication. The same pattern of checking SmileIDResult.Success/Error is repeated in all submission functions. lib/src/main/java/com/smileidentity/submissions/utils/JobSubmissionUtils.kt [39-51] -when (val result = submission.executeSubmission()) { - is SmileIDResult.Success -> { - if (deleteFilesOnSuccess) { - cleanup(jobId) +private fun handleSubmissionResult(result: SmileIDResult<*>, jobId: String, deleteFilesOnSuccess: Boolean) { + when (result) { + is SmileIDResult.Success -> { + if (deleteFilesOnSuccess) { + cleanup(jobId) + } + Timber.d("Upload finished successfully") } - Timber.d("Upload finished successfully") - } - is SmileIDResult.Error -> { - Timber.e(result.throwable, "Upload failed") - throw result.throwable + is SmileIDResult.Error -> { + Timber.e(result.throwable, "Upload failed") + throw result.throwable + } } } Suggestion importance[1-10]: 7 Why: The suggestion identifies significant code duplication across submission functions and proposes a clean refactoring solution that would improve maintainability and reduce the chance of inconsistent error handling.	7
Best practice	Use specific exception types for error handling instead of checking error codes directly Consider using a more specific exception type for the retry error case instead of checking the error code directly. This would make the error handling more type-safe and explicit. lib/src/main/java/com/smileidentity/submissions/base/BaseJobSubmission.kt [111-116] -if (!isRetry && smileIDException.details.code == ERROR_CODE_RETRY) { +if (!isRetry && smileIDException is RetryableSmileIDException) { executePrepUploadWithRetry(prepUploadRequest.copy(retry = true), true) } else { throw smileIDException } Suggestion importance[1-10]: 6 Why: The suggestion promotes type-safe error handling by using specific exception types instead of error codes, which would make the code more maintainable and less prone to errors.	6

[github-actions]

This PR is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 7 days.

[github-actions]

This PR was closed because it has been stalled for 7 days with no activity.

[JNdhlovu]

Reopening as it was marked stale

[prfectionist]

Persistent review updated to latest commit f4bce0e