fix: after mapping of stock entry map correct addresses and gstin#3998
fix: after mapping of stock entry map correct addresses and gstin#3998karm1000 wants to merge 7 commits intoresilient-tech:developfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces hardcoded checks in 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@india_compliance/gst_india/overrides/subcontracting_transaction.py`:
- Around line 99-121: The Stock Entry branch currently returns the generic
from_fields/to_fields tuple (billing_address/company_gstin →
supplier_address/supplier_gstin) which don't exist on Stock Entry; update the
branch where source_doc.doctype == "Stock Entry" (the conditional that checks
doc.purpose == "Material Transfer" and not doc.is_return) to return Stock
Entry-specific keys: ("bill_from_address", "bill_from_gstin") as the from side
and ("bill_to_address", "bill_to_gstin") as the to side (leave the other
branches unchanged).
karm1000
force-pushed
the
fix/after-mapping-stock-entry
branch
from
ce26838 to
78be8d3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
india_compliance/gst_india/overrides/subcontracting_transaction.py (1)
117-122:⚠️ Potential issue | 🟠 MajorStock Entry branch uses incorrect field names.
This issue was previously flagged. When
source_doc.doctype == "Stock Entry", the function returns("billing_address", "company_gstin")and("supplier_address", "supplier_gstin"), but Stock Entry usesbill_from_address,bill_from_gstin,bill_to_address, andbill_to_gstinfields. This will causesource_doc.get()to returnNonefor all address/GSTIN fields.,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@india_compliance/gst_india/overrides/subcontracting_transaction.py` around lines 117 - 122, In the branch that checks source_doc.doctype == "Stock Entry" (inside the function handling subcontracting transactions when doc.purpose == "Material Transfer" and not doc.is_return), replace the returned field-name tuples so they reference Stock Entry field names: return from_fields as ("bill_from_address", "bill_from_gstin") and to_fields as ("bill_to_address", "bill_to_gstin") instead of the current ("billing_address","company_gstin") / ("supplier_address","supplier_gstin"); update the return statement that currently returns from_fields, to_fields accordingly so subsequent source_doc.get(...) calls retrieve the correct address/GSTIN values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@india_compliance/gst_india/overrides/subcontracting_transaction.py`:
- Around line 127-147: The function set_item_tax_template contains a dead
Purchase Order branch and will raise AttributeError when source_doc lacks
supplied_items; remove the dead check for "Purchase Order" and add a doctype
guard before iterating source_doc.supplied_items (only Subcontracting Order has
supplied_items per _get_fields_mapping). Update logic in set_item_tax_template
to: if source_doc.doctype == "Subcontracting Order" (or hasattr(source_doc,
"supplied_items")) iterate supplied_items to build item_tax_template_map,
otherwise fall back to using source_doc.items (or another appropriate field for
Purchase Receipt/Stock Entry) when resolving item.item_tax_template via
rm_detail_field so the function safely handles all valid source doctypes.
---
Duplicate comments:
In `@india_compliance/gst_india/overrides/subcontracting_transaction.py`:
- Around line 117-122: In the branch that checks source_doc.doctype == "Stock
Entry" (inside the function handling subcontracting transactions when
doc.purpose == "Material Transfer" and not doc.is_return), replace the returned
field-name tuples so they reference Stock Entry field names: return from_fields
as ("bill_from_address", "bill_from_gstin") and to_fields as ("bill_to_address",
"bill_to_gstin") instead of the current ("billing_address","company_gstin") /
("supplier_address","supplier_gstin"); update the return statement that
currently returns from_fields, to_fields accordingly so subsequent
source_doc.get(...) calls retrieve the correct address/GSTIN values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 02b43948-4f6a-488c-b9da-5227ea9318e9
📒 Files selected for processing (1)
india_compliance/gst_india/overrides/subcontracting_transaction.py
Confidence Score: 2/5
|
| Filename | Overview |
|---|---|
| india_compliance/gst_india/overrides/subcontracting_transaction.py | Adds _get_fields_mapping and set_item_tax_template helpers and reworks after_mapping_stock_entry to correctly handle address/GSTIN propagation for multiple source doctypes. Contains a destructive null-assignment bug in the second loop of set_item_tax_template and a behavioral regression for the SCO → Material Transfer (non-return) case. |
Reviews (3): Last reviewed commit: "fix: correct field mapping for addresses..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
india_compliance/gst_india/overrides/subcontracting_transaction.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
♻️ Duplicate comments (2)
india_compliance/gst_india/overrides/subcontracting_transaction.py (2)
99-123:⚠️ Potential issue | 🟠 MajorUse Stock Entry field names in the Stock Entry branch.
When
source_doc.doctype == "Stock Entry", this still returns the genericbilling_address/company_gstin→supplier_address/supplier_gstinpair. Stock Entry usesbill_from_*/bill_to_*in this file, so Lines 90-93 will readNoneand wipe the mapped address/GSTIN values on Stock Entry-to-Stock Entry transfers.💡 Suggested fix
def _get_fields_mapping(doc, source_doc): from_fields = ("billing_address", "company_gstin") to_fields = ("supplier_address", "supplier_gstin") + stock_entry_from_fields = ("bill_from_address", "bill_from_gstin") + stock_entry_to_fields = ("bill_to_address", "bill_to_gstin") @@ elif ( source_doc.doctype == "Stock Entry" and doc.purpose == "Material Transfer" and not doc.is_return ): - return from_fields, to_fields + return stock_entry_from_fields, stock_entry_to_fields
127-153:⚠️ Potential issue | 🟠 Major
set_item_tax_template()skips two source doctypes this hook already supports.
_get_fields_mapping()explicitly handlesPurchase ReceiptandStock Entry, but Lines 128-129 return before any item-tax mapping for those flows. That means address/GSTIN mapping succeeds whileitem.item_tax_templatestays unset on the same mapped Stock Entry, which can skew later GST tax mapping. The remainingPurchase Orderbranch also no longer matches the source doctypes accepted above.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 35f02f80-7524-4c77-a664-3dd17ac66049
📒 Files selected for processing (1)
india_compliance/gst_india/overrides/subcontracting_transaction.py
india_compliance/gst_india/overrides/subcontracting_transaction.py
Outdated
Show resolved
Hide resolved
|
@Mergifyio rebase |
✅ Branch has been successfully rebased |
ljain112
force-pushed
the
fix/after-mapping-stock-entry
branch
from
d722b57 to
0d1a439
Compare
| for item in doc.items: | ||
| item.item_tax_template = item_tax_template_map.get(item.get(rm_detail_field)) |
There was a problem hiding this comment.
item_tax_template_map is only populated for supplied items whose corresponding source order item actually has an item_tax_template set (line 153 guards with if item_tax_template:). For every doc.items row whose sco_rm_detail either doesn't match any key in the map or whose source item had no template, the assignment unconditionally writes None, destroying whatever template the mapping process may have already placed on the item.
Consider only overwriting when a real value is found:
for item in doc.items:
template = item_tax_template_map.get(item.get(rm_detail_field))
if template:
item.item_tax_template = templateThis preserves any template that was already correctly set on items that lack a lookup match.
|
Add test cases for both issues. |
|
@karm1000 rebase |
|
@Mergifyio rebase |
✅ Branch has been successfully rebased |
karm1000
force-pushed
the
fix/after-mapping-stock-entry
branch
from
fbc71fe to
0cd0f19
Compare
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 25 |
TIP This summary will be updated as you push new changes. Give us feedback
2 participants
depends on: frappe/erpnext#52724