feat: course_roles setup #33609

hsinkoff · 2023-10-27T17:50:57Z

Description

This pull request adds the new CourseRoles model that will be used for creating roles with associated permissions that can be assigned to users on a course, as an org-wide course role (the user has access as if they have the role on all courses in the org), or instance-wide course role (the user has access as if they have the role on all courses within the instance).

This initial PR does not create any roles, but does setup the ability to do so in the future.

Additionally this PR adds code at each location where a user's django_commentclientrole or student_courseaccessrole is currently checked. At each of these locations an or clause was added to check if the user has the permission for this level of access. Which permission to check was determined based on which permissions the role would have (if migrated to the new course_roles system) and which of those permissions best corresponds to the functionality of the code.

If implemented correctly this code will have no immediate impact on system users, but will instead set the code up for future iterations that will allow implementation of new roles using this system.

Supporting information

This PR is the initial work for the Tech Spec related to Course Roles.

Testing instructions

In order to run the code fully, a waffle flag (course_roles.use_permission_checks) should be created. If the waffle flag is not present or is false, the permissions checks will return false. Only if the waffle flag is true will the permissions checks check the db for access.

As noted previously, there is no intended change in functionality for a user. Testing should reflect no noticeable user experience changes as there are currently no course_roles_roles created and no users who are assigned the new course_roles_roles.

Other information

Requested Areas of Additional Focus

code related to adding data for xblocks
default value of the waffle flag (if it is not created)

Starting Point for PR Review

openedx/core/djangoapps/course_roles/docs/decisions/0001-course_level_roles.rst provides context on these changes and may be a good starting point for reviewing the PR

bszabo · 2023-11-07T16:09:11Z

openedx/core/djangoapps/course_roles/docs/decisions/0001-course_level_roles.rst

@@ -0,0 +1,125 @@
+1. Course Level Roles


These ADRs files are an excellent place for reviewers to start review of this PR. Might be useful to call out in the PR description

The Tech Spec organizes the work in phases, including an MVP designation. In terms of that work breakdown, where does this PR fit in?

This work is the first part of the foundational work needed to get to MVP, but it does not include the user facing/value add portions of the MVP. There will also be a second section of foundational work needed to fully utilize the permissions in the way they are intended to be used before the new role and CMS role visualization changes are implemented.

connorhaugh

One initial thing -- have you thought about updating the unit tests in test_roles.py and test_authz.py? That could be helpful in instilling confidence in making sure that we can be confident in the essential workflows here remaining intact after cutover.

bszabo · 2023-11-08T13:00:21Z

common/djangoapps/student/auth.py

