[GitHub] [fineract] galovics commented on a diff in pull request #2928: FINERACT-1855-Arrears-aging-job-as-business-step

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

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


##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/loanaccount/jobs/updateloanarrearsageing/UpdateLoanArrearsAgeingTasklet.java:
##########
@@ -84,18 +98,25 @@ public RepeatStatus execute(StepContribution contribution, ChunkContext chunkCon
         }
 
         log.debug("{}: Records affected by updateLoanArrearsAgeingDetails: {}", ThreadLocalContextUtil.getTenant().getName(), result);
-        return RepeatStatus.FINISHED;
     }
 
-    private List<String> updateLoanArrearsAgeingDetailsWithOriginalSchedule() {
+    private List<String> updateLoanArrearsAgeingDetailsWithOriginalSchedule(String loanIdsForUpdate) {
         List<String> insertStatement = new ArrayList<>();
 
         final StringBuilder loanIdentifier = new StringBuilder();
         loanIdentifier.append("select ml.id as loanId FROM m_loan ml  ");

Review Comment:
   Ruchi, wild idea but don't we wanna use String blocks (introduced in Java15) to make this more readable? (`"""`)



##########
fineract-provider/src/test/java/org/apache/fineract/cob/loan/UpdateLoanArrearsAgeingTest.java:
##########
@@ -0,0 +1,158 @@
+/**
+ * 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.cob.loan;
+
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.mockito.ArgumentMatchers.anyBoolean;
+import static org.mockito.ArgumentMatchers.anyString;
+import static org.mockito.Mockito.anyList;
+import static org.mockito.Mockito.anyMap;
+import static org.mockito.Mockito.doNothing;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+import java.time.LocalDate;
+import java.time.ZoneId;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import org.apache.fineract.infrastructure.businessdate.domain.BusinessDateType;
+import org.apache.fineract.infrastructure.core.domain.FineractPlatformTenant;
+import org.apache.fineract.infrastructure.core.service.ThreadLocalContextUtil;
+import org.apache.fineract.infrastructure.core.service.database.DatabaseSpecificSQLGenerator;
+import org.apache.fineract.portfolio.loanaccount.domain.Loan;
+import org.apache.fineract.portfolio.loanaccount.jobs.updateloanarrearsageing.UpdateLoanArrearsAgeingTasklet;
+import org.apache.fineract.portfolio.loanaccount.service.LoanArrearsAgingService;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.mockito.ArgumentCaptor;
+import org.mockito.InjectMocks;
+import org.mockito.Mock;
+import org.mockito.Mockito;
+import org.mockito.junit.jupiter.MockitoExtension;
+import org.mockito.junit.jupiter.MockitoSettings;
+import org.mockito.quality.Strictness;
+import org.springframework.batch.core.StepContribution;
+import org.springframework.batch.core.scope.context.ChunkContext;
+import org.springframework.jdbc.core.JdbcTemplate;
+
+@ExtendWith(MockitoExtension.class)
+@MockitoSettings(strictness = Strictness.LENIENT)
+public class UpdateLoanArrearsAgeingTest {

Review Comment:
   I don't really see the value in a mocked test case where the whole logic is native queries. Can't we use a component test for this?



##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/loanaccount/jobs/updateloanarrearsageing/UpdateLoanArrearsAgeingTasklet.java:
##########
@@ -67,15 +74,22 @@ public RepeatStatus execute(StepContribution contribution, ChunkContext chunkCon
         updateSqlBuilder.append(" FROM m_loan ml ");
         updateSqlBuilder.append(" INNER JOIN m_loan_repayment_schedule mr on mr.loan_id = ml.id ");
         updateSqlBuilder.append(" left join m_product_loan_recalculation_details prd on prd.product_id = ml.product_id ");
-        updateSqlBuilder.append(" WHERE ml.loan_status_id = 300 "); // active
+        updateSqlBuilder.append(" WHERE ml.loan_status_id = 300 ");// active
+        if (!loanIdsForUpdate.isEmpty()) {
+            if (!loanIdsForUpdate.equalsIgnoreCase(ALL_ACTIVE_LOANS)) {
+                updateSqlBuilder.append(" and ml.id IN (");
+                updateSqlBuilder.append(loanIdsForUpdate);

Review Comment:
   Instead of directly appending it to the SQL string, let's use parameterized queries.



##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/loanaccount/jobs/updateloanarrearsageing/UpdateLoanArrearsAgeingTasklet.java:
##########
@@ -127,4 +148,28 @@ private List<Map<String, Object>> getLoanSummary(final String loanIdsAsString) {
         List<Map<String, Object>> loanSummary = this.jdbcTemplate.queryForList(transactionsSql.toString());
         return loanSummary;
     }
+
+    @Override
+    public Loan execute(Loan loan) {
+        Long loanId = loan.getId();
+
+        // delete existing record for loan from m_loan_arrears_aging table
+        final StringBuilder deleteSQL = new StringBuilder();
+        deleteSQL.append("delete from m_loan_arrears_aging where loan_id = ");
+        deleteSQL.append(loanId);

Review Comment:
   Let's use a parameterized query for this.



##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/loanaccount/jobs/updateloanarrearsageing/UpdateLoanArrearsAgeingTasklet.java:
##########
@@ -35,16 +37,21 @@
 
 @Slf4j
 @RequiredArgsConstructor
-public class UpdateLoanArrearsAgeingTasklet implements Tasklet {
+public class UpdateLoanArrearsAgeingTasklet implements Tasklet, LoanCOBBusinessStep {
 
+    private static final String ALL_ACTIVE_LOANS = "All";

Review Comment:
   Let's not do this :))



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