adamsaghy commented on code in PR #6817:
URL: https://github.com/apache/fineract/pull/6817#discussion_r4467760044
##########
fineract-provider/src/main/java/org/apache/fineract/accounting/journalentry/service/CashBasedAccountingProcessorForSavings.java:
##########
@@ -57,6 +59,13 @@ public void createJournalEntriesForSavings(final SavingsDTO
savingsDTO) {
this.helper.checkForBranchClosures(latestGLClosure,
transactionDate);
+ // 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) {
Review 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?
##########
fineract-provider/src/main/java/org/apache/fineract/accounting/journalentry/service/AccountingProcessorHelperImpl.java:
##########
@@ -494,6 +494,27 @@ public void
createCashBasedJournalEntriesAndReversalsForSavings(final Office off
loanId, transactionId, transactionDate, amount);
}
+ /**
+ * Books the opposite of every entry stored for a reversed savings
transaction, so the reversal nets to zero on each
+ * GL account however the transaction was booked: by an accounting
processor, or by the Post Interest For Savings
+ * job, which writes its own entries, and under GL mappings that may have
changed since. A transaction with nothing
+ * booked gets no reversal entries.
+ */
+ @Override
+ public void createJournalEntriesForReversedSavingsTransaction(final String
transactionId, final LocalDate transactionDate) {
Review Comment:
Two things I'd like you to confirm are expected:
1. 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: `deriveAccountingBridgeData`
sends 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.
2. 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.
##########
fineract-savings/src/main/java/org/apache/fineract/portfolio/savings/service/SavingsSchedularInterestPoster.java:
##########
@@ -132,21 +135,20 @@ private void batchUpdateJournalEntries(final
List<SavingsAccountData> savingsAcc
if (savingsAccountData.getGlAccountIdForSavingsControl()
!= 0
&&
savingsAccountData.getGlAccountIdForInterestOnSavings() != 0) {
OffsetDateTime auditDatetime =
DateUtils.getAuditOffsetDateTime();
- BigDecimal amount =
savingsAccountTransactionData.getAmount();
- // interest posted to an overdrawn account pays down
the overdraft first, as in the accounting
- // processors; the debit side is the same for both
parts
- final BigDecimal overdraftAmount =
savingsAccountTransactionData.getTransactionType().isInterestPosting()
- ?
savingsAccountTransactionData.getOverdraftAmount()
- : null;
- if (MathUtil.isGreaterThanZero(overdraftAmount)) {
+ if
(savingsAccountTransactionData.getTransactionType().isInterestPosting()) {
+ // booked from the same rules as a manual posting
in the accounting processors
+ for (final Leg leg :
SavingsInterestPostingLegs.legsFor(
+
savingsAccountData.isCashBasedAccountingEnabledOnSavingsProduct(),
+ savingsAccountTransactionData.getAmount(),
savingsAccountTransactionData.getOverdraftAmount())) {
+ addJournalEntryPair(paramsForGLInsertion,
savingsAccountData, savingsAccountTransactionData,
+ glAccountId(savingsAccountData,
leg.accountTypeToDebit()),
+ glAccountId(savingsAccountData,
leg.accountTypeToCredit()), leg.amount(), currencyCode, userId,
+ auditDatetime);
+ }
+ } else {
Review 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.
##########
fineract-provider/src/main/java/org/apache/fineract/accounting/journalentry/service/AccrualBasedAccountingProcessorForSavings.java:
##########
@@ -58,6 +60,13 @@ public void createJournalEntriesForSavings(final SavingsDTO
savingsDTO) {
this.helper.checkForBranchClosures(latestGLClosure,
transactionDate);
+ // 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) {
Review 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.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