+        # Remove the user_has_role call when course_roles Django app are implemented.
+        if not (
+            user_has_role(caller, CourseInstructorRole(role.course_key)) or
+            course_permission_check(caller, CourseRolesPermission.MANAGE_ALL_USERS.value, role.course_key)


While this is functionally correct, it depends on the current set of permissions assigned to instructor and staff roles; if these were to change, the above check might be wrong. Additionally, the line is far less clear in intent than the previous line it's replacing. This is intended to check whether the caller has a certain role, and the test ought to be in terms of roles and not permissions.

I looked again at this code, trying to understand it at a deeper level. Specifically, if the intent behind a role/permissions solution is that all actions be gated by having appropriate permissions, then we should never have role-based tests. Put another way, if a role is duplicated under a new name, then the original role and the duplicate will have the same permissions. A role-based tests would pass the first role but fail the duplicate, whereas a permissions-based test would pass both. Clearly the latter is what should happen, and this corroborates the notion that gates should always be permission-based, and not role-based. What this example shows is that gates in existing code are sometimes role-based, and when this occurs it's necessary to convert them to permission-based gates.

Part of the complication in doing this is that we may need to generate new permissions to deal with such cases. Here, do we need a CourseRolesPermission.MANAGE_ROLES permission? It strikes me that the ability to modify which permissions go with a role is outside what I would expect in a MANAGE_ALL_USERS permission. MANAGE_ROLES is a permission a platform admin role might have, whereas MANAGE_ALL_USERS is a permission a course owner role might have.

The goal of this PR is to convert role based gating to a combined role and permission based gating as an intermediate step to be used until all existing course level roles (student_courseaccessrole and django_commentclientroles) are migrated over to course_roles_role.

We worked with product to determine the initial list of permissions, but a permission was not created that would allow a user to modify which permissions belong to a role. CourseRoles was designed to allow for adding permissions in the future (with code changes) and roles in the future (without needing to modify code). This full design is not implemented in this PR, but this sets the foundational work for that end goal.

To my knowledge there are not current plans to make modifying the permissions assigned to a role (or even creating a new role) something that is possible within the LMS/CMS. If this changes, we will need a new permission to address that need, but it likely would not be a course level permission, but rather an instance or org level permission.

From tracing this code usage of this function, we determined it was used to decide who could assign roles to users which fit the MANAGE_ALL_USERS permission. If we mis-interpreted the code usage, please let us know.

Course staff are not given the MANAGE_ALL_USERS permission, so this test will only allow the course instructor to make these changes, as is currently the case. But when I look at the user_has_role() test, I immediately understand what's intended. When I look at the MANAGE_ALL_USERS test, the intent is far less clear. I understand your reasoning, but it seems like a step backwards in terms of code clarity.

We will be changing the function names to increase clarity on their purpose.

bszabo · 2023-11-08T13:15:58Z

lms/djangoapps/courseware/access.py

@@ -174,7 +187,14 @@ def has_staff_access_to_preview_mode(user, course_key):
    """
    has_admin_access_to_course = any(administrative_accesses_to_course_for_user(user, course_key))

-    return has_admin_access_to_course or is_masquerading_as_student(user, course_key)


The original test was role-based, while here we have a permissions-based test. Does it continue to work as intended if permissions assignments change? As I suggested in a different comment, I believe a requirement for the roles-permissions app is that permissions granted at runtime to one role should never be affected by configuration changes to permissions made to another role. If a permissions-based test is most appropriate here, then we might be missing a permission: PREVIEW_ALL_CONTENT.

* feat: add CourseRolesService model * feat: add CourseRolesRole model * feat: add CourseRolesPermission model * feat: add CourseRolesRolePermissions model * feat: add CourseRolesUserRole model * feat: remove wrong unique_together in CourseRolesUserRole * feat: add coures_roles app to cms and lms common envs * feat: add migrations * feat: delete wrong migration * feat: change many to many relationships to foreign keys * feat: add many to many relationships between role-user and role-permission * feat: add initial migration * docs: add readme file * docs: update readme file * chore: add course_roles in quality matrix CI * feat: remove fields related to custom roles * docs: update docstrings * feat: add minimal string representation change * feat: remove descriptions from role and permission tables * feat: swich the role to service relationship into a many to many relationship * docs: update role docstring * feat: update initial migration * feat: change on_delete for org and coures on UserRole model to cascade * feat: update initial migration * docs: update CourseRolesPermission model docstring

* feat: add permission_check function def * test: add permission_check base test case * test: add permission check tests * feat: add permission check logic * test: remove allowed field from role permission * test: update tests * feat: split permission_check function * feat: update CourseRolesUserRole model * feat: update initial migration * test: rewrite the permission check tests and add new cases * chore: update unit test shards * docs: update docstrings

* test: add test cases for permission list check functions * test: update tests * feat: add helper functions to check lists of permissions * style: improve code style * feat: add course roles checks in the contentstore app * feat: add course roles checks in the student app * feat: add course roles checks in the lms discussion app * feat: add course roles checks in the lms instructor app * feat: add course roles checks in the Learning Sequences package * style: fix code style * fix: course_permission_check calls * feat: add validation for AnonymousUser in course permission check helper functions * fix: disable some pylint warnings * test: update number of querys asserted in has_course_author_access * feat: add helper functions to check course or organization permissions * test: update course_roles tests * feat: replace course or organization helper functions in auth * docs: update course_roles docstrings

* feat: add migration to load permissions in the database * feat: add Permission enum * feat: change Permission enum name to CourseRolesPermission * feat: replace permission constants with the CourseRolesPermission enum * feat: add unique decorator to permissions enum * feat: add course_roles_permissions dict with names and descriptions (with i18n) * docs: add CourseRolesPermission docstring * style: fix pylint errors * style: fix pylint errors * docs: add permissions module docstring

* feat: ROLES-3 Model Setup (#33229) * feat: add permission checks * feat: add new rule to validate permissions with bridgekeeper * feat: add permission checks * feat: add permission checks comments * feat: add permission checks * fix: minor att name fix * feat: add permission checks * feat: add permission check helper function for any permission in a list * feat: change translation import to gettext from ugettext * style: fix pep8 errors * style: fix trailing whitespace * test: increase number of querys asserted in course_api blocks tests * fix: fix conditional * test: increase number of querys asserted in grades tests * test: increase number of querys asserted in ccx tests * test: increase number of querys asserted in openedx schedules and features tests * fix: fix conditional * fix: error if can't find the course in the modulestore * test: increase number of querys asserted in courseware * test: increase number of querys asserted in courseware * test: increase number of querys asserted in courseware * test: increase number of querys asserted in discussion * fix: minor code fix * docs: remove TODO comments

* feat: Roles 15 - permission checks back end changes part 1 (#33347) * test: add test cases for permission list check functions * test: update tests * feat: add helper functions to check lists of permissions * style: improve code style * feat: add course roles checks in the contentstore app * feat: add course roles checks in the student app * feat: add course roles checks in the lms discussion app * feat: add course roles checks in the lms instructor app * feat: add course roles checks in the Learning Sequences package * style: fix code style * fix: course_permission_check calls * feat: add validation for AnonymousUser in course permission check helper functions * fix: disable some pylint warnings * test: update number of querys asserted in has_course_author_access * feat: add helper functions to check course or organization permissions * test: update course_roles tests * feat: replace course or organization helper functions in auth * docs: update course_roles docstrings * feat: ROLES-23 Create permissions in db table (#33394) * feat: add migration to load permissions in the database * feat: add Permission enum * feat: change Permission enum name to CourseRolesPermission * feat: replace permission constants with the CourseRolesPermission enum * feat: add unique decorator to permissions enum * feat: add course_roles_permissions dict with names and descriptions (with i18n) * docs: add CourseRolesPermission docstring * style: fix pylint errors * style: fix pylint errors * docs: add permissions module docstring * feat: add helper function to get user permissions for a course * test: add test for get_all_user_permissions_for_a_course * feat: add views to coures roles api * test: add test for course roles views * feat: add urls for course roles api * feat: add course roles api urls to lms and cms * docs: add comment to indicate which urls are from course roles api * feat: add translation to ValueError exception message * feat: add translation to ValueError exception message * feat: raise exeption if course does not exist * feat: add instance permissions in get_all_user_permissions_for_a_course helper * feat: improve validations in get_all_user_permissions_for_a_course * feat: improve validations in UserPermissionsView * test: update get user permissions tests * docs: update UserPermissionsView docstring * feat: change message errors * docs: update docstrings in test_views * fix: add missing super method call in a class * fix: add password to test user * fix: chain re-raising exceptions

…hecked to determine access

openedx/core/djangoapps/course_roles/readme.rst

openedx/core/djangoapps/course_roles/models.py

ormsbee

Did a second pass through the models, with some questions/suggestions w.r.t. Permissions and the API. Anything with a "nit" prefix isn't required for merge–I just wanted to make sure you were aware of the possibilities.

I'm going to make a second review sometime tonight to go over some performance aspects.

ormsbee · 2023-11-15T16:27:14Z

openedx/core/djangoapps/course_roles/models.py

+    The services field defines in which service UI the role is intended to be assigned, such as CMS and/or LMS.
+
+    """
+    name = models.CharField(max_length=255)


Should this have a unique constraint on it?

We do not need unique names

so how do we know which role we're using if there are duplicates?

openedx/core/djangoapps/course_roles/models.py

ormsbee · 2023-11-15T16:44:41Z

openedx/core/djangoapps/course_roles/models.py

+        return f"{self.role} - {self.permission}"
+
+
+class UserRole(models.Model):


[Question, because I'm not sure about all the implications]: Would it make sense to put another level of normalization here where UserRole is separate from UserRoleCourse? So you'd have one row to represent "This person is an Instructor" and a M:M representation of what courses they're an instructor in?

We've discussed this and given that a permission granted for any object in the hierarchy (Course, Org, Instance) grants the permission to the objects lower in the hierarchy additional normalization would increase the sql joins needed to pull data in some of the most common queries. Keeping in mind that Open edX DB design allows for nullable fields, we think it is best to continue with Course and Org as nullable fields in the UserRole table.

openedx/core/djangoapps/course_roles/models.py

ormsbee · 2023-11-15T17:55:09Z

openedx/core/djangoapps/course_roles/permissions.py

@@ -0,0 +1,143 @@
+"""
+Permissions for course roles app.
+"""


Please explain in this module docstring:

How these classes tie into the Permission model.

What the proper procedure is for adding, changing, or removing a permission–esp. w.r.t. migrations.

openedx/core/djangoapps/course_roles/permissions.py

openedx/core/djangoapps/course_roles/helpers.py

ormsbee

This is the performance-oriented review.

ormsbee · 2023-11-15T18:46:09Z

openedx/features/content_type_gating/tests/test_models.py

@@ -71,7 +71,7 @@ def test_enabled_for_enrollment(
        user = self.user
        course_key = self.course_overview.id

-        query_count = 7
+        query_count = 14


This is a major performance regression. Please explain why this is happening.

ormsbee · 2023-11-15T22:54:15Z

openedx/core/djangoapps/schedules/tests/test_resolvers.py

@@ -274,7 +274,7 @@ def create_resolver(self, user_start_date_offset=8):
    def test_schedule_context(self):
        resolver = self.create_resolver()
        # using this to make sure the select_related stays intact
-        with self.assertNumQueries(30):
+        with self.assertNumQueries(46):


This is a major performance regression.

For historical context: permissions checks are an area where we've been burned really badly by performance regressions in the past. A jump like this is a red flag.

ormsbee · 2023-11-15T23:07:55Z

openedx/core/djangoapps/course_roles/helpers.py

+    """
+    if not use_permission_checks():
+        return False
+    if isinstance(user, AnonymousUser) or not isinstance(user, User):


Shouldn't this be an error if user is not some kind of User?

Also, it's not a blanket denial for anonymous users, is it? We allow people to view course content anonymously if you set the right flags and course settings.

ormsbee · 2023-11-15T23:10:19Z

openedx/core/djangoapps/course_roles/helpers.py

+    return UserRole.objects.filter(
+        user=user,
+        role__permissions__name=permission_name,
+        course=course_id,


Doesn't this code also need to check if they have the permission on the course because they have global or org-level access?

Oh, I see, that logic is in user_has_permission_org. FWIW, I think that the name of this function implies that it answers "does this user have permission to do X in course Y?", not "does this user have a course-only level permission to do X in course Y?" Though I guess it would be named user_has_permission_for_course in that case...

Still, I think this will confuse others as well.

ormsbee · 2023-11-15T23:16:24Z

openedx/core/djangoapps/course_roles/helpers.py

+    return any(user_has_permission_course(user, permission_name, course_id) for permission_name in permission_names)
+
+
+def user_has_permission_org(user, permission_name, organization_name):


You might want to use the request_cached decorator here (and elsewhere in this module).

(Please see my comment about get_all_user_permissions_for_a_course.)

ormsbee · 2023-11-15T23:25:47Z

openedx/core/djangoapps/course_roles/helpers.py

+    if isinstance(user, AnonymousUser) or not isinstance(user, User):
+        return False
+    if organization_name is None:
+        course = modulestore().get_course(course_id)


Avoid modulestore access unless you absolutely cannot help it–it can get surprisingly slow. A lot of metadata about a course can be retrieved from the CourseOverview model instead. But the org here can be derived directly from a CourseKey, like course_key.org.

openedx/core/djangoapps/course_roles/helpers.py

ormsbee · 2023-11-16T02:30:03Z

openedx/core/djangoapps/course_roles/helpers.py

+    )
+
+
+def get_all_user_permissions_for_a_course(user_id, course_id):


As long as the number of possible permissions is relatively low (like it is today), one approach you could consider is to have some of your other functions call this one, and use the @request_cached decorator on this function only. As long as the indexes support doing that one query well, it should be fast, small, and it would only need to be run once per request, regardless of how many other pieces of code make permissions checks.

(Assuming those permissions checks are for the same course, and that's by far the most common use case. I guess the other query pattern we'd have to worry about is, "What courses does user X have Y permission in?", which would require a different function.)

ormsbee · 2023-11-16T14:25:42Z

@hsinkoff: I was thinking a bit more on what you were saying before, I do believe this PR should be broken up. More for the sake of deployment than anything else–so that we don't have to revert everything if something goes wrong in one of the many apps being touched.

So maybe the first PR is the course_roles app itself and its various models + APIs, and then each additional app switchover is a new PR?

hsinkoff · 2023-11-16T15:53:40Z

We will be splitting this PR into multiple PRs with the first focused on the course_roles service (API, model, etc). We will link to that PR once it is ready.

hsinkoff · 2023-11-16T20:29:54Z

First PR - focus on the CourseRoles service.

mariajgrimaldi · 2023-11-15T15:23:30Z

cms/djangoapps/contentstore/views/user.py

@@ -127,7 +129,11 @@ def _course_team_user(request, course_key, email):
        }
        # what's the highest role that this user has? (How should this report global staff?)
        for role in role_hierarchy:
-            if role(course_key).has_user(user):
+            # TODO: course roles: If the course roles feature flag is disabled the user_has_permission_course call


is this still a TODO? I see the user_has_permission_course checks whether the flag is enabled.

mariajgrimaldi · 2023-11-15T16:27:29Z

common/djangoapps/student/auth.py

+    COURSE_INSTRUCTOR_ROLE_PERMISSIONS = [
+        CourseRolesPermission.MANAGE_CONTENT.value,
+        CourseRolesPermission.MANAGE_COURSE_SETTINGS.value,
+        CourseRolesPermission.MANAGE_ADVANCED_SETTINGS.value,
+        CourseRolesPermission.VIEW_COURSE_SETTINGS.value,
+        CourseRolesPermission.MANAGE_ALL_USERS.value,
+    ]
+    STAFF_ROLE_PERMISSIONS = [
+        CourseRolesPermission.MANAGE_CONTENT.value,
+        CourseRolesPermission.MANAGE_COURSE_SETTINGS.value,
+        CourseRolesPermission.MANAGE_ADVANCED_SETTINGS.value,
+        CourseRolesPermission.VIEW_COURSE_SETTINGS.value,
+        CourseRolesPermission.MANAGE_USERS_EXCEPT_ADMIN_AND_STAFF.value,
+    ]


Why don't we move these constants to the top of the file?

mariajgrimaldi · 2023-11-15T16:28:09Z

common/djangoapps/student/auth.py

@@ -89,9 +111,23 @@ def get_user_permissions(user, course_key, org=None):
        return STUDIO_NO_PERMISSIONS
    all_perms = STUDIO_EDIT_ROLES | STUDIO_VIEW_USERS | STUDIO_EDIT_CONTENT | STUDIO_VIEW_CONTENT


Will this be replaced later on by these permissions definitions?

mariajgrimaldi · 2023-11-20T16:34:15Z

lms/djangoapps/courseware/rules.py

@@ -178,3 +179,24 @@ def check(self, user, instance=None):
            if OrgRole(role, course_key.org).has_user(user):
                return True
        return False
+
+
+class HasPermissionRule(Rule):  # lint-amnesty, pylint: disable=abstract-method, missing-class-docstring


Can we add a docstring here to add more context to the rule?

mariajgrimaldi · 2023-11-24T17:10:11Z

openedx/core/djangoapps/course_roles/docs/decisions/0001-course_level_roles.rst

+- be assigned at the course level 
+- be assigned as an org-wide role (granting access to all courses within the org)
+- be assigned as an instance-wide role (granting access to all courses that are within the instance)
+- provide the flexibility to allow users to edit or create new roles (permission sets) that can be assigned to users


I think this is not rendering correctly:

The rendering issues include the tables below starting from line 39

mariajgrimaldi · 2023-11-24T17:28:54Z

openedx/core/djangoapps/course_roles/docs/decisions/0002-data_storage.rst

+Consequences
+************
+
+This decision will mean that any futurue default role additions or permissions changes will require changes in the code and the database. It also means that there is a chance of a default role name being listed in the UI using the name value in the database. This would occur if a role was added to the database, but the role was not added to the data structure in the code.


Suggested change

This decision will mean that any futurue default role additions or permissions changes will require changes in the code and the database. It also means that there is a chance of a default role name being listed in the UI using the name value in the database. This would occur if a role was added to the database, but the role was not added to the data structure in the code.

This decision will mean that any future default role additions or permissions changes will require changes in the code and the database. It also means that there is a chance of a default role name being listed in the UI using the name value in the database. This would occur if a role was added to the database, but the role was not added to the data structure in the code.

mariajgrimaldi · 2023-11-24T17:30:04Z

openedx/core/djangoapps/course_roles/docs/decisions/0002-data_storage.rst

+Consequences
+************
+
+This decision will mean that any futurue default role additions or permissions changes will require changes in the code and the database. It also means that there is a chance of a default role name being listed in the UI using the name value in the database. This would occur if a role was added to the database, but the role was not added to the data structure in the code.


So If I create a new role, I'll need to open a PR updating the data structure in the code? how would that look like for custom specific roles?

As I'm reading below, it'd be enough to create the role in the database in the language I need. So it wouldn't necessarily be translatable. If I need a role in multiple languages, would I need to add multiple roles in those languages?

mariajgrimaldi · 2023-11-24T17:35:40Z

openedx/core/djangoapps/course_roles/docs/decisions/0002-data_storage.rst

+* Code Based Data Objects Only - Utilize dictionaries, constants, etc to create roles and permissions
+  * Pros: Allows for use of all Open edX defined i18n best practices
+  * Cons: Does not allow for different roles on different systems, Slower data querying, 
+* Database Only - Utilize database tables to store data and store translation data for the strings
+  * Pros: Allows for different roles on different instances, Allows for easy addition of new roles
+  * Cons: Requires custom built translation option 


This is not rendering correctly:

mariajgrimaldi · 2023-11-24T18:15:43Z

openedx/core/djangoapps/content/learning_sequences/api/processors/schedule.py

+            user_has_permission_course(
+                self.user,
+                CourseRolesPermission.VIEW_ALL_PUBLISHED_CONTENT.value,
+                self.course_key
+            )
+            or user_has_permission_course(
+                self.user,
+                CourseRolesPermission.VIEW_ONLY_LIVE_PUBLISHED_CONTENT.value,
+                self.course_key
+            )


is this different than checking for the permissions list?

mariajgrimaldi · 2023-11-24T18:28:30Z

Question: can we have an admin view so we can test easily?

ormsbee · 2023-11-25T00:06:21Z

@mariajgrimaldi: Just wanted to make sure you saw @hsinkoff's note that this work is being split into smaller PRs, the first of which is here: #33734

mariajgrimaldi · 2023-11-28T14:31:42Z

@ormsbee thanks. I wasn't aware. I'll be reviewing that one then. Please just ignore my comments here if you'd like.

hsinkoff force-pushed the ROLES-2-course_roles_setup branch 3 times, most recently from 8a8c510 to eab726e Compare November 2, 2023 19:24

bszabo reviewed Nov 7, 2023

View reviewed changes

connorhaugh reviewed Nov 7, 2023

View reviewed changes

bszabo reviewed Nov 8, 2023

View reviewed changes

julianpalmerio and others added 16 commits November 8, 2023 21:49

docs: course_roles readme file

fa998c5

docs: adr for data storage in course_roles djangoapp

3615345

docs: adr for course_level roles

cc47ebd

chore: update new table diagram

722eb4c

feat: Waffle Flag for course_roles helper functions

447423f

feat: add course_roles permisisons checks where roles are currently c…

c56064b

…hecked to determine access

feat: remove caching from course_roles

7ece66f

chore: prepare branch to PR to master

d1a1bdb

feat: expose course_roles.use_permission_checks waffle flag for mfe use

4b8e1f3

docs: update documenation related to course_roles

97c568e

hsinkoff force-pushed the ROLES-2-course_roles_setup branch 2 times, most recently from dd86292 to 5045e7d Compare November 8, 2023 22:02

feat: rename course_roles helper functions

9f766af

hsinkoff force-pushed the ROLES-2-course_roles_setup branch from 5045e7d to 9f766af Compare November 8, 2023 22:13

ormsbee reviewed Nov 9, 2023

View reviewed changes

openedx/core/djangoapps/course_roles/readme.rst Show resolved Hide resolved

ormsbee reviewed Nov 9, 2023

View reviewed changes

openedx/core/djangoapps/course_roles/models.py Outdated Show resolved Hide resolved

ormsbee reviewed Nov 9, 2023

View reviewed changes

openedx/core/djangoapps/course_roles/models.py Outdated Show resolved Hide resolved

julianpalmerio added 7 commits November 13, 2023 10:58

feat: remove course roles namespace for models

a15b42d

feat: remove course roles namespace for models

0098ee4

feat: update migrations

372235e

feat: add unique constraints in many to many relationships

ccebf0d

feat: update migrations

ec4c505

test: Add CourseOverview loading to test_helpers.py

8458fb2

test: Add CourseOverview loading int test_views.py

ac71c59

hsinkoff requested review from ormsbee and connorhaugh November 14, 2023 15:36

ormsbee requested changes Nov 15, 2023

View reviewed changes

ormsbee requested changes Nov 16, 2023

View reviewed changes

hsinkoff mentioned this pull request Nov 16, 2023

feat: Course roles service #33734

Merged

julianpalmerio changed the title ~~course_roles setup~~ feat: course_roles setup Nov 17, 2023

mariajgrimaldi reviewed Nov 20, 2023

View reviewed changes

mariajgrimaldi reviewed Nov 24, 2023

View reviewed changes

hsinkoff changed the base branch from master to CourseRoles January 19, 2024 21:16

hsinkoff force-pushed the CourseRoles branch from 3333457 to 404b2d2 Compare January 22, 2024 21:32

hsinkoff closed this Jan 23, 2024

hsinkoff deleted the ROLES-2-course_roles_setup branch January 23, 2024 14:42

		return f"{self.role} - {self.permission}"


		class UserRole(models.Model):

		return any(user_has_permission_course(user, permission_name, course_id) for permission_name in permission_names)


		def user_has_permission_org(user, permission_name, organization_name):

		)


		def get_all_user_permissions_for_a_course(user_id, course_id):

		@@ -89,9 +111,23 @@ def get_user_permissions(user, course_key, org=None):
		return STUDIO_NO_PERMISSIONS
		all_perms = STUDIO_EDIT_ROLES \| STUDIO_VIEW_USERS \| STUDIO_EDIT_CONTENT \| STUDIO_VIEW_CONTENT

	This decision will mean that any futurue default role additions or permissions changes will require changes in the code and the database. It also means that there is a chance of a default role name being listed in the UI using the name value in the database. This would occur if a role was added to the database, but the role was not added to the data structure in the code.
	This decision will mean that any future default role additions or permissions changes will require changes in the code and the database. It also means that there is a chance of a default role name being listed in the UI using the name value in the database. This would occur if a role was added to the database, but the role was not added to the data structure in the code.

feat: course_roles setup #33609

feat: course_roles setup #33609

Conversation

hsinkoff commented Oct 27, 2023 • edited Loading

Description

Supporting information

Testing instructions

Other information

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connorhaugh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ormsbee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ormsbee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ormsbee commented Nov 16, 2023

hsinkoff commented Nov 16, 2023

hsinkoff commented Nov 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mariajgrimaldi Nov 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mariajgrimaldi Nov 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mariajgrimaldi commented Nov 24, 2023

ormsbee commented Nov 25, 2023

mariajgrimaldi commented Nov 28, 2023

hsinkoff commented Oct 27, 2023 •

edited

Loading

mariajgrimaldi Nov 15, 2023 •

edited

Loading

mariajgrimaldi Nov 24, 2023 •

edited

Loading