Skip to content

Conversation

@0x29a
Copy link

@0x29a 0x29a commented Jul 27, 2023

Description

image

Adds a new course team role, eSHE Instructor. This role inherits all Staff permissions, except the ability to assign course team roles and enroll / un-enroll students.

The role is designed to be composable with other roles. This means that if a user has Staff and eSHE Instructor roles, it'll have a union of permission sets of both.

Here is the default Instructor Dashboard tabs available to the new role:
image

The Membership tab is hidden here. However, if a user has both Forum discussion and eSHE Instructor role, the Membership tab is visible, but without the students enrollment section:

image

Testing instructions

  1. Switch your devstack to the 0x29a/bb7489/eshe-instructor branch from the git@github.com:open-craft/edx-platform.git repo.
  2. Create two users: course_staff@example.com and eshe_instructor@example.com. Give the former Staff course team role using the Instructor Dashboard.
  3. Since we've changed the CAN_ENROLL permission, verify that course_staff@example.com still can enroll students (Membership tab).
  4. Now, give eshe_instructor@example.com the eSHE Instructor role.
  5. As eshe_instructor@example.com, verify that you have access to the Instructor Dashboard, but not to the Membership tab.
  6. Now, give eshe_instructor@example.com the Staff role and verify that it has all Staff permissions (can enroll students, and see the Membership tab).
  7. Remove the Staff role from eshe_instructor@example.com, give Discussion Admin and verify that it can assign forum moderator roles.
  8. Remove the Forum Discussion role from eshe_instructor@example.com, give Data Researcher and verify that it can access the Data Download tab.

Additionally, this script can be used to test that the CAN_ENROLL permission works as expected:

from django.contrib.auth import get_user_model
from openedx.core.lib.courses import get_course_by_id
from opaque_keys.edx.keys import CourseKey
from lms.djangoapps.courseware.rules import HasAccessRule, HasRolesRule

course_key = CourseKey.from_string("course-v1:Test+Test+Test")
course = get_course_by_id(course_key)
user = get_user_model().objects.get(email="eshe_instructor@example.com")

user.has_perm('instructor.enroll', course_key)

@0x29a 0x29a force-pushed the 0x29a/bb7489/eshe-instructor branch 2 times, most recently from fcecd1f to 90adc68 Compare July 30, 2023 01:01
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.
@0x29a 0x29a force-pushed the 0x29a/bb7489/eshe-instructor branch from 90adc68 to eba8b0c Compare July 30, 2023 16:54
@0x29a 0x29a changed the title WIP feat: eSHE Instructor role [BB-7489] Jul 31, 2023
Comment on lines +47 to +53
# 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'))
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@0x29a Could you explain this more in depth for me? I don't understand why we can't set it to HasAccessRule('staff'), and do we need to check with no inheritance for the HasAccessRule('staff', strict=True), since staff doesn't inherit from the eshe_instructor role, it's the other way around.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and do we need to check with no inheritance for the HasAccessRule('staff', strict=True), since staff doesn't inherit from the eshe_instructor role, it's the other way around.

Never mind, I got confused. I realize that we need to check strictly because HasAccessRule('staff') can return true if someone has eshe_instructor role, since we are inheriting. And the rest is so we don't break the inheritance for the other roles that inherit from the staff.

Copy link
Author

@0x29a 0x29a Aug 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, that's right @Cup0fCoffee. Actually, I'm not sure that explicit / implicit wording in comments explaining this is the best choice. I'll think how I can explain this workaround more clearly.

# 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'])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@0x29a I would regroup it like:

Suggested change
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'])

because semantically it makes more sense, e.g. we always display it if user is forum_admin, and if they are an eshe_instructor we have to check if they are explicit_staff or not.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, should this section be visible if a user is a staff, but not a forum_admin?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Cup0fCoffee, yes, should be visible for staff, as they by default it has access to bulk enrollments / assigning course team roles.

@Cup0fCoffee
Copy link
Member

👍

  • I tested this: followed the testing instructions
  • I read through the code
  • Includes documentation
  • [N/A] I made sure any change in configuration variables is reflected in the corresponding client's configuration-secure repository.

@0x29a 0x29a merged commit 48c3b81 into opencraft-release/nutmeg.2 Aug 7, 2023
@0x29a 0x29a deleted the 0x29a/bb7489/eshe-instructor branch August 7, 2023 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants