fix(feature/client): correct loan repayment navigation flow#2599
fix(feature/client): correct loan repayment navigation flow#2599Kartikey15dem wants to merge 1 commit intoopenMF:developmentfrom
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)
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.
...re/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentViewModel.kt
Show resolved
Hide resolved
...rc/commonMain/kotlin/com/mifos/feature/client/clientLoanAccounts/ClientLoanAccountsScreen.kt
Show resolved
Hide resolved
![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.
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentScreen.kt
Show resolved
Hide resolved
...re/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentViewModel.kt
Show resolved
Hide resolved
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentScreen.kt
Show resolved
Hide resolved
| MutableStateFlow<LoanRepaymentUiState>(LoanRepaymentUiState.ShowProgressbar) | ||
| val loanRepaymentUiState: StateFlow<LoanRepaymentUiState> get() = _loanRepaymentUiState | ||
|
|
||
| init { |
There was a problem hiding this comment.
Call checkDatabaseLoanRepaymentByLoanId() in this init block only. (and remove it from the loadLoanById function)
it would look like this
init {
if (_arg.loanAccountNumber.isEmpty()) {
loadLoanById()
}
checkDatabaseLoanRepaymentByLoanId()
There was a problem hiding this comment.
Yes, I observed that checkDatabaseLoanRepaymentByLoanId() can be called regardless of the fact whether the loan details are being loaded or we already have from the args as in both we are getting the loanId by args but I have not called the function in viewmodel and kept it like it was before my changes because calling it only in init block may display stale data until the screen is in backstack . Suppose , if we navigate to another screen and navigate back to this screen then launched effect will load the data again but the init block will not do so until the viewmodel is destroyed and created again.
There was a problem hiding this comment.
"If we navigate to another screen and navigate back to this screen then launched effect will load the data again"
Are you sure about this? because on the internet I see that LaunchedEffect runs only two times 1. Entering in the composition, 2. when the keys are updated. Navigating to other screen doesn't remove current screen from composition, current screen stays in the backstack. So navigating back will not add LaunchedEffect again in the composition and hence checkDatabaseLoanRepaymentByLoanId will not be called again I think.
Have you tested that?
Because if that's not happening then we should add a button to load them
There was a problem hiding this comment.
@sahilshivekar
I cross checked by adding an empty notification screen and navigated to that screen and then navigated back and the launched effect code ran again . You can check once for account 053 in maria. Although I am still a bit confused as different LLMs are giving different answers and clarify it from any senior but keeping the launched effect is better in either case as it manually checked it .
There was a problem hiding this comment.
You can check for loan account 053 in maria.
There was a problem hiding this comment.
I am also confused that why LaunchedEffect is again triggering if the composition is in the backstack.
Need to ask about it to a senior. Ask about it in daily standup tmrw if you are also unsure
There was a problem hiding this comment.
@sahilshivekar
But still I think it can be approved because using launchedeffect won't cause any harm, instead it will better.
fix(feature/client): correct loan repayment navigation flow fix(feature/client): correct loan repayment navigation flow fix(feature/client): correct loan repayment navigation flow
Kartikey15dem
force-pushed
the
fix/loan-repayment-navigation
branch
from
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.
...re/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentViewModel.kt
Show resolved
Hide resolved
2 participants
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