feature(savingsAccount) : charge details step impl #2516
Conversation
after opening bottom sheet without entering any amount it is showing error message.
|
| val fieldOfficerOptions: List<FieldOfficerOptions>? = null, | ||
|
|
||
| @IgnoredOnParcel | ||
| val chargeOptions: List<ChargeOptions>? = null, |
There was a problem hiding this comment.
remove @IgnoreOnParcel annotation
| confirmValueChange = { newValue -> | ||
| newValue != SheetValue.Hidden | ||
| }, |
There was a problem hiding this comment.
The confirmValueChange callback blocks the sheet from reaching Hidden, preventing swipe-down dismissal and breaking standard Material Design UX.
Suggestion: Remove confirmValueChange or allow dismissal.
| confirmValueChange = { newValue -> | |
| newValue != SheetValue.Hidden | |
| }, | |
| confirmValueChange = { newValue -> | |
| true // Allow all state changes | |
| }, |
There was a problem hiding this comment.
I intentionally added that to prevent the Bottom Sheet from accidentally dismissing when scrolling through view charges, as that too seems like a bad UX.
There was a problem hiding this comment.
This is a screen Recording highlighting the issue i m trying to mention:
Screen_recording_20251008_214517.webm
| } else { | ||
| state.savingsProductTemplate?.chargeOptions?.get(state.chooseChargeIndex)?.name ?: "" | ||
| }, | ||
| selectedDate = if (state.chargeDate >= DateHelper.getDateAsStringFromLong(Clock.System.now().toEpochMilliseconds())) { |
There was a problem hiding this comment.
Dates are compared as strings using >=, which may produce incorrect results since string comparison is lexicographic, not chronological.
Suggested fix: Try using epoch milliseconds itself for comparison.
| selectedChargeName = if (state.chooseChargeIndex == -1) { | ||
| "" | ||
| } else { | ||
| state.savingsProductTemplate?.chargeOptions?.get(state.chooseChargeIndex)?.name ?: "" |
There was a problem hiding this comment.
List access occurs without verifying index bounds, which may cause crashes.
Suggested fix: Use safe access.
| state.savingsProductTemplate?.chargeOptions?.get(state.chooseChargeIndex)?.name ?: "" | |
| state.savingsProductTemplate?.chargeOptions?.getOrNull(state.chooseChargeIndex)?.name ?: "" |
| chargeCollectedOn = if (state.chooseChargeIndex == -1) { | ||
| "" | ||
| } else { | ||
| state.savingsProductTemplate?.chargeOptions?.get(state.chooseChargeIndex)?.chargeTimeType?.value ?: "" |
| import com.mifos.core.model.objects.account.loan.Currency | ||
| import com.mifos.core.model.objects.account.saving.FieldOfficerOptions | ||
| import com.mifos.core.model.objects.commonfiles.InterestType | ||
| import com.mifos.core.model.objects.template.loan.ChargeOptions |
There was a problem hiding this comment.
Imports loan.ChargeOptions in a savings-related class. This is semantically incorrect and may cause confusion or issues later.
Suggested fix:
Verify if client.ChargeOptions or a savings-specific version should be used instead.
| input.count { it == '.' } > 1 -> Res.string.error_invalid_number | ||
| input.any { !it.isDigit() && it != '.' } -> Res.string.error_digits_only | ||
| input.toDoubleOrNull() == null -> Res.string.error_invalid_number | ||
| input.toDouble() == 0.0 -> Res.string.error_number_zero |
There was a problem hiding this comment.
Direct floating-point equality check (input.toDouble() == 0.0) may cause precision problems.
Suggested fix:
Compare as strings or safely parse as shown below:
input == "0" || input == "0.0" || input.toDoubleOrNull() == 0.0There was a problem hiding this comment.
doubleNumberValidator treats blank input as valid while numberValidator flags it as an error. This inconsistency can confuse developers.
Suggested fix:
Add a required: Boolean flag or clarify the intended behavior through documentation.
Fixes - Jira-#538
Screen Recording:
chargePage.mp4
After Update:
Screen_recording_20251004_184531.webm
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.