From 176de06291aafe4a8f8c9a3a9afdec24cedb332e Mon Sep 17 00:00:00 2001 From: 0x29a Date: Tue, 13 Feb 2024 11:58:09 +0100 Subject: [PATCH] feat: Teaching Assistant role --- common/djangoapps/student/roles.py | 13 ++++- common/djangoapps/student/tests/test_roles.py | 2 + lms/djangoapps/instructor/access.py | 2 + lms/djangoapps/instructor/permissions.py | 27 ++++++--- .../tests/views/test_instructor_dashboard.py | 55 +++++++++++++++++-- .../instructor/views/instructor_dashboard.py | 8 ++- lms/envs/common.py | 10 ++++ .../instructor_dashboard/membership.html | 1 + .../instructor_dashboard_2/membership.html | 22 +++++++- .../instructor_dashboard_2/student_admin.html | 16 ++++-- 10 files changed, 135 insertions(+), 21 deletions(-) diff --git a/common/djangoapps/student/roles.py b/common/djangoapps/student/roles.py index ff6be033f9f5..9da8e5bd33ae 100644 --- a/common/djangoapps/student/roles.py +++ b/common/djangoapps/student/roles.py @@ -300,12 +300,23 @@ class CourseLimitedStaffRole(CourseStaffRole): @register_access_role class eSHEInstructorRole(CourseStaffRole): - """A Staff member of a course without access to Studio.""" + """A Staff member of a course without access to the membership tab and enrollment-related operations.""" ROLE = 'eshe_instructor' BASE_ROLE = CourseStaffRole.ROLE +@register_access_role +class TeachingAssistantRole(CourseStaffRole): + """ + A Staff member of a course without access to the membership tab, enrollment-related operations and + grade-related operations. + """ + + ROLE = 'teaching_assistant' + BASE_ROLE = CourseStaffRole.ROLE + + @register_access_role class CourseInstructorRole(CourseRole): """A course Instructor""" diff --git a/common/djangoapps/student/tests/test_roles.py b/common/djangoapps/student/tests/test_roles.py index 831d33045d66..cc51622913f2 100644 --- a/common/djangoapps/student/tests/test_roles.py +++ b/common/djangoapps/student/tests/test_roles.py @@ -17,6 +17,7 @@ CourseFinanceAdminRole, CourseSalesAdminRole, eSHEInstructorRole, + TeachingAssistantRole, LibraryUserRole, CourseDataResearcherRole, GlobalStaff, @@ -170,6 +171,7 @@ class RoleCacheTestCase(TestCase): # lint-amnesty, pylint: disable=missing-clas (CourseStaffRole(IN_KEY), ('staff', IN_KEY, 'edX')), (CourseLimitedStaffRole(IN_KEY), ('limited_staff', IN_KEY, 'edX')), (eSHEInstructorRole(IN_KEY), ('eshe_instructor', IN_KEY, 'edX')), + (TeachingAssistantRole(IN_KEY), ('teaching_assistant', IN_KEY, 'edX')), (CourseInstructorRole(IN_KEY), ('instructor', IN_KEY, 'edX')), (OrgStaffRole(IN_KEY.org), ('staff', None, 'edX')), (CourseFinanceAdminRole(IN_KEY), ('finance_admin', IN_KEY, 'edX')), diff --git a/lms/djangoapps/instructor/access.py b/lms/djangoapps/instructor/access.py index 343b6923ccee..c5bc5fb83370 100644 --- a/lms/djangoapps/instructor/access.py +++ b/lms/djangoapps/instructor/access.py @@ -20,6 +20,7 @@ CourseLimitedStaffRole, CourseStaffRole, eSHEInstructorRole, + TeachingAssistantRole, ) from lms.djangoapps.instructor.enrollment import enroll_email, get_email_params from openedx.core.djangoapps.django_comment_common.models import Role @@ -32,6 +33,7 @@ 'staff': CourseStaffRole, 'limited_staff': CourseLimitedStaffRole, 'eshe_instructor': eSHEInstructorRole, + 'teaching_assistant': TeachingAssistantRole, 'ccx_coach': CourseCcxCoachRole, 'data_researcher': CourseDataResearcherRole, } diff --git a/lms/djangoapps/instructor/permissions.py b/lms/djangoapps/instructor/permissions.py index 0d8353653b93..80dc649e5a95 100644 --- a/lms/djangoapps/instructor/permissions.py +++ b/lms/djangoapps/instructor/permissions.py @@ -32,6 +32,21 @@ VIEW_ENROLLMENTS = 'instructor.view_enrollments' VIEW_FORUM_MEMBERS = 'instructor.view_forum_members' +# Due to how the roles iheritance is implemented currently, eshe_instructor and teaching_assistant have implicit +# staff access, but unlike staff, they shouldn't be able to enroll and do grade-related operations as per client's +# requirements. At the same time, all other staff-derived roles, like Limited Staff, should be able to enroll students. +# This solution is far from perfect, but it's probably the best we can do untill the roles system is reworked. +_is_teaching_assistant = HasRolesRule('teaching_assistant') +_is_eshe_instructor = HasRolesRule('eshe_instructor') +_is_eshe_instructor_or_teaching_assistant = _is_teaching_assistant | _is_eshe_instructor +is_staff_but_not_teaching_assistant = ( + (_is_teaching_assistant & HasAccessRule('staff', strict=True)) | + (~_is_teaching_assistant & HasAccessRule('staff')) +) +is_staff_but_not_eshe_instructor_or_teaching_assistant = ( + (_is_eshe_instructor_or_teaching_assistant & HasAccessRule('staff', strict=True)) | + (~_is_eshe_instructor_or_teaching_assistant & HasAccessRule('staff')) +) perms[ALLOW_STUDENT_TO_BYPASS_ENTRANCE_EXAM] = HasAccessRule('staff') perms[ASSIGN_TO_COHORTS] = HasAccessRule('staff') @@ -41,23 +56,17 @@ perms[ENABLE_CERTIFICATE_GENERATION] = is_staff perms[GENERATE_CERTIFICATE_EXCEPTIONS] = is_staff perms[GENERATE_BULK_CERTIFICATE_EXCEPTIONS] = is_staff -perms[GIVE_STUDENT_EXTENSION] = HasAccessRule('staff') +perms[GIVE_STUDENT_EXTENSION] = is_staff_but_not_teaching_assistant perms[VIEW_ISSUED_CERTIFICATES] = HasAccessRule('staff') | HasRolesRule('data_researcher') # only global staff or those with the data_researcher role can access the data download tab # HasAccessRule('staff') also includes course staff perms[CAN_RESEARCH] = is_staff | HasRolesRule('data_researcher') -# eshe_instructor implicitly gets staff access, but shouldn't be able to enroll -perms[CAN_ENROLL] = ( - # can enroll if a user is an eshe_instructor and has an explicit staff role - (HasRolesRule('eshe_instructor') & HasAccessRule('staff', strict=True)) | - # can enroll if a user is just staff - (~HasRolesRule('eshe_instructor') & HasAccessRule('staff')) -) +perms[CAN_ENROLL] = is_staff_but_not_eshe_instructor_or_teaching_assistant perms[CAN_BETATEST] = HasAccessRule('instructor') perms[ENROLLMENT_REPORT] = HasAccessRule('staff') | HasRolesRule('data_researcher') perms[VIEW_COUPONS] = HasAccessRule('staff') | HasRolesRule('data_researcher') perms[EXAM_RESULTS] = HasAccessRule('staff') -perms[OVERRIDE_GRADES] = HasAccessRule('staff') +perms[OVERRIDE_GRADES] = is_staff_but_not_teaching_assistant perms[SHOW_TASKS] = HasAccessRule('staff') | HasRolesRule('data_researcher') perms[EMAIL] = HasAccessRule('staff') perms[RESCORE_EXAMS] = HasAccessRule('instructor') diff --git a/lms/djangoapps/instructor/tests/views/test_instructor_dashboard.py b/lms/djangoapps/instructor/tests/views/test_instructor_dashboard.py index 6572379b6eb9..058b3d579c9b 100644 --- a/lms/djangoapps/instructor/tests/views/test_instructor_dashboard.py +++ b/lms/djangoapps/instructor/tests/views/test_instructor_dashboard.py @@ -316,9 +316,10 @@ def test_membership_reason_field_visibility(self, enbale_reason_field): else: self.assertNotContains(response, reason_field) - def test_membership_tab_for_eshe_instructor(self): + @ddt.data('eshe_instructor', 'teaching_assistant') + def test_membership_tab_content(self, role): """ - Verify that eSHE instructors don't have access to membership tab and + Verify that eSHE Instructors and Teaching Assistants don't have access to membership tab and work correctly with other roles. """ @@ -336,11 +337,11 @@ def test_membership_tab_for_eshe_instructor(self): user = UserFactory.create() self.client.login(username=user.username, password="test") - # eSHE instructors shouldn't have access to membership tab + # eSHE Instructors / Teaching Assistants shouldn't have access to membership tab CourseAccessRoleFactory( course_id=self.course.id, user=user, - role='eshe_instructor', + role=role, org=self.course.id.org ) response = self.client.get(self.url) @@ -366,6 +367,52 @@ def test_membership_tab_for_eshe_instructor(self): self.assertContains(response, membership_section) self.assertContains(response, batch_enrollment) + def test_student_admin_tab_content(self): + """ + Verify that Teaching Assistants don't have access to the gradebook-related sections + of the student admin tab. + """ + + # Should be visible to Teaching Assistants + view_enrollment_status = '
' + view_progress = '
' + + # Should not be visible to Teaching Assistants + view_gradebook = '
' + adjust_learner_grade = '
' + adjust_all_learners_grades = '
' + + user = UserFactory.create() + self.client.login(username=user.username, password="test") + + # Teaching Assistants shouldn't have access to the gradebook-related sections + CourseAccessRoleFactory( + course_id=self.course.id, + user=user, + role='teaching_assistant', + org=self.course.id.org + ) + response = self.client.get(self.url) + self.assertContains(response, view_enrollment_status) + self.assertContains(response, view_progress) + self.assertNotContains(response, view_gradebook) + self.assertNotContains(response, adjust_learner_grade) + self.assertNotContains(response, adjust_all_learners_grades) + + # However if combined with instructor, they should have access to all sections + CourseAccessRoleFactory( + course_id=self.course.id, + user=user, + role='instructor', + org=self.course.id.org + ) + response = self.client.get(self.url) + self.assertContains(response, view_enrollment_status) + self.assertContains(response, view_progress) + self.assertContains(response, view_gradebook) + self.assertContains(response, adjust_learner_grade) + self.assertContains(response, adjust_all_learners_grades) + def test_student_admin_staff_instructor(self): """ Verify that staff users are not able to see course-wide options, while still diff --git a/lms/djangoapps/instructor/views/instructor_dashboard.py b/lms/djangoapps/instructor/views/instructor_dashboard.py index 25ec258aab4a..1e5ddf582c39 100644 --- a/lms/djangoapps/instructor/views/instructor_dashboard.py +++ b/lms/djangoapps/instructor/views/instructor_dashboard.py @@ -37,6 +37,7 @@ CourseSalesAdminRole, CourseStaffRole, eSHEInstructorRole, + TeachingAssistantRole, strict_role_checking, ) from common.djangoapps.util.json_request import JsonResponse @@ -132,6 +133,7 @@ def instructor_dashboard_2(request, course_id): # lint-amnesty, pylint: disable 'admin': request.user.is_staff, 'instructor': bool(has_access(request.user, 'instructor', course)), 'eshe_instructor': eSHEInstructorRole(course_key).has_user(request.user), + 'teaching_assistant': TeachingAssistantRole(course_key).has_user(request.user), 'finance_admin': CourseFinanceAdminRole(course_key).has_user(request.user), 'sales_admin': CourseSalesAdminRole(course_key).has_user(request.user), 'staff': bool(has_access(request.user, 'staff', course)), @@ -506,7 +508,11 @@ def _section_membership(course, access): # section if the user doesn't have the Course Staff role set explicitly # or have the Discussion Admin role. 'is_hidden': ( - not access['forum_admin'] and (access['eshe_instructor'] and not access['explicit_staff']) + not access['forum_admin'] + and ( + (access['eshe_instructor'] or access['teaching_assistant']) + and not access['explicit_staff'] + ) ), } return section_data diff --git a/lms/envs/common.py b/lms/envs/common.py index f3e8d1533b89..419c966e1bb1 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -1069,6 +1069,16 @@ # .. toggle_target_removal_date: None # .. toggle_tickets: 'https://github.com/open-craft/edx-platform/pull/561/files' 'ENABLE_ESHE_INSTRUCTOR_ROLE': False, + + # .. toggle_name: FEATURES['ENABLE_TEACHING_ASSISTANT_ROLE'] + # .. toggle_implementation: DjangoSetting + # .. toggle_default: False + # .. toggle_description: Whether to enable the Teaching Assistant role + # .. toggle_use_cases: open_edx + # .. toggle_creation_date: 2024-02-12 + # .. toggle_target_removal_date: None + # .. toggle_tickets: 'https://github.com/open-craft/edx-platform/pull/632/files' + 'ENABLE_TEACHING_ASSISTANT_ROLE': False, } # Specifies extra XBlock fields that should available when requested via the Course Blocks API diff --git a/lms/static/js/fixtures/instructor_dashboard/membership.html b/lms/static/js/fixtures/instructor_dashboard/membership.html index 6b77a148df69..a33a5525d78a 100644 --- a/lms/static/js/fixtures/instructor_dashboard/membership.html +++ b/lms/static/js/fixtures/instructor_dashboard/membership.html @@ -12,6 +12,7 @@

