Implement New Share Account Charges Step #2545
Conversation
WalkthroughAdds charge-management data and UI: a new ChargeOptions model and chargeOptions on ShareTemplate; refactors share-account creation into CreateShareAccount* types with dialogs and charge flows; tweaks UI components, string resources, server default tenant, and small behavior fixes in loan/savings features. Changes
Sequence DiagramsequenceDiagram
participant User
participant ChargesPage
participant CreateShareAccountScreen
participant CreateShareAccountViewModel
participant DataModel
User->>ChargesPage: tap "Add New"/"View Charges"
ChargesPage->>CreateShareAccountScreen: emit ShowAddChargeDialog / ShowListOfChargesDialog
CreateShareAccountScreen->>CreateShareAccountViewModel: dispatch dialog action
CreateShareAccountViewModel->>CreateShareAccountViewModel: update dialogState in CreateShareAccountState
CreateShareAccountViewModel->>CreateShareAccountScreen: render with updated state
CreateShareAccountScreen->>User: shows AddChargeBottomSheet / MifosBottomSheet
User->>CreateShareAccountScreen: select charge / enter amount / confirm
CreateShareAccountScreen->>CreateShareAccountViewModel: dispatch AddChargeToList / EditCharge / DeleteCharge...
CreateShareAccountViewModel->>CreateShareAccountViewModel: modify addedCharges list, reset dialogState
CreateShareAccountViewModel->>DataModel: (optionally) read chargeOptions from ShareTemplate
CreateShareAccountViewModel->>CreateShareAccountScreen: emit updated CreateShareAccountState
CreateShareAccountScreen->>ChargesPage: updated state reflects new charge count/list
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas to focus during review:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/pages/DetailsPage.kt (1)
107-122: Guard against out‑of‑bounds access onproductOptionHere:
value = if (state.shareProductIndex == null) { "" } else { state.productOption[state.shareProductIndex].name }
shareProductIndexbeing non‑null doesn’t guarantee it’s withinproductOption.indices. If state ever gets out of sync (e.g., template reloaded, options list shrinks), this will crash withIndexOutOfBoundsException.You’re already using
getOrNullin similar places (e.g., savings accounts inTermsPage), so I’d align with that here:- value = if (state.shareProductIndex == null) { - "" - } else { - state.productOption[state.shareProductIndex].name - } + value = state.shareProductIndex + ?.let { index -> state.productOption.getOrNull(index)?.name } + .orEmpty()
🧹 Nitpick comments (5)
core/network/src/commonMain/kotlin/com/mifos/core/network/model/share/ShareTemplate.kt (1)
62-74: Verify that the all-nullable design matches the API contract.All properties in
ChargeOptionsare nullable with defaultnullvalues. While this provides flexibility for partial data from the API, it may lead to extensive null-checking in consuming code.Please confirm that:
- The backend API for share account charges can indeed return partial/optional data for these fields
- This nullable design aligns with how other similar template models handle optional data in the codebase
Consider adding KDoc comments to document:
- The purpose of
ChargeOptions- Which properties are typically present vs. truly optional
- Any business rules around these charge fields
Example:
/** * Represents charge configuration options available for share accounts. * All properties are nullable as the API may return partial charge data. * * @property id Unique identifier for the charge option * @property name Display name of the charge * ... */ @Serializable data class ChargeOptions( ... )feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountScreen.kt (1)
453-453: UseIntinstead ofInt?for expandedIndex.The
expandedIndexis declared asInt?but is never actually set tonull—it's always either-1or a valid index. This makes the nullable type misleading.Apply this diff to clarify the type:
- var expandedIndex: Int? by rememberSaveable { mutableStateOf(-1) } + var expandedIndex: Int by rememberSaveable { mutableStateOf(-1) }feature/client/src/commonMain/composeResources/values/strings.xml (1)
572-583: Clarify and de‑duplicate “Add” vs “Add New” charge stringsThe new charge strings look fine overall, but these are a bit confusing:
feature_share_account_charge_add_new→ "Add New"feature_share_account_charge_add→ "Add"feature_share_account_charge_btn_add_new→ "Add"You effectively have two different keys whose values are "Add". That can make translations and future maintenance harder.
Consider either:
- Reusing a single
"Add"key for both buttons, or- Renaming
feature_share_account_charge_btn_add_new(and/or adjusting its value) to clearly distinguish it from the plain"Add"confirm action, e.g."Add New Charge".feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/pages/ChargesPage.kt (1)
47-52: Minor layout & copy improvements for ChargesPageThe new state‑driven ChargesPage looks functionally correct and well‑wired to
CreateShareAccountAction. A couple of small refinements to consider:
- Honor the passed
modifierat the rootRight now:
Column(modifier = Modifier.fillMaxSize()) { Column( modifier = modifier.weight(1f).verticalScroll(rememberScrollState()), ) { ... } ... }This means any padding/semantics passed via
modifieronly affect the scrollable content, not the entire page. To keep things composable-friendly:- Column(modifier = Modifier.fillMaxSize()) { + Column(modifier = modifier.fillMaxSize()) { - Column( - modifier = modifier.weight(1f).verticalScroll(rememberScrollState()), - ) { + Column( + modifier = Modifier.weight(1f).verticalScroll(rememberScrollState()), + ) {
- Pluralization for active charges label
text = state.addedCharges.size.toString() + " " + stringResource(Res.string.feature_share_account_charge_active_charge)This will show “0 Active Charge” / “2 Active Charge”. If possible, consider a plural resource or at least a basic pluralization check to improve UX.
Also applies to: 53-101, 102-112
feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/CreateShareAccountViewModel.kt (1)
574-577: Consider non-nullable fields with validation.The
CreatedChargesdata class allows nullablechargeIdandamount, which could result in invalid charges being added to the list. While this works with the current implementation, making these fields non-nullable and ensuring validation before creation (as suggested in earlier comments) would provide stronger type safety.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
core/common/src/commonMain/kotlin/com/mifos/core/common/utils/ServerConfig.kt(1 hunks)core/network/src/commonMain/kotlin/com/mifos/core/network/model/share/ShareTemplate.kt(3 hunks)core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/AddChargeDialog.kt(3 hunks)core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosActionsListingCardComponent.kt(2 hunks)feature/client/src/commonMain/composeResources/values/strings.xml(1 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/CreateShareAccountScreen.kt(6 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/CreateShareAccountViewModel.kt(18 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/pages/ChargesPage.kt(1 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/pages/DetailsPage.kt(6 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/pages/TermsPage.kt(12 hunks)feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountScreen.kt(2 hunks)feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountv2/SavingsAccountViewModel.kt(1 hunks)feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountv2/pages/DetailsPage.kt(0 hunks)
💤 Files with no reviewable changes (1)
- feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountv2/pages/DetailsPage.kt
🧰 Additional context used
🧬 Code graph analysis (6)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/pages/ChargesPage.kt (2)
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/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/pages/DetailsPage.kt (1)
core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/component/MifosOutlinedTextField.kt (4)
MifosOutlinedTextField(66-124)MifosOutlinedTextField(126-243)MifosOutlinedTextField(245-308)MifosOutlinedTextField(415-487)
core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/AddChargeDialog.kt (1)
core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/component/MifosOutlinedTextField.kt (1)
MifosDatePickerTextField(363-413)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/CreateShareAccountViewModel.kt (1)
feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountv2/SavingsAccountViewModel.kt (2)
moveToNextStep(617-626)moveToPreviousStep(608-615)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/CreateShareAccountScreen.kt (7)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountScreen.kt (2)
AddNewChargeDialog(378-445)ShowChargesDialog(447-529)core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosProgressIndicator.kt (2)
MifosProgressIndicator(41-68)MifosProgressIndicatorOverlay(70-102)core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/component/MifosSweetError.kt (1)
MifosSweetError(40-83)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(598-730)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/MifosActionsListingCardComponent.kt (1)
core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/PrintTextUtil.kt (1)
PrintTextUtil(18-27)
⏰ 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 (22)
core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/AddChargeDialog.kt (3)
53-55: Good separation of concerns for date handling.The changes correctly separate two aspects:
showDatePickercontrols the date picker modal stateselectedDatecontrols whether the date field should be renderedMaking
selectedDatenullable with a default allows charges without date fields, which aligns with the PR objective of handling different charge types.
112-120: Clean conditional rendering of date field.The date picker field and its associated spacing are correctly rendered only when
selectedDateis not null, providing a clean UI for charge types that don't require dates.
166-166: Correct validation focusing on user input.The validation logic appropriately checks only the user-provided
chargeAmountfield and its error state. SincechargeTypeis a read-only field populated by the system (lines 137-145), it doesn't need validation.core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosActionsListingCardComponent.kt (2)
602-602: Consistent optional date handling.Making the
dateparameter nullable with a default value aligns with the changes inAddChargeDialog.ktand supports displaying charges that don't have associated dates.
645-653: Proper conditional rendering of optional date.The date is correctly displayed only when present, maintaining clean UI layout for charges without dates.
core/network/src/commonMain/kotlin/com/mifos/core/network/model/share/ShareTemplate.kt (2)
32-32: LGTM! Good default value for optional charge options.The
chargeOptionsfield is properly added with an appropriate default empty list, following Kotlin conventions for optional collections.
12-15: Imports verified—all Charge-related types are available in the codebase.The four imported types (ChargeAppliesTo, ChargeCalculationType, ChargePaymentMode, ChargeTimeType) from
com.mifos.core.model.objects.template.clientall exist and will resolve correctly. No issues with the imports.core/common/src/commonMain/kotlin/com/mifos/core/common/utils/ServerConfig.kt (1)
34-34: Verify that this tenant configuration change is intentional and necessary.The git history confirms that the tenant value was changed from "default" to "dev" in this PR commit. However, this modification appears unrelated to the stated objective of implementing Share Account Charges functionality.
Concerns:
- No explanation provided in the PR description for this configuration change
- Could be an accidental commit or leftover from local development
- Affects default behavior for all application users connecting to the demo server
- While the change may be correct (dev tenant for tt.mifos.community), it requires explicit clarification
Please confirm:
- Is this change intentional and required for the Share Account Charges feature?
- If intentional, what is the reason for changing the default tenant?
- Should this be part of a separate configuration PR with proper documentation?
feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountv2/SavingsAccountViewModel.kt (2)
1-627: Field officer validation was removed without explanation; verify if this change is intentional.The commit removes the validation check for
fieldOfficerIndex == -1inhandleOnDetailsSubmit()(previously lines 260-263) and removes thefieldOfficerErrorproperty from state, yet the UI still displays the field officer dropdown. When no field officer is selected,fieldOfficerIdwill benullin the payload (line 140). TheSavingsPayload.fieldOfficerIdis nullable, but the backend API requirement is unclear. The PR description does not explain this change.Verify:
- Whether the backend API accepts
nullfieldOfficerId- Whether field officer should be optional or remain required
- If optional, the UI should clarify this (e.g., mark as "Optional" or remove the field entirely)
254-275: Field officer validation removed from Savings Account Details submission - verify intentionality.Verification confirms the field officer validation has been removed from
handleOnDetailsSubmit(). Unlike similar account types (RecurringDeposit and FixedDeposit), which still enforce field officer selection through both button enabling logic (fieldOfficerIndex != -1) and error validation, the Savings v2 implementation allows users to proceed without selecting a field officer. WithfieldOfficerIndexdefaulting to -1, users can submit without a valid selection, resulting in anullfieldOfficerIdin the payload.While the API payload defines
fieldOfficerIdas nullable (suggesting it's optional), this inconsistency with RecurringDeposit and FixedDeposit patterns warrants clarification: was this removal intentional as part of the Savings refactoring, or was validation inadvertently omitted during migration?feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/pages/DetailsPage.kt (1)
44-55: CreateShareAccount state/action refactor is consistentThe switch to
CreateShareAccountState/CreateShareAccountActionhere is consistent with the rest of the flow and keeps the event surface coherent across pages.feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/pages/TermsPage.kt (1)
55-66: Terms page refactor toCreateShareAccount*looks solidThe migration to
CreateShareAccountState/CreateShareAccountActionand the updated callbacks (application date, total shares, savings account, min active/lock‑in fields, dividends flag, step navigation) are consistent and use safer patterns (getOrNull,isNotBlank) for indexed options.No issues spotted in the new wiring.
Also applies to: 77-183, 193-265, 267-275
feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/CreateShareAccountScreen.kt (2)
70-88: CreateShareAccount refactor and dialog orchestration are cohesive*The migration of the screen to
CreateShareAccountState/CreateShareAccountAction/CreateShareAccountEventplus the extraction ofCreateShareAccountDialogandCreateShareAccountContentreads clean and keeps responsibilities separated:
- Event handling via
EventsEffectis straightforward.- Steps wiring (
DetailsPage,TermsPage,ChargesPage,PreviewPage) all run through a singleonActionchannel.- Screen state handling (
Loading/Success/Error) and the overlay indicator are consistent with other features.No behavioral red flags here.
Also applies to: 90-114, 116-147, 149-183
244-309: ShowChargesDialog implementation matches existing patterns and looks correct
ShowChargesDialogmirrors the loan charges listing pattern:
- Uses
MifosBottomSheetwith a clear title (feature_share_account_charge_view_charges).- Resolves each charge’s metadata from
state.chargeOptionsbychargeId.- Delegates edit/delete actions through
CreateShareAccountActionvariants.- Uses
rememberSaveableto keep the expanded row across recompositions and configuration changes.- Provides a bottom
MifosTwoButtonRowto dismiss or jump to “Add charge”.Given
ChargesPagedisables the “View” button whenaddedChargesis empty, you also avoid the empty-list edge case.feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/CreateShareAccountViewModel.kt (8)
50-143: LGTM: Template loading correctly updated.The changes to
loadShareTemplateFromProductandloadShareTemplatecorrectly:
- Accept nullable
productIdfor flexible template fetching- Update state transitions to
CreateShareAccountState.ScreenStatetypes- Manage
isOverLayLoadingActiveflag during async operations- Populate
chargeOptionsfrom the template response
165-177: LGTM: Detail navigation correctly loads product-specific template.The function correctly passes
state.productIdto load the template for the selected product before advancing to the next step.
179-226: LGTM: Validation logic is correct.The formatting adjustments improve readability while maintaining the correct validation logic for terms step.
375-381: LGTM: Charge management actions are correctly implemented.The handlers for
OnChargeAmountChange,OnChooseChargeIndexChange,OnDismissDialog,DeleteChargeFromSelectedCharges,ShowAddChargeDialog, andShowListOfChargesDialogcorrectly update state for their respective operations.Also applies to: 417-431, 433-440, 459-473
228-373: LGTM: Action handlers correctly renamed and updated.All action handlers have been correctly migrated to use
CreateShareAccountActiontypes, with appropriate logic preserved. The addition ofproductIdtracking at line 284 supports the product-specific template loading.
478-534: LGTM: State model correctly extended for charge management.The
CreateShareAccountStatedata class correctly adds:
- Dialog state management via
DialogStatesealed interface- Charge selection and input fields (
chooseChargeIndex,chargeAmount)- Template data (
chargeOptions,productId)- Loading state (
isOverLayLoadingActive)- Added charges tracking (
addedCharges)All fields have appropriate defaults and types.
536-567: LGTM: Action interface comprehensively covers charge operations.The
CreateShareAccountActionsealed interface correctly defines all actions for:
- Navigation and step management
- Form field updates
- Charge management (add, edit, delete, show dialogs)
Action naming is clear and parameters are appropriate.
569-572: LGTM: Event interface correctly defined.The
CreateShareAccountEventsealed interface appropriately defines navigation events.
| @Composable | ||
| private fun AddNewChargeDialog( | ||
| isEdit: Boolean, | ||
| index: Int = -1, | ||
| state: CreateShareAccountState, | ||
| onAction: (CreateShareAccountAction) -> Unit, | ||
| ) { | ||
| AddChargeBottomSheet( | ||
| title = if (isEdit) { | ||
| stringResource(Res.string.feature_share_account_charge_edit_charge) | ||
| } else { | ||
| stringResource(Res.string.feature_share_account_charge_add_new_charge) | ||
| }, | ||
| confirmText = if (isEdit) { | ||
| stringResource(Res.string.feature_share_account_charge_edit_charge) | ||
| } else { | ||
| stringResource(Res.string.feature_share_account_charge_add) | ||
| }, | ||
| dismissText = stringResource(Res.string.feature_share_account_back), | ||
| selectedChargeName = if (state.chooseChargeIndex == null) { | ||
| "" | ||
| } else { | ||
| state.chargeOptions[state.chooseChargeIndex].name ?: "" | ||
| }, | ||
| chargeAmount = state.chargeAmount, | ||
| chargeType = if (state.chooseChargeIndex == null) { | ||
| "" | ||
| } else { | ||
| state.chargeOptions[state.chooseChargeIndex].chargeCalculationType?.value | ||
| ?: "" | ||
| }, | ||
| chargeCollectedOn = if (state.chooseChargeIndex == null) { | ||
| "" | ||
| } else { | ||
| state.chargeOptions[state.chooseChargeIndex].chargeTimeType?.value | ||
| ?: "" | ||
| }, | ||
| chargeOptions = state.chargeOptions.map { it.name ?: "" }, | ||
| onConfirm = { | ||
| if (isEdit) { | ||
| onAction(CreateShareAccountAction.EditCharge(index)) | ||
| } else { | ||
| onAction(CreateShareAccountAction.AddChargeToList) | ||
| } | ||
| }, | ||
| onDismiss = { onAction(CreateShareAccountAction.OnDismissDialog) }, | ||
| onChargeSelected = { index, _ -> | ||
| onAction(CreateShareAccountAction.OnChooseChargeIndexChange(index)) | ||
| }, | ||
| onDatePick = {}, | ||
| onDateChange = {}, | ||
| onAmountChange = { amount -> | ||
| onAction(CreateShareAccountAction.OnChargeAmountChange(amount)) | ||
| }, | ||
| ) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Charge date handling in AddNewChargeDialog is effectively disabled
In AddNewChargeDialog you wire AddChargeBottomSheet like this:
AddChargeBottomSheet(
// ...
onDatePick = {},
onDateChange = {},
// no showDatePicker / selectedDate passed
)Given the shared AddChargeBottomSheet API supports a charge date (with showDatePicker, selectedDate, onDatePick, onDateChange), this means the share-account flow currently cannot:
- Toggle the date picker, or
- Persist/update any chosen date for a charge.
If share-account charges never require an explicit date (and backend defaults are always correct), this might be intentional. If some charge types are “due on” a specific date, the current wiring will be insufficient.
I recommend either:
- Explicitly deciding that share-account charges don’t use a date (and, if so, hiding the date UI in
AddChargeBottomSheetfor this flow), or - Adding date state to
CreateShareAccountStateand passing the propershowDatePicker,selectedDate,onDatePick, andonDateChangecallbacks, mirroring the loan new-account implementation.
🏁 Script executed:
#!/bin/bash
# Verify the exact code at lines 187-242 in the CreateShareAccountScreen.kt file
cat feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/CreateShareAccountScreen.kt | sed -n '187,242p'Length of output: 2048
🏁 Script executed:
#!/bin/bash
# Find and inspect the AddChargeBottomSheet composable definition
rg -n "fun AddChargeBottomSheet|@Composable.*AddChargeBottomSheet" -A 20 --type kotlinLength of output: 4807
🏁 Script executed:
#!/bin/bash
# Search for CreateShareAccountState to understand its structure
rg -n "data class CreateShareAccountState|class CreateShareAccountState" -A 15 --type kotlinLength of output: 2454
🏁 Script executed:
#!/bin/bash
# Find loan new-account implementation to compare date handling
fd -e kt -path "*/loan*" -path "*newAccount*" | head -10Length of output: 168
🏁 Script executed:
#!/bin/bash
# Search for date-related state or handling in share account charges
rg -n "chargeDate|datePicke|OnDate" -C 3 --type kotlin feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check if AddChargeBottomSheet has showDatePicker and selectedDate parameters
rg -n "showDatePicker|selectedDate" -C 2 --type kotlin | grep -i "addcharge\|composable" -A 10 -B 5Length of output: 5163
🏁 Script executed:
#!/bin/bash
# Find CreateShareAccountAction definition to see if date actions exist
rg -n "sealed class CreateShareAccountAction|data class.*Charge|object.*Charge" -A 3 --type kotlin feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/Length of output: 3354
🏁 Script executed:
#!/bin/bash
# Search for loan feature charge handling implementation
rg -n "AddChargeBottomSheet" -B 5 -A 20 --type kotlin feature/loan/ | head -100Length of output: 7547
🏁 Script executed:
#!/ab/bash
# Check if share account charges are used in any API call or submission
rg -n "chargeAmount|AddChargeToList|EditCharge" -C 3 --type kotlin feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/Length of output: 15160
🏁 Script executed:
#!/bin/bash
# Check the complete AddChargeBottomSheet implementation to see if date field is always rendered
cat core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/AddChargeDialog.kt | sed -n '49,170p'Length of output: 4580
Confirm date handling is intentionally disabled or implement date support for share-account charges
The current wiring in AddNewChargeDialog (lines 187-242) passes empty lambdas for onDatePick and onDateChange without providing showDatePicker or selectedDate parameters. Since AddChargeBottomSheet only renders the date field when selectedDate is not null, the date UI is completely hidden for share-account charges.
The data layer confirms this: CreateShareAccountState contains no date fields for charges, and both AddChargeToList and EditCharge actions only store chargeId and amount in CreatedCharges—no date.
By contrast, the loan new-account implementation passes showDatePicker = state.showChargesDatePick and selectedDate = state.chargeDate, demonstrating this feature already works elsewhere.
Either explicitly decide share-account charges don't require dates (and hide the date UI in AddChargeBottomSheet for this flow), or add date state to CreateShareAccountState and wire the callbacks properly to match the loan implementation.
🤖 Prompt for AI Agents
In
feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/CreateShareAccountScreen.kt
around lines 187-242, the date callbacks (onDatePick/onDateChange) are no-ops
and selectedDate/showDatePicker are not passed, which hides the date UI while
the state/actions also lack charge date handling; either explicitly remove/hide
the date field in AddChargeBottomSheet when no date support is intended (modify
AddChargeBottomSheet call to pass null/flag to hide date and adjust its
rendering), or add date support by extending CreateShareAccountState with
chargeDate and showChargesDatePick, update CreateShareAccountAction to carry
date for AddChargeToList/EditCharge and CreatedCharges to store the date, and
wire onDatePick to toggle showChargesDatePick and onDateChange to dispatch
charge date changes, passing selectedDate = state.chargeDate and showDatePicker
= state.showChargesDatePick like the loan flow.
| import com.mifos.core.network.model.share.SavingsAccountOption | ||
| import com.mifos.core.ui.util.BaseViewModel | ||
| import com.mifos.core.ui.util.TextFieldsValidator | ||
| import com.mifos.feature.loan.newLoanAccount.NewLoanAccountState.DialogState |
There was a problem hiding this comment.
Remove unused import from the loan feature.
This import references DialogState from NewLoanAccountState (loan feature), but the code defines its own DialogState sealed interface at lines 530-533. The import is unused and misleading.
Apply this diff:
-import com.mifos.feature.loan.newLoanAccount.NewLoanAccountState.DialogState📝 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 com.mifos.feature.loan.newLoanAccount.NewLoanAccountState.DialogState |
🤖 Prompt for AI Agents
In
feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/CreateShareAccountViewModel.kt
around line 27, remove the unused import "import
com.mifos.feature.loan.newLoanAccount.NewLoanAccountState.DialogState" because
this file defines its own DialogState sealed interface later (lines ~530-533);
delete that import line and run a build/IDE reformat to ensure there are no
unresolved references or unused-import warnings remaining.
| is CreateShareAccountAction.AddChargeToList -> { | ||
| val createdData = CreatedCharges( | ||
| chargeId = state.chargeOptions[state.chooseChargeIndex!!].id, | ||
| amount = state.chargeAmount.toDoubleOrNull(), | ||
| ) | ||
|
|
||
| mutableStateFlow.update { | ||
| it.copy( | ||
| addedCharges = it.addedCharges + createdData, | ||
| chooseChargeIndex = null, | ||
| dialogState = null, | ||
| chargeAmount = "", | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
Add validation before creating charge.
The code has several validation gaps:
- Line 385: Force unwrap
state.chooseChargeIndex!!without null check will crash if null - Line 386:
toDoubleOrNull()can return null with no validation - No bounds check before accessing
chargeOptions[state.chooseChargeIndex!!]
Consider validating that a charge is selected and amount is valid before creating the charge.
Apply this diff:
is CreateShareAccountAction.AddChargeToList -> {
+ val chargeIndex = state.chooseChargeIndex
+ val amount = state.chargeAmount.toDoubleOrNull()
+
+ if (chargeIndex == null || amount == null || amount <= 0) {
+ // Handle validation error - show error message or ignore
+ return
+ }
+
val createdData = CreatedCharges(
- chargeId = state.chargeOptions[state.chooseChargeIndex!!].id,
- amount = state.chargeAmount.toDoubleOrNull(),
+ chargeId = state.chargeOptions[chargeIndex].id,
+ amount = amount,
)
mutableStateFlow.update {
it.copy(
addedCharges = it.addedCharges + createdData,
chooseChargeIndex = null,
dialogState = null,
chargeAmount = "",
)
}
}📝 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 CreateShareAccountAction.AddChargeToList -> { | |
| val createdData = CreatedCharges( | |
| chargeId = state.chargeOptions[state.chooseChargeIndex!!].id, | |
| amount = state.chargeAmount.toDoubleOrNull(), | |
| ) | |
| mutableStateFlow.update { | |
| it.copy( | |
| addedCharges = it.addedCharges + createdData, | |
| chooseChargeIndex = null, | |
| dialogState = null, | |
| chargeAmount = "", | |
| ) | |
| } | |
| } | |
| is CreateShareAccountAction.AddChargeToList -> { | |
| val chargeIndex = state.chooseChargeIndex | |
| val amount = state.chargeAmount.toDoubleOrNull() | |
| if (chargeIndex == null || amount == null || amount <= 0) { | |
| // Handle validation error - show error message or ignore | |
| return | |
| } | |
| val createdData = CreatedCharges( | |
| chargeId = state.chargeOptions[chargeIndex].id, | |
| amount = amount, | |
| ) | |
| mutableStateFlow.update { | |
| it.copy( | |
| addedCharges = it.addedCharges + createdData, | |
| chooseChargeIndex = null, | |
| dialogState = null, | |
| chargeAmount = "", | |
| ) | |
| } | |
| } |
| is CreateShareAccountAction.EditCharge -> { | ||
| val createdData = CreatedCharges( | ||
| chargeId = state.chargeOptions[action.index].id, | ||
| amount = state.chargeAmount.toDoubleOrNull(), | ||
| ) | ||
|
|
||
| val currentAddedCharges = state.addedCharges.toMutableList() | ||
| currentAddedCharges[action.index] = createdData | ||
| mutableStateFlow.update { | ||
| it.copy( | ||
| addedCharges = currentAddedCharges, | ||
| chooseChargeIndex = null, | ||
| dialogState = CreateShareAccountState.DialogState.ShowCharges, | ||
| chargeAmount = "", | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical: Incorrect index used to access charge options.
Line 401 uses action.index to access chargeOptions, but action.index is the position in addedCharges, not chargeOptions. This will access the wrong charge or crash with an index out of bounds error.
The correct approach is to use state.chooseChargeIndex (set by EditChargeDialog at line 450) to access the selected charge from chargeOptions.
Additionally, the same validation issues from AddChargeToList apply here.
Apply this diff:
is CreateShareAccountAction.EditCharge -> {
+ val chargeIndex = state.chooseChargeIndex
+ val amount = state.chargeAmount.toDoubleOrNull()
+
+ if (chargeIndex == null || amount == null || amount <= 0) {
+ // Handle validation error
+ return
+ }
+
val createdData = CreatedCharges(
- chargeId = state.chargeOptions[action.index].id,
- amount = state.chargeAmount.toDoubleOrNull(),
+ chargeId = state.chargeOptions[chargeIndex].id,
+ amount = amount,
)
val currentAddedCharges = state.addedCharges.toMutableList()
currentAddedCharges[action.index] = createdData
mutableStateFlow.update {
it.copy(
addedCharges = currentAddedCharges,
chooseChargeIndex = null,
dialogState = CreateShareAccountState.DialogState.ShowCharges,
chargeAmount = "",
)
}
}📝 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 CreateShareAccountAction.EditCharge -> { | |
| val createdData = CreatedCharges( | |
| chargeId = state.chargeOptions[action.index].id, | |
| amount = state.chargeAmount.toDoubleOrNull(), | |
| ) | |
| val currentAddedCharges = state.addedCharges.toMutableList() | |
| currentAddedCharges[action.index] = createdData | |
| mutableStateFlow.update { | |
| it.copy( | |
| addedCharges = currentAddedCharges, | |
| chooseChargeIndex = null, | |
| dialogState = CreateShareAccountState.DialogState.ShowCharges, | |
| chargeAmount = "", | |
| ) | |
| } | |
| } | |
| is CreateShareAccountAction.EditCharge -> { | |
| val chargeIndex = state.chooseChargeIndex | |
| val amount = state.chargeAmount.toDoubleOrNull() | |
| if (chargeIndex == null || amount == null || amount <= 0) { | |
| // Handle validation error | |
| return | |
| } | |
| val createdData = CreatedCharges( | |
| chargeId = state.chargeOptions[chargeIndex].id, | |
| amount = amount, | |
| ) | |
| val currentAddedCharges = state.addedCharges.toMutableList() | |
| currentAddedCharges[action.index] = createdData | |
| mutableStateFlow.update { | |
| it.copy( | |
| addedCharges = currentAddedCharges, | |
| chooseChargeIndex = null, | |
| dialogState = CreateShareAccountState.DialogState.ShowCharges, | |
| chargeAmount = "", | |
| ) | |
| } | |
| } |
🤖 Prompt for AI Agents
In
feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/CreateShareAccountViewModel.kt
around lines 399 to 415, the code incorrectly uses action.index to read from
state.chargeOptions (action.index is the position in addedCharges), which can
pick the wrong charge or throw an OOB; change it to read the charge id from
state.chargeOptions at state.chooseChargeIndex (null-check or return early if
null or OOB), parse/validate state.chargeAmount the same way as AddChargeToList
(ensure amount is non-null and positive), and then create the CreatedCharges and
replace the entry in currentAddedCharges at action.index; finally update the
state clearing chooseChargeIndex and dialogState as before. Ensure all index
bounds checks and null/parse validation mirror AddChargeToList to avoid crashes
or invalid entries.
| is CreateShareAccountAction.EditChargeDialog -> { | ||
| val selectedEditCharge = state.addedCharges[action.index] | ||
| val chooseChargeIndex = state.chargeOptions | ||
| .indexOfFirst { it.id == selectedEditCharge.chargeId } | ||
|
|
||
| mutableStateFlow.update { | ||
| it.copy( | ||
| chargeAmount = selectedEditCharge.amount.toString(), | ||
| chooseChargeIndex = chooseChargeIndex, | ||
| dialogState = CreateShareAccountState.DialogState.AddNewCharge( | ||
| true, | ||
| action.index, | ||
| ), | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
Handle nullable amount safely when editing.
Line 449 calls toString() on selectedEditCharge.amount which is nullable (Double?). If the amount is null, this will produce the string "null" instead of an empty string, which would be confusing in the UI.
Apply this diff:
mutableStateFlow.update {
it.copy(
- chargeAmount = selectedEditCharge.amount.toString(),
+ chargeAmount = selectedEditCharge.amount?.toString() ?: "",
chooseChargeIndex = chooseChargeIndex,
dialogState = CreateShareAccountState.DialogState.AddNewCharge(
true,
action.index,
),
)
}🤖 Prompt for AI Agents
In
feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/CreateShareAccountViewModel.kt
around lines 442 to 457, the code calls toString() on selectedEditCharge.amount
(Double?) which yields "null" when amount is null; change the assignment to
convert null to an empty string (for example using
selectedEditCharge.amount?.toString() ?: "") so the UI shows an empty field
instead of the literal "null", and update any related formatting if needed.
| isExpanded = expandedIndex == index, | ||
| onExpandToggle = { | ||
| expandedIndex = if (expandedIndex == it.id) -1 else it.id | ||
| expandedIndex = if (expandedIndex == index) -1 else index | ||
| }, |
There was a problem hiding this comment.
Index-based expansion tracking may cause UI glitches after deletion.
When a charge is deleted (lines 494-498), the expandedIndex is not updated. This can cause the wrong charge to appear expanded because list indices shift after deletion. For example, if charge at index 1 is expanded and charge at index 0 is deleted, the charge previously at index 2 will now appear expanded.
Consider one of these solutions:
Solution 1: Reset expansion state on deletion
onActionClicked = { action ->
when (action) {
is Actions.Delete -> {
expandedIndex = -1 // Reset expansion
onAction(
NewLoanAccountAction.DeleteChargeFromSelectedCharges(
index,
),
)
}
// ...
}
}Solution 2: Use ID-based tracking (preferred for stability)
Replace index-based tracking with charge ID-based tracking:
- var expandedIndex: Int? by rememberSaveable { mutableStateOf(-1) }
+ var expandedChargeId: Int? by rememberSaveable { mutableStateOf(null) }Then update comparisons:
- isExpanded = expandedIndex == index,
+ isExpanded = expandedChargeId == it.id,
onExpandToggle = {
- expandedIndex = if (expandedIndex == index) -1 else index
+ expandedChargeId = if (expandedChargeId == it.id) null else it.id
},Note: This requires that charge items have stable id properties.
Also applies to: 508-511
🤖 Prompt for AI Agents
In
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountScreen.kt
around lines 477-480 (and similarly 508-511), the UI uses index-based expansion
state which breaks when list items are removed; update the deletion handling or
the expansion tracking: either (quick fix) reset the expansion state before
performing a delete by setting expandedIndex = -1 immediately when handling
Actions.Delete, or (preferred) switch to ID-based tracking by replacing
expandedIndex with a nullable expandedId (type matching charge.id), update
onExpandToggle to set/unset expandedId using the item id, compare item.id to
expandedId when rendering isExpanded, and ensure you clear expandedId when the
deleted charge id equals expandedId; apply the same change to all places
mentioned.
2 participants
Fixes - Jira-#545
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.