You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@fineract.apache.org by GitBox <gi...@apache.org> on 2022/09/29 11:33:41 UTC
[GitHub] [fineract] taskain7 opened a new pull request, #2625: [FINERACT-1678] business step configuration fix
taskain7 opened a new pull request, #2625:
URL: https://github.com/apache/fineract/pull/2625
## Description
Fix for the business step order configuration.
--
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
[GitHub] [fineract] adamsaghy commented on a diff in pull request #2625: [FINERACT-1678] business step configuration fix
Posted by GitBox <gi...@apache.org>.
adamsaghy commented on code in PR #2625:
URL: https://github.com/apache/fineract/pull/2625#discussion_r984285596
##########
fineract-provider/src/main/java/org/apache/fineract/cob/service/ConfigJobParameterServiceImpl.java:
##########
@@ -59,15 +60,22 @@ public JobBusinessStepConfigData getBusinessStepConfigByJobName(String jobName)
public CommandProcessingResult updateStepConfigByJobName(JsonCommand command, String jobName)
throws BusinessStepNotBelongsToJobException {
List<BusinessStep> businessSteps = dataParser.parseUpdate(command);
- List<BatchBusinessStep> batchBusinessSteps = batchBusinessStepRepository.findAllByJobName(jobName);
- businessSteps.forEach(newBusinessStepConfig -> {
- BatchBusinessStep batchBusinessStep = batchBusinessSteps.stream()
- .filter(oldBusinessStepConfig -> oldBusinessStepConfig.getStepName().equals(newBusinessStepConfig.getStepName()))
- .findFirst().orElseThrow(BusinessStepNotBelongsToJobException::new);
- batchBusinessStep.setStepName(newBusinessStepConfig.getStepName());
- batchBusinessStep.setStepOrder(newBusinessStepConfig.getOrder());
- batchBusinessStepRepository.save(batchBusinessStep);
- });
+ List<String> availableBusinessStepNames = getAvailableBusinessStepsByJobName(BusinessStepCategory.LOAN.name())
Review Comment:
Please merge together the "availableBusinessStepNames" and "notValidBusinessStepNames" filter
--
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
[GitHub] [fineract] adamsaghy commented on a diff in pull request #2625: [FINERACT-1678] business step configuration fix
Posted by GitBox <gi...@apache.org>.
adamsaghy commented on code in PR #2625:
URL: https://github.com/apache/fineract/pull/2625#discussion_r984321022
##########
fineract-provider/src/main/java/org/apache/fineract/cob/service/ConfigJobParameterServiceImpl.java:
##########
@@ -59,15 +60,22 @@ public JobBusinessStepConfigData getBusinessStepConfigByJobName(String jobName)
public CommandProcessingResult updateStepConfigByJobName(JsonCommand command, String jobName)
throws BusinessStepNotBelongsToJobException {
List<BusinessStep> businessSteps = dataParser.parseUpdate(command);
- List<BatchBusinessStep> batchBusinessSteps = batchBusinessStepRepository.findAllByJobName(jobName);
- businessSteps.forEach(newBusinessStepConfig -> {
- BatchBusinessStep batchBusinessStep = batchBusinessSteps.stream()
- .filter(oldBusinessStepConfig -> oldBusinessStepConfig.getStepName().equals(newBusinessStepConfig.getStepName()))
- .findFirst().orElseThrow(BusinessStepNotBelongsToJobException::new);
- batchBusinessStep.setStepName(newBusinessStepConfig.getStepName());
- batchBusinessStep.setStepOrder(newBusinessStepConfig.getOrder());
- batchBusinessStepRepository.save(batchBusinessStep);
- });
+ List<String> availableBusinessStepNames = getAvailableBusinessStepsByJobName(BusinessStepCategory.LOAN.name())
+ .getAvailableBusinessSteps().stream().map(BusinessStepDetail::getStepName).toList();
+ List<String> notValidBusinessStepNames = businessSteps.stream().map(BusinessStep::getStepName)
+ .filter(businessStepName -> !availableBusinessStepNames.contains(businessStepName)).toList();
+ if (notValidBusinessStepNames.isEmpty()) {
Review Comment:
Is it allowed to remove all of the business steps for a job?
--
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
[GitHub] [fineract] adamsaghy commented on a diff in pull request #2625: [FINERACT-1678] business step configuration fix
Posted by GitBox <gi...@apache.org>.
adamsaghy commented on code in PR #2625:
URL: https://github.com/apache/fineract/pull/2625#discussion_r984311259
##########
fineract-provider/src/main/java/org/apache/fineract/cob/domain/BatchBusinessStep.java:
##########
@@ -33,6 +33,7 @@
public class BatchBusinessStep extends AbstractPersistableCustom {
@Column(name = "job_name", nullable = false)
+ @Setter
Review Comment:
Minor: You can put the "@Setter" on the class.
--
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
[GitHub] [fineract] adamsaghy commented on a diff in pull request #2625: [FINERACT-1678] business step configuration fix
Posted by GitBox <gi...@apache.org>.
adamsaghy commented on code in PR #2625:
URL: https://github.com/apache/fineract/pull/2625#discussion_r984305215
##########
integration-tests/src/test/java/org/apache/fineract/integrationtests/BusinessConfigurationApiTest.java:
##########
@@ -96,10 +96,8 @@ public void shouldUpdateStepOrder() {
.filter(businessStep -> APPLY_CHARGE_TO_OVERDUE_LOANS.equals(businessStep.getStepName())) //
.findFirst().get().getOrder();
assertEquals(originalOrder + 1, newOrder);
-
- List<BusinessStep> originalBusinessSteps = getBusinessSteps(originalOrder);
BusinessStepConfigurationHelper.updateBusinessStepOrder(requestSpec, responseSpec, LOAN_JOB_NAME,
- BusinessStepConfigurationHelper.toJsonString(originalBusinessSteps));
+ BusinessStepConfigurationHelper.toJsonString(originalStepConfig.getBusinessSteps()));
}
Review Comment:
Please add the following test cases:
1.
- Remove all the steps from the "Loan_job_name"
- Check there is no business steps configured for the loan
- Add "Apply charge to overdue_loans" step
- Check there is 1 business step configured with order 1
- Add "Delinquency Tags business step" to be the first step
- Check there is 2 business steps and the order is correct
- Remove Delinquency Tags business step
- Check there is 1 business step configured with order 1
2.
- Add a business steps which does not supported by the application -> Correct error flow occurred
--
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
[GitHub] [fineract] adamsaghy commented on a diff in pull request #2625: [FINERACT-1678] business step configuration fix
Posted by GitBox <gi...@apache.org>.
adamsaghy commented on code in PR #2625:
URL: https://github.com/apache/fineract/pull/2625#discussion_r984315705
##########
fineract-provider/src/main/java/org/apache/fineract/cob/service/ConfigJobParameterServiceImpl.java:
##########
@@ -59,15 +60,22 @@ public JobBusinessStepConfigData getBusinessStepConfigByJobName(String jobName)
public CommandProcessingResult updateStepConfigByJobName(JsonCommand command, String jobName)
throws BusinessStepNotBelongsToJobException {
List<BusinessStep> businessSteps = dataParser.parseUpdate(command);
- List<BatchBusinessStep> batchBusinessSteps = batchBusinessStepRepository.findAllByJobName(jobName);
- businessSteps.forEach(newBusinessStepConfig -> {
- BatchBusinessStep batchBusinessStep = batchBusinessSteps.stream()
- .filter(oldBusinessStepConfig -> oldBusinessStepConfig.getStepName().equals(newBusinessStepConfig.getStepName()))
- .findFirst().orElseThrow(BusinessStepNotBelongsToJobException::new);
- batchBusinessStep.setStepName(newBusinessStepConfig.getStepName());
- batchBusinessStep.setStepOrder(newBusinessStepConfig.getOrder());
- batchBusinessStepRepository.save(batchBusinessStep);
- });
+ List<String> availableBusinessStepNames = getAvailableBusinessStepsByJobName(BusinessStepCategory.LOAN.name())
Review Comment:
Performance improvement: You make this class to implement InitializingBean. In the afterPropertiesSet method can call the "getAvailableBusinessStepsByJobName" method and save the response at class level. The (application wise) available business step implementation will not be changed during runtime.
--
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