You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@fineract.apache.org by "alberto-art3ch (via GitHub)" <gi...@apache.org> on 2026/10/06 03:21:01 UTC
[PR] FNERACT-2684: Working Capital undo disbursal clears schedules and action [fineract]
alberto-art3ch opened a new pull request, #6586:
URL: https://github.com/apache/fineract/pull/6586
## Description
Undoing a Working Capital loan disbursement left the breach schedule, the delinquency range schedule (and its tags), the breach/delinquency/near-breach actions and the period payment rate changes in place, while `hasSchedule()` blocked their regeneration on a later disbursement. The loan went back to `APPROVED` carrying periods anchored on the undone disbursement date (see E2E scenario C106711, previously `@Skip`).
Undo approval now also deletes the approval-time amortization model and projects a new one from the submitted values, as the documentation already stated.
[FINERACT-2684](https://issues.apache.org/jira/browse/FINERACT-2684)
## 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-2684: WC undo disbursal reverses rate changes, undo approval drops projection [fineract]
Posted by "alberto-art3ch (via GitHub)" <gi...@apache.org>.
alberto-art3ch commented on PR #6586:
URL: https://github.com/apache/fineract/pull/6586#issuecomment-6157845964
> @alberto-art3ch Please check my below concerns:
>
> 1. **C106711 is not un-skipped.** The description says C106711 is no longer skipped, but the diff doesn't touch it. On `develop`, C106711 is an unrelated, non-skipped scenario ("delinquency Id is overridable… on modify loan account – UC2" in `WorkingCapitalDelinquency.feature`), and no skip tag is removed anywhere in the PR. The ID is probably wrong; the new scenarios are C111070–C111080.
> 2. **C111077's rate-change check doesn't test undo approval.** In C111077 the rate change is reversed by the undo disbursal step. Undo approval can't have a rate change to reverse, because rate changes are only allowed on open loans. So C111077 doesn't cover any rate-change behaviour that belongs to undo approval, despite what the description says.
> 3. **Title doesn't match the change.** "Working Capital undo disbursal clears schedules and action" describes behaviour that, per the description, was already on `develop`. This PR adds rate-change reversal on undo disbursal and projection deletion on undo approval.
> 4. **The description leaves out about half the diff.** Not mentioned: about nine more E2E scenarios (batch API undo, breach/near-breach/delinquency overrides on modify after undo disbursement), renamed and split step definitions, and the adoc updates.
Thanks @adamsaghy — all four addressed: dropped the wrong C106711 reference, removed the rate change from C111077 (C111076 already covers the reversal) and retitled it to what undo approval does, and rewrote the title and description to cover the full diff.
--
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-2684: WC undo disbursal reverses rate changes, undo approval drops projection [fineract]
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy merged PR #6586:
URL: https://github.com/apache/fineract/pull/6586
--
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-2684: Working Capital undo disbursal clears schedules and action [fineract]
Posted by "alberto-art3ch (via GitHub)" <gi...@apache.org>.
alberto-art3ch commented on code in PR #6586:
URL: https://github.com/apache/fineract/pull/6586#discussion_r4261502755
##########
fineract-e2e-tests-core/src/test/java/org/apache/fineract/test/stepdef/loan/WorkingCapitalLoanAccountStepDef.java:
##########
@@ -3410,6 +3474,15 @@ public void adminChecksWorkingCapitalPeriodPaymentRateChangesHistoryByExternalId
checkPeriodPaymentRateChangeHistory(data, rateChangesResponse, header, resourceId);
}
+ @Then("Working Capital Loan Period Payment Rate changes history is empty")
+ public void periodPaymentRateChangesHistoryIsEmpty() {
Review Comment:
Removed
--
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-2684: Working Capital undo disbursal clears schedules and action [fineract]
Posted by "alberto-art3ch (via GitHub)" <gi...@apache.org>.
alberto-art3ch commented on PR #6586:
URL: https://github.com/apache/fineract/pull/6586#issuecomment-6066743645
> ## Review summary
> Hi @alberto-art3ch, thanks for the PR. The undo disbursal part looks good, and it's nice that C106711 isn't skipped anymore. I have two questions about the code, plus a few things that need fixing before we can merge.
>
> Blocking:
>
> * `@TestRailId:C111080` is on two different new scenarios (`WorkingCapitalBatchApi.feature:490` and `WorkingCapitalNearBreachEvaluation.feature:1302`), so the results will overwrite each other.
> * The PR has two commits (`8a65dc5728` and `f2a9d71c53`), please squash into one.
> * The undo approval e2e check (C111077, `netDisbursementAmount = 9000.00`) can't fail on this change: approved and proposed principal are both 9000 and discount is 0, so the approval-time projection passes too. Use different approved and proposed amounts, or a non-zero proposed discount.
> * Also the description says this PR adds deleting the schedules and actions on undo disbursal, but that's already on develop (`resetToApprovedState`). The real changes are the rate change delete and the re-projection on undo approval, can you make the description say that?
>
> Smaller: a leftover commented-out step at `WorkingCapitalLoanAccount.feature:1030`, five near-identical "modify loan with X override data" step defs that could share a helper, a few unrelated renames, a redundant business date step in C111080, and the new unit test at `WorkingCapitalLoanAmortizationScheduleWriteServiceImplTest:195` only exercises fallback logic that was already there.
>
> Recommendation: REQUEST_CHANGES
>
> ## Inline comments
> ### `fineract-working-capital-loan/src/main/java/org/apache/fineract/portfolio/workingcapitalloan/service/WorkingCapitalLoanWritePlatformServiceImpl.java:253`
> ```java
> this.loanRepository.saveAndFlush(loan);
>
> + // The approval-time projection is overwritten by one made from the submitted values, now that the loan is back
> + // in submitted status: with the approved principal zeroed, the projection falls back to the expected amount.
> + this.amortizationScheduleWriteService.generateAndSaveAmortizationScheduleOnApproval(loan);
> ```
>
> > I'm not convinced this is the right thing to do. A freshly submitted loan doesn't have a projected schedule at all, `submitApplication` never writes one, and `modifyApplication` never refreshes it. So after undo approval we now persist a schedule for a SUBMITTED loan that nobody keeps in sync. If the user modifies the principal/expected disbursement date/rate (which is the whole point of undoing the approval), the stored projection stays stale until the next approval overwrites it.
> > Plus this can now fail on undo approval (`Validate.notNull`/`isTrue` give a 500, the strategy/calculability checks a 400) where it always succeeded before. Can submission validation guarantee the submitted values are calculable?
> > Wouldn't it make more sense to just delete the model, like `deleteApplication` does with `projectedAmortizationLoanModelRepository.findByLoanId(...).ifPresent(...::delete)`? That puts the loan back to the same state as a never approved one, and it also matches what the docs say ("Deletes the approval-time schedule"), because right now the code doesn't delete anything, it overwrites.
>
> ### `fineract-working-capital-loan/src/main/java/org/apache/fineract/portfolio/workingcapitalloan/service/WorkingCapitalLoanWritePlatformServiceImpl.java:1494`
> ```java
> delinquencyRangeScheduleService.deleteScheduleAndActions(loan.getId());
> breachScheduleService.deleteScheduleAndActions(loan.getId());
> + rateChangeRepository.deleteByWorkingCapitalLoanId(loan.getId());
> deactivateCharges(loan);
> ```
>
> > Why a hard delete here? `WorkingCapitalLoanPeriodPaymentRateChange` already has a `reversed` flag + `reverse(LocalDate)`, and every reader that matters (schedule rebuild, validator, `rateInEffectAt`) goes through `findByWorkingCapitalLoanIdAndReversedFalse`, so reversing them with the business date would give the same result. These were user-entered commands with notes and we published `WorkingCapitalLoanPeriodPaymentRateChangedBusinessEvent` for each of them, so wiping the rows means downstream consumers have events pointing at IDs that don't exist anymore and we lose the audit trail. The transactions in the same undo are reversed, not deleted, so I'd keep this consistent with them. What do you think?
Thanks @galovics, both code comments applied as suggested. Duplicate TestRailId, leftover commented step, redundant business date step and the unit test at `WorkingCapitalLoanAmortizationScheduleWriteServiceImplTest:195` are fixed too.
Title and description now describe the actual change (rate change reversal on undo disbursal, projection deletion on undo approval). Kept as two commits, development and E2E, per the team convention.
--
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-2684: Working Capital undo disbursal clears schedules and action [fineract]
Posted by "mariiaKraievska (via GitHub)" <gi...@apache.org>.
mariiaKraievska commented on PR #6586:
URL: https://github.com/apache/fineract/pull/6586#issuecomment-6123250346
Branch is approximately 50 commits behind develop — please rebase and re-run the pipeline before merge
--
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-2684: Working Capital undo disbursal clears schedules and action [fineract]
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on PR #6586:
URL: https://github.com/apache/fineract/pull/6586#issuecomment-6148189749
@alberto-art3ch Please check my below concerns:
1. **C106711 is not un-skipped.** The description says C106711 is no longer skipped, but the diff doesn't touch it. On `develop`, C106711 is an unrelated, non-skipped scenario ("delinquency Id is overridable… on modify loan account – UC2" in `WorkingCapitalDelinquency.feature`), and no skip tag is removed anywhere in the PR. The ID is probably wrong; the new scenarios are C111070–C111080.
2. **C111077's rate-change check doesn't test undo approval.** In C111077 the rate change is reversed by the undo disbursal step. Undo approval can't have a rate change to reverse, because rate changes are only allowed on open loans. So C111077 doesn't cover any rate-change behaviour that belongs to undo approval, despite what the description says.
3. **Title doesn't match the change.** "Working Capital undo disbursal clears schedules and action" describes behaviour that, per the description, was already on `develop`. This PR adds rate-change reversal on undo disbursal and projection deletion on undo approval.
4. **The description leaves out about half the diff.** Not mentioned: about nine more E2E scenarios (batch API undo, breach/near-breach/delinquency overrides on modify after undo disbursement), renamed and split step definitions, and the adoc updates.
--
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-2684: Working Capital undo disbursal clears schedules and action [fineract]
Posted by "mariiaKraievska (via GitHub)" <gi...@apache.org>.
mariiaKraievska commented on code in PR #6586:
URL: https://github.com/apache/fineract/pull/6586#discussion_r4247413658
##########
fineract-e2e-tests-core/src/test/java/org/apache/fineract/test/stepdef/loan/WorkingCapitalLoanAccountStepDef.java:
##########
@@ -3410,6 +3474,15 @@ public void adminChecksWorkingCapitalPeriodPaymentRateChangesHistoryByExternalId
checkPeriodPaymentRateChangeHistory(data, rateChangesResponse, header, resourceId);
}
+ @Then("Working Capital Loan Period Payment Rate changes history is empty")
+ public void periodPaymentRateChangesHistoryIsEmpty() {
Review Comment:
This step isn’t used anywhere. Please drop it (or use it if there’s a case that truly expects empty history).
--
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