Merge pull request #25876 from resilient-tech/fix-ssa-selection

fix: choose correct Salary Structure Assignment when getting data for formula eval

erpnext/payroll/doctype/salary_slip/salary_slip.py

@@ -115,10 +115,23 @@ class SalarySlip(TransactionBase):
 			status = "Cancelled"
 		return status
 
-	def validate_dates(self):
+	def validate_dates(self, joining_date=None, relieving_date=None):
 		if date_diff(self.end_date, self.start_date) < 0:
 			frappe.throw(_("To date cannot be before From date"))
 
+		if not joining_date:
+			joining_date, relieving_date = frappe.get_cached_value(
+				"Employee",
+				self.employee,
+				("date_of_joining", "relieving_date")
+			)
+
+		if date_diff(self.end_date, joining_date) < 0:
+			frappe.throw(_("Cannot create Salary Slip for Employee joining after Payroll Period"))
+
+		if relieving_date and date_diff(relieving_date, self.start_date) < 0:
+			frappe.throw(_("Cannot create Salary Slip for Employee who has left before Payroll Period"))
+
 	def is_rounding_total_disabled(self):
 		return cint(frappe.db.get_single_value("Payroll Settings", "disable_rounded_total"))
 
@@ -154,9 +167,14 @@ class SalarySlip(TransactionBase):
 
 			if not self.salary_slip_based_on_timesheet:
 				self.get_date_details()
-			self.validate_dates()
+
-			joining_date, relieving_date = frappe.get_cached_value("Employee", self.employee,
+			joining_date, relieving_date = frappe.get_cached_value(
-				["date_of_joining", "relieving_date"])
+				"Employee",
+				self.employee,
+				("date_of_joining", "relieving_date")
+			)
+
+			self.validate_dates(joining_date, relieving_date)
 
 			#getin leave details
 			self.get_working_days_details(joining_date, relieving_date)
@@ -492,11 +510,39 @@ class SalarySlip(TransactionBase):
 	def get_data_for_eval(self):
 		'''Returns data for evaluating formula'''
 		data = frappe._dict()
+		employee = frappe.get_doc("Employee", self.employee).as_dict()
 
-		data.update(frappe.get_doc("Salary Structure Assignment",
+		start_date = getdate(self.start_date)
-			{"employee": self.employee, "salary_structure": self.salary_structure}).as_dict())
+		date_to_validate = (
+			employee.date_of_joining
+			if employee.date_of_joining > start_date
+			else start_date
+		)
 
-		data.update(frappe.get_doc("Employee", self.employee).as_dict())
+		salary_structure_assignment = frappe.get_value(
+			"Salary Structure Assignment",
+			{
+				"employee": self.employee,
+				"salary_structure": self.salary_structure,
+				"from_date": ("<=", date_to_validate),
+				"docstatus": 1,
+			},
+			"*",
+			order_by="from_date desc",
+			as_dict=True,
+		)
+
+		if not salary_structure_assignment:
+			frappe.throw(
+				_("Please assign a Salary Structure for Employee {0} "
+				"applicable from or before {1} first").format(
+					frappe.bold(self.employee_name),
+					frappe.bold(formatdate(date_to_validate)),
+				)
+			)
+
+		data.update(salary_structure_assignment)
+		data.update(employee)
 		data.update(self.as_dict())
 
 		# set values for components

erpnext/payroll/doctype/salary_slip/test_salary_slip.py

@@ -8,7 +8,6 @@ import erpnext
 import calendar
 import random
 from erpnext.accounts.utils import get_fiscal_year
-from frappe.utils.make_random import get_random
 from frappe.utils import getdate, nowdate, add_days, add_months, flt, get_first_day, get_last_day, cstr
 from erpnext.payroll.doctype.salary_structure.salary_structure import make_salary_slip
 from erpnext.payroll.doctype.payroll_entry.payroll_entry import get_month_details
@@ -155,12 +154,14 @@ class TestSalarySlip(unittest.TestCase):
 		self.assertEqual(ss.gross_pay, 78000)
 
 	def test_payment_days(self):
+		from erpnext.payroll.doctype.salary_structure.test_salary_structure import create_salary_structure_assignment
+
 		no_of_days = self.get_no_of_days()
 		# Holidays not included in working days
 		frappe.db.set_value("Payroll Settings", None, "include_holidays_in_total_working_days", 1)
 
 		# set joinng date in the same month
