Merge pull request #56173 from mihir-kandoi/pg-manufacturing-maintenance

refactor(postgres): port Manufacturing & Maintenance module queries to the query builder

erpnext/maintenance/doctype/maintenance_schedule/maintenance_schedule.py

@@ -185,9 +185,7 @@ class MaintenanceSchedule(TransactionBase):
 		else:
 			holiday_list = frappe.get_cached_value("Company", self.company, "default_holiday_list")
 
-		holidays = frappe.db.sql_list(
-			"""select holiday_date from `tabHoliday` where parent=%s""", holiday_list
-		)
+		holidays = frappe.get_all("Holiday", filters={"parent": holiday_list}, pluck="holiday_date")
 
 		if not validated and holidays:
 			# max iterations = len(holidays)
@@ -235,16 +233,22 @@ class MaintenanceSchedule(TransactionBase):
 				throw(_("Start date should be less than end date for Item {0}").format(d.item_code))
 
 	def validate_sales_order(self):
+		ms = frappe.qb.DocType("Maintenance Schedule")
+		msi = frappe.qb.DocType("Maintenance Schedule Item")
 		for d in self.get("items"):
 			if d.sales_order:
-				chk = frappe.db.sql(
-					"""select ms.name from `tabMaintenance Schedule` ms,
-					`tabMaintenance Schedule Item` msi where msi.parent=ms.name and
-					msi.sales_order=%s and ms.docstatus=1""",
-					d.sales_order,
+				# filter the parent schedule's docstatus (matches the original ms.docstatus = 1)
+				chk = (
+					frappe.qb.from_(ms)
+					.inner_join(msi)
+					.on(msi.parent == ms.name)
+					.select(ms.name)
+					.where((msi.sales_order == d.sales_order) & (ms.docstatus == 1))
+					.limit(1)
+					.run(pluck=True)
 				)
 				if chk:
-					throw(_("Maintenance Schedule {0} exists against {1}").format(chk[0][0], d.sales_order))
+					throw(_("Maintenance Schedule {0} exists against {1}").format(chk[0], d.sales_order))
 
 	def validate_items_table_change(self):
 		doc_before_save = self.get_doc_before_save()

erpnext/maintenance/doctype/maintenance_schedule/test_maintenance_schedule.py

@@ -168,6 +168,51 @@ class TestMaintenanceSchedule(ERPNextTestSuite):
 		ms.save()
 		self.assertEqual(len(ms.schedules), 2)
 
+	def test_validate_sales_order_duplicate_throws(self):
+		# validate_sales_order joins Maintenance Schedule + its item filtering the PARENT schedule's
+		# docstatus=1; a second schedule against a Sales Order already used by a submitted schedule
+		# must be rejected.
+		from erpnext.selling.doctype.sales_order.test_sales_order import make_sales_order
+
+		so = make_sales_order()
+		first = make_maintenance_schedule(sales_order=so.name)
+		self.assertEqual(first.items[0].sales_order, so.name)
+		first.submit()
+
+		self.assertRaises(frappe.ValidationError, make_maintenance_schedule, sales_order=so.name)
+
+	def test_validate_schedule_date_skips_holiday(self):
+		# validate_schedule_date_for_holiday_list reads the holiday list via the converted
+		# get_all("Holiday", {"parent": <list>}, pluck="holiday_date") and shifts a schedule date
+		# that lands on a holiday back by a day; a non-holiday date is returned unchanged.
+		from frappe.utils import getdate
+
+		from erpnext.setup.doctype.holiday_list.test_holiday_list import make_holiday_list
+
+		holiday = add_days(today(), 5)
+		hl = make_holiday_list(
+			"_Test MS Holidays " + frappe.generate_hash("", 6),
+			from_date=today(),
+			to_date=add_days(today(), 10),
+			holiday_dates=[{"holiday_date": holiday, "description": "Test Holiday"}],
+		)
+
+		ms = make_maintenance_schedule()
+		# a Sales Person with no linked employee routes to the company-default-holiday-list branch
+		sp = frappe.get_doc(
+			{"doctype": "Sales Person", "sales_person_name": "_Test MS SP " + frappe.generate_hash("", 5)}
+		).insert(ignore_permissions=True)
+		frappe.db.set_value("Company", ms.company, "default_holiday_list", hl.name)
+
+		# a date on the holiday is shifted back one day...
+		shifted = ms.validate_schedule_date_for_holiday_list(getdate(holiday), sp.name)
+		self.assertEqual(getdate(shifted), getdate(add_days(holiday, -1)))
+
+		# ...a non-holiday date is returned unchanged
+		non_holiday = add_days(today(), 7)
+		unchanged = ms.validate_schedule_date_for_holiday_list(getdate(non_holiday), sp.name)
+		self.assertEqual(getdate(unchanged), getdate(non_holiday))
+
 
 def make_serial_item_with_serial(self, item_code):
 	serial_item_doc = create_item(item_code, is_stock_item=1)
@@ -202,6 +247,7 @@ def make_maintenance_schedule(**args):
 			"no_of_visits": 4,
 			"serial_no": args.get("serial_no"),
 			"sales_person": "Sales Team",
+			"sales_order": args.get("sales_order"),
 		},
 	)
 	ms.insert(ignore_permissions=True)

