MIFOSAC-552 Implement Settings Step for New Recurring Deposits Account Flow #2528
Conversation
…agement in recurring account
There was a problem hiding this comment.
Suggestion: For the viewModel, you can take ref from how the vm is written in loanAccount or savingsAccount
Some additional changes:
-
Checkbox options dynamically influence field visibility (penal interest fields should only appear if checked).
Currently the field are visible even when Apply Penal Interest is unchecked.
- The spacing looks uneven.
UI mockup:
- Same here:
UI mockup:
| }, | ||
| ) | ||
| } | ||
| RecurringAccountState.DialogState.Loading -> { |
There was a problem hiding this comment.
The Loading State should be a ScreenState and not a DialogState.
| // Must be removed from here after the implementation of detail screen is finished. | ||
| loadTemplateByProduct() | ||
| } else { | ||
| setErrorState("No internet connection") |
There was a problem hiding this comment.
Hardcoded string can be moved to strings.xml and use getString() here
| // date in dd MM yyyy format. | ||
| submittedOnDate = s.recurringDepositAccountDetail.submittedOnDate, | ||
| ) | ||
| recurringAccountRepository.createRecurringDepositAccount(payload).collect { dataState -> |
There was a problem hiding this comment.
Before making a network call, check if the user is online or not. So include the logic from observe network here too.
| sendEvent(RecurringAccountEvent.NavigateBack) | ||
| } | ||
| RecurringAccountAction.RecurringAccountSettingsAction.OnNextPress -> { | ||
| if (state.currentStep < state.totalSteps) { |
There was a problem hiding this comment.
This part is same for the RecurringAccountAction.NextStep action as well, and can be extracted into a function like so:
private fun moveToNextStep() {
val current = state.currentStep
if (current < state.totalSteps) {
mutableStateFlow.update {
it.copy(
currentStep = current + 1,
)
}
} else {
sendEvent(SavingsAccountEvent.Finish)
}
}
| onFirstBtnClick = { onAction(RecurringAccountAction.RecurringAccountSettingsAction.OnBackPress) }, | ||
| onSecondBtnClick = { onAction(RecurringAccountAction.RecurringAccountSettingsAction.OnNextPress) }, | ||
| isButtonIconVisible = true, | ||
| isSecondButtonEnabled = settingsState.preMatureClosure.penalInterest.isNotBlank() && |
There was a problem hiding this comment.
Could extract this validation logic into the viewModel and use a state to represent this, something like:
val isTermsNextEnabled = isDetailsNextEnabled &&
currencyIndex != -1 &&
interestCalcIndex != -1 &&
interestPostingPeriodIndex != -1 &&
interestCompPeriodIndex != -1
| >(RecurringAccountState()) { | ||
|
|
||
| init { | ||
| observeNetwork() |
There was a problem hiding this comment.
I believe observerNetwork() is not really required. Instead only check network status right before making an api call or network request. So for each request include this logic before making the network request like so:
private fun loadClientTemplate() = viewModelScope.launch {
val online = networkMonitor.isOnline.first()
if (online) {
getClientTemplateUseCase().collect { result ->
sendAction(SavingsAccountAction.Internal.OnReceivingClientTemplate(result))
}
} else {
mutableStateFlow.update {
it.copy(
screenState = SavingsAccountState.ScreenState.NetworkError,
)
}
}
}
private fun loadSavingsProductTemplate() = viewModelScope.launch {
val online = networkMonitor.isOnline.first()
if (online) {
getSavingsProductTemplateUseCase().collect { result ->
sendAction(SavingsAccountAction.Internal.OnReceivingSavingsProductTemplate(result))
}
} else {
mutableStateFlow.update {
it.copy(
screenState = SavingsAccountState.ScreenState.NetworkError,
)
}
}
}
For reference you can check the NewLoanAccount pr's and the NewSavingsAccount pr's that are already merged.
| fun getRecuttingAccountTemplate(): Flow<DataState<RecurringDepositAccountTemplate>> | ||
|
|
||
| fun getRecuttingAccountRepositoryBtProduct( | ||
| fun getRecuttingAccountTemplateByProduct( |
There was a problem hiding this comment.
It should Recurring instead of Recutting
| override fun getRecuttingAccountTemplate(): Flow<DataState<RecurringDepositAccountTemplate>> { | ||
| return dataManagerRecurringAccount.getRecurringDepositAccountTemplate | ||
| .asDataStateFlow() | ||
| } | ||
|
|
||
| override fun getRecuttingAccountRepositoryBtProduct( | ||
| override fun getRecuttingAccountTemplateByProduct( |
…in RecurringAccount components
…ingAccountViewModel and SettingsPage
…in RecurringAccount components
|
Note Reviews pausedUse the following commands to manage reviews:
WalkthroughAdds a recurring-deposit feature: new feature module and strings, DI bindings and data manager singleton, repository method renames, removal of an unused constructor dependency, navigation threaded with clientId, and a network-aware multi-step RecurringAccount UI with ViewModel, dialog handling, and state-driven settings. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ClientScreen as Client Apply Screen
participant Nav as NavController
participant RecurringScreen as Recurring Account Screen
participant VM as RecurringAccountViewModel
participant Repo as RecurringAccountRepository
participant Net as NetworkMonitor
User->>ClientScreen: Select "Apply Recurring Account"
ClientScreen->>Nav: navigateToRecurringAccountRoute(clientId)
Nav->>RecurringScreen: open with clientId
RecurringScreen->>VM: init / observeNetwork()
VM->>Net: subscribe status
alt online
VM->>Repo: loadTemplate() / loadTemplateByProduct()
Repo-->>VM: DataState<Template>
VM-->>RecurringScreen: state with template
else offline
VM-->>RecurringScreen: state with ScreenState=Error (no-internet)
end
User->>RecurringScreen: interact / onAction (settings, next, create)
RecurringScreen->>VM: onAction(...)
VM->>Repo: createRecurringDepositAccount(payload)
Repo-->>VM: DataState<Result>
alt success
VM-->>RecurringScreen: navigate/finish
else error
VM-->>RecurringScreen: state with ScreenState=Error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45–60 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
feature/loan/src/commonMain/composeResources/values/strings.xml (1)
229-229: Fix XML syntax error: extra characters in closing tag.Line 229 has malformed XML with an extra
">at the end of the closing tag, which will cause parsing failures.- <string name="principle_paid_off">Principle Paid Off</string>"> + <string name="principle_paid_off">Principle Paid Off</string>feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/pages/TermsPage.kt (3)
193-204: Fix the empty-check logic to validate the new input value.Line 196 checks
state.principalAmount.isEmpty()(the current state) instead ofit.isEmpty()(the new input value). This causes incorrect behavior: when the field is empty and the user types a value, it will be replaced with "0" instead of using what they typed.Apply this diff to fix the logic:
MifosOutlinedTextField( value = state.principalAmount, onValueChange = { - onAction(NewLoanAccountAction.OnPrincipalAmountChange(if (state.principalAmount.isEmpty()) "0" else it)) + onAction(NewLoanAccountAction.OnPrincipalAmountChange(if (it.isEmpty()) "0" else it)) }, label = stringResource(Res.string.principal),
356-371: Fix the blank-check logic to validate the new input value.Lines 359-363 check
state.nominalInterestRate.isBlank()(the current state) instead ofit.isBlank()(the new input value), causing the same logic error as in the principal amount field.Apply this diff to fix the logic:
MifosOutlinedTextField( value = state.nominalInterestRate, onValueChange = { onAction( NewLoanAccountAction.OnNominalInterestRateChange( - if (state.nominalInterestRate.isBlank()) "0" else it, + if (it.isBlank()) "0" else it, ), ) },
522-539: Fix the clickable toggle to match the checkbox state.Line 525 toggles
!state.isCheckedEqualAmortization, but the checkbox on line 530 is bound tostate.isCheckedInterestPartialPeriod. This copy-paste error causes clicking the row to toggle the wrong checkbox state.Apply this diff:
Row( Modifier.fillMaxWidth() .clickable { - onAction(NewLoanAccountAction.OnInterestPartialPeriodCheckChange(!state.isCheckedEqualAmortization)) + onAction(NewLoanAccountAction.OnInterestPartialPeriodCheckChange(!state.isCheckedInterestPartialPeriod)) }, verticalAlignment = Alignment.CenterVertically, ) {feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountScreen.kt (1)
55-61: Wire Retry to ViewModel actions.Retry button currently does nothing. Pass
onActioninto the dialog and dispatchRecurringAccountAction.Retry.Apply this diff:
- RecurringDepositAccountDialogBox(state = state) + RecurringDepositAccountDialogBox( + state = state, + onAction = { viewModel.trySendAction(it) }, + )-@Composable -fun RecurringDepositAccountDialogBox( - state: RecurringAccountState, -) { +@Composable +fun RecurringDepositAccountDialogBox( + state: RecurringAccountState, + onAction: (RecurringAccountAction) -> Unit, +) { when (state.screenState) { is RecurringAccountState.ScreenState.Error -> { MifosErrorComponent( message = state.screenState.message, isRetryEnabled = true, - onRetry = { - // Retry action can be handled here - }, + onRetry = { onAction(RecurringAccountAction.Retry) }, ) } RecurringAccountState.ScreenState.Loading -> { MifosProgressIndicator() } null -> {} } }Optionally, differentiate network errors via a dedicated
ScreenState.NetworkErrorto driveisNetworkConnected = false.Also applies to: 119-138
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountViewModel.kt (1)
895-921: Duplicate map key overwrites value in repayment schedule.installment_paid is used twice; the second overwrites the first. Use a distinct key for remaining/unpaid installments.
- Res.string.installment_paid to ( + Res.string.total_installments to ( dataState.data.repaymentSchedule.periods ?.count { it.complete == false } ?.toString() ?: "N/A" ),Or introduce a dedicated resource (e.g., installments_remaining).
♻️ Duplicate comments (3)
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/SettingsPage.kt (1)
320-336: Move button-enabled validation to ViewModel (state).Keeps UI dumb and simplifies previews/testing. Mirrors prior feedback.
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountViewModel.kt (2)
482-495: ScreenState good; add aNetworkErrorcase to drive dedicated UI.You already moved Loading into
ScreenState(agree). Consider adding:sealed interface ScreenState { data class Error(val message: String) : ScreenState data object Loading : ScreenState + data object NetworkError : ScreenState }Then set
NetworkErrorin network failures and, in the dialog, passisNetworkConnected = falsetoMifosErrorComponent.
40-44: Initialization order + test-only side effects inobserveNetwork().
init { observeNetwork() }runs beforeclientId(declared after). While the coroutine likely starts later, this is order-fragile. MoveclientIdaboveinitor pass it into the call.observeNetwork()contains test-onlyloadTemplateByProduct(); this causes unintended network calls (including from other code paths that invokeobserveNetwork()).Apply this diff:
-class RecurringAccountViewModel( +class RecurringAccountViewModel( savedStateHandle: SavedStateHandle, private val networkMonitor: NetworkMonitor, private val recurringAccountRepository: RecurringAccountRepository, ) : BaseViewModel< @@ - init { - observeNetwork() - } - private val clientId = savedStateHandle.toRoute<RecurringAccountRoute>().clientId + private val clientId = savedStateHandle.toRoute<RecurringAccountRoute>().clientId + init { + observeNetwork() + } @@ - if (isConnected) { -// loadTemplate() - // TODO - // Used only for testing purpose - // Must be removed from here after the implementation of detail screen is finished. - loadTemplateByProduct() - } else { + if (!isConnected) { setErrorState( getString(Res.string.no_internet_connection), ) }Prefer checking connectivity right before each network call (see next comment).
Also applies to: 60-78
🧹 Nitpick comments (25)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/pages/PreviewPage.kt (2)
117-121: Consider using safer conversion to prevent potential crashes.The
toDouble()conversion can throw aNumberFormatExceptionifprincipalAmountcontains invalid data. While previous validation steps likely prevent this, usingtoDoubleOrNull()with a fallback would make the code more defensive.Apply this diff for safer conversion:
principal = CurrencyFormatter.format( - balance = state.principalAmount.toDouble(), + balance = state.principalAmount.toDoubleOrNull() ?: 0.0, currencyCode = state.loanTemplate?.currency?.code, maximumFractionDigits = 2, ),
155-156: Address TODO comments for incomplete fields.The preview page has three fields with incomplete implementation:
installmentAmount(line 156)enableInstallmentLevelDelinquency(line 169)daysInMonth(line 178)These are currently set to empty strings, which means users won't see values for these fields in the preview.
Would you like me to help implement these fields or open an issue to track completing these TODOs?
Also applies to: 168-169, 177-178
core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosGeneralCard.kt (3)
32-53: Consider parameterizing top corner radius for consistency.The top corners are hardcoded to 12.dp while bottom corners use the
borderCornerparameter. If this is an intentional design constraint, consider documenting it. Otherwise, making the top corners configurable would improve component reusability.Example refactor:
@Composable fun MifosGeneralCardComponentOutline( modifier: Modifier = Modifier, + topCorner: Dp = 12.dp, borderCorner: Dp = DesignToken.sizes.iconMiny, content: @Composable () -> Unit, ) { Box( modifier = modifier .border( width = 1.dp, shape = RoundedCornerShape( - topStart = 12.dp, - topEnd = 12.dp, + topStart = topCorner, + topEnd = topCorner, bottomStart = borderCorner, bottomEnd = borderCorner, ), color = MaterialTheme.colorScheme.secondaryContainer, ), ) { content() } }
76-97: Consider more flexible separator handling.The separator is currently concatenated with the key text (line 88), which means it inherits the same text style. If you anticipate needing different styling for the separator in the future, consider rendering it as a separate composable.
Alternative approach:
MifosGeneralRowItem( - keyContent = { Text(text = map.key + separator, style = MifosTypography.labelMediumEmphasized) }, + keyContent = { + Row { + Text(text = map.key, style = MifosTypography.labelMediumEmphasized) + Text(text = separator, style = MifosTypography.labelMediumEmphasized) + } + }, valueContent = { Text(text = map.value, style = MifosTypography.labelMediumEmphasized) }, )This would allow passing different text styles as parameters if needed.
32-97: Add KDoc documentation for public API functions.All public composable functions lack documentation. Consider adding KDoc comments explaining the purpose, parameters, and usage of each component to improve maintainability and developer experience.
Example for the main component:
/** * A card component that displays a map of key-value pairs in a bordered container. * * @param modifier The modifier to be applied to the card * @param contentMap A map of key-value pairs to display. Order is preserved. * @param separator The separator to display between keys and values (default: " : ") */ @Composable fun MifosGeneralCard( modifier: Modifier = Modifier, contentMap: Map<String, String>, separator: String = " : ", ) { // ... }core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosStepper.kt (1)
78-134: Consider refactoring to idiomatic LazyRow pattern.The current implementation using
forEachIndexed+repeat(2)+itemis functional but unconventional. Therepeat(2) { it ->pattern with implicititvariable makes it unclear that we're rendering step content (it==0) vs separator (it==1).Consider refactoring to the more idiomatic Compose pattern:
-LazyRow( - state = listState, - contentPadding = PaddingValues(horizontal = DesignToken.padding.small), -) { - steps.forEachIndexed { index, step -> - repeat(2) { it -> - item { - if (it == 0) { +LazyRow( + state = listState, + contentPadding = PaddingValues(horizontal = DesignToken.padding.small), +) { + itemsIndexed(steps) { index, step -> - Column( - horizontalAlignment = Alignment.CenterHorizontally, - modifier = Modifier.width(DesignToken.sizes.avatarMediumExtra), - ) { - Box( - modifier = Modifier - .size(DesignToken.sizes.iconLarge) - .clip(CircleShape) - .background( - when { - index == currentIndex -> AppColors.customWhite - else -> AppColors.stepperColor - }, - ) - .clickable(enabled = index < currentIndex) { - if (index < currentIndex) onStepChange(index) - }, - contentAlignment = Alignment.Center, - ) { - Text( - text = (index + 1).toString(), - color = MaterialTheme.colorScheme.primary, - ) - } - Spacer(modifier = Modifier.height(DesignToken.padding.small)) - BasicText( - text = step.name, - autoSize = TextAutoSize.StepBased( - minFontSize = 2.sp, - maxFontSize = 11.sp, - ), - style = MifosTypography.labelSmall.copy( - color = AppColors.customWhite, - ), - ) - } - } else if (index != steps.lastIndex) { + Column( + horizontalAlignment = Alignment.CenterHorizontally, + modifier = Modifier.width(DesignToken.sizes.avatarMediumExtra), + ) { + Box( + modifier = Modifier + .size(DesignToken.sizes.iconLarge) + .clip(CircleShape) + .background( + when { + index == currentIndex -> AppColors.customWhite + else -> AppColors.stepperColor + }, + ) + .clickable(enabled = index < currentIndex) { + if (index < currentIndex) onStepChange(index) + }, + contentAlignment = Alignment.Center, + ) { + Text( + text = (index + 1).toString(), + color = MaterialTheme.colorScheme.primary, + ) + } + Spacer(modifier = Modifier.height(DesignToken.padding.small)) + BasicText( + text = step.name, + autoSize = TextAutoSize.StepBased( + minFontSize = 2.sp, + maxFontSize = 11.sp, + ), + style = MifosTypography.labelSmall.copy( + color = AppColors.customWhite, + ), + ) + } + + if (index != steps.lastIndex) { - Box( - modifier = Modifier - .padding(vertical = DesignToken.padding.large) - .width(DesignToken.padding.small) - .height(1.dp) - .background(AppColors.stepperColor), - ) - } - } - } + Box( + modifier = Modifier + .padding(vertical = DesignToken.padding.large) + .width(DesignToken.padding.small) + .height(1.dp) + .background(AppColors.stepperColor), + ) + } } -} +}This refactoring:
- Uses
itemsIndexed()which is the standard Compose DSL for LazyRow- Eliminates the unclear
repeat(2)pattern- Improves code readability by making the step + separator structure explicit
- Maintains the same visual result and functionality
core/network/src/commonMain/kotlin/com/mifos/core/network/services/LoanService.kt (1)
79-79: Consider standardizing the response handling approach across all service methods.This service now has inconsistent return types: some methods return typed domain models (e.g.,
LoanWithAssociationsEntityat line 42,LoanProductsat line 76), while others returnHttpResponse(line 79) orGenericResponse(line 53). This inconsistency increases cognitive load and makes the API harder to use correctly.Consider one of these approaches:
- Standardize on typed responses throughout the service for type safety
- Or document the reasoning for when each approach is used
core/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerSavings.kt (1)
291-310: Consider architectural consistency with Loan flow.The Loan account creation flow returns
HttpResponsethrough all layers (service → DataManager → repository → use case), but this Savings flow parses theHttpResponsetoSavingsat the DataManager level. While the error handling here is robust, this inconsistency makes the codebase harder to maintain.Consider either:
- Parsing
HttpResponsetoSavingsat the repository or use case layer (like Loan does), or- Standardizing both flows to parse at the same layer
</review_comment_end -->
core/model/src/commonMain/kotlin/com/mifos/core/model/objects/payloads/RecurringDepositAccountPayload.kt (3)
35-36: Currency amount should not be Int.mandatoryRecommendedDepositAmount is modeled as Int?. Currency often needs decimals and locale-safe parsing. Prefer a decimal type or string with a serializer.
Example:
- val mandatoryRecommendedDepositAmount: Int? = null, + // Use a decimal-friendly representation; parse/format at boundaries + val mandatoryRecommendedDepositAmount: String? = null,If you have a Decimal/BigDecimal abstraction in common code, use that with a kotlinx-serialization serializer.
23-24: Use type-safe dates instead of raw String.expectedFirstDepositOnDate and submittedOnDate as String rely on dateFormat/locale and are error-prone. Prefer kotlinx-datetime LocalDate with a serializer.
Sketch:
@Serializable(with = LocalDateAsStringSerializer::class) val expectedFirstDepositOnDate: LocalDate? = null,Also applies to: 40-41
16-16: Remove commented code or introduce a typed model for charges.Commented Any-based field is a smell. Either define a proper ChargesPayload type or drop the comment to keep the model clean.
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/SettingsPage.kt (2)
81-84: Use MifosCheckBox for consistency and better semantics.The first row uses raw Checkbox + Text while the rest use MifosCheckBox. Unify for consistent behavior and larger tap target.
- Row(verticalAlignment = Alignment.CenterVertically) { - Checkbox(settingsState.isMandatory, onCheckedChange = { onAction(RecurringAccountAction.RecurringAccountSettingsAction.ToggleMandatoryDeposit) }) - Text(stringResource(Res.string.is_mandatory_deposit)) - } + MifosCheckBox( + text = stringResource(Res.string.is_mandatory_deposit), + checked = settingsState.isMandatory, + onCheckChanged = { onAction(RecurringAccountAction.RecurringAccountSettingsAction.ToggleMandatoryDeposit) }, + )
118-121: Inconsistent nullability handling on template.Use a single approach (non-null contract or consistent null-safe access) to reduce branching and mistakes.
Also applies to: 228-231
feature/recurringDeposit/src/commonMain/composeResources/values/string.xml (1)
18-38: Minor copy/style nits for consistency.
- Consider sentence case across labels (e.g., “Deposit frequency same as group/center meeting”).
- “Apply Penal Interest (less)” reads odd; confirm intended phrasing (“Apply penal interest”).
If these are server-specified labels, ignore.
core/network/src/commonMain/kotlin/com/mifos/core/network/di/DataMangerModule.kt (1)
33-33: UsesingleOf(and keep the import) for the new binding.This simplifies DI registration and avoids the extra lambda.
Apply this diff:
- single{ DataManagerRecurringAccount(get())} + singleOf(::DataManagerRecurringAccount)Also applies to: 56-57
core/data/src/commonMain/kotlin/com/mifos/core/data/repository/RecurringAccountRepository.kt (1)
27-30: Makepayloadnon-null or enforce null handling contract.Creation without a payload should not proceed. Either:
- Make it non-null and validate upstream, or
- Explicitly document and short-circuit on
null.feature/savings/src/commonMain/composeResources/values/feature_savings_strings.xml (2)
16-16: Avoid relying on “*” to indicate required; add accessible required semantics.Screen readers won’t announce “*”. Prefer:
- Keep labels without “*”.
- Render a separate required indicator in UI with contentDescription "Required".
- Or append “(Required)” via localized string for a11y.
Confirm design-system guidance for required-field indicators and whether a dedicated accessibility label exists.
Also applies to: 31-31, 70-70
142-152: Use plurals resource for count string instead of simple string.
feature_savings_charges_active_countcurrently uses%1$dplaceholder as a simple<string>element but should be a<plurals>resource to handle 0/1/other cases correctly for proper localization.- Duplicate key verification: No duplicates found among the newly added keys.
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientDetailsProfile/ClientProfileDetailsScreen.kt (1)
102-105: Avoid sending sentinel values (-1, empty status) in navigation.Guard the call until both id and status are present to prevent invalid routes.
- navigateToApplyNewApplication( - state.client?.id ?: -1, - state.client?.status?.value ?: "", - ) + val id = state.client?.id + val status = state.client?.status?.value + if (id != null && !status.isNullOrBlank()) { + navigateToApplyNewApplication(id, status) + }Confirm downstream expects non-empty status values and that “unknown” is not a valid status token.
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountScreen.kt (1)
285-289: Avoid silent coercion of collateral quantity; validate and clamp.Converting non-numeric input to 0 can enable inconsistent totals. Validate numeric and non-negative, and disable confirm on invalid input instead of coercing.
- onValueChange = { - onAction( - NewLoanAccountAction.OnCollateralQuantityChanged( - it.toIntOrNull() ?: 0, - ), - ) - }, + onValueChange = { + val q = it.toIntOrNull() + // Treat null/negative as invalid; keep 0 only when explicitly entered + onAction( + NewLoanAccountAction.OnCollateralQuantityChanged( + q?.coerceAtLeast(0) ?: 0 + ), + ) + // Consider exposing an error state to disable confirm when q == null + },Confirm business rule: is 0 a valid collateral quantity? If not, clamp to at least 1 and gate confirm.
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientApplyNewApplications/ClientApplyNewApplicationsScreen.kt (1)
51-51: Avoid UI depending on Room-layer constants.Comparing state.status to ClientStatusEntity.STATUS_ACTIVE leaks persistence-layer details into UI. Prefer a UI/domain enum or a boolean isClientActive on state.
- import com.mifos.room.entities.client.ClientStatusEntity ... - if (state.status == ClientStatusEntity.STATUS_ACTIVE) { + if (state.isClientActive) {Consider mapping status -> isClientActive in ViewModel.
Also applies to: 110-110
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientApplyNewApplications/ClientApplyNewApplicationRoute.kt (1)
19-21: Use a type-safe status instead of raw String.Define an enum/sealed class (e.g., @serializable enum class ClientStatus) to prevent magic strings and typos in navigation args.
- data class ClientApplyNewApplicationRoute( - val clientId: Int, - val status: String, - ) + @Serializable + data class ClientApplyNewApplicationRoute( + val clientId: Int, + val status: ClientStatus, + )feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountv2/SavingsAccountViewModel.kt (1)
163-206: Optional: clear status dialog state on dismiss.Consider adding an action to reset screenState from ShowStatusDialog back to Success to avoid stale dialog on rotation/restore.
feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountv2/pages/PreviewPage.kt (1)
62-70: Minor: remember derived maps to reduce recomposition churn.Compute previewDetailsMap and termsDetailsMap with remember keyed to their inputs.
- val previewDetailsMap = mapOf( + val previewDetailsMap = remember( + state.savingsProductSelected, state.fieldOfficerIndex, state.submissionDate, state.externalId + ) { mapOf( /* existing entries */ - ) + ) } ... - val termsDetailsMap = mapOf( + val termsDetailsMap = remember( + state.currencyIndex, state.decimalPlaces, state.interestCompPeriodIndex, state.interestPostingPeriodIndex, + state.interestCalcIndex, state.daysInYearIndex, state.isCheckedApplyWithdrawalFee, state.isCheckedOverdraftAllowed, + state.freqTypeIndex, state.frequency, state.monthlyMinimumBalance, state.minimumOpeningBalance + ) { mapOf( /* existing entries */ - ) + ) }Add:
import androidx.compose.runtime.rememberAlso applies to: 71-130
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountViewModel.kt (1)
940-949: Name nit: responseErrorMsg used for success text.Consider renaming to responseMessage or adding a separate successMessage to avoid confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (53)
cmp-navigation/build.gradle.kts(1 hunks)cmp-navigation/src/commonMain/kotlin/cmp/navigation/di/KoinModules.kt(2 hunks)core/data/src/commonMain/kotlin/com/mifos/core/data/repository/DataTableListRepository.kt(1 hunks)core/data/src/commonMain/kotlin/com/mifos/core/data/repository/LoanAccountRepository.kt(2 hunks)core/data/src/commonMain/kotlin/com/mifos/core/data/repository/RecurringAccountRepository.kt(1 hunks)core/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/DataTableListRepositoryImp.kt(2 hunks)core/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/LoanAccountRepositoryImp.kt(2 hunks)core/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/RecurringAccountRepositoryImp.kt(1 hunks)core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/component/MifosSweetError.kt(3 hunks)core/domain/src/commonMain/kotlin/com/mifos/core/domain/useCases/CreateLoanAccountUseCase.kt(1 hunks)core/domain/src/commonMain/kotlin/com/mifos/core/domain/useCases/CreateSavingsAccountUseCase.kt(1 hunks)core/model/objects/payloads/RecurringDepositAccountPayload.kt(1 hunks)core/model/src/commonMain/kotlin/com/mifos/core/model/objects/payloads/RecurringDepositAccountPayload.kt(1 hunks)core/model/src/commonMain/kotlin/com/mifos/core/model/objects/payloads/SavingsPayload.kt(1 hunks)core/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerLoan.kt(2 hunks)core/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerRecurringAccount.kt(0 hunks)core/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerSavings.kt(3 hunks)core/network/src/commonMain/kotlin/com/mifos/core/network/di/DataMangerModule.kt(2 hunks)core/network/src/commonMain/kotlin/com/mifos/core/network/model/LoansPayload.kt(1 hunks)core/network/src/commonMain/kotlin/com/mifos/core/network/services/LoanService.kt(2 hunks)core/network/src/commonMain/kotlin/com/mifos/core/network/services/SavingsAccountService.kt(2 hunks)core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosGeneralCard.kt(1 hunks)core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosStepper.kt(4 hunks)core/ui/src/commonMain/kotlin/com/mifos/core/ui/util/TextFieldsValidator.kt(1 hunks)feature/client/src/commonMain/composeResources/values/strings.xml(1 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientApplyNewApplications/ClientApplyNewApplicationRoute.kt(2 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientApplyNewApplications/ClientApplyNewApplicationsScreen.kt(6 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientApplyNewApplications/ClientApplyNewApplicationsViewModel.kt(1 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientDetailsProfile/ClientProfileDetailsNavigation.kt(1 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientDetailsProfile/ClientProfileDetailsScreen.kt(2 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientGeneral/ClientProfileGeneralNavigation.kt(2 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/navigation/ClientNavigation.kt(2 hunks)feature/loan/src/commonMain/composeResources/values/strings.xml(5 hunks)feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountScreen.kt(11 hunks)feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountViewModel.kt(17 hunks)feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/pages/DetailsPage.kt(3 hunks)feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/pages/PreviewPage.kt(2 hunks)feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/pages/SchedulePage.kt(0 hunks)feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/pages/TermsPage.kt(15 hunks)feature/recurringDeposit/src/commonMain/composeResources/values/string.xml(1 hunks)feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/di/RecurringDepositModule.kt(1 hunks)feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountRoute.kt(2 hunks)feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountScreen.kt(6 hunks)feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountViewModel.kt(3 hunks)feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/SettingsPage.kt(1 hunks)feature/savings/src/commonMain/composeResources/values/feature_savings_strings.xml(4 hunks)feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/navigation/SavingsNavigation.kt(2 hunks)feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccount/SavingAccountViewModel.kt(2 hunks)feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountv2/SavingsAccountRoute.kt(1 hunks)feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountv2/SavingsAccountScreen.kt(6 hunks)feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountv2/SavingsAccountViewModel.kt(7 hunks)feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountv2/pages/PreviewPage.kt(1 hunks)version.txt(1 hunks)
💤 Files with no reviewable changes (2)
- core/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerRecurringAccount.kt
- feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/pages/SchedulePage.kt
🧰 Additional context used
🧬 Code graph analysis (8)
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/SettingsPage.kt (3)
core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosCheckBox.kt (1)
MifosCheckBox(24-46)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)
feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountv2/SavingsAccountScreen.kt (3)
mifosng-android/src/main/java/com/mifos/mifosxdroid/AndroidClientViewModel.kt (1)
onFinish(124-126)feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountv2/pages/PreviewPage.kt (1)
PreviewPage(56-199)core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosStatusDialog.kt (1)
MifosStatusDialog(36-103)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/pages/TermsPage.kt (2)
core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/component/MifosTextFieldDropdown.kt (1)
MifosTextFieldDropdown(39-112)core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/component/MifosOutlinedTextField.kt (4)
MifosOutlinedTextField(66-124)MifosOutlinedTextField(126-243)MifosOutlinedTextField(245-308)MifosOutlinedTextField(404-469)
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountScreen.kt (2)
core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosErrorComponent.kt (1)
MifosErrorComponent(41-60)core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosProgressIndicator.kt (1)
MifosProgressIndicator(41-68)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountScreen.kt (4)
core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosBreadCrumb.kt (1)
MifosBreadcrumbNavBar(36-107)core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosProgressIndicator.kt (2)
MifosProgressIndicator(41-68)MifosProgressIndicatorOverlay(70-102)core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosStepper.kt (1)
MifosStepper(47-139)core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/component/MifosSweetError.kt (1)
MifosSweetError(40-83)
feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountv2/pages/PreviewPage.kt (3)
core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosGeneralCard.kt (1)
MifosGeneralCard(76-97)core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosRowWithTextAndButton.kt (1)
MifosRowWithTextAndButton(35-95)core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosTwoButtonRow.kt (1)
MifosTwoButtonRow(31-91)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/pages/DetailsPage.kt (1)
core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosTwoButtonRow.kt (1)
MifosTwoButtonRow(31-91)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientApplyNewApplications/ClientApplyNewApplicationsScreen.kt (1)
core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/component/MifosSweetError.kt (1)
MifosSweetError(40-83)
| operator fun invoke(loansPayload: LoansPayload): Flow<DataState<HttpResponse>> = | ||
| loanAccountRepository.createLoansAccount(loansPayload) |
There was a problem hiding this comment.
Architectural concern: HttpResponse leaking into the domain layer.
The domain layer (use case) now depends on HttpResponse from the Ktor HTTP client library. This violates clean architecture principles, as the domain layer should be independent of infrastructure concerns like HTTP.
Consider one of these approaches:
- Parse
HttpResponsein the repository layer and return domain models to use cases - Create a domain-specific response wrapper that doesn't expose HTTP details
- Document the architectural decision if this is intentional
This allows the domain layer to remain independent of HTTP client implementation details.
| operator fun invoke(savingsPayload: SavingsPayload?): Flow<DataState<Savings>> = | ||
| repository.createSavingsAccount(savingsPayload) |
There was a problem hiding this comment.
🧩 Analysis chain
Verify null handling for savingsPayload parameter.
The return type changed from Flow<DataState<Savings?>> to Flow<DataState<Savings>> (non-nullable), which is appropriate since DataManagerSavings throws IllegalStateException on errors rather than returning null. However, the parameter remains nullable (SavingsPayload?).
Ensure that the repository properly handles a null savingsPayload parameter, either by throwing an appropriate exception or by having a non-null contract.
</review_comment_end -->
🏁 Script executed:
#!/bin/bash
# Description: Check how null savingsPayload is handled in the repository and service layers
# Find the repository implementation
ast-grep --pattern 'fun createSavingsAccount(savingsPayload: $_) {
$$$
}'
# Check service layer handling
rg -n --type=kt "fun createSavingsAccount" -A 5Length of output: 26
🏁 Script executed:
#!/bin/bash
# Description: Search for repository implementation and null handling
# Search for createSavingsAccount in Kotlin files
rg -n "createSavingsAccount" -g "*.kt" -A 5 | head -50
# Search for null checks or IllegalStateException related to savingsPayload
rg -n "savingsPayload" -g "*.kt" -B 2 -A 2 | head -50Length of output: 13113
🏁 Script executed:
#!/bin/bash
# Description: Find repository implementation and null handling
# Find SavingsRepository or repository implementation
find . -name "*Repository*.kt" -type f | grep -i saving | head -20
# Search for DataManagerSavings or similar
rg -n "DataManagerSavings\|SavingsRepository" -g "*.kt" -B 2 -A 5 | head -100
# Search for null checks with savingsPayload
rg -n "requireNotNull|checkNotNull|require|check" -g "*.kt" | grep -i "saving" | head -20Length of output: 5461
🏁 Script executed:
#!/bin/bash
# Description: Examine the repository implementation of createSavingsAccount
# Read the SavingsAccountRepositoryImp.kt file to find createSavingsAccount method
cat ./core/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/SavingsAccountRepositoryImp.kt | grep -A 20 "createSavingsAccount"
# Also check the repository interface
cat ./core/data/src/commonMain/kotlin/com/mifos/core/data/repository/SavingsAccountRepository.kt | grep -A 10 "createSavingsAccount"Length of output: 298
🏁 Script executed:
#!/bin/bash
# Description: Find DataManagerSavings implementation
# Search for DataManagerSavings class definition
rg -n "class DataManagerSavings" -g "*.kt" -A 30 | head -50
# Search for createSavingsAccount in DataManagerSavings
rg -n "fun createSavingsAccount" -g "*.kt" -B 5 -A 20 | grep -A 20 "DataManagerSavings\|savingsPayload\|throw"Length of output: 15913
Change parameter savingsPayload from nullable to non-nullable.
The return type was correctly changed from Flow<DataState<Savings?>> to Flow<DataState<Savings>>, making the return non-nullable. However, the parameter remains nullable (SavingsPayload?), which contradicts this contract. All call sites pass non-null SavingsPayload instances, and the repository chain has no explicit null validation—any null would silently propagate to the HTTP service and eventually fail as IllegalStateException. For consistency and type safety, change the parameter to SavingsPayload (non-nullable) throughout: CreateSavingsAccountUseCase, SavingsAccountRepository, SavingsAccountRepositoryImp, and DataManagerSavings.
🤖 Prompt for AI Agents
In
core/domain/src/commonMain/kotlin/com/mifos/core/domain/useCases/CreateSavingsAccountUseCase.kt
around lines 26-27 the parameter savingsPayload is declared nullable
(SavingsPayload?) but the function returns non-nullable
Flow<DataState<Savings>>, which is inconsistent; change the parameter type to
non-nullable (SavingsPayload) and propagate this change through the repository
interface and implementations (SavingsAccountRepository,
SavingsAccountRepositoryImp, and DataManagerSavings) so all method signatures
accept SavingsPayload (non-nullable) and remove any nullable handling that would
otherwise allow null to reach the HTTP layer.
|
|
||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Remove stray empty file to avoid confusion/duplication.
This empty file shadows the real payload in core/model/src/... and can confuse IDE search and future contributors. Please delete it.
Run to confirm duplicates:
🏁 Script executed:
#!/bin/bash
fd -a 'RecurringDepositAccountPayload.kt' -H
sed -n '1,60p' core/model/objects/payloads/RecurringDepositAccountPayload.kt || trueLength of output: 206
Delete the stray empty file to prevent IDE search conflicts.
The verification confirms a duplicate exists: the file at core/model/objects/payloads/RecurringDepositAccountPayload.kt is empty, while the actual payload is properly located at core/model/src/commonMain/kotlin/com/mifos/core/model/objects/payloads/RecurringDepositAccountPayload.kt. Delete the stray empty file to avoid IDE search confusion and duplication issues.
🤖 Prompt for AI Agents
In core/model/objects/payloads/RecurringDepositAccountPayload.kt lines 1-2, this
file is an empty stray duplicate that conflicts with the real implementation at
core/model/src/commonMain/kotlin/com/mifos/core/model/objects/payloads/RecurringDepositAccountPayload.kt;
delete the empty file from the repository (remove the file and commit the
deletion) so the IDE and search tooling only see the canonical payload
implementation.
| fun createLoansAccount(loansPayload: LoansPayload?): Flow<HttpResponse> { | ||
| return mBaseApiManager.loanService.createLoansAccount(loansPayload) | ||
| } |
There was a problem hiding this comment.
Breaking change propagated correctly from service layer.
The method signature correctly reflects the underlying loanService.createLoansAccount change to return HttpResponse. The delegation is straightforward and correct.
However, ensure all callers of this data manager method are updated to handle HttpResponse deserialization and error handling appropriately.
🤖 Prompt for AI Agents
In
core/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerLoan.kt
around lines 93 to 95, the data manager now returns Flow<HttpResponse> (matching
the service), so update all callers to consume and handle HttpResponse
explicitly: collect the Flow and check response.status(), read and deserialize
the response body (e.g., bodyAsText() and parse with your serializer or
response.receive<T>()), map the deserialized payload into the domain model,
handle non-2xx statuses by throwing or converting to a domain error type, and
wrap networking exceptions appropriately; update any caller signatures that
assumed a deserialized model to return Flow<HttpResponse> or adapt with a
mapping operator (map/flatMap) so downstream code receives a parsed result or a
well-defined error.
...n/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountViewModel.kt
Show resolved
Hide resolved
...n/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountViewModel.kt
Show resolved
Hide resolved
| data class RecurringAccountDetailsState( | ||
| // TODO | ||
| // productId is set 6 only for testing. | ||
| // It should be set -1 when the implementation of detail screen is finished. | ||
| val productId: Int = 6, | ||
| val externalId: String = "", | ||
| val submittedOnDate: String = "", | ||
| val fieldOfficer: FieldOfficerOption? = null, | ||
| ) |
There was a problem hiding this comment.
Remove test-only default for productId.
Defaulting to 6 risks accidental real calls. Use -1 and gate network calls until a valid product is selected.
Apply this diff:
- // productId is set 6 only for testing.
- // It should be set -1 when the implementation of detail screen is finished.
- val productId: Int = 6,
+ val productId: Int = -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 RecurringAccountDetailsState( | |
| // TODO | |
| // productId is set 6 only for testing. | |
| // It should be set -1 when the implementation of detail screen is finished. | |
| val productId: Int = 6, | |
| val externalId: String = "", | |
| val submittedOnDate: String = "", | |
| val fieldOfficer: FieldOfficerOption? = null, | |
| ) | |
| data class RecurringAccountDetailsState( | |
| // TODO | |
| val productId: Int = -1, | |
| val externalId: String = "", | |
| val submittedOnDate: String = "", | |
| val fieldOfficer: FieldOfficerOption? = null, | |
| ) |
🤖 Prompt for AI Agents
In
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountViewModel.kt
around lines 497 to 505, the RecurringAccountDetailsState currently defaults
productId to 6 for testing; change the default to -1 and update code paths that
trigger network calls (e.g., view model initialization or submit handlers) to
gate those calls until productId > 0 (or a valid product is selected) to prevent
accidental real network requests. Ensure any UI/form validation treats -1 as
"none selected" and avoids invoking APIs until a proper productId is provided.
| is DataState.Success -> { | ||
| mutableStateFlow.update { | ||
| it.copy( | ||
| isOverLayLoadingActive = false, | ||
| screenState = SavingsAccountState.ScreenState.ShowStatusDialog( | ||
| ResultStatus.SUCCESS, | ||
| getString(Res.string.feature_savings_new_savings_account_submitted_success), | ||
| ), | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
Suspend call inside StateFlow.update causes compile-time error.
getString(...) is suspend and cannot be called inside update { }. Compute it before update.
- is DataState.Success -> {
- mutableStateFlow.update {
- it.copy(
- isOverLayLoadingActive = false,
- screenState = SavingsAccountState.ScreenState.ShowStatusDialog(
- ResultStatus.SUCCESS,
- getString(Res.string.feature_savings_new_savings_account_submitted_success),
- ),
- )
- }
- }
+ is DataState.Success -> {
+ val successMsg = getString(Res.string.feature_savings_new_savings_account_submitted_success)
+ mutableStateFlow.update {
+ it.copy(
+ isOverLayLoadingActive = false,
+ screenState = SavingsAccountState.ScreenState.ShowStatusDialog(
+ ResultStatus.SUCCESS,
+ successMsg,
+ ),
+ )
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| is DataState.Success -> { | |
| mutableStateFlow.update { | |
| it.copy( | |
| isOverLayLoadingActive = false, | |
| screenState = SavingsAccountState.ScreenState.ShowStatusDialog( | |
| ResultStatus.SUCCESS, | |
| getString(Res.string.feature_savings_new_savings_account_submitted_success), | |
| ), | |
| ) | |
| } | |
| } | |
| is DataState.Success -> { | |
| val successMsg = getString(Res.string.feature_savings_new_savings_account_submitted_success) | |
| mutableStateFlow.update { | |
| it.copy( | |
| isOverLayLoadingActive = false, | |
| screenState = SavingsAccountState.ScreenState.ShowStatusDialog( | |
| ResultStatus.SUCCESS, | |
| successMsg, | |
| ), | |
| ) | |
| } | |
| } |
🤖 Prompt for AI Agents
In
feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountv2/SavingsAccountViewModel.kt
around lines 175 to 185: getString(...) is a suspend function and cannot be
invoked inside StateFlow.update { ... }; move the suspend call out of update by
calling val message =
getString(Res.string.feature_savings_new_savings_account_submitted_success) (or
an appropriately named val) before the update block, then call
mutableStateFlow.update { it.copy(isOverLayLoadingActive = false, screenState =
SavingsAccountState.ScreenState.ShowStatusDialog(ResultStatus.SUCCESS, message))
} so the suspend call executes before entering the non-suspending update lambda.
| mutableStateFlow.update { | ||
| it.copy( | ||
| screenState = SavingsAccountState.ScreenState.ShowStatusDialog( | ||
| ResultStatus.FAILURE, | ||
| msg = result.exception.message ?: getString(Res.string.feature_savings_new_savings_account_submitted_failed), | ||
| ), | ||
| isOverLayLoadingActive = false, | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
Suspend call inside StateFlow.update (error path) as well.
Same issue when building the fallback error message.
- is DataState.Error -> {
- mutableStateFlow.update {
- it.copy(
- screenState = SavingsAccountState.ScreenState.ShowStatusDialog(
- ResultStatus.FAILURE,
- msg = result.exception.message ?: getString(Res.string.feature_savings_new_savings_account_submitted_failed),
- ),
- isOverLayLoadingActive = false,
- )
- }
- }
+ is DataState.Error -> {
+ val defaultMsg = getString(Res.string.feature_savings_new_savings_account_submitted_failed)
+ val message = result.exception.message ?: defaultMsg
+ mutableStateFlow.update {
+ it.copy(
+ screenState = SavingsAccountState.ScreenState.ShowStatusDialog(
+ ResultStatus.FAILURE,
+ msg = message,
+ ),
+ isOverLayLoadingActive = false,
+ )
+ }
+ }🤖 Prompt for AI Agents
In
feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountv2/SavingsAccountViewModel.kt
around lines 187 to 196, you're calling a suspend function (getString) inside
StateFlow.update in the error path; move any suspend work out of the update
block: first compute the fallback message by calling getString (or resolving the
suspend value) before invoking mutableStateFlow.update, then pass the resolved
String into the update { it.copy(...) } call so the update block remains
strictly non-suspending.
…ingAccount settings
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountViewModel.kt (2)
537-545: Remove test-only productId default to prevent accidental API calls.The
productIdis hardcoded to6for testing (line 541). This is a major risk - if the detail screen is not implemented or bypassed, real API calls will be made with this test value, potentially corrupting data.Apply this diff:
data class RecurringAccountDetailsState( - // TODO - // productId is set 6 only for testing. - // It should be set -1 when the implementation of detail screen is finished. - val productId: Int = 6, + val productId: Int = -1, val externalId: String = "", val submittedOnDate: String = "", val fieldOfficer: FieldOfficerOption? = null, )Also ensure that API calls in
loadTemplateByProduct()andcreateRecurringDepositAccount()validate thatproductId > 0before proceeding.
208-259: Critical: Race condition and unsafe parsing will cause crashes.Two critical issues remain from previous review:
Race condition: Line 239 calls
observeNetwork()but immediately checksstate.isOnlineat line 241. The state update is asynchronous, soisOnlinemay not reflect the current network status.Unsafe parsing: Lines 229 and 231 use
.toInt()which throwsNumberFormatExceptionon invalid input:
lockinPeriodFrequencyfrom a text field (line 229)mandatoryRecommendedDepositAmountfrom a formatted currency string with symbols (line 231)Apply this diff to fix both issues:
private fun createRecurringDepositAccount() { viewModelScope.launch { + // One-shot connectivity check + val online = networkMonitor.isOnline.first() + if (!online) { + setErrorState(getString(Res.string.no_internet_connection)) + return@launch + } + val s = state val settings = s.recurringDepositAccountSettings + + // Safe numeric parsing with validation + val lockInFreq = settings.lockInPeriod.frequency.toIntOrNull() + val depositAmount = settings.recurringDepositDetails.depositAmount + .replace("[^0-9]".toRegex(), "") + .toIntOrNull() + + if (lockInFreq == null || depositAmount == null) { + setErrorState("Invalid numeric input") + return@launch + } + val payload = RecurringDepositAccountPayload( adjustAdvanceTowardsFuturePayments = settings.adjustAdvancePayments, allowWithdrawal = settings.allowWithdrawals, clientId = clientId, dateFormat = "dd MMMM yyyy", depositPeriod = settings.depositPeriod.period.toIntOrNull(), depositPeriodFrequencyId = settings.depositPeriod.periodType, expectedFirstDepositOnDate = null, externalId = s.recurringDepositAccountDetail.externalId, fieldOfficerId = s.recurringDepositAccountDetail.fieldOfficer?.id, interestCalculationDaysInYearType = null, interestCalculationType = null, interestCompoundingPeriodType = null, interestPostingPeriodType = null, isCalendarInherited = null, isMandatoryDeposit = settings.isMandatory, locale = "en", - lockinPeriodFrequency = settings.lockInPeriod.frequency.toInt(), + lockinPeriodFrequency = lockInFreq, lockinPeriodFrequencyType = settings.lockInPeriod.frequencyTypeIndex, - mandatoryRecommendedDepositAmount = settings.recurringDepositDetails.depositAmount.toInt(), + mandatoryRecommendedDepositAmount = depositAmount, monthDayFormat = "dd MMMM", productId = s.recurringDepositAccountDetail.productId, recurringFrequency = settings.minDepositTerm.frequency.toIntOrNull(), recurringFrequencyType = settings.minDepositTerm.frequencyTypeIndex, submittedOnDate = s.recurringDepositAccountDetail.submittedOnDate, ) - observeNetwork() - - if (state.isOnline) { - recurringAccountRepository.createRecurringDepositAccount(payload).collect { dataState -> - when (dataState) { - is DataState.Error -> { - setErrorState(dataState.message) - } - is DataState.Loading -> { - setLoadingState() - } - is DataState.Success -> { - mutableStateFlow.update { - it.copy(screenState = null) - } + + recurringAccountRepository.createRecurringDepositAccount(payload).collect { dataState -> + when (dataState) { + is DataState.Error -> { + setErrorState(dataState.message) + } + is DataState.Loading -> { + setLoadingState() + } + is DataState.Success -> { + mutableStateFlow.update { + it.copy(screenState = null) } } } } } }feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/SettingsPage.kt (2)
332-339: Next button validation incorrectly requires penal interest fields when toggle is off.The
isNextButtonActivelogic requirespenalInterestandminimumBalanceForInterestCalculationto be non-blank regardless of whetherapplyPenalInterestis checked (line 277). This blocks users from proceeding when the penal interest feature is disabled.Apply this diff:
-val isNextButtonActive = settingsState.preMatureClosure.penalInterest.isNotBlank() && - settingsState.preMatureClosure.minimumBalanceForInterestCalculation.isNotBlank() && - settingsState.recurringDepositDetails.depositAmount.isNotBlank() && +val baseFieldsValid = + settingsState.recurringDepositDetails.depositAmount.isNotBlank() && settingsState.depositPeriod.period.isNotBlank() && settingsState.lockInPeriod.frequency.isNotBlank() && settingsState.minDepositTerm.frequency.isNotBlank() && settingsState.minDepositTerm.frequencyAfterInMultiplesOf.isNotBlank() && settingsState.maxDepositTerm.frequency.isNotBlank() + +val penalFieldsValid = !settingsState.preMatureClosure.applyPenalInterest || + (settingsState.preMatureClosure.penalInterest.isNotBlank() && + settingsState.preMatureClosure.minimumBalanceForInterestCalculation.isNotBlank()) + +val isNextButtonActive = baseFieldsValid && penalFieldsValid
120-134: Unsafe dropdown index access will crash on out-of-bounds.Line 121 uses
.get(index)which throwsIndexOutOfBoundsExceptionif the list size changes or the index is invalid (even with the-1check, list could be smaller than expected).Similar unsafe accesses exist at lines 164, 202, 231, 260, and 303.
Apply this pattern to all dropdown value lookups:
-value = if (settingsState.lockInPeriod.frequencyTypeIndex != -1) { - state.recurringDepositAccountTemplate.lockinPeriodFrequencyTypeOptions?.get(settingsState.lockInPeriod.frequencyTypeIndex)?.value ?: "" -} else { - "" -}, +value = state.recurringDepositAccountTemplate.lockinPeriodFrequencyTypeOptions + ?.getOrNull(settingsState.lockInPeriod.frequencyTypeIndex) + ?.value ?: "",Repeat for:
- Line 164:
depositPeriod.periodType- Line 202:
minDepositTerm.frequencyTypeIndex- Line 231:
minDepositTerm.frequencyTypeIndexAfterInMultiplesOf- Line 260:
maxDepositTerm.frequencyTypeIndex- Line 303:
preMatureClosure.interestPeriodIndex
🧹 Nitpick comments (1)
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountViewModel.kt (1)
149-205: Overly complex formatting helper reduces maintainability.The
formattedAmountmethod spans 56 lines with multiple edge cases, special handling for dots and signs, and complex string manipulation logic. This complexity makes the code difficult to understand, test, and maintain.Consider simplifying by:
- Using a decimal formatter library if available in the Kotlin Multiplatform context
- Breaking the logic into smaller, testable helper functions
- Adding unit tests to cover the edge cases
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountViewModel.kt(4 hunks)feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/SettingsPage.kt(1 hunks)version.txt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- version.txt
🧰 Additional context used
🧬 Code graph analysis (1)
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/SettingsPage.kt (3)
core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosCheckBox.kt (1)
MifosCheckBox(24-46)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 (1)
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountViewModel.kt (1)
60-78: Verify network check pattern aligns with codebase conventions.The
observeNetwork()method uses a one-shot check with.first(), which is better than polling. However, past review comments suggest checking network status immediately before each API call rather than maintaining anisOnlinestate field.Please verify whether this pattern is consistent with the merged NewLoanAccount and NewSavingsAccount implementations referenced in past reviews. Based on past feedback, the preferred pattern appears to be checking connectivity inline before each network request rather than maintaining state.
...n/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/SettingsPage.kt
Outdated
Show resolved
Hide resolved
# Conflicts: # feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientApplyNewApplications/ClientApplyNewApplicationsScreen.kt # version.txt
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (9)
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountViewModel.kt (4)
40-43: Don’t observe network in init; check connectivity right before requests.This pattern can race with request timing and complicates state. Prefer one-shot checks at call sites. See earlier reviewer note from sam-arth07.
60-78: Make network checks one-shot per request; avoid keepingisOnlineas source of truth.Replace observeNetwork() with a suspend check in each loader/submitter; update screen state accordingly. This simplifies flow and removes races. Matches prior feedback.
- private fun observeNetwork() { - viewModelScope.launch { - val isConnected = networkMonitor.isOnline.first() - mutableStateFlow.update { - it.copy(isOnline = isConnected) - } - if (isConnected) { - // loadTemplate() - // TODO testing only - loadTemplateByProduct() - } else { - setErrorState(getString(Res.string.no_internet_connection)) - } - } - } + // Prefer one-shot checks inside request functions instead of a global observer.
208-259: Crash risk: unsafe parsing and racy connectivity check in submit.
toInt()on user-formatted strings (e.g., "$ 1,000", "", ".") will throw.- Calling
observeNetwork()then readingstate.isOnlineis racy.Use a one-shot online check and safe parsing; bail with an error if parsing fails. This was flagged earlier.
private fun createRecurringDepositAccount() { viewModelScope.launch { val s = state val settings = s.recurringDepositAccountSettings + // One-shot connectivity check + val online = networkMonitor.isOnline.first() + if (!online) { + setErrorState(getString(Res.string.no_internet_connection)) + return@launch + } + + // Safe numeric parsing helpers + fun String.onlyDigits(): String = filter(Char::isDigit) + val lockinFreq = settings.lockInPeriod.frequency.toIntOrNull() + val depositAmountInt = settings.recurringDepositDetails.depositAmount.onlyDigits().toIntOrNull() + val depositPeriodInt = settings.depositPeriod.period.toIntOrNull() + val recurringFreq = settings.minDepositTerm.frequency.toIntOrNull() + + if (lockinFreq == null || depositAmountInt == null) { + setErrorState(message = "Please enter required numeric fields.") + return@launch + } val payload = RecurringDepositAccountPayload( adjustAdvanceTowardsFuturePayments = settings.adjustAdvancePayments, allowWithdrawal = settings.allowWithdrawals, clientId = clientId, dateFormat = "dd MMMM yyyy", - depositPeriod = settings.depositPeriod.period.toIntOrNull(), + depositPeriod = depositPeriodInt, depositPeriodFrequencyId = settings.depositPeriod.periodType, expectedFirstDepositOnDate = null, externalId = s.recurringDepositAccountDetail.externalId, fieldOfficerId = s.recurringDepositAccountDetail.fieldOfficer?.id, interestCalculationDaysInYearType = null, interestCalculationType = null, interestCompoundingPeriodType = null, interestPostingPeriodType = null, isCalendarInherited = null, isMandatoryDeposit = settings.isMandatory, locale = "en", - lockinPeriodFrequency = settings.lockInPeriod.frequency.toInt(), + lockinPeriodFrequency = lockinFreq, lockinPeriodFrequencyType = settings.lockInPeriod.frequencyTypeIndex, - mandatoryRecommendedDepositAmount = settings.recurringDepositDetails.depositAmount.toInt(), + mandatoryRecommendedDepositAmount = depositAmountInt, monthDayFormat = "dd MMMM", productId = s.recurringDepositAccountDetail.productId, - recurringFrequency = settings.minDepositTerm.frequency.toIntOrNull(), + recurringFrequency = recurringFreq, recurringFrequencyType = settings.minDepositTerm.frequencyTypeIndex, // date in dd MM yyyy format. submittedOnDate = s.recurringDepositAccountDetail.submittedOnDate, ) - observeNetwork() - - if (state.isOnline) { + if (online) { recurringAccountRepository.createRecurringDepositAccount(payload).collect { dataState -> when (dataState) { is DataState.Error -> { setErrorState(dataState.message) } is DataState.Loading -> { setLoadingState() } is DataState.Success -> { mutableStateFlow.update { it.copy(screenState = null) } } } } } } }
537-545: Remove test-only default for productId; use -1 and block network calls until selected.Using 6 risks accidental real calls. Default to -1 and treat it as “none selected.” This was previously flagged.
data class RecurringAccountDetailsState( - // TODO - // productId is set 6 only for testing. - // It should be set -1 when the implementation of detail screen is finished. - val productId: Int = 6, + val productId: Int = -1, val externalId: String = "", val submittedOnDate: String = "", val fieldOfficer: FieldOfficerOption? = null, )feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountScreen.kt (1)
34-35: Align name with file/route: SettingPage → SettingsPage.This was flagged earlier; apply consistently across definition and call site.
-import com.mifos.feature.recurringDeposit.newRecurringDepositAccount.pages.SettingPage +import com.mifos.feature.recurringDeposit.newRecurringDepositAccount.pages.SettingsPage @@ - SettingPage( + SettingsPage( state = state, onAction = onAction, )Also applies to: 82-85
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/SettingsPage.kt (4)
65-69: Rename to SettingsPage and fix preview.Keep names consistent with file/route; also update the preview.
-@OptIn(ExperimentalMaterial3Api::class) -@Composable -fun SettingPage( +@OptIn(ExperimentalMaterial3Api::class) +@Composable +fun SettingsPage( @@ -@Preview -@Composable -private fun SettingPagePreview() { - SettingPage( +@Preview +@Composable +private fun SettingsPagePreview() { + SettingsPage( state = RecurringAccountState(), onAction = {}, ) }Apply corresponding import/call-site changes (see RecurringAccountScreen.kt).
Also applies to: 352-359
120-127: Bounds-check all option lookups with getOrNull.Direct get(index) can crash. Use getOrNull on every options access.
- state.recurringDepositAccountTemplate.lockinPeriodFrequencyTypeOptions?.get(settingsState.lockInPeriod.frequencyTypeIndex)?.value ?: "" + state.recurringDepositAccountTemplate.lockinPeriodFrequencyTypeOptions + ?.getOrNull(settingsState.lockInPeriod.frequencyTypeIndex) + ?.value ?: "" @@ - state.recurringDepositAccountTemplate.periodFrequencyTypeOptions?.get(settingsState.depositPeriod.periodType)?.value ?: "" + state.recurringDepositAccountTemplate.periodFrequencyTypeOptions + ?.getOrNull(settingsState.depositPeriod.periodType) + ?.value ?: "" @@ - state.recurringDepositAccountTemplate.periodFrequencyTypeOptions?.get(settingsState.minDepositTerm.frequencyTypeIndex)?.value ?: "" + state.recurringDepositAccountTemplate.periodFrequencyTypeOptions + ?.getOrNull(settingsState.minDepositTerm.frequencyTypeIndex) + ?.value ?: "" @@ - state.recurringDepositAccountTemplate?.periodFrequencyTypeOptions?.get(settingsState.minDepositTerm.frequencyTypeIndexAfterInMultiplesOf)?.value ?: "" + state.recurringDepositAccountTemplate.periodFrequencyTypeOptions + ?.getOrNull(settingsState.minDepositTerm.frequencyTypeIndexAfterInMultiplesOf) + ?.value ?: "" @@ - state.recurringDepositAccountTemplate.periodFrequencyTypeOptions?.get(settingsState.maxDepositTerm.frequencyTypeIndex)?.value ?: "" + state.recurringDepositAccountTemplate.periodFrequencyTypeOptions + ?.getOrNull(settingsState.maxDepositTerm.frequencyTypeIndex) + ?.value ?: "" @@ - state.recurringDepositAccountTemplate.preClosurePenalInterestOnTypeOptions?.get(settingsState.preMatureClosure.interestPeriodIndex)?.value ?: "" + state.recurringDepositAccountTemplate.preClosurePenalInterestOnTypeOptions + ?.getOrNull(settingsState.preMatureClosure.interestPeriodIndex) + ?.value ?: ""Also applies to: 163-170, 201-208, 230-237, 259-266, 302-309
201-205: Wrong guard for minDepositTerm dropdown.Check the correct index you use below.
- value = if (settingsState.depositPeriod.periodType != -1) { + value = if (settingsState.minDepositTerm.frequencyTypeIndex != -1) { state.recurringDepositAccountTemplate.periodFrequencyTypeOptions - ?.get(settingsState.minDepositTerm.frequencyTypeIndex)?.value ?: "" + ?.getOrNull(settingsState.minDepositTerm.frequencyTypeIndex)?.value ?: "" } else { "" },
332-339: Don’t require penal-interest fields when toggle is off.Gate them behind the toggle.
- val isNextButtonActive = settingsState.preMatureClosure.penalInterest.isNotBlank() && - settingsState.preMatureClosure.minimumBalanceForInterestCalculation.isNotBlank() && - settingsState.recurringDepositDetails.depositAmount.isNotBlank() && - settingsState.depositPeriod.period.isNotBlank() && - settingsState.lockInPeriod.frequency.isNotBlank() && - settingsState.minDepositTerm.frequency.isNotBlank() && - settingsState.minDepositTerm.frequencyAfterInMultiplesOf.isNotBlank() && - settingsState.maxDepositTerm.frequency.isNotBlank() + val baseOk = + settingsState.recurringDepositDetails.depositAmount.isNotBlank() && + settingsState.depositPeriod.period.isNotBlank() && + settingsState.lockInPeriod.frequency.isNotBlank() && + settingsState.minDepositTerm.frequency.isNotBlank() && + settingsState.minDepositTerm.frequencyAfterInMultiplesOf.isNotBlank() && + settingsState.maxDepositTerm.frequency.isNotBlank() + val penalOk = !settingsState.preMatureClosure.applyPenalInterest || + (settingsState.preMatureClosure.penalInterest.isNotBlank() && + settingsState.preMatureClosure.minimumBalanceForInterestCalculation.isNotBlank()) + val isNextButtonActive = baseOk && penalOk
🧹 Nitpick comments (16)
core/model/src/commonMain/kotlin/com/mifos/core/model/objects/payloads/RecurringDepositAccountPayload.kt (1)
16-16: Consider removing the commented-outchargesfield entirely.Keeping commented-out code in the repository can reduce clarity and maintainability. If this field is needed for future use or documentation, consider adding a proper comment explaining its purpose instead.
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountRoute.kt (2)
18-20: Avoid sentinel default; require explicit clientId.Defaulting to -1 risks accidental navigation with an invalid client. Make clientId mandatory (no default) to catch mistakes at compile time.
-@Serializable -data class RecurringAccountRoute( - val clientId: Int = -1, -) +@Serializable +data class RecurringAccountRoute( + val clientId: Int, +)
31-36: Validate clientId before navigating.Defensive check prevents navigating with invalid ids and simplifies downstream error handling.
-fun NavController.navigateToRecurringAccountRoute( - clientId: Int, -) { +fun NavController.navigateToRecurringAccountRoute( + clientId: Int, +) { + require(clientId > 0) { "clientId must be > 0" } this.navigate( RecurringAccountRoute(clientId = clientId), ) }feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountViewModel.kt (3)
79-87: Retry should re-run the failing request with a one-shot connectivity check, not re-start observation.Call the specific loader guarded by an online check.
private fun handleRetry() { mutableStateFlow.update { it.copy( screenState = null, recurringDepositAccountTemplate = RecurringDepositAccountTemplate(), ) } - observeNetwork() + // Example: retry last attempted action; minimally, re-load by selected product if any. + if (state.recurringDepositAccountDetail.productId > 0) { + loadTemplateByProduct() + } else { + loadTemplate() + } }
123-147: Gate template load by product with connectivity and valid productId.Avoid network calls when offline or when productId is unset.
private fun loadTemplateByProduct() { viewModelScope.launch { - recurringAccountRepository.getRecurringAccountTemplateByProduct( - clientId = clientId, - productId = state.recurringDepositAccountDetail.productId, - ).collect { templateState -> + val productId = state.recurringDepositAccountDetail.productId + if (productId <= 0) { + setErrorState(message = "Select a product first") + return@launch + } + val online = networkMonitor.isOnline.first() + if (!online) { + setErrorState(getString(Res.string.no_internet_connection)) + return@launch + } + recurringAccountRepository.getRecurringAccountTemplateByProduct( + clientId = clientId, + productId = productId, + ).collect { templateState -> when (templateState) { is DataState.Error -> { setErrorState(message = templateState.message) } is DataState.Loading -> { setLoadingState() } is DataState.Success -> { mutableStateFlow.update { it.copy( screenState = null, recurringDepositAccountTemplate = templateState.data, ) } } } } } }
508-517: Gate product change side-effect with validation and connectivity.Avoid firing a network call when offline or when productId is invalid.
is RecurringAccountAction.RecurringAccountDetailsAction.SetProductId -> { mutableStateFlow.update { it.copy( recurringDepositAccountDetail = it.recurringDepositAccountDetail.copy( productId = action.productId, ), ) } - loadTemplateByProduct() + if (action.productId > 0) { + loadTemplateByProduct() + } else { + setErrorState("Select a valid product") + } }core/network/src/commonMain/kotlin/com/mifos/core/network/di/DataMangerModule.kt (2)
27-34: Remove unused import.
singleOfis imported but unused in this file.-import org.koin.core.module.dsl.singleOf
56-57: New binding: confirm constructor deps.Verify
DataManagerRecurringAccount(get())matches its constructor (e.g., expects BaseApiManager). If more deps are needed, wire them here.feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/di/RecurringDepositModule.kt (1)
16-18: Rename module val to lowerCamelCase.Kotlin vals should be lowerCamelCase. Rename to avoid confusion with types.
-val RecurringDepositModule = module { +val recurringDepositModule = module { viewModelOf(::RecurringAccountViewModel) }core/data/src/commonMain/kotlin/com/mifos/core/data/repository/RecurringAccountRepository.kt (1)
27-30: Make payload non-null.Creation should require a payload. Tighten the API to avoid null plumbing.
- fun createRecurringDepositAccount( - recurringDepositAccountPayload: RecurringDepositAccountPayload?, - ): Flow<DataState<RecurringDeposit>> + fun createRecurringDepositAccount( + recurringDepositAccountPayload: RecurringDepositAccountPayload, + ): Flow<DataState<RecurringDeposit>>feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountScreen.kt (1)
55-56: Wire retry and rename component (it’s not a dialog).onRetry currently does nothing. Expose a callback and call it; also consider a more accurate name.
- RecurringDepositAccountDialogBox(state = state) + RecurringDepositAccountScreenState( + state = state, + onRetry = { viewModel.trySendAction(RecurringAccountAction.Retry /* adjust to your actual action */) } + ) @@ -@Composable -fun RecurringDepositAccountDialogBox( - state: RecurringAccountState, -) { +@Composable +fun RecurringDepositAccountScreenState( + state: RecurringAccountState, + onRetry: () -> Unit, +) { @@ - MifosErrorComponent( + MifosErrorComponent( message = state.screenState.message, isRetryEnabled = true, - onRetry = { - // Retry action can be handled here - }, + onRetry = onRetry, )Please adjust the Retry action name to match your ViewModel.
Also applies to: 119-138
core/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/RecurringAccountRepositoryImp.kt (2)
21-23: Restrict visibility of dependency.Keep internals private to the repo.
-class RecurringAccountRepositoryImp( - val dataManagerRecurringAccount: DataManagerRecurringAccount, +class RecurringAccountRepositoryImp( + private val dataManagerRecurringAccount: DataManagerRecurringAccount, ) : RecurringAccountRepository {
21-21: Style nit: prefer “Impl”.If you’re touching names later, consider RecurringAccountRepositoryImpl for consistency with common conventions. No change required in this PR.
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/SettingsPage.kt (1)
274-281: Use consistent checkbox component.Elsewhere you use MifosCheckBox. Standardize for consistent UX.
- Row(verticalAlignment = Alignment.CenterVertically) { - Checkbox( - settingsState.preMatureClosure.applyPenalInterest, - onCheckedChange = { onAction(RecurringAccountAction.RecurringAccountSettingsAction.TogglePreMatureClosureApplyPenalInterest) }, - ) - Text(stringResource(Res.string.apply_penal_interest)) - } + Row(verticalAlignment = Alignment.CenterVertically) { + MifosCheckBox( + text = stringResource(Res.string.apply_penal_interest), + checked = settingsState.preMatureClosure.applyPenalInterest, + onCheckChanged = { onAction(RecurringAccountAction.RecurringAccountSettingsAction.TogglePreMatureClosureApplyPenalInterest) }, + ) + }feature/recurringDeposit/src/commonMain/composeResources/values/string.xml (2)
18-20: Consider moving question marks from string labels to UI layer.Form field labels typically should not include question marks—those are better handled as separate labels or prompts in the UI layout. This is a minor usability convention, but moving the punctuation to the presentation layer keeps strings reusable and clearer.
For example, consider:
- <string name="is_mandatory_deposit">Is Mandatory Deposit?</string> + <string name="is_mandatory_deposit">Mandatory Deposit</string>
29-29: Clarify ambiguous string phrasing for product alignment.
- Line 29: "And thereafter, in Multiples of" is grammatically awkward. Consider: "In Multiples of" or "Thereafter in Multiples of" for clarity.
- Line 32: "(less)" in "Apply Penal Interest (less)" is ambiguous. Confirm this phrasing aligns with product intent—does it mean deduction, reduction, or something else?
Also applies to: 32-32
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
cmp-navigation/build.gradle.kts(1 hunks)cmp-navigation/src/commonMain/kotlin/cmp/navigation/di/KoinModules.kt(2 hunks)core/data/src/commonMain/kotlin/com/mifos/core/data/repository/RecurringAccountRepository.kt(1 hunks)core/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/RecurringAccountRepositoryImp.kt(1 hunks)core/model/src/commonMain/kotlin/com/mifos/core/model/objects/payloads/RecurringDepositAccountPayload.kt(1 hunks)core/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerRecurringAccount.kt(0 hunks)core/network/src/commonMain/kotlin/com/mifos/core/network/di/DataMangerModule.kt(2 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientApplyNewApplications/ClientApplyNewApplicationRoute.kt(1 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientApplyNewApplications/ClientApplyNewApplicationsScreen.kt(2 hunks)feature/recurringDeposit/src/commonMain/composeResources/values/string.xml(1 hunks)feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/di/RecurringDepositModule.kt(1 hunks)feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountRoute.kt(2 hunks)feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountScreen.kt(6 hunks)feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountViewModel.kt(4 hunks)feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/SettingsPage.kt(1 hunks)
💤 Files with no reviewable changes (1)
- core/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerRecurringAccount.kt
🔇 Additional comments (11)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientApplyNewApplications/ClientApplyNewApplicationRoute.kt (1)
28-28: LGTM! Navigation signature now consistent with client-scoped account types.The signature change from
() -> Unitto(Int) -> Unitproperly aligns recurring account navigation with the existing loan and savings account patterns, enabling clientId to be threaded through the recurring deposit flow.feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientApplyNewApplications/ClientApplyNewApplicationsScreen.kt (2)
65-65: LGTM! Signature change consistent with route definition.The parameter signature properly matches the route definition update and maintains parity with other client-scoped account navigation callbacks.
78-80: LGTM! Event handling properly threads clientId through navigation.All three client-scoped account types (Loan, Recurring, Savings) now consistently pass
state.clientIdto their navigation callbacks, ensuring the client context flows correctly through the recurring deposit feature.cmp-navigation/build.gradle.kts (1)
49-49: Dependency add looks good; double-check for cycles.Ensure feature.recurringDeposit doesn’t depend back on cmp-navigation to avoid a circular dependency at compile time.
cmp-navigation/src/commonMain/kotlin/cmp/navigation/di/KoinModules.kt (2)
34-34: Import is correct and non-duplicated now.
87-87: Including RecurringDepositModule here is correct.Wires the feature into DI with existing modules; no issues spotted.
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/di/RecurringDepositModule.kt (1)
13-18: Verify the Koin DSL import for ViewModel.Please confirm that org.koin.core.module.dsl.viewModelOf is valid for your Koin version and commonMain target. If not, switch to the androidx DSL or fallback to factoryOf.
Example fixes:
- If using AndroidX ViewModel DSL:
-import org.koin.core.module.dsl.viewModelOf +import org.koin.androidx.viewmodel.dsl.viewModelOf
- If core-only in commonMain, consider:
- viewModelOf(::RecurringAccountViewModel) + factoryOf(::RecurringAccountViewModel)Then obtain it with the appropriate koinViewModel/get in the UI layer.
core/data/src/commonMain/kotlin/com/mifos/core/data/repository/RecurringAccountRepository.kt (1)
20-25: Renames look good.The method names now use “Recurring” and read clearly. No issues spotted.
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountScreen.kt (1)
104-116: LGTM: ScreenState now drives visibility.Hiding the stepper when state.screenState != null aligns with the earlier feedback to use ScreenState.
core/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/RecurringAccountRepositoryImp.kt (1)
24-37: Interface → implementation mapping looks correct.Renamed methods align with DataManager calls and asDataStateFlow usage.
feature/recurringDeposit/src/commonMain/composeResources/values/string.xml (1)
1-39: Well-structured string resources supporting recurring deposit feature.The XML is valid, all 21 new string keys follow the consistent snake_case naming convention, and the externalization is well-suited for localization. The additions align with the settings step and account creation flows described in the PR objectives.
| onOptionSelected = { id, name -> | ||
| onAction( | ||
| RecurringAccountAction.RecurringAccountSettingsAction.SetLockInPeriodType(id), | ||
| ) | ||
| }, | ||
| label = stringResource(Res.string.type), | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Confirm ID vs index semantics for onOptionSelected.
You store “frequencyTypeIndex” but you pass id to actions. Ensure id is indeed the list index; else store both (id for payload, index for display) or change actions to accept index.
To verify across codebase, run:
Also applies to: 172-176, 210-215, 239-244, 268-273, 311-316
🏁 Script executed:
#!/bin/bash
# Inspect actions and state fields for index vs id
rg -nP -C2 'RecurringAccountSettingsAction\.(SetLockInPeriodType|SetDepositPeriodType|SetMinDepositTermFreqType|SetMinDepositTermFreqTypeAfterInMultiOf|SetMaxDepositTermFreqType|SetPreMatureClosureInterestPeriodIndex)\('
rg -nP -C3 'frequencyType(Index|Id)|interestPeriod(Index|Id)' --type=ktLength of output: 5638
---
🏁 Script executed:
```shell
#!/bin/bash
# Find action definitions
fd -e kt -x grep -l "SetLockInPeriodType\|SetDepositPeriodType\|SetPreMatureClosureInterestPeriodIndex" {} \;
Length of output: 272
🏁 Script executed:
#!/bin/bash
# Search for action definitions with broader pattern
rg "sealed.*RecurringAccountSettingsAction|data class Set(LockInPeriodType|DepositPeriodType|MinDepositTermFreqType|PreMatureClosureInterestPeriodIndex)" -A2Length of output: 3866
🏁 Script executed:
#!/bin/bash
# Search for state fields to see how they're stored (index vs id)
rg "val.*frequencyType|val.*interestPeriod|var.*frequencyType|var.*interestPeriod" --type=kt -B1 -A1Length of output: 26
🏁 Script executed:
#!/bin/bash
# Find the dropdown/option component to understand what onOptionSelected receives
rg "onOptionSelected\s*=" -B5 -A1 | head -100Length of output: 13955
🏁 Script executed:
#!/bin/bash
# Look for SelectField or DropDown component definition
fd -e kt -name "*Select*" -o -name "*Dropdown*" -o -name "*Option*" | head -20Length of output: 230
🏁 Script executed:
#!/bin/bash
# Search for the component that defines onOptionSelected callback signature
rg "onOptionSelected.*Callback|fun.*onOptionSelected" -B2 -A2Length of output: 717
---
🏁 Script executed:
```shell
#!/bin/bash
# Find MifosTextFieldDropdown component definition
rg "fun MifosTextFieldDropdown|class MifosTextFieldDropdown|@Composable.*MifosTextFieldDropdown" -A15
Length of output: 6282
🏁 Script executed:
#!/bin/bash
# Search more broadly for dropdown implementations
rg "onOptionSelected" --type=kotlin -B3 -A3 2>/dev/null || rg "onOptionSelected" -B3 -A3 | grep -A3 -B3 "fun\|@"Length of output: 121770
🏁 Script executed:
#!/bin/bash
# Look for where onOptionSelected callback is invoked with index
rg "onOptionSelected\(" -B2 -A1Length of output: 1239
🏁 Script executed:
#!/bin/bash
# Search for the list mapping and how options are indexed in SettingsPage
cat "feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/SettingsPage.kt" | head -150Length of output: 7863
Rename callback parameter from id to index for clarity and consistency with codebase convention.
The onOptionSelected callback receives the list index (0-based position in options), not an ID. SettingsPage.kt names this parameter id, which is inconsistent with the pattern used across the codebase (savings, loan, client features all use { index, value ->). Renaming to index clarifies the semantics and aligns with the rest of the codebase.
Applies to:
- Lines 129–135 (SetLockInPeriodType)
- Lines 172–176 (SetDepositPeriodType)
- Lines 210–215 (SetMinDepositTermFreqType)
- Lines 239–244 (SetMinDepositTermFreqTypeAfterInMultiOf)
- Lines 268–273 (SetMaxDepositTermFreqType)
- Lines 311–316 (SetPreMatureClosureInterestPeriodIndex)
🤖 Prompt for AI Agents
In
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/SettingsPage.kt
around lines 129–135, 172–176, 210–215, 239–244, 268–273 and 311–316 the
onOptionSelected lambda parameter is named `id` but actually receives the
0-based list index; rename the lambda parameter to `index` in each occurrence
and update any uses inside the lambda (e.g., pass `index` into
RecurringAccountAction constructors like SetLockInPeriodType(index)) so the
naming matches the rest of the codebase and clarifies semantics.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
@coderabbitai pause |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountViewModel.kt (1)
534-542: Test-onlyproductIddefault still present.Despite previous review feedback,
productIdstill defaults to6for testing (lines 536-538). This risks accidental real API calls in production builds and should be changed to-1to require explicit selection.Apply this diff:
data class RecurringAccountDetailsState( - // TODO - // productId is set 6 only for testing. - // It should be set -1 when the implementation of detail screen is finished. - val productId: Int = 6, + val productId: Int = -1,Also update
loadTemplateByProduct()(line 123) to skip the API call ifproductId <= 0:private fun loadTemplateByProduct() { viewModelScope.launch { + if (state.recurringDepositAccountDetail.productId <= 0) { + setErrorState("Please select a product first") + return@launch + } val online = networkMonitor.isOnline.first()Based on learnings
🧹 Nitpick comments (5)
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/SettingsPage.kt (2)
86-89: Use MifosCheckBox for consistency.The first checkbox uses the plain
Checkboxcomponent while the other two checkboxes in this section useMifosCheckBox. For consistency and to follow the established pattern, refactor this to useMifosCheckBox.Apply this diff:
- Row(verticalAlignment = Alignment.CenterVertically) { - Checkbox(settingsState.isMandatory, onCheckedChange = { onAction(RecurringAccountAction.RecurringAccountSettingsAction.ToggleMandatoryDeposit) }) - Text(stringResource(Res.string.is_mandatory_deposit)) - } + Row(verticalAlignment = Alignment.CenterVertically) { + MifosCheckBox( + text = stringResource(Res.string.is_mandatory_deposit), + checked = settingsState.isMandatory, + onCheckChanged = { onAction(RecurringAccountAction.RecurringAccountSettingsAction.ToggleMandatoryDeposit) }, + ) + }
235-237: Remove inconsistent null-safety operator.Line 235 uses
state.recurringDepositAccountTemplate?.periodFrequencyTypeOptionswith optional chaining, while all other dropdown lookups in this file usestate.recurringDepositAccountTemplate.periodFrequencyTypeOptionswithout the?operator (lines 125, 168, 206, 264, 307). Remove the extra?for consistency.Apply this diff:
- options = state.recurringDepositAccountTemplate?.periodFrequencyTypeOptions?.map { + options = state.recurringDepositAccountTemplate.periodFrequencyTypeOptions?.map { it.value ?: "" } ?: emptyList(),feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountViewModel.kt (3)
79-87: Multiple retry clicks may launch concurrent coroutines.Each
handleRetry()call launches a newviewModelScope.launch(viaobserveNetwork()) without cancelling previous ones. WhileviewModelScopewill eventually clean them up, rapid retry clicks can queue redundant template fetches.Consider storing a single
Jobreference and cancelling it before launching a new one, or use a mechanism likeflow.collectLatestto automatically cancel previous work.
149-190: Complex formatting logic needs test coverage.The 40+ line
formattedAmountfunction handles many edge cases (empty, "-", ".", decimal truncation, etc.) but its complexity makes correctness hard to verify by inspection alone. Line 169's condition|| parts[1].isEmpty()is redundant (always true whenparts.size > 1), and the logic at lines 175-187 is difficult to follow.Add unit tests covering:
- Empty string, "-", "."
- Negative numbers
- Decimal handling (e.g., ".5", "-.5", "1.234" → "1.23")
- Leading zeros (e.g., "007.5" → "7.5")
- Non-numeric input (e.g., "abc123")
Then simplify the logic if tests reveal redundant branches.
544-555: UnusedcanDoNextfield.The
canDoNextfield (line 545) is initialized tofalsebut never updated anywhere in the ViewModel. If it's intended for form validation before allowing navigation, add logic to compute it based on required fields; otherwise, remove it.If validation is needed:
val canDoNext: Boolean = false, + // Or compute it: + val canDoNext: Boolean = recurringDepositDetails.depositAmount.isNotEmpty() + && lockInPeriod.frequency.isNotEmpty() + // ... other required fieldsOr remove it if unused:
- val canDoNext: Boolean = false,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountViewModel.kt(4 hunks)feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/SettingsPage.kt(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/SettingsPage.kt (3)
core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosCheckBox.kt (1)
MifosCheckBox(24-46)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 (3)
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountViewModel.kt (3)
89-99: LGTM!The step progression logic correctly compares
currentStepagainsttotalStepsand emitsFinishwhen complete. Extraction into a dedicated function improves readability.
258-516: LGTM!Action handling correctly delegates to helper functions (
moveToNextStep,handleRetry) and performs immutable state updates. TheSetProductIdaction appropriately triggersloadTemplateByProduct()to refresh the template when the product changes.While the function is lengthy (~250 lines), this is typical for MVI-style action reducers.
593-634: LGTM!Action and event hierarchies are well-structured. The sealed class design ensures exhaustive
whenhandling, and the comprehensiveRecurringAccountSettingsActionsubtypes cover all form field mutations.
| private fun observeNetwork() { | ||
| viewModelScope.launch { | ||
| val isConnected = networkMonitor.isOnline.first() | ||
| mutableStateFlow.update { | ||
| it.copy(isOnline = isConnected) | ||
| } | ||
| if (isConnected) { | ||
| // loadTemplate() | ||
| // TODO | ||
| // Used only for testing purpose | ||
| // Must be removed from here after the implementation of detail screen is finished. | ||
| loadTemplateByProduct() | ||
| } else { | ||
| setErrorState( | ||
| getString(Res.string.no_internet_connection), | ||
| ) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Misleading function name + commented code in production.
observeNetwork()calls.first()which retrieves a single value rather than continuously observing connectivity changes—the name implies ongoing monitoring but the implementation is one-shot.- Lines 67-71 contain commented-out code and a TODO referencing test-only behavior that should be resolved before merging.
Apply this diff to clarify intent and remove test artifacts:
- private fun observeNetwork() {
+ private fun checkInitialConnectivity() {
viewModelScope.launch {
val isConnected = networkMonitor.isOnline.first()
mutableStateFlow.update {
it.copy(isOnline = isConnected)
}
if (isConnected) {
-// loadTemplate()
- // TODO
- // Used only for testing purpose
- // Must be removed from here after the implementation of detail screen is finished.
loadTemplateByProduct()
} else {And update the call site at line 41:
init {
- observeNetwork()
+ checkInitialConnectivity()
}And line 86:
- observeNetwork()
+ checkInitialConnectivity()| private fun loadTemplate() { | ||
| viewModelScope.launch { | ||
| recurringAccountRepository.getRecurringAccountTemplate().collect { templateState -> | ||
| when (templateState) { | ||
| is DataState.Error -> { | ||
| setErrorState(message = templateState.message) | ||
| } | ||
| is DataState.Loading -> { | ||
| setLoadingState() | ||
| } | ||
| is DataState.Success -> { | ||
| mutableStateFlow.update { | ||
| it.copy( | ||
| screenState = null, | ||
| recurringDepositAccountTemplate = templateState.data, | ||
| ) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| private fun loadTemplateByProduct() { | ||
| viewModelScope.launch { | ||
| recurringAccountRepository.getRecurringAccountTemplateByProduct( | ||
| clientId = clientId, | ||
| productId = state.recurringDepositAccountDetail.productId, | ||
| ).collect { templateState -> | ||
| when (templateState) { | ||
| is DataState.Error -> { | ||
| setErrorState(message = templateState.message) | ||
| } | ||
| is DataState.Loading -> { | ||
| setLoadingState() | ||
| } | ||
| is DataState.Success -> { | ||
| mutableStateFlow.update { | ||
| it.copy( | ||
| screenState = null, | ||
| recurringDepositAccountTemplate = templateState.data, | ||
| ) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add network check before API calls.
Both template-loading methods make network requests without verifying connectivity first. Per earlier review feedback, check networkMonitor.isOnline.first() before calling the repository and update screenState to NetworkError if offline.
Apply this diff:
private fun loadTemplate() {
viewModelScope.launch {
+ val online = networkMonitor.isOnline.first()
+ if (!online) {
+ setErrorState(getString(Res.string.no_internet_connection))
+ return@launch
+ }
recurringAccountRepository.getRecurringAccountTemplate().collect { templateState -> private fun loadTemplateByProduct() {
viewModelScope.launch {
+ val online = networkMonitor.isOnline.first()
+ if (!online) {
+ setErrorState(getString(Res.string.no_internet_connection))
+ return@launch
+ }
recurringAccountRepository.getRecurringAccountTemplateByProduct(Based on learnings
🤖 Prompt for AI Agents
In
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountViewModel.kt
around lines 101–147, both loadTemplate() and loadTemplateByProduct() call the
repository without checking network connectivity; before calling the repository,
call networkMonitor.isOnline.first() and if it returns false update
mutableStateFlow to set screenState = ScreenState.NetworkError (or the existing
NetworkError enum/object) and return early; if true proceed with the existing
repository.collect flow. Ensure the network check happens inside the same
viewModelScope.launch before any repository invocation so no network request is
attempted when offline.
| private fun createRecurringDepositAccount() { | ||
| viewModelScope.launch { | ||
| val s = state | ||
| val settings = s.recurringDepositAccountSettings | ||
|
|
||
| val online = networkMonitor.isOnline.first() | ||
| if (!online) { | ||
| setErrorState(getString(Res.string.no_internet_connection)) | ||
| return@launch | ||
| } | ||
|
|
||
| val lockinFreq = settings.lockInPeriod.frequency.toIntOrNull() | ||
| val depositAmountInt = settings.recurringDepositDetails.depositAmount | ||
| .filter(Char::isDigit) | ||
| .toIntOrNull() | ||
| val recurringFreq = settings.minDepositTerm.frequency.toIntOrNull() | ||
|
|
||
| val payload = RecurringDepositAccountPayload( | ||
| adjustAdvanceTowardsFuturePayments = settings.adjustAdvancePayments, | ||
| allowWithdrawal = settings.allowWithdrawals, | ||
| clientId = clientId, | ||
| dateFormat = "dd MMMM yyyy", | ||
| depositPeriod = settings.depositPeriod.period.toIntOrNull(), | ||
| depositPeriodFrequencyId = settings.depositPeriod.periodType, | ||
| expectedFirstDepositOnDate = null, | ||
| externalId = s.recurringDepositAccountDetail.externalId, | ||
| fieldOfficerId = s.recurringDepositAccountDetail.fieldOfficer?.id, | ||
| interestCalculationDaysInYearType = null, | ||
| interestCalculationType = null, | ||
| interestCompoundingPeriodType = null, | ||
| interestPostingPeriodType = null, | ||
| isCalendarInherited = null, | ||
| isMandatoryDeposit = settings.isMandatory, | ||
| locale = "en", | ||
| lockinPeriodFrequency = lockinFreq, | ||
| lockinPeriodFrequencyType = settings.lockInPeriod.frequencyTypeIndex, | ||
| mandatoryRecommendedDepositAmount = depositAmountInt, | ||
| monthDayFormat = "dd MMMM", | ||
| productId = s.recurringDepositAccountDetail.productId, | ||
| recurringFrequency = recurringFreq, | ||
| recurringFrequencyType = settings.minDepositTerm.frequencyTypeIndex, | ||
| // date in dd MM yyyy format. | ||
| submittedOnDate = s.recurringDepositAccountDetail.submittedOnDate, | ||
| ) | ||
|
|
||
| if (state.isOnline) { | ||
| recurringAccountRepository.createRecurringDepositAccount(payload).collect { dataState -> | ||
| when (dataState) { | ||
| is DataState.Error -> { | ||
| setErrorState(dataState.message) | ||
| } | ||
| is DataState.Loading -> { | ||
| setLoadingState() | ||
| } | ||
| is DataState.Success -> { | ||
| mutableStateFlow.update { | ||
| it.copy(screenState = null) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Redundant connectivity check + missing null validation.
- Line 238 checks
state.isOnlineafter line 198 already obtainedonlinefromnetworkMonitor.isOnline.first()and returned early if offline—the second check is unreachable if offline. - Lines 204-208 parse with
toIntOrNull()(good), but there's no validation ifnullis acceptable for the API. IflockinPeriodFrequency,mandatoryRecommendedDepositAmount, orrecurringFrequencyare required fields, passingnullmay cause a backend error.
Apply this diff to remove the redundant check and validate required fields:
- if (state.isOnline) {
+ // Validate required fields
+ if (depositAmountInt == null) {
+ setErrorState("Deposit amount is required")
+ return@launch
+ }
+ if (lockinFreq == null) {
+ setErrorState("Lock-in period frequency is required")
+ return@launch
+ }
+ if (recurringFreq == null) {
+ setErrorState("Recurring frequency is required")
+ return@launch
+ }
+
recurringAccountRepository.createRecurringDepositAccount(payload).collect { dataState ->
when (dataState) {
is DataState.Error -> {
@@ -247,7 +255,6 @@
}
}
}
- }
}
}Verify with your backend/API documentation which fields are truly required.
🤖 Prompt for AI Agents
In
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountViewModel.kt
around lines 193–256, remove the redundant online check at line ~238 (we already
used networkMonitor.isOnline.first() at line ~198 and returned if offline) and
instead validate the parsed integers lockinFreq, depositAmountInt, and
recurringFreq (lines ~204–208) before constructing the payload: for each value
that the API requires, if toIntOrNull() returned null set an appropriate error
state and return from the coroutine early; only build and call the repository
when required fields are non-null (or convert/replace with safe defaults if
backend allows). Ensure the code uses the previously-obtained local online
variable and does not keep the unreachable state.isOnline branch.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (9)
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/SettingsPage.kt (4)
65-68: Rename function to SettingsPage to match filename.The composable is named
SettingPagebut should beSettingsPageto match the filename (SettingsPage.kt) and align with the step name "step_settings".-fun SettingPage( +fun SettingsPage( state: RecurringAccountState, onAction: (RecurringAccountAction.RecurringAccountSettingsAction) -> Unit, ) {Also update the preview function name at line 354:
-private fun SettingPagePreview() { - SettingPage( +private fun SettingsPagePreview() { + SettingsPage(And the import/call in RecurringAccountScreen.kt line 34 and 82.
121-127: Protect dropdown lookups against IndexOutOfBounds.Multiple dropdown value lookups use
.get(index)with only a-1guard. If the options list is modified or truncated, this can throwIndexOutOfBoundsException. Use.getOrNull(index)instead.Apply this pattern to all dropdown value lookups:
-value = if (settingsState.lockInPeriod.frequencyTypeIndex != -1) { - state.recurringDepositAccountTemplate.lockinPeriodFrequencyTypeOptions?.get(settingsState.lockInPeriod.frequencyTypeIndex)?.value ?: "" -} else { - "" -}, +value = state.recurringDepositAccountTemplate.lockinPeriodFrequencyTypeOptions + ?.getOrNull(settingsState.lockInPeriod.frequencyTypeIndex) + ?.value ?: "",Apply the same pattern to lines 164, 202, 231, 260, and 303.
Also applies to: 164-170, 202-208, 231-237, 260-266, 303-309
332-339: Fix Next button validation to respect penal interest toggle.The validation always requires
penalInterestandminimumBalanceForInterestCalculationto be non-blank, even whenapplyPenalInterestis false. This blocks users from proceeding when the penal interest fields are hidden.- val isNextButtonActive = settingsState.preMatureClosure.penalInterest.isNotBlank() && - settingsState.preMatureClosure.minimumBalanceForInterestCalculation.isNotBlank() && - settingsState.recurringDepositDetails.depositAmount.isNotBlank() && + val baseFieldsValid = + settingsState.recurringDepositDetails.depositAmount.isNotBlank() && settingsState.depositPeriod.period.isNotBlank() && settingsState.lockInPeriod.frequency.isNotBlank() && settingsState.minDepositTerm.frequency.isNotBlank() && settingsState.minDepositTerm.frequencyAfterInMultiplesOf.isNotBlank() && settingsState.maxDepositTerm.frequency.isNotBlank() + val penalFieldsValid = !settingsState.preMatureClosure.applyPenalInterest || + (settingsState.preMatureClosure.penalInterest.isNotBlank() && + settingsState.preMatureClosure.minimumBalanceForInterestCalculation.isNotBlank()) + val isNextButtonActive = baseFieldsValid && penalFieldsValid
129-135: Rename callback parameter fromidtoindexfor clarity.The
onOptionSelectedcallback receives the 0-based list index, not an ID. Rename the parameter toindexto match the pattern used throughout the codebase and clarify the semantics.Apply to all occurrences:
-onOptionSelected = { id, name -> - onAction( - RecurringAccountAction.RecurringAccountSettingsAction.SetLockInPeriodType(id), - ) -}, +onOptionSelected = { index, name -> + onAction( + RecurringAccountAction.RecurringAccountSettingsAction.SetLockInPeriodType(index), + ) +},Apply the same pattern to lines 172-176, 210-215, 239-244, 268-273, and 311-316.
Also applies to: 172-176, 210-215, 239-244, 268-273, 311-316
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountViewModel.kt (5)
40-43: Rename to one‑shot connectivity check; remove test‑only code and gate by productId.Current
observeNetwork()is one‑shot and still contains test comments and unconditional template load. Make intent clear, drop test artifacts, and only load when a valid product is selected.- init { - observeNetwork() - } + init { + checkInitialConnectivity() + } @@ - private fun observeNetwork() { + private fun checkInitialConnectivity() { viewModelScope.launch { val isConnected = networkMonitor.isOnline.first() mutableStateFlow.update { it.copy(isOnline = isConnected) } - if (isConnected) { -// loadTemplate() - // TODO - // Used only for testing purpose - // Must be removed from here after the implementation of detail screen is finished. - loadTemplateByProduct() - } else { + if (isConnected) { + val pid = state.recurringDepositAccountDetail.productId + if (pid > 0) loadTemplateByProduct() + } else { setErrorState( getString(Res.string.no_internet_connection), ) } } } @@ - observeNetwork() + checkInitialConnectivity()Also applies to: 60-78, 86-87
101-122: Add connectivity guard before repository calls.Both template loaders call the repo without checking connectivity; align with earlier feedback and guard with a one‑shot check.
private fun loadTemplate() { viewModelScope.launch { + val online = networkMonitor.isOnline.first() + if (!online) { + setErrorState(getString(Res.string.no_internet_connection)) + return@launch + } recurringAccountRepository.getRecurringAccountTemplate().collect { templateState ->
123-147: Same here: guardloadTemplateByProduct()with network check.private fun loadTemplateByProduct() { viewModelScope.launch { + val online = networkMonitor.isOnline.first() + if (!online) { + setErrorState(getString(Res.string.no_internet_connection)) + return@launch + } recurringAccountRepository.getRecurringAccountTemplateByProduct( clientId = clientId, productId = state.recurringDepositAccountDetail.productId,
534-541: SetproductIddefault to -1 and remove test-only comment.Using
6risks accidental real API calls on init.- // TODO - // productId is set 6 only for testing. - // It should be set -1 when the implementation of detail screen is finished. - val productId: Int = 6, + val productId: Int = -1,
193-256: Remove redundantstate.isOnlinecheck; validate required fields before submit.You already computed
onlineand returned if offline. The second check at Line 238 is redundant and may skip valid submissions ifstate.isOnlineis stale. Also, ensure required numeric fields are non‑null before building/calling the API.val lockinFreq = settings.lockInPeriod.frequency.toIntOrNull() val depositAmountInt = settings.recurringDepositDetails.depositAmount .filter(Char::isDigit) .toIntOrNull() val recurringFreq = settings.minDepositTerm.frequency.toIntOrNull() + // Validate required fields + if (depositAmountInt == null) { + setErrorState("Deposit amount is required") + return@launch + } + if (lockinFreq == null) { + setErrorState("Lock-in period frequency is required") + return@launch + } + if (recurringFreq == null) { + setErrorState("Recurring frequency is required") + return@launch + } + - val payload = RecurringDepositAccountPayload( + val payload = RecurringDepositAccountPayload( adjustAdvanceTowardsFuturePayments = settings.adjustAdvancePayments, @@ - if (state.isOnline) { - recurringAccountRepository.createRecurringDepositAccount(payload).collect { dataState -> + recurringAccountRepository.createRecurringDepositAccount(payload).collect { dataState -> when (dataState) { @@ - } - } + } + }
🧹 Nitpick comments (3)
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/SettingsPage.kt (1)
332-339: Consider moving validation logic to ViewModel.The validation logic is currently in the composable. Per the past review suggestion, consider extracting this to the ViewModel and exposing it as a state property (e.g.,
isNextEnabled).In the ViewModel:
val isNextEnabled = settingsState.let { val baseFieldsValid = /* ... */ val penalFieldsValid = /* ... */ baseFieldsValid && penalFieldsValid }Then in the composable:
isSecondButtonEnabled = state.isNextEnabledfeature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountViewModel.kt (2)
149-190: HardenformattedAmount: handle multiple '-' and '.'; base logic on sanitized input.Current logic can produce
"--12"or mis-handle inputs like"12..3"or"₹-.5"due to using rawamountfor checks and allowing extra '-' in the magnitude.- private fun formattedAmount(amount: String): String { - - val currencySymbol = state.recurringDepositAccountTemplate.currency?.displaySymbol ?: "" - if (amount.isEmpty()) return "" - - val cleaned = amount.replace("[^0-9.-]".toRegex(), "") - - if (cleaned.isEmpty()) return "" - if (cleaned == "-") return "-" - if (cleaned == ".") return "." // Allows user to type just '.' - - val negative = cleaned.startsWith("-") - val magnitude = if (negative) cleaned.substring(1) else cleaned - - val parts = magnitude.split('.', limit = 2) - var intPart = parts[0].trimStart('0').ifEmpty { "0" } // Clean up leading zeros - - var decimalPart = "" - if (parts.size > 1) { - decimalPart = parts[1].take(2) - if (decimalPart.isNotEmpty() || parts[1].isEmpty()) { - decimalPart = ".$decimalPart" - } - } - - if (intPart == "0" && magnitude.startsWith('.') && decimalPart.isNotEmpty()) { - intPart = "" - } - - val numericPart = when { - amount.startsWith('.') && decimalPart.isNotEmpty() -> decimalPart - amount.startsWith("-.") && decimalPart.isNotEmpty() -> decimalPart - else -> "$intPart$decimalPart" - } - - val signedNumericPart = if (negative && numericPart.isNotEmpty()) "-$numericPart" else numericPart - - if (amount == "." && signedNumericPart.isEmpty()) return "." - - return "$currencySymbol$signedNumericPart" - } + private fun formattedAmount(input: String): String { + val symbol = state.recurringDepositAccountTemplate.currency?.displaySymbol.orEmpty() + if (input.isEmpty()) return "" + val trimmed = input.trim() + if (trimmed == ".") return ".$symbol".replace(symbol, "") // allow single '.' + if (trimmed == "-." || trimmed == "-") return trimmed.replace("-.", "-.") // allow typing stage + + val negative = trimmed.startsWith("-") + // Keep only digits and at most one dot; drop any extra '-' beyond leading sign + val digitsAndDot = buildString { + var dotSeen = false + for (ch in trimmed) { + if (ch.isDigit()) append(ch) + else if (ch == '.' && !dotSeen) { append('.'); dotSeen = true } + } + } + if (digitsAndDot.isEmpty()) return if (negative) "-" else "" + + val parts = digitsAndDot.split('.', limit = 2) + val intPart = parts[0].trimStart('0').ifEmpty { "0" } + val frac = if (parts.size > 1) parts[1].take(2) else "" + val number = if (frac.isNotEmpty()) "$intPart.$frac" else intPart + + val signed = if (negative && number != "0") "-$number" else number + return "$symbol$signed" + }
505-514: Gate template load on valid product selection.Avoid calling
loadTemplateByProduct()with invalid sentinel values.mutableStateFlow.update { it.copy( recurringDepositAccountDetail = it.recurringDepositAccountDetail.copy( productId = action.productId, ), ) } - loadTemplateByProduct() + if (action.productId > 0) { + loadTemplateByProduct() + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
cmp-navigation/build.gradle.kts(1 hunks)cmp-navigation/src/commonMain/kotlin/cmp/navigation/di/KoinModules.kt(2 hunks)core/data/src/commonMain/kotlin/com/mifos/core/data/repository/RecurringAccountRepository.kt(1 hunks)core/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/RecurringAccountRepositoryImp.kt(1 hunks)core/model/src/commonMain/kotlin/com/mifos/core/model/objects/payloads/RecurringDepositAccountPayload.kt(1 hunks)core/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerRecurringAccount.kt(0 hunks)core/network/src/commonMain/kotlin/com/mifos/core/network/di/DataMangerModule.kt(2 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientApplyNewApplications/ClientApplyNewApplicationRoute.kt(1 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientApplyNewApplications/ClientApplyNewApplicationsScreen.kt(2 hunks)feature/recurringDeposit/src/commonMain/composeResources/values/string.xml(1 hunks)feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/di/RecurringDepositModule.kt(1 hunks)feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountRoute.kt(2 hunks)feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountScreen.kt(6 hunks)feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountViewModel.kt(4 hunks)feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/SettingsPage.kt(1 hunks)
💤 Files with no reviewable changes (1)
- core/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerRecurringAccount.kt
🔇 Additional comments (12)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientApplyNewApplications/ClientApplyNewApplicationsScreen.kt (2)
65-65: LGTM!The parameter signature change is consistent with the route definition and properly typed.
78-80: No changes required. The code correctly implements the intended design.The review comment contains inaccurate observations:
"Pattern inconsistency" is actually intentional design: The callback function signatures differ by requirement. Lines 77 and 81 use
onNavigateApplyFixedAccount()andonNavigateApplyShareAccount()which are typed as() -> Unit, while lines 78-80 use callbacks typed as(Int) -> Unit. The implementation correctly matches these contracts—this is proper API design, not an inconsistency."Missing validation" concern is invalid: The
clientIdfield is defined as non-nullableInt(notInt?) inClientApplyNewApplicationsState. It's initialized from route parameters in the ViewModel and will always contain a valid integer. Additional runtime validation would be redundant.Likely an incorrect or invalid review comment.
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientApplyNewApplications/ClientApplyNewApplicationRoute.kt (1)
28-28: Breaking change is correctly implemented across all call sites.The signature change from
() -> Unitto(Int) -> Unithas been properly propagated:
- Route parameter updated (ClientApplyNewApplicationRoute.kt:28) ✓
- Screen parameter updated (ClientApplyNewApplicationsScreen.kt:65) ✓
- Screen implementation calls with clientId (ClientApplyNewApplicationsScreen.kt:79) ✓
- Navigation binding passes correct function reference (ClientNavigation.kt:315) ✓
- Target function accepts Int parameter (RecurringAccountRoute.kt:31) ✓
The change is consistent with similar account navigation patterns in the codebase.
cmp-navigation/build.gradle.kts (1)
49-49: LGTM!The recurring deposit feature module dependency is correctly added and consistent with other feature module dependencies in this file.
core/network/src/commonMain/kotlin/com/mifos/core/network/di/DataMangerModule.kt (1)
27-27: LGTM!The DI binding for
DataManagerRecurringAccountfollows the established pattern for other data managers in this module.Also applies to: 56-56
core/data/src/commonMain/kotlin/com/mifos/core/data/repository/RecurringAccountRepository.kt (1)
20-25: LGTM!The method renames correctly fix the typo from "Recutting" to "Recurring", addressing the past review feedback.
core/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/RecurringAccountRepositoryImp.kt (1)
24-37: LGTM!The method renames correctly implement the interface changes, fixing the "Recutting" typo and maintaining proper Flow-based data state handling.
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountScreen.kt (3)
44-44: LGTM!The switch to
koinViewModel()correctly wires the ViewModel through the DI framework, aligning with the newRecurringDepositModulesetup.
104-116: LGTM!The refactor correctly addresses the past review feedback by using
screenStateinstead ofdialogState, and the newRecurringDepositAccountDialogBoxcomposable properly handles loading and error states.Also applies to: 119-138
83-84: Update call site after SettingsPage rename.Once
SettingPageis renamed toSettingsPagein SettingsPage.kt, update the call here and the import on line 34.-import com.mifos.feature.recurringDeposit.newRecurringDepositAccount.pages.SettingPage +import com.mifos.feature.recurringDeposit.newRecurringDepositAccount.pages.SettingsPage- SettingPage( + SettingsPage( state = state, onAction = onAction, )cmp-navigation/src/commonMain/kotlin/cmp/navigation/di/KoinModules.kt (1)
34-34: DI wiring looks correct.
RecurringDepositModuleis properly imported and included with other feature modules.Also applies to: 87-87
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/di/RecurringDepositModule.kt (1)
16-18: LGTM.Module definition is minimal and correct.
| <string name="is_mandatory_deposit">Is Mandatory Deposit?</string> | ||
| <string name="adjust_advance_payments">Adjust advance payments toward future installments?</string> | ||
| <string name="allow_withdrawals">Allow Withdrawals?</string> | ||
| <string name="lock_in_period">Lock-in Period</string> | ||
| <string name="frequency">Frequency</string> | ||
| <string name="type">Type</string> | ||
| <string name="recurring_deposit_details">Recurring Deposit Details</string> | ||
| <string name="recurring_deposit_amount">Recurring Deposit Amount</string> | ||
| <string name="deposit_period">Deposit Period</string> | ||
| <string name="deposit_frequency_same_as_meeting">Deposit Frequency Same as Group/Center meeting</string> | ||
| <string name="minimum_deposit_term">Minimum Deposit Term</string> | ||
| <string name="in_multiples_of">And thereafter, in Multiples of</string> | ||
| <string name="maximum_deposit_term">Maximum Deposit Term</string> | ||
| <string name="for_pre_mature_closure">For Pre-mature closure</string> | ||
| <string name="apply_penal_interest">Apply Penal Interest (less)</string> | ||
| <string name="penal_interest_percentage">Penal Interest (%)</string> | ||
| <string name="period">Period</string> | ||
| <string name="minimum_balance_for_interest">Minimum Balance For Interest Calculation</string> | ||
| <string name="back">Back</string> | ||
| <string name="next">Next</string> | ||
| <string name="no_internet_connection">No Internet Connection</string> |
There was a problem hiding this comment.
Polish copy for clarity and consistency; avoid question-style labels.
Several labels use title-case/questions and have minor grammar issues. Recommend sentence‑case, remove “?” where not interactive questions, and clearer phrasing.
Apply this diff to update only string values (no key renames):
- <string name="is_mandatory_deposit">Is Mandatory Deposit?</string>
+ <string name="is_mandatory_deposit">Mandatory deposit</string>
- <string name="adjust_advance_payments">Adjust advance payments toward future installments?</string>
+ <string name="adjust_advance_payments">Adjust advance payments toward future installments</string>
- <string name="allow_withdrawals">Allow Withdrawals?</string>
+ <string name="allow_withdrawals">Allow withdrawals</string>
<string name="lock_in_period">Lock-in Period</string>
<string name="frequency">Frequency</string>
<string name="type">Type</string>
<string name="recurring_deposit_details">Recurring Deposit Details</string>
<string name="recurring_deposit_amount">Recurring Deposit Amount</string>
<string name="deposit_period">Deposit Period</string>
- <string name="deposit_frequency_same_as_meeting">Deposit Frequency Same as Group/Center meeting</string>
+ <string name="deposit_frequency_same_as_meeting">Deposit frequency same as group/center meeting</string>
<string name="minimum_deposit_term">Minimum Deposit Term</string>
- <string name="in_multiples_of">And thereafter, in Multiples of</string>
+ <string name="in_multiples_of">And thereafter in multiples of</string>
<string name="maximum_deposit_term">Maximum Deposit Term</string>
- <string name="for_pre_mature_closure">For Pre-mature closure</string>
+ <string name="for_pre_mature_closure">For premature closure</string>
- <string name="apply_penal_interest">Apply Penal Interest (less)</string>
+ <string name="apply_penal_interest">Apply penal interest</string>
- <string name="penal_interest_percentage">Penal Interest (%)</string>
+ <string name="penal_interest_percentage">Penal interest (%)</string>
<string name="period">Period</string>
- <string name="minimum_balance_for_interest">Minimum Balance For Interest Calculation</string>
+ <string name="minimum_balance_for_interest">Minimum balance for interest calculation</string>
<string name="back">Back</string>
<string name="next">Next</string>
- <string name="no_internet_connection">No Internet Connection</string>
+ <string name="no_internet_connection">No internet connection</string>Optional: “frequency” and “type” are very generic keys; consider namespacing (e.g., recurring_frequency_label, recurring_type_label) to avoid collisions across modules.
🤖 Prompt for AI Agents
In feature/recurringDeposit/src/commonMain/composeResources/values/string.xml
around lines 18-38, several string values use question punctuation, title-case,
or awkward phrasing; update only the string values (do not rename keys) to use
sentence-case, remove trailing question marks for labels, and clarify wording
(e.g., change "Is Mandatory Deposit?" → "Mandatory deposit", "Adjust advance
payments toward future installments?" → "Adjust advance payments toward future
installments", "Allow Withdrawals?" → "Allow withdrawals", "Lock-in Period" →
"Lock-in period", "Type" → "Type" or a clearer label like 'Recurring type',
"Frequency" → "Frequency" or 'Recurring frequency', "For Pre-mature closure" →
"For premature closure", "Apply Penal Interest (less)" → "Apply penal interest
(less)", "Penal Interest (%)" → "Penal interest (%)", "Minimum Balance For
Interest Calculation" → "Minimum balance for interest calculation"); optionally
consider namespacing ambiguous keys like frequency/type in a separate follow-up
if needed.
| data class RecurringAccountRoute( | ||
| val clientId: Int = -1, | ||
| ) |
There was a problem hiding this comment.
Validate or remove the default clientId value.
The default value of -1 suggests an invalid/unset state. Consider either:
- Removing the default to force callers to provide a valid clientId
- Adding validation to ensure clientId > 0 before proceeding
-data class RecurringAccountRoute(
- val clientId: Int = -1,
-)
+data class RecurringAccountRoute(
+ val clientId: Int,
+)📝 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 RecurringAccountRoute( | |
| val clientId: Int = -1, | |
| ) | |
| data class RecurringAccountRoute( | |
| val clientId: Int, | |
| ) |
🤖 Prompt for AI Agents
In
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountRoute.kt
around lines 18 to 20, the data class uses a default clientId = -1 which
represents an invalid/unset state; either remove the default so callers must
supply a clientId, or keep the field but add validation where instances are
created/used to enforce clientId > 0 and throw or return a clear error when
invalid; implement one of these two options consistently across
constructors/factory functions and any deserialization paths.
| import androidclient.feature.recurringdeposit.generated.resources.Res | ||
| import androidclient.feature.recurringdeposit.generated.resources.no_internet_connection | ||
| import androidx.lifecycle.SavedStateHandle | ||
| import androidx.lifecycle.viewModelScope | ||
| import androidx.navigation.toRoute |
There was a problem hiding this comment.
Remove unused import.
no_internet_connection is not used directly (you reference Res.string.no_internet_connection). Drop the symbol import.
-import androidclient.feature.recurringdeposit.generated.resources.no_internet_connection📝 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.
| import androidclient.feature.recurringdeposit.generated.resources.Res | |
| import androidclient.feature.recurringdeposit.generated.resources.no_internet_connection | |
| import androidx.lifecycle.SavedStateHandle | |
| import androidx.lifecycle.viewModelScope | |
| import androidx.navigation.toRoute | |
| import androidclient.feature.recurringdeposit.generated.resources.Res | |
| import androidx.lifecycle.SavedStateHandle | |
| import androidx.lifecycle.viewModelScope | |
| import androidx.navigation.toRoute |
🤖 Prompt for AI Agents
In
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountViewModel.kt
around lines 12 to 16, remove the unused symbol import `no_internet_connection`
(currently imported from
androidclient.feature.recurringdeposit.generated.resources) since the code
references Res.string.no_internet_connection instead; delete that specific
import line to avoid an unused import warning and keep only the necessary
imports.
✅ Actions performedReviews paused. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
…ks in RecurringAccountViewModel
…agerModule and RecurringAccountViewModel
|
|
||
| @Serializable | ||
| data class RecurringDepositAccountPayload( | ||
| // val charges: List<Any>? = null, |
There was a problem hiding this comment.
Can you tell me why this has been commented out?
There was a problem hiding this comment.
I couldn't find a representation of that parameter in the api call.
It sends an empty array or object.
I also couldn't find its use.
So, in future, if someone does find its use or the proper representation of the data type, they can update the code.
There was a problem hiding this comment.
Please follow the naming convention for a string resource used in other feature modules. The name should be prefixed with the name of the feature. Check other feature modules where it has been implemented appropriately. This avoids wrong usage of strings elsewhere.
There was a problem hiding this comment.
I fixed this issue now.
Actually, I had AI extract hardcoded strings for me from the files 😅 so, I did not notice the naming convention was not followed.
4 participants
Jira-#MIFOSAC-552
Record_2025-10-28-19-35-55_026f621985d6cfa1a169c52d305dc178.mp4
Summary by CodeRabbit
New Features
Bug Fixes