Repository navigation
FINERACT-3047: Share the savings interest rules and reverse stored entries - #6817
Abhishek6122008 wants to merge 2 commits into
Conversation
…tries Savings interest postings were booked from two copies of the GL rules: the accounting processors for manual postings, and the Post Interest For Savings job, which writes its own journal entries. The copies drifted apart, which is how FINERACT-2909 happened. A reversal did not undo the drift either: the processor rebuilt the reversed transaction's entries from its current rules and swapped debit and credit, so a posting booked by the job, or under GL mappings changed since, did not net to zero. SavingsInterestPostingLegs now holds the interest posting rules, and both processors and the job book from it. Overdraft interest postings are not part of it: the job and the processors book them to different accounts, and that difference needs a fix of its own. A reversed savings transaction now gets the opposite of every entry stored for it, as a reversed loan transaction already does, instead of entries rebuilt from the rules. A transaction with nothing booked gets no reversal entries.
adamsaghy
left a comment
adamsaghy
left a comment
There was a problem hiding this comment.
Thanks, the shared legs and mirroring the stored entries on reversal both look like the right direction. I didn't find anything blocking, but the change affects more than interest postings, so could you confirm that the behaviour in the inline comments is correct and expected?
In short:
- Every reversed savings transaction now has its stored entries mirrored, not only interest postings. Is that the intended scope, and which tests cover the other transaction types?
- A reversed transaction with nothing booked now gets no reversal entries, where it used to get the swapped entries rebuilt from the rules.
- In the job, any new transaction other than an interest posting or overdraft interest posting now gets no GL accounts from
selectAccountId.
|
|
||
| // a reversal undoes what was booked for the transaction, so it mirrors the stored entries instead of | ||
| // rebuilding them from the rules below, which may not be the ones the transaction was booked with | ||
| if (isReversal) { |
There was a problem hiding this comment.
Could you confirm this scope is intended? The early return applies to every savings transaction type, not only interest postings: reversed deposits, withdrawals, transfers, charge payments, withholding tax and accruals all have their stored entries mirrored now, where before they were rebuilt from the rules. The new unit and integration tests only cover interest postings. Which existing integration tests cover undoing the other types with accounting enabled, so we know they still net to zero?
A smaller point: after this continue, every isReversal passed further down in this method is always false, so the reversal path in createCashBasedJournalEntriesAndReversalsForSavings (and ...ForSavingsTax) can no longer be reached from the savings processors. Could you pass false explicitly, or tidy that up, so it doesn't look like those branches still run?
There was a problem hiding this comment.
Yes, intended: a reversal undoes what was booked, whatever the type, as for loans. Besides SavingsChargeTaxIntegrationTest (undo of a taxed charge payment), I added ITs that undo a deposit, a withdrawal and a withdrawal into overdraft on cash and accrual products and check every GL account nets to zero. The isReversal args below now pass false.
|
|
||
| // a reversal undoes what was booked for the transaction, so it mirrors the stored entries instead of | ||
| // rebuilding them from the rules below, which may not be the ones the transaction was booked with | ||
| if (isReversal) { |
There was a problem hiding this comment.
Same question as in CashBasedAccountingProcessorForSavings: is it intended that every reversed transaction type takes this path, and which tests cover the types that aren't interest postings? The isReversal arguments below this are also always false now.
There was a problem hiding this comment.
Same as the cash processor: intended, covered by the same new ITs, args now pass false.
| * booked gets no reversal entries. | ||
| */ | ||
| @Override | ||
| public void createJournalEntriesForReversedSavingsTransaction(final String transactionId, final LocalDate transactionDate) { |
There was a problem hiding this comment.
Two things I'd like you to confirm are expected:
- A reversed transaction with no stored entries now books nothing. Before, it got the swapped entries rebuilt from the rules. One case is a transaction that's created and reversed in the same request:
deriveAccountingBridgeDatasends it only once, as a reversal, so its original entries were never booked. Booking nothing seems more correct to me than the old one-sided reversal, but it does change behaviour, so please confirm it's intended. - The mirrors copy the entity type, so the mirror of a job-booked credit entry also carries entity type 1 (LOAN). The description already says that's a separate fix, so this is only to make sure it's a known consequence.
There was a problem hiding this comment.
- Yes, intended. Nothing booked means nothing to undo; the old one-sided reversal left the GL off.
- Yes, known. The mirror copies the entity type so it cancels the original. I'll file the job's entity type separately.
| glAccountId(savingsAccountData, leg.accountTypeToCredit()), leg.amount(), currencyCode, userId, | ||
| auditDatetime); | ||
| } | ||
| } else { |
There was a problem hiding this comment.
Is this the expected behaviour for transaction types other than overdraft interest? selectAccountId now only sets accounts for overdraft interest, so any other new transaction that reaches this branch gets a null debit and a null credit account. acc_gl_journal_entry.account_id can't be null, so the batch insert would fail and roll back the whole batch.
It can't happen today. The only other type the job could create is withholding tax, and the job never creates it: it loads the tax group with TaxGroupData.lookup(taxGroupId, null), so there are no tax associations and createWithHoldTransaction returns early. Before this PR, such a transaction was booked Dr Interest Payable / Cr Savings Control by the else that was removed from selectAccountId.
Could you confirm that's intended? It might also be worth limiting this branch to overdraft interest, or failing with a clear error when an account is null, so that a later fix to withholding tax in the job doesn't start breaking the batch.
There was a problem hiding this comment.
Good catch. The branch now books only overdraft interest, and any other type fails with a clear IllegalStateException, with a unit test. #6614 books withholding tax in its own branch, so it isn't affected, but one of the two will need a rebase.
The job's journal entries now cover overdraft interest by name, and any other new transaction type, which it has no GL rule for, fails with a clear error instead of inserting entries without accounts. The savings processors pass false for isReversal explicitly, since every reversal now returns early and mirrors the stored entries, and their comments no longer mention reversals. New integration tests undo a deposit, a withdrawal and a withdrawal into overdraft on cash-based and accrual products, and check that every GL account nets to zero.
|
@adamsaghy Thanks for the review! Answered inline and pushed f141a0b. |
Description
FINERACT-3047, follow-up to #6761 (FINERACT-2909).
Shared interest posting rules.
SavingsInterestPostingLegsturns an interest posting into its debit and credit legs: the part that pays down an overdraft goes to Overdraft Portfolio Control, the rest to Savings Control, debited from Interest on Savings (cash-based) or Interest Payable (accrual). Both savings accounting processors and the Post Interest For Savings job now book from it, soselectAccountIdkeeps only the overdraft interest rule.Overdraft interest postings are left out on purpose. The job books them Dr Overdraft Portfolio Control (or Savings Control) / Cr Income from Interest or Interest Receivable, while the processors book Dr Savings Reference / Cr Income from Interest. Sharing them would change one of the two, so that needs its own ticket.
Reversals mirror the stored entries. A reversed savings transaction now gets the opposite of each entry stored for it (
createJournalEntriesForReversedSavingsTransaction), the waycreateJournalEntryForReversedLoanTransactionalready works for loans. Before, the processor rebuilt the entries from its current rules and swapped debit and credit, so a posting the job booked differently, or one booked under GL mappings changed since, didn't net to zero. This also covers the pre-FINERACT-2909 postings in its upgrade note. A transaction with nothing booked gets no reversal entries.The lookup goes by transaction id only, because the job stores its credit entries with entity type 1 (it passes the journal entry type there). That's a separate fix.
Tests:
SavingsInterestPostingLegsTest: account in credit, partly and fully overdrawn, no interest; cash-based and accrualCashBasedAccountingProcessorForSavingsTest,AccrualBasedAccountingProcessorForSavingsTest: interest postings book the shared legs; a reversal mirrors the stored entries and rebuilds nothingAccountingProcessorHelperImplTest: mirrors keep each entry's account, amount and entity type with debit and credit swapped; nothing booked, nothing reversedSavingsSchedularInterestPosterJournalEntriesTest: the job books interest postings from the shared legs, cash-based and accrualSavingsAccrualAccountingIntegrationTest: reversing a job-posted overdraft interest posting with a backdated deposit nets every GL account to zero. Rebuilding gave Dr Income from Interest / Cr Savings Reference against the job's Dr Overdraft Portfolio Control / Cr Income from Interest.Checklist
Please make sure these boxes are checked before submitting your pull request - thanks!
Your assigned reviewer(s) will follow our guidelines for code reviews.