Merge pull request #31284 from rmehta/item-form-cleanup

fix(ux): Add tabs in Item
diff --git a/erpnext/accounts/doctype/purchase_invoice/purchase_invoice.py b/erpnext/accounts/doctype/purchase_invoice/purchase_invoice.py
index e6da666..23ad223 100644
--- a/erpnext/accounts/doctype/purchase_invoice/purchase_invoice.py
+++ b/erpnext/accounts/doctype/purchase_invoice/purchase_invoice.py
@@ -545,7 +545,16 @@
 					from_repost=from_repost,
 				)
 			elif self.docstatus == 2:
+				provisional_entries = [a for a in gl_entries if a.voucher_type == "Purchase Receipt"]
 				make_reverse_gl_entries(voucher_type=self.doctype, voucher_no=self.name)
+				if provisional_entries:
+					for entry in provisional_entries:
+						frappe.db.set_value(
+							"GL Entry",
+							{"voucher_type": "Purchase Receipt", "voucher_detail_no": entry.voucher_detail_no},
+							"is_cancelled",
+							1,
+						)
 
 			if update_outstanding == "No":
 				update_outstanding_amt(
@@ -1127,7 +1136,7 @@
 		# Stock ledger value is not matching with the warehouse amount
 		if (
 			self.update_stock
-			and voucher_wise_stock_value.get(item.name)
+			and voucher_wise_stock_value.get((item.name, item.warehouse))
 			and warehouse_debit_amount
 			!= flt(voucher_wise_stock_value.get((item.name, item.warehouse)), net_amt_precision)
 		):
diff --git a/erpnext/accounts/doctype/purchase_invoice/test_purchase_invoice.py b/erpnext/accounts/doctype/purchase_invoice/test_purchase_invoice.py
index 30d26ac..3c70e24 100644
--- a/erpnext/accounts/doctype/purchase_invoice/test_purchase_invoice.py
+++ b/erpnext/accounts/doctype/purchase_invoice/test_purchase_invoice.py
@@ -27,12 +27,13 @@
 	make_purchase_receipt,
 )
 from erpnext.stock.doctype.stock_entry.test_stock_entry import get_qty_after_transaction
+from erpnext.stock.tests.test_utils import StockTestMixin
 
 test_dependencies = ["Item", "Cost Center", "Payment Term", "Payment Terms Template"]
 test_ignore = ["Serial No"]
 
 
-class TestPurchaseInvoice(unittest.TestCase):
+class TestPurchaseInvoice(unittest.TestCase, StockTestMixin):
 	@classmethod
 	def setUpClass(self):
 		unlink_payment_on_cancel_of_invoice()
@@ -693,6 +694,80 @@
 			self.assertEqual(expected_values[gle.account][0], gle.debit)
 			self.assertEqual(expected_values[gle.account][1], gle.credit)
 
