adamsaghy commented on PR #6467: URL: https://github.com/apache/fineract/pull/6467#issuecomment-5889051754
> Heads-up on changeset `wcl-0085-2` in `0085_wc_remove_unsupported_payment_allocation_rules.xml` — the second `<delete>`, the one targeting `m_wc_loan_payment_allocation_rule`. > > That table is **per-loan**, not per-product: `WorkingCapitalLoanPaymentAllocationRule` has a `wc_loan_id` FK. And rows land in it for real loans — `WorkingCapitalLoanAssemblerImpl.copyPaymentAllocationRules()` copies the product's rules onto the loan at assembly, and again on update. Before this PR the product API accepted the full transaction-type set (the Swagger `allowableValues` this PR narrows from 14 values to 5), so a tenant that configured e.g. `MERCHANT_ISSUED_REFUND` or `CHARGE_REFUND` on a WC product has those rows on live loans today. > > What happens after the delete isn't an error, which is what makes it easy to miss. `WorkingCapitalLoanAllocationRequestFactory.getAllocationRule()` resolves _"specific rule, falling back to DEFAULT"_ — so a transaction whose type-specific rule was just deleted is silently allocated with the DEFAULT order instead of the configured one. Principal / interest / fee / penalty split differently while the **total still matches**, so no assertion on totals would catch it, and the components map to different GL accounts. Since WC loans reprocess transactions retroactively, allocations already posted can be rewritten under the fallback order. > > The changeset comment says rules for the other types "were never applied". That may hold for the tenants you have visibility into, but nothing in the change verifies it, and the code path above says they _would_ be applied whenever a transaction of that type occurs on the loan. > > Two changes would keep the intent and remove the risk: > > * a `<preConditions>` with `expectedResult 0` on a count of the rows about to be deleted, so the migration **fails loudly** on any tenant where the premise doesn't hold rather than silently deleting; > * archiving the rows, or at minimum a `<rollback>` block — `<delete>` has no automatic rollback in Liquibase, and the allocation rule in force is what explains how a past payment was split, so removing it also removes the explanation of transactions already booked. > > Separately, and much smaller: the type validation moved out of `WorkingCapitalAdvancedPaymentAllocationsValidator.validateAllocationRule()` and into `WorkingCapitalAdvancedPaymentAllocationsJsonParser`. The validator was the chokepoint; the parser is one entry path. Any path that builds allocation rules without going through the JSON parser no longer gets the null-type check. > > _Disclosure:_ I'm not a Fineract user. I found this while benchmarking an automated code-review tool against real merged PRs in payments and ledger repositories, and this was the most serious thing that run produced, so it seemed wrong not to raise it. I've only read this diff and the files linked above, so I may well be missing context you have — happy to be told it's a non-issue. I don't have an ASF JIRA account yet; glad to open a ticket there if you'd rather have it tracked formally. Thank you for raising your concerns. The delete is justified hence no any implementation exists for those transaction types. -- 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]
