fix(tds): correct tax logic for customer

[ljain112]

The Issue

• On first save: New tax row is created → add_deduct_tax is set correctly
• Doc reloads from UI (after save)
• On submit: The existing tax row is fetched from existing_taxes (database)

Problem: The existing tax row not have correct add_deduct_tax because it is not present in Sales Taxes and Charges causing error to be raised.

[coderabbitai]

📝 Walkthrough

Walkthrough

The changes modify tax withholding entry handling to make the add/deduct behavior configurable per party type. A dynamic add_deduct_tax value is introduced (defaulting to "Deduct" but set to "Add" for Customer party types) in the update_tax_rows method and propagated to individual tax rows. The _set_item_wise_tax_for_tds method now uses this value to compute item-level tax amounts with a configurable sign instead of a hard-coded deduction. Test cases remove explicit add_deduct_tax field declarations and add a reload statement in one test path.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Suggested labels

needs-tests

Suggested reviewers

• ruthra-kumar
• rohitwaghchaure
• mihir-kandoi

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 14.29% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly identifies the main change as fixing TDS (Tax Deduction at Source) tax logic for customers, which aligns with the core issue described in the PR where add_deduct_tax was not being set correctly for customer party types.
Description check	✅ Passed	The description is directly related to the changeset, explaining the specific problem with add_deduct_tax not being correctly set when existing tax rows are fetched from the database during submission.

✨ Finishing touches

• 📝 Generate docstrings

---

📜 Recent review details

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between cb5926f and 86b0f67.

📒 Files selected for processing (2)

• erpnext/accounts/doctype/tax_withholding_category/test_tax_withholding_category.py
• erpnext/accounts/doctype/tax_withholding_entry/tax_withholding_entry.py

🧰 Additional context used

🧠 Learnings (4)

📓 Common learnings

Learnt from: vorasmit
Repo: frappe/erpnext PR: 51099
File: erpnext/patches/v16_0/migrate_tax_withholding_data.py:904-910
Timestamp: 2025-12-23T11:28:49.128Z
Learning: In the old Tax Withholding system (before the Tax Withholding Entry refactor), only one TCS account per Sales Invoice was supported. When reviewing migration code in erpnext/patches/v16_0/migrate_tax_withholding_data.py, the query selecting TCS accounts from GL Entries doesn't need explicit aggregation because historical data will only have one TCS account per invoice.

📚 Learning: 2025-12-23T11:28:49.128Z

Learnt from: vorasmit
Repo: frappe/erpnext PR: 51099
File: erpnext/patches/v16_0/migrate_tax_withholding_data.py:904-910
Timestamp: 2025-12-23T11:28:49.128Z
Learning: In the old Tax Withholding system (before the Tax Withholding Entry refactor), only one TCS account per Sales Invoice was supported. When reviewing migration code in erpnext/patches/v16_0/migrate_tax_withholding_data.py, the query selecting TCS accounts from GL Entries doesn't need explicit aggregation because historical data will only have one TCS account per invoice.

Applied to files:

• erpnext/accounts/doctype/tax_withholding_category/test_tax_withholding_category.py
• erpnext/accounts/doctype/tax_withholding_entry/tax_withholding_entry.py

📚 Learning: 2025-12-16T05:33:58.723Z

Learnt from: Abdeali099
Repo: frappe/erpnext PR: 51078
File: erpnext/accounts/doctype/financial_report_template/financial_report_engine.py:486-491
Timestamp: 2025-12-16T05:33:58.723Z
Learning: In ERPNext/Frappe codebase, query.run(as_dict=True) returns frappe._dict objects that support both dict-style access (obj["key"]) and attribute-style access (obj.key). Therefore, attribute access on query results is valid and will not raise AttributeError. When reviewing Python code, prefer attribute access (obj.key) for readability where the key is known to exist, but ensure existence checks or fallback handling if there is any doubt about missing keys.

Applied to files:

• erpnext/accounts/doctype/tax_withholding_category/test_tax_withholding_category.py
• erpnext/accounts/doctype/tax_withholding_entry/tax_withholding_entry.py

📚 Learning: 2025-12-23T15:00:20.490Z

