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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