-		make_employee("test_payment_days@salary.com")
+		employee = make_employee("test_payment_days@salary.com")
 		if getdate(nowdate()).day >= 15:
 			relieving_date = getdate(add_days(nowdate(),-10))
 			date_of_joining = getdate(add_days(nowdate(),-10))
@@ -174,25 +175,30 @@ class TestSalarySlip(unittest.TestCase):
 			date_of_joining = getdate(nowdate())
 			relieving_date = getdate(nowdate())
 
-		frappe.db.set_value("Employee", frappe.get_value("Employee",
+		frappe.db.set_value("Employee", employee, {
-			{"employee_name":"test_payment_days@salary.com"}, "name"), "date_of_joining", date_of_joining)
+			"date_of_joining": date_of_joining,
-		frappe.db.set_value("Employee", frappe.get_value("Employee",
+			"relieving_date": None,
-			{"employee_name":"test_payment_days@salary.com"}, "name"), "relieving_date", None)
+			"status": "Active"
-		frappe.db.set_value("Employee", frappe.get_value("Employee",
+		})
-			{"employee_name":"test_payment_days@salary.com"}, "name"), "status", "Active")
 
-		ss = make_employee_salary_slip("test_payment_days@salary.com", "Monthly", "Test Payment Days")
+		salary_structure = "Test Payment Days"
+		ss = make_employee_salary_slip("test_payment_days@salary.com", "Monthly", salary_structure)
 
 		self.assertEqual(ss.total_working_days, no_of_days[0])
 		self.assertEqual(ss.payment_days, (no_of_days[0] - getdate(date_of_joining).day + 1))
 
 		# set relieving date in the same month
-		frappe.db.set_value("Employee",frappe.get_value("Employee",
+		frappe.db.set_value("Employee", employee, {
-			{"employee_name":"test_payment_days@salary.com"}, "name"), "date_of_joining", (add_days(nowdate(),-60)))
+			"date_of_joining": add_days(nowdate(),-60),
-		frappe.db.set_value("Employee", frappe.get_value("Employee",
+			"relieving_date": relieving_date,
-			{"employee_name":"test_payment_days@salary.com"}, "name"), "relieving_date", relieving_date)
+			"status": "Left"
-		frappe.db.set_value("Employee", frappe.get_value("Employee",
+		})
-			{"employee_name":"test_payment_days@salary.com"}, "name"), "status", "Left")
+
+		if date_of_joining.day > 1:
+			self.assertRaises(frappe.ValidationError, ss.save)
+
+		create_salary_structure_assignment(employee, salary_structure)
+		ss.reload()
 		ss.save()
 
 		self.assertEqual(ss.total_working_days, no_of_days[0])
@@ -285,6 +291,7 @@ class TestSalarySlip(unittest.TestCase):
 
 	def test_multi_currency_salary_slip(self):
 		from erpnext.payroll.doctype.salary_structure.test_salary_structure import make_salary_structure
+
 		applicant = make_employee("test_multi_currency_salary_slip@salary.com", company="_Test Company")
 		frappe.db.sql("""delete from `tabSalary Structure` where name='Test Multi Currency Salary Slip'""")
 		salary_structure = make_salary_structure("Test Multi Currency Salary Slip", "Monthly", employee=applicant, company="_Test Company", currency='USD')
@@ -325,7 +332,8 @@ class TestSalarySlip(unittest.TestCase):
 	def test_component_wise_year_to_date_computation(self):
 		from erpnext.payroll.doctype.salary_structure.test_salary_structure import make_salary_structure
 
-		applicant = make_employee("test_ytd@salary.com", company="_Test Company")
+		employee_name = "test_component_wise_ytd@salary.com"
+		applicant = make_employee(employee_name, company="_Test Company")
 
 		payroll_period = create_payroll_period(name="_Test Payroll Period 1", company="_Test Company")
 
@@ -336,13 +344,13 @@ class TestSalarySlip(unittest.TestCase):
 			"Monthly", employee=applicant, company="_Test Company", currency="INR", payroll_period=payroll_period)
 
 		# clear salary slip for this employee
-		frappe.db.sql("DELETE FROM `tabSalary Slip` where employee_name = 'test_ytd@salary.com'")
+		frappe.db.sql("DELETE FROM `tabSalary Slip` where employee_name = '%s'" % employee_name)
 
 		create_salary_slips_for_payroll_period(applicant, salary_structure.name,
 			payroll_period, deduct_random=False, num=3)
 
 		salary_slips = frappe.get_all("Salary Slip", fields=["name"], filters={"employee_name":
-			"test_ytd@salary.com"}, order_by = "posting_date")
+			employee_name}, order_by="posting_date")
 
 		year_to_date = dict()
 		for slip in salary_slips:
