Skip to content

feat(loan): implement loan repayment form parity with Web App (MIFOSA…#2597

Open
gurnoorpannu wants to merge 23 commits intoopenMF:developmentfrom
gurnoorpannu:feature/loan-repayment-form-parity-clean
Open

feat(loan): implement loan repayment form parity with Web App (MIFOSA…#2597
gurnoorpannu wants to merge 23 commits intoopenMF:developmentfrom
gurnoorpannu:feature/loan-repayment-form-parity-clean

Conversation

@gurnoorpannu
Copy link
Contributor

@gurnoorpannu gurnoorpannu commented Feb 2, 2026

Fixes - MIFOSAC-643

Before:-
image

image

After:-
image

image image

Please make sure these boxes are checked before submitting your pull request - thanks!

  • Run the static analysis check ./gradlew check or ci-prepush.sh to make sure you didn't break anything

  • If you have multiple commits please combine them into one commit by squashing them.

Summary by CodeRabbit

Release Notes

  • New Features
    • Added payment details section displaying transaction breakdown (interest, fees, and penalties)
    • Added toggle to show/hide additional payment information fields (account, cheque, routing, and receipt numbers, external ID, notes)
    • Added option to waive penalties when available
    • Added payment success confirmation message with transaction ID reference
    • Enhanced amount formatting throughout the repayment form

@coderabbitai
Copy link

coderabbitai bot commented Feb 2, 2026

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

This 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

Cohort / File(s) Summary
String Resources for Payment Details
feature/loan/src/commonMain/composeResources/values/strings.xml
Adds 14 new string keys for loan repayment form labels: transaction breakdown, interest, fees, penalties breakdown, payment details toggle, external ID, cheque number, routing code, receipt number, bank number, note field, waive penalties option, no penalties message, and payment success message.
Loan Repayment Screen Logic & UI
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentScreen.kt
Extends LoanRepaymentScreen with showPaymentDetails toggle state and eight new optional payment fields (accountNumber, externalId, chequeNumber, routingCode, receiptNumber, bankNumber, note, waivePenalties). Updates ShowLoanRepaymentConfirmationDialog signature to accept all new fields and conditionally display payment details. Modifies calculateTotal function to accept penaltyChargesPortion and waivePenalties parameters, zeroing penalties when waived. Updates request building to include optional fields when provided.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

Possibly related PRs

Suggested reviewers

  • biplab1
  • itsPronay
  • TheKalpeshPawar

Poem

🐰 Hoppy hops through payment flows,
New fields bloom where details show,
Penalties waived with a toggle's grace,
Confirmation dance in its rightful place!

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 8.33% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title clearly and specifically describes the main change: implementing loan repayment form parity with the Web App, addressing MIFOSAC-643, which aligns with the provided objectives and file changes.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 | 🟠 Major

Confirmation dialog shows the wrong account number when user overrides it.
accountNumber can override loanAccountNumber in the request, but the dialog always displays loanAccountNumber. 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 | 🟠 Major

Remove externalId parameter or add support for it in LoanRepaymentRequestEntity.

The externalId is captured from user input and displayed, but is not included in the LoanRepaymentRequestEntity when submitting the payment request. LoanRepaymentRequestEntity currently has no externalId field. 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".

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 | 🟡 Minor

Confirmation dialog should echo the entered account number.
Line 704 always displays loanAccountNumber, which can contradict a user-entered accountNumber.

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 | 🟠 Major

Add externalId field mapping to LoanRepaymentRequestEntity if API supports it.

The externalId parameter 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 },

onCheckChanged = { showPaymentDetails = it },
)

// Payment Details Fields (conditionally shown)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove all this comments unless heavily required

)
}

// Note Field
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here also, fix all these coomments across the files

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 | 🟡 Minor

Confirmation dialog shows loanAccountNumber but request may use custom accountNumber.

Line 702 always displays loanAccountNumber, but line 667 uses accountNumber.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.

Comment on lines 666 to 680
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 },
)
Copy link

@coderabbitai coderabbitai bot Feb 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Search for LoanRepaymentRequestEntity definition
rg -n -A 30 'data class LoanRepaymentRequestEntity' --type kotlin

Repository: openMF/android-client

Length of output: 4018


🏁 Script executed:

cat -n core/database/src/commonMain/kotlin/com/mifos/room/entities/accounts/loans/LoanRepaymentRequestEntity.kt

Repository: openMF/android-client

Length of output: 1839


🏁 Script executed:

sed -n '640,741p' feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentScreen.kt

Repository: 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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 !?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 | 🟡 Minor

Resolve account number display inconsistency in confirmation dialog.

The dialog displays loanAccountNumber (line 704) but sends accountNumber.ifBlank { loanAccountNumber } in the request (line 668). When a user provides accountNumber, 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(

Comment on lines 666 to 680
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 },
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 !?

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 | 🟡 Minor

Pre-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 resource feature_loan_external_id_field and reuse feature_loan_external_id instead.

Line 26 already defines feature_loan_external_id with the identical value "External ID". The feature_loan_external_id_field at line 301 is a duplicate. Update LoanRepaymentScreen.kt (lines 511 and 716) to import and use feature_loan_external_id instead of feature_loan_external_id_field, then remove the duplicate string definition.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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: Duplicate setValue helper — extract and reuse.

This local setValue function is identical to the one in LoanRepaymentScreen.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_field has the same value as the existing feature_loan_external_id on line 26. Consider reusing the existing key instead of introducing a new one.

