FINERACT-2080: move validations from loan entity to own validator classes #4172
Conversation
oleksii-novikov-onix
force-pushed
the
FINERACT-2080/extract-validations-from-loan-entity-into-validator-classes
branch
2 times, most recently
from
c1efa0e to
bd3f72b
Compare
oleksii-novikov-onix
force-pushed
the
FINERACT-2080/extract-validations-from-loan-entity-into-validator-classes
branch
2 times, most recently
from
5cbb88b to
a6088dc
Compare
oleksii-novikov-onix
force-pushed
the
FINERACT-2080/extract-validations-from-loan-entity-into-validator-classes
branch
from
a6088dc to
48cfa12
Compare
There was a problem hiding this comment.
Found 1-2 things, overall I like the approach on stripping the Loan class to reduce its size but the PR is really hard to read and validate. Hence the only thing I'm concerned about is the test coverage. Did all validations have corresponding automated tests?
| import org.springframework.stereotype.Component; | ||
|
|
||
| @Component | ||
| public final class LoanChargeValidator { |
There was a problem hiding this comment.
In general, I strongly advocate for immutability. When it comes to inheritance, it often causes more harm than good. As a result, I make a conscious effort to avoid it and restrict its use by default whenever possible.
| } | ||
|
|
||
| @Bean | ||
| @ConditionalOnMissingBean(LoanDisbursementDetailsAssembler.class) | ||
| public LoanDisbursementDetailsAssembler loanDisbursementDetailsAssembler(FromJsonHelper fromApiJsonHelper) { | ||
| return new LoanDisbursementDetailsAssembler(fromApiJsonHelper); | ||
| } | ||
|
|
||
| @Bean | ||
| @ConditionalOnMissingBean(LoanDisbursementService.class) |
There was a problem hiding this comment.
Any reason these conditionals are not applied on the class themselves? That way you don't need to write any Configuration impl.
There was a problem hiding this comment.
Sorry, I'm not sure I fully understand your question. Are you asking about the reason for using @ConditionalOnMissingBean at all, or is there a different way we could use it?
There was a problem hiding this comment.
I havent tried to apply directly these conditional on the component so far, so i am not sure whether there is any difference between the order of processing when it got applied or how the override of beans works in this case. The examples I have seen so far, was always using this annotations in @configuration classes...
I would say this might be something that should discuss outside of this PR. If using these annotations directly on the classes are causing no harm and not effect customization of the Fineract, we might can decide to refactor all of them and move away from the @configuration classes...
What do you think?
There was a problem hiding this comment.
It seems the recommended way is to use @ConditionalOnMissingBean with configuration classes. From the documentation of the ConditionalOnMissingBean class:
The condition can only match the bean definitions that have been processed by the application context so far and, as such, it is strongly recommended to use this condition on auto-configuration classes only. If a candidate bean may be created by another auto-configuration, make sure that the one using this condition runs after.
I think we can learn about it in depth, but at the moment with code
@Service
@ConditionalOnMissingBean(LoanDisbursementService.class)
@RequiredArgsConstructor
public class LoanDisbursementService {
I have the next error:
Parameter 20 of constructor in org.apache.fineract.portfolio.loanaccount.loanschedule.service.LoanScheduleAssembler
required a bean of type 'org.apache.fineract.portfolio.loanaccount.service.LoanDisbursementService'
that could not be found.
That's a nice question! Let me look into it and get back to you. |
I think we are covered, so if all tests are green we are good to go. |
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.