@@ -380,10 +388,10 @@ class TestSalarySlip(unittest.TestCase):
 
 		from erpnext.payroll.doctype.salary_structure.test_salary_structure import \
 			make_salary_structure, create_salary_structure_assignment
+
 		salary_structure = make_salary_structure("Stucture to test tax", "Monthly",
-			other_details={"max_benefits": 100000}, test_tax=True)
+			other_details={"max_benefits": 100000}, test_tax=True,
-		create_salary_structure_assignment(employee, salary_structure.name,
+			employee=employee, payroll_period=payroll_period)
-			payroll_period.start_date)
 
 		# create salary slip for whole period deducting tax only on last period
 		# to find the total tax amount paid
@@ -469,6 +477,7 @@ class TestSalarySlip(unittest.TestCase):
 
 def make_employee_salary_slip(user, payroll_frequency, salary_structure=None):
 	from erpnext.payroll.doctype.salary_structure.test_salary_structure import make_salary_structure
+
 	if not salary_structure:
 		salary_structure = payroll_frequency + " Salary Structure Test for Salary Slip"
 

erpnext/payroll/doctype/salary_structure/test_salary_structure.py

@@ -6,7 +6,7 @@ import frappe
 import unittest
 import erpnext
 from frappe.utils.make_random import get_random
-from frappe.utils import nowdate, add_days, add_years, getdate, add_months
+from frappe.utils import nowdate, add_years, get_first_day, date_diff
 from erpnext.payroll.doctype.salary_structure.salary_structure import make_salary_slip
 from erpnext.payroll.doctype.salary_slip.test_salary_slip import make_earning_salary_component,\
 	make_deduction_salary_component, make_employee_salary_slip, create_tax_slab
@@ -113,8 +113,9 @@ class TestSalaryStructure(unittest.TestCase):
 		sal_struct = make_salary_structure("Salary Structure Multi Currency", "Monthly", currency='USD')
 		self.assertEqual(sal_struct.currency, 'USD')
 
-def make_salary_structure(salary_structure, payroll_frequency, employee=None, dont_submit=False, other_details=None,
+def make_salary_structure(salary_structure, payroll_frequency, employee=None,
-	test_tax=False, company=None, currency=erpnext.get_default_currency(), payroll_period=None):
+	from_date=None, dont_submit=False, other_details=None,test_tax=False,
+	company=None, currency=erpnext.get_default_currency(), payroll_period=None):
 	if test_tax:
 		frappe.db.sql("""delete from `tabSalary Structure` where name=%s""",(salary_structure))
 
@@ -139,10 +140,23 @@ def make_salary_structure(salary_structure, payroll_frequency, employee=None, do
 	else:
 		salary_structure_doc = frappe.get_doc("Salary Structure", salary_structure)
 
+	filters = {'employee':employee, 'docstatus': 1}
+	if not from_date and payroll_period:
+		from_date = payroll_period.start_date
+
+	if from_date:
+		filters['from_date'] = from_date
+
 	if employee and not frappe.db.get_value("Salary Structure Assignment",
-		{'employee':employee, 'docstatus': 1}) and salary_structure_doc.docstatus==1:
+		filters) and salary_structure_doc.docstatus==1:
-			create_salary_structure_assignment(employee, salary_structure, company=company, currency=currency,
+		create_salary_structure_assignment(
-			payroll_period=payroll_period)
+			employee,
+			salary_structure,
+			from_date=from_date,
+			company=company,
+			currency=currency,
+			payroll_period=payroll_period
+		)
 
 	return salary_structure_doc
 
@@ -165,12 +179,13 @@ def create_salary_structure_assignment(employee, salary_structure, from_date=Non
 	salary_structure_assignment.base = 50000
 	salary_structure_assignment.variable = 5000
 
-	if getdate(nowdate()).day == 1:
+	if not from_date:
-		date = from_date or nowdate()
+		from_date = get_first_day(nowdate())
-	else:
+		joining_date = frappe.get_cached_value("Employee", employee, "date_of_joining")
-		date = from_date or add_days(nowdate(), -1)
+		if date_diff(joining_date, from_date) > 0:
+			from_date = joining_date
 
-	salary_structure_assignment.from_date = date
+	salary_structure_assignment.from_date = from_date
 	salary_structure_assignment.salary_structure = salary_structure
 	salary_structure_assignment.currency = currency
 	salary_structure_assignment.payroll_payable_account = get_payable_account(company)