fix(client) : Fixed the Loan Accounts Screen not displaying loan details.#2601
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds nullable loan fields (originalLoan, loanBalance, amountPaid) to the network loan model, renames loan type field to Changes
Sequence Diagram(s)sequenceDiagram
participant API as Network Model
participant Mapper as ClientIdAccountMapper
participant DB as Room LoanEntity
participant UI as ClientLoanAccountsScreen
API->>Mapper: Return GetClientsLoanAccounts (originalLoan, loanBalance, amountPaid, loanType.value)
Mapper->>DB: Map to LoanAccountEntity (include originalLoan, loanBalance, amountPaid)
DB-->>Mapper: Persist/return entity
Mapper->>UI: Provide domain loan with loanBalance & amountPaid
UI->>UI: Render loanBalance/amountPaid (use stringResource fallback if pendingApproval)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@core/network/src/commonMain/kotlin/com/mifos/core/network/mappers/clients/GetClientsClientIdAccountMapper.kt`:
- Around line 114-119: The amountPaid calculation in
GetClientsClientIdAccountMapper currently treats a null loanBalance as 0.0
(using it.loanBalance ?: 0.0), which makes amountPaid equal originalLoan when
balance is unknown; change the logic so that when originalLoan is non-null but
loanBalance is null, amountPaid is set to null, otherwise compute amountPaid =
max(0.0, it.originalLoan - it.loanBalance); update the amountPaid branch (the
block referencing it.originalLoan and it.loanBalance) to reflect this safer
null-handling behavior.
🧹 Nitpick comments (1)
core/network/src/commonMain/kotlin/com/mifos/core/network/model/GetClientsLoanAccounts.kt (1)
45-47: New fields added correctly; consider ifamountPaidbelongs in the network model.The nullable Double fields with defaults are safe for backward compatibility. However,
amountPaidappears to be computed locally in the mapper asmax(0.0, originalLoan - loanBalance)rather than received from the API. If the backend doesn't returnamountPaid, consider removing it from this network DTO to clarify that it's a derived value, keeping it only in the entity/domain layer.Also, the KDoc (lines 14-25) should be updated to document these new parameters.
.../commonMain/kotlin/com/mifos/core/network/mappers/clients/GetClientsClientIdAccountMapper.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
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)
174-176:⚠️ Potential issue | 🟡 MinorFallback to
0for nullloan.idmay cause incorrect navigation.If
loan.idis null, passing0as the ID toViewAccountor downstream navigation could result in invalid API calls or unexpected behavior.🛡️ Suggested approach
Consider filtering out loans with null IDs earlier, or handle the null case explicitly (e.g., show an error or skip the action):
is Actions.ViewAccount -> { + loan.id?.let { id -> + onAction(ClientLoanAccountsAction.ViewAccount(id)) + } - onAction( - ClientLoanAccountsAction.ViewAccount(loan.id ?: 0), - ) }
🤖 Fix all issues with AI agents
In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientLoanAccounts/ClientLoanAccountsScreen.kt`:
- Around line 137-148: The code in ClientLoanAccountsScreen.kt is currently
prepending the currency symbol to the "Not Available" placeholder for
originalLoan, amountPaid, and loanBalance; change the construction so the symbol
(variable symbol) is concatenated only when the underlying value is
non-null/actual (e.g., loan.originalLoan, loan.amountPaid, loan.loanBalance),
and return the plain "Not Available" string if the value is null—update the
expressions that build originalLoan, amountPaid, and loanBalance to
conditionally prepend symbol only for real numeric values.
🧹 Nitpick comments (2)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientLoanAccounts/ClientLoanAccountsScreen.kt (2)
135-149: Hardcoded "Not Available" strings should use string resources for localization.The string
"Not Available"is repeated multiple times. Extract it to a string resource for consistency and i18n support.♻️ Suggested approach
Define a string resource (e.g.,
Res.string.feature_client_not_available) and replace all occurrences:val notAvailable = stringResource(Res.string.feature_client_not_available) MifosActionsLoanListingComponent( accountNo = loan.accountNo ?: notAvailable, loanProduct = loan.productName ?: notAvailable, originalLoan = loan.originalLoan?.let { symbol + it.toString() } ?: notAvailable, // ... etc )
181-182:else -> nullin when expression is unnecessary.The
onActionClickedlambda doesn't require a return value. Consider usingUnitor an empty block for clarity, or remove the else branch entirely if all expected actions are handled.♻️ Suggested fix
onActionClicked = { actions -> when (actions) { is Actions.ViewAccount -> onAction( ClientLoanAccountsAction.ViewAccount(loan.id ?: 0), ) is Actions.MakeRepayment -> onAction( ClientLoanAccountsAction.MakeRepayment, ) - else -> null + else -> Unit } },
...rc/commonMain/kotlin/com/mifos/feature/client/clientLoanAccounts/ClientLoanAccountsScreen.kt
Outdated
Show resolved
Hide resolved
|
@techsavvy185 Can you tell us the reason why some account are still showing "Not Available"? 
|
|
@biplab1 Actually that is due to the fact that those accounts are not approved yet. So there actually is no data for them at present. |
For example, I have approved 1 more account and it is now showing the relevant details.
|
.../commonMain/kotlin/com/mifos/core/network/mappers/clients/GetClientsClientIdAccountMapper.kt
Outdated
Show resolved
Hide resolved
@techsavvy185 Yes, I thought so. Thanks for confirming. |
There was a problem hiding this comment.
Actionable comments posted: 1
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)
137-139:⚠️ Potential issue | 🟡 Minor
originalLoanis missing thependingApprovalguard that was added foramountPaidandloanBalance.If the intent is to show "Not Available" for unapproved loans,
originalLoanshould follow the same pattern. Currently it will display"$null"→"$Not Available"regardless of approval status, which is inconsistent with the other two fields.Proposed fix (combined with the symbol-prepend fix)
- originalLoan = symbol + ( - (loan.originalLoan ?: "Not Available").toString() - ), + originalLoan = if (loan.status?.pendingApproval == true) { + "Not Available" + } else { + loan.originalLoan?.let { symbol + it.toString() } ?: "Not Available" + },
🤖 Fix all issues with AI agents
In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientLoanAccounts/ClientLoanAccountsScreen.kt`:
- Around line 140-163: The current logic prepends symbol to a stringified
fallback "0" for amountPaid and loanBalance even when the values are null (in
ClientLoanAccountsScreen code using amountPaid, loanBalance, symbol and
loan.status?.pendingApproval), which can be misleading; change the fallback so
that when loan.status?.pendingApproval == true you show "Not Available" as now,
and when pendingApproval == false but amountPaid or loanBalance is null use "Not
Available" (or "N/A") instead of "0", and only prepend the currency symbol when
the displayed value is numeric—i.e., check if the chosen display value is
numeric before concatenating symbol so non-numeric "Not Available" is shown
without a currency symbol.
...rc/commonMain/kotlin/com/mifos/feature/client/clientLoanAccounts/ClientLoanAccountsScreen.kt
Show resolved
Hide resolved
...rc/commonMain/kotlin/com/mifos/feature/client/clientLoanAccounts/ClientLoanAccountsScreen.kt
Outdated
Show resolved
Hide resolved
...rc/commonMain/kotlin/com/mifos/feature/client/clientLoanAccounts/ClientLoanAccountsScreen.kt
Outdated
Show resolved
Hide resolved
...rc/commonMain/kotlin/com/mifos/feature/client/clientLoanAccounts/ClientLoanAccountsScreen.kt
Outdated
Show resolved
Hide resolved
...rc/commonMain/kotlin/com/mifos/feature/client/clientLoanAccounts/ClientLoanAccountsScreen.kt
Outdated
Show resolved
Hide resolved
biplab1
left a comment
biplab1
left a comment
There was a problem hiding this comment.
Looks good to me. This can be merged once the requested updates have been addressed.
techsavvy185
force-pushed
the
loanAccountsScreenDetails
branch
from
70865c0 to
09b27ae
Compare
...rc/commonMain/kotlin/com/mifos/feature/client/clientLoanAccounts/ClientLoanAccountsScreen.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
core/network/src/commonMain/kotlin/com/mifos/core/network/model/GetClientsLoanAccountsType.kt (1)
14-19:⚠️ Potential issue | 🟡 MinorStale KDoc:
@param descriptionshould be@param value.The field was renamed from
descriptiontovalue, but the doc comment still referencesdescription.📝 Proposed fix
/** * * * `@param` code - * `@param` description + * `@param` value * `@param` id */core/network/src/commonMain/kotlin/com/mifos/core/network/mappers/clients/GetClientsClientIdAccountMapper.kt (1)
165-171:⚠️ Potential issue | 🟡 MinorPre-existing: force-unwrap on
it.currency!!will crash on null currency.Not introduced by this PR, but worth noting — if a savings account has a null
currency, this will throw aNullPointerException. Consider using safe calls with?.here as well.
techsavvy185
force-pushed
the
loanAccountsScreenDetails
branch
from
4e44af6 to
e9eb8bc
Compare
techsavvy185
force-pushed
the
loanAccountsScreenDetails
branch
from
4bef93c to
baaae17
Compare
|
3 participants
Fixes - Jira-#636
Screen_recording_20260209_182033.webm
Please make sure these boxes are checked before submitting your pull request - thanks!
Run the static analysis check
./gradlew checkorci-prepush.shto make sure you didn't break anythingIf you have multiple commits please combine them into one commit by squashing them.
Summary by CodeRabbit
New Features
Bug Fixes