feat: add support for 'not applicable' tax in item tax templates #50898
Conversation
ljain112
requested review from
mihir-kandoi,
rohitwaghchaure and
ruthra-kumar
as code owners
github-actions
bot
added
accounts
buying
needs-tests
ljain112
force-pushed
the
feat-not-applicable-tax
branch
from
0e7786d to
144e0e1
Compare
|
Warning Rate limit exceeded@ljain112 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 10 minutes and 54 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis pull request introduces functionality to mark taxes as "not applicable" to specific items. A new Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–25 minutes Areas requiring extra attention:
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
erpnext/controllers/taxes_and_totals.py (2)
29-29: Centralize NOT_APPLICABLE_TAX usage across modules / layersDefining a single
NOT_APPLICABLE_TAX = "N/A"constant here is good, but the same sentinel is also used in other Python modules and in the JS controller in this PR. Consider either:
- Importing this constant wherever possible on the Python side, and
- Adding a short comment noting that JS must use the same literal,
to reduce the risk of future drift between back-end and front-end or between modules.
582-623: Confirm omission of N/A taxes from item‑wise tax details is desiredThe early return when
tax_rate == NOT_APPLICABLE_TAXhappens beforeset_item_wise_taxis called, so for “N/A” taxes:
current_tax_amountandcurrent_net_amountremain zero (no effect on totals), and- No row is appended to
self.doc._item_wise_tax_details, soget_itemised_tax/ reports won’t show this tax at all for that item.This behavior seems consistent with “not applicable” (tax is ignored rather than shown as 0%), but it does mean item-wise tax breakup will not explicitly show an N/A entry. Please confirm this matches functional expectations for reports and UI; otherwise, you may want to record a zero-amount row with a special rate instead.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
erpnext/accounts/doctype/item_tax_template_detail/item_tax_template_detail.json(2 hunks)erpnext/accounts/doctype/item_tax_template_detail/item_tax_template_detail.py(1 hunks)erpnext/controllers/accounts_controller.py(2 hunks)erpnext/controllers/buying_controller.py(2 hunks)erpnext/controllers/taxes_and_totals.py(4 hunks)erpnext/public/js/controllers/taxes_and_totals.js(5 hunks)erpnext/stock/get_item_details.py(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 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/controllers/accounts_controller.py
🧬 Code graph analysis (1)
erpnext/stock/get_item_details.py (1)
erpnext/public/js/controllers/taxes_and_totals.js (1)
NOT_APPLICABLE_TAX(5-5)
⏰ 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 (3)
- GitHub Check: Python Unit Tests (4)
- GitHub Check: Python Unit Tests (2)
- GitHub Check: Python Unit Tests (1)
- GitHub Check: Summary
🔇 Additional comments (9)
erpnext/accounts/doctype/item_tax_template_detail/item_tax_template_detail.json (1)
9-11: Item Tax Template “Not Applicable” flag is well‑integratedField definition (Check with default
"0", clear description, and inclusion infield_order/ list view) androw_format: "Dynamic"all look consistent with intended usage and the rest of the DocType. No further changes needed here.Also applies to: 26-33, 45-45
erpnext/accounts/doctype/item_tax_template_detail/item_tax_template_detail.py (1)
14-23: Type hint matches new DocType field
not_applicable: DF.Checkcorrectly reflects the new Check field and keeps the auto‑generated typing block consistent. Nothing to change here.erpnext/controllers/buying_controller.py (1)
21-21: Buying valuation correctly skips “not applicable” item taxesThe additional check in
get_item_tax_amounttocontinuewhenrate == NOT_APPLICABLE_TAXcleanly avoids treating"N/A"as a 0% rate while excluding those lines from valuation tax. This aligns with how NOT_APPLICABLE_TAX is handled elsewhere and maintains backward compatibility for numeric rates.Also applies to: 502-519
erpnext/public/js/controllers/taxes_and_totals.js (1)
4-6: Client-side NOT_APPLICABLE_TAX handling is consistent and safeThe JS changes correctly introduce a scoped
NOT_APPLICABLE_TAXconstant and:
- Short‑circuit
get_current_tax_fractionandget_current_tax_amountwhen the per‑item rate is"N/A", avoiding numeric operations on the sentinel and excluding those taxes from per‑item tax math.- Propagate the sentinel through
_get_tax_ratewithout converting it, while still formatting numeric rates as before.- Skip creating header‑level tax rows in
add_taxes_from_item_tax_templatefor"N/A"entries.This mirrors the server‑side semantics and keeps the sentinel confined to
item_tax_ratemaps, so overall the client behaviour looks correct.Also applies to: 295-307, 331-341, 383-397, 530-538
erpnext/stock/get_item_details.py (1)
24-24: Verify non‑numeric rate handling foras_json=FalsecallersThe
NOT_APPLICABLE_TAXsentinel propagation inget_item_tax_mapis consistent with the pipeline's treatment (Python and JS both handle this case). However, whenas_json=False, callers may now receive string values ("N/A") instead of exclusively numeric rates. Search the codebase for uses ofget_item_tax_map(..., as_json=False)to confirm whether external or custom consumers depend on numeric-only values initem_tax_map; if so, this contract change should be documented.erpnext/controllers/accounts_controller.py (1)
66-66: Verify iftaxes_and_totals.pyimports fromaccounts_controller.pyto confirm circular dependency riskThe suggestion to move
NOT_APPLICABLE_TAXimport into the function body assumes thaterpnext.controllers.taxes_and_totalsimports helpers fromaccounts_controller.py. Without access to the actual codebase, this cannot be confirmed. If such imports exist, the module-level import could indeed reintroduce a circular dependency. If they do not exist, the module-level import is safe.Verify the dependency chain in
taxes_and_totals.pyto determine if this refactoring is necessary.erpnext/controllers/taxes_and_totals.py (3)
22-26: Imports align item tax template validation with shared utilitiesUsing
ItemDetailsCtx,_get_item_tax_template, andget_item_tax_mapfromerpnext.stock.get_item_detailskeeps item tax validation/mapping consistent with the central item-details logic and avoids duplication. The imports are used correctly invalidate_item_tax_templateandupdate_item_tax_map.
341-377: Short‑circuit for NOT_APPLICABLE_TAX in inclusive tax fraction is correctThe early return when
tax_rate == NOT_APPLICABLE_TAXensures that:
- No arithmetic is performed on the
"N/A"sentinel, and- Inclusive tax fractions and per‑qty inclusive amounts treat not-applicable taxes as if they don’t exist for that item,
which matches the intended semantics for per-item “not applicable” taxes.
378-387: Verify other consumers of item_tax_rate for "N/A" compatibility
_get_tax_ratenow propagates the"N/A"sentinel fromitem_tax_mapinstead of coercing it to a float. This meansitem.item_tax_rateJSON can now contain non-numeric values. While the two call sites within this class guard against"N/A", a comprehensive search across the codebase is needed to ensure no other consumers performflt()or numeric operations onitem_tax_ratewithout checking for the sentinel value first.Scan the codebase for all usages of
item_tax_rateto identify any potential incompatibilities.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #50898 +/- ##
========================================
Coverage 78.89% 78.90%
========================================
Files 1174 1174
Lines 119302 119352 +50
========================================
+ Hits 94123 94172 +49
- Misses 25179 25180 +1
🚀 New features to boost your workflow:
|
1 participant
closes: #44219