-
Notifications
You must be signed in to change notification settings - Fork 671
client collateral button fix #2585
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
client collateral button fix #2585
Conversation
📝 WalkthroughWalkthroughRefactors the ClientCollateral UI from a scaffold-based layout to a Column-based composition, converts quantity state from Int to String with validation and quantityError, adds isOverlayLoading and MifosProgressIndicatorOverlay, consolidates breadcrumb padding, and replaces the action row with MifosTwoButtonRow. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Screen
participant ViewModel
participant Validator
participant Repo
User->>Screen: Select collateral / enter quantity
Screen->>ViewModel: OnCollateralSelected / OnQuantityChange(quantity:String)
ViewModel-->>Screen: Update UI state (selection, quantity, clear errors)
User->>Screen: Tap Submit
Screen->>ViewModel: OnSave
ViewModel->>Validator: TextFieldsValidator.validate(fields)
alt Validation passes
Validator-->>ViewModel: Valid
ViewModel->>ViewModel: set isOverlayLoading = true
ViewModel->>Screen: show MifosProgressIndicatorOverlay
ViewModel->>Repo: saveCollateral(data, quantity:String)
Repo-->>ViewModel: Result
ViewModel->>ViewModel: set isOverlayLoading = false
ViewModel->>Screen: show ShowStatusDialog
else Validation fails
Validator-->>ViewModel: Invalid(errors)
ViewModel->>Screen: set quantityError / show field errors
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
...nt/src/commonMain/kotlin/com/mifos/feature/client/clientCollateral/ClientCollateralScreen.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientCollateral/ClientCollateralScreen.kt`:
- Around line 122-130: When the user selects a different collateral in
MifosTextFieldDropdown the handler onOptionSelected currently only dispatches
ClientCollateralAction.OptionChanged(index) so derived totals remain stale;
update onOptionSelected to also trigger a recalculation by dispatching
ClientCollateralAction.OnQuantityChange(state.quantity) (or the equivalent
recalc action used in your reducer) immediately after OptionChanged so totals
and derived fields refresh on selection change.
- Around line 96-121: The root Column in ClientCollateralScreen is not receiving
the caller-provided modifier (it's only applied to the inner scroll Column),
which drops root-level size/semantics/test tags; move the incoming parameter
named modifier from the inner Column to the outer Column (the top-level Column
in ClientCollateralScreen) and replace the inner Column's usage with a plain
Modifier (e.g., Modifier.fillMaxWidth()...) so the outer container preserves
caller semantics while the inner scrollable Column uses its own local Modifier.
In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientCollateral/ClientCollateralViewModel.kt`:
- Around line 137-148: The code in ClientCollateralViewModel inside the is
ClientCollateralAction.OnQuantityChange branch currently calls
action.quantity.toInt() unguarded, which can throw NumberFormatException on
empty/invalid input; change the logic in mutableStateFlow.update to parse
action.quantity with toIntOrNull(), compute total and totalCollateral only when
the parsedInt is non-null, and otherwise set total and totalCollateral to 0.0
(and set quantityError appropriately); locate the update block handling
currentCollateral and replace the unguarded toInt() usage with a safe parse
(e.g., parsedQty) and conditional calculations using currentCollateral.basePrice
and currentCollateral.pctToBase.
...nt/src/commonMain/kotlin/com/mifos/feature/client/clientCollateral/ClientCollateralScreen.kt
Show resolved
Hide resolved
...nt/src/commonMain/kotlin/com/mifos/feature/client/clientCollateral/ClientCollateralScreen.kt
Show resolved
Hide resolved
...src/commonMain/kotlin/com/mifos/feature/client/clientCollateral/ClientCollateralViewModel.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientCollateral/ClientCollateralScreen.kt`:
- Around line 203-208: Add an isEnabled boolean to ClientCollateralState
(computed in ClientCollateralViewModel) derived from your validation rules
(e.g., quantityError == null and any other field-level errors are null) so the
view knows when submission is allowed; update the state calculation inside
ClientCollateralViewModel (where other derived fields are set) to set isEnabled
accordingly; then wire that field into ClientCollateralScreen by passing
state.isEnabled to MifosTwoButtonRow (so the submit button is disabled when
isEnabled is false) while keeping the existing onSecondBtnClick = {
onAction(ClientCollateralAction.OnSave) } handler.
Fixes - Jira-#630
Didn't create a Jira ticket, click here to create new.
Please Add Screenshots If there are any UI changes.
|


|
|
Please make sure these boxes are checked before submitting your pull request - thanks!
Run the static analysis check
./gradlew checkorci-prepush.shto make sure you didn't break anythingIf you have multiple commits please combine them into one commit by squashing them.
Summary by CodeRabbit
New Features
Bug Fixes
Style
✏️ Tip: You can customize this high-level summary in your review settings.