Add permission checks to role checks - courseware, learner home, teams, content #33991
Conversation
hsinkoff
force-pushed
the
CourseRoles
branch
2 times, most recently
from
4822202 to
a63cae5
Compare
hsinkoff
force-pushed
the
ROLES-65-roles_checks_to_permission_checks
branch
6 times, most recently
from
2006bd2 to
41cd6dc
Compare
hsinkoff
force-pushed
the
ROLES-65-roles_checks_to_permission_checks
branch
8 times, most recently
from
11fb831 to
8294f96
Compare
hsinkoff
force-pushed
the
ROLES-65-roles_checks_to_permission_checks
branch
4 times, most recently
from
44428ef to
28f420f
Compare
hsinkoff
force-pushed
the
ROLES-65-roles_checks_to_permission_checks
branch
from
28f420f to
7c94e63
Compare
| # The user_is_staff, user_is_beta_test, and user_role cannot be removed until | ||
| # all x-blocks have been updated to use permissions, once they have been updated | ||
| # remove role check in favor of permissions checks |
There was a problem hiding this comment.
Just as a thought / question: how might we verify / track that blocks have moved over to the new permissions pattern? It is minority difficult to verify that all the internal blocks we own follow this permission pattern, but we will need to socialize this change well and mark it as a breaking change for Open edX release for environments with custom blocks.
There was a problem hiding this comment.
We agree that it will be necessary to socialize this change well and expect that removing some checks will wait longer than others. Likely this will be one that is done later in the process because of the complications you've mentioned.
lms/djangoapps/teams/api.py
Outdated
| if has_course_staff_privileges(user, course_key): | ||
| # TODO: remove role checks once course_roles is fully impelented and data is migrated | ||
| if ( | ||
| has_course_staff_privileges(user, course_key) or |
There was a problem hiding this comment.
This feels redundant since has_course_staff_privileges asks the same has_perm check. Or is this just here for greater clarity?
There was a problem hiding this comment.
It was added for clarity, but it does seem redundant and will be removed.
…, teams, content (#33991) * feat: courseware, learner home, teams, content role checks to permission checks
…, teams, content (#33991) * feat: courseware, learner home, teams, content role checks to permission checks
Description
This PR adds permissions checks alongside the existing roles checks for course level permissions in courseware, learner_home, teams, and content. These permissions are designed to be assigned to course level roles that will be assigned to a user.
This PR should have no immediate impact on any users. Later PRs will create new course_roles and migrate existing student_courseaccessrole user roles to the new course_roles user roles. Only after that time will these permissions grant access to courseware, learner_home, teams, and content.
Supporting information
course_roles tech spec
Testing instructions
Testing will be completed on the feature branch once additional services have been updated to add permissions checks. Testing will involve creating a course_roles role and assigning it to a user. This user will then be used to confirm the correct access is granted to the user.
Other information
This is a PR to a feature branch.