From eba8b0c76a109f7e511946457d2b08599ef0e4e9 Mon Sep 17 00:00:00 2001
From: 0x29a <demid@opencraft.com>
Date: Thu, 27 Jul 2023 14:31:43 +0200
Subject: [PATCH 1/2] 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.
---
 common/djangoapps/student/roles.py            | 20 +++++++++++++++++++
 common/djangoapps/student/tests/test_roles.py |  2 ++
 lms/djangoapps/courseware/rules.py            |  8 ++++++--
 lms/djangoapps/instructor/access.py           |  2 ++
 lms/djangoapps/instructor/permissions.py      |  8 +++++++-
 .../instructor/views/instructor_dashboard.py  | 18 +++++++++++++++--
 .../instructor_dashboard/membership.html      |  1 +
 .../instructor_dashboard_2/membership.html    | 14 +++++++++++++
 8 files changed, 68 insertions(+), 5 deletions(-)

diff --git a/common/djangoapps/student/roles.py b/common/djangoapps/student/roles.py
index fe2b6c91d689..8a591fd9bf3b 100644
--- a/common/djangoapps/student/roles.py
+++ b/common/djangoapps/student/roles.py
@@ -7,6 +7,7 @@
 import logging
 from abc import ABCMeta, abstractmethod
 from collections import defaultdict
+from contextlib import contextmanager
 
 from django.contrib.auth.models import User  # lint-amnesty, pylint: disable=imported-auth-user
 from opaque_keys.edx.django.models import CourseKeyField
@@ -44,6 +45,17 @@ def register_access_role(cls):
     return cls
 
 
+@contextmanager
+def strict_role_checking():
+    """
+    Context manager that temporarily disables role inheritance.
+    """
+    OLD_ACCESS_ROLES_INHERITANCE = ACCESS_ROLES_INHERITANCE.copy()
+    ACCESS_ROLES_INHERITANCE.clear()
+    yield
+    ACCESS_ROLES_INHERITANCE.update(OLD_ACCESS_ROLES_INHERITANCE)
+
+
 class BulkRoleCache:  # lint-amnesty, pylint: disable=missing-class-docstring
     CACHE_NAMESPACE = "student.roles.BulkRoleCache"
     CACHE_KEY = 'roles_by_user'
@@ -286,6 +298,14 @@ class CourseLimitedStaffRole(CourseStaffRole):
     BASE_ROLE = CourseStaffRole.ROLE
 
 
+@register_access_role
+class eSHEInstructorRole(CourseStaffRole):
+    """A Staff member of a course without access to Studio."""
+
+    ROLE = 'eshe_instructor'
+    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 c670c7c2b75b..831d33045d66 100644
--- a/common/djangoapps/student/tests/test_roles.py
+++ b/common/djangoapps/student/tests/test_roles.py
@@ -16,6 +16,7 @@
     CourseStaffRole,
     CourseFinanceAdminRole,
     CourseSalesAdminRole,
+    eSHEInstructorRole,
     LibraryUserRole,
     CourseDataResearcherRole,
     GlobalStaff,
