oleksii-novikov-onix commented on PR #6669: URL: https://github.com/apache/fineract/pull/6669#issuecomment-6291923449
> ## Review > Looks fine overall, one thing that bugs me about where "payment" is defined now. > > `fineract-working-capital-loan/src/main/java/org/apache/fineract/portfolio/workingcapitalloan/serialization/serializer/WorkingCapitalLoanBusinessEventSerializer.java`: > > > With this change `totalPayment` in the event comes from `WorkingCapitalLoanTransactionFinder.PAYMENT_TYPES` (`getRepaymentLikeTransactionTypes()`), but `totalPaymentReversed` right below still comes from the serializer's own `PAYMENT_TRANSACTION_TYPES` list. Today they happen to resolve to the same thing for WC since we don't book merchant issued refund / charge refund / down payment / interest payment waiver here, but the moment somebody touches either list the two values in the same payload will silently mean different things. Can we have a single definition of the payment types and use it for both the active and the reversed sum? Thoughts? > > Two smaller ones: > > * All six new `summary.totalPayment` assertions are on loans without reversed transactions, so the `t.reversed = false` filter in `sumPayments` isn't covered on GET. One assertion after a repayment reversal in an existing scenario would do it. > * The event path now runs a second aggregate (`sumActiveAmountsOfTypes`) on top of `fetchTotalsPerTypeAndReversed`. Small, but it's the same aggregation twice. Also it takes a `List` where `sumAmountsOfTypesUpTo` takes a `Collection`. > > PR description is the empty template, pls fill it in before taking it out of draft. > > Recommendation: COMMENT Thanks for the review @galovics 1. Payment types: done, both totalPayment and totalPaymentReversed now use WorkingCapitalLoanTransactionFinder.PAYMENT_TYPES, with no extra query and no change to event values. 2. Reversal on GET: C102396 already covers it (repayments of 20 and 30, the 30 undone, totalPayment 20.0 instead of 50.0 without the filter). 3. Second aggregate: deliberate, the event takes totalPayment from the retrieveOne it already calls, so the API and the event can’t drift apart. 4. List vs Collection: sumAmountsOfTypesUpTo now takes a List too. 5. Description: filled in. -- 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]
