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


##########
fineract-progressive-loan/src/main/java/org/apache/fineract/portfolio/loanaccount/domain/transactionprocessor/impl/AdvancedPaymentScheduleTransactionProcessor.java:
##########
@@ -1527,6 +1507,74 @@ private Money 
processAllocationsHorizontally(LoanTransaction loanTransaction, Tr
         return transactionAmountUnprocessed;
     }
 
+    private Money 
handlingPaymentAllocationForInterestBearingProgressiveLoan(LoanTransaction 
loanTransaction,
+            Money transactionAmountUnprocessed, Balances balances, 
PaymentAllocationType paymentAllocationType,
+            LoanRepaymentScheduleInstallment inAdvanceInstallment, 
ProgressiveTransactionCtx ctx,
+            LoanTransactionToRepaymentScheduleMapping 
loanTransactionToRepaymentScheduleMapping,
+            Set<LoanCharge> inAdvanceInstallmentCharges) {
+        Money paidPortion;
+        ProgressiveLoanInterestScheduleModel model = ctx.getModel();
+        LocalDate payDate = loanTransaction.getTransactionDate();
+        if (DueType.IN_ADVANCE.equals(paymentAllocationType.getDueType())) {
+            payDate = 
calculateNewPayDateInCaseOfInAdvancePayment(loanTransaction, 
inAdvanceInstallment);
+            updateRepaymentPeriodBalances(paymentAllocationType, 
inAdvanceInstallment, model, payDate);
+        }
+
+        paidPortion = processPaymentAllocation(paymentAllocationType, 
inAdvanceInstallment, loanTransaction, transactionAmountUnprocessed,
+                loanTransactionToRepaymentScheduleMapping, 
inAdvanceInstallmentCharges, balances,
+                LoanRepaymentScheduleInstallment.PaymentAction.PAY);
+
+        if (PRINCIPAL.equals(paymentAllocationType.getAllocationType())) {
+            emiCalculator.payPrincipal(model, 
inAdvanceInstallment.getDueDate(), payDate, paidPortion);
+            updateRepaymentPeriods(loanTransaction, ctx, model);
+        } else if (INTEREST.equals(paymentAllocationType.getAllocationType())) 
{
+            emiCalculator.payInterest(model, 
inAdvanceInstallment.getDueDate(), payDate, paidPortion);
+            updateRepaymentPeriods(loanTransaction, ctx, model);
+        }
+        return paidPortion;
+    }
+
+    private static void updateRepaymentPeriods(LoanTransaction 
loanTransaction, ProgressiveTransactionCtx ctx,
+            ProgressiveLoanInterestScheduleModel model) {
+        model.repaymentPeriods().forEach(rm -> {
+            LoanRepaymentScheduleInstallment installment = 
ctx.getInstallments().stream()
+                    .filter(ri -> ri.getDueDate().equals(rm.getDueDate()) && 
!ri.isDownPayment()).findFirst().orElse(null);
+            if (installment != null) {
+                installment.updatePrincipal(rm.getDuePrincipal().getAmount());
+                
installment.updateInterestCharged(rm.getDueInterest().getAmount());
+                installment.updateObligationsMet(ctx.getCurrency(), 
loanTransaction.getTransactionDate());
+            }
+        });
+    }
+
+    private void updateRepaymentPeriodBalances(PaymentAllocationType 
paymentAllocationType,
+            LoanRepaymentScheduleInstallment inAdvanceInstallment, 
ProgressiveLoanInterestScheduleModel model, LocalDate payDate) {
+        PayableDetails payableDetails = emiCalculator.getPayableDetails(model, 
inAdvanceInstallment.getDueDate(), payDate);
+
+        switch (paymentAllocationType) {
+            case IN_ADVANCE_INTEREST -> 
inAdvanceInstallment.updateInterestCharged(payableDetails.getPayableInterest().getAmount());
+            case IN_ADVANCE_PRINCIPAL -> 
inAdvanceInstallment.updatePrincipal(payableDetails.getPayablePrincipal().getAmount());
+            default -> {
+            }
+        }
+    }
+
+    private static LocalDate 
calculateNewPayDateInCaseOfInAdvancePayment(LoanTransaction loanTransaction,

Review Comment:
   Same here, probably shouldn't be static.



##########
fineract-progressive-loan/src/main/java/org/apache/fineract/portfolio/loanaccount/domain/transactionprocessor/impl/AdvancedPaymentScheduleTransactionProcessor.java:
##########
@@ -1527,6 +1507,74 @@ private Money 
processAllocationsHorizontally(LoanTransaction loanTransaction, Tr
         return transactionAmountUnprocessed;
     }
 
+    private Money 
handlingPaymentAllocationForInterestBearingProgressiveLoan(LoanTransaction 
loanTransaction,
+            Money transactionAmountUnprocessed, Balances balances, 
PaymentAllocationType paymentAllocationType,
+            LoanRepaymentScheduleInstallment inAdvanceInstallment, 
ProgressiveTransactionCtx ctx,
+            LoanTransactionToRepaymentScheduleMapping 
loanTransactionToRepaymentScheduleMapping,
+            Set<LoanCharge> inAdvanceInstallmentCharges) {
+        Money paidPortion;
+        ProgressiveLoanInterestScheduleModel model = ctx.getModel();
+        LocalDate payDate = loanTransaction.getTransactionDate();
+        if (DueType.IN_ADVANCE.equals(paymentAllocationType.getDueType())) {
+            payDate = 
calculateNewPayDateInCaseOfInAdvancePayment(loanTransaction, 
inAdvanceInstallment);
+            updateRepaymentPeriodBalances(paymentAllocationType, 
inAdvanceInstallment, model, payDate);
+        }
+
+        paidPortion = processPaymentAllocation(paymentAllocationType, 
inAdvanceInstallment, loanTransaction, transactionAmountUnprocessed,
+                loanTransactionToRepaymentScheduleMapping, 
inAdvanceInstallmentCharges, balances,
+                LoanRepaymentScheduleInstallment.PaymentAction.PAY);
+
+        if (PRINCIPAL.equals(paymentAllocationType.getAllocationType())) {
+            emiCalculator.payPrincipal(model, 
inAdvanceInstallment.getDueDate(), payDate, paidPortion);
+            updateRepaymentPeriods(loanTransaction, ctx, model);
+        } else if (INTEREST.equals(paymentAllocationType.getAllocationType())) 
{
+            emiCalculator.payInterest(model, 
inAdvanceInstallment.getDueDate(), payDate, paidPortion);
+            updateRepaymentPeriods(loanTransaction, ctx, model);
+        }
+        return paidPortion;
+    }
+
+    private static void updateRepaymentPeriods(LoanTransaction 
loanTransaction, ProgressiveTransactionCtx ctx,

Review Comment:
   Shouldn't be static I'd assume.



##########
fineract-progressive-loan/src/main/java/org/apache/fineract/portfolio/loanaccount/loanschedule/data/InterestPeriod.java:
##########
@@ -0,0 +1,110 @@
+/**
+ * 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.loanaccount.loanschedule.data;
+
+import jakarta.validation.constraints.NotNull;
+import java.math.BigDecimal;
+import java.time.LocalDate;
+import java.util.Optional;
+import lombok.EqualsAndHashCode;
+import lombok.Getter;
+import lombok.Setter;
+import org.apache.fineract.infrastructure.core.service.MathUtil;
+import org.apache.fineract.organisation.monetary.domain.Money;
+
+@Getter
+@EqualsAndHashCode(exclude = { "repaymentPeriod" })
+public class InterestPeriod implements Comparable<InterestPeriod> {
+
+    private final RepaymentPeriod repaymentPeriod;
+    private final LocalDate fromDate;
+    @Setter
+    private LocalDate dueDate;
+    @Setter
+    private BigDecimal rateFactor;
+    private Money disbursementAmount;
+    private Money balanceCorrectionAmount;
+    private Money outstandingLoanBalance;
+
+    public InterestPeriod(RepaymentPeriod repaymentPeriod, LocalDate fromDate, 
LocalDate dueDate, BigDecimal rateFactor,
+            Money disbursementAmount, Money balanceCorrectionAmount, Money 
outstandingLoanBalance) {
+        this.repaymentPeriod = repaymentPeriod;
+        this.fromDate = fromDate;
+        this.dueDate = dueDate;
+        this.rateFactor = rateFactor;
+        this.disbursementAmount = disbursementAmount;
+        this.balanceCorrectionAmount = balanceCorrectionAmount;
+        this.outstandingLoanBalance = outstandingLoanBalance;
+    }
+
+    public InterestPeriod(RepaymentPeriod repaymentPeriod, InterestPeriod 
interestPeriod) {
+        this(repaymentPeriod, interestPeriod.getFromDate(), 
interestPeriod.getDueDate(), interestPeriod.getRateFactor(),
+                interestPeriod.getDisbursementAmount(), 
interestPeriod.getBalanceCorrectionAmount(),
+                interestPeriod.getOutstandingLoanBalance());
+    }
+
+    @Override
+    public int compareTo(@NotNull InterestPeriod o) {
+        return dueDate.compareTo(o.dueDate);
+    }
+
+    public void addBalanceCorrectionAmount(final Money 
balanceCorrectionAmount) {
+        this.balanceCorrectionAmount = 
MathUtil.plus(this.balanceCorrectionAmount, balanceCorrectionAmount);
+    }
+
+    public void addDisbursementAmount(final Money disbursementAmount) {
+        this.disbursementAmount = MathUtil.plus(this.disbursementAmount, 
disbursementAmount);
+    }
+
+    public Money getCalculatedDueInterest() {
+        return getOutstandingLoanBalance().multipliedBy(getRateFactor());
+    }
+
+    public void updateOutstandingLoanBalance() {
+        if (isFirstInterestPeriod()) {
+            Optional<RepaymentPeriod> previousRepaymentPeriod = 
getRepaymentPeriod().getPrevious();
+            if (previousRepaymentPeriod.isPresent()) {
+                InterestPeriod previousInterestPeriod = 
previousRepaymentPeriod.get().getInterestPeriods()
+                        
.get(previousRepaymentPeriod.get().getInterestPeriods().size() - 1);
+                this.outstandingLoanBalance = 
previousInterestPeriod.getOutstandingLoanBalance()//
+                        .plus(previousInterestPeriod.getDisbursementAmount())//
+                        
.plus(previousInterestPeriod.getBalanceCorrectionAmount())//
+                        
.minus(previousRepaymentPeriod.get().getDuePrincipal())//
+                        
.plus(previousRepaymentPeriod.get().getPaidPrincipal());//
+            }
+        } else {
+            int index = 
getRepaymentPeriod().getInterestPeriods().indexOf(this);
+            InterestPeriod previousInterestPeriod = 
getRepaymentPeriod().getInterestPeriods().get(index - 1);
+            this.outstandingLoanBalance = 
previousInterestPeriod.getOutstandingLoanBalance() //
+                    .plus(previousInterestPeriod.getBalanceCorrectionAmount()) 
//
+                    .plus(previousInterestPeriod.getDisbursementAmount()); //
+        }
+    }
+
+    private boolean isFirstInterestPeriod() {
+        return getRepaymentPeriod().getInterestPeriods().get(0).equals(this);
+    }
+
+    @Override
+    public String toString() {

Review Comment:
   Why not using a Lombok toString?



##########
fineract-progressive-loan/src/main/java/org/apache/fineract/portfolio/loanaccount/loanschedule/data/ProgressiveLoanInterestScheduleModel.java:
##########
@@ -19,45 +19,167 @@
 package org.apache.fineract.portfolio.loanaccount.loanschedule.data;
 
 import java.math.BigDecimal;
-import java.math.MathContext;
 import java.time.LocalDate;
 import java.time.temporal.ChronoUnit;
 import java.util.ArrayList;
-import java.util.Collections;
 import java.util.List;
+import java.util.Optional;
+import java.util.function.Consumer;
+import lombok.Data;
+import lombok.experimental.Accessors;
+import org.apache.fineract.organisation.monetary.domain.Money;
 import 
org.apache.fineract.portfolio.loanproduct.domain.LoanProductRelatedDetail;
 
-public record 
ProgressiveLoanInterestScheduleModel(List<ProgressiveLoanInterestRepaymentModel>
 repayments, //
-        List<ProgressiveLoanInterestRate> interestRates, //
-        LoanProductRelatedDetail loanProductRelatedDetail, //
-        Integer installmentAmountInMultiplesOf, //
-        MathContext mc) {
+@Data
+@Accessors(fluent = true)
+public class ProgressiveLoanInterestScheduleModel {
 
-    public 
ProgressiveLoanInterestScheduleModel(List<ProgressiveLoanInterestRepaymentModel>
 repayments,
-            LoanProductRelatedDetail loanProductRelatedDetail, Integer 
installmentAmountInMultiplesOf, MathContext mc) {
-        this(repayments, new ArrayList<>(1), loanProductRelatedDetail, 
installmentAmountInMultiplesOf, mc);
+    private final List<RepaymentPeriod> repaymentPeriods;
+    private final List<InterestRate> interestRates;
+    private final LoanProductRelatedDetail loanProductRelatedDetail;
+    private final Integer installmentAmountInMultiplesOf;
+
+    public ProgressiveLoanInterestScheduleModel(List<RepaymentPeriod> 
repaymentPeriods, LoanProductRelatedDetail loanProductRelatedDetail,
+            Integer installmentAmountInMultiplesOf) {
+        this.repaymentPeriods = repaymentPeriods;
+        this.interestRates = new ArrayList<>();
+        this.loanProductRelatedDetail = loanProductRelatedDetail;
+        this.installmentAmountInMultiplesOf = installmentAmountInMultiplesOf;
+    }
+
+    private ProgressiveLoanInterestScheduleModel(List<RepaymentPeriod> 
repaymentPeriods, final List<InterestRate> interestRates,
+            LoanProductRelatedDetail loanProductRelatedDetail, Integer 
installmentAmountInMultiplesOf) {
+        this.repaymentPeriods = copyRepaymentPeriods(repaymentPeriods);
+        this.interestRates = new ArrayList<>(interestRates);
+        this.loanProductRelatedDetail = loanProductRelatedDetail;
+        this.installmentAmountInMultiplesOf = installmentAmountInMultiplesOf;
+    }
+
+    public ProgressiveLoanInterestScheduleModel deepCopy() {
+        return new ProgressiveLoanInterestScheduleModel(repaymentPeriods, 
interestRates, loanProductRelatedDetail,
+                installmentAmountInMultiplesOf);
     }
 
-    public void addInterestRate(final LocalDate newInterestDueDate, final 
BigDecimal newInterestRate) {
-        interestRates.add(new ProgressiveLoanInterestRate(newInterestDueDate, 
newInterestDueDate.plusDays(1), newInterestRate));
-        interestRates.sort(Collections.reverseOrder());
+    private List<RepaymentPeriod> copyRepaymentPeriods(final 
List<RepaymentPeriod> repaymentPeriods) {

Review Comment:
   I'm not sure that this is the right place for this logic. Can we extract it?



##########
fineract-progressive-loan/src/main/java/org/apache/fineract/portfolio/loanaccount/loanschedule/data/RepaymentPeriod.java:
##########
@@ -0,0 +1,176 @@
+/**
+ * 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.loanaccount.loanschedule.data;
+
+import java.math.BigDecimal;
+import java.time.LocalDate;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.Optional;
+import lombok.EqualsAndHashCode;
+import lombok.Getter;
+import lombok.Setter;
+import lombok.ToString;
+import org.apache.fineract.infrastructure.core.service.MathUtil;
+import org.apache.fineract.organisation.monetary.domain.Money;
+
+@ToString
+@EqualsAndHashCode(exclude = { "previous", "next" })
+public class RepaymentPeriod {
+
+    @ToString.Exclude
+    private final RepaymentPeriod previous;
+    @Getter
+    private final LocalDate fromDate;
+    @Getter
+    private final LocalDate dueDate;
+    @Getter

Review Comment:
   By exposing the getter, you're making it possible to change the contents of 
the List without using the addInterestPeriod method which is the only thing 
taking care of the sorting, hence the sorted invariant cannot be fully true.



##########
fineract-progressive-loan/src/main/java/org/apache/fineract/portfolio/loanaccount/loanschedule/data/RepaymentPeriod.java:
##########
@@ -0,0 +1,176 @@
+/**
+ * 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.loanaccount.loanschedule.data;
+
+import java.math.BigDecimal;
+import java.time.LocalDate;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.Optional;
+import lombok.EqualsAndHashCode;
+import lombok.Getter;
+import lombok.Setter;
+import lombok.ToString;
+import org.apache.fineract.infrastructure.core.service.MathUtil;
+import org.apache.fineract.organisation.monetary.domain.Money;
+
+@ToString
+@EqualsAndHashCode(exclude = { "previous", "next" })
+public class RepaymentPeriod {
+
+    @ToString.Exclude
+    private final RepaymentPeriod previous;

Review Comment:
   Are you sure you want to create a linked list from this? 
   
   What's the reasoning behind the recursive structure?



##########
fineract-progressive-loan/src/main/java/org/apache/fineract/portfolio/loanaccount/loanschedule/data/InterestPeriod.java:
##########
@@ -0,0 +1,110 @@
+/**
+ * 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.loanaccount.loanschedule.data;
+
+import jakarta.validation.constraints.NotNull;
+import java.math.BigDecimal;
+import java.time.LocalDate;
+import java.util.Optional;
+import lombok.EqualsAndHashCode;
+import lombok.Getter;
+import lombok.Setter;
+import org.apache.fineract.infrastructure.core.service.MathUtil;
+import org.apache.fineract.organisation.monetary.domain.Money;
+
+@Getter
+@EqualsAndHashCode(exclude = { "repaymentPeriod" })
+public class InterestPeriod implements Comparable<InterestPeriod> {
+
+    private final RepaymentPeriod repaymentPeriod;
+    private final LocalDate fromDate;
+    @Setter
+    private LocalDate dueDate;
+    @Setter
+    private BigDecimal rateFactor;
+    private Money disbursementAmount;
+    private Money balanceCorrectionAmount;
+    private Money outstandingLoanBalance;
+
+    public InterestPeriod(RepaymentPeriod repaymentPeriod, LocalDate fromDate, 
LocalDate dueDate, BigDecimal rateFactor,
+            Money disbursementAmount, Money balanceCorrectionAmount, Money 
outstandingLoanBalance) {
+        this.repaymentPeriod = repaymentPeriod;
+        this.fromDate = fromDate;
+        this.dueDate = dueDate;
+        this.rateFactor = rateFactor;
+        this.disbursementAmount = disbursementAmount;
+        this.balanceCorrectionAmount = balanceCorrectionAmount;
+        this.outstandingLoanBalance = outstandingLoanBalance;
+    }
+
+    public InterestPeriod(RepaymentPeriod repaymentPeriod, InterestPeriod 
interestPeriod) {
+        this(repaymentPeriod, interestPeriod.getFromDate(), 
interestPeriod.getDueDate(), interestPeriod.getRateFactor(),
+                interestPeriod.getDisbursementAmount(), 
interestPeriod.getBalanceCorrectionAmount(),
+                interestPeriod.getOutstandingLoanBalance());
+    }
+
+    @Override
+    public int compareTo(@NotNull InterestPeriod o) {
+        return dueDate.compareTo(o.dueDate);

Review Comment:
   Isn't thisrelying on the fact that the dueDate will be not null? As far as 
the code is concerned above, there's a setter generated by Lombok for the 
dueDate so somebody can rather easily mess up the not null invariant.



##########
fineract-progressive-loan/src/main/java/org/apache/fineract/portfolio/loanaccount/loanschedule/data/ProgressiveLoanInterestScheduleModel.java:
##########
@@ -19,45 +19,167 @@
 package org.apache.fineract.portfolio.loanaccount.loanschedule.data;
 
 import java.math.BigDecimal;
-import java.math.MathContext;
 import java.time.LocalDate;
 import java.time.temporal.ChronoUnit;
 import java.util.ArrayList;
-import java.util.Collections;
 import java.util.List;
+import java.util.Optional;
+import java.util.function.Consumer;
+import lombok.Data;
+import lombok.experimental.Accessors;
+import org.apache.fineract.organisation.monetary.domain.Money;
 import 
org.apache.fineract.portfolio.loanproduct.domain.LoanProductRelatedDetail;
 
-public record 
ProgressiveLoanInterestScheduleModel(List<ProgressiveLoanInterestRepaymentModel>
 repayments, //
-        List<ProgressiveLoanInterestRate> interestRates, //
-        LoanProductRelatedDetail loanProductRelatedDetail, //
-        Integer installmentAmountInMultiplesOf, //
-        MathContext mc) {
+@Data
+@Accessors(fluent = true)
+public class ProgressiveLoanInterestScheduleModel {
 
-    public 
ProgressiveLoanInterestScheduleModel(List<ProgressiveLoanInterestRepaymentModel>
 repayments,
-            LoanProductRelatedDetail loanProductRelatedDetail, Integer 
installmentAmountInMultiplesOf, MathContext mc) {
-        this(repayments, new ArrayList<>(1), loanProductRelatedDetail, 
installmentAmountInMultiplesOf, mc);
+    private final List<RepaymentPeriod> repaymentPeriods;
+    private final List<InterestRate> interestRates;
+    private final LoanProductRelatedDetail loanProductRelatedDetail;
+    private final Integer installmentAmountInMultiplesOf;
+
+    public ProgressiveLoanInterestScheduleModel(List<RepaymentPeriod> 
repaymentPeriods, LoanProductRelatedDetail loanProductRelatedDetail,
+            Integer installmentAmountInMultiplesOf) {
+        this.repaymentPeriods = repaymentPeriods;

Review Comment:
   What's the reason we're not copying the repayment periods here?



##########
fineract-progressive-loan/src/main/java/org/apache/fineract/portfolio/loanaccount/loanschedule/data/ProgressiveLoanInterestScheduleModel.java:
##########
@@ -19,45 +19,167 @@
 package org.apache.fineract.portfolio.loanaccount.loanschedule.data;
 
 import java.math.BigDecimal;
-import java.math.MathContext;
 import java.time.LocalDate;
 import java.time.temporal.ChronoUnit;
 import java.util.ArrayList;
-import java.util.Collections;
 import java.util.List;
+import java.util.Optional;
+import java.util.function.Consumer;
+import lombok.Data;
+import lombok.experimental.Accessors;
+import org.apache.fineract.organisation.monetary.domain.Money;
 import 
org.apache.fineract.portfolio.loanproduct.domain.LoanProductRelatedDetail;
 
-public record 
ProgressiveLoanInterestScheduleModel(List<ProgressiveLoanInterestRepaymentModel>
 repayments, //
-        List<ProgressiveLoanInterestRate> interestRates, //
-        LoanProductRelatedDetail loanProductRelatedDetail, //
-        Integer installmentAmountInMultiplesOf, //
-        MathContext mc) {
+@Data
+@Accessors(fluent = true)
+public class ProgressiveLoanInterestScheduleModel {
 
-    public 
ProgressiveLoanInterestScheduleModel(List<ProgressiveLoanInterestRepaymentModel>
 repayments,
-            LoanProductRelatedDetail loanProductRelatedDetail, Integer 
installmentAmountInMultiplesOf, MathContext mc) {
-        this(repayments, new ArrayList<>(1), loanProductRelatedDetail, 
installmentAmountInMultiplesOf, mc);
+    private final List<RepaymentPeriod> repaymentPeriods;
+    private final List<InterestRate> interestRates;
+    private final LoanProductRelatedDetail loanProductRelatedDetail;
+    private final Integer installmentAmountInMultiplesOf;
+
+    public ProgressiveLoanInterestScheduleModel(List<RepaymentPeriod> 
repaymentPeriods, LoanProductRelatedDetail loanProductRelatedDetail,
+            Integer installmentAmountInMultiplesOf) {
+        this.repaymentPeriods = repaymentPeriods;
+        this.interestRates = new ArrayList<>();
+        this.loanProductRelatedDetail = loanProductRelatedDetail;
+        this.installmentAmountInMultiplesOf = installmentAmountInMultiplesOf;
+    }
+
+    private ProgressiveLoanInterestScheduleModel(List<RepaymentPeriod> 
repaymentPeriods, final List<InterestRate> interestRates,
+            LoanProductRelatedDetail loanProductRelatedDetail, Integer 
installmentAmountInMultiplesOf) {
+        this.repaymentPeriods = copyRepaymentPeriods(repaymentPeriods);
+        this.interestRates = new ArrayList<>(interestRates);
+        this.loanProductRelatedDetail = loanProductRelatedDetail;
+        this.installmentAmountInMultiplesOf = installmentAmountInMultiplesOf;
+    }
+
+    public ProgressiveLoanInterestScheduleModel deepCopy() {
+        return new ProgressiveLoanInterestScheduleModel(repaymentPeriods, 
interestRates, loanProductRelatedDetail,
+                installmentAmountInMultiplesOf);
     }
 
-    public void addInterestRate(final LocalDate newInterestDueDate, final 
BigDecimal newInterestRate) {
-        interestRates.add(new ProgressiveLoanInterestRate(newInterestDueDate, 
newInterestDueDate.plusDays(1), newInterestRate));
-        interestRates.sort(Collections.reverseOrder());
+    private List<RepaymentPeriod> copyRepaymentPeriods(final 
List<RepaymentPeriod> repaymentPeriods) {
+        final List<RepaymentPeriod> repaymentCopies = new 
ArrayList<>(repaymentPeriods.size());
+        RepaymentPeriod previousPeriod = null;
+        for (RepaymentPeriod repaymentPeriod : repaymentPeriods) {
+            RepaymentPeriod currentPeriod = new 
RepaymentPeriod(previousPeriod, repaymentPeriod);
+            previousPeriod = currentPeriod;
+            repaymentCopies.add(currentPeriod);
+        }
+        return repaymentCopies;
     }
 
     public BigDecimal getInterestRate(final LocalDate effectiveDate) {
         return interestRates.isEmpty() ? 
loanProductRelatedDetail.getAnnualNominalInterestRate() : 
findInterestRate(effectiveDate);
     }
 
     private BigDecimal findInterestRate(final LocalDate effectiveDate) {
-        return interestRates.stream().filter(ir -> 
!ir.effectiveFrom().isAfter(effectiveDate))
-                
.map(ProgressiveLoanInterestRate::interestRate).findFirst().orElse(loanProductRelatedDetail.getAnnualNominalInterestRate());
+        return interestRates.stream().filter(ir -> 
!ir.effectiveFrom().isAfter(effectiveDate)).map(InterestRate::interestRate).findFirst()
+                
.orElse(loanProductRelatedDetail.getAnnualNominalInterestRate());
+    }
+
+    public Optional<RepaymentPeriod> findRepaymentPeriod(final LocalDate 
repaymentPeriodDueDate) {
+        if (repaymentPeriodDueDate == null) {
+            return Optional.empty();
+        }
+        return repaymentPeriods.stream()//
+                .filter(repaymentPeriodItem -> 
repaymentPeriodItem.getDueDate().isEqual(repaymentPeriodDueDate))//
+                .findFirst();
+    }
+
+    public List<RepaymentPeriod> getRelatedRepaymentPeriods(final LocalDate 
calculateFromRepaymentPeriodDueDate) {
+        if (calculateFromRepaymentPeriodDueDate == null) {
+            return repaymentPeriods;
+        }
+        return repaymentPeriods.stream()//
+                .filter(period -> 
!period.getDueDate().isBefore(calculateFromRepaymentPeriodDueDate))//
+                .toList();//
     }
 
     public int getLoanTermInDays() {
-        if (repayments.isEmpty()) {
+        if (repaymentPeriods.isEmpty()) {
             return 0;
         }
-        final var firstPeriod = repayments.get(0);
-        final var lastPeriod = repayments.size() > 1 ? 
repayments.get(repayments.size() - 1) : firstPeriod;
+        final RepaymentPeriod firstPeriod = repaymentPeriods.get(0);
+        final RepaymentPeriod lastPeriod = repaymentPeriods.size() > 1 ? 
repaymentPeriods.get(repaymentPeriods.size() - 1) : firstPeriod;

Review Comment:
   Seems like again relying on a non-existing invariant.



##########
fineract-progressive-loan/src/main/java/org/apache/fineract/portfolio/loanaccount/loanschedule/data/InterestPeriod.java:
##########
@@ -0,0 +1,110 @@
+/**
+ * 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.loanaccount.loanschedule.data;
+
+import jakarta.validation.constraints.NotNull;
+import java.math.BigDecimal;
+import java.time.LocalDate;
+import java.util.Optional;
+import lombok.EqualsAndHashCode;
+import lombok.Getter;
+import lombok.Setter;
+import org.apache.fineract.infrastructure.core.service.MathUtil;
+import org.apache.fineract.organisation.monetary.domain.Money;
+
+@Getter
+@EqualsAndHashCode(exclude = { "repaymentPeriod" })
+public class InterestPeriod implements Comparable<InterestPeriod> {
+
+    private final RepaymentPeriod repaymentPeriod;
+    private final LocalDate fromDate;
+    @Setter
+    private LocalDate dueDate;
+    @Setter
+    private BigDecimal rateFactor;
+    private Money disbursementAmount;
+    private Money balanceCorrectionAmount;
+    private Money outstandingLoanBalance;
+
+    public InterestPeriod(RepaymentPeriod repaymentPeriod, LocalDate fromDate, 
LocalDate dueDate, BigDecimal rateFactor,
+            Money disbursementAmount, Money balanceCorrectionAmount, Money 
outstandingLoanBalance) {
+        this.repaymentPeriod = repaymentPeriod;
+        this.fromDate = fromDate;
+        this.dueDate = dueDate;
+        this.rateFactor = rateFactor;
+        this.disbursementAmount = disbursementAmount;
+        this.balanceCorrectionAmount = balanceCorrectionAmount;
+        this.outstandingLoanBalance = outstandingLoanBalance;
+    }

Review Comment:
   I think this is easily could be converted into a Builder rather, because we 
can mess up the order of the params.



##########
fineract-progressive-loan/src/main/java/org/apache/fineract/portfolio/loanaccount/loanschedule/data/RepaymentPeriod.java:
##########
@@ -0,0 +1,176 @@
+/**
+ * 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.loanaccount.loanschedule.data;
+
+import java.math.BigDecimal;
+import java.time.LocalDate;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.Optional;
+import lombok.EqualsAndHashCode;
+import lombok.Getter;
+import lombok.Setter;
+import lombok.ToString;
+import org.apache.fineract.infrastructure.core.service.MathUtil;
+import org.apache.fineract.organisation.monetary.domain.Money;
+
+@ToString
+@EqualsAndHashCode(exclude = { "previous", "next" })
+public class RepaymentPeriod {
+
+    @ToString.Exclude
+    private final RepaymentPeriod previous;
+    @Getter
+    private final LocalDate fromDate;
+    @Getter
+    private final LocalDate dueDate;
+    @Getter
+    private final List<InterestPeriod> interestPeriods;
+    @Setter
+    @ToString.Exclude
+    private RepaymentPeriod next;
+    @Setter
+    @Getter
+    private Money emi;
+    @Getter
+    private Money paidPrincipal;
+    @Getter
+    private Money paidInterest;
+
+    public RepaymentPeriod(RepaymentPeriod previous, LocalDate fromDate, 
LocalDate dueDate, Money emi) {
+        this.previous = previous;
+        if (previous != null) {
+            previous.setNext(this);
+        }
+        this.fromDate = fromDate;
+        this.dueDate = dueDate;
+        this.emi = emi;
+        this.interestPeriods = new ArrayList<>();
+        // There is always at least 1 interest period, by default with same 
from-due date as repayment period
+        getInterestPeriods().add(new InterestPeriod(this, getFromDate(), 
getDueDate(), BigDecimal.ZERO, getZero(), getZero(), getZero()));
+        this.paidInterest = getZero();
+        this.paidPrincipal = getZero();
+    }
+
+    public RepaymentPeriod(RepaymentPeriod previous, RepaymentPeriod 
repaymentPeriod) {
+        this.previous = previous;
+        if (previous != null) {
+            previous.setNext(this);
+        }
+        this.fromDate = repaymentPeriod.fromDate;
+        this.dueDate = repaymentPeriod.dueDate;
+        this.emi = repaymentPeriod.emi;
+        this.interestPeriods = new ArrayList<>();
+        this.paidPrincipal = repaymentPeriod.paidPrincipal;
+        this.paidInterest = repaymentPeriod.paidInterest;
+        // There is always at least 1 interest period, by default with same 
from-due date as repayment period
+        for (InterestPeriod interestPeriod : repaymentPeriod.interestPeriods) {
+            interestPeriods.add(new InterestPeriod(this, interestPeriod));
+        }
+    }
+
+    public void addInterestPeriod(InterestPeriod interestPeriod) {
+        interestPeriods.add(interestPeriod);
+        Collections.sort(interestPeriods);

Review Comment:
   Why don't we use a sorted data structure instead? What about a TreeSet?



##########
fineract-progressive-loan/src/main/java/org/apache/fineract/portfolio/loanaccount/loanschedule/data/InterestPeriod.java:
##########
@@ -0,0 +1,110 @@
+/**
+ * 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.loanaccount.loanschedule.data;
+
+import jakarta.validation.constraints.NotNull;
+import java.math.BigDecimal;
+import java.time.LocalDate;
+import java.util.Optional;
+import lombok.EqualsAndHashCode;
+import lombok.Getter;
+import lombok.Setter;
+import org.apache.fineract.infrastructure.core.service.MathUtil;
+import org.apache.fineract.organisation.monetary.domain.Money;
+
+@Getter
+@EqualsAndHashCode(exclude = { "repaymentPeriod" })
+public class InterestPeriod implements Comparable<InterestPeriod> {
+
+    private final RepaymentPeriod repaymentPeriod;
+    private final LocalDate fromDate;
+    @Setter
+    private LocalDate dueDate;
+    @Setter
+    private BigDecimal rateFactor;
+    private Money disbursementAmount;
+    private Money balanceCorrectionAmount;
+    private Money outstandingLoanBalance;
+
+    public InterestPeriod(RepaymentPeriod repaymentPeriod, LocalDate fromDate, 
LocalDate dueDate, BigDecimal rateFactor,
+            Money disbursementAmount, Money balanceCorrectionAmount, Money 
outstandingLoanBalance) {
+        this.repaymentPeriod = repaymentPeriod;
+        this.fromDate = fromDate;
+        this.dueDate = dueDate;
+        this.rateFactor = rateFactor;
+        this.disbursementAmount = disbursementAmount;
+        this.balanceCorrectionAmount = balanceCorrectionAmount;
+        this.outstandingLoanBalance = outstandingLoanBalance;
+    }
+
+    public InterestPeriod(RepaymentPeriod repaymentPeriod, InterestPeriod 
interestPeriod) {
+        this(repaymentPeriod, interestPeriod.getFromDate(), 
interestPeriod.getDueDate(), interestPeriod.getRateFactor(),
+                interestPeriod.getDisbursementAmount(), 
interestPeriod.getBalanceCorrectionAmount(),
+                interestPeriod.getOutstandingLoanBalance());
+    }
+
+    @Override
+    public int compareTo(@NotNull InterestPeriod o) {
+        return dueDate.compareTo(o.dueDate);
+    }
+
+    public void addBalanceCorrectionAmount(final Money 
balanceCorrectionAmount) {
+        this.balanceCorrectionAmount = 
MathUtil.plus(this.balanceCorrectionAmount, balanceCorrectionAmount);
+    }
+
+    public void addDisbursementAmount(final Money disbursementAmount) {
+        this.disbursementAmount = MathUtil.plus(this.disbursementAmount, 
disbursementAmount);
+    }
+
+    public Money getCalculatedDueInterest() {
+        return getOutstandingLoanBalance().multipliedBy(getRateFactor());
+    }
+
+    public void updateOutstandingLoanBalance() {
+        if (isFirstInterestPeriod()) {
+            Optional<RepaymentPeriod> previousRepaymentPeriod = 
getRepaymentPeriod().getPrevious();
+            if (previousRepaymentPeriod.isPresent()) {
+                InterestPeriod previousInterestPeriod = 
previousRepaymentPeriod.get().getInterestPeriods()
+                        
.get(previousRepaymentPeriod.get().getInterestPeriods().size() - 1);
+                this.outstandingLoanBalance = 
previousInterestPeriod.getOutstandingLoanBalance()//
+                        .plus(previousInterestPeriod.getDisbursementAmount())//
+                        
.plus(previousInterestPeriod.getBalanceCorrectionAmount())//
+                        
.minus(previousRepaymentPeriod.get().getDuePrincipal())//
+                        
.plus(previousRepaymentPeriod.get().getPaidPrincipal());//
+            }
+        } else {
+            int index = 
getRepaymentPeriod().getInterestPeriods().indexOf(this);
+            InterestPeriod previousInterestPeriod = 
getRepaymentPeriod().getInterestPeriods().get(index - 1);
+            this.outstandingLoanBalance = 
previousInterestPeriod.getOutstandingLoanBalance() //
+                    .plus(previousInterestPeriod.getBalanceCorrectionAmount()) 
//
+                    .plus(previousInterestPeriod.getDisbursementAmount()); //
+        }
+    }
+
+    private boolean isFirstInterestPeriod() {
+        return getRepaymentPeriod().getInterestPeriods().get(0).equals(this);

Review Comment:
   Doesn't this rely on a false invariant again that the periods are ordered? 
Is this enforced somewhere?



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


Reply via email to