New Fixed Deposits Account Settings Step #2555
Conversation
WalkthroughThis PR extends the Fixed Deposit and Recurring Deposit account creation flows with enhanced state management, new form fields, and network-aware validation. Changes include expanding the FixedDepositTemplate data model, refactoring CreateFixedDepositAccountViewmodel to use comprehensive state-driven actions with network monitoring, revamping multiple Composable pages to bind to expanded state, adding localization strings, and applying consistent spacing adjustments across multiple feature modules. Changes
Sequence DiagramsequenceDiagram
participant User
participant CreateFixedDepositAccountScreen
participant CreateFixedDepositAccountViewmodel
participant NetworkMonitor
participant FixedDepositRepository
User->>CreateFixedDepositAccountScreen: Open Form
CreateFixedDepositAccountScreen->>CreateFixedDepositAccountViewmodel: init()
CreateFixedDepositAccountViewmodel->>NetworkMonitor: Check Network
alt Network Online
CreateFixedDepositAccountViewmodel->>FixedDepositRepository: loadFixedDepositTemplate()
FixedDepositRepository-->>CreateFixedDepositAccountViewmodel: Template Data (Success)
CreateFixedDepositAccountViewmodel->>CreateFixedDepositAccountViewmodel: Update State
else Network Offline
CreateFixedDepositAccountViewmodel->>CreateFixedDepositAccountViewmodel: Show Network Error
end
rect rgb(220, 240, 255)
Note over User,FixedDepositRepository: Details Page
User->>CreateFixedDepositAccountScreen: Select Product & Proceed
CreateFixedDepositAccountScreen->>CreateFixedDepositAccountViewmodel: OnDetailNext
CreateFixedDepositAccountViewmodel->>CreateFixedDepositAccountViewmodel: Validate Product
alt Validation Pass
CreateFixedDepositAccountViewmodel->>NetworkMonitor: Check Network
CreateFixedDepositAccountViewmodel->>FixedDepositRepository: loadRecurringAccountTemplateWithProduct()
CreateFixedDepositAccountViewmodel->>CreateFixedDepositAccountViewmodel: moveToNextStep()
else Validation Fail
CreateFixedDepositAccountViewmodel->>CreateFixedDepositAccountViewmodel: Set productError
end
end
rect rgb(240, 255, 240)
Note over User,FixedDepositRepository: Terms Page
User->>CreateFixedDepositAccountScreen: Enter Deposit Terms & Proceed
CreateFixedDepositAccountScreen->>CreateFixedDepositAccountViewmodel: OnTermNext
CreateFixedDepositAccountViewmodel->>CreateFixedDepositAccountViewmodel: Validate Terms (Amount, Period, Type)
alt Validation Pass
CreateFixedDepositAccountViewmodel->>CreateFixedDepositAccountViewmodel: moveToNextStep()
else Validation Fail
CreateFixedDepositAccountViewmodel->>CreateFixedDepositAccountViewmodel: Set Error Fields
end
end
rect rgb(255, 245, 230)
Note over User,CreateFixedDepositAccountViewmodel: Settings Page
User->>CreateFixedDepositAccountScreen: Configure Lock-in, Multiples, Maturity, Penal Interest
CreateFixedDepositAccountScreen->>CreateFixedDepositAccountViewmodel: OnLockInPeriodChange / OnMaturityInstructionChange / etc.
CreateFixedDepositAccountViewmodel->>CreateFixedDepositAccountViewmodel: Update State Fields
end
User->>CreateFixedDepositAccountScreen: Submit
CreateFixedDepositAccountViewmodel->>CreateFixedDepositAccountViewmodel: Show isOverlayLoading
CreateFixedDepositAccountScreen->>CreateFixedDepositAccountScreen: Render MifosProgressIndicatorOverlay
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/pages/ChargesPage.kt (1)
90-124: Overdue charges button enablement is tied to the wrong stateIn the overdue charges section:
if (state.loanTemplate?.overdueCharges?.isNotEmpty() ?: false) { ... btnEnabled = state.addedCharges.isNotEmpty(), }The "View" button for overdue charges is enabled based on
state.addedCharges, not on the presence of overdue charges. This means users may be blocked from viewing overdue charges when there are none inaddedChargesbut some inoverdueCharges.Recommend enabling the button based on the same condition as the surrounding
if:- btnEnabled = state.addedCharges.isNotEmpty(), + btnEnabled = state.loanTemplate?.overdueCharges?.isNotEmpty() ?: false,The added Spacer below this block is fine and keeps the content visually separated from the bottom buttons.
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountViewModel.kt (2)
781-795: Potential null pointer exception when adding charge to list.The force-unwrap
state.chooseChargeIndex!!on line 783 will throw ifchooseChargeIndexis null. Consider adding a null check or usingletto safely handle this case.RecurringAccountAction.AddChargeToList -> { + val chargeIndex = state.chooseChargeIndex ?: return val createdData = ChargeItem( - chargeId = state.template.chargeOptions?.get(state.chooseChargeIndex!!)?.id, + chargeId = state.template.chargeOptions?.get(chargeIndex)?.id, amount = state.chargeAmount.toDoubleOrNull(), ) mutableStateFlow.update { it.copy( addedCharges = it.addedCharges + createdData, chooseChargeIndex = null, dialogState = null, chargeAmount = "", ) } }
714-730: EditCharge uses action.index incorrectly for both charge lookup and list update.On line 716,
action.indexis used to get the charge fromchargeOptions, but on line 721, it's used as the index intoaddedCharges. These are different lists with potentially different indices. The index fromaction.indexshould correspond to the position inaddedCharges, notchargeOptions.The charge ID should come from the currently selected
chooseChargeIndex(which was set inEditChargeDialog), not fromaction.index:is RecurringAccountAction.EditCharge -> { + val chargeIndex = state.chooseChargeIndex ?: return val createdData = ChargeItem( - chargeId = state.template.chargeOptions?.get(action.index)?.id, + chargeId = state.template.chargeOptions?.get(chargeIndex)?.id, amount = state.chargeAmount.toDoubleOrNull(), ) val currentAddedCharges = state.addedCharges.toMutableList() currentAddedCharges[action.index] = createdData
🧹 Nitpick comments (4)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/pages/ChargesPage.kt (1)
91-113: Double vertical spacing before bottom buttonsYou now have both:
Spacer(Modifier.height(DesignToken.padding.large))afterMifosRowWithTextAndButton, andMifosTwoButtonRow(..., modifier = Modifier.padding(top = DesignToken.padding.small)).This will give more space above the buttons than similar pages that only use a Spacer or only top padding. If you’re aiming for consistent rhythm with the other Charges/Preview pages in this PR, consider dropping either the Spacer or the top padding (likely the top padding).
core/model/src/commonMain/kotlin/com/mifos/core/model/objects/template/recurring/MaturityInstructionOption.kt (1)
14-19: LGTM!The data class is well-structured with nullable properties and appropriate defaults for safe deserialization. The
@Serializableannotation enables kotlinx.serialization support.Consider whether the package location (
template.recurring) is semantically correct, given this class is also used byFixedDepositTemplate. If this is a shared template option, a more generic package liketemplate.commonmight be more appropriate.feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/pages/SettingsPage.kt (1)
83-99: Use safe list access to prevent potentialIndexOutOfBoundsException.Direct list access via
get(index)can throw an exception if the index is invalid. Consider usinggetOrNullfor defensive coding, similar to how it's done in other pages.MifosTextFieldDropdown( value = if (state.lockInPeriodTypeIndex != -1) { - state.template.lockinPeriodFrequencyTypeOptions?.get(state.lockInPeriodTypeIndex)?.value.orEmpty() + state.template.lockinPeriodFrequencyTypeOptions?.getOrNull(state.lockInPeriodTypeIndex)?.value.orEmpty() } else { "" },Apply the same pattern to other dropdown value bindings at lines 126, 162, 201, 241, and 298.
feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/pages/TermsPage.kt (1)
110-129: Consider adding spacing between deposit period type dropdown and the next section.The dropdown for deposit period type has
Spacer(Modifier.height(DesignToken.padding.small))after it (line 130), while other fields useDesignToken.padding.large. This creates inconsistent spacing before the "Interest Compounding" section header.label = stringResource(Res.string.feature_fixed_deposit_deposit_period_type) + "*", errorMessage = state.fixedDepositAccountTerms.depositPeriodTypeError?.let { stringResource(it) }, ) - Spacer(Modifier.height(DesignToken.padding.small)) + Spacer(Modifier.height(DesignToken.padding.large)) Text( text = stringResource(Res.string.feature_fixed_interest_compounding),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
core/data/src/commonMain/kotlin/com/mifos/core/data/repository/FixedDepositRepository.kt(1 hunks)core/model/src/commonMain/kotlin/com/mifos/core/model/objects/template/recurring/MaturityInstructionOption.kt(1 hunks)core/network/src/commonMain/kotlin/com/mifos/core/network/model/FixedDepositTemplate.kt(2 hunks)feature/client/src/commonMain/composeResources/values/strings.xml(1 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/pages/ChargesPage.kt(1 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/pages/DetailsPage.kt(0 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/pages/PreviewPage.kt(2 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/pages/TermsPage.kt(2 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/CreateFixedDepositAccountScreen.kt(6 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/CreateFixedDepositAccountViewmodel.kt(10 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/pages/DetailsPage.kt(3 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/pages/SettingsPage.kt(1 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/pages/TermsPage.kt(2 hunks)feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/pages/ChargesPage.kt(1 hunks)feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/pages/PreviewPage.kt(2 hunks)feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/pages/SchedulePage.kt(1 hunks)feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/pages/TermsPage.kt(1 hunks)feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountViewModel.kt(7 hunks)feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/ChargesPage.kt(1 hunks)feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/DetailsPage.kt(1 hunks)feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/InterestPage.kt(1 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(0 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(0 hunks)
💤 Files with no reviewable changes (3)
- feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/pages/DetailsPage.kt
- feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountv2/pages/DetailsPage.kt
- feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountv2/pages/TermsPage.kt
🧰 Additional context used
🧬 Code graph analysis (5)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/pages/SettingsPage.kt (3)
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/MifosCheckBox.kt (1)
MifosCheckBox(24-46)core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosTwoButtonRow.kt (1)
MifosTwoButtonRow(31-91)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/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/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/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 (1)
MifosDatePickerTextField(363-413)core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosTwoButtonRow.kt (1)
MifosTwoButtonRow(31-91)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/CreateFixedDepositAccountScreen.kt (2)
core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosErrorComponent.kt (1)
MifosErrorComponent(41-60)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/RecurringAccountViewModel.kt (1)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/CreateFixedDepositAccountViewmodel.kt (1)
loadRecurringAccountTemplateWithProduct(279-321)
⏰ 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 (36)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/pages/PreviewPage.kt (2)
33-36: Imports for Spacer/height are correct and minimal
Spacerandheightare both used exactly once and keep the imports set focused and consistent with Compose usage. No issues here.
82-93: Additional Spacer after charges row is reasonablePlacing
Spacer(Modifier.height(DesignToken.padding.large))inside the scrollable content column cleanly separates the charges section from the bottom action row without affecting logic or state. This aligns with token-based spacing and the broader layout pattern.feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/pages/SchedulePage.kt (1)
68-75: SchedulePage spacing addition looks goodThe extra Spacer after
RepaymentScheduleListkeeps content visually separated from the bottom action row and is consistent with the rest of the flow. No functional impact.feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountv2/pages/ChargesPage.kt (1)
89-99: Savings ChargesPage spacer is consistent and safeThe added Spacer after the charges summary cleanly separates the list summary from the bottom action area and matches the spacing approach seen elsewhere in this PR. No issues.
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/pages/TermsPage.kt (1)
679-699: Collateral section spacing is appropriateThe Spacer after the collateral
MifosRowWithTextAndButtonadds clear separation from the bottom action row and aligns with the spacing approach elsewhere in the loan flow. Looks good.feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/ChargesPage.kt (1)
104-118: Recurring deposit ChargesPage spacing is consistentThe new Spacer after the charges summary row cleanly separates content from the bottom buttons and matches the pattern used in other account flows. No issues.
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/pages/PreviewPage.kt (1)
54-58: PreviewPage layout spacing change is soundAdding
Spacer(Modifier.height(DesignToken.padding.large))after the chargesMifosRowWithTextAndButtonand relying on that instead of top padding onMifosTwoButtonRowgives a clear and consistent visual gap between content and actions. No behavioral concerns.Also applies to: 188-201
feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountv2/pages/PreviewPage.kt (1)
172-197: Savings PreviewPage charges spacing looks correctThe added Spacer after the charges row provides consistent vertical separation from the bottom action row while preserving existing behavior. All good.
feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/pages/TermsPage.kt (2)
227-228: LGTM!The added spacing before the lock-in period section improves visual separation between form sections.
268-268: LGTM!Adding large spacing at the end of the scrollable content ensures consistent bottom padding before the button row.
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/DetailsPage.kt (1)
176-176: LGTM!Removing the required indicator (
*) from the field officer label aligns with the updated validation logic where field officer is no longer a required field.feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/InterestPage.kt (1)
101-101: LGTM!Adding the spacer at the end of the scrollable content maintains consistent spacing before the button row, aligning with the layout pattern used in other pages.
feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/CreateFixedDepositAccountScreen.kt (3)
56-61: LGTM!Clean refactoring from
newFixedDepositAccountStatetostateimproves readability and aligns with common naming conventions for state-driven Compose components.
86-91: LGTM!Properly wiring
SettingPagewith bothstateandonActionenables state-driven UI and action dispatching, consistent with the updated component signature.
137-139: LGTM!The progress overlay provides clear visual feedback during async operations. Placing it at the end of the scaffold content ensures it renders above other content when active.
feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/pages/SettingsPage.kt (3)
54-67: LGTM!The refactored signature to accept
stateandonActionaligns with the state-driven architecture used across other page components in this flow. The layout structure with scrollable content and fixed button row is well-organized.
274-316: LGTM!Good use of
AnimatedVisibilityto conditionally show penal interest fields. This provides a smooth UX when the user toggles the checkbox.
319-324: LGTM!The two-button row correctly dispatches
PreviousStepandOnNextPressactions for step navigation.core/network/src/commonMain/kotlin/com/mifos/core/network/model/FixedDepositTemplate.kt (1)
57-58: Add@SerialNameannotations for consistency and safety.The new properties lack
@SerialNameannotations, unlike other properties in this class. This could cause deserialization failures if the API JSON keys differ from the Kotlin property names (e.g., snake_case vs camelCase).core/data/src/commonMain/kotlin/com/mifos/core/data/repository/FixedDepositRepository.kt (1)
17-20: Verify all implementations handle the optionalproductIdparameter correctly.Adding a default value to an interface parameter requires updating all implementations. Ensure the implementation(s) of
FixedDepositRepositorycorrectly handlenullvalues forproductIdwhen constructing API requests. Search the codebase for all classes implementing this interface and verify theirgetFixedDepositTemplatemethod properly handles the null case.feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/pages/TermsPage.kt (4)
54-57: Good layout structure with weighted scrollable content.The two-tiered column approach correctly separates the scrollable content from the fixed bottom navigation. The outer modifier is applied correctly to the inner column.
63-88: Deposit amount field with proper validation and currency prefix.The field correctly displays the currency symbol prefix, required indicator, and error handling. The error text is properly resolved via
stringResource.
136-212: Interest compounding dropdowns are consistently implemented.The pattern of checking index != -1 before accessing options and falling back to empty string is correct. The
onValueChanged = {}is appropriate since these are read-only dropdowns.
214-219: Navigation buttons correctly wired to new action names.The Back button triggers
PreviousStepand Next button triggersOnTermNext, aligning with the updated action structure in the ViewModel.feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/pages/DetailsPage.kt (4)
98-105: Consistent layout structure with TermsPage.The two-tiered column approach with weighted scrollable content and fixed bottom navigation matches the pattern established in TermsPage.
107-123: Product dropdown correctly bound to state with error handling.The dropdown properly displays product options and handles product selection errors.
125-169: Verify: Submission date and external ID hidden when no field officers available.The conditional block
if (!state.template.fieldOfficerOptions.isNullOrEmpty())hides the submission date picker, field officer dropdown, and external ID field whenfieldOfficerOptionsis null or empty. This means these fields won't be visible until a product is selected and the product-specific template is loaded.Is this the intended UX? The submission date might be needed regardless of field officer availability.
171-177: Navigation properly wired with OnDetailNext action.The second button correctly triggers
OnDetailNextfor validation before proceeding.feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountViewModel.kt (3)
159-168: Conditional template loading when field officers not available.The logic to reload template with product only when
fieldOfficerOptionsis null is consistent with the Fixed Deposit flow pattern.
630-646: OnDetailNext validates only product selection.The removal of
fieldOfficerErrorvalidation simplifies the flow, requiring only product selection to proceed.
823-828: DialogState sealed interface correctly extended.The new
ShowChargesstate is properly added alongside existing dialog states.feature/client/src/commonMain/composeResources/values/strings.xml (1)
608-622: New Fixed Deposit settings strings added with consistent naming.The string resources follow the established naming convention and provide appropriate labels for the settings UI. The
%%escape for the percent sign in "Penal Interest (%%)" is correct for Android/Compose resources.feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/CreateFixedDepositAccountViewmodel.kt (4)
237-276: Network-aware template loading with proper error handling.The implementation correctly checks network availability before making API calls and provides user-friendly error messages when offline.
96-133: Validation logic for OnDetailNext and OnTermNext is well-structured.The validation properly checks required fields and updates error state before allowing navigation to proceed.
139-229: New settings state update handlers are consistent.All the new action handlers for settings fields follow the same pattern and correctly update state.
487-503: New settings state fields added to support the Settings step.The additional state fields for lock-in period, deposit terms, maturity instructions, and penal interest settings are properly typed and initialized with sensible defaults.
| private fun loadRecurringAccountTemplateWithProduct() = viewModelScope.launch { | ||
| if (networkMonitor.isOnline.first()) { | ||
| fixedDepositRepository.getFixedDepositTemplate( | ||
| clientId = state.clientId, | ||
| productId = state.template.productOptions?.get(state.fixedDepositAccountDetail.productSelected)?.id, | ||
| ).collect { state -> | ||
| when (state) { | ||
| is DataState.Success -> { | ||
| mutableStateFlow.update { | ||
| it.copy( | ||
| screenState = NewFixedDepositAccountState.ScreenState.Success, | ||
| isOverlayLoading = false, | ||
| template = state.data, | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| is DataState.Error -> { | ||
| mutableStateFlow.update { | ||
| it.copy( | ||
| screenState = NewFixedDepositAccountState.ScreenState.Error(state.message), | ||
| isOverlayLoading = false, | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| DataState.Loading -> { | ||
| mutableStateFlow.update { | ||
| it.copy( | ||
| isOverlayLoading = true, | ||
| ) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } else { | ||
| mutableStateFlow.update { | ||
| it.copy( | ||
| screenState = NewFixedDepositAccountState.ScreenState.Error(getString(Res.string.feature_client_error_network_not_available)), | ||
| ) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Function name doesn't match its purpose.
The function is named loadRecurringAccountTemplateWithProduct but it loads a Fixed Deposit template using fixedDepositRepository.getFixedDepositTemplate(). This appears to be a copy-paste issue from the RecurringAccountViewModel.
- private fun loadRecurringAccountTemplateWithProduct() = viewModelScope.launch {
+ private fun loadFixedDepositTemplateWithProduct() = viewModelScope.launch {Also update the call site on line 357:
if (state.template.fieldOfficerOptions == null) {
- loadRecurringAccountTemplateWithProduct()
+ loadFixedDepositTemplateWithProduct()
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/CreateFixedDepositAccountViewmodel.kt
around lines 279-321, the function is misnamed
`loadRecurringAccountTemplateWithProduct` though it loads a Fixed Deposit
template; rename the function to something accurate (e.g.,
`loadFixedDepositTemplateWithProduct`) and update its declaration and all call
sites (including the call on line 357) to the new name so the name matches its
behavior and avoids copy-paste confusion.
| private fun moveToPreviousStep() { | ||
| val current = state.currentStep | ||
| mutableStateFlow.update { | ||
| it.copy( | ||
| fixedDepositAccountDetail = it.fixedDepositAccountDetail.copy( | ||
| externalId = action.value, | ||
| ), | ||
| currentStep = current - 1, | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing bounds check in moveToPreviousStep.
The function decrements currentStep without checking if it's already at 0, which could result in a negative step index.
private fun moveToPreviousStep() {
val current = state.currentStep
+ if (current <= 0) return
mutableStateFlow.update {
it.copy(
currentStep = current - 1,
)
}
}🤖 Prompt for AI Agents
In
feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/CreateFixedDepositAccountViewmodel.kt
around lines 371 to 378, moveToPreviousStep() decrements currentStep without
bounds checking which can produce a negative index; modify the function to check
currentStep > 0 (or clamp to 0) before decrementing — either return early if
currentStep is 0 or set currentStep = maxOf(0, current - 1) when updating
mutableStateFlow so the step never becomes negative.
| sealed class NewFixedDepositAccountTermsAction : NewFixedDepositAccountAction() { | ||
| data class SetFixedDepositAmount(val depositAmount: String) : NewFixedDepositAccountAction() | ||
| data class SetFixedDepositPeriod(val period: String) : NewFixedDepositAccountAction() | ||
| data class SetFixedDepositPeriodType(val depositPeriodTypeIndex: Int) : NewFixedDepositAccountAction() | ||
| data class SetInterestCompoundingPeriod(val interestCompoundingPeriodTypeIndex: Int) : NewFixedDepositAccountAction() | ||
| data class SetInterestPostingPeriod(val interestPostingPeriodTypeIndex: Int) : NewFixedDepositAccountAction() | ||
| data class SetInterestCalculationType(val interestCalculationPeriodTypeIndex: Int) : NewFixedDepositAccountAction() | ||
| data class SetInterestCalculationDaysInYearType(val periodTypeIndex: Int) : NewFixedDepositAccountAction() | ||
| data class SetFixedDepositPeriodType(val depositPeriodTypeIndex: Int) : | ||
| NewFixedDepositAccountAction() | ||
|
|
||
| data class SetInterestCompoundingPeriod(val interestCompoundingPeriodTypeIndex: Int) : | ||
| NewFixedDepositAccountAction() | ||
|
|
||
| data class SetInterestPostingPeriod(val interestPostingPeriodTypeIndex: Int) : | ||
| NewFixedDepositAccountAction() | ||
|
|
||
| data class SetInterestCalculationType(val interestCalculationPeriodTypeIndex: Int) : | ||
| NewFixedDepositAccountAction() | ||
|
|
||
| data class SetInterestCalculationDaysInYearType(val periodTypeIndex: Int) : | ||
| NewFixedDepositAccountAction() | ||
| } |
There was a problem hiding this comment.
Nested action classes extend wrong parent type.
The action classes inside NewFixedDepositAccountTermsAction sealed class extend NewFixedDepositAccountAction() directly instead of NewFixedDepositAccountTermsAction(). This breaks the sealed class hierarchy and means these actions won't match is NewFixedDepositAccountAction.NewFixedDepositAccountTermsAction patterns correctly.
sealed class NewFixedDepositAccountTermsAction : NewFixedDepositAccountAction() {
- data class SetFixedDepositAmount(val depositAmount: String) : NewFixedDepositAccountAction()
- data class SetFixedDepositPeriod(val period: String) : NewFixedDepositAccountAction()
- data class SetFixedDepositPeriodType(val depositPeriodTypeIndex: Int) :
- NewFixedDepositAccountAction()
-
- data class SetInterestCompoundingPeriod(val interestCompoundingPeriodTypeIndex: Int) :
- NewFixedDepositAccountAction()
-
- data class SetInterestPostingPeriod(val interestPostingPeriodTypeIndex: Int) :
- NewFixedDepositAccountAction()
-
- data class SetInterestCalculationType(val interestCalculationPeriodTypeIndex: Int) :
- NewFixedDepositAccountAction()
-
- data class SetInterestCalculationDaysInYearType(val periodTypeIndex: Int) :
- NewFixedDepositAccountAction()
+ data class SetFixedDepositAmount(val depositAmount: String) : NewFixedDepositAccountTermsAction()
+ data class SetFixedDepositPeriod(val period: String) : NewFixedDepositAccountTermsAction()
+ data class SetFixedDepositPeriodType(val depositPeriodTypeIndex: Int) :
+ NewFixedDepositAccountTermsAction()
+
+ data class SetInterestCompoundingPeriod(val interestCompoundingPeriodTypeIndex: Int) :
+ NewFixedDepositAccountTermsAction()
+
+ data class SetInterestPostingPeriod(val interestPostingPeriodTypeIndex: Int) :
+ NewFixedDepositAccountTermsAction()
+
+ data class SetInterestCalculationType(val interestCalculationPeriodTypeIndex: Int) :
+ NewFixedDepositAccountTermsAction()
+
+ data class SetInterestCalculationDaysInYearType(val periodTypeIndex: Int) :
+ NewFixedDepositAccountTermsAction()
}🤖 Prompt for AI Agents
In
feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/CreateFixedDepositAccountViewmodel.kt
around lines 574-591, the nested action data classes inside the sealed class
NewFixedDepositAccountTermsAction mistakenly extend NewFixedDepositAccountAction
instead of extending NewFixedDepositAccountTermsAction; update each nested data
class declaration so it extends NewFixedDepositAccountTermsAction() (i.e.,
replace NewFixedDepositAccountAction() with NewFixedDepositAccountTermsAction())
ensuring the sealed hierarchy is correct and pattern matching against
NewFixedDepositAccountAction.NewFixedDepositAccountTermsAction works as
intended.
| MifosOutlinedTextField( | ||
| value = state.minimumDispositTermFrequency, | ||
| onValueChange = { | ||
| onAction( | ||
| NewFixedDepositAccountAction.OnMinimumDepositTermFrequencyChange(it), | ||
| ) | ||
| }, | ||
| label = stringResource(Res.string.feature_fixed_deposit_setting_thereafter_in_multiples), | ||
| config = MifosTextFieldConfig( | ||
| keyboardOptions = KeyboardOptions( | ||
| keyboardType = KeyboardType.Number, | ||
| imeAction = ImeAction.Next, | ||
| ), | ||
| ), | ||
| ) |
There was a problem hiding this comment.
Possible label mismatch for minimum deposit term frequency field.
The field uses feature_fixed_deposit_setting_thereafter_in_multiples as the label (line 115), but this appears to be the "Minimum Deposit Term" frequency input based on the section header (line 103) and the action OnMinimumDepositTermFrequencyChange.
Verify this is intentional or consider using a more appropriate label like feature_fixed_deposit_setting_frequency.
MifosOutlinedTextField(
value = state.minimumDispositTermFrequency,
onValueChange = {
onAction(
NewFixedDepositAccountAction.OnMinimumDepositTermFrequencyChange(it),
)
},
- label = stringResource(Res.string.feature_fixed_deposit_setting_thereafter_in_multiples),
+ label = stringResource(Res.string.feature_fixed_deposit_setting_frequency),
config = MifosTextFieldConfig(Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/pages/SettingsPage.kt
around lines 108 to 122, the TextField for minimum deposit term frequency uses
the label resource feature_fixed_deposit_setting_thereafter_in_multiples which
likely mismatches the field purpose; update the label to the correct resource
(e.g., feature_fixed_deposit_setting_frequency or another appropriate string) so
it matches the OnMinimumDepositTermFrequencyChange action and section header,
and ensure the chosen string resource exists and is used consistently across the
form.
2 participants
Fixes - Jira-#559
Summary by CodeRabbit
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.