Course Team Management

+ diff --git a/lms/templates/instructor/instructor_dashboard_2/membership.html b/lms/templates/instructor/instructor_dashboard_2/membership.html index 3e0affc7fd29..c6403b2d5a98 100644 --- a/lms/templates/instructor/instructor_dashboard_2/membership.html +++ b/lms/templates/instructor/instructor_dashboard_2/membership.html @@ -5,7 +5,13 @@ from django.utils.translation import pgettext from openedx.core.djangolib.markup import HTML, Text %> -% if not section_data['access']['eshe_instructor'] or section_data['access']['explicit_staff']: + +<% + eshe_instructor_or_teaching_assistant_access = section_data['access']['eshe_instructor'] or section_data['access']['teaching_assistant'] + membership_section_visible = not eshe_instructor_or_teaching_assistant_access or section_data['access']['explicit_staff'] +%> + +% if membership_section_visible:
${_("Batch Enrollment")}
%endif + %if settings.FEATURES.get('ENABLE_TEACHING_ASSISTANT_ROLE', None): +
+ %endif% + ## Note that "Admin" is identified as "Instructor" in the Django admin panel.
<%! from django.utils.translation import ugettext as _ %> + +<% + teaching_assistant_access = section_data['access']['teaching_assistant'] + gradebook_related_sections_hidden = teaching_assistant_access and not section_data['access']['explicit_staff'] +%> + %if section_data['access']['staff'] or section_data['access']['instructor']: -
+
%if section_data.get('writable_gradebook_url') or section_data.get('is_small_course'):

${_("View gradebook for enrolled learners")}

@@ -59,7 +65,7 @@

${_("View a specific learner's grades and progress")}

-
+

${_("Adjust a learner's grade for a specific problem")}

% if course.entrance_exam_enabled: -
+

${_("Adjust a learner's entrance exam results")}

@@ -194,7 +200,7 @@
${_("Task Status")}
%if section_data['access']['instructor']: %if settings.FEATURES.get('ENABLE_INSTRUCTOR_BACKGROUND_TASKS'): -
+

${_("Adjust all enrolled learners' grades for a specific problem")}

@@ -232,7 +238,7 @@
${_("Task Status")}
%endif %if settings.FEATURES.get('ENABLE_INSTRUCTOR_BACKGROUND_TASKS'): -
+

${_("Pending Tasks")}