FINERACT-2705: Skip journal entries with unresolved GL accounts in savings interest posting job - #6174
Conversation
adamsaghy
changed the title
1770e86 to
7fc1523
Compare
|
@adamsaghy can you have another look at my PR when you’re free, just to make sure everything looks good? |
|
@adamsaghy @Aman-Mittal gentle ping on this one — CI is green, could you review and let me know if the approach looks right? |
| @@ -150,6 +150,17 @@ private void batchUpdateJournalEntries(final List<SavingsAccountData> savingsAcc | |||
| savingsAccountData.getId(), auditDatetime, auditDatetime, false, BigDecimal.ZERO, BigDecimal.ZERO, | |||
| null, savingsAccountTransactionData.getTransactionDate(), null, userId, userId, | |||
| DateUtils.getBusinessLocalDate() }); | |||
| } else { | |||
There was a problem hiding this comment.
I dont really like this idea:
- if no accounting is set on the loan product -> we should not reach this point: Saving accounts without accounting should not be picked up
- If cash based accounting is set and no accrual transactions to be created-> we should not reach this point: Savings accounts with Cash based accounting (hence no Accruals) should not be picked up.
@bharathc27 What do you think?
There was a problem hiding this comment.
@adamsaghy I agree that non-accounting loan products and their associated loans can be skipped. However, I believe income posting would still apply to loan accounts under both cash-based and accrual-based accounting.
From a practical perspective, it makes sense for the job to continue processing even if one account fails. However, we should maintain a list of the accounts that failed, along with the relevant error messages, so that users can review them and take the necessary action.
I hope this fix is taking care of that as well
There was a problem hiding this comment.
@adamsaghy @bharathcgowda Thank you for the suggestions. Based on your feedback, I was thinking of keeping this guard as a safety net, so that if the account selection is tightened later, the branch would simply stop firing.
For the reporting gap Bharath mentioned, I was also thinking of collecting the skipped/failed accounts and logging a single WARN summary per run instead of the current silent debug.
Please let me know if you think this approach makes sense, and I'll go ahead with it.
There was a problem hiding this comment.
Lets review first whether none accounting reach this point: it should not i believe.
cash based accounting and income posting: @bharathcgowda can you help me understand better?
…vings interest posting job batchUpdateJournalEntries guarded journal-entry creation with the product-level SAVINGS_CONTROL and INTEREST_ON_SAVINGS mappings but inserted the transaction-level accounts resolved by selectAccountId, which uses accrual-only mappings (INTEREST_PAYABLE/INTEREST_RECEIVABLE). For cash-based products these resolve to 0, so the single-batch INSERT into acc_gl_journal_entry violates its foreign key and rolls back interest posting for every account in the tenant. Check the actual transaction-level accounts and skip unresolved ones with a warning instead of aborting the whole batch.
8434b2c to
4d9e068
Compare
4 participants
Description
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!
Your assigned reviewer(s) will follow our guidelines for code reviews.