Implement New Recurring Deposits Account Preview Step Flow #2559
Conversation
WalkthroughThis PR updates recurring deposit account creation to return generic responses instead of typed models, adds payload fields for penalty configurations, and introduces a network-availability wrapper pattern across feature modules. A comprehensive preview page is added to the recurring deposit flow with detailed account information rendering. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/InterestPage.kt (1)
103-111: The naming ofisRateChartEmptyis semantically inverted and should be corrected.The field is defined as
!template.accountChart?.chartSlabs.isNullOrEmpty(), which means:
isRateChartEmpty = truewhen chartSlabs contains data (is NOT empty)isRateChartEmpty = falsewhen chartSlabs is empty or nullThis contradicts the field name. For clarity, rename to
hasRateChartorisRateChartAvailable, or invert the logic definition. The same issue exists in the FixedDeposit feature (line 543 of CreateFixedDepositAccountViewmodel.kt).
♻️ Duplicate comments (1)
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/PreviewPage.kt (1)
183-191: Same inverted logic issue as InterestPage.This has the same potentially confusing logic where
isRateChartEmpty = trueshows the chart label and enables the button, whileisRateChartEmpty = falseshows "No Interest Rate Chart available". See comment on InterestPage.kt for details.
🧹 Nitpick comments (14)
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountScreen.kt (2)
117-128: Review the snackbar timing and delay logic.The current flow has a potential UX issue:
showSnackbar()suspends until the snackbar is dismissed (defaultShortduration is ~4 seconds)- Then waits an additional 1000ms delay
- Then navigates back
This results in the user waiting 5+ seconds after a successful action, which may feel sluggish.
Consider either:
- Removing the extra delay since
showSnackbaralready handles the display duration- Or specifying a shorter snackbar duration and relying solely on the delay
is RecurringAccountState.DialogState.SuccessResponseStatus -> { LaunchedEffect(state.launchEffectKey) { snackbarHostState.showSnackbar( message = state.dialogState.msg, + duration = SnackbarDuration.Short, ) - if (state.dialogState.successStatus) { - delay(1000) onAction(RecurringAccountAction.NavigateBack) } } }Also, verify that
state.launchEffectKeyis properly updated when a new success response is triggered, otherwise subsequent success responses may not re-trigger the effect.
86-91: Consider makingRecurringAccountDialogprivate.
RecurringAccountDialogis public whileRecurringAccountScreenisinternalandRecurringAccountScaffoldisprivate. For consistency and proper encapsulation, consider making this functionprivatesince it appears to only be used within this file.@Composable -fun RecurringAccountDialog( +private fun RecurringAccountDialog( state: RecurringAccountState, onAction: (RecurringAccountAction) -> Unit, snackbarHostState: SnackbarHostState, ) {feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountViewModel.kt (1)
62-67: Sequential loading in init may delay UI readiness.
loadAllLoans()andloadCollaterals()are called sequentially. Since they appear to be independent operations, consider launching them concurrently for faster initialization:init { viewModelScope.launch { - loadAllLoans() - loadCollaterals() + launch { loadAllLoans() } + launch { loadCollaterals() } } }feature/recurringDeposit/src/commonMain/composeResources/values/feature_recurring_deposit_string.xml (1)
77-79: LGTM with minor naming inconsistency.The new strings support the preview flow well. However,
feature_error_network_not_availableuses a genericfeature_error_prefix instead of the file's convention offeature_recurring_deposit_. Consider renaming tofeature_recurring_deposit_error_network_not_availablefor consistency, or alternatively, move this generic error string to a shared/common resources file if it's intended to be reused across features.feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/PreviewPage.kt (1)
85-88: Use design token for consistent spacing.Line 87 uses a hardcoded
20.dpvalue while the rest of the file usesDesignToken.padding.*values for consistency.Column( modifier = modifier.weight(1f).verticalScroll(rememberScrollState()), - verticalArrangement = Arrangement.spacedBy(20.dp), + verticalArrangement = Arrangement.spacedBy(DesignToken.padding.large), ) {core/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerRecurringAccount.kt (1)
12-42: Error handling and GenericResponse decoding look solid; consider centralizing JSON parsing if reusedThe new flow—inspecting
response.status, extracting an error message, and decoding the body intoGenericResponsewithignoreUnknownKeys—looks correct and matches the service’s newFlow<HttpResponse>signature. If you end up repeating this pattern for other endpoints, consider extracting a shared helper or reusing a commonJsoninstance to keep error handling and serialization config consistent.feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountv2/SavingsAccountViewModel.kt (2)
48-52: CentralizedisOnlinehelper + call sites look correct, but consider tightening visibility and reuseThe new
isOnlinesuspend helper and its usage insubmitSavingsApplication,loadClientTemplate,loadSavingsProductTemplate, andhandleRetryare consistent and correctly scoped inside coroutines; offline handling viaScreenState.Erroris clear and avoids starting the flows when offline. Two small refinements you might consider:
- Mark
isOnlineasprivateto keep the ViewModel’s public API lean, since it’s only used internally here.- This same pattern now exists in multiple ViewModels; longer‑term, you could extract a small shared helper (e.g., in
BaseViewModelor a utility with an injected error message) to avoid duplication, while still allowing feature‑specific strings.Also applies to: 58-70, 180-181, 569-575, 577-583, 586-588
161-161: Make thelockinPeriodFrequencyTypeexpression a bit clearerThe
takeIf { state.frequency.toIntOrNull() != null }guard is logically correct (you only send a type when the frequency parses), but the current single line is quite dense. For readability, consider restructuring:val lockinFrequency = state.frequency.toIntOrNull() lockinPeriodFrequency = lockinFrequency lockinPeriodFrequencyType = if (lockinFrequency != null) { state.savingsProductTemplate ?.lockinPeriodFrequencyTypeOptions ?.getOrNull(state.freqTypeIndex) ?.id } else { null }This keeps the same behavior but makes the dependency between the two fields explicit.
feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/CreateShareAccountViewModel.kt (1)
54-66:isOnlinewrapper and wrapped share account calls are correct; consider reuse and visibilityThe
isOnlinehelper here correctly guardscreateShareAccount,loadShareTemplateFromProduct, andloadShareTemplateso that repository flows are only started when online, and it centralizes offline UI feedback viaScreenState.Error(feature_client_error_network_not_available). Loading/overlay flags are managed consistently within the flows, and offline paths don’t leave spinners stuck.Given the same
isOnlinepattern now exists in multiple ViewModels, you might:
- Make this helper
privatein this class.- Later, extract a shared helper (e.g., an extension or BaseViewModel utility that takes the error string as a parameter) to reduce duplication across features.
Also applies to: 87-137, 140-181, 183-219
feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/CreateFixedDepositAccountViewmodel.kt (2)
53-65: Online guard around fixed‑deposit template loads is sound; consider small cleanupThe new
isOnlinehelper and its use in bothloadFixedDepositTemplateandloadRecurringAccountTemplateWithProductcorrectly prevent template fetches when offline and surface a clear network error viaScreenState.Error(feature_client_error_network_not_available). TheDataStatebranches still setscreenState/isOverlayLoadingin a consistent way, so loading indicators and error states should behave as before (with the added offline guard).As with the other ViewModels:
- You can safely mark
isOnlineasprivatehere.- Longer‑term, extracting a reusable helper (e.g., base/utility taking the error string as a parameter) would reduce duplication across fixed‑deposit, share, savings, loan, and RD ViewModels.
Also applies to: 279-313, 315-351
265-271: Rate chart dialog action wiring looks good; consider renamingisRateChartEmptyHooking
OnShowRateChartup to setdialogState = DialogState.RateChartDialogis straightforward and consistent with how other dialogs are controlled in this ViewModel.One minor naming nit:
val isRateChartEmpty = !template.accountChart?.chartSlabs.isNullOrEmpty()is actuallytruewhen there are slabs (non‑empty list). Renaming this to something likehasRateChartor inverting the boolean would make the intent clearer.feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountViewModel.kt (3)
271-274:isOnlinewrapper and template loading flow are clean; consider minor overlay tweakThe new
suspend fun isOnline(content: suspend () -> Unit)centralizes online/offline checks nicely, and bothloadRecurringAccountTemplateandloadRecurringAccountTemplateWithProductcorrectly use it to gate repository calls and setScreenStatebased onDataState.One small refinement you might consider: in the
elsebranch ofisOnline, also clearisOverlayLoadingActive(for safety, in case a previous operation left ittrue) so that an offline error never leaves a stale overlay spinner visible.Also applies to: 315-347, 349-385, 387-399, 401-405
579-590: Recurring frequency & amount validation logic is sound; keep state/ID usage consistentThe switch to
recurringDepositDetails.depositAmountwithdoubleNumberValidator, plus explicitrecurringFrequency/recurringFrequencyTypeIndexfields and therecurringErrorHandlegate, makes the settings step validation clearer. Tying the recurring‑frequency errors todepositFrequencySameAsGroupCenterMeeting(and clearing/resetting them on toggle and on change) avoids spurious errors when the calendar is inherited.Once the ID/index mismatch raised earlier is fixed for the related frequency‑type fields, this validation block and the new
OnRecurringFrequencyChange/OnRecurringFrequencyTypeIndexChangeactions will provide a solid UX.Also applies to: 612-623, 638-648, 650-682, 684-704, 980-985, 1095-1097
836-849: Charge edit mapping viaindexOfFirstis reasonable; consider handling the “not found” caseUsing
indexOfFirst { it.id == selectedEditCharge.chargeId }to derivechooseChargeIndexfromchargeIdis a good improvement over assuming list indices are stable.If for any reason the charge ID is no longer present in
template.chargeOptions,indexOfFirstwill return-1, which will propagate tochooseChargeIndexand may later cause issues when dereferencingchargeOptions[chooseChargeIndex!!]. It might be worth treating-1/nullhere as “non‑editable” (e.g., disable the save button or show an error) rather than relying on downstream code to handle it implicitly.Also applies to: 884-898
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
core/data/src/commonMain/kotlin/com/mifos/core/data/repository/RecurringAccountRepository.kt(2 hunks)core/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/RecurringAccountRepositoryImp.kt(2 hunks)core/model/src/commonMain/kotlin/com/mifos/core/model/objects/payloads/RecurringDepositAccountPayload.kt(1 hunks)core/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerRecurringAccount.kt(1 hunks)core/network/src/commonMain/kotlin/com/mifos/core/network/services/RecurringAccountService.kt(1 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/CreateShareAccountScreen.kt(0 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/CreateShareAccountViewModel.kt(4 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/CreateFixedDepositAccountViewmodel.kt(4 hunks)feature/loan/src/commonMain/composeResources/values/strings.xml(1 hunks)feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountViewModel.kt(10 hunks)feature/recurringDeposit/src/commonMain/composeResources/values/feature_recurring_deposit_string.xml(2 hunks)feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountRoute.kt(1 hunks)feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountScreen.kt(10 hunks)feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountViewModel.kt(17 hunks)feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/InterestPage.kt(3 hunks)feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/PreviewPage.kt(1 hunks)feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/SettingsPage.kt(4 hunks)feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountv2/SavingsAccountViewModel.kt(6 hunks)
💤 Files with no reviewable changes (1)
- feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/CreateShareAccountScreen.kt
🧰 Additional context used
🧬 Code graph analysis (7)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/CreateShareAccountViewModel.kt (4)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/CreateFixedDepositAccountViewmodel.kt (1)
isOnline(53-65)feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountViewModel.kt (1)
isOnline(69-85)feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountViewModel.kt (1)
isOnline(387-399)feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountv2/SavingsAccountViewModel.kt (1)
isOnline(58-70)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/CreateFixedDepositAccountViewmodel.kt (4)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/CreateShareAccountViewModel.kt (1)
isOnline(54-66)feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountViewModel.kt (1)
isOnline(69-85)feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountViewModel.kt (1)
isOnline(387-399)feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountv2/SavingsAccountViewModel.kt (1)
isOnline(58-70)
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/PreviewPage.kt (3)
core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosRowWithTextAndButton.kt (1)
MifosRowWithTextAndButton(35-95)core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosTwoButtonRow.kt (1)
MifosTwoButtonRow(31-91)core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosListingComponent.kt (1)
MifosDefaultListingComponentFromStringResources(244-267)
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountScreen.kt (1)
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/PreviewPage.kt (1)
PreviewPage(76-143)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountViewModel.kt (4)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/CreateShareAccountViewModel.kt (1)
isOnline(54-66)feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/CreateFixedDepositAccountViewmodel.kt (1)
isOnline(53-65)feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountViewModel.kt (1)
isOnline(387-399)feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountv2/SavingsAccountViewModel.kt (1)
isOnline(58-70)
feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountv2/SavingsAccountViewModel.kt (4)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/CreateShareAccountViewModel.kt (1)
isOnline(54-66)feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/CreateFixedDepositAccountViewmodel.kt (1)
isOnline(53-65)feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountViewModel.kt (1)
isOnline(69-85)feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountViewModel.kt (1)
isOnline(387-399)
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/SettingsPage.kt (3)
core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosCheckBox.kt (1)
MifosCheckBox(24-46)core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/component/MifosOutlinedTextField.kt (4)
MifosOutlinedTextField(66-124)MifosOutlinedTextField(126-243)MifosOutlinedTextField(245-308)MifosOutlinedTextField(415-487)core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/component/MifosTextFieldDropdown.kt (1)
MifosTextFieldDropdown(39-112)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: PR Checks / Static Analysis Check
🔇 Additional comments (15)
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountScreen.kt (2)
171-176: LGTM!The
PreviewPagestep is properly integrated into the stepper flow. The composable parameters match the expected signature fromPreviewPage.kt, withmodifierusing its default value.
179-218: LGTM!The scaffold implementation correctly integrates the
snackbarHostStateand the overlay loading indicator. The property rename toisOverlayLoadingActivealigns with the state model changes.feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountRoute.kt (1)
26-30: LGTM!The back navigation is now properly wired to the NavController.
Minor observation: Both
navControllerandonNavigateBackare passed toRecurringAccountScreen. IfonNavigateBackalways callspopBackStack(), consider whether passing only one is cleaner, though current approach allows flexibility.feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountViewModel.kt (1)
69-85: LGTM - isOnline pattern is consistent with codebase.The network-aware helper follows the same pattern used in other ViewModels (
RecurringAccountViewModel,SavingsAccountViewModel,CreateShareAccountViewModel, etc.), maintaining consistency across the codebase. Based on relevant code snippets.feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/SettingsPage.kt (1)
254-310: LGTM - Conditional frequency configuration block.The new
AnimatedVisibilityblock correctly shows the recurring frequency fields whendepositFrequencySameAsGroupCenterMeetingis disabled. The implementation:
- Follows the same pattern as the existing pre-mature closure section
- Includes proper validation with error states (
recurringFrequencyError,recurringFrequencyTypeError)- Uses consistent spacing and component structure
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/InterestPage.kt (1)
73-84: LGTM - Enhanced interest chart details.The additional fields (
name,end_date,description) provide more comprehensive information about the interest rate chart, improving the user experience.feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/PreviewPage.kt (1)
76-143: Well-structured preview page.The
PreviewPagecomposable is well-organized with clear section separations for Details, Terms, Settings, Interest, and Charges. The composition follows the established patterns in the codebase.core/model/src/commonMain/kotlin/com/mifos/core/model/objects/payloads/RecurringDepositAccountPayload.kt (1)
35-43: Confirm amount type change and new pre-closure fields against backend schemaSwitching
mandatoryRecommendedDepositAmounttoDouble?and adding the threepreClosure*fields looks consistent with other amount fields, but it is a wire-format change. Please double-check these names and types match the backend API and that any callers previously treatingmandatoryRecommendedDepositAmountasIntare updated.core/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/RecurringAccountRepositoryImp.kt (1)
16-41: Repository impl now correctly propagates GenericResponseThe implementation cleanly switches
createRecurringDepositAccounttoFlow<DataState<GenericResponse>>and continues delegating toDataManagerRecurringAccountwithasDataStateFlow(). The change is cohesive with the updated DataManager signature and doesn’t introduce extra behavior.core/data/src/commonMain/kotlin/com/mifos/core/data/repository/RecurringAccountRepository.kt (1)
15-27: Repository API now returns DataState; ensure all callers handle the new shapeChanging
createRecurringDepositAccountto returnFlow<DataState<GenericResponse>>instead ofDataState<RecurringDeposit>is an intentional but breaking API change. As long as all call sites have been updated to consumeGenericResponse(and any UI logic expecting a fullRecurringDepositmodel has been adjusted), this interface looks good.core/network/src/commonMain/kotlin/com/mifos/core/network/services/RecurringAccountService.kt (1)
19-27: Returning Flow matches the new DataManager logic; confirm Ktorfit supports this shapeSwitching
createRecurringDepositAccounttoFlow<HttpResponse>aligns with the manual parsing inDataManagerRecurringAccount. Please confirm this return type is supported by your Ktorfit configuration (or matches patterns used in other services) so that the Flow adapter and call serialization behave as expected.feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountv2/SavingsAccountViewModel.kt (2)
288-292: Robust handling of edited charge index mappingUsing
indexOfFirstwith safe calls and?: -1when mappingselectedEditChargeback tochargeOptionsis a good improvement: it handlessavingsProductTemplate/chargeOptionsbeing null and gracefully falls back when the charge id is not found. No functional issues here.
460-472: Index initialization from template options remains soundThe reflowed index calculations for posting period, interest calculation, compounding period, and days‑in‑year types still correctly default to
-1when the corresponding option or selected value is missing. This keeps state initialization resilient to incomplete templates, which is good.feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountViewModel.kt (2)
73-158: Account creation payload & dialog/overlay handling look consistentThe expanded
RecurringDepositAccountPayloadwiring (charges, recurring frequency, lock‑in, and pre‑closure penalty fields) is coherent and null‑safe, and theDataStatehandling correctly drivesisOverlayLoadingActiveplus the newSuccessResponseStatusdialog for both success and theIllegalStateExceptionerror case.Assuming the repository emits
IllegalStateExceptiononly for API‑level “business” errors, this split between inline dialog vs. full‑screen error is a reasonable UX choice, and thelaunchEffectKey = Random.nextInt()pattern should work well for triggering one‑shot effects in the UI.Also applies to: 188-205, 918-924, 931-937
271-274: Submit action wiring and retry flow look correctThe introduction of
RecurringAccountAction.OnSubmitRecurringAccountand its handling viacreateRecurringDepositAccount()wires the new preview step cleanly into the flow, and reusing the sameretry()/loadRecurringAccountTemplate()path underRecurringAccountAction.Retrykeeps the error‑recovery story simple.No functional issues spotted here.
Also applies to: 400-405, 884-898, 900-903, 1037-1037
| <string name="feature_error_network_not_available">Network is not Available</string> | ||
|
|
There was a problem hiding this comment.
Duplicate network error string with inconsistent naming and text.
This file already contains feature_loan_error_network_not_available at line 88 with value "Network not available". The new string:
- Uses different naming convention (missing
feature_loan_prefix) - Has different text capitalization ("Network is not Available" vs "Network not available")
Consider removing one and standardizing on a single network error string, or if both are intentionally different, align the naming convention.
🤖 Prompt for AI Agents
In feature/loan/src/commonMain/composeResources/values/strings.xml around lines
14-15, you added a duplicate network error string with a different key and
capitalization; remove the new <string
name="feature_error_network_not_available"> entry and use the existing
feature_loan_error_network_not_available (line 88) with the standardized text
"Network not available"; if both variants are intentionally required, rename the
new key to include the feature_loan_ prefix and align its text to the project's
canonical wording, then update any callers to reference the chosen key.
| Res.string.installment_paid to ( | ||
| dataState.data.repaymentSchedule.periods | ||
| ?.count { it.complete == true } | ||
| ?.toString() | ||
| ?: "N/A" | ||
| dataState.data.repaymentSchedule.periods?.count { it.complete == true } | ||
| ?.toString() ?: "N/A" | ||
| ), | ||
| Res.string.installment_paid to ( | ||
| dataState.data.repaymentSchedule.periods | ||
| ?.count { it.complete == false } | ||
| ?.toString() | ||
| ?: "N/A" | ||
| dataState.data.repaymentSchedule.periods?.count { it.complete == false } | ||
| ?.toString() ?: "N/A" | ||
| ), | ||
| Res.string.total_installments to dataState.data.termFrequency.toString(), |
There was a problem hiding this comment.
Bug: Duplicate map key causes data loss.
Res.string.installment_paid is used as the key for both entries at lines 916 and 920. In a Kotlin mapOf, duplicate keys result in only the last value being retained, so the count of completed installments is lost.
Based on the filter logic (complete == true vs complete == false), the second entry should likely use Res.string.installment_left.
Apply this diff to fix the duplicate key:
Res.string.installment_paid to (
dataState.data.repaymentSchedule.periods?.count { it.complete == true }
?.toString() ?: "N/A"
),
- Res.string.installment_paid to (
+ Res.string.installment_left to (
dataState.data.repaymentSchedule.periods?.count { it.complete == false }
?.toString() ?: "N/A"
),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Res.string.installment_paid to ( | |
| dataState.data.repaymentSchedule.periods | |
| ?.count { it.complete == true } | |
| ?.toString() | |
| ?: "N/A" | |
| dataState.data.repaymentSchedule.periods?.count { it.complete == true } | |
| ?.toString() ?: "N/A" | |
| ), | |
| Res.string.installment_paid to ( | |
| dataState.data.repaymentSchedule.periods | |
| ?.count { it.complete == false } | |
| ?.toString() | |
| ?: "N/A" | |
| dataState.data.repaymentSchedule.periods?.count { it.complete == false } | |
| ?.toString() ?: "N/A" | |
| ), | |
| Res.string.total_installments to dataState.data.termFrequency.toString(), | |
| Res.string.installment_paid to ( | |
| dataState.data.repaymentSchedule.periods?.count { it.complete == true } | |
| ?.toString() ?: "N/A" | |
| ), | |
| Res.string.installment_left to ( | |
| dataState.data.repaymentSchedule.periods?.count { it.complete == false } | |
| ?.toString() ?: "N/A" | |
| ), | |
| Res.string.total_installments to dataState.data.termFrequency.toString(), |
🤖 Prompt for AI Agents
In
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountViewModel.kt
around lines 916 to 924, there is a duplicate map key
(Res.string.installment_paid) used for both completed and incomplete installment
counts which causes the first value to be overwritten; change the second key to
Res.string.installment_left so the map contains both "installments paid" and
"installments left" entries, keeping the existing count logic and formatting
unchanged.
| Res.string.feature_recurring_deposit_penal_interest_percentage to state.recurringDepositAccountSettings.preMatureClosure.penalInterest, | ||
|
|
||
| Res.string.feature_recurring_deposit_period to if (state.recurringDepositAccountSettings.preMatureClosure.interestPeriodIndex != -1) { | ||
| state.template.preClosurePenalInterestOnTypeOptions?.getOrNull( | ||
| state.recurringDepositAccountSettings.preMatureClosure.interestPeriodIndex, | ||
| )?.value.orEmpty() | ||
| } else { | ||
| "" | ||
| }, | ||
|
|
||
| Res.string.feature_recurring_deposit_period to state.recurringDepositAccountSettings.preMatureClosure.minimumBalanceForInterestCalculation, |
There was a problem hiding this comment.
Bug: Duplicate map key feature_recurring_deposit_period causes data loss.
Res.string.feature_recurring_deposit_period is used as a key twice in this map (lines 293 and 301). The second entry will overwrite the first, losing the pre-closure interest period data.
Line 293 shows the interest period type, while line 301 shows the minimum balance for interest calculation. These should have distinct keys.
Consider using a different string resource key for line 301, such as a new feature_recurring_deposit_minimum_balance_for_interest or similar:
Res.string.feature_recurring_deposit_period to if (state.recurringDepositAccountSettings.preMatureClosure.interestPeriodIndex != -1) {
state.template.preClosurePenalInterestOnTypeOptions?.getOrNull(
state.recurringDepositAccountSettings.preMatureClosure.interestPeriodIndex,
)?.value.orEmpty()
} else {
""
},
- Res.string.feature_recurring_deposit_period to state.recurringDepositAccountSettings.preMatureClosure.minimumBalanceForInterestCalculation,
+ Res.string.feature_recurring_deposit_minimum_balance_for_interest to state.recurringDepositAccountSettings.preMatureClosure.minimumBalanceForInterestCalculation,Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/PreviewPage.kt
around lines 291 to 301, the map uses the same key
Res.string.feature_recurring_deposit_period twice so the second entry overwrites
the first; change the second key to a distinct string resource (e.g.,
Res.string.feature_recurring_deposit_minimum_balance_for_interest or an existing
appropriate key), update resources if needed, and ensure the map entry value
remains
state.recurringDepositAccountSettings.preMatureClosure.minimumBalanceForInterestCalculation.
| Res.string.feature_recurring_deposit_product_name to state.template.productOptions?.get( | ||
| state.recurringDepositAccountDetail.loanProductSelected, | ||
| )?.name.orEmpty(), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for loanProductSelected definition and usage
rg -n "loanProductSelected" --type=kotlin -B 3 -A 3 | head -100Repository: openMF/android-client
Length of output: 15302
🏁 Script executed:
#!/bin/bash
# Find RecurringAccountDetail definition
rg -n "data class RecurringAccountDetail|class RecurringAccountDetail" --type=kotlin -A 20Repository: openMF/android-client
Length of output: 8116
🏁 Script executed:
#!/bin/bash
# Check for similar product selection properties in recurring deposit context
rg -n "productSelected|ProductSelected" feature/recurringDeposit --type=kotlin | head -50Repository: openMF/android-client
Length of output: 1875
Rename loanProductSelected to recurringDepositProductSelected in RecurringAccountDetailsState.
The property loanProductSelected is defined in RecurringAccountDetailsState and uses "loan" terminology despite being part of the recurring deposit feature. This appears to be a copy-paste oversight from the loan account feature. The property is accessed as an index into recurring deposit product options and should be renamed for semantic clarity. This change affects 8 usages across the recurring deposit module (RecurringAccountViewModel, PreviewPage, and DetailsPage).
🤖 Prompt for AI Agents
In
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/PreviewPage.kt
around lines 354-356, the state property loanProductSelected is incorrectly
named for recurring deposits; rename it to recurringDepositProductSelected in
RecurringAccountDetailsState and update all usages (RecurringAccountViewModel,
PreviewPage, DetailsPage and any other 7 remaining references) to use the new
name, keeping the same type and nullability, update any serialization/parcel/map
keys or constructor parameters that reference the old name, and run a build to
fix compiler errors and adjust tests or migrations accordingly.
| import kotlin.time.ExperimentalTime | ||
|
|
||
| const val TOTAL_STEPS = 4 | ||
| const val TOTAL_STEPS = 6 |
There was a problem hiding this comment.
Guard against negative currentStep on back press and re‑confirm TOTAL_STEPS semantics
moveToNextStep uses if (state.currentStep < state.totalSteps) currentStep++ else Finish, and NavigateToStep allows index in 0..state.totalSteps. With TOTAL_STEPS = 6, that means you’re treating totalSteps as the max step index (0..6) rather than a pure “count”. That’s fine, but it’s worth double‑checking that the UI step indicators also assume 0‑based indices up to TOTAL_STEPS, not 1..TOTAL_STEPS.
More critically, OnBackPress now unconditionally does currentStep = current - 1, which can drive currentStep negative if invoked on step 0. Safer would be to delegate to NavigateBack when at the first step:
RecurringAccountAction.OnBackPress -> {
- val current = state.currentStep
- mutableStateFlow.update {
- it.copy(
- currentStep = current - 1,
- )
- }
+ val current = state.currentStep
+ if (current <= 0) {
+ sendEvent(RecurringAccountEvent.NavigateBack)
+ } else {
+ mutableStateFlow.update {
+ it.copy(
+ currentStep = current - 1,
+ )
+ }
+ }
}This avoids negative indices and cleanly exits the flow when backing from the first step.
Also applies to: 60-71, 750-761, 763-770
| depositPeriodFrequencyId = ( | ||
| state.template.periodFrequencyTypeOptions?.getOrNull( | ||
| state.recurringDepositAccountSettings.depositPeriod.periodType, | ||
| )?.id | ||
| ), |
There was a problem hiding this comment.
Fix ID/index mismatch for period/lock‑in/pre‑closure option mapping
In several places you store option IDs into fields that are named and later treated as indices, then look them up again with get/getOrNull when building the payload. That will resolve the wrong option (or null) when IDs and list positions diverge, so the server can receive incorrect depositPeriodFrequencyId, lockinPeriodFrequencyType, or preClosurePenalInterestOnTypeId values.
Concretely:
SetDepositPeriodTypewritesperiodType = options?.get(action.periodType)?.id, butdepositPeriodFrequencyIdlater doesoptions?.getOrNull(depositPeriod.periodType)?.id.SetLockInPeriodTypewritesfrequencyTypeIndex = options?.get(action.frequencyTypeIndex)?.id, but the payload uses it as an index ingetOrNull(...).SetPreMatureClosureInterestPeriodIndexwritesinterestPeriodIndex = options?.get(action.interestPeriodIndex)?.id, and the payload again treats it as an index.
Align these by keeping indices in state and only converting to IDs when constructing the payload. For example:
- is RecurringAccountAction.RecurringAccountSettingsAction.SetDepositPeriodType -> {
- mutableStateFlow.update { state ->
- state.copy(
- recurringDepositAccountSettings = state.recurringDepositAccountSettings.copy(
- depositPeriod = state.recurringDepositAccountSettings.depositPeriod.copy(
- periodType = state.template.periodFrequencyTypeOptions?.get(
- action.periodType,
- )?.id ?: -1,
- ),
- depositPeriodTypeError = null,
- ),
- )
- }
- }
+ is RecurringAccountAction.RecurringAccountSettingsAction.SetDepositPeriodType -> {
+ mutableStateFlow.update { state ->
+ state.copy(
+ recurringDepositAccountSettings = state.recurringDepositAccountSettings.copy(
+ // keep the selected index; map to ID when building the payload
+ depositPeriod = state.recurringDepositAccountSettings.depositPeriod.copy(
+ periodType = action.periodType,
+ ),
+ depositPeriodTypeError = null,
+ ),
+ )
+ }
+ }- is RecurringAccountAction.RecurringAccountSettingsAction.SetLockInPeriodType -> {
- mutableStateFlow.update { state ->
- state.copy(
- recurringDepositAccountSettings = state.recurringDepositAccountSettings.copy(
- lockInPeriod = state.recurringDepositAccountSettings.lockInPeriod.copy(
- frequencyTypeIndex = state.template.lockinPeriodFrequencyTypeOptions?.get(
- action.frequencyTypeIndex,
- )?.id ?: -1,
- ),
- ),
- )
- }
- }
+ is RecurringAccountAction.RecurringAccountSettingsAction.SetLockInPeriodType -> {
+ mutableStateFlow.update { state ->
+ state.copy(
+ recurringDepositAccountSettings = state.recurringDepositAccountSettings.copy(
+ lockInPeriod = state.recurringDepositAccountSettings.lockInPeriod.copy(
+ // store index; payload maps index -> ID
+ frequencyTypeIndex = action.frequencyTypeIndex,
+ ),
+ ),
+ )
+ }
+ }- is RecurringAccountAction.RecurringAccountSettingsAction.SetPreMatureClosureInterestPeriodIndex -> {
- mutableStateFlow.update {
- state.copy(
- recurringDepositAccountSettings = state.recurringDepositAccountSettings.copy(
- preMatureClosure = state.recurringDepositAccountSettings.preMatureClosure.copy(
- interestPeriodIndex = state.template.preClosurePenalInterestOnTypeOptions?.get(
- action.interestPeriodIndex,
- )?.id ?: -1,
- ),
- ),
- )
- }
- }
+ is RecurringAccountAction.RecurringAccountSettingsAction.SetPreMatureClosureInterestPeriodIndex -> {
+ mutableStateFlow.update {
+ state.copy(
+ recurringDepositAccountSettings = state.recurringDepositAccountSettings.copy(
+ preMatureClosure = state.recurringDepositAccountSettings.preMatureClosure.copy(
+ // keep the selected index; payload maps index -> ID
+ interestPeriodIndex = action.interestPeriodIndex,
+ ),
+ ),
+ )
+ }
+ }With this change, the existing payload lookups using getOrNull(<index>)?.id become consistent and deterministic.
Also applies to: 134-140, 151-157, 422-435, 449-461, 541-553
🤖 Prompt for AI Agents
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountViewModel.kt
lines 84-88 (also apply same fix to ranges 134-140, 151-157, 422-435, 449-461,
541-553): state fields currently store option IDs where later code treats them
as list indices, causing wrong lookups; change the reducers to store the
selected option index (the raw index from the action) in state and only map
index->id when building the payload (i.e., keep
periodType/frequencyTypeIndex/interestPeriodIndex as indices in state, remove
storing .id there), then when constructing depositPeriodFrequencyId,
lockinPeriodFrequencyType, preClosurePenalInterestOnTypeId call
options?.getOrNull(index)?.id to convert to IDs; update all affected setters and
any state names if necessary to reflect they hold indices, ensuring consistency
across the listed line ranges.
2 participants
Fixes - Jira-#614
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.