Merge pull request #56139 from nabinhait/refactor-si-timesheet-billing

refactor(sales_invoice): simplify TimesheetBillingService link decision

erpnext/accounts/doctype/sales_invoice/services/timesheet_billing.py

@@ -99,23 +99,24 @@ class TimesheetBillingService:
 		doc.total_billing_hours = sum(flt(ts.billing_hours) for ts in doc.timesheets)
 
 	def _update_time_sheet_detail(self, timesheet, args, sales_invoice: str | None) -> None:
-		doc = self.doc
 		for data in timesheet.time_logs:
-			if (
-				(doc.project and args.timesheet_detail == data.name)
-				or (not doc.project and not data.sales_invoice and args.timesheet_detail == data.name)
-				or (
-					not sales_invoice
-					and data.sales_invoice == doc.name
-					and args.timesheet_detail == data.name
-				)
-				or (
-					doc.is_return
-					and doc.return_against
-					and data.sales_invoice
-					and data.sales_invoice == doc.return_against
-					and not sales_invoice
-					and args.timesheet_detail == data.name
-				)
-			):
+			if args.timesheet_detail == data.name and self._should_set_sales_invoice(data, sales_invoice):
 				data.sales_invoice = sales_invoice
+
+	def _should_set_sales_invoice(self, time_log, sales_invoice: str | None) -> bool:
+		"""Whether this time log's sales-invoice link should be (re)set to sales_invoice."""
+		doc = self.doc
+		if doc.project:
+			return True
+		if not time_log.sales_invoice:
+			return True
+		if not sales_invoice and time_log.sales_invoice == doc.name:
+			# clearing the link on cancellation of this invoice
+			return True
+		# clearing the link on a return raised against the original invoice
+		return bool(
+			doc.is_return
+			and doc.return_against
+			and not sales_invoice
+			and time_log.sales_invoice == doc.return_against
+		)

erpnext/projects/doctype/timesheet/test_timesheet.py

@@ -126,6 +126,80 @@ class TestTimesheet(ERPNextTestSuite):
 		self.assertEqual(ts.per_billed, 100)
 		self.assertEqual(ts.time_logs[0].sales_invoice, sales_invoice.name)
 
+	def _bill_timesheet_into_invoice(self, emp):
+		"""Submit a billable timesheet into a Sales Invoice; return (timesheet, invoice)."""
+		timesheet = make_timesheet(emp, simulate=True, is_billable=1)
+		sales_invoice = make_sales_invoice(timesheet.name, "_Test Item", "_Test Customer", currency="INR")
+		sales_invoice.due_date = nowdate()
+		sales_invoice.submit()
+		timesheet.reload()
+		# Submitting links the timesheet detail to the invoice and marks it billed
+		self.assertEqual(timesheet.time_logs[0].sales_invoice, sales_invoice.name)
+		self.assertEqual(timesheet.status, "Billed")
+		return timesheet, sales_invoice
+
+	def test_timesheet_billing_link_lifecycle(self):
+		emp = make_employee("test_employee_6@salary.com", company="_Test Company")
+
+		with self.subTest("link released on cancel"):
+			timesheet, sales_invoice = self._bill_timesheet_into_invoice(emp)
+			sales_invoice.reload()
+			sales_invoice.cancel()
+			timesheet.reload()
+			self.assertFalse(timesheet.time_logs[0].sales_invoice)
+			self.assertNotEqual(timesheet.status, "Billed")
+
+		with self.subTest("link released on sales return"):
+			timesheet, sales_invoice = self._bill_timesheet_into_invoice(emp)
+			sales_return = make_sales_return(sales_invoice.name)
+			sales_return.insert()
+			sales_return.submit()
+			timesheet.reload()
+			self.assertFalse(timesheet.time_logs[0].sales_invoice)
+
+	def test_timesheet_billing_validations(self):
+		emp = make_employee("test_employee_6@salary.com", company="_Test Company")
+
+		with self.subTest("unsubmitted timesheet is rejected"):
+			draft = make_timesheet(emp, simulate=True, is_billable=1, do_not_submit=True)
+			sales_invoice = self._invoice_with_timesheet_row(draft.name, draft.time_logs[0].name)
+			self.assertRaises(frappe.ValidationError, sales_invoice.save)
+
+		with self.subTest("already invoiced detail is rejected"):
+			timesheet, _ = self._bill_timesheet_into_invoice(emp)
+			sales_invoice = self._invoice_with_timesheet_row(timesheet.name, timesheet.time_logs[0].name)
+			self.assertRaises(frappe.ValidationError, sales_invoice.save)
+
+	@ERPNextTestSuite.change_settings("Projects Settings", {"fetch_timesheet_in_sales_invoice": 1})
+	def test_timesheet_billing_data_population(self):
+		emp = make_employee("test_employee_6@salary.com", company="_Test Company")
+
+		with self.subTest("blank hours/amount are back-filled from the timesheet"):
+			timesheet = make_timesheet(emp, simulate=True, is_billable=1)
+			sales_invoice = self._invoice_with_timesheet_row(
+				timesheet.name, timesheet.time_logs[0].name, with_amounts=False
+			)
+			sales_invoice.save()
+			self.assertEqual(sales_invoice.timesheets[0].billing_hours, 2)
+			self.assertEqual(sales_invoice.timesheets[0].billing_amount, 100)
+
+		with self.subTest("project invoice auto-fetches the project's timesheets"):
+			project = frappe.get_value("Project", {"project_name": "_Test Project"})
+			make_timesheet(emp, simulate=True, is_billable=1, project=project, company="_Test Company")
+			sales_invoice = create_sales_invoice(do_not_save=True)
+			sales_invoice.project = project
+			sales_invoice.set("timesheets", [])
+			sales_invoice.save()
+			self.assertTrue(sales_invoice.timesheets)
+
+	def _invoice_with_timesheet_row(self, time_sheet, timesheet_detail, with_amounts=True):
+		sales_invoice = create_sales_invoice(do_not_save=True)
+		row = {"time_sheet": time_sheet, "timesheet_detail": timesheet_detail}
+		if with_amounts:
+			row.update({"billing_hours": 2, "billing_amount": 100})
+		sales_invoice.append("timesheets", row)
+		return sales_invoice
+
 	def test_timesheet_time_overlap(self):
 		emp = make_employee("test_employee_6@salary.com", company="_Test Company")