adamsaghy commented on PR #6666: URL: https://github.com/apache/fineract/pull/6666#issuecomment-6194573124
Thanks for the thorough write-up. I reviewed this against the description and found no blocking defects. The approach is sound. Below is what I verified, followed by a few things I'd fix or harden before merge. ### Verified - **`computeNetChargePayments` (the 403 fix):** subtracting a charge's tax from its first row only is correct today. The only consumer, `AccountingProcessorHelperImpl#createJournalEntriesForLoanChargesInternal`, sums the rows per GL account before posting. All rows of one charge share a `chargeId`, so they share an income account. - **Tax streams:** `PAYMENT_TAX_STREAM` matches the types the accrual and cash processors send to `createJournalEntriesForRepayments`. Those are `LoanTransactionEnumData#isRepaymentType()` without charge adjustment, plus repayment at disbursement and charge payment. Tax details are read nowhere else, so restricting the computation to the two streams drops nothing that used to be posted. - **Reversed transactions:** both processors handle them through `createJournalEntryForReversedLoanTransaction`, which mirrors the original entries. Not computing tax for them is safe. - **Id-based "earlier":** this holds up under reverse-replay. Replacement transactions get higher ids, and the old ones are already flushed as reversed by `saveLoanTransactionWithDataIntegrityViolationChecks` (`saveAndFlush`), even inside the `COMMIT`-flush `ReprocessLoanTransactionsServiceImpl`. The telescoping sum then works whatever order the transactions are posted in. - **One-minor-unit bound:** it holds for every `RoundingMode`. Rounding never decreases a larger amount, so a line is never negative, and the accrual-adjustment branch swaps `paidFrom`/`paidTo` correctly. - **`LoanChargePaidBy` as a `HashMap` key:** fine, since rows compare by identity and unsaved rows don't merge. ### Should fix 1. **The doc sentence overstates the guarantee for multi-component tax groups** (`taxes-on-loan-charges.adoc`, and the same wording in the PR and commit message). Each component is rounded on its own (`taxLines`), so a fully paid charge posts Σ round(Tᵢ), which can differ from round(Σ Tᵢ) by up to (number of components − 1) minor units. For example, two components of 0.005 each post 0.00 + 0.00, while the charge's tax rounded as a whole is 0.01. Suggest "the lines of each tax component sum to that component's tax rounded as a whole". ### Hardening suggestions 2. **`PAYMENT_TAX_STREAM` duplicates the processors' dispatch condition.** If a new repayment-like type is added to `isRepaymentType()` and routed to `createJournalEntriesForRepayments`, it would silently post no tax, because the poster wouldn't compute any for it. Consider a unit test that pins the set against `LoanTransactionEnumData#isRepaymentType()` (minus charge adjustment, plus repayment at disbursement and charge payment) over all `LoanTransactionType` values. A shared predicate would also work. 3. **Avoid negative intermediate rows in `computeNetChargePayments`.** Its correctness depends on every consumer summing rows per account. `aRowSmallerThanTheChargesTaxGoesNegativeAndTheChargeNetsOut` documents this but doesn't protect it. If a future consumer posts per row, or validates that rows are positive, it breaks. Merging the rows of each `loanChargeId` into one row before subtracting the tax gives the same per-account totals with no negative amounts. 4. **Fail fast when the transaction has no id.** `findChargeAmountPaidByEarlierTransactions(loanCharge, loanTransaction.getId(), …)` with a null id evaluates `lt.id < NULL`, matches nothing, and silently treats the payment as the charge's first. That breaks the cumulative sum without any error. Every current caller persists the transaction first, but an `Objects.requireNonNull` (or `IllegalStateException`) at the start of `prorateChargeTaxes` would surface a future caller that doesn't. 5. **Document the flush dependency.** The base comes from a JPQL query, so it only sees earlier rows that have been written to the database. That holds today only because the posting call sites use `saveAndFlush`/`saveAll` + `flush`, and some run under `@WithFlushMode(COMMIT)`. A short comment on `prorateChargeTaxes` (or on the repository method) would keep someone from removing a flush and quietly breaking the rounding. 6. **Fragile tax rate in the integration test.** `TAX_RATE_23_IN_123 = 18.699187f` depends on float-to-double conversion storing 18.699186, and the expected amounts are tuned to that value. Please send it as a `double` or `BigDecimal` with the intended value. Also, the unit tests use 186.991870 while the PR text and integration tests use 186.991860. It's harmless, but aligning them would make the examples easier to follow. Points 1 and 2 are the ones I'd most like to see addressed. The rest are cheap and make the change more robust against future edits. -- 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]
