galovics commented on code in PR #2507:
URL: https://github.com/apache/fineract/pull/2507#discussion_r948849540
##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/loanaccount/service/LoanReadPlatformServiceImpl.java:
##########
@@ -1647,6 +1648,22 @@ public LoanTermVariationsData mapRow(final ResultSet rs,
@SuppressWarnings("unus
}
+ @Override
+ public Collection<LoanScheduleDelinquencyData>
retrieveScheduleDelinquencyData(LocalDate businessLocalDate) {
+ LoanScheduleDelinquencyMapper mapper = new
LoanScheduleDelinquencyMapper(DateUtils.getBusinessLocalDate());
+ final StringBuilder sqlBuilder = new StringBuilder(400);
+ sqlBuilder.append("select ").append(mapper.schema())
Review Comment:
Could you please explain why we are relying on the native query here instead
of a regular JPQL/Criteria API query?
##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/loanaccount/jobs/setloandelinquencytags/SetLoanDelinquencyTagsTasklet.java:
##########
@@ -0,0 +1,58 @@
+/**
+ * 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.jobs.setloandelinquencytags;
+
+import java.util.Collection;
+import lombok.RequiredArgsConstructor;
+import lombok.extern.slf4j.Slf4j;
+import org.apache.fineract.infrastructure.core.service.DateUtils;
+import org.apache.fineract.infrastructure.core.service.ThreadLocalContextUtil;
+import
org.apache.fineract.portfolio.delinquency.service.DelinquencyWritePlatformService;
+import
org.apache.fineract.portfolio.loanaccount.data.LoanScheduleDelinquencyData;
+import
org.apache.fineract.portfolio.loanaccount.service.LoanReadPlatformService;
+import org.apache.fineract.portfolio.loanproduct.domain.LoanProductRepository;
+import org.springframework.batch.core.StepContribution;
+import org.springframework.batch.core.scope.context.ChunkContext;
+import org.springframework.batch.core.step.tasklet.Tasklet;
+import org.springframework.batch.repeat.RepeatStatus;
+
+@Slf4j
+@RequiredArgsConstructor
+public class SetLoanDelinquencyTagsTasklet implements Tasklet {
+
+ private final LoanProductRepository loanProductRepository;
+ private final DelinquencyWritePlatformService
delinquencyWritePlatformService;
+ private final LoanReadPlatformService loanReadPlatformService;
+
+ @Override
+ public RepeatStatus execute(StepContribution contribution, ChunkContext
chunkContext) throws Exception {
+ Collection<LoanScheduleDelinquencyData> loanScheduleDelinquencyData =
this.loanReadPlatformService
+
.retrieveScheduleDelinquencyData(DateUtils.getBusinessLocalDate());
+ log.info("Were found {} items", loanScheduleDelinquencyData.size());
Review Comment:
This is way too verbose imho.
##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/loanaccount/jobs/setloandelinquencytags/SetLoanDelinquencyTagsConfig.java:
##########
@@ -0,0 +1,61 @@
+/**
+ * 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.jobs.setloandelinquencytags;
+
+import lombok.AllArgsConstructor;
+import org.apache.fineract.infrastructure.jobs.service.JobName;
+import
org.apache.fineract.portfolio.delinquency.service.DelinquencyWritePlatformService;
+import
org.apache.fineract.portfolio.loanaccount.service.LoanReadPlatformService;
+import org.apache.fineract.portfolio.loanproduct.domain.LoanProductRepository;
+import org.springframework.batch.core.Job;
+import org.springframework.batch.core.Step;
+import
org.springframework.batch.core.configuration.annotation.JobBuilderFactory;
+import
org.springframework.batch.core.configuration.annotation.StepBuilderFactory;
+import org.springframework.batch.core.launch.support.RunIdIncrementer;
+import org.springframework.context.annotation.Bean;
+import org.springframework.context.annotation.Configuration;
+
+@Configuration
+@AllArgsConstructor
+public class SetLoanDelinquencyTagsConfig {
+
+ private JobBuilderFactory jobs;
+ private StepBuilderFactory steps;
+
+ private LoanProductRepository loanProductRepository;
+ private DelinquencyWritePlatformService delinquencyWritePlatformService;
+ private LoanReadPlatformService loanReadPlatformService;
+
+ @Bean
+ protected Step setLoanDelinquencyTagsStep() {
Review Comment:
Should be public.
##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/delinquency/service/DelinquencyWritePlatformServiceImpl.java:
##########
@@ -242,37 +276,89 @@ public int compare(DelinquencyRange o1, DelinquencyRange
o2) {
}
}
- private boolean isOverlaped(DelinquencyRange o1, DelinquencyRange o2) {
- return Math.max(o1.getMinimumAgeDays(), o2.getMinimumAgeDays()) <=
Math.min(o1.getMaximumAgeDays(), o2.getMaximumAgeDays());
+ private boolean isOverlapped(DelinquencyRange o1, DelinquencyRange o2) {
+ if (o2.getMaximumAgeDays() != null) { // Max Age undefined - Last one
+ return Math.max(o1.getMinimumAgeDays(), o2.getMinimumAgeDays()) <=
Math.min(o1.getMaximumAgeDays(), o2.getMaximumAgeDays());
+ } else {
+ return Math.max(o1.getMinimumAgeDays(), o2.getMinimumAgeDays()) <=
Math.min(o1.getMaximumAgeDays(), o2.getMinimumAgeDays());
+ }
+ }
+
+ private Map<String, Object> lookUpDelinquencyRange(final Loan loan, final
DelinquencyBucket delinquencyBucket, long ageDays) {
+ Map<String, Object> changes = new HashMap<>();
+
+ if (ageDays <= 0) { // No Delinquency
+ log.info("Loan {} without delinquency range with {} days",
loan.getId(), ageDays);
+ changes = setLoanDelinquencyTag(loan, null);
+
+ } else {
+ for (DelinquencyRange delinquencyRange :
delinquencyBucket.getRanges()) {
+ if (delinquencyRange.getMaximumAgeDays() == null) {
+ if (delinquencyRange.getMinimumAgeDays() <= ageDays) {
+ log.info("Loan {} with delinquency range {} with {}
days", loan.getId(), delinquencyRange.getClassification(),
Review Comment:
Too verbose.
##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/loanaccount/jobs/setloandelinquencytags/SetLoanDelinquencyTagsTasklet.java:
##########
@@ -0,0 +1,58 @@
+/**
+ * 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.jobs.setloandelinquencytags;
+
+import java.util.Collection;
+import lombok.RequiredArgsConstructor;
+import lombok.extern.slf4j.Slf4j;
+import org.apache.fineract.infrastructure.core.service.DateUtils;
+import org.apache.fineract.infrastructure.core.service.ThreadLocalContextUtil;
+import
org.apache.fineract.portfolio.delinquency.service.DelinquencyWritePlatformService;
+import
org.apache.fineract.portfolio.loanaccount.data.LoanScheduleDelinquencyData;
+import
org.apache.fineract.portfolio.loanaccount.service.LoanReadPlatformService;
+import org.apache.fineract.portfolio.loanproduct.domain.LoanProductRepository;
+import org.springframework.batch.core.StepContribution;
+import org.springframework.batch.core.scope.context.ChunkContext;
+import org.springframework.batch.core.step.tasklet.Tasklet;
+import org.springframework.batch.repeat.RepeatStatus;
+
+@Slf4j
+@RequiredArgsConstructor
+public class SetLoanDelinquencyTagsTasklet implements Tasklet {
+
+ private final LoanProductRepository loanProductRepository;
+ private final DelinquencyWritePlatformService
delinquencyWritePlatformService;
+ private final LoanReadPlatformService loanReadPlatformService;
+
+ @Override
+ public RepeatStatus execute(StepContribution contribution, ChunkContext
chunkContext) throws Exception {
+ Collection<LoanScheduleDelinquencyData> loanScheduleDelinquencyData =
this.loanReadPlatformService
+
.retrieveScheduleDelinquencyData(DateUtils.getBusinessLocalDate());
+ log.info("Were found {} items", loanScheduleDelinquencyData.size());
+ for (LoanScheduleDelinquencyData loanDelinquencyData :
loanScheduleDelinquencyData) {
+
this.delinquencyWritePlatformService.applyDelinquencyTagToLoan(loanDelinquencyData.getLoanId(),
+ loanDelinquencyData.getAgeDays());
+ }
+
+ log.info("{}: Records affected by setLoanDelinquencyTags: {}",
ThreadLocalContextUtil.getTenant().getName(),
Review Comment:
Same as above.
##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/delinquency/service/DelinquencyWritePlatformServiceImpl.java:
##########
@@ -242,37 +276,89 @@ public int compare(DelinquencyRange o1, DelinquencyRange
o2) {
}
}
- private boolean isOverlaped(DelinquencyRange o1, DelinquencyRange o2) {
- return Math.max(o1.getMinimumAgeDays(), o2.getMinimumAgeDays()) <=
Math.min(o1.getMaximumAgeDays(), o2.getMaximumAgeDays());
+ private boolean isOverlapped(DelinquencyRange o1, DelinquencyRange o2) {
+ if (o2.getMaximumAgeDays() != null) { // Max Age undefined - Last one
+ return Math.max(o1.getMinimumAgeDays(), o2.getMinimumAgeDays()) <=
Math.min(o1.getMaximumAgeDays(), o2.getMaximumAgeDays());
+ } else {
+ return Math.max(o1.getMinimumAgeDays(), o2.getMinimumAgeDays()) <=
Math.min(o1.getMaximumAgeDays(), o2.getMinimumAgeDays());
+ }
+ }
+
+ private Map<String, Object> lookUpDelinquencyRange(final Loan loan, final
DelinquencyBucket delinquencyBucket, long ageDays) {
+ Map<String, Object> changes = new HashMap<>();
+
+ if (ageDays <= 0) { // No Delinquency
+ log.info("Loan {} without delinquency range with {} days",
loan.getId(), ageDays);
+ changes = setLoanDelinquencyTag(loan, null);
+
+ } else {
+ for (DelinquencyRange delinquencyRange :
delinquencyBucket.getRanges()) {
+ if (delinquencyRange.getMaximumAgeDays() == null) {
+ if (delinquencyRange.getMinimumAgeDays() <= ageDays) {
+ log.info("Loan {} with delinquency range {} with {}
days", loan.getId(), delinquencyRange.getClassification(),
+ ageDays);
+ changes = setLoanDelinquencyTag(loan,
delinquencyRange.getId());
+ break;
Review Comment:
Since you are breaking out of the loop and the way the logic is implemented,
I think the delinquency bucket ranges should be an ordered list, am I wrong?
In the current case, sometimes it might be an ordered list but sometimes it
might not be unless we do an explicit ordering based on the ages.
Thoughts?
##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/delinquency/service/DelinquencyWritePlatformServiceImpl.java:
##########
@@ -242,37 +276,89 @@ public int compare(DelinquencyRange o1, DelinquencyRange
o2) {
}
}
- private boolean isOverlaped(DelinquencyRange o1, DelinquencyRange o2) {
- return Math.max(o1.getMinimumAgeDays(), o2.getMinimumAgeDays()) <=
Math.min(o1.getMaximumAgeDays(), o2.getMaximumAgeDays());
+ private boolean isOverlapped(DelinquencyRange o1, DelinquencyRange o2) {
+ if (o2.getMaximumAgeDays() != null) { // Max Age undefined - Last one
+ return Math.max(o1.getMinimumAgeDays(), o2.getMinimumAgeDays()) <=
Math.min(o1.getMaximumAgeDays(), o2.getMaximumAgeDays());
+ } else {
+ return Math.max(o1.getMinimumAgeDays(), o2.getMinimumAgeDays()) <=
Math.min(o1.getMaximumAgeDays(), o2.getMinimumAgeDays());
+ }
+ }
+
+ private Map<String, Object> lookUpDelinquencyRange(final Loan loan, final
DelinquencyBucket delinquencyBucket, long ageDays) {
+ Map<String, Object> changes = new HashMap<>();
+
+ if (ageDays <= 0) { // No Delinquency
+ log.info("Loan {} without delinquency range with {} days",
loan.getId(), ageDays);
+ changes = setLoanDelinquencyTag(loan, null);
+
+ } else {
+ for (DelinquencyRange delinquencyRange :
delinquencyBucket.getRanges()) {
+ if (delinquencyRange.getMaximumAgeDays() == null) {
+ if (delinquencyRange.getMinimumAgeDays() <= ageDays) {
+ log.info("Loan {} with delinquency range {} with {}
days", loan.getId(), delinquencyRange.getClassification(),
+ ageDays);
+ changes = setLoanDelinquencyTag(loan,
delinquencyRange.getId());
+ break;
+ }
+ } else {
+ if (delinquencyRange.getMinimumAgeDays() <= ageDays &&
delinquencyRange.getMaximumAgeDays() >= ageDays) {
+ log.info("Loan {} with delinquency range {} with {}
days", loan.getId(), delinquencyRange.getClassification(),
Review Comment:
Too verbose.
##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/delinquency/service/DelinquencyWritePlatformServiceImpl.java:
##########
@@ -242,37 +276,89 @@ public int compare(DelinquencyRange o1, DelinquencyRange
o2) {
}
}
- private boolean isOverlaped(DelinquencyRange o1, DelinquencyRange o2) {
- return Math.max(o1.getMinimumAgeDays(), o2.getMinimumAgeDays()) <=
Math.min(o1.getMaximumAgeDays(), o2.getMaximumAgeDays());
+ private boolean isOverlapped(DelinquencyRange o1, DelinquencyRange o2) {
+ if (o2.getMaximumAgeDays() != null) { // Max Age undefined - Last one
+ return Math.max(o1.getMinimumAgeDays(), o2.getMinimumAgeDays()) <=
Math.min(o1.getMaximumAgeDays(), o2.getMaximumAgeDays());
+ } else {
+ return Math.max(o1.getMinimumAgeDays(), o2.getMinimumAgeDays()) <=
Math.min(o1.getMaximumAgeDays(), o2.getMinimumAgeDays());
+ }
+ }
+
+ private Map<String, Object> lookUpDelinquencyRange(final Loan loan, final
DelinquencyBucket delinquencyBucket, long ageDays) {
+ Map<String, Object> changes = new HashMap<>();
+
+ if (ageDays <= 0) { // No Delinquency
+ log.info("Loan {} without delinquency range with {} days",
loan.getId(), ageDays);
Review Comment:
Too verbose.
--
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]
