Enhance Terms and setting screen of recurring account #2550
Conversation
WalkthroughThis PR restructures the recurring deposit account creation flow with overlay loading state management, integrated form validation on submission, expanded error handling via StringResource fields, and comprehensive UI layout redesigns. It also refactors the fixed deposit account view model to track overlay loading states and adds minor padding adjustments to savings account pages. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountViewModel.kt (3)
76-122: WireproductIdin details state before submitting payloadIn
createRecurringDepositAccountyou send:productId = s.recurringDepositAccountDetail.productId,but in
RecurringAccountDetailsState:data class RecurringAccountDetailsState( val productId: Int = -1, ... )there’s no code in this viewmodel that ever updates
productIdfrom the selected product (loanProductSelected/ template.productOptions). As written,productIdremains-1for all submissions, which is very likely invalid for the backend.Given you already load the product-specific template based on selection, a simple fix would be to set
productIdwhen the product changes, e.g.:private fun handleProductNameChange(action: RecurringAccountAction.RecurringAccountDetailsAction.OnProductNameChange) { mutableStateFlow.update { val product = it.template.productOptions?.getOrNull(action.index) it.copy( recurringDepositAccountDetail = it.recurringDepositAccountDetail.copy( loanProductSelected = action.index, productId = product?.id ?: -1, productError = null, fieldOfficerError = null, ), ) } loadRecurringAccountTemplateWithProduct( state.clientId, product?.id ?: -1, ) }or derive the ID directly when creating the payload from
loanProductSelectedandtemplate.productOptions. Without this, account creation will almost certainly fail.Also applies to: 713-729
206-221: Retry currently drops clientId and restarts with an invalid state
resetForRetry()sets:clientId = -1, ... screenState = ScreenState.Loading, ...and then immediately calls
loadRecurringAccountTemplate(), which usesstate.clientIdas the argument. This means any Retry after an error will call the repository withclientId = -1, not the original route client ID, and likely never recover.Prefer preserving the original
clientIdand other immutable identity fields on retry, for example:private fun resetForRetry() { val currentClientId = state.clientId mutableStateFlow.update { it.copy( isOnline = false, clientId = currentClientId, currentStep = 0, screenState = ScreenState.Loading, recurringDepositAccountDetail = RecurringAccountDetailsState(), template = RecurringDepositAccountTemplate(), recurringDepositAccountSettings = RecurringDepositAccountSettingsState(), currencyIndex = -1, currencyError = null, ) } loadRecurringAccountTemplate() }so retry can actually reload for the same client.
663-671: Back button should move to the previous step, not hard-code step 3
OnBackPresscurrently does:RecurringAccountAction.OnBackPress -> { mutableStateFlow.update { it.copy(currentStep = 3) } }This hard-codes step index 3 regardless of the current step. Since “Back” is used on multiple steps (e.g., Terms and Settings), this will jump the user to the Interest page rather than the previous step.
A more predictable behavior is to step back by one within bounds:
RecurringAccountAction.OnBackPress -> { mutableStateFlow.update { it.copy(currentStep = (state.currentStep - 1).coerceAtLeast(0)) } }You can still expose
NavigateBackas the action for leaving the flow entirely.
🧹 Nitpick comments (4)
feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountv2/pages/TermsPage.kt (1)
66-66: Bottom padding addition looks good; consider threading through themodifier(optional).Adding
padding(bottom = DesignToken.padding.large)on the outerColumnshould help keep the bottom buttons and content away from the edge and aligns with the pattern used on other savings pages. Functionally this is fine.If you ever need external customization of this screen’s padding or behavior, you might consider applying the passed-in
modifieron the outer container instead, for example:Column( modifier = modifier .fillMaxSize() .padding(bottom = DesignToken.padding.large), ) { Column( modifier = Modifier .weight(1f) .verticalScroll(rememberScrollState()), ) { ... } }Not required for this PR, just a small consistency/extensibility tweak you may want to consider.
feature/client/src/commonMain/composeResources/values/strings.xml (1)
596-606: Minor: normalize XML comment spacing for consistencyEverywhere else section headers use a space after the opening marker (e.g.,
<!-- Client Products -->), but this one is<!--Fixed Deposit Account -->. Consider changing to<!-- Fixed Deposit Account -->for consistency.core/ui/src/commonMain/kotlin/com/mifos/core/ui/util/TextFieldsValidator.kt (1)
58-63: dropDownEmptyValidator behavior matches usageThe implementation (
true→error_field_empty, otherwisenull) fits the current call sites that pass conditions likeindex == -1. You might consider renaming the parameter to something likeisEmptyfor clarity, but functionally this is fine.feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/TermsPage.kt (1)
43-131: Scrollable layout is good; consider safer option indexingThe new full-height, scrollable layout with a step header reads well and keeps the four interest-related dropdowns intact.
For the
valueprops you’re doing:state.template.interestCompoundingPeriodTypeOptions ?.get(state.recurringDepositAccountInterestChart.interestCompoundingPeriodType) ?.value ?: ""If template options change (e.g., after a template reload) while the stored index is still pointing at an old position, this can throw. Using
getOrNull(...)instead ofget(...)would fail gracefully and let the user reselect:state.template.interestCompoundingPeriodTypeOptions ?.getOrNull(state.recurringDepositAccountInterestChart.interestCompoundingPeriodType) ?.value ?: ""Same comment applies to the other three dropdowns.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
core/ui/src/commonMain/kotlin/com/mifos/core/ui/util/TextFieldsValidator.kt(1 hunks)feature/client/src/commonMain/composeResources/values/strings.xml(1 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/CreateFixedDepositAccountViewmodel.kt(2 hunks)feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountScreen.kt(2 hunks)feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountViewModel.kt(19 hunks)feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/di/RecurringDepositModule.kt(0 hunks)feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/DetailsPage.kt(5 hunks)feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/SettingsPage.kt(2 hunks)feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/TermsPage.kt(2 hunks)feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountv2/pages/ChargesPage.kt(1 hunks)feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountv2/pages/DetailsPage.kt(1 hunks)feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountv2/pages/PreviewPage.kt(1 hunks)feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountv2/pages/TermsPage.kt(1 hunks)
💤 Files with no reviewable changes (1)
- feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/di/RecurringDepositModule.kt
🧰 Additional context used
🧬 Code graph analysis (4)
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/DetailsPage.kt (3)
core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/component/MifosTextFieldDropdown.kt (1)
MifosTextFieldDropdown(39-112)core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/component/MifosOutlinedTextField.kt (5)
MifosDatePickerTextField(363-413)MifosOutlinedTextField(66-124)MifosOutlinedTextField(126-243)MifosOutlinedTextField(245-308)MifosOutlinedTextField(415-487)core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosTwoButtonRow.kt (1)
MifosTwoButtonRow(31-91)
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountScreen.kt (1)
core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosProgressIndicator.kt (1)
MifosProgressIndicatorOverlay(70-102)
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/TermsPage.kt (2)
core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/component/MifosTextFieldDropdown.kt (1)
MifosTextFieldDropdown(39-112)core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosTwoButtonRow.kt (1)
MifosTwoButtonRow(31-91)
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountViewModel.kt (1)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/CreateFixedDepositAccountViewmodel.kt (6)
moveToNextStep(254-265)handleSubmissionDateChange(120-128)handleSubmissionDatePick(134-142)handleFieldOfficerChange(155-163)handleExternalIdChange(165-173)handleInterestCalculationType(224-232)
⏰ 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 (7)
feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountv2/pages/DetailsPage.kt (1)
101-101: LGTM! Bottom padding improves layout consistency.The addition of bottom padding using
DesignToken.padding.largefollows best practices and aligns with similar spacing improvements across other pages in the savings feature.feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountv2/pages/ChargesPage.kt (1)
52-52: LGTM! Bottom padding improves page layout.The addition of bottom padding to the outer Column ensures proper spacing below the navigation buttons, preventing them from sitting at the screen edge. This aligns with similar UI enhancements applied to other savings account pages.
feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountv2/pages/PreviewPage.kt (1)
132-132: LGTM! Consistent padding improvement.The bottom padding addition improves the layout spacing and prevents the button row from being too close to the screen edge, which is consistent with similar UI improvements across related screens.
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountScreen.kt (1)
106-145: Overlay loader integration looks correctUsing
MifosProgressIndicatorOverlay()underif (state.isOverlayLoading)at the same scaffold level as the main content is a good pattern; in a Box-based scaffold it will render above the content without changing existing screenState-based loading/error handling.feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/CreateFixedDepositAccountViewmodel.kt (1)
92-99: Overlay loading state management for fixed deposits looks consistent
DataState.Loadingnow flipsisOverlayLoading = true, while both success and error paths clear it and set an appropriatescreenState. This gives you a hard “initial load” viaScreenState.Loadingand softer overlay for subsequent template fetches, which is a reasonable UX split.Also applies to: 103-116, 268-278
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountViewModel.kt (2)
264-291: Overlay loading wiring for product-specific template loading is solid
loadRecurringAccountTemplateWithProductcleanly togglesisOverlayLoadingonDataState.Loadingand resets it on both success and error, while also updatingscreenState. Combined withRecurringAccountState.isOverlayLoadingand the overlay composable inRecurringAccountScreen, this should give a responsive UX for product changes without regressing the base template load behavior.Also applies to: 293-327, 692-711
329-367: Action hierarchy and state modeling are well-structuredThe split into
RecurringAccountDetailsAction,RecurringAccountSettingsAction, andRecurringAccountInterestChartAction, plus the newOnDetailNext/OnSettingNextactions andScreenStatesealed type, makes the flow much clearer and easier to validate per step. Combined with the new error fields (productError,depositAmountError, etc.), this is a good structural improvement.Also applies to: 811-866
| if (!state.template.fieldOfficerOptions.isNullOrEmpty()) { | ||
| MifosDatePickerTextField( | ||
| value = state.recurringDepositAccountDetail.submissionDate, | ||
| label = stringResource(Res.string.feature_recurring_deposit_submitted_on) + "*", | ||
| openDatePicker = { | ||
| onAction( | ||
| RecurringAccountAction.RecurringAccountDetailsAction.OnSubmissionDatePick( | ||
| true, | ||
| ), | ||
| ) | ||
| }, | ||
| ) | ||
|
|
||
| Spacer(Modifier.height(DesignToken.padding.large)) | ||
| MifosTextFieldDropdown( | ||
| value = if (state.recurringDepositAccountDetail.fieldOfficerIndex == -1) { | ||
| "" | ||
| } else { | ||
| state.template.fieldOfficerOptions | ||
| ?.get(state.recurringDepositAccountDetail.fieldOfficerIndex)?.displayName | ||
| ?: "" | ||
| }, | ||
| onValueChanged = {}, | ||
| onOptionSelected = { index, value -> | ||
| onAction( | ||
| RecurringAccountAction.RecurringAccountDetailsAction.OnFieldOfficerChange( | ||
| index, | ||
| ), | ||
| ) | ||
| }, | ||
| options = state.template.fieldOfficerOptions?.map { | ||
| it.displayName ?: "" | ||
| } ?: emptyList(), | ||
| label = stringResource(Res.string.feature_recurring_deposit_field_officer) + "*", | ||
| errorMessage = state.recurringDepositAccountDetail.fieldOfficerError?.let { stringResource(it) }, | ||
| ) | ||
|
|
||
| if (state.recurringDepositAccountDetail.isMiniLoaderActive) { | ||
| MifosProgressIndicatorMini() | ||
| MifosOutlinedTextField( | ||
| value = state.recurringDepositAccountDetail.externalId, | ||
| onValueChange = { | ||
| onAction( | ||
| RecurringAccountAction.RecurringAccountDetailsAction.OnExternalIdChange( | ||
| it, | ||
| ), | ||
| ) | ||
| }, | ||
| label = stringResource(Res.string.feature_recurring_deposit_external_id), | ||
| config = MifosTextFieldConfig( | ||
| keyboardOptions = KeyboardOptions( | ||
| keyboardType = KeyboardType.Text, | ||
| ), | ||
| ), | ||
| ) | ||
| Spacer(Modifier.height(DesignToken.padding.large)) | ||
| } |
There was a problem hiding this comment.
Align field officer validation with conditional UI
Here the field officer dropdown (and even the date picker/external ID block) is only rendered when state.template.fieldOfficerOptions is not null or empty. However, in RecurringAccountViewModel, OnDetailNext currently always validates:
val fieldOfficerError = TextFieldsValidator.dropDownEmptyValidator(
state.recurringDepositAccountDetail.fieldOfficerIndex == -1,
)If a template comes back with no field officer options, the UI won’t show a field officer field but the validation will still fail, blocking navigation.
Consider making the field-officer validation conditional on there actually being options, e.g. skip it when fieldOfficerOptions.isNullOrEmpty(), or always render the field when it’s required.
| value = if (settingsState.lockInPeriod.frequencyTypeIndex != -1) { | ||
| state.template.lockinPeriodFrequencyTypeOptions | ||
| ?.getOrNull(settingsState.lockInPeriod.frequencyTypeIndex)?.value.orEmpty() | ||
| } else { | ||
| "" | ||
| }, | ||
| options = state.template.lockinPeriodFrequencyTypeOptions?.map { | ||
| it.value.orEmpty() | ||
| } ?: emptyList(), | ||
| onValueChanged = {}, | ||
| onOptionSelected = { id, name -> | ||
| onAction( | ||
| RecurringAccountAction.RecurringAccountSettingsAction.SetLockInPeriodType(id), | ||
| ) | ||
| }, | ||
| label = stringResource(Res.string.feature_recurring_deposit_type), | ||
| ) |
There was a problem hiding this comment.
Fix mismatch between dropdown indices and stored IDs in settings state
Multiple dropdowns here treat the stored value as a list index:
value = if (settingsState.lockInPeriod.frequencyTypeIndex != -1) {
state.template.lockinPeriodFrequencyTypeOptions
?.getOrNull(settingsState.lockInPeriod.frequencyTypeIndex)?.value.orEmpty()
} else ""But in RecurringAccountViewModel you set these fields to option IDs rather than indices, for example:
lockInPeriod = lockInPeriod.copy(
frequencyTypeIndex = state.template.lockinPeriodFrequencyTypeOptions
?.get(action.frequencyTypeIndex)?.id ?: -1,
)The same pattern appears for:
depositPeriod.periodTypevsperiodFrequencyTypeOptionsminimumDepositTerm.frequencyTypeIndexandfrequencyTypeIndexAfterInMultiplesOfmaxDepositTerm.frequencyTypeIndexpreMatureClosure.interestPeriodIndex
Using IDs as indices means:
- The displayed label may not correspond to the selected ID.
- If IDs are not zero-based and contiguous,
getOrNull(id)can select the wrong item or nothing at all.
You probably want to either:
- Store the dropdown index in state and only translate to
.idwhen building the payload, or - Keep storing IDs, but derive the label as
options.firstOrNull { it.id == storedId }?.value.orEmpty()instead of indexing into the list.
I’d fix this before relying on these values for UX or payloads.
Also applies to: 205-227, 260-278, 305-323, 350-368, 405-424
🤖 Prompt for AI Agents
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/SettingsPage.kt
lines 132-148 (also apply same fix to 205-227, 260-278, 305-323, 350-368,
405-424): the UI is treating stored IDs as list indices when rendering dropdown
labels; instead of using getOrNull(storedId) use options.firstOrNull { it.id ==
storedId }?.value.orEmpty() to derive the displayed label (or change state to
store the index consistently), and keep options = state.template...map {
it.value.orEmpty() } as-is and ensure onOptionSelected continues to dispatch the
selected option ID; update all listed ranges to match this approach so labels
reflect the selected IDs correctly.
| RecurringAccountAction.RecurringAccountSettingsAction.OnSettingNext -> { | ||
| val depositAmountError = | ||
| TextFieldsValidator.stringValidator(state.recurringDepositAccountSettings.recurringDepositDetails.depositAmount) | ||
| val depositPeriodTypeError = TextFieldsValidator.dropDownEmptyValidator( | ||
| state.recurringDepositAccountSettings.depositPeriod.periodType == -1, | ||
| ) | ||
| val depositPeriodError = | ||
| TextFieldsValidator.stringValidator(state.recurringDepositAccountSettings.depositPeriod.period) | ||
|
|
||
| if (depositAmountError != null || depositPeriodTypeError != null || depositPeriodError != null) { | ||
| mutableStateFlow.update { | ||
| it.copy( | ||
| recurringDepositAccountSettings = it.recurringDepositAccountSettings.copy( | ||
| depositPeriodError = depositPeriodError, | ||
| depositAmountError = depositAmountError, | ||
| depositPeriodTypeError = depositPeriodTypeError, | ||
| ), | ||
| ) | ||
| } | ||
| } else { | ||
| moveToNextStep() | ||
| } | ||
| } |
There was a problem hiding this comment.
Good addition of step-level validation; refine conditions for nullable fields
The new validation branches are a solid improvement:
- Settings step (
OnSettingNext) now enforces non-empty deposit amount, deposit period, and a selected period type. - Details step (
OnDetailNext) enforces product selection and field officer selection viadropDownEmptyValidator.
Two refinements to consider:
-
Field officer optionality
OnDetailNextalways validatesfieldOfficerIndex == -1, but the UI only shows the field officer dropdown whentemplate.fieldOfficerOptionsis not empty. If that list is empty, the user will be blocked from progressing even though they were never shown a choice.You can align this with the UI by making the field officer check conditional:
val productError = TextFieldsValidator.dropDownEmptyValidator( state.recurringDepositAccountDetail.loanProductSelected == -1, ) val fieldOfficerError = if (!state.template.fieldOfficerOptions.isNullOrEmpty()) { TextFieldsValidator.dropDownEmptyValidator( state.recurringDepositAccountDetail.fieldOfficerIndex == -1, ) } else { null }
-
Numeric validation
For
depositAmountErroranddepositPeriodErroryou’re usingstringValidator, which only checks for emptiness and invalid characters. If these fields must be numeric and > 0, you may want to usenumberValidator/doubleNumberValidatorinstead to enforce that more strictly. That’s optional but would give more precise error feedback.
Overall the pattern is good; just tighten the conditions to match what the screens actually present.
Also applies to: 625-646
🤖 Prompt for AI Agents
In
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountViewModel.kt
around lines 577-599 and also 625-646, the validation currently unconditionally
treats some fields as required and uses stringValidator for numeric fields;
update the logic so the field-officer dropdown is only validated when
template.fieldOfficerOptions is not null or empty (i.e. skip/return null for
fieldOfficerError if no options are present), and replace stringValidator for
depositAmount and depositPeriod with the appropriate numeric validators
(numberValidator or doubleNumberValidator) if those fields must be numeric and >
0, then wire those errors into the same mutableStateFlow update branch as the
other validators.
2 participants
Fixes - Jira-#551
Summary by CodeRabbit
Release Notes
New Features
UI/UX Improvements
✏️ Tip: You can customize this high-level summary in your review settings.