FINERACT-2090: Restructure Loan creation and modification #3909
Conversation
3fca516 to
3dd9e81
Compare
| if (command.has(LoanApiConstants.disbursementDataParameterName)) { | ||
| final JsonArray disbursementDataArray = command.getAsJsonArray(LoanApiConstants.disbursementDataParameterName); | ||
| if (disbursementDataArray != null && !disbursementDataArray.isEmpty()) { | ||
| int i = 0; |
There was a problem hiding this comment.
@adamsaghy I made an attempt to improve this code slightly further.
Please check my commit at d544a35 and tell me if you wish to have a PR of this version.
There was a problem hiding this comment.
Yes, LGTM. I am happy to add your changes as part of this PR.
3dd9e81 to
5fc956e
Compare
|
@taskain7 @vidakovic @kjozsa @ruchiD @alberto-art3ch Please as your time lets you give me a review. I know this is a monster PR, but it was unfortunately required to have a clearer restructure... |
There was a problem hiding this comment.
... minor: technically needs this
@Serial
private static final long serialVersionUID = 1L;
There was a problem hiding this comment.
Great! Mapstruct... I think you could get away without a default function implementation and just provide:
Set<LoanChargeCommand> map(Set<LoanCharge> loanCharge, MonetaryCurrency currency);
I think Mapstruct is smart enough to create automatically a for loop and use the single value implementation on each element; with just the parameter "loanCharge" I'd be 100% sure, with the additional currency parameter I'd say 90%.
Minor issue.
There was a problem hiding this comment.
I was getting the following exception:
error: The return type Set<LoanChargeCommand> is an abstract class or interface. Provide a non abstract / non interface result type or a factory method.
Set<LoanChargeCommand> map(Set<LoanCharge> loanCharge, MonetaryCurrency currency);
There was a problem hiding this comment.
If i changed to return HashSet, it generated a dummy method:
@Override
public HashSet<LoanChargeCommand> map(Set<LoanCharge> loanCharge, MonetaryCurrency currency) {
if ( loanCharge == null && currency == null ) {
return null;
}
HashSet<LoanChargeCommand> hashSet = new HashSet<LoanChargeCommand>();
return hashSet;
}
5fc956e to
d1f0cd1
Compare
alberto-art3ch
left a comment
There was a problem hiding this comment.
In genral LGTM, Thanks!
Added further refactor that shared by @kjozsa! Thank you for it! ;)
The submit / modify loan application feature implementation contains various validations and conditions to decide on what logic to be executed based on the provided configuration.
The validations and conditions got mixed a little bit and now it is hard to understand what is happening and why.
The job here was to refactor a little bit an d group the validations and logics to be executed together and extract the business logic into methods to separate them a little bit.
The rule of thumb was
- Data validations
- Domain rule validations (sometimes it is only after entity was assembled)
- Assemble the entity
- Some actions on the entity to be extracted into utility classes or service classes
- Move some of the logic from service into here if make sense
- Update product related details
- New assemblers
- collateral
- charges
- etc
- Generate schedule
- Notes
- GLIM
- Calendar
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.