feat(loan): implement loan repayment form parity with Web App (MIFOSA… #2597
Conversation
📝 WalkthroughWalkthroughAdds 13 loan repayment string resources and enhances the Loan Repayment UI with a payment-details toggle, optional payment fields (accountNumber, externalId, chequeNumber, routingCode, receiptNumber, bankNumber, note), a waive-penalties option, updated total calculation, and expanded confirmation dialog. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentScreen.kt (2)
669-705:⚠️ Potential issue | 🟠 MajorConfirmation dialog shows the wrong account number when user overrides it.
accountNumbercan overrideloanAccountNumberin the request, but the dialog always displaysloanAccountNumber. This is a UX correctness issue.🔧 Suggested fix
private fun ShowLoanRepaymentConfirmationDialog( onDismiss: () -> Unit, loanAccountNumber: String, @@ waivePenalties: Boolean, submitPayment: (request: LoanRepaymentRequestEntity) -> Unit, ) { + val effectiveAccountNumber = accountNumber.ifBlank { loanAccountNumber } AlertDialog( @@ - val request = LoanRepaymentRequestEntity( - accountNumber = accountNumber.ifBlank { loanAccountNumber }, + val request = LoanRepaymentRequestEntity( + accountNumber = effectiveAccountNumber, paymentTypeId = paymentTypeId, @@ Text( - text = stringResource(Res.string.feature_loan_account_number) + " : " + loanAccountNumber + text = stringResource(Res.string.feature_loan_account_number) + " : " + effectiveAccountNumber )
652-682:⚠️ Potential issue | 🟠 MajorRemove
externalIdparameter or add support for it inLoanRepaymentRequestEntity.The
externalIdis captured from user input and displayed, but is not included in theLoanRepaymentRequestEntitywhen submitting the payment request.LoanRepaymentRequestEntitycurrently has noexternalIdfield. Either add the field to the entity and include it in the submission (lines 652-682), or remove the UI field collection to avoid misleading users.
🤖 Fix all issues with AI agents
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentScreen.kt`:
- Around line 32-45: Spotless formatting violations exist in LoanRepaymentScreen
imports (e.g., the androidclient.feature.loan.generated.resources imports like
feature_loan_transaction_breakdown, feature_loan_principal,
feature_loan_interest, etc.); run the Kotlin formatter via ./gradlew
:feature:loan:spotlessApply to auto-fix formatting, verify the imports and file
compile, then re-commit the updated LoanRepaymentScreen.kt changes.
- Around line 557-565: The Note field (MifosOutlinedTextField) in
LoanRepaymentScreen is always rendered; wrap it (and the other fields referenced
at 567-568) in the same conditional that controls the “Show Payment Details”
toggle so they only render when the toggle state (e.g., showPaymentDetails or
similar boolean state in LoanRepaymentScreen) is true; locate the toggle state
and use it to gate the MifosOutlinedTextField render calls so the Note and the
other payment-detail fields are hidden when the toggle is off.
- Around line 734-735: The Text showing "Yes" in the LoanRepaymentScreen UI is
hardcoded; update the Text inside the if (waivePenalties) block to use the
localized string resource instead of the literal "Yes" by calling
stringResource(Res.string.yes) and concatenating it with the existing label
(feature_loan_waive_penalties) so the displayed "Yes" is localized; modify the
Text in LoanRepaymentScreen (the branch with if (waivePenalties)) to use
stringResource(Res.string.yes) rather than the hardcoded "Yes".
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentScreen.kt
Outdated
Show resolved
Hide resolved
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentScreen.kt
Outdated
Show resolved
Hide resolved
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentScreen.kt
Outdated
Show resolved
Hide resolved
…ty-clean' into feature/loan-repayment-form-parity-clean
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentScreen.kt (2)
704-718:⚠️ Potential issue | 🟡 MinorConfirmation dialog should echo the entered account number.
Line 704 always displaysloanAccountNumber, which can contradict a user-enteredaccountNumber.Suggested fix
- Text(text = stringResource(Res.string.feature_loan_account_number) + " : " + loanAccountNumber) + Text( + text = stringResource(Res.string.feature_loan_account_number) + + " : " + accountNumber.ifBlank { loanAccountNumber } + )
652-682:⚠️ Potential issue | 🟠 MajorAdd
externalIdfield mapping toLoanRepaymentRequestEntityif API supports it.The
externalIdparameter is collected from user input but not mapped to the repayment request entity. Either add the field to the entity and include it in the request builder (if the API supports it), or remove it from the screen if it's not required.Diff (if supported by API)
- note = note.ifBlank { null }, + note = note.ifBlank { null }, + externalId = externalId.ifBlank { null },
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentScreen.kt
Outdated
Show resolved
Hide resolved
| onCheckChanged = { showPaymentDetails = it }, | ||
| ) | ||
|
|
||
| // Payment Details Fields (conditionally shown) |
There was a problem hiding this comment.
Remove all this comments unless heavily required
| ) | ||
| } | ||
|
|
||
| // Note Field |
There was a problem hiding this comment.
Here also, fix all these coomments across the files
…ty-clean' into feature/loan-repayment-form-parity-clean
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/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentScreen.kt (1)
700-702:⚠️ Potential issue | 🟡 MinorConfirmation dialog shows
loanAccountNumberbut request may use customaccountNumber.Line 702 always displays
loanAccountNumber, but line 667 usesaccountNumber.ifBlank { loanAccountNumber }. If a user enters a custom account number, the dialog will show a different value than what will be submitted, which could be confusing.🔧 Suggested fix
text = { Column { - Text(text = stringResource(Res.string.feature_loan_account_number) + " : " + loanAccountNumber) + Text(text = stringResource(Res.string.feature_loan_account_number) + " : " + accountNumber.ifBlank { loanAccountNumber })
🤖 Fix all issues with AI agents
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentScreen.kt`:
- Around line 666-680: The confirmation dialog collects and shows externalId but
it isn’t sent: add an externalId property to the LoanRepaymentRequestEntity data
class and include externalId = externalId.ifBlank { null } (or appropriate
nullable mapping) when constructing the request in LoanRepaymentScreen where
LoanRepaymentRequestEntity is created; update any serializer/constructor usages
and tests that construct LoanRepaymentRequestEntity so the new field is handled
consistently and matches the backend API name/nullable semantics.
| val request = LoanRepaymentRequestEntity( | ||
| accountNumber = loanAccountNumber, | ||
| accountNumber = accountNumber.ifBlank { loanAccountNumber }, | ||
| paymentTypeId = paymentTypeId, | ||
| dateFormat = "dd-MM-yyyy", | ||
| locale = "en", | ||
| transactionAmount = total, | ||
| transactionDate = DateHelper.getDateAsStringFromLong( | ||
| repaymentDate, | ||
| ), | ||
| checkNumber = chequeNumber.ifBlank { null }, | ||
| routingCode = routingCode.ifBlank { null }, | ||
| receiptNumber = receiptNumber.ifBlank { null }, | ||
| bankNumber = bankNumber.ifBlank { null }, | ||
| note = note.ifBlank { null }, | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for LoanRepaymentRequestEntity definition
rg -n -A 30 'data class LoanRepaymentRequestEntity' --type kotlinRepository: openMF/android-client
Length of output: 4018
🏁 Script executed:
cat -n core/database/src/commonMain/kotlin/com/mifos/room/entities/accounts/loans/LoanRepaymentRequestEntity.ktRepository: openMF/android-client
Length of output: 1839
🏁 Script executed:
sed -n '640,741p' feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentScreen.ktRepository: openMF/android-client
Length of output: 4463
externalId is collected and displayed but not included in the request.
The externalId field is passed to the dialog, displayed in the confirmation dialog (line 714-716), but never included in LoanRepaymentRequestEntity. The entity definition lacks an externalId field. If the backend API accepts this field, add it to LoanRepaymentRequestEntity and include it in the request creation.
🤖 Prompt for AI Agents
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentScreen.kt`
around lines 666 - 680, The confirmation dialog collects and shows externalId
but it isn’t sent: add an externalId property to the LoanRepaymentRequestEntity
data class and include externalId = externalId.ifBlank { null } (or appropriate
nullable mapping) when constructing the request in LoanRepaymentScreen where
LoanRepaymentRequestEntity is created; update any serializer/constructor usages
and tests that construct LoanRepaymentRequestEntity so the new field is handled
consistently and matches the backend API name/nullable semantics.
There was a problem hiding this comment.
@gurnoorpannu even tho we are taking externalId from the user, why are we not passing it through the api?
What is the behaviour of the web app in this case? What we did there !?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentScreen.kt (1)
668-704:⚠️ Potential issue | 🟡 MinorResolve account number display inconsistency in confirmation dialog.
The dialog displays
loanAccountNumber(line 704) but sendsaccountNumber.ifBlank { loanAccountNumber }in the request (line 668). When a user providesaccountNumber, they won't see it reflected in the dialog preview, creating a UX mismatch. Extract the resolved value once and use it consistently in both the display and request:Suggested fix
private fun ShowLoanRepaymentConfirmationDialog( onDismiss: () -> Unit, loanAccountNumber: String, paymentTypeId: String, @@ -658,7 +658,8 @@ private fun ShowLoanRepaymentConfirmationDialog( waivePenalties: Boolean, submitPayment: (request: LoanRepaymentRequestEntity) -> Unit, ) { + val resolvedAccountNumber = accountNumber.ifBlank { loanAccountNumber } AlertDialog( onDismissRequest = { onDismiss() }, confirmButton = { @@ -667,7 +668,7 @@ private fun ShowLoanRepaymentConfirmationDialog( onDismiss() val request = LoanRepaymentRequestEntity( - accountNumber = accountNumber.ifBlank { loanAccountNumber }, + accountNumber = resolvedAccountNumber, paymentTypeId = paymentTypeId, dateFormat = "dd-MM-yyyy", locale = "en", @@ -704,7 +705,7 @@ private fun ShowLoanRepaymentConfirmationDialog( text = { Column { - Text(text = stringResource(Res.string.feature_loan_account_number) + " : " + loanAccountNumber) + Text(text = stringResource(Res.string.feature_loan_account_number) + " : " + resolvedAccountNumber) Text(
| val request = LoanRepaymentRequestEntity( | ||
| accountNumber = loanAccountNumber, | ||
| accountNumber = accountNumber.ifBlank { loanAccountNumber }, | ||
| paymentTypeId = paymentTypeId, | ||
| dateFormat = "dd-MM-yyyy", | ||
| locale = "en", | ||
| transactionAmount = total, | ||
| transactionDate = DateHelper.getDateAsStringFromLong( | ||
| repaymentDate, | ||
| ), | ||
| checkNumber = chequeNumber.ifBlank { null }, | ||
| routingCode = routingCode.ifBlank { null }, | ||
| receiptNumber = receiptNumber.ifBlank { null }, | ||
| bankNumber = bankNumber.ifBlank { null }, | ||
| note = note.ifBlank { null }, | ||
| ) |
There was a problem hiding this comment.
@gurnoorpannu even tho we are taking externalId from the user, why are we not passing it through the api?
What is the behaviour of the web app in this case? What we did there !?
| <string name="feature_loan_cheque_number">Cheque #</string> | ||
| <string name="feature_loan_routing_code">Routing Code</string> | ||
| <string name="feature_loan_receipt_number">Receipt #</string> | ||
| <string name="feature_loan_bank_number">Bank #</string> |
There was a problem hiding this comment.
@gurnoorpannu i see, for some text fields, there is a hash sign, i checked the web app and its the same there.
What's the reason behind it? Is this a mandatory field?
3 participants
Fixes - MIFOSAC-643
Before:-
After:-
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