[GitHub] [fineract] galovics commented on a diff in pull request #2724: FINERACT-1761: New Charge Adjustment transaction

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

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


##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/loanaccount/api/LoanChargesApiResource.java:
##########
@@ -224,13 +220,16 @@ public String executeLoanCharge(@PathParam("loanId") @Parameter(description = "l
             @Parameter(hidden = true) final String apiRequestBodyAsJson) {
 
         final CommandWrapperBuilder builder = new CommandWrapperBuilder().withJson(apiRequestBodyAsJson);
-        CommandProcessingResult result = null;
-        if (is(commandParam, "waive")) {
+        CommandProcessingResult result;
+        if (CommandParameterUtil.is(commandParam, "waive")) {
             final CommandWrapper commandRequest = builder.waiveLoanCharge(loanId, loanChargeId).build();
             result = this.commandsSourceWritePlatformService.logCommandSource(commandRequest);
-        } else if (is(commandParam, "pay")) {
+        } else if (CommandParameterUtil.is(commandParam, "pay")) {
             final CommandWrapper commandRequest = builder.payLoanCharge(loanId, loanChargeId).build();
             result = this.commandsSourceWritePlatformService.logCommandSource(commandRequest);
+        } else if (CommandParameterUtil.is(commandParam, "adjustment")) {

Review Comment:
   Slight cleanup but can you pls extract these magic strings into constants in this class? Thanks.



##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/charge/service/ChargeEnumerations.java:
##########
@@ -191,4 +192,36 @@ public static EnumOptionData chargePaymentMode(final ChargePaymentMode type) {
         return optionData;
     }
 
+    public static EnumOptionData feeFrequencyType(final int id) {
+        return feeFrequencyType(PeriodFrequencyType.fromInt(id));
+    }
+
+    public static EnumOptionData feeFrequencyType(final PeriodFrequencyType frequencyType) {
+        EnumOptionData optionData = null;
+        switch (frequencyType) {
+            case DAYS -> {
+                optionData = new EnumOptionData(PeriodFrequencyType.DAYS.getValue().longValue(), PeriodFrequencyType.DAYS.getCode(),
+                        "Daily");
+            }

Review Comment:
   Dummy question but don't you need a break here and for the other cases?



##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/loanaccount/domain/LoanCharge.java:
##########
@@ -1101,4 +1105,18 @@ public ChargeTimeType getChargeTimeType() {
         return ChargeTimeType.fromInt(this.chargeTime);
     }
 
+    public LoanChargeData toData() {
+        EnumOptionData chargeTimeTypeData = new EnumOptionData((long) getChargeTimeType().ordinal(), getChargeTimeType().getCode(),
+                String.valueOf(getChargeTimeType().getValue()));
+        EnumOptionData chargeCalculationTypeData = new EnumOptionData((long) getChargeCalculation().ordinal(),
+                getChargeCalculation().getCode(), String.valueOf(getChargeCalculation().getValue()));
+        EnumOptionData chargePaymentModeData = new EnumOptionData((long) getChargePaymentMode().ordinal(), getChargePaymentMode().getCode(),
+                String.valueOf(getChargePaymentMode().getValue()));
+        Set<LoanInstallmentChargeData> loanInstallmentChargeDataSet = installmentCharges().stream().map(LoanInstallmentCharge::toData)
+                .collect(Collectors.toSet());
+        return new LoanChargeData(getId(), getCharge().getId(), getCharge().getName(), getCharge().toData().getCurrency(), amount,

Review Comment:
   Probably a builder would be much better for readability in this case. (@Builder annotation from Lombok could be an option on the class)



##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/loanaccount/domain/Loan.java:
##########
@@ -3379,7 +3379,7 @@ public List<LoanTransaction> retrieveListOfTransactionsByType(LoanTransactionTyp
     private boolean doPostLoanTransactionChecks(final LocalDate transactionDate,
             final LoanLifecycleStateMachine loanLifecycleStateMachine) {
         boolean statusChanged = false;
-        if (isOverPaid()) {
+        if (getTotalOverpaid() != null && getTotalOverpaid().compareTo(BigDecimal.ZERO) > 0) {

Review Comment:
   I think this condition could be extracted at least to a variable. It makes the code much easier to read.



##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/loanaccount/service/LoanReadPlatformServiceImpl.java:
##########
@@ -264,11 +263,10 @@ public Collection<LoanTransactionData> retrieveLoanTransactions(final Long loanI
             final String sql = "select " + rm.loanPaymentsSchema() + " where tr.loan_id = ? and tr.transaction_type_enum not in (0, 3) "
                     + " and (tr.is_reversed=false or tr.manually_adjusted_or_reversed = true)  order by tr.transaction_date ASC,id ";
             Collection<LoanTransactionData> loanTransactionData = this.jdbcTemplate.query(sql, rm, loanId); // NOSONAR
+            // TODO: rework

Review Comment:
   Is this something you forgot to do now and this was accidentally left here or the comment is for the future? If so, probably would be best if you explain it a little bit better whats the issue here - for future reference.



##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/loanaccount/handler/LoanChargeAdjustmentCommandHandler.java:
##########
@@ -0,0 +1,42 @@
+/**
+ * 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.handler;
+
+import lombok.RequiredArgsConstructor;
+import org.apache.fineract.commands.annotation.CommandType;
+import org.apache.fineract.commands.handler.NewCommandSourceHandler;
+import org.apache.fineract.infrastructure.core.api.JsonCommand;
+import org.apache.fineract.infrastructure.core.data.CommandProcessingResult;
+import org.apache.fineract.portfolio.loanaccount.service.LoanChargeWritePlatformService;
+import org.springframework.stereotype.Service;
+import org.springframework.transaction.annotation.Transactional;
+
+@Service
+@RequiredArgsConstructor
+@CommandType(entity = "LOANCHARGE", action = "ADJUSTMENT")
+public class LoanChargeAdjustmentCommandHandler implements NewCommandSourceHandler {
+
+    private final LoanChargeWritePlatformService writePlatformService;
+
+    @Transactional
+    @Override
+    public CommandProcessingResult processCommand(final JsonCommand command) {
+        return this.writePlatformService.adjustmentForLoanCharge(command.getLoanId(), command.entityId(), command);

Review Comment:
   No need for this.



##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/loanaccount/domain/DefaultLoanLifecycleStateMachine.java:
##########
@@ -18,23 +18,20 @@
  */
 package org.apache.fineract.portfolio.loanaccount.domain;
 
-import java.util.Arrays;
 import java.util.List;
 import lombok.RequiredArgsConstructor;
 import org.apache.fineract.infrastructure.event.business.domain.loan.LoanStatusChangedBusinessEvent;
 import org.apache.fineract.infrastructure.event.business.service.BusinessEventNotifierService;
+import org.springframework.stereotype.Component;
 
 // TODO: introduce tests for the state machine
+@Component

Review Comment:
   Nice job ;-)
   



##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/loanaccount/domain/DefaultLoanLifecycleStateMachine.java:
##########
@@ -164,54 +161,59 @@ private LoanStatus getNextStatus(LoanEvent loanEvent, Loan loan) {
                     newState = activeTransition();
                 }
             break;
+            case LOAN_CHARGE_ADJUSTMENT:
+                if (from.hasStateOf(LoanStatus.CLOSED_OBLIGATIONS_MET)) {
+                    newState = overpaidTransition();
+                }
+            break;
             default:
             break;
         }
         return newState;
     }
 
     private LoanStatus transferOnHold() {
-        return stateOf(LoanStatus.TRANSFER_ON_HOLD, this.allowedLoanStatuses);
+        return stateOf(LoanStatus.TRANSFER_ON_HOLD, this.ALLOWED_LOAN_STATUSES);
     }
 
     private LoanStatus transferInProgress() {
-        return stateOf(LoanStatus.TRANSFER_IN_PROGRESS, this.allowedLoanStatuses);
+        return stateOf(LoanStatus.TRANSFER_IN_PROGRESS, this.ALLOWED_LOAN_STATUSES);
     }
 
     private LoanStatus overpaidTransition() {
-        return stateOf(LoanStatus.OVERPAID, this.allowedLoanStatuses);
+        return stateOf(LoanStatus.OVERPAID, this.ALLOWED_LOAN_STATUSES);

Review Comment:
   No need for this here.



##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/loanaccount/domain/LoanInstallmentCharge.java:
##########
@@ -322,4 +323,9 @@ public LoanRepaymentScheduleInstallment getInstallment() {
     public void setInstallment(LoanRepaymentScheduleInstallment installment) {
         this.installment = installment;
     }
+
+    public LoanInstallmentChargeData toData() {
+        return new LoanInstallmentChargeData(installment.getInstallmentNumber(), installment.getDueDate(), amount, amountOutstanding,

Review Comment:
   Builder could be useful here too.



##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/loanaccount/service/LoanChargeReadPlatformServiceImpl.java:
##########
@@ -483,12 +484,15 @@ public LoanInstallmentChargeData mapRow(final ResultSet rs, @SuppressWarnings("u
             final Integer installmentNumber = rs.getInt("installmentNumber");
             final BigDecimal amountUnrecognized = rs.getBigDecimal("amountUnrecognized");
             LoanInstallmentChargeData installmentChargeData = this.installmentChargeDatas.get(installmentNumber);
-            return new LoanInstallmentChargeData(installmentChargeData, amountUnrecognized);
+            return new LoanInstallmentChargeData(installmentChargeData.getInstallmentNumber(), installmentChargeData.getDueDate(),

Review Comment:
   Builder?



##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/loanaccount/service/LoanChargeReadPlatformServiceImpl.java:
##########
@@ -443,11 +444,11 @@ public LoanInstallmentChargeData mapRow(final ResultSet rs, @SuppressWarnings("u
             final BigDecimal amount = rs.getBigDecimal("amount");
             final BigDecimal amountWaived = rs.getBigDecimal("amountWaived");
             final boolean paid = rs.getBoolean("paid");
-            final boolean waived = rs.getBoolean("waied");
+            final boolean waived = rs.getBoolean("waived");
             final BigDecimal amountAccrued = rs.getBigDecimal("amountAccrued");
 
             return new LoanInstallmentChargeData(installmentNumber, dueAsOfDate, amount, amountOutstanding, amountWaived, paid, waived,

Review Comment:
   Builder?



##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/loanaccount/domain/LoanTransactionRelationRepository.java:
##########
@@ -25,7 +25,6 @@
 public interface LoanTransactionRelationRepository
         extends JpaRepository<LoanTransactionRelation, Long>, JpaSpecificationExecutor<LoanTransactionRelation> {
 
-    List<LoanTransactionRelation> findByFromTransactionAndRelationType(LoanTransaction loanTransaction,
-            LoanTransactionRelationTypeEnum relationType);
+    List<LoanTransactionRelation> findByFromTransaction(LoanTransaction loanTransaction);

Review Comment:
   Let's name the parameter as fromTransaction just for consistency with the method naming.



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