Skip to content
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

feat: Share Serial Nos between unique Items #37209

Closed
wants to merge 23 commits into from

Conversation

marination
Copy link
Collaborator

@marination marination commented Sep 22, 2023

no-docs (temporary)

Case:

  • Serial Nos are purchased from Supplier and must be maintained in the system as per supplier
  • A company buys different items from a supplier, who (supplier) starts all their serialised items with 1 and then autoincrements. This way item A can have serial “3” and item B can also have serial no “3”.
  • Additionally: A serial no is also something that a supplier/ manufacturer can choose freely.

Solution:

  • Make Serial Nos unique only on Item level (Item A cannot have Serial No "3" twice)
  • This means Item A and B can have the same Serial No, but different Serial No document names
    Screenshot 2023-09-29 at 4 50 18 PM

Flow:

  • Serialised Item A receives some Serial Nos via a Purchase Receipt. These are auto created using a naming series. SN-0001 is one of them
  • We want to register a Serial No for Serialised Item B, which also has the same Serial No
  • We can do one of the following:
    • Create a new Serial No (manual or data import) > Use it in the new Purchase Receipt's popup, by scanning or entering it in the table
    • In the new Purchase Receipt popup, upload a CSV of the Serial No(s).
    • Create a new Serial and Batch Bundle > click on "Make Serial Nos" in the form > insert/upload the Serial No(s)
  • Now we should have a Serial and Batch Bundle created that includes a Serial No with ID: SN-0001-1 and Serial No: SN-0001

All of the above methods handle naming clashes via the autoname function in the Serial No.

Note: This does not work for naming series (auto naming) as the last count is maintained series wise. So if we have a series SN-.#### shared by two items, the insertion of the Item-A creates SN-0001 (series count is 1) and then the following insertion of Item-B will naturally create SN-0002.

For now only manual addition of serial no.s through various modes is supported for reusability

ToDo:

  • Link formatting for Serial No in tables and reports
  • Tests

@github-actions github-actions bot added accounts needs-tests This PR needs automated unit-tests. stock labels Sep 22, 2023
@marination marination removed needs-tests This PR needs automated unit-tests. accounts labels Oct 10, 2023
@marination marination marked this pull request as ready for review October 10, 2023 15:19
@marination marination force-pushed the shared-serial-nos branch 3 times, most recently from 2148383 to a3af740 Compare October 31, 2023 16:35
Copy link

codecov bot commented Oct 31, 2023

Codecov Report

Attention: Patch coverage is 70.32967% with 27 lines in your changes are missing coverage. Please review.

Project coverage is 60.13%. Comparing base (d0df5df) to head (f8a8f5b).
Report is 767 commits behind head on develop.

❗ Current head f8a8f5b differs from pull request most recent head 162732d. Consider uploading reports for the commit 162732d to get more accurate results

Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #37209      +/-   ##
===========================================
- Coverage    60.24%   60.13%   -0.12%     
===========================================
  Files          758      757       -1     
  Lines        71199    70533     -666     
===========================================
- Hits         42893    42414     -479     
+ Misses       28306    28119     -187     
Files Coverage Δ
...xt/accounts/doctype/sales_invoice/sales_invoice.py 74.39% <100.00%> (+0.07%) ⬆️
...ctype/maintenance_schedule/maintenance_schedule.py 71.98% <100.00%> (ø)
erpnext/stock/report/stock_ageing/stock_ageing.py 91.07% <100.00%> (ø)
erpnext/stock/stock_ledger.py 83.50% <100.00%> (+2.99%) ⬆️
...rpnext/accounts/doctype/pos_invoice/pos_invoice.py 49.69% <0.00%> (+0.31%) ⬆️
erpnext/controllers/accounts_controller.py 85.33% <0.00%> (-0.23%) ⬇️
erpnext/controllers/sales_and_purchase_return.py 91.18% <50.00%> (ø)
erpnext/controllers/stock_controller.py 81.73% <50.00%> (-1.75%) ⬇️
erpnext/controllers/subcontracting_controller.py 89.38% <0.00%> (-0.06%) ⬇️
...ext/manufacturing/doctype/work_order/work_order.py 75.46% <75.00%> (+0.09%) ⬆️
... and 9 more

