FINERACT-2284: Enforce Charge Rounding using Money rounding rules#5940
FINERACT-2284: Enforce Charge Rounding using Money rounding rules#5940Dhanno98 wants to merge 1 commit into
Conversation
Dhanno98
force-pushed
the
FINERACT-2284/enforce-charge-rounding
branch
from
f44e89e to
38bd72e
Compare
Dhanno98
force-pushed
the
FINERACT-2284/enforce-charge-rounding
branch
from
38bd72e to
8009f8c
Compare
| update(loanCharge, chargeAmt, loanCharge.getDueLocalDate(), amount, loan.fetchNumberOfInstallmentsAfterExceptions(), | ||
| totalChargeAmt); | ||
|
|
||
| // Skip zero-value charges |
There was a problem hiding this comment.
Adding 0 charge is incorrect and it should be stopped at validation and or during charge amount calculation. Silent return is misleading and hiding a potential issue!
There was a problem hiding this comment.
See no changes...no reply...
| numberOfRepayments = loanCharge.getLoan().fetchNumberOfInstallmentsAfterExceptions(); | ||
| } | ||
| loanCharge.setAmount(amount.multiply(BigDecimal.valueOf(numberOfRepayments))); | ||
| loanCharge.setAmount(loanCharge.minimumAndMaximumCap(amount.multiply(BigDecimal.valueOf(numberOfRepayments)))); |
There was a problem hiding this comment.
Unrelated change? Why loanCharge.minimumAndMaximumCap is needed here? Charge amount calculation should cover the rounding, not this...
There was a problem hiding this comment.
See no changes...no reply...
| @Schema(example = "en") | ||
| public String locale; | ||
| @Schema(example = "dd MMMM yyyy") | ||
| public String dateFormat; | ||
| @Schema(example = "01 January 2026") | ||
| public String activatedDate; | ||
| @Schema(example = "05 May 2026") | ||
| public String requestedDate; | ||
| @Schema(example = "01 January 2026") | ||
| public String closedDate; | ||
| @Schema(description = "Can represent either number of shares or transaction IDs") | ||
| public Object requestedShares; | ||
|
|
||
| static final class PostAccountsRequestedShares { | ||
|
|
||
| private PostAccountsRequestedShares() {} | ||
|
|
||
| @Schema(example = "35") | ||
| public Long id; | ||
| } | ||
|
|
||
| public Set<PostAccountsRequestedShares> requestedShares; | ||
| } |
There was a problem hiding this comment.
See no changes...no reply...
|
|
||
| public static ClientCharge createNew(final Client client, final Charge charge, final JsonCommand command) { | ||
| BigDecimal amount = command.bigDecimalValueOfParameterNamed(ClientApiConstants.amountParamName); | ||
| public static ClientCharge createNew(final Client client, final Charge charge, final BigDecimal amount, final JsonCommand command) { |
There was a problem hiding this comment.
If you’ve started providing the fields as incoming arguments, don’t stop halfway. Remove the command parameter and ensure that the due date is provided as a parameter.
There was a problem hiding this comment.
See no changes...no reply...
| } | ||
|
|
||
| final ClientCharge clientCharge = ClientCharge.createNew(client, charge, command); | ||
| BigDecimal roundedAmount = calculateRoundedChargeAmount(charge, command); |
There was a problem hiding this comment.
Using Money here would be better, no?
There was a problem hiding this comment.
See no changes...no reply...
| final ClientCharge clientCharge = ClientCharge.createNew(client, charge, command); | ||
| BigDecimal roundedAmount = calculateRoundedChargeAmount(charge, command); | ||
| if (roundedAmount.compareTo(BigDecimal.ZERO) == 0) { | ||
| return new CommandProcessingResultBuilder().withOfficeId(client.getOffice().getId()).withClientId(client.getId()).build(); |
There was a problem hiding this comment.
Why to accept 0 amount charge? 0 amount charge is incorrect outcome!
There was a problem hiding this comment.
See no changes...no reply...
| LoanTrancheDisbursementCharge loanTrancheDisbursementCharge; | ||
| ExternalId externalId = externalIdFactory.createFromCommand(command, "externalId"); | ||
| boolean isFirst = true; | ||
| boolean atLeastOneChargePersisted = false; |
There was a problem hiding this comment.
what are these flags for?
There was a problem hiding this comment.
See no changes...no reply...
|
|
||
| // if charge amount is rounded to zero, then continue and look for another disbursement | ||
| if (isZeroCharge(tempCharge)) { | ||
| continue; |
There was a problem hiding this comment.
0 amount charge sounds incorrect to me.
| } | ||
|
|
||
| // tempCharge was persisted so this is true | ||
| atLeastOneChargePersisted = true; |
| } | ||
| if (loanCharge == null) { | ||
| // No eligible disbursement | ||
| if (!eligibleDisbursementFound) { |
| validateAddLoanCharge(loan, chargeDefinition, loanCharge); | ||
| isAppliedOnBackDate = addCharge(loan, chargeDefinition, loanCharge); | ||
|
|
||
| // if charge amount is rounded to zero, then return early |
There was a problem hiding this comment.
Sounds incorrect result to me.
| if (!isZeroCharge(loanCharge)) { | ||
| loanCharge = this.loanChargeRepository.saveAndFlush(loanCharge); | ||
|
|
||
| // we want to apply charge transactions only for those loans charges that are applied when a loan is active |
| .notifyPostBusinessEvent(new LoanAccrualTransactionCreatedBusinessEvent(applyLoanChargeTransaction)); | ||
|
|
||
| // If charge amount is not zero | ||
| if (!isZeroCharge(loanCharge)) { |
| } | ||
| final SavingsAccountCharge savingsAccountCharge = SavingsAccountCharge.createNewFromJson(savingsAccount, chargeDefinition, command); | ||
|
|
||
| if (isFlatCharge(savingsAccountCharge) && isZeroCharge(savingsAccountCharge, savingsAccount.getCurrency())) { |
There was a problem hiding this comment.
I dont like it... 0 amount charge is incorrect result.
There was a problem hiding this comment.
I stopped review at half time.. I dont like this approach, neither i think it is correct.
The introduced 0 amount handling is incorrect. 0 amount is incorrect outcome and charge cannot be created with this value, but nevertheless, we cannot accept the charge creation action and after not create the charge. This is incorrect and unwanted behaviour:
- Add proper validation (first thing to do)
- Calculate the amount, if it is incorrect (like 0 amount) -> Raise error and explain the issue!
- Remove the unnecessary flags and checks... it makes the code hard to read and maintained...
- Remove the unrelated changes, the PR should focus on 1 thing only!
|
@adamsaghy Thanks for your suggestions! I am working on the fix. |
|
Hi @adamsaghy, I have a question regarding future charges such as Savings Account Percentage of Withdrawal Charges and Share Account Redeem Charges. In these cases, the charge is attached successfully first and the amount is calculated only when the business transaction occurs. If the calculated charge amount becomes 0 after rounding, what is the expected behavior:
Could you please clarify the expected behavior for these future-charge scenarios so I can align the implementation consistently across Savings and Shares? Thank you! |
@bharathc27 fyi @Dhanno98 let me review |
I see no changes, no reply on my questions and concerns.. i have stopped reviewing the PR at half time... |
Hi @adamsaghy, Sorry for the confusion. When I wrote that I had updated the charge attachment flows locally, I meant that the changes were only completed in my local branch and had not yet been pushed or amended into the PR. My intention was to first finish addressing all of your review comments, validate everything thoroughly, and then push a single updated commit together with replies to each review comment. Because of that, I had not yet responded individually to the review feedback. I realize now that my comment made it sound like the PR had already been updated, which was not the case. Sorry for causing you to spend time reopening the review. I am currently working through all of the review comments and will push the updated changes together with responses shortly. Regarding the future-charge scenarios, that question was meant as a design clarification while implementing the remaining changes so I can keep the behavior consistent across Savings and Shares. This is the only thing that I need to understand and resolve and then I will ammend the commit. Thank you for your patience and for the detailed review feedback. |
Thank you for the clarification ;) Regarding zero amount: Lets use option 1 Reject the business transaction (withdrawal/redeem) with a validation error. |
Thank you for the clarification. Just to confirm the expected behavior for percentage-based future charges: Suppose a Savings Account has a "Percentage of Withdrawal" charge configured as 0.5%. When the charge is attached to the account, the percentage value (0.5%) is stored successfully because the actual charge amount is not yet known. Later, if a withdrawal of 100 is performed and the account currency uses 0 decimal places (for example JPY), the calculated charge amount would be: 0.5% × 100 = 0.5 which would then round to 0 according to the currency rounding rules. With option 1, my understanding is that the withdrawal transaction itself should be rejected with a validation error because the calculated charge amount becomes zero after rounding. Is that the intended behavior, even though the withdrawal amount itself is valid and only the associated charge rounds to zero? I just want to confirm this edge case before implementing the validation in the future-charge flows. |
With this use case, it’s not as straightforward as it seems. I might have been overly strict here. Just because the charge is too small, we still want to allow the withdrawal. So, if the calculated charge is zero, we simply ignore it and don’t create it. @bharathc27, what are your thoughts on this? |
|
@adamsaghy @Dhanno98 In any entity and scenario if the calculated charge amount is zero (after rounding) that charge should not be created. |
Hi @bharathcgowda @adamsaghy For percentage-based future charges (withdrawal/redeem), I understand that the charge definition should be stored and, when the calculated amount rounds to zero, the transaction should continue and the charge should simply not be applied. For flat future charges (for example a Shares Flat Redeem Charge of 0.5 in a currency with 0 decimal places), the rounded charge amount is known to be zero already at attachment time and can never become non-zero later. Currently, the existing behavior appears to allow such a Flat Redeem Charge to be attached to the share account without any validation. During the attachment flow, the configured charge amount (0.5 in this example) is stored as-is and no currency rounding is performed. The amount is only passed through For this scenario, what should be the expected behavior:
|
3 participants
Description
FINERACT-2284: Enforce charge rounding using Money rounding rules
This PR fixes charge amount handling across Fineract by ensuring that charge values are rounded using the application's configured currency rules before they are persisted or processed.
Previously, several charge flows stored or processed raw
BigDecimalvalues directly. As a result, charge amounts could bypass currency rounding settings such as:digitsAfterDecimalinMultiplesOfThis led to inconsistent behavior between charge calculations and other monetary operations that already rely on
Money.This PR standardizes charge amount handling by using
Moneyfor charge rounding across all supported charge types:In addition, if a charge amount rounds to zero after applying currency rules, the charge is no longer persisted or created.
Functional Changes
Loan Charges
Savings Account Charges
Money.Share Account Charges
Client Charges
API Documentation
Updated Swagger/OpenAPI schemas where required to improve request model documentation and examples.
Why This Change
Fineract already provides currency-specific monetary rules through
Money, including support for:digitsAfterDecimal)inMultiplesOf)Charges should follow the same monetary rules as all other financial amounts.
By enforcing rounding consistently:
Test Coverage
Added integration test coverage for all supported charge types:
LoanChargeRoundingTestSavingsAccountChargeRoundingTestShareAccountChargeRoundingTestClientChargeRoundingTestThe tests verify:
inMultiplesOfanddigitsAfterDecimalrounding is enforced.Verification
Executed the complete charge rounding integration test suite successfully.
Result: 44/44 tests passing.
Checklist
Please make sure these boxes are checked before submitting your pull request - thanks!
Your assigned reviewer(s) will follow our guidelines for code reviews.