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
2 times, most recently
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...
There was a problem hiding this comment.
Fixed. The silent return for zero-amount charges has been removed. A validation check is added in LoanChargeService.addLoanCharge(). If the calculated charge amount becomes zero after applying rounding rules, a LoanChargeCannotBeAddedException is now raised with the message error.msg.loanCharge.cannot.be.added.as.amount.rounded.to.zero. This prevents the charge from being created and returns an explicit validation error to the client instead of silently ignoring the request.
| 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...
There was a problem hiding this comment.
You're right. I originally reused minimumAndMaximumCap() because percentage-based charges already passed through that path and received currency-based rounding there. However, that couples rounding with cap enforcement. I've removed the call from the flat charge flow and now perform the rounding directly during flat charge amount calculation.
| @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...
There was a problem hiding this comment.
The reason for this change is that I needed to add integration test coverage for the Share Account charge rounding flow introduced in this PR. I intentionally used the generated Fineract client APIs instead of the deprecated RestAssured helpers, since the project is gradually moving toward client-based integration tests.
While implementing the Share Account tests, I found that PostAccountsTypeAccountIdRequest was incomplete in the generated client because the OpenAPI schema only exposed requestedShares. However, share account commands used by the tests (activate, redeemshares, etc.) require fields such as locale, dateFormat, activatedDate, and requestedDate. Without exposing those fields in the OpenAPI definition, the generated client model could not be used to implement these test flows.
So the schema update was made to enable the ShareAccountChargeRoundingTest that covers the rounding changes, rather than as an independent Swagger cleanup.
That said, if you'd prefer this to be split into a separate PR, I'm happy to do that.
|
|
||
| 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...
There was a problem hiding this comment.
Done! I removed the command parameter, and now the due date is provided as a parameter.
| } | ||
|
|
||
| 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...
There was a problem hiding this comment.
Done! I have used of Money instead of BigDecimal here in the latest changes.
| 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...
There was a problem hiding this comment.
Yes, it is incorrect, and I have made the change to throw now a PlatformApiDataValidationException when the final charge amount rounds to 0.
| 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...
There was a problem hiding this comment.
Reverted. These changes were not required for the final solution and have been removed.
|
|
||
| // 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; |
There was a problem hiding this comment.
Reverted. I was understanding things incorrectly here. So not needed.
| } | ||
| 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 |
There was a problem hiding this comment.
Reverted. This was not needed.
| .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.
Good point. I updated the implementation to prevent zero-amount savings charges from being persisted when the final rounded amount is known at charge attachment time.
After the charge amount is rounded according to the savings product currency configuration (digitsAfterDecimal/inMultiplesOf), I validate the resulting amount before persistence. If the rounded amount becomes zero, a validation error is returned, and the charge is not added.
This validation is currently applied only to flat savings charges because percentage-based charges (for eg withdrawal fees) are calculated later during transaction processing, so the final charge amount is not available when the charge is attached.
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:
|
I dont think we should reject and throw error if charge became 0... @bharathcgowda thoughts? |
Dhanno98
force-pushed
the
FINERACT-2284/enforce-charge-rounding
branch
from
8009f8c to
84f7719
Compare
Dhanno98
force-pushed
the
FINERACT-2284/enforce-charge-rounding
branch
from
84f7719 to
8a82b79
Compare
Thanks @adamsaghy. The current implementation in this PR already follows that behavior for Share Redeem Charges and Savings Percentage Withdrawal Charges. These charges continue to be attached and stored as configured. When they are later applied, the calculated charge amount goes through the existing So Share Flat Redeem Charges that eventually round to 0 during the Redeem Shares flow are currently not rejected during attachment. I've also updated the PR with the latest changes and fixes. |
Dhanno98
force-pushed
the
FINERACT-2284/enforce-charge-rounding
branch
from
8a82b79 to
1bcae1a
Compare
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.
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.