[GitHub] [fineract] galovics commented on a diff in pull request #2739: FINERACT-1706: Fix Loan Delinquency Classification after Repayment

[GitBox <gi...@apache.org>]

galovics commented on code in PR #2739:
URL: https://github.com/apache/fineract/pull/2739#discussion_r1023711751


##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/delinquency/service/DelinquencyUtils.java:
##########
@@ -0,0 +1,132 @@
+/**
+ * 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.delinquency.service;
+
+import java.math.BigDecimal;
+import java.time.LocalDate;
+import lombok.extern.slf4j.Slf4j;
+import org.apache.fineract.infrastructure.core.service.DateUtils;
+import org.apache.fineract.organisation.monetary.domain.MonetaryCurrency;
+import org.apache.fineract.portfolio.loanaccount.data.CollectionData;
+import org.apache.fineract.portfolio.loanaccount.domain.Loan;
+import org.apache.fineract.portfolio.loanaccount.domain.LoanRepaymentScheduleInstallment;
+import org.apache.fineract.portfolio.loanaccount.domain.LoanTransaction;
+import org.apache.fineract.portfolio.loanaccount.domain.LoanTransactionToRepaymentScheduleMapping;
+
+@Slf4j
+public final class DelinquencyUtils {

Review Comment:
   I don't like this. When I see the following names:
   - Manager
   - Utils
   - Util
   - etc
   
   I'm always careful because usually it's something fishy. Not always but most of the times. I think this is rather a poor implementation for separation of concerns. On one hand, let's forbid ourselves from using static methods unless there's a good reason for it. 
   
   In this case this class is handling business logic. That's not a Util and that shouldn't be handled in static methods.
   
   Try to generalize the class/method around single responsibilities and try to split it that way into components.



##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/loanaccount/domain/Loan.java:
##########
@@ -6781,15 +6781,19 @@ public Collection<LoanCharge> getLoanCharges() {
     public void initializeLazyCollections() {
         checkAndFetchLazyCollection(this.charges);
         checkAndFetchLazyCollection(this.trancheCharges);
-        checkAndFetchLazyCollection(this.repaymentScheduleInstallments);
-        checkAndFetchLazyCollection(this.loanTransactions);
+        initializeLazyBasicCollections();
         checkAndFetchLazyCollection(this.disbursementDetails);
         checkAndFetchLazyCollection(this.loanTermVariations);
         checkAndFetchLazyCollection(this.collateral);
         checkAndFetchLazyCollection(this.loanOfficerHistory);
         checkAndFetchLazyCollection(this.loanCollateralManagements);
     }
 
+    public void initializeLazyBasicCollections() {

Review Comment:
   Let's get rid of this method rather. It's spreading a bad pattern across the system. Let's try to force ourselves to properly define our transaction boundaries and the methods like this will never be used.



##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/delinquency/service/DelinquencyReadPlatformServiceImpl.java:
##########
@@ -109,89 +103,23 @@ public Collection<LoanDelinquencyTagHistoryData> retrieveDelinquencyRangeHistory
 
     @Override
     public CollectionData calculateLoanCollectionData(final Long loanId) {
-        final LocalDate businessDate = DateUtils.getBusinessLocalDate();
         final Optional<Loan> optLoan = this.loanRepository.findById(loanId);
 
         CollectionData collectionData = CollectionData.template();
         if (optLoan.isPresent()) {
+            final LocalDate businessDate = DateUtils.getBusinessLocalDate();
             final Loan loan = optLoan.get();
+            loan.initializeLazyBasicCollections();

Review Comment:
   100% agreed. TBH the initializeLazyXY method calls should be eliminated completely. They are mostly used to overcome the fact that the transaction boundaries are not properly defined.



##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/loanaccount/domain/LoanRepaymentScheduleInstallment.java:
##########
@@ -140,6 +140,9 @@ public class LoanRepaymentScheduleInstallment extends AbstractAuditableWithUTCDa
     @OneToMany(cascade = CascadeType.ALL, orphanRemoval = true, fetch = FetchType.LAZY, mappedBy = "installment")
     private Set<LoanInstallmentCharge> installmentCharges = new HashSet<>();
 
+    @OneToMany(cascade = CascadeType.ALL, orphanRemoval = true, fetch = FetchType.EAGER, mappedBy = "installment")
+    private Set<LoanTransactionToRepaymentScheduleMapping> loanTransactionToRepaymentScheduleMappings = new HashSet<>();

Review Comment:
   > we would read an association when the entity is already detached.
   
   Not necessarily. Eager fetching comes handy when the parent entity - from a business point of view - can't live without the child entity. 
   I'm not sure if this is the case here though so I'm also waiting for some explanation here.



##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/loanaccount/domain/LoanRepaymentScheduleInstallment.java:
##########
@@ -718,6 +721,9 @@ public void addToCredits(final BigDecimal amount) {
     }
 
     public BigDecimal getTotalPaidInAdvance() {
+        if (this.totalPaidInAdvance == null) {

Review Comment:
   This is kinda fishy to me again. 
   If the column can be null, that means there was no payment in advance, right? We introduce that logic and the users of this class will never be able to make that decision, right? Since we hide it here with this condition.
   
   Also, if this logic has been introduced here, did you check the whole codebase for native SQLs/JPQLs/etc where the same DB table has been used and the same column has been retrieved? We'll need this same behavior there, right?



##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/delinquency/service/DelinquencyUtils.java:
##########
@@ -0,0 +1,132 @@
+/**
+ * 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.delinquency.service;
+
+import java.math.BigDecimal;
+import java.time.LocalDate;
+import lombok.extern.slf4j.Slf4j;
+import org.apache.fineract.infrastructure.core.service.DateUtils;
+import org.apache.fineract.organisation.monetary.domain.MonetaryCurrency;
+import org.apache.fineract.portfolio.loanaccount.data.CollectionData;
+import org.apache.fineract.portfolio.loanaccount.domain.Loan;
+import org.apache.fineract.portfolio.loanaccount.domain.LoanRepaymentScheduleInstallment;
+import org.apache.fineract.portfolio.loanaccount.domain.LoanTransaction;
+import org.apache.fineract.portfolio.loanaccount.domain.LoanTransactionToRepaymentScheduleMapping;
+
+@Slf4j
+public final class DelinquencyUtils {

Review Comment:
   Bonus, why don't I see unit tests for this class?



-- 
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: commits-unsubscribe@fineract.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org