erpnext/maintenance/doctype/maintenance_visit/maintenance_visit.py

@@ -143,9 +143,22 @@ class MaintenanceVisit(TransactionBase):
 						elif self.completion_status == "Partially Completed":
 							status = "Work In Progress"
 					else:
-						nm = frappe.db.sql(
-							"select t1.name, t1.mntc_date, t2.service_person, t2.work_done from `tabMaintenance Visit` t1, `tabMaintenance Visit Purpose` t2 where t2.parent = t1.name and t1.completion_status = 'Partially Completed' and t2.prevdoc_docname = %s and t1.name!=%s and t1.docstatus = 1 order by t1.name desc limit 1",
-							(d.prevdoc_docname, self.name),
+						mv = frappe.qb.DocType("Maintenance Visit")
+						mvp = frappe.qb.DocType("Maintenance Visit Purpose")
+						nm = (
+							frappe.qb.from_(mv)
+							.inner_join(mvp)
+							.on(mvp.parent == mv.name)
+							.select(mv.name, mv.mntc_date, mvp.service_person, mvp.work_done)
+							.where(
+								(mv.completion_status == "Partially Completed")
+								& (mvp.prevdoc_docname == d.prevdoc_docname)
+								& (mv.name != self.name)
+								& (mv.docstatus == 1)
+							)
+							.orderby(mv.name, order=frappe.qb.desc)
+							.limit(1)
+							.run()
 						)
 
 						if nm:
@@ -180,14 +193,27 @@ class MaintenanceVisit(TransactionBase):
 				# check_for_doctype = d.prevdoc_doctype
 
 		if check_for_docname:
-			check = frappe.db.sql(
-				"select t1.name from `tabMaintenance Visit` t1, `tabMaintenance Visit Purpose` t2 where t2.parent = t1.name and t1.name!=%s and t2.prevdoc_docname=%s and t1.docstatus = 1 and (t1.mntc_date > %s or (t1.mntc_date = %s and t1.mntc_time > %s))",
-				(self.name, check_for_docname, self.mntc_date, self.mntc_date, self.mntc_time),
+			mv = frappe.qb.DocType("Maintenance Visit")
+			mvp = frappe.qb.DocType("Maintenance Visit Purpose")
+			check = (
+				frappe.qb.from_(mv)
+				.inner_join(mvp)
+				.on(mvp.parent == mv.name)
+				.select(mv.name)
+				.where(
+					(mv.name != self.name)
+					& (mvp.prevdoc_docname == check_for_docname)
+					& (mv.docstatus == 1)
+					& (
+						(mv.mntc_date > self.mntc_date)
+						| ((mv.mntc_date == self.mntc_date) & (mv.mntc_time > self.mntc_time))
+					)
+				)
+				.run(pluck=True)
 			)
 
 			if check:
-				check_lst = [x[0] for x in check]
-				check_lst = ",".join(check_lst)
+				check_lst = ",".join(check)
 				frappe.throw(
 					_("Cancel Material Visits {0} before cancelling this Maintenance Visit").format(check_lst)
 				)

erpnext/maintenance/doctype/maintenance_visit/test_maintenance_visit.py

@@ -2,13 +2,139 @@
 # See license.txt
 
 import frappe
-from frappe.utils.data import today
+from frappe.utils.data import add_days, getdate, today
 
 from erpnext.tests.utils import ERPNextTestSuite
 
 
 class TestMaintenanceVisit(ERPNextTestSuite):
-	pass
+	def setUp(self):
+		self.sales_person = make_sales_person("_Test Maintenance Service Person")
+
+	def make_warranty_claim(self):
+		# Warranty Claim is not submittable; it provides a real target for the
+		# purposes-row Dynamic Link (prevdoc_doctype/prevdoc_docname).
+		claim = frappe.new_doc("Warranty Claim")
+		claim.status = "Open"
+		claim.complaint_date = today()
+		claim.customer = "_Test Customer"
+		claim.item_code = "_Test Item"
+		claim.complaint = "Device stopped working under warranty"
+		claim.company = "_Test Company"
+		claim.insert(ignore_permissions=True)
+		return claim
+
+	def make_visit(self, claim, completion_status, mntc_date=None, mntc_time=None, submit=True):
+		visit = frappe.new_doc("Maintenance Visit")
+		visit.company = "_Test Company"
+		visit.customer = "_Test Customer"
+		visit.mntc_date = mntc_date or today()
+		if mntc_time:
+			visit.mntc_time = mntc_time
+		visit.maintenance_type = "Unscheduled"
+		visit.completion_status = completion_status
+		visit.append(
+			"purposes",
+			{
+				"item_code": "_Test Item",
+				"service_person": self.sales_person.name,
+				"work_done": "Replaced the faulty component",
+				"description": "Warranty repair",
+				"prevdoc_doctype": "Warranty Claim",
+				"prevdoc_docname": claim.name,
+			},
+		)
+		visit.insert(ignore_permissions=True)
+		if submit:
+			visit.submit()
+		return visit
+
+	def test_cancel_blocked_when_later_visit_exists(self):
+		# check_if_last_visit's converted join query (B): cancelling an EARLIER
+		# submitted visit must be blocked while a LATER one (greater mntc_date)
+		# referencing the same prevdoc_docname is still active.
+		claim = self.make_warranty_claim()
+		earlier = self.make_visit(claim, "Partially Completed", mntc_date=today())
+		later = self.make_visit(claim, "Partially Completed", mntc_date=add_days(today(), 5))
+
+		# Sanity: both are submitted and share the prevdoc_docname the query keys on.
+		self.assertEqual(earlier.docstatus, 1)
+		self.assertEqual(later.docstatus, 1)
+		self.assertEqual(later.purposes[0].prevdoc_docname, claim.name)
+
+		# The throw originates in check_if_last_visit's query (B): a later visit exists.
+		self.assertRaisesRegex(frappe.ValidationError, later.name, earlier.cancel)
+
+	def test_cancel_blocked_by_same_date_later_time(self):
+		# Same converted query (B), time-tiebreak branch: equal mntc_date, but the
+		# blocking visit has a strictly greater mntc_time.
+		claim = self.make_warranty_claim()
+		earlier = self.make_visit(claim, "Partially Completed", mntc_date=today(), mntc_time="09:00:00")
+		later = self.make_visit(claim, "Partially Completed", mntc_date=today(), mntc_time="15:00:00")
+
+		self.assertRaisesRegex(frappe.ValidationError, later.name, earlier.cancel)
+
+	def test_cancel_allowed_for_latest_visit(self):
+		# The latest visit has no later sibling -> query (B) returns nothing ->
+		# cancellation proceeds and the visit is marked Cancelled.
+		claim = self.make_warranty_claim()
+		earlier = self.make_visit(claim, "Partially Completed", mntc_date=today())
+		later = self.make_visit(claim, "Partially Completed", mntc_date=add_days(today(), 5))
+
+		later.cancel()
+
+		self.assertEqual(frappe.db.get_value("Maintenance Visit", later.name, "docstatus"), 2)
+		self.assertEqual(frappe.db.get_value("Maintenance Visit", later.name, "status"), "Cancelled")
+		# The earlier one is untouched and still submitted.
+		self.assertEqual(frappe.db.get_value("Maintenance Visit", earlier.name, "docstatus"), 1)
+
+	def test_cancel_reopens_claim_to_work_in_progress_from_prior_partial(self):
+		# Drives the status-update query (A) inside update_customer_issue(flag=0).
+		# A submitted "Partially Completed" visit (prior) exists for the claim; when
+		# a LATER "Fully Completed" visit is cancelled, query (A) finds that prior
+		# partial visit and the Warranty Claim is reopened to "Work In Progress"
+		# carrying the prior visit's resolution data.
+		claim = self.make_warranty_claim()
+		prior = self.make_visit(claim, "Partially Completed", mntc_date=today())
+		latest = self.make_visit(claim, "Fully Completed", mntc_date=add_days(today(), 3))
+
+		# After submitting the "Fully Completed" visit the claim is Closed.
+		self.assertEqual(frappe.db.get_value("Warranty Claim", claim.name, "status"), "Closed")
+
+		# Cancelling the latest visit: no later sibling blocks it, so cancel runs
+		# update_customer_issue(0), which executes query (A) and reopens the claim.
+		latest.cancel()
+
+		claim.reload()
+		self.assertEqual(claim.status, "Work In Progress")
+		# Resolution data is back-filled from the prior partial visit found by query (A).
+		self.assertEqual(claim.resolved_by, self.sales_person.name)
+		self.assertEqual(claim.resolution_details, prior.purposes[0].work_done)
+		self.assertEqual(getdate(claim.resolution_date), getdate(prior.mntc_date))
+
+	def test_cancel_reopens_claim_to_open_when_no_prior_partial(self):
+		# Inverse of query (A): a lone "Fully Completed" visit with no prior
+		# "Partially Completed" sibling -> query (A) returns nothing -> the claim
+		# is reset to "Open" with cleared resolution fields on cancel.
+		claim = self.make_warranty_claim()
+		visit = self.make_visit(claim, "Fully Completed", mntc_date=today())
+		self.assertEqual(frappe.db.get_value("Warranty Claim", claim.name, "status"), "Closed")
+
+		visit.cancel()
+
+		claim.reload()
+		self.assertEqual(claim.status, "Open")
+		self.assertIsNone(claim.resolved_by)
+		self.assertIsNone(claim.resolution_details)
+		self.assertIsNone(claim.resolution_date)
+
+
+def make_sales_person(name):
+	sales_person = frappe.get_doc({"doctype": "Sales Person", "sales_person_name": name})
+	sales_person.insert(ignore_if_duplicate=True)
+	if not sales_person.name:
+		sales_person = frappe.get_doc("Sales Person", {"sales_person_name": name})
+	return sales_person
 
 
 def make_maintenance_visit():
@@ -33,10 +159,3 @@ def make_maintenance_visit():
 	mv.insert(ignore_permissions=True)
 
 	return mv
-
-
-def make_sales_person(name):
-	sales_person = frappe.get_doc({"doctype": "Sales Person", "sales_person_name": name})
-	sales_person.insert(ignore_if_duplicate=True)
-
-	return sales_person

erpnext/manufacturing/doctype/work_order/work_order.py

@@ -843,15 +843,16 @@ class WorkOrder(Document):
 			frappe.throw(_("Stopped Work Order cannot be cancelled, Unstop it first to cancel"))
 
 		# Check whether any stock entry exists against this Work Order
-		stock_entry = frappe.db.sql(
-			"""select name from `tabStock Entry`
-			where work_order = %s and docstatus = 1""",
-			self.name,
+		stock_entry = frappe.get_all(
+			"Stock Entry",
+			filters={"work_order": self.name, "docstatus": 1},
+			pluck="name",
+			limit=1,
 		)
 		if stock_entry:
 			frappe.throw(
 				_("Cannot cancel because submitted Stock Entry {0} exists").format(
-					frappe.utils.get_link_to_form("Stock Entry", stock_entry[0][0])
+					frappe.utils.get_link_to_form("Stock Entry", stock_entry[0])
 				)
 			)
 
@@ -942,14 +943,20 @@ class WorkOrder(Document):
 
 	@frappe.whitelist()
 	def make_bom(self):
-		data = frappe.db.sql(
-			""" select sed.item_code, sed.qty, sed.s_warehouse
-			from `tabStock Entry Detail` sed, `tabStock Entry` se
-			where se.name = sed.parent and se.purpose = 'Manufacture'
-			and (sed.t_warehouse is null or sed.t_warehouse = '') and se.docstatus = 1
-			and se.work_order = %s""",
-			(self.name),
-			as_dict=1,
+		sed = frappe.qb.DocType("Stock Entry Detail")
+		se = frappe.qb.DocType("Stock Entry")
+		data = (
+			frappe.qb.from_(sed)
+			.inner_join(se)
+			.on(se.name == sed.parent)
+			.select(sed.item_code, sed.qty, sed.s_warehouse)
+			.where(
+				(se.purpose == "Manufacture")
+				& (sed.t_warehouse.isnull() | (sed.t_warehouse == ""))
+				& (se.docstatus == 1)
+				& (se.work_order == self.name)
+			)
+			.run(as_dict=1)
 		)
 
 		bom = frappe.new_doc("BOM")

erpnext/manufacturing/doctype/workstation/test_workstation.py

@@ -110,6 +110,16 @@ class TestWorkstation(ERPNextTestSuite):
 		self.assertEqual(bom_doc.operations[0].hour_rate, 250)
 		self.assertEqual(bom_doc.operations[1].hour_rate, 250)
 
+		# update_bom_operation() (run on w1.save()) must write the new rate directly onto the
+		# Routing's BOM Operation rows. This is the converted query's own effect (not the BOM
+		# update_cost above) and is what silently skipped on Postgres when parenttype was 'routing'.
+		routing_op_rate = frappe.db.get_value(
+			"BOM Operation",
+			{"parent": routing_doc.name, "parenttype": "Routing", "workstation": "_Test Workstation A"},
+			"hour_rate",
+		)
+		self.assertEqual(routing_op_rate, 250)
+
 
 def make_workstation(*args, **kwargs):
 	args = args if args else kwargs

erpnext/manufacturing/doctype/workstation/workstation.py

@@ -169,15 +169,20 @@ class Workstation(Document):
 	def validate_overlap_for_operation_timings(self):
 		"""Check if there is no overlap in setting Workstation Operating Hours"""
 		for d in self.get("working_hours"):
-			existing = frappe.db.sql_list(
-				"""select idx from `tabWorkstation Working Hour`
-				where parent = %s and name != %s
-					and (
-						(start_time between %s and %s) or
-						(end_time between %s and %s) or
-						(%s between start_time and end_time))
-				""",
-				(self.name, d.name, d.start_time, d.end_time, d.start_time, d.end_time, d.start_time),
+			wh = frappe.qb.DocType("Workstation Working Hour")
+			existing = (
+				frappe.qb.from_(wh)
+				.select(wh.idx)
+				.where(
+					(wh.parent == self.name)
+					& (wh.name != d.name)
+					& (
+						wh.start_time.between(d.start_time, d.end_time)
+						| wh.end_time.between(d.start_time, d.end_time)
+						| ((wh.start_time <= d.start_time) & (wh.end_time >= d.start_time))
+					)
+				)
+				.run(pluck=True)
 			)
 
 			if existing:
@@ -187,17 +192,22 @@ class Workstation(Document):
 				)
 
 	def update_bom_operation(self):
-		bom_list = frappe.db.sql(
-			"""select DISTINCT parent from `tabBOM Operation`
-			where workstation = %s and parenttype = 'routing' """,
-			self.name,
+		bom_list = frappe.get_all(
+			"BOM Operation",
+			# DocType is "Routing"; the original raw SQL used 'routing', which matched only via
+			# MariaDB's case-insensitive collation and silently matched nothing on Postgres.
+			filters={"workstation": self.name, "parenttype": "Routing"},
+			pluck="parent",
+			distinct=True,
 		)
 
-		for bom_no in bom_list:
-			frappe.db.sql(
-				"""update `tabBOM Operation` set hour_rate = %s
-				where parent = %s and workstation = %s""",
-				(self.hour_rate, bom_no[0], self.name),
+		if bom_list:
+			bom_op = frappe.qb.DocType("BOM Operation")
+			(
+				frappe.qb.update(bom_op)
+				.set(bom_op.hour_rate, self.hour_rate)
+				.where(bom_op.parent.isin(bom_list) & (bom_op.workstation == self.name))
+				.run()
 			)
 
 	def validate_workstation_holiday(self, schedule_date, skip_holiday_list_check=False):
@@ -451,12 +461,15 @@ def check_workstation_for_holiday(workstation, from_datetime, to_datetime):
 	holiday_list = frappe.db.get_value("Workstation", workstation, "holiday_list")
 	if holiday_list and from_datetime and to_datetime:
 		applicable_holidays = []
-		for d in frappe.db.sql(
-			"""select holiday_date from `tabHoliday` where parent = %s
-			and holiday_date between %s and %s """,
-			(holiday_list, getdate(from_datetime), getdate(to_datetime)),
+		for holiday_date in frappe.get_all(
+			"Holiday",
+			filters={
+				"parent": holiday_list,
+				"holiday_date": ["between", [getdate(from_datetime), getdate(to_datetime)]],
+			},
+			pluck="holiday_date",
 		):
-			applicable_holidays.append(formatdate(d[0]))
+			applicable_holidays.append(formatdate(holiday_date))
 
 		if applicable_holidays:
 			frappe.throw(