feat!: Introduce tax withholding entry#51099
Conversation
…itholding category
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
erpnext/accounts/doctype/tax_withholding_category/tax_withholding_category.js (1)
24-38: Consider: Data loss on threshold disable.When a user disables a threshold (sets the disable flag to
1), all values in the corresponding threshold column are immediately cleared without confirmation. While this maintains data consistency (a disabled threshold shouldn't have values), users might accidentally lose data if they toggle the flag by mistake.Consider adding a confirmation dialog before clearing:
Optional: Add confirmation before data loss
disable_cumulative_threshold: function (frm) { toggle_threshold_settings(frm, "disable_cumulative_threshold"); if (frm.doc.disable_cumulative_threshold) { - reset_rates_column(frm, "cumulative_threshold"); + const has_values = (frm.doc.rates || []).some(row => row.cumulative_threshold); + if (has_values) { + frappe.confirm( + __("This will clear all cumulative threshold values. Are you sure?"), + () => reset_rates_column(frm, "cumulative_threshold") + ); + } else { + reset_rates_column(frm, "cumulative_threshold"); + } } update_rates_read_only_state(frm); },Apply similar logic for
disable_transaction_threshold.erpnext/patches/v16_0/migrate_tax_withholding_data.py (1)
28-49: Consider wrapping migration in a transaction or adding rollback capability.The migration deletes existing entries (line 38) and then proceeds with multiple independent migration steps. If any step fails midway, the database could be left in an inconsistent state with some entries migrated and others missing. Consider:
- Wrapping the entire migration in a database transaction if supported
- Adding rollback logic or clear error messaging to guide manual recovery
- Logging progress checkpoints so partial failures can be diagnosed
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
erpnext/accounts/doctype/tax_withholding_category/tax_withholding_category.jserpnext/patches/v16_0/migrate_tax_withholding_data.py
🧰 Additional context used
🧠 Learnings (6)
📓 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:40.898Z
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:40.898Z
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:40.898Z
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/patches/v16_0/migrate_tax_withholding_data.py
📚 Learning: 2025-08-20T11:58:32.385Z
Learnt from: ljain112
Repo: frappe/erpnext PR: 48864
File: erpnext/controllers/accounts_controller.py:2586-2596
Timestamp: 2025-08-20T11:58:32.385Z
Learning: In erpnext/controllers/accounts_controller.py, the posting_date parameter passed to get_due_date() and get_discount_date() is mandatory and calculated from bill_date/posting_date/transaction_date, so it should not be None.
Applied to files:
erpnext/patches/v16_0/migrate_tax_withholding_data.py
📚 Learning: 2025-10-09T06:59:10.528Z
Learnt from: ljain112
Repo: frappe/erpnext PR: 49963
File: erpnext/accounts/doctype/journal_entry/journal_entry.py:0-0
Timestamp: 2025-10-09T06:59:10.528Z
Learning: In erpnext/accounts/doctype/journal_entry/journal_entry.py, the validate() method calls validate_multi_currency() before apply_tax_withholding(). The validate_multi_currency() method internally calls set_exchange_rate(), so exchange rates are properly set on all account rows before TDS calculations occur in apply_tax_withholding().
Applied to files:
erpnext/patches/v16_0/migrate_tax_withholding_data.py
📚 Learning: 2025-09-05T20:59:45.680Z
Learnt from: diptanilsaha
Repo: frappe/erpnext PR: 49485
File: erpnext/setup/demo.py:78-80
Timestamp: 2025-09-05T20:59:45.680Z
Learning: In erpnext/setup/setup_wizard/operations/install_fixtures.py, the create_bank_account function can return None even when demo=True in two scenarios: 1) when bank_account_group is not found in the database, and 2) when a DuplicateEntryError occurs during account creation (the exception is caught and passed silently).
Applied to files:
erpnext/patches/v16_0/migrate_tax_withholding_data.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/patches/v16_0/migrate_tax_withholding_data.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: Patch Test
- GitHub Check: Python Unit Tests (4)
- GitHub Check: Python Unit Tests (2)
- GitHub Check: Python Unit Tests (3)
- GitHub Check: Python Unit Tests (1)
- GitHub Check: Summary
🔇 Additional comments (21)
erpnext/accounts/doctype/tax_withholding_category/tax_withholding_category.js (3)
20-22: LGTM! Proper UI state initialization.Calling
update_rates_read_only_statein the refresh handler ensures threshold field read-only states are correctly initialized when the form loads.
51-69: LGTM! Well-structured helper functions.Both helper functions are correctly implemented:
update_rates_read_only_state: Properly uses the grid API to control field editability based on disable flags.reset_rates_column: Safely handles undefined data with the|| []pattern and appropriately sets numeric thresholds to0.
41-49: The current implementation is correct and the suggestion would break functionality.The field descriptions in the doctype clarify the intended behavior:
disable_cumulative_thresholdmeans "only transaction threshold will be applied," anddisable_transaction_thresholdmeans "only cumulative threshold will be applied." This supports three valid states:
- Both unchecked (0,0): Both thresholds apply together
- disable_cumulative_threshold checked (1,0): Only transaction threshold applies
- disable_transaction_threshold checked (0,1): Only cumulative threshold applies
- Both checked (1,1): Invalid—would disable both thresholds
The current toggle logic correctly prevents state (1,1) while allowing (0,0), which is the intended combined-threshold mode. The requirement is "at least one threshold must be active," not "exactly one threshold active." The suggested change would force exactly one threshold, breaking the valid combined-threshold scenario.
Likely an incorrect or invalid review comment.
erpnext/patches/v16_0/migrate_tax_withholding_data.py (18)
51-78: LGTM!The column cache logic correctly detects schema variations across doctypes to handle different installations gracefully.
80-107: LGTM!The account mapping logic correctly handles ambiguous accounts (same account used by multiple categories) by setting them to
None, and provides both per-account and per-company lookups for efficient querying.
134-142: LGTM!The date validation has been properly addressed with checks for
Nonevalues before comparison, preventingTypeErrorexceptions during migration.
145-163: LGTM!The party tax ID resolution correctly uses column cache to detect available fields and implements per-party caching for performance.
165-187: Clarify the empty status case.When both
taxable_nameandwithholding_nameare absent/falsy (and nounder_withheld_reasonoris_duplicate), the function returns an empty string (line 186). This case seems unusual for a migration scenario. Consider:
- Is an empty status valid in the Tax Withholding Entry schema?
- Should this case log a warning or raise an error to flag data quality issues?
- Could this occur from malformed historical data?
If this is a valid fallback for edge cases in historical data, consider adding a comment explaining when this occurs.
189-288: LGTM!The bulk insert logic now properly prevents name collisions by:
- Pre-fetching existing names into a set
- Generating unique names with a loop that checks the set before returning
- Tracking generated names to avoid same-batch duplicates
The use of
ignore_duplicates=Trueat line 287 is now safe as a fallback since names are pre-validated.
295-334: LGTM!The migrator initialization is well-structured with clear separation between raw query results, lookup dictionaries, tracking sets, and output structures.
350-451: LGTM!The data fetching queries are well-structured, efficiently using joins to gather related information in single queries and properly filtering by submitted documents and TDS account presence.
456-545: LGTM!The lookup building methods efficiently organize the raw query data into structured lookups. The PE tax handling correctly accounts for "Deduct" vs. "Add" operations (lines 506-509), and cross-invoice TDS tracking (lines 478-480) properly links withheld vouchers.
550-629: LGTM!The invoice processing orchestration and context building correctly gather all necessary information for migration, with proper fallback for category resolution (lines 586-591) and accurate calculation of past taxable amounts (lines 611-613).
630-669: LGTM!The advance tax processing correctly back-calculates taxable amounts from allocated TDS (lines 642-645), creates entries in both the PI and PE (with duplicate marking), and properly accumulates taxable amounts in the context.
670-728: LGTM!The invoice TDS processing correctly:
- Splits TDS between current and past invoices for non-tax_on_excess categories (lines 679-683)
- Creates threshold exemption entries for tax_on_excess scenarios (lines 693-709)
- Back-calculates taxable amounts appropriately (lines 687-690)
730-787: LGTM!The withheld voucher processing correctly creates linked entries in both current and past invoices with duplicate marking (lines 757-760), and the under-withheld handling properly distinguishes threshold exemption cases (line 773).
792-838: LGTM!The unallocated Payment Entry TDS handling correctly:
- Calculates unallocated amounts (lines 801-805)
- Back-calculates taxable amounts (lines 815-818)
- Creates over-withheld entries with only withholding side populated
843-868: LGTM!The entry creation helper correctly handles return invoices by negating amounts (lines 846-850) and cleanly merges context with additional parameters (lines 852-860).
875-1016: LGTM!The Sales Invoice migration correctly:
- Aggregates TCS from GL entries with proper Case statement (lines 913-914)
- Handles return invoices by ensuring amounts are properly negated (lines 952-954, 963-964)
- The threshold exemption logic at lines 960-964 is correct:
net_grand_totalis always positive (abs applied at line 928), and the comparison withabs(calculated_taxable)works for both regular and return invoicesThe concern raised in past review about line 960 appears to have been addressed - the use of
abs()on line 928 ensures consistent positive values for comparison.
1023-1183: LGTM!The Journal Entry migration correctly:
- Resolves party information from non-TDS account rows (lines 1070-1083)
- Excludes TDS payment entries by checking if total_tds <= 0 (lines 1102-1104)
- Caps calculated taxable at total_debit to avoid over-calculation (line 1128)
- Applies threshold exemption logic consistently (lines 1133-1152)
1190-1243: LGTM!The category propagation functions correctly:
- Check for column existence before attempting updates (lines 1194-1201, 1223-1226)
- Only update items where the category is not already set (lines 1214, 1241)
- For Purchase Invoices, copy from parent document (line 1210)
- For Sales Invoices, copy from the customer master (line 1238)
The difference in source (parent vs. customer) aligns with the business logic where Sales Invoices derive TCS categories from the customer.
…n addition to category for sales and purchase invoices.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
erpnext/accounts/doctype/purchase_invoice/purchase_invoice.py (1)
1711-1725: Consider removing legacy 'Tax Withheld Vouchers' from ignore_linked_doctypes
ignore_linked_doctypesstill includes"Tax Withheld Vouchers"even though this doctype is being removed in the new withholding model. It’s harmless but stale; cleaning it up and relying solely on"Tax Withholding Entry"here would reduce confusion for future maintainers.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
erpnext/accounts/doctype/purchase_invoice/purchase_invoice.jserpnext/accounts/doctype/purchase_invoice/purchase_invoice.pyerpnext/accounts/doctype/sales_invoice/sales_invoice.jserpnext/accounts/doctype/sales_invoice/sales_invoice.pyerpnext/accounts/doctype/subscription/subscription.pyerpnext/public/js/utils/party.js
🧰 Additional context used
🧠 Learnings (6)
📓 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:40.898Z
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:40.898Z
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:40.898Z
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/subscription/subscription.pyerpnext/accounts/doctype/sales_invoice/sales_invoice.jserpnext/accounts/doctype/sales_invoice/sales_invoice.pyerpnext/accounts/doctype/purchase_invoice/purchase_invoice.jserpnext/accounts/doctype/purchase_invoice/purchase_invoice.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/subscription/subscription.pyerpnext/accounts/doctype/sales_invoice/sales_invoice.pyerpnext/accounts/doctype/purchase_invoice/purchase_invoice.py
📚 Learning: 2025-10-17T12:17:35.397Z
Learnt from: sagarvora
Repo: frappe/erpnext PR: 50155
File: erpnext/controllers/taxes_and_totals.py:722-736
Timestamp: 2025-10-17T12:17:35.397Z
Learning: In ERPNext discount validation (erpnext/controllers/taxes_and_totals.py), the apply_discount_on field is only used to determine the base amount for calculating discount_amount from additional_discount_percentage. Once the discount_amount is determined (whether from percentage or manual entry), it should always be validated against grand_total, not against the field specified in apply_discount_on, because the discount cannot exceed the total invoice amount.
Applied to files:
erpnext/accounts/doctype/sales_invoice/sales_invoice.jserpnext/accounts/doctype/purchase_invoice/purchase_invoice.js
📚 Learning: 2025-10-09T06:59:10.528Z
Learnt from: ljain112
Repo: frappe/erpnext PR: 49963
File: erpnext/accounts/doctype/journal_entry/journal_entry.py:0-0
Timestamp: 2025-10-09T06:59:10.528Z
Learning: In erpnext/accounts/doctype/journal_entry/journal_entry.py, the validate() method calls validate_multi_currency() before apply_tax_withholding(). The validate_multi_currency() method internally calls set_exchange_rate(), so exchange rates are properly set on all account rows before TDS calculations occur in apply_tax_withholding().
Applied to files:
erpnext/accounts/doctype/sales_invoice/sales_invoice.jserpnext/accounts/doctype/sales_invoice/sales_invoice.pyerpnext/accounts/doctype/purchase_invoice/purchase_invoice.jserpnext/accounts/doctype/purchase_invoice/purchase_invoice.py
📚 Learning: 2025-08-18T11:24:53.334Z
Learnt from: diptanilsaha
Repo: frappe/erpnext PR: 49210
File: erpnext/accounts/doctype/sales_invoice/sales_invoice.js:61-67
Timestamp: 2025-08-18T11:24:53.334Z
Learning: In Sales Invoice onload, the direct frappe.db.get_value call for disable_grand_total_to_default_mop is necessary for saved POS invoices, as the is_pos trigger may not fire for already-saved invoices, meaning set_pos_data wouldn't run and skip_default_payment wouldn't be set from the server response.
Applied to files:
erpnext/accounts/doctype/sales_invoice/sales_invoice.pyerpnext/accounts/doctype/purchase_invoice/purchase_invoice.py
🧬 Code graph analysis (1)
erpnext/accounts/doctype/purchase_invoice/purchase_invoice.py (1)
erpnext/accounts/doctype/tax_withholding_entry/tax_withholding_entry.py (5)
PurchaseTaxWithholding(1117-1123)TaxWithholdingEntry(19-342)on_validate(1112-1114)on_submit(1079-1082)on_cancel(1084-1088)
⏰ 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 (1)
- GitHub Check: Python Unit Tests (4)
- GitHub Check: Python Unit Tests (2)
- GitHub Check: Python Unit Tests (3)
- GitHub Check: Patch Test
- GitHub Check: Summary
🔇 Additional comments (13)
erpnext/accounts/doctype/subscription/subscription.py (1)
414-418: The implementation is correct and consistent across the codebase. Thetax_withholding_groupfield exists in the Supplier doctype, and the same pattern—checking bothtax_withholding_categoryandtax_withholding_groupto setapply_tds—is consistently applied in PurchaseInvoice (onload and set_missing_values), SalesInvoice, and Subscription invoice creation methods.erpnext/public/js/utils/party.js (1)
110-113: Tax withholding fields correctly propagated from party detailsAssigning
tax_withholding_categoryandtax_withholding_groupontofrmfromr.messagematches how Sales and Purchase Invoice controllers later read these properties, so party-level withholding config will be available without extra RPCs.erpnext/accounts/doctype/sales_invoice/sales_invoice.js (4)
25-29: Clearing tax_withholding_entries on company change is appropriateResetting
tax_withholding_entriesincompany()avoids stale withholding rows tied to a previous company, which is important since thresholds and accounts are company-specific.
371-389: apply_tds now respects both tax_withholding_category and tax_withholding_groupDeriving
doc.apply_tdsfromme.frm.tax_withholding_category || me.frm.tax_withholding_groupand clearingtax_withholding_entriesin the party-details callback keeps the Sales Invoice’s withholding state aligned with the customer’s configuration and the new Tax Withholding Group concept, matching howparty.jspopulates these properties.
605-608: apply_tds field handler keeps withholding table in syncUsing the
apply_tds(frm)handler to cleartax_withholding_entrieswhen the checkbox changes is a minimal, predictable client-side behaviour and leaves the actual withholding computation to the server-sideSalesTaxWithholdinglogic.
823-835: Onload initialization of apply_tds and tax_withholding_entries for new invoicesSeeding
frm.doc.apply_tdsfromfrm.doc.__onload.apply_tdsonly for new documents, and clearingtax_withholding_entriesunder the same condition, ensures mapped/duplicated invoices get the correct default while existing submitted/draft invoices retain their current withholding configuration.erpnext/accounts/doctype/purchase_invoice/purchase_invoice.js (4)
340-369: Supplier party refresh correctly drives apply_tds and resets withholding entriesIn the
supplier()callback, computingdoc.apply_tdsfromme.frm.tax_withholding_category || me.frm.tax_withholding_groupand clearingtax_withholding_entrieskeeps the Purchase Invoice’s withholding state in sync with the supplier’s configuration as populated byerpnext.utils.get_party_details.
379-382: Purchase Invoice apply_tds handler mirrors Sales Invoice behaviourClearing
tax_withholding_entriesin theapply_tds(frm)handler is consistent with the Sales Invoice flow and ensures the client-side table is reset whenever the user toggles withholding on or off; all heavy logic remains on the server.
671-685: Onload initialization of apply_tds and withholding entries for new Purchase InvoicesUsing
frm.doc.__onload.apply_tdsto seedapply_tdsonly whenfrm.is_new(), and clearingtax_withholding_entriesunder the same condition, correctly applies supplier defaults for newly created or mapped invoices without touching existing records.
708-711: Clearing tax_withholding_entries on company change avoids cross-company leakageResetting
tax_withholding_entrieswhencompanychanges helps ensure no withholding rows created under the previous company linger on the document, which would otherwise confuse thresholds and ledger selection.erpnext/accounts/doctype/sales_invoice/sales_invoice.py (1)
29-30: SalesTaxWithholding integration and new withholding fields look consistentImporting
SalesTaxWithholding, exposingapply_tds,ignore_tax_withholding_threshold,override_tax_withholding_entries,tax_withholding_entries, andtax_withholding_groupin the auto-generated types, wiringSalesTaxWithholding(self).on_validate() / on_submit() / on_cancel(), and adding"Tax Withholding Entry"toignore_linked_doctypescollectively give Sales Invoice a clean integration with the new Tax Withholding Entry workflow. The placement ofSalesTaxWithholding(self).on_validate()aftercheck_conversion_rate()and the rest of the core validation keeps exchange rates and base amounts stable before withholding is computed, mirroring the established Journal Entry pattern where currency validation precedes tax withholding. Based on learnings, this ordering is appropriate.Also applies to: 78-79, 92-93, 166-167, 219-221, 289-295, 305-306, 448-449, 589-632
erpnext/accounts/doctype/purchase_invoice/purchase_invoice.py (2)
27-28: PurchaseTaxWithholding wiring and new withholding fields are coherentImporting
PurchaseTaxWithholding, adding type hints forignore_tax_withholding_threshold,override_tax_withholding_entries,tax_withholding_entries, andtax_withholding_group, seeding__onload.apply_tdsandapply_tdsfrom the Supplier’s tax_withholding_category/group, and invokingPurchaseTaxWithholding(self)invalidate(),on_submit(), andon_cancel()aligns Purchase Invoice with the new centralized Tax Withholding Entry flow. The placement ofPurchaseTaxWithholding(self).on_validate()aftercheck_conversion_rate()and other core validations keeps currency information stable before withholding is computed, which matches the established ordering used in Journal Entry withholding logic. Based on learnings, this sequencing is appropriate.Also applies to: 72-73, 126-127, 148-149, 198-200, 242-251, 301-302, 354-360, 749-751, 1675-1676, 1723-1725
808-888: No double-booking of TDS GL entries occurs.PurchaseTaxWithholding.on_submit()does not create GL entries—it only processes tax withholding adjustments by updating existingTaxWithholdingEntryrecords via database updates. GL entries for Purchase Invoices are created exclusively inmake_gl_entries_for_tax_withholding(), which handles the supplier-side GL posting whenapply_tdsis set. TheTaxWithholdingEntrysystem serves as a tracking and reconciliation layer, not a GL posting mechanism.
… tax withholding details in invoices.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
erpnext/accounts/doctype/purchase_invoice/purchase_invoice.py (1)
355-362: Consider documenting the conditional apply_tds behavior.The logic correctly auto-sets
apply_tds = 1only during initial value population (not for_validate), preventing inadvertent overrides during validation. This preserves user choices during form submission while still initializing the field appropriately for new invoices.Consider adding an inline comment explaining why
apply_tdsis only set whennot for_validate, as this pattern may not be immediately obvious to future maintainers.💡 Suggested inline comment
if self.supplier: tax_withholding_category, tax_withholding_group = frappe.get_cached_value( "Supplier", self.supplier, ["tax_withholding_category", "tax_withholding_group"] ) if not for_validate: + # Only auto-set apply_tds during initialization, not during validation + # This prevents overwriting user's explicit choice during form submission if tax_withholding_category or tax_withholding_group: self.apply_tds = 1
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
erpnext/accounts/doctype/purchase_invoice/purchase_invoice.pyerpnext/accounts/doctype/sales_invoice/sales_invoice.py
🧰 Additional context used
🧠 Learnings (5)
📓 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:40.898Z
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:40.898Z
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:40.898Z
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/purchase_invoice/purchase_invoice.pyerpnext/accounts/doctype/sales_invoice/sales_invoice.py
📚 Learning: 2025-10-09T06:59:10.528Z
Learnt from: ljain112
Repo: frappe/erpnext PR: 49963
File: erpnext/accounts/doctype/journal_entry/journal_entry.py:0-0
Timestamp: 2025-10-09T06:59:10.528Z
Learning: In erpnext/accounts/doctype/journal_entry/journal_entry.py, the validate() method calls validate_multi_currency() before apply_tax_withholding(). The validate_multi_currency() method internally calls set_exchange_rate(), so exchange rates are properly set on all account rows before TDS calculations occur in apply_tax_withholding().
Applied to files:
erpnext/accounts/doctype/purchase_invoice/purchase_invoice.pyerpnext/accounts/doctype/sales_invoice/sales_invoice.py
📚 Learning: 2025-08-18T11:24:53.334Z
Learnt from: diptanilsaha
Repo: frappe/erpnext PR: 49210
File: erpnext/accounts/doctype/sales_invoice/sales_invoice.js:61-67
Timestamp: 2025-08-18T11:24:53.334Z
Learning: In Sales Invoice onload, the direct frappe.db.get_value call for disable_grand_total_to_default_mop is necessary for saved POS invoices, as the is_pos trigger may not fire for already-saved invoices, meaning set_pos_data wouldn't run and skip_default_payment wouldn't be set from the server response.
Applied to files:
erpnext/accounts/doctype/purchase_invoice/purchase_invoice.pyerpnext/accounts/doctype/sales_invoice/sales_invoice.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/purchase_invoice/purchase_invoice.pyerpnext/accounts/doctype/sales_invoice/sales_invoice.py
🧬 Code graph analysis (1)
erpnext/accounts/doctype/purchase_invoice/purchase_invoice.py (1)
erpnext/accounts/doctype/tax_withholding_entry/tax_withholding_entry.py (5)
PurchaseTaxWithholding(1117-1123)TaxWithholdingEntry(19-342)on_validate(1112-1114)on_submit(1079-1082)on_cancel(1084-1088)
⏰ 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). (7)
- GitHub Check: Python Unit Tests (1)
- GitHub Check: Python Unit Tests (4)
- GitHub Check: Python Unit Tests (2)
- GitHub Check: Python Unit Tests (3)
- GitHub Check: Patch Test
- GitHub Check: semgrep
- GitHub Check: Summary
🔇 Additional comments (9)
erpnext/accounts/doctype/sales_invoice/sales_invoice.py (5)
29-29: LGTM: Clean migration to the new Tax Withholding controller.The import change correctly replaces the old function-based approach with the new
SalesTaxWithholdingcontroller class, consistent with the refactoring described in the PR.
78-78: LGTM: Type annotations are correct and consistent.The new fields and type hints follow ERPNext conventions and properly integrate the Tax Withholding Entry child table with the Sales Invoice document.
Also applies to: 92-92, 138-138, 166-166, 219-220
289-296: LGTM: onload method correctly fetches tax withholding settings.The implementation properly:
- Guards against missing customer (defensive programming, even though onload only fires for saved documents)
- Fetches both
tax_withholding_categoryandtax_withholding_groupin a single cached query- Sets
apply_tdsto whichever field is populated (category takes precedence if both exist)
306-306: LGTM: Tax withholding validation correctly integrated.The call to
SalesTaxWithholding(self).on_validate()is properly placed aftersuper().validate()(which calculates amounts) and follows the new controller-based architecture.
449-449: LGTM: Submit and cancel hooks properly integrated.The lifecycle integrations are correct:
on_submit()creates tax withholding entries before stock updateson_cancel()reverses withholding entries before stock reversal- Adding "Tax Withholding Entry" to
ignore_linked_doctypesprevents cascade deletion errorsThe implementation maintains proper symmetry and follows ERPNext patterns.
Also applies to: 590-590, 632-632
erpnext/accounts/doctype/purchase_invoice/purchase_invoice.py (4)
27-27: LGTM: Clean integration with new tax withholding module.The import of
PurchaseTaxWithholdingcontroller and the TYPE_CHECKING import ofTaxWithholdingEntryproperly integrate with the new tax withholding architecture.Also applies to: 72-72
126-126: LGTM: New field declarations are properly typed.The new tax withholding fields (
ignore_tax_withholding_threshold,override_tax_withholding_entries,tax_withholding_entries,tax_withholding_group) follow Frappe typing conventions correctly.Also applies to: 148-148, 198-199
244-251: LGTM: Proper initialization of tax withholding state.The onload method correctly:
- Fetches supplier tax withholding configuration using cached values
- Sets UI state via
set_onload- Initializes the tax_withholding_entries child table for new documents
302-302: LGTM: Controller integration follows proper lifecycle patterns.The tax withholding controller is properly integrated:
on_validate()at line 302: Called after core validations, ensuring tax calculations work with validated dataon_submit()at line 752: Called before GL entry creation (line 785), allowing withholding entries to be processed firston_cancel()at line 1677: Called early to clean up withholding state- Line 1725: Correctly adds "Tax Withholding Entry" to
ignore_linked_doctypesfor automatic cleanupThe separation between
tax_withholding_entries(tracking/audit) and thetaxestable (GL source) is maintained—the controller likely updatestaxesbased on withholding calculations.Also applies to: 752-752, 1677-1677, 1725-1725
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
erpnext/patches/v16_0/migrate_tax_withholding_data.py (1)
1167-1176: Consider removing redundant abs() call.Line 1176 applies
abs(calculated_taxable), butcalculated_taxableis already guaranteed to be non-negative from line 1170 (result ofmin(positive, positive)). Theabs()is harmless but unnecessary.Optional simplification
- if tax_on_excess and calculated_taxable < info.total_debit: - taxable_exemption_amount = flt(info.total_debit - abs(calculated_taxable), 2) + if tax_on_excess and calculated_taxable < info.total_debit: + taxable_exemption_amount = flt(info.total_debit - calculated_taxable, 2)
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
erpnext/patches/v16_0/migrate_tax_withholding_data.pyerpnext/stock/get_item_details.py
🚧 Files skipped from review as they are similar to previous changes (1)
- erpnext/stock/get_item_details.py
🧰 Additional context used
🧠 Learnings (5)
📓 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:40.898Z
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:40.898Z
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:40.898Z
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/patches/v16_0/migrate_tax_withholding_data.py
📚 Learning: 2025-08-20T11:58:32.385Z
Learnt from: ljain112
Repo: frappe/erpnext PR: 48864
File: erpnext/controllers/accounts_controller.py:2586-2596
Timestamp: 2025-08-20T11:58:32.385Z
Learning: In erpnext/controllers/accounts_controller.py, the posting_date parameter passed to get_due_date() and get_discount_date() is mandatory and calculated from bill_date/posting_date/transaction_date, so it should not be None.
Applied to files:
erpnext/patches/v16_0/migrate_tax_withholding_data.py
📚 Learning: 2025-10-09T06:59:10.528Z
Learnt from: ljain112
Repo: frappe/erpnext PR: 49963
File: erpnext/accounts/doctype/journal_entry/journal_entry.py:0-0
Timestamp: 2025-10-09T06:59:10.528Z
Learning: In erpnext/accounts/doctype/journal_entry/journal_entry.py, the validate() method calls validate_multi_currency() before apply_tax_withholding(). The validate_multi_currency() method internally calls set_exchange_rate(), so exchange rates are properly set on all account rows before TDS calculations occur in apply_tax_withholding().
Applied to files:
erpnext/patches/v16_0/migrate_tax_withholding_data.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/patches/v16_0/migrate_tax_withholding_data.py
🧬 Code graph analysis (1)
erpnext/patches/v16_0/migrate_tax_withholding_data.py (1)
erpnext/patches/v16_0/update_tax_withholding_field_in_payment_entry.py (1)
execute(5-10)
⏰ 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 (1)
- GitHub Check: Python Unit Tests (3)
- GitHub Check: Python Unit Tests (2)
- GitHub Check: Patch Test
- GitHub Check: Summary
🔇 Additional comments (17)
erpnext/patches/v16_0/migrate_tax_withholding_data.py (17)
28-48: Well-structured migration orchestration.The main
execute()function properly orchestrates the migration with clear separation of concerns: building lookup tables, cleaning existing migration data for idempotency, delegating to specialized migrators, and propagating categories to items. The use of shared caches (party_tax_id_cache, column_cache) across migrators is efficient.
80-106: Good handling of ambiguous TDS account configurations.The account mapping logic correctly identifies when multiple categories use the same account (lines 95-97) and marks them as ambiguous (
None). This prevents incorrect category assignment during migration.
134-142: Date validation now correctly implemented.The function properly validates that
posting_date,rate.from_date, andrate.to_dateare all present before performing comparisons, preventingTypeErrorin Python 3.
165-186: Clear status determination logic.The status derivation correctly handles all scenarios: Duplicate, Settled (with reason or both docs), Under Withheld (taxable only), and Over Withheld (withholding only).
189-291: Robust bulk insertion with collision avoidance.The unique name generation fetches existing names upfront and maintains a set to prevent collisions within the batch. Combined with
frappe.generate_hash(length=10)(which provides 62^10 possible values), collision probability is very low. The use ofignore_duplicates=Trueis acceptable given the collision safeguards in place.
299-348: Well-architected migrator class with comprehensive data structures.The
PurchaseInvoiceMigratorclass is excellently structured with clear separation between raw query results, lookup dictionaries, tracking sets, and output. The orchestration inmigrate()follows a logical flow: fetch → build lookups → fetch dates → process → bulk insert.
354-465: Comprehensive data fetching with proper joins.The four queries efficiently fetch all necessary data upfront:
- Invoices with TDS in taxes table
- Tax withheld vouchers with PI info
- Advance taxes with PI info
- Payment Entries with TDS
All queries use proper joins and appropriate filters (
docstatus == 1, account in TDS accounts). The approach minimizes database round-trips.
645-683: Correct handling of advance tax allocations.The logic properly:
- Back-calculates taxable amount from allocated TDS using the tax rate (lines 657-660)
- Accumulates this into
current_taxablefor threshold calculations (line 662)- Creates matching entries in both Purchase Invoice and Payment Entry (lines 665-681)
- Marks the PE entry as duplicate to avoid double-counting
685-743: Sophisticated TDS splitting logic for cumulative thresholds.The implementation correctly:
- Splits TDS between current and past invoices when
tax_on_excessis False (lines 694-698)- Handles threshold exemption for
tax_on_excesscategories by creating a separate exemption entry (lines 708-724)- Uses defensive
max(0, ...)to handle edge cases (line 696)This aligns with the PR's stated support for cumulative threshold tracking.
868-887: Proper handling of return invoice sign conventions.The
_create_entry()method correctly negates bothtaxable_amountandwithholding_amountfor return invoices (lines 871-875), ensuring consistent data representation for reversed transactions.
902-950: Efficient Sales Invoice TCS migration using GL aggregation.The query correctly aggregates TCS from GL Entries using conditional
Sum()and groups by Sales Invoice. The use ofMax()for account selection is acceptable given the learning that only one TCS account per Sales Invoice was supported in the old system.Based on learnings, the account selection approach is appropriate for the historical data constraints.
980-1014: Threshold exemption logic now correctly handles returns.For return invoices:
- Line 982-983: Amounts are negated to represent reversals
- Line 989: Uses
abs(calculated_taxable)for comparison with positivenet_grand_total✓- Line 990: Computes exemption from absolute values ✓
- Lines 992-993: Applies negative sign for returns ✓
The logic correctly handles both normal and return invoices after the fixes from previous reviews.
1041-1048: Proper propagation of TCS flag to items.The code correctly sets both
apply_tdsandtax_withholding_categoryon Sales Invoice Items after creating entries. This ensures items are properly marked for the new TDS system.
1056-1123: Correct Journal Entry TDS identification and party resolution.The migration properly:
- Identifies TDS from GL Entries on TDS accounts (lines 1076-1095)
- Extracts party information from JE Account rows that are not TDS accounts (line 1118), ensuring party is from the receivable/payable side, not the TDS liability account
1142-1144: Appropriate filtering of TDS payment entries.Skipping Journal Entries with
total_tds <= 0correctly excludes TDS payment/reversal entries, focusing the migration on TDS booking entries. This aligns with the migration's goal of creating Tax Withholding Entry records for withholding transactions, not payments.
1236-1262: Selective category propagation for Purchase Invoice Items.The function only copies
tax_withholding_categoryto Purchase Invoice Items whereapply_tds == 1(line 1259), preserving the item-level TDS designation from the old system. This ensures only items that previously had TDS applied receive the category.
1265-1289: Broad category propagation for Sales Invoice Items from Customer.Unlike the purchase migration, this function copies the customer's
tax_withholding_categoryto all items without an existing category (noapply_tdsfilter). The difference in approach reflects that Sales Invoice item-level TCS may not have been tracked in the old system, so the migration infers it from the customer level.
4 participants
This PR introduces a comprehensive refactor of the Tax Withholding (TDS/TCS) system in ERPNext. The key change is the introduction of a new child table Tax Withholding Entry that tracks every tax withholding transaction with full auditability, replacing the previous approach of calculating TDS/TCS on-the-fly.
Breaking Changes
Tax Withheld Voucherschild table - Replaced byTax Withholding Entryallocated_amountfield from Advance Taxes and charges table - Advance TDS allocation is now tracked through Tax Withholding Entriesconsider_party_ledger_amountsetting - Simplified threshold calculation logic (User will have select the checkbox for document to be considered).New Architecture
Tax Withholding Entry (Child Table)
A new child doctype that tracks every tax withholding transaction with the following key fields:
tax_withholding_categorytaxable_amountwithholding_amounttax_ratetaxable_doctype/namewithholding_doctype/namestatusunder_withheld_reasonlower_deduction_certificateEntry Status Flow
Controller Architecture
Key Features
1. Automatic Entry Matching
When a document is submitted, the system automatically matches:
2. Cumulative Threshold Tracking
3. Tax on Excess Amount
When
tax_on_excess_amountis enabled:under_withheld_reason = "Threshold Exemption"4. Lower Deduction Certificate (LDC) Support
5. Advance Payment TDS/TCS
Scenario: Payment Entry with TDS → Sales Invoice allocation
6. Item-Level Tax Withholding
tax_withholding_category7. Return Invoice Handling
8. Tax Withholding Group
Introduces Tax Withholding Group to support different TDS/TCS rates within the same category based on entity type.
Use Case
In India, Section 194C has different rates:
Migration
A patch
update_tax_withholding_field_in_payment_entryhandles:apply_tax_withholding_amounttoapply_tdsin Payment EntryFuture Enhancements
Checklist
Features
no-docs