fixing MIFOSAC-635 : improving code in LoanSummaryScreen#2588
fixing MIFOSAC-635 : improving code in LoanSummaryScreen#2588gurnoorpannu wants to merge 31 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:
📝 WalkthroughWalkthroughReplaces a sealed UiState with a data-driven LoanAccountSummaryState and action/event types; refactors the ViewModel to an internal BaseViewModel with init-based loading and handleAction; renames/composable API to LoanAccountSummaryScreenRoute and replaces navigateBack → onNavigateBack. Changes
Sequence Diagram(s)sequenceDiagram
participant Screen as LoanAccountSummaryScreenRoute
participant VM as LoanAccountSummaryViewModel
participant Repo as LoanAccountSummaryRepository
participant Nav as NavController
Screen->>VM: init / onAction(user)
VM->>Repo: loadLoanById(loanId)
Repo-->>VM: Success(loan) / Failure(error)
VM->>Screen: update state (LoanAccountSummaryState) / emit event (DialogState, Snackbar, Navigation)
Screen->>Nav: perform navigation / onNavigateBack
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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: 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/loanAccountSummary/LoanAccountSummaryScreen.kt (1)
298-316: Prevent rendering"null"string in summary cards.When
summaryis null, the safe access operator evaluates to null, and calling.toString()on null produces the literal string"null"in the UI. Use?.toString().orEmpty()for null-safe display.Suggested fix
- infoText = summary?.totalExpectedRepayment.toString(), + infoText = summary?.totalExpectedRepayment?.toString().orEmpty(), ... - infoText = summary?.totalRepayment.toString(), + infoText = summary?.totalRepayment?.toString().orEmpty(), ... - infoText = summary?.totalOutstanding.toString(), + infoText = summary?.totalOutstanding?.toString().orEmpty(),
🤖 Fix all issues with AI agents
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryScreen.kt`:
- Line 158: Run the module’s spotlessApply to fix formatting issues and update
the offending lambda/spacing so the code matches project style; specifically, in
LoanAccountSummaryScreen.kt correct the onRetry assignment (the
remember(viewModel) { {
viewModel.trySendAction(LoanAccountSummaryAction.OnRetry) } } expression) and
the other similar lambda/spacing occurrences referenced by Spotless so they
conform to the formatter (keep consistent spacing, indentation, and trailing
commas as the codebase requires) and re-run spotlessApply to verify all
violations are resolved.
- Around line 657-695: The button text logic in LoanActionButton treats
closedObligationsMet as active which yields "Make Repayment" while the button is
disabled; change the when and isEnabled logic so closedObligationsMet maps to
the closed label and does not enable the button: update the when in
LoanActionButton (the buttonText computation) to check status.active == true
first, add an explicit branch for status.closedObligationsMet == true that
returns stringResource(Res.string.feature_loan_closed), and ensure the isEnabled
boolean excludes closedObligationsMet (i.e., only true for active,
pendingApproval, or waitingForDisbursal).
.../src/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryScreen.kt
Outdated
Show resolved
Hide resolved
.../src/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryScreen.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/loanAccountSummary/LoanAccountSummaryScreen.kt (1)
300-318: Avoid displaying literal "null" when summary is hidden.
summary?.… .toString()yields"null"whensummaryis absent, which surfaces in the UI. Use a safe fallback string.✅ Suggested fix
- infoText = summary?.totalExpectedRepayment.toString(), + infoText = summary?.totalExpectedRepayment?.toString().orEmpty(), @@ - infoText = summary?.totalRepayment.toString(), + infoText = summary?.totalRepayment?.toString().orEmpty(), @@ - infoText = summary?.totalOutstanding.toString(), + infoText = summary?.totalOutstanding?.toString().orEmpty(),
| .height(DesignToken.sizes.buttonHeightMedium), | ||
| shape = DesignToken.shapes.small, | ||
| onClick = onAction, | ||
| colors = ButtonDefaults.buttonColors(containerColor = MaterialTheme.colorScheme.primary), |
There was a problem hiding this comment.
This line is unnecessary as well. The button by default use the primary color.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
.../src/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryScreen.kt
Outdated
Show resolved
Hide resolved
.../src/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryScreen.kt
Outdated
Show resolved
Hide resolved
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/loanAccountSummary/LoanAccountSummaryScreen.kt`:
- Around line 117-147: The EventsEffect block in LoanAccountSummaryScreen
contains Spotless Kotlin formatting violations caused by inconsistent formatting
around the when branches for LoanAccountSummaryEvent; run the formatter and
apply fixes (e.g., execute ./gradlew :feature:loan:spotlessApply) then reformat
the EventsEffect block in LoanAccountSummaryScreen so that the when clauses and
lambda braces conform to project Spotless rules; ensure LoanAccountSummaryEvent
branches (NavigateBack, NavigateToMoreInfo, NavigateToTransactions,
NavigateToRepaymentSchedule, NavigateToDocuments, NavigateToCharges,
NavigateToApproveLoan, NavigateToDisburseLoan, NavigateToMakeRepayment) are
neatly aligned, no trailing whitespace exists, and commit the resulting changes.
- Around line 706-709: The button label Text composable is using
MaterialTheme.colorScheme.background for its color which can lack contrast on a
primary-colored button; update the Text color to
MaterialTheme.colorScheme.onPrimary so the buttonText contrasts correctly with
primary backgrounds—locate the Text where buttonText is used in
LoanAccountSummaryScreen (the Text call around the button rendering) and replace
the color reference from background to onPrimary.
🧹 Nitpick comments (2)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryScreen.kt (2)
356-367: Consider locale-aware date formatting.The hardcoded
DD/MM/YYYYformat may not match all users' locale expectations. Consider using a locale-aware formatter if internationalization is a requirement.♻️ Suggested locale-aware approach
// Alternative using kotlinx-datetime or platform-specific formatters // For now, keeping as-is is acceptable if locale consistency isn't critical
532-557: Consider adding default value formodifierparameter.Compose convention is to provide
Modifier = Modifieras the default. While this is a private composable and currently always receives an explicit modifier, adding a default improves API consistency.♻️ Suggested fix
private fun InfoCard( titleText: String, infoText: String, - modifier: Modifier, + modifier: Modifier = Modifier, ) {
.../src/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryScreen.kt
Show resolved
Hide resolved
.../src/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryScreen.kt
Outdated
Show resolved
Hide resolved
|
@gurnoorpannu, please don't resolve conversations on your own. |
| } | ||
|
|
||
| MifosScaffold( | ||
| modifier = modifier, |
There was a problem hiding this comment.
remove mifosScaffold, we don't need it
.../src/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryScreen.kt
Outdated
Show resolved
Hide resolved
.../src/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryScreen.kt
Outdated
Show resolved
Hide resolved
.../src/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryScreen.kt
Outdated
Show resolved
Hide resolved
| .height(DesignToken.sizes.buttonHeightMedium), | ||
| shape = DesignToken.shapes.small, | ||
| onClick = onAction, | ||
| colors = ButtonDefaults.buttonColors(containerColor = MaterialTheme.colorScheme.primary), |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
...c/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryViewModel.kt
Show resolved
Hide resolved
|
@itsPronay i implemented the changes requested, can you review them |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryScreen.kt`:
- Around line 161-166: LaunchedEffect watching state.showLoanIdCopiedMessage
currently only dispatches OnMessageShown and doesn't present any UI feedback;
update the effect to show the message (e.g., via a SnackbarHostState or Toast)
using state.loanIdCopiedMessage before calling
onAction(LoanAccountSummaryAction.OnMessageShown). Locate the LaunchedEffect
block in LoanAccountSummaryScreen, obtain or create a SnackbarHostState (or pass
one into the composable), call
snackbarHostState.showSnackbar(state.loanIdCopiedMessage) (or trigger a Toast)
when state.showLoanIdCopiedMessage is true, then invoke
onAction(LoanAccountSummaryAction.OnMessageShown) to clear the flag. Ensure you
reference the existing state.showLoanIdCopiedMessage, state.loanIdCopiedMessage,
LaunchedEffect, and onAction(LoanAccountSummaryAction.OnMessageShown) symbols.
- Around line 319-332: The Canvas in LoanAccountSummaryScreen currently sets
contentDescription = "" which is inaccessible; create a descriptive label (e.g.,
"Loan status: Active", "Loan status: Pending approval", "Loan status: Waiting
for disbursal", or "Loan status: Other") by computing a statusText using the
same status checks used for the circle color (inspect
loanWithAssociations.status.active, pendingApproval, waitingForDisbursal) and
pass that string to the Canvas contentDescription instead of an empty string;
ensure the text is localized if app uses resources and matches the color logic
in the onDraw block.
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryViewModel.kt`:
- Around line 79-83: loadLoanById currently launches a new collector each call
which can cause overlapping collectors to race; change the collection to cancel
any in‑flight work before starting a new one by either replacing
repository.getLoanById(...).collect { ... } with
repository.getLoanById(...).collectLatest { ... } (preferred) or track and
cancel a Job (e.g., a loanLoadJob property) and cancel it before launching a new
viewModelScope.launch; ensure the rest of the handler that updates
mutableStateFlow (and the DialogState) remains unchanged so only the latest
emission is applied.
- Around line 97-101: The call to getString (a suspend function) is inside
mutableStateFlow.update's non-suspending lambda; compute the error message
before calling mutableStateFlow.update and pass the precomputed String into the
update. Concretely, call
getString(Res.string.feature_loan_unknown_error_occured) into a local val (e.g.,
errorMsg) and then call mutableStateFlow.update { it.copy(dialogState =
LoanAccountSummaryState.DialogState.Error(errorMsg)) } so the update lambda no
longer invokes getString.
🧹 Nitpick comments (1)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryScreen.kt (1)
283-290: Consider localizing the date format.The date format
"$day/$month/$year"is hardcoded as DD/MM/YYYY. This may not match user locale expectations (e.g., US uses MM/DD/YYYY). Consider using a localized date formatter.
.../src/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryScreen.kt
Outdated
Show resolved
Hide resolved
.../src/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryScreen.kt
Outdated
Show resolved
Hide resolved
...c/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryViewModel.kt
Show resolved
Hide resolved
...c/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryViewModel.kt
Outdated
Show resolved
Hide resolved
# Conflicts: # feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryScreen.kt
gurnoorpannu
force-pushed
the
refactor/improve-loan-account-summary-screen
branch
from
bba628e to
b62727f
Compare
| sendEvent( | ||
| LoanAccountSummaryEvent.NavigateToApproveLoan( | ||
| loanAccountNumber, | ||
| action.loanWithAssociations, |
There was a problem hiding this comment.
here we are relying on the UI to pass the LoanWithAssociationsEntity back to us. I suggest you should change this to retrieve the data directly from the ViewModel's own state. this will avoid stale data issues.
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/loanAccountSummary/LoanAccountSummaryScreen.kt (1)
308-321:⚠️ Potential issue | 🟡 MinorUnsafe cast may throw
ClassCastException.Line 311 uses
it as List<Int>which could throwClassCastExceptionif the actual type differs. Thecatchblock only handlesIndexOutOfBoundsException, leavingClassCastExceptionunhandled.🛡️ Suggested fix
fun getActualDisbursementDateInStringFormat(): String { try { return loanWithAssociations.timeline.actualDisbursementDate?.let { - DateHelper.getDateAsString(it as List<Int>) + (it as? List<*>)?.filterIsInstance<Int>()?.let { dateList -> + DateHelper.getDateAsString(dateList) + } } ?: "" - } catch (exception: IndexOutOfBoundsException) { + } catch (exception: Exception) { scope.launch { snackbarHostState.showSnackbar( message = message, ) } return "" } }
🤖 Fix all issues with AI agents
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryScreen.kt`:
- Around line 268-276: Handle the edge case in LoanAccountSummaryScreen where
dialogState == null and state.loanWithAssociations == null by adding a fallback
UI or guard: inside the branch that currently checks null -> {
state.loanWithAssociations?.let { ... } } update the logic in
LoanAccountSummaryScreen so that if state.loanWithAssociations is null you
render a sensible fallback (e.g., a Loading/Empty/Error composable or a message
with retry) instead of nothing, or alternatively ensure the parent state cannot
produce this combination; reference the dialogState and
state.loanWithAssociations checks and the LoanAccountSummaryContent call when
implementing the fallback.
🧹 Nitpick comments (2)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryScreen.kt (2)
197-197: Consider localizing the content description.The
contentDescription = "More options"is a hardcoded English string. For accessibility and i18n consistency, consider using a string resource.
741-742: MarkgetButtonTextasprivate.
getButtonTextis public while similar utility functions (getButtonActiveStatus,getInflateLoanSummaryValue) are markedprivate. Since this function is only used internally at line 553 within this file, make itprivatefor consistency and to enforce proper encapsulation of internal implementation details.
.../src/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryScreen.kt
Outdated
Show resolved
Hide resolved
|
@gurnoorpannu i see the ticket is tiled 'Open Loan Account Summary in full compose screen' , Are we supposed to get rid of the topbar here? this whole section i mean 
|
@itsPronay I had the same confusion earlier, so I asked Rajan sir. He told me the quality of code in this screen was not at par with other screens so I need to improve the code as it is in other screens . SO I took reference from other similar screens and made changes. |
.../src/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryScreen.kt
Outdated
Show resolved
Hide resolved
.../src/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryScreen.kt
Outdated
Show resolved
Hide resolved
.../src/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryScreen.kt
Outdated
Show resolved
Hide resolved
.../src/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryScreen.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
These types of functions should be in the viewModel according to the KMP structure that we are following.
By 'these' I meant anything logical/not ui related
...c/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryViewModel.kt
Outdated
Show resolved
Hide resolved
.../src/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryScreen.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 (2)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryScreen.kt (2)
585-592: 🛠️ Refactor suggestion | 🟠 MajorSame issue:
LoanSummaryDataTableshould not inject ViewModel viakoinViewModel().This composable also calls
koinViewModel(). Apply the same fix—accept the ViewModel as a parameter from the parent.♻️ Suggested fix
`@Composable` private fun LoanSummaryDataTable( loanSummary: LoansAccountSummaryEntity, inflateLoanSummary: Boolean, currencyCode: String?, decimalPlaces: Int?, - viewModel: LoanAccountSummaryViewModel = koinViewModel(), + viewModel: LoanAccountSummaryViewModel, ) {Then update the call site at line 522-528 to pass the viewModel.
574-580:⚠️ Potential issue | 🟡 MinorButton text color uses
backgroundinstead ofonPrimary.Using
MaterialTheme.colorScheme.backgroundfor text on aprimary-colored button may not provide sufficient contrast. UseonPrimarywhich is designed to contrast withprimary.🎨 Suggested fix
) { Text( - color = MaterialTheme.colorScheme.background, + color = MaterialTheme.colorScheme.onPrimary, text = buttonText, ) }
🤖 Fix all issues with AI agents
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryScreen.kt`:
- Around line 295-301: LoanAccountSummaryContent currently calls koinViewModel()
which can create or scope a different ViewModel instance; change its signature
to accept the existing LoanAccountSummaryViewModel from the parent
(LoanAccountSummaryScreen) instead of calling koinViewModel(), update all call
sites in LoanAccountSummaryScreen to pass that viewModel instance, and if only
specific data/formatting is needed consider passing those values or pure utility
functions rather than the entire ViewModel from LoanAccountSummaryViewModel.
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryViewModel.kt`:
- Around line 178-188: The function getActualDisbursementDateInStringFormat
currently unsafely casts actualDisbursementDate to List<Int>, which can throw
ClassCastException; change it to perform a safe type-check/cast before calling
com.mifos.core.common.utils.DateHelper.getDateAsString (e.g., verify
actualDisbursementDate is a List<*> and map/convert elements to Int or use
safeCast as? List<Int>), return "" for null/invalid types, and keep the existing
IndexOutOfBoundsException catch for bounds errors; reference the
getActualDisbursementDateInStringFormat method and DateHelper.getDateAsString
when applying the fix.
🧹 Nitpick comments (1)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryViewModel.kt (1)
148-176:formatCurrencyandformatAmountare identical—consolidate into one function.Both functions have identical implementations. Remove one and use a single formatting function throughout.
♻️ Suggested consolidation
- fun formatCurrency( - amount: Double?, - currencyCode: String?, - decimalPlaces: Int?, - ): String { - if (amount == null) return "" - if (currencyCode.isNullOrBlank()) return amount.toString() - - return com.mifos.core.common.utils.CurrencyFormatter.format( - balance = amount, - currencyCode = currencyCode, - maximumFractionDigits = decimalPlaces, - ) - } - - fun formatAmount( + fun formatAmount( amount: Double?, currencyCode: String?, decimalPlaces: Int?, ): String { if (amount == null) return "" if (currencyCode.isNullOrBlank()) return amount.toString() return com.mifos.core.common.utils.CurrencyFormatter.format( balance = amount, currencyCode = currencyCode, maximumFractionDigits = decimalPlaces, ) }Then update
LoanAccountSummaryContentto useformatAmountinstead offormatCurrency.
.../src/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryScreen.kt
Show resolved
Hide resolved
...c/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryViewModel.kt
Outdated
Show resolved
Hide resolved
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/loanAccountSummary/LoanAccountSummaryScreen.kt`:
- Around line 527-541: The buttonText is remembered only by
loanWithAssociations.status so it won't update when locale changes; update the
remember key to include the localized strings or remove remember entirely:
change the block that computes buttonText (the remember(...) using
loanWithAssociations.status and the when(viewModel.getPrimaryAction(...))) to
either not use remember or use remember(loanWithAssociations.status,
makeRepaymentText, approveLoanText, disburseLoanText, overpaidText, closedText)
so stringResource changes trigger recomposition.
- Around line 192-196: The contentDescription strings are hardcoded and need to
be moved to localized string resources; update the IconButton/Icon usage
(IconButton onClick block and Icon with imageVector = MifosIcons.MoreVert) to
use a string resource (e.g., stringResource(R.string.more_options)) instead of
the literal "More options", and replace all loan status labels ("Active",
"Pending Approval", "Waiting for Disbursal", "Closed") where they are emitted
(e.g., in the LoanAccountSummaryScreen UI logic that renders status text) to use
stringResource references (R.string.loan_status_active,
R.string.loan_status_pending_approval,
R.string.loan_status_waiting_for_disbursal, R.string.loan_status_closed); add
corresponding entries to the platform/common string resources so the UI reads
localized values at runtime.
.../src/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryScreen.kt
Outdated
Show resolved
Hide resolved
.../src/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryScreen.kt
Outdated
Show resolved
Hide resolved
|
@itsPronay Temporarily commented out the preview as it now requires a |
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/loanAccountSummary/LoanAccountSummaryScreen.kt`:
- Around line 468-472: The button label in LoanAccountSummaryScreen currently
sets Text color to MaterialTheme.colorScheme.background which is incorrect for
content on a primary-colored Button; change the color to
MaterialTheme.colorScheme.onPrimary for the Text that displays buttonText (the
Text inside the Button composable) so the label uses the semantic onPrimary
color and ensures proper contrast across themes.
🧹 Nitpick comments (2)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryViewModel.kt (1)
137-148: Unnecessary safe calls on non-null parameterloan.
loanis typed asLoanWithAssociationsEntity(non-null), but lines 139, 142, 146, and 147 useloan?.which is redundant. This can mislead readers into thinkingloancould be null.♻️ Remove unnecessary safe calls
private fun fillLoanSummary(loan: LoanWithAssociationsEntity) { val actualDisbursementDate = formatActualDisbursementDate( - loan?.timeline?.actualDisbursementDate, + loan.timeline?.actualDisbursementDate, ) - val shouldInflateLoanSummary = loan?.status?.shouldInflateLoanSummary() - // dataTable should be empty if [inflateLoanSummary] is false - val summary = if (shouldInflateLoanSummary == true) loan.summary else null + val shouldInflateLoanSummary = loan.status?.shouldInflateLoanSummary() ?: false + val summary = if (shouldInflateLoanSummary) loan.summary else null - val currencyCode = loan?.currency?.code - val decimalPlaces = loan?.currency?.decimalPlaces + val currencyCode = loan.currency?.code + val decimalPlaces = loan.currency?.decimalPlacesfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryScreen.kt (1)
181-186: Unnecessary lambda wrapper foronAction.Line 176 already uses
viewModel::trySendActionas theonActioncallback. Theremember+ lambda wrapper at lines 183–185 achieves the same thing with extra allocation. Consider reusing the same reference.♻️ Simplify onAction for dialog
LoadAccountSummaryDialog( state.dialogState, - onAction = remember(viewModel) { - { viewModel.trySendAction(it) } - }, + onAction = viewModel::trySendAction, )
.../src/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryScreen.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/loanAccountSummary/LoanAccountSummaryScreen.kt`:
- Around line 641-658: Rename the composable function LoadAccountSummaryDialog
to LoanAccountSummaryDialog and update all call sites that invoke
LoadAccountSummaryDialog (e.g., the existing call in the same file) to use the
new name; ensure the function signature (parameters dialogState:
LoanAccountSummaryState.DialogState? and onAction: (LoanAccountSummaryAction) ->
Unit) and its internal when branches remain unchanged so references to
LoanAccountSummaryState.DialogState.Error and
LoanAccountSummaryState.DialogState.Loading continue to compile.
🧹 Nitpick comments (1)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryViewModel.kt (1)
124-131: Generic error message discards the actual error.
DataState.Errorlikely carries an error message or throwable, but the handler always substitutes a generic string. Consider extracting the real message (with the generic string as fallback) to aid debugging and provide more useful feedback.Suggested approach
is DataState.Error -> { - val errorMessage = getString(Res.string.feature_loan_unknown_error_occured) + val errorMessage = dataState.message + ?: getString(Res.string.feature_loan_unknown_error_occured) mutableStateFlow.update {
| private fun LoadAccountSummaryDialog( | ||
| dialogState: LoanAccountSummaryState.DialogState?, | ||
| onAction: (LoanAccountSummaryAction) -> Unit, | ||
| ) { | ||
| when (dialogState) { | ||
| is LoanAccountSummaryState.DialogState.Error -> { | ||
| MifosSweetError( | ||
| message = dialogState.message, | ||
| onclick = { onAction(LoanAccountSummaryAction.OnRetry) }, | ||
| ) | ||
| } | ||
|
|
||
| status.overpaid == true -> { | ||
| stringResource(Res.string.feature_loan_overpaid) | ||
| LoanAccountSummaryState.DialogState.Loading -> { | ||
| MifosProgressIndicator() | ||
| } | ||
|
|
||
| else -> { | ||
| stringResource(Res.string.feature_loan_closed) | ||
| } | ||
| null -> Unit | ||
| } |
There was a problem hiding this comment.
Typo in composable name: LoadAccountSummaryDialog → LoanAccountSummaryDialog.
The function is named LoadAccountSummaryDialog but should be LoanAccountSummaryDialog to match the feature naming convention used everywhere else in this file.
Suggested fix
-private fun LoadAccountSummaryDialog(
+private fun LoanAccountSummaryDialog(And update the call site at Line 181:
- LoadAccountSummaryDialog(
+ LoanAccountSummaryDialog(🤖 Prompt for AI Agents
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryScreen.kt`
around lines 641 - 658, Rename the composable function LoadAccountSummaryDialog
to LoanAccountSummaryDialog and update all call sites that invoke
LoadAccountSummaryDialog (e.g., the existing call in the same file) to use the
new name; ensure the function signature (parameters dialogState:
LoanAccountSummaryState.DialogState? and onAction: (LoanAccountSummaryAction) ->
Unit) and its internal when branches remain unchanged so references to
LoanAccountSummaryState.DialogState.Error and
LoanAccountSummaryState.DialogState.Loading continue to compile.
| MifosMenuDropDownItem( | ||
| option = Constants.DATA_TABLE_LOAN_NAME, | ||
| onClick = { | ||
| onAction(LoanAccountSummaryAction.ToggleDropdown) | ||
| onAction(LoanAccountSummaryAction.OnMoreInfoClick) | ||
| }, | ||
| ) | ||
| MifosMenuDropDownItem( | ||
| option = stringResource(Res.string.feature_loan_transactions), | ||
| onClick = { | ||
| onAction(LoanAccountSummaryAction.ToggleDropdown) | ||
| onAction(LoanAccountSummaryAction.OnTransactionsClick) | ||
| }, | ||
| ) | ||
| MifosMenuDropDownItem( | ||
| option = stringResource(Res.string.feature_loan_repayment_schedule), | ||
| onClick = { | ||
| onAction(LoanAccountSummaryAction.ToggleDropdown) | ||
| onAction(LoanAccountSummaryAction.OnRepaymentScheduleClick) | ||
| }, | ||
| ) | ||
| MifosMenuDropDownItem( | ||
| option = stringResource(Res.string.feature_loan_documents), | ||
| onClick = { |
There was a problem hiding this comment.
@gurnoorpannu instead of calling onAction(LoanAccountSummaryAction.ToggleDropdown) again and again, create a enum class called LoanSummaryDropDownAction
enum class LoanSummaryDropDownAction{
OnMoreInfoClick,
OnTransactionsClick,
OnRepaymentScheduleClick ,
OnDocumentsClick ,
OnChargesClick ,
}when dialog is clicked, pass the action like this
MifosMenuDropDownItem(
option = stringResource(Res.string.feature_loan_repayment_schedule),
onClick = {
onAction(LoanAccountSummaryAction.DropdownAction(LoanSummaryDropDownAction.OnRepaymentScheduleClick))
},
)and then in viewmodel handle it like this
mutableStateFlow.update { showdropdown = false }
when( action ) {
LoanSummaryDropDownAction.OnMoreInfoClick->
LoanSummaryDropDownAction.OnChargesClick->
}it reduces redundancy and makes it cleaner
…mmary-screen' into refactor/improve-loan-account-summary-screen
| mutableStateFlow.value.loanWithAssociations?.let { loan -> | ||
| sendEvent(LoanAccountSummaryEvent.NavigateToMakeRepayment(loan)) | ||
| } |
There was a problem hiding this comment.
you can access loan directly from the state. do state.loanWithAssociations like this instead
sendEvent(LoanAccountSummaryEvent.NavigateToMakeRepayment(state.loanWithAssociations))
| @JvmInline | ||
| value class DropdownAction(val action: LoanSummaryDropDownAction) : LoanAccountSummaryAction |
There was a problem hiding this comment.
why including jvminLine ?
Just make it data class instead
| CLOSED, | ||
| } | ||
|
|
||
| enum class LoanSummaryDropDownAction { |
...c/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryViewModel.kt
Outdated
Show resolved
Hide resolved
- Convert DropdownAction from value class to data class - Simplify state access and remove unnecessary braces
| mutableStateFlow.value.loanWithAssociations?.let { loan -> | ||
| sendEvent( | ||
| LoanAccountSummaryEvent.NavigateToApproveLoan( | ||
| loanAccountNumber, | ||
| loan, | ||
| ), | ||
| ) |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| CLOSED, | ||
| } | ||
|
|
||
| enum class LoanSummaryDropDownAction { |
…mmary-screen' into refactor/improve-loan-account-summary-screen
.../src/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryScreen.kt
Show resolved
Hide resolved
...c/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryViewModel.kt
Show resolved
Hide resolved
|
| val statusDescription = when { | ||
| loanWithAssociations.status.active == true -> "Active" | ||
| loanWithAssociations.status.pendingApproval == true -> "Pending Approval" | ||
| loanWithAssociations.status.waitingForDisbursal == true -> "Waiting for Disbursal" |
4 participants
Fixes - MIFOSAC-#635
All Changes implemented:-
Adopted event-based navigation and UI flow
Replaced direct callback-based navigation with a proper LoanAccountSummaryEvent system
Navigation is now handled via EventsEffect, consistent with other feature screens
UI now communicates with ViewModel using a single onAction callback
Improved state handling
Replaced isLoading and error: String? with a structured DialogState sealed interface
Follows the same pattern used in ClientProfileState and SettingsState
Makes loading and error handling more predictable and type-safe
Cleaned up ViewModel responsibilities
Marked LoanAccountSummaryViewModel as internal (consistent with other ViewModels)
Removed public helper methods that were being exposed unnecessarily
Business logic helpers were moved out of the ViewModel or made private
Removed business logic from composables
Functions like date formatting and summary expansion logic were removed from UI lambdas
UI now receives clean, prepared state instead of calling ViewModel helpers directly
This improves testability and separation of concerns
Fixed composable structure and naming
Removed duplicate composables with the same name
Introduced a clear hierarchy:
Screen → Scaffold → Content → Dialog
Matches the structure used by ClientProfileScreen and SearchScreen
Improved action definitions
Removed unused parameters from actions (e.g. OnLoanIdCopied)
Actions now only contain data that is actually used
Minor UI improvements
Added proper top bar handling consistent with other full-screen Compose screens
Ensured the Loan Account Summary behaves as a true full-screen destination
| Before :
2026-01-29.14.27.53.mp4
After:
vid.mp4
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
User Experience
Refactor
Breaking Changes