fix: corrective job card creation (backport #31083) (#31084)

* test: simplify job card tests

(cherry picked from commit e625394488)

* fix: creation of corrective job card fails

This used to fail because sub_operations is a child table that's not
initalized by default till v13, in develop branch we init tables with
empty list.

(cherry picked from commit 66cf9aa344)

Co-authored-by: Ankush Menat <ankush@frappe.io>

erpnext/manufacturing/doctype/job_card/job_card.py

@@ -866,6 +866,7 @@ def make_corrective_job_card(source_name, operation=None, for_operation=None, ta
 		target.set("time_logs", [])
 		target.set("employee", [])
 		target.set("items", [])
+		target.set("sub_operations", [])
 		target.set_sub_operations()
 		target.get_required_items()
 		target.validate_time_logs()

erpnext/manufacturing/doctype/job_card/test_job_card.py

@@ -2,14 +2,20 @@
 # See license.txt
 
 import frappe
-from frappe.tests.utils import FrappeTestCase
+from frappe.tests.utils import FrappeTestCase, change_settings
 from frappe.utils import random_string
+from frappe.utils.data import add_to_date, now
 
-from erpnext.manufacturing.doctype.job_card.job_card import OperationMismatchError, OverlapError
+from erpnext.manufacturing.doctype.job_card.job_card import (
+	OperationMismatchError,
+	OverlapError,
+	make_corrective_job_card,
+)
 from erpnext.manufacturing.doctype.job_card.job_card import (
 	make_stock_entry as make_stock_entry_from_jc,
 )
 from erpnext.manufacturing.doctype.work_order.test_work_order import make_wo_order_test_record
+from erpnext.manufacturing.doctype.work_order.work_order import WorkOrder
 from erpnext.manufacturing.doctype.workstation.test_workstation import make_workstation
 from erpnext.stock.doctype.stock_entry.stock_entry_utils import make_stock_entry
 
@@ -17,34 +23,36 @@ from erpnext.stock.doctype.stock_entry.stock_entry_utils import make_stock_entry
 class TestJobCard(FrappeTestCase):
 	def setUp(self):
 		make_bom_for_jc_tests()
+		self.transfer_material_against = "Work Order"
+		self.source_warehouse = None
+		self._work_order = None
 
-		transfer_material_against, source_warehouse = None, None
+	@property
+	def work_order(self) -> WorkOrder:
+		"""Work Order lazily created for tests."""
+		if not self._work_order:
+			self._work_order = make_wo_order_test_record(
+				item="_Test FG Item 2",
+				qty=2,
+				transfer_material_against=self.transfer_material_against,
+				source_warehouse=self.source_warehouse,
+			)
+		return self._work_order
 
-		tests_that_skip_setup = ("test_job_card_material_transfer_correctness",)
+	def generate_required_stock(self, work_order: WorkOrder) -> None:
-		tests_that_transfer_against_jc = (
+		"""Create twice the stock for all required items in work order."""
-			"test_job_card_multiple_materials_transfer",
+		for item in work_order.required_items:
-			"test_job_card_excess_material_transfer",
+			make_stock_entry(
-			"test_job_card_partial_material_transfer",
+				item_code=item.item_code,
-		)
+				target=item.source_warehouse or self.source_warehouse,
-
+				qty=item.required_qty * 2,
-		if self._testMethodName in tests_that_skip_setup:
+				basic_rate=100,
-			return
+			)
-
-		if self._testMethodName in tests_that_transfer_against_jc:
-			transfer_material_against = "Job Card"
-			source_warehouse = "Stores - _TC"
-
-		self.work_order = make_wo_order_test_record(
-			item="_Test FG Item 2",
-			qty=2,
-			transfer_material_against=transfer_material_against,
-			source_warehouse=source_warehouse,
-		)
 
 	def tearDown(self):
 		frappe.db.rollback()
 
-	def test_job_card(self):
+	def test_job_card_operations(self):
 
 		job_cards = frappe.get_all(
 			"Job Card", filters={"work_order": self.work_order.name}, fields=["operation_id", "name"]
@@ -58,9 +66,6 @@ class TestJobCard(FrappeTestCase):
 			doc.operation_id = "Test Data"
 			self.assertRaises(OperationMismatchError, doc.save)
 
-		for d in job_cards:
-			frappe.delete_doc("Job Card", d.name)
-
 	def test_job_card_with_different_work_station(self):
 		job_cards = frappe.get_all(
 			"Job Card",
@@ -96,19 +101,11 @@ class TestJobCard(FrappeTestCase):
 			)
 			self.assertEqual(completed_qty, job_card.for_quantity)
 
-			doc.cancel()
-
-			for d in job_cards:
-				frappe.delete_doc("Job Card", d.name)
-
 	def test_job_card_overlap(self):
 		wo2 = make_wo_order_test_record(item="_Test FG Item 2", qty=2)
 
-		jc1_name = frappe.db.get_value("Job Card", {"work_order": self.work_order.name})
+		jc1 = frappe.get_last_doc("Job Card", {"work_order": self.work_order.name})
-		jc2_name = frappe.db.get_value("Job Card", {"work_order": wo2.name})
+		jc2 = frappe.get_last_doc("Job Card", {"work_order": wo2.name})
-
-		jc1 = frappe.get_doc("Job Card", jc1_name)
-		jc2 = frappe.get_doc("Job Card", jc2_name)
 
 		employee = "_T-Employee-00001"  # from test records
 
@@ -137,10 +134,10 @@ class TestJobCard(FrappeTestCase):
 
 	def test_job_card_multiple_materials_transfer(self):
 		"Test transferring RMs separately against Job Card with multiple RMs."
-		make_stock_entry(item_code="_Test Item", target="Stores - _TC", qty=10, basic_rate=100)
+		self.transfer_material_against = "Job Card"
-		make_stock_entry(
+		self.source_warehouse = "Stores - _TC"
-			item_code="_Test Item Home Desktop Manufactured", target="Stores - _TC", qty=6, basic_rate=100
+
-		)
+		self.generate_required_stock(self.work_order)
 
 		job_card_name = frappe.db.get_value("Job Card", {"work_order": self.work_order.name})
 		job_card = frappe.get_doc("Job Card", job_card_name)
@@ -167,22 +164,21 @@ class TestJobCard(FrappeTestCase):
 
 	def test_job_card_excess_material_transfer(self):
 		"Test transferring more than required RM against Job Card."
-		make_stock_entry(item_code="_Test Item", target="Stores - _TC", qty=25, basic_rate=100)
+		self.transfer_material_against = "Job Card"
-		make_stock_entry(
+		self.source_warehouse = "Stores - _TC"
-			item_code="_Test Item Home Desktop Manufactured", target="Stores - _TC", qty=15, basic_rate=100
-		)
 
-		job_card_name = frappe.db.get_value("Job Card", {"work_order": self.work_order.name})
+		self.generate_required_stock(self.work_order)
-		job_card = frappe.get_doc("Job Card", job_card_name)
+
+		job_card = frappe.get_last_doc("Job Card", {"work_order": self.work_order.name})
 		self.assertEqual(job_card.status, "Open")
 
 		# fully transfer both RMs
-		transfer_entry_1 = make_stock_entry_from_jc(job_card_name)
+		transfer_entry_1 = make_stock_entry_from_jc(job_card.name)
 		transfer_entry_1.insert()
 		transfer_entry_1.submit()
 
 		# transfer extra qty of both RM due to previously damaged RM
-		transfer_entry_2 = make_stock_entry_from_jc(job_card_name)
+		transfer_entry_2 = make_stock_entry_from_jc(job_card.name)
 		# deliberately change 'For Quantity'
 		transfer_entry_2.fg_completed_qty = 1
 		transfer_entry_2.items[0].qty = 5
@@ -195,7 +191,7 @@ class TestJobCard(FrappeTestCase):
 
 		# Check if 'For Quantity' is negative
 		# as 'transferred_qty' > Qty to Manufacture
-		transfer_entry_3 = make_stock_entry_from_jc(job_card_name)
+		transfer_entry_3 = make_stock_entry_from_jc(job_card.name)
 		self.assertEqual(transfer_entry_3.fg_completed_qty, 0)
 
 		job_card.append(
@@ -210,17 +206,15 @@ class TestJobCard(FrappeTestCase):
 
 	def test_job_card_partial_material_transfer(self):
 		"Test partial material transfer against Job Card"
+		self.transfer_material_against = "Job Card"
+		self.source_warehouse = "Stores - _TC"
 
-		make_stock_entry(item_code="_Test Item", target="Stores - _TC", qty=25, basic_rate=100)
+		self.generate_required_stock(self.work_order)
-		make_stock_entry(
-			item_code="_Test Item Home Desktop Manufactured", target="Stores - _TC", qty=15, basic_rate=100
-		)
 
-		job_card_name = frappe.db.get_value("Job Card", {"work_order": self.work_order.name})
+		job_card = frappe.get_last_doc("Job Card", {"work_order": self.work_order.name})
-		job_card = frappe.get_doc("Job Card", job_card_name)
 
 		# partially transfer
-		transfer_entry = make_stock_entry_from_jc(job_card_name)
+		transfer_entry = make_stock_entry_from_jc(job_card.name)
 		transfer_entry.fg_completed_qty = 1
 		transfer_entry.get_items()
 		transfer_entry.insert()
@@ -232,7 +226,7 @@ class TestJobCard(FrappeTestCase):
 		self.assertEqual(transfer_entry.items[1].qty, 3)
 
 		# transfer remaining
-		transfer_entry_2 = make_stock_entry_from_jc(job_card_name)
+		transfer_entry_2 = make_stock_entry_from_jc(job_card.name)
 
 		self.assertEqual(transfer_entry_2.fg_completed_qty, 1)
 		self.assertEqual(transfer_entry_2.items[0].qty, 5)
@@ -277,7 +271,49 @@ class TestJobCard(FrappeTestCase):
 		self.assertEqual(transfer_entry.items[0].item_code, "_Test Item")
 		self.assertEqual(transfer_entry.items[0].qty, 2)
 
-		# rollback via tearDown method
+	@change_settings(
+		"Manufacturing Settings", {"add_corrective_operation_cost_in_finished_good_valuation": 1}
+	)
+	def test_corrective_costing(self):
+		job_card = frappe.get_last_doc("Job Card", {"work_order": self.work_order.name})
+
+		job_card.append(
+			"time_logs",
+			{"from_time": now(), "to_time": add_to_date(now(), hours=1), "completed_qty": 2},
+		)
+		job_card.submit()
+
+		self.work_order.reload()
+		original_cost = self.work_order.total_operating_cost
+
+		# Create a corrective operation against it
+		corrective_action = frappe.get_doc(
+			doctype="Operation", is_corrective_operation=1, name=frappe.generate_hash()
+		).insert()
+
+		corrective_job_card = make_corrective_job_card(
+			job_card.name, operation=corrective_action.name, for_operation=job_card.operation
+		)
+		corrective_job_card.hour_rate = 100
+		corrective_job_card.insert()
+		corrective_job_card.append(
+			"time_logs",
+			{
+				"from_time": add_to_date(now(), hours=2),
+				"to_time": add_to_date(now(), hours=2, minutes=30),
+				"completed_qty": 2,
+			},
+		)
+		corrective_job_card.submit()
+
+		self.work_order.reload()
+		cost_after_correction = self.work_order.total_operating_cost
+		self.assertGreater(cost_after_correction, original_cost)
+
+		corrective_job_card.cancel()
+		self.work_order.reload()
+		cost_after_cancel = self.work_order.total_operating_cost
+		self.assertEqual(cost_after_cancel, original_cost)
 
 
 def create_bom_with_multiple_operations():