From 88c5de533a862429c5fc9e1b5fa2cb36cda79235 Mon Sep 17 00:00:00 2001 From: Rucha Mahabal Date: Fri, 10 Mar 2023 15:32:30 +0530 Subject: [PATCH 1/7] fix: exclude carry forwarding leaves while updating leaves after submission --- erpnext/hr/doctype/leave_allocation/leave_allocation.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/erpnext/hr/doctype/leave_allocation/leave_allocation.py b/erpnext/hr/doctype/leave_allocation/leave_allocation.py index 8fae2a9a888..49ece5a2341 100755 --- a/erpnext/hr/doctype/leave_allocation/leave_allocation.py +++ b/erpnext/hr/doctype/leave_allocation/leave_allocation.py @@ -99,7 +99,11 @@ class LeaveAllocation(Document): # run required validations again since total leaves are being updated self.validate_leave_days_and_dates() - leaves_to_be_added = self.new_leaves_allocated - self.get_existing_leave_count() + leaves_to_be_added = ( + frappe.db.get_value("Leave Allocation", self.name, "new_leaves_allocated") + - self.get_existing_leave_count() + ) + args = { "leaves": leaves_to_be_added, "from_date": self.from_date, @@ -118,6 +122,7 @@ class LeaveAllocation(Document): "employee": self.employee, "company": self.company, "leave_type": self.leave_type, + "is_carry_forward": 0, }, pluck="leaves", ) From fc10c8e44ee4307f5dcae8260db21c186705e3c1 Mon Sep 17 00:00:00 2001 From: Rucha Mahabal Date: Fri, 10 Mar 2023 15:33:12 +0530 Subject: [PATCH 2/7] test: leaves updated after submission with carry forwarding --- .../leave_allocation/test_leave_allocation.py | 89 +++++++++++++++---- 1 file changed, 71 insertions(+), 18 deletions(-) diff --git a/erpnext/hr/doctype/leave_allocation/test_leave_allocation.py b/erpnext/hr/doctype/leave_allocation/test_leave_allocation.py index 48953003000..d9482a596cd 100644 --- a/erpnext/hr/doctype/leave_allocation/test_leave_allocation.py +++ b/erpnext/hr/doctype/leave_allocation/test_leave_allocation.py @@ -69,7 +69,6 @@ class TestLeaveAllocation(FrappeTestCase): def test_validation_for_over_allocation(self): leave_type = create_leave_type(leave_type_name="Test Over Allocation", is_carry_forward=1) - leave_type.save() doc = frappe.get_doc( { @@ -137,9 +136,9 @@ class TestLeaveAllocation(FrappeTestCase): ) ).insert() - leave_type = create_leave_type(leave_type_name="_Test Allocation Validation", is_carry_forward=1) - leave_type.max_leaves_allowed = 25 - leave_type.save() + leave_type = create_leave_type( + leave_type_name="_Test Allocation Validation", is_carry_forward=1, max_leaves_allowed=25 + ) # 15 leaves allocated in this period allocation = create_leave_allocation( @@ -174,9 +173,9 @@ class TestLeaveAllocation(FrappeTestCase): ) ).insert() - leave_type = create_leave_type(leave_type_name="_Test Allocation Validation", is_carry_forward=1) - leave_type.max_leaves_allowed = 30 - leave_type.save() + leave_type = create_leave_type( + leave_type_name="_Test Allocation Validation", is_carry_forward=1, max_leaves_allowed=30 + ) # 15 leaves allocated allocation = create_leave_allocation( @@ -207,7 +206,6 @@ class TestLeaveAllocation(FrappeTestCase): def test_validate_back_dated_allocation_update(self): leave_type = create_leave_type(leave_type_name="_Test_CF_leave", is_carry_forward=1) - leave_type.save() # initial leave allocation = 15 leave_allocation = create_leave_allocation( @@ -235,10 +233,12 @@ class TestLeaveAllocation(FrappeTestCase): self.assertRaises(BackDatedAllocationError, leave_allocation.save) def test_carry_forward_calculation(self): - leave_type = create_leave_type(leave_type_name="_Test_CF_leave", is_carry_forward=1) - leave_type.maximum_carry_forwarded_leaves = 10 - leave_type.max_leaves_allowed = 30 - leave_type.save() + leave_type = create_leave_type( + leave_type_name="_Test_CF_leave", + is_carry_forward=1, + maximum_carry_forwarded_leaves=10, + max_leaves_allowed=30, + ) # initial leave allocation = 15 leave_allocation = create_leave_allocation( @@ -286,7 +286,6 @@ class TestLeaveAllocation(FrappeTestCase): is_carry_forward=1, expire_carry_forwarded_leaves_after_days=90, ) - leave_type.save() # initial leave allocation leave_allocation = create_leave_allocation( @@ -352,12 +351,40 @@ class TestLeaveAllocation(FrappeTestCase): ) leave_allocation.submit() leave_allocation.reload() - self.assertTrue(leave_allocation.total_leaves_allocated, 15) + self.assertEqual(leave_allocation.total_leaves_allocated, 15) leave_allocation.new_leaves_allocated = 40 - leave_allocation.submit() + leave_allocation.save() leave_allocation.reload() - self.assertTrue(leave_allocation.total_leaves_allocated, 40) + self.assertEqual(leave_allocation.total_leaves_allocated, 40) + + def test_leave_addition_after_submit_with_carry_forward(self): + from hrms.hr.doctype.leave_application.test_leave_application import ( + create_carry_forwarded_allocation, + ) + + leave_type = create_leave_type( + leave_type_name="_Test_CF_leave_expiry", + is_carry_forward=1, + include_holiday=True, + ) + + leave_allocation = create_carry_forwarded_allocation(self.employee, leave_type) + + self.assertEqual(leave_allocation.total_leaves_allocated, 30) + + leave_allocation.new_leaves_allocated = 32 + leave_allocation.save() + leave_allocation.reload() + + updated_entry = frappe.db.get_all( + "Leave Ledger Entry", + {"transaction_name": leave_allocation.name}, + pluck="leaves", + order_by="creation desc", + limit=1, + ) + self.assertEqual(updated_entry[0], 17) def test_leave_subtraction_after_submit(self): leave_allocation = create_leave_allocation( @@ -365,12 +392,38 @@ class TestLeaveAllocation(FrappeTestCase): ) leave_allocation.submit() leave_allocation.reload() - self.assertTrue(leave_allocation.total_leaves_allocated, 15) + self.assertEqual(leave_allocation.total_leaves_allocated, 15) leave_allocation.new_leaves_allocated = 10 leave_allocation.submit() leave_allocation.reload() - self.assertTrue(leave_allocation.total_leaves_allocated, 10) + self.assertEqual(leave_allocation.total_leaves_allocated, 10) + + def test_leave_subtraction_after_submit_with_carry_forward(self): + from hrms.hr.doctype.leave_application.test_leave_application import ( + create_carry_forwarded_allocation, + ) + + leave_type = create_leave_type( + leave_type_name="_Test_CF_leave_expiry", + is_carry_forward=1, + include_holiday=True, + ) + + leave_allocation = create_carry_forwarded_allocation(self.employee, leave_type) + self.assertEqual(leave_allocation.total_leaves_allocated, 30) + + leave_allocation.new_leaves_allocated = 8 + leave_allocation.save() + + updated_entry = frappe.db.get_all( + "Leave Ledger Entry", + {"transaction_name": leave_allocation.name}, + pluck="leaves", + order_by="creation desc", + limit=1, + ) + self.assertEqual(updated_entry[0], -7) def test_validation_against_leave_application_after_submit(self): from erpnext.payroll.doctype.salary_slip.test_salary_slip import make_holiday_list From 7b9784ce10a4c976fc0300687145f2c6650da2af Mon Sep 17 00:00:00 2001 From: Rucha Mahabal Date: Fri, 10 Mar 2023 15:33:35 +0530 Subject: [PATCH 3/7] refactor(tests): `create_leave_type` usage --- .../leave_application/test_leave_application.py | 15 ++++++--------- erpnext/hr/doctype/leave_type/test_leave_type.py | 7 ++++++- .../test_employee_leave_balance.py | 1 - .../doctype/salary_slip/test_salary_slip.py | 1 - 4 files changed, 12 insertions(+), 12 deletions(-) diff --git a/erpnext/hr/doctype/leave_application/test_leave_application.py b/erpnext/hr/doctype/leave_application/test_leave_application.py index 231b14f6163..e30b84bbf34 100644 --- a/erpnext/hr/doctype/leave_application/test_leave_application.py +++ b/erpnext/hr/doctype/leave_application/test_leave_application.py @@ -702,7 +702,7 @@ class TestLeaveApplication(unittest.TestCase): leave_type_name="_Test_CF_leave_expiry", is_carry_forward=1, expire_carry_forwarded_leaves_after_days=90, - ).insert() + ) create_carry_forwarded_allocation(employee, leave_type) details = get_leave_balance_on( @@ -774,7 +774,6 @@ class TestLeaveApplication(unittest.TestCase): employee = get_employee() leave_type = create_leave_type(leave_type_name="Test Leave Type 1") - leave_type.save() leave_allocation = create_leave_allocation( employee=employee.name, employee_name=employee.employee_name, leave_type=leave_type.name @@ -817,7 +816,6 @@ class TestLeaveApplication(unittest.TestCase): expire_carry_forwarded_leaves_after_days=90, include_holiday=True, ) - leave_type.submit() create_carry_forwarded_allocation(employee, leave_type) @@ -856,7 +854,6 @@ class TestLeaveApplication(unittest.TestCase): is_carry_forward=1, expire_carry_forwarded_leaves_after_days=90, ) - leave_type.submit() create_carry_forwarded_allocation(employee, leave_type) @@ -1005,7 +1002,7 @@ class TestLeaveApplication(unittest.TestCase): leave_type_name="_Test_CF_leave_expiry", is_carry_forward=1, expire_carry_forwarded_leaves_after_days=90, - ).insert() + ) leave_alloc = create_carry_forwarded_allocation(employee, leave_type) cf_expiry = frappe.db.get_value( @@ -1038,7 +1035,7 @@ class TestLeaveApplication(unittest.TestCase): leave_type_name="_Test_CF_leave_expiry", is_carry_forward=1, expire_carry_forwarded_leaves_after_days=90, - ).insert() + ) leave_alloc = create_carry_forwarded_allocation(employee, leave_type) cf_expiry = frappe.db.get_value( @@ -1072,7 +1069,7 @@ class TestLeaveApplication(unittest.TestCase): leave_type_name="_Test_CF_leave_expiry", is_carry_forward=1, expire_carry_forwarded_leaves_after_days=90, - ).insert() + ) leave_alloc = create_carry_forwarded_allocation(employee, leave_type) cf_expiry = frappe.db.get_value( @@ -1114,7 +1111,7 @@ class TestLeaveApplication(unittest.TestCase): leave_type_name="_Test_CF_leave_expiry", is_carry_forward=1, expire_carry_forwarded_leaves_after_days=90, - ).insert() + ) leave_alloc = create_carry_forwarded_allocation(employee, leave_type) cf_expiry = frappe.db.get_value( @@ -1148,7 +1145,7 @@ class TestLeaveApplication(unittest.TestCase): leave_type_name="_Test_CF_leave_expiry", is_carry_forward=1, expire_carry_forwarded_leaves_after_days=90, - ).insert() + ) leave_alloc = create_carry_forwarded_allocation(employee, leave_type) cf_expiry = frappe.db.get_value( diff --git a/erpnext/hr/doctype/leave_type/test_leave_type.py b/erpnext/hr/doctype/leave_type/test_leave_type.py index 69f9e125203..628f16d0654 100644 --- a/erpnext/hr/doctype/leave_type/test_leave_type.py +++ b/erpnext/hr/doctype/leave_type/test_leave_type.py @@ -9,7 +9,8 @@ test_records = frappe.get_test_records("Leave Type") def create_leave_type(**args): args = frappe._dict(args) if frappe.db.exists("Leave Type", args.leave_type_name): - return frappe.get_doc("Leave Type", args.leave_type_name) + frappe.delete_doc("Leave Type", args.leave_type_name) + leave_type = frappe.get_doc( { "doctype": "Leave Type", @@ -23,10 +24,14 @@ def create_leave_type(**args): "expire_carry_forwarded_leaves_after_days": args.expire_carry_forwarded_leaves_after_days or 0, "encashment_threshold_days": args.encashment_threshold_days or 5, "earning_component": "Leave Encashment", + "max_leaves_allowed": args.max_leaves_allowed, + "maximum_carry_forwarded_leaves": args.maximum_carry_forwarded_leaves, } ) if leave_type.is_ppl: leave_type.fraction_of_daily_salary_per_leave = args.fraction_of_daily_salary_per_leave or 0.5 + leave_type.insert() + return leave_type diff --git a/erpnext/hr/report/employee_leave_balance/test_employee_leave_balance.py b/erpnext/hr/report/employee_leave_balance/test_employee_leave_balance.py index d167d1d86c9..75cb4991299 100644 --- a/erpnext/hr/report/employee_leave_balance/test_employee_leave_balance.py +++ b/erpnext/hr/report/employee_leave_balance/test_employee_leave_balance.py @@ -154,7 +154,6 @@ class TestEmployeeLeaveBalance(unittest.TestCase): @set_holiday_list("_Test Emp Balance Holiday List", "_Test Company") def test_opening_balance_considers_carry_forwarded_leaves(self): leave_type = create_leave_type(leave_type_name="_Test_CF_leave_expiry", is_carry_forward=1) - leave_type.insert() # 30 leaves allocated for first half of the year allocation1 = make_allocation_record( diff --git a/erpnext/payroll/doctype/salary_slip/test_salary_slip.py b/erpnext/payroll/doctype/salary_slip/test_salary_slip.py index 69cfce6badc..5ad549fea28 100644 --- a/erpnext/payroll/doctype/salary_slip/test_salary_slip.py +++ b/erpnext/payroll/doctype/salary_slip/test_salary_slip.py @@ -267,7 +267,6 @@ class TestSalarySlip(FrappeTestCase): make_leave_application(emp_id, first_sunday, add_days(first_sunday, 3), "Leave Without Pay") leave_type_ppl = create_leave_type(leave_type_name="Test Partially Paid Leave", is_ppl=1) - leave_type_ppl.save() alloc = create_leave_allocation( employee=emp_id, From 91cad9e985982aa1b86d14a8f554c996fccaa9ae Mon Sep 17 00:00:00 2001 From: Rucha Mahabal Date: Fri, 10 Mar 2023 15:48:51 +0530 Subject: [PATCH 4/7] fix: exclude cancelled leave ledger entries --- .../hr/doctype/leave_allocation/leave_allocation.py | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/erpnext/hr/doctype/leave_allocation/leave_allocation.py b/erpnext/hr/doctype/leave_allocation/leave_allocation.py index 49ece5a2341..c5cf3b43a62 100755 --- a/erpnext/hr/doctype/leave_allocation/leave_allocation.py +++ b/erpnext/hr/doctype/leave_allocation/leave_allocation.py @@ -99,7 +99,7 @@ class LeaveAllocation(Document): # run required validations again since total leaves are being updated self.validate_leave_days_and_dates() - leaves_to_be_added = ( + leaves_to_be_added = flt( frappe.db.get_value("Leave Allocation", self.name, "new_leaves_allocated") - self.get_existing_leave_count() ) @@ -123,14 +123,12 @@ class LeaveAllocation(Document): "company": self.company, "leave_type": self.leave_type, "is_carry_forward": 0, + "docstatus": 1, }, - pluck="leaves", + fields=["SUM(leaves) as total_leaves"], ) - total_existing_leaves = 0 - for entry in ledger_entries: - total_existing_leaves += entry - return total_existing_leaves + return ledger_entries[0].total_leaves if ledger_entries else 0 def validate_against_leave_applications(self): leaves_taken = get_approved_leaves_for_period( From 238769e6b51b5decac6ab3bbf5747211e757982d Mon Sep 17 00:00:00 2001 From: Rucha Mahabal Date: Tue, 14 Mar 2023 10:51:35 +0530 Subject: [PATCH 5/7] fix: precision for newly allocated leaves --- erpnext/hr/doctype/leave_allocation/leave_allocation.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/erpnext/hr/doctype/leave_allocation/leave_allocation.py b/erpnext/hr/doctype/leave_allocation/leave_allocation.py index c5cf3b43a62..6f289b405f3 100755 --- a/erpnext/hr/doctype/leave_allocation/leave_allocation.py +++ b/erpnext/hr/doctype/leave_allocation/leave_allocation.py @@ -100,8 +100,8 @@ class LeaveAllocation(Document): self.validate_leave_days_and_dates() leaves_to_be_added = flt( - frappe.db.get_value("Leave Allocation", self.name, "new_leaves_allocated") - - self.get_existing_leave_count() + (self.new_leaves_allocated - self.get_existing_leave_count()), + self.precision("new_leaves_allocated"), ) args = { From cdf73bb7818132643dd0872f7f67638014f6bf58 Mon Sep 17 00:00:00 2001 From: Rucha Mahabal Date: Tue, 14 Mar 2023 11:15:38 +0530 Subject: [PATCH 6/7] test: update for total leaves allocated post submission --- .../leave_allocation/leave_allocation.py | 1 + .../leave_allocation/test_leave_allocation.py | 24 ++++++++++++++++++- 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/erpnext/hr/doctype/leave_allocation/leave_allocation.py b/erpnext/hr/doctype/leave_allocation/leave_allocation.py index 6f289b405f3..09484d33755 100755 --- a/erpnext/hr/doctype/leave_allocation/leave_allocation.py +++ b/erpnext/hr/doctype/leave_allocation/leave_allocation.py @@ -90,6 +90,7 @@ class LeaveAllocation(Document): if self.carry_forward: self.set_carry_forwarded_leaves_in_previous_allocation(on_cancel=True) + # nosemgrep: frappe-semgrep-rules.rules.frappe-modifying-but-not-comitting def on_update_after_submit(self): if self.has_value_changed("new_leaves_allocated"): self.validate_against_leave_applications() diff --git a/erpnext/hr/doctype/leave_allocation/test_leave_allocation.py b/erpnext/hr/doctype/leave_allocation/test_leave_allocation.py index d9482a596cd..3a234b6fa97 100644 --- a/erpnext/hr/doctype/leave_allocation/test_leave_allocation.py +++ b/erpnext/hr/doctype/leave_allocation/test_leave_allocation.py @@ -356,6 +356,16 @@ class TestLeaveAllocation(FrappeTestCase): leave_allocation.new_leaves_allocated = 40 leave_allocation.save() leave_allocation.reload() + + updated_entry = frappe.db.get_all( + "Leave Ledger Entry", + {"transaction_name": leave_allocation.name}, + pluck="leaves", + order_by="creation desc", + limit=1, + ) + + self.assertEqual(updated_entry[0], 25) self.assertEqual(leave_allocation.total_leaves_allocated, 40) def test_leave_addition_after_submit_with_carry_forward(self): @@ -370,7 +380,7 @@ class TestLeaveAllocation(FrappeTestCase): ) leave_allocation = create_carry_forwarded_allocation(self.employee, leave_type) - + # 15 new leaves, 15 carry forwarded leaves self.assertEqual(leave_allocation.total_leaves_allocated, 30) leave_allocation.new_leaves_allocated = 32 @@ -385,6 +395,7 @@ class TestLeaveAllocation(FrappeTestCase): limit=1, ) self.assertEqual(updated_entry[0], 17) + self.assertEqual(leave_allocation.total_leaves_allocated, 47) def test_leave_subtraction_after_submit(self): leave_allocation = create_leave_allocation( @@ -397,6 +408,16 @@ class TestLeaveAllocation(FrappeTestCase): leave_allocation.new_leaves_allocated = 10 leave_allocation.submit() leave_allocation.reload() + + updated_entry = frappe.db.get_all( + "Leave Ledger Entry", + {"transaction_name": leave_allocation.name}, + pluck="leaves", + order_by="creation desc", + limit=1, + ) + + self.assertEqual(updated_entry[0], -5) self.assertEqual(leave_allocation.total_leaves_allocated, 10) def test_leave_subtraction_after_submit_with_carry_forward(self): @@ -424,6 +445,7 @@ class TestLeaveAllocation(FrappeTestCase): limit=1, ) self.assertEqual(updated_entry[0], -7) + self.assertEqual(leave_allocation.total_leaves_allocated, 23) def test_validation_against_leave_application_after_submit(self): from erpnext.payroll.doctype.salary_slip.test_salary_slip import make_holiday_list From 2f62a9641e7260e2fcb73cf00d8ade6262d448e6 Mon Sep 17 00:00:00 2001 From: Rucha Mahabal Date: Tue, 14 Mar 2023 12:24:56 +0530 Subject: [PATCH 7/7] fix: leave allocation tests --- erpnext/hr/doctype/leave_allocation/test_leave_allocation.py | 5 +++-- erpnext/hr/doctype/leave_type/test_leave_type.py | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/erpnext/hr/doctype/leave_allocation/test_leave_allocation.py b/erpnext/hr/doctype/leave_allocation/test_leave_allocation.py index 3a234b6fa97..07792b5ea54 100644 --- a/erpnext/hr/doctype/leave_allocation/test_leave_allocation.py +++ b/erpnext/hr/doctype/leave_allocation/test_leave_allocation.py @@ -18,6 +18,7 @@ class TestLeaveAllocation(FrappeTestCase): def setUp(self): frappe.db.delete("Leave Period") frappe.db.delete("Leave Allocation") + frappe.db.delete("Leave Ledger Entry") emp_id = make_employee("test_emp_leave_allocation@salary.com", company="_Test Company") self.employee = frappe.get_doc("Employee", emp_id) @@ -369,7 +370,7 @@ class TestLeaveAllocation(FrappeTestCase): self.assertEqual(leave_allocation.total_leaves_allocated, 40) def test_leave_addition_after_submit_with_carry_forward(self): - from hrms.hr.doctype.leave_application.test_leave_application import ( + from erpnext.hr.doctype.leave_application.test_leave_application import ( create_carry_forwarded_allocation, ) @@ -421,7 +422,7 @@ class TestLeaveAllocation(FrappeTestCase): self.assertEqual(leave_allocation.total_leaves_allocated, 10) def test_leave_subtraction_after_submit_with_carry_forward(self): - from hrms.hr.doctype.leave_application.test_leave_application import ( + from erpnext.hr.doctype.leave_application.test_leave_application import ( create_carry_forwarded_allocation, ) diff --git a/erpnext/hr/doctype/leave_type/test_leave_type.py b/erpnext/hr/doctype/leave_type/test_leave_type.py index 628f16d0654..56bf641d261 100644 --- a/erpnext/hr/doctype/leave_type/test_leave_type.py +++ b/erpnext/hr/doctype/leave_type/test_leave_type.py @@ -9,7 +9,7 @@ test_records = frappe.get_test_records("Leave Type") def create_leave_type(**args): args = frappe._dict(args) if frappe.db.exists("Leave Type", args.leave_type_name): - frappe.delete_doc("Leave Type", args.leave_type_name) + frappe.delete_doc("Leave Type", args.leave_type_name, force=True) leave_type = frappe.get_doc( {