... and 75 files with indirect coverage changes

@marination
Copy link
Collaborator Author

@rohitwaghchaure Whenever you can please take a look. Would love to get your review :)

@marination
Copy link
Collaborator Author

marination commented Nov 6, 2023

TODO: Add more tests for stock transfer, stock reservation, delivery note

@marination marination added the needs-tests This PR needs automated unit-tests. label Nov 6, 2023
Copy link

stale bot commented Nov 22, 2023

This pull request has been automatically marked as inactive because it has not had recent activity. It will be closed within 3 days if no further activity occurs, but it only takes a comment to keep a contribution alive :) Also, even if it is closed, you can always reopen the PR when you're ready. Thank you for contributing.

@stale stale bot added the inactive label Nov 22, 2023
@bosue
Copy link
Contributor

bosue commented Nov 24, 2023

True, the Serial Number can’t be unique across all items.
However, if it isn’t unique, so we need a separate unique ID, then I think it’s less confusing if that unique ID is a random hash.
We certainly don‘t want users to confuse the two. Also, unlike with, say, amended submittable documents, there may be nothing two accidentally identical S/Ns that are referring to different items have in common.

Still we may want an additional provision allowing to enforce unique serial numbers over a particular Item Group, as for some items out in the world, general standards for S/N exist. But that‘s matter for a separate ticket.

@barredterra
Copy link
Collaborator

@bosue this PR is supposed to get backported all the way to v14, so we can't just change the naming scheme. This is a completely optional approach, for users who are aware of the implications. Regarding future versions I agree with you, but that will have to be a separate PR.

Copy link
Contributor

@bosue bosue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just tidbits.

If we already know that at a later point – preferably soon – we want to allow Uniqueness per Item Group as a third option, we might want to use a select rightaway so we're flexible. But it‘s perfectly fine to keep it simple now.

erpnext/stock/doctype/stock_settings/stock_settings.json Outdated Show resolved Hide resolved
erpnext/stock/doctype/stock_settings/stock_settings.json Outdated Show resolved Hide resolved
@marination
Copy link
Collaborator Author

marination commented Nov 27, 2023

@barredterra To Test:

  • Enable the setting
  • Create two serialised items. Let the first item be autonamed and create serial nos for it (Purchase Receipt) (eg. SN-001, SN-002, etc.)

To insert Serial Nos for the second item, these are multiple ways to achieve the same goal :

  • Use the "Download Template" > "Attach CSV" feature. Make sure the CSV has SN-002 (same as the first item) and any other SN. The expectation is that the the new SN should be created under the name SN-002-1
    Screenshot 2023-11-27 at 5 58 48 PM
  • Manually create a Serial No from the serial no. list view and select it in the popup above. A duplicate Serial No of another item should work just fine
  • Use Data Import to insert a duplicate Serial No record
  • In "Serial and Batch Bundle", fill in the basic details, click on "Make Serial Nos", try inserting a duplicate using the text field and CSV.

To move the serial no:

  • In a Delivery Note, use the Scan Serial No field in the Serial No popup. The expectation is that if SN-002 is scanned for the second item, the table in the popup should be populated with SN-002-1
  • The same can be checked for a stock entry where the Serial No is transferred from one warehouse to another
  • The same check can be done for a Serial and Batch Item, along with the right Serial No ID, the right batch no. will be populated in the popup table too

Stock Reservation:

  • Enable Stock Reservation in Stock Settings https://docs.erpnext.com/docs/user/manual/en/stock-reservation and also enable auto reserve serial and batch nos
  • Create a Sales Order for the second Serial No (with duplicate) > submit > Reserve stock
  • Check that the created stock reservation entry has our duplicate serial no with the right name that links to the right record

You can check the PR description where Serial No. link formatting is also fixed in this PR for a duplicate case.

