-
Notifications
You must be signed in to change notification settings - Fork 2.1k
FINERACT-2250: New command processing - Maker Checker (org.apache.fineract.commands) #5037
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
FINERACT-2250: New command processing - Maker Checker (org.apache.fineract.commands) #5037
Conversation
|
||
@Component | ||
@RequiredArgsConstructor | ||
public class DeleteMakerCheckerCommandHandler implements CommandHandler<DeleteMakerCheckerRequest, DeleteMakerCheckerResponse> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrong command service
public class ApproveRejectMakerCheckerCommandHandler | ||
implements CommandHandler<ApproveRejectMakerCheckerRequest, ApproveRejectMakerCheckerResponse> { | ||
|
||
private final PortfolioCommandSourceWritePlatformService writePlatformService; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrong command service
+ "makercheckers?officeId=1\n" + "\n" + "makercheckers?officeId=1&includeJson=true") | ||
public List<AuditData> retrieveCommands(@Context final UriInfo uriInfo, @BeanParam MakerCheckerRequest makerCheckerRequest) { | ||
final SQLBuilder extraCriteria = getExtraCriteria(makerCheckerRequest); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if this should be secured!?? Usually there is a validation happening here...
@Path("/v1/makercheckers") | ||
@Component | ||
@Tag(name = "Maker Checker (or 4-eye) functionality") | ||
@Consumes({ MediaType.APPLICATION_JSON }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's leave per function annotations... scope too broad
@Getter | ||
@Setter | ||
@SuperBuilder | ||
public class DeleteMakerCheckerResponse extends MakerCheckerResponseDTO implements Serializable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Request and responses should be as simple as possible... extending a parent is not. Embrace duplication (for request and response attributes), don't tie things to a common ancestor; this makes things very rigid... and over time use cases (and attributes) will diverge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will re-write this PR according to the comments.
Description
Describe the changes made and why they were made.
New Command processing infrastructure added for MakerChecker
Ignore if these details are present on the associated Apache Fineract JIRA ticket.
Checklist
Please make sure these boxes are checked before submitting your pull request - thanks!
FYI our guidelines for code reviews are at https://cwiki.apache.org/confluence/display/FINERACT/Code+Review+Guide.