@@ -168,6 +169,7 @@ class RoleCacheTestCase(TestCase):  # lint-amnesty, pylint: disable=missing-clas
     ROLES = (
         (CourseStaffRole(IN_KEY), ('staff', IN_KEY, 'edX')),
         (CourseLimitedStaffRole(IN_KEY), ('limited_staff', IN_KEY, 'edX')),
+        (eSHEInstructorRole(IN_KEY), ('eshe_instructor', 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/courseware/rules.py b/lms/djangoapps/courseware/rules.py
index 25931b6a894b..678edc27ccf2 100644
--- a/lms/djangoapps/courseware/rules.py
+++ b/lms/djangoapps/courseware/rules.py
@@ -18,7 +18,7 @@
 from openedx.core.djangoapps.content.course_overviews.models import CourseOverview
 from openedx.core.djangoapps.enrollments.api import is_enrollment_valid_for_proctoring
 from common.djangoapps.student.models import CourseAccessRole
-from common.djangoapps.student.roles import CourseRole, OrgRole
+from common.djangoapps.student.roles import CourseRole, OrgRole, strict_role_checking
 from xmodule.course_module import CourseBlock  # lint-amnesty, pylint: disable=wrong-import-order
 from xmodule.error_module import ErrorBlock  # lint-amnesty, pylint: disable=wrong-import-order
 from xmodule.x_module import XModule  # lint-amnesty, pylint: disable=wrong-import-order
@@ -48,10 +48,14 @@ class HasAccessRule(Rule):
     """
     A rule that calls `has_access` to determine whether it passes
     """
-    def __init__(self, action):
+    def __init__(self, action, strict=False):
         self.action = action
+        self.strict = strict
 
     def check(self, user, instance=None):
+        if self.strict:
+            with strict_role_checking():
+                return has_access(user, self.action, instance)
         return has_access(user, self.action, instance)
 
     def query(self, user):
diff --git a/lms/djangoapps/instructor/access.py b/lms/djangoapps/instructor/access.py
index 9255d113f038..343b6923ccee 100644
--- a/lms/djangoapps/instructor/access.py
+++ b/lms/djangoapps/instructor/access.py
@@ -19,6 +19,7 @@
     CourseInstructorRole,
     CourseLimitedStaffRole,
     CourseStaffRole,
+    eSHEInstructorRole,
 )
 from lms.djangoapps.instructor.enrollment import enroll_email, get_email_params
 from openedx.core.djangoapps.django_comment_common.models import Role
@@ -30,6 +31,7 @@
     'instructor': CourseInstructorRole,
     'staff': CourseStaffRole,
     'limited_staff': CourseLimitedStaffRole,
+    'eshe_instructor': eSHEInstructorRole,
     'ccx_coach': CourseCcxCoachRole,
     'data_researcher': CourseDataResearcherRole,
 }
diff --git a/lms/djangoapps/instructor/permissions.py b/lms/djangoapps/instructor/permissions.py
index 20426761e66b..ac9b430acec1 100644
--- a/lms/djangoapps/instructor/permissions.py
+++ b/lms/djangoapps/instructor/permissions.py
@@ -44,7 +44,13 @@
 # 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')
-perms[CAN_ENROLL] = HasAccessRule('staff')
+# 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_BETATEST] = HasAccessRule('instructor')
 perms[ENROLLMENT_REPORT] = HasAccessRule('staff') | HasRolesRule('data_researcher')
 perms[VIEW_COUPONS] = HasAccessRule('staff') | HasRolesRule('data_researcher')
diff --git a/lms/djangoapps/instructor/views/instructor_dashboard.py b/lms/djangoapps/instructor/views/instructor_dashboard.py
index d10c4f80b6b2..39a2f549ec98 100644
--- a/lms/djangoapps/instructor/views/instructor_dashboard.py
+++ b/lms/djangoapps/instructor/views/instructor_dashboard.py
@@ -35,7 +35,9 @@
     CourseFinanceAdminRole,
     CourseInstructorRole,
     CourseSalesAdminRole,
-    CourseStaffRole
+    CourseStaffRole,
+    eSHEInstructorRole,
+    strict_role_checking,
 )
 from common.djangoapps.util.json_request import JsonResponse
 from lms.djangoapps.bulk_email.api import is_bulk_email_feature_enabled
@@ -124,6 +126,7 @@ def instructor_dashboard_2(request, course_id):  # lint-amnesty, pylint: disable
     access = {
         'admin': request.user.is_staff,
         'instructor': bool(has_access(request.user, 'instructor', course)),
+        'eshe_instructor': eSHEInstructorRole(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)),
@@ -131,6 +134,9 @@ def instructor_dashboard_2(request, course_id):  # lint-amnesty, pylint: disable
         'data_researcher': request.user.has_perm(permissions.CAN_RESEARCH, course_key),
     }
 
+    with strict_role_checking():
+        access['explicit_staff'] = bool(has_access(request.user, 'staff', course))
+
     if not request.user.has_perm(permissions.VIEW_DASHBOARD, course_key):
         raise Http404()
 
@@ -488,7 +494,15 @@ def _section_membership(course, access):
             'update_forum_role_membership',
             kwargs={'course_id': str(course_key)}
         ),
-        'is_reason_field_enabled': configuration_helpers.get_value('ENABLE_MANUAL_ENROLLMENT_REASON_FIELD', False)
+        'is_reason_field_enabled': configuration_helpers.get_value('ENABLE_MANUAL_ENROLLMENT_REASON_FIELD', False),
+
+        # Membership section should be hidden for eSHE instructors.
+        # Since they get Course Staff role implicitly, we need to hide this
+        # section if the user doesn't have the Course Staff role set explicitly
+        # or have the Discussion Admin role.
+        'is_hidden': (
+            access['eshe_instructor'] and not (access['explicit_staff'] or access['forum_admin'])
+        )
     }
     return section_data
 
diff --git a/lms/static/js/fixtures/instructor_dashboard/membership.html b/lms/static/js/fixtures/instructor_dashboard/membership.html
index 962bfac5b4f1..6b77a148df69 100644
--- a/lms/static/js/fixtures/instructor_dashboard/membership.html
+++ b/lms/static/js/fixtures/instructor_dashboard/membership.html
@@ -11,6 +11,7 @@ <h3 class="hd hd-3">Course Team Management</h3>
         <select id="member-lists-selector" class="member-lists-selector">
           <option value="staff">Staff</option>
           <option value="limited_staff">Limited Staff</option>
