fix: resolve virtual fields using property instead of handling them manually using onload

[ljain112]

Issue: virtual fields needed to be handled manually; now we are resolving them by adding a property in the extended class.

Notes:

• Fields are not in __dict__ so we cant do .get so we shouldn't use virtual fields in logic.
• In Print, it also does .get, so we have not removed the before_print hook for now.
• From the frontend, print works, but in the backend, it will fail.
• This means virtual fields are only for the frontend.

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics -3 complexity

Metric	Results
Complexity	-3

View in Codacy

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.}

[coderabbitai]

📝 Walkthrough

Walkthrough

Changes

This PR refactors several document-mutating setter functions into pure getter functions exposed via new Frappe virtual field extension classes (virtual_fields.py), covering GST breakup tables, e-commerce supply type, address display fields, and purchase ineligibility reasons. Corresponding onload hooks calling transaction.onload were removed from hooks.py for several doctypes, and a new CLASS_EXTENSION_MAP wires extension classes into extend_doctype_class. Client-side JavaScript functions that previously computed these fields on load were deleted. Tests were updated and expanded to verify the new virtual field behavior.

Related PRs: None specified in provided input.

Suggested labels: refactor, needs-review

Suggested reviewers: None specified in provided input.

Poem:
A rabbit hopped through fields of code,
Turned setters into getters' mode,
No more onload, hooks trimmed clean,
Virtual fields now bridge the scene,
Breakup, address, ITC in tow—
Hop, hop, refactor, off we go!

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title accurately summarizes the main change: resolving virtual fields via properties instead of manual onload handling.
Description check	✅ Passed	The description is clearly related to the changeset and explains the virtual-field property approach and print caveat.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

🧹 Nitpick comments (1)

india_compliance/gst_india/overrides/ineligible_itc.py (1)

76-98: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Recompute is_eligibility_restricted_due_to_pos() once, not per item.

is_eligibility_restricted_due_to_pos (Line 315) now calls get_ineligibility_reason(self.doc), which itself iterates over doc.items. Since Line 88 calls self.is_eligibility_restricted_due_to_pos() inside the for item in self.doc.items loop, the overall complexity becomes O(n²) versus the prior O(1) self.doc.get("ineligibility_reason") lookup. For invoices/receipts with many line items this adds unnecessary repeated work.

⚡ Proposed fix: hoist the check outside the loop

     def update_item_ineligibility(self):
         self.doc._has_ineligible_itc_items = False
         stock_items = self.doc.get_stock_items()
 
         self.tax_account_dict = {
             row.gst_tax_type: row.account_head for row in self.doc.taxes if row.gst_tax_type
         }
 
         if not self.tax_account_dict:
             return
 
+        is_pos_restricted = self.is_eligibility_restricted_due_to_pos()
+
         for item in self.doc.items:
-            if not self.is_eligibility_restricted_due_to_pos() and not item.is_ineligible_for_itc:
+            if not is_pos_restricted and not item.is_ineligible_for_itc:
                 continue

Also applies to: 314-315

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 2e247519-15c9-4fe1-bf88-bce9e1e5171b

📥 Commits

Reviewing files that changed from the base of the PR and between a8439f8 and 71324c6.

📒 Files selected for processing (10)

• india_compliance/gst_india/overrides/ineligible_itc.py
• india_compliance/gst_india/overrides/purchase_invoice.py
• india_compliance/gst_india/overrides/purchase_receipt.py
• india_compliance/gst_india/overrides/subcontracting_transaction.py
• india_compliance/gst_india/overrides/test_subcontracting_transaction.py
• india_compliance/gst_india/overrides/test_transaction.py
• india_compliance/gst_india/overrides/transaction.py
• india_compliance/gst_india/overrides/virtual_fields.py
• india_compliance/hooks.py
• india_compliance/public/js/transaction.js

💤 Files with no reviewable changes (1)

• india_compliance/public/js/transaction.js

[greptile-apps]

Confidence Score: 5/5

Safe to merge — the change is a clean architectural refactor with well-targeted guards and no regressions in the tested paths.

Every code path that previously relied on onload to materialise virtual fields is now covered by the property mechanism, the print renderer workaround is explicit and documented, and the new tests verify both direct property access and the as_dict() round-trip. No functional regressions were identified.

No files require special attention. The before_print hook in transaction.py intentionally duplicates the property logic for Frappe's print renderer and should stay until the upstream Frappe fix lands.

Important Files Changed

Filename	Overview
india_compliance/gst_india/overrides/virtual_fields.py	New file introducing four mixin classes (GSTBreakupExt, EcommerceSupplyTypeExt, AddressDisplayExt, IneligibilityReasonExt) that back virtual fields via @Property; clean, no issues.
india_compliance/hooks.py	Removes transaction.onload from all doctype hooks and wires up extend_doctype_class using a module-level loop over CLASS_EXTENSION_MAP; format matches Frappe's hook convention.
india_compliance/gst_india/overrides/transaction.py	Converts set_gst_breakup/set_ecommerce_supply_type to pure getters; before_print now materialises both virtual fields via doc.set() for the print renderer; onload removed.
india_compliance/gst_india/overrides/purchase_invoice.py	Extracts get_ineligibility_reason (pure getter) and show_ineligibility_alert; adds null-guards on place_of_supply/company_gstin slicing; set_ineligibility_reason preserved for stored-field Purchase Invoice path.
india_compliance/gst_india/overrides/purchase_receipt.py	Removes set_ineligibility_reason from onload (now virtual) and validate; validate shows the alert via show_ineligibility_alert without writing to the doc, which is correct for a virtual field.
india_compliance/gst_india/overrides/ineligible_itc.py	Removes PurchaseReceipt.init that called run_method("onload"); is_eligibility_restricted_due_to_pos now calls get_ineligibility_reason directly instead of doc.get(), avoiding the property-bypass problem.
india_compliance/gst_india/overrides/subcontracting_transaction.py	Removes set_address_display and all call sites; address display fields are now backed by AddressDisplayExt properties; onload retains e-Waybill GSTIN mapping.
india_compliance/gst_india/overrides/test_transaction.py	Adds three new tests covering gst_breakup_table virtual field, print path materialisation, and ecommerce_supply_type virtual field; tests verify both direct property access and as_dict() round-trip.
india_compliance/gst_india/overrides/test_subcontracting_transaction.py	Updates test to verify AddressDisplayExt properties and as_dict() round-trip instead of relying on run_method("onload"); retains onload call for e-Waybill mapping.
india_compliance/public/js/transaction.js	Removes client-side refresh helpers (set_gst_tax_breakup_on_load, set_e_commerce_ecommerce_supply_type) that are superseded by server-side properties; intentional UX trade-off (fields update on save, not in real-time).

_{Reviews (2): Last reviewed commit: "fix: resolve virtual fields using proper..." | Re-trigger Greptile}

[ljain112]

@greptileai review

[ljain112]

#4538