Documentation for features/loan-account #2984
Conversation
WalkthroughThis pull request extends the loan account feature module with navigation infrastructure, expanded state management, and new UI components. It introduces navigation routes and helpers for loan accounts, details, summaries, and repayment schedules, while significantly expanding ViewModel state structures and adding corresponding composables with enhanced documentation. Changes
Sequence DiagramsequenceDiagram
participant User
participant Navigation as Navigation Layer
participant LoanGraph as Loan Nav Graph
participant LoanAcct as Loan Account Screen
participant LoanDetails as Loan Details Screen
participant LoanVM as Loan ViewModel
participant SummaryVM as Summary ViewModel
participant Repo as Repository
User->>Navigation: Navigate to Loan Graph
Navigation->>LoanGraph: loanNavGraph() with callbacks
LoanGraph->>LoanAcct: loanAccountDestination()
User->>LoanAcct: View account list / Click account
LoanAcct->>LoanVM: OnAccountClicked(accountId)
LoanVM->>Repo: Fetch account details
Repo-->>LoanVM: DataState<AccountDetails>
LoanVM->>LoanVM: Update state with account data
LoanVM->>Navigation: Navigate to Loan Details
Navigation->>LoanGraph: navigateToLoanAccountDetailsScreen()
LoanGraph->>LoanDetails: loanAccountDetailsDestination()
LoanDetails->>SummaryVM: Load summary data
SummaryVM->>Repo: Fetch with network monitor
Repo-->>SummaryVM: LoanWithAssociations
SummaryVM->>SummaryVM: Populate displayItems, transactionList
SummaryVM-->>LoanDetails: Updated state
User->>LoanDetails: Click action (Pay, Summary, Schedule, QR, etc.)
LoanDetails->>Navigation: Navigate to respective screen
Note over LoanVM,SummaryVM: Network status monitored<br/>Error dialogs handled via DialogState
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
feature/loan-account/src/commonMain/kotlin/org/mifos/mobile/feature/loanaccount/loanAccountRepaymentSchedule/RepaymentScheduleViewModel.kt (1)
192-192: Remove debug println statement.This debug print statement should be removed before merging to production.
Apply this diff to remove the debug statement:
is DataState.Success -> { val result = dataState.data - println("from success ${ result?.repaymentSchedule?.periods}") val currencyCode = result?.currency?.codefeature/loan-account/src/commonMain/kotlin/org/mifos/mobile/feature/loanaccount/loanAccount/LoanAccountViewModel.kt (1)
140-144: Correct the function documentation.The documentation mentions "
fetchClient" and "fetchLonPurpose" functions that don't exist in the code. Line 150 shows the actual implementation callsLoadAccounts.Apply this diff to fix the documentation:
/** * Retries the data fetching process. If the network is unavailable, it shows - * a network error dialog. Otherwise, it triggers the `loadAccounts` `fetchClient`, - * `fetchLonPurpose` function. + * a network error dialog. Otherwise, it triggers the `loadAccounts` action. */feature/loan-account/src/commonMain/kotlin/org/mifos/mobile/feature/loanaccount/loanAccountDetails/LoanAccountDetailsScreen.kt (1)
268-301: Correct the documentation - this is for loan accounts, not savings accounts.The documentation states "savings account" but this composable is clearly for loan accounts based on the parameter type
Set<LoanActionItems>and its location in the loan-account feature.Apply this diff to fix the documentation:
/** - * A composable that displays the actions that can be performed on a savings account. + * A composable that displays the actions that can be performed on a loan account. * * @param visibleActions A set of [LoanActionItems] that should be visible. * @param onActionClick A callback that is invoked when an action is clicked. */feature/loan-account/src/commonMain/kotlin/org/mifos/mobile/feature/loanaccount/loanAccountDetails/LoanAccountDetailsViewModel.kt (2)
180-196: Fix invalid nested return and guard against “null” strings in QR payload.The function uses
return if (...) { return ... } else { ... }, which is invalid and can fail compilation. Also,toString()on nullable fields yields the literal "null".
- Remove the inner
return.- Ensure officeName, clientName, and accountNumber are all non‑blank before building the QR string.
- Use
orEmpty()to avoid "null" literals.Apply:
fun getQrString(): String { - val officeName = userPreferencesRepository.userInfo.value.officeName - return if (officeName.isNotEmpty()) { - return getAccountDetailsInString( - clientName = state.clientName.toString(), - accountNumber = state.accountNumber.toString(), - accountType = AccountType( - id = 1, - code = "accountType.loan", - value = "Loan Account", - ), - officeName = officeName, - ) - } else { - "" - } + val officeName = userPreferencesRepository.userInfo.value.officeName + val client = state.clientName.orEmpty() + val accNo = state.accountNumber.orEmpty() + if (officeName.isEmpty() || client.isBlank() || accNo.isBlank()) return "" + return getAccountDetailsInString( + clientName = client, + accountNumber = accNo, + accountType = AccountType( + id = 1, + code = "accountType.loan", + value = "Loan Account", + ), + officeName = officeName, + ) }
262-276: Preserve route accountId when response is null; updateisEmptyflag.Overwriting with
-1Lon null loses the known id. Also,isEmptystays true forever.Apply:
- it.copy( - accountId = loan?.id?.toLong() ?: -1L, + it.copy( + accountId = loan?.id?.toLong() ?: it.accountId, accountStatus = loan?.status?.loanStatus, accountNumber = loan?.accountNo, clientName = loan?.clientName, product = loan?.loanProductName, submissionDate = DateHelper.getDateAsString(loan?.timeline?.submittedOnDate ?: emptyList()), displayItems = displayItems, transactionList = transactions, totalOutStandingBalance = loan?.summary?.totalOutstanding, - uiState = ScreenUiState.Success, + isEmpty = loan == null, + uiState = ScreenUiState.Success, )Optional: use
updateState { ... }for consistency with the helper.
🧹 Nitpick comments (12)
feature/loan-account/src/commonMain/kotlin/org/mifos/mobile/feature/loanaccount/component/AccountSummaryCard.kt (2)
55-63: Document the default parameter value and special behaviors.The documentation is clear but could be more complete:
- The
titleparameter has a default value of""(line 68) which isn't mentioned in the KDoc.- The special color treatment for status labels (lines 130-134) is an undocumented behavior that might be useful for users of this component to know.
Consider applying this diff to improve the documentation:
/** * A composable that displays a summary of account details in a card format. * The card is expandable to show more details. * * @param keyValuePairs A map of key-value pairs to display as account details. The key is a * [StringResource] for the label, and the value is the string to display. * @param modifier The modifier to be applied to the component. - * @param title The title of the card. + * @param title The title of the card. Defaults to an empty string. */
144-163: Consider using more realistic test data in the preview.The preview uses mismatched test data where labels don't align with their values (e.g., account number label shows "John Miller", product type shows a date). Using more semantically accurate test data would make the preview more helpful for developers.
For example:
AccountSummaryCard( keyValuePairs = mapOf( Res.string.feature_loan_account_number_label to "000123456", Res.string.feature_loan_product_type_label to "Personal Loan", Res.string.feature_loan_scheme_label to "Standard Scheme", Res.string.feature_loan_account_status_label to "Active", ), )feature/loan-account/src/commonMain/kotlin/org/mifos/mobile/feature/loanaccount/loanAccountRepaymentSchedule/RepaymentScheduleViewModel.kt (1)
291-336: Consider using consistent types for transferSuccessDestination.
RepaymentScheduleEvent.PayInstallmentusesStringfortransferSuccessDestination(line 296), whileRepaymentScheduleAction.OnPayInstallmentuses theTransferSuccessDestinationenum (line 335). This requires a conversion via.nameat line 97, which adds complexity and may confuse future maintainers about why the types differ.Consider using the same type in both interfaces for consistency, or document why they differ if there's a specific architectural reason.
feature/loan-account/src/commonMain/kotlin/org/mifos/mobile/feature/loanaccount/loanAccountRepaymentSchedule/RepaymentScheduleScreen.kt (1)
163-167: Pass null instead of empty string for consistency.The
RepaymentScheduleListsignature now acceptsString?forcurrencyCode(line 200), but line 165 still falls back to an empty string when the code is null. For consistency with the nullable signature, consider passingnullinstead.Apply this diff:
RepaymentScheduleList( periods = state.getPeriods, - currencyCode = state.loanWithAssociations?.currency?.code ?: "", + currencyCode = state.loanWithAssociations?.currency?.code, maxDigits = state.loanWithAssociations?.currency?.decimalPlaces?.toInt(), onPayClick = { period ->feature/loan-account/src/commonMain/kotlin/org/mifos/mobile/feature/loanaccount/loanAccountDetails/LoanAccountDetailsScreen.kt (1)
218-266: Consider simplifying the parameter signature.The
detailsparameter combines a nullable type with a default value (List<LabelValueItem>? = emptyList()), which is redundant. Consider either making it non-nullable with a default or nullable without a default for clearer semantics.Option 1 (preferred): Non-nullable with default
internal fun AccountDetailsGrid( label: String? = null, - details: List<LabelValueItem>? = emptyList(), + details: List<LabelValueItem> = emptyList(), ) { Column( modifier = Modifier .fillMaxWidth(), verticalArrangement = Arrangement.spacedBy(DesignToken.spacing.largeIncreased), ) { if (label != null) { Text( text = label, style = MifosTypography.labelLargeEmphasized, color = MaterialTheme.colorScheme.onSurface, ) } - if (details != null) { + if (details.isNotEmpty()) { FlowRow(Option 2: Nullable without default
internal fun AccountDetailsGrid( label: String? = null, - details: List<LabelValueItem>? = emptyList(), + details: List<LabelValueItem>?, ) {feature/loan-account/src/commonMain/kotlin/org/mifos/mobile/feature/loanaccount/loanAccountSummary/AccountSummaryViewModel.kt (2)
322-338: Consider tracking these incomplete features.Two TODO comments indicate missing fields in the data model/API:
- Lines 322-338: Interest and principal outstanding details
- Lines 376-380: Linked account information
These are properly documented with TODO comments, but consider creating tracking issues to ensure they're addressed.
Would you like me to open issues to track these incomplete features?
Also applies to: 376-380
416-422: Consider simplifying nullable list types.The properties are declared as
List<LabelValueItem>?with a default ofemptyList(), making them nullable types with non-null defaults. This pattern can be confusing since the values will never actually be null.Consider using non-nullable types instead:
- val accountDetails: List<LabelValueItem>? = emptyList(), - val payOffDetails: List<LabelValueItem>? = emptyList(), - val chargeDetails: List<LabelValueItem>? = emptyList(), - val waiversDetails: List<LabelValueItem>? = emptyList(), - val paidOffDetails: List<LabelValueItem>? = emptyList(), - val outStandingDetails: List<LabelValueItem>? = emptyList(), - val installmentDetails: List<LabelValueItem>? = emptyList(), + val accountDetails: List<LabelValueItem> = emptyList(), + val payOffDetails: List<LabelValueItem> = emptyList(), + val chargeDetails: List<LabelValueItem> = emptyList(), + val waiversDetails: List<LabelValueItem> = emptyList(), + val paidOffDetails: List<LabelValueItem> = emptyList(), + val outStandingDetails: List<LabelValueItem> = emptyList(), + val installmentDetails: List<LabelValueItem> = emptyList(),This eliminates unnecessary null checks in consuming code.
feature/loan-account/src/commonMain/kotlin/org/mifos/mobile/feature/loanaccount/loanAccountDetails/LoanAccountDetailsViewModel.kt (5)
186-191: Avoid hard‑coded AccountType literals; prefer shared constants or localized values.Using
id = 1,code = "accountType.loan", and"Loan Account"in-line risks drift and i18n gaps. Source from a shared constant/enum or resources.
249-260: Format installment amount with currency instead of rawtoString().Surface uses formatted outstanding; keep consistency for
txn.amount.Apply:
- LabelValueItem( - Res.string.feature_loan_installment_amount_label, - txn.amount.toString(), - ), + LabelValueItem( + Res.string.feature_loan_installment_amount_label, + CurrencyFormatter.format( + txn.amount, + loan?.currency?.code, + loan?.currency?.decimalPlaces?.toInt() + ), + ),If
txn.amountis not a numeric type supported byCurrencyFormatter.format, adjust accordingly. Please confirm the signature.
300-316: Tighten state nullability; consider renamingtotalOutStandingBalance.
transactionListanduiStatecan be non‑nullable with emptyList()/Loading defaults.- Spelling:
totalOutstandingBalanceimproves readability (rename if API surface permits).Apply:
- val transactionList: List<LabelValueItem>? = emptyList(), + val transactionList: List<LabelValueItem> = emptyList(), ... - val uiState: ScreenUiState? = ScreenUiState.Loading, + val uiState: ScreenUiState = ScreenUiState.Loading,Renaming the balance field is optional and may be deferred if it touches many call sites.
55-59: RenameloanAccountRepositoryImptoloanRepositoryfor clarity.Variable name implies a concrete implementation while the type is the abstraction.
175-179: KDoc for QR helper is good; consider noting other preconditions.State that clientName/accountNumber must be non‑blank (in addition to officeName), matching the guard in code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
feature/loan-account/src/commonMain/kotlin/org/mifos/mobile/feature/loanaccount/component/AccountSummaryCard.kt(1 hunks)feature/loan-account/src/commonMain/kotlin/org/mifos/mobile/feature/loanaccount/component/LoanActionItems.kt(2 hunks)feature/loan-account/src/commonMain/kotlin/org/mifos/mobile/feature/loanaccount/component/RepaymentPeriodCard.kt(1 hunks)feature/loan-account/src/commonMain/kotlin/org/mifos/mobile/feature/loanaccount/loanAccount/LoanAccountNavigation.kt(1 hunks)feature/loan-account/src/commonMain/kotlin/org/mifos/mobile/feature/loanaccount/loanAccount/LoanAccountScreen.kt(3 hunks)feature/loan-account/src/commonMain/kotlin/org/mifos/mobile/feature/loanaccount/loanAccount/LoanAccountViewModel.kt(4 hunks)feature/loan-account/src/commonMain/kotlin/org/mifos/mobile/feature/loanaccount/loanAccountDetails/LoanAccountDetailsNavigation.kt(1 hunks)feature/loan-account/src/commonMain/kotlin/org/mifos/mobile/feature/loanaccount/loanAccountDetails/LoanAccountDetailsScreen.kt(6 hunks)feature/loan-account/src/commonMain/kotlin/org/mifos/mobile/feature/loanaccount/loanAccountDetails/LoanAccountDetailsViewModel.kt(5 hunks)feature/loan-account/src/commonMain/kotlin/org/mifos/mobile/feature/loanaccount/loanAccountRepaymentSchedule/RepaymentScheduleRoute.kt(1 hunks)feature/loan-account/src/commonMain/kotlin/org/mifos/mobile/feature/loanaccount/loanAccountRepaymentSchedule/RepaymentScheduleScreen.kt(4 hunks)feature/loan-account/src/commonMain/kotlin/org/mifos/mobile/feature/loanaccount/loanAccountRepaymentSchedule/RepaymentScheduleViewModel.kt(6 hunks)feature/loan-account/src/commonMain/kotlin/org/mifos/mobile/feature/loanaccount/loanAccountSummary/AccountSummaryRoute.kt(1 hunks)feature/loan-account/src/commonMain/kotlin/org/mifos/mobile/feature/loanaccount/loanAccountSummary/AccountSummaryScreen.kt(3 hunks)feature/loan-account/src/commonMain/kotlin/org/mifos/mobile/feature/loanaccount/loanAccountSummary/AccountSummaryViewModel.kt(3 hunks)feature/loan-account/src/commonMain/kotlin/org/mifos/mobile/feature/loanaccount/navigation/LoanNavigation.kt(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
feature/loan-account/src/commonMain/kotlin/org/mifos/mobile/feature/loanaccount/loanAccountRepaymentSchedule/RepaymentScheduleRoute.kt (1)
feature/loan-account/src/commonMain/kotlin/org/mifos/mobile/feature/loanaccount/loanAccountRepaymentSchedule/RepaymentScheduleScreen.kt (1)
RepaymentScheduleScreen(58-97)
🔇 Additional comments (19)
feature/loan-account/src/commonMain/kotlin/org/mifos/mobile/feature/loanaccount/loanAccountSummary/AccountSummaryScreen.kt (1)
51-57: Solid documentation additions across all composables.The KDoc blocks are well-written, clear, and follow Kotlin conventions consistently. All parameters are documented with appropriate type references, and the descriptions concisely convey the purpose and behavior of each composable. The documentation provides value for future maintainers and IDE consumers.
Also applies to: 86-92, 111-118
feature/loan-account/src/commonMain/kotlin/org/mifos/mobile/feature/loanaccount/component/RepaymentPeriodCard.kt (1)
52-62: LGTM! Clear and accurate documentation.The KDoc effectively documents the composable's purpose, parameters, and behavior. The documentation accurately reflects the implementation and will help developers understand how to use this component.
feature/loan-account/src/commonMain/kotlin/org/mifos/mobile/feature/loanaccount/loanAccount/LoanAccountViewModel.kt (3)
33-40: LGTM! Clear and comprehensive class documentation.The documentation accurately describes the ViewModel's responsibilities and parameters.
56-306: Excellent documentation additions.The comprehensive documentation for functions, state management, and data flow significantly improves code maintainability and readability.
309-479: Well-structured documentation for state, actions, and events.The detailed KDoc comments for
LoanAccountsState,LoanAccountsAction, andLoanAccountsEventprovide clear understanding of the data structures and their purposes.feature/loan-account/src/commonMain/kotlin/org/mifos/mobile/feature/loanaccount/loanAccountDetails/LoanAccountDetailsScreen.kt (2)
61-146: LGTM! Clear documentation for screen composables.The documentation accurately describes the purpose and parameters for both the main screen and content composables.
303-337: LGTM! Clear documentation for dialogs and data class.The documentation accurately describes the purpose and parameters for the dialog composable and the
LabelValueItemdata class.feature/loan-account/src/commonMain/kotlin/org/mifos/mobile/feature/loanaccount/component/LoanActionItems.kt (4)
32-40: Excellent documentation for the sealed class.The KDoc clearly describes the purpose of LoanActionItems and documents all properties with appropriate detail.
47-49: Well-documented action items.Each action now has clear documentation explaining its purpose, making the codebase more maintainable.
Also applies to: 57-59, 67-69, 77-79, 87-89, 97-99
108-118: Complete and well-documented action list.The
loanAccountActionslist correctly includes all defined actions, uses immutable collections appropriate for Compose, and has clear documentation.
97-105: No issues found — QrCode action is properly integrated.Verification confirms that
Constants.QR_CODEis defined incore/common/src/commonMain/kotlin/org/mifos/mobile/core/common/Constants.ktand correctly integrated throughout the codebase. The navigation routing is properly handled inLoanAccountDetailsScreen.kt(lines 116–118), the action follows the same pattern as the correspondingSavingsActionItems.QrCode, and the navigation callback is wired correctly in the graph. All resource strings, icons, and routes are consistent.feature/loan-account/src/commonMain/kotlin/org/mifos/mobile/feature/loanaccount/loanAccountSummary/AccountSummaryViewModel.kt (4)
56-75: LGTM! Clear and comprehensive documentation.The ViewModel documentation clearly describes its purpose and documents all constructor parameters following KDoc conventions.
372-375: Verify the auto-debit logic mapping.The auto-debit status is determined by
if (loan?.npa == true) "Off" else "On". Since NPA typically indicates "Non-Performing Asset" (a loan in default), please verify that this correctly maps the NPA flag to the auto-debit display value according to business requirements.
441-488: LGTM! Well-structured and documented action/event definitions.The sealed interfaces for events and actions are clearly documented with comprehensive KDoc comments. The separation of internal actions is a good design pattern.
349-367: FilterfirstPeriodby unpaid status for accurate "next payment" display.The code inconsistently handles period selection. Line 353 explicitly identifies unpaid periods (
it.complete == false), but line 349 usesfirstOrNull()without checking completion status for "next payment" and "regular payment" display.If the first period is completed or a disbursement entry, these labels will show incorrect information. Align with the established pattern elsewhere in the codebase:
val nextUnpaidPeriod = loan?.repaymentSchedule ?.periods ?.firstOrNull { it.complete == false }Then use
nextUnpaidPeriodinstead offirstPeriodfor lines 359 and 366.feature/loan-account/src/commonMain/kotlin/org/mifos/mobile/feature/loanaccount/loanAccountDetails/LoanAccountDetailsViewModel.kt (4)
46-53: KDoc additions for constructor and responsibilities look good.Clearer docs improve maintainability.
280-297: State KDoc expansion reads well.Covers all fields and their intent succinctly.
382-397: Event KDoc improvements LGTM.Clear navigation semantics.
402-441: Action KDoc improvements LGTM.Good separation of UI vs. Internal actions.
...ount/src/commonMain/kotlin/org/mifos/mobile/feature/loanaccount/component/LoanActionItems.kt
Show resolved
Hide resolved
2 participants
Fixes - Jira-#Issue_Number
Didn't create a Jira ticket, click here to create new.
Please Add Screenshots If there are any UI changes.
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
New Features
Improvements