fix: employee reminders fixes (#29548)

erpnext/hr/doctype/employee/employee_reminders.py

@@ -20,6 +20,7 @@ def send_reminders_in_advance_weekly():
 
 	send_advance_holiday_reminders("Weekly")
 
+
 def send_reminders_in_advance_monthly():
 	to_send_in_advance = int(frappe.db.get_single_value("HR Settings", "send_holiday_reminders"))
 	frequency = frappe.db.get_single_value("HR Settings", "frequency")
@@ -28,6 +29,7 @@ def send_reminders_in_advance_monthly():
 
 	send_advance_holiday_reminders("Monthly")
 
+
 def send_advance_holiday_reminders(frequency):
 	"""Send Holiday Reminders in Advance to Employees
 	`frequency` (str): 'Weekly' or 'Monthly'
@@ -42,7 +44,7 @@ def send_advance_holiday_reminders(frequency):
 	else:
 		return
 
-	employees = frappe.db.get_all('Employee', pluck='name')
+	employees = frappe.db.get_all('Employee', filters={'status': 'Active'}, pluck='name')
 	for employee in employees:
 		holidays = get_holidays_for_employee(
 			employee,
@@ -51,10 +53,13 @@ def send_advance_holiday_reminders(frequency):
 			raise_exception=False
 		)
 
-		if not (holidays is None):
 		send_holidays_reminder_in_advance(employee, holidays)
 
+
 def send_holidays_reminder_in_advance(employee, holidays):
+	if not holidays:
+		return
+
 	employee_doc = frappe.get_doc('Employee', employee)
 	employee_email = get_employee_email(employee_doc)
 	frequency = frappe.db.get_single_value("HR Settings", "frequency")
@@ -101,6 +106,7 @@ def send_birthday_reminders():
 				reminder_text, message = get_birthday_reminder_text_and_message(others)
 				send_birthday_reminder(person_email, reminder_text, others, message)
 
+
 def get_birthday_reminder_text_and_message(birthday_persons):
 	if len(birthday_persons) == 1:
 		birthday_person_text = birthday_persons[0]['name']
@@ -116,6 +122,7 @@ def get_birthday_reminder_text_and_message(birthday_persons):
 
 	return reminder_text, message
 
+
 def send_birthday_reminder(recipients, reminder_text, birthday_persons, message):
 	frappe.sendmail(
 		recipients=recipients,
@@ -129,10 +136,12 @@ def send_birthday_reminder(recipients, reminder_text, birthday_persons, message)
 		header=_("Birthday Reminder 🎂")
 	)
 
+
 def get_employees_who_are_born_today():
 	"""Get all employee born today & group them based on their company"""
 	return get_employees_having_an_event_today("birthday")
 
+
 def get_employees_having_an_event_today(event_type):
 	"""Get all employee who have `event_type` today
 	& group them based on their company. `event_type`
@@ -210,13 +219,14 @@ def send_work_anniversary_reminders():
 				reminder_text, message = get_work_anniversary_reminder_text_and_message(others)
 				send_work_anniversary_reminder(person_email, reminder_text, others, message)
 
+
 def get_work_anniversary_reminder_text_and_message(anniversary_persons):
 	if len(anniversary_persons) == 1:
 		anniversary_person = anniversary_persons[0]['name']
 		persons_name = anniversary_person
 		# Number of years completed at the company
 		completed_years = getdate().year - anniversary_persons[0]['date_of_joining'].year
-		anniversary_person += f" completed {completed_years} years"
+		anniversary_person += f" completed {completed_years} year(s)"
 	else:
 		person_names_with_years = []
 		names = []
@@ -225,7 +235,7 @@ def get_work_anniversary_reminder_text_and_message(anniversary_persons):
 			names.append(person_text)
 			# Number of years completed at the company
 			completed_years = getdate().year - person['date_of_joining'].year
-			person_text += f" completed {completed_years} years"
+			person_text += f" completed {completed_years} year(s)"
 			person_names_with_years.append(person_text)
 
 		# converts ["Jim", "Rim", "Dim"] to Jim, Rim & Dim
@@ -239,6 +249,7 @@ def get_work_anniversary_reminder_text_and_message(anniversary_persons):
 
 	return reminder_text, message
 
+
 def send_work_anniversary_reminder(recipients, reminder_text, anniversary_persons, message):
 	frappe.sendmail(
 		recipients=recipients,
@@ -249,5 +260,5 @@ def send_work_anniversary_reminder(recipients, reminder_text, anniversary_person
 			anniversary_persons=anniversary_persons,
 			message=message,
 		),
-		header=_("🎊️🎊️ Work Anniversary Reminder 🎊️🎊️")
+		header=_("Work Anniversary Reminder")
 	)

erpnext/hr/doctype/employee/test_employee_reminders.py

@@ -5,10 +5,12 @@ import unittest
 from datetime import timedelta
 
 import frappe
-from frappe.utils import getdate
+from frappe.utils import add_months, getdate
 
+from erpnext.hr.doctype.employee.employee_reminders import send_holidays_reminder_in_advance
 from erpnext.hr.doctype.employee.test_employee import make_employee
 from erpnext.hr.doctype.hr_settings.hr_settings import set_proceed_with_frequency_change
+from erpnext.hr.utils import get_holidays_for_employee
 
 
 class TestEmployeeReminders(unittest.TestCase):
@@ -46,6 +48,24 @@ class TestEmployeeReminders(unittest.TestCase):
 		cls.test_employee = test_employee
 		cls.test_holiday_dates = test_holiday_dates
 
+		# Employee without holidays in this month/week
+		test_employee_2 = make_employee('test@empwithoutholiday.io', company="_Test Company")
+		test_employee_2 = frappe.get_doc('Employee', test_employee_2)
+
+		test_holiday_list = make_holiday_list(
+			'TestHolidayRemindersList2',
+			holiday_dates=[
+				{'holiday_date': add_months(getdate(), 1), 'description': 'test holiday1'},
+			],
+			from_date=add_months(getdate(), -2),
+			to_date=add_months(getdate(), 2)
+		)
+		test_employee_2.holiday_list = test_holiday_list.name
+		test_employee_2.save()
+
+		cls.test_employee_2 = test_employee_2
+		cls.holiday_list_2 = test_holiday_list
+
 	@classmethod
 	def get_test_holiday_dates(cls):
 		today_date = getdate()
@@ -61,6 +81,7 @@ class TestEmployeeReminders(unittest.TestCase):
 	def setUp(self):
 		# Clear Email Queue
 		frappe.db.sql("delete from `tabEmail Queue`")
+		frappe.db.sql("delete from `tabEmail Queue Recipient`")
 
 	def test_is_holiday(self):
 		from erpnext.hr.doctype.employee.employee import is_holiday
@@ -103,11 +124,10 @@ class TestEmployeeReminders(unittest.TestCase):
 		self.assertTrue("Subject: Birthday Reminder" in email_queue[0].message)
 
 	def test_work_anniversary_reminders(self):
-		employee = frappe.get_doc("Employee", frappe.db.sql_list("select name from tabEmployee limit 1")[0])
+		make_employee("test_work_anniversary@gmail.com",
-		employee.date_of_joining = "1998" + frappe.utils.nowdate()[4:]
+			date_of_joining="1998" + frappe.utils.nowdate()[4:],
-		employee.company_email = "test@example.com"
+			company="_Test Company",
-		employee.company = "_Test Company"
+		)
-		employee.save()
 
 		from erpnext.hr.doctype.employee.employee_reminders import (
 			get_employees_having_an_event_today,
@@ -115,7 +135,12 @@ class TestEmployeeReminders(unittest.TestCase):
 		)
 
 		employees_having_work_anniversary = get_employees_having_an_event_today('work_anniversary')
-		self.assertTrue(employees_having_work_anniversary.get("_Test Company"))
+		employees = employees_having_work_anniversary.get("_Test Company") or []
+		user_ids = []
+		for entry in employees:
+			user_ids.append(entry.user_id)
+
+		self.assertTrue("test_work_anniversary@gmail.com" in user_ids)
 
 		hr_settings = frappe.get_doc("HR Settings", "HR Settings")
 		hr_settings.send_work_anniversary_reminders = 1
@@ -126,16 +151,24 @@ class TestEmployeeReminders(unittest.TestCase):
 		email_queue = frappe.db.sql("""select * from `tabEmail Queue`""", as_dict=True)
 		self.assertTrue("Subject: Work Anniversary Reminder" in email_queue[0].message)
 
-	def test_send_holidays_reminder_in_advance(self):
+	def test_work_anniversary_reminder_not_sent_for_0_years(self):
-		from erpnext.hr.doctype.employee.employee_reminders import send_holidays_reminder_in_advance
+		make_employee("test_work_anniversary_2@gmail.com",
-		from erpnext.hr.utils import get_holidays_for_employee
+			date_of_joining=getdate(),
+			company="_Test Company",
+		)
 
-		# Get HR settings and enable advance holiday reminders
+		from erpnext.hr.doctype.employee.employee_reminders import get_employees_having_an_event_today
-		hr_settings = frappe.get_doc("HR Settings", "HR Settings")
+
-		hr_settings.send_holiday_reminders = 1
+		employees_having_work_anniversary = get_employees_having_an_event_today('work_anniversary')
-		set_proceed_with_frequency_change()
+		employees = employees_having_work_anniversary.get("_Test Company") or []
-		hr_settings.frequency = 'Weekly'
+		user_ids = []
-		hr_settings.save()
+		for entry in employees:
+			user_ids.append(entry.user_id)
+
+		self.assertTrue("test_work_anniversary_2@gmail.com" not in user_ids)
+
+	def test_send_holidays_reminder_in_advance(self):
+		setup_hr_settings('Weekly')
 
 		holidays = get_holidays_for_employee(
 					self.test_employee.get('name'),
@@ -151,32 +184,80 @@ class TestEmployeeReminders(unittest.TestCase):
 
 		email_queue = frappe.db.sql("""select * from `tabEmail Queue`""", as_dict=True)
 		self.assertEqual(len(email_queue), 1)
+		self.assertTrue("Holidays this Week." in email_queue[0].message)
 
 	def test_advance_holiday_reminders_monthly(self):
 		from erpnext.hr.doctype.employee.employee_reminders import send_reminders_in_advance_monthly
 
-		# Get HR settings and enable advance holiday reminders
+		setup_hr_settings('Monthly')
-		hr_settings = frappe.get_doc("HR Settings", "HR Settings")
+
-		hr_settings.send_holiday_reminders = 1
+		# disable emp 2, set same holiday list
-		set_proceed_with_frequency_change()
+		frappe.db.set_value('Employee', self.test_employee_2.name, {
-		hr_settings.frequency = 'Monthly'
+			'status': 'Left',
-		hr_settings.save()
+			'holiday_list': self.test_employee.holiday_list
+		})
 
 		send_reminders_in_advance_monthly()
-
 		email_queue = frappe.db.sql("""select * from `tabEmail Queue`""", as_dict=True)
 		self.assertTrue(len(email_queue) > 0)
 
+		# even though emp 2 has holiday, non-active employees should not be recipients
+		recipients = frappe.db.get_all('Email Queue Recipient', pluck='recipient')
+		self.assertTrue(self.test_employee_2.user_id not in recipients)
+
+		# teardown: enable emp 2
+		frappe.db.set_value('Employee', self.test_employee_2.name, {
+			'status': 'Left',
+			'holiday_list': self.holiday_list_2
+		})
+
 	def test_advance_holiday_reminders_weekly(self):
 		from erpnext.hr.doctype.employee.employee_reminders import send_reminders_in_advance_weekly
 
+		setup_hr_settings('Weekly')
+
+		# disable emp 2, set same holiday list
+		frappe.db.set_value('Employee', self.test_employee_2.name, {
+			'status': 'Left',
+			'holiday_list': self.test_employee.holiday_list
+		})
+
+		send_reminders_in_advance_weekly()
+		email_queue = frappe.db.sql("""select * from `tabEmail Queue`""", as_dict=True)
+		self.assertTrue(len(email_queue) > 0)
+
+		# even though emp 2 has holiday, non-active employees should not be recipients
+		recipients = frappe.db.get_all('Email Queue Recipient', pluck='recipient')
+		self.assertTrue(self.test_employee_2.user_id not in recipients)
+
+		# teardown: enable emp 2
+		frappe.db.set_value('Employee', self.test_employee_2.name, {
+			'status': 'Left',
+			'holiday_list': self.holiday_list_2
+		})
+
+	def test_reminder_not_sent_if_no_holdays(self):
+		setup_hr_settings('Monthly')
+
+		# reminder not sent if there are no holidays
+		holidays = get_holidays_for_employee(
+			self.test_employee_2.get('name'),
+			getdate(), getdate() + timedelta(days=3),
+			only_non_weekly=True,
+			raise_exception=False
+		)
+		send_holidays_reminder_in_advance(
+			self.test_employee_2.get('name'),
+			holidays
+		)
+		email_queue = frappe.db.sql("""select * from `tabEmail Queue`""", as_dict=True)
+		self.assertEqual(len(email_queue), 0)
+
+
+def setup_hr_settings(frequency=None):
 	# Get HR settings and enable advance holiday reminders
 	hr_settings = frappe.get_doc("HR Settings", "HR Settings")
 	hr_settings.send_holiday_reminders = 1
-		hr_settings.frequency = 'Weekly'
+	set_proceed_with_frequency_change()
+	hr_settings.frequency = frequency or 'Weekly'
 	hr_settings.save()
-
-		send_reminders_in_advance_weekly()
-
-		email_queue = frappe.db.sql("""select * from `tabEmail Queue`""", as_dict=True)
-		self.assertTrue(len(email_queue) > 0)