FINERACT-1958-Autopayment-downpayment #3367
Conversation
| </changeSet> | ||
| <changeSet author="fineract" id="2"> | ||
| <addColumn tableName="m_loan"> | ||
| <column defaultValueBoolean="false" type="boolean" name="enable_auto_repayment_for_down_payment"> |
There was a problem hiding this comment.
we can go with default value is 'true'
ruchiD
force-pushed
the
FINERACT-1958-Autopayment-downpayment
branch
from
753a1b8
to
9a1212a
Compare
| } | ||
| } | ||
|
|
||
| private BigDecimal percentageOf(final BigDecimal value, final BigDecimal percentage) { |
There was a problem hiding this comment.
It might be better to move into MathUtil class
There was a problem hiding this comment.
We already seem to have percentageOf on Money. Can we reuse that maybe?
There was a problem hiding this comment.
I am just thinking on adding one more testcase which somehow validates that the expected rounding rules are used. E.g.: disbursedAmount = 99.19, percentage is 19. In this case what value needs to be saved in the database?
There was a problem hiding this comment.
Moved method to MathUtil as operation is performed on BigDecimal amount.
@reluxa I checked amount saved in db for this example disbursedAmount = 99.19, percentage is 19. Downpayment amount is saved as 18.85 as rounding of and numbers after decimal for amount are derived for currency settings. For this test number of digits to be considered after decimal for currency was set as 2.
| @@ -59,7 +59,7 @@ public enum LoanTransactionType { | |||
| CHARGE_REFUND(24, "loanTransactionType.chargeRefund"), // | |||
| CHARGEBACK(25, "loanTransactionType.chargeback"), // | |||
| CHARGE_ADJUSTMENT(26, "loanTransactionType.chargeAdjustment"), // | |||
| CHARGE_OFF(27, "loanTransactionType.chargeOff"); | |||
| CHARGE_OFF(27, "loanTransactionType.chargeOff"), DOWN_PAYMENT(28, "loanTransactionType.downPayment"); | |||
There was a problem hiding this comment.
Please follow the code styling of the LoanTransactionType enum: New entries goes to new line
| @@ -141,4 +141,5 @@ | |||
| <include file="parts/0119_add_loan_product_down_payment_configuration.xml" relativeToChangelogFile="true" /> | |||
| <include file="parts/0120_transaction_summary_with_asset_owner_report_typo_fix.xml" relativeToChangelogFile="true" /> | |||
| <include file="parts/0121_transaction_summary_with_asset_owner_report_typo_fix_2.xml" relativeToChangelogFile="true" /> | |||
| <include file="parts/0122_add_loan_product_auto_repayment_down_payment_configuration.xml" relativeToChangelogFile="true" /> | |||
There was a problem hiding this comment.
Please move this into the fineract-loan module. You can find a couple example for liquibase changes.
| @@ -0,0 +1,39 @@ | |||
| <?xml version="1.0" encoding="UTF-8"?> | |||
| <!-- | |||
| </changeSet> | ||
| <changeSet author="fineract" id="2"> | ||
| <addColumn tableName="m_loan"> | ||
| <column defaultValueBoolean="true" type="boolean" name="enable_auto_repayment_for_down_payment"> |
There was a problem hiding this comment.
why do we have different default value for the m_loan and for the m_product_loan? cc: @adamsaghy
There was a problem hiding this comment.
@ruchiD Can you please take a look on this?
There was a problem hiding this comment.
Setting it to be false by default for both.
| Money downPaymentMoney = Money.of(getCurrency(), percentageOf(disbursedAmount, disbursedAmountPercentageForDownPayment)); | ||
| LoanTransaction downPaymentTransaction = LoanTransaction.downPayment(getOffice(), downPaymentMoney, null, disbursedOn, | ||
| externalId); | ||
| /** |
There was a problem hiding this comment.
It's a minor / cosmetic issue. The compiler settings that we use creates a warning when javadoc style comment is used inside of a method body. We can just change this to block coment.
|
|
||
| // Loan Product creation with down-payment configuration | ||
| final GetLoanProductsProductIdResponse getLoanProductsProductResponse = createLoanProductWithDownPaymentConfigurationAndAccrualAccounting( | ||
| loanTransactionHelper, delinquencyBucketId, enableDownPayment, "25", true, assetAccount, incomeAccount, expenseAccount, |
There was a problem hiding this comment.
in line 336 enableAutoRepaymentForDownPayment is initialized, but that is not used here
There was a problem hiding this comment.
And the same is true for the other testcases where the enableAutoRepaymentForDownPayment was added.
ruchiD
force-pushed
the
FINERACT-1958-Autopayment-downpayment
branch
from
9a1212a
to
2fd3469
Compare
Description
Changes made:
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.