+          <option value="eshe_instructor">eSHE Instructor</option>
           <option value="instructor">Admin</option>
           <option value="beta">Beta Testers</option>
           <option value="Administrator">Discussion Admins</option>
diff --git a/lms/templates/instructor/instructor_dashboard_2/membership.html b/lms/templates/instructor/instructor_dashboard_2/membership.html
index 482b364c2b70..e27f506f1148 100644
--- a/lms/templates/instructor/instructor_dashboard_2/membership.html
+++ b/lms/templates/instructor/instructor_dashboard_2/membership.html
@@ -5,6 +5,7 @@
 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']:
 <fieldset class="batch-enrollment membership-section">
     <legend id="heading-batch-enrollment" class="hd hd-3">${_("Batch Enrollment")}</legend>
     <label>
@@ -56,6 +57,7 @@
     <div class="request-response"></div>
     <div class="request-response-error"></div>
 </fieldset>
+%endif
 
 %if static.get_value('ALLOW_AUTOMATED_SIGNUPS', settings.FEATURES.get('ALLOW_AUTOMATED_SIGNUPS', False)):
 <hr class="divider" />
@@ -190,6 +192,18 @@ <h3 class="hd hd-3">${_("Course Team Management")}</h3>
       data-add-button-label="${_("Add Limited Staff")}"
     ></div>
 
+    <div class="auth-list-container"
+      data-rolename="eshe_instructor"
+      data-display-name="${_("eSHE Instructor")}"
+      data-info-text="
+        ${_("Course team members with the eSHE Instructor role help you manage your course. "
+        "eSHE Instructor permissions are similar to Staff, but they can't assign course team roles and "
+        "can't enroll and unenroll learners")}"
+      data-list-endpoint="${ section_data['list_course_role_members_url'] }"
+      data-modify-endpoint="${ section_data['modify_access_url'] }"
+      data-add-button-label="${_("Add eSHE Instructor")}"
+    ></div>
+
     ## Note that "Admin" is identified as "Instructor" in the Django admin panel.
     <div class="auth-list-container"
       data-rolename="instructor"

From 9e1cb3a19f4c0d455497ccd6771468ac9f517b19 Mon Sep 17 00:00:00 2001
From: 0x29a <demid@opencraft.com>
Date: Mon, 7 Aug 2023 14:26:15 +0200
Subject: [PATCH 2/2] feat: feature flag, tests

---
 .../tests/views/test_instructor_dashboard.py  | 52 +++++++++++++++++++
 .../instructor/views/instructor_dashboard.py  |  4 +-
 lms/envs/common.py                            | 10 ++++
 .../instructor_dashboard_2/membership.html    |  5 +-
 4 files changed, 68 insertions(+), 3 deletions(-)

diff --git a/lms/djangoapps/instructor/tests/views/test_instructor_dashboard.py b/lms/djangoapps/instructor/tests/views/test_instructor_dashboard.py
index 4f0e49152e16..745ed00f8237 100644
--- a/lms/djangoapps/instructor/tests/views/test_instructor_dashboard.py
+++ b/lms/djangoapps/instructor/tests/views/test_instructor_dashboard.py
@@ -30,6 +30,7 @@
 from lms.djangoapps.courseware.tabs import get_course_tab_list
 from lms.djangoapps.courseware.tests.factories import StudentModuleFactory
 from lms.djangoapps.courseware.tests.helpers import LoginEnrollmentTestCase
+from lms.djangoapps.discussion.django_comment_client.tests.factories import RoleFactory
 from lms.djangoapps.grades.config.waffle import WRITABLE_GRADEBOOK, waffle_flags
 from lms.djangoapps.instructor.toggles import DATA_DOWNLOAD_V2
 from lms.djangoapps.instructor.views.gradebook_api import calculate_page_info
@@ -38,6 +39,7 @@
     ENABLE_PAGES_AND_RESOURCES_MICROFRONTEND,
     OVERRIDE_DISCUSSION_LEGACY_SETTINGS_FLAG
 )
+from openedx.core.djangoapps.django_comment_common.models import FORUM_ROLE_ADMINISTRATOR
 from openedx.core.djangoapps.site_configuration.models import SiteConfiguration
 
 
@@ -309,6 +311,56 @@ def test_membership_reason_field_visibility(self, enbale_reason_field):
         else:
             self.assertNotContains(response, reason_field)
 
