feat(loan): implement loan repayment form parity with Web App (MIFOSA…#2597
feat(loan): implement loan repayment form parity with Web App (MIFOSA…#2597gurnoorpannu wants to merge 23 commits intoopenMF:developmentfrom
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:
📝 WalkthroughWalkthroughThis PR adds fourteen new string resources for loan repayment form enhancements and extends the LoanRepaymentScreen to support additional payment details, including optional fields for bank information, cheque numbers, routing codes, receipt numbers, and a waive penalties toggle. The confirmation dialog and total calculation logic are updated to handle these new inputs. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 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: 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
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: 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.
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentScreen.kt
Show resolved
Hide resolved
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(
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentScreen.kt
Show resolved
Hide resolved
feature/loan/src/commonMain/composeResources/values/strings.xml
Outdated
Show resolved
Hide resolved
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/composeResources/values/strings.xml (1)
244-244:⚠️ Potential issue | 🟡 MinorPre-existing XML syntax error: stray
">after closing tag.This line has a trailing
">after</string>which is malformed XML. While not introduced in this PR, it could cause issues with stricter XML parsers or tooling.<string name="principle_paid_off">Principle Paid Off</string>">Suggested fix
- <string name="principle_paid_off">Principle Paid Off</string>"> + <string name="principle_paid_off">Principal Paid Off</string>(Also note: "Principle" → "Principal" is the correct financial term.)
🧹 Nitpick comments (1)
feature/loan/src/commonMain/composeResources/values/strings.xml (1)
301-301: Remove duplicate string resourcefeature_loan_external_id_fieldand reusefeature_loan_external_idinstead.Line 26 already defines
feature_loan_external_idwith the identical value"External ID". Thefeature_loan_external_id_fieldat line 301 is a duplicate. UpdateLoanRepaymentScreen.kt(lines 511 and 716) to import and usefeature_loan_external_idinstead offeature_loan_external_id_field, then remove the duplicate string definition.
…ty-clean' into feature/loan-repayment-form-parity-clean
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentViewModel.kt`:
- Around line 126-135: The validation in isAllFieldsValid currently accepts zero
for all numeric fields; update the logic so amount.trim().toDoubleOrNull() is
required to be > 0 while additionalPayment and fees can remain >= 0, and still
ensure paymentType.isNotBlank(); modify the check inside
LoanRepaymentViewModel.isAllFieldsValid to explicitly validate amount as
strictly positive and the other two fields as non-negative using their existing
toDoubleOrNull parsing.
- Around line 118-124: formatCurrency currently passes a nullable currency code
into CurrencyFormatter.format which causes Android/Desktop implementations
(which call Currency.getInstance) to throw when null; update formatCurrency to
supply a safe default (e.g., use code ?: "USD") before calling
CurrencyFormatter.format so CurrencyFormatter.format(balance = amount,
currencyCode = code ?: "USD", maximumFractionDigits = decimalPlaces ?: 2) and
avoid null being propagated into Currency.getInstance.
🧹 Nitpick comments (2)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentViewModel.kt (1)
105-116: DuplicatesetValuehelper — extract and reuse.This local
setValuefunction is identical to the one inLoanRepaymentScreen.kt(lines 754–763). Consider extracting it into a shared utility (e.g., a top-level function or extension) so both call sites reference the same implementation.feature/loan/src/commonMain/composeResources/values/strings.xml (1)
302-302: Duplicate string resource for "External ID".
feature_loan_external_id_fieldhas the same value as the existingfeature_loan_external_idon line 26. Consider reusing the existing key instead of introducing a new one.
...re/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentViewModel.kt
Show resolved
Hide resolved
...re/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentViewModel.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@feature/loan/src/commonMain/composeResources/values/strings.xml`:
- Line 310: The string resource feature_loan_payment_success_message currently
ends with a dangling colon and expects the caller to concatenate the transaction
ID, which breaks localization; change the resource to include a format
placeholder (e.g., use %1$s) so translators can reorder the text, and update
callers that currently concatenate to use String.format / stringResource with
the transactionId argument; specifically modify the string named
feature_loan_payment_success_message and update any call sites that append the
transaction ID to use the formatted-resource API.
🧹 Nitpick comments (1)
feature/loan/src/commonMain/composeResources/values/strings.xml (1)
302-302: Consider consolidatingfeature_loan_external_id_fieldinto the existingfeature_loan_external_idkey.Both strings have identical value
"External ID"and serve the same purpose across screens (LoanRepaymentScreen, GroupLoanAccountScreen, LoanAccountScreen). Consolidating to a single key avoids maintenance burden and potential future divergence between identical strings.
…ty-clean' into feature/loan-repayment-form-parity-clean
|
@gurnoorpannu please address this one #2597 (comment) There are 2 possible senario's here
|
|
@itsPronay The webapp does include externalId in their loan repayment form |
|
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
Release Notes