You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@fineract.apache.org by GitBox <gi...@apache.org> on 2021/01/21 13:16:31 UTC
[GitHub] [fineract] davidyaha opened a new pull request #1587: Add days in year at loan level #FINERACT-1302
davidyaha opened a new pull request #1587:
URL: https://github.com/apache/fineract/pull/1587
## Description
Added days in year at loan level.
## Checklist
Please make sure these boxes are checked before submitting your pull request - thanks!
- [x] Write the commit message as per https://github.com/apache/fineract/#pull-requests
- [x] 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.
- [x] Follow coding conventions at https://cwiki.apache.org/confluence/display/FINERACT/Coding+Conventions.
- [x] Add required Swagger annotation and update API documentation at fineract-provider/src/main/resources/static/api-docs/apiLive.htm with details of any API changes
- [x] 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vorburger commented on a change in pull request #1587: Add days in year at loan level #FINERACT-1302
Posted by GitBox <gi...@apache.org>.
vorburger commented on a change in pull request #1587:
URL: https://github.com/apache/fineract/pull/1587#discussion_r562184018
##########
File path: fineract-provider/src/main/java/org/apache/fineract/portfolio/loanaccount/loanschedule/service/LoanScheduleAssembler.java
##########
@@ -336,7 +336,14 @@ private LoanApplicationTerms assembleLoanApplicationTermsFrom(final JsonElement
*/
final DaysInMonthType daysInMonthType = loanProduct.fetchDaysInMonthType();
- final DaysInYearType daysInYearType = loanProduct.fetchDaysInYearType();
+ DaysInYearType daysInYearType = null;
Review comment:
Nit pick: remove the = null, it's not needed (because it gets assigned in either case), nicer to read, AND safer (if the code below EVER changes, it would cause a compilation failure instead of potential NPE). Makes sense?
##########
File path: fineract-provider/src/main/java/org/apache/fineract/portfolio/loanaccount/api/LoansApiResourceSwagger.java
##########
@@ -515,6 +515,8 @@ private PostLoansRequest() {}
public String expectedDisbursementDate;
@Schema(example = "2")
public Integer transactionProcessingStrategyId;
+ @Schema(example = "360", allowableValues = "1, 360, 364, 36")
Review comment:
As below, curious values? Perhaps for a good reason which I don't understand.
##########
File path: fineract-provider/src/main/java/org/apache/fineract/portfolio/loanaccount/serialization/LoanApplicationCommandFromApiJsonHelper.java
##########
@@ -485,7 +485,14 @@ public void validateForCreate(final String json, final boolean isMeetingMandator
baseDataValidator.reset().parameter(LoanApiConstants.datatables).value(datatables).notNull().jsonArrayNotEmpty();
}
- validateLoanMultiDisbursementdate(element, baseDataValidator, expectedDisbursementDate, principal);
+ if (this.fromApiJsonHelper.parameterExists(LoanApiConstants.daysInYearTypeParameterName, element)) {
+ final Integer daysInYearType = this.fromApiJsonHelper.extractIntegerNamed(LoanApiConstants.daysInYearTypeParameterName, element,
+ Locale.getDefault());
+ baseDataValidator.reset().parameter(LoanApiConstants.daysInYearTypeParameterName).value(daysInYearType).notNull()
+ .isOneOfTheseValues(1, 360, 364, 365);
Review comment:
O don't know the details of this code, so this a general review which someone else with more in-depth knowledge may want to comment on, but years normally have 365 or 366 (leap year) days.. what's 364 used for, and 360 - and 1?!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] edcable commented on pull request #1587: Add days in year at loan level #FINERACT-1302
Posted by GitBox <gi...@apache.org>.
edcable commented on pull request #1587:
URL: https://github.com/apache/fineract/pull/1587#issuecomment-790154765
@vorburger @Nayan and members of the conflux team are becoming active members of the community again so I'll try to have them do a review of this commit as well.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vorburger commented on a change in pull request #1587: Add days in year at loan level #FINERACT-1302
Posted by GitBox <gi...@apache.org>.
vorburger commented on a change in pull request #1587:
URL: https://github.com/apache/fineract/pull/1587#discussion_r571704060
##########
File path: fineract-provider/src/main/java/org/apache/fineract/portfolio/loanaccount/loanschedule/service/LoanScheduleAssembler.java
##########
@@ -336,7 +336,14 @@ private LoanApplicationTerms assembleLoanApplicationTermsFrom(final JsonElement
*/
final DaysInMonthType daysInMonthType = loanProduct.fetchDaysInMonthType();
- final DaysInYearType daysInYearType = loanProduct.fetchDaysInYearType();
+ DaysInYearType daysInYearType = null;
Review comment:
@davidyaha it looks like you haven't had time to address this feedback. As it's only minor, I'll not let this hold up merging.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vorburger merged pull request #1587: Add days in year at loan level #FINERACT-1302
Posted by GitBox <gi...@apache.org>.
vorburger merged pull request #1587:
URL: https://github.com/apache/fineract/pull/1587
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] ptuomola commented on a change in pull request #1587: Add days in year at loan level #FINERACT-1302
Posted by GitBox <gi...@apache.org>.
ptuomola commented on a change in pull request #1587:
URL: https://github.com/apache/fineract/pull/1587#discussion_r563219324
##########
File path: fineract-provider/src/main/java/org/apache/fineract/portfolio/loanaccount/serialization/LoanApplicationCommandFromApiJsonHelper.java
##########
@@ -485,7 +485,14 @@ public void validateForCreate(final String json, final boolean isMeetingMandator
baseDataValidator.reset().parameter(LoanApiConstants.datatables).value(datatables).notNull().jsonArrayNotEmpty();
}
- validateLoanMultiDisbursementdate(element, baseDataValidator, expectedDisbursementDate, principal);
+ if (this.fromApiJsonHelper.parameterExists(LoanApiConstants.daysInYearTypeParameterName, element)) {
+ final Integer daysInYearType = this.fromApiJsonHelper.extractIntegerNamed(LoanApiConstants.daysInYearTypeParameterName, element,
+ Locale.getDefault());
+ baseDataValidator.reset().parameter(LoanApiConstants.daysInYearTypeParameterName).value(daysInYearType).notNull()
+ .isOneOfTheseValues(1, 360, 364, 365);
Review comment:
Hi @vorburger - that's the joy of archaic day count conventions for interest rate calculations :-). There are hundreds of conventions but 365 and 360 are the most common. See https://www.investopedia.com/terms/d/daycount.asp (or http://strata.opengamma.io/apidocs/com/opengamma/strata/basics/date/DayCounts.html for some more exotic ones :-)).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] davidyaha commented on pull request #1587: Add days in year at loan level #FINERACT-1302
Posted by GitBox <gi...@apache.org>.
davidyaha commented on pull request #1587:
URL: https://github.com/apache/fineract/pull/1587#issuecomment-764636191
@vorburger @awasum would love your review on this one. thanks!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vorburger commented on pull request #1587: Add days in year at loan level #FINERACT-1302
Posted by GitBox <gi...@apache.org>.
vorburger commented on pull request #1587:
URL: https://github.com/apache/fineract/pull/1587#issuecomment-764919587
@avikganguly01 saw that you were active in #1586 (welcome "back"), would you be able to review this PR from a promising new contributor, to support the ecosystem? Thank You!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vorburger commented on pull request #1587: Add days in year at loan level #FINERACT-1302
Posted by GitBox <gi...@apache.org>.
vorburger commented on pull request #1587:
URL: https://github.com/apache/fineract/pull/1587#issuecomment-774773285
@davidyaha I don't know enough about the full details of this code, and unfortunately neither @avikganguly01 nor any other remaining active maintainer has reviewed this in the last 2 weeks. You look like you know what you are doing, and we want to encourage new contributions, so I'll go ahead and merge this now.
PS: Adding tests when you can would be great (in future contributions).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vorburger commented on a change in pull request #1587: Add days in year at loan level #FINERACT-1302
Posted by GitBox <gi...@apache.org>.
vorburger commented on a change in pull request #1587:
URL: https://github.com/apache/fineract/pull/1587#discussion_r571703852
##########
File path: fineract-provider/src/main/java/org/apache/fineract/portfolio/loanaccount/serialization/LoanApplicationCommandFromApiJsonHelper.java
##########
@@ -485,7 +485,14 @@ public void validateForCreate(final String json, final boolean isMeetingMandator
baseDataValidator.reset().parameter(LoanApiConstants.datatables).value(datatables).notNull().jsonArrayNotEmpty();
}
- validateLoanMultiDisbursementdate(element, baseDataValidator, expectedDisbursementDate, principal);
+ if (this.fromApiJsonHelper.parameterExists(LoanApiConstants.daysInYearTypeParameterName, element)) {
+ final Integer daysInYearType = this.fromApiJsonHelper.extractIntegerNamed(LoanApiConstants.daysInYearTypeParameterName, element,
+ Locale.getDefault());
+ baseDataValidator.reset().parameter(LoanApiConstants.daysInYearTypeParameterName).value(daysInYearType).notNull()
+ .isOneOfTheseValues(1, 360, 364, 365);
Review comment:
@ptuomola oh what fun. OK, then I'll Resolve this.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] davidyaha commented on pull request #1587: Add days in year at loan level #FINERACT-1302
Posted by GitBox <gi...@apache.org>.
davidyaha commented on pull request #1587:
URL: https://github.com/apache/fineract/pull/1587#issuecomment-764636191
@vorburger @awasum would love your review on this one. thanks!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vorburger commented on pull request #1587: Add days in year at loan level #FINERACT-1302
Posted by GitBox <gi...@apache.org>.
vorburger commented on pull request #1587:
URL: https://github.com/apache/fineract/pull/1587#issuecomment-764919587
@avikganguly01 saw that you were active in #1586 (welcome "back"), would you be able to review this PR from a promising new contributor, to support the ecosystem? Thank You!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vorburger commented on a change in pull request #1587: Add days in year at loan level #FINERACT-1302
Posted by GitBox <gi...@apache.org>.
vorburger commented on a change in pull request #1587:
URL: https://github.com/apache/fineract/pull/1587#discussion_r562184018
##########
File path: fineract-provider/src/main/java/org/apache/fineract/portfolio/loanaccount/loanschedule/service/LoanScheduleAssembler.java
##########
@@ -336,7 +336,14 @@ private LoanApplicationTerms assembleLoanApplicationTermsFrom(final JsonElement
*/
final DaysInMonthType daysInMonthType = loanProduct.fetchDaysInMonthType();
- final DaysInYearType daysInYearType = loanProduct.fetchDaysInYearType();
+ DaysInYearType daysInYearType = null;
Review comment:
Nit pick: remove the = null, it's not needed (because it gets assigned in either case), nicer to read, AND safer (if the code below EVER changes, it would cause a compilation failure instead of potential NPE). Makes sense?
##########
File path: fineract-provider/src/main/java/org/apache/fineract/portfolio/loanaccount/api/LoansApiResourceSwagger.java
##########
@@ -515,6 +515,8 @@ private PostLoansRequest() {}
public String expectedDisbursementDate;
@Schema(example = "2")
public Integer transactionProcessingStrategyId;
+ @Schema(example = "360", allowableValues = "1, 360, 364, 36")
Review comment:
As below, curious values? Perhaps for a good reason which I don't understand.
##########
File path: fineract-provider/src/main/java/org/apache/fineract/portfolio/loanaccount/serialization/LoanApplicationCommandFromApiJsonHelper.java
##########
@@ -485,7 +485,14 @@ public void validateForCreate(final String json, final boolean isMeetingMandator
baseDataValidator.reset().parameter(LoanApiConstants.datatables).value(datatables).notNull().jsonArrayNotEmpty();
}
- validateLoanMultiDisbursementdate(element, baseDataValidator, expectedDisbursementDate, principal);
+ if (this.fromApiJsonHelper.parameterExists(LoanApiConstants.daysInYearTypeParameterName, element)) {
+ final Integer daysInYearType = this.fromApiJsonHelper.extractIntegerNamed(LoanApiConstants.daysInYearTypeParameterName, element,
+ Locale.getDefault());
+ baseDataValidator.reset().parameter(LoanApiConstants.daysInYearTypeParameterName).value(daysInYearType).notNull()
+ .isOneOfTheseValues(1, 360, 364, 365);
Review comment:
O don't know the details of this code, so this a general review which someone else with more in-depth knowledge may want to comment on, but years normally have 365 or 366 (leap year) days.. what's 364 used for, and 360 - and 1?!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org