fix: error handling and messages

- remove savepoints since submission should stop if any error occurs

- refactor variable naming and msgprints

- test Salary Slip creation failure

- fix(test): explicitly commit after payroll entry creation so that the first salary slip creation failure does not rollback the Payroll Entry insert

erpnext/payroll/doctype/payroll_entry/payroll_entry.py

@@ -54,7 +54,7 @@ class PayrollEntry(Document):
 			if self.validate_employee_attendance():
 				frappe.throw(_("Cannot Submit, Employees left to mark attendance"))
 
-	def set_status(self, status=None, update=True):
+	def set_status(self, status=None, update=False):
 		if not status:
 			status = {0: "Draft", 1: "Submitted", 2: "Cancelled"}[self.docstatus or 0]
 
@@ -850,7 +850,6 @@ def log_payroll_failure(process, payroll_entry, error):
 
 def create_salary_slips_for_employees(employees, args, publish_progress=True):
 	try:
-		frappe.db.savepoint("salary_slip_creation")
 		payroll_entry = frappe.get_doc("Payroll Entry", args.payroll_entry)
 		salary_slips_exist_for = get_existing_salary_slips(employees, args)
 		count = 0
@@ -879,7 +878,7 @@ def create_salary_slips_for_employees(employees, args, publish_progress=True):
 			)
 
 	except Exception as e:
-		frappe.db.rollback(save_point="salary_slip_creation")
+		frappe.db.rollback()
 		log_payroll_failure("creation", payroll_entry, e)
 
 	finally:
@@ -887,24 +886,25 @@ def create_salary_slips_for_employees(employees, args, publish_progress=True):
 		frappe.publish_realtime("completed_salary_slip_creation")
 
 
-def show_payroll_submission_status(submitted, not_submitted, salary_slip):
+def show_payroll_submission_status(submitted, unsubmitted, payroll_entry):
-	if not submitted and not not_submitted:
+	if not submitted and not unsubmitted:
 		frappe.msgprint(
 			_(
 				"No salary slip found to submit for the above selected criteria OR salary slip already submitted"
 			)
 		)