+    def test_membership_tab_for_eshe_instructor(self):
+        """
+        Verify that eSHE instructors don't have access to membership tab and
+        work correctly with other roles.
+        """
+
+        membership_section = (
+            '<li class="nav-item">'
+            '<button type="button" class="btn-link membership" data-section="membership">'
+            'Membership'
+            '</button>'
+            '</li>'
+        )
+        batch_enrollment = (
+            '<fieldset class="batch-enrollment membership-section">'
+        )
+
+        user = UserFactory.create()
+        self.client.login(username=user.username, password="test")
+
+        # eSHE instructors shouldn't have access to membership tab
+        CourseAccessRoleFactory(
+            course_id=self.course.id,
+            user=user,
+            role='eshe_instructor',
+            org=self.course.id.org
+        )
+        response = self.client.get(self.url)
+        self.assertNotContains(response, membership_section)
+
+        # However if combined with forum_admin, they should have access to this
+        # tab, but not to batch enrollment
+        forum_admin_role = RoleFactory(name=FORUM_ROLE_ADMINISTRATOR, course_id=self.course.id)
+        forum_admin_role.users.add(user)
+        response = self.client.get(self.url)
+        self.assertContains(response, membership_section)
+        self.assertNotContains(response, batch_enrollment)
+
+        # Combined with course staff, should have union of all three roles
+        # permissions sets
+        CourseAccessRoleFactory(
+            course_id=self.course.id,
+            user=user,
+            role='staff',
+            org=self.course.id.org
+        )
+        response = self.client.get(self.url)
+        self.assertContains(response, membership_section)
+        self.assertContains(response, batch_enrollment)
+
     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 39a2f549ec98..9da941b2ca52 100644
--- a/lms/djangoapps/instructor/views/instructor_dashboard.py
+++ b/lms/djangoapps/instructor/views/instructor_dashboard.py
@@ -501,8 +501,8 @@ 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': (
-            access['eshe_instructor'] and not (access['explicit_staff'] or access['forum_admin'])
-        )
+            not access['forum_admin'] and (access['eshe_instructor'] and not access['explicit_staff'])
+        ),
     }
     return section_data
 
diff --git a/lms/envs/common.py b/lms/envs/common.py
index 5291ff37439b..5f190ad22268 100644
--- a/lms/envs/common.py
+++ b/lms/envs/common.py
@@ -1027,6 +1027,16 @@
     # .. toggle_creation_date: 2022-06-06
     # .. toggle_tickets: 'https://github.com/edx/edx-platform/pull/29538'
     'DISABLE_ALLOWED_ENROLLMENT_IF_ENROLLMENT_CLOSED': False,
+
+    # .. toggle_name: FEATURES['ENABLE_ESHE_INSTRUCTOR_ROLE']
+    # .. toggle_implementation: DjangoSetting
+    # .. toggle_default: False
+    # .. toggle_description: Whether to enable the ESHE Instructor role
+    # .. toggle_use_cases: open_edx
+    # .. toggle_creation_date: 2023-07-31
+    # .. toggle_target_removal_date: None
+    # .. toggle_tickets: 'https://github.com/open-craft/edx-platform/pull/561/files'
+    'ENABLE_ESHE_INSTRUCTOR_ROLE': False,
 }
 
 # Specifies extra XBlock fields that should available when requested via the Course Blocks API
diff --git a/lms/templates/instructor/instructor_dashboard_2/membership.html b/lms/templates/instructor/instructor_dashboard_2/membership.html
index e27f506f1148..4169f9af0c71 100644
--- a/lms/templates/instructor/instructor_dashboard_2/membership.html
+++ b/lms/templates/instructor/instructor_dashboard_2/membership.html
@@ -57,7 +57,6 @@
     <div class="request-response"></div>
     <div class="request-response-error"></div>
 </fieldset>
-%endif
 
 %if static.get_value('ALLOW_AUTOMATED_SIGNUPS', settings.FEATURES.get('ALLOW_AUTOMATED_SIGNUPS', False)):
 <hr class="divider" />
@@ -94,6 +93,8 @@ <h3 class="hd hd-3">${_("Register/Enroll Students")}</h3>
 </div>
 %endif
 
+%endif
+
 <hr class="divider" />
 
 %if section_data['access']['instructor']:
@@ -192,6 +193,7 @@ <h3 class="hd hd-3">${_("Course Team Management")}</h3>
       data-add-button-label="${_("Add Limited Staff")}"
     ></div>
 
+    %if settings.FEATURES.get('ENABLE_ESHE_INSTRUCTOR_ROLE', None):
     <div class="auth-list-container"
       data-rolename="eshe_instructor"
       data-display-name="${_("eSHE Instructor")}"
@@ -203,6 +205,7 @@ <h3 class="hd hd-3">${_("Course Team Management")}</h3>
       data-modify-endpoint="${ section_data['modify_access_url'] }"
       data-add-button-label="${_("Add eSHE Instructor")}"
     ></div>
+    %endif
 
     ## Note that "Admin" is identified as "Instructor" in the Django admin panel.
     <div class="auth-list-container"