New fixed deposits account preview step #2560
Conversation
WalkthroughThis PR extends fixed deposit account creation functionality across network, data, and feature layers. It introduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Fixed Deposit Screen
participant VM as CreateFixedDepositAccountViewmodel
participant Repo as FixedDepositRepository
participant DM as DataManagerFixedDeposit
participant Service as FixedDepositService
participant API as Mifos API
UI->>VM: OnSubmitFixedAccount()
VM->>VM: createFixedDepositAccount()
VM->>Repo: createFixedDepositAccount(payload)
Repo->>DM: createFixedDepositAccount(payload)
DM->>Service: createFixedDepositAccount(payload)
Service->>API: POST /fixeddeposits (payload)
alt Success
API-->>Service: HttpResponse(success)
Service-->>DM: Flow<HttpResponse>
DM->>DM: map & validate isSuccess
DM-->>Repo: Flow<Unit>
Repo->>Repo: asDataStateFlow()
Repo-->>VM: Flow<DataState<Unit>>
VM->>VM: emit loading, then Success
VM->>UI: dialogState = SuccessResponseStatus
UI->>UI: Show snackbar
UI->>UI: Navigate back after delay
else Error
API-->>Service: HttpResponse(error)
Service-->>DM: Flow<HttpResponse>
DM->>DM: extractErrorMessage, throw IllegalStateException
DM-->>Repo: Exception
Repo->>Repo: asDataStateFlow() catches
Repo-->>VM: Flow<DataState.Error>
VM->>UI: dialogState = updated error state
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/PreviewPage.kt (1)
293-300: Duplicate map key will cause data loss.
Res.string.feature_recurring_deposit_periodis used twice in this map (lines 293 and 300). In Kotlin maps, duplicate keys cause the second value to overwrite the first, so the pre-closure penal interest period type will never be displayed.Consider using a distinct string resource for the minimum balance field:
- Res.string.feature_recurring_deposit_period to state.template.currency?.displaySymbol.orEmpty() + " " + state.recurringDepositAccountSettings.preMatureClosure.minimumBalanceForInterestCalculation, + Res.string.feature_recurring_deposit_minimum_balance_for_interest to state.template.currency?.displaySymbol.orEmpty() + " " + state.recurringDepositAccountSettings.preMatureClosure.minimumBalanceForInterestCalculation,You'll need to add the corresponding string resource.
🧹 Nitpick comments (2)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/pages/PreviewPage.kt (1)
305-310: Logic appears inverted for rate chart display.The naming
isRateChartEmptyis confusing because whenchartSlabsis NOT null/empty,isRateChartEmptybecomestrue. This causes:
- Text shows "Interest Rate Chart" when chart exists (correct text, but variable name is misleading)
- Button enabled when chart exists
The variable is actually checking if the chart is not empty. Consider renaming to
hasRateChartorisRateChartAvailablefor clarity.feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/CreateFixedDepositAccountViewmodel.kt (1)
921-927: Inconsistent parameter naming in action classes.The parameter naming is confusing:
OnEditCharge(chargeIndex)-chargeIndexis actually the position inaddedChargeslistEditChargeDialog(index)- correctly usesindexDeleteChargeFromSelectedCharges(index)- correctly usesindexConsider renaming for consistency:
- data class OnEditCharge(val chargeIndex: Int) : NewFixedDepositAccountAction() + data class OnEditCharge(val index: Int) : NewFixedDepositAccountAction()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
core/data/src/commonMain/kotlin/com/mifos/core/data/repository/FixedDepositRepository.kt(1 hunks)core/data/src/commonMain/kotlin/com/mifos/core/data/repository/ShareAccountRepository.kt(1 hunks)core/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/FixedDepositRepositoryImpl.kt(2 hunks)core/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/ShareAccountRepositoryImpl.kt(1 hunks)core/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerFixedDeposit.kt(1 hunks)core/network/src/commonMain/kotlin/com/mifos/core/network/model/fixedDeposit/FixedDepositPayload.kt(1 hunks)core/network/src/commonMain/kotlin/com/mifos/core/network/model/fixedDeposit/FixedDepositProductOption.kt(1 hunks)core/network/src/commonMain/kotlin/com/mifos/core/network/model/fixedDeposit/FixedDepositTemplate.kt(3 hunks)core/network/src/commonMain/kotlin/com/mifos/core/network/model/share/ShareAccountPayload.kt(1 hunks)core/network/src/commonMain/kotlin/com/mifos/core/network/services/FixedDepositService.kt(2 hunks)feature/client/src/commonMain/composeResources/values/strings.xml(3 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/CreateShareAccountViewModel.kt(1 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/CreateFixedDepositAccountRoute.kt(0 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/CreateFixedDepositAccountScreen.kt(6 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/CreateFixedDepositAccountViewmodel.kt(15 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/pages/ChargesPage.kt(6 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/pages/PreviewPage.kt(1 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/pages/SettingsPage.kt(4 hunks)feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/PreviewPage.kt(2 hunks)
💤 Files with no reviewable changes (1)
- feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/CreateFixedDepositAccountRoute.kt
🧰 Additional context used
🧬 Code graph analysis (4)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/pages/SettingsPage.kt (1)
core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/component/MifosTextFieldDropdown.kt (1)
MifosTextFieldDropdown(39-112)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/pages/PreviewPage.kt (4)
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/PreviewPage.kt (3)
DetailsCard(347-370)TermsCard(306-345)SettingCard(195-304)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)core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosListingComponent.kt (1)
MifosDefaultListingComponentFromStringResources(244-267)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/pages/ChargesPage.kt (3)
core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/AddChargeDialog.kt (1)
AddChargeBottomSheet(47-171)core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/component/MifosBottomSheet.kt (1)
MifosBottomSheet(47-89)core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosActionsListingCardComponent.kt (1)
MifosActionsChargeListingComponent(601-735)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/CreateFixedDepositAccountViewmodel.kt (4)
core/data/src/commonMain/kotlin/com/mifos/core/data/repository/FixedDepositRepository.kt (1)
createFixedDepositAccount(23-25)core/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/FixedDepositRepositoryImpl.kt (1)
createFixedDepositAccount(31-33)core/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerFixedDeposit.kt (1)
createFixedDepositAccount(25-32)feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountv2/SavingsAccountViewModel.kt (1)
isOnline(58-70)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: PR Checks / Static Analysis Check
🔇 Additional comments (16)
core/network/src/commonMain/kotlin/com/mifos/core/network/model/fixedDeposit/FixedDepositProductOption.kt (1)
10-10: LGTM!Package reorganization to
com.mifos.core.network.model.fixedDepositis consistent with the broader refactor consolidating fixed deposit models.core/data/src/commonMain/kotlin/com/mifos/core/data/repository/ShareAccountRepository.kt (1)
21-21: LGTM!Removing
suspendfrom a function returningFlowis correct. The Flow itself handles asynchronous execution, and suspension occurs when collecting. This also maintains consistency withgetShareTemplate.feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/PreviewPage.kt (1)
183-190: Verify the inverted boolean logic for rate chart display.The logic appears counterintuitive: when
isRateChartEmptyistrue, the button is enabled and shows "interest rate chart" text. When it'sfalse, the button is disabled with "no interest chart" text.Semantically, this seems backwards—you'd expect to disable the "View" button when the chart is empty, not when it has data.
Please verify if
isRateChartEmptyshould be renamed tohasRateChartor if the boolean conditions should be inverted.core/network/src/commonMain/kotlin/com/mifos/core/network/model/fixedDeposit/FixedDepositTemplate.kt (1)
10-10: LGTM!Package reorganization aligns with the fixed deposit model consolidation. The new
savingsAccountsfield is backward-compatible (nullable with default) and supports the savings account linking feature in the preview step.Also applies to: 23-23, 65-65
core/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/ShareAccountRepositoryImpl.kt (1)
30-36: LGTM!Implementation correctly aligns with the interface change. The
flow { emit(...) }.asDataStateFlow()pattern appropriately wraps the data manager call for reactive state handling.feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/CreateShareAccountViewModel.kt (1)
23-23: LGTM!Import migration to the shared
ChargeItemtype is consistent with the payload layer changes.core/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerFixedDeposit.kt (1)
25-32: LGTM!The error handling pattern using
extractErrorMessageandIllegalStateExceptionis consistent with how other data managers handle API errors. TheUnitreturn type is intentional—the ViewModel only needs success/failure confirmation, not response body data, as evidenced by the generic success message shown in the UI state.core/network/src/commonMain/kotlin/com/mifos/core/network/model/share/ShareAccountPayload.kt (1)
12-12: LGTM!Migrating
ChargeItemto a shared payloads package (com.mifos.core.model.objects.payloads.ChargeItem) promotes reuse across share and fixed deposit features. The sharedChargeItemmaintains the expected serialization structure withchargeIdandamountfields and is now consistently used across fixed deposit, recurring deposit, and share account modules.core/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/FixedDepositRepositoryImpl.kt (1)
30-33: LGTM!The new
createFixedDepositAccountmethod follows the established repository pattern, delegating to the data manager and converting the result viaasDataStateFlow(). This is consistent with the existinggetFixedDepositTemplateimplementation.core/data/src/commonMain/kotlin/com/mifos/core/data/repository/FixedDepositRepository.kt (1)
22-25: LGTM!The new
createFixedDepositAccountmethod is correctly defined in the repository interface. UsingFlow<DataState<Unit>>is appropriate for a create operation where only the success/failure status matters.core/network/src/commonMain/kotlin/com/mifos/core/network/services/FixedDepositService.kt (1)
29-33: LGTM!The new
createFixedDepositAccountAPI method is correctly defined with the appropriate@POSTannotation and@Bodyparameter. ReturningFlow<HttpResponse>allows the data manager layer to handle response parsing and error extraction flexibly.feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/CreateFixedDepositAccountScreen.kt (2)
110-121: VerifylaunchEffectKeymanagement prevents duplicate effects.The success flow correctly shows a snackbar and navigates back after a delay. However, ensure that
state.launchEffectKeyis properly updated each time a new success response occurs to prevent theLaunchedEffectfrom being skipped on subsequent successes with the same key.
165-170: LGTM!The PreviewPage is correctly integrated as the final step in the fixed deposit account creation flow, following the same pattern as other steps.
core/network/src/commonMain/kotlin/com/mifos/core/network/model/fixedDeposit/FixedDepositPayload.kt (1)
15-51: LGTM!The
FixedDepositPayloaddata class is well-structured with appropriate nullable fields and sensible defaults. The use of@Serializableannotation and default values for all fields provides good flexibility for partial payloads.feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/pages/ChargesPage.kt (2)
137-209: LGTM!The
AddNewChargeDialogcomposable properly implements the state-driven dialog pattern. The empty lambdas foronDatePickandonDateChangeare appropriate since fixed deposits don't require date selection for charges.
211-284: LGTM!The
ShowChargesDialogproperly usesfirstOrNullfor safe charge lookup by ID and handles the edit/delete actions appropriately through the state-driven action pattern.
| <string name="feature_fixed_deposit_interest_yes">Yes</string> | ||
| <string name="feature_fixed_deposit_interest_no">No</string> | ||
| <string name="feature_fixed_deposit_interest_empty_date">Date Not Found</string> | ||
| <string name="feature_fixed_account_created_successfully">Share account created successfully</string> |
There was a problem hiding this comment.
Incorrect success message text.
The string says "Share account created successfully" but this resource is for fixed deposit account creation (feature_fixed_account_created_successfully). This appears to be a copy-paste error from share account strings.
- <string name="feature_fixed_account_created_successfully">Share account created successfully</string>
+ <string name="feature_fixed_account_created_successfully">Fixed deposit account created successfully</string>📝 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.
| <string name="feature_fixed_account_created_successfully">Share account created successfully</string> | |
| <string name="feature_fixed_account_created_successfully">Fixed deposit account created successfully</string> |
🤖 Prompt for AI Agents
In feature/client/src/commonMain/composeResources/values/strings.xml around line
636, the string resource key feature_fixed_account_created_successfully has the
wrong copy-pasted value "Share account created successfully"; update the value
to the correct fixed deposit account success text (e.g., "Fixed deposit account
created successfully" or the product-approved phrasing) so the resource matches
the key and UI context.
| is NewFixedDepositAccountAction.NewFixedDepositAccountChargesAction.OnEditCharge -> { | ||
| val createdData = ChargeItem( | ||
| chargeId = state.template.chargeOptions?.get(action.chargeIndex)?.id, | ||
| amount = state.fixedDepositAccountCharges.chargeAmount.toDoubleOrNull(), | ||
| ) | ||
|
|
||
| val currentAddedCharges = | ||
| state.fixedDepositAccountCharges.addedCharges.toMutableList() | ||
| currentAddedCharges[action.chargeIndex] = createdData | ||
| mutableStateFlow.update { | ||
| it.copy( | ||
| fixedDepositAccountCharges = it.fixedDepositAccountCharges.copy( | ||
| addedCharges = currentAddedCharges, | ||
| selectedChargeIndex = null, | ||
| chargeAmount = "", | ||
| ), | ||
| dialogState = NewFixedDepositAccountState.DialogState.ShowCharges, | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
Bug: Using wrong index to update charge list.
The OnEditCharge handler receives chargeIndex which represents the position in addedCharges list (passed from the dialog). However, line 326 incorrectly uses this index to look up the charge in template.chargeOptions. The correct behavior should use selectedChargeIndex (set during EditChargeDialog) for the template lookup.
is NewFixedDepositAccountAction.NewFixedDepositAccountChargesAction.OnEditCharge -> {
val createdData = ChargeItem(
- chargeId = state.template.chargeOptions?.get(action.chargeIndex)?.id,
+ chargeId = state.template.chargeOptions?.getOrNull(state.fixedDepositAccountCharges.selectedChargeIndex ?: -1)?.id,
amount = state.fixedDepositAccountCharges.chargeAmount.toDoubleOrNull(),
)
val currentAddedCharges =
state.fixedDepositAccountCharges.addedCharges.toMutableList()
- currentAddedCharges[action.chargeIndex] = createdData
+ currentAddedCharges[action.index] = createdDataNote: The action class at line 923 uses chargeIndex but should be renamed to index for consistency with how it's used.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/CreateFixedDepositAccountViewmodel.kt
around lines 324 to 343, the handler for
NewFixedDepositAccountAction.NewFixedDepositAccountChargesAction.OnEditCharge is
using action.chargeIndex to pick the template chargeOption but chargeIndex
refers to the position in addedCharges; change the template lookup to use the
viewmodel state's selectedChargeIndex (or the selectedChargeIndex value saved
when opening the EditChargeDialog) so the correct template id is used when
building createdData, and after that update addedCharges as before; also rename
the action property at ~line 923 from chargeIndex to index for
clarity/consistency with its actual meaning.
| NewFixedDepositAccountAction.NewFixedDepositAccountChargesAction.AddChargeToList -> { | ||
| val createdData = ChargeItem( | ||
| chargeId = state.template.chargeOptions?.get(state.fixedDepositAccountCharges.selectedChargeIndex!!)?.id, | ||
| amount = state.fixedDepositAccountCharges.chargeAmount.toDoubleOrNull(), | ||
| ) | ||
|
|
||
| NewFixedDepositAccountAction.OnDismissDialog -> { | ||
| mutableStateFlow.update { | ||
| it.copy( | ||
| fixedDepositAccountCharges = it.fixedDepositAccountCharges.copy( | ||
| addedCharges = it.fixedDepositAccountCharges.addedCharges + createdData, | ||
| chargeAmount = "", | ||
| selectedChargeIndex = null, | ||
| ), | ||
| dialogState = null, | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
Potential NullPointerException with force unwrap.
The selectedChargeIndex!! force unwrap on line 363 will crash if selectedChargeIndex is null. This could happen if the user somehow triggers the add action without selecting a charge.
- is NewFixedDepositAccountAction.NewFixedDepositAccountChargesAction.AddChargeToList -> {
- val createdData = ChargeItem(
- chargeId = state.template.chargeOptions?.get(state.fixedDepositAccountCharges.selectedChargeIndex!!)?.id,
- amount = state.fixedDepositAccountCharges.chargeAmount.toDoubleOrNull(),
- )
+ is NewFixedDepositAccountAction.NewFixedDepositAccountChargesAction.AddChargeToList -> {
+ val selectedIndex = state.fixedDepositAccountCharges.selectedChargeIndex ?: return
+ val createdData = ChargeItem(
+ chargeId = state.template.chargeOptions?.getOrNull(selectedIndex)?.id,
+ amount = state.fixedDepositAccountCharges.chargeAmount.toDoubleOrNull(),
+ )📝 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.
| NewFixedDepositAccountAction.NewFixedDepositAccountChargesAction.AddChargeToList -> { | |
| val createdData = ChargeItem( | |
| chargeId = state.template.chargeOptions?.get(state.fixedDepositAccountCharges.selectedChargeIndex!!)?.id, | |
| amount = state.fixedDepositAccountCharges.chargeAmount.toDoubleOrNull(), | |
| ) | |
| NewFixedDepositAccountAction.OnDismissDialog -> { | |
| mutableStateFlow.update { | |
| it.copy( | |
| fixedDepositAccountCharges = it.fixedDepositAccountCharges.copy( | |
| addedCharges = it.fixedDepositAccountCharges.addedCharges + createdData, | |
| chargeAmount = "", | |
| selectedChargeIndex = null, | |
| ), | |
| dialogState = null, | |
| ) | |
| } | |
| } | |
| NewFixedDepositAccountAction.NewFixedDepositAccountChargesAction.AddChargeToList -> { | |
| val selectedIndex = state.fixedDepositAccountCharges.selectedChargeIndex ?: return | |
| val createdData = ChargeItem( | |
| chargeId = state.template.chargeOptions?.getOrNull(selectedIndex)?.id, | |
| amount = state.fixedDepositAccountCharges.chargeAmount.toDoubleOrNull(), | |
| ) | |
| mutableStateFlow.update { | |
| it.copy( | |
| fixedDepositAccountCharges = it.fixedDepositAccountCharges.copy( | |
| addedCharges = it.fixedDepositAccountCharges.addedCharges + createdData, | |
| chargeAmount = "", | |
| selectedChargeIndex = null, | |
| ), | |
| dialogState = null, | |
| ) | |
| } | |
| } |
🤖 Prompt for AI Agents
In
feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/CreateFixedDepositAccountViewmodel.kt
around lines 361 to 377, the code force-unwraps selectedChargeIndex which can be
null and cause an NPE; change to safely handle a null index by using
selectedChargeIndex?.let { idx -> val chargeId =
state.template.chargeOptions?.getOrNull(idx)?.id ... } and only create and
append ChargeItem when idx is non-null (otherwise ignore the action or surface a
validation error), and ensure amount conversion uses toDoubleOrNull() as already
done so null amounts are handled consistently.
| depositAmount = state.fixedDepositAccountTerms.depositAmount.toDouble(), | ||
| depositPeriod = state.fixedDepositAccountTerms.depositPeriod.toInt(), |
There was a problem hiding this comment.
Potential NumberFormatException on empty/invalid input.
toDouble() and toInt() will throw NumberFormatException if the strings are empty or contain non-numeric values. While validation should catch this earlier, defensive coding is safer here.
- depositAmount = state.fixedDepositAccountTerms.depositAmount.toDouble(),
- depositPeriod = state.fixedDepositAccountTerms.depositPeriod.toInt(),
+ depositAmount = state.fixedDepositAccountTerms.depositAmount.toDoubleOrNull() ?: 0.0,
+ depositPeriod = state.fixedDepositAccountTerms.depositPeriod.toIntOrNull() ?: 0,Alternatively, add validation before calling createFixedDepositAccount() to ensure these fields have valid values, and consider making the payload fields non-nullable with proper validation.
🤖 Prompt for AI Agents
In
feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/CreateFixedDepositAccountViewmodel.kt
around lines 442-443, the direct calls to toDouble() and toInt() can throw
NumberFormatException for empty or invalid strings; change to safe parsing
(e.g., toDoubleOrNull()/toIntOrNull()) and handle nulls by returning/propagating
a validation error or providing a sensible default before calling
createFixedDepositAccount(), or move validation earlier so these fields are
guaranteed non-null/valid and convert them only after validation so payload
fields can be non-nullable.
| data class ChargeData( | ||
| val id: Int, | ||
| val name: String, | ||
| val type: String, | ||
| val collectedOn: String, | ||
| val amount: Double, | ||
| val id: Int?, | ||
| val amount: Double?, | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove unused ChargeData class.
This class appears to be legacy code that has been replaced by ChargeItem. It's no longer referenced anywhere in the file.
-data class ChargeData(
- val id: Int?,
- val amount: Double?,
-)📝 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 ChargeData( | |
| val id: Int, | |
| val name: String, | |
| val type: String, | |
| val collectedOn: String, | |
| val amount: Double, | |
| val id: Int?, | |
| val amount: Double?, | |
| ) |
🤖 Prompt for AI Agents
In
feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/CreateFixedDepositAccountViewmodel.kt
around lines 858-861, remove the unused legacy data class ChargeData (val id:
Int?, val amount: Double?) since ChargeItem replaces it; delete the class
declaration and any now-unreferenced imports or references in this file to keep
code clean and compile without unused symbol warnings.
| Res.string.feature_fixed_interest_compounding_period to if (state.fixedDepositAccountTerms.interestPostingPeriodTypeIndex != -1) { | ||
| state.template.interestPostingPeriodTypeOptions?.getOrNull(state.fixedDepositAccountTerms.interestPostingPeriodTypeIndex)?.value.orEmpty() | ||
| } else { | ||
| "" | ||
| }, |
There was a problem hiding this comment.
Bug: Wrong index used for Interest Compounding Period.
The label feature_fixed_interest_compounding_period is mapped to interestPostingPeriodTypeIndex instead of interestCompoundingPeriodTypeIndex. This displays the wrong value.
- Res.string.feature_fixed_interest_compounding_period to if (state.fixedDepositAccountTerms.interestPostingPeriodTypeIndex != -1) {
- state.template.interestPostingPeriodTypeOptions?.getOrNull(state.fixedDepositAccountTerms.interestPostingPeriodTypeIndex)?.value.orEmpty()
+ Res.string.feature_fixed_interest_compounding_period to if (state.fixedDepositAccountTerms.interestCompoundingPeriodTypeIndex != -1) {
+ state.template.interestCompoundingPeriodTypeOptions?.getOrNull(state.fixedDepositAccountTerms.interestCompoundingPeriodTypeIndex)?.value.orEmpty()📝 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.
| Res.string.feature_fixed_interest_compounding_period to if (state.fixedDepositAccountTerms.interestPostingPeriodTypeIndex != -1) { | |
| state.template.interestPostingPeriodTypeOptions?.getOrNull(state.fixedDepositAccountTerms.interestPostingPeriodTypeIndex)?.value.orEmpty() | |
| } else { | |
| "" | |
| }, | |
| Res.string.feature_fixed_interest_compounding_period to if (state.fixedDepositAccountTerms.interestCompoundingPeriodTypeIndex != -1) { | |
| state.template.interestCompoundingPeriodTypeOptions?.getOrNull(state.fixedDepositAccountTerms.interestCompoundingPeriodTypeIndex)?.value.orEmpty() | |
| } else { | |
| "" | |
| }, |
🤖 Prompt for AI Agents
In
feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/pages/PreviewPage.kt
around lines 177 to 181, the mapping for
Res.string.feature_fixed_interest_compounding_period incorrectly uses
interestPostingPeriodTypeIndex and interestPostingPeriodTypeOptions; change it
to use state.fixedDepositAccountTerms.interestCompoundingPeriodTypeIndex and
state.template.interestCompoundingPeriodTypeOptions?.getOrNull(state.fixedDepositAccountTerms.interestCompoundingPeriodTypeIndex)?.value.orEmpty()
(with the same -1 check) so the compounding period label displays the correct
option.
| Res.string.feature_fixed_deposit_setting_lock_in_period to state.lockInPeriodFrequency + " " + if (state.lockInPeriodTypeIndex != -1) { | ||
| state.template.lockinPeriodFrequencyTypeOptions?.get(state.lockInPeriodTypeIndex)?.value.orEmpty() | ||
| } else { | ||
| "" | ||
| }, |
There was a problem hiding this comment.
Use getOrNull() for safer list access.
Using get() with an index can throw IndexOutOfBoundsException if the index is invalid or the list has changed. This is inconsistent with the pattern used in TermsCard.
- Res.string.feature_fixed_deposit_setting_lock_in_period to state.lockInPeriodFrequency + " " + if (state.lockInPeriodTypeIndex != -1) {
- state.template.lockinPeriodFrequencyTypeOptions?.get(state.lockInPeriodTypeIndex)?.value.orEmpty()
+ Res.string.feature_fixed_deposit_setting_lock_in_period to state.lockInPeriodFrequency + " " + if (state.lockInPeriodTypeIndex != -1) {
+ state.template.lockinPeriodFrequencyTypeOptions?.getOrNull(state.lockInPeriodTypeIndex)?.value.orEmpty()Apply similar changes to lines 212, 217, 222, 234, 239, 244, and 258 where get() is used.
📝 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.
| Res.string.feature_fixed_deposit_setting_lock_in_period to state.lockInPeriodFrequency + " " + if (state.lockInPeriodTypeIndex != -1) { | |
| state.template.lockinPeriodFrequencyTypeOptions?.get(state.lockInPeriodTypeIndex)?.value.orEmpty() | |
| } else { | |
| "" | |
| }, | |
| Res.string.feature_fixed_deposit_setting_lock_in_period to state.lockInPeriodFrequency + " " + if (state.lockInPeriodTypeIndex != -1) { | |
| state.template.lockinPeriodFrequencyTypeOptions?.getOrNull(state.lockInPeriodTypeIndex)?.value.orEmpty() | |
| } else { | |
| "" | |
| }, |
🤖 Prompt for AI Agents
In
feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/pages/PreviewPage.kt
around lines 206-210 (and similarly at 212, 217, 222, 234, 239, 244, 258), you
are using list.get(index) which can throw IndexOutOfBoundsException; replace
those get(...) calls with safe getOrNull(...) and null-safe access (e.g.,
getOrNull(index)?.value.orEmpty()) or fallback to an empty string when index is
invalid, following the same pattern used in TermsCard so the code won’t crash if
the list or index is out of sync.
| AnimatedVisibility(state.transferLinkedSavingAccountInterest) { | ||
| MifosTextFieldDropdown( | ||
| value = if (state.linkedSavingAccountIndex != -1) { | ||
| state.template.savingsAccounts?.get(state.linkedSavingAccountIndex)?.accountNo.orEmpty() | ||
| } else { | ||
| "" | ||
| }, | ||
| options = state.template.savingsAccounts?.map { | ||
| it.accountNo | ||
| } ?: emptyList(), | ||
| onValueChanged = {}, | ||
| onOptionSelected = { index, name -> | ||
| onAction( | ||
| NewFixedDepositAccountAction.OnLinkedSavingAccount(index), | ||
| ) | ||
| }, | ||
| label = stringResource(Res.string.feature_fixed_deposit_setting_linked_saving_account_field), | ||
| errorMessage = state.linkedSavingAccountError?.let { stringResource(it) }, | ||
| ) | ||
| } |
There was a problem hiding this comment.
Potential null safety issue in options mapping.
Line 243 uses it.accountNo directly in the options mapping, while line 238 uses it.accountNo.orEmpty() for displaying the selected value. If accountNo can be null, this could cause issues in the dropdown options list.
options = state.template.savingsAccounts?.map {
- it.accountNo
+ it.accountNo.orEmpty()
} ?: emptyList(),📝 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.
| AnimatedVisibility(state.transferLinkedSavingAccountInterest) { | |
| MifosTextFieldDropdown( | |
| value = if (state.linkedSavingAccountIndex != -1) { | |
| state.template.savingsAccounts?.get(state.linkedSavingAccountIndex)?.accountNo.orEmpty() | |
| } else { | |
| "" | |
| }, | |
| options = state.template.savingsAccounts?.map { | |
| it.accountNo | |
| } ?: emptyList(), | |
| onValueChanged = {}, | |
| onOptionSelected = { index, name -> | |
| onAction( | |
| NewFixedDepositAccountAction.OnLinkedSavingAccount(index), | |
| ) | |
| }, | |
| label = stringResource(Res.string.feature_fixed_deposit_setting_linked_saving_account_field), | |
| errorMessage = state.linkedSavingAccountError?.let { stringResource(it) }, | |
| ) | |
| } | |
| AnimatedVisibility(state.transferLinkedSavingAccountInterest) { | |
| MifosTextFieldDropdown( | |
| value = if (state.linkedSavingAccountIndex != -1) { | |
| state.template.savingsAccounts?.get(state.linkedSavingAccountIndex)?.accountNo.orEmpty() | |
| } else { | |
| "" | |
| }, | |
| options = state.template.savingsAccounts?.map { | |
| it.accountNo.orEmpty() | |
| } ?: emptyList(), | |
| onValueChanged = {}, | |
| onOptionSelected = { index, name -> | |
| onAction( | |
| NewFixedDepositAccountAction.OnLinkedSavingAccount(index), | |
| ) | |
| }, | |
| label = stringResource(Res.string.feature_fixed_deposit_setting_linked_saving_account_field), | |
| errorMessage = state.linkedSavingAccountError?.let { stringResource(it) }, | |
| ) | |
| } |
🤖 Prompt for AI Agents
In
feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/pages/SettingsPage.kt
around lines 235 to 254, the options list maps savingsAccounts using
it.accountNo which may be null while the selected value uses
it.accountNo.orEmpty(); update the options mapping to avoid nulls by using
either it.accountNo.orEmpty() for each mapped entry or filter out null accountNo
entries before mapping so the dropdown receives non-null strings and remains
consistent with the selected-value handling.
| MifosTextFieldDropdown( | ||
| value = if (state.investingAccountIndex != -1) { | ||
| state.template.savingsAccounts?.get(state.investingAccountIndex)?.accountNo.orEmpty() | ||
| } else { | ||
| "" | ||
| }, | ||
| options = state.template.savingsAccounts?.map { | ||
| it.accountNo | ||
| } ?: emptyList(), | ||
| onValueChanged = {}, | ||
| onOptionSelected = { index, name -> | ||
| onAction( | ||
| NewFixedDepositAccountAction.OnInvestingAccountChange(index), | ||
| ) | ||
| }, | ||
| label = stringResource(Res.string.feature_fixed_deposit_setting_investing_account), | ||
| errorMessage = state.investingAccountError?.let { stringResource(it) }, | ||
| ) |
There was a problem hiding this comment.
Same null safety inconsistency.
The options mapping at line 287 has the same issue as the linked saving account dropdown - inconsistent null handling between the displayed value (line 282 uses orEmpty()) and the options list (line 287 does not).
options = state.template.savingsAccounts?.map {
- it.accountNo
+ it.accountNo.orEmpty()
} ?: emptyList(),🤖 Prompt for AI Agents
In
feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/pages/SettingsPage.kt
around lines 280 to 297, the options list mapping uses
state.template.savingsAccounts?.map { it.accountNo } which can produce null
entries while the displayed value uses orEmpty(); change the mapping to
state.template.savingsAccounts?.map { it.accountNo.orEmpty() } ?: emptyList() so
both the dropdown value and options handle nulls consistently.
2 participants
Fixes - Jira-#562
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.