refactor(data): abstract data manager and migrate to KMP tests #3034
Conversation
📝 WalkthroughWalkthroughRepositories were refactored to depend on a new DataManagerProvider interface; DataManager now implements it and DI exposes DataManagerProvider. Multiplatform test utilities, fixtures, and fakes were added (JSON fixtures, JsonLoader, FakeDataManager, FakeClientService, tests). Numerous JVM Mockito test files were removed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Repo as Repository
participant Provider as DataManagerProvider
participant API as Service API
participant Network as KtorfitClient/Network
Note over Repo,Provider: Repository now resolves APIs via DataManagerProvider
Repo->>Provider: requireNotNull(provider.<api>)
alt API present
Provider-->>Repo: returns API
Repo->>API: call endpoint (suspend / returns Flow)
API->>Network: perform HTTP request
Network-->>API: HTTP response
API-->>Repo: Flow/response
Repo-->>Caller: emit asDataStateFlow / map / flowOn
else API missing
Provider-->>Repo: null
Repo-->>Caller: throw / emit DataState.Error ("... must be provided")
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/ShareAccountRepositoryImp.kt (1)
42-47: Bug:catchblock does not emit the error state.The
catchblock returnsDataState.Error(...)but doesn't actually emit it. Thecatchoperator's lambda should callemit()to propagate the error state downstream. As written, exceptions are caught but the flow terminates without emitting any error value.🔎 Proposed fix
override fun getShareProducts(clientId: Long?): Flow<DataState<Page<ShareProduct>>> { return shareAccountApi.getShareProducts(clientId) - .map { response -> DataState.Success(response) } - .catch { exception -> DataState.Error(exception, exception.message) } + .asDataStateFlow() .flowOn(ioDispatcher) }Alternatively, if you want custom error handling:
override fun getShareProducts(clientId: Long?): Flow<DataState<Page<ShareProduct>>> { return shareAccountApi.getShareProducts(clientId) .map { response -> DataState.Success(response) } - .catch { exception -> DataState.Error(exception, exception.message) } + .catch { exception -> emit(DataState.Error(exception, exception.message)) } .flowOn(ioDispatcher) }
🧹 Nitpick comments (14)
core/data/src/desktopTest/kotlin/org/mifos/mobile/core/data/utils/TestResourceReader.desktop.kt (1)
12-17: Add error handling for missing resources.The non-null assertions on lines 14-15 will throw NPEs without descriptive error messages if the classLoader is null or the resource doesn't exist. This makes debugging test failures more difficult.
🔎 Proposed fix with descriptive error handling
actual fun readResource(path: String): String { - return object {}.javaClass - .classLoader!! - .getResource(path)!! - .readText() + val classLoader = object {}.javaClass.classLoader + ?: error("Failed to get class loader") + val resource = classLoader.getResource(path) + ?: error("Resource not found: $path") + return resource.readText() }core/data/src/commonTest/kotlin/org/mifos/mobile/core/data/utils/TestFixtures.kt (1)
14-20: Consider lazy initialization to avoid repeated JSON parsing.The
get()accessor causes the JSON to be parsed every timeclientAccountsis accessed. If tests access this property multiple times, consider usinglazyinitialization for better performance.🔎 Proposed refactor using lazy initialization
object TestFixtures { - val clientAccounts - get() = JsonLoader.fromResource<ClientAccounts>( - "json/ClientAccounts.json", - ) + val clientAccounts by lazy { + JsonLoader.fromResource<ClientAccounts>( + "json/ClientAccounts.json", + ) + } }core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/ClientRepositoryImp.kt (1)
27-29: Consider makingclientsApiprivate.The
clientsApiproperty is public by default (no visibility modifier). Since it's only used internally within the repository, exposing it publicly breaks encapsulation and leaks implementation details. This pattern appears across multiple repositories in this PR.🔎 Proposed fix
- val clientsApi = requireNotNull(dataManager.clientsApi) { + private val clientsApi = requireNotNull(dataManager.clientsApi) { "ClientService must be provided" }core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/HomeRepositoryImp.kt (1)
31-33: Consider makingclientsApiprivate.Same encapsulation concern as in
ClientRepositoryImp- this property is used only internally and should not be part of the public API.🔎 Proposed fix
- val clientsApi = requireNotNull(dataManager.clientsApi) { + private val clientsApi = requireNotNull(dataManager.clientsApi) { "ClientService must be provided" }core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/ShareAccountRepositoryImp.kt (1)
38-40: Consider makingshareAccountApiprivate.Same encapsulation concern as in other repositories - this property is used only internally.
🔎 Proposed fix
- val shareAccountApi = requireNotNull(dataManager.shareAccountApi) { + private val shareAccountApi = requireNotNull(dataManager.shareAccountApi) { "ShareAccountService must be provided" }core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/TransferRepositoryImp.kt (1)
32-38: Consider making API properties private.Both
savingAccountsListApiandthirdPartyTransferApiare internal implementation details and should not be exposed publicly.🔎 Proposed fix
- val savingAccountsListApi = requireNotNull(dataManager.savingAccountsListApi) { + private val savingAccountsListApi = requireNotNull(dataManager.savingAccountsListApi) { "SavingAccountsListService must be provided" } - val thirdPartyTransferApi = requireNotNull(dataManager.thirdPartyTransferApi) { + private val thirdPartyTransferApi = requireNotNull(dataManager.thirdPartyTransferApi) { "ThirdPartyTransferService must be provided" }core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/ReviewLoanApplicationRepositoryImpl.kt (1)
43-43: Replace debug println statements with proper logging.The
printlnstatements should be replaced with a proper logging framework for better observability and production readiness. Debug output to stdout is not suitable for production code.💡 Consider using a logging framework
- println("response $response") + // Log response if needed using proper loggerAlso applies to: 48-48
core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/ClientChargeRepositoryImp.kt (1)
28-28: Consider removing or tracking commented-out database code.The commented-out code for
chargeDaoand local database operations appears to be legacy code. Consider either:
- Removing it if the database layer is no longer planned
- Creating a tracking issue if this functionality will be implemented later
- Adding a comment explaining why it's temporarily disabled
Also applies to: 54-67
core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/BeneficiaryRepositoryImp.kt (1)
34-37: Consider makingbeneficiaryApiprivate to maintain encapsulation.The
beneficiaryApiproperty is declared asval(public), which exposes an internal implementation detail. This could allow external callers to bypass repository methods and access the API directly.Suggested fix
- val beneficiaryApi = requireNotNull(dataManager.beneficiaryApi) { + private val beneficiaryApi = requireNotNull(dataManager.beneficiaryApi) { "BeneficiaryService must be provided" }core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/ThirdPartyTransferRepositoryImp.kt (1)
25-28: Consider makingthirdPartyTransferApiprivate.Same encapsulation concern as noted in other repositories—exposing internal API access publicly.
Suggested fix
- val thirdPartyTransferApi = requireNotNull(dataManager.thirdPartyTransferApi) { + private val thirdPartyTransferApi = requireNotNull(dataManager.thirdPartyTransferApi) { "ThirdPartyTransferService must be provided" }core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/UserAuthRepositoryImp.kt (2)
33-43: Consider making API properties private for encapsulation.All three API properties (
registrationApi,authenticationApi,userDetailsApi) are public. Consider making themprivateto prevent external access to internal services.Suggested fix
- val registrationApi = requireNotNull(dataManager.registrationApi) { + private val registrationApi = requireNotNull(dataManager.registrationApi) { "RegistrationService must be provided" } - val authenticationApi = requireNotNull(dataManager.authenticationApi) { + private val authenticationApi = requireNotNull(dataManager.authenticationApi) { "AuthenticationService must be provided" } - val userDetailsApi = requireNotNull(dataManager.userDetailsApi) { + private val userDetailsApi = requireNotNull(dataManager.userDetailsApi) { "UserDetailsService must be provided" }
63-85: Inconsistent try-catch placement compared to other methods.In
login(), the try-catch wrapswithContext, while inregisterUser(),verifyUser(), andupdateAccountPassword(), the try-catch is insidewithContext. This inconsistency could lead to subtle differences in exception propagation behavior and makes the code harder to maintain.Suggested fix for consistency
override suspend fun login(username: String, password: String): DataState<User> { val loginPayload = LoginPayload( username = username, password = password, ) - return try { - withContext(ioDispatcher) { + return withContext(ioDispatcher) { + try { val user = authenticationApi.authenticate(loginPayload) if (user.base64EncodedAuthenticationKey != null) { DataState.Success(user) } else { DataState.Error(Exception("Invalid Credentials"), null) } + } catch (e: ClientRequestException) { + val errorMessage = extractErrorMessage(e.response) + DataState.Error(Exception(errorMessage), null) + } catch (e: IOException) { + DataState.Error(Exception("Network error: ${e.message ?: "Please check your connection"}"), null) + } catch (e: ServerResponseException) { + DataState.Error(Exception("Server error: ${e.message}"), null) } - } catch (e: ClientRequestException) { - val errorMessage = extractErrorMessage(e.response) - DataState.Error(Exception(errorMessage), null) - } catch (e: IOException) { - DataState.Error(Exception("Network error: ${e.message ?: "Please check your connection"}"), null) - } catch (e: ServerResponseException) { - DataState.Error(Exception("Server error: ${e.message}"), null) } }core/data/src/commonTest/kotlin/org/mifos/mobile/core/data/repoTests/AccountsRepositoryTest.kt (1)
109-111: Inconsistent assertion style for checking empty lists.Line 109 uses
assertEquals(...?.isEmpty(), true)while lines 110-111 useassertTrue(...isEmpty()). The nullable-safe approach on line 109 suggestssavingsAccountsmight be nullable, but the assertion style is inconsistent.Suggested fix for consistency
- assertEquals(data.savingsAccounts?.isEmpty(), true) - assertTrue(data.loanAccounts.isEmpty()) - assertTrue(data.shareAccounts.isEmpty()) + assertTrue(data.savingsAccounts?.isEmpty() == true) + assertTrue(data.loanAccounts.isEmpty()) + assertTrue(data.shareAccounts.isEmpty())core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/SavingsAccountRepositoryImp.kt (1)
38-40: Consider makingsavingAccountsListApiprivate.Same encapsulation concern as other repositories.
Suggested fix
- val savingAccountsListApi = requireNotNull(dataManager.savingAccountsListApi) { + private val savingAccountsListApi = requireNotNull(dataManager.savingAccountsListApi) { "SavingAccountsListService must be provided" }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (46)
core/data/build.gradle.ktscore/data/src/androidUnitTest/kotlin/org/mifos/mobile/core/data/utils/TestResourceReader.android.ktcore/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/AccountsRepositoryImp.ktcore/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/BeneficiaryRepositoryImp.ktcore/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/ClientChargeRepositoryImp.ktcore/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/ClientRepositoryImp.ktcore/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/GuarantorRepositoryImp.ktcore/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/HomeRepositoryImp.ktcore/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/LoanRepositoryImp.ktcore/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/RecentTransactionRepositoryImp.ktcore/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/ReviewLoanApplicationRepositoryImpl.ktcore/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/SavingsAccountRepositoryImp.ktcore/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/ShareAccountRepositoryImp.ktcore/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/ThirdPartyTransferRepositoryImp.ktcore/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/TransferRepositoryImp.ktcore/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/UserAuthRepositoryImp.ktcore/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/UserDetailRepositoryImp.ktcore/data/src/commonTest/kotlin/org/mifos/mobile/core/data/fakeServices/FakeClientService.ktcore/data/src/commonTest/kotlin/org/mifos/mobile/core/data/fakeServices/FakeDataManager.ktcore/data/src/commonTest/kotlin/org/mifos/mobile/core/data/repoTests/AccountsRepositoryTest.ktcore/data/src/commonTest/kotlin/org/mifos/mobile/core/data/utils/JsonLoader.ktcore/data/src/commonTest/kotlin/org/mifos/mobile/core/data/utils/TestFixtures.ktcore/data/src/commonTest/kotlin/org/mifos/mobile/core/data/utils/TestResourceReader.ktcore/data/src/commonTest/resources/json/ClientAccounts.jsoncore/data/src/desktopTest/kotlin/org/mifos/mobile/core/data/utils/TestResourceReader.desktop.ktcore/data/src/jsCommonTest/kotlin/org/mifos/mobile/core/data/utils/TestResourceReader.jsCommon.ktcore/data/src/nativeTest/kotlin/org/mifos/mobile/core/data/utils/TestResourceReader.native.ktcore/data/src/test/kotlin/org/mifos/mobile/core/data/repositories/AccountsRepositoryImpTest.ktcore/data/src/test/kotlin/org/mifos/mobile/core/data/repositories/BeneficiaryRepositoryImpTest.ktcore/data/src/test/kotlin/org/mifos/mobile/core/data/repositories/ClientChargeRepositoryImpTest.ktcore/data/src/test/kotlin/org/mifos/mobile/core/data/repositories/ClientRepositoryImpTest.ktcore/data/src/test/kotlin/org/mifos/mobile/core/data/repositories/GuarantorRepositoryImpTest.ktcore/data/src/test/kotlin/org/mifos/mobile/core/data/repositories/HomeRepositoryImpTest.ktcore/data/src/test/kotlin/org/mifos/mobile/core/data/repositories/LoanRepositoryImpTest.ktcore/data/src/test/kotlin/org/mifos/mobile/core/data/repositories/NotificationRepositoryImpTest.ktcore/data/src/test/kotlin/org/mifos/mobile/core/data/repositories/RecentTransactionRepositoryImpTest.ktcore/data/src/test/kotlin/org/mifos/mobile/core/data/repositories/SavingsAccountRepositoryImpTest.ktcore/data/src/test/kotlin/org/mifos/mobile/core/data/repositories/ThirdPartyTransferRepositoryImpTest.ktcore/data/src/test/kotlin/org/mifos/mobile/core/data/repositories/TransferRepositoryImpTest.ktcore/data/src/test/kotlin/org/mifos/mobile/core/data/repositories/UserAuthRepositoryImpTest.ktcore/data/src/test/kotlin/org/mifos/mobile/core/data/repositories/UserDetailRepositoryImpTest.ktcore/data/src/test/resources/mockito-extensions/org.mockito.plugins.MockMakercore/network/src/commonMain/kotlin/org/mifos/mobile/core/network/DataManager.ktcore/network/src/commonMain/kotlin/org/mifos/mobile/core/network/DataManagerProvider.ktcore/network/src/commonMain/kotlin/org/mifos/mobile/core/network/di/NetworkModule.ktgradle/libs.versions.toml
💤 Files with no reviewable changes (15)
- core/data/src/test/kotlin/org/mifos/mobile/core/data/repositories/UserAuthRepositoryImpTest.kt
- core/data/src/test/kotlin/org/mifos/mobile/core/data/repositories/NotificationRepositoryImpTest.kt
- core/data/src/test/kotlin/org/mifos/mobile/core/data/repositories/HomeRepositoryImpTest.kt
- core/data/src/test/kotlin/org/mifos/mobile/core/data/repositories/RecentTransactionRepositoryImpTest.kt
- core/data/src/test/kotlin/org/mifos/mobile/core/data/repositories/SavingsAccountRepositoryImpTest.kt
- core/data/src/test/kotlin/org/mifos/mobile/core/data/repositories/TransferRepositoryImpTest.kt
- core/data/src/test/kotlin/org/mifos/mobile/core/data/repositories/ClientChargeRepositoryImpTest.kt
- core/data/src/test/kotlin/org/mifos/mobile/core/data/repositories/LoanRepositoryImpTest.kt
- core/data/src/test/kotlin/org/mifos/mobile/core/data/repositories/BeneficiaryRepositoryImpTest.kt
- core/data/src/test/kotlin/org/mifos/mobile/core/data/repositories/ClientRepositoryImpTest.kt
- core/data/src/test/kotlin/org/mifos/mobile/core/data/repositories/AccountsRepositoryImpTest.kt
- core/data/src/test/kotlin/org/mifos/mobile/core/data/repositories/UserDetailRepositoryImpTest.kt
- core/data/src/test/resources/mockito-extensions/org.mockito.plugins.MockMaker
- core/data/src/test/kotlin/org/mifos/mobile/core/data/repositories/ThirdPartyTransferRepositoryImpTest.kt
- core/data/src/test/kotlin/org/mifos/mobile/core/data/repositories/GuarantorRepositoryImpTest.kt
🧰 Additional context used
🧬 Code graph analysis (1)
core/data/src/commonTest/kotlin/org/mifos/mobile/core/data/utils/JsonLoader.kt (5)
core/data/src/androidUnitTest/kotlin/org/mifos/mobile/core/data/utils/TestResourceReader.android.kt (1)
readResource(12-19)core/data/src/commonTest/kotlin/org/mifos/mobile/core/data/utils/TestResourceReader.kt (1)
readResource(12-12)core/data/src/desktopTest/kotlin/org/mifos/mobile/core/data/utils/TestResourceReader.desktop.kt (1)
readResource(12-17)core/data/src/jsCommonTest/kotlin/org/mifos/mobile/core/data/utils/TestResourceReader.jsCommon.kt (1)
readResource(12-14)core/data/src/nativeTest/kotlin/org/mifos/mobile/core/data/utils/TestResourceReader.native.kt (1)
readResource(18-31)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: PR Checks KMP / Static Analysis Check
🔇 Additional comments (36)
core/data/build.gradle.kts (2)
26-28: LGTM! Proper test resource configuration for KMP.The configuration correctly points the Android test resources to the commonTest resources directory, enabling resource sharing across platforms.
50-57: LGTM! Standard KMP test dependencies.The commonTest dependencies are well-chosen for KMP testing:
kotlin.testandkotlin.test.annotations.commonfor cross-platform test APIskotlinx.coroutines.testfor coroutine testing utilitiesturbinefor Flow testingkoin.testfor dependency injection testinggradle/libs.versions.toml (1)
245-245: LGTM! Version catalog entry for KMP test annotations.The library entry correctly references the Kotlin version and follows the established naming pattern in the catalog.
core/data/src/commonTest/kotlin/org/mifos/mobile/core/data/fakeServices/FakeDataManager.kt (1)
27-41: LGTM! Clean test fake implementation.The
FakeDataManagerproperly implementsDataManagerProviderwith nullable service properties that default to null. This enables flexible test setup where only required services need to be provided.core/data/src/commonTest/kotlin/org/mifos/mobile/core/data/utils/TestResourceReader.kt (1)
12-12: LGTM! Clean multiplatform expect declaration.The expect function provides a clear contract for platform-specific resource reading implementations.
core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/GuarantorRepositoryImp.kt (2)
26-35: LGTM! Clean migration to DataManagerProvider.The refactoring properly introduces the
DataManagerProviderabstraction with a clear null-safety check. TherequireNotNullprovides a descriptive error message that will help diagnose configuration issues during testing or runtime.
37-79: LGTM! Consistent usage of local API property.All method implementations correctly use the local
guarantorApiproperty instead of accessing through the data manager. This improves readability and ensures the null-check is only performed once during initialization.core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/ClientRepositoryImp.kt (1)
31-34: LGTM!The
loadClient()implementation correctly uses the newclientsApiproperty and maintains the existing flow behavior withasDataStateFlow()andflowOn(ioDispatcher).core/data/src/commonTest/kotlin/org/mifos/mobile/core/data/utils/JsonLoader.kt (1)
14-23: LGTM!The
JsonLoaderutility is well-structured for test fixtures. The JSON configuration (ignoreUnknownKeys = true,explicitNulls = false) is appropriate for test data that may evolve independently. The inline reified function provides type-safe deserialization.core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/HomeRepositoryImp.kt (1)
44-59: LGTM!The
clientImageimplementation correctly handles the base64 data URI scheme by extracting the pure base64 content after the comma. The exhaustivewhenmapping of allDataStatevariants ensures proper error and loading state propagation.core/network/src/commonMain/kotlin/org/mifos/mobile/core/network/di/NetworkModule.kt (1)
43-45: LGTM!The DI binding correctly exposes
DataManagerasDataManagerProvider, enabling interface-based dependency injection. This is the key change that allows repositories to depend on the abstraction and enables test fakes to be substituted.core/data/src/androidUnitTest/kotlin/org/mifos/mobile/core/data/utils/TestResourceReader.android.kt (1)
12-18: LGTM!The Android implementation provides proper error handling with a descriptive message and uses
use {}for safe resource cleanup. This serves as a good reference for the native implementation which should follow the same error-handling pattern.core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/TransferRepositoryImp.kt (1)
40-62: LGTM!The transfer routing logic correctly directs SELF transfers through
savingAccountsListApiand other types throughthirdPartyTransferApi. The generic error message forServerResponseExceptionappropriately avoids exposing internal server details to users.core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/AccountsRepositoryImp.kt (2)
19-19: LGTM! Improved abstraction with DataManagerProvider.The migration from
DataManagertoDataManagerProviderimproves abstraction and aligns with the PR's architectural goals of introducing a provider interface for better dependency injection.Also applies to: 22-22
30-37: Verify null-safety: potential NPE from force-unwrapping clientId.The implementation correctly uses
requireNotNullfor early API validation. However,clientId!!on Line 35 will throw an NPE ifclientIdis null. Since the method signature acceptsclientIdas nullable (Long?), consider either:
- Making
clientIdnon-nullable in the signature if it's always required, or- Validating
clientIdwithrequireNotNulland providing a descriptive error message🔎 Suggested improvement for null-safety
val clientsApi = requireNotNull(dataManager.clientsApi) { "ClientService must be provided" } + val safeClientId = requireNotNull(clientId) { + "Client ID must be provided" + } return clientsApi - .getAccounts(clientId!!, accountType) + .getAccounts(safeClientId, accountType) .asDataStateFlow() .flowOn(ioDispatcher)core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/ReviewLoanApplicationRepositoryImpl.kt (2)
22-22: LGTM! Consistent migration to DataManagerProvider.The constructor update aligns with the provider-based abstraction pattern introduced in this PR.
Also applies to: 25-25
29-31: LGTM! Appropriate use of requireNotNull for API initialization.The property-level initialization with
requireNotNullprovides early failure detection with a clear error message, ensuring the service is available before use.core/data/src/commonTest/resources/json/ClientAccounts.json (2)
1-554: LGTM! Comprehensive test fixture for KMP testing.The test resource provides comprehensive account data covering savings, loan, and share accounts with varied statuses and transaction histories. This aligns well with the PR's objective to introduce KMP-compatible tests using JSON-based test fixtures.
194-194: Verify: Unusually large account balance for testing.Line 194 contains an extremely large
accountBalancevalue (3117780552329.610000). If this is intentional for edge-case testing (e.g., testing large number handling or overflow scenarios), consider adding a comment in the test that uses this fixture to explain the rationale.core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/LoanRepositoryImp.kt (4)
29-29: LGTM! Consistent provider-based dependency injection.The migration to
DataManagerProvidermaintains consistency with the architectural refactoring across repository implementations.Also applies to: 32-32
36-38: LGTM! Property-level API initialization with clear error message.The
requireNotNullcheck at the property level ensures early failure with a descriptive error message, improving debuggability.
44-51: Good addition of try-catch for error handling.Wrapping the API call in a try-catch block and emitting
DataState.Errorimproves resilience compared to letting exceptions propagate. However, note thatloanId!!on Line 45 will throw an NPE ifloanIdis null.Consider validating
loanIdexplicitly:): Flow<DataState<LoanWithAssociations?>> = flow { try { - loanAccountsListApi.getLoanWithAssociations(loanId!!, associationType) + val safeLoanId = requireNotNull(loanId) { "Loan ID must be provided" } + loanAccountsListApi.getLoanWithAssociations(safeLoanId, associationType)
64-82: Verify null-safety: potential NPE from loanId!! operator.Line 71 uses
loanId!!which will throw an NPE ifloanIdis null. Since the method signature accepts nullableLong?, ensure upstream callers always provide non-null values or add explicit validation.core/data/src/commonTest/kotlin/org/mifos/mobile/core/data/fakeServices/FakeClientService.kt (2)
20-31: LGTM! Well-designed test fake with error scenario support.The
FakeClientServiceimplementation is appropriate for KMP testing. TheshouldFailparameter enables testing error scenarios, and the ability to inject customaccountsdata supports various test cases.
33-36: Acceptable use of TODO() for unused test fake methods.Using
TODO()for unimplemented methods in a test fake is acceptable, as these methods are not required for the current test scenarios. If future tests need these methods, they can be implemented incrementally.core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/ClientChargeRepositoryImp.kt (2)
24-24: LGTM! Consistent migration to DataManagerProvider.The constructor update maintains consistency with the architectural refactoring pattern.
Also applies to: 27-27
32-38: LGTM! Dual API initialization with clear error messages.Initializing both
clientChargeApiandshareAccountApias properties withrequireNotNullensures early failure detection for both dependencies.core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/UserDetailRepositoryImp.kt (3)
21-21: LGTM! Consistent provider-based abstraction.The migration to
DataManagerProvidermaintains architectural consistency across repository implementations.Also applies to: 24-24
28-30: LGTM! Clear API initialization with descriptive error message.The
requireNotNullcheck ensures the notification service is available before use, with a clear error message for debugging.
31-59: LGTM! Error handling preserved correctly.The refactoring maintains the existing error handling patterns while using the new
notificationApiproperty, ensuring no regression in error recovery behavior.core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/RecentTransactionRepositoryImp.kt (3)
20-20: LGTM! Consistent architectural refactoring.The migration to
DataManagerProvideraligns with the PR's objective to improve abstraction across repository implementations.Also applies to: 23-23
27-29: LGTM! Property-level API initialization with clear error message.The
requireNotNullcheck provides early failure detection with a descriptive error message.
31-41: Verify null-safety: potential NPE from clientId!! operator.Line 37 uses
clientId!!which will throw an NPE ifclientIdis null. Since the method signature acceptsclientIdas nullable (Long?), consider adding explicit validation:🔎 Suggested improvement
): Flow<DataState<Page<Transaction>>> { + val safeClientId = requireNotNull(clientId) { "Client ID must be provided" } return recentTransactionsApi.getRecentTransactionsList( - clientId!!, + safeClientId, offset, limit, ).asDataStateFlow().flowOn(ioDispatcher)core/data/src/commonTest/kotlin/org/mifos/mobile/core/data/repoTests/AccountsRepositoryTest.kt (1)
29-142: Good test structure with comprehensive coverage.The test class covers success, empty data, and error scenarios using Turbine for Flow testing. The use of fakes (FakeDataManager, FakeClientService) aligns well with the KMP testing strategy.
core/network/src/commonMain/kotlin/org/mifos/mobile/core/network/DataManager.kt (1)
12-41: Clean implementation of DataManagerProvider interface.The
DataManagercorrectly implementsDataManagerProvider, overriding all properties with lazy initialization fromktorfitClient. This maintains the existing lazy-loading behavior while providing the abstraction needed for testing.core/network/src/commonMain/kotlin/org/mifos/mobile/core/network/DataManagerProvider.kt (1)
26-40: Nullable API design is well-implemented with consistent consumer safety checks.All 13 service properties are nullable, enabling flexible testing where
FakeDataManagerprovides only required services. Every repository properly validates this at initialization viarequireNotNull()with descriptive error messages (e.g., "ClientService must be provided"), eliminating the need for scattered null checks throughout method code. The productionDataManagerprovides non-null lazy properties.
...ata/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/AccountsRepositoryImp.kt
Outdated
Show resolved
Hide resolved
.../src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/BeneficiaryRepositoryImp.kt
Show resolved
Hide resolved
.../src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/BeneficiaryRepositoryImp.kt
Show resolved
Hide resolved
...c/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/SavingsAccountRepositoryImp.kt
Show resolved
Hide resolved
...data/src/jsCommonTest/kotlin/org/mifos/mobile/core/data/utils/TestResourceReader.jsCommon.kt
Show resolved
Hide resolved
core/data/src/nativeTest/kotlin/org/mifos/mobile/core/data/utils/TestResourceReader.native.kt
Show resolved
Hide resolved
Nagarjuna0033
force-pushed
the
MM-477
branch
from
b0d5a9b to
93073df
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (5)
core/data/src/nativeTest/kotlin/org/mifos/mobile/core/data/utils/TestResourceReader.native.kt (1)
23-30: Address the error handling issues flagged in the previous review.The force unwrap on
resourcePath!!and theerror = nullparameter remain unaddressed from the previous review. These will cause test failures with unclear error messages when resources are missing or cannot be read, making debugging difficult.Please implement the error handling suggested in the previous review to ensure tests fail with actionable error messages.
core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/AccountsRepositoryImp.kt (1)
27-28: Dead code should be removed.The commented-out implementation is no longer needed and should be removed to keep the codebase clean.
core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/SavingsAccountRepositoryImp.kt (1)
46-49: Validate nullable accountId before force-unwrap.Using
accountId!!on line 47 can cause NPE ifaccountIdis null. Consider validating upfront and returning an error state.core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/BeneficiaryRepositoryImp.kt (2)
73-73: Force-unwrap on nullable parameter remains unaddressed.As noted in previous reviews, using
beneficiaryId!!will throwNullPointerExceptionif null is passed. Consider validating upfront and returningDataState.Errorwith a descriptive message instead.
89-89: Same force-unwrap issue as in updateBeneficiary.Apply the same null-safety handling as suggested for
updateBeneficiary.
🧹 Nitpick comments (3)
core/data/src/commonTest/kotlin/org/mifos/mobile/core/data/repoTests/AccountsRepositoryTest.kt (1)
109-111: Inconsistent nullable handling in assertions.Line 109 uses safe-call operator (
savingsAccounts?.isEmpty()) treating the property as nullable, while lines 110-111 directly accessloanAccountsandshareAccountswithout null-safety. According to theClientAccountsmodel, these properties should have consistent nullability handling.🔎 Suggested fix for consistency
- assertEquals(data.savingsAccounts?.isEmpty(), true) - assertTrue(data.loanAccounts.isEmpty()) - assertTrue(data.shareAccounts.isEmpty()) + assertTrue(data.savingsAccounts?.isEmpty() == true) + assertTrue(data.loanAccounts?.isEmpty() == true) + assertTrue(data.shareAccounts?.isEmpty() == true)core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/LoanRepositoryImp.kt (1)
43-52: Consider consistent error handling pattern.The
getLoanWithAssociationsmethod uses a genericcatch (exception: Exception)block, while other methods in this repository use specific exception types (ClientRequestException,IOException,ServerResponseException) with tailored error messages.Consider aligning the error handling pattern for consistency and better error diagnostics.
🔎 Suggested pattern alignment
override fun getLoanWithAssociations( associationType: String?, loanId: Long?, ): Flow<DataState<LoanWithAssociations?>> = flow { + emit(DataState.Loading) try { - loanAccountsListApi.getLoanWithAssociations(loanId!!, associationType) - .collect { response -> - emit(DataState.Success(response)) - } - } catch (exception: Exception) { - emit(DataState.Error(exception)) + val response = loanAccountsListApi.getLoanWithAssociations(loanId!!, associationType) + emit(DataState.Success(response)) + } catch (e: ClientRequestException) { + val errorMessage = extractErrorMessage(e.response) + emit(DataState.Error(Exception(errorMessage))) + } catch (e: IOException) { + emit(DataState.Error(Exception("Network error: ${e.message ?: "Please check your connection"}"))) + } catch (e: ServerResponseException) { + emit(DataState.Error(Exception("Server error: ${e.message}"))) } }.flowOn(ioDispatcher)core/network/src/commonMain/kotlin/org/mifos/mobile/core/network/DataManagerProvider.kt (1)
27-39: All service properties are nullable with pervasiverequireNotNullchecks.The nullable design is intentional for test flexibility—
FakeDataManagerallows selective service injection. However, every repository implementation usesrequireNotNullto enforce initialization at runtime (e.g.,val clientsApi = requireNotNull(dataManager.clientsApi) { "Service must be provided" }). If all services are mandatory, consider making them non-nullable in the interface to catch missing dependencies at compile-time rather than requiring runtime checks across 11+ repositories.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (46)
core/data/build.gradle.ktscore/data/src/androidUnitTest/kotlin/org/mifos/mobile/core/data/utils/TestResourceReader.android.ktcore/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/AccountsRepositoryImp.ktcore/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/BeneficiaryRepositoryImp.ktcore/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/ClientChargeRepositoryImp.ktcore/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/ClientRepositoryImp.ktcore/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/GuarantorRepositoryImp.ktcore/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/HomeRepositoryImp.ktcore/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/LoanRepositoryImp.ktcore/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/RecentTransactionRepositoryImp.ktcore/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/ReviewLoanApplicationRepositoryImpl.ktcore/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/SavingsAccountRepositoryImp.ktcore/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/ShareAccountRepositoryImp.ktcore/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/ThirdPartyTransferRepositoryImp.ktcore/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/TransferRepositoryImp.ktcore/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/UserAuthRepositoryImp.ktcore/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/UserDetailRepositoryImp.ktcore/data/src/commonTest/kotlin/org/mifos/mobile/core/data/fakeServices/FakeClientService.ktcore/data/src/commonTest/kotlin/org/mifos/mobile/core/data/fakeServices/FakeDataManager.ktcore/data/src/commonTest/kotlin/org/mifos/mobile/core/data/repoTests/AccountsRepositoryTest.ktcore/data/src/commonTest/kotlin/org/mifos/mobile/core/data/utils/JsonLoader.ktcore/data/src/commonTest/kotlin/org/mifos/mobile/core/data/utils/TestFixtures.ktcore/data/src/commonTest/kotlin/org/mifos/mobile/core/data/utils/TestResourceReader.ktcore/data/src/commonTest/resources/json/ClientAccounts.jsoncore/data/src/desktopTest/kotlin/org/mifos/mobile/core/data/utils/TestResourceReader.desktop.ktcore/data/src/jsCommonTest/kotlin/org/mifos/mobile/core/data/utils/TestResourceReader.jsCommon.ktcore/data/src/nativeTest/kotlin/org/mifos/mobile/core/data/utils/TestResourceReader.native.ktcore/data/src/test/kotlin/org/mifos/mobile/core/data/repositories/AccountsRepositoryImpTest.ktcore/data/src/test/kotlin/org/mifos/mobile/core/data/repositories/BeneficiaryRepositoryImpTest.ktcore/data/src/test/kotlin/org/mifos/mobile/core/data/repositories/ClientChargeRepositoryImpTest.ktcore/data/src/test/kotlin/org/mifos/mobile/core/data/repositories/ClientRepositoryImpTest.ktcore/data/src/test/kotlin/org/mifos/mobile/core/data/repositories/GuarantorRepositoryImpTest.ktcore/data/src/test/kotlin/org/mifos/mobile/core/data/repositories/HomeRepositoryImpTest.ktcore/data/src/test/kotlin/org/mifos/mobile/core/data/repositories/LoanRepositoryImpTest.ktcore/data/src/test/kotlin/org/mifos/mobile/core/data/repositories/NotificationRepositoryImpTest.ktcore/data/src/test/kotlin/org/mifos/mobile/core/data/repositories/RecentTransactionRepositoryImpTest.ktcore/data/src/test/kotlin/org/mifos/mobile/core/data/repositories/SavingsAccountRepositoryImpTest.ktcore/data/src/test/kotlin/org/mifos/mobile/core/data/repositories/ThirdPartyTransferRepositoryImpTest.ktcore/data/src/test/kotlin/org/mifos/mobile/core/data/repositories/TransferRepositoryImpTest.ktcore/data/src/test/kotlin/org/mifos/mobile/core/data/repositories/UserAuthRepositoryImpTest.ktcore/data/src/test/kotlin/org/mifos/mobile/core/data/repositories/UserDetailRepositoryImpTest.ktcore/data/src/test/resources/mockito-extensions/org.mockito.plugins.MockMakercore/network/src/commonMain/kotlin/org/mifos/mobile/core/network/DataManager.ktcore/network/src/commonMain/kotlin/org/mifos/mobile/core/network/DataManagerProvider.ktcore/network/src/commonMain/kotlin/org/mifos/mobile/core/network/di/NetworkModule.ktgradle/libs.versions.toml
💤 Files with no reviewable changes (15)
- core/data/src/test/kotlin/org/mifos/mobile/core/data/repositories/LoanRepositoryImpTest.kt
- core/data/src/test/kotlin/org/mifos/mobile/core/data/repositories/TransferRepositoryImpTest.kt
- core/data/src/test/resources/mockito-extensions/org.mockito.plugins.MockMaker
- core/data/src/test/kotlin/org/mifos/mobile/core/data/repositories/BeneficiaryRepositoryImpTest.kt
- core/data/src/test/kotlin/org/mifos/mobile/core/data/repositories/GuarantorRepositoryImpTest.kt
- core/data/src/test/kotlin/org/mifos/mobile/core/data/repositories/AccountsRepositoryImpTest.kt
- core/data/src/test/kotlin/org/mifos/mobile/core/data/repositories/UserDetailRepositoryImpTest.kt
- core/data/src/test/kotlin/org/mifos/mobile/core/data/repositories/RecentTransactionRepositoryImpTest.kt
- core/data/src/test/kotlin/org/mifos/mobile/core/data/repositories/UserAuthRepositoryImpTest.kt
- core/data/src/test/kotlin/org/mifos/mobile/core/data/repositories/ThirdPartyTransferRepositoryImpTest.kt
- core/data/src/test/kotlin/org/mifos/mobile/core/data/repositories/ClientRepositoryImpTest.kt
- core/data/src/test/kotlin/org/mifos/mobile/core/data/repositories/HomeRepositoryImpTest.kt
- core/data/src/test/kotlin/org/mifos/mobile/core/data/repositories/NotificationRepositoryImpTest.kt
- core/data/src/test/kotlin/org/mifos/mobile/core/data/repositories/SavingsAccountRepositoryImpTest.kt
- core/data/src/test/kotlin/org/mifos/mobile/core/data/repositories/ClientChargeRepositoryImpTest.kt
🚧 Files skipped from review as they are similar to previous changes (17)
- core/data/src/commonTest/kotlin/org/mifos/mobile/core/data/utils/TestFixtures.kt
- gradle/libs.versions.toml
- core/data/src/commonTest/kotlin/org/mifos/mobile/core/data/fakeServices/FakeDataManager.kt
- core/data/src/jsCommonTest/kotlin/org/mifos/mobile/core/data/utils/TestResourceReader.jsCommon.kt
- core/data/src/commonTest/kotlin/org/mifos/mobile/core/data/utils/JsonLoader.kt
- core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/UserDetailRepositoryImp.kt
- core/data/src/desktopTest/kotlin/org/mifos/mobile/core/data/utils/TestResourceReader.desktop.kt
- core/data/src/commonTest/kotlin/org/mifos/mobile/core/data/fakeServices/FakeClientService.kt
- core/data/src/commonTest/kotlin/org/mifos/mobile/core/data/utils/TestResourceReader.kt
- core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/ThirdPartyTransferRepositoryImp.kt
- core/data/src/commonTest/resources/json/ClientAccounts.json
- core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/GuarantorRepositoryImp.kt
- core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/RecentTransactionRepositoryImp.kt
- core/network/src/commonMain/kotlin/org/mifos/mobile/core/network/DataManager.kt
- core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/ClientRepositoryImp.kt
- core/data/build.gradle.kts
- core/data/src/androidUnitTest/kotlin/org/mifos/mobile/core/data/utils/TestResourceReader.android.kt
🔇 Additional comments (16)
core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/HomeRepositoryImp.kt (2)
31-33: LGTM - Fail-fast initialization pattern.The
requireNotNullcheck at property initialization ensures the repository fails immediately during construction ifClientServiceis unavailable, rather than deferring the error until first use. This provides clear feedback and is consistent with the refactoring pattern applied across all repositories.
45-58: LGTM - Response mapping preserves error handling.The
clientImagemethod correctly maps the response to extract Base64 data while preserving allDataStatevariants (Success, Error, Loading) and maintaining theflowOn(ioDispatcher)control.core/network/src/commonMain/kotlin/org/mifos/mobile/core/network/di/NetworkModule.kt (1)
43-45: LGTM - Proper DI abstraction.Binding
DataManagerasDataManagerProvidercorrectly exposes the interface type for dependency injection while constructing the concrete implementation. This follows standard DI best practices and enables repositories to depend on the abstraction.core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/AccountsRepositoryImp.kt (1)
30-37: LGTM - Consistent API access pattern.The
requireNotNullguard and localclientsApireference follow the established pattern for accessing services viaDataManagerProvider. The flow operations correctly preserve the original behavior withasDataStateFlowandflowOn(ioDispatcher).core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/ReviewLoanApplicationRepositoryImpl.kt (2)
29-31: LGTM - Service initialization follows pattern.The
requireNotNullguard ensuresLoanAccountsListServiceis available at construction time, consistent with the refactoring pattern applied across all repositories.
40-51: LGTM - API usage migrated correctly.Both
CREATEandUPDATEbranches correctly route through theloanAccountsListApiproperty while preserving the existing error handling and response processing logic.core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/LoanRepositoryImp.kt (1)
36-38: LGTM - Service initialization follows pattern.The
requireNotNullguard ensuresLoanAccountsListServiceis available at construction time, consistent with the refactoring applied across repositories.core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/SavingsAccountRepositoryImp.kt (1)
38-40: LGTM - Service initialization follows pattern.The
requireNotNullguard ensuresSavingAccountsListServiceis available at construction time, consistent with the refactoring pattern applied across all repositories.core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/ClientChargeRepositoryImp.kt (2)
27-27: LGTM—Constructor refactored to use DataManagerProvider.The migration from
DataManagertoDataManagerProvideraligns with the PR's abstraction goals.
40-50: LGTM—API usage correctly updated.The methods now correctly use the local API properties, maintaining the same functional behavior.
core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/BeneficiaryRepositoryImp.kt (1)
39-48: LGTM—Template method correctly refactored.The migration to use
beneficiaryApiis correct and error handling is appropriate.core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/TransferRepositoryImp.kt (2)
46-49: LGTM—Transfer routing correctly implemented.The conditional routing between SELF and third-party transfers using the appropriate APIs is correct.
53-60: LGTM—Error handling is comprehensive and user-friendly.The exception handling covers the main failure scenarios with appropriate user-facing messages.
core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/UserAuthRepositoryImp.kt (2)
45-61: LGTM—Registration flow correctly refactored with robust error handling.The migration to
registrationApiis correct, and the error handling covers all relevant exception types with user-friendly messages.
63-85: LGTM—Login implementation correctly refactored.The authentication flow properly validates the response and includes comprehensive error handling.
core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/ShareAccountRepositoryImp.kt (1)
42-78: LGTM—Share account operations correctly refactored.All methods properly use the
shareAccountApiproperty with appropriate error handling and flow operators.
.../src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/BeneficiaryRepositoryImp.kt
Show resolved
Hide resolved
...src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/ClientChargeRepositoryImp.kt
Show resolved
Hide resolved
...src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/ShareAccountRepositoryImp.kt
Show resolved
Hide resolved
...ata/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/TransferRepositoryImp.kt
Show resolved
Hide resolved
...ata/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/UserAuthRepositoryImp.kt
Show resolved
Hide resolved
- Introduce `DataManagerProvider` interface and implement it in `DataManager`. - Refactor all repositories to depend on `DataManagerProvider` for better abstraction. - Remove legacy Mockito-based JVM unit tests. - Add KMP-compatible tests using Fakes (`FakeDataManager`, `FakeClientService`) and `Turbine`. - Implement `JsonLoader` and `TestResourceReader` utilities for loading test fixtures across platforms. - Update build configuration to include test resources and KMP test dependencies.
Nagarjuna0033
force-pushed
the
MM-477
branch
from
00bffd2 to
935b042
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/LoanRepositoryImp.kt (2)
44-51: Force unwrap on nullableloanIdmay throw NPE.The parameter
loanId: Long?is nullable, but you force-unwrap it withloanId!!on line 45. If a caller passesnull, this will throw aNullPointerExceptionat runtime instead of a meaningful error.Consider either making the parameter non-nullable if null is never valid, or handling the null case explicitly.
🔎 Proposed fix (option 1: make non-nullable)
override fun getLoanWithAssociations( associationType: String?, - loanId: Long?, + loanId: Long, ): Flow<DataState<LoanWithAssociations?>> = flow { try { - loanAccountsListApi.getLoanWithAssociations(loanId!!, associationType) + loanAccountsListApi.getLoanWithAssociations(loanId, associationType)🔎 Proposed fix (option 2: handle null explicitly)
): Flow<DataState<LoanWithAssociations?>> = flow { + val id = loanId ?: run { + emit(DataState.Error(IllegalArgumentException("loanId cannot be null"))) + return@flow + } try { - loanAccountsListApi.getLoanWithAssociations(loanId!!, associationType) + loanAccountsListApi.getLoanWithAssociations(id, associationType)
64-81: Same force unwrap issue onloanIdinwithdrawLoanAccount.Line 71 uses
loanId!!on a nullable parameter. Apply the same fix as suggested forgetLoanWithAssociations.core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/ShareAccountRepositoryImp.kt (1)
42-47: Error not emitted ingetShareProducts.The
catchblock creates aDataState.Errorbut doesn't emit it - the error is silently discarded.🔎 Proposed fix
override fun getShareProducts(clientId: Long?): Flow<DataState<Page<ShareProduct>>> { return shareAccountApi.getShareProducts(clientId) .map { response -> DataState.Success(response) } - .catch { exception -> DataState.Error(exception, exception.message) } + .catch { exception -> emit(DataState.Error(exception, exception.message)) } .flowOn(ioDispatcher) }
♻️ Duplicate comments (12)
core/data/src/jsCommonTest/kotlin/org/mifos/mobile/core/data/utils/TestResourceReader.jsCommon.kt (1)
12-14: Incomplete implementation blocks JS/Common testing.The
TODOplaceholder will cause runtime failures if tests run on the JS/Common platform and attempt to load resources.core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/GuarantorRepositoryImp.kt (1)
33-35: Make API property private to preserve encapsulation.The
guarantorApiproperty is an implementation detail and should not be part of the public API surface. Making it private prevents external code from bypassing the repository interface.🔎 Proposed fix
- val guarantorApi = requireNotNull(dataManager.guarantorApi) { + private val guarantorApi = requireNotNull(dataManager.guarantorApi) { "GuarantorService must be provided" }core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/ClientRepositoryImp.kt (1)
27-30: Make API property private to preserve encapsulation.The
clientsApiproperty is an implementation detail and should not be part of the public API surface. Making it private prevents external code from bypassing the repository interface.🔎 Proposed fix
- val clientsApi = requireNotNull(dataManager.clientsApi) { + private val clientsApi = requireNotNull(dataManager.clientsApi) { "ClientService must be provided" }core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/RecentTransactionRepositoryImp.kt (1)
27-29: Make API property private to preserve encapsulation.The
recentTransactionsApiproperty is an implementation detail and should not be part of the public API surface. Making it private prevents external code from bypassing the repository interface.🔎 Proposed fix
- val recentTransactionsApi = requireNotNull(dataManager.recentTransactionsApi) { + private val recentTransactionsApi = requireNotNull(dataManager.recentTransactionsApi) { "RecentTransactionsService must be provided" }core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/HomeRepositoryImp.kt (1)
31-33: Make API property private to preserve encapsulation.The
clientsApiproperty is an implementation detail and should not be part of the public API surface. Making it private prevents external code from bypassing the repository interface.🔎 Proposed fix
- val clientsApi = requireNotNull(dataManager.clientsApi) { + private val clientsApi = requireNotNull(dataManager.clientsApi) { "ClientService must be provided" }core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/UserDetailRepositoryImp.kt (1)
28-30: Make API property private to preserve encapsulation.The
notificationApiproperty is an implementation detail and should not be part of the public API surface. Making it private prevents external code from bypassing the repository interface.🔎 Proposed fix
- val notificationApi = requireNotNull(dataManager.notificationApi) { + private val notificationApi = requireNotNull(dataManager.notificationApi) { "NotificationService must be provided" }core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/TransferRepositoryImp.kt (1)
32-38: Make API properties private to preserve encapsulation.As noted in a previous review, both
savingAccountsListApiandthirdPartyTransferApishould be private implementation details.core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/SavingsAccountRepositoryImp.kt (1)
46-49: Force-unwrap (!!) on nullableaccountIdrisks NPE.As noted in a previous review, multiple methods use
accountId!!(lines 47, 105, 125, 142). Since parameters are declared nullable, validate upfront and return an error state rather than crashing.core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/BeneficiaryRepositoryImp.kt (2)
35-37: MakebeneficiaryApiprivate to preserve encapsulation.As noted in a previous review, the API property should not be exposed as part of the public API surface.
73-73: Force-unwrap (!!) on nullablebeneficiaryIdrisks NPE.As noted in previous reviews, this applies to both
updateBeneficiary(line 73) anddeleteBeneficiary(line 89). Validate upfront and return a descriptive error state rather than crashing.core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/UserAuthRepositoryImp.kt (1)
33-43: Make all API properties private to preserve encapsulation.As noted in a previous review, the three API properties (
registrationApi,authenticationApi,userDetailsApi) should not be exposed publicly.core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/ShareAccountRepositoryImp.kt (1)
38-40: Encapsulation issue already flagged.This was previously identified -
shareAccountApishould beprivate valinstead ofval.
🧹 Nitpick comments (8)
gradle/libs.versions.toml (1)
245-245: Fix naming inconsistency in the library key.The key
kotlin-test-annotations_commonuses an underscore before "common", which is inconsistent with the naming convention used throughout this file. Other entries likekotlin-test,kotlin-stdlib, andkotlin-reflectuse hyphens consistently.🔎 Proposed fix
-kotlin-test-annotations_common = { group = "org.jetbrains.kotlin", name = "kotlin-test-annotations-common", version.ref = "kotlin" } +kotlin-test-annotations-common = { group = "org.jetbrains.kotlin", name = "kotlin-test-annotations-common", version.ref = "kotlin" }core/data/src/commonTest/kotlin/org/mifos/mobile/core/data/utils/TestFixtures.kt (1)
16-19: Consider lazy initialization for test fixtures.The custom getter deserializes the JSON resource on every access. For immutable test data, consider using
by lazyto cache the result and avoid repeated deserialization.🔎 Proposed optimization
- val clientAccounts - get() = JsonLoader.fromResource<ClientAccounts>( - "json/ClientAccounts.json", - ) + val clientAccounts by lazy { + JsonLoader.fromResource<ClientAccounts>("json/ClientAccounts.json") + }core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/ReviewLoanApplicationRepositoryImpl.kt (1)
29-31: MakeloanAccountsListApiprivate to preserve encapsulation.The API property should not be exposed as part of the public API surface. This aligns with the encapsulation pattern expected across repository implementations.
🔎 Proposed fix
- val loanAccountsListApi = requireNotNull(dataManager.loanAccountsListApi) { + private val loanAccountsListApi = requireNotNull(dataManager.loanAccountsListApi) { "LoanAccountsListService must be provided" }core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/ThirdPartyTransferRepositoryImp.kt (1)
26-28: MakethirdPartyTransferApiprivate to preserve encapsulation.Consistent with other repository implementations, the API property should not be exposed publicly.
🔎 Proposed fix
- val thirdPartyTransferApi = requireNotNull(dataManager.thirdPartyTransferApi) { + private val thirdPartyTransferApi = requireNotNull(dataManager.thirdPartyTransferApi) { "ThirdPartyTransferService must be provided" }core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/SavingsAccountRepositoryImp.kt (1)
38-40: MakesavingAccountsListApiprivate to preserve encapsulation.The API property should not be exposed as part of the public API surface.
🔎 Proposed fix
- val savingAccountsListApi = requireNotNull(dataManager.savingAccountsListApi) { + private val savingAccountsListApi = requireNotNull(dataManager.savingAccountsListApi) { "SavingAccountsListService must be provided" }core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/BeneficiaryRepositoryImp.kt (1)
39-48: Consider simplifying the flow pattern.The nested
collectinsideflow { }could potentially be replaced with a more direct transformation if the upstream API already returns aFlow. However, if the current pattern is intentional for error boundary handling, this is acceptable.core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/UserAuthRepositoryImp.kt (1)
63-85: Minor: Inconsistent try-catch placement inlogin.The
loginmethod places the try-catch outside ofwithContext, while other methods (registerUser,verifyUser,updateAccountPassword) place it inside. Both work correctly, but consistency would improve maintainability.🔎 Proposed fix for consistency
override suspend fun login(username: String, password: String): DataState<User> { val loginPayload = LoginPayload( username = username, password = password, ) - return try { - withContext(ioDispatcher) { + return withContext(ioDispatcher) { + try { val user = authenticationApi.authenticate(loginPayload) if (user.base64EncodedAuthenticationKey != null) { DataState.Success(user) } else { DataState.Error(Exception("Invalid Credentials"), null) } + } catch (e: ClientRequestException) { + val errorMessage = extractErrorMessage(e.response) + DataState.Error(Exception(errorMessage), null) + } catch (e: IOException) { + DataState.Error(Exception("Network error: ${e.message ?: "Please check your connection"}"), null) + } catch (e: ServerResponseException) { + DataState.Error(Exception("Server error: ${e.message}"), null) } - } catch (e: ClientRequestException) { - val errorMessage = extractErrorMessage(e.response) - DataState.Error(Exception(errorMessage), null) - } catch (e: IOException) { - DataState.Error(Exception("Network error: ${e.message ?: "Please check your connection"}"), null) - } catch (e: ServerResponseException) { - DataState.Error(Exception("Server error: ${e.message}"), null) } }core/data/src/commonTest/kotlin/org/mifos/mobile/core/data/repoTests/AccountsRepositoryTest.kt (1)
106-111: Inconsistent null handling for account lists.
savingsAccountsis checked with safe call (?.isEmpty()), whileloanAccountsandshareAccountsare accessed directly. This suggests either:
savingsAccountsis nullable while others are not, or- The null handling is inconsistent
If
savingsAccountscan be null, consider usingassertTrue(data.savingsAccounts.isNullOrEmpty())for clarity. If it shouldn't be null, use the same pattern as the other assertions.🔎 Proposed fix (if savingsAccounts should be treated like others)
- assertEquals(data.savingsAccounts?.isEmpty(), true) + assertTrue(data.savingsAccounts?.isEmpty() == true) assertTrue(data.loanAccounts.isEmpty()) assertTrue(data.shareAccounts.isEmpty())
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (46)
core/data/build.gradle.ktscore/data/src/androidUnitTest/kotlin/org/mifos/mobile/core/data/utils/TestResourceReader.android.ktcore/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/AccountsRepositoryImp.ktcore/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/BeneficiaryRepositoryImp.ktcore/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/ClientChargeRepositoryImp.ktcore/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/ClientRepositoryImp.ktcore/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/GuarantorRepositoryImp.ktcore/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/HomeRepositoryImp.ktcore/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/LoanRepositoryImp.ktcore/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/RecentTransactionRepositoryImp.ktcore/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/ReviewLoanApplicationRepositoryImpl.ktcore/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/SavingsAccountRepositoryImp.ktcore/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/ShareAccountRepositoryImp.ktcore/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/ThirdPartyTransferRepositoryImp.ktcore/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/TransferRepositoryImp.ktcore/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/UserAuthRepositoryImp.ktcore/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/UserDetailRepositoryImp.ktcore/data/src/commonTest/kotlin/org/mifos/mobile/core/data/fakeServices/FakeClientService.ktcore/data/src/commonTest/kotlin/org/mifos/mobile/core/data/fakeServices/FakeDataManager.ktcore/data/src/commonTest/kotlin/org/mifos/mobile/core/data/repoTests/AccountsRepositoryTest.ktcore/data/src/commonTest/kotlin/org/mifos/mobile/core/data/utils/JsonLoader.ktcore/data/src/commonTest/kotlin/org/mifos/mobile/core/data/utils/TestFixtures.ktcore/data/src/commonTest/kotlin/org/mifos/mobile/core/data/utils/TestResourceReader.ktcore/data/src/commonTest/resources/json/ClientAccounts.jsoncore/data/src/desktopTest/kotlin/org/mifos/mobile/core/data/utils/TestResourceReader.desktop.ktcore/data/src/jsCommonTest/kotlin/org/mifos/mobile/core/data/utils/TestResourceReader.jsCommon.ktcore/data/src/nativeTest/kotlin/org/mifos/mobile/core/data/utils/TestResourceReader.native.ktcore/data/src/test/kotlin/org/mifos/mobile/core/data/repositories/AccountsRepositoryImpTest.ktcore/data/src/test/kotlin/org/mifos/mobile/core/data/repositories/BeneficiaryRepositoryImpTest.ktcore/data/src/test/kotlin/org/mifos/mobile/core/data/repositories/ClientChargeRepositoryImpTest.ktcore/data/src/test/kotlin/org/mifos/mobile/core/data/repositories/ClientRepositoryImpTest.ktcore/data/src/test/kotlin/org/mifos/mobile/core/data/repositories/GuarantorRepositoryImpTest.ktcore/data/src/test/kotlin/org/mifos/mobile/core/data/repositories/HomeRepositoryImpTest.ktcore/data/src/test/kotlin/org/mifos/mobile/core/data/repositories/LoanRepositoryImpTest.ktcore/data/src/test/kotlin/org/mifos/mobile/core/data/repositories/NotificationRepositoryImpTest.ktcore/data/src/test/kotlin/org/mifos/mobile/core/data/repositories/RecentTransactionRepositoryImpTest.ktcore/data/src/test/kotlin/org/mifos/mobile/core/data/repositories/SavingsAccountRepositoryImpTest.ktcore/data/src/test/kotlin/org/mifos/mobile/core/data/repositories/ThirdPartyTransferRepositoryImpTest.ktcore/data/src/test/kotlin/org/mifos/mobile/core/data/repositories/TransferRepositoryImpTest.ktcore/data/src/test/kotlin/org/mifos/mobile/core/data/repositories/UserAuthRepositoryImpTest.ktcore/data/src/test/kotlin/org/mifos/mobile/core/data/repositories/UserDetailRepositoryImpTest.ktcore/data/src/test/resources/mockito-extensions/org.mockito.plugins.MockMakercore/network/src/commonMain/kotlin/org/mifos/mobile/core/network/DataManager.ktcore/network/src/commonMain/kotlin/org/mifos/mobile/core/network/DataManagerProvider.ktcore/network/src/commonMain/kotlin/org/mifos/mobile/core/network/di/NetworkModule.ktgradle/libs.versions.toml
💤 Files with no reviewable changes (15)
- core/data/src/test/resources/mockito-extensions/org.mockito.plugins.MockMaker
- core/data/src/test/kotlin/org/mifos/mobile/core/data/repositories/ClientChargeRepositoryImpTest.kt
- core/data/src/test/kotlin/org/mifos/mobile/core/data/repositories/ThirdPartyTransferRepositoryImpTest.kt
- core/data/src/test/kotlin/org/mifos/mobile/core/data/repositories/GuarantorRepositoryImpTest.kt
- core/data/src/test/kotlin/org/mifos/mobile/core/data/repositories/UserAuthRepositoryImpTest.kt
- core/data/src/test/kotlin/org/mifos/mobile/core/data/repositories/NotificationRepositoryImpTest.kt
- core/data/src/test/kotlin/org/mifos/mobile/core/data/repositories/TransferRepositoryImpTest.kt
- core/data/src/test/kotlin/org/mifos/mobile/core/data/repositories/SavingsAccountRepositoryImpTest.kt
- core/data/src/test/kotlin/org/mifos/mobile/core/data/repositories/BeneficiaryRepositoryImpTest.kt
- core/data/src/test/kotlin/org/mifos/mobile/core/data/repositories/HomeRepositoryImpTest.kt
- core/data/src/test/kotlin/org/mifos/mobile/core/data/repositories/UserDetailRepositoryImpTest.kt
- core/data/src/test/kotlin/org/mifos/mobile/core/data/repositories/LoanRepositoryImpTest.kt
- core/data/src/test/kotlin/org/mifos/mobile/core/data/repositories/AccountsRepositoryImpTest.kt
- core/data/src/test/kotlin/org/mifos/mobile/core/data/repositories/RecentTransactionRepositoryImpTest.kt
- core/data/src/test/kotlin/org/mifos/mobile/core/data/repositories/ClientRepositoryImpTest.kt
🚧 Files skipped from review as they are similar to previous changes (7)
- core/data/src/commonTest/kotlin/org/mifos/mobile/core/data/utils/TestResourceReader.kt
- core/data/build.gradle.kts
- core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/AccountsRepositoryImp.kt
- core/data/src/nativeTest/kotlin/org/mifos/mobile/core/data/utils/TestResourceReader.native.kt
- core/network/src/commonMain/kotlin/org/mifos/mobile/core/network/DataManagerProvider.kt
- core/data/src/commonTest/kotlin/org/mifos/mobile/core/data/fakeServices/FakeDataManager.kt
- core/data/src/commonTest/kotlin/org/mifos/mobile/core/data/fakeServices/FakeClientService.kt
🧰 Additional context used
🧬 Code graph analysis (1)
core/data/src/commonTest/kotlin/org/mifos/mobile/core/data/utils/JsonLoader.kt (5)
core/data/src/androidUnitTest/kotlin/org/mifos/mobile/core/data/utils/TestResourceReader.android.kt (1)
readResource(12-19)core/data/src/commonTest/kotlin/org/mifos/mobile/core/data/utils/TestResourceReader.kt (1)
readResource(12-12)core/data/src/desktopTest/kotlin/org/mifos/mobile/core/data/utils/TestResourceReader.desktop.kt (1)
readResource(12-17)core/data/src/jsCommonTest/kotlin/org/mifos/mobile/core/data/utils/TestResourceReader.jsCommon.kt (1)
readResource(12-14)core/data/src/nativeTest/kotlin/org/mifos/mobile/core/data/utils/TestResourceReader.native.kt (1)
readResource(18-31)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: PR Checks KMP / Static Analysis Check
🔇 Additional comments (15)
core/data/src/androidUnitTest/kotlin/org/mifos/mobile/core/data/utils/TestResourceReader.android.kt (1)
12-18: Excellent error handling and resource management!The use of
requireNotNullwith a descriptive error message and proper resource cleanup viabufferedReader().usedemonstrates solid defensive programming. This implementation should serve as the reference pattern for other platform implementations.core/network/src/commonMain/kotlin/org/mifos/mobile/core/network/di/NetworkModule.kt (1)
43-45: LGTM!The binding change correctly abstracts the DataManager behind the DataManagerProvider interface, enabling better testability and adherence to the dependency inversion principle.
core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/GuarantorRepositoryImp.kt (1)
88-92: Verify that demo data usage is intentional.The method returns hardcoded demo data while the actual API call is commented out. Please confirm whether this is intentional (e.g., API not yet implemented) or if the real API call should be enabled.
core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/HomeRepositoryImp.kt (1)
44-59: LGTM!The response mapping correctly extracts the pure base64 string from the data URI format, and properly preserves error and loading states throughout the flow.
core/data/src/commonTest/resources/json/ClientAccounts.json (1)
1-554: Well-structured test fixture covering multiple account types and statuses.The JSON fixture provides comprehensive test data for savings, loan, and share accounts with various statuses (active, overpaid) and complete metadata. The large balance value (line 194) appears intentional for testing edge cases with high precision numbers.
core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/TransferRepositoryImp.kt (1)
40-62: LGTM - Transfer logic and error handling look good.The routing based on
TransferTypeis clear, and the generic server error message improves user experience by not exposing internal details.core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/ThirdPartyTransferRepositoryImp.kt (1)
30-33: LGTM - Clean flow-based implementation.The method correctly wraps the API call with
asDataStateFlow()and appliesflowOn(ioDispatcher)for proper dispatcher context.core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/SavingsAccountRepositoryImp.kt (1)
52-60: LGTM - New transaction details method.Clean implementation following the established pattern with proper flow composition.
core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/LoanRepositoryImp.kt (1)
29-34: Refactoring toDataManagerProviderlooks good.The abstraction allows for better testability by enabling injection of fake implementations.
core/data/src/commonTest/kotlin/org/mifos/mobile/core/data/repoTests/AccountsRepositoryTest.kt (3)
29-53: Well-structured test setup with fakes.Good use of
FakeDataManagerandFakeClientServicefor test isolation. TheUnconfinedTestDispatcheris appropriate for synchronous test execution.
55-79: Comprehensive success case test.Good coverage of the happy path with proper Turbine flow testing and assertions on all account types.
117-141: Error scenario test is appropriate.Validates that the repository correctly propagates errors from the underlying service.
core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/ShareAccountRepositoryImp.kt (1)
31-36: Refactoring toDataManagerProvideris correct.The abstraction pattern is consistent with other repository implementations.
core/network/src/commonMain/kotlin/org/mifos/mobile/core/network/DataManager.kt (2)
12-14: Clean interface implementation.
DataManagernow properly implementsDataManagerProvider, enabling dependency injection of the interface for better testability.
16-40: Lazy initialization with interface overrides looks good.All API properties correctly override the interface declarations while preserving lazy initialization for deferred instantiation.
| val loanAccountsListApi = requireNotNull(dataManager.loanAccountsListApi) { | ||
| "LoanAccountsListService must be provided" | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Make loanAccountsListApi private to preserve encapsulation.
This property is an internal implementation detail and should not be exposed publicly. Same issue applies to other repository implementations in this PR.
🔎 Proposed fix
- val loanAccountsListApi = requireNotNull(dataManager.loanAccountsListApi) {
+ private val loanAccountsListApi = requireNotNull(dataManager.loanAccountsListApi) {
"LoanAccountsListService must be provided"
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| val loanAccountsListApi = requireNotNull(dataManager.loanAccountsListApi) { | |
| "LoanAccountsListService must be provided" | |
| } | |
| private val loanAccountsListApi = requireNotNull(dataManager.loanAccountsListApi) { | |
| "LoanAccountsListService must be provided" | |
| } |
🤖 Prompt for AI Agents
In
core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/LoanRepositoryImp.kt
around lines 36 to 38, the loanAccountsListApi property is currently declared as
a public val; make it a private val to preserve encapsulation so implementation
details aren't exposed. Update the declaration to private val
loanAccountsListApi = requireNotNull(dataManager.loanAccountsListApi) {
"LoanAccountsListService must be provided" } and run a quick search for
similarly exposed API properties in other repository implementations in this PR
and change them to private as well.
| val response = loanAccountsListApi.createLoansAccount(loansPayload) | ||
| println("response $response") | ||
| return@withContext DataState.Success("Loan Created Successfully") | ||
| } | ||
| LoanState.UPDATE -> { | ||
| val response = dataManager.loanAccountsListApi.updateLoanAccount(loanId, loansPayload) | ||
| val response = loanAccountsListApi.updateLoanAccount(loanId, loansPayload) | ||
| println("response $response") | ||
| return@withContext DataState.Success("Loan Updated Successfully") |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove debug println statements.
Debug print statements should not remain in production code. Consider using structured logging if runtime diagnostics are needed, or remove these statements entirely.
🔎 Proposed fix
LoanState.CREATE -> {
val response = loanAccountsListApi.createLoansAccount(loansPayload)
- println("response $response")
return@withContext DataState.Success("Loan Created Successfully")
}
LoanState.UPDATE -> {
val response = loanAccountsListApi.updateLoanAccount(loanId, loansPayload)
- println("response $response")
return@withContext DataState.Success("Loan Updated Successfully")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| val response = loanAccountsListApi.createLoansAccount(loansPayload) | |
| println("response $response") | |
| return@withContext DataState.Success("Loan Created Successfully") | |
| } | |
| LoanState.UPDATE -> { | |
| val response = dataManager.loanAccountsListApi.updateLoanAccount(loanId, loansPayload) | |
| val response = loanAccountsListApi.updateLoanAccount(loanId, loansPayload) | |
| println("response $response") | |
| return@withContext DataState.Success("Loan Updated Successfully") | |
| val response = loanAccountsListApi.createLoansAccount(loansPayload) | |
| return@withContext DataState.Success("Loan Created Successfully") | |
| } | |
| LoanState.UPDATE -> { | |
| val response = loanAccountsListApi.updateLoanAccount(loanId, loansPayload) | |
| return@withContext DataState.Success("Loan Updated Successfully") |
🤖 Prompt for AI Agents
In
core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/ReviewLoanApplicationRepositoryImpl.kt
around lines 42 to 49, remove the debug println("response $response") statements
left in both the CREATE and UPDATE branches; either delete them or replace them
with a structured logger call (e.g., logger.debug or equivalent) that does not
print to stdout, and add the appropriate logger import/initialization if you opt
to log instead.
| object JsonLoader { | ||
|
|
||
| val json = Json { | ||
| ignoreUnknownKeys = true | ||
| explicitNulls = false | ||
| } | ||
|
|
||
| inline fun <reified T> fromResource(path: String): T { | ||
| return json.decodeFromString(readResource(path)) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find TestResourceReader files (expect/actual pattern)
find . -name "*TestResourceReader*" -type f | head -20Repository: openMF/mifos-mobile
Length of output: 547
🏁 Script executed:
#!/bin/bash
# Search for the readResource function and its implementations
rg "readResource" -A 3 -B 1 --type kotlin | head -80Repository: openMF/mifos-mobile
Length of output: 3127
🏁 Script executed:
#!/bin/bash
# Check for JS test configurations
rg -n "jsTest|jsCommonTest" --type gradle,kts | head -30Repository: openMF/mifos-mobile
Length of output: 97
🏁 Script executed:
#!/bin/bash
# Search for gradle files and check for JS test configurations
find . -name "*.gradle.kts" -o -name "*.gradle" | head -10Repository: openMF/mifos-mobile
Length of output: 381
🏁 Script executed:
#!/bin/bash
# Check core/data build configuration for JS platform tests
grep -r "js" build.gradle.kts 2>/dev/null | head -20Repository: openMF/mifos-mobile
Length of output: 45
🏁 Script executed:
#!/bin/bash
# Look at the build.gradle.kts in core/data directory
cat core/data/build.gradle.kts | grep -A 5 -B 5 "js\|jsTest" | head -50Repository: openMF/mifos-mobile
Length of output: 518
🏁 Script executed:
#!/bin/bash
# Get the full build.gradle.kts file for core/data to see test configuration
cat core/data/build.gradle.ktsRepository: openMF/mifos-mobile
Length of output: 1680
🏁 Script executed:
#!/bin/bash
# Also check if there's any CI configuration that runs JS tests
find . -name ".github" -o -name ".gitlab-ci.yml" -o -name "*.yaml" 2>/dev/null | grep -i "ci\|github\|gitlab" | head -10Repository: openMF/mifos-mobile
Length of output: 205
🏁 Script executed:
#!/bin/bash
# Check CI workflows to see which platforms are tested
cat .github/workflows/upload-demo-app-on-firebase-kmp.yamlRepository: openMF/mifos-mobile
Length of output: 1421
🏁 Script executed:
#!/bin/bash
# Also check the kmp.library.convention plugin to see which platforms are enabled by default
find . -path "*/convention/*" -name "*.gradle.kts" | xargs grep -l "js\|platform" | head -5Repository: openMF/mifos-mobile
Length of output: 45
🏁 Script executed:
#!/bin/bash
# Search for kmp.library.convention to see which platforms are enabled
find . -path "*/convention/*" -name "*.gradle.kts" | xargs cat | grep -A 20 -B 5 "kotlin {" | head -100Repository: openMF/mifos-mobile
Length of output: 45
🏁 Script executed:
#!/bin/bash
# Check which modules actually have JS targets enabled
rg "js\(\)" . --type kotlin --type gradle-kts | head -20Repository: openMF/mifos-mobile
Length of output: 97
🏁 Script executed:
#!/bin/bash
# Find and examine the kmp.library.convention plugin
find . -path "*/build-logic/*" -name "*.gradle.kts" | xargs ls -laRepository: openMF/mifos-mobile
Length of output: 236
🏁 Script executed:
#!/bin/bash
# Search for any JS platform definitions in gradle files
rg "js\(\)|jsTest|@OptIn.*JS" . --type kts | head -30Repository: openMF/mifos-mobile
Length of output: 90
🏁 Script executed:
#!/bin/bash
# Check what the kmp library convention file contains
cat build-logic/convention/src/main/kotlin/kmp.library.convention.gradle.kts 2>/dev/null || find build-logic -name "*kmp*library*" -type fRepository: openMF/mifos-mobile
Length of output: 45
🏁 Script executed:
#!/bin/bash
# Search more broadly for gradle convention files
find build-logic -name "*.gradle.kts" -exec cat {} \; | head -200Repository: openMF/mifos-mobile
Length of output: 4900
🏁 Script executed:
#!/bin/bash
# Look for actual platform configuration in any gradle file
grep -r "androidTarget\|jsTarget\|iosTarget" . --include="*.gradle.kts" | head -20Repository: openMF/mifos-mobile
Length of output: 112
🏁 Script executed:
#!/bin/bash
# Check if there are any references to JS platform tests in the codebase
rg "jsTest|jsCommon" . --type yaml --type toml --type md | head -20Repository: openMF/mifos-mobile
Length of output: 827
🏁 Script executed:
#!/bin/bash
# Find the KMPLibraryConventionPlugin implementation
find build-logic -name "*KMPLibrary*" -type fRepository: openMF/mifos-mobile
Length of output: 131
🏁 Script executed:
#!/bin/bash
# Search for the plugin source files
find build-logic/convention/src -type f -name "*.kt" | head -20Repository: openMF/mifos-mobile
Length of output: 1531
🏁 Script executed:
#!/bin/bash
# Look for kotlin multiplatform platform definitions
cat build-logic/convention/src/main/kotlin/KMPLibraryConventionPlugin.kt 2>/dev/null || find build-logic -name "*.kt" -exec grep -l "kotlin.*multiplatform\|androidTarget\|jsTarget" {} \;Repository: openMF/mifos-mobile
Length of output: 1742
🏁 Script executed:
#!/bin/bash
# Find the configureKotlinMultiplatform function
grep -r "fun configureKotlinMultiplatform" build-logic --include="*.kt"Repository: openMF/mifos-mobile
Length of output: 45
🏁 Script executed:
#!/bin/bash
# Search for the multiplatform configuration file
find build-logic -name "*.kt" -exec grep -l "configureKotlinMultiplatform\|androidTarget\|iosTarget\|jsTarget" {} \;Repository: openMF/mifos-mobile
Length of output: 210
🏁 Script executed:
#!/bin/bash
# Examine the KotlinMultiplatform.kt configuration file
cat build-logic/convention/src/main/kotlin/org/mifos/mobile/KotlinMultiplatform.ktRepository: openMF/mifos-mobile
Length of output: 952
The readResource implementation for JS platform must be completed before JS tests can run.
The JSON loader utility is clean, but the associated readResource function in TestResourceReader.jsCommon.kt contains TODO("Not yet implemented"). Since the Kotlin multiplatform configuration explicitly includes the JS target (js(IR) { nodejs(); binaries.executable() }), any JS tests attempting to use JsonLoader.fromResource() will fail at runtime with a NotImplementedError.
🤖 Prompt for AI Agents
In
core/data/src/commonTest/kotlin/org/mifos/mobile/core/data/utils/JsonLoader.kt
(lines 14-24) the JSON loader calls readResource which is unimplemented for the
JS target; implement the JS platform function in TestResourceReader.jsCommon.kt
to synchronously read the test resource file and return its contents as a String
(use Node APIs since the JS target is nodejs):
require("fs").readFileSync(resolvedPath, "utf8") where resolvedPath is built
from process.cwd() or the known test resources folder (e.g. process.cwd() +
"/src/commonTest/resources/" + path), handle any file-not-found I/O errors by
rethrowing or converting to a descriptive exception, and ensure the function
signature returns String so JsonLoader.fromResource works in JS tests.
| actual fun readResource(path: String): String { | ||
| return object {}.javaClass | ||
| .classLoader!! | ||
| .getResource(path)!! | ||
| .readText() | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Improve error handling to match Android implementation.
The multiple non-null assertions (!!) will throw unhelpful NPEs when resources are missing. The Android implementation uses requireNotNull with a descriptive error message, which provides better debugging context.
🔎 Proposed fix to align with Android's error handling
actual fun readResource(path: String): String {
- return object {}.javaClass
- .classLoader!!
- .getResource(path)!!
- .readText()
+ return requireNotNull(
+ object {}.javaClass.classLoader
+ ?.getResourceAsStream(path),
+ ) {
+ "Test resource not found: $path"
+ }.bufferedReader().use { it.readText() }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| actual fun readResource(path: String): String { | |
| return object {}.javaClass | |
| .classLoader!! | |
| .getResource(path)!! | |
| .readText() | |
| } | |
| actual fun readResource(path: String): String { | |
| return requireNotNull( | |
| object {}.javaClass.classLoader | |
| ?.getResourceAsStream(path), | |
| ) { | |
| "Test resource not found: $path" | |
| }.bufferedReader().use { it.readText() } | |
| } |
🤖 Prompt for AI Agents
In
core/data/src/desktopTest/kotlin/org/mifos/mobile/core/data/utils/TestResourceReader.desktop.kt
around lines 12 to 17, replace the chained non-null assertions with
requireNotNull checks that provide descriptive error messages: requireNotNull
for the classLoader with a message like "ClassLoader is null while reading test
resource", requireNotNull for the result of getResource(path) with "Resource not
found: <path>", then call readText() on the non-null resource; this mirrors the
Android implementation's safer, more debuggable error handling.
1 participant
DataManagerProviderinterface and implement it inDataManager.DataManagerProviderfor better abstraction.FakeDataManager,FakeClientService) andTurbine.JsonLoaderandTestResourceReaderutilities for loading test fixtures across platforms.Fixes - Jira-#477
Summary by CodeRabbit
Chores
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.