galovics commented on code in PR #4098: URL: https://github.com/apache/fineract/pull/4098#discussion_r1801174763
########## fineract-loan/src/main/java/org/apache/fineract/portfolio/loanaccount/service/InterestRefundService.java: ########## @@ -0,0 +1,35 @@ +/** + * 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.service; + +import java.math.BigDecimal; +import java.time.LocalDate; +import org.apache.fineract.portfolio.loanaccount.domain.Loan; + +public interface InterestRefundService { + + boolean canHandle(Loan loan); + + BigDecimal calculateTotalPaidInterest(Loan loan); + + BigDecimal calculateInterestRefundAmount(Loan loan, BigDecimal relatedRefundTransactionAmount, LocalDate relatedRefundTransactionDate, + BigDecimal totalPaidInterest, BigDecimal totalPayableInterestBeforeTransaction); Review Comment: Frankly speaking the existing codebase works with tons of methods with a lot of parameters. Since the types are the same, I'd say a parameter object here would make sense - especially that it can be easily extended in the future. Thoughts? Also, consider it for the other methods as well just for consistency. ########## fineract-progressive-loan/src/main/java/org/apache/fineract/portfolio/loanaccount/service/ProgressiveLoanInterestRefundServiceImpl.java: ########## @@ -0,0 +1,130 @@ +/** + * 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.service; + +import java.math.BigDecimal; +import java.time.LocalDate; +import java.util.List; +import java.util.concurrent.atomic.AtomicReference; +import lombok.RequiredArgsConstructor; +import lombok.extern.slf4j.Slf4j; +import org.apache.fineract.organisation.monetary.domain.MonetaryCurrency; +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.transactionprocessor.impl.AdvancedPaymentScheduleTransactionProcessor; +import org.apache.fineract.portfolio.loanaccount.loanschedule.data.ProgressiveLoanInterestScheduleModel; +import org.apache.fineract.portfolio.loanaccount.starter.AdvancedPaymentScheduleTransactionProcessorCondition; +import org.apache.fineract.portfolio.loanproduct.calc.EMICalculator; +import org.springframework.context.annotation.Conditional; +import org.springframework.stereotype.Service; + +@Slf4j +@RequiredArgsConstructor +@Conditional(AdvancedPaymentScheduleTransactionProcessorCondition.class) +@Service +public class ProgressiveLoanInterestRefundServiceImpl implements InterestRefundService { + + private final AdvancedPaymentScheduleTransactionProcessor processor; + private final EMICalculator emiCalculator; + + private static LoanRepaymentScheduleInstallment duplicate(LoanRepaymentScheduleInstallment rs) { + MonetaryCurrency currency = rs.getLoan().getCurrency(); + + return new LoanRepaymentScheduleInstallment(rs.getLoan(), rs.getInstallmentNumber(), rs.getFromDate(), rs.getDueDate(), + rs.getPrincipal(currency).getAmount(), rs.getInterestCharged(currency).getAmount(), + rs.getFeeChargesCharged(currency).getAmount(), rs.getPenaltyChargesCharged(currency).getAmount(), + rs.isRecalculatedInterestComponent(), rs.getLoanCompoundingDetails(), rs.getRescheduleInterestPortion(), + rs.isDownPayment()); + } Review Comment: I'd say this would be rather a responsibility of a separate class, what do you think? ########## fineract-progressive-loan/src/main/java/org/apache/fineract/portfolio/loanaccount/service/ProgressiveLoanInterestRefundServiceImpl.java: ########## @@ -0,0 +1,130 @@ +/** + * 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.service; + +import java.math.BigDecimal; +import java.time.LocalDate; +import java.util.List; +import java.util.concurrent.atomic.AtomicReference; +import lombok.RequiredArgsConstructor; +import lombok.extern.slf4j.Slf4j; +import org.apache.fineract.organisation.monetary.domain.MonetaryCurrency; +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.transactionprocessor.impl.AdvancedPaymentScheduleTransactionProcessor; +import org.apache.fineract.portfolio.loanaccount.loanschedule.data.ProgressiveLoanInterestScheduleModel; +import org.apache.fineract.portfolio.loanaccount.starter.AdvancedPaymentScheduleTransactionProcessorCondition; +import org.apache.fineract.portfolio.loanproduct.calc.EMICalculator; +import org.springframework.context.annotation.Conditional; +import org.springframework.stereotype.Service; + +@Slf4j +@RequiredArgsConstructor +@Conditional(AdvancedPaymentScheduleTransactionProcessorCondition.class) +@Service +public class ProgressiveLoanInterestRefundServiceImpl implements InterestRefundService { + + private final AdvancedPaymentScheduleTransactionProcessor processor; + private final EMICalculator emiCalculator; + + private static LoanRepaymentScheduleInstallment duplicate(LoanRepaymentScheduleInstallment rs) { + MonetaryCurrency currency = rs.getLoan().getCurrency(); + + return new LoanRepaymentScheduleInstallment(rs.getLoan(), rs.getInstallmentNumber(), rs.getFromDate(), rs.getDueDate(), + rs.getPrincipal(currency).getAmount(), rs.getInterestCharged(currency).getAmount(), + rs.getFeeChargesCharged(currency).getAmount(), rs.getPenaltyChargesCharged(currency).getAmount(), + rs.isRecalculatedInterestComponent(), rs.getLoanCompoundingDetails(), rs.getRescheduleInterestPortion(), + rs.isDownPayment()); + } + + private ProgressiveLoanInterestScheduleModel getModelSafely(final Loan loan, BigDecimal refundAmount) { + final AtomicReference<BigDecimal> refundFinal = new AtomicReference<>(refundAmount); + List<LoanTransaction> withoutSpecificDisbursements = loan.getLoanTransactions().stream() + .filter(lt -> !lt.isAccrual() && !lt.isAccrualActivity() && !lt.isReversed()).filter(lt -> { + if (lt.getTypeOf().isDisbursement() && refundFinal.get().compareTo(BigDecimal.ZERO) > 0) { + // + if (lt.getAmount().compareTo(refundFinal.get()) <= 0) { + refundFinal.set(refundFinal.get().subtract(lt.getAmount())); + return false; + } + } + return true; + }).toList(); + List<LoanTransaction> transactionsToReprocess = withoutSpecificDisbursements.stream().map(lt -> { + if (lt.getTypeOf().isDisbursement() && refundFinal.get().compareTo(BigDecimal.ZERO) > 0) { + refundFinal.set(BigDecimal.ZERO); + return new LoanTransaction(lt.getLoan(), lt.getLoan().getOffice(), lt.getTypeOf().getValue(), lt.getDateOf(), + lt.getAmount().subtract(refundFinal.get()), lt.getPrincipalPortion(), lt.getInterestPortion(), + lt.getFeeChargesPortion(), lt.getPenaltyChargesPortion(), + lt.getOverPaymentPortion(lt.getLoan().getCurrency()).getAmount(), lt.isReversed(), lt.getPaymentDetail(), + lt.getExternalId()); + } + return new LoanTransaction(lt.getLoan(), lt.getLoan().getOffice(), lt.getTypeOf().getValue(), lt.getDateOf(), lt.getAmount(), + lt.getPrincipalPortion(), lt.getInterestPortion(), lt.getFeeChargesPortion(), lt.getPenaltyChargesPortion(), + lt.getOverPaymentPortion(lt.getLoan().getCurrency()).getAmount(), lt.isReversed(), lt.getPaymentDetail(), + lt.getExternalId()); Review Comment: Since you've already created a copy method for the LoanRepaymentScheduleInstallment, can't we create a similar one for LoanTransaction as well? Parameter usage seems excessive especially we can easily miss a param. ########## fineract-progressive-loan/src/main/java/org/apache/fineract/portfolio/loanaccount/service/ProgressiveLoanInterestRefundServiceImpl.java: ########## @@ -0,0 +1,130 @@ +/** + * 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.service; + +import java.math.BigDecimal; +import java.time.LocalDate; +import java.util.List; +import java.util.concurrent.atomic.AtomicReference; +import lombok.RequiredArgsConstructor; +import lombok.extern.slf4j.Slf4j; +import org.apache.fineract.organisation.monetary.domain.MonetaryCurrency; +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.transactionprocessor.impl.AdvancedPaymentScheduleTransactionProcessor; +import org.apache.fineract.portfolio.loanaccount.loanschedule.data.ProgressiveLoanInterestScheduleModel; +import org.apache.fineract.portfolio.loanaccount.starter.AdvancedPaymentScheduleTransactionProcessorCondition; +import org.apache.fineract.portfolio.loanproduct.calc.EMICalculator; +import org.springframework.context.annotation.Conditional; +import org.springframework.stereotype.Service; + +@Slf4j +@RequiredArgsConstructor +@Conditional(AdvancedPaymentScheduleTransactionProcessorCondition.class) +@Service +public class ProgressiveLoanInterestRefundServiceImpl implements InterestRefundService { + + private final AdvancedPaymentScheduleTransactionProcessor processor; + private final EMICalculator emiCalculator; + + private static LoanRepaymentScheduleInstallment duplicate(LoanRepaymentScheduleInstallment rs) { + MonetaryCurrency currency = rs.getLoan().getCurrency(); + + return new LoanRepaymentScheduleInstallment(rs.getLoan(), rs.getInstallmentNumber(), rs.getFromDate(), rs.getDueDate(), + rs.getPrincipal(currency).getAmount(), rs.getInterestCharged(currency).getAmount(), + rs.getFeeChargesCharged(currency).getAmount(), rs.getPenaltyChargesCharged(currency).getAmount(), + rs.isRecalculatedInterestComponent(), rs.getLoanCompoundingDetails(), rs.getRescheduleInterestPortion(), + rs.isDownPayment()); + } + + private ProgressiveLoanInterestScheduleModel getModelSafely(final Loan loan, BigDecimal refundAmount) { + final AtomicReference<BigDecimal> refundFinal = new AtomicReference<>(refundAmount); + List<LoanTransaction> withoutSpecificDisbursements = loan.getLoanTransactions().stream() + .filter(lt -> !lt.isAccrual() && !lt.isAccrualActivity() && !lt.isReversed()).filter(lt -> { + if (lt.getTypeOf().isDisbursement() && refundFinal.get().compareTo(BigDecimal.ZERO) > 0) { + // + if (lt.getAmount().compareTo(refundFinal.get()) <= 0) { + refundFinal.set(refundFinal.get().subtract(lt.getAmount())); + return false; + } + } + return true; + }).toList(); + List<LoanTransaction> transactionsToReprocess = withoutSpecificDisbursements.stream().map(lt -> { + if (lt.getTypeOf().isDisbursement() && refundFinal.get().compareTo(BigDecimal.ZERO) > 0) { + refundFinal.set(BigDecimal.ZERO); + return new LoanTransaction(lt.getLoan(), lt.getLoan().getOffice(), lt.getTypeOf().getValue(), lt.getDateOf(), + lt.getAmount().subtract(refundFinal.get()), lt.getPrincipalPortion(), lt.getInterestPortion(), + lt.getFeeChargesPortion(), lt.getPenaltyChargesPortion(), + lt.getOverPaymentPortion(lt.getLoan().getCurrency()).getAmount(), lt.isReversed(), lt.getPaymentDetail(), + lt.getExternalId()); + } + return new LoanTransaction(lt.getLoan(), lt.getLoan().getOffice(), lt.getTypeOf().getValue(), lt.getDateOf(), lt.getAmount(), + lt.getPrincipalPortion(), lt.getInterestPortion(), lt.getFeeChargesPortion(), lt.getPenaltyChargesPortion(), + lt.getOverPaymentPortion(lt.getLoan().getCurrency()).getAmount(), lt.isReversed(), lt.getPaymentDetail(), + lt.getExternalId()); + }).toList(); + + List<LoanRepaymentScheduleInstallment> installmentsToReprocess = new java.util.ArrayList<>( Review Comment: Why the fully qualified name for ArrayList? ########## fineract-progressive-loan/src/main/java/org/apache/fineract/portfolio/loanaccount/service/ProgressiveLoanInterestRefundServiceImpl.java: ########## @@ -0,0 +1,130 @@ +/** + * 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.service; + +import java.math.BigDecimal; +import java.time.LocalDate; +import java.util.List; +import java.util.concurrent.atomic.AtomicReference; +import lombok.RequiredArgsConstructor; +import lombok.extern.slf4j.Slf4j; +import org.apache.fineract.organisation.monetary.domain.MonetaryCurrency; +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.transactionprocessor.impl.AdvancedPaymentScheduleTransactionProcessor; +import org.apache.fineract.portfolio.loanaccount.loanschedule.data.ProgressiveLoanInterestScheduleModel; +import org.apache.fineract.portfolio.loanaccount.starter.AdvancedPaymentScheduleTransactionProcessorCondition; +import org.apache.fineract.portfolio.loanproduct.calc.EMICalculator; +import org.springframework.context.annotation.Conditional; +import org.springframework.stereotype.Service; + +@Slf4j +@RequiredArgsConstructor +@Conditional(AdvancedPaymentScheduleTransactionProcessorCondition.class) +@Service +public class ProgressiveLoanInterestRefundServiceImpl implements InterestRefundService { + + private final AdvancedPaymentScheduleTransactionProcessor processor; + private final EMICalculator emiCalculator; + + private static LoanRepaymentScheduleInstallment duplicate(LoanRepaymentScheduleInstallment rs) { + MonetaryCurrency currency = rs.getLoan().getCurrency(); + + return new LoanRepaymentScheduleInstallment(rs.getLoan(), rs.getInstallmentNumber(), rs.getFromDate(), rs.getDueDate(), + rs.getPrincipal(currency).getAmount(), rs.getInterestCharged(currency).getAmount(), + rs.getFeeChargesCharged(currency).getAmount(), rs.getPenaltyChargesCharged(currency).getAmount(), + rs.isRecalculatedInterestComponent(), rs.getLoanCompoundingDetails(), rs.getRescheduleInterestPortion(), + rs.isDownPayment()); + } + + private ProgressiveLoanInterestScheduleModel getModelSafely(final Loan loan, BigDecimal refundAmount) { + final AtomicReference<BigDecimal> refundFinal = new AtomicReference<>(refundAmount); + List<LoanTransaction> withoutSpecificDisbursements = loan.getLoanTransactions().stream() + .filter(lt -> !lt.isAccrual() && !lt.isAccrualActivity() && !lt.isReversed()).filter(lt -> { + if (lt.getTypeOf().isDisbursement() && refundFinal.get().compareTo(BigDecimal.ZERO) > 0) { + // + if (lt.getAmount().compareTo(refundFinal.get()) <= 0) { + refundFinal.set(refundFinal.get().subtract(lt.getAmount())); + return false; + } + } + return true; + }).toList(); + List<LoanTransaction> transactionsToReprocess = withoutSpecificDisbursements.stream().map(lt -> { + if (lt.getTypeOf().isDisbursement() && refundFinal.get().compareTo(BigDecimal.ZERO) > 0) { + refundFinal.set(BigDecimal.ZERO); + return new LoanTransaction(lt.getLoan(), lt.getLoan().getOffice(), lt.getTypeOf().getValue(), lt.getDateOf(), + lt.getAmount().subtract(refundFinal.get()), lt.getPrincipalPortion(), lt.getInterestPortion(), + lt.getFeeChargesPortion(), lt.getPenaltyChargesPortion(), + lt.getOverPaymentPortion(lt.getLoan().getCurrency()).getAmount(), lt.isReversed(), lt.getPaymentDetail(), + lt.getExternalId()); + } + return new LoanTransaction(lt.getLoan(), lt.getLoan().getOffice(), lt.getTypeOf().getValue(), lt.getDateOf(), lt.getAmount(), + lt.getPrincipalPortion(), lt.getInterestPortion(), lt.getFeeChargesPortion(), lt.getPenaltyChargesPortion(), + lt.getOverPaymentPortion(lt.getLoan().getCurrency()).getAmount(), lt.isReversed(), lt.getPaymentDetail(), + lt.getExternalId()); + }).toList(); + + List<LoanRepaymentScheduleInstallment> installmentsToReprocess = new java.util.ArrayList<>( + loan.getRepaymentScheduleInstallments().stream().filter(i -> !i.isReAged() && !i.isAdditional()) + .map(ProgressiveLoanInterestRefundServiceImpl::duplicate).toList()); + + return processor.reprocessProgressiveLoanTransactions(loan.getDisbursementDate(), transactionsToReprocess, loan.getCurrency(), + installmentsToReprocess, loan.getActiveCharges()).getRight(); + } + + @Override + public boolean canHandle(Loan loan) { + return loan != null && loan.isInterestBearing() + && (AdvancedPaymentScheduleTransactionProcessor.ADVANCED_PAYMENT_ALLOCATION_STRATEGY + .equalsIgnoreCase(loan.getTransactionProcessingStrategyCode()) + || AdvancedPaymentScheduleTransactionProcessor.ADVANCED_PAYMENT_ALLOCATION_STRATEGY_NAME + .equalsIgnoreCase(loan.getTransactionProcessingStrategyName())); Review Comment: Is this even possible that the allocation strategy code doesn't match the name? ########## fineract-provider/src/main/java/org/apache/fineract/portfolio/loanaccount/domain/LoanAccountDomainServiceJpa.java: ########## @@ -159,6 +164,10 @@ public void updateLoanCollateralStatus(Set<LoanCollateralManagement> loanCollate this.loanCollateralManagementRepository.saveAll(loanCollateralManagementSet); } + private InterestRefundService lookUpInterestRefundService(final Loan finalLoan) { + return interestRefundServices.stream().filter(service -> service.canHandle(finalLoan)).findFirst().orElse(null); + } Review Comment: I'd say let's move this responsibility out of this class. Rather create a Delegate class for this. Something like InterestRefundServiceDelegate -> does this logic -> calls the right implementation. ########## fineract-provider/src/main/java/org/apache/fineract/portfolio/loanaccount/domain/LoanAccountDomainServiceJpa.java: ########## @@ -170,6 +179,19 @@ public LoanTransaction makeRepayment(final LoanTransactionType repaymentTransact LoanBusinessEvent repaymentEvent = getLoanRepaymentTypeBusinessEvent(repaymentTransactionType, isRecoveryRepayment, loan); businessEventNotifierService.notifyPreBusinessEvent(repaymentEvent); + boolean shouldCreateInterestRefundTransaction = loan.getLoanProductRelatedDetail().getSupportedInterestRefundTypes().stream() + .map(LoanSupportedInterestRefundTypes::getTransactionType) + .anyMatch(transactionType -> transactionType.equals(repaymentTransactionType)); + InterestRefundService interestRefundService = lookUpInterestRefundService(loan); + BigDecimal interestRefundAmount = BigDecimal.ZERO; + + if (shouldCreateInterestRefundTransaction && interestRefundService != null) { + BigDecimal totalPaidInterest = interestRefundService.calculateTotalPaidInterest(loan); + BigDecimal totalPayableInterestBeforeTransaction = interestRefundService.calculatePayableInterest(loan, transactionDate); + interestRefundAmount = interestRefundService.calculateInterestRefundAmount(loan, transactionAmount, transactionDate, + totalPaidInterest, totalPayableInterestBeforeTransaction); + } Review Comment: Probably I don't understand something but why are we creating interest refund transactions when we're making a repayment? Isn't it all about the ability to refund the interest portion of an existing tx? -- 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]
