implemented terms page of fixed deposit accounts Feature #2543
Conversation
WalkthroughThe pull request extends the fixed deposit feature by adding six new optional properties to FixedDepositTemplate for currency and interest/period type options, refactoring TermsPage to use state-driven architecture with form inputs and action handlers, introducing FixedDepositAccountTermsState for validation, and updating related services, repositories, and navigation components accordingly. Changes
Sequence DiagramsequenceDiagram
participant UI as TermsPage
participant VM as CreateFixedDepositAccountViewmodel
participant State as NewFixedDepositAccountState
rect rgb(200, 220, 240)
note over UI,State: User Input Flow
UI->>VM: onAction(SetFixedDepositAmount)
VM->>State: Update fixedDepositAccountTerms.depositAmount
VM->>State: Compute isTermsNextEnabled
State->>UI: Recompose with updated state
end
rect rgb(200, 220, 240)
note over UI,State: Dropdown Selection Flow
UI->>VM: onAction(SetFixedDepositPeriodType)
VM->>State: Update fixedDepositAccountTerms.depositPeriodTypeIndex
VM->>State: Recompute isTermsNextEnabled
State->>UI: Recompose with new options
end
rect rgb(220, 240, 200)
note over UI,VM: Navigation Flow
UI->>VM: onAction(OnNextPress)
alt isTermsNextEnabled = true
VM->>VM: Proceed to next step
else isTermsNextEnabled = false
VM->>VM: Keep Next button disabled
end
UI->>VM: onAction(NavigateBack)
VM->>VM: Navigate to previous step
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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: 5
♻️ Duplicate comments (1)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/pages/TermsPage.kt (1)
113-128: Same index/ID confusion in all interest dropdowns.All four interest-related dropdowns (compounding period, posting period, calculation type, days in year) have the same issue as the period type dropdown. The state stores option IDs but the UI uses them as array indices.
Apply the same fix pattern as suggested for the period type dropdown to all these handlers in the viewmodel.
Also applies to: 129-144, 145-160, 161-176
🧹 Nitpick comments (3)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/createFixedDepositAccountScreen.kt (1)
87-101: Consider aligning remaining pages with state-driven pattern.While DetailsPage and TermsPage use the state-driven pattern (
state+onAction), the remaining pages (SettingPage, InterestPage, ChargesPage) still use the callback-based pattern (onNext). For consistency and maintainability, consider migrating these pages to the same state-driven approach in future iterations.feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/pages/TermsPage.kt (1)
65-91: Consider adding input validation for numeric fields.The deposit amount and period fields accept any string input without validation. Consider adding:
- Decimal format validation for deposit amount
- Integer validation for deposit period
- Range validation (e.g., positive numbers only)
- Error messages for invalid input
feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/createFixedDepositAccountViewmodel.kt (1)
309-310: Consider numeric validation for amount and period fields.The validation currently only checks if the fields are non-empty. Consider adding:
- Numeric format validation (e.g., valid decimal for amount, valid integer for period)
- Range validation (e.g., positive values only)
📜 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/composeResources/values/strings.xml(1 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/createFixedDepositAccountScreen.kt(1 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/TermsPage.kt(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
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)
🔇 Additional comments (8)
core/network/src/commonMain/kotlin/com/mifos/core/network/model/FixedDepositTemplate.kt (1)
4-9: LGTM! Template properties properly extended.The new template properties for currency and various option types are correctly implemented with proper serialization annotations. The use of nullable types with default null values is appropriate for optional API fields.
Also applies to: 18-48
feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/createFixedDepositAccountScreen.kt (1)
81-84: State-driven pattern adopted successfully.The TermsPage now properly receives state and onAction, aligning with the new architecture. This is consistent with DetailsPage (lines 75-78).
feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/pages/TermsPage.kt (3)
49-58: LGTM! Proper state-driven architecture adopted.The component signature correctly accepts state and onAction, enabling a clean unidirectional data flow. The scrollable column layout is appropriate for a multi-field form.
65-79: Good input configuration for deposit amount.The numeric keyboard type and currency prefix enhance the UX. Safe null handling with
orEmpty()prevents display issues when currency is unavailable.
177-184: Navigation controls properly implemented.The two-button row correctly enables/disables the Next button based on form validation state, providing good user feedback.
feature/client/src/commonMain/composeResources/values/strings.xml (1)
581-590: LGTM! String resources properly added.The new string resources follow consistent naming conventions and align with their usage in the TermsPage UI.
feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/createFixedDepositAccountViewmodel.kt (2)
60-66: Action routing properly implemented.All new terms-related actions are correctly routed to their respective handler functions.
187-195: Text input handlers correctly implemented.The deposit amount and period handlers properly store string values, delegating validation to the state's computed property.
Also applies to: 197-205
...kotlin/com/mifos/feature/client/newFixedDepositAccount/createFixedDepositAccountViewmodel.kt
Show resolved
Hide resolved
...kotlin/com/mifos/feature/client/newFixedDepositAccount/createFixedDepositAccountViewmodel.kt
Show resolved
Hide resolved
| data class FixedDepositAccountTermsState( | ||
| val depositAmount: String = "", | ||
| val depositPeriod: String = "", | ||
| val depositPeriodType: Int = -1, | ||
| val interestCompoundingPeriodType: Int = -1, | ||
| val interestPostingPeriodType: Int = -1, | ||
| val interestCalculationType: Int = -1, | ||
| val interestCalculationDaysInYearType: Int = -1, | ||
| ){ | ||
| val isTermsNextEnabled = depositAmount.isNotEmpty() && depositPeriod.isNotEmpty() | ||
| && depositPeriodType!=-1 && interestCompoundingPeriodType!=-1 && interestCalculationType!=-1 && interestCalculationDaysInYearType!=-1 | ||
| } |
There was a problem hiding this comment.
Major: Incomplete validation logic.
Line 309-310: The isTermsNextEnabled validation is missing a check for interestPostingPeriodType, which is part of the state (line 305). This allows users to proceed without selecting this required field.
Apply this fix:
val isTermsNextEnabled = depositAmount.isNotEmpty() && depositPeriod.isNotEmpty()
&& depositPeriodType!=-1 && interestCompoundingPeriodType!=-1
+ && interestPostingPeriodType!=-1
&& interestCalculationType!=-1 && interestCalculationDaysInYearType!=-1📝 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.
| data class FixedDepositAccountTermsState( | |
| val depositAmount: String = "", | |
| val depositPeriod: String = "", | |
| val depositPeriodType: Int = -1, | |
| val interestCompoundingPeriodType: Int = -1, | |
| val interestPostingPeriodType: Int = -1, | |
| val interestCalculationType: Int = -1, | |
| val interestCalculationDaysInYearType: Int = -1, | |
| ){ | |
| val isTermsNextEnabled = depositAmount.isNotEmpty() && depositPeriod.isNotEmpty() | |
| && depositPeriodType!=-1 && interestCompoundingPeriodType!=-1 && interestCalculationType!=-1 && interestCalculationDaysInYearType!=-1 | |
| } | |
| data class FixedDepositAccountTermsState( | |
| val depositAmount: String = "", | |
| val depositPeriod: String = "", | |
| val depositPeriodType: Int = -1, | |
| val interestCompoundingPeriodType: Int = -1, | |
| val interestPostingPeriodType: Int = -1, | |
| val interestCalculationType: Int = -1, | |
| val interestCalculationDaysInYearType: Int = -1, | |
| ){ | |
| val isTermsNextEnabled = depositAmount.isNotEmpty() && depositPeriod.isNotEmpty() | |
| && depositPeriodType!=-1 && interestCompoundingPeriodType!=-1 | |
| && interestPostingPeriodType!=-1 | |
| && interestCalculationType!=-1 && interestCalculationDaysInYearType!=-1 | |
| } |
🤖 Prompt for AI Agents
In
feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/createFixedDepositAccountViewmodel.kt
around lines 300 to 311, the isTermsNextEnabled boolean currently omits checking
interestPostingPeriodType; update the validation expression to include
interestPostingPeriodType != -1 so the Next button remains disabled until that
required field is selected, ensuring all state fields (depositAmount,
depositPeriod, depositPeriodType, interestCompoundingPeriodType,
interestPostingPeriodType, interestCalculationType,
interestCalculationDaysInYearType) are validated.
...kotlin/com/mifos/feature/client/newFixedDepositAccount/createFixedDepositAccountViewmodel.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/CreateFixedDepositAccountViewmodel.kt (1)
285-296: Validation bug: interest posting period not required inisTermsNextEnabled
FixedDepositAccountTermsState.isTermsNextEnabledcurrently checks:
depositAmountnon-emptydepositPeriodnon-emptydepositPeriodTypeIndex != -1interestCompoundingPeriodTypeIndex != -1interestCalculationTypeIndex != -1interestCalculationDaysInYearTypeIndex != -1but does not validate
interestPostingPeriodTypeIndex. This matches the earlier review comment you received and still allows progressing from the Terms step without choosing an interest posting period, even though you track it in state.You likely want to include
interestPostingPeriodTypeIndexin the predicate:data class FixedDepositAccountTermsState( val depositAmount: String = "", val depositPeriod: String = "", val depositPeriodTypeIndex: Int = -1, val interestCompoundingPeriodTypeIndex: Int = -1, val interestPostingPeriodTypeIndex: Int = -1, val interestCalculationTypeIndex: Int = -1, val interestCalculationDaysInYearTypeIndex: Int = -1, ) { - val isTermsNextEnabled = depositAmount.isNotEmpty() && depositPeriod.isNotEmpty() && - depositPeriodTypeIndex != -1 && interestCompoundingPeriodTypeIndex != -1 && interestCalculationTypeIndex != -1 && interestCalculationDaysInYearTypeIndex != -1 + val isTermsNextEnabled = depositAmount.isNotEmpty() && depositPeriod.isNotEmpty() && + depositPeriodTypeIndex != -1 && + interestCompoundingPeriodTypeIndex != -1 && + interestPostingPeriodTypeIndex != -1 && + interestCalculationTypeIndex != -1 && + interestCalculationDaysInYearTypeIndex != -1 }Without this, the Terms “Next” button will be enabled even if the posting period has not been selected.
#!/bin/bash # Confirm that the Terms page uses isTermsNextEnabled to control the Next button. rg -n "isTermsNextEnabled" -C5
🧹 Nitpick comments (2)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/CreateFixedDepositAccountViewmodel.kt (1)
46-67: Refactor recommendation is valid; this improves sealed hierarchy consistencyThe refactoring is safe and aligns the class hierarchy properly. All usages rely on the fully qualified path (
NewFixedDepositAccountAction.NewFixedDepositAccountTermsAction.SetFixedDepositAmount) and inheritance chain resolution, not direct parent class identity. Theistype checks inwhenexpressions, handler function signatures, and action instantiations will all continue to work without modification.The change correctly addresses the organizational inconsistency: the nested sealed class
NewFixedDepositAccountTermsActioncurrently doesn't group its subclasses, which defeats the purpose of having an intermediate sealed type. Updating the nested data classes to extendNewFixedDepositAccountTermsAction()makes the hierarchy intention clear and can simplify futurewhenbranches that operate on terms actions as a set.feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/pages/TermsPage.kt (1)
132-179: Consider extracting a reusable dropdown helper.All five dropdowns follow an identical pattern: check if index != -1, lookup value by index, map options to values, and handle selection. Consider extracting a composable helper function to reduce the ~15 lines of duplicated code per dropdown.
Example helper function:
@Composable private fun TermsDropdown( selectedIndex: Int, options: List<EnumOptionData>?, onSelect: (Int) -> Unit, label: String, modifier: Modifier = Modifier ) { MifosTextFieldDropdown( value = if (selectedIndex != -1) { options?.getOrNull(selectedIndex)?.value.orEmpty() } else { "" }, options = options?.map { it.value.orEmpty() } ?: emptyList(), onValueChanged = {}, onOptionSelected = { index, _ -> onSelect(index) }, label = label, modifier = modifier, ) }Then each dropdown becomes:
TermsDropdown( selectedIndex = state.fixedDepositAccountTerms.interestPostingPeriodTypeIndex, options = state.template.interestPostingPeriodTypeOptions, onSelect = { onAction(NewFixedDepositAccountAction.NewFixedDepositAccountTermsAction.SetInterestPostingPeriod(it)) }, label = stringResource(Res.string.feature_fixed_interest_posting_period), modifier = Modifier.fillMaxWidth(), )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
core/data/src/commonMain/kotlin/com/mifos/core/data/repository/FixedDepositRepository.kt(2 hunks)core/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/FixedDepositRepositoryImpl.kt(1 hunks)core/database/src/commonMain/kotlin/com/mifos/room/basemodel/APIEndPoint.kt(1 hunks)core/network/src/commonMain/kotlin/com/mifos/core/network/BaseApiManager.kt(2 hunks)core/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerFixedDeposit.kt(1 hunks)core/network/src/commonMain/kotlin/com/mifos/core/network/di/DataMangerModule.kt(1 hunks)core/network/src/commonMain/kotlin/com/mifos/core/network/model/FixedDepositProductOption.kt(2 hunks)core/network/src/commonMain/kotlin/com/mifos/core/network/model/FixedDepositTemplate.kt(1 hunks)core/network/src/commonMain/kotlin/com/mifos/core/network/services/FixedDepositService.kt(1 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/navigation/ClientNavigation.kt(0 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/CreateFixedDepositAccountRoute.kt(1 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(7 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/pages/DetailsPage.kt(4 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/pages/TermsPage.kt(1 hunks)
💤 Files with no reviewable changes (1)
- feature/client/src/commonMain/kotlin/com/mifos/feature/client/navigation/ClientNavigation.kt
✅ Files skipped from review due to trivial changes (5)
- core/network/src/commonMain/kotlin/com/mifos/core/network/services/FixedDepositService.kt
- core/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerFixedDeposit.kt
- core/network/src/commonMain/kotlin/com/mifos/core/network/model/FixedDepositProductOption.kt
- core/database/src/commonMain/kotlin/com/mifos/room/basemodel/APIEndPoint.kt
- feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/pages/DetailsPage.kt
🧰 Additional context used
🧬 Code graph analysis (1)
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)
🔇 Additional comments (13)
core/network/src/commonMain/kotlin/com/mifos/core/network/di/DataMangerModule.kt (1)
22-22: DataManagerFixedDeposit import wiring is consistentThe added import aligns with the existing
single { DataManagerFixedDeposit(get()) }binding and doesn’t alter behavior. Looks good.core/data/src/commonMain/kotlin/com/mifos/core/data/repository/FixedDepositRepository.kt (1)
16-21: Repository signature change is formatting-only
getFixedDepositTemplatekeeps the same contract and return type; the multi-line + trailing comma style is consistent with the impl. No issues.core/network/src/commonMain/kotlin/com/mifos/core/network/BaseApiManager.kt (1)
36-37: FixedDepositService exposure matches existing service patternImporting
FixedDepositService/createFixedDepositServiceand exposingfixedDepositServiceinitialized viaktorfitis consistent with how other services are wired in this class. No functional concerns.Also applies to: 56-57, 75-82
core/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/FixedDepositRepositoryImpl.kt (1)
19-28: Repository implementation delegation is correct
FixedDepositRepositoryImplcleanly delegatesgetFixedDepositTemplatetodataManagerFixedDeposit.getFixedDepositTemplate(...).asDataStateFlow(). The changes are formatting-only with no behavioral impact.feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/CreateFixedDepositAccountViewmodel.kt (1)
28-40: State refactor and default initialization look soundSwitching the viewmodel to
BaseViewModel<NewFixedDepositAccountState, NewFixedDepositAccountEvent, NewFixedDepositAccountAction>and introducingfixedDepositAccountTerms: FixedDepositAccountTermsState = FixedDepositAccountTermsState()plus a non‑nulltemplate: FixedDepositTemplate = FixedDepositTemplate()provides a clear separation between details and terms, while keeping the template always present.No functional issues spotted in this part.
Also applies to: 268-277
core/network/src/commonMain/kotlin/com/mifos/core/network/model/FixedDepositTemplate.kt (1)
12-19: Field naming is consistent with established backend patternsThe option field names (
periodFrequencyTypeOptions,interestCompoundingPeriodTypeOptions,interestPostingPeriodTypeOptions,interestCalculationDaysInYearTypeOptions,interestCalculationTypeOptions) follow the exact same naming convention already used in RecurringDepositAccountTemplate and SavingProductsTemplate, with matching @SerialName annotations. These fields are actively used across multiple features (recurring deposits, fixed deposits, savings), confirming the backend returns them with these exact JSON keys.feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/CreateFixedDepositAccountScreen.kt (1)
81-82: LGTM! Consistent state-driven pattern.The refactored TermsPage invocation correctly passes
stateandonAction, aligning with the state-driven architecture used by DetailsPage and the broader feature refactor.feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/pages/TermsPage.kt (6)
50-53: LGTM! Clean migration to state-driven architecture.The signature refactor from a simple
onNextcallback to state and action parameters enables proper form state management and validation.
54-65: LGTM! Proper scrollable form layout.The scrollable column with full width ensures the form is accessible on smaller screens, and the header provides clear context.
66-80: LGTM! Proper numeric input with currency prefix.The field correctly uses numeric keyboard type and displays the currency symbol. String storage is appropriate for form inputs, with validation presumably handled via
isTermsNextEnabled.
81-92: LGTM! Consistent numeric input pattern.The deposit period field follows the same pattern as the deposit amount field with appropriate keyboard configuration.
109-131: LGTM! Consistent dropdown pattern with clear section header.The interest compounding period dropdown follows the same index-based pattern as the deposit period type dropdown, with a clear section header improving form organization.
180-187: LGTM! Proper state-driven button enablement.The button row correctly handles navigation and uses
isTermsNextEnabledto prevent form submission when validation fails, following good form UX patterns.
| @Serializable | ||
|
|
||
| data class createFixedDepositAccountRoute( | ||
| data class CreateFixedDepositAccountRoute( | ||
| val clientId: Int, | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
Route rename and typed navigation look correct; verify onFinish behavior
The route rename to CreateFixedDepositAccountRoute and usage in composable<CreateFixedDepositAccountRoute> plus the navigateToCreateFixedDepositRoute helper are consistent and type-safe.
One thing to double‑check: onFinish = {} means the screen won’t trigger any navigation on completion. If the UX is supposed to move to account details or back to the client screen after creating the fixed deposit, you may want to wire a real navigation callback here.
Naming nit (optional): for consistency you might consider renaming navigateToCreateFixedDepositRoute to navigateToCreateFixedDepositAccountRoute.
Also applies to: 22-30, 32-35
🏁 Script executed:
#!/bin/bash
# Grep usages of the navigate helper and onFinish to confirm expected flow.
rg -n "navigateToCreateFixedDepositRoute" -C3
rg -n "CreateFixedDepositAccountScreen" -C5Length of output: 6921
🏁 Script executed:
#!/bin/bash
# View complete route file to see the full structure including any builder functions
cat feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/createFixedDepositAccountRoute.ktLength of output: 1116
Data class naming violates Kotlin conventions; review comment shows intended but unapplied fix
The review comment displays CreateFixedDepositAccountRoute (PascalCase) as the "final state," but the actual code defines the data class as createFixedDepositAccountRoute (camelCase). Kotlin conventions require data classes to use PascalCase. The code needs to be updated:
- Rename data class from
createFixedDepositAccountRoutetoCreateFixedDepositAccountRoute - Update composable type parameter:
composable<CreateFixedDepositAccountRoute> - Update instantiation in navigator:
CreateFixedDepositAccountRoute(clientId=clientId)
The onFinish = {} (no-op) and navigation wiring are as expected. The helper function navigateToCreateFixedDepositRoute is correctly imported and used in ClientNavigation.kt.
🤖 Prompt for AI Agents
In
feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/CreateFixedDepositAccountRoute.kt
around lines 17 to 20, the data class is incorrectly named using camelCase;
rename the type from createFixedDepositAccountRoute to
CreateFixedDepositAccountRoute (PascalCase), then update all usages: change the
composable type parameter to composable<CreateFixedDepositAccountRoute> and
update instantiation in navigator to CreateFixedDepositAccountRoute(clientId =
clientId); keep onFinish and navigation wiring as-is.
| MifosTextFieldDropdown( | ||
| value = if (state.fixedDepositAccountTerms.depositPeriodTypeIndex != -1) { | ||
| state.template.periodFrequencyTypeOptions?.getOrNull(state.fixedDepositAccountTerms.depositPeriodTypeIndex)?.value.orEmpty() | ||
| } else { | ||
| "" | ||
| }, | ||
| options = state.template.periodFrequencyTypeOptions?.map { | ||
| it.value.orEmpty() | ||
| } ?: emptyList(), | ||
| onValueChanged = {}, | ||
| onOptionSelected = { id, name -> | ||
| onAction(NewFixedDepositAccountAction.NewFixedDepositAccountTermsAction.SetFixedDepositPeriodType(id)) | ||
| }, | ||
| label = stringResource(Res.string.feature_fixed_deposit_deposit_period_type), | ||
| modifier = Modifier.fillMaxWidth(), | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
Verify that the viewmodel stores the index, not the option ID.
The UI-side implementation appears correct: the state now uses depositPeriodTypeIndex, looks up values by index (line 95), and onOptionSelected receives the dropdown index as its first parameter. However, based on the past review comment, please verify that the viewmodel's SetFixedDepositPeriodType action handler now stores the index directly rather than converting it to an option ID.
Run the following script to verify the viewmodel implementation:
🏁 Script executed:
#!/bin/bash
# Description: Check that the viewmodel stores indices not IDs for dropdown state
# Find the viewmodel file and check the action handlers
ast-grep --pattern $'handleSetFixedDepositPeriodType($$$) {
$$$
}'Length of output: 0
🏁 Script executed:
#!/bin/bash
# Find the viewmodel file for NewFixedDepositAccount
fd -type f -name "*ViewModel*" | xargs grep -l "SetFixedDepositPeriodType" 2>/dev/nullLength of output: 170
🏁 Script executed:
#!/bin/bash
# Search for SetFixedDepositPeriodType action handler in the codebase
rg -n "SetFixedDepositPeriodType" --type kotlin -A 10 -B 2Length of output: 9477
🏁 Script executed:
#!/bin/bash
# Find the NewFixedDepositAccount action definitions
fd -type f -name "*.kt" | xargs grep -l "NewFixedDepositAccountAction" | head -5Length of output: 170
Critical field mismatch: UI and viewmodel are updating different state fields.
The UI reads from state.fixedDepositAccountTerms.depositPeriodTypeIndex (line 94 of TermsPage.kt), but the viewmodel's handleSetFixedDepositPeriodType writes to state.fixedDepositAccountTerms.depositPeriodType (createFixedDepositAccountViewmodel.kt line 211). Additionally, the viewmodel stores the option's id rather than the index itself (?.id ?: -1), perpetuating the index/ID confusion from the original issue.
Update handleSetFixedDepositPeriodType (createFixedDepositAccountViewmodel.kt:207-217) to:
- Write to
depositPeriodTypeIndexinstead ofdepositPeriodType - Store
action.depositPeriodTypedirectly (the index), not?.id
🤖 Prompt for AI Agents
In
feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/pages/TermsPage.kt
(lines ~93-108) and
feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/createFixedDepositAccountViewmodel.kt
(lines ~207-217), the UI reads depositPeriodTypeIndex but the ViewModel writes
depositPeriodType and stores an option id; change the ViewModel handler
handleSetFixedDepositPeriodType to update
state.fixedDepositAccountTerms.depositPeriodTypeIndex (not depositPeriodType)
and assign the incoming action.depositPeriodType value directly (treat it as the
index), ensuring you immutably copy/update the state field with the integer
index and remove any logic that converts to or stores an option id.
3 participants
Fixes - Jira-#558
Screen_recording_20251119_000522.mp4
Summary by CodeRabbit
New Features
Style
Chores