alberto-art3ch commented on PR #6301: URL: https://github.com/apache/fineract/pull/6301#issuecomment-5416369346
> Went back over the money-critical paths specifically - recovery-cap race, transaction boundaries, authorization, rounding - since this is a new feature that moves money. All of it holds up: > > * The recovery cap is read and written inside one transaction, and `WorkingCapitalLoanBalance` carries `@Version`, so two concurrent recovery payments can't both pass - the loser gets an optimistic-lock failure rather than an over-recovery. > * Command handler and write service are both `@Transactional`, transaction/balance/journal entries commit or roll back together. > * Authorization goes through the command framework (`RECOVERYPAYMENT_WORKINGCAPITALLOAN`), not just the migration seeding a permission row - confirmed the write service is actually gated by it. > * Rounding/locale handling matches the rest of the module (same `MathContext`, same `Locale.US` parsing as disbursement/repayment) - nothing new introduced here. > * Liquibase migration only inserts config rows with `MARK_RAN` preconditions, no deletes - follows the write-off PR's pattern correctly. > > A few things worth addressing as follow-ups, not blockers: > > 1. `undoRecoveryPayment` doesn't emit the generic `WorkingCapitalLoanTransactionReversedBusinessEvent` that every other WC reversal emits alongside its specific event - a consumer reconciling reversals generically will miss recovery reversals. > 2. No unit test for the write service itself (`WorkingCapitalLoanRecoveryPaymentWriteServiceImpl`) - the PR has good unit coverage for the balance math and validation, but the actual balance-mutation/JE-posting/event-emission logic is only covered by integration/e2e tests, which don't run in the fast build. The write-off feature has a dedicated unit test for its write service; a mirror of that would be good here. > 3. Two stranded Javadoc blocks left over from the diff (`AccrualWithDeferredRevenueAmortizationAccountingProcessorForWorkingCapitalLoan` and `WorkingCapitalLoanDataValidator`) - worth a quick cleanup pass. > 4. `MathUtil.subtract(...).max(BigDecimal.ZERO)` on the undo path isn't null-safe if the first argument were ever null, and it silently floors at zero rather than failing if `totalRecovered` were ever less than the reversed amount - unreachable today given the NOT NULL/default-0 columns, but worth the `nullToZero` idiom used elsewhere in the class for consistency, and I'd rather this fail loudly than clamp silently on a money path. > > Recommendation: APPROVE Thanks for the thorough review! All four addressed: generic TransactionReversed event added to the undo (note: undo write-off has the same gap — separate PR), dedicated unit test for the write service added mirroring the write-off one, both stranded Javadocs cleaned up, and the undo path now fails loudly (GeneralPlatformDomainRuleException) instead of clamping totalRecovered at zero. -- 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]
