utafrali commented on code in PR #6348:
URL: https://github.com/apache/fineract/pull/6348#discussion_r3881335410
##########
fineract-working-capital-loan/src/main/java/org/apache/fineract/portfolio/workingcapitalloan/domain/WorkingCapitalLoan.java:
##########
@@ -226,6 +228,17 @@ public Long getClientId() {
return client != null ? client.getId() : null;
}
+ /**
+ * The earliest non-null actual disbursement date, or {@code null} when
nothing was disbursed yet.
Review Comment:
`getFirstActualDisbursementDate()` runs a full stream over
`disbursementDetails` independently from `getFirstActualDisbursement()`. When
`getFirstActualDisbursementAmount()` is called, both methods fire, producing
two O(n) passes on the same collection that are guaranteed to agree. Consider
delegating to avoid the duplication:
```java
public LocalDate getFirstActualDisbursementDate() {
return Optional.ofNullable(getFirstActualDisbursement())
.map(WorkingCapitalLoanDisbursementDetails::getActualDisbursementDate)
.orElse(null);
}
```
This also makes it structurally impossible for the date and the detail
returned by `getFirstActualDisbursement()` to ever disagree.
##########
fineract-working-capital-loan/src/main/java/org/apache/fineract/portfolio/workingcapitalloan/domain/WorkingCapitalLoanDisbursementDetailsComparator.java:
##########
@@ -0,0 +1,33 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.fineract.portfolio.workingcapitalloan.domain;
+
+import java.util.Comparator;
+
+/**
+ * Orders actual disbursement details chronologically, using the persisted
detail ID to break ties on the same date.
+ */
+public final class WorkingCapitalLoanDisbursementDetailsComparator {
+
+ public static final Comparator<WorkingCapitalLoanDisbursementDetails>
ACTUAL_DISBURSEMENT_ORDER = Comparator
Review Comment:
The primary comparator key (`getActualDisbursementDate`) will throw
`NullPointerException` if called on a detail with a null actual date. Every
current call site pre-filters for non-null dates before using this comparator,
so it is safe today, but the class carries no documentation of that contract. A
future caller who skips the filter will get a silent NPE at runtime.
Either add a Javadoc precondition note ("Callers must filter out null
`actualDisbursementDate` values before sorting"), or make the comparator
defensive by wrapping the first key with `Comparator.nullsLast(...)` so it is
safe to use on unfiltered streams.
##########
fineract-working-capital-loan/src/main/java/org/apache/fineract/portfolio/workingcapitalloan/mapper/WorkingCapitalLoanMapper.java:
##########
@@ -166,13 +166,12 @@ default LoanApplicationTimelineData timelineData(final
WorkingCapitalLoan loan)
timelineData.setApprovedByLastname(loan.getApprovedBy().getLastname());
timelineData.setApprovedOnDate(loan.getApprovedOnDate());
}
- final WorkingCapitalLoanDisbursementDetails firstDisbursement =
loan.getDisbursementDetails().stream()
- .filter(d -> d.getActualDisbursementDate() !=
null).findFirst().orElse(null);
+ final WorkingCapitalLoanDisbursementDetails firstDisbursement =
loan.getFirstActualDisbursement();
if (firstDisbursement != null && firstDisbursement.getDisbursedBy() !=
null) {
+
timelineData.setActualDisbursementDate(firstDisbursement.getActualDisbursementDate());
timelineData.setDisbursedByUsername(firstDisbursement.getDisbursedBy().getUsername());
timelineData.setDisbursedByFirstname(firstDisbursement.getDisbursedBy().getFirstname());
Review Comment:
`setActualDisbursementDate` is still gated on
`firstDisbursement.getDisbursedBy() != null`. A disbursed loan with no user
linked to that disbursement record will produce a null `actualDisbursementDate`
in the full loan timeline, silently losing the date.
`WorkingCapitalLoanSummaryMapper` does not have this condition — it calls
`loan.getFirstActualDisbursementDate()` unconditionally. This PR's stated goal
is consistent date handling, but these two mappers still behave differently.
Suggested fix:
```java
if (firstDisbursement != null) {
timelineData.setActualDisbursementDate(firstDisbursement.getActualDisbursementDate());
if (firstDisbursement.getDisbursedBy() != null) {
timelineData.setDisbursedByUsername(firstDisbursement.getDisbursedBy().getUsername());
timelineData.setDisbursedByFirstname(firstDisbursement.getDisbursedBy().getFirstname());
timelineData.setDisbursedByLastname(firstDisbursement.getDisbursedBy().getLastname());
}
}
```
--
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]
