Skip to content

Commit 5d160c2

Browse files
committed
feat: eSHE Instructor role
Adds the eSHE Instructor role, which inherits Course Staff permissions, but isn't able to enroll / un-enroll students and can't assing course team roles unless in combination with Course Staff / Instructor / Discussion admin roles.
1 parent 86041f9 commit 5d160c2

File tree

10 files changed

+137
-9
lines changed

10 files changed

+137
-9
lines changed

common/djangoapps/student/roles.py

Lines changed: 24 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
import logging
88
from abc import ABCMeta, abstractmethod
99
from collections import defaultdict
10+
from contextlib import contextmanager
1011

1112
from django.contrib.auth.models import User # lint-amnesty, pylint: disable=imported-auth-user
1213
from opaque_keys.edx.django.models import CourseKeyField
@@ -44,6 +45,17 @@ def register_access_role(cls):
4445
return cls
4546

4647

48+
@contextmanager
49+
def strict_role_checking():
50+
"""
51+
Context manager that temporarily disables role inheritance.
52+
"""
53+
OLD_ACCESS_ROLES_INHERITANCE = ACCESS_ROLES_INHERITANCE.copy()
54+
ACCESS_ROLES_INHERITANCE.clear()
55+
yield
56+
ACCESS_ROLES_INHERITANCE.update(OLD_ACCESS_ROLES_INHERITANCE)
57+
58+
4759
class BulkRoleCache: # lint-amnesty, pylint: disable=missing-class-docstring
4860
CACHE_NAMESPACE = "student.roles.BulkRoleCache"
4961
CACHE_KEY = 'roles_by_user'
@@ -78,7 +90,7 @@ def __init__(self, user):
7890
)
7991

