From ba394926c5c485493f10a2caf1e08be9a1ca4407 Mon Sep 17 00:00:00 2001 From: raizasafeel <89463672+raizasafeel@users.noreply.github.com> Date: Thu, 12 Mar 2026 14:50:19 +0530 Subject: [PATCH 1/3] fix: synched evaluator across roles and 'course evaluator' doctype --- lms/lms/api.py | 42 ++++++++++++++----- .../course_evaluator/course_evaluator.py | 5 +++ 2 files changed, 37 insertions(+), 10 deletions(-) diff --git a/lms/lms/api.py b/lms/lms/api.py index 527914e0..e2801cc1 100644 --- a/lms/lms/api.py +++ b/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") diff --git a/lms/lms/doctype/course_evaluator/course_evaluator.py b/lms/lms/doctype/course_evaluator/course_evaluator.py index fa127395..6bdabbe0 100644 --- a/lms/lms/doctype/course_evaluator/course_evaluator.py +++ b/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: From 6a6b4e01391f46b1a62e5bafd30164dab0530280 Mon Sep 17 00:00:00 2001 From: raizasafeel <89463672+raizasafeel@users.noreply.github.com> Date: Thu, 12 Mar 2026 14:52:26 +0530 Subject: [PATCH 2/3] fix: add patch to sync batch evaluator role with course evaluator doctype --- lms/patches.txt | 3 ++- lms/patches/v2_0/sync_evaluator_roles.py | 31 ++++++++++++++++++++++++ 2 files changed, 33 insertions(+), 1 deletion(-) create mode 100644 lms/patches/v2_0/sync_evaluator_roles.py diff --git a/lms/patches.txt b/lms/patches.txt index 32558c2b..4ce25b76 100644 --- a/lms/patches.txt +++ b/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 \ No newline at end of file +lms.patches.v2_0.set_conferencing_provider_for_zoom +lms.patches.v2_0.sync_evaluator_roles \ No newline at end of file diff --git a/lms/patches/v2_0/sync_evaluator_roles.py b/lms/patches/v2_0/sync_evaluator_roles.py new file mode 100644 index 00000000..ce988cff --- /dev/null +++ b/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) From 8458985c28c91d22c7d813e135bda074aaabaafa Mon Sep 17 00:00:00 2001 From: raizasafeel <89463672+raizasafeel@users.noreply.github.com> Date: Thu, 12 Mar 2026 15:05:43 +0530 Subject: [PATCH 3/3] test: test sync between evaluator doc and role --- .../course_evaluator/test_course_evaluator.py | 58 +++++++++++++++++++ 1 file changed, 58 insertions(+) diff --git a/lms/lms/doctype/course_evaluator/test_course_evaluator.py b/lms/lms/doctype/course_evaluator/test_course_evaluator.py index e3e7f9af..7a4dc4a8 100644 --- a/lms/lms/doctype/course_evaluator/test_course_evaluator.py +++ b/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")