From 6130dc7173fc29d0435c057878cf9248172e24fa Mon Sep 17 00:00:00 2001 From: Rucha Mahabal Date: Fri, 10 Mar 2023 15:32:30 +0530 Subject: [PATCH 1/8] fix: exclude carry forwarding leaves while updating leaves after submission (cherry picked from commit 75603c8b3e81a5ae944c19be0defe20a42da3d88) --- hrms/hr/doctype/leave_allocation/leave_allocation.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/hrms/hr/doctype/leave_allocation/leave_allocation.py b/hrms/hr/doctype/leave_allocation/leave_allocation.py index e2de40268a..737e9277a7 100755 --- a/hrms/hr/doctype/leave_allocation/leave_allocation.py +++ b/hrms/hr/doctype/leave_allocation/leave_allocation.py @@ -99,7 +99,11 @@ def on_update_after_submit(self): # 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 @@ def get_existing_leave_count(self): "employee": self.employee, "company": self.company, "leave_type": self.leave_type, + "is_carry_forward": 0, }, pluck="leaves", ) From a32619c31f4810161a525536644d4141cc054828 Mon Sep 17 00:00:00 2001 From: Rucha Mahabal Date: Fri, 10 Mar 2023 15:33:12 +0530 Subject: [PATCH 2/8] test: leaves updated after submission with carry forwarding (cherry picked from commit 1f26d042cb843b987ba245aafc4177e4df8e505f) --- .../leave_allocation/test_leave_allocation.py | 89 +++++++++++++++---- 1 file changed, 71 insertions(+), 18 deletions(-) diff --git a/hrms/hr/doctype/leave_allocation/test_leave_allocation.py b/hrms/hr/doctype/leave_allocation/test_leave_allocation.py index 67472f970b..b326d97f68 100644 --- a/hrms/hr/doctype/leave_allocation/test_leave_allocation.py +++ b/hrms/hr/doctype/leave_allocation/test_leave_allocation.py @@ -71,7 +71,6 @@ def test_invalid_period(self): 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( { @@ -139,9 +138,9 @@ def test_validation_for_over_allocation_based_on_leave_setup(self): ) ).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( @@ -176,9 +175,9 @@ def test_validation_for_over_allocation_based_on_leave_setup_post_submission(sel ) ).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( @@ -209,7 +208,6 @@ def test_validation_for_over_allocation_based_on_leave_setup_post_submission(sel 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( @@ -237,10 +235,12 @@ def test_validate_back_dated_allocation_update(self): 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( @@ -288,7 +288,6 @@ def test_carry_forward_leaves_expiry(self): is_carry_forward=1, expire_carry_forwarded_leaves_after_days=90, ) - leave_type.save() # initial leave allocation leave_allocation = create_leave_allocation( @@ -354,12 +353,40 @@ def test_leave_addition_after_submit(self): ) 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( @@ -367,12 +394,38 @@ def test_leave_subtraction_after_submit(self): ) 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 hrms.payroll.doctype.salary_slip.test_salary_slip import make_holiday_list From fac5557c954745fe25b18a3cc617e5b550a8863e Mon Sep 17 00:00:00 2001 From: Rucha Mahabal Date: Fri, 10 Mar 2023 15:33:35 +0530 Subject: [PATCH 3/8] refactor(tests): `create_leave_type` usage (cherry picked from commit 347e08caf7b0eeed864bce1d9382098b11580c71) --- .../leave_application/test_leave_application.py | 15 ++++++--------- hrms/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/hrms/hr/doctype/leave_application/test_leave_application.py b/hrms/hr/doctype/leave_application/test_leave_application.py index 9659654919..418044a62c 100644 --- a/hrms/hr/doctype/leave_application/test_leave_application.py +++ b/hrms/hr/doctype/leave_application/test_leave_application.py @@ -703,7 +703,7 @@ def test_leave_balance_near_allocaton_expiry(self): 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( @@ -754,7 +754,6 @@ def test_creation_of_leave_ledger_entry_on_submit(self): 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 @@ -797,7 +796,6 @@ def test_ledger_entry_creation_on_intermediate_allocation_expiry(self): expire_carry_forwarded_leaves_after_days=90, include_holiday=True, ) - leave_type.submit() create_carry_forwarded_allocation(employee, leave_type) @@ -836,7 +834,6 @@ def test_leave_application_creation_after_expiry(self): is_carry_forward=1, expire_carry_forwarded_leaves_after_days=90, ) - leave_type.submit() create_carry_forwarded_allocation(employee, leave_type) @@ -937,7 +934,7 @@ def test_leave_details_with_expired_cf_leaves(self): 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( @@ -970,7 +967,7 @@ def test_leave_details_with_application_across_cf_expiry(self): 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( @@ -1004,7 +1001,7 @@ def test_leave_details_with_application_across_cf_expiry_2(self): 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( @@ -1046,7 +1043,7 @@ def test_leave_details_with_application_after_cf_expiry(self): 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( @@ -1080,7 +1077,7 @@ def test_get_leave_allocation_records(self): 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/hrms/hr/doctype/leave_type/test_leave_type.py b/hrms/hr/doctype/leave_type/test_leave_type.py index 69f9e12520..628f16d065 100644 --- a/hrms/hr/doctype/leave_type/test_leave_type.py +++ b/hrms/hr/doctype/leave_type/test_leave_type.py @@ -9,7 +9,8 @@ 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/hrms/hr/report/employee_leave_balance/test_employee_leave_balance.py b/hrms/hr/report/employee_leave_balance/test_employee_leave_balance.py index 21311ba1dd..187f95f857 100644 --- a/hrms/hr/report/employee_leave_balance/test_employee_leave_balance.py +++ b/hrms/hr/report/employee_leave_balance/test_employee_leave_balance.py @@ -155,7 +155,6 @@ def test_opening_balance_on_alloc_boundary_dates(self): @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/hrms/payroll/doctype/salary_slip/test_salary_slip.py b/hrms/payroll/doctype/salary_slip/test_salary_slip.py index 64aacb5ea1..969a240b7f 100644 --- a/hrms/payroll/doctype/salary_slip/test_salary_slip.py +++ b/hrms/payroll/doctype/salary_slip/test_salary_slip.py @@ -301,7 +301,6 @@ def test_payment_days_based_on_leave_application(self): 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 593eed54fe4753c5677227597d74875239c0ed21 Mon Sep 17 00:00:00 2001 From: Rucha Mahabal Date: Fri, 10 Mar 2023 15:48:51 +0530 Subject: [PATCH 4/8] fix: exclude cancelled leave ledger entries (cherry picked from commit e8fce17ff522116197505d5fe3f66e422e023adb) --- hrms/hr/doctype/leave_allocation/leave_allocation.py | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/hrms/hr/doctype/leave_allocation/leave_allocation.py b/hrms/hr/doctype/leave_allocation/leave_allocation.py index 737e9277a7..ea8dfefec8 100755 --- a/hrms/hr/doctype/leave_allocation/leave_allocation.py +++ b/hrms/hr/doctype/leave_allocation/leave_allocation.py @@ -99,7 +99,7 @@ def on_update_after_submit(self): # 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 @@ def get_existing_leave_count(self): "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 8932ed7ce03555eb0aef588056befdac73503721 Mon Sep 17 00:00:00 2001 From: Rucha Mahabal Date: Tue, 14 Mar 2023 10:51:35 +0530 Subject: [PATCH 5/8] fix: precision for newly allocated leaves (cherry picked from commit 6466303d21b241821e55cb968ffc7ae10c95c9ee) --- hrms/hr/doctype/leave_allocation/leave_allocation.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/hrms/hr/doctype/leave_allocation/leave_allocation.py b/hrms/hr/doctype/leave_allocation/leave_allocation.py index ea8dfefec8..e9bbd57924 100755 --- a/hrms/hr/doctype/leave_allocation/leave_allocation.py +++ b/hrms/hr/doctype/leave_allocation/leave_allocation.py @@ -100,8 +100,8 @@ def on_update_after_submit(self): 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 75b0fee6b0771160d20e60db56557cd823820293 Mon Sep 17 00:00:00 2001 From: Rucha Mahabal Date: Tue, 14 Mar 2023 11:15:38 +0530 Subject: [PATCH 6/8] test: update for total leaves allocated post submission (cherry picked from commit d802d4c1fbade17d0d2a933a924f6a4e3d94df0e) --- .../leave_allocation/leave_allocation.py | 1 + .../leave_allocation/test_leave_allocation.py | 24 ++++++++++++++++++- 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/hrms/hr/doctype/leave_allocation/leave_allocation.py b/hrms/hr/doctype/leave_allocation/leave_allocation.py index e9bbd57924..23d0d0b3ca 100755 --- a/hrms/hr/doctype/leave_allocation/leave_allocation.py +++ b/hrms/hr/doctype/leave_allocation/leave_allocation.py @@ -90,6 +90,7 @@ def on_cancel(self): 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/hrms/hr/doctype/leave_allocation/test_leave_allocation.py b/hrms/hr/doctype/leave_allocation/test_leave_allocation.py index b326d97f68..b89f66d689 100644 --- a/hrms/hr/doctype/leave_allocation/test_leave_allocation.py +++ b/hrms/hr/doctype/leave_allocation/test_leave_allocation.py @@ -358,6 +358,16 @@ def test_leave_addition_after_submit(self): 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): @@ -372,7 +382,7 @@ def test_leave_addition_after_submit_with_carry_forward(self): ) 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 @@ -387,6 +397,7 @@ def test_leave_addition_after_submit_with_carry_forward(self): 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( @@ -399,6 +410,16 @@ def test_leave_subtraction_after_submit(self): 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): @@ -426,6 +447,7 @@ def test_leave_subtraction_after_submit_with_carry_forward(self): 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 hrms.payroll.doctype.salary_slip.test_salary_slip import make_holiday_list From e0dc9ca1dce3ef30af879f963eab7622808ff82f Mon Sep 17 00:00:00 2001 From: "mergify[bot]" <37929162+mergify[bot]@users.noreply.github.com> Date: Tue, 14 Mar 2023 16:01:00 +0530 Subject: [PATCH 7/8] fix(postgres): convert salary detail join query to qb (backport #387) (#389) Co-authored-by: Rucha Mahabal --- .../set_salary_details_submittable.py | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/hrms/patches/post_install/set_salary_details_submittable.py b/hrms/patches/post_install/set_salary_details_submittable.py index e5ecce6486..0c46c6ee40 100644 --- a/hrms/patches/post_install/set_salary_details_submittable.py +++ b/hrms/patches/post_install/set_salary_details_submittable.py @@ -2,10 +2,13 @@ def execute(): - frappe.db.sql( - """ - update `tabSalary Structure` ss, `tabSalary Detail` sd - set sd.docstatus=1 - where ss.name=sd.parent and ss.docstatus=1 and sd.parenttype='Salary Structure' - """ - ) + ss = frappe.qb.DocType("Salary Structure").as_("ss") + sd = frappe.qb.DocType("Salary Detail").as_("sd") + + ( + frappe.qb.update(sd) + .inner_join(ss) + .on(ss.name == sd.parent) + .set(sd.docstatus, 1) + .where((ss.docstatus == 1) & (sd.parenttype == "Salary Structure")) + ).run() From 3c3f9b436ba551765a96f50119dbd0c06f286bad Mon Sep 17 00:00:00 2001 From: Rucha Mahabal Date: Wed, 15 Mar 2023 11:42:51 +0530 Subject: [PATCH 8/8] chore: bump version to 14.1.2 --- hrms/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hrms/__init__.py b/hrms/__init__.py index 6ec2efc7f9..22d0a9b083 100644 --- a/hrms/__init__.py +++ b/hrms/__init__.py @@ -1 +1 @@ -__version__ = "14.1.1" +__version__ = "14.1.2"