8092
@staticmethod
81-
def _get_roles(role):
93+
def get_roles(role):
8294
"""
8395
Return the roles that should have the same permissions as the specified role.
8496
"""
@@ -90,7 +102,7 @@ def has_role(self, role, course_id, org):
90102
or a role that inherits from the specified role, course_id and org.
91103
"""
92104
return any(
93-
access_role.role in self._get_roles(role) and
105+
access_role.role in self.get_roles(role) and
94106
access_role.course_id == course_id and
95107
access_role.org == org
96108
for access_role in self._roles
@@ -286,6 +298,14 @@ class CourseLimitedStaffRole(CourseStaffRole):
286298
BASE_ROLE = CourseStaffRole.ROLE
287299

288300

301+
@register_access_role
302+
class eSHEInstructorRole(CourseStaffRole):
303+
"""A Staff member of a course without access to Studio."""
304+
305+
ROLE = 'eshe_instructor'
306+
BASE_ROLE = CourseStaffRole.ROLE
307+
308+
289309
@register_access_role
290310
class CourseInstructorRole(CourseRole):
291311
"""A course Instructor"""
@@ -463,11 +483,11 @@ def remove_courses(self, *course_keys):
463483

464484
def courses_with_role(self):
465485
"""
466-
Return a django QuerySet for all of the courses with this user x role. You can access
486+
Return a django QuerySet for all of the courses with this user x (or derived from x) role. You can access
467487
any of these properties on each result record:
468488
* user (will be self.user--thus uninteresting)
469489
* org
470490
* course_id
471491
* role (will be self.role--thus uninteresting)
472492
"""
473-
return CourseAccessRole.objects.filter(role=self.role, user=self.user)
493+
return CourseAccessRole.objects.filter(role__in=RoleCache.get_roles(self.role), user=self.user)

common/djangoapps/student/tests/test_roles.py

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
CourseStaffRole,
1717
CourseFinanceAdminRole,
1818
CourseSalesAdminRole,
19+
eSHEInstructorRole,
1920
LibraryUserRole,
2021
CourseDataResearcherRole,
2122
GlobalStaff,
@@ -168,6 +169,7 @@ class RoleCacheTestCase(TestCase): # lint-amnesty, pylint: disable=missing-clas
168169
ROLES = (
169170
(CourseStaffRole(IN_KEY), ('staff', IN_KEY, 'edX')),
170171
(CourseLimitedStaffRole(IN_KEY), ('limited_staff', IN_KEY, 'edX')),
172+
(eSHEInstructorRole(IN_KEY), ('eshe_instructor', IN_KEY, 'edX')),
171173
(CourseInstructorRole(IN_KEY), ('instructor', IN_KEY, 'edX')),
172174
(OrgStaffRole(IN_KEY.org), ('staff', None, 'edX')),
173175
(CourseFinanceAdminRole(IN_KEY), ('finance_admin', IN_KEY, 'edX')),

lms/djangoapps/courseware/rules.py

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@
1818
from openedx.core.djangoapps.content.course_overviews.models import CourseOverview
1919
from openedx.core.djangoapps.enrollments.api import is_enrollment_valid_for_proctoring
2020
from common.djangoapps.student.models import CourseAccessRole
21-
from common.djangoapps.student.roles import CourseRole, OrgRole
21+
from common.djangoapps.student.roles import CourseRole, OrgRole, strict_role_checking
2222
from xmodule.course_block import CourseBlock # lint-amnesty, pylint: disable=wrong-import-order
2323
from xmodule.error_block import ErrorBlock # lint-amnesty, pylint: disable=wrong-import-order
2424

@@ -47,10 +47,14 @@ class HasAccessRule(Rule):
4747
"""
4848
A rule that calls `has_access` to determine whether it passes
4949
"""
50-
def __init__(self, action):
50+
def __init__(self, action, strict=False):
5151
self.action = action
52+
self.strict = strict
5253

5354
def check(self, user, instance=None):
55+
if self.strict:
56+
with strict_role_checking():
57+
return has_access(user, self.action, instance)
5458
return has_access(user, self.action, instance)
5559

5660
def query(self, user):

lms/djangoapps/instructor/access.py

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919
CourseInstructorRole,
2020
CourseLimitedStaffRole,
2121
CourseStaffRole,
22+
eSHEInstructorRole,
2223
)
2324
from lms.djangoapps.instructor.enrollment import enroll_email, get_email_params
2425
from openedx.core.djangoapps.django_comment_common.models import Role
@@ -30,6 +31,7 @@
3031
'instructor': CourseInstructorRole,
3132
'staff': CourseStaffRole,
3233
'limited_staff': CourseLimitedStaffRole,
34+
'eshe_instructor': eSHEInstructorRole,
3335
'ccx_coach': CourseCcxCoachRole,
3436
'data_researcher': CourseDataResearcherRole,
3537
}

lms/djangoapps/instructor/permissions.py

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,13 @@
4444
# only global staff or those with the data_researcher role can access the data download tab
4545
# HasAccessRule('staff') also includes course staff
4646
perms[CAN_RESEARCH] = is_staff | HasRolesRule('data_researcher')
47-
perms[CAN_ENROLL] = HasAccessRule('staff')
47+
# eshe_instructor implicitly gets staff access, but shouldn't be able to enroll
48+
perms[CAN_ENROLL] = (
49+
# can enroll if a user is an eshe_instructor and has an explicit staff role
50+
(HasRolesRule('eshe_instructor') & HasAccessRule('staff', strict=True)) |
51+
# can enroll if a user is just staff
52+
(~HasRolesRule('eshe_instructor') & HasAccessRule('staff'))
53+
)
4854
perms[CAN_BETATEST] = HasAccessRule('instructor')
4955
perms[ENROLLMENT_REPORT] = HasAccessRule('staff') | HasRolesRule('data_researcher')
5056
perms[VIEW_COUPONS] = HasAccessRule('staff') | HasRolesRule('data_researcher')

lms/djangoapps/instructor/tests/views/test_instructor_dashboard.py

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@
2929
from lms.djangoapps.courseware.tabs import get_course_tab_list
3030
from lms.djangoapps.courseware.tests.factories import StudentModuleFactory
3131
from lms.djangoapps.courseware.tests.helpers import LoginEnrollmentTestCase
32+
from lms.djangoapps.discussion.django_comment_client.tests.factories import RoleFactory
3233
from lms.djangoapps.grades.config.waffle import WRITABLE_GRADEBOOK
3334
from lms.djangoapps.instructor.toggles import DATA_DOWNLOAD_V2
3435
from lms.djangoapps.instructor.views.gradebook_api import calculate_page_info
@@ -37,6 +38,7 @@
3738
ENABLE_PAGES_AND_RESOURCES_MICROFRONTEND,
3839
OVERRIDE_DISCUSSION_LEGACY_SETTINGS_FLAG
3940
)
41+
from openedx.core.djangoapps.django_comment_common.models import FORUM_ROLE_ADMINISTRATOR
4042
from openedx.core.djangoapps.site_configuration.models import SiteConfiguration
4143

4244

@@ -308,6 +310,56 @@ def test_membership_reason_field_visibility(self, enbale_reason_field):
308310
else:
309311
self.assertNotContains(response, reason_field)
310312

313+
def test_membership_tab_for_eshe_instructor(self):
314+
"""
315+
Verify that eSHE instructors don't have access to membership tab and
316+
work correctly with other roles.
317+
"""
318+
319+
membership_section = (
320+
'<li class="nav-item">'
321+
'<button type="button" class="btn-link membership" data-section="membership">'
322+
'Membership'
323+
'</button>'
324+
'</li>'
325+
)
326+
batch_enrollment = (
327+
'<fieldset class="batch-enrollment membership-section">'
328+
)
329+
330+
user = UserFactory.create()
331+
self.client.login(username=user.username, password="test")
332+
333+
# eSHE instructors shouldn't have access to membership tab
334+
CourseAccessRoleFactory(
335+
course_id=self.course.id,
336+
user=user,
337+
role='eshe_instructor',
338+
org=self.course.id.org
339+
)
340+
response = self.client.get(self.url)
341+
self.assertNotContains(response, membership_section)
342+
343+
# However if combined with forum_admin, they should have access to this
344+
# tab, but not to batch enrollment
345+
forum_admin_role = RoleFactory(name=FORUM_ROLE_ADMINISTRATOR, course_id=self.course.id)
346+
forum_admin_role.users.add(user)
347+
response = self.client.get(self.url)
348+
self.assertContains(response, membership_section)
349+
self.assertNotContains(response, batch_enrollment)
350+
351+
# Combined with course staff, should have union of all three roles
352+
# permissions sets
353+
CourseAccessRoleFactory(
354+
course_id=self.course.id,
355+
user=user,
356+
role='staff',
357+
org=self.course.id.org
358+
)
359+
response = self.client.get(self.url)
360+
self.assertContains(response, membership_section)
361+
self.assertContains(response, batch_enrollment)
362+
311363
def test_student_admin_staff_instructor(self):
312364
"""
313365
Verify that staff users are not able to see course-wide options, while still

