[Ashutosh-Panda2004]

The Post Interest For Savings job never withholds tax. SavingAccountMapperForInterestPosting built the account's tax group with TaxGroupData.lookup(taxGroupId, null), so the tax associations were always null and SavingsAccountInterestPostingServiceImpl.createWithHoldTransaction returned without creating a withhold tax transaction. Posting interest through the API works because that path loads the tax group with its mappings.

This change loads the tax group with its mappings, components (percentage, validity dates, credit account) and component history in a separate query, once per page (loadTaxGroupsForInterestPosting), and sets it on the accounts, so the job creates the withhold tax transaction the same way the manual path does. SavingsSchedularInterestPoster now also inserts the transaction's rows into m_savings_account_transaction_tax_details in its batch update, and books the journal entries the way the savings accounting processors do: Savings Control is debited with the total withheld tax, and each tax component's credit account is credited with that component's share, falling back to the product's savings reference account when a component has no credit account. SavingsAccountData gains the glAccountIdForSavingsReference needed for that fallback.

A unit test in SavingsSchedularInterestPosterTest covers the batch persistence and the per-component journal entries for a withhold tax transaction.

Note on local verification: I could not run the Gradle build locally (no network access to fetch the dependencies in my environment), so the change was verified by static analysis against the current develop sources. CI will have to be the build check.

[Ashutosh-Panda2004]

@Abhishek6122008 now that #6174 is merged, I will rebase this PR on top of it and update here once it is done.

[Abhishek6122008]

Yes @Ashutosh-Panda2004 go ahead, thank you so much for waiting.

[Ashutosh-Panda2004]

Done, the rebase is complete. This PR is now a single commit on top of current develop, which includes #6174. The workflows will need the usual approval to run after the force-push.

[Ashutosh-Panda2004]

@adamsaghy @IOhacker requesting a review when you get a chance, now that this is rebased on #6174.

[adamsaghy]

Does not compile. It calls the old 4-argument TaxGroupMappingsData constructor; develop requires a 5th componentEditable argument since FINERACT-2769. build-core fails, and the mapper change that is the actual fix has no test.

[Ashutosh-Panda2004]

@adamsaghy You are right, sorry about that. The mapper still called the old 4-argument TaxGroupMappingsData constructor. It now passes componentEditable the way develop's own TaxGroupMappingsMapper leaves it, as null, since the flag is only set later in the tax group template flow and nothing in the interest posting path reads it.

The mapper change now has tests in SavingsAccountReadPlatformServiceImplTest: one for the new tax group joins in the SQL, and one that runs the extractor over two result rows and checks the loaded tax group, its mapping, component and history. The existing product mapping test also needed apm6 added to its alias list.

The branch is rebased on the current develop in a single signed commit. CI will be the first full compile on my side, so please let me know if anything else comes up.

[adamsaghy]

@Ashutosh-Panda2004 Please fix the failing build

[galovics]

Hi @Ashutosh-Panda2004, thanks for the PR, the root cause analysis is spot on.

Couple of things though. I'm not convinced about loading the tax group by joining it into the interest posting query, see the inline comment. And this is a fix on a batch job that books journal entries, so I'd really like to see an integration test for it. There's SavingsInterestPostingJobIntegrationTest already - let's add a case there with a withhold-tax savings product + tax group, run the Post Interest For Savings job and verify the withhold tax transaction, its tax details and the journal entries. Especially since you mentioned you couldn't build it locally.

Also please rebase. build-core failed on LoanApplicationTermsTest which is not related to your change, but it means none of the new tests have actually run yet.

Thanks.

fineract-provider/src/main/java/org/apache/fineract/portfolio/savings/service/SavingsAccountReadPlatformServiceImpl.java:382

+            sqlBuilder.append("left join m_tax_group_mappings tgm on tgm.tax_group_id = tg.id ");
+            sqlBuilder.append("left join m_tax_component tgcomp on tgcomp.id = tgm.tax_component_id ");
+            sqlBuilder.append("left join m_tax_component_history tgch on tgch.tax_component_id = tgcomp.id ");

I'm a bit afraid of this one. This query already returns one row per transaction (times charges, times tax details), and now every one of those rows gets multiplied by mappings x component history for every account with a tax group. A tax group with 2 components and a couple of history rows each and we're reading 4-6x the rows for the whole transaction history of the account, on every job run.

