fix(tds): multiple fixes for tax withholding entry#54483
Draft
ljain112 wants to merge 8 commits intofrappe:developfrom
Draft
fix(tds): multiple fixes for tax withholding entry#54483ljain112 wants to merge 8 commits intofrappe:developfrom
ljain112 wants to merge 8 commits intofrappe:developfrom
Conversation
ljain112
requested review from
mihir-kandoi,
rohitwaghchaure and
ruthra-kumar
as code owners
github-actions
Bot
added
accounts
needs-tests
Contributor
📝 WalkthroughWalkthroughClient-side invoice logic now centralizes TDS/withholding behavior in TransactionController by adding Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@erpnext/public/js/controllers/transaction.js`:
- Around line 3189-3195: The withholding table is cleared with
me.frm.clear_table("tax_withholding_entries") but only the items grid is
refreshed; update the code that sets item.apply_tds (the block using var me =
this; and $.each(this.frm.doc.items || [], ...)) to also refresh the cleared
withholding table by calling me.frm.refresh_field("tax_withholding_entries")
after clearing it so stale rows are not shown.
- Around line 3200-3217: In update_item_tax_withholding_categories, prevent
stale async responses overwriting newer party fallbacks by capturing the current
party identifier or a request token before making the frappe.call and verifying
it in the callback; e.g., store a local const requestParty = party_category (or
increment a this._itemTaxRequestId and capture it) and in the callback return
early if the saved token/party doesn't match the current one on this.frm (or the
latest request id). Apply this check before assigning
item.tax_withholding_category and refreshing the items field so only the latest
response updates rows.
In `@erpnext/stock/get_item_details.py`:
- Around line 1412-1418: The call to frappe.get_list in the return statement
should explicitly set limit_page_length to len(item_codes) so all requested
items are returned; update the frappe.get_list(...) invocation (the call that
uses filters={"name": ["in", item_codes]} and fields=["name", field]) to include
limit_page_length=len(item_codes) while keeping the surrounding
frappe._dict(...) return unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 17a6f4f4-77df-4255-a442-01ee419e78b8
📒 Files selected for processing (7)
erpnext/accounts/doctype/purchase_invoice/purchase_invoice.jserpnext/accounts/doctype/purchase_invoice_item/purchase_invoice_item.jsonerpnext/accounts/doctype/sales_invoice/sales_invoice.jserpnext/accounts/doctype/sales_invoice_item/sales_invoice_item.jsonerpnext/controllers/accounts_controller.pyerpnext/public/js/controllers/transaction.jserpnext/stock/get_item_details.py
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #54483 +/- ##
===========================================
- Coverage 79.44% 79.41% -0.03%
===========================================
Files 1160 1160
Lines 125984 126114 +130
===========================================
+ Hits 100083 100156 +73
- Misses 25901 25958 +57
🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@erpnext/stock/get_item_details.py`:
- Around line 1407-1419: The current code uses frappe.get_all (which bypasses
record-level permissions) and only does a generic frappe.has_permission check,
and it silently defaults to purchase category for unsupported doctype values;
change to frappe.get_list (which enforces permissions) when fetching Items in
the get_item_details flow, passing the same filters/fields/as_list options so
record permissions are applied; keep or tighten the DocType-level check but rely
on get_list for per-record enforcement; additionally validate the incoming
doctype against sales_doctypes and purchase_doctypes and raise a clear error
(e.g., frappe.throw or ValueError) instead of falling back to the purchase field
when doctype is unsupported; also ensure item_codes is parsed/validated as a
list before calling get_list.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 6b842d62-b1af-4beb-b50d-bccf205365b4
📒 Files selected for processing (2)
erpnext/public/js/controllers/transaction.jserpnext/stock/get_item_details.py
…x withholding categories
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
erpnext/stock/get_item_details.py (1)
1402-1419:⚠️ Potential issue | 🟡 MinorSet
limit_page_lengthexplicitly and validatedoctypeinput.Two concerns on this new whitelisted endpoint:
frappe.get_listapplies a default page length of 20 in many contexts; invoices with more than 20 items will silently get partial results and those rows will fall back toparty_categoryon the client. Passlimit_page_length=len(item_codes)(or0for no limit) to make intent explicit.- Any
doctypevalue outsidesales_doctypessilently falls back to the purchase field. Since this is a@frappe.whitelist()endpoint reachable by any logged-in user, validatedoctypeagainst the allowed list and throw otherwise.Also worth parsing/guarding
item_codesfor the empty/string case before the query.Proposed fix
`@frappe.whitelist`() def get_item_tax_withholding_categories(item_codes: list | str, doctype: str) -> dict: """ Return a mapping of {item_code: tax_withholding_category} from the Item master. """ item_codes = frappe.parse_json(item_codes) + if isinstance(item_codes, str): + item_codes = [item_codes] + if not item_codes: + return {} + + if doctype not in [*sales_doctypes, *purchase_doctypes]: + frappe.throw(_("Invalid doctype for tax withholding category lookup")) + field = ( "sales_tax_withholding_category" if doctype in sales_doctypes else "purchase_tax_withholding_category" ) return frappe._dict( frappe.get_list( "Item", filters={"name": ["in", item_codes]}, fields=["name", field], as_list=1, + limit_page_length=len(item_codes), ) )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@erpnext/stock/get_item_details.py` around lines 1402 - 1419, In get_item_tax_withholding_categories, ensure you validate the doctype against the allowed sales_doctypes list and raise an error if it is not a recognized sales or purchase document; also guard/parse item_codes to return an empty dict quickly when item_codes is empty or invalid. When calling frappe.get_list, pass an explicit limit_page_length (use len(item_codes) or 0 for unlimited) and keep the existing field selection logic that picks "sales_tax_withholding_category" or "purchase_tax_withholding_category"; reference the function name get_item_tax_withholding_categories, the local variable field, the sales_doctypes symbol, and the frappe.get_list call so you update those spots only.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@erpnext/stock/get_item_details.py`:
- Around line 1402-1419: In get_item_tax_withholding_categories, ensure you
validate the doctype against the allowed sales_doctypes list and raise an error
if it is not a recognized sales or purchase document; also guard/parse
item_codes to return an empty dict quickly when item_codes is empty or invalid.
When calling frappe.get_list, pass an explicit limit_page_length (use
len(item_codes) or 0 for unlimited) and keep the existing field selection logic
that picks "sales_tax_withholding_category" or
"purchase_tax_withholding_category"; reference the function name
get_item_tax_withholding_categories, the local variable field, the
sales_doctypes symbol, and the frappe.get_list call so you update those spots
only.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 17afe543-3602-4b6c-9c9f-c3b5eaf3fdbc
📒 Files selected for processing (1)
erpnext/stock/get_item_details.py
|
This pull request has been automatically marked as inactive because it has not had recent activity. It will be closed within 3 days if no further activity occurs, but it only takes a comment to keep a contribution alive :) Also, even if it is closed, you can always reopen the PR when you're ready. Thank you for contributing. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.