lms/djangoapps/instructor/views/instructor_dashboard.py

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,9 @@
3535
CourseFinanceAdminRole,
3636
CourseInstructorRole,
3737
CourseSalesAdminRole,
38-
CourseStaffRole
38+
CourseStaffRole,
39+
eSHEInstructorRole,
40+
strict_role_checking,
3941
)
4042
from common.djangoapps.util.json_request import JsonResponse
4143
from lms.djangoapps.bulk_email.api import is_bulk_email_feature_enabled
@@ -126,13 +128,17 @@ def instructor_dashboard_2(request, course_id): # lint-amnesty, pylint: disable
126128
access = {
127129
'admin': request.user.is_staff,
128130
'instructor': bool(has_access(request.user, 'instructor', course)),
131+
'eshe_instructor': eSHEInstructorRole(course_key).has_user(request.user),
129132
'finance_admin': CourseFinanceAdminRole(course_key).has_user(request.user),
130133
'sales_admin': CourseSalesAdminRole(course_key).has_user(request.user),
131134
'staff': bool(has_access(request.user, 'staff', course)),
132135
'forum_admin': has_forum_access(request.user, course_key, FORUM_ROLE_ADMINISTRATOR),
133136
'data_researcher': request.user.has_perm(permissions.CAN_RESEARCH, course_key),
134137
}
135138

139+
with strict_role_checking():
140+
access['explicit_staff'] = bool(has_access(request.user, 'staff', course))
141+
136142
if not request.user.has_perm(permissions.VIEW_DASHBOARD, course_key):
137143
raise Http404()
138144

@@ -490,7 +496,15 @@ def _section_membership(course, access):
490496
'update_forum_role_membership',
491497
kwargs={'course_id': str(course_key)}
492498
),
493-
'is_reason_field_enabled': configuration_helpers.get_value('ENABLE_MANUAL_ENROLLMENT_REASON_FIELD', False)
499+
'is_reason_field_enabled': configuration_helpers.get_value('ENABLE_MANUAL_ENROLLMENT_REASON_FIELD', False),
500+
501+
# Membership section should be hidden for eSHE instructors.
502+
# Since they get Course Staff role implicitly, we need to hide this
503+
# section if the user doesn't have the Course Staff role set explicitly
504+
# or have the Discussion Admin role.
505+
'is_hidden': (
506+
not access['forum_admin'] and (access['eshe_instructor'] and not access['explicit_staff'])
507+
),
494508
}
495509
return section_data
496510

lms/envs/common.py

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1048,6 +1048,16 @@
10481048
# .. toggle_target_removal_date: None
10491049
# .. toggle_tickets: 'https://github.com/openedx/edx-platform/pull/30832'
10501050
'ENABLE_LEGACY_MD5_HASH_FOR_ANONYMOUS_USER_ID': False,
1051+
1052+
# .. toggle_name: FEATURES['ENABLE_ESHE_INSTRUCTOR_ROLE']
1053+
# .. toggle_implementation: DjangoSetting
1054+
# .. toggle_default: False
1055+
# .. toggle_description: Whether to enable the ESHE Instructor role
1056+
# .. toggle_use_cases: open_edx
1057+
# .. toggle_creation_date: 2023-07-31
1058+
# .. toggle_target_removal_date: None
1059+
# .. toggle_tickets: 'https://github.com/open-craft/edx-platform/pull/561/files'
1060+
'ENABLE_ESHE_INSTRUCTOR_ROLE': False,
10511061
}
10521062

10531063
# Specifies extra XBlock fields that should available when requested via the Course Blocks API

lms/static/js/fixtures/instructor_dashboard/membership.html

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ <h3 class="hd hd-3">Course Team Management</h3>
1111
<select id="member-lists-selector" class="member-lists-selector">
1212
<option value="staff">Staff</option>
1313
<option value="limited_staff">Limited Staff</option>
14+
<option value="eshe_instructor">eSHE Instructor</option>
1415
<option value="instructor">Admin</option>
1516
<option value="beta">Beta Testers</option>
1617
<option value="Administrator">Discussion Admins</option>

lms/templates/instructor/instructor_dashboard_2/membership.html

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
from django.utils.translation import pgettext
66
from openedx.core.djangolib.markup import HTML, Text
77
%>
8+
% if not section_data['access']['eshe_instructor'] or section_data['access']['explicit_staff']:
89
<fieldset class="batch-enrollment membership-section">
910
<legend id="heading-batch-enrollment" class="hd hd-3">${_("Batch Enrollment")}</legend>
1011
<label>
@@ -92,6 +93,8 @@ <h3 class="hd hd-3">${_("Register/Enroll Students")}</h3>
9293
</div>
9394
%endif
9495

96+
%endif
97+
9598
<hr class="divider" />
9699

97100
%if section_data['access']['instructor']:
@@ -191,6 +194,20 @@ <h3 class="hd hd-3">${_("Course Team Management")}</h3>
191194
data-add-button-label="${_("Add Limited Staff")}"
192195
></div>
193196

197+
%if settings.FEATURES.get('ENABLE_ESHE_INSTRUCTOR_ROLE', None):
198+
<div class="auth-list-container"
199+
data-rolename="eshe_instructor"
200+
data-display-name="${_("eSHE Instructor")}"
201+
data-info-text="
202+
${_("Course team members with the eSHE Instructor role help you manage your course. "
203+
"eSHE Instructor permissions are similar to Staff, but they can't assign course team roles and "
204+
"can't enroll and unenroll learners")}"
205+
data-list-endpoint="${ section_data['list_course_role_members_url'] }"
206+
data-modify-endpoint="${ section_data['modify_access_url'] }"
207+
data-add-button-label="${_("Add eSHE Instructor")}"
208+
></div>
209+
%endif
210+
194211
## Note that "Admin" is identified as "Instructor" in the Django admin panel.
195212
<div class="auth-list-container"
196213
data-rolename="instructor"

0 commit comments

Comments
 (0)