You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@fineract.apache.org by "Samer-Melhem-FOO (via GitHub)" <gi...@apache.org> on 2026/07/28 13:53:29 UTC
[PR] FINERACT-2721: Fix loan collateral quantity adjustment and modify-loan change detection [fineract]
Samer-Melhem-FOO opened a new pull request, #6196:
URL: https://github.com/apache/fineract/pull/6196
## Description
Two related defects in loan collateral handling (`LoanCollateralAssembler` / `LoanAssemblerImpl`):
1. **Repeated quantity deduction.** `LoanCollateralAssembler.fromParsedJson(...)` deducted client collateral quantity on every call, including from read-only paths (loan application validation, schedule preview), not just actual loan submission/update. Since the same parsing method is reused across both, a client's available collateral quantity could be decremented more than once for the same loan, and submitting a loan with linked collateral could fail with an "invalid amount of collateral quantity" error even with sufficient collateral available.
2. **Inverted change detection on loan modification.** In `LoanAssemblerImpl`, the check that decides whether collateral changed on a Modify Loan Application request was inverted:
```java
possiblyModifedLoanCollateralItems.equals(loan.getLoanCollateralManagements())
```
This recorded a "collateral changed" entry (and triggered `loan.updateLoanCollateral(...)`) only when the new and existing collateral sets were **equal** — i.e. when nothing changed — and skipped it when they genuinely differed. Real collateral changes on loan modification were silently dropped.
**Fix:** `LoanCollateralAssembler.fromParsedJson` now takes an explicit `adjustClientCollateralQuantity` flag, defaulting to `false` for read-only/validation callers (existing single-arg overload preserved) and `true` only for actual loan submission/update. The modify-loan condition is flipped to `!equals(...)` so genuine changes are detected and persisted.
JIRA: https://issues.apache.org/jira/browse/FINERACT-2721
## Checklist
- [x] Write the commit message as per [our guidelines](https://github.com/apache/fineract/blob/develop/CONTRIBUTING.md#pull-requests)
- [x] 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.
- [x] 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
- [x] This PR must not be a "code dump". Large changes can be made in a branch, with assistance.
- [ ] If merging this PR resolves a JIRA issue, I will mark that issue as resolved and set "Fix Version/s" appropriately.
Note: no existing unit/integration test covers `LoanCollateralAssembler`/`LoanAssemblerImpl`'s collateral-handling logic, so none were added — happy to add coverage if a reviewer points to the right harness (loan submission with collateral is normally exercised via integration 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
Re: [PR] FINERACT-2721: Fix loan collateral quantity adjustment and modify-loan change detection [fineract]
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on PR #6196:
URL: https://github.com/apache/fineract/pull/6196#issuecomment-5526727981
@Samer-Melhem-FOO Please rebase this PR
--
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-2721: Fix loan collateral quantity adjustment and modify-loan change detection [fineract]
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on PR #6196:
URL: https://github.com/apache/fineract/pull/6196#issuecomment-6417470980
@Samer-Melhem-FOO Are you still working on this?
--
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-2721: Fix loan collateral quantity adjustment and modify-loan change detection [fineract]
Posted by "Samer-Melhem-FOO (via GitHub)" <gi...@apache.org>.
Samer-Melhem-FOO commented on PR #6196:
URL: https://github.com/apache/fineract/pull/6196#issuecomment-5890343313
@adamsaghy Rebased onto the latest develop and fixed the failing collateral test.
The test was failing because modifying a loan's collateral still didn't persist: the modify path replaced the loan's collateral rows with detached copies, so a changed quantity was saved as a second row next to the old one. The loan now updates the matching row in place. Resubmitting an unchanged quantity also no longer changes the client's remaining collateral.
The three collateral tests are ported to the Feign client, and they now check both the loan's collateral and the client's remaining quantity. For that, the loan collateral swagger DTOs declare the `id` field the API already returns and accepts. All of `ClientLoanIntegrationTest` passes locally against PostgreSQL. Please take another look.
--
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-2721: Fix loan collateral quantity adjustment and modify-loan change detection [fineract]
Posted by "Samer-Melhem-FOO (via GitHub)" <gi...@apache.org>.
Samer-Melhem-FOO commented on PR #6196:
URL: https://github.com/apache/fineract/pull/6196#issuecomment-5143738283
Pushed a rebase onto latest `develop` plus integration test coverage for the collateral quantity fix, per the review request above.
CI is green except for **MariaDB Tests (Shard 7 of 15)**, which failed before any test code ran, unrelated to this PR:
```
Could not download org.jacoco.agent-0.8.14.jar
Received status code 429 from server: Too Many Requests
```
That's Maven Central rate-limiting the JaCoCo agent download, which took down every module's `:test` task in that shard (`fineract-core`, `fineract-provider`, `integration-tests`, etc.) at dependency resolution, before any tests executed. All other 14 MariaDB shards, all PostgreSQL/MySQL shards, and all E2E shards passed. Job log: https://github.com/apache/fineract/actions/runs/30627828308/job/91149034361
Would appreciate a re-run of that job when convenient.
--
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-2721: Fix loan collateral quantity adjustment and modify-loan change detection [fineract]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #6196:
URL: https://github.com/apache/fineract/pull/6196#issuecomment-5472291936
This pull request seems to be stale. Are you still planning to work on it? We will automatically close it in 30 days.
--
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-2721: Fix loan collateral quantity adjustment and modify-loan change detection [fineract]
Posted by "Samer-Melhem-FOO (via GitHub)" <gi...@apache.org>.
Samer-Melhem-FOO commented on PR #6196:
URL: https://github.com/apache/fineract/pull/6196#issuecomment-5117000900
The only failing check here (`MariaDB Tests (Shard 14 of 15)`) is `CobPartitioningTest.testLoanCOBPartitioningQuery()` failing with `Expected status code <200> but was <403>` — a permission/auth assertion in the Close-of-Business job partitioning test suite. This PR only touches `LoanCollateralAssembler` and `LoanAssemblerImpl` (collateral quantity handling), and doesn't touch permissions, job scheduling, or COB logic at all, so this looks like pre-existing test flakiness rather than something introduced here.
Would appreciate a re-run of that job when convenient — happy to investigate further if it fails again on a fresh run.
--
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-2721: Fix loan collateral quantity adjustment and modify-loan change detection [fineract]
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on PR #6196:
URL: https://github.com/apache/fineract/pull/6196#issuecomment-5954100674
The gap: removing collateral doesn't return quantity to the client
The Javadoc on mergeLoanCollateral says it removes any existing row that isn't in the new set. Here is what happens on modify:
The request sends collateral: [], or leaves out an existing row's id.
The assembler returns a set without that row.
!equals is true, so changes records collateral as changed.
mergeLoanCollateral drops the row from the loan, and orphanRemoval deletes it from the database.
Nothing adds that row's quantity back to the client.
Other paths do return it: the dedicated delete endpoint (LoanCollateralManagementWriteServiceImpl:45) and releaseAttachedCollaterals (LoanApplicationWritePlatformServiceJpaRepositoryImpl:862).
Replacing a row is worse. If the request sends a new item without an id in place of an existing one, the new quantity is taken from the client, the old row is deleted, and the old quantity is never returned. Before this PR the loan's collateral never changed on modify, so the PR is what makes this reachable.
The fix is to return the quantity to the client for each row mergeLoanCollateral removes, either there or in the assembler. A test that modifies the loan with collateral: [] and checks the client is back at 100 would cover it.
Smaller mismatch
The PR description still says no tests were added and doesn't mention the Loan.mergeLoanCollateral and Swagger changes. Both came in later commits, and the description should be updated to match.
--
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