- Amend Serial No name on Insertion of Serial No if it has a duplicate in another item
- `get_serial_nos` must accept item_code since serial not can be same for different items.
- SN text fields contain the Serial No not the docname
- Removed `get_serial_nos_data`
- If duplicate serial nos are allowed, `get_serial_nos` requires item code, throw error.
- Else, `item_code` is optional for backwards compatibility
- On uploading new Serial Nos via CSV, make new SNs and insert SN Name in serial batch bundle
- Remove unused args & pass item_code to `get_available_serial_nos`
`serial_no` is a link field, so `serial_no_name` seems redundant.
@barredterra
Copy link
Collaborator

Linter failure is unrelated; fixed in #39511

@frappe frappe deleted a comment from stale bot Jan 23, 2024
@barredterra
Copy link
Collaborator

@rohitwaghchaure since you are the expert on this topic, we would really appreciate your review and guidance on this PR.

@barredterra
Copy link
Collaborator

barredterra commented Feb 15, 2024

Manual tests completed

Before – after comparison of reports:

  • Cost of poor Quality (no data)
  • Incorrect Serial No Valuation (no data)
  • Serial No Ledger
  • Stock Ledger
  • Stock Qty vs Serial No Count

Sales and Purchase Cycle with one serialized and one non-serialized item. Setting "Allow Duplicate Serial Nos Across Items" disabled.

  • Purchase Order
  • Purchase Receipt
  • Stock Entry (Type: "Material Transfer")
  • Stock Reconciliation
  • Sales Order
  • Stock Reservation
  • Subcontracting
  • Delivery Note

Sales and Purchase Cycle with one serialized and one non-serialized item. Setting "Allow Duplicate Serial Nos Across Items" enabled.

  • Purchase Order
  • Purchase Receipt
  • Stock Entry (Type: "Material Transfer")
  • Stock Reconciliation
  • Sales Order
  • Stock Reservation
  • Subcontracting
  • Delivery Note

@barredterra barredterra removed their assignment Feb 15, 2024
@barredterra barredterra added the squash Meant to tell reviewers that this PR should be squashed into a single commit while merging. label Feb 15, 2024
@rohitwaghchaure
Copy link
Collaborator

Found few failed cases during the testing

  • User Serial / Batch fields feature not working
    • Created same serial number with different items first
    • While making the purchase receipt, enabled "Use Serial / Batch Fields" checkbox in the line item.
    • Enter the Serial Number created at step 1 and submitted
    • System has created new serial number with has value, also system has ignored "Serial Number Series" from the item master while creating new serial no (primary key should has value not the serial no field's value)
  • Getting below error while making the stock reconciliation with User Serial / Batch fields
image

@marination
Copy link
Collaborator Author

marination commented Feb 19, 2024

ToDo:

  • Test Serial No Series, if autoname obstructs serial no series naming
  • text field should have serial no (and not name) and name should be handled on the backend
  • Case: Scanning serial no does not know which item the srrial no belongs to, same with POS

@stale stale bot added inactive and removed inactive labels Mar 14, 2024
Comment on lines +90 to +96
if frappe.db.exists("Serial No", {"serial_no": self.serial_no, "item_code": self.item_code}):
frappe.throw(
msg=_("Serial No {0} already exists for Item {1}").format(
frappe.bold(self.serial_no), frappe.bold(self.item_code)
),
title=_("Duplicate"),
)
Copy link
Member

@ankush ankush Mar 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While validation is fine, this should also be a DB level unique constraint using db.add_unique we have had many such problem with Bin where ORM level validations are not sufficient.

@frappe frappe deleted a comment from PatrickDEissler Mar 23, 2024
@frappe frappe deleted a comment from stale bot Mar 23, 2024
@stale stale bot added the inactive label Apr 10, 2024
@frappe frappe deleted a comment from stale bot Apr 11, 2024
Copy link

stale bot commented Apr 27, 2024

This pull request has been automatically marked as inactive because it has not had recent activity. It will be closed within 3 days if no further activity occurs, but it only takes a comment to keep a contribution alive :) Also, even if it is closed, you can always reopen the PR when you're ready. Thank you for contributing.

@stale stale bot added the inactive label Apr 27, 2024
@stale stale bot closed this May 3, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 18, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backport version-15-hotfix inactive needs-tests This PR needs automated unit-tests. squash Meant to tell reviewers that this PR should be squashed into a single commit while merging. stock
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants