Copilot commented on code in PR #5813:
URL: https://github.com/apache/fineract/pull/5813#discussion_r3153745795
##########
fineract-loan/src/main/java/org/apache/fineract/portfolio/delinquency/service/DelinquencyReadPlatformServiceImpl.java:
##########
@@ -158,7 +158,9 @@ public CollectionData calculateLoanCollectionData(final
Long loanId) {
// loans
collectionData =
loanDelinquencyDomainService.getOverdueCollectionData(loan,
effectiveDelinquencyList);
collectionData.setAvailableDisbursementAmount(calculateAvailableDisbursementAmount(loan));
-
collectionData.setAvailableDisbursementAmountWithOverApplied(calculateAvailableDisbursementAmountWithOverApplied(loan));
+ if (loan.getLoanProduct() != null) {
Review Comment:
`calculateAvailableDisbursementAmountWithOverApplied(loan)` still
dereferences `loan.getLoanProduct()` without a null check, and there are other
call sites that invoke it unconditionally (e.g.,
`LoanReadPlatformServiceImpl.retrieveApprovalTemplate` /
`retrieveDisbursalTemplate`). If `loan.getLoanProduct()` can be null, this PR
fixes the NPE in `calculateLoanCollectionData`, but the same HTTP 500 can still
occur elsewhere. Consider making
`calculateAvailableDisbursementAmountWithOverApplied` null-safe (treat null
loanProduct as over-apply disabled) or enforce/validate non-null `LoanProduct`
before exposing this API.
```suggestion
if (loan.getLoanProduct() != null &&
loan.getLoanProduct().isAllowApprovedDisbursedAmountsOverApplied()) {
```
##########
fineract-loan/src/main/java/org/apache/fineract/portfolio/delinquency/service/DelinquencyReadPlatformServiceImpl.java:
##########
@@ -141,7 +141,7 @@ public CollectionData calculateLoanCollectionData(final
Long loanId) {
// If the Loan is not Active yet or is cancelled (rejected or
withdrawn), return template data
if (loan.isSubmittedAndPendingApproval() || loan.isApproved() ||
loan.isCancelled()) {
- if (loan.getLoanProduct() == null ||
loan.getLoanProduct().isAllowApprovedDisbursedAmountsOverApplied()) {
+ if (loan.getLoanProduct() != null &&
loan.getLoanProduct().isAllowApprovedDisbursedAmountsOverApplied()) {
collectionData.setAvailableDisbursementAmountWithOverApplied(calculateAvailableDisbursementAmountWithOverApplied(loan));
Review Comment:
The new null guards in `calculateLoanCollectionData` change behavior for
loans with a null `LoanProduct`, but there are no regression tests in this PR
that exercise the two affected branches (pending/approved/cancelled and active
loans) with `loan.getLoanProduct() == null`. Please add unit tests that assert
the method returns successfully (no exception) and that
`availableDisbursementAmountWithOverApplied` is set to an expected value for
this edge case.
##########
fineract-provider/src/test/java/org/apache/fineract/portfolio/delinquency/service/DelinquencyReadPlatformServiceImplTest.java:
##########
@@ -25,6 +25,8 @@
import java.util.Arrays;
import java.util.Collection;
import java.util.List;
+
Review Comment:
Import formatting looks inconsistent with the rest of the test suite:
there’s an extra blank line separating `java.*` and `org.*` imports here. Also
consider letting the formatter handle this to avoid checkstyle failures in
repositories that disallow empty lines within the non-static import block.
```suggestion
```
##########
fineract-provider/src/test/java/org/apache/fineract/portfolio/delinquency/service/DelinquencyReadPlatformServiceImplTest.java:
##########
@@ -75,6 +78,16 @@ class DelinquencyReadPlatformServiceImplTest {
@Mock
private LoanDelinquencyActionRepository loanDelinquencyActionRepository;
+ @Mock
+ private ConfigurationDomainService configurationDomainService;
+
+ @Mock
+ private LoanTransactionRepository loanTransactionRepository;
+
+ @Mock
+ private PossibleNextRepaymentCalculationServiceDiscovery
possibleNextRepaymentCalculationServiceDiscovery;
+
+
Review Comment:
This test uses `@InjectMocks` with `DelinquencyReadPlatformServiceImpl`,
which has a `final DelinquencyEffectivePauseHelper` constructor dependency, but
the test class still doesn’t declare a `@Mock` for it. Mockito will pass `null`
for that parameter, which will break any new tests that call
`calculateLoanCollectionData`. Also, the added `@Mock` annotations include
trailing whitespace (`@Mock `) which can trip formatting/lint rules—please
remove the extra spaces.
```suggestion
@Mock
private ConfigurationDomainService configurationDomainService;
@Mock
private LoanTransactionRepository loanTransactionRepository;
@Mock
private PossibleNextRepaymentCalculationServiceDiscovery
possibleNextRepaymentCalculationServiceDiscovery;
@Mock
private DelinquencyEffectivePauseHelper delinquencyEffectivePauseHelper;
```
--
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]
