[GitHub] [fineract] galovics commented on a diff in pull request #2577: FINERACT-1678: Manage scheduler and cob jobs

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

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


##########
fineract-provider/src/main/java/org/apache/fineract/cob/api/ConfigureBusinessStepResource.java:
##########
@@ -69,6 +71,20 @@ public class ConfigureBusinessStepResource {
     private final ConfigJobParameterService configJobParameterService;
     private final PortfolioCommandSourceWritePlatformService commandWritePlatformService;
 
+    @GET
+    @Path("/names")
+    @Produces({ MediaType.APPLICATION_JSON })
+    @Operation(summary = "List Business Jobs", description = "Returns the configured Business Jobs")

Review Comment:
   I'm not sure what you mean by "Business Jobs". Probably you wanna write something like "List of jobs with configurable business steps".



##########
fineract-provider/src/main/java/org/apache/fineract/cob/api/ConfigureBusinessStepResource.java:
##########
@@ -69,6 +71,20 @@ public class ConfigureBusinessStepResource {
     private final ConfigJobParameterService configJobParameterService;
     private final PortfolioCommandSourceWritePlatformService commandWritePlatformService;
 
+    @GET
+    @Path("/names")
+    @Produces({ MediaType.APPLICATION_JSON })
+    @Operation(summary = "List Business Jobs", description = "Returns the configured Business Jobs")
+    @ApiResponses({
+            @ApiResponse(responseCode = "200", description = "OK", content = @Content(array = @ArraySchema(schema = @Schema(implementation = ConfigureBusinessStepResourceSwagger.GetBusinessJobConfigResponse.class)))) })
+    public String retrieveAllConfiguredBusinessJobs(@Context final UriInfo uriInfo) {

Review Comment:
   Method naming is wrong - see above.



##########
fineract-provider/src/main/java/org/apache/fineract/cob/api/ConfigureBusinessStepResourceSwagger.java:
##########
@@ -27,6 +27,15 @@ private ConfigureBusinessStepResourceSwagger() {
 
     }
 
+    @Schema(description = "GetBusinessJobConfigResponse")
+    public static final class GetBusinessJobConfigResponse {
+
+        private GetBusinessJobConfigResponse() {}
+
+        public List<String> businessJobs;

Review Comment:
   Naming.



##########
fineract-provider/src/main/java/org/apache/fineract/cob/api/ConfigureBusinessStepResource.java:
##########
@@ -69,6 +71,20 @@ public class ConfigureBusinessStepResource {
     private final ConfigJobParameterService configJobParameterService;
     private final PortfolioCommandSourceWritePlatformService commandWritePlatformService;
 
+    @GET
+    @Path("/names")
+    @Produces({ MediaType.APPLICATION_JSON })
+    @Operation(summary = "List Business Jobs", description = "Returns the configured Business Jobs")
+    @ApiResponses({
+            @ApiResponse(responseCode = "200", description = "OK", content = @Content(array = @ArraySchema(schema = @Schema(implementation = ConfigureBusinessStepResourceSwagger.GetBusinessJobConfigResponse.class)))) })
+    public String retrieveAllConfiguredBusinessJobs(@Context final UriInfo uriInfo) {
+
+        List<String> businessJobNames = configJobParameterService.getAllJobNames();
+        final Gson gson = new Gson();

Review Comment:
   We're not using GSON directly to serialize API data. Please look at existing other API implementations.



##########
fineract-provider/src/main/java/org/apache/fineract/cob/domain/BatchBusinessStepRepository.java:
##########
@@ -21,8 +21,13 @@
 import java.util.List;
 import org.springframework.data.jpa.repository.JpaRepository;
 import org.springframework.data.jpa.repository.JpaSpecificationExecutor;
+import org.springframework.data.jpa.repository.Query;
 
 public interface BatchBusinessStepRepository extends JpaRepository<BatchBusinessStep, Long>, JpaSpecificationExecutor<BatchBusinessStep> {
 
     List<BatchBusinessStep> findAllByJobName(String jobName);
+
+    @Query("SELECT DISTINCT bbs.jobName FROM BatchBusinessStep bbs")
+    List<String> findJobNames();

Review Comment:
   Don't like the naming here, probably like `findConfiguredJobNames`



##########
fineract-provider/src/main/java/org/apache/fineract/cob/api/ConfigureBusinessStepResource.java:
##########
@@ -69,6 +71,20 @@ public class ConfigureBusinessStepResource {
     private final ConfigJobParameterService configJobParameterService;
     private final PortfolioCommandSourceWritePlatformService commandWritePlatformService;
 
+    @GET
+    @Path("/names")

Review Comment:
   Definitely not the proper naming for the API (`/jobs/names`). Please figure out a business step related API naming here.



##########
fineract-provider/src/main/java/org/apache/fineract/cob/service/ConfigJobParameterService.java:
##########
@@ -30,4 +31,7 @@ public interface ConfigJobParameterService {
     CommandProcessingResult updateStepConfigByJobName(JsonCommand command, String jobName);
 
     JobBusinessStepDetail getAvailableBusinessStepsByJobName(String jobName);
+
+    List<String> getAllJobNames();

Review Comment:
   Naming I don't like, see other comments.



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