Comment on lines 118 to 124
fun formatCurrency(amount: Double?, code: String?, decimalPlaces: Int?): String {
return CurrencyFormatter.format(
balance = amount,
currencyCode = code,
maximumFractionDigits = decimalPlaces ?: 2,
)
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

find . -name "CurrencyFormatter.kt" -type f

Repository: openMF/android-client

Length of output: 149


🏁 Script executed:

# Find the actual file and inspect all implementations
fd -e kt "CurrencyFormatter" | head -20

Repository: openMF/android-client

Length of output: 421


🏁 Script executed:

# Search for CurrencyFormatter.format method in codebase
rg -n "fun format" -A 15 -B 2 --type=kt | grep -A 15 "CurrencyFormatter\|fun format.*currencyCode"

Repository: openMF/android-client

Length of output: 91


🏁 Script executed:

cat -n ./core/common/src/commonMain/kotlin/com/mifos/core/common/utils/CurrencyFormatter.kt

Repository: openMF/android-client

Length of output: 754


🏁 Script executed:

cat -n ./core/common/src/androidMain/kotlin/com/mifos/core/common/utils/CurrencyFormatter.android.kt

Repository: openMF/android-client

Length of output: 1103


🏁 Script executed:

cat -n ./core/common/src/desktopMain/kotlin/com/mifos/core/common/utils/CurrencyFormatter.desktop.kt

Repository: openMF/android-client

Length of output: 1082


🏁 Script executed:

cat -n ./core/common/src/nativeMain/kotlin/com/mifos/core/common/utils/CurrencyFormatter.native.kt

Repository: openMF/android-client

Length of output: 1279


🏁 Script executed:

cat -n ./feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentViewModel.kt | sed -n '110,130p'

Repository: openMF/android-client

Length of output: 836


Null currencyCode causes crashes in Android and Desktop implementations of CurrencyFormatter.

The formatCurrency method accepts nullable code: String? but passes it directly to CurrencyFormatter.format. While the Native implementation safely defaults to "$", the Android and Desktop implementations call Currency.getInstance(currencyCode) without null-checking, which throws IllegalArgumentException if currencyCode is null. Apply a safe default: code ?: "USD" or code ?: "$".

Proposed fix
 fun formatCurrency(amount: Double?, code: String?, decimalPlaces: Int?): String {
     return CurrencyFormatter.format(
         balance = amount,
-        currencyCode = code,
+        currencyCode = code ?: "$",
         maximumFractionDigits = decimalPlaces ?: 2,
     )
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
fun formatCurrency(amount: Double?, code: String?, decimalPlaces: Int?): String {
return CurrencyFormatter.format(
balance = amount,
currencyCode = code,
maximumFractionDigits = decimalPlaces ?: 2,
)
}
fun formatCurrency(amount: Double?, code: String?, decimalPlaces: Int?): String {
return CurrencyFormatter.format(
balance = amount,
currencyCode = code ?: "$",
maximumFractionDigits = decimalPlaces ?: 2,
)
}
🤖 Prompt for AI Agents
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentViewModel.kt`
around lines 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.

Comment on lines 126 to 135
fun isAllFieldsValid(
amount: String,
additionalPayment: String,
fees: String,
paymentType: String,
): Boolean {
return listOf(amount, additionalPayment, fees).all {
it.trim().toDoubleOrNull()?.let { n -> n >= 0 } == true
} && paymentType.isNotBlank()
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Validation allows zero-amount payments.

isAllFieldsValid checks n >= 0, meaning a payment of 0.0 for all three fields passes validation. If a zero-total payment should be rejected, consider requiring at least the amount field to be strictly positive (> 0).

🤖 Prompt for AI Agents
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentViewModel.kt`
around lines 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.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 consolidating feature_loan_external_id_field into the existing feature_loan_external_id key.

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.

<string name="feature_loan_note">Note</string>
<string name="feature_loan_waive_penalties">Waive Penalties</string>
<string name="feature_loan_no_penalties_found">No penalties found</string>
<string name="feature_loan_payment_success_message">Payment submitted successfully. Transaction ID: </string>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Use a format placeholder instead of string concatenation for the transaction ID.

This string ends with ": " and expects the caller to concatenate the transaction ID at runtime. This breaks localization — translators can't reorder the sentence. Line 148 already uses the correct pattern with %1$s.

Proposed fix
-    <string name="feature_loan_payment_success_message">Payment submitted successfully. Transaction ID: </string>
+    <string name="feature_loan_payment_success_message">Payment submitted successfully. Transaction ID: %1$s</string>

Note: The calling code will also need to change from string concatenation to String.format() or the Compose equivalent (e.g., stringResource(R.string.feature_loan_payment_success_message, transactionId)).

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
<string name="feature_loan_payment_success_message">Payment submitted successfully. Transaction ID: </string>
<string name="feature_loan_payment_success_message">Payment submitted successfully. Transaction ID: %1$s</string>
🤖 Prompt for AI Agents
In `@feature/loan/src/commonMain/composeResources/values/strings.xml` at 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.

@itsPronay
Copy link
Contributor

itsPronay commented Feb 13, 2026

@gurnoorpannu please address this one #2597 (comment)

There are 2 possible senario's here

  1. Api doesn't have any field called externalid, in that case we should remove that external id textfield
  2. If api is talking externalid as a field, then we need to pass it here

@gurnoorpannu
Copy link
Contributor Author

@itsPronay The webapp does include externalId in their loan repayment form

@sonarqubecloud
Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants