budaidev commented on PR #5850: URL: https://github.com/apache/fineract/pull/5850#issuecomment-4461503525
> Hi, thanks for the PR - nice to see integration tests covering this. A couple of things. > > **`WorkingCapitalLoanDiscountFeeAmortizationServiceImpl.java` line 290:** > > `isChargedOff` is always hardcoded to `false` here, but the accounting processor has the full branch for `CHARGE_OFF_EXPENSE` vs `INCOME_FROM_DISCOUNT_FEE`. If a charged-off WC loan still accumulates realized income through COB, the amortization will always post to income rather than the charge-off expense account. Is this intentional? If so, I'd probably remove the `isChargedOff` parameter from the interface entirely to make the intent explicit - right now the dead branch is confusing. > > **`FeignWorkingCapitalLoanDiscountFeeAmortizationTest.java` line 827:** > > The comment says the transaction date should be the business date (June 16), not the repayment date (June 15), but the assertion only checks `assertNotNull`. It doesn't actually verify the date is June 16. Wouldn't it make more sense to assert the exact date here given that's the whole point of the test? Thank you for the feedback, great points. We are planning to add the other half of the implementation in a later phase, where the isChargedOff part makes sense. I thought about adding a TODO comment here, but I think it's clean enough for now. You are right about the test, I will fix this part to make sure it has meaningful assertations. -- 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]
