MIFOSAC-561 Fixed deposit charges step #2558
Conversation
This commit introduces the UI and state management for the "Charges" step in the new fixed deposit account creation process. Key changes include: - Adding `FixedDepositAccountChargesState` to the main state to manage charge-related data. - Implementing new actions and handler functions in `CreateFixedDepositAccountViewmodel` for adding, editing, deleting, and viewing charges. - Developing the `ChargesPage.kt` composable, which includes UI for selecting charges, adding new ones via a bottom sheet, and viewing a list of currently added charges. - Integrating the new `ChargesPage` into the multi-step wizard in `CreateFixedDepositAccountScreen`.
…account flow Integrates the `chargeOptions` from the account template into the "Charges" step of the new fixed deposit account creation process. This change removes the previously used sample charge data. Key changes: - Fetches charge options directly from the `state.template.chargeOptions`. - Simplifies the `handleAddCharge` logic by removing handling for an editing state, as editing now has its own distinct flow. - Refactors `handleEditCharge` to correctly populate the edit dialog with existing charge data and update the charge upon confirmation. - Removes the now-redundant `getSampleChargeOptions` and `ChargeOption` data class. - Adjusts the `FixedDepositAccountChargesState` by making `selectedChargeIndex` nullable. - The UI in `ChargesPage` is updated to remove the standalone "Choose Charge" dropdown, as charge selection now happens within the "Add Charge" bottom sheet.
WalkthroughAdds charge management to the Fixed Deposit flow: network model adds Changes
Sequence DiagramsequenceDiagram
actor User
participant UI as ChargesPage / BottomSheets
participant VM as ViewModel
participant State as NewFixedDepositAccountState
User->>UI: Tap "Add New Charge"
UI->>VM: OnShowAddChargeDialog(true)
VM->>State: set showAddChargeDialog = true
State-->>UI: render AddChargeBottomSheet
User->>UI: Fill details & Confirm
UI->>VM: OnAddCharge
VM->>State: append ChargeData to addedCharges
State-->>UI: render updated charges list
User->>UI: Tap "View Charges"
UI->>VM: OnShowViewChargesDialog(true)
VM->>State: set showViewChargesDialog = true
State-->>UI: render ShowChargesDialog
User->>UI: Tap "Delete" on a charge
UI->>VM: OnDeleteCharge(index)
VM->>State: remove charge at index
State-->>UI: render updated charges list
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
TheKalpeshPawar
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/CreateFixedDepositAccountViewmodel.kt (2)
626-654: Consider adding validation before adding a charge.The
handleAddChargefunction allows adding charges with:
- An amount of
0.0if the input is empty or invalid (line 637)- No validation to prevent duplicate charges
This could lead to unexpected UX where users add charges without realizing the amount defaulted to zero.
Consider adding validation similar to other term fields:
private fun handleAddCharge() { val chargesState = state.fixedDepositAccountCharges + val amountError = TextFieldsValidator.doubleNumberValidator(chargesState.chargeAmount) + if (amountError != null) { + // Update state with error instead of silently defaulting + return + } if (chargesState.selectedChargeIndex != null) {
806-818: Inconsistent null representation for "no selection" state.
selectedChargeIndexusesnullto represent no selection, whileeditingChargeIndexuses-1. This inconsistency requires different checks throughout the codebase. Consider using a consistent approach for clarity.feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/pages/ChargesPage.kt (1)
62-70: Unusedmodifierparameter.The
modifierparameter passed toChargesPage(line 66) is not applied. The rootColumnon line 70 usesModifier.fillMaxSize()directly instead.- Column(modifier = Modifier.fillMaxSize().padding(bottom = DesignToken.padding.large)) { + Column(modifier = modifier.fillMaxSize().padding(bottom = DesignToken.padding.large)) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
core/network/src/commonMain/kotlin/com/mifos/core/network/model/FixedDepositTemplate.kt(2 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/CreateFixedDepositAccountScreen.kt(2 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/CreateFixedDepositAccountViewmodel.kt(5 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/pages/ChargesPage.kt(1 hunks)version.txt(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/pages/ChargesPage.kt (5)
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/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/component/MifosBottomSheet.kt (1)
MifosBottomSheet(47-89)core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosActionsListingCardComponent.kt (1)
MifosActionsChargeListingComponent(601-735)core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosEmptyCard.kt (1)
MifosEmptyCard(28-49)
⏰ 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 (5)
version.txt (1)
1-1: LGTM!Version bump to 2025.11.3-beta.0.7 appropriately accompanies the new charges subsystem feature.
core/network/src/commonMain/kotlin/com/mifos/core/network/model/FixedDepositTemplate.kt (1)
62-63: LGTM!The
chargeOptionsfield follows the existing pattern for optional fields in this data class. The nullable type with defaultnullis appropriate for optional API response data.feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/CreateFixedDepositAccountScreen.kt (1)
119-124: LGTM!The updated
ChargesPageinvocation correctly passesstateandonAction, consistent with other step pages (DetailsPage, TermsPage, etc.) in this wizard flow.feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/CreateFixedDepositAccountViewmodel.kt (1)
243-288: LGTM!All charge-related actions are properly wired to their respective handler functions, following the established pattern in this viewmodel.
feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/pages/ChargesPage.kt (1)
212-276: LGTM!
ShowChargesDialogis well-structured with proper state management for expanded items and correct integration withMifosActionsChargeListingComponent. The empty state handling withMifosEmptyCardprovides good UX.
...kotlin/com/mifos/feature/client/newFixedDepositAccount/CreateFixedDepositAccountViewmodel.kt
Show resolved
Hide resolved
...kotlin/com/mifos/feature/client/newFixedDepositAccount/CreateFixedDepositAccountViewmodel.kt
Show resolved
Hide resolved
...t/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/pages/ChargesPage.kt
Show resolved
Hide resolved
...t/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/pages/ChargesPage.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/pages/ChargesPage.kt (1)
217-217: Use consistent type for expandedIndex.The variable
expandedIndexis declared asInt?but uses-1as a sentinel value instead ofnull. For consistency, either:
- Change the type to
Int(since you're using-1as the sentinel), or- Use
nullinstead of-1throughout (lines 217, 241, 254)Apply this diff to use a non-nullable type:
- var expandedIndex: Int? by rememberSaveable { mutableStateOf(-1) } + var expandedIndex: Int by rememberSaveable { mutableStateOf(-1) }feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/CreateFixedDepositAccountViewmodel.kt (2)
544-553: Reset selectedChargeIndex when closing dialog for consistency.When dismissing the add charge dialog (
action.show = false), the code resetseditingChargeIndexandchargeAmountbut doesn't resetselectedChargeIndex. This could leave stale selection state if the user cancels the dialog and reopens it later.Apply this diff to reset
selectedChargeIndex:private fun handleShowAddChargeDialog(action: NewFixedDepositAccountAction.NewFixedDepositAccountChargesAction.OnShowAddChargeDialog) { mutableStateFlow.update { it.copy( fixedDepositAccountCharges = it.fixedDepositAccountCharges.copy( showAddChargeDialog = action.show, editingChargeIndex = if (action.show) it.fixedDepositAccountCharges.editingChargeIndex else -1, chargeAmount = if (!action.show) "" else it.fixedDepositAccountCharges.chargeAmount, + selectedChargeIndex = if (!action.show) null else it.fixedDepositAccountCharges.selectedChargeIndex, ), ) } }
626-654: Consider validating charge amount before adding.Line 637 uses
toDoubleOrNull() ?: 0.0, which defaults to0.0if the amount is empty or invalid. This allows adding charges with zero amount, which may not be the intended behavior. Consider validating that the amount is positive before adding the charge, or handle empty input at the UI level.Example validation:
private fun handleAddCharge() { val chargesState = state.fixedDepositAccountCharges if (chargesState.selectedChargeIndex != null) { val selectedCharge = state.template.chargeOptions?.getOrNull(chargesState.selectedChargeIndex) if (selectedCharge != null) { + val amount = chargesState.chargeAmount.toDoubleOrNull() + if (amount == null || amount <= 0.0) { + // Handle invalid amount (e.g., show error) + return + } + val newCharge = ChargeData( id = selectedCharge.id ?: -1, name = selectedCharge.name ?: "", type = selectedCharge.chargeCalculationType?.value ?: "", collectedOn = selectedCharge.chargeTimeType?.value ?: "", - amount = chargesState.chargeAmount.toDoubleOrNull() ?: 0.0, + amount = amount, )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/CreateFixedDepositAccountViewmodel.kt(5 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/pages/ChargesPage.kt(1 hunks)version.txt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- version.txt
🧰 Additional context used
🧬 Code graph analysis (1)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/pages/ChargesPage.kt (5)
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/AddChargeDialog.kt (1)
AddChargeBottomSheet(47-171)core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosActionsListingCardComponent.kt (1)
MifosActionsChargeListingComponent(601-735)core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosEmptyCard.kt (1)
MifosEmptyCard(28-49)
⏰ 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 (10)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/pages/ChargesPage.kt (4)
62-67: LGTM: Clean state-driven refactoring.The function signature has been properly updated to accept state and dispatch actions, replacing the previous callback-based approach. This aligns well with the unidirectional data flow pattern.
69-132: LGTM: Well-structured UI layout.The main UI properly uses state to control button states and dispatches actions for user interactions. The scrollable column with proper spacing is a good UX choice.
134-186: LGTM: Safe charge option access.The dialog correctly uses
getOrNullfor accessingchargeOptions(lines 153, 159, 164), which addresses the past review comment about potentialIndexOutOfBoundsException. The add/edit mode logic is properly implemented.
188-207: LGTM: Dialog flow properly addressed past feedback.The view charges dialog integration is correct, and the past review comment about the incorrect null check for
selectedChargeIndexhas been properly addressed (lines 195-197 now unconditionally show the add dialog).feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/CreateFixedDepositAccountViewmodel.kt (6)
243-287: LGTM: Comprehensive action handling.All charge-related actions are properly dispatched to their respective handler methods, following a consistent pattern with the rest of the view model.
566-624: LGTM: Clean state update handlers.The remaining charge state handlers (
handleChargeAmountChange,handleChargeTypeSelected, date handlers) are straightforward state updates with no side effects.
656-704: LGTM: Safe charge editing with proper index handling.The
handleEditChargefunction properly addresses past review comments by usinggetOrNullfor safe access tochargeOptions(lines 666-669). The array access at line 674 is safe because it's protected by the null check at lines 657-658, which validates thataction.chargeIndexis within bounds.
706-718: LGTM: Bounds check properly addresses past feedback.The
handleDeleteChargefunction now includes a bounds check (line 711) before callingremoveAt, which properly addresses the past review comment about potentialIndexOutOfBoundsException.
807-827: LGTM: Well-structured charge state model.The
FixedDepositAccountChargesStateandChargeDataclasses provide a clean, comprehensive model for managing charge operations. The use of nullableIntforselectedChargeIndexproperly represents the "no selection" state.
881-894: LGTM: Comprehensive charge action hierarchy.The
NewFixedDepositAccountChargesActionsealed class provides complete coverage of all charge-related user interactions, maintaining consistency with the rest of the action hierarchy.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/CreateFixedDepositAccountViewmodel.kt (1)
659-707: Consider adding bounds check for defensive programming.The edit charge logic is mostly safe, using
getOrNullfor initial access. However, at line 677, direct list indexing is used. While the priorgetOrNullcheck at line 660 ensures the index was valid at that moment, adding an explicit bounds check would make the code more defensive.- val updatedCharges = state.fixedDepositAccountCharges.addedCharges.toMutableList().apply { - this[action.chargeIndex] = updatedCharge - } + val updatedCharges = state.fixedDepositAccountCharges.addedCharges.toMutableList().apply { + if (action.chargeIndex in indices) { + this[action.chargeIndex] = updatedCharge + } + }Note: The dual-path logic (updating vs. initializing edit) works correctly but could benefit from explanatory comments distinguishing when
selectedIndexis set vs. when opening the edit dialog.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/CreateFixedDepositAccountViewmodel.kt(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/CreateFixedDepositAccountViewmodel.kt (2)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountViewModel.kt (3)
handleShowAddChargeDialog(352-356)handleEditCharge(412-436)handleDeleteCharge(388-395)feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountv2/SavingsAccountViewModel.kt (3)
handleShowAddChargeDialog(399-403)handleEditCharge(373-397)handleDeleteCharge(303-310)
⏰ 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/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/CreateFixedDepositAccountViewmodel.kt (7)
257-301: LGTM! Action routing is well-structured.The charge-related actions are properly routed to their respective handlers, following the established pattern in the codebase.
537-627: LGTM! State mutation handlers are correctly implemented.All the simple state update handlers (for selected charge, dialog visibility, amount, dates, etc.) follow the immutable state update pattern correctly and are well-structured.
629-657: LGTM! Add charge handler is safely implemented.The method properly handles null checks and uses
getOrNullfor safe list access. The immutable state update pattern is correctly followed.
709-721: LGTM! Deletion handler properly implements bounds checking.The bounds check at line 714 correctly prevents
IndexOutOfBoundsException, and the immutable state update pattern is properly followed. This addresses the previously raised concern.
810-830: LGTM! Data classes are well-designed.The state structures are appropriately designed with:
- Nullable
selectedChargeIndexfor optional selection- Sentinel value
-1foreditingChargeIndexfollowing common patterns- Proper separation between UI state (dialogs, pickers) and data (addedCharges)
884-897: LGTM! Action definitions are well-structured.The sealed class hierarchy for charge-related actions follows the established pattern in the codebase (consistent with
NewFixedDepositAccountTermsAction) and provides type-safe action handling.
746-746: LGTM! Charges state is properly integrated.The charges state is correctly integrated into the main state with appropriate default initialization.
2 participants
Jira-#561
screen-20251211-114612.mp4
Summary by CodeRabbit
New Features
Chore
✏️ Tip: You can customize this high-level summary in your review settings.