-
Notifications
You must be signed in to change notification settings - Fork 10.1k
fix: skip item selection call while item quick entry is open #51406
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
6a5756a to
506a8de
Compare
📝 WalkthroughWalkthroughThe change adds an early return in the Item handling flow in erpnext/public/js/controllers/transaction.js to skip the server-side process_item_selection call when an item is entered via Item Quick Entry and has not yet been saved. The early return triggers when from_link.field_obj.df.fieldname equals "item_code" and from_link.field_obj.get_options() returns "Item", preventing the server request in that specific context. Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (1)📓 Common learnings⏰ 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)
🔇 Additional comments (1)
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
erpnext/public/js/controllers/transaction.js
🧰 Additional context used
🧬 Code graph analysis (1)
erpnext/public/js/controllers/transaction.js (1)
erpnext/accounts/doctype/payment_entry/payment_entry.js (1)
frappe(1709-1709)
⏰ 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
🔇 Additional comments (2)
erpnext/public/js/controllers/transaction.js (2)
619-627: Clarify the "two locations" mentioned in the AI summary.The AI-generated summary states: "This change appears in two locations of the Item handling flow, effectively preventing the server request in both contexts." However, only one early return is visible in the provided code (lines 619-627).
Could you clarify:
- Is there another location in this file or another file where a similar check should be added?
- Or does "two locations" refer to something else in the implementation?
This will help ensure complete coverage of the fix.
619-627: Verify cleanup and timing guarantees forfrappe._from_linkwith the Frappe framework team.The
frappe._from_linkcheck is intentionally defensive with scoped field validation, but relies on framework-level cleanup that cannot be verified from the erpnext codebase alone. The code is marked "Experimental" and designed to work withfrappe.run_serially()sequencing (whereitem_code()triggers again after Quick Entry saves), suggesting cleanup is expected between calls. However, confirm with Frappe maintainers:
- Whether
frappe._from_linkis guaranteed to be cleared after Quick Entry completes- If this pattern is officially recommended or if a more robust API exists
- Whether the timing guarantees hold under concurrent saves or network delays
Consider adding a safeguard to explicitly clear or validate the state before the second
item_code()trigger executes.
506a8de to
17ccee7
Compare
|
Fixed #51607 |
Resovles Frappe Issue #35455
Skip process_item_selection while Item Quick Entry is open from a link to avoid “Item not found”.