fix(client): enable loan repayment navigation from client loan accounts screen - #2599
Conversation
📝 WalkthroughWalkthroughMakeRepayment actions now carry a loan ID and navigation is wired to pass that ID to LoanRepaymentScreen. LoanRepaymentViewModel gained a LoanAccountSummaryRepository, exposes route args via StateFlow, and auto-loads loan details when only loanId is provided. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ClientScreen as Client Loan Accounts Screen
participant ClientVM as Client Loan Accounts ViewModel
participant NavController as Navigation Controller
participant LoanScreen as Loan Repayment Screen
participant LoanVM as Loan Repayment ViewModel
participant SummaryRepo as Loan Summary Repository
User->>ClientScreen: Tap "Make Repayment" (loan)
ClientScreen->>ClientVM: Dispatch MakeRepayment(loanId)
ClientVM->>NavController: navigateToLoanRepaymentScreen(loanId)
NavController->>LoanScreen: Navigate with loanId
LoanScreen->>LoanVM: Initialize with loanId
alt loanAccountNumber is empty
LoanVM->>SummaryRepo: getLoanById(loanId)
SummaryRepo-->>LoanVM: LoanWithAssociationsEntity
LoanVM->>LoanVM: update arg StateFlow with loan details
LoanVM->>LoanVM: checkDatabaseLoanRepaymentByLoanId()
end
LoanVM-->>LoanScreen: emit updated state
LoanScreen-->>User: show repayment UI (populated)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
There was a problem hiding this comment.
Pull request overview
Fixes the “Make Repayment” action from the Client Loan Accounts list by wiring navigation correctly and supporting repayment-screen navigation with either a full loan entity or just a loan ID (with lazy-loading in the repayment ViewModel).
Changes:
- Wire
ClientLoanAccountsScreen“Make Repayment” to actually navigate to the repayment screen. - Add an
Int-basednavigateToLoanRepaymentScreen(loanId)overload and make repayment route fields optional. - Update
LoanRepaymentViewModel/UI to handle loanId-only navigation by fetching full loan details when needed.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentViewModel.kt | Adds lazy-loading of loan details via LoanAccountSummaryRepository when only loanId is provided. |
| feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentScreenRoute.kt | Makes route params optional and adds navigateToLoanRepaymentScreen(loanId: Int) overload. |
| feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentScreen.kt | Switches to observing route args via StateFlow and gates initial DB-check call. |
| feature/client/src/commonMain/kotlin/com/mifos/feature/client/navigation/ClientNavigation.kt | Connects the previously-empty repayment navigation lambda. |
| feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientLoanAccounts/ClientLoanAccountsViewModel.kt | Changes MakeRepayment action to carry a loanId and emits an event with that ID. |
| feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientLoanAccounts/ClientLoanAccountsScreen.kt | Passes loanId into the MakeRepayment action from the loan list item. |
Comments suppressed due to low confidence (1)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentScreen.kt:116
onRetryalways callscheckDatabaseLoanRepaymentByLoanId(). When this screen is reached via the new loanId-only navigation, the initial failure is likely inloadLoanById(), and retrying only the DB check won’t reload the loan details. Consider making retry branch based on whetherarg.loanAccountNumberis empty (callloadLoanById()first) or centralize retry logic in the ViewModel so it repeats the last failing step.
uiState = uiState,
navigateBack = navigateBack,
onRetry = { viewmodel.checkDatabaseLoanRepaymentByLoanId() },
submitPayment = {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientLoanAccounts/ClientLoanAccountsScreen.kt (1)
172-179:⚠️ Potential issue | 🟡 MinorAvoid navigating with a synthetic loanId of 0.
Lines 174 and 178 fall back to
0whenloan.idis null. SinceLoanAccountEntity.idis nullable (Int? = null), this synthetic ID can route to an invalid loan. Prefer guarding the action (or disabling the menu item) whenloan.idis missing. This applies to bothViewAccountandMakeRepaymentactions.🛡️ Suggested change
- is Actions.MakeRepayment -> onAction( - ClientLoanAccountsAction.MakeRepayment(loan.id ?: 0), - ) + is Actions.MakeRepayment -> { + loan.id?.let { loanId -> + onAction( + ClientLoanAccountsAction.MakeRepayment(loanId), + ) + } + }
🤖 Fix all issues with AI agents
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentScreen.kt`:
- Around line 99-115: The retry currently always calls
viewmodel.checkDatabaseLoanRepaymentByLoanId(), which does not repopulate
missing args like clientName or loanAccountNumber when arg.loanAccountNumber is
empty; update the onRetry logic in LoanRepaymentScreen usage to inspect
arg.loanAccountNumber (or arg.loanId) and call
viewmodel.loadLoanById(arg.loanId) when loanAccountNumber is blank, otherwise
call viewmodel.checkDatabaseLoanRepaymentByLoanId(), so retry will reload full
loan details and repopulate the UI.
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentViewModel.kt`:
- Around line 58-66: In the DataState.Success branch inside
LoanRepaymentViewModel, don't fall back to a new LoanWithAssociationsEntity()
when dataState.data is null; instead detect null and treat it as an error/early
return so you don't overwrite _arg.value with empty/zeroed fields. Update the
handler around the DataState.Success case (the block that currently declares
loanWithAssociations) to check if dataState.data == null and, if so, log or emit
an error and skip the DB check/assignment to _arg.value; only copy values into
_arg.value when loanWithAssociations is non-null.
d7e9e18 to
8138006
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentViewModel.kt`:
- Around line 46-79: loadLoanById() updates _arg on DataState.Success but never
advances the UI or triggers the next step, leaving the screen stuck on
ShowProgressbar; modify LoanRepaymentViewModel.loadLoanById() so that in the
DataState.Success branch after updating _arg.value you either set
_loanRepaymentUiState to the appropriate next state (e.g., a state that signals
navigation/check) or directly call the existing
checkDatabaseLoanRepaymentByLoanId() method to continue the repayment flow;
ensure this change uses the same loanAccountNumber/loanId from
loanWithAssociations and remove or gate the LaunchedEffect navigation call in
the Screen to avoid the race.
🧹 Nitpick comments (2)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentScreenRoute.kt (1)
18-25: Consider usingvalinstead ofvarfor route data class fields.All fields in
LoanRepaymentScreenRouteare declared asvar, but route arguments should be immutable after construction. The ViewModel already wraps this in aMutableStateFlowfor mutability. Usingvalwould better express intent and prevent accidental mutation.feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentViewModel.kt (1)
39-40: Exposing route object as mutable state works but is unconventional.Using
LoanRepaymentScreenRoute(a navigation route data class) as the ViewModel's state object conflates navigation concerns with presentation state. This works for now, but consider extracting a dedicated state class if this grows more complex.
0c42b5b to
bb8ae99
Compare
bb8ae99 to
9f56889
Compare
de371f6 to
501ac72
Compare
|
@coderabbitai Improve the PR title following this: https://github.com/openMF/mobile-wallet/wiki/Commit-style-guide. Just give the title. |
|
Tip For best results, initiate chat on the files or code changes.
|
biplab1
changed the title
5251358 to
43f22f5
Compare
c7f1ee1 to
bd98618
Compare
ee645e6 to
73513a2
Compare
feat(feature/loan): enable loan repayment navigation flow feat(feature/loan): enable loan repayment navigation flow feat(feature/loan): enable loan repayment navigation flow feat(feature/loan): enable loan repayment navigation flow
73513a2 to
2f47937
Compare
|
biplab1
left a comment
biplab1
left a comment
There was a problem hiding this comment.
I am merging this for now.
| val loanWithAssociations = dataState.data | ||
| if (loanWithAssociations == null) { | ||
| _loanRepaymentUiState.value = LoanRepaymentUiState.ShowError( | ||
| Res.string.feature_loan_profile_error_details_not_found, |
There was a problem hiding this comment.
The error is still vague. This needs to be handled in a separate PR and also this module needs to be migrated from MVVM to MVI.
Fixes - Jira-#MIFOSAC-640
##Description
This PR fixes the broken navigation flow for loan repayments from the Client Loan Accounts screen and introduces a more flexible navigation mechanism to handle both full entity objects and individual loan IDs.
Problem
ClientLoanAccountsScreenwas empty, making the "Make Repayment" action non-functional.LoanWithAssociationsEntity. While this works for theLoanAccountSummaryScreen, only theloanId(Int) is readily available in the context of the client loan list.LoanWithAssociationsEntityis deeply integrated into the current summary screen logic and could not be removed or replaced without breaking existing functionality.Behavior Changes
repaymentBef.webm
repaymentAft.webm
Solution & Implementation
To resolve this without breaking existing features, I implemented a dual-support navigation strategy:
1. Navigation Overloading
I overloaded the
navigateToLoanRepaymentScreenfunction. It now supports:LoanWithAssociationsEntity.loanId: Int.2. Route & ViewModel Enhancements
LoanRepaymentScreenRouteto make fields likeloanAccountNumberandclientNameoptional.LoanRepaymentViewModelto handle cases where only aloanIdis provided. If the loan details are missing on initialization, the ViewModel now uses theLoanAccountSummaryRepositoryto fetch the complete loan data by ID.ClientLoanAccountsScreento correctly pass theloanIdto the action handler.How I Managed the Constraints
Instead of refactoring the entire navigation system—which would have affected the
LoanAccountSummaryScreen—I chose to overload the navigation function. This allows the app to "lazy-load" the necessary loan details in theLoanRepaymentViewModelif they weren't provided during navigation. This ensures a seamless user experience regardless of which screen the repayment flow is started from.Changes
ClientNavigation.kt: Connected the navigation lambda to the repayment screen.LoanRepaymentScreenRoute.kt: AddedloanIdoverload and default parameter values.LoanRepaymentViewModel.kt: Added logic to fetch loan details by ID if parameters are missing.ClientLoanAccountsScreen.kt/ViewModel.kt: Updated actions to passloanId.Summary by CodeRabbit
Release Notes
Bug Fixes
Improvements