fix: Linter and minor code refactor

- Create an indexed map of stale packed items table to avoid loops to check if packed item row exists
- Reset packed items if row deletion takes place
- Renamed functions to self-explain them

erpnext/public/js/controllers/buying.js

@@ -441,7 +441,7 @@ erpnext.buying.get_items_from_product_bundle = function(frm) {
 				type: "GET",
 				method: "erpnext.stock.doctype.packed_item.packed_item.get_items_from_product_bundle",
 				args: {
-					args: {
+					row: {
 						item_code: args.product_bundle,
 						quantity: args.quantity,
 						parenttype: frm.doc.doctype,

erpnext/stock/doctype/packed_item/packed_item.py

@@ -21,17 +21,22 @@ def make_packing_list(doc):
 	if doc.get("_action") and doc._action == "update_after_submit":
 		return
 
+	packed_items_table = get_indexed_packed_items_table(doc)
+	reset = False
+	parent_items = []
+
 	if not doc.is_new():
-		reset_packing_list_if_deleted_items_exist(doc)
+		reset = reset_packing_list_if_deleted_items_exist(doc)
 
-	parent_items = []
 	for item in doc.get("items"):
 		if frappe.db.exists("Product Bundle", {"new_item_code": item.item_code}):
 			for bundle_item in get_product_bundle_items(item.item_code):
 				update_packing_list_item(
-					doc=doc, packing_item_code=bundle_item.item_code,
-					qty=flt(bundle_item.qty) * flt(item.stock_qty),
-					main_item_row=item, description=bundle_item.description
+					doc=doc,
+					packing_item=bundle_item,
+					main_item_row=item,
+					packed_items_table=packed_items_table,
+					reset=reset
 				)
 
 			if [item.item_code, item.name] not in parent_items:
@@ -40,15 +45,34 @@ def make_packing_list(doc):
 	if frappe.db.get_single_value("Selling Settings", "editable_bundle_item_rates"):
 		update_product_bundle_price(doc, parent_items)
 
+def get_indexed_packed_items_table(doc):
+	"""
+		Create dict from stale packed items table like:
+		{
+			(Bundle 1, Bundle Item 1, ae4b5678): {...},
+			(Bundle 1, Bundle Item 2, ae4b5678): {...}
+		}
+	"""
+	indexed_table = {}
+	for packed_item in doc.get("packed_items"):
+		key = (packed_item.parent_item, packed_item.item_code, packed_item.parent_detail_docname)
+		indexed_table[key] = packed_item
+
+	return indexed_table
+
 def reset_packing_list_if_deleted_items_exist(doc):
 	doc_before_save = doc.get_doc_before_save()
+	reset_table = False
+
 	if doc_before_save:
-		items_are_deleted = len(doc_before_save.get("items")) != len(doc.get("items"))
+		# reset table if items were deleted
+		reset_table = len(doc_before_save.get("items")) > len(doc.get("items"))
 	else:
-		items_are_deleted = True
+		reset_table = True # reset if via Update Items (cannot determine action)
 
-	if items_are_deleted:
+	if reset_table:
 		doc.set("packed_items", [])
+	return reset_table
 
 def get_product_bundle_items(item_code):
 	product_bundle = frappe.qb.DocType("Product Bundle")
@@ -70,50 +94,65 @@ def get_product_bundle_items(item_code):
 	)
 	return query.run(as_dict=True)
 
-def update_packing_list_item(doc, packing_item_code, qty, main_item_row, description):
-	old_packed_items_map = None
+def update_packing_list_item(doc, packing_item, main_item_row, packed_items_table, reset):
+	"""
+		doc: Transaction document
+		packing_item (dict): Packed Item details
+		main_item_row (dict): Items table row corresponding to packed item
+		packed_items_table (dict): Packed Items table before save (indexed)
+	"""
+	prev_doc_packed_items_map, exists = None, False
 
 	if doc.amended_from:
-		old_packed_items_map = get_old_packed_item_details(doc.packed_items)
+		prev_doc_packed_items_map = get_cancelled_doc_packed_item_details(doc.packed_items)
 
+	packing_item_code = packing_item.item_code
 	item = get_packing_item_details(packing_item_code, doc.company)
 
-	# check if exists
-	exists = 0
-	for d in doc.get("packed_items"):
-		if (d.parent_item == main_item_row.item_code and
-			d.item_code == packing_item_code and
-			d.parent_detail_docname == main_item_row.name
-		):
-			pi, exists = d, 1
-			break
+	# check if row already exists in packed items table
+	key = (main_item_row.item_code, packing_item_code, main_item_row.name)
+	if packed_items_table.get(key):
+		pi = packed_items_table.get(key)
+		exists = True
+
+	if not exists or reset:
+		row = pi if reset else {}
+		if reset:
+			row.idx, row.name = None, None
 
-	if not exists:
-		pi = doc.append('packed_items', {})
+		pi = doc.append('packed_items', row)
 
 	pi.parent_item = main_item_row.item_code
 	pi.item_code = packing_item_code
 	pi.item_name = item.item_name
 	pi.parent_detail_docname = main_item_row.name
 	pi.uom = item.stock_uom
-	pi.qty = flt(qty)
+	pi.qty = flt(packing_item.qty) * flt(main_item_row.stock_qty)
 	pi.conversion_factor = main_item_row.conversion_factor
-	if description and not pi.description:
-		pi.description = description
+
+	if not pi.description:
+		pi.description = packing_item.get("description")
+
 	if not pi.warehouse and not doc.amended_from:
 		pi.warehouse = (main_item_row.warehouse if ((doc.get('is_pos') or item.is_stock_item \
 			or not item.default_warehouse) and main_item_row.warehouse) else item.default_warehouse)
-	if not pi.batch_no and not doc.amended_from:
-		pi.batch_no = cstr(main_item_row.get("batch_no"))
+
+	# if not pi.batch_no and not doc.amended_from:
+	# 	pi.batch_no = cstr(main_item_row.get("batch_no"))
+
 	if not pi.target_warehouse:
 		pi.target_warehouse = main_item_row.get("target_warehouse")
+
 	bin = get_packed_item_bin_qty(packing_item_code, pi.warehouse)
 	pi.actual_qty = flt(bin.get("actual_qty"))
 	pi.projected_qty = flt(bin.get("projected_qty"))
-	if old_packed_items_map and old_packed_items_map.get((packing_item_code, main_item_row.item_code)):
-		pi.batch_no = old_packed_items_map.get((packing_item_code, main_item_row.item_code))[0].batch_no
-		pi.serial_no = old_packed_items_map.get((packing_item_code, main_item_row.item_code))[0].serial_no
-		pi.warehouse = old_packed_items_map.get((packing_item_code, main_item_row.item_code))[0].warehouse
+
+	if prev_doc_packed_items_map and prev_doc_packed_items_map.get((packing_item_code, main_item_row.item_code)):
+		pi.batch_no = prev_doc_packed_items_map.get((packing_item_code, main_item_row.item_code))[0].batch_no
+		pi.serial_no = prev_doc_packed_items_map.get((packing_item_code, main_item_row.item_code))[0].serial_no
+		pi.warehouse = prev_doc_packed_items_map.get((packing_item_code, main_item_row.item_code))[0].warehouse
+	# TODO: actual_batch_qty
+	# TODO: incoming_rate
 
 def get_packing_item_details(item_code, company):
 	item = frappe.qb.DocType("Item")
@@ -144,33 +183,11 @@ def get_packed_item_bin_qty(item, warehouse):
 
 	return bin_data[0] if bin_data else {}
 
-def get_old_packed_item_details(old_packed_items):
-	old_packed_items_map = {}
+def get_cancelled_doc_packed_item_details(old_packed_items):
+	prev_doc_packed_items_map = {}
 	for items in old_packed_items:
-		old_packed_items_map.setdefault((items.item_code ,items.parent_item), []).append(items.as_dict())
-	return old_packed_items_map
-
-def add_item_to_packing_list(doc, packed_item):
-	doc.append("packed_items", {
-		'parent_item': packed_item.parent_item,
-		'item_code': packed_item.item_code,
-		'item_name': packed_item.item_name,
-		'uom': packed_item.uom,
-		'qty': packed_item.qty,
-		'rate': packed_item.rate,
-		'conversion_factor': packed_item.conversion_factor,
-		'description': packed_item.description,
-		'warehouse': packed_item.warehouse,
-		'batch_no': packed_item.batch_no,
-		'actual_batch_qty': packed_item.actual_batch_qty,
-		'serial_no': packed_item.serial_no,
-		'target_warehouse': packed_item.target_warehouse,
-		'actual_qty': packed_item.actual_qty,
-		'projected_qty': packed_item.projected_qty,
-		'incoming_rate': packed_item.incoming_rate,
-		'prevdoc_doctype': packed_item.prevdoc_doctype,
-		'parent_detail_docname': packed_item.parent_detail_docname
-	})
+		prev_doc_packed_items_map.setdefault((items.item_code ,items.parent_item), []).append(items.as_dict())
+	return prev_doc_packed_items_map
 
 def update_product_bundle_price(doc, parent_items):
 	"""Updates the prices of Product Bundles based on the rates of the Items in the bundle."""
@@ -204,17 +221,18 @@ def update_parent_item_price(doc, parent_item_code, bundle_price):
 		parent_item_doc.amount = bundle_price
 		parent_item_doc.rate = bundle_price/(parent_item_doc.qty or 1)
 
+
 @frappe.whitelist()
-def get_items_from_product_bundle(args):
-	args, items = json.loads(args), []
+def get_items_from_product_bundle(row):
+	row, items = json.loads(row), []
 
-	bundled_items = get_product_bundle_items(args["item_code"])
+	bundled_items = get_product_bundle_items(row["item_code"])
 	for item in bundled_items:
-		args.update({
+		row.update({
 			"item_code": item.item_code,
-			"qty": flt(args["quantity"]) * flt(item.qty)
+			"qty": flt(row["quantity"]) * flt(item.qty)
 		})
-		items.append(get_item_details(args))
+		items.append(get_item_details(row))
 
 	return items