fix(loan): improved repayment screen ui/ux#2596
fix(loan): improved repayment screen ui/ux#2596therajanmaurya merged 6 commits intoopenMF:developmentfrom
Conversation
📝 WalkthroughWalkthroughReplaces a combined success string with separate title and transaction-label resources; moves confirmation from a modal dialog to a bottom sheet; adds currency-aware helpers (calculateTotal, formatCurrency, isAllFieldsValid) to the ViewModel and threads them through LoanRepayment composables. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as LoanRepaymentScreen
participant VM as LoanRepaymentViewModel
participant API as PaymentSubmission
User->>UI: enter amount, fees, additional payment, payment type
UI->>VM: isAllFieldsValid(amount, additional, fees, type)?
VM-->>UI: valid/invalid
alt valid
UI->>VM: calculateTotal(fees, amount, additional)
VM-->>UI: total
UI->>UI: formatCurrency(total, currencyCode, decimals)
UI->>UI: show ConfirmationBottomSheet(formatted values)
User->>UI: confirm
UI->>VM: submitPayment(...)
VM->>API: perform payment request
API-->>VM: success (transactionId)
VM-->>UI: success result
UI->>UI: show SuccessBottomSheet(title, transaction label + id)
else invalid
UI-->>User: show validation errors
end
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: 1
🤖 Fix all issues with AI agents
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentViewModel.kt`:
- Around line 105-115: The calculateTotal function currently treats non-numeric
input as 0.0, allowing invalid (non-numeric or negative) values to pass
validation; update calculateTotal and any similar routine (see calculateTotal
usage around the second occurrence) to (1) explicitly validate each input string
is a valid number using a safe parse (reject strings that fail to parse), (2)
ensure parsed values are non-negative (return an error/NaN or throw/flag invalid
input instead of silently using 0.0), and (3) compute and return a consistently
rounded total (e.g., round to 2 decimal places) so callers like isAllFieldsValid
can detect and reject invalid totals rather than submitting incorrect payments.
Ensure you reference and update the calculateTotal function and its callers to
handle validation errors consistently.
🧹 Nitpick comments (2)
feature/loan/src/commonMain/composeResources/values/strings.xml (1)
146-147: Prefer a formatted string if the ID is appended in code.If the UI concatenates the transaction ID onto this label, localization/RTL ordering can break. Consider a formatted resource and pass the ID as an argument.
♻️ Suggested adjustment
- <string name="feature_loan_payment_success_message">Transaction ID:</string> + <string name="feature_loan_payment_success_message">Transaction ID: %1$s</string>feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentViewModel.kt (1)
117-134: Adopt locale-aware currency formatting viaexpect/actualpattern for KMP.Manual string splitting and rounding ignore locale conventions for decimal separators, currency symbols, and formatting. For Kotlin Multiplatform, wrap platform-specific formatters:
- Android/JVM:
java.text.NumberFormat.getCurrencyInstance(locale)- iOS:
NSNumberFormatterwithNSNumberFormatterCurrencyStyle- JS:
Intl.NumberFormatwithstyle: "currency"This ensures correct formatting across locales and avoids fragile string manipulation.
...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/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentViewModel.kt`:
- Around line 118-135: The formatCurrency function currently uses
rounded.toString() which can produce scientific notation for large values;
change formatCurrency to compute integer cents (e.g., cents = round(value *
100).toLong()), use integer division and modulus to derive integerPart = cents /
100 and fractionalPart = absolute(cents % 100) padded to 2 digits, then build
finalAmount = "$integerPart.$fractionalPart" (preserving sign for negatives) and
prepend the currency symbol handling already done for
Constants.CURRENCY_USD/Constants.SYMBOL_DOLLAR; this guarantees fixed
two-decimal output and avoids scientific-notation issues.
...re/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentViewModel.kt
Outdated
Show resolved
Hide resolved
core/common/src/commonMain/kotlin/com/mifos/core/common/utils/Constants.kt
Outdated
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: 1
🤖 Fix all issues with AI agents
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentScreen.kt`:
- Around line 207-214: The ShowPaymentSubmittedSuccessfully branch currently
ignores the case where uiState.loanRepaymentResponse is null; update the
LoanRepaymentScreen to handle that edge case by providing a fallback UI or
treating it as an error: when
LoanRepaymentUiState.ShowPaymentSubmittedSuccessfully is emitted and
loanRepaymentResponse is null, render a fallback (for example a generic
SuccessBottomSheet with a default response payload or an
ErrorBottomSheet/message) and ensure onDismiss still calls navigateBack so the
user gets clear feedback; reference
LoanRepaymentUiState.ShowPaymentSubmittedSuccessfully,
uiState.loanRepaymentResponse, SuccessBottomSheet, and navigateBack to locate
where to implement the null-handling.
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: 2
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)
455-466:⚠️ Potential issue | 🟡 MinorHardcoded colors break dark theme compatibility.
BlackandDarkGrayare hardcoded, which will cause poor contrast or visibility issues in dark mode. Use semantic colors fromMaterialTheme.colorSchemeinstead.🎨 Suggested fix
Text( style = MaterialTheme.typography.bodyLarge, text = title, - color = Black, + color = MaterialTheme.colorScheme.onSurface, ) Text( style = MaterialTheme.typography.bodyLarge, text = value, - color = DarkGray, + color = MaterialTheme.colorScheme.onSurfaceVariant, fontWeight = FontWeight.Medium, )
🤖 Fix all issues with AI agents
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentScreen.kt`:
- Around line 574-575: Replace the hardcoded DarkGray color usage in the Text
composable (inside the component that renders label/value — same pattern as
FarApartTextItem) with a semantic color from the Material theme (e.g., use
MaterialTheme.colorScheme.onSurfaceVariant or
MaterialTheme.colorScheme.onSurface for body text) so the label adapts to
light/dark themes; update the Text call that currently uses DarkGray to
reference the chosen MaterialTheme.colorScheme property instead.
- Around line 611-616: Replace the string concatenation in LoanRepaymentScreen
where Text uses
"${stringResource(Res.string.feature_loan_payment_success_transaction_label)}
${response.resourceId}" with a formatted string resource (e.g.,
feature_loan_payment_success_transaction with a %1$s placeholder) and call
stringResource(...) with response.resourceId as the format argument so
localization can reorder placeholders; also replace the hardcoded DarkGray color
with a semantic MaterialTheme color (for example a surface/foreground semantic
token such as MaterialTheme.colorScheme.onSurfaceVariant or onBackground) so the
UI follows theme semantics.
🧹 Nitpick comments (2)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentScreen.kt (2)
276-276: Avoid unnecessary String↔Double roundtrip fortotal.
calculateTotalreturns aDouble, which is converted toStringhere, passed toConfirmationBottomSheet, then parsed back toDoubleon line 535. Pass theDoubledirectly to avoid redundant conversions and potential parsing edge cases.♻️ Suggested change
Change the
totalparameter type inConfirmationBottomSheetfromStringtoDouble:- total = calculateTotal(fees, amount, additionalPayment).toString(), + total = calculateTotal(fees, amount, additionalPayment),And update
ConfirmationBottomSheetsignature and usage accordingly (line 480 and 535).
367-367: Simplify null handling forpaymentTypeOptions.The
!!operator after a null check is redundant and can be simplified using safe calls.♻️ Suggested refactor
- options = if (loanRepaymentTemplate.paymentTypeOptions != null) loanRepaymentTemplate.paymentTypeOptions!!.map { it.name } else listOf(), + options = loanRepaymentTemplate.paymentTypeOptions?.map { it.name }.orEmpty(),
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
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/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentScreen.kt`:
- Around line 431-442: isAllFieldsValid currently forces non-blank numeric
values for amount/additionalPayment/fees while calculateTotal treats blanks as
zero, blocking users who leave optional fields empty; update the validation used
before showing the confirmation (the isAllFieldsValid call) to only require a
valid amount and paymentType and treat additionalPayment and fees as valid when
blank (or parse blanks as zero) so it matches calculateTotal behavior and allow
MifosTwoButtonRow's second button to enable when optional inputs are omitted.
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanRepayment/LoanRepaymentScreen.kt
Show resolved
Hide resolved
|
@kartikey004 Make sure to update the screen recording or screenshot after every commit. |
Yes i did verify the flow locally before every commit but since these changes were internal refactoring and did not result in any visual changes, I did not update the recording. I will keep this in mind for future updates. |
kartikey004
force-pushed
the
fix/repayment-screen-ui
branch
from
ea0fb98 to
175d147
Compare
biplab1
left a comment
biplab1
left a comment
There was a problem hiding this comment.
Looks good to me. This can be merged.
3 participants
Fixes - Jira-#642
Description - I improved the Loan Repayment Screen UI. Fixed input field sizing and switched native dialogs to
MifosBottomSheet. Also corrected currency formatting, removed side-effects that were causing navigation crashes and added proper form validation.Video:
UI_LoanRepayment.mp4
Summary by CodeRabbit
New Features
Refactor