Merge pull request #2186 from raizasafeel/fix/ui-teardown

fix: evaluator role synched between doctype and role

lms/lms/api.py

@@ -1429,23 +1429,45 @@ def save_role(user: str, role: str, value: int):
 	if role not in LMS_ROLES:
 		frappe.throw(_("You do not have permission to modify this role."), frappe.PermissionError)
 
+	if role == "Batch Evaluator":
+		return save_evaluator_role(user, value)
+
 	if cint(value):
-		doc = frappe.get_doc(
-			{
-				"doctype": "Has Role",
-				"parent": user,
-				"role": role,
-				"parenttype": "User",
-				"parentfield": "roles",
-			}
-		)
-		doc.save(ignore_permissions=True)
+		if not frappe.db.exists("Has Role", {"parent": user, "role": role}):
+			doc = frappe.new_doc("Has Role")
+			doc.parent = user
+			doc.parenttype = "User"
+			doc.parentfield = "roles"
+			doc.role = role
+			doc.save(ignore_permissions=True)
 	else:
 		frappe.db.delete("Has Role", {"parent": user, "role": role})
 	frappe.clear_cache(user=user)
 	return True
 
 
+def save_evaluator_role(user: str, value: int):
+	frappe.only_for("Moderator")
+	if cint(value):
+		if not frappe.db.exists("Has Role", {"parent": user, "role": "Batch Evaluator"}):
+			doc = frappe.new_doc("Has Role")
+			doc.parent = user
+			doc.parenttype = "User"
+			doc.parentfield = "roles"
+			doc.role = "Batch Evaluator"
+			doc.save(ignore_permissions=True)
+		if not frappe.db.exists("Course Evaluator", {"evaluator": user}):
+			doc = frappe.new_doc("Course Evaluator")
+			doc.evaluator = user
+			doc.save(ignore_permissions=True)
+	else:
+		frappe.db.delete("Has Role", {"parent": user, "role": "Batch Evaluator"})
+		if frappe.db.exists("Course Evaluator", {"evaluator": user}):
+			frappe.db.delete("Course Evaluator", {"evaluator": user})
+	frappe.clear_cache(user=user)
+	return True
+
+
 @frappe.whitelist()
 def add_an_evaluator(email: str):
 	frappe.only_for("Moderator")

lms/lms/doctype/course_evaluator/course_evaluator.py

@@ -17,6 +17,11 @@ class CourseEvaluator(Document):
 		self.validate_time_slots()
 		self.validate_unavailability()
 
+	def on_trash(self):
+		roles = frappe.get_roles(self.evaluator)
+		if "Batch Evaluator" in roles:
+			frappe.get_doc("User", self.evaluator).remove_roles("Batch Evaluator")
+
 	def validate_evaluator_role(self):
 		roles = frappe.get_roles(self.evaluator)
 		if "Batch Evaluator" not in roles:

lms/lms/doctype/course_evaluator/test_course_evaluator.py

@@ -1,8 +1,10 @@
 # Copyright (c) 2022, Frappe and Contributors
 # See license.txt
 
+import frappe
 from frappe.utils import add_days, format_time, getdate
 
+from lms.lms.api import save_role
 from lms.lms.doctype.course_evaluator.course_evaluator import get_schedule, get_schedule_range_end_date
 from lms.lms.test_helpers import BaseTestUtils
 
@@ -63,3 +65,59 @@ class TestCourseEvaluator(BaseTestUtils):
 		for row in schedule:
 			schedule_date = getdate(row.get("date"))
 			self.assertFalse(unavailable_from < schedule_date < unavailable_to)
