[GitHub] [fineract] galovics commented on a diff in pull request #2944: FINERACT-1839: Reversal on the loan account after CBR / Chargeback

["galovics (via GitHub)" <gi...@apache.org>]

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


##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/delinquency/service/LoanDelinquencyDomainServiceImpl.java:
##########
@@ -48,12 +48,15 @@ public CollectionData getOverdueCollectionData(final Loan loan) {
         boolean oldestOverdueInstallment = false;
         boolean overdueSinceDateWasSet = false;
         boolean firstNotYetDueInstallment = false;
+        LoanRepaymentScheduleInstallment latestInstallment = loan.getRepaymentScheduleInstallments()

Review Comment:
   Just from a usability point of view, I think we could create a method on the loan itself to get the last installment. Seems like a very common use-case to me and makes this code much more readable.



##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/delinquency/service/LoanDelinquencyDomainServiceImpl.java:
##########
@@ -48,12 +48,15 @@ public CollectionData getOverdueCollectionData(final Loan loan) {
         boolean oldestOverdueInstallment = false;
         boolean overdueSinceDateWasSet = false;
         boolean firstNotYetDueInstallment = false;
+        LoanRepaymentScheduleInstallment latestInstallment = loan.getRepaymentScheduleInstallments()
+                .get(loan.getRepaymentScheduleInstallments().size() - 1);
+
+        List<LoanTransaction> chargebackTransactions = loan.getLoanTransactions().stream().filter(LoanTransaction::isChargeback).toList();

Review Comment:
   Same thing as above, usability perspective, let's move this into the Loan to get only the chargeBack transactions. Even better if you make it more abstract and do like this:
   
   ```
   loan.getLoanTransactions(LoanTransaction::isChargeback);
   ```



##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/delinquency/service/LoanDelinquencyDomainServiceImpl.java:
##########
@@ -66,10 +69,11 @@ public CollectionData getOverdueCollectionData(final Loan loan) {
 
                         amountAvailable = installment.getTotalPaid(loanCurrency).getAmount();
 
-                        for (LoanTransactionToRepaymentScheduleMapping mappingInstallment : installment
-                                .getLoanTransactionToRepaymentScheduleMappings()) {
-                            final LoanTransaction loanTransaction = mappingInstallment.getLoanTransaction();
-                            if (loanTransaction.isChargeback()) {
+                        boolean isLatestInstallment = installment.equals(latestInstallment);
+                        for (LoanTransaction loanTransaction : chargebackTransactions) {
+                            if (!loanTransaction.getTransactionDate().isBefore(installment.getFromDate()) && ((!isLatestInstallment

Review Comment:
   We have many many places with logic like this and I always feel they are a bit hard to read. Do you think it'd make sense to extract these individual condition pieces into variables before the if statement? Like this:
   
   ```
   boolean isTransactionBeforeInstallmentStart = loanTransaction.getTransactionDate().isBefore(installment.getFromDate();
   ....
   if (!isTransactionBeforeInstallmentStart && ...) {
       ...
   }
   ```
   Also, positive conditions are usually better, please evaluate whether we can get rid of the negate operator and rename the variable accordingly.



##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/delinquency/service/LoanDelinquencyDomainServiceImpl.java:
##########
@@ -66,10 +69,11 @@ public CollectionData getOverdueCollectionData(final Loan loan) {
 
                         amountAvailable = installment.getTotalPaid(loanCurrency).getAmount();
 
-                        for (LoanTransactionToRepaymentScheduleMapping mappingInstallment : installment
-                                .getLoanTransactionToRepaymentScheduleMappings()) {
-                            final LoanTransaction loanTransaction = mappingInstallment.getLoanTransaction();
-                            if (loanTransaction.isChargeback()) {
+                        boolean isLatestInstallment = installment.equals(latestInstallment);

Review Comment:
   Be careful with this. The LoanRepaymentScheduleInstallment doesn't have the equals/hashCode overriden. The often given advice is to do equality checks based on identity and override the equals/hashCode accordingly.
   Can't we check the ID here instead of the full object?



-- 
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