You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@fineract.apache.org by "SamaSVM (via GitHub)" <gi...@apache.org> on 2026/09/10 09:58:10 UTC
[PR] FINERACT-1554: Snapshot overdue charge amount on loan to prevent retroactive product changes [fineract]
SamaSVM opened a new pull request, #6426:
URL: https://github.com/apache/fineract/pull/6426
## Description
The overdue-installment charge amount was always read directly from the loan product. As a result, editing the charge amount on the product retroactively changed the penalties for loans where the charge had already been applied.
To prevent this, capture the charge amount on the loan at creation time and use this snapshot as the source of truth in both the overdue-penalty scheduler job and the Close of Business business step. Fall back to the current product value only when no snapshot exists, for example, for loans created before this change or loans that had no overdue charge configured at creation time.
## Checklist
Please make sure these boxes are checked before submitting your pull request - thanks!
- [ ] Write the commit message as per [our guidelines](https://github.com/apache/fineract/blob/develop/CONTRIBUTING.md#pull-requests)
- [ ] Acknowledge that we will not review PRs that are not passing the build _("green")_ - it is your responsibility to get a proposed PR to pass the build, not primarily the project's maintainers.
- [ ] Create/update [unit or integration tests](https://fineract.apache.org/docs/current/#_testing) for verifying the changes made.
- [ ] Follow our [coding conventions](https://cwiki.apache.org/confluence/display/FINERACT/Coding+Conventions).
- [ ] Add required Swagger annotation and update API documentation at fineract-provider/src/main/resources/static/legacy-docs/apiLive.htm with details of any API changes
- [ ] [This PR must not be a "code dump"](https://cwiki.apache.org/confluence/display/FINERACT/Pull+Request+Size+Limit). Large changes can be made in a branch, with assistance. Ask for help on the [developer mailing list](https://fineract.apache.org/#contribute).
- [ ] If merging this PR resolves a JIRA issue, I will mark that issue as resolved and set "Fix Version/s" appropriately.
- [ ] I followed the [AI Policy](https://github.com/apache/fineract/blob/develop/CONTRIBUTING.md#ai-policy).
Your assigned reviewer(s) will follow our [guidelines for code reviews](https://cwiki.apache.org/confluence/display/FINERACT/Code+Review+Guide).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-1554: Snapshot overdue charge amount on loan to prevent retroactive product changes [fineract]
Posted by "adamsaghy (via GitHub) via issues" <is...@fineract.apache.org>.
adamsaghy commented on PR #6426:
URL: https://github.com/apache/fineract/pull/6426#issuecomment-6611046313
@SamaSVM Please review my concerns
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-1554: Snapshot overdue charge amount on loan to prevent retroactive product changes [fineract]
Posted by "SamaSVM (via GitHub) via issues" <is...@fineract.apache.org>.
SamaSVM commented on PR #6426:
URL: https://github.com/apache/fineract/pull/6426#issuecomment-6578139490
@galovics Please, check it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-1554: Snapshot overdue charge amount on loan to prevent retroactive product changes [fineract]
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on PR #6426:
URL: https://github.com/apache/fineract/pull/6426#issuecomment-6502335502
Thanks @SamaSVM, all three points are addressed: the test is on Feign and CI is green, the calculation type is snapshotted and carried through both the job and COB paths, and the backfill covers all statuses. Two small follow-ups, neither blocking:
1. The 1037 backfill now runs one unbatched `UPDATE` over all of `m_loan` again (the lock concern from the first review comes back with the wider scope). Please mention this in the description so operators of large tenants know to expect it.
2. The changeset comment says lowest-id selection is consistent with `LoanProduct#getOverdueInstallmentCharge()`, but `LoanProduct.charges` has no `@OrderBy`, so `findFirst()` isn't guaranteed to agree. Either add `@OrderBy("id")` / sort by id, or drop that sentence.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-1554: Snapshot overdue charge amount on loan to prevent retroactive product changes [fineract]
Posted by "adamsaghy (via GitHub) via issues" <is...@fineract.apache.org>.
adamsaghy commented on PR #6426:
URL: https://github.com/apache/fineract/pull/6426#issuecomment-6632574726
Thanks @SamaSVM, both points are addressed: the Javadoc and the description now cover the job/COB asymmetry, and `getOverdueInstallmentCharge()` uses `min(id)`, with a test that would catch the ordering. Remaining:
1. Please rebase. The PR conflicts with `develop` in `LoanProductTest.java`; both sides added tests at the end of the class, so keep both.
2. Nit, non-blocking: `LoanReadPlatformServiceImpl#retrieveAllOverdueInstallmentsForLoan` still selects the charge with its own `findFirst()`. It matches today only because of `@OrderBy("id")`. Calling `loan.getLoanProduct().getOverdueInstallmentCharge()` there would keep one selection rule.
Once rebased and CI is green, LGTM.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-1554: Snapshot overdue charge amount on loan to prevent retroactive product changes [fineract]
Posted by "adamsaghy (via GitHub) via issues" <is...@fineract.apache.org>.
adamsaghy commented on PR #6426:
URL: https://github.com/apache/fineract/pull/6426#issuecomment-6633394613
```
> Compilation failed; see the compiler output below.
For more on this, please refer to https://docs.gradle.org/9.7.1/userguide/command_line_interface.html#sec:command_line_warnings in the Gradle documentation.
Error: /home/runner/work/fineract/fineract/integration-tests/src/test/java/org/apache/fineract/integrationtests/LoanOverdueChargeSnapshotTest.java:45: error: cannot find symbol
324 actionable tasks: 192 executed, 74 from cache, 58 up-to-date
import org.apache.fineract.integrationtests.common.BusinessStepHelper;
^
symbol: class BusinessStepHelper
location: package org.apache.fineract.integrationtests.common
10 errors
52 warnings
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-1554: Snapshot overdue charge amount on loan to prevent retroactive product changes [fineract]
Posted by "SamaSVM (via GitHub)" <gi...@apache.org>.
SamaSVM commented on PR #6426:
URL: https://github.com/apache/fineract/pull/6426#issuecomment-6443239665
@adamsaghy Done. Thank you for the review!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-1554: Snapshot overdue charge amount on loan to prevent retroactive product changes [fineract]
Posted by "adamsaghy (via GitHub) via issues" <is...@fineract.apache.org>.
adamsaghy commented on PR #6426:
URL: https://github.com/apache/fineract/pull/6426#issuecomment-6658862804
Thanks @SamaSVM, the rebase is in, the compile error is gone (`FeignBusinessStepHelper` instead of `BusinessStepHelper`) and CI is green. One thing still needs a change:
1. **`LoanOverdueChargeSnapshotTest` leaves the COB steps changed for the tests that run after it.** Both COB tests (lines 126 and 203) call `updateSteps("LOAN_CLOSE_OF_BUSINESS", "APPLY_CHARGE_TO_OVERDUE_LOANS")` and nothing puts the previous steps back; `tearDown` only resets the global configurations. Around 40 integration test classes run COB without configuring the steps themselves (e.g. `DelinquencyActionIntegrationTests`), so whichever of them runs after this class on the same server gets a COB that skips delinquency classification, accruals and the rest. That makes failures depend on test order. Please read the configured steps before changing them and restore them in a `finally` block, as `LoanWithdrawalTest` does.
2. Nit, still non-blocking: `LoanReadPlatformServiceImpl#retrieveAllOverdueInstallmentsForLoan` (line 1636) still picks the charge with its own `findFirst()`. It gives the same result as `getOverdueInstallmentCharge()` today only because of `@OrderBy("id")`.
With 1 fixed, LGTM.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-1554: Snapshot overdue charge amount on loan to prevent retroactive product changes [fineract]
Posted by "SamaSVM (via GitHub) via issues" <is...@fineract.apache.org>.
SamaSVM commented on PR #6426:
URL: https://github.com/apache/fineract/pull/6426#issuecomment-6632157631
@adamsaghy Done.
**1. Javadoc / job-COB asymmetry**
Fixed both, as suggested:
- Rewrote the Javadoc for `LoanProduct#getOverdueInstallmentCharge()` to describe its behavior precisely (returns the overdue-installment charge with the lowest ID) and document the actual difference: the Close of Business step applies a single charge per loan, whereas the "Apply penalty to overdue loans" job returns one row per overdue-installment charge defined on the product and applies all of them through `ApplyChargeToOverdueLoanInstallmentTasklet`. When a product has multiple such charges, the job still applies the remaining charges using their current product-level amounts and calculation types; only the lowest-ID charge is covered by the snapshot.
- Added the same explanation under a "Known limitation" paragraph in the PR description, clarifying that this behavior predates the PR and remains unchanged.
**2. Determinism test**
You're right that the test passed without `@OrderBy` because the list was already sorted. Instead of simply renaming it, I moved the ID-ordering logic into `getOverdueInstallmentCharge()` itself, replacing `.findFirst()` with `.min(Comparator.comparing(Charge::getId))`.
Determinism is now guaranteed by the method itself rather than being a side effect of the collection's load order. The `@OrderBy` annotation on `LoanProduct.charges` remains in place for collections loaded from the database.
I also renamed the test to `testGetOverdueInstallmentChargeReturnsTheLowestIdWhenProductHasSeveralOverdueCharges` and changed the input order to provide the charges in reverse ID order, ensuring the test fails without the fix.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-1554: Snapshot overdue charge amount on loan to prevent retroactive product changes [fineract]
Posted by "adamsaghy (via GitHub) via issues" <is...@fineract.apache.org>.
adamsaghy commented on PR #6426:
URL: https://github.com/apache/fineract/pull/6426#issuecomment-6608761533
Thanks @SamaSVM, both follow-ups are in: `@OrderBy("id")` on `LoanProduct.charges` makes `getOverdueInstallmentCharge()` agree with the migration, the description now warns about the single unbatched `UPDATE` on `m_loan`, and the changeset is renumbered to `1038`. Nothing blocking from my side once CI is green. Two small notes:
1. The Javadoc on `LoanProduct#getOverdueInstallmentCharge()` says both `applyOverdueChargesForLoan` and `retrieveAllOverdueInstallmentsForLoan` "pick the first match". That holds for the COB path, but the "Apply penalty to overdue loans" job does not: its SQL returns one row per overdue charge on the product, and `ApplyChargeToOverdueLoanInstallmentTasklet` applies all of them. So on a product with more than one overdue-installment charge, the job still applies the other charges at the live product values; the snapshot only covers the lowest-id one. This job/COB difference predates the PR, but please either fix the Javadoc or mention it in the description as a known limitation.
2. `testGetOverdueInstallmentChargeIsDeterministicWhenProductHasSeveralOverdueCharges` passes in a list that is already sorted by id, so it would pass without `@OrderBy` too. Fine as a test of `findFirst`, but the name suggests it covers the ordering.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-1554: Snapshot overdue charge amount on loan to prevent retroactive product changes [fineract]
Posted by "SamaSVM (via GitHub)" <gi...@apache.org>.
SamaSVM commented on PR #6426:
URL: https://github.com/apache/fineract/pull/6426#issuecomment-6291411670
@galovics Thank you for your review.
**Snapshot incompleteness (flat ↔ percentage, charge swap)**
Confirmed. `LoanChargeAssembler.createNewFromJson()` takes `amount` from the snapshot but `ChargeCalculationType` from the live `chargeDefinition`. On the read side, the `COALESCE`/`findFirst()` logic was also joined against the *current* product charge. As a result, when a charge was swapped, the new charge ID could be paired with the old snapshot amount.
Fixed by snapshotting the charge identity alongside the amount:
- Added `m_loan.overdue_charge_id` next to `overdue_charge_amount`.
- `Loan.getOverdueChargeAmountOrDefault(Charge)` now uses the snapshot only when `overdueChargeId` still matches the charge being applied. Otherwise, it falls back to the live charge amount.
- Applied this logic in both `LoanReadPlatformServiceImpl#retrieveAllOverdueInstallmentsForLoan` and the SQL mapper:
`COALESCE(CASE WHEN ml.overdue_charge_id = mc.id THEN ml.overdue_charge_amount END, mc.amount)`.
- Added an integration test, `overdueChargeSwappedInProductFallsBackToNewChargeAmount`, covering the swap case: the stale snapshot is ignored and the new charge's amount is used.
**Migration (1036)**
Reworked the backfill:
- Scoped it to `loan_status_id = 300` (ACTIVE) only. Overdue charges are never applied to closed/written-off loans anyway: both `retrieveAllOverdueInstallmentsForLoan` and the scheduled job already filter on active loans, so there is no reason to lock every row.
- Rewrote the comment to describe the actual behavior: the migration backfills *active* loans using the product's *current* charge. Historical amounts cannot be recovered, so existing loans are pinned to today's amount rather than the amount at loan creation time. This also means the fallback path only matters for loans whose product has no overdue charge, or where the charge is swapped later.
- Prefixed changeset IDs with `1036-1`/`1036-2` to match the convention used in neighboring files (e.g. `1029-*`).
- Added the missing `charge_applies_to_enum = 1` filter to match the runtime query.
- Added a comment for `charge_time_enum = 9` (`ChargeTimeType.OVERDUE_INSTALLMENT`).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-1554: Snapshot overdue charge amount on loan to prevent retroactive product changes [fineract]
Posted by "SamaSVM (via GitHub) via issues" <is...@fineract.apache.org>.
SamaSVM commented on PR #6426:
URL: https://github.com/apache/fineract/pull/6426#issuecomment-6659358685
@adamsaghy, please rerun tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org