New Fixed Deposits Account Interest Charts Step #2557
Conversation
WalkthroughThis PR introduces dialog state management for rate charts in fixed deposit and recurring deposit account creation flows, refactors the InterestPage to a state-driven component, extends data models with new optional fields (description in AccountChart, accountChart in FixedDepositTemplate), standardizes Composable parameter ordering across multiple features (moving modifier before onAction), adds modifier parameters to several page composables for layout customization, and extends UI string resources for fixed deposit interest functionality. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/CreateFixedDepositAccountViewmodel.kt (1)
306-348: Method nameloadRecurringAccountTemplateWithProductis misleading.This method loads a Fixed Deposit template (calling
fixedDepositRepository.getFixedDepositTemplate), but the name suggests it handles recurring deposits. This appears to be a copy-paste from the recurring deposit feature.Rename for clarity:
- private fun loadRecurringAccountTemplateWithProduct() = viewModelScope.launch { + private fun loadFixedDepositTemplateWithProduct() = viewModelScope.launch {And update the call site at line 383:
- loadRecurringAccountTemplateWithProduct() + loadFixedDepositTemplateWithProduct()
🧹 Nitpick comments (8)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/pages/PreviewPage.kt (1)
53-64: Modifier parameter wiring is safe; consider whether it should scope the whole pageThe new
modifier: Modifier = Modifierparameter and its use on the inner scrollableColumnare correct and source‑compatible with existing call sites. Right now, any modifier passed by callers will only affect the scrollable content area, while the outerColumn(and thus the bottomMifosTwoButtonRow) still uses a fixedModifier.fillMaxSize().padding(bottom = ...).If the intent is to allow screen‑level layout customization (padding, background, etc.) that also applies to the button row, consider applying the external modifier to the outer
Columnand keeping a localModifierfor the inner scroll container, for example:- Column( - Modifier.fillMaxSize().padding(bottom = DesignToken.padding.large), - ) { - Column( - modifier = modifier.weight(1f).verticalScroll(rememberScrollState()), + Column( + modifier = modifier.fillMaxSize().padding(bottom = DesignToken.padding.large), + ) { + Column( + modifier = Modifier.weight(1f).verticalScroll(rememberScrollState()), verticalArrangement = Arrangement.spacedBy(20.dp), ) {If the current behavior (customization limited to the scroll area) is intentional and consistent with other pages, you can keep it as is.
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/InterestPage.kt (1)
53-61: Modifier plumbing looks good; note it only affects scrollable contentAdding
modifier: Modifier = Modifierand applying it asmodifier.weight(1f).verticalScroll(...)is consistent and non-breaking. Just be aware this customizes only the scrollable content column; the outer Column (and bottom button row) still use a fixedModifier.fillMaxSize().padding(...). If you ever need page-level padding or insets, you may want to thread the modifier into the root Column as well.feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountViewModel.kt (2)
151-165: Avoid calling template reload with an invalid product id
handleProductNameChangenow always callsloadRecurringAccountTemplateWithProduct, but falls back toproductId = -1when the selected option or its id is missing:loadRecurringAccountTemplateWithProduct( state.clientId, state.template.productOptions?.get(state.recurringDepositAccountDetail.loanProductSelected)?.id ?: -1, )It would be cleaner (and cheaper) to guard the call and skip the reload when no valid product id is available, instead of pushing
-1down to the repository.
685-699: DialogState modeling is solid; consider renamingisRateChartEmptyfor clarityThe introduction of a sealed
DialogStateand central handling forOnDismissDialog/OnShowRateChartDialoglooks good and should make dialog flows easier to reason about.However, in
RecurringAccountState:val isRateChartEmpty = !template.accountChart?.chartSlabs.isNullOrEmpty()This property evaluates to
truewhen there are chart slabs, which is opposite to what “Empty” suggests. Since this flag is used to enable the “View” button and choose the label inInterestPage, consider either:
- renaming it to something like
hasRateChart, or- flipping the boolean and adjusting the usages.
This will reduce confusion for future readers.
Also applies to: 809-827
feature/client/src/commonMain/composeResources/values/strings.xml (1)
623-633: New fixed-deposit interest strings look consistentThe added
feature_fixed_deposit_interest_*strings are well-namespaced and match the existing naming style. Minor nit:"Grouping By Amount"here vs"Grouping by Amount"in recurring-deposit resources is a small capitalization inconsistency; align them if you care about uniform UI copy.feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/pages/InterestPage.kt (2)
96-107: Confusing conditional due to inverted naming inisRateChartEmpty.The
isRateChartEmptyflag istruewhen the rate chart has data (see ViewModel line 540:!template.accountChart?.chartSlabs.isNullOrEmpty()). This makes the conditional here read counterintuitively:
- When data exists (
isRateChartEmpty = true): shows "interest rate chart" and enables button ✓- When no data (
isRateChartEmpty = false): shows "no interest chart" and disables button ✓The logic produces correct behavior, but the naming is misleading. Consider renaming to
hasRateChartDatain the ViewModel for clarity.
139-155: Consider extracting chart slab item rendering to improve readability.The
MifosActionsChargeListingComponentis being repurposed for displaying rate chart slabs. The component's parameter names (chargeTitle,collectedOn) don't semantically match their usage here. This works but could be confusing for future maintainers.Also, the
onActionClicked,isExpanded, andonExpandToggleparameters are effectively no-ops here. Consider either:
- Creating a dedicated
RateChartSlabItemcomposable- Adding a comment explaining this reuse pattern
feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/CreateFixedDepositAccountScreen.kt (1)
93-124: InconsistentStep()constructor usage.Some
Stepinstantiations use the named parametername =while others use positional arguments. Consider using a consistent style throughout for better readability.val steps = listOf( - Step(stringResource(Res.string.step_details)) { + Step(name = stringResource(Res.string.step_details)) { DetailsPage( state = state, onAction = onAction, ) }, Step(name = stringResource(Res.string.step_terms)) { ... }, Step(name = stringResource(Res.string.step_settings)) { ... }, Step(name = stringResource(Res.string.step_interest)) { ... }, - Step(stringResource(Res.string.step_charges)) { + Step(name = stringResource(Res.string.step_charges)) { ChargesPage( onNext = { onAction(NewFixedDepositAccountAction.OnNextPress) }, ) }, )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
core/model/src/commonMain/kotlin/com/mifos/core/model/objects/template/recurring/AccountChart.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/DetailsPage.kt(1 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/pages/PreviewPage.kt(1 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/pages/TermsPage.kt(1 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/CreateFixedDepositAccountScreen.kt(3 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/CreateFixedDepositAccountViewmodel.kt(7 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/pages/DetailsPage.kt(1 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/pages/InterestPage.kt(1 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(1 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/DetailsPage.kt(1 hunks)feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/pages/PreviewPage.kt(1 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/composeResources/values/feature_recurring_deposit_string.xml(1 hunks)feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountViewModel.kt(2 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(2 hunks)feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/InterestPage.kt(1 hunks)feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/SettingsPage.kt(1 hunks)feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/TermsPage.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(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)
🧰 Additional context used
🧬 Code graph analysis (3)
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(306-348)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/CreateFixedDepositAccountViewmodel.kt (1)
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountViewModel.kt (1)
loadRecurringAccountTemplateWithProduct(294-328)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/CreateFixedDepositAccountScreen.kt (1)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/pages/InterestPage.kt (2)
FixedDepositRateChart(120-170)InterestPage(53-118)
⏰ 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 (26)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/pages/TermsPage.kt (1)
63-67: Parameter reordering looks good; ensure all call sites are updatedMoving
onActionaftermodifieraligns this composable with the normalized API pattern across the PR and matches common Compose conventions. The change is source-compatible only for callers using named arguments; any positional calls likeTermsPage(state, onAction)will now fail to compile or misinterpret arguments.Please verify that all
TermsPageinvocations have been updated to either:
- use named arguments (
TermsPage(state = ..., onAction = ...)), or- follow the new positional order (
TermsPage(state, modifier, onAction)).feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountv2/pages/ChargesPage.kt (1)
47-51: Verify call sites for ChargesPage parameter order changeParameter order was changed to
state, modifier, onAction. This aligns with Compose conventions (required params, modifier with default, then callbacks), but any existing call sites using positional arguments will now bindmodifierandonActionincorrectly. Ensure all callers use named arguments or are updated to match the new order.feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/pages/DetailsPage.kt (1)
52-56: Verify all call sites have been updated for the new parameter order.The parameter reordering (moving
modifierbeforeonAction) aligns with standard Compose conventions wheremodifiertypically appears early in the parameter list, after required parameters. This improves consistency across the codebase.Since this changes the function signature's parameter order, all call sites of
DetailsPagemust be verified to ensure they use the correct parameter order. Any code using positional arguments will break with this change. Check that either:
- All call sites have been updated to use the new order, or
- All call sites use named arguments (which would be unaffected by the reordering)
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountViewModel.kt (1)
65-71: ClearingdialogStatewhen moving to the next step is a good safety netResetting
dialogStatealongsidecurrentStepavoids leaking an open dialog into the next step and keeps the UI state machine simpler.feature/recurringDeposit/src/commonMain/composeResources/values/feature_recurring_deposit_string.xml (1)
64-64: Updated empty-date copy is fineChanging
"Empty Date"to"Date Not Found"is a safe UX text improvement with no behavioral impact. Just ensure any other locales (if present) are kept in sync.feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/pages/SettingsPage.kt (1)
55-63: Modifier injection follows the shared patternExposing
modifier: Modifier = Modifierand applying it to the scrollable inner Column (modifier.weight(1f).verticalScroll(...)) is consistent with other screens and keeps existing behavior for the bottom button row. Looks good.feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/DetailsPage.kt (1)
56-60: Composable now exposes a modifier; implementation matches the shared pattern
DetailsPagefor recurring deposits now takesmodifier: Modifier = Modifierand applies it to the scrollable inner Column viamodifier.weight(1f).verticalScroll(...), which is consistent with other screens in this PR. This keeps the bottom button row layout stable while allowing callers to tweak the content area’s layout.Also applies to: 113-116
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/pages/PreviewPage.kt (1)
75-87: Composable now supports external layout control; verify call sites after signature changeAdding
modifier: Modifier = Modifierand using it asmodifier.weight(1f).verticalScroll(...)is idiomatic and preserves internal behavior. Since the parameter was inserted beforeonAction, any positional calls likePreviewPage(state, onAction)must be updated (e.g., to named args) to compile. Verify that all call sites have been updated accordingly.feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountv2/pages/DetailsPage.kt (1)
56-60: Parameter reordering may break positional callers; modifier wiring is fineReordering to
DetailsPage(state, modifier: Modifier = Modifier, onAction)aligns with other composables and the modifier is correctly applied to the scrollable Column. However, any existing positional calls likeDetailsPage(state, onAction)will now mis-bind arguments. Please confirm all call sites were updated to use the new ordering (or named parameters).Also applies to: 101-104
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/pages/ChargesPage.kt (1)
46-50: LGTM! Parameter ordering follows Compose conventions.The reordering of
modifierbeforeonActionaligns with Compose API guidelines where modifier parameters conventionally precede trailing lambdas. This improves API consistency across the codebase.feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/pages/DetailsPage.kt (1)
54-57: LGTM! Consistent parameter ordering.The parameter reordering matches the pattern applied across other pages in this PR, improving consistency and following Compose best practices.
feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/pages/TermsPage.kt (1)
49-52: LGTM! Parameter ordering improvement.Consistent with the standardization effort across the codebase to place
modifierbefore action callbacks.feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/pages/DetailsPage.kt (1)
63-66: LGTM! Consistent API improvement.The parameter reordering aligns with Compose conventions and matches the pattern applied throughout this PR.
feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountv2/pages/PreviewPage.kt (1)
57-60: LGTM! Standard Compose parameter ordering.The signature update maintains consistency with other pages being refactored in this PR.
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/ChargesPage.kt (1)
61-68: LGTM! Modifier parameter properly added and wired.The addition of the
modifierparameter with a default value maintains backward compatibility while enabling external layout customization. The modifier is correctly threaded to the inner Column at line 68.feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/TermsPage.kt (1)
39-46: LGTM! Modifier parameter correctly implemented.The modifier parameter is properly added with a default value and correctly propagated to the inner Column, enabling external layout control while maintaining backward compatibility.
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/pages/SchedulePage.kt (1)
39-42: LGTM! Completes the consistent parameter ordering refactor.This change aligns with the standardization pattern applied throughout the PR, placing
modifierbefore the action callback per Compose conventions.feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/SettingsPage.kt (1)
64-73: LGTM!The modifier parameter addition follows Compose conventions (state → modifier → callbacks). The modifier is appropriately applied to the scrollable content area while keeping the outer container's layout fixed.
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/pages/TermsPage.kt (1)
103-107: LGTM!Parameter reordering to place
modifierbeforeonActionaligns with Compose conventions and is consistent with changes across other page composables in this PR.feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountv2/pages/TermsPage.kt (1)
61-68: LGTM!Parameter reordering follows Compose conventions and maintains consistency with the broader pattern established across the PR.
feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/CreateFixedDepositAccountViewmodel.kt (2)
243-256: LGTM on dialog state management.The
OnDismissDialogandOnShowRateChartaction handlers correctly updatedialogState. The pattern is consistent with dialog management practices in Android/Compose.
488-500: LGTM on resetting dialog state during step navigation.Resetting
dialogState = nullwhen moving to the next step ensures dialogs are dismissed automatically, preventing stale UI state.feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/CreateFixedDepositAccountScreen.kt (2)
69-84: Clean dialog state management implementation.The separation of dialog handling into its own composable with the sealed
DialogStatepattern is well-structured and follows good Compose practices for managing dialog visibility.
119-123:ChargesPageuses a different callback pattern than other pages.Other pages receive
stateandonAction, butChargesPageonly receivesonNext. IfChargesPagewill eventually need access to state or additional actions, consider aligning its signature with the other pages for consistency. If this is intentional (e.g.,ChargesPagedoesn't need state), this is acceptable as-is.core/network/src/commonMain/kotlin/com/mifos/core/network/model/FixedDepositTemplate.kt (1)
58-61: Verify the JSON key foraccountChart.The new
accountChartfield lacks a@SerialNameannotation, similar tolockinPeriodFrequencyTypeOptionsandmaturityInstructionOptions. This means kotlinx.serialization will use"accountChart"as the JSON key. Ensure this matches the actual API response field name.core/model/src/commonMain/kotlin/com/mifos/core/model/objects/template/recurring/AccountChart.kt (1)
33-35: LGTM!The
descriptionfield addition is consistent with the existing data class patterns - nullable type with null default, no@IgnoredOnParcelannotation (appropriate for simple types), and logically placed near the relatednamefield.
| data object RateChartDialog : DialogState | ||
| } | ||
|
|
||
| val isRateChartEmpty = !template.accountChart?.chartSlabs.isNullOrEmpty() |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Property isRateChartEmpty has inverted semantics.
The property returns true when the chart slabs are not empty, which contradicts what the name suggests. This will confuse anyone reading the code.
Rename to reflect actual semantics:
- val isRateChartEmpty = !template.accountChart?.chartSlabs.isNullOrEmpty()
+ val hasRateChartData = !template.accountChart?.chartSlabs.isNullOrEmpty()Then update InterestPage.kt accordingly:
- text = if (state.isRateChartEmpty) {
+ text = if (state.hasRateChartData) {
stringResource(Res.string.feature_fixed_deposit_interest_interest_rate_chart)
} else {
stringResource(Res.string.feature_fixed_deposit_interest_no_interest_chart)
},
- btnEnabled = state.isRateChartEmpty,
+ btnEnabled = state.hasRateChartData,📝 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.
| val isRateChartEmpty = !template.accountChart?.chartSlabs.isNullOrEmpty() | |
| val hasRateChartData = !template.accountChart?.chartSlabs.isNullOrEmpty() |
🤖 Prompt for AI Agents
In
feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/CreateFixedDepositAccountViewmodel.kt
around line 540, the property currently named isRateChartEmpty has inverted
semantics (it returns true when chart slabs are NOT empty); rename it to reflect
actual meaning (e.g., hasRateChart or isRateChartPresent) and invert the boolean
expression to match the new name (or if you keep the name, invert the logic to
use isNullOrEmpty correctly). Then update all usages (specifically
InterestPage.kt) to reference the new property name and adjust any conditional
logic to preserve original behavior.
2 participants
Fixes - Jira-#560
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.