-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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-1960: Accrual Transactions For Savings batch job - Interest #3973
base: develop
Are you sure you want to change the base?
FINERACT-1960: Accrual Transactions For Savings batch job - Interest #3973
Conversation
bb57643
to
0a11c8f
Compare
@adamsaghy @marta-jankovics will you be able to help us in reviewing this PR please? |
] | ||
}, | ||
{ | ||
"default": null, |
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.
Should it support accrued fee and penalty as well no?
@@ -57,6 +57,7 @@ public enum JobName { | |||
SEND_ASYNCHRONOUS_EVENTS("Send Asynchronous Events"), // | |||
PURGE_EXTERNAL_EVENTS("Purge External Events"), // | |||
PURGE_PROCESSED_COMMANDS("Purge Processed Commands"), // | |||
ADD_PERIODIC_ACCRUAL_ENTRIES_FOR_SAVINGS_WITH_INCOME_POSTED_AS_TRANSACTIONS("Add Accrual Transactions For Savings"); // |
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.
Are you sure the job name is correct? Usually there are two types of job:
- Create periodic accruals (ADD_PERIODIC_ACCRUAL_ENTRIES_FOR_SAVINGS)
- Create income postings (ADD_PERIODIC_ACCRUAL_ENTRIES_FOR_SAVINGS_WITH_INCOME_POSTED_AS_TRANSACTIONS vs POST_INTEREST_FOR_SAVINGS)
Which one is needed here? The job name and the description is not really matching as far as i can tell.
fineract-core/src/main/java/org/apache/fineract/organisation/staff/domain/StaffRepository.java
Outdated
Show resolved
Hide resolved
@@ -56,13 +56,15 @@ public class SavingsAccountSummaryData implements Serializable { | |||
private LocalDate interestPostedTillDate; | |||
private LocalDate prevInterestPostedTillDate; | |||
private transient BigDecimal runningBalanceOnInterestPostingTillDate = BigDecimal.ZERO; | |||
private LocalDate accruedTillDate; | |||
private BigDecimal totalInterestAccrued; |
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.
Dont we need accrued fee and penalty?
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.
The Accrual Fee mainly will be in other PR
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.
I am wondering whether it would be better to send support for fee and penalty as well in the same PR or disable the job till it is not complete. THoughts?
final Long loanChargeId = (Long) loanChargePaid.get("savingsChargeId"); | ||
final boolean isPenalty = (Boolean) loanChargePaid.get("isPenalty"); | ||
final BigDecimal chargeAmountPaid = (BigDecimal) loanChargePaid.get("amount"); | ||
for (final Map<String, Object> savingsChargesPaid : savingsChargesPaidData) { |
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.
nice catch! :)
AccrualAccountsForSavings.INTEREST_ON_SAVINGS.getValue(), AccrualAccountsForSavings.INTEREST_PAYABLE.getValue(), | ||
savingsProductId, paymentTypeId, savingsId, transactionId, transactionDate, amount, isReversal); | ||
if (feePayments.size() > 0 || penaltyPayments.size() > 0) { | ||
this.helper.createCashBasedJournalEntriesAndReversalsForSavings(office, currencyCode, |
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.
Periodic accrual for cash based accounting? @bharathcgowda
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.
Would it make sense to only gather the proper credit and debit accounts into a variable and after just call one time the this.helper.createCashBasedJournalEntriesAndReversalsForSavings
method?
@@ -205,35 +212,45 @@ else if (savingsTransactionDTO.getTransactionType().isFeeDeduction() && savingsT | |||
savingsProductId, paymentTypeId, savingsId, transactionId, transactionDate, overdraftAmount, isReversal, | |||
penaltyPayments); | |||
if (isPositive) { | |||
final ChargePaymentDTO chargePaymentDTO = penaltyPayments.get(0); |
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.
unused variable chargePaymentDTO
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.
Done! removed
@@ -205,35 +212,45 @@ else if (savingsTransactionDTO.getTransactionType().isFeeDeduction() && savingsT | |||
savingsProductId, paymentTypeId, savingsId, transactionId, transactionDate, overdraftAmount, isReversal, | |||
penaltyPayments); | |||
if (isPositive) { | |||
final ChargePaymentDTO chargePaymentDTO = penaltyPayments.get(0); | |||
AccrualAccountsForSavings accountTypeToBeDebited = AccrualAccountsForSavings.SAVINGS_CONTROL; |
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.
I am not sure why to put this into a variable. The credit pair was inline in the method... :/
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.
Done! variable moved to be set before. This variable will be changed when we include Accrued Charges
} | ||
} else { | ||
this.helper.createAccrualBasedJournalEntriesAndReversalsForSavingsCharges(office, currencyCode, | ||
AccrualAccountsForSavings.OVERDRAFT_PORTFOLIO_CONTROL, AccrualAccountsForSavings.INCOME_FROM_FEES, | ||
savingsProductId, paymentTypeId, savingsId, transactionId, transactionDate, overdraftAmount, isReversal, | ||
feePayments); | ||
if (isPositive) { | ||
final ChargePaymentDTO chargePaymentDTO = feePayments.get(0); |
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.
unused variable chargePaymentDTO
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.
accountTypeToBeDebited
is in variable but account to be credited does not... i dont think we need to do this at all..
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.
Done! variable moved to be set before. This variable will be changed when we include Accrued Charges
} | ||
} | ||
} | ||
|
||
else if (savingsTransactionDTO.getTransactionType().isFeeDeduction()) { | ||
// Is the Charge a penalty? | ||
if (penaltyPayments.size() > 0) { | ||
final ChargePaymentDTO chargePaymentDTO = penaltyPayments.get(0); |
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.
unused variable chargePaymentDTO
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.
accountTypeToBeDebited
is in variable but account to be credited does not... i dont think we need to do this at all..
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.
Done! variable moved to be set before. This variable will be changed when we include Accrued Charges
} else { | ||
final ChargePaymentDTO chargePaymentDTO = feePayments.get(0); |
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.
unused variable chargePaymentDTO
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.
accountTypeToBeDebited
is in variable but account to be credited does not... i dont think we need to do this at all..
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.
Done! variable moved to be set before. This variable will be changed when we include Accrued Charges
@@ -233,6 +235,33 @@ private PostSavingsAccountsRequest() {} | |||
public String submittedOnDate; | |||
@Schema(example = "123") | |||
public String externalId; | |||
@Schema(example = "5.0") |
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.
Seems unrelated change... should it be extracted into a separate PR?
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 dont use Double
. It is either whole number and Integer / Long or BigDecimal to support fractions.
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 dont use Double types...
...c/main/java/org/apache/fineract/portfolio/savings/api/SavingsProductsApiResourceSwagger.java
Show resolved
Hide resolved
@Column(name = "accrued_till_date") | ||
protected LocalDate accruedTillDate; | ||
|
||
@Column(name = "total_interest_accrued_derived", scale = 6, precision = 19, nullable = true) |
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.
What about accrued fee and penalty?
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.
The Accrual Fee mainly will be in other PR
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.
I think it should be handled together... but if you insist to handle differently please disable the job hence it will produce incorrect accrual transactions
xsi:schemaLocation="http://www.liquibase.org/xml/ns/dbchangelog http://www.liquibase.org/xml/ns/dbchangelog/dbchangelog-4.3.xsd"> | ||
<changeSet author="fineract" id="1"> | ||
<addColumn tableName="m_savings_account"> | ||
<column defaultValueComputed="NULL" name="total_interest_accrued_derived" type="DECIMAL(19, 6)"/> |
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.
Should it support accrued fee or penalty?
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.
The Accrual Fee mainly will be in other PR
import org.hamcrest.Matcher; | ||
import org.hamcrest.Matchers; | ||
|
||
public abstract class BaseIntegrationTest { |
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.
Why do we need this? Most of them are duplicate from BaseLoanIntegrationTest
. You might wanna have this class and change the BaseLoanIntegrationTest
to extend BaseIntegrationTest
and remove these common functions from BaseLoanIntegrationTest
.
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.
Done! BaseIntegrationTest classes updated to use inherit
.../src/test/java/org/apache/fineract/integrationtests/common/savings/SavingsAccountHelper.java
Show resolved
Hide resolved
assertNotNull(transactionResponse); | ||
assertNotNull(transactionResponse.getResourceId()); | ||
|
||
savingsAccountDetails = savingsAccountHelper.getSavingsAccount(savingsAccountId.get()); |
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.
What does this test testing? I dont see any accrual related assertions...
// 3. Create a Savings account | ||
// 4. Approve and Activate the Savings account | ||
// 5. Add a Deposit | ||
// 6. |
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.
Missing steps...
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.
Done! Comments updated
// UC1: Simple Savings account creation with Accrual Accounting enabled
// 1. Create a client account
// 2. Create a Savings product with Accrual accounting enabled
// 3. Create a Savings account
// 4. Approve and Activate the Savings account
// 5. Add a Deposit and validate the account balance
// ------ Using a second business date
// 6. Add a second Deposit transaction to have other balance
// 7. Run the new batch job to Add the Accrual transactions in the Savings account
// 8. Get the Savings details to:
// a) Validate the accrued till date
// b) Validate the total amount of the accrual transactions generated
// c) Validate the Journal Entry of the first Accrual transaction generated
final GetSavingsAccountsTransaction accrualTransaction = optTransaction.get(); | ||
final List<HashMap> journalEntries = journalEntryHelper.getJournalEntriesByTransactionId("S" + accrualTransaction.getId()); | ||
assertEquals(2, journalEntries.size()); | ||
assertEquals(accrualTransaction.getAmount().floatValue(), journalEntries.get(0).get("amount")); |
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.
It does not check anywhere whether the proper gl account was used.
.../java/org/apache/fineract/integrationtests/savings/accrual/SavingsAccrualAccountingTest.java
Show resolved
Hide resolved
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 check my review!
I have done my best, but it would be nice to read a couple sentences about what this PR is intended to implement. |
0a11c8f
to
4c28638
Compare
Done, almost all the comments were attended or commented |
4c28638
to
a99c79b
Compare
@@ -103,7 +103,6 @@ plugins { | |||
id 'org.asciidoctor.jvm.pdf' version '3.3.2' apply false | |||
id 'org.asciidoctor.jvm.epub' version '3.3.2' apply false | |||
id 'org.asciidoctor.jvm.revealjs' version '3.3.2' apply false | |||
id 'org.asciidoctor.jvm.gems' version '3.3.2' apply false |
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.
Why to remove this?
Money total = Money.zero(currency); | ||
for (final SavingsAccountTransactionData transaction : transactions) { | ||
if (transaction.isAccrual() && !transaction.isReversalTransaction()) { | ||
total = total.plus(transaction.getAmount()); |
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.
THis will be incorrect the moment accrual will be done for fee and penalties as well. I think we should immediately use the interest portion here, instead of the transaction amount.
@@ -59,23 +59,23 @@ public void createJournalEntriesForSavings(final SavingsDTO savingsDTO) { | |||
if (savingsTransactionDTO.getTransactionType().isWithdrawal() && savingsTransactionDTO.isOverdraftTransaction()) { | |||
boolean isPositive = amount.subtract(overdraftAmount).compareTo(BigDecimal.ZERO) > 0; | |||
if (savingsTransactionDTO.isAccountTransfer()) { | |||
this.helper.createCashBasedJournalEntriesAndReversalsForSavings(office, currencyCode, |
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.
I am not sure this is correct. You just changed from cash based journal entries to accrual based one without any condition. I am assuming accounting wise after this PR cash based and accrual based can be selected and both should work. But here you are overriding the behaviour. Am i missing something?
this.helper.createCashBasedJournalEntriesAndReversalsForSavings(office, currencyCode, | ||
AccrualAccountsForSavings.INTEREST_ON_SAVINGS.getValue(), AccrualAccountsForSavings.INTEREST_PAYABLE.getValue(), | ||
savingsProductId, paymentTypeId, savingsId, transactionId, transactionDate, amount, isReversal); | ||
if (feePayments.size() > 0 || penaltyPayments.size() > 0) { |
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.
Seems you are trying to support fees and penalties, so i reckon the PR should involve accrual handling for them as well.
@@ -198,6 +205,7 @@ else if (savingsTransactionDTO.getTransactionType().isWithholdTax()) { | |||
/** Handle Fees Deductions and reversals of Fees Deductions **/ | |||
else if (savingsTransactionDTO.getTransactionType().isFeeDeduction() && savingsTransactionDTO.isOverdraftTransaction()) { | |||
boolean isPositive = amount.subtract(overdraftAmount).compareTo(BigDecimal.ZERO) > 0; | |||
AccrualAccountsForSavings accountTypeToBeDebited = AccrualAccountsForSavings.SAVINGS_CONTROL; |
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.
Why to extract this into a field? the accountTypeToBeCredited still inline...
} | ||
} | ||
} | ||
|
||
else if (savingsTransactionDTO.getTransactionType().isFeeDeduction()) { | ||
AccrualAccountsForSavings accountTypeToBeCredited = AccrualAccountsForSavings.INCOME_FROM_PENALTIES; |
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.
Why to extract this into a field? the accountTypeToBeCredited still inline...
} | ||
} | ||
|
||
/** Handle Transfers proposal **/ | ||
else if (savingsTransactionDTO.getTransactionType().isInitiateTransfer()) { | ||
this.helper.createCashBasedJournalEntriesAndReversalsForSavings(office, currencyCode, |
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.
Why do you override the cash based journal entry handling?
AccrualAccountsForSavings.SAVINGS_CONTROL.getValue(), AccrualAccountsForSavings.TRANSFERS_SUSPENSE.getValue(), | ||
savingsProductId, paymentTypeId, savingsId, transactionId, transactionDate, amount, isReversal); | ||
} | ||
|
||
/** Handle Transfer Withdrawal or Acceptance **/ | ||
else if (savingsTransactionDTO.getTransactionType().isWithdrawTransfer() | ||
|| savingsTransactionDTO.getTransactionType().isApproveTransfer()) { | ||
this.helper.createCashBasedJournalEntriesAndReversalsForSavings(office, currencyCode, | ||
this.helper.createAccrualBasedJournalEntriesAndReversalsForSavings(office, currencyCode, |
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.
Why do you override the cash based journal entry handling?
AccrualAccountsForSavings.TRANSFERS_SUSPENSE.getValue(), AccrualAccountsForSavings.SAVINGS_CONTROL.getValue(), | ||
savingsProductId, paymentTypeId, savingsId, transactionId, transactionDate, amount, isReversal); | ||
} | ||
|
||
/** overdraft **/ | ||
else if (savingsTransactionDTO.getTransactionType().isOverdraftInterest()) { | ||
this.helper.createCashBasedJournalEntriesAndReversalsForSavings(office, currencyCode, | ||
this.helper.createAccrualBasedJournalEntriesAndReversalsForSavings(office, currencyCode, |
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.
Why do you override the cash based journal entry handling?
AccrualAccountsForSavings.SAVINGS_REFERENCE.getValue(), AccrualAccountsForSavings.INCOME_FROM_INTEREST.getValue(), | ||
savingsProductId, paymentTypeId, savingsId, transactionId, transactionDate, amount, isReversal); | ||
} else if (savingsTransactionDTO.getTransactionType().isWrittenoff()) { | ||
this.helper.createCashBasedJournalEntriesAndReversalsForSavings(office, currencyCode, | ||
this.helper.createAccrualBasedJournalEntriesAndReversalsForSavings(office, currencyCode, |
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.
Why do you override the cash based journal entry handling?
final boolean isReversed = false; | ||
final Boolean lienTransaction = false; | ||
final String refNo = ExternalId.generate().getValue(); | ||
return new SavingsAccountTransaction(savingsAccount, office, SavingsAccountTransactionType.ACCRUAL.getValue(), date, amount, |
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.
I think balance portions are important: fee, penalty and interest portion of the transaction.
} | ||
} | ||
if (!errors.isEmpty()) { | ||
throw new JobExecutionException(errors); |
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.
I think this error logic should be moved into the job... JobExecutionException seems pretty job specific error.
} | ||
} | ||
|
||
public boolean isChargeToBeRecognizedAsAccrual(final Collection<Long> chargeIds, final SavingsAccountCharge savingsAccountCharge) { |
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.
This seems to me a logic that does not really required to be a method... you are checking whether the 1st parameter collection contains the charge id of the 2nd parameter...
SavingsAccountCharge savingsAccountCharge, LocalDate transactionDate) { | ||
final MonetaryCurrency currency = savingsAccount.getCurrency(); | ||
final Money chargeAmount = savingsAccountCharge.getAmount(currency); | ||
SavingsAccountTransaction savingsAccountTransaction = SavingsAccountTransaction.accrual(savingsAccount, savingsAccount.office(), |
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 are creating accrual for a charge... so charges should be supported along with interest, no?
@alberto-art3ch Kindly see my review! |
This pull request seems to be stale. Are you still planning to work on it? We will automatically close it in 30 days. |
@alberto-art3ch is this being worked on? |
@alberto-art3ch can you provide an update on this please? |
Description
Describe the changes made and why they were made.
Financial institutions will want to recognize expenses/incomes accrued on deposit product accounts too similar to the Loan products.
In this case we are adding the
Accrual Transactions For Savings
batch jobFINERACT-1960
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.