Implement Charges Step for New Recurring Deposits Account Flow #2554
Conversation
WalkthroughThis pull request introduces charge management functionality across multiple feature modules (recurring deposits, savings, loan, client) by adding typed charge models, state management, and UI dialogs for creating, editing, and viewing charges. The changes include a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
feature/loan/src/commonMain/composeResources/values/strings.xml (1)
231-231: Fix malformed XML closing tag onprinciple_paid_offThe line
<string name="principle_paid_off">Principle Paid Off</string">has an extra
"after</string>, which will cause XML/resource compilation to fail. Remove the trailing quote so it becomes:<string name="principle_paid_off">Principle Paid Off</string>
🧹 Nitpick comments (6)
feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountv2/SavingsAccountScreen.kt (2)
308-308: Consider renamingexpandedIndextoexpandedChargeIdfor clarity.The variable stores a charge's
id(used on lines 352, 354), not its index. The current nameexpandedIndexis misleading.- var expandedIndex: Int? by rememberSaveable { mutableStateOf(-1) } + var expandedChargeId: Int? by rememberSaveable { mutableStateOf(-1) }
358-362: Consider customizing theMifosEmptyCardtitle for better context.The default title is "No item found". A more specific title would improve UX in the charges context.
MifosEmptyCard( + title = "No Charges", msg = stringResource(Res.string.feature_savings_charges_click_on_add_new), )Note: Consider adding a string resource for the title if localization is needed.
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/ChargesPage.kt (1)
110-112: Consider using a string resource with placeholder for proper i18n.The string concatenation
state.addedCharges.size.toString() + " " + stringResource(...)may cause localization issues in languages where word order differs. Consider defining a string resource with a%dplaceholder like"%d Active Charges".feature/recurringDeposit/src/commonMain/composeResources/values/feature_recurring_deposit_string.xml (1)
65-73: LGTM!The new string resources follow the established naming convention and provide appropriate UI text for the charge management functionality. Consider adding a pluralized version of
feature_recurring_deposit_active_charge(e.g., "Active Charges") or using a format string with count placeholder for better grammatical correctness.feature/loan/src/commonMain/composeResources/values/strings.xml (1)
224-224: Tighten the helper text phrasing for better UXThe new copy works but reads a bit awkwardly. Consider a more natural phrasing such as:
<string name="click_on_add_new">Click "Add New" to add a charge</string>This matches common UI wording and keeps “Add New” visually distinct as the control label.
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountScreen.kt (1)
486-521: Good empty-state UX and safer expansion behavior for charges list
- Guarding on
state.addedCharges.isNotEmpty()and showingMifosEmptyCardwithclick_on_add_newprovides a clear, guided empty state instead of a blank list.- Resetting
expandedIndex = -1before deleting a charge prevents the next item in the list from appearing “mysteriously” expanded after indices shift.If you ever revisit this, you could slightly simplify the state by making
expandedIndexa non-nullInt(no?) with-1as the “no selection” sentinel, to better reflect how it’s used. Not required for correctness.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
core/database/src/commonMain/kotlin/com/mifos/room/entities/templates/recurringDeposit/RecurringDepositAccountTemplate.kt(0 hunks)core/model/src/commonMain/kotlin/com/mifos/core/model/objects/payloads/RecurringDepositAccountPayload.kt(2 hunks)feature/client/src/commonMain/composeResources/values/strings.xml(1 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/CreateShareAccountScreen.kt(5 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/TermsPage.kt(2 hunks)feature/loan/src/commonMain/composeResources/values/strings.xml(1 hunks)feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountScreen.kt(4 hunks)feature/recurringDeposit/src/commonMain/composeResources/values/feature_recurring_deposit_string.xml(1 hunks)feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountScreen.kt(3 hunks)feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountViewModel.kt(6 hunks)feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/ChargesPage.kt(1 hunks)feature/savings/src/commonMain/composeResources/values/feature_savings_strings.xml(1 hunks)feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountv2/SavingsAccountScreen.kt(4 hunks)feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountv2/pages/ChargesPage.kt(0 hunks)
💤 Files with no reviewable changes (2)
- core/database/src/commonMain/kotlin/com/mifos/room/entities/templates/recurringDeposit/RecurringDepositAccountTemplate.kt
- feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountv2/pages/ChargesPage.kt
🧰 Additional context used
🧬 Code graph analysis (4)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/CreateShareAccountScreen.kt (2)
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)
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountScreen.kt (4)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/CreateShareAccountScreen.kt (2)
AddNewChargeDialog(217-272)ShowChargesDialog(274-346)feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountScreen.kt (2)
AddNewChargeDialog(380-447)ShowChargesDialog(449-538)feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/ChargesPage.kt (2)
AddNewChargeDialog(128-183)ShowChargesDialog(185-258)feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountv2/SavingsAccountScreen.kt (2)
AddNewChargeDialog(217-301)ShowChargesDialog(303-376)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountScreen.kt (2)
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)
feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountv2/SavingsAccountScreen.kt (3)
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)core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosTwoButtonRow.kt (1)
MifosTwoButtonRow(31-91)
⏰ 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 (16)
feature/savings/src/commonMain/composeResources/values/feature_savings_strings.xml (1)
149-150: LGTM!The new string resource follows the existing naming convention and provides clear guidance for the empty charges state.
feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountv2/SavingsAccountScreen.kt (3)
14-14: LGTM!Import changes align with the updated usage in
ShowChargesDialogfor empty state handling.Also applies to: 53-53
247-247: LGTM!Using "Back" instead of "Cancel" provides consistent navigation terminology across the charge dialogs.
315-373: LGTM!The dialog layout refactor is well-structured:
- Clean separation between header, content, and action buttons
- Proper empty state handling with
MifosEmptyCardMifosTwoButtonRowcorrectly positioned outside the items loopfeature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/pages/TermsPage.kt (1)
141-141: LGTM! Good UX improvement to indicate required fields.Appending asterisks to mark required fields aligns with common form validation patterns and is consistent with the existing error handling for
totalSharesErrorandsavingsAccountErrorin the state.Also applies to: 165-165
feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/pages/ChargesPage.kt (1)
52-52: LGTM! Consistent layout adjustment.The addition of bottom padding to the outer Column aligns with the same pattern used in other pages (e.g., TermsPage.kt line 108) within the CreateShareAccount flow.
feature/client/src/commonMain/composeResources/values/strings.xml (1)
586-586: LGTM! Clear empty-state message.The new string resource provides appropriate guidance to users when no charges have been added yet.
feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/CreateShareAccountScreen.kt (2)
17-17: LGTM! Imports properly utilized.The new imports for
feature_share_account_charge_click_on_add_newandMifosEmptyCardare correctly used in the ShowChargesDialog function for empty-state rendering.Also applies to: 52-52
294-331: LGTM! Well-implemented empty state handling.The conditional rendering properly handles both populated and empty charge lists:
- When charges exist, displays the expandable list with edit/delete actions
- When empty, shows an appropriate empty-state card with guidance
- Resetting
expandedIndex = -1before deletion (line 306) prevents potential stale-state issues after items are removedcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/payloads/RecurringDepositAccountPayload.kt (1)
43-47: LGTM!The
ChargeItemdata class is clean and properly annotated with@Serializable. Consider whether aChargeItemwithnullchargeIdshould be allowed to be added to the list, as this could result in invalid payload data being sent to the server.feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountScreen.kt (2)
88-102: LGTM!The dialog state handling for
AddNewChargeandShowChargesfollows the established pattern and correctly passes the required parameters. The implementation aligns with similar dialog patterns in other screens (CreateShareAccount, Savings, Loan) per the relevant code snippets.
140-143: LGTM!The updated
ChargesPageinvocation correctly passes the full state and action handler, enabling the state-driven UI pattern used throughout the application.feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/ChargesPage.kt (1)
177-178: Empty date handlers are intentional.The empty lambdas for
onDatePickandonDateChangeappear intentional since recurring deposit charges may not require date selection, consistent with patterns in similar screens.feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountViewModel.kt (2)
809-811: LGTM!The new state fields for charge management (
addedCharges,chargeAmount,chooseChargeIndex) are properly defined with sensible defaults.
917-924: LGTM!The new action types for charge management are well-defined and follow the existing action pattern in the codebase.
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountScreen.kt (1)
387-399: Dismiss label change to “Back” looks consistent with other dialogsSwitching
dismissTexttobackaligns this bottom sheet withShowCollateralsDialogand the rest of the stepper-style flow, keeping navigation language consistent. No issues from a behavior standpoint.
| chargeTitle = chargesValue?.name ?: "", | ||
| type = chargesValue?.chargeCalculationType?.value ?: "", | ||
| collectedOn = chargesValue?.chargeTimeType?.value ?: "", | ||
| amount = charge.amount.toString(), |
There was a problem hiding this comment.
Handle potential null amount display.
If charge.amount is null, calling .toString() will display "null" to the user. Consider providing a fallback value.
- amount = charge.amount.toString(),
+ amount = charge.amount?.toString() ?: "0.0",📝 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.
| amount = charge.amount.toString(), | |
| amount = charge.amount?.toString() ?: "0.0", |
🤖 Prompt for AI Agents
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/ChargesPage.kt
around line 214: the code currently uses charge.amount.toString(), which will
render "null" if amount is null; replace this with a null-safe display that uses
a fallback (e.g., empty string, localized "—", or formatted "0.00") and
preferably format numbers consistently (use charge.amount?.let {
formatDecimal(it) } ?: fallback) so users never see the literal "null".
| is RecurringAccountAction.EditCharge -> { | ||
| val createdData = ChargeItem( | ||
| chargeId = state.template.chargeOptions?.get(action.index)?.id, | ||
| amount = state.chargeAmount.toDoubleOrNull(), | ||
| ) | ||
|
|
||
| val currentAddedCharges = state.addedCharges.toMutableList() | ||
| currentAddedCharges[action.index] = createdData | ||
| mutableStateFlow.update { | ||
| it.copy( | ||
| addedCharges = currentAddedCharges, | ||
| chooseChargeIndex = null, | ||
| dialogState = RecurringAccountState.DialogState.ShowCharges, | ||
| chargeAmount = "", | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
Bug: Incorrect index used for chargeId lookup in EditCharge.
The action.index represents the index in addedCharges list, but it's being used to index into chargeOptions. This will result in the wrong chargeId being assigned when editing a charge.
The correct implementation should use state.chooseChargeIndex (which was set by EditChargeDialog based on the selected charge) to look up the chargeId from chargeOptions.
is RecurringAccountAction.EditCharge -> {
val createdData = ChargeItem(
- chargeId = state.template.chargeOptions?.get(action.index)?.id,
+ chargeId = state.chooseChargeIndex?.let { state.template.chargeOptions?.get(it)?.id },
amount = state.chargeAmount.toDoubleOrNull(),
)
val currentAddedCharges = state.addedCharges.toMutableList()
currentAddedCharges[action.index] = createdData
mutableStateFlow.update {
it.copy(
addedCharges = currentAddedCharges,
chooseChargeIndex = null,
dialogState = RecurringAccountState.DialogState.ShowCharges,
chargeAmount = "",
)
}
}📝 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.
| is RecurringAccountAction.EditCharge -> { | |
| val createdData = ChargeItem( | |
| chargeId = state.template.chargeOptions?.get(action.index)?.id, | |
| amount = state.chargeAmount.toDoubleOrNull(), | |
| ) | |
| val currentAddedCharges = state.addedCharges.toMutableList() | |
| currentAddedCharges[action.index] = createdData | |
| mutableStateFlow.update { | |
| it.copy( | |
| addedCharges = currentAddedCharges, | |
| chooseChargeIndex = null, | |
| dialogState = RecurringAccountState.DialogState.ShowCharges, | |
| chargeAmount = "", | |
| ) | |
| } | |
| } | |
| is RecurringAccountAction.EditCharge -> { | |
| val createdData = ChargeItem( | |
| chargeId = state.chooseChargeIndex?.let { state.template.chargeOptions?.get(it)?.id }, | |
| amount = state.chargeAmount.toDoubleOrNull(), | |
| ) | |
| val currentAddedCharges = state.addedCharges.toMutableList() | |
| currentAddedCharges[action.index] = createdData | |
| mutableStateFlow.update { | |
| it.copy( | |
| addedCharges = currentAddedCharges, | |
| chooseChargeIndex = null, | |
| dialogState = RecurringAccountState.DialogState.ShowCharges, | |
| chargeAmount = "", | |
| ) | |
| } | |
| } |
🤖 Prompt for AI Agents
In
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountViewModel.kt
around lines 714-730, the code uses action.index to read from
state.template.chargeOptions which is wrong because action.index is the index
into addedCharges, not chargeOptions; change the lookup to use
state.chooseChargeIndex (which EditChargeDialog sets) when retrieving the
chargeId from chargeOptions, while still using action.index to update the
addedCharges list. Also add a null/bounds guard: resolve an indexToUse =
state.chooseChargeIndex ?: action.index and ensure it’s within chargeOptions
bounds before accessing id to avoid NPE/IndexOutOfBounds.
| RecurringAccountAction.AddChargeToList -> { | ||
| val createdData = ChargeItem( | ||
| chargeId = state.template.chargeOptions?.get(state.chooseChargeIndex!!)?.id, | ||
| amount = state.chargeAmount.toDoubleOrNull(), | ||
| ) | ||
|
|
||
| mutableStateFlow.update { | ||
| it.copy( | ||
| addedCharges = it.addedCharges + createdData, | ||
| chooseChargeIndex = null, | ||
| dialogState = null, | ||
| chargeAmount = "", | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
Potential crash from force-unwrap of nullable index.
Using state.chooseChargeIndex!! on line 778 will throw a NullPointerException if chooseChargeIndex is null. While the UI flow should prevent this, defensive coding is recommended.
RecurringAccountAction.AddChargeToList -> {
+ val chargeIndex = state.chooseChargeIndex ?: return@handleAction
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 = "",
)
}
}🤖 Prompt for AI Agents
In
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountViewModel.kt
around lines 776 to 790, the code force-unwraps state.chooseChargeIndex (!!)
which can throw if null; change to a safe-check/early-return or use let with
getOrNull to safely obtain the charge option before creating ChargeItem, e.g.,
guard that chooseChargeIndex is not null and within bounds (or use
state.template.chargeOptions?.getOrNull(index)) and only update the state when a
valid chargeId is obtained, otherwise clear dialog/inputs or log/handle the
invalid case without throwing.
2 participants
Fixes - Jira-#554
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Style
Chores
✏️ Tip: You can customize this high-level summary in your review settings.