Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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: Course roles service #33734
feat: Course roles service #33734
Changes from 60 commits
8f7f612a7935773883210775200954cec189c180e0d8ffa7b668510e29f1f765051704343656b9d5e690f596a537f937d92cffa51ab2cce34cd51d072495161198f991fac6ec7b4a7dc5cb6194c6eeb28c11118ed09690e1385fb095d4ebeb56fdb145452792aa8be4828863d19d4e96bd89bbc8e6b144c67f544bd3fb111f9d3afbd157c573c6f58f7cd1409a25c60b675a6eda941de85e3a4ebbe7130e445147030adfeae37a84e7ea5c4d03ac3322a05d7ab2de2be688cbae9702ad0ed7b41f44fe1fef74b7faf387acdcdc9559a270e237b8e5cdbb3d30c0e5f634edd1c590a98723cb6a0197eab69301ddcaaa289b546641af3f6c73444961e74da0366d8b680fFile filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Long term note: One architectural failure mode I'm worried about here is if people find it difficult to add new permissions and instead start using other permissions that imply a role.
For instance, say someone wants to add a permission for the Staff Debug information under Components in the LMS rendering. But adding a permission and doing the data migration sounds hard. Then they browse through this class and think to themselves, "Well, if they can
VIEW_ALL_CONTENTANDMANAGE_STUDENTS, then they're course staff and they should be able to see the Staff Debug info..." and we get into the mode where people are re-deriving roles implicitly from permissions and extrapolating those onto other implied permissions.I'm not saying we're doomed to this. Just that it's a risk and that we'll need docs and outreach, presentations, etc. in order to make sure that developers working on this will have the right mindset. This gets more likely if there's any real friction to adding permissions–long approval process, unclear docs, any perception of operational risk in the event of a rollback, etc.
Come to think on it, this whole thing would make for a great Open edX Conference talk or a Meetup presentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We agree this presents a risk and have been thinking through strategies to mitigate for this risk. We do not think this presents an immediate risk, but plan to add documentation around how and when to add a permission in OEP-66 and within the djangoapps/course_roles/docs folder.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: It seems odd to have a permission that tells you what you can't do. It looks like this is supposed to be exclusive with
VIEW_ALL_CONTENTso that you only have one or the other, but I would have expected this permission to beVIEW_LIVE_PUBLISHED_CONTENTand for staff to have both.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A role can have both permissions, but in that case
VIEW_ONLY_LIVE_PUBLISHED_CONTENTwill not actually add any access. All content includes all live published content so having both would be a redundancy, but it won't cause any issues in the code.This permission (
VIEW_ONLY_LIVE_PUBLISHED_CONTENT) exists so that it can be given to users who should only see live published content and not published content before it is live or unpublished content.@cablaa77 Please let us know if you agree the permission name should be changed to
VIEW_LIVE_PUBLISHED_CONTENTfromVIEW_ONLY_LIVE_PUBLISHED_CONTENT.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the general pattern I am leery of is that people will start to use a permission check as a proxy for the absence of other permissions, and that it will introduce unnecessary coupling when we want to change things later. But I don't have a concrete case off the top of my head, and I won't block approving this PR over this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The permission name is being changed to
VIEW_LIVE_PUBLISHED_CONTENTThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is managing Libraries a Course Role/Permission?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This permission is for the v1 content libraries. It will not need to exist if v2 content libraries is live, migrations are completed, and v1 code is removed before this code goes live, but a permission was included in case v1 content libraries is still live when a new role built off these permissions is created. This permission will be removed once v1 content libraries is fully sunset.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. So v1 libraries will definitely still be around when this rolls out (for the community, even if edx.org decides to do the upgrade before then).
I'd like to understand a little more what it means when someone has this permission, because I'm not clear on what it would mean to have permission to
MANAGE_LIBRARIESfor a specific course, when the library exists outside of that course.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking through the existing code, we found only one location (so far) where the code checks if a user has the
LibraryUserrole.This permission will be added as a permission check where the role is currently checked and used to create the
LibraryUserrole incourse_roles, but at this time we haven't identified any additional locations where we would add a check for the new permission.course_rolespermissions checks are not dependent on the level on which the permission was granted, but just that the user has that permission in the context in question. This means if the user has theLibraryUserrole at the org-wide level, they have that permission for all courses within the org. It also means if we check if the user has the permission, we are checking for course, org, and instance wide assignment of the permission.course_roleshas a fk dependency so we cannot assign acourse_roleon the library itself. It is also possible that the migration order of roles fromstudent_courseaccessroletocourse_roles_rolecan be altered so that theLibraryUserrole stays on thestudent_courseaccessroleuntil after the point in time when the named release is using v2 content libraries and this permission (and role) are no longer relevant.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, I'd suggest leaving this behind in
student_courseaccessrole, but I'm okay either way in the short term.