-
Notifications
You must be signed in to change notification settings - Fork 2.3k
FINERACT-2198: Extend Manual Journal entry to support Asset Externalization #4785
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-2198: Extend Manual Journal entry to support Asset Externalization #4785
Conversation
...eract/accounting/journalentry/service/JournalEntryWritePlatformServiceJpaRepositoryImpl.java
Show resolved
Hide resolved
adamsaghy
left a comment
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.
Please kindly see my review!
Please make sure you are adding proper testing!
016de77 to
2bcfb14
Compare
Testing has been added |
2bcfb14 to
40d49c9
Compare
adamsaghy
left a comment
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.
https://github.com/apache/fineract/pull/4785/files#r2160481795
Please address this!
7a56656 to
ac0a403
Compare
Done! code updated |
ac0a403 to
a241201
Compare
| } | ||
| final Optional<ExternalAssetOwner> optExternalAssetOwner = externalAssetOwnerRepository.findByExternalId(externalId); | ||
| if (!optExternalAssetOwner.isPresent()) { | ||
| throw new ExternalAssetOwnerTransferNotFoundException(externalId); |
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.
You meant "ExternalAssetOwnerNotFoundException", right?
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.
ExternalAssetOwnerNotFoundException
That exception is new, code updated
adamsaghy
left a comment
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.
Please see my review!
92c7860 to
2b78d7d
Compare
|
@alberto-art3ch Please fix the failing test! |
2b78d7d to
0e52067
Compare
…es with asset externalization owner
adamsaghy
left a comment
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.
LGTM
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!
FYI our guidelines for code reviews are at https://cwiki.apache.org/confluence/display/FINERACT/Code+Review+Guide.