Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: eSHE Instructor role [BB-7489] #561

Merged
merged 2 commits into from
Aug 7, 2023

Conversation

0x29a
Copy link
Member

@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 [email protected]:open-craft/edx-platform.git repo.
  2. Create two users: [email protected] and [email protected]. Give the former Staff course team role using the Instructor Dashboard.
  3. Since we've changed the CAN_ENROLL permission, verify that [email protected] still can enroll students (Membership tab).
  4. Now, give [email protected] the eSHE Instructor role.
  5. As [email protected], verify that you have access to the Instructor Dashboard, but not to the Membership tab.
  6. Now, give [email protected] 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 [email protected], give Discussion Admin and verify that it can assign forum moderator roles.
  8. Remove the Forum Discussion role from [email protected], 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="[email protected]")

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
Member 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
Member 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
30 checks passed
@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.

2 participants