feat: New loan amount transfer screen#2613
Merged
biplab1 merged 16 commits intoopenMF:developmentfrom Mar 11, 2026
Merged
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:
📝 WalkthroughWalkthroughAdds an account Amount Transfer feature (models, network, repository, DI, navigation, ViewModel, Compose UI, and strings) and a Create New Client navigation route with NavGraphBuilder/NavController helpers. Changes
Sequence DiagramsequenceDiagram
participant User
participant LoanSummary as "Loan Account\nSummary Screen"
participant LoanVM as "LoanAccount\nViewModel"
participant Nav as "NavController"
participant TransferScreen as "Amount Transfer\nScreen/Route"
participant TransferVM as "AmountTransfer\nViewModel"
User->>LoanSummary: tap Overpaid / Transfer action
LoanSummary->>LoanVM: emit NavigateToLoanTransfer(loanId)
LoanVM->>Nav: navigateToTransferScreen(loanId)
Nav->>TransferScreen: open AmountTransferRoute(loanId)
TransferScreen->>TransferVM: init / fetchTemplate()
TransferVM->>TransferScreen: dialogState = Loading → null
User->>TransferScreen: fill form & click Transfer
TransferScreen->>TransferVM: OnTransferClicked
TransferVM->>TransferVM: validate inputs
alt valid
TransferVM->>Nav: submitAccountTransfer(...) and navigate back
else invalid
TransferVM->>TransferScreen: emit field errors
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
… to Account Transfer screen
itsPronay
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (2)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/amountTransfer/AmountTransferScreenRoute.kt (1)
17-24: Consider renamingidtoloanIdfor clarity and consistency.Other routes in this module use descriptive parameter names (e.g.,
LoanAccountSummaryScreenRoute.loanAccountNumber). Using justidis ambiguous—it could refer to any entity. Also,AmountTransferRouteis aclasswhile similar routes (e.g.,LoanAccountSummaryScreenRoute) usedata class; usingdata classgives youequals/hashCode/toStringfor free.Suggested change
`@Serializable` -class AmountTransferRoute( - val id: Int, +data class AmountTransferRoute( + val loanId: Int, ) -fun NavController.navigateToTransferScreen(id: Int) { - navigate(AmountTransferRoute(id)) +fun NavController.navigateToTransferScreen(loanId: Int) { + navigate(AmountTransferRoute(loanId)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/amountTransfer/AmountTransferScreenRoute.kt` around lines 17 - 24, Rename the ambiguous parameter and make the route a data class: change AmountTransferRoute to a data class with a descriptive parameter name loanId instead of id, and update the navigateToTransferScreen function signature and body to accept loanId and construct AmountTransferRoute(loanId); also update any call sites that pass or read the old id to use loanId and rely on the generated equals/hashCode/toString from the data class.feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/amountTransfer/AmountTransferViewModel.kt (1)
95-124: Validation does not clear errors on re-validation of the other field.Consider a scenario: the user enters an invalid amount and an empty description → both errors are set. They then fix the amount and click transfer. Lines 107–108 clear
amountError, but if the description is still empty,descriptionErroris set again (correct). However, thestatecheck on Line 121 reads frommutableStateFlow.valueafter the individualupdatecalls. While this works for the current two-field case, a cleaner approach would be to do a single atomic state update for all validation results.Also, the amount validation at Line 101 calls
toDouble()on a string that already passedtoDoubleOrNull() != null, which is safe but redundant — you could reuse the result.Suggested refactor (single atomic update)
private fun validateFields() { - if (state.amount.toDoubleOrNull() == null) { - mutableStateFlow.update { - it.copy(amountError = Res.string.feature_loan_invalid_amount) - } - } else if (state.amount.toDouble() <= 0.00) { - mutableStateFlow.update { - it.copy( - amountError = Res.string.feature_loan_transfer_amount_can_not_be_zero, - ) - } - } else { - mutableStateFlow.update { it.copy(amountError = null) } - } - - if (state.description.trim().isEmpty()) { - mutableStateFlow.update { - it.copy( - descriptionError = Res.string.feature_loan_description_can_not_be_empty, - ) - } - } else { - mutableStateFlow.update { it.copy(descriptionError = null) } - } - - if (state.amountError == null && state.descriptionError == null) { - onTransferClicked() - } + val parsedAmount = state.amount.toDoubleOrNull() + val amountError = when { + parsedAmount == null -> Res.string.feature_loan_invalid_amount + parsedAmount <= 0.0 -> Res.string.feature_loan_transfer_amount_can_not_be_zero + else -> null + } + val descriptionError = if (state.description.trim().isEmpty()) { + Res.string.feature_loan_description_can_not_be_empty + } else null + + mutableStateFlow.update { + it.copy(amountError = amountError, descriptionError = descriptionError) + } + + if (amountError == null && descriptionError == null) { + onTransferClicked() + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/amountTransfer/AmountTransferViewModel.kt` around lines 95 - 124, The validateFields function should compute validation results once and apply them in a single atomic state update: parse the amount once with state.amount.toDoubleOrNull() into a local val (avoid calling toDouble() again), compute local amountError and descriptionError values, call mutableStateFlow.update { it.copy(amountError = amountError, descriptionError = descriptionError) } in one update, and then if both local errors are null call onTransferClicked(); this ensures errors are cleared/set together and avoids redundant parsing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/createNewClient/CreateNewClientRoute.kt`:
- Line 19: The parameter typed as KFunction4 in CreateNewClientRoute.kt should
be changed to the idiomatic Kotlin function type that matches what
CreateNewClientScreen expects (e.g., a regular (Param1, Param2, Param3, Param4)
-> ReturnType signature) so callers can pass lambdas directly; update the
hasDatatables parameter's type to that function type, remove the
kotlin.reflect.KFunction4 import, and remove the extra wrapping lambda around
the passed reference where CreateNewClientScreen is invoked so the function is
forwarded directly.
- Line 12: The file is missing the full package import for FormWidgetDTO, uses
KFunction4 (kotlin.reflect) which is too restrictive and unsafe without
declaring kotlin-reflect, and creates an ephemeral mutableList that gets
discarded; fix by: 1) replace the bare import with the full package import
com.mifos.feature.dataTable.dataTableList.FormWidgetDTO wherever FormWidgetDTO
is used; 2) change the parameter type from KFunction4<...> to the plain function
type (List<DataTableEntity>, Any?, Int, MutableList<List<FormWidgetDTO>>) ->
Unit in the relevant function signatures (e.g., in CreateNewClientRoute and any
callers) so callers can pass lambdas without kotlin-reflect; 3) stop creating
the temporary mutableListOf() inline — allocate a named
MutableList<List<FormWidgetDTO>> before calling navigateDataTableList (or have
navigateDataTableList return the list) and pass that variable as the formWidget
parameter so populated data is retained.
- Around line 31-33: The lambda provided to hasDatatables is passing a freshly
created mutableListOf() as the 4th (output) parameter and then discarding it,
losing any populated FormWidgetDTO lists; update the lambda in
CreateNewClientRoute (and the analogous site in ClientNavigation) to create a
val formWidgets = mutableListOf<List<FormWidgetDTO>>() before calling
hasDatatables(datatables, clientPayload, Constants.CREATE_CLIENT, formWidgets),
then use/return/propagate formWidgets where the populated widgets are needed so
the filled output list is not discarded.
In `@feature/loan/src/commonMain/composeResources/values/strings.xml`:
- Line 307: Rename the string resource key from feature_loan_transfer_detaiils
to feature_loan_transfer_details in the XML and update all usages to the new
identifier; specifically change the XML <string
name="feature_loan_transfer_detaiils"> to feature_loan_transfer_details and
update any Kotlin references like Res.string.feature_loan_transfer_detaiils in
AmountTransferScreen.kt (and other call sites) to
Res.string.feature_loan_transfer_details so the generated binding matches.
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/amountTransfer/AmountTransferScreen.kt`:
- Around line 194-200: Replace the hardcoded "Description" label in the
MifosOutlinedTextField with the string resource
Res.string.feature_loan_description by calling stringResource(...) for the
label; update the MifosOutlinedTextField invocation (the one using
state.description and dispatching AmountTransferAction.OnDescriptionChange) so
its label uses the existing resource instead of the literal string.
- Around line 143-181: The four MifosTextFieldDropdown usages in
AmountTransferScreen currently use hardcoded placeholder options; replace those
static lists with the corresponding options from the ViewModel/state (e.g., use
state.officeOptions, state.clientOptions, state.accountTypeOptions,
state.accountOptions) and ensure the dropdowns' onValueChanged and value bind to
the state so selections update via onAction
(AmountTransferAction.OnOfficeChanged, OnClientChange, OnAccountTypeChange,
OnAccountChange) — fetch/populate those option lists in the ViewModel and expose
them on the state, then pass them into each MifosTextFieldDropdown instead of
listOf("Account 1", ...).
- Around line 211-226: The error dialog in AmountTransferDialogContent leaves
the user stuck because AmountTransferUiState.DialogState.Error renders
MifosSweetError without any click handler; update AmountTransferDialogContent so
that when dialogState is Error you pass an onclick (and optionally secondary
action) to MifosSweetError that delegates out to the screen’s action handler
(e.g., call onAction with a new AmountTransferAction.OnRetry or
AmountTransferAction.OnDismiss similar to LoanAccountSummaryDialog wiring to
LoanAccountSummaryAction.OnRetry); ensure you reference
AmountTransferUiState.DialogState.Error, MifosSweetError, and the screen-level
onAction/AmountTransferAction to implement retry and/or dismiss behavior.
- Around line 114-133: The LoanDetailRow instances are using hardcoded
placeholder strings; replace them by reading real values from the
AmountTransferViewModel's UI state (AmountTransferUiState) that is populated in
fetchTemplate() and from the loanId route param, e.g., bind label values like
applicantName, officeName, fromAccount, accountType, currency from the
viewModel.state (or state.value) instead of literals; ensure the ViewModel
receives the loanId (passed into AmountTransferScreen) and that fetchTemplate()
populates those fields on AmountTransferUiState so the composables render real
loan data.
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/amountTransfer/AmountTransferViewModel.kt`:
- Around line 126-127: The onTransferClicked() in AmountTransferViewModel is
currently a no-op; implement it to perform the actual transfer: validate inputs
(reuse existing validation logic), set a loading state, call the transfer API
method on your repository/service (the same data source used in
fetchTemplate()), handle the API response by updating UI state (success -> show
confirmation/navigation/close dialog; failure -> set error message and log), and
clear loading state in finally; also ensure fetchTemplate() is implemented or
gate the whole flow behind a feature flag so users cannot trigger a
non-functional transfer.
- Around line 83-93: fetchTemplate() currently fakes loading with delay(2000)
and never retrieves template data; replace the stub by calling the real data
sources (e.g., repository or API methods) to load offices/clients/accounts,
update mutableStateFlow with DialogState.Loaded (or populated dialog fields) on
success and DialogState.Error on failure, and perform this inside
viewModelScope.launch with proper try/catch and cancellation handling; update
references to DialogState and the view model's state payload in the
mutableStateFlow.update calls to include the fetched lists and clear loading,
and if you cannot implement it now open a follow-up issue and remove the
hardcoded delay/TODO before merging.
- Around line 24-28: AmountTransferViewModel lacks access to the route loan ID;
update its constructor to accept a SavedStateHandle (like
LoanRepaymentViewModel) and read the route arg key used by AmountTransferRoute
(e.g., "id" or the route's param name) to initialize the ViewModel with the
loanId so it can load data and submit transfers; specifically, add a
SavedStateHandle parameter to AmountTransferViewModel, extract the Int id from
it (with a safe default or error path), store/use that loanId in the ViewModel
init/loading logic, and ensure callers/DI provide the SavedStateHandle when
instantiating AmountTransferViewModel.
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryViewModel.kt`:
- Around line 117-133: The code has unreachable direct action handlers for
OnTransactionsClick, OnRepaymentScheduleClick, OnDocumentsClick, and
OnChargesClick; remove those dead branches from the direct
LoanAccountSummaryAction handling and delete the corresponding enum entries from
LoanSummaryDropDownAction (or any direct action variants that duplicate them),
keeping only the DropdownAction path and the handlers inside
handleDropdownAction (specifically referenced symbols: handleDropdownAction,
LoanSummaryDropDownAction, and LoanAccountSummaryAction.DropdownAction); ensure
any switch/when over LoanAccountSummaryAction no longer expects the removed
direct variants and update any pattern matches or import usages accordingly so
only DropdownAction routes these events.
---
Nitpick comments:
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/amountTransfer/AmountTransferScreenRoute.kt`:
- Around line 17-24: Rename the ambiguous parameter and make the route a data
class: change AmountTransferRoute to a data class with a descriptive parameter
name loanId instead of id, and update the navigateToTransferScreen function
signature and body to accept loanId and construct AmountTransferRoute(loanId);
also update any call sites that pass or read the old id to use loanId and rely
on the generated equals/hashCode/toString from the data class.
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/amountTransfer/AmountTransferViewModel.kt`:
- Around line 95-124: The validateFields function should compute validation
results once and apply them in a single atomic state update: parse the amount
once with state.amount.toDoubleOrNull() into a local val (avoid calling
toDouble() again), compute local amountError and descriptionError values, call
mutableStateFlow.update { it.copy(amountError = amountError, descriptionError =
descriptionError) } in one update, and then if both local errors are null call
onTransferClicked(); this ensures errors are cleared/set together and avoids
redundant parsing.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/amountTransfer/AmountTransferViewModel.kt (1)
99-103: Avoid parsingstate.amounttwice invalidateFields().
toDoubleOrNull()is called on line 99, and thentoDouble()is called again on line 103 after the non-null branch. Cache the result once.♻️ Proposed refactor
- if (state.amount.toDoubleOrNull() == null) { + val amount = state.amount.toDoubleOrNull() + if (amount == null) { mutableStateFlow.update { it.copy(amountError = Res.string.feature_loan_invalid_amount) } - } else if (state.amount.toDouble() <= 0.00) { + } else if (amount <= 0.00) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/amountTransfer/AmountTransferViewModel.kt` around lines 99 - 103, In validateFields(), avoid parsing state.amount twice: call state.amount.toDoubleOrNull() once, store the result in a local (e.g., val amount = state.amount.toDoubleOrNull()), use that to check null and to validate <= 0.0, and update mutableStateFlow with amountError = Res.string.feature_loan_invalid_amount as before; reference validateFields(), state.amount, mutableStateFlow, and amountError to locate and replace the duplicate toDouble()/toDoubleOrNull() calls.feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/amountTransfer/AmountTransferScreen.kt (1)
75-84: Wrap siblings in aBoxto make the overlay explicit.
AmountTransferContentandAmountTransferDialogContentrely on the parent NavHost's implicitBoxfor the overlay to render on top. Wrapping them in an explicitBoxmakes the intent self-documenting and safe against context changes.♻️ Proposed refactor
- AmountTransferContent( - navController, - state = state, - onAction = viewModel::trySendAction, - ) - - AmountTransferDialogContent( - dialogState = state.dialogState, - onAction = viewModel::trySendAction, - ) + Box { + AmountTransferContent( + navController, + state = state, + onAction = viewModel::trySendAction, + ) + AmountTransferDialogContent( + dialogState = state.dialogState, + onAction = viewModel::trySendAction, + ) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/amountTransfer/AmountTransferScreen.kt` around lines 75 - 84, The two siblings AmountTransferContent and AmountTransferDialogContent currently rely on the parent NavHost's implicit Box for overlay ordering; wrap both in an explicit Box so the dialog overlay is intentional and not dependent on parent layout. Replace the adjacent calls with a Box composable containing AmountTransferContent(...) first and AmountTransferDialogContent(...) after (ensuring the dialog is declared last so it renders on top), and add the necessary Compose import for Box if missing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/amountTransfer/AmountTransferScreen.kt`:
- Line 25: The import and usage reference `feature_loan_transfer_detaiils`
contains a typo (extra "i"); rename the string resource key to
`feature_loan_transfer_details` in the strings.xml resource, regenerate or
update the generated resources so the import becomes
`feature_loan_transfer_details`, and replace all uses in AmountTransferScreen
(the import line and any reference where `feature_loan_transfer_detaiils` is
used) with the corrected `feature_loan_transfer_details` identifier so names are
consistent across resource, generated code, and usage.
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/amountTransfer/AmountTransferViewModel.kt`:
- Around line 50-52: The OnAmountChange handler in AmountTransferViewModel
currently only updates amount and leaves validation errors intact; modify the
AmountTransferAction.OnAmountChange branch to also clear the amount error (e.g.,
set amountError = null or empty) when updating state, and likewise update the
AmountTransferAction.OnDescriptionChange branch to clear descriptionError on
each keystroke—update the mutableStateFlow.update calls in both handlers to copy
the new value and reset the corresponding error field.
- Around line 97-126: The validateFields() function currently only checks amount
and description and may call onTransferClicked() while selectedOfficeId,
selectedClientId, accountTypeId, or accountId are null; extend validation to
check each of these IDs and set new error properties on AmountTransferUiState
(e.g., officeError, clientError, accountTypeError, accountError) via
mutableStateFlow.update when a value is null, and only call onTransferClicked()
if all four new error fields plus amountError and descriptionError are null;
update the UI state class (AmountTransferUiState) to include the four error
fields and use their identifiers (selectedOfficeId, selectedClientId,
accountTypeId, accountId) in validateFields() to locate and fix the logic.
---
Duplicate comments:
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/amountTransfer/AmountTransferScreen.kt`:
- Around line 145-183: The four MifosTextFieldDropdown instances are using
hardcoded options; replace those with the appropriate option lists from the
screen state (e.g., use state.officeOptions for the office dropdown,
state.clientOptions for client, state.accountTypeOptions for account type, and
state.accountOptions for account name) and ensure the option items are mapped to
display strings if they are model objects; keep existing onOptionSelected
handlers (AmountTransferAction.OnOfficeChanged, OnClientChange,
OnAccountTypeChange, OnAccountChange) and wire them to the selected index/text
from those real lists so the UI reflects real data from the ViewModel/state
rather than listOf("Account 1", ...).
- Around line 116-135: The LoanDetailRow entries in AmountTransferScreen still
use hardcoded strings; replace them to read from the AmountTransferViewModel UI
state (e.g., amountTransferViewModel.uiState or state) instead of literals. For
each row (LoanDetailRow) bind value to the corresponding state properties
(applicantName, officeName, fromAccount, accountType, currency), using safe
fallbacks (empty string or stringResource) if null, and ensure the screen
collects the view model state (e.g., collectAsState()) so the UI reflects real
data from AmountTransferViewModel rather than static placeholders.
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/amountTransfer/AmountTransferViewModel.kt`:
- Around line 24-28: The AmountTransferViewModel currently has no access to the
route loanId so it cannot load or submit transfers for the correct loan; modify
the AmountTransferViewModel constructor to accept a loanId parameter (e.g.,
loanId: String or Long) and store it as a private property, update any creation
sites (factories/DI/ViewModelProvider instantiations) to pass the route's loanId
into AmountTransferViewModel, and use this loanId inside the ViewModel's
load/submit methods (e.g., functions that call the repository or trigger
actions) so all calls reference the correct loan identifier; locate the
AmountTransferViewModel class and its usages to apply this change.
- Around line 128-129: onTransferClicked() is currently a no-op; implement it to
perform validation and initiate the transfer flow: call the existing input
validation function (e.g., validateInputs or similar) and if validation fails
emit the appropriate error state (e.g., _state.postValue or showError method),
otherwise emit a loading state, call the amount transfer use-case (e.g.,
amountTransferUseCase or performTransfer) and handle its result by emitting
success (e.g., navigateBack or showSuccess) or failure (emit error state with
the exception/message). Ensure you update any ViewModel state holders (e.g.,
_state, LiveData/StateFlow) and clear loading when finished so the UI shows
visible feedback for both success and error paths.
- Around line 85-95: fetchTemplate() currently contains a 2s delay stub which
leaves dropdowns empty; replace the delay with real async loading: call the
repository/service method that fetches the transfer template (e.g.,
AmountTransferRepository.getTemplate or the appropriate dataSource), run it
inside viewModelScope.launch, update mutableStateFlow with DialogState.Loading
before the call, on success update mutableStateFlow to set dialogState = null
and populate the dropdown data fields in the state (account lists, currency,
etc.), and on failure set dialogState = DialogState.Error (or appropriate error
state) and log/handle the exception; ensure you remove the TODO and the
hardcoded delay and reference AmountTransferViewModel.fetchTemplate,
mutableStateFlow, and viewModelScope when implementing.
---
Nitpick comments:
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/amountTransfer/AmountTransferScreen.kt`:
- Around line 75-84: The two siblings AmountTransferContent and
AmountTransferDialogContent currently rely on the parent NavHost's implicit Box
for overlay ordering; wrap both in an explicit Box so the dialog overlay is
intentional and not dependent on parent layout. Replace the adjacent calls with
a Box composable containing AmountTransferContent(...) first and
AmountTransferDialogContent(...) after (ensuring the dialog is declared last so
it renders on top), and add the necessary Compose import for Box if missing.
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/amountTransfer/AmountTransferViewModel.kt`:
- Around line 99-103: In validateFields(), avoid parsing state.amount twice:
call state.amount.toDoubleOrNull() once, store the result in a local (e.g., val
amount = state.amount.toDoubleOrNull()), use that to check null and to validate
<= 0.0, and update mutableStateFlow with amountError =
Res.string.feature_loan_invalid_amount as before; reference validateFields(),
state.amount, mutableStateFlow, and amountError to locate and replace the
duplicate toDouble()/toDoubleOrNull() calls.
Nagarjuna0033
approved these changes
Feb 21, 2026
revanthkumarJ
approved these changes
Feb 22, 2026
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/amountTransfer/AmountTransferViewModel.kt (1)
183-209: Minor naming inconsistency in error fields.
selectedOfficeIdError/selectedClientIdErroruse theselectedprefix, whileaccountTypeIdError/accountIdErrordo not. Consider aligning the naming for consistency (e.g., all with or all without theselectedprefix).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/amountTransfer/AmountTransferViewModel.kt` around lines 183 - 209, Rename the error fields in AmountTransferUiState to be consistent: change accountTypeIdError and accountIdError to selectedAccountTypeIdError and selectedAccountIdError (or alternatively remove `selected` from the other two, but prefer adding `selected` to keep parity with selectedOfficeIdError and selectedClientIdError). Update all references/usages in the ViewModel, reducers, state copy calls, and UI bindings that read or set these properties (search for AmountTransferUiState, accountTypeIdError, accountIdError, selectedOfficeIdError, selectedClientIdError) so the new property names are used everywhere.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/amountTransfer/AmountTransferViewModel.kt`:
- Around line 110-177: In validateFields(), remove the premature call to
onTransferClicked() that happens immediately after checking amountError and
descriptionError (the early invocation when state.amountError == null &&
state.descriptionError == null) so the transfer is not fired before dropdown
validations; keep the later consolidated guard that checks
state.selectedOfficeIdError, state.selectedClientIdError,
state.accountTypeIdError, state.accountIdError, state.amountError and
state.descriptionError and call onTransferClicked() only there (references:
validateFields(), onTransferClicked(), state.amountError,
state.descriptionError, state.selectedOfficeIdError,
state.selectedClientIdError, state.accountTypeIdError, state.accountIdError).
---
Duplicate comments:
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/amountTransfer/AmountTransferViewModel.kt`:
- Around line 28-33: AmountTransferViewModel currently has an empty constructor
so it cannot read the loanId from navigation; change the constructor to accept a
SavedStateHandle parameter (e.g., savedStateHandle: SavedStateHandle), use that
handle inside AmountTransferViewModel to retrieve the loanId passed by
AmountTransferRoute (e.g., val loanId = savedStateHandle.get<String>("loanId")
or the route key you use), and update any callers/factory to provide the
SavedStateHandle so the ViewModel can initialize state or load data based on
that loanId.
- Around line 98-108: fetchTemplate() is a stub that only sets
DialogState.Loading and waits with delay(2000); replace the hardcoded delay with
a real data fetch and proper success/error handling: inside
viewModelScope.launch call the suspend repository/use-case (e.g.,
fetchTemplateFromRepository or an injected AmountTransferRepository method) to
retrieve the template, update mutableStateFlow with the loaded template on
success (clearing DialogState.Loading) and set an error state on failure (catch
exceptions and update dialogState accordingly), and remove the TODO; keep
references to fetchTemplate, mutableStateFlow, viewModelScope.launch and
DialogState.Loading when making the change.
- Around line 179-180: Implement the actual transfer flow inside
AmountTransferViewModel.onTransferClicked(): after running the existing
validation (use whatever validation method/fields already present in the
ViewModel), set a loading state on _uiState, call the transfer use-case/service
(e.g., amountTransferUseCase.transfer(...) or the appropriate repository method)
using the ViewModel's transferAmount and destinationAccountId fields, handle
success by updating _uiState (clear loading, emit success/navigate event) and
handle errors by catching exceptions and updating _uiState with an error
message; ensure you update observable state fields and emit events so the UI
reacts to loading, success and failure.
---
Nitpick comments:
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/amountTransfer/AmountTransferViewModel.kt`:
- Around line 183-209: Rename the error fields in AmountTransferUiState to be
consistent: change accountTypeIdError and accountIdError to
selectedAccountTypeIdError and selectedAccountIdError (or alternatively remove
`selected` from the other two, but prefer adding `selected` to keep parity with
selectedOfficeIdError and selectedClientIdError). Update all references/usages
in the ViewModel, reducers, state copy calls, and UI bindings that read or set
these properties (search for AmountTransferUiState, accountTypeIdError,
accountIdError, selectedOfficeIdError, selectedClientIdError) so the new
property names are used everywhere.
There was a problem hiding this comment.
🧹 Nitpick comments (3)
core/network/src/commonMain/kotlin/com/mifos/core/network/services/LoanService.kt (1)
161-164: Prefer typed response for transfer submit endpoint.Since a dedicated
AccountTransferResponsemodel exists, returning that type here would make the API contract stronger and simplify callers.♻️ Suggested refactor
+import com.mifos.core.model.objects.account.loan.transfer.AccountTransferResponse import io.ktor.client.statement.HttpResponse @@ `@POST`(APIEndPoint.ACCOUNT_TRANSFERS) suspend fun submitAccountTransfer( `@Body` request: AccountTransferRequest, - ): HttpResponse + ): AccountTransferResponse🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/network/src/commonMain/kotlin/com/mifos/core/network/services/LoanService.kt` around lines 161 - 164, The submitAccountTransfer endpoint currently returns a generic HttpResponse; change its return type to the dedicated model AccountTransferResponse to enforce a typed contract — update the function signature in submitAccountTransfer to return AccountTransferResponse (and add the necessary import for AccountTransferResponse), ensure the serializer/serialization setup supports deserializing into AccountTransferResponse, and update any callers that expected HttpResponse to work with the typed result.core/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerLoan.kt (2)
305-325: KDoc parameter order does not match function signature.The KDoc lists
fromOfficeIdas the first parameter, but in the function signature it's the 5th parameter (afterfromClientId,fromAccountType,fromAccountId,toOfficeId). This could cause confusion for callers.Suggested KDoc fix
/** * Retrieve account transfer template for populating UI dropdowns * - * `@param` fromOfficeId Source office ID * `@param` fromClientId Source client ID * `@param` fromAccountType Source account type ID * `@param` fromAccountId Source account ID * `@param` toOfficeId Destination office ID (optional) + * `@param` fromOfficeId Source office ID (optional) * `@param` toClientId Destination client ID (optional) * `@param` toAccountType Destination account type ID (optional) * `@return` AccountTransferTemplate with available options */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerLoan.kt` around lines 305 - 325, The KDoc for getAccountTransferTemplate has parameters listed in a different order than the function signature (e.g., fromOfficeId is documented first but appears fifth in the signature); update the KDoc parameter list to exactly match the function parameter order and descriptions (fromClientId, fromAccountType, fromAccountId, toOfficeId, fromOfficeId, toClientId, toAccountType) so each `@param` maps to the correct parameter name in the function.
344-353: Consider using consistent Json configuration withignoreUnknownKeys = true.Line 352 uses a default
Jsoninstance to deserialize the response. Compare this tocalculateLoanSchedule(line 370) which usesJson { ignoreUnknownKeys = true }. Without this configuration, deserialization will fail if the API response contains any unexpected fields.Suggested fix
suspend fun submitAccountTransfer( request: AccountTransferRequest, ): AccountTransferResponse { val response = mBaseApiManager.loanService.submitAccountTransfer(request) if (!response.status.isSuccess()) { val errorMessage = extractErrorMessage(response) throw IllegalStateException(errorMessage) } - return Json.decodeFromString<AccountTransferResponse>(response.bodyAsText()) + return Json { ignoreUnknownKeys = true }.decodeFromString<AccountTransferResponse>(response.bodyAsText()) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerLoan.kt` around lines 344 - 353, In submitAccountTransfer, replace the default Json deserialization with a Json instance configured with ignoreUnknownKeys = true (matching calculateLoanSchedule) so that additional/unexpected fields in the API response won't break decoding; locate the call in submitAccountTransfer and use Json { ignoreUnknownKeys = true } to decode the response body into AccountTransferResponse.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@core/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerLoan.kt`:
- Around line 305-325: The KDoc for getAccountTransferTemplate has parameters
listed in a different order than the function signature (e.g., fromOfficeId is
documented first but appears fifth in the signature); update the KDoc parameter
list to exactly match the function parameter order and descriptions
(fromClientId, fromAccountType, fromAccountId, toOfficeId, fromOfficeId,
toClientId, toAccountType) so each `@param` maps to the correct parameter name in
the function.
- Around line 344-353: In submitAccountTransfer, replace the default Json
deserialization with a Json instance configured with ignoreUnknownKeys = true
(matching calculateLoanSchedule) so that additional/unexpected fields in the API
response won't break decoding; locate the call in submitAccountTransfer and use
Json { ignoreUnknownKeys = true } to decode the response body into
AccountTransferResponse.
In
`@core/network/src/commonMain/kotlin/com/mifos/core/network/services/LoanService.kt`:
- Around line 161-164: The submitAccountTransfer endpoint currently returns a
generic HttpResponse; change its return type to the dedicated model
AccountTransferResponse to enforce a typed contract — update the function
signature in submitAccountTransfer to return AccountTransferResponse (and add
the necessary import for AccountTransferResponse), ensure the
serializer/serialization setup supports deserializing into
AccountTransferResponse, and update any callers that expected HttpResponse to
work with the typed result.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
core/data/src/commonMain/kotlin/com/mifos/core/data/di/RepositoryModule.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/repository/AmountTransferRepository.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/AmountTransferRepositoryImp.ktcore/database/src/commonMain/kotlin/com/mifos/room/basemodel/APIEndPoint.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/transfer/AccountOption.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/transfer/AccountTransferChanges.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/transfer/AccountTransferRequest.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/transfer/AccountTransferResponse.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/transfer/AccountTransferTemplate.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/transfer/AccountTypeOption.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/transfer/ClientOption.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/transfer/OfficeOption.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerLoan.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/services/LoanService.ktfeature/client/src/commonMain/kotlin/com/mifos/feature/client/createNewClient/CreateNewClientRoute.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- feature/client/src/commonMain/kotlin/com/mifos/feature/client/createNewClient/CreateNewClientRoute.kt
biplab1
previously approved these changes
Mar 4, 2026
Contributor
biplab1
left a comment
biplab1
left a comment
There was a problem hiding this comment.
Some minor changes requested.
Looks good to me. This can be merged after the requested changes have been addressed.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@feature/loan/src/commonMain/composeResources/values/strings.xml`:
- Line 410: The string resource feature_loan_must_select_account_type has the
wrong text (duplicate of the previous line); update its value to a message that
matches the key semantics such as "Must select account type" or "Please select
an account type" so validation for account type selection displays the correct
prompt; edit the <string name="feature_loan_must_select_account_type"> entry to
contain the corrected wording.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 62af8dad-91ba-45f6-bcb7-4e50006314ac
📒 Files selected for processing (1)
feature/loan/src/commonMain/composeResources/values/strings.xml
Contributor
|
@itsPronay Please fix the Android build issue in the PR checks. The dialog doesn't seem to align with the new UI, please review: 
And, also there seems to be multiple steps to making the account transfer in the web-app (screenshots in the ticket), can you confirm if all the steps have been covered? If not, does this require a stepper implementation like in Client Flow? |
Contributor
|
I have updated the screen recording in the PR description by reducing it size using handbrake (open source video compression software). |
Member
Author
Yes, I'll do it, looks like its occuring because I merged the remote branch.
Could you please tell me what kind of dialog UI you are looking for, a reference would be helpful.
Yes, all the steps have been covered. |
Contributor
For example: 
|
Contributor
|
@itsPronay Can you please tell us the status of this PR? |
Member
Author
|
@biplab1 I have updated the code to use the new MifosStatusDialog, here Success - Screenshot_20260310_215301_MifosXDroid.jpg |
|
biplab1
approved these changes
Mar 11, 2026
Contributor
biplab1
left a comment
biplab1
left a comment
There was a problem hiding this comment.
Looks good to me. This can be merged.
Contributor
|
The dialog box still does not appear fully aligned with the Client Flow UI/UX (e.g., icon sizes). However, it was merged to expedite making PR #2612 mergeable. We will revisit this feature once the UI/UX is finalized for the remaining feature modules. |
Member
Author
we just need to modify this componet here to increase the size, I used the extra large, seems like we need to increase it.
|
This was referenced Apr 1, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes - https://mifosforge.jira.com/browse/MIFOSAC-658
Adding Screen recording here for quick access:
Xrecorder.20260303.02.mp4
Screen Record - https://mifos.slack.com/archives/C06FC73DZPW/p1772543703431959?thread_ts=1771441608.045129&cid=C06FC73DZPW
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