fix(stock): prevent serial number reuse in manufacturing after delivery #50317
Conversation
Delivered serial numbers could be reused in manufacturing, overwriting the original item data. Now blocked for non-return transactions while allowing legitimate returns.
elliotnor
requested review from
mihir-kandoi and
rohitwaghchaure
as code owners
📝 WalkthroughWalkthroughThis PR modifies the serial and batch bundle module to replace separate posting_date and posting_time fields with a unified posting_datetime field, and introduces return transaction detection to prevent duplicate serial number reuse for non-return transactions while allowing reuse in return scenarios. It expands method signatures to pass prev_sle through valuation flows, updates validation logic in validate_serial_nos_duplicate, modifies get_serial_nos_for_validate to return batch information alongside serial numbers, introduces helper functions for reserved serial management, adds stock-queue handling for FIFO valuation, and performs numerous query and ledger-fetching updates to use posting_datetime. Test cases are added to verify delivered serial number reuse blocking and return transaction handling. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (3 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://cdn.statically.io/img/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/serial_and_batch_bundle/serial_and_batch_bundle.py (1)
274-331: Theis_return_transaction()method does not check Stock Entry'sis_returnflag, breaking manufacturing return scenarios.The implementation prevents reusing delivered serial numbers for non-return inward transactions (lines 290-307), which is correct. However,
is_return_transaction()(lines 332-360) only checksis_returnflag for these voucher types: Delivery Note, Sales Invoice, Purchase Invoice, Purchase Receipt, POS Invoice, and Subcontracting Receipt.Stock Entry is missing from this list. Since manufacturing entries can legitimately have
is_return=1(as seen in work_order.py line 2373), manufacturing return entries will fail the delivered serial number check even though they should allow reuse. Manufacturing finished goods are "Inward" transactions, so they'll hit the validation, butis_return_transaction()returnsFalse, triggering a false error.Add "Stock Entry" to the checked voucher types in
is_return_transaction()at line 345:if self.voucher_type in [ "Delivery Note", "Sales Invoice", "Purchase Invoice", "Purchase Receipt", "POS Invoice", "Stock Entry", # Add this line "Subcontracting Receipt", ]:
🧹 Nitpick comments (2)
erpnext/stock/doctype/serial_and_batch_bundle/test_serial_and_batch_bundle.py (2)
909-952: Consider simulating actual delivery flow for more comprehensive testing.The test directly sets the serial number status to "Delivered" (line 926) without simulating a full delivery transaction. While this tests the validation logic, it doesn't verify that the serial status is correctly updated through the actual delivery flow. Consider creating a delivery note similar to
test_return_transaction_allows_delivered_serialto make the test more representative of real-world usage.Example approach:
def test_delivered_serial_no_reuse_blocked(self): from erpnext.stock.doctype.serial_and_batch_bundle.serial_and_batch_bundle import ( SerialNoDuplicateError, ) + from erpnext.stock.doctype.delivery_note.test_delivery_note import create_delivery_note item_code = make_item( "Test Delivered Serial Reuse", properties={"has_serial_no": 1, "serial_no_series": "TEST-DSR-.####", "is_stock_item": 1}, ).name serial_no = "TEST-DSR-0001" if not frappe.db.exists("Serial No", serial_no): sn_doc = frappe.get_doc( { "doctype": "Serial No", "serial_no": serial_no, "item_code": item_code, - "status": "Delivered", } ) sn_doc.insert(ignore_permissions=True) - else: - frappe.db.set_value("Serial No", serial_no, "status", "Delivered") + + # First receive the serial number + make_stock_entry( + item_code=item_code, + target="_Test Warehouse - _TC", + qty=1, + rate=100, + serial_no=[serial_no], + ) + + # Deliver it to make status "Delivered" + create_delivery_note( + item_code=item_code, qty=1, rate=100, warehouse="_Test Warehouse - _TC", serial_no=[serial_no] + ) with self.assertRaises(SerialNoDuplicateError) as context: make_serial_batch_bundle( { "item_code": item_code, "warehouse": "_Test Warehouse - _TC", "voucher_type": "Stock Entry", "posting_date": today(), "posting_time": nowtime(), "qty": 1, "rate": 100, "serial_nos": [serial_no], "type_of_transaction": "Inward", "do_not_submit": True, } ) self.assertIn("already delivered", str(context.exception).lower()) frappe.db.rollback()
953-999: Enhance test with additional assertions for return transaction verification.The test correctly creates a return flow but only asserts that
return_dn.nameexists (line 996). Consider adding more comprehensive assertions to verify:
- The serial number status after the return (should it remain "Delivered" or change?)
- The warehouse quantity is updated correctly
- The bundle was created successfully for the return transaction
return_dn = create_delivery_note( item_code=item_code, qty=-1, rate=100, warehouse="_Test Warehouse - _TC", serial_no=[serial_no], is_return=1, return_against=dn.name, ) self.assertTrue(return_dn.name) + + # Verify the return transaction was successful + self.assertEqual(return_dn.docstatus, 1, "Return delivery note should be submitted") + + # Verify serial number warehouse is updated back + sn_warehouse = frappe.db.get_value("Serial No", serial_no, "warehouse") + self.assertEqual(sn_warehouse, "_Test Warehouse - _TC", "Serial should be back in warehouse after return") frappe.db.rollback()
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
erpnext/stock/doctype/serial_and_batch_bundle/serial_and_batch_bundle.py(32 hunks)erpnext/stock/doctype/serial_and_batch_bundle/test_serial_and_batch_bundle.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
erpnext/stock/doctype/serial_and_batch_bundle/test_serial_and_batch_bundle.py (3)
erpnext/stock/doctype/serial_and_batch_bundle/serial_and_batch_bundle.py (1)
SerialNoDuplicateError(44-45)erpnext/stock/doctype/item/test_item.py (1)
make_item(35-74)erpnext/stock/doctype/delivery_note/test_delivery_note.py (1)
create_delivery_note(2667-2739)
erpnext/stock/doctype/serial_and_batch_bundle/serial_and_batch_bundle.py (3)
erpnext/stock/doctype/stock_ledger_entry/stock_ledger_entry.py (1)
on_doctype_update(348-351)erpnext/stock/serial_batch_bundle.py (3)
get(958-959)get_serial_nos_from_bundle(552-553)get_batches_from_bundle(532-549)erpnext/stock/utils.py (1)
get_combine_datetime(680-692)
🪛 Ruff (0.14.2)
erpnext/stock/doctype/serial_and_batch_bundle/serial_and_batch_bundle.py
231-231: Unpacked variable batches is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
356-357: try-except-pass detected, consider logging the exception
(S110)
356-356: Do not catch blind exception: Exception
(BLE001)
2257-2257: Unused function argument: kwargs
(ARG001)
⏰ 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
|
|
||
| if self.voucher_type == "Stock Reconciliation": | ||
| serial_nos = self.get_serial_nos_for_validate() | ||
| serial_nos, batches = self.get_serial_nos_for_validate() |
There was a problem hiding this comment.
Fix unused variable from tuple unpacking.
The batches variable is unpacked but never used in validate_serial_nos_inventory. Use _ to indicate it's intentionally unused.
Apply this diff:
def validate_serial_nos_inventory(self):
if not (self.has_serial_no and self.type_of_transaction == "Outward"):
return
if self.voucher_type == "Stock Reconciliation":
- serial_nos, batches = self.get_serial_nos_for_validate()
+ serial_nos, _ = self.get_serial_nos_for_validate()
else:
serial_nos = [d.serial_no for d in self.entries if d.serial_no]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| serial_nos, batches = self.get_serial_nos_for_validate() | |
| serial_nos, _ = self.get_serial_nos_for_validate() |
🧰 Tools
🪛 Ruff (0.14.2)
231-231: Unpacked variable batches is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
🤖 Prompt for AI Agents
In erpnext/stock/doctype/serial_and_batch_bundle/serial_and_batch_bundle.py
around line 231, the tuple unpacking assigns serial_nos and batches but batches
is unused; replace the second variable with an underscore (e.g., serial_nos, _ =
self.get_serial_nos_for_validate()) to indicate the value is intentionally
ignored and remove the unused variable warning.
| def is_return_transaction(self): | ||
| """ | ||
| Check if this is a return transaction. | ||
| Return transactions legitimately reuse serial numbers. | ||
| """ | ||
| # Check if returned_against field is set | ||
| if self.returned_against: | ||
| return True | ||
|
|
||
| # Check if parent voucher is a return | ||
| if self.voucher_type in [ | ||
| "Delivery Note", | ||
| "Sales Invoice", | ||
| "Purchase Invoice", | ||
| "Purchase Receipt", | ||
| "POS Invoice", | ||
| "Subcontracting Receipt", | ||
| ]: | ||
| try: | ||
| voucher_details = frappe.db.get_value( | ||
| self.voucher_type, self.voucher_no, ["is_return", "return_against"], as_dict=True | ||
| ) | ||
| if voucher_details and voucher_details.get("is_return"): | ||
| return True | ||
| except Exception: | ||
| pass | ||
|
|
||
| return False | ||
|
|
There was a problem hiding this comment.
Replace broad exception handling with specific error handling or logging.
The try-except block catches all exceptions and silently passes (lines 350-357), which can hide legitimate errors like database connection issues or permission problems. This makes debugging difficult and violates best practices.
Since frappe.db.get_value returns None when the record doesn't exist, the try-except may not be necessary. If you need to handle specific errors, catch only those exceptions and log them:
def is_return_transaction(self):
"""
Check if this is a return transaction.
Return transactions legitimately reuse serial numbers.
"""
# Check if returned_against field is set
if self.returned_against:
return True
# Check if parent voucher is a return
if self.voucher_type in [
"Delivery Note",
"Sales Invoice",
"Purchase Invoice",
"Purchase Receipt",
"POS Invoice",
"Subcontracting Receipt",
]:
- try:
- voucher_details = frappe.db.get_value(
- self.voucher_type, self.voucher_no, ["is_return", "return_against"], as_dict=True
- )
- if voucher_details and voucher_details.get("is_return"):
- return True
- except Exception:
- pass
+ if not self.voucher_no:
+ return False
+
+ voucher_details = frappe.db.get_value(
+ self.voucher_type, self.voucher_no, ["is_return", "return_against"], as_dict=True
+ )
+ if voucher_details and voucher_details.get("is_return"):
+ return True
return False🧰 Tools
🪛 Ruff (0.14.2)
356-357: try-except-pass detected, consider logging the exception
(S110)
356-356: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
In erpnext/stock/doctype/serial_and_batch_bundle/serial_and_batch_bundle.py
around lines 332-360, the broad try-except around frappe.db.get_value silently
swallows all errors; remove the unnecessary try-except (since get_value returns
None for missing records) so exceptions can propagate, or if you really need to
handle DB-specific failures catch only the specific exceptions (e.g., database
operational/connection errors or framework-specific exceptions) and log them
with context using frappe.logger or process logger instead of passing silently.
2 participants
Closes #49520
Description
This PR fixes a bug where serial numbers that have already been delivered can be reused in manufacturing (Stock Entry with "Manufacture" purpose). This causes data loss as the original serial number information gets overwritten, and the delivered item loses its serial number assignment.
Problem
When creating a Stock Entry for manufacturing with the "Manufacture" purpose, we were not validating whether serial numbers in the incoming bundle had already been delivered.
Solution
Added validation in
validate_serial_nos_duplicate()method to:returned_againstfield on the bundleis_returnflag on the parent voucherSerialNoDuplicateErrorif a delivered serial number is being reused in a non-return transactionChanges Made
Files Modified:
erpnext/stock/doctype/serial_and_batch_bundle/serial_and_batch_bundle.pyvalidate_serial_nos_duplicate()to check for delivered serial numbersis_return_transaction()method to properly identify return transactionsTests Added:
test_delivered_serial_no_reuse_blocked()- Verifies that delivered serial numbers cannot be reusedtest_return_transaction_allows_delivered_serial()- Verifies that return transactions can legitimately use delivered serial numbersScreencast
Before:
screencast_before.webm
After:
screencast_after.webm