refactor: stock_entry file to improve readability and maintainability#54466
refactor: stock_entry file to improve readability and maintainability#54466rohitwaghchaure wants to merge 5 commits intofrappe:developfrom
Conversation
github-actions
Bot
added
skip-release-notes
📝 WalkthroughWalkthroughThis pull request refactors manufacturing and disassembly-related logic in Stock Entry by extracting it into a new dedicated handler module. A new file Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Key observationsThe refactoring consolidates ~1,712 lines of new code into 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
erpnext/stock/doctype/stock_entry/stock_entry.py (1)
296-338:⚠️ Potential issue | 🔴 Critical
validate_work_order()call on line 249 will raiseAttributeErroron every Stock Entry save.
validate()callsself.validate_work_order()(line 249), but this method is no longer defined inStockEntryor its parentStockController; it was moved toManufactureHandlerinmanufacturing_handler.py. The instance method resolution will fail becauseStockEntrydoes not inherit fromManufactureHandler. Movevalidate_work_order()back to theStockEntryclass, or route the call through the appropriate handler based on the entry's purpose.Additionally,
get_completed_job_card_qty()(line 2286) and the import ofget_batch_nos(line 59) appear to have no callers and can be removed as cleanup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@erpnext/stock/doctype/stock_entry/stock_entry.py` around lines 296 - 338, The call to self.validate_work_order() from StockEntry.validate is failing because validate_work_order() was moved out of StockEntry/StockController into ManufactureHandler; restore behavior by either reintroducing a validate_work_order(self) instance method on the StockEntry class that delegates to ManufactureHandler (e.g., ManufactureHandler(self).validate_work_order()) when purpose indicates manufacturing/repack disassemble, or change the call site in validate() to route through ManufactureHandler based on self.purpose; also remove the dead helpers get_completed_job_card_qty() and the unused import get_batch_nos to clean up unused code.
🤖 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/doctype/stock_entry/manufacturing_handler.py`:
- Around line 346-362: The backflush_based_on property currently treats falsy
cached values as "not cached" because it checks `if not getattr(self,
"_backflush_based_on", None)`, causing repeated calls to get_backflush_based_on;
change the caching to use a sentinel (e.g., define a module/class-level _MISSING
= object()) and initialize `_backflush_based_on = _MISSING`, then in
backflush_based_on check `if self._backflush_based_on is _MISSING` and set it
from get_backflush_based_on(self.se_doc.bom_no); adjust reset to restore
`_backflush_based_on = _MISSING` so legitimate falsy returns (None/''/0) are
preserved in the cache.
In `@erpnext/stock/doctype/stock_entry/stock_entry.py`:
- Around line 2020-2037: The fallback branch that instantiates
ManufactureHandler(self) when PURPOSE_HANDLER_MAP.get(self.purpose) is falsy
currently skips handler.validate(), which bypasses
validate_posting_date_and_time(); update the logic so that any instantiated
handler (both from handler_class and the fallback ManufactureHandler created in
the elif self.bom_no: branch) has handler.validate() called before
handler.set_items() to ensure posting date/time validation runs, and as a minor
refactor move PURPOSE_HANDLER_MAP out of the get_items()/local scope into
module/class scope to avoid reconstructing it on each call.
---
Outside diff comments:
In `@erpnext/stock/doctype/stock_entry/stock_entry.py`:
- Around line 296-338: The call to self.validate_work_order() from
StockEntry.validate is failing because validate_work_order() was moved out of
StockEntry/StockController into ManufactureHandler; restore behavior by either
reintroducing a validate_work_order(self) instance method on the StockEntry
class that delegates to ManufactureHandler (e.g.,
ManufactureHandler(self).validate_work_order()) when purpose indicates
manufacturing/repack disassemble, or change the call site in validate() to route
through ManufactureHandler based on self.purpose; also remove the dead helpers
get_completed_job_card_qty() and the unused import get_batch_nos to clean up
unused code.
🪄 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: a937262b-c173-4581-bad1-81ecc4e590fb
📒 Files selected for processing (9)
erpnext/manufacturing/doctype/bom/bom.jsonerpnext/manufacturing/doctype/bom/bom.pyerpnext/manufacturing/doctype/job_card/job_card.pyerpnext/manufacturing/doctype/work_order/work_order.pyerpnext/stock/doctype/purchase_receipt/purchase_receipt.jserpnext/stock/doctype/stock_entry/manufacturing_handler.pyerpnext/stock/doctype/stock_entry/stock_entry.jserpnext/stock/doctype/stock_entry/stock_entry.jsonerpnext/stock/doctype/stock_entry/stock_entry.py
💤 Files with no reviewable changes (1)
- erpnext/manufacturing/doctype/work_order/work_order.py
rohitwaghchaure
force-pushed
the
revamp-stock-entry
branch
11 times, most recently
from
d4d3f28 to
b9a045c
Compare
rohitwaghchaure
force-pushed
the
revamp-stock-entry
branch
from
b9a045c to
8a3f55e
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #54466 +/- ##
===========================================
+ Coverage 79.28% 79.45% +0.17%
===========================================
Files 1158 1161 +3
Lines 125604 126266 +662
===========================================
+ Hits 99580 100321 +741
+ Misses 26024 25945 -79
🚀 New features to boost your workflow:
|
rohitwaghchaure
force-pushed
the
revamp-stock-entry
branch
4 times, most recently
from
c326ab0 to
f196e06
Compare
rohitwaghchaure
force-pushed
the
revamp-stock-entry
branch
from
f196e06 to
cd36da7
Compare
rohitwaghchaure
force-pushed
the
revamp-stock-entry
branch
from
a6fb2fb to
c0c4f22
Compare
rohitwaghchaure
force-pushed
the
revamp-stock-entry
branch
from
953c446 to
f2c3848
Compare
rohitwaghchaure
force-pushed
the
revamp-stock-entry
branch
from
5df0ddb to
adbe9bc
Compare
| self._backflush_based_on = None | ||
|
|
||
|
|
||
| class ManufactureHandler(BaseManufacturingHandler): |
There was a problem hiding this comment.
Should this not be ManufacturingHandler ?
|
|
||
| self.add_to_stock_entry_detail(item_dict) | ||
|
|
||
| def get_subcontract_order_supplied_items(self): |
There was a problem hiding this comment.
move to a separate subcontracting_handler.py ?
| order_by="creation", | ||
| ) | ||
|
|
||
| def update_batches_to_be_consume(self, batches, row, qty): |
There was a problem hiding this comment.
batching_handler.py?
maybe just drop the handler suffix, since they are all in the stock_entry folder?
2 participants
The stock_entry.py file has grown to over 4,000 lines of code, making it difficult to read, understand, and maintain.
Redundant code