From d641f260352d5aff7ec77def19bb318dca4a5054 Mon Sep 17 00:00:00 2001 From: Rucha Mahabal Date: Thu, 2 Jun 2022 15:08:49 +0530 Subject: [PATCH] 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 --- .../doctype/payroll_entry/payroll_entry.py | 38 +++++++++---------- .../payroll_entry/test_payroll_entry.py | 28 ++++++++++---- 2 files changed, 38 insertions(+), 28 deletions(-) diff --git a/erpnext/payroll/doctype/payroll_entry/payroll_entry.py b/erpnext/payroll/doctype/payroll_entry/payroll_entry.py index edcada1451..620fcadceb 100644 --- a/erpnext/payroll/doctype/payroll_entry/payroll_entry.py +++ b/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): - if not submitted and not not_submitted: +def show_payroll_submission_status(submitted, unsubmitted, payroll_entry): + 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 - - if submitted: + elif submitted and not unsubmitted: frappe.msgprint( - _("Salary Slip submitted for period from {0} to {1}").format( - salary_slip.start_date, salary_slip.end_date + _("Salary Slips submitted for period from {0} to {1}").format( + 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: diff --git a/erpnext/payroll/doctype/payroll_entry/test_payroll_entry.py b/erpnext/payroll/doctype/payroll_entry/test_payroll_entry.py index 84f1575381..0363a0c3de 100644 --- a/erpnext/payroll/doctype/payroll_entry/test_payroll_entry.py +++ b/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):