-		return
+	elif submitted and not unsubmitted:
-
-	if submitted:
 		frappe.msgprint(
-			_("Salary Slip submitted for period from {0} to {1}").format(
+			_("Salary Slips submitted for period from {0} to {1}").format(
-				salary_slip.start_date, salary_slip.end_date
+				payroll_entry.start_date, payroll_entry.end_date
+			)
+		)
+	elif unsubmitted:
+		frappe.msgprint(
+			_("Could not submit some Salary Slips: {}").format(
+				", ".join(get_link_to_form("Salary Slip", entry) for entry in unsubmitted)
 			)
 		)
-
-	if not_submitted:
-		frappe.msgprint(_("Could not submit some Salary Slips"))
 
 
 def get_existing_salary_slips(employees, args):
@@ -922,23 +922,21 @@ def get_existing_salary_slips(employees, args):
 
 def submit_salary_slips_for_employees(payroll_entry, salary_slips, publish_progress=True):
 	try:
-		frappe.db.savepoint("salary_slip_submission")
-
 		submitted = []
-		not_submitted = []
+		unsubmitted = []
 		frappe.flags.via_payroll_entry = True
 		count = 0
 
 		for entry in salary_slips:
 			salary_slip = frappe.get_doc("Salary Slip", entry[0])
 			if salary_slip.net_pay < 0:
-				not_submitted.append(entry[0])
+				unsubmitted.append(entry[0])
 			else:
 				try:
 					salary_slip.submit()
 					submitted.append(salary_slip)
 				except frappe.ValidationError:
-					not_submitted.append(entry[0])
+					unsubmitted.append(entry[0])
 
 			count += 1
 			if publish_progress:
@@ -949,10 +947,10 @@ def submit_salary_slips_for_employees(payroll_entry, salary_slips, publish_progr
 			payroll_entry.email_salary_slip(submitted)
 			payroll_entry.db_set({"salary_slips_submitted": 1, "status": "Submitted", "error_message": ""})
 
-		show_payroll_submission_status(submitted, not_submitted, salary_slip)
+		show_payroll_submission_status(submitted, unsubmitted, payroll_entry)
 
 	except Exception as e:
-		frappe.db.rollback(save_point="salary_slip_submission")
+		frappe.db.rollback()
 		log_payroll_failure("submission", payroll_entry, e)
 
 	finally:

erpnext/payroll/doctype/payroll_entry/test_payroll_entry.py

@@ -299,7 +299,7 @@ class TestPayrollEntry(FrappeTestCase):
 		# enqueue salary slip creation via payroll entry
 		# Payroll Entry status should change to Queued
 		dates = get_start_end_dates("Monthly", nowdate())
-		payroll_entry = get_payroll_entry_data(
+		payroll_entry = get_payroll_entry(
 			start_date=dates.start_date,
 			end_date=dates.end_date,
 			payable_account=company_doc.default_payroll_payable_account,
@@ -308,7 +308,7 @@ class TestPayrollEntry(FrappeTestCase):
 			cost_center="Main - _TC",
 		)
 		frappe.flags.enqueue_payroll_entry = True
-		payroll_entry.create_salary_slips()
+		payroll_entry.submit()
 		payroll_entry.reload()
 
 		self.assertEqual(payroll_entry.status, "Queued")
@@ -335,7 +335,7 @@ class TestPayrollEntry(FrappeTestCase):
 		# salary slip submission via payroll entry
 		# Payroll Entry status should change to Failed because of the missing account setup
 		dates = get_start_end_dates("Monthly", nowdate())
-		payroll_entry = get_payroll_entry_data(
+		payroll_entry = get_payroll_entry(
 			start_date=dates.start_date,
 			end_date=dates.end_date,
 			payable_account=company_doc.default_payroll_payable_account,
@@ -343,7 +343,16 @@ class TestPayrollEntry(FrappeTestCase):
 			company=company_doc.name,
 			cost_center="Main - _TC",
 		)
-		payroll_entry.create_salary_slips()
+
+		# set employee as Inactive to check creation failure
+		frappe.db.set_value("Employee", employee, "status", "Inactive")
+		payroll_entry.submit()
+		payroll_entry.reload()
+		self.assertEqual(payroll_entry.status, "Failed")
+		self.assertIsNotNone(payroll_entry.error_message)
+
+		frappe.db.set_value("Employee", employee, "status", "Active")
+		payroll_entry.submit()
 		payroll_entry.submit_salary_slips()
 
 		payroll_entry.reload()
@@ -369,7 +378,7 @@ class TestPayrollEntry(FrappeTestCase):
 		setup_salary_structure(employee, company_doc)
 
 		dates = get_start_end_dates("Monthly", nowdate())
-		payroll_entry = get_payroll_entry_data(
+		payroll_entry = get_payroll_entry(
 			start_date=dates.start_date,
 			end_date=dates.end_date,
 			payable_account=company_doc.default_payroll_payable_account,
@@ -384,7 +393,7 @@ class TestPayrollEntry(FrappeTestCase):
 		self.assertEqual(payroll_entry.status, "Cancelled")
 
 
-def get_payroll_entry_data(**args):
+def get_payroll_entry(**args):
 	args = frappe._dict(args)
 
 	payroll_entry = frappe.new_doc("Payroll Entry")
@@ -407,13 +416,16 @@ def get_payroll_entry_data(**args):
 		payroll_entry.payment_account = args.payment_account
 
 	payroll_entry.fill_employee_details()
-	payroll_entry.save()
+	payroll_entry.insert()
+
+	# Commit so that the first salary slip creation failure does not rollback the Payroll Entry insert.
+	frappe.db.commit()  # nosemgrep
 
 	return payroll_entry
 
 
 def make_payroll_entry(**args):
-	payroll_entry = get_payroll_entry_data(**args)
+	payroll_entry = get_payroll_entry(**args)
 	payroll_entry.submit()
 	payroll_entry.submit_salary_slips()
 	if payroll_entry.get_sal_slip_list(ss_status=1):