+	def test_standalone_return_using_pi(self):
+		from erpnext.stock.doctype.stock_entry.test_stock_entry import make_stock_entry
+
+		item = self.make_item().name
+		company = "_Test Company with perpetual inventory"
+		warehouse = "Stores - TCP1"
+
+		make_stock_entry(item_code=item, target=warehouse, qty=50, rate=120)
+
+		return_pi = make_purchase_invoice(
+			is_return=1,
+			item=item,
+			qty=-10,
+			update_stock=1,
+			rate=100,
+			company=company,
+			warehouse=warehouse,
+			cost_center="Main - TCP1",
+		)
+
+		# assert that stock consumption is with actual rate
+		self.assertGLEs(
+			return_pi,
+			[{"credit": 1200, "debit": 0}],
+			gle_filters={"account": "Stock In Hand - TCP1"},
+		)
+
+		# assert loss booked in COGS
+		self.assertGLEs(
+			return_pi,
+			[{"credit": 0, "debit": 200}],
+			gle_filters={"account": "Cost of Goods Sold - TCP1"},
+		)
+
+	def test_return_with_lcv(self):
+		from erpnext.controllers.sales_and_purchase_return import make_return_doc
+		from erpnext.stock.doctype.landed_cost_voucher.test_landed_cost_voucher import (
+			create_landed_cost_voucher,
+		)
+
+		item = self.make_item().name
+		company = "_Test Company with perpetual inventory"
+		warehouse = "Stores - TCP1"
+		cost_center = "Main - TCP1"
+
+		pi = make_purchase_invoice(
+			item=item,
+			company=company,
+			warehouse=warehouse,
+			cost_center=cost_center,
+			update_stock=1,
+			qty=10,
+			rate=100,
+		)
+
+		# Create landed cost voucher - will increase valuation of received item by 10
+		create_landed_cost_voucher("Purchase Invoice", pi.name, pi.company, charges=100)
+		return_pi = make_return_doc(pi.doctype, pi.name)
+		return_pi.save().submit()
+
+		# assert that stock consumption is with actual in rate
+		self.assertGLEs(
+			return_pi,
+			[{"credit": 1100, "debit": 0}],
+			gle_filters={"account": "Stock In Hand - TCP1"},
+		)
+
+		# assert loss booked in COGS
+		self.assertGLEs(
+			return_pi,
+			[{"credit": 0, "debit": 100}],
+			gle_filters={"account": "Cost of Goods Sold - TCP1"},
+		)
+
 	def test_multi_currency_gle(self):
 		pi = make_purchase_invoice(
 			supplier="_Test Supplier USD",
@@ -1526,6 +1601,18 @@
 
 		check_gl_entries(self, pr.name, expected_gle_for_purchase_receipt, pr.posting_date)
 
+		# Cancel purchase invoice to check reverse provisional entry cancellation
+		pi.cancel()
+
+		expected_gle_for_purchase_receipt_post_pi_cancel = [
+			["Provision Account - _TC", 0, 250, pi.posting_date],
+			["_Test Account Cost for Goods Sold - _TC", 250, 0, pi.posting_date],
+		]
+
+		check_gl_entries(
+			self, pr.name, expected_gle_for_purchase_receipt_post_pi_cancel, pr.posting_date
+		)
+
 		company.enable_provisional_accounting_for_non_stock_items = 0
 		company.save()
 
diff --git a/erpnext/accounts/report/sales_register/sales_register.py b/erpnext/accounts/report/sales_register/sales_register.py
index 777d96c..33bd3c7 100644
--- a/erpnext/accounts/report/sales_register/sales_register.py
+++ b/erpnext/accounts/report/sales_register/sales_register.py
@@ -367,8 +367,8 @@
 		if not filters.get(field) or field in accounting_dimensions_list:
 			return ""
 		return f""" and exists(select name from `tab{table}`
-			 where parent=`tabSales Invoice`.name
-			 	and ifnull(`tab{table}`.{field}, '') = %({field})s)"""
+				where parent=`tabSales Invoice`.name
+					and ifnull(`tab{table}`.{field}, '') = %({field})s)"""
 
 	conditions += get_sales_invoice_item_field_condition("mode_of_payments", "Sales Invoice Payment")
 	conditions += get_sales_invoice_item_field_condition("cost_center")
diff --git a/erpnext/accounts/test/test_reports.py b/erpnext/accounts/test/test_reports.py
index 19fe74f..3f06c30 100644
--- a/erpnext/accounts/test/test_reports.py
+++ b/erpnext/accounts/test/test_reports.py
@@ -28,6 +28,7 @@
 	("Item-wise Sales Register", {}),
 	("Item-wise Purchase Register", {}),
 	("Sales Register", {}),
+	("Sales Register", {"item_group": "All Item Groups"}),
 	("Purchase Register", {}),
 	(
 		"Tax Detail",
diff --git a/erpnext/controllers/sales_and_purchase_return.py b/erpnext/controllers/sales_and_purchase_return.py
index bd4b59b..d24ac3f 100644
--- a/erpnext/controllers/sales_and_purchase_return.py
+++ b/erpnext/controllers/sales_and_purchase_return.py
@@ -316,7 +316,7 @@
 	return data[0]
 
 
-def make_return_doc(doctype, source_name, target_doc=None):
+def make_return_doc(doctype: str, source_name: str, target_doc=None):
 	from frappe.model.mapper import get_mapped_doc
 
 	from erpnext.stock.doctype.serial_no.serial_no import get_serial_nos
diff --git a/erpnext/e_commerce/redisearch_utils.py b/erpnext/e_commerce/redisearch_utils.py
index 61b4b9e..1f649c7 100644
--- a/erpnext/e_commerce/redisearch_utils.py
+++ b/erpnext/e_commerce/redisearch_utils.py
@@ -38,7 +38,7 @@
 		out = cache.execute_command("MODULE LIST")
 
 		parsed_output = " ".join(
-			(" ".join([s.decode() for s in o if not isinstance(s, int)]) for o in out)
+			(" ".join([frappe.as_unicode(s) for s in o if not isinstance(s, int)]) for o in out)
 		)
 		return "search" in parsed_output
 	except Exception:
diff --git a/erpnext/loan_management/doctype/loan/loan.js b/erpnext/loan_management/doctype/loan/loan.js
index 940a1bb..38328e6 100644
--- a/erpnext/loan_management/doctype/loan/loan.js
+++ b/erpnext/loan_management/doctype/loan/loan.js
@@ -93,6 +93,12 @@
 					frm.trigger("make_loan_refund");
 				},__('Create'));
 			}
+
+			if (frm.doc.status == "Loan Closure Requested" && frm.doc.is_term_loan && !frm.doc.is_secured_loan) {
+				frm.add_custom_button(__('Close Loan'), function() {
+					frm.trigger("close_unsecured_term_loan");
+				},__('Status'));
+			}
 		}
 		frm.trigger("toggle_fields");
 	},
@@ -174,6 +180,18 @@
 		})
 	},
 
+	close_unsecured_term_loan: function(frm) {
+		frappe.call({
+			args: {
+				"loan": frm.doc.name
+			},
+			method: "erpnext.loan_management.doctype.loan.loan.close_unsecured_term_loan",
+			callback: function () {
+				frm.refresh();
+			}
+		})
+	},
+
 	request_loan_closure: function(frm) {
 		frappe.confirm(__("Do you really want to close this loan"),
 			function() {
diff --git a/erpnext/loan_management/doctype/loan/loan.py b/erpnext/loan_management/doctype/loan/loan.py
index 3b76ba4..ac8b362 100644
--- a/erpnext/loan_management/doctype/loan/loan.py
+++ b/erpnext/loan_management/doctype/loan/loan.py
@@ -60,11 +60,11 @@
 				)
 
 	def validate_cost_center(self):
-		if not self.cost_center and self.rate_of_interest != 0:
+		if not self.cost_center and self.rate_of_interest != 0.0:
 			self.cost_center = frappe.db.get_value("Company", self.company, "cost_center")
 
-		if not self.cost_center:
-			frappe.throw(_("Cost center is mandatory for loans having rate of interest greater than 0"))
+			if not self.cost_center:
+				frappe.throw(_("Cost center is mandatory for loans having rate of interest greater than 0"))
 
 	def on_submit(self):
 		self.link_loan_security_pledge()
@@ -342,6 +342,22 @@
 		return loan.as_dict()
 
 
+@frappe.whitelist()
+def close_unsecured_term_loan(loan):
+	loan_details = frappe.db.get_value(
+		"Loan", {"name": loan}, ["status", "is_term_loan", "is_secured_loan"], as_dict=1
+	)
+
+	if (
+		loan_details.status == "Loan Closure Requested"
+		and loan_details.is_term_loan
+		and not loan_details.is_secured_loan
+	):
+		frappe.db.set_value("Loan", loan, "status", "Closed")
+	else:
+		frappe.throw(_("Cannot close this loan until full repayment"))
+
+
 def close_loan(loan, total_amount_paid):
 	frappe.db.set_value("Loan", loan, "total_amount_paid", total_amount_paid)
 	frappe.db.set_value("Loan", loan, "status", "Closed")
diff --git a/erpnext/regional/india/e_invoice/utils.py b/erpnext/regional/india/e_invoice/utils.py
index 9add09b..5eb14a5 100644
--- a/erpnext/regional/india/e_invoice/utils.py
+++ b/erpnext/regional/india/e_invoice/utils.py
@@ -55,6 +55,9 @@
 		return False
 
 	invalid_company = not frappe.db.get_value("E Invoice User", {"company": doc.get("company")})
+	invalid_company_gstin = not frappe.db.get_value(
+		"E Invoice User", {"gstin": doc.get("company_gstin")}
+	)
 	invalid_supply_type = doc.get("gst_category") not in [
 		"Registered Regular",
 		"Registered Composition",
@@ -71,6 +74,7 @@
 
 	if (
 		invalid_company
+		or invalid_company_gstin
 		or invalid_supply_type
 		or company_transaction
 		or no_taxes_applied
diff --git a/erpnext/stock/doctype/stock_entry/stock_entry_utils.py b/erpnext/stock/doctype/stock_entry/stock_entry_utils.py
index c5c0cef..41a3b89 100644
--- a/erpnext/stock/doctype/stock_entry/stock_entry_utils.py
+++ b/erpnext/stock/doctype/stock_entry/stock_entry_utils.py
@@ -2,11 +2,37 @@
 # See license.txt
 
 
+from typing import TYPE_CHECKING, Optional, overload
+
 import frappe
 from frappe.utils import cint, flt
 
 import erpnext
 
+if TYPE_CHECKING:
+	from erpnext.stock.doctype.stock_entry.stock_entry import StockEntry
+
+
+@overload
+def make_stock_entry(
+	*,
+	item_code: str,
+	qty: float,
+	company: Optional[str] = None,
+	from_warehouse: Optional[str] = None,
+	to_warehouse: Optional[str] = None,
+	rate: Optional[float] = None,
+	serial_no: Optional[str] = None,
+	batch_no: Optional[str] = None,
+	posting_date: Optional[str] = None,
+	posting_time: Optional[str] = None,
+	purpose: Optional[str] = None,
+	do_not_save: bool = False,
+	do_not_submit: bool = False,
+	inspection_required: bool = False,
+) -> "StockEntry":
+	...
+
 
 @frappe.whitelist()
 def make_stock_entry(**args):
diff --git a/erpnext/stock/doctype/stock_ledger_entry/test_stock_ledger_entry.py b/erpnext/stock/doctype/stock_ledger_entry/test_stock_ledger_entry.py
index eb1e0fc..55a213c 100644
--- a/erpnext/stock/doctype/stock_ledger_entry/test_stock_ledger_entry.py
+++ b/erpnext/stock/doctype/stock_ledger_entry/test_stock_ledger_entry.py
@@ -24,9 +24,10 @@
 	create_stock_reconciliation,
 )
 from erpnext.stock.stock_ledger import get_previous_sle
+from erpnext.stock.tests.test_utils import StockTestMixin
 
 
-class TestStockLedgerEntry(FrappeTestCase):
+class TestStockLedgerEntry(FrappeTestCase, StockTestMixin):
 	def setUp(self):
 		items = create_items()
 		reset("Stock Entry")
@@ -541,30 +542,6 @@
 				"Incorrect 'Incoming Rate' values fetched for DN items",
 			)
 
-	def assertSLEs(self, doc, expected_sles, sle_filters=None):
-		"""Compare sorted SLEs, useful for vouchers that create multiple SLEs for same line"""
-
-		filters = {"voucher_no": doc.name, "voucher_type": doc.doctype, "is_cancelled": 0}
-		if sle_filters:
-			filters.update(sle_filters)
-		sles = frappe.get_all(
-			"Stock Ledger Entry",
-			fields=["*"],
-			filters=filters,
-			order_by="timestamp(posting_date, posting_time), creation",
-		)
-
-		for exp_sle, act_sle in zip(expected_sles, sles):
-			for k, v in exp_sle.items():
-				act_value = act_sle[k]
-				if k == "stock_queue":
-					act_value = json.loads(act_value)
-					if act_value and act_value[0][0] == 0:
-						# ignore empty fifo bins
-						continue
-
-				self.assertEqual(v, act_value, msg=f"{k} doesn't match \n{exp_sle}\n{act_sle}")
-
 	def test_batchwise_item_valuation_stock_reco(self):
 		item, warehouses, batches = setup_item_valuation_test()
 		state = {"stock_value": 0.0, "qty": 0.0}
diff --git a/erpnext/stock/doctype/stock_reconciliation/test_stock_reconciliation.py b/erpnext/stock/doctype/stock_reconciliation/test_stock_reconciliation.py
index 9088eb8..191c03f 100644
--- a/erpnext/stock/doctype/stock_reconciliation/test_stock_reconciliation.py
+++ b/erpnext/stock/doctype/stock_reconciliation/test_stock_reconciliation.py
@@ -10,7 +10,7 @@
 from frappe.utils import add_days, cstr, flt, nowdate, nowtime, random_string
 
 from erpnext.accounts.utils import get_stock_and_account_balance
-from erpnext.stock.doctype.item.test_item import create_item, make_item
+from erpnext.stock.doctype.item.test_item import create_item
 from erpnext.stock.doctype.purchase_receipt.test_purchase_receipt import make_purchase_receipt
 from erpnext.stock.doctype.serial_no.serial_no import get_serial_nos
 from erpnext.stock.doctype.stock_reconciliation.stock_reconciliation import (
@@ -19,10 +19,11 @@
 )
 from erpnext.stock.doctype.warehouse.test_warehouse import create_warehouse
 from erpnext.stock.stock_ledger import get_previous_sle, update_entries_after
+from erpnext.stock.tests.test_utils import StockTestMixin
 from erpnext.stock.utils import get_incoming_rate, get_stock_value_on, get_valuation_method
 
 
-class TestStockReconciliation(FrappeTestCase):
+class TestStockReconciliation(FrappeTestCase, StockTestMixin):
 	@classmethod
 	def setUpClass(cls):
 		create_batch_or_serial_no_items()
@@ -40,7 +41,7 @@
 		self._test_reco_sle_gle("Moving Average")
 
 	def _test_reco_sle_gle(self, valuation_method):
-		item_code = make_item(properties={"valuation_method": valuation_method}).name
+		item_code = self.make_item(properties={"valuation_method": valuation_method}).name
 
 		se1, se2, se3 = insert_existing_sle(warehouse="Stores - TCP1", item_code=item_code)
 		company = frappe.db.get_value("Warehouse", "Stores - TCP1", "company")
@@ -392,7 +393,7 @@
 		SR4		| Reco	|	0	|	6	(posting date: today-1) [backdated]
 		PR3		| PR	|	1	|	7	(posting date: today) # can't post future PR
 		"""
