-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
FINERACT-1977: 4-eye-principle, maker-checker permission #3649
Conversation
9089ab9
to
bbd47f2
Compare
I have reviewed the code and it is Ok for the create client maker-checker action. Is there a way to include in the integration test case some coverage for Loan, Savings, Transaction (disbursement, withdrawal/deposit)? This is because I think that this is an important feature enhancement. |
bbd47f2
to
c1d5ed2
Compare
Thank you for your review. |
67d089e
to
0c985d2
Compare
fineract-core/src/main/java/org/apache/fineract/batch/service/BatchApiServiceImpl.java
Show resolved
Hide resolved
if (sameTransaction) { | ||
commandSource = commandSourceService.getInitialCommandSource(wrapper, command, user, idempotencyKey); | ||
} else { | ||
commandSource = commandSourceService.saveInitialNewTransaction(wrapper, command, user, idempotencyKey); |
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.
This seems not correct to me. If we are not storing the initial command, anyone can call it again without any error... Am i missing something?
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.
Any other call would fail, because the idempotency key is unique, but not with 'Too early' error, which would be the correct error code in this case.
...rg/apache/fineract/infrastructure/bulkimport/importhandler/savings/SavingsImportHandler.java
Show resolved
Hide resolved
integration-tests/src/test/java/org/apache/fineract/integrationtests/BatchApiTest.java
Show resolved
Hide resolved
0c985d2
to
341c2a4
Compare
341c2a4
to
aa60a8a
Compare
Description
Describe the changes made and why they were made.
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!
Write the commit message as per https://github.com/apache/fineract/#pull-requests
Acknowledge that we will not review PRs that are not passing the build ("green") - it is your responsibility to get a proposed PR to pass the build, not primarily the project's maintainers.
Create/update unit or integration tests for verifying the changes made.
Follow coding conventions at https://cwiki.apache.org/confluence/display/FINERACT/Coding+Conventions.
Add required Swagger annotation and update API documentation at fineract-provider/src/main/resources/static/legacy-docs/apiLive.htm with details of any API changes
Submission is not a "code dump". (Large changes can be made "in repository" via a branch. Ask on the developer mailing list for guidance, if required.)
FYI our guidelines for code reviews are at https://cwiki.apache.org/confluence/display/FINERACT/Code+Review+Guide.