Merge pull request #39886 from barredterra/refactor-sales-invoice-item

refactor(Sales Invoice): move methods into child row controller

erpnext/accounts/doctype/pos_invoice_item/pos_invoice_item.py

@@ -3,10 +3,10 @@
 
 
 # import frappe
-from frappe.model.document import Document
+from erpnext.accounts.doctype.sales_invoice_item.sales_invoice_item import SalesInvoiceItem
 
 
-class POSInvoiceItem(Document):
+class POSInvoiceItem(SalesInvoiceItem):
 	# begin: auto-generated types
 	# This code is auto-generated. Do not modify anything in this block.
 

erpnext/accounts/doctype/sales_invoice/sales_invoice.py

@@ -8,6 +8,7 @@ from frappe.contacts.doctype.address.address import get_address_display
 from frappe.model.mapper import get_mapped_doc
 from frappe.model.utils import get_fetch_values
 from frappe.utils import add_days, cint, cstr, flt, formatdate, get_link_to_form, getdate, nowdate
+from frappe.utils.data import comma_and
 
 import erpnext
 from erpnext.accounts.deferred_revenue import validate_service_stop_date
@@ -27,7 +28,6 @@ from erpnext.accounts.party import get_due_date, get_party_account, get_party_de
 from erpnext.accounts.utils import cancel_exchange_gain_loss_journal, get_account_currency
 from erpnext.assets.doctype.asset.depreciation import (
 	depreciate_asset,
-	get_disposal_account_and_cost_center,
 	get_gl_entries_on_asset_disposal,
 	get_gl_entries_on_asset_regain,
 	reset_depreciation_schedule,
@@ -39,7 +39,6 @@ from erpnext.controllers.selling_controller import SellingController
 from erpnext.projects.doctype.timesheet.timesheet import get_projectwise_timesheet_data
 from erpnext.setup.doctype.company.company import update_company_current_month_sales
 from erpnext.stock.doctype.delivery_note.delivery_note import update_billed_amount_based_on_so
-from erpnext.stock.doctype.serial_no.serial_no import get_delivery_note_serial_no, get_serial_nos
 
 form_grid_templates = {"items": "templates/form_grid/item_grid.html"}
 
@@ -297,10 +296,12 @@ class SalesInvoice(SellingController):
 		if cint(self.is_pos):
 			self.validate_pos()
 
-		if cint(self.update_stock):
 		self.validate_dropship_item()
+
+		if cint(self.update_stock):
 			self.validate_warehouse()
 			self.update_current_stock()
+
 		self.validate_delivery_note()
 
 		# validate service stop date to lie in between start and end date
@@ -379,13 +380,7 @@ class SalesInvoice(SellingController):
 
 	def validate_item_cost_centers(self):
 		for item in self.items:
-			cost_center_company = frappe.get_cached_value("Cost Center", item.cost_center, "company")
-			if cost_center_company != self.company:
-				frappe.throw(
-					_("Row #{0}: Cost Center {1} does not belong to company {2}").format(
-						frappe.bold(item.idx), frappe.bold(item.cost_center), frappe.bold(self.company)
-					)
-				)
+			item.validate_cost_center(self.company)
 
 	def validate_income_account(self):
 		for item in self.get("items"):
@@ -601,7 +596,9 @@ class SalesInvoice(SellingController):
 		self.delete_auto_created_batches()
 
 	def update_status_updater_args(self):
-		if cint(self.update_stock):
+		if not cint(self.update_stock):
+			return
+
 		self.status_updater.append(
 			{
 				"source_dt": "Sales Invoice Item",
@@ -623,7 +620,10 @@ class SalesInvoice(SellingController):
 				where name=`tabSales Invoice Item`.parent and update_stock = 1)""",
 			}
 		)
-			if cint(self.is_return):
+
+		if not cint(self.is_return):
+			return
+
 		self.status_updater.append(
 			{
 				"source_dt": "Sales Invoice Item",
@@ -662,13 +662,8 @@ class SalesInvoice(SellingController):
 	def unlink_sales_invoice_from_timesheets(self):
 		for row in self.timesheets:
 			timesheet = frappe.get_doc("Timesheet", row.time_sheet)
-			for time_log in timesheet.time_logs:
-				if time_log.sales_invoice == self.name:
-					time_log.sales_invoice = None
-			timesheet.calculate_total_amounts()
-			timesheet.calculate_percentage_billed()
+			timesheet.unlink_sales_invoice(self.name)
 			timesheet.flags.ignore_validate_update_after_submit = True
-			timesheet.set_status()
 			timesheet.db_update_all()
 
 	@frappe.whitelist()
@@ -1011,11 +1006,16 @@ class SalesInvoice(SellingController):
 				frappe.throw(_("Warehouse required for stock Item {0}").format(d.item_code))
 
 	def validate_delivery_note(self):
-		for d in self.get("items"):
-			if d.delivery_note:
-				msgprint(
-					_("Stock cannot be updated against Delivery Note {0}").format(d.delivery_note),
-					raise_exception=1,
+		"""If items are linked with a delivery note, stock cannot be updated again."""
+		if not cint(self.update_stock):
+			return
+
+		notes = [item.delivery_note for item in self.items if item.delivery_note]
+		if notes:
+			frappe.throw(
+				_("Stock cannot be updated against the following Delivery Notes: {0}").format(
+					comma_and(notes)
+				),
 			)
 
 	def validate_write_off_account(self):
@@ -1030,29 +1030,23 @@ class SalesInvoice(SellingController):
 			msgprint(_("Please enter Account for Change Amount"), raise_exception=1)
 
 	def validate_dropship_item(self):
-		for item in self.items:
-			if item.sales_order:
-				if frappe.db.get_value("Sales Order Item", item.so_detail, "delivered_by_supplier"):
-					frappe.throw(_("Could not update stock, invoice contains drop shipping item."))
+		"""If items are drop shipped, stock cannot be updated."""
+		if not cint(self.update_stock):
+			return
+
+		if any(item.delivered_by_supplier for item in self.items):
+			frappe.throw(
+				_(
+					"Stock cannot be updated because the invoice contains a drop shipping item. Please disable 'Update Stock' or remove the drop shipping item."
+				),
+			)
 
 	def update_current_stock(self):
-		for d in self.get("items"):
-			if d.item_code and d.warehouse:
-				bin = frappe.db.sql(
-					"select actual_qty from `tabBin` where item_code = %s and warehouse = %s",
-					(d.item_code, d.warehouse),
-					as_dict=1,
-				)
-				d.actual_qty = bin and flt(bin[0]["actual_qty"]) or 0
+		for item in self.items:
+			item.set_actual_qty()
 
-		for d in self.get("packed_items"):
-			bin = frappe.db.sql(
-				"select actual_qty, projected_qty from `tabBin` where item_code =	%s and warehouse = %s",
-				(d.item_code, d.warehouse),
-				as_dict=1,
-			)
-			d.actual_qty = bin and flt(bin[0]["actual_qty"]) or 0
-			d.projected_qty = bin and flt(bin[0]["projected_qty"]) or 0
+		for packed_item in self.packed_items:
+			packed_item.set_actual_and_projected_qty()
 
 	def update_packing_list(self):
 		if cint(self.update_stock) == 1:
@@ -1127,17 +1121,8 @@ class SalesInvoice(SellingController):
 		return warehouse
 
 	def set_income_account_for_fixed_assets(self):
-		disposal_account = depreciation_cost_center = None
-		for d in self.get("items"):
-			if d.is_fixed_asset:
-				if not disposal_account:
-					disposal_account, depreciation_cost_center = get_disposal_account_and_cost_center(
-						self.company
-					)
-
-				d.income_account = disposal_account
-				if not d.cost_center:
-					d.cost_center = depreciation_cost_center
+		for item in self.items:
+			item.set_income_account_for_fixed_asset(self.company)
 
 	def check_prev_docstatus(self):
 		for d in self.get("items"):
@@ -1510,12 +1495,16 @@ class SalesInvoice(SellingController):
 					)
 
 			if not skip_change_gl_entries:
-				self.make_gle_for_change_amount(gl_entries)
+				gl_entries.extend(self.get_gle_for_change_amount())
 
-	def make_gle_for_change_amount(self, gl_entries):
-		if self.change_amount:
-			if self.account_for_change_amount:
-				gl_entries.append(
+	def get_gle_for_change_amount(self) -> list[dict]:
+		if not self.change_amount:
+			return []
+
+		if not self.account_for_change_amount:
+			frappe.throw(_("Please set Account for Change Amount"), title=_("Mandatory Field"))
+
+		return [
 			self.get_gl_dict(
 				{
 					"account": self.debit_to,
@@ -1535,10 +1524,7 @@ class SalesInvoice(SellingController):
 				},
 				self.party_account_currency,
 				item=self,
-					)
-				)
-
-				gl_entries.append(
+			),
 			self.get_gl_dict(
 				{
 					"account": self.account_for_change_amount,
@@ -1547,10 +1533,8 @@ class SalesInvoice(SellingController):
 					"cost_center": self.cost_center,
 				},
 				item=self,
-					)
-				)
-			else:
-				frappe.throw(_("Select change amount account"), title=_("Mandatory Field"))
+			),
+		]
 
 	def make_write_off_gl_entry(self, gl_entries):
 		# write off entries, applicable if only pos
@@ -1659,48 +1643,9 @@ class SalesInvoice(SellingController):
 		"""
 		validate serial number agains Delivery Note and Sales Invoice
 		"""
-		self.set_serial_no_against_delivery_note()
-		self.validate_serial_against_delivery_note()
-
-	def set_serial_no_against_delivery_note(self):
 		for item in self.items:
-			if item.serial_no and item.delivery_note and item.qty != len(get_serial_nos(item.serial_no)):
-				item.serial_no = get_delivery_note_serial_no(item.item_code, item.qty, item.delivery_note)
-
-	def validate_serial_against_delivery_note(self):
-		"""
-		validate if the serial numbers in Sales Invoice Items are same as in
-		Delivery Note Item
-		"""
-
-		for item in self.items:
-			if not item.delivery_note or not item.dn_detail:
-				continue
-
-			serial_nos = frappe.db.get_value("Delivery Note Item", item.dn_detail, "serial_no") or ""
-			dn_serial_nos = set(get_serial_nos(serial_nos))
-
-			serial_nos = item.serial_no or ""
-			si_serial_nos = set(get_serial_nos(serial_nos))
-			serial_no_diff = si_serial_nos - dn_serial_nos
-
-			if serial_no_diff:
-				dn_link = frappe.utils.get_link_to_form("Delivery Note", item.delivery_note)
-				serial_no_msg = ", ".join(frappe.bold(d) for d in serial_no_diff)
-
-				msg = _("Row #{0}: The following Serial Nos are not present in Delivery Note {1}:").format(
-					item.idx, dn_link
-				)
-				msg += " " + serial_no_msg
-
-				frappe.throw(msg=msg, title=_("Serial Nos Mismatch"))
-
-			if item.serial_no and cint(item.qty) != len(si_serial_nos):
-				frappe.throw(
-					_("Row #{0}: {1} Serial numbers required for Item {2}. You have provided {3}.").format(
-						item.idx, item.qty, item.item_code, len(si_serial_nos)
-					)
-				)
+			item.set_serial_no_against_delivery_note()
+			item.validate_serial_against_delivery_note()
 
 	def update_project(self):
 		if self.project:

erpnext/accounts/doctype/sales_invoice_item/sales_invoice_item.py

@@ -2,7 +2,13 @@
 # License: GNU General Public License v3. See license.txt
 
 
+import frappe
+from frappe import _
 from frappe.model.document import Document
+from frappe.utils.data import cint
+
+from erpnext.assets.doctype.asset.depreciation import get_disposal_account_and_cost_center
+from erpnext.stock.doctype.serial_no.serial_no import get_delivery_note_serial_no, get_serial_nos
 
 
 class SalesInvoiceItem(Document):
@@ -92,4 +98,67 @@ class SalesInvoiceItem(Document):
 		weight_uom: DF.Link | None
 	# end: auto-generated types
 
-	pass
+	def validate_cost_center(self, company: str):
+		cost_center_company = frappe.get_cached_value("Cost Center", self.cost_center, "company")
+		if cost_center_company != company:
+			frappe.throw(
+				_("Row #{0}: Cost Center {1} does not belong to company {2}").format(
+					frappe.bold(self.idx), frappe.bold(self.cost_center), frappe.bold(company)
+				)
+			)
+
+	def set_actual_qty(self):
+		if self.item_code and self.warehouse:
+			self.actual_qty = (
+				frappe.db.get_value(
+					"Bin", {"item_code": self.item_code, "warehouse": self.warehouse}, "actual_qty"
+				)
+				or 0
+			)
+
+	def set_income_account_for_fixed_asset(self, company: str):
+		"""Set income account for fixed asset item based on company's disposal account and cost center."""
+		if not self.is_fixed_asset:
+			return
+
+		disposal_account, depreciation_cost_center = get_disposal_account_and_cost_center(company)
+
+		self.income_account = disposal_account
+		if not self.cost_center:
+			self.cost_center = depreciation_cost_center
+
+	def set_serial_no_against_delivery_note(self):
+		"""Set serial no based on delivery note."""
+		if self.serial_no and self.delivery_note and self.qty != len(get_serial_nos(self.serial_no)):
+			self.serial_no = get_delivery_note_serial_no(self.item_code, self.qty, self.delivery_note)
+
+	def validate_serial_against_delivery_note(self):
+		"""Ensure the serial numbers in this Sales Invoice Item are same as in the linked Delivery Note."""
+		if not self.delivery_note or not self.dn_detail:
+			return
+
+		serial_nos = frappe.db.get_value("Delivery Note Item", self.dn_detail, "serial_no") or ""
+		dn_serial_nos = set(get_serial_nos(serial_nos))
+
+		serial_nos = self.serial_no or ""
+		si_serial_nos = set(get_serial_nos(serial_nos))
+		serial_no_diff = si_serial_nos - dn_serial_nos
+
+		if serial_no_diff:
+			dn_link = frappe.utils.get_link_to_form("Delivery Note", self.delivery_note)
+			msg = (
+				_("Row #{0}: The following serial numbers are not present in Delivery Note {1}:").format(
+					self.idx, dn_link
+				)
+				+ " "
+				+ ", ".join(frappe.bold(d) for d in serial_no_diff)
+			)
+
+			frappe.throw(msg=msg, title=_("Serial Nos Mismatch"))
+
+		if self.serial_no and cint(self.qty) != len(si_serial_nos):
+			frappe.throw(
+				_(
+					"Row #{0}: {1} serial numbers are required for Item {2}. You have provided {3} serial numbers."
+				).format(self.idx, self.qty, self.item_code, len(si_serial_nos))
+			)

erpnext/projects/doctype/timesheet/timesheet.py

@@ -256,6 +256,16 @@ class Timesheet(Document):
 		if not ts_detail.is_billable:
 			ts_detail.billing_rate = 0.0
 
+	def unlink_sales_invoice(self, sales_invoice: str):
+		"""Remove link to Sales Invoice from all time logs."""
+		for time_log in self.time_logs:
+			if time_log.sales_invoice == sales_invoice:
+				time_log.sales_invoice = None
+
+		self.calculate_total_amounts()
+		self.calculate_percentage_billed()
+		self.set_status()
+
 
 @frappe.whitelist()
 def get_projectwise_timesheet_data(project=None, parent=None, from_time=None, to_time=None):

erpnext/stock/doctype/packed_item/packed_item.py

@@ -51,7 +51,16 @@ class PackedItem(Document):
 		warehouse: DF.Link | None
 	# end: auto-generated types
 
-	pass
+	def set_actual_and_projected_qty(self):
+		"Set actual and projected qty based on warehouse and item_code"
+		_bin = frappe.db.get_value(
+			"Bin",
+			{"item_code": self.item_code, "warehouse": self.warehouse},
+			["actual_qty", "projected_qty"],
+			as_dict=True,
+		)
+		self.actual_qty = _bin.actual_qty if _bin else 0
+		self.projected_qty = _bin.projected_qty if _bin else 0
 
 
 def make_packing_list(doc):