adamsaghy commented on PR #6223:
URL: https://github.com/apache/fineract/pull/6223#issuecomment-5214765907
> > @alberto-art3ch Please kindly see my concerns:
> >
> > 1. Migration 0066 never inserts the permissions. Charge-off's
parts/0063_wc_loan_charge_off.xml:70-88 inserts CHARGEOFF_WORKINGCAPITALLOAN /
UNDOCHARGEOFF_WORKINGCAPITALLOAN into m_permission; 0066_wc_loan_write_off.xml
inserts nothing.
> >
> > **Two consequences:**
> > With the maker-checker global config enabled,
ConfigurationDomainServiceJpa.java:54 does
permissionRepository.findOneByCode("WRITEOFF_WORKINGCAPITALLOAN") and throws
PermissionNotFoundException — every write-off call fails. No role other than a
super-user (ALL_FUNCTIONS) can ever be granted the permission. The new doc
chapter documents both permissions as if they exist. Add the two m_permission
changesets mirroring 0063.
> >
> > 2. writeOffReasonId vs progressive's writeoffReasonId. The AC says
incoming parameters follow progressive loans. Progressive/cumulative use
lower-case writeoffReasonId — LoanTransactionValidatorImpl.java:437 and
LoanWritePlatformServiceJpaRepositoryImpl.java:1500. The PR introduces
writeOffReasonId. Everything else in the set (transactionDate, note, locale,
dateFormat, externalId) matches.
> >
> > Also the field was not added to
PostWorkingCapitalLoanTransactionsRequest in
WorkingCapitalLoanTransactionsApiResourceSwagger.java:192-215, even though
chargeOffReasonId is there (line 209). So the generated client can't send a
write-off reason at all, which is also why no test exercises it. Same for
reversalExternalId on undo — accepted by the validator, absent from the request
model.
> >
> > 3. Write-off of an already charged-off loan double-credits the
receivables. Charge-off keeps the loan ACTIVE and validateWriteOff only
requires ACTIVE, so write-off is reachable straight after a charge-off.
chargeOffPostings already credits LOAN_PORTFOLIO / FEES_RECEIVABLE /
PENALTIES_RECEIVABLE for the full outstanding; the new writeOffPostings credits
them again — the service hardcodes postJournalEntries(..., false) and the
WRITEOFF case ignores isChargedOff entirely. Regular loans branch explicitly at
AccrualBasedAccountingProcessorForLoan.java:1379 into
createJournalEntriesForWriteOffsWhenLoanIsChargedOff (Dr CHARGE_OFF_EXPENSE /
INCOME_FROM_CHARGE_OFF_FEES / _PENALTY, Cr FUND_SOURCE, with
CHARGE_OFF_FRAUD_EXPENSE when the loan is flagged fraud — and
m_wc_loan.is_fraud exists here too).
> >
> > Let's replicate the implementation from the progressive loan handling.
> >
> > 4. The "post-write-off lock" NOTE in the doc is wrong. The chapter says
the lock is "pending confirmation and not yet enforced". It's actually already
enforced by pre-existing status gates: repayment/goodwill/payout-refund via
REPAYMENT_LIKE_TXN_ALLOWED_LOAN_STATUSES
(WorkingCapitalLoanDataValidator.java:136), transaction undo via
validateUndoTransaction (line 958), charges via
WorkingCapitalLoanChargeWritePlatformServiceImpl.java:398, charge-off and
undo-disbursal via their own ACTIVE checks. So the AC is met — but nothing pins
it. Drop the NOTE and add a test or two asserting a repayment and a
transaction-undo are rejected on a written-off loan.
> > 5. No status-changed / balance-changed events, no delinquency reset.
Regular write-off emits LoanBalanceChangedBusinessEvent and calls
setLoanDelinquencyTag; the WC repayment path emits
WorkingCapitalLoanBalanceChangedBusinessEvent and calls
delinquencyRangeScheduleService.reprocessDelinquencySchedule. The write-off
service emits only the two new transaction events. Since CLOSED_WRITTEN_OFF is
excluded from COB (WorkingCapitalLoanRetrieveIdServiceImpl
NON_CLOSED_LOAN_STATUSES), a delinquent loan stays tagged delinquent forever
after write-off, and undo never re-derives it.
> > 6. Null balance is silently tolerated. zeroOutstanding returns early
when balance == null and the service defaults all portions to zero — the loan
closes as written-off with a zero-amount transaction. Charge-off deliberately
throws error.msg.wc.loan.balance.not.found for exactly this case. Make it
consistent.
>
> @adamsaghy all the points are addressed:
>
> 1. **Permissions** — 0066 now inserts `WRITEOFF_WORKINGCAPITALLOAN` and
`UNDOWRITEOFF_WORKINGCAPITALLOAN`, mirroring 0063.
> 2. **Naming** — renamed to `writeoffReasonId` to match progressive, and
both it and `reversalExternalId` are now in
`PostWorkingCapitalLoanTransactionsRequest` (the latter was missing for
`undoChargeOff` too). Unit tests pin the parameter name; an IT sends the reason
and asserts it round-trips.
> 3. **Charged-off write-off** — the service now passes
`loan.isChargedOff()`, and in that case `writeOffPostings` books nothing:
charge-off already credited LOAN_PORTFOLIO / FEES_RECEIVABLE /
PENALTIES_RECEIVABLE for the same portions, so re-crediting drove those assets
negative and booked the loss twice.
> 4. **Doc** — the NOTE is gone, replaced by the actual gate table; tests
now assert a repayment and a transaction undo are rejected on a written-off
loan, and allowed again after the undo write-off.
> 5. **Events / delinquency** — both paths reprocess the delinquency
schedule and publish `BalanceChanged` and `StatusChanged` alongside the
transaction event.
> 6. **Null balance** — now throws `error.msg.wc.loan.balance.not.found` on
write-off and undo, same as charge-off.
>
> One open question on 3 before I align any further with core:
`createJournalEntriesForWriteOffsWhenLoanIsChargedOff` passes
`CHARGE_OFF_EXPENSE` as the **credit** and `FUND_SOURCE` as the **debit**
(`populateCreditDebitMaps(.., creditAccountType, debitAccountType, ..)`,
AccrualBasedAccountingProcessorForLoan.java:1640-1647) — the opposite direction
to the one described in your comment. Net of the charge-off, that reads as
"loan settled in cash, zero loss recognised". Booking nothing seemed the safer
treatment, since the receivables are already off the books and the loss is
already in P&L. Happy to switch to core's version verbatim if you prefer the
consistency.
@alberto-art3ch Thank you for the quick turnaround.
Regarding the accounting in case the loan was already charged-off:
- We need to move from Charge-off GL accounts to Write-off accounts the rest
of the balances.
Progressive loan implementation:
`org.apache.fineract.accounting.journalentry.service.AccrualBasedAccountingProcessorForLoan#createJournalEntriesForWriteOffsWhenLoanIsChargedOff`
We are looking for the same for Working Capital
--
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]
