FINERACT-2493: New command processing - job scheduling#5903
Conversation
nidhiii128
force-pushed
the
FINERACT-2493-update-job
branch
from
52322d5 to
9851afe
Compare
|
@nidhiii128 Fix that failing checks. |
nidhiii128
force-pushed
the
FINERACT-2493-update-job
branch
from
9851afe to
a1278c6
Compare
nidhiii128
force-pushed
the
FINERACT-2493-update-job
branch
from
a1278c6 to
28d0073
Compare
|
@vidakovic please review |
| import org.apache.fineract.client.models.ExecuteJobRequest; | ||
| import org.apache.fineract.client.models.GetJobsResponse; | ||
| import org.apache.fineract.client.models.PutJobsJobIDRequest; | ||
| import org.apache.fineract.client.models.UpdateJobRequest; |
There was a problem hiding this comment.
Let's keep the following pattern for the DTOs (aka request/response classes):
[Domain] + [Action] + [Suffix]
... in this case...
JobUpdateRequest
There are and will be a lot of "Update" commands in the code base... with the proposed naming pattern you will immediately see the relevant classes while typing instead listing half of the classes available in the entire system.
| @Consumes(MediaType.APPLICATION_JSON) | ||
| @Operation(summary = "Update a Job", description = "Updates the details of a job.") | ||
| @RequestBody(required = true, content = @Content(schema = @Schema(implementation = SchedulerJobApiResourceSwagger.PutJobsJobIDRequest.class))) | ||
| @ApiResponse(responseCode = "200", description = "OK") |
There was a problem hiding this comment.
This annotation is trivial. HTTP 200 is the normal case, don't have to indicate explicitly (I know, we've done that in a lot of places... but let's avoid code if we don't really need it). Please remove. Note: the API compatibility plugin will indicate a backwards incompatible change... ignore it, not an error.
| public String updateJobDetail(@PathParam(SchedulerJobApiConstants.JOB_ID) @Parameter(description = "jobId") final Long jobId, | ||
| @Parameter(hidden = true) final String jsonRequestBody) { | ||
| return updateJobDetail(IdTypeResolver.resolveDefault(), Objects.toString(jobId, null), jsonRequestBody); | ||
| public UpdateJobResponse updateJobDetail(@PathParam("jobId") Long jobId, UpdateJobRequest request) { |
There was a problem hiding this comment.
See above comment about naming request/response.
| @Data | ||
| @NoArgsConstructor | ||
| @AllArgsConstructor | ||
| public class UpdateJobResponse implements Serializable { |
| @Slf4j | ||
| @Component | ||
| @RequiredArgsConstructor | ||
| public class UpdateJobCommandHandler implements CommandHandler<UpdateJobRequest, UpdateJobResponse> { |
There was a problem hiding this comment.
Naming pattern:
[Domain] + [Action] + "CommandHandler"
... in this case...
JobUpdateCommandHandler
| @@ -39,7 +39,7 @@ public interface SchedularWritePlatformService { | |||
|
|
|||
| ScheduledJobDetail findByJobId(Long jobId); | |||
There was a problem hiding this comment.
Doesn't this belong in a "read" service?
| CommandProcessingResult updateJobDetail(Long jobId, JsonCommand command); | ||
| UpdateJobResponse updateJobDetail(UpdateJobRequest request); | ||
|
|
||
| SchedulerDetail retriveSchedulerDetail(); |
There was a problem hiding this comment.
Doesn't this belong in a "read" service? Also: typo. "retrive" vs "retrieve".
| private final JobDetailDataValidator dataValidator; | ||
|
|
||
| @Autowired | ||
| public SchedularWritePlatformServiceJpaRepositoryImpl(final ScheduledJobDetailRepository scheduledJobDetailsRepository, |
There was a problem hiding this comment.
Use of "@Autowired" is deprecated and not necessary anymore. Let's use Lombok "@requiredargsconstructor" then we can avoid the constructor boilerplate code.
| } | ||
|
|
||
| public void updateSchedulerJob(long jobId, PutJobsJobIDRequest request) { | ||
| public void updateSchedulerJob(long jobId, UpdateJobRequest request) { |
There was a problem hiding this comment.
Why not adding "jobId" to the request object? Having multiple parameters in a function signature makes it over time brittle. If you only have the request object as input parameter and tomorrow you add a new attribute to it then the function stays the same doesn't trigger a refactoring fest.
3 participants
Description
Migrates the "update job" REST endpoint (by jobId) from the legacy command-processing path to the new
fineract-commandframework.Scope (PR 1 of 2 for FINERACT-2493)
UpdateJobRequest/UpdateJobResponsePOJOs with Jakarta ValidationUpdateJobCommandandUpdateJobCommandHandlerusingCommandHandler<REQ, RES>SchedularWritePlatformService.updateJobDetailsignature changed to accept typed requestJobDetailDataValidatorno longer called, replaced by@AssertTrueannotationsNot in this PR
Checklist
Please make sure these boxes are checked before submitting your pull request - thanks!
Your assigned reviewer(s) will follow our guidelines for code reviews.