-		item_code = make_item().name
+		item_code = self.make_item().name
 		warehouse = "_Test Warehouse - _TC"
 
 		frappe.flags.dont_execute_stock_reposts = True
@@ -458,7 +459,7 @@
 		from erpnext.stock.doctype.delivery_note.test_delivery_note import create_delivery_note
 		from erpnext.stock.stock_ledger import NegativeStockError
 
-		item_code = make_item().name
+		item_code = self.make_item().name
 		warehouse = "_Test Warehouse - _TC"
 
 		pr1 = make_purchase_receipt(
@@ -506,7 +507,7 @@
 		from erpnext.stock.doctype.delivery_note.test_delivery_note import create_delivery_note
 		from erpnext.stock.stock_ledger import NegativeStockError
 
-		item_code = make_item().name
+		item_code = self.make_item().name
 		warehouse = "_Test Warehouse - _TC"
 
 		sr = create_stock_reconciliation(
@@ -549,7 +550,7 @@
 		# repost will make this test useless, qty should update in realtime without reposts
 		frappe.flags.dont_execute_stock_reposts = True
 
-		item_code = make_item().name
+		item_code = self.make_item().name
 		warehouse = "_Test Warehouse - _TC"
 
 		sr = create_stock_reconciliation(
diff --git a/erpnext/stock/tests/test_utils.py b/erpnext/stock/tests/test_utils.py
index 9ee0c9f..b046dbd 100644
--- a/erpnext/stock/tests/test_utils.py
+++ b/erpnext/stock/tests/test_utils.py
@@ -1,16 +1,67 @@
+import json
+
 import frappe
 from frappe.tests.utils import FrappeTestCase
 
-from erpnext.stock.doctype.item.test_item import make_item
 from erpnext.stock.utils import scan_barcode
 
 
-class TestStockUtilities(FrappeTestCase):
+class StockTestMixin:
+	"""Mixin to simplfy stock ledger tests, useful for all stock transactions."""
+
+	def make_item(self, item_code=None, properties=None, *args, **kwargs):
+		from erpnext.stock.doctype.item.test_item import make_item
+
+		return make_item(item_code, properties, *args, **kwargs)
+
+	def assertSLEs(self, doc, expected_sles, sle_filters=None):
+		"""Compare sorted SLEs, useful for vouchers that create multiple SLEs for same line"""
+
+		filters = {"voucher_no": doc.name, "voucher_type": doc.doctype, "is_cancelled": 0}
+		if sle_filters:
+			filters.update(sle_filters)
+		sles = frappe.get_all(
+			"Stock Ledger Entry",
+			fields=["*"],
+			filters=filters,
+			order_by="timestamp(posting_date, posting_time), creation",
+		)
+
+		for exp_sle, act_sle in zip(expected_sles, sles):
+			for k, v in exp_sle.items():
+				act_value = act_sle[k]
+				if k == "stock_queue":
+					act_value = json.loads(act_value)
+					if act_value and act_value[0][0] == 0:
+						# ignore empty fifo bins
+						continue
+
+				self.assertEqual(v, act_value, msg=f"{k} doesn't match \n{exp_sle}\n{act_sle}")
+
+	def assertGLEs(self, doc, expected_gles, gle_filters=None, order_by=None):
+		filters = {"voucher_no": doc.name, "voucher_type": doc.doctype, "is_cancelled": 0}
+
+		if gle_filters:
+			filters.update(gle_filters)
+		actual_gles = frappe.get_all(
+			"GL Entry",
+			fields=["*"],
+			filters=filters,
+			order_by=order_by or "posting_date, creation",
+		)
+
+		for exp_gle, act_gle in zip(expected_gles, actual_gles):
+			for k, exp_value in exp_gle.items():
+				act_value = act_gle[k]
+				self.assertEqual(exp_value, act_value, msg=f"{k} doesn't match \n{exp_gle}\n{act_gle}")
+
+
+class TestStockUtilities(FrappeTestCase, StockTestMixin):
 	def test_barcode_scanning(self):
-		simple_item = make_item(properties={"barcodes": [{"barcode": "12399"}]})
+		simple_item = self.make_item(properties={"barcodes": [{"barcode": "12399"}]})
 		self.assertEqual(scan_barcode("12399")["item_code"], simple_item.name)
 
-		batch_item = make_item(properties={"has_batch_no": 1, "create_new_batch": 1})
+		batch_item = self.make_item(properties={"has_batch_no": 1, "create_new_batch": 1})
 		batch = frappe.get_doc(doctype="Batch", item=batch_item.name).insert()
 
 		batch_scan = scan_barcode(batch.name)
@@ -19,7 +70,7 @@
 		self.assertEqual(batch_scan["has_batch_no"], 1)
 		self.assertEqual(batch_scan["has_serial_no"], 0)
 
-		serial_item = make_item(properties={"has_serial_no": 1})
+		serial_item = self.make_item(properties={"has_serial_no": 1})
 		serial = frappe.get_doc(
 			doctype="Serial No", item_code=serial_item.name, serial_no=frappe.generate_hash()
 		).insert()