Fix item_code[1] bug in get_item_tax_info (Closes #26136) #50726
Conversation
…rappe#26136) This PR fixes issue frappe#26136. - Replaced incorrect `item_code[1]` usage in `get_item_tax_info` with `item_code`. - Ensures correct tax template and item tax rate mapping without causing 404 errors. Closes frappe#26136.
devendra1973
requested review from
mihir-kandoi and
rohitwaghchaure
as code owners
📝 WalkthroughWalkthroughThe Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
erpnext/stock/get_item_details.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: rohitwaghchaure
Repo: frappe/erpnext PR: 49766
File: erpnext/manufacturing/doctype/production_plan/production_plan.py:1717-1717
Timestamp: 2025-09-30T11:04:46.510Z
Learning: In the Production Plan's `get_items_for_material_requests` function in `erpnext/manufacturing/doctype/production_plan/production_plan.py`, always use `data.get("sales_order")` instead of `doc.get("sales_order")` when iterating over `po_items`. This ensures raw materials are correctly grouped by each production item's respective Sales Order, not a global document-level Sales Order.
⏰ 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). (1)
- GitHub Check: Summary
| for item_code in item_codes: | ||
| if not item_code or item_code[1] in out or not item_tax_templates.get(item_code[1]): | ||
| if not item_code or item_code in out or not item_tax_templates.get(item_code): | ||
|
|
||
| continue | ||
|
|
||
| out[item_code[1]] = ItemDetails() | ||
| item = frappe.get_cached_doc("Item", item_code[0]) | ||
| out[item_code] = ItemDetails() | ||
| item = frappe.get_cached_doc("Item", item_code) | ||
| ctx: ItemDetailsCtx = { | ||
| "company": doc.company, | ||
| "tax_category": tax_category, | ||
| "base_net_rate": item_rates.get(item_code[1]), | ||
| "base_net_rate": item_rates.get(item_code), | ||
| } | ||
|
|
||
| if item_tax_templates: | ||
| ctx.update({"item_tax_template": item_tax_templates.get(item_code[1])}) | ||
| ctx.update({"item_tax_template": item_tax_templates.get(item_code)}) | ||
|
|
||
| get_item_tax_template(ctx, item, out[item_code[1]]) | ||
| out[item_code[1]]["item_tax_rate"] = get_item_tax_map( | ||
| get_item_tax_template(ctx, item, out[item_code]) | ||
| out[item_code]["item_tax_rate"] = get_item_tax_map( | ||
| doc=doc, | ||
| tax_template=out[item_code[1]].get("item_tax_template"), | ||
| tax_template=out[item_code].get("item_tax_template"), | ||
| as_json=True, | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
Fix correctly normalizes get_item_tax_info to use plain item_code keys everywhere
The updated loop now consistently treats each item_code entry as the actual item code string (for the guard, out key, Item fetch, item_rates, and item_tax_templates), and passes that through to get_item_tax_template / get_item_tax_map. This aligns with how callers supply data and should eliminate the 404s caused by looking up Item with item_code[1].
One thing to double‑check: if any legacy call sites are still passing tuples (e.g. (idx, item_code)) instead of bare strings into item_codes / item_rates / item_tax_templates, those will now break. It’s worth quickly scanning usages to confirm everything passes plain item codes.
You can use a script like this from the repo root to inspect how get_item_tax_info is called:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show all Python & JS call sites with a bit of context
rg -n "get_item_tax_info" -S -C5Length of output: 1455
I can see one call site in the JavaScript file. Let me verify how item_codes and item_rates are constructed to ensure they're passing plain strings, not tuples.
---
🏁 Script executed:
```shell
#!/bin/bash
set -euo pipefail
# Search in transaction.js for where item_codes and item_rates are built, with more context
rg -n "item_codes|item_rates" erpnext/public/js/controllers/transaction.js -B10 -A10 | head -100
Length of output: 84
---
🏁 Script executed:
```shell
#!/bin/bash
set -euo pipefail
# Find where item_codes and item_rates are populated in transaction.js
rg -n "item_codes\s*=" erpnext/public/js/controllers/transaction.js -B5 -A5
Length of output: 250
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find other Python call sites to get_item_tax_info
rg "get_item_tax_info" --type=py -B3 -A3Length of output: 398
🏁 Script executed:
#!/bin/bash
set -euoypipefail
# Check the full args being passed in the JS call
rg -n "get_item_tax_info" erpnext/public/js/controllers/transaction.js -A15Length of output: 657
---
🏁 Script executed:
```shell
#!/bin/bash
set -euo pipefail
# Check the full args being passed in the JS call (fixed syntax)
rg -n "get_item_tax_info" erpnext/public/js/controllers/transaction.js -A15
Length of output: 581
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find where values are pushed into item_codes array
rg -n "item_codes\.push\|item_codes\[" erpnext/public/js/controllers/transaction.js -B3 -A3Length of output: 0
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for where item_rates and item_tax_templates are populated
rg -n "item_rates\[|item_tax_templates\[" erpnext/public/js/controllers/transaction.js -B2 -A2Length of output: 285
Critical: The fix introduces a breaking mismatch between Python and JavaScript code
The Python code after the fix treats item_code as a plain string, but the JavaScript caller at erpnext/public/js/controllers/transaction.js:2561 pushes [item.item_code, item.name] tuples into the item_codes array. Additionally, item_rates and item_tax_templates are keyed by item.name (not item_code).
This will cause:
item_tax_templates.get(item_code)on line 668 to fail (looking up an array as key in a dict keyed by names)frappe.get_cached_doc("Item", item_code)on line 673 to fail (passing an array instead of a string)item_rates.get(item_code)on line 676 to fail (same dict lookup issue)
Fix required: Unpack the tuple after the loop start:
for item_code_pair in item_codes:
item_code, item_name = item_code_pair # or use tuple unpacking in the for statementThen use item_code for Item fetches and item_name for all dict lookups (item_rates, item_tax_templates, out keys).
🤖 Prompt for AI Agents
In erpnext/stock/get_item_details.py around lines 667-688, item_codes is
populated by the JS caller as [item_code, item_name] tuples but the loop treats
each entry as a plain string, causing dict lookups and frappe.get_cached_doc to
fail; unpack each tuple at the start of the loop (either in the for statement or
by assignment) into item_code and item_name, then use item_code only for
fetching the Item (frappe.get_cached_doc) and use item_name for lookups into
item_rates, item_tax_templates and as the key in out (i.e., ensure out is keyed
by item_name where the JS expects names), and update subsequent references
accordingly.
1 participant
This PR fixes issue #26136.
item_code[1]usage inget_item_tax_infowithitem_code.Closes #26136.