Learnt from: ljain112
Repo: frappe/erpnext PR: 51099
File: erpnext/accounts/doctype/tax_withholding_entry/tax_withholding_entry.py:1370-1379
Timestamp: 2025-12-23T15:00:20.490Z
Learning: In erpnext/accounts/doctype/tax_withholding_entry/tax_withholding_entry.py, when dividing by exchange_rate (obtained from _get_exchange_rate() or from document fields), you can assume the exchange_rate is non-zero at runtime and do not add defensive zero-division guards for this specific context. If future changes could affect exchange_rate guarantees, reassess this assumption.

Applied to files:

• erpnext/accounts/doctype/tax_withholding_entry/tax_withholding_entry.py

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)

• GitHub Check: Python Unit Tests (4)
• GitHub Check: Python Unit Tests (3)
• GitHub Check: Python Unit Tests (1)
• GitHub Check: Python Unit Tests (2)
• GitHub Check: Patch Test
• GitHub Check: Summary

🔇 Additional comments (4)

erpnext/accounts/doctype/tax_withholding_entry/tax_withholding_entry.py (3)

718-721: LGTM! Correctly differentiates tax behavior by party type.

The logic properly sets add_deduct_tax to "Add" for Customer (TCS increases invoice total) and "Deduct" for Supplier (TDS reduces payment). This ensures the tax row is created with the correct behavior based on the transaction type.

---

738-738: Key fix: Persists add_deduct_tax on tax row.

Assigning add_deduct_tax to the tax row before item-wise tax calculation ensures the value is persisted to the database on save. This resolves the issue where the existing tax row fetched during submit was missing the correct add_deduct_tax value.

---

821-828: LGTM! Dynamic sign calculation for item-wise tax amounts.

The multiplier logic correctly applies:

• -1 for "Deduct" (TDS): Tax amounts are negative (deductions from supplier payment)
• +1 for "Add" (TCS): Tax amounts are positive (additions to customer invoice)

This replaces the previous hard-coded deduction behavior with a configurable approach based on party type.

erpnext/accounts/doctype/tax_withholding_category/test_tax_withholding_category.py (1)

544-546: The reload explanation needs verification - add_deduct_tax doesn't appear in this test context.

The si.reload() addition between save and submit is present only for the third invoice, while the first two invoices in the same test function save and submit without reload. Additionally, the mentioned add_deduct_tax field does not appear in the TCS test snippet (lines 544-546); it's referenced elsewhere in a different test context. The actual functional necessity of this reload call cannot be confirmed from the visible code.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.05%. Comparing base (e1f9adf) to head (86b0f67).
⚠️ Report is 26 commits behind head on develop.

Additional details and impacted files

@@           Coverage Diff            @@
##           develop   #51412   +/-   ##
========================================
  Coverage    79.04%   79.05%           
========================================
  Files         1179     1179           
  Lines       121265   121269    +4     
========================================
+ Hits         95853    95865   +12     
+ Misses       25412    25404    -8     

Files with missing lines	Coverage Δ
...hholding_category/test_tax_withholding_category.py	99.00% <100.00%> (+<0.01%)	⬆️
...ype/tax_withholding_entry/tax_withholding_entry.py	95.66% <100.00%> (+0.03%)	⬆️

... and 8 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[ruthra-kumar]

@ljain112

Can't reproduce. Any other scenario?

[ljain112]

@ljain112

Can't reproduce. Any other scenario?

Have you tested it on the Sales Invoice?

[ruthra-kumar]

@ljain112

There is no add_deduct_tax field in Sales Invoice tax

[ljain112]

That was the issue.
taxes_and_totals verify tax amount based on this field.
In the sales invoice, it is not present. Still, the test case was working because on validate add_deduct_tax was getting added to the doc object.
But from UI, once the document is saved and after that submitted error was raised for incorrect taxes because on doc reload add_deduct_tax was not present in the doc object.
Also, for the sales invoice amount should be added, not deducted.

erpnext/erpnext/controllers/taxes_and_totals.py

Lines 506 to 508 in dd94e51

	multiplier = -1 if tax.get("add_deduct_tax") == "Deduct" else 1
	expected_amount = tax.base_tax_amount_after_discount_amount * multiplier
	actual_breakup = tax._total_tax_breakup