Mifosac 560 new fixed deposits account interest charts step #2515
Conversation
|
@KadariPavani can you upload the screenshots what you have updated |
WalkthroughAdds two public data models (RateChartItem, ChartInformation) and a public composable InterestPage(onNext). Implements an in-page, modal-driven Rate Chart UI with listing, selection, action modal, edit dialog with validation, and delete confirmation using in-memory sample data and local state. Changes
Sequence DiagramsequenceDiagram
participant User
participant InterestPage
participant RateChartModal
participant ActionModal
participant EditDialog
participant DeleteDialog
User->>InterestPage: Open InterestPage / tap "View Rate Chart"
InterestPage->>RateChartModal: show list (sample data)
User->>RateChartModal: select RateChartItem
RateChartModal->>ActionModal: present Edit / Delete / Download options
alt Edit
User->>ActionModal: Choose Edit
ActionModal->>EditDialog: open with item
User->>EditDialog: modify & confirm
EditDialog->>InterestPage: update item state
else Delete
User->>ActionModal: Choose Delete
ActionModal->>DeleteDialog: open confirmation
User->>DeleteDialog: confirm
DeleteDialog->>InterestPage: remove item
else Download
User->>ActionModal: Choose Download
ActionModal->>InterestPage: trigger download placeholder
end
User->>InterestPage: Navigate Back / Next
InterestPage->>User: navigate away
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches✅ 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: 13
🧹 Nitpick comments (11)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/pages/InterestPage.kt (4)
78-85: Make dialog state saveable across config changes.Use rememberSaveable for transient UI state (dialogs, selection).
+// add import (top-level) +import androidx.compose.runtime.saveable.rememberSaveable @@ - var showRateChartModal by remember { mutableStateOf(false) } - var showActionModal by remember { mutableStateOf(false) } - var showEditDialog by remember { mutableStateOf(false) } - var showDeleteConfirmation by remember { mutableStateOf(false) } - var selectedItemIndex by remember { mutableStateOf(-1) } + var showRateChartModal by rememberSaveable { mutableStateOf(false) } + var showActionModal by rememberSaveable { mutableStateOf(false) } + var showEditDialog by rememberSaveable { mutableStateOf(false) } + var showDeleteConfirmation by rememberSaveable { mutableStateOf(false) } + var selectedItemIndex by rememberSaveable { mutableStateOf(-1) }
124-136: Wire the Back action; expose onBack with a safe default.Back button is a no‑op. Add onBack param and invoke it.
-fun InterestPage(onNext: () -> Unit) { +fun InterestPage(onNext: () -> Unit, onBack: () -> Unit = {}) { @@ - OutlinedButton( - onClick = { /* Handle back - will be managed by stepper */ }, + OutlinedButton( + onClick = onBack, modifier = Modifier.weight(1f), shape = RoundedCornerShape(8.dp) ) { Text("Back") }Also applies to: 79-80
397-401: Use proper icons instead of raw glyphs for arrows/indicators.Replace "▲" and "›" text with material icons for better a11y and theme consistency.
+// add imports (top-level) +import androidx.compose.material.icons.filled.ArrowUpward +import androidx.compose.material.icons.filled.KeyboardArrowRight @@ - Text( - text = "▲", - fontSize = 10.sp, - color = Color(0xFF4CAF50) - ) + Icon( + imageVector = Icons.Filled.ArrowUpward, + contentDescription = null + ) @@ - Text( - text = "›", - fontSize = 24.sp, - color = Color(0xFF9E9E9E), - modifier = Modifier.padding(start = 2.dp) - ) + Icon( + imageVector = Icons.Filled.KeyboardArrowRight, + contentDescription = null, + tint = MaterialTheme.colorScheme.onSurfaceVariant + ) @@ - Text( - text = "›", - fontSize = 24.sp, - color = Color(0xFF9E9E9E), - modifier = Modifier.padding(start = 2.dp) - ) + Icon( + imageVector = Icons.Filled.KeyboardArrowRight, + contentDescription = null, + tint = MaterialTheme.colorScheme.onSurfaceVariant + )Also applies to: 524-530, 693-697
148-163: Download actions are stubs; thread through callbacks to ViewModel.onDownload handlers are TODOs. Expose onDownloadAll/onDownloadItem and delegate to VM/service (export to CSV/PDF or share intent).
Minimal signature change:
-private fun RateChartModal( - rateChartItems: List<RateChartItem>, - onDismiss: () -> Unit, - onItemClick: (Int) -> Unit, - onDownload: () -> Unit +private fun RateChartModal( + rateChartItems: List<RateChartItem>, + onDismiss: () -> Unit, + onItemClick: (Int) -> Unit, + onDownloadAll: () -> Unit ) @@ - Button( - onClick = onDownload, + Button( + onClick = onDownloadAll,Then add onDownloadItem: (Int) -> Unit to ActionModal and call it from the item’s "Download" action.
Also applies to: 181-183
feature/client/src/commonMain/kotlin/com/mifos/feature/client/fixedDepositAccount/creation/FixedDepositAccountViewModel.kt (1)
86-89: Download action shows success without implementation.Emit a neutral info or disable until implemented to avoid misleading UX.
-// TODO: Implement chart download -sendEvent(FixedDepositAccountEvent.ShowDownloadSuccess) +sendEvent(FixedDepositAccountEvent.ShowError("Download not implemented yet"))Or gate the “Download” button behind actual availability.
feature/client/src/commonMain/kotlin/com/mifos/feature/client/fixedDepositAccount/creation/pages/InterestChartsPage.kt (2)
70-92: Externalize hardcoded strings and use theme colors for accessibility.Replace inline strings (“Chart Name”, “Back”, “Next”, “Download”, etc.) with string resources; avoid
Color.Greenand preferMaterialTheme.colorScheme.*.Example:
-Text("View Interest Rate Chart") +Text(stringResource(Res.string.feature_fixed_deposit_view_interest_chart)) -Text("Back") +Text(stringResource(Res.string.common_back)) -Text("Next") +Text(stringResource(Res.string.common_next)) -Text("Interest Rate: ${chart.interestRate}%") +Text(stringResource(Res.string.feature_fixed_deposit_interest_rate_fmt, chart.interestRate)) - color = Color.Green + color = MaterialTheme.colorScheme.primary -Text("Download") +Text(stringResource(Res.string.common_download)) -Text("Close") +Text(stringResource(Res.string.common_close)) -Text("Confirm Delete") +Text(stringResource(Res.string.common_confirm_delete)) -Text("Are you sure you want to delete this interest rate entry?") +Text(stringResource(Res.string.feature_fixed_deposit_delete_rate_confirm)) -Text("Delete") +Text(stringResource(Res.string.common_delete)) -Text("Cancel") +Text(stringResource(Res.string.common_cancel))Add missing keys in composeResources accordingly.
Also applies to: 96-101, 112-124, 127-165, 170-190, 237-249
127-149: Large list inside AlertDialog text: constrain height to avoid overflow.
LazyColumninAlertDialogbody can overflow on small screens. Wrap in aBoxwith a max height or useDialogProperties(usePlatformDefaultWidth = false)with a sized container.- text = { - LazyColumn { + text = { + Box(Modifier.heightIn(max = DesignToken.sizes.dialogBodyMax)) { + LazyColumn { items(state.interestRateCharts) { chart -> /* ... */ } - } + } + } }feature/client/src/commonMain/kotlin/com/mifos/feature/client/fixedDepositAccount/creation/FixedDepositAccountState.kt (2)
18-18: State imports model from a UI package; decouple layers.Importing
InterestRateChartfrom...pagesties state/domain to UI. Move the model to a non-UI package/file and import from there.-import com.mifos.feature.client.fixedDepositAccount.creation.pages.InterestRateChart +import com.mifos.feature.client.fixedDepositAccount.creation.model.InterestRateChartCreate
creation/model/InterestRateChart.ktwith the data class.Also applies to: 30-36
30-36: Dates as String: consider kotlinx-datetime for correctness.
fromDate/endDateasStringmake validation/formatting hard. PreferLocalDatefromkotlinx-datetimeand format in UI.feature/client/src/commonMain/kotlin/com/mifos/feature/client/fixedDepositAccount/creation/FixedDepositAccountCreationScreen.kt (2)
41-52: Handle navigation events to move the Stepper.Update step when receiving
NavigateToStep.is FixedDepositAccountEvent.NavigateToStep -> { - // Handle step navigation + viewModel.trySendAction(FixedDepositAccountAction.SetStep(event.step)) }
33-38: Accept and use initial args from route.Add
initialClientIdandinitialStepparameters (see Route comment), and initialize state once.-fun FixedDepositAccountCreationScreen( +fun FixedDepositAccountCreationScreen( navController: NavController, navigateBack: () -> Unit, modifier: Modifier = Modifier, - viewModel: FixedDepositAccountViewModel = koinViewModel() + viewModel: FixedDepositAccountViewModel = koinViewModel(), + initialClientId: Int = -1, + initialStep: Int = 3, ) { val state by viewModel.stateFlow.collectAsStateWithLifecycle() + LaunchedEffect(initialClientId, initialStep) { + viewModel.trySendAction(FixedDepositAccountAction.SetStep(initialStep.coerceIn(0, 5))) + // Optionally store clientId in state if needed + }Also applies to: 55-63
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
feature/client/src/commonMain/composeResources/values/strings.xml(1 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/fixedDepositAccount/creation/FixedDepositAccountCreationRoute.kt(1 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/fixedDepositAccount/creation/FixedDepositAccountCreationScreen.kt(1 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/fixedDepositAccount/creation/FixedDepositAccountState.kt(1 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/fixedDepositAccount/creation/FixedDepositAccountViewModel.kt(1 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/fixedDepositAccount/creation/pages/InterestChartsPage.kt(1 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/pages/InterestPage.kt(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/pages/InterestPage.kt (1)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/fixedDepositAccount/creation/pages/InterestChartsPage.kt (1)
RateChartItem(194-253)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/fixedDepositAccount/creation/FixedDepositAccountCreationScreen.kt (3)
core/ui/src/commonMain/kotlin/com/mifos/core/ui/util/EventsEffect.kt (1)
EventsEffect(28-43)feature/client/src/commonMain/kotlin/com/mifos/feature/client/fixedDepositAccount/creation/pages/InterestChartsPage.kt (1)
InterestChartsPage(39-192)core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosStepper.kt (1)
MifosStepper(47-137)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/fixedDepositAccount/creation/FixedDepositAccountCreationRoute.kt (1)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/fixedDepositAccount/creation/FixedDepositAccountCreationScreen.kt (1)
FixedDepositAccountCreationScreen(32-116)
🔇 Additional comments (4)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/pages/InterestPage.kt (1)
137-144: Good guard: disallow Next when no chart items.Enabling Next only when rateChartItems.isNotEmpty() aligns with “Non-empty chart requirement.”
Confirm the stepper also enforces this rule on navigation to prevent backdoor progression via programmatic calls.
feature/client/src/commonMain/kotlin/com/mifos/feature/client/fixedDepositAccount/creation/FixedDepositAccountCreationRoute.kt (2)
41-51: LGTM for type-safe navigation helper.Helper reads well and keeps callers concise.
23-27: Route step parameter is not used in screen implementation.The
stepparameter inFixedDepositAccountCreationRouteis defined with a default of 0, but it's never extracted or used byFixedDepositAccountCreationScreen. The screen relies entirely onstate.currentStepfrom the ViewModel (which defaults to 4), not the route parameter. Validation of the route's step parameter is unnecessary since deep-linked values would be ignored anyway. If step navigation is intended to be routable, the screen implementation needs to extract and use the route parameter first.Likely an incorrect or invalid review comment.
feature/client/src/commonMain/composeResources/values/strings.xml (1)
31-39: Well‑organized localization strings for Fixed Deposit Account creation.The new string resources follow established naming conventions, are properly organized with a comment header, and align with the multi‑step account creation flow described in the PR (Product → Details → Terms → Interest Charts → Charges → Preview, plus Chart Information).
| composable<FixedDepositAccountCreationRoute> { | ||
| FixedDepositAccountCreationScreen( | ||
| navController = navController, | ||
| navigateBack = navigateBack | ||
| ) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Route args are ignored; wire clientId/step into screen/VM.
Currently you don’t read FixedDepositAccountCreationRoute inside the destination, so clientId and step are dropped. Parse the typed route and pass values onward (screen or VM).
Apply:
- composable<FixedDepositAccountCreationRoute> {
- FixedDepositAccountCreationScreen(
+ composable<FixedDepositAccountCreationRoute> { backStackEntry ->
+ val route = backStackEntry.toRoute<FixedDepositAccountCreationRoute>()
+ FixedDepositAccountCreationScreen(
navController = navController,
- navigateBack = navigateBack
+ navigateBack = navigateBack,
+ initialClientId = route.clientId,
+ initialStep = route.step
)
}Add matching parameters to FixedDepositAccountCreationScreen and forward to the VM (via Koin parameters or an init action).
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
feature/client/src/commonMain/kotlin/com/mifos/feature/client/fixedDepositAccount/creation/FixedDepositAccountCreationRoute.kt
around lines 33 to 38, the composable is ignoring the typed
FixedDepositAccountCreationRoute so clientId and step are dropped; update the
destination to read/parse the route instance, extract clientId and step, add
matching parameters to FixedDepositAccountCreationScreen signature, and forward
those values into the screen's ViewModel either by supplying Koin parameters
when resolving the VM or by dispatching an init action from the screen to the
VM.
| val steps = listOf( | ||
| Step( | ||
| title = stringResource(Res.string.feature_fixed_deposit_step_product), | ||
| content = { | ||
| // Product Selection Page | ||
| } | ||
| ), | ||
| Step( | ||
| title = stringResource(Res.string.feature_fixed_deposit_step_details), | ||
| content = { | ||
| // Details Page | ||
| } | ||
| ), | ||
| Step( | ||
| title = stringResource(Res.string.feature_fixed_deposit_step_terms), | ||
| content = { | ||
| // Terms Page | ||
| } | ||
| ), | ||
| Step( | ||
| title = stringResource(Res.string.feature_fixed_deposit_step_interest), | ||
| content = { | ||
| InterestChartsPage( | ||
| state = state, | ||
| onAction = remember(viewModel) { { viewModel.trySendAction(it) } } | ||
| ) | ||
| } | ||
| ), | ||
| Step( | ||
| title = stringResource(Res.string.feature_fixed_deposit_step_charges), | ||
| content = { | ||
| // Charges Page | ||
| } | ||
| ), | ||
| Step( | ||
| title = stringResource(Res.string.feature_fixed_deposit_step_preview), | ||
| content = { | ||
| // Preview Page | ||
| } | ||
| ) | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
Step API mismatch: use name = instead of title =.
MifosStepper renders step.name; passing title = won’t compile if Step doesn’t define that parameter.
Apply:
- Step(
- title = stringResource(Res.string.feature_fixed_deposit_step_product),
+ Step(
+ name = stringResource(Res.string.feature_fixed_deposit_step_product),
content = { /* ... */ }
),
- Step(
- title = stringResource(Res.string.feature_fixed_deposit_step_details),
+ Step(
+ name = stringResource(Res.string.feature_fixed_deposit_step_details),
content = { /* ... */ }
),
- Step(
- title = stringResource(Res.string.feature_fixed_deposit_step_terms),
+ Step(
+ name = stringResource(Res.string.feature_fixed_deposit_step_terms),
content = { /* ... */ }
),
- Step(
- title = stringResource(Res.string.feature_fixed_deposit_step_interest),
+ Step(
+ name = stringResource(Res.string.feature_fixed_deposit_step_interest),
content = { /* ... */ }
),
- Step(
- title = stringResource(Res.string.feature_fixed_deposit_step_charges),
+ Step(
+ name = stringResource(Res.string.feature_fixed_deposit_step_charges),
content = { /* ... */ }
),
- Step(
- title = stringResource(Res.string.feature_fixed_deposit_step_preview),
+ Step(
+ name = stringResource(Res.string.feature_fixed_deposit_step_preview),
content = { /* ... */ }
)🏁 Script executed:
#!/bin/bash
# Confirm Step's API
rg -nP --type=kotlin "data\\s+class\\s+Step\\s*\\(" core/ui | sed -n '1,120p'Length of output: 94
---
🏁 Script executed:
```shell
#!/bin/bash
# Extract full Step data class definition
cat core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosStepper.kt | sed -n '42,60p'
Length of output: 408
---
🏁 Script executed:
```shell
#!/bin/bash
# Verify the actual code in FixedDepositAccountCreationScreen.kt uses title =
cat feature/client/src/commonMain/kotlin/com/mifos/feature/client/fixedDepositAccount/creation/FixedDepositAccountCreationScreen.kt | sed -n '64,104p'
Length of output: 1575
Change title = to name = in all Step instantiations (lines 66, 71, 76, 81, 89, 97).
Step's constructor parameter is name, not title. The current code will not compile.
- Step(
- title = stringResource(Res.string.feature_fixed_deposit_step_product),
+ Step(
+ name = stringResource(Res.string.feature_fixed_deposit_step_product),
content = { /* ... */ }
),
- Step(
- title = stringResource(Res.string.feature_fixed_deposit_step_details),
+ Step(
+ name = stringResource(Res.string.feature_fixed_deposit_step_details),
content = { /* ... */ }
),
- Step(
- title = stringResource(Res.string.feature_fixed_deposit_step_terms),
+ Step(
+ name = stringResource(Res.string.feature_fixed_deposit_step_terms),
content = { /* ... */ }
),
- Step(
- title = stringResource(Res.string.feature_fixed_deposit_step_interest),
+ Step(
+ name = stringResource(Res.string.feature_fixed_deposit_step_interest),
content = { /* ... */ }
),
- Step(
- title = stringResource(Res.string.feature_fixed_deposit_step_charges),
+ Step(
+ name = stringResource(Res.string.feature_fixed_deposit_step_charges),
content = { /* ... */ }
),
- Step(
- title = stringResource(Res.string.feature_fixed_deposit_step_preview),
+ Step(
+ name = stringResource(Res.string.feature_fixed_deposit_step_preview),
content = { /* ... */ }
)🤖 Prompt for AI Agents
In
feature/client/src/commonMain/kotlin/com/mifos/feature/client/fixedDepositAccount/creation/FixedDepositAccountCreationScreen.kt
around lines 64 to 104, the Step instances use the wrong constructor parameter
`title`; change each `title =` to `name =` for all Step(...) calls (lines ~66,
71, 76, 81, 89, 97) so the code matches the Step constructor and compiles; keep
the stringResource(...) expressions as-is and only rename the parameter.
| onStepChange = { newStep -> | ||
| // Handle step change here | ||
| }, | ||
| modifier = Modifier.fillMaxWidth() | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Wire onStepChange to update state.
Currently a no-op; hook into the VM.
- onStepChange = { newStep ->
- // Handle step change here
- },
+ onStepChange = { newStep ->
+ viewModel.trySendAction(FixedDepositAccountAction.SetStep(newStep))
+ },📝 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.
| onStepChange = { newStep -> | |
| // Handle step change here | |
| }, | |
| modifier = Modifier.fillMaxWidth() | |
| ) | |
| onStepChange = { newStep -> | |
| viewModel.trySendAction(FixedDepositAccountAction.SetStep(newStep)) | |
| }, | |
| modifier = Modifier.fillMaxWidth() | |
| ) |
🤖 Prompt for AI Agents
In
feature/client/src/commonMain/kotlin/com/mifos/feature/client/fixedDepositAccount/creation/FixedDepositAccountCreationScreen.kt
around lines 109 to 113, the onStepChange lambda is currently a no-op; hook it
up to the screen's ViewModel so UI step changes update state. Replace the empty
lambda with a call into the VM (e.g., viewModel.onStepChange(newStep) or
viewModel.setCurrentStep(newStep)); if the ViewModel lacks a suitable method,
add one that updates its step/state and exposes it via State/Flow so the UI
re-composes accordingly. Ensure the call runs on the main thread if needed and
remove the placeholder comment.
| data class FixedDepositAccountState( | ||
| val clientId: Int = -1, | ||
| val currentStep: Int = 4, | ||
| val template: FixedDepositTemplate? = null, | ||
| val interestRateCharts: List<InterestRateChart> = emptyList(), | ||
| val selectedChart: InterestRateChart? = null, | ||
| val error: String? = null, | ||
| val isLoading: Boolean = false | ||
| ) |
There was a problem hiding this comment.
currentStep default points to the wrong page.
With 6 steps and zero-based indexing, Interest step index is 3, but default is 4 (Charges). Set to the intended starting step or read from route.
- val currentStep: Int = 4,
+ val currentStep: Int = 3,Also consider initializing from the route (see Route comment).
📝 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 FixedDepositAccountState( | |
| val clientId: Int = -1, | |
| val currentStep: Int = 4, | |
| val template: FixedDepositTemplate? = null, | |
| val interestRateCharts: List<InterestRateChart> = emptyList(), | |
| val selectedChart: InterestRateChart? = null, | |
| val error: String? = null, | |
| val isLoading: Boolean = false | |
| ) | |
| data class FixedDepositAccountState( | |
| val clientId: Int = -1, | |
| val currentStep: Int = 3, | |
| val template: FixedDepositTemplate? = null, | |
| val interestRateCharts: List<InterestRateChart> = emptyList(), | |
| val selectedChart: InterestRateChart? = null, | |
| val error: String? = null, | |
| val isLoading: Boolean = false | |
| ) |
🤖 Prompt for AI Agents
In
feature/client/src/commonMain/kotlin/com/mifos/feature/client/fixedDepositAccount/creation/FixedDepositAccountState
around lines 20 to 28, the default currentStep is set to 4 which points to the
Charges page; change the default to 3 (the Interest step, using zero-based
indexing) or initialize currentStep from the routing entry instead of a
hardcoded value; update the constructor default to currentStep: Int = 3 or
replace the default with a call that reads the initial step from the route so
the state starts on the intended page.
| sealed class FixedDepositAccountAction { | ||
| object NavigateBack : FixedDepositAccountAction() | ||
| object NavigateNext : FixedDepositAccountAction() | ||
| object DownloadChart : FixedDepositAccountAction() | ||
| data class EditInterestRate(val chart: InterestRateChart) : FixedDepositAccountAction() | ||
| data class DeleteInterestRate(val chartId: String) : FixedDepositAccountAction() | ||
| data class AddInterestRate( | ||
| val amountRange: String, | ||
| val period: String, | ||
| val interestRate: Double, | ||
| val description: String? | ||
| ) : FixedDepositAccountAction() | ||
| data class UpdateInterestRate( | ||
| val chartId: String, | ||
| val amountRange: String, | ||
| val period: String, | ||
| val interestRate: Double, | ||
| val description: String? | ||
| ) : FixedDepositAccountAction() | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add a SetStep action to support direct step changes (Stepper + events).
To make Stepper interactive and handle NavigateToStep intents, add:
sealed class FixedDepositAccountAction {
+ data class SetStep(val step: Int) : FixedDepositAccountAction()
object NavigateBack : FixedDepositAccountAction()
object NavigateNext : FixedDepositAccountAction()
...
}Handle it in the ViewModel (see corresponding comment).
📝 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.
| sealed class FixedDepositAccountAction { | |
| object NavigateBack : FixedDepositAccountAction() | |
| object NavigateNext : FixedDepositAccountAction() | |
| object DownloadChart : FixedDepositAccountAction() | |
| data class EditInterestRate(val chart: InterestRateChart) : FixedDepositAccountAction() | |
| data class DeleteInterestRate(val chartId: String) : FixedDepositAccountAction() | |
| data class AddInterestRate( | |
| val amountRange: String, | |
| val period: String, | |
| val interestRate: Double, | |
| val description: String? | |
| ) : FixedDepositAccountAction() | |
| data class UpdateInterestRate( | |
| val chartId: String, | |
| val amountRange: String, | |
| val period: String, | |
| val interestRate: Double, | |
| val description: String? | |
| ) : FixedDepositAccountAction() | |
| } | |
| sealed class FixedDepositAccountAction { | |
| data class SetStep(val step: Int) : FixedDepositAccountAction() | |
| object NavigateBack : FixedDepositAccountAction() | |
| object NavigateNext : FixedDepositAccountAction() | |
| object DownloadChart : FixedDepositAccountAction() | |
| data class EditInterestRate(val chart: InterestRateChart) : FixedDepositAccountAction() | |
| data class DeleteInterestRate(val chartId: String) : FixedDepositAccountAction() | |
| data class AddInterestRate( | |
| val amountRange: String, | |
| val period: String, | |
| val interestRate: Double, | |
| val description: String? | |
| ) : FixedDepositAccountAction() | |
| data class UpdateInterestRate( | |
| val chartId: String, | |
| val amountRange: String, | |
| val period: String, | |
| val interestRate: Double, | |
| val description: String? | |
| ) : FixedDepositAccountAction() | |
| } |
| data class InterestRateChart( | ||
| val id: String, | ||
| val amountRange: String, | ||
| val period: String, | ||
| val interestRate: Double, | ||
| val description: String? = null | ||
| ) No newline at end of file |
There was a problem hiding this comment.
🧩 Analysis chain
Move InterestRateChart out of UI file to a shared model.
State/VM import this UI-local class, creating an undesirable UI→domain coupling.
- Create
creation/model/InterestRateChart.kt(same package...creation) and move the data class there. - Update imports in state and VM to that model.
🏁 Script executed:
#!/bin/bash
rg -nP --type=kotlin "\bInterestRateChart\b" -C2Length of output: 7570
🏁 Script executed:
#!/bin/bash
# Verify all usages of InterestRateChart across the entire codebase
rg -n "InterestRateChart" --type=kotlin -g '!test' -g '!Test'Length of output: 1797
🏁 Script executed:
#!/bin/bash
# Check if model directory already exists in the creation package
fd -t d "model" feature/client/src/commonMain/kotlin/com/mifos/feature/client/fixedDepositAccount/creation/Length of output: 0
🏁 Script executed:
#!/bin/bash
# Verify the file structure of the creation package
fd -t f --max-depth 3 . feature/client/src/commonMain/kotlin/com/mifos/feature/client/fixedDepositAccount/creation/ | head -20Length of output: 615
Move InterestRateChart to a dedicated model package to eliminate UI→domain coupling.
The InterestRateChart data class is currently defined in pages/InterestChartsPage.kt but imported by both FixedDepositAccountState.kt and FixedDepositAccountViewModel.kt, creating improper layering between UI and business logic.
Refactor required:
- Create
creation/model/InterestRateChart.ktand move the data class there - Update imports in
FixedDepositAccountState.kt(line 18) andFixedDepositAccountViewModel.kt(line 20) to reference the new model package - No other usages exist beyond these two files
🤖 Prompt for AI Agents
In
feature/client/src/commonMain/kotlin/com/mifos/feature/client/fixedDepositAccount/creation/pages/InterestChartsPage.kt
lines 274-280 the InterestRateChart data class is declared in the UI page
causing UI→domain coupling; move that data class into a new model file at
feature/client/src/commonMain/kotlin/com/mifos/feature/client/fixedDepositAccount/creation/model/InterestRateChart.kt
containing the same data class definition, remove it from InterestChartsPage.kt,
and update the import in
feature/client/src/commonMain/kotlin/com/mifos/feature/client/fixedDepositAccount/creation/FixedDepositAccountState.kt
(line 18) and in
feature/client/src/commonMain/kotlin/com/mifos/feature/client/fixedDepositAccount/creation/FixedDepositAccountViewModel.kt
(line 20) to import
com.mifos.feature.client.fixedDepositAccount.creation.model.InterestRateChart
instead; verify no other usages remain and run a build to confirm imports
resolve.
| // Data class for Rate Chart Item | ||
| data class RateChartItem( | ||
| val amountRange: String, | ||
| val description: String, | ||
| val period: String, | ||
| val interestRate: String | ||
| ) | ||
|
|
||
| // Data class for Chart Information | ||
| data class ChartInformation( | ||
| val name: String, | ||
| val validFromDate: String, | ||
| val endDate: String, | ||
| val description: String, | ||
| val groupingByAmount: Boolean | ||
| ) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Unify rate‑chart models; avoid duplication and stringly‑typed fields.
This file introduces RateChartItem and ChartInformation that overlap with the creation flow’s InterestRateChart model. Duplicating models risks drift and mapping bugs. Prefer reusing the existing model (or elevate a shared domain/UI model) and keep percent/currency as numeric types with UI formatting only.
Run to locate existing definitions/usages and assess consolidation:
🏁 Script executed:
#!/bin/bash
rg -nC2 -g '!**/build/**' -P '(data\s+class\s+InterestRateChart\b|class\s+.*Interest.*Chart\b|RateChartItem\b)'Length of output: 8417
---
🏁 Script executed:
```shell
#!/bin/bash
# Extract full InterestRateChart definition from InterestChartsPage.kt
sed -n '274,290p' feature/client/src/commonMain/kotlin/com/mifos/feature/client/fixedDepositAccount/creation/pages/InterestChartsPage.kt
Length of output: 170
🏁 Script executed:
#!/bin/bash
# Extract full RateChartItem and ChartInformation definitions from InterestPage.kt
sed -n '61,76p' feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/pages/InterestPage.ktLength of output: 381
🏁 Script executed:
#!/bin/bash
# Check imports and model reachability in InterestPage.kt
head -30 feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/pages/InterestPage.kt | grep -i importLength of output: 942
🏁 Script executed:
#!/bin/bash
# Verify if InterestRateChart is exported or accessible from InterestChartsPage module
rg -n "export|public" feature/client/src/commonMain/kotlin/com/mifos/feature/client/fixedDepositAccount/creation/pages/InterestChartsPage.kt | head -5Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check the module structure: is there a shared models file or package?
fd -type f -name "*.kt" feature/client/src/commonMain/kotlin/com/mifos/feature/client/ | grep -i model | head -10Length of output: 170
🏁 Script executed:
#!/bin/bash
# Verify if RateChartItem is used outside InterestPage.kt (risk of duplication drift)
rg -l "RateChartItem" --type kotlin feature/client/src/Length of output: 225
🏁 Script executed:
#!/bin/bash
# Verify if InterestRateChart is used outside InterestChartsPage/FixedDepositAccountState
rg -l "InterestRateChart" --type kotlin feature/client/src/Length of output: 360
🏁 Script executed:
#!/bin/bash
# Check if there is any mapping logic between models in the codebase
rg -n "InterestRateChart|RateChartItem" -A 2 -B 2 feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/pages/InterestPage.kt | grep -E "(toInterest|toRate|map|convert)" || echo "No mapping found"Length of output: 16
🏁 Script executed:
#!/bin/bash
# Look at the navigation/usage between the two screens to understand data flow
rg -n "InterestPage|InterestChartsPage" feature/client/src/commonMain/kotlin/com/mifos/feature/client/ --type kotlin | grep -E "(Navigation|Route|navigate)" | head -10Length of output: 0
---
🏁 Script executed:
```shell
#!/bin/bash
# Check file context: when was InterestPage.kt added, is it preview or integration planned?
fd -name InterestPage.kt feature/client/src/ -exec ls -lh {} \;
Length of output: 230
🏁 Script executed:
#!/bin/bash
# Look for validation rules on interestRate in both files
rg -n "interestRate|rate.*valid" feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/pages/InterestPage.kt -A 1 -B 1 | head -20Length of output: 729
🏁 Script executed:
#!/bin/bash
# Check if amountRange is parsed/formatted anywhere in InterestPage.kt
rg -n "amountRange|amount.*Range|toDouble|toInt|parse" feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/pages/InterestPage.kt | head -15Length of output: 557
🏁 Script executed:
#!/bin/bash
# Verify structure: are these flows connected or separate products?
rg -n "newFixedDepositAccount|fixedDepositAccount" feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/pages/InterestPage.kt | head -5Length of output: 64
🏁 Script executed:
#!/bin/bash
# Look for the key validation constraint mentioned in review (rate 0-100)
rg -n "rate.*>.*0.*<.*100|rate.*range|percent" feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/pages/InterestPage.ktLength of output: 148
🏁 Script executed:
#!/bin/bash
# Check ChartInformation usage in InterestPage.kt
rg -n "ChartInformation" feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/pages/InterestPage.ktLength of output: 169
🏁 Script executed:
#!/bin/bash
# Determine if RateChartItem fields are ever converted to numeric
rg -nP "(amountRange|interestRate).*=.*\(" feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/pages/InterestPage.kt | head -20Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search across repo for any shared domain/UI model layers
fd -name "*.kt" feature/client/src/commonMain/kotlin/com/mifos/feature/client/ -path "*/data/*" -o -path "*/domain/*" -o -path "*/model/*" | head -20Length of output: 230
Eliminate stringly-typed numeric fields; fix validation bounds mismatch.
The models RateChartItem and ChartInformation are scoped to this new flow and don't duplicate the creation flow's InterestRateChart (separate packages). However, stringly-typed fields are the real problem:
- interestRate (line 66): stored as
String("7%"), parsed to Double only at validation (line 903). UseDoublewith UI-only formatting. - amountRange (line 63): stored as
String; no validation or bounds. Clarify: numeric range (Double..Double) or display string? - Validation bounds mismatch (line 904): code enforces
rate > 0 && rate < 100(excludes 0 and 100), but error message (line 890) says "0-100" (inclusive). Fix either validation or message.
Suggested structure:
data class RateChartItem(
val amountRangeMin: Double,
val amountRangeMax: Double,
val description: String,
val period: String,
val interestRate: Double // numeric, formatted in UI
)🤖 Prompt for AI Agents
In
feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/pages/InterestPage.kt
around lines 61 to 77, replace stringly-typed numeric fields with numeric types
and fix the bounds mismatch: change interestRate from String to Double (keep
formatting to a percent in the UI layer only), replace amountRange:String with
explicit amountRangeMin:Double and amountRangeMax:Double (or a clear numeric
range type) and update all places that parsed those strings to use the numeric
fields directly; also reconcile the validation and message by either making the
validation inclusive (rate >= 0 && rate <= 100) to match the "0-100" message or
change the message to reflect exclusive bounds, and update any
parsing/validation callsites to handle null/NaN safely and show formatted values
in the UI.
| onConfirm = { | ||
| rateChartItems.removeAt(selectedItemIndex) | ||
| showDeleteConfirmation = false | ||
| showRateChartModal = true | ||
| } | ||
| ) |
There was a problem hiding this comment.
Reset selection after delete to avoid stale index.
After removal, selectedItemIndex can point at a removed element. Reset it.
onConfirm = {
rateChartItems.removeAt(selectedItemIndex)
+ selectedItemIndex = -1
showDeleteConfirmation = false
showRateChartModal = true
}📝 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.
| onConfirm = { | |
| rateChartItems.removeAt(selectedItemIndex) | |
| showDeleteConfirmation = false | |
| showRateChartModal = true | |
| } | |
| ) | |
| onConfirm = { | |
| rateChartItems.removeAt(selectedItemIndex) | |
| selectedItemIndex = -1 | |
| showDeleteConfirmation = false | |
| showRateChartModal = true | |
| } | |
| ) |
🤖 Prompt for AI Agents
In
feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/pages/InterestPage.kt
around lines 210 to 215, after removing the item from rateChartItems you must
reset the selection to avoid selectedItemIndex pointing at a removed element;
update the onConfirm flow to set selectedItemIndex to an appropriate sentinel
(e.g., -1) or clamp it within the new list bounds after calling removeAt, and
then proceed to close the confirmation and open the modal as before.
| @Composable | ||
| private fun ChartInformationCard( | ||
| chartInfo: ChartInformation, | ||
| onViewClick: () -> Unit | ||
| ) { | ||
| Card( | ||
| modifier = Modifier.fillMaxWidth(), | ||
| shape = RoundedCornerShape(12.dp), | ||
| border = BorderStroke(1.dp, Color(0xFFE0E0E0)), | ||
| colors = CardDefaults.cardColors( | ||
| containerColor = Color.White | ||
| ), | ||
| elevation = CardDefaults.cardElevation(defaultElevation = 0.dp) | ||
| ) { | ||
| Column( | ||
| modifier = Modifier | ||
| .fillMaxWidth() | ||
| .padding(16.dp), | ||
| verticalArrangement = Arrangement.spacedBy(12.dp) | ||
| ) { | ||
| InfoRow(label = "Name:", value = chartInfo.name) | ||
| InfoRow(label = "Valid from Date:", value = chartInfo.validFromDate) | ||
| InfoRow(label = "End Date:", value = chartInfo.endDate) | ||
| InfoRow(label = "Description:", value = chartInfo.description) | ||
| InfoRow( | ||
| label = "Grouping by Amount:", | ||
| value = if (chartInfo.groupingByAmount) "Yes" else "No" | ||
| ) | ||
|
|
||
| Spacer(modifier = Modifier.height(4.dp)) | ||
|
|
||
| Row( | ||
| modifier = Modifier.fillMaxWidth(), | ||
| horizontalArrangement = Arrangement.SpaceBetween, | ||
| verticalAlignment = Alignment.CenterVertically | ||
| ) { | ||
| Text( | ||
| text = "INTEREST RATE CHART", | ||
| style = MaterialTheme.typography.labelLarge, | ||
| fontWeight = FontWeight.SemiBold, | ||
| fontSize = 13.sp, | ||
| color = Color(0xFF212121) | ||
| ) | ||
|
|
||
| TextButton( | ||
| onClick = onViewClick, | ||
| shape = RoundedCornerShape(4.dp) | ||
| ) { | ||
| Text( | ||
| text = "View", | ||
| color = MaterialTheme.colorScheme.primary, | ||
| fontSize = 14.sp, | ||
| fontWeight = FontWeight.Medium | ||
| ) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Replace hard‑coded strings/colors with resources and theme tokens (i18n + dark mode).
Literal text and colors break localization and dark theme. Use stringResource(Res.string.*) and MaterialTheme.colorScheme/typography (or DesignToken) consistently. Apply across this file.
Example edits (apply pattern broadly):
- text = "INTEREST RATE CHART",
+ text = stringResource(Res.string.interest_rate_chart_title),
@@
- TextButton(
- onClick = onViewClick,
+ TextButton(
+ onClick = onViewClick,
shape = RoundedCornerShape(4.dp)
) {
- Text(
- text = "View",
- color = MaterialTheme.colorScheme.primary,
- fontSize = 14.sp,
- fontWeight = FontWeight.Medium
- )
+ Text(stringResource(Res.string.view))
}And for colors:
- colors = CardDefaults.cardColors(containerColor = Color.White),
+ colors = CardDefaults.cardColors(containerColor = MaterialTheme.colorScheme.surface),
@@
- color = Color(0xFF212121)
+ color = MaterialTheme.colorScheme.onSurfaceIf keys don’t exist yet (e.g., view, back, next, download, rate_chart_title, amount_range, description, period, interest_rate, delete, cancel), please add them to shared resources. I can help generate them.
Also applies to: 306-459
| @OptIn(ExperimentalMaterial3Api::class) | ||
| @Composable | ||
| private fun EditRateChartDialog( | ||
| item: RateChartItem, | ||
| onDismiss: () -> Unit, | ||
| onConfirm: (RateChartItem) -> Unit | ||
| ) { | ||
| var amountRange by remember { mutableStateOf(item.amountRange) } | ||
| var description by remember { mutableStateOf(item.description) } | ||
| var period by remember { mutableStateOf(item.period) } | ||
| var interestRate by remember { mutableStateOf(item.interestRate.replace("%", "")) } | ||
| var interestRateError by remember { mutableStateOf(false) } | ||
|
|
||
| AlertDialog( | ||
| onDismissRequest = onDismiss, | ||
| shape = RoundedCornerShape(16.dp), | ||
| containerColor = Color.White, | ||
| title = { | ||
| Text( | ||
| text = "Edit Interest Rate", | ||
| style = MaterialTheme.typography.titleLarge, | ||
| fontWeight = FontWeight.Bold, | ||
| fontSize = 20.sp, | ||
| color = Color(0xFF212121) | ||
| ) | ||
| }, | ||
| text = { | ||
| Column( | ||
| modifier = Modifier | ||
| .fillMaxWidth() | ||
| .verticalScroll(rememberScrollState()), | ||
| verticalArrangement = Arrangement.spacedBy(16.dp) | ||
| ) { | ||
| OutlinedTextField( | ||
| value = amountRange, | ||
| onValueChange = { amountRange = it }, | ||
| label = { Text("Amount Range") }, | ||
| modifier = Modifier.fillMaxWidth(), | ||
| shape = RoundedCornerShape(8.dp) | ||
| ) | ||
|
|
||
| OutlinedTextField( | ||
| value = description, | ||
| onValueChange = { description = it }, | ||
| label = { Text("Description") }, | ||
| modifier = Modifier.fillMaxWidth(), | ||
| shape = RoundedCornerShape(8.dp) | ||
| ) | ||
|
|
||
| OutlinedTextField( | ||
| value = period, | ||
| onValueChange = { period = it }, | ||
| label = { Text("Period") }, | ||
| modifier = Modifier.fillMaxWidth(), | ||
| shape = RoundedCornerShape(8.dp) | ||
| ) | ||
|
|
||
| OutlinedTextField( | ||
| value = interestRate, | ||
| onValueChange = { | ||
| interestRate = it | ||
| val rate = it.toDoubleOrNull() | ||
| interestRateError = rate == null || rate <= 0 || rate >= 100 | ||
| }, | ||
| label = { Text("Interest Rate (%)") }, | ||
| isError = interestRateError, | ||
| supportingText = { | ||
| if (interestRateError) { | ||
| Text( | ||
| "Please enter a valid percentage (0-100)", | ||
| color = Color(0xFFD32F2F) | ||
| ) | ||
| } | ||
| }, | ||
| modifier = Modifier.fillMaxWidth(), | ||
| shape = RoundedCornerShape(8.dp) | ||
| ) | ||
| } | ||
| }, | ||
| confirmButton = { | ||
| Button( | ||
| onClick = { | ||
| val rate = interestRate.toDoubleOrNull() | ||
| if (rate != null && rate > 0 && rate < 100) { | ||
| onConfirm( | ||
| RateChartItem( | ||
| amountRange = amountRange, | ||
| description = description, | ||
| period = period, | ||
| interestRate = "$interestRate%" | ||
| ) | ||
| ) | ||
| } | ||
| }, | ||
| enabled = !interestRateError && interestRate.isNotEmpty(), | ||
| shape = RoundedCornerShape(8.dp), | ||
| colors = ButtonDefaults.buttonColors( | ||
| containerColor = Color(0xFF5C6BC0) | ||
| ) | ||
| ) { | ||
| Text("Save", fontWeight = FontWeight.Medium) | ||
| } | ||
| }, | ||
| dismissButton = { | ||
| TextButton( | ||
| onClick = onDismiss, | ||
| shape = RoundedCornerShape(8.dp) | ||
| ) { | ||
| Text("Cancel", color = Color(0xFF757575)) | ||
| } | ||
| } | ||
| ) | ||
| } |
There was a problem hiding this comment.
Fix validation to match 0–100% requirement and force numeric keyboard.
Bounds are exclusive; PR says 0–100%. Also prefer numeric keyboard and trimmed parsing.
+// add imports (top-level)
+import androidx.compose.foundation.text.KeyboardOptions
+import androidx.compose.ui.text.input.KeyboardType
@@
- var interestRate by remember { mutableStateOf(item.interestRate.replace("%", "")) }
+ var interestRate by remember { mutableStateOf(item.interestRate.replace("%", "")) }
var interestRateError by remember { mutableStateOf(false) }
@@
- OutlinedTextField(
+ OutlinedTextField(
value = interestRate,
onValueChange = {
- interestRate = it
- val rate = it.toDoubleOrNull()
- interestRateError = rate == null || rate <= 0 || rate >= 100
+ val input = it.trim()
+ interestRate = input
+ val rate = input.toDoubleOrNull()
+ interestRateError = rate == null || rate < 0 || rate > 100
},
label = { Text("Interest Rate (%)") },
isError = interestRateError,
+ keyboardOptions = KeyboardOptions(keyboardType = KeyboardType.Number),
+ singleLine = true,
supportingText = {
if (interestRateError) {
Text(
- "Please enter a valid percentage (0-100)",
+ "Please enter a valid percentage (0–100 inclusive)",
color = Color(0xFFD32F2F)
)
}
},
modifier = Modifier.fillMaxWidth(),
shape = RoundedCornerShape(8.dp)
)
@@
- Button(
+ Button(
onClick = {
- val rate = interestRate.toDoubleOrNull()
- if (rate != null && rate > 0 && rate < 100) {
+ val rate = interestRate.trim().toDoubleOrNull()
+ if (rate != null && rate >= 0 && rate <= 100) {
onConfirm(
RateChartItem(
amountRange = amountRange,
description = description,
period = period,
interestRate = "$interestRate%"
)
)
}
},
- enabled = !interestRateError && interestRate.isNotEmpty(),
+ enabled = !interestRateError && interestRate.isNotEmpty(),
shape = RoundedCornerShape(8.dp),
colors = ButtonDefaults.buttonColors(
containerColor = Color(0xFF5C6BC0)
)
) {
Text("Save", fontWeight = FontWeight.Medium)
}🤖 Prompt for AI Agents
feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/pages/InterestPage.kt
lines 821-933: current validation treats bounds as exclusive and parses raw
input, plus it doesn’t force a numeric keyboard; update the interestRate
handling to trim and strip any '%' before parsing (use
interestRate.trim().replace("%","")), use KeyboardOptions(keyboardType =
KeyboardType.Decimal) on the OutlinedTextField to force numeric input, change
validation to allow inclusive bounds (error when rate == null || rate < 0 ||
rate > 100) and use the same trimmed/cleaned value when converting for the
confirm action so saved interestRate uses a normalized value with a trailing
"%".
KadariPavani
force-pushed
the
MIFOSAC-560-new-fixed-deposits-account-interest-charts-step
branch
from
20d4797 to
efc0851
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/pages/InterestPage.kt (2)
210-215: This issue was already flagged in a previous review. After removing an item,selectedItemIndexmust be reset to avoid pointing at a stale or out-of-bounds index.
878-897: The validation bounds mismatch and missing numeric keyboard were already flagged in a previous review. The validation should be inclusive (0-100) to match the error message, andKeyboardOptions(keyboardType = KeyboardType.Decimal)should be added.
🧹 Nitpick comments (3)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/pages/InterestPage.kt (3)
854-876: Consider adding validation for other fields.The
amountRange,description, andperiodfields accept any input, including empty strings. WhileinterestRatehas validation, saving items with empty or invalid values in other fields could degrade UX.Consider adding:
- Format validation for
amountRange(e.g., "$200 - $250")- Non-empty validation for
descriptionandperiod- Enable/disable the Save button based on all field validation states
726-731: Add content descriptions for accessibility.The Edit and Delete icons lack
contentDescriptionvalues, which impacts screen reader accessibility. While the text labels are present, ensure each icon has a proper description for users relying on assistive technologies.Apply this pattern:
Icon( imageVector = Icons.Default.Edit, - contentDescription = "Edit", + contentDescription = null, // decorative since text is present tint = Color(0xFF757575), modifier = Modifier.padding(end = 12.dp) )Or provide meaningful descriptions if the icons can be interacted with independently of the text.
Also applies to: 762-767
158-161: Placeholder download functionality requires implementation.The download callbacks are currently no-ops. Ensure these are implemented before release or add TODO comments to track the work.
Do you want me to help outline the download implementation or open an issue to track this task?
Also applies to: 181-183
📜 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/pages/InterestPage.kt(1 hunks)
.../src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/pages/InterestPage.kt
Outdated
Show resolved
Hide resolved
please review the code as i updated the intrest chart page with the requirements that got from the issue
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/pages/InterestPage.kt (4)
329-339: Replace hard-coded strings/colors with resources and theme tokens (i18n + dark mode).Use stringResource(...) and MaterialTheme.colorScheme/typography. Sample edits below; apply the pattern broadly across this file.
- TextButton( - onClick = onViewClick, - shape = RoundedCornerShape(4.dp), - ) { - Text( - text = "View", - color = MaterialTheme.colorScheme.primary, - fontSize = 14.sp, - fontWeight = FontWeight.Medium, - ) - } + TextButton(onClick = onViewClick, shape = RoundedCornerShape(4.dp)) { + Text(stringResource(Res.string.view)) + } @@ - Surface( + Surface( modifier = Modifier .fillMaxWidth(0.92f) .padding(horizontal = 16.dp), shape = RoundedCornerShape(20.dp), - color = Color.White, + color = MaterialTheme.colorScheme.surface, ) { @@ - Text( - text = "Rate Chart", - style = MaterialTheme.typography.titleLarge, - fontWeight = FontWeight.Bold, - fontSize = 22.sp, - color = Color(0xFF212121), - modifier = Modifier.padding(bottom = 20.dp), - ) + Text( + text = stringResource(Res.string.rate_chart_title), + style = MaterialTheme.typography.titleLarge, + color = MaterialTheme.colorScheme.onSurface, + modifier = Modifier.padding(bottom = 20.dp), + ) @@ - OutlinedButton( + OutlinedButton( onClick = onDismiss, modifier = Modifier.weight(1f), shape = RoundedCornerShape(8.dp), - border = BorderStroke(1.dp, Color(0xFFBDBDBD)), + border = BorderStroke(1.dp, MaterialTheme.colorScheme.outline), ) { - Text( - "Back", - color = Color(0xFF212121), - fontSize = 15.sp, - fontWeight = FontWeight.Medium, - ) + Text(stringResource(Res.string.back)) } @@ - Button( + Button( onClick = onDownload, modifier = Modifier.weight(1f), shape = RoundedCornerShape(8.dp), - colors = ButtonDefaults.buttonColors( - containerColor = Color(0xFF5C6BC0), - ), + colors = ButtonDefaults.buttonColors(containerColor = MaterialTheme.colorScheme.primary), ) { - Text( - "Download", - fontSize = 15.sp, - fontWeight = FontWeight.Medium, - ) + Text(stringResource(Res.string.download)) }Verification helper to find literals:
#!/bin/bash rg -nP '\bText\(\s*"|label\s*=\s*\{\s*Text\("|\bColor\(' -g '!**/build/**' feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/pages/InterestPage.ktAs per coding guidelines.
Also applies to: 398-405, 446-459, 390-393, 291-296, 352-359, 361-369
205-210: Reset selection after delete to avoid stale index.After removal, selectedItemIndex still points to the deleted row. Reset it.
onConfirm = { rateChartItems.removeAt(selectedItemIndex) + selectedItemIndex = -1 showDeleteConfirmation = false showRateChartModal = true }
829-848: Make 0–100% validation inclusive and force numeric keyboard; sanitize input.Spec says 0–100%; current check is exclusive and allows stray '%' input to fail silently.
OutlinedTextField( - value = interestRate, - onValueChange = { - interestRate = it - val rate = it.toDoubleOrNull() - interestRateError = rate == null || rate <= 0 || rate >= 100 - }, + value = interestRate, + onValueChange = { + val input = it.trim().replace("%", "") + interestRate = input + val rate = input.toDoubleOrNull() + interestRateError = rate == null || rate < 0 || rate > 100 + }, label = { Text("Interest Rate (%)") }, isError = interestRateError, + keyboardOptions = KeyboardOptions(keyboardType = KeyboardType.Decimal), + singleLine = true, supportingText = { if (interestRateError) { Text( - "Please enter a valid percentage (0-100)", + "Please enter a valid percentage (0–100 inclusive)", color = Color(0xFFD32F2F), ) } }, ) @@ Button( onClick = { - val rate = interestRate.toDoubleOrNull() - if (rate != null && rate > 0 && rate < 100) { + val rate = interestRate.trim().toDoubleOrNull() + if (rate != null && rate >= 0 && rate <= 100) { onConfirm( RateChartItem( amountRange = amountRange, description = description, period = period, - interestRate = "$interestRate%", + interestRate = "$interestRate%", ), ) } },Add imports:
import androidx.compose.foundation.text.KeyboardOptions import androidx.compose.ui.text.input.KeyboardTypeAlso applies to: 852-865
57-73: Constrain API surface and prefer strong types for numeric data.
- Make these models internal to avoid leaking unstable UI types.
- Replace stringly-typed numeric fields with numeric types; format in the UI only.
Minimal visibility change (safe now):
-data class RateChartItem( +internal data class RateChartItem( @@ -) +) @@ -data class ChartInformation( +internal data class ChartInformation( @@ -) +)Recommended type refactor (apply when wiring ViewModel data):
internal data class RateChartItem( val amountRangeMin: Double, val amountRangeMax: Double, val description: String, val periodYears: Int, val interestRatePercent: Double )This removes parsing bugs and aligns with validation. Confirm if these models must be public; if not, keep them internal.
🧹 Nitpick comments (4)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/pages/InterestPage.kt (4)
413-423: Use LazyColumn for potentially long lists.Avoid Column with forEachIndexed for lists; use LazyColumn for performance and better memory use.
- Column( - modifier = Modifier.weight(1f, fill = false), - verticalArrangement = Arrangement.spacedBy(12.dp), - ) { - rateChartItems.forEachIndexed { index, item -> - RateChartItemCard( - item = item, - onClick = { onItemClick(index) }, - ) - } - } + LazyColumn( + modifier = Modifier.weight(1f), + verticalArrangement = Arrangement.spacedBy(12.dp), + ) { + itemsIndexed(rateChartItems) { index, item -> + RateChartItemCard(item = item, onClick = { onItemClick(index) }) + } + }Add imports:
import androidx.compose.foundation.lazy.LazyColumn import androidx.compose.foundation.lazy.itemsIndexed
275-279: Use vector icons (and hide decorative ones) instead of glyph text.Replace "▲" and "›" text with Icons.* and set contentDescription = null so screen readers don’t announce them.
- Text( - text = "▲", - fontSize = 10.sp, - color = Color(0xFF4CAF50), - ) + Icon( + imageVector = Icons.Default.ArrowDropUp, + contentDescription = null, + tint = Color(0xFF4CAF50), + ) @@ - Text( - text = "›", - fontSize = 24.sp, - color = Color(0xFF9E9E9E), - modifier = Modifier.padding(start = 2.dp), - ) + Icon( + imageVector = Icons.Default.KeyboardArrowRight, + contentDescription = null, + tint = Color(0xFF9E9E9E), + modifier = Modifier.padding(start = 2.dp), + )Also applies to: 532-537
75-103: Hoist UI state to ViewModel; avoid mutable state in the page for prod.Sample data + in-page state is fine for a prototype, but wire this to FixedDepositAccountViewModel and a UiState to survive recomposition and configuration changes. Also derive “Next enabled” from state (non-empty validated chart).
219-223: Remove zero-width transparent borders.BorderStroke(0.dp, Color.Transparent) is a no-op; drop it to simplify.
- border = BorderStroke(0.dp, Color.Transparent), + // border removedAlso applies to: 662-667
📜 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/pages/InterestPage.kt(1 hunks)
Record_2025-11-05-11-00-00_026f621985d6cfa1a169c52d305dc178.mp4 |
Please make you UI same as figma design which is attach in the ticket For interest details use separate card and for view interest use separate card don't include in one card and also starting and ending padding is too much which is not aline with stepper. For listing charge use pre build component and also for view interest use pre build component |
|
@Arinyadav1 please do a review now if any changes needed WhatsApp.Video.2025-11-06.at.06.17.18_9c1d8af6.mp4 |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/pages/InterestPage.kt (3)
218-223: Reset selection after delete to avoid stale index.After removing an item from
rateChartItems,selectedItemIndexremains pointing to the old index. While the bounds checks at lines 174 and 212 (selectedItemIndex >= 0) provide some safety, the index should be explicitly reset to maintain clean state and avoid off-by-one issues if the list is later accessed.Apply this fix:
onConfirm = { rateChartItems.removeAt(selectedItemIndex) + selectedItemIndex = -1 showDeleteConfirmation = false showRateChartModal = true },Based on learnings
58-73: Validation bounds mismatch causes user confusion; stringly-typed fields remain unresolved.The previous review's concerns about stringly-typed numeric fields and validation bounds are still present:
Validation bounds mismatch (lines 800, 807, 821): Code enforces exclusive bounds (
rate <= 0 || rate >= 100andrate > 0 && rate < 100), but the error message says "0-100", implying inclusive. Users entering exactly 0% or 100% will see validation errors despite the message suggesting these are valid.Stringly-typed numeric fields:
interestRatestored as"7%"requires parsing every time it's validated or processed.amountRangestored as"$200 - $250"has no validation at all. Use numeric types (Doubleor range types) with UI-only formatting.Fix the validation bounds mismatch first:
Choose one approach:
- Option A (inclusive): Change validation to
rate < 0 || rate > 100and message to "0–100 inclusive"- Option B (exclusive): Keep validation as-is and change message to "greater than 0 and less than 100"
Then refactor to numeric types:
data class RateChartItem( - val amountRange: String, + val amountRangeMin: Double, + val amountRangeMax: Double, val description: String, val period: String, - val interestRate: String, + val interestRate: Double // Store as decimal, format to "X%" in UI )Update initialization (line 96-101) and all usages to work with numeric values, formatting only for display.
795-830: Fix validation bounds mismatch and add numeric keyboard for better UX.The validation logic and error message are inconsistent, and the lack of a numeric keyboard makes data entry error-prone:
Validation bounds mismatch: Line 800 uses
rate <= 0 || rate >= 100(exclusive), line 821 usesrate > 0 && rate < 100(exclusive), but line 807's message says "0-100" (implies inclusive). Users entering 0 or 100 will see unexpected errors.Missing numeric keyboard: Without
KeyboardType.Decimal, users see the full text keyboard, increasing the chance of invalid input.No input trimming: Whitespace could cause
toDoubleOrNull()to fail.Apply this fix:
+import androidx.compose.foundation.text.KeyboardOptions +import androidx.compose.ui.text.input.KeyboardType // In EditRateChartDialog: - var interestRate by remember { mutableStateOf(item.interestRate.replace("%", "")) } + var interestRate by remember { mutableStateOf(item.interestRate.replace("%", "").trim()) } var interestRateError by remember { mutableStateOf(false) } OutlinedTextField( value = interestRate, onValueChange = { - interestRate = it - val rate = it.toDoubleOrNull() - interestRateError = rate == null || rate <= 0 || rate >= 100 + interestRate = it.trim() + val rate = it.trim().toDoubleOrNull() + interestRateError = rate == null || rate < 0 || rate > 100 }, label = { Text("Interest Rate (%)") }, isError = interestRateError, + keyboardOptions = KeyboardOptions(keyboardType = KeyboardType.Decimal), + singleLine = true, supportingText = { if (interestRateError) { Text( - "Please enter a valid percentage (0-100)", + "Please enter a valid percentage (0–100 inclusive)", color = Color(0xFFD32F2F), ) } }, modifier = Modifier.fillMaxWidth(), shape = RoundedCornerShape(8.dp), ) Button( onClick = { - val rate = interestRate.toDoubleOrNull() - if (rate != null && rate > 0 && rate < 100) { + val rate = interestRate.trim().toDoubleOrNull() + if (rate != null && rate >= 0 && rate <= 100) { onConfirm( RateChartItem( amountRange = amountRange, description = description, period = period, - interestRate = "$interestRate%", + interestRate = "${interestRate.trim()}%", ), ) } },Based on learnings
🧹 Nitpick comments (1)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/pages/InterestPage.kt (1)
228-315: Migrate hard-coded strings and colors to resources for i18n and theming.The file contains numerous hard-coded strings and colors that prevent localization and break dark mode support. While the previous review flagged this extensively, it remains unaddressed:
Hard-coded strings (breaks i18n):
- UI labels: "Interest Details" (line 249), "INTEREST RATE CHART" (line 292), "View" (line 307), "Rate Chart" (line 444), "Edit Interest Rate" (line 757), etc.
- Field labels: "Name:", "Valid from Date:", "Description:" (lines 256-263)
- Buttons: "Back" (line 484), "Download" (line 500), "Save" (line 838), "Cancel" (line 846), "Delete" (line 885)
- Messages: "Are you sure you want to delete..." (line 872), "Please enter a valid percentage..." (line 807)
Hard-coded colors (breaks dark mode):
Color.Whitefor card backgrounds (lines 238, 280, 435, etc.) — should useMaterialTheme.colorScheme.surfaceColor(0xFF212121)for text (lines 253, 296, etc.) — should useMaterialTheme.colorScheme.onSurfaceColor(0xFF757575)for secondary text — should useMaterialTheme.colorScheme.onSurfaceVariant- Spot uses of
MaterialTheme.colorScheme.primary(line 308) show the right patternExample migration pattern (apply throughout):
+import androidx.compose.ui.res.stringResource +// Assuming you have a Res object or similar resource system - text = "Interest Details", + text = stringResource(Res.string.interest_details_title), - color = Color(0xFF212121), + color = MaterialTheme.colorScheme.onSurface, - colors = CardDefaults.cardColors(containerColor = Color.White), + colors = CardDefaults.cardColors(containerColor = MaterialTheme.colorScheme.surface),Add missing string keys to your resources (e.g.,
strings.xmlor resource file):
interest_details_title,interest_rate_chart_title,view,back,next,download,rate_chart,edit,delete,save,cancelname_label,valid_from_date_label,end_date_label,description_label,grouping_by_amount_labelamount_range,period,interest_rateedit_interest_rate_title,delete_confirmation_title,delete_confirmation_message,interest_rate_validation_errorBased on learnings
Also applies to: 346-509, 589-897
📜 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/pages/InterestPage.kt(1 hunks)
this is also not same as attach UI check so also make it same as. |
For all this UI we have already in-built component in for listing data use first make these changes. |
3 participants
Fixes - Jira-#560
Didn't create a Jira ticket, click here to create new.
Please Add Screenshots If there are any UI changes.
Description of Changes:
Implement Interest Charts step in the Fixed Deposit Account creation flow (Step 4) that allows users to:
Please Add Screenshots If there are any UI changes.
Changes include:
New Components:
Features:
Validation Rules:
Didn't create a Jira ticket, click here to create new.
Please make sure these boxes are checked before submitting your pull request - thanks!
./gradlew checkorci-prepush.shto make sure you didn't break anythingPlease make sure these boxes are checked before submitting your pull request - thanks!
Run the static analysis check
./gradlew checkorci-prepush.shto make sure you didn't break anythingIf you have multiple commits please combine them into one commit by squashing them.
Summary by CodeRabbit