There's probably a handful of tax groups in the whole system. Wouldn't it make more sense to collect the distinct tax group ids from the page and load them once in a separate query (with the component histories, TaxComponentMapper ignores those so TaxReadPlatformService.retrieveTaxGroupData isn't a drop-in as-is), then set them on the accounts? That'd also get rid of the ~40 lines of manual dedup mapping here and the fake ResultSet in the test. Thoughts?

fineract-savings/src/main/java/org/apache/fineract/portfolio/savings/service/SavingsSchedularInterestPoster.java:133

-                    if (savingsAccountData.getGlAccountIdForSavingsControl() != 0
+                    if (savingsAccountTransactionData.isWithHoldTaxAndNotReversed()) {
+                        addWithholdTaxJournalEntries(paramsForGLInsertion, savingsAccountData, savingsAccountTransactionData, currencyCode,
+                                userId);
+                    } else if (savingsAccountData.getGlAccountIdForSavingsControl() != 0

Question: what happens in the correction path of the posting service where the old withhold transaction gets reversed and a new one is created? The reversed one only goes through the update batch here, so its journal entries are never reversed, and now the new one gets booked on top of it. I see the interest posting transaction itself has the same problem already, so not blocking this PR because of it, but now it'll double-count the tax in the GL too. Can we at least cover it in the integration test so we know what we're dealing with?

[adamsaghy]

Thanks for the rework. Loading the tax groups in a separate query per page is the right shape, and the poster's withhold tax entries match createCashBasedJournalEntriesAndReversalsForSavingsTax. Two things still need fixing, see the inline comments:

1. The correction half of the new integration test doesn't exercise the job's correction path, and it will fail as written.
2. The new apm6 join also matches the payment channel to fund source mappings.

CI hasn't run on this commit yet (it is waiting for workflow approval), so the build and the new tests are still unverified.

[Ashutosh-Panda2004]

Hi @adamsaghy, thanks for the detailed review. I pushed a new commit with both fixes.

On the apm6 join, I added and apm6.payment_type is null and apm6.charge_id is null, matching findCoreProductToFinAccountMapping. I also checked the other product mapping joins in the same query: payment channel and charge specific mappings only exist for the fund source account (type 1) and for the income from fees and penalties accounts, which this query does not join, so apm to apm5 can only match the core mapping of their account types and needed no change.

On the integration test, the correction is now driven by the job itself. The test posts interest at the period end, so the first run's last posting is dated 31 July, and the deposit that raises the balance is dated that same day. That date is not before the last posting period, so the deposit API does not repost anything, and the test asserts that. The next job run then recalculates the July period, reverses its interest and withhold tax transactions and replaces them, and the test checks that the reversed withhold transaction keeps its three journal entries, none of them reversed, and that the GL carries the July tax twice while the account summary carries it once.

CI has not run on the new commit yet, it is still waiting for workflow approval.

[adamsaghy]

@Ashutosh-Panda2004 Please rebase

[Ashutosh-Panda2004]

@adamsaghy rebased on the latest develop. It is still two commits, and both fixes from your review are included.

[adamsaghy]

@Ashutosh-Panda2004 Can you please review @galovics concerns and address them?

[Ashutosh-Panda2004]

@Ashutosh-Panda2004 Can you please review @galovics concerns and address them?

Sure, on it

[Ashutosh-Panda2004]

@adamsaghy I went through @galovics points again on the current head. The tax group joins are removed from the interest posting query, the groups are loaded once in a separate query and set on the accounts, and the dedup block and the fake ResultSet in the test are gone. The integration test in SavingsInterestPostingJobIntegrationTest runs the job on a withhold tax product and checks the withhold tax transactions, the journal entries per tax component and the correction case, including the double count in the GL. The branch is rebased on the latest develop. The only thing pending is the CI run on the new head, it is still waiting for workflow approval.

All concerns are addressed

[adamsaghy]

@Ashutosh-Panda2004 Please rebase and review the failing tests

[Ashutosh-Panda2004]

@adamsaghy rebased on the latest develop, still three commits, and I reviewed the failing tests.

Two of the failures were mine. The poster unit test built a Money without a tenant context, which MoneyHelper rejects, so I set the tenant context up in the test setup and cleanup. Spotless also flagged formatting in the poster and its test, which I fixed.

The account transfer office hierarchy failure was not from this change. The test itself had a wrong transfer id on develop and got a 404 instead of the expected 403. It was fixed on develop today in FINERACT-2968, and this rebase picks that fix up.

The new withhold tax integration test passed in the last run. The checks on the new head need an approval click to run.

[adamsaghy]

Thanks for this. I checked the change against the description: for new interest postings it does what it says. The tax group is now loaded with its mappings, components and history, so createWithHoldTransaction creates the withhold tax transaction. Its tax details go into m_savings_account_transaction_tax_details. The journal entries match createCashBasedJournalEntriesAndReversalsForSavingsTax / createAccrualBasedJournalEntriesAndReversalsForSavingsTax: savings control is debited, and each component's credit account is credited, falling back to the core savings reference mapping.

One thing I'd like resolved before merge:

Reversed withhold transactions keep their journal entries. When the job recalculates a period, it reverses the old withhold tax transaction and books a new one. The reversal only goes through the update batch (is_reversed), so the old journal entries are never reversed. The accounting processors would reverse them, which means the tax credit accounts end up overstated. I know the poster already behaves this way for reversed interest postings, but until now the job never created withhold transactions, so the tax accounts weren't affected.

testPostInterestJobWithholdsTaxAndBooksJournalEntries currently asserts this double count as expected ("Known behaviour: ... counted twice in the GL"). That makes a wrong GL balance a passing condition, and a later fix would have to break the test. Could you either:

• reverse the journal entries of the transactions the job reverses (for withhold tax, at least), or
• if that's out of scope here, drop the double-count assertion, open a follow-up JIRA for the journal entry reversal, and mention the limitation in the PR description.

Minor: the description says the tax group is loaded "in the interest posting mapper". After the second commit it's loaded by a separate query, once per page (loadTaxGroupsForInterestPosting). Please update the description to match.

[Ashutosh-Panda2004]

@adamsaghy I took the first option. The poster now reverses the journal entries of the withhold tax transactions it reverses. It reads back the entries booked for the transaction and books the mirror of what is still outstanding per GL account, debit and credit sides swapped under the same transaction id, the way the accounting processors do it. A transaction whose entries were already reversed books nothing.

The integration test no longer asserts the double count. The correction case now checks that the reversed withhold transaction's credits are cancelled by the reversal entries, and that the GL carries only the corrected withheld tax, the same total as the account summary.

I also fixed the description: the tax group is loaded by a separate query, once per page, in loadTaxGroupsForInterestPosting.

The checks on the new head need an approval click to run.

[Ashutosh-Panda2004]

Pushed a spotless fix for the tax group loading code, it only rewraps the lines the check flagged in the read service and its test.