+
+
+class TestEvaluatorRoleCRUD(BaseTestUtils):
+	def setUp(self):
+		super().setUp()
+		self.admin = self._create_user(
+			"frappe@example.com", "Frappe", "Admin", ["Moderator", "Course Creator", "Batch Evaluator"]
+		)
+		self.test_user = self._create_user("eval_test@example.com", "Eval", "Tester", ["LMS Student"])
+
+	def _has_batch_evaluator_role(self, user):
+		return frappe.db.exists("Has Role", {"parent": user, "role": "Batch Evaluator"})
+
+	def _has_course_evaluator(self, user):
+		return frappe.db.exists("Course Evaluator", {"evaluator": user})
+
+	def test_add_evaluator_role_creates_both(self):
+		"""save_role with value=1 should create Has Role AND Course Evaluator."""
+		frappe.set_user("frappe@example.com")
+		save_role(self.test_user.email, "Batch Evaluator", 1)
+		frappe.set_user("Administrator")
+
+		self.assertTrue(self._has_batch_evaluator_role(self.test_user.email))
+		self.assertTrue(self._has_course_evaluator(self.test_user.email))
+
+		self.cleanup_items.append(("Course Evaluator", self.test_user.email))
+
+	def test_remove_evaluator_role_removes_both(self):
+		"""save_role with value=0 should remove Has Role AND Course Evaluator."""
+		frappe.set_user("frappe@example.com")
+		save_role(self.test_user.email, "Batch Evaluator", 1)
+		save_role(self.test_user.email, "Batch Evaluator", 0)
+		frappe.set_user("Administrator")
+
+		self.assertFalse(self._has_batch_evaluator_role(self.test_user.email))
+		self.assertFalse(self._has_course_evaluator(self.test_user.email))
+
+	def test_remove_evaluator_role_no_error_when_missing(self):
+		"""Removing role that doesn't exist should not raise an error."""
+		frappe.set_user("frappe@example.com")
+		save_role(self.test_user.email, "Batch Evaluator", 0)
+		frappe.set_user("Administrator")
+
+		self.assertFalse(self._has_batch_evaluator_role(self.test_user.email))
+
+	def test_reject_non_lms_role(self):
+		"""Assigning a role outside LMS_ROLES should raise PermissionError."""
+		frappe.set_user("frappe@example.com")
+		self.assertRaises(frappe.PermissionError, save_role, self.test_user.email, "System Manager", 1)
+		frappe.set_user("Administrator")
+
+	def test_non_moderator_cannot_save_role(self):
+		"""[A non-moderator user should not be able to assign roles.]"""
+		frappe.set_user(self.test_user.email)
+		self.assertRaises(frappe.PermissionError, save_role, self.test_user.email, "Course Creator", 1)
+		frappe.set_user("Administrator")

lms/patches.txt

@@ -120,4 +120,5 @@ lms.patches.v2_0.share_enrollment
 lms.patches.v2_0.give_user_list_permission #11-02-2026
 lms.patches.v2_0.rename_badge_assignment_event
 lms.patches.v2_0.enable_allow_job_posting
-lms.patches.v2_0.set_conferencing_provider_for_zoom
+lms.patches.v2_0.set_conferencing_provider_for_zoom
+lms.patches.v2_0.sync_evaluator_roles

lms/patches/v2_0/sync_evaluator_roles.py

@@ -0,0 +1,31 @@
+import frappe
+
+
+def execute():
+	evaluator_users = frappe.get_all("Course Evaluator", pluck="evaluator")
+
+	# Add Batch Evaluator role to all Course Evaluator users
+	for user in evaluator_users:
+		if not frappe.db.exists("Has Role", {"parent": user, "role": "Batch Evaluator"}):
+			doc = frappe.new_doc("Has Role")
+			doc.parent = user
+			doc.parenttype = "User"
+			doc.parentfield = "roles"
+			doc.role = "Batch Evaluator"
+			doc.save(ignore_permissions=True)
+
+	# Remove Batch Evaluator role from users who are not in Course Evaluator
+	stale_users = frappe.get_all(
+		"Has Role",
+		filters={
+			"role": "Batch Evaluator",
+			"parent": ["not in", evaluator_users or [""]],
+			"parenttype": "User",
+		},
+		pluck="parent",
+	)
+	for user in stale_users:
+		frappe.db.delete("Has Role", {"parent": user, "role": "Batch Evaluator"})
+
+	for user in set(evaluator_users + stale_users):
+		frappe.clear_cache(user=user)