diff --git a/cms/djangoapps/contentstore/course_group_config.py b/cms/djangoapps/contentstore/course_group_config.py index 569a1f72b372..1c1b3fe624bf 100644 --- a/cms/djangoapps/contentstore/course_group_config.py +++ b/cms/djangoapps/contentstore/course_group_config.py @@ -13,11 +13,11 @@ from common.djangoapps.util.db import MYSQL_MAX_INT, generate_int_id from lms.lib.utils import get_parent_unit from openedx.core.djangoapps.course_groups.partition_scheme import get_cohorted_user_partition -from xmodule.partitions.partitions import MINIMUM_STATIC_PARTITION_ID, ReadOnlyUserPartitionError, UserPartition # lint-amnesty, pylint: disable=wrong-import-order +from xmodule.partitions.partitions import MINIMUM_UNUSED_PARTITION_ID, ReadOnlyUserPartitionError, UserPartition # lint-amnesty, pylint: disable=wrong-import-order from xmodule.partitions.partitions_service import get_all_partitions_for_course # lint-amnesty, pylint: disable=wrong-import-order from xmodule.split_test_block import get_split_user_partitions # lint-amnesty, pylint: disable=wrong-import-order -MINIMUM_GROUP_ID = MINIMUM_STATIC_PARTITION_ID +MINIMUM_GROUP_ID = MINIMUM_UNUSED_PARTITION_ID RANDOM_SCHEME = "random" COHORT_SCHEME = "cohort" diff --git a/cms/djangoapps/contentstore/rest_api/v1/views/home.py b/cms/djangoapps/contentstore/rest_api/v1/views/home.py index b06cec44b7d3..d41ceb2647c5 100644 --- a/cms/djangoapps/contentstore/rest_api/v1/views/home.py +++ b/cms/djangoapps/contentstore/rest_api/v1/views/home.py @@ -215,4 +215,5 @@ def get(self, request: Request): library_context = get_library_context(request) serializer = LibraryTabSerializer(library_context) + return Response(serializer.data) diff --git a/cms/djangoapps/contentstore/utils.py b/cms/djangoapps/contentstore/utils.py index 9bcd77c3eea9..9ed3f1ce4b36 100644 --- a/cms/djangoapps/contentstore/utils.py +++ b/cms/djangoapps/contentstore/utils.py @@ -2251,7 +2251,7 @@ def send_course_update_notification(course_key, content, user): notification_data = CourseNotificationData( course_key=course_key, content_context={ - "course_update_content": text_content, + "course_update_content": text_content if len(text_content.strip()) < 10 else "Click here to view", **extra_context, }, notification_type="course_update", diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index b56c989b411e..dce82809dfad 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/cms/djangoapps/contentstore/views/course.py @@ -52,7 +52,7 @@ CourseStaffRole, GlobalStaff, UserBasedRole, - OrgStaffRole + OrgStaffRole, ) from common.djangoapps.util.json_request import JsonResponse, JsonResponseBadRequest, expect_json from common.djangoapps.util.string_utils import _has_non_ascii_characters diff --git a/cms/djangoapps/contentstore/views/tests/test_block.py b/cms/djangoapps/contentstore/views/tests/test_block.py index b37ef49f6bda..a41f902524bc 100644 --- a/cms/djangoapps/contentstore/views/tests/test_block.py +++ b/cms/djangoapps/contentstore/views/tests/test_block.py @@ -48,7 +48,7 @@ ) from xmodule.partitions.partitions import ( ENROLLMENT_TRACK_PARTITION_ID, - MINIMUM_STATIC_PARTITION_ID, + MINIMUM_UNUSED_PARTITION_ID, Group, UserPartition, ) @@ -421,16 +421,16 @@ def test_get_user_partitions_and_groups(self): # by dynamic user partitions. self.course.user_partitions = [ UserPartition( - id=MINIMUM_STATIC_PARTITION_ID, + id=MINIMUM_UNUSED_PARTITION_ID, name="Random user partition", scheme=UserPartition.get_scheme("random"), description="Random user partition", groups=[ Group( - id=MINIMUM_STATIC_PARTITION_ID + 1, name="Group A" + id=MINIMUM_UNUSED_PARTITION_ID + 1, name="Group A" ), # See note above. Group( - id=MINIMUM_STATIC_PARTITION_ID + 2, name="Group B" + id=MINIMUM_UNUSED_PARTITION_ID + 2, name="Group B" ), # See note above. ], ), @@ -462,18 +462,18 @@ def test_get_user_partitions_and_groups(self): ], }, { - "id": MINIMUM_STATIC_PARTITION_ID, + "id": MINIMUM_UNUSED_PARTITION_ID, "name": "Random user partition", "scheme": "random", "groups": [ { - "id": MINIMUM_STATIC_PARTITION_ID + 1, + "id": MINIMUM_UNUSED_PARTITION_ID + 1, "name": "Group A", "selected": False, "deleted": False, }, { - "id": MINIMUM_STATIC_PARTITION_ID + 2, + "id": MINIMUM_UNUSED_PARTITION_ID + 2, "name": "Group B", "selected": False, "deleted": False, @@ -2443,13 +2443,13 @@ def setUp(self): self.user = UserFactory() self.first_user_partition_group_1 = Group( - str(MINIMUM_STATIC_PARTITION_ID + 1), "alpha" + str(MINIMUM_UNUSED_PARTITION_ID + 1), "alpha" ) self.first_user_partition_group_2 = Group( - str(MINIMUM_STATIC_PARTITION_ID + 2), "beta" + str(MINIMUM_UNUSED_PARTITION_ID + 2), "beta" ) self.first_user_partition = UserPartition( - MINIMUM_STATIC_PARTITION_ID, + MINIMUM_UNUSED_PARTITION_ID, "first_partition", "First Partition", [self.first_user_partition_group_1, self.first_user_partition_group_2], @@ -2458,16 +2458,16 @@ def setUp(self): # There is a test point below (test_create_groups) that purposefully wants the group IDs # of the 2 partitions to overlap (which is not something that normally happens). self.second_user_partition_group_1 = Group( - str(MINIMUM_STATIC_PARTITION_ID + 1), "Group 1" + str(MINIMUM_UNUSED_PARTITION_ID + 1), "Group 1" ) self.second_user_partition_group_2 = Group( - str(MINIMUM_STATIC_PARTITION_ID + 2), "Group 2" + str(MINIMUM_UNUSED_PARTITION_ID + 2), "Group 2" ) self.second_user_partition_group_3 = Group( - str(MINIMUM_STATIC_PARTITION_ID + 3), "Group 3" + str(MINIMUM_UNUSED_PARTITION_ID + 3), "Group 3" ) self.second_user_partition = UserPartition( - MINIMUM_STATIC_PARTITION_ID + 10, + MINIMUM_UNUSED_PARTITION_ID + 10, "second_partition", "Second Partition", [ @@ -2540,10 +2540,10 @@ def test_create_groups(self): self.assertEqual("vertical", vertical_0.category) self.assertEqual("vertical", vertical_1.category) self.assertEqual( - "Group ID " + str(MINIMUM_STATIC_PARTITION_ID + 1), vertical_0.display_name + "Group ID " + str(MINIMUM_UNUSED_PARTITION_ID + 1), vertical_0.display_name ) self.assertEqual( - "Group ID " + str(MINIMUM_STATIC_PARTITION_ID + 2), vertical_1.display_name + "Group ID " + str(MINIMUM_UNUSED_PARTITION_ID + 2), vertical_1.display_name ) # Verify that the group_id_to_child mapping is correct. diff --git a/cms/djangoapps/models/settings/course_metadata.py b/cms/djangoapps/models/settings/course_metadata.py index d5523e69f8a6..93ce9bb87c5d 100644 --- a/cms/djangoapps/models/settings/course_metadata.py +++ b/cms/djangoapps/models/settings/course_metadata.py @@ -22,7 +22,7 @@ from xmodule.course_block import get_available_providers # lint-amnesty, pylint: disable=wrong-import-order from xmodule.modulestore.django import modulestore # lint-amnesty, pylint: disable=wrong-import-order from xmodule.modulestore.exceptions import InvalidProctoringProvider # lint-amnesty, pylint: disable=wrong-import-order -from xmodule.partitions.partitions import MINIMUM_STATIC_PARTITION_ID +from xmodule.partitions.partitions import MINIMUM_UNUSED_PARTITION_ID from xmodule.partitions.partitions_service import get_all_partitions_for_course LOGGER = logging.getLogger(__name__) @@ -316,7 +316,7 @@ def fill_teams_user_partitions_ids(cls, block, teams_config): if not team_set.user_partition_id: team_set.user_partition_id = cls.get_user_partition_id( block, - MINIMUM_STATIC_PARTITION_ID, + MINIMUM_UNUSED_PARTITION_ID, MYSQL_MAX_INT, ) return TeamsConfig( diff --git a/cms/lib/xblock/test/test_authoring_mixin.py b/cms/lib/xblock/test/test_authoring_mixin.py index f3721a622d86..eee86df8bf4f 100644 --- a/cms/lib/xblock/test/test_authoring_mixin.py +++ b/cms/lib/xblock/test/test_authoring_mixin.py @@ -10,7 +10,7 @@ from xmodule.modulestore.tests.factories import CourseFactory, BlockFactory from xmodule.partitions.partitions import ( ENROLLMENT_TRACK_PARTITION_ID, - MINIMUM_STATIC_PARTITION_ID, + MINIMUM_UNUSED_PARTITION_ID, Group, UserPartition ) @@ -78,7 +78,7 @@ def create_content_groups(self, content_groups): """ # pylint: disable=attribute-defined-outside-init self.content_partition = UserPartition( - MINIMUM_STATIC_PARTITION_ID, + MINIMUM_UNUSED_PARTITION_ID, self.CONTENT_GROUPS_TITLE, 'Contains Groups for Cohorted Courseware', content_groups, diff --git a/common/djangoapps/student/auth.py b/common/djangoapps/student/auth.py index fb2999eca98d..08b4255feeae 100644 --- a/common/djangoapps/student/auth.py +++ b/common/djangoapps/student/auth.py @@ -23,7 +23,7 @@ OrgContentCreatorRole, OrgInstructorRole, OrgLibraryUserRole, - OrgStaffRole + OrgStaffRole, ) # Studio permissions: @@ -106,6 +106,7 @@ def get_user_permissions(user, course_key, org=None): # Staff have all permissions except EDIT_ROLES: if OrgStaffRole(org=org).has_user(user) or (course_key and user_has_role(user, CourseStaffRole(course_key))): return STUDIO_VIEW_USERS | STUDIO_EDIT_CONTENT | STUDIO_VIEW_CONTENT + # Otherwise, for libraries, users can view only: if course_key and isinstance(course_key, LibraryLocator): if OrgLibraryUserRole(org=org).has_user(user) or user_has_role(user, LibraryUserRole(course_key)): diff --git a/common/djangoapps/student/role_helpers.py b/common/djangoapps/student/role_helpers.py index 8a12bfa0ac90..64ed5cc17efb 100644 --- a/common/djangoapps/student/role_helpers.py +++ b/common/djangoapps/student/role_helpers.py @@ -75,4 +75,4 @@ def get_course_roles(user: User) -> list[CourseAccessRole]: """ # pylint: disable=protected-access role_cache = get_role_cache(user) - return list(role_cache._roles) + return list(role_cache.all_roles_set) diff --git a/common/djangoapps/student/roles.py b/common/djangoapps/student/roles.py index 7bbd0cf92454..971433c9c523 100644 --- a/common/djangoapps/student/roles.py +++ b/common/djangoapps/student/roles.py @@ -4,9 +4,9 @@ """ +from collections import defaultdict import logging from abc import ABCMeta, abstractmethod -from collections import defaultdict from contextlib import contextmanager from django.contrib.auth.models import User # lint-amnesty, pylint: disable=imported-auth-user @@ -23,6 +23,9 @@ # A mapping of roles to the roles that they inherit permissions from. ACCESS_ROLES_INHERITANCE = {} +# The key used to store roles for a user in the cache that do not belong to a course or do not have a course id. +ROLE_CACHE_UNGROUPED_ROLES__KEY = 'ungrouped' + def register_access_role(cls): """ @@ -60,21 +63,52 @@ def strict_role_checking(): ACCESS_ROLES_INHERITANCE.update(OLD_ACCESS_ROLES_INHERITANCE) +def get_role_cache_key_for_course(course_key=None): + """ + Get the cache key for the course key. + """ + return str(course_key) if course_key else ROLE_CACHE_UNGROUPED_ROLES__KEY + + class BulkRoleCache: # lint-amnesty, pylint: disable=missing-class-docstring + """ + This class provides a caching mechanism for roles grouped by users and courses, + using a nested dictionary structure to optimize lookup performance. The cache structure is designed as follows: + + { + user_id_1: { + course_id_1: {role1, role2, role3}, # Set of roles associated with course_id_1 + course_id_2: {role4, role5, role6}, # Set of roles associated with course_id_2 + [ROLE_CACHE_UNGROUPED_ROLES_KEY]: {role7, role8} # Set of roles not tied to any specific course or library + }, + user_id_2: { ... } # Similar structure for another user + } + + - Each top-level dictionary entry keys by `user_id` to access role data for a specific user. + - Nested within each user's dictionary, entries are keyed by `course_id` grouping roles by course. + - The special key `ROLE_CACHE_UNGROUPED_ROLES_KEY` (a constant defined above) + stores roles that are not associated with any specific course or library. + """ + CACHE_NAMESPACE = "student.roles.BulkRoleCache" CACHE_KEY = 'roles_by_user' @classmethod def prefetch(cls, users): # lint-amnesty, pylint: disable=missing-function-docstring - roles_by_user = defaultdict(set) + roles_by_user = defaultdict(lambda: defaultdict(set)) get_cache(cls.CACHE_NAMESPACE)[cls.CACHE_KEY] = roles_by_user for role in CourseAccessRole.objects.filter(user__in=users).select_related('user'): - roles_by_user[role.user.id].add(role) + user_id = role.user.id + course_id = get_role_cache_key_for_course(role.course_id) + + # Add role to the set in roles_by_user[user_id][course_id] + user_roles_set_for_course = roles_by_user[user_id][course_id] + user_roles_set_for_course.add(role) users_without_roles = [u for u in users if u.id not in roles_by_user] for user in users_without_roles: - roles_by_user[user.id] = set() + roles_by_user[user.id] = {} @classmethod def get_user_roles(cls, user): @@ -83,15 +117,32 @@ def get_user_roles(cls, user): class RoleCache: """ - A cache of the CourseAccessRoles held by a particular user + A cache of the CourseAccessRoles held by a particular user. + Internal data structures should be accessed by getter and setter methods; + don't use `_roles_by_course_id` or `_roles` directly. + _roles_by_course_id: This is the data structure as saved in the RequestCache. + It contains all roles for a user as a dict that's keyed by course_id. + The key ROLE_CACHE_UNGROUPED_ROLES__KEY is used for all roles + that are not associated with a course. + _roles: This is a set of all roles for a user, ungrouped. It's used for some types of + lookups and collected from _roles_by_course_id on initialization + so that it doesn't need to be recalculated. + """ def __init__(self, user): try: - self._roles = BulkRoleCache.get_user_roles(user) + self._roles_by_course_id = BulkRoleCache.get_user_roles(user) except KeyError: - self._roles = set( - CourseAccessRole.objects.filter(user=user).all() - ) + self._roles_by_course_id = {} + roles = CourseAccessRole.objects.filter(user=user).all() + for role in roles: + course_id = get_role_cache_key_for_course(role.course_id) + if not self._roles_by_course_id.get(course_id): + self._roles_by_course_id[course_id] = set() + self._roles_by_course_id[course_id].add(role) + self._roles = set() + for roles_for_course in self._roles_by_course_id.values(): + self._roles.update(roles_for_course) @staticmethod def get_roles(role): @@ -100,16 +151,24 @@ def get_roles(role): """ return ACCESS_ROLES_INHERITANCE.get(role, set()) | {role} + @property + def all_roles_set(self): + return self._roles + + @property + def roles_by_course_id(self): + return self._roles_by_course_id + def has_role(self, role, course_id, org): """ Return whether this RoleCache contains a role with the specified role or a role that inherits from the specified role, course_id and org. """ + course_id_string = get_role_cache_key_for_course(course_id) + course_roles = self._roles_by_course_id.get(course_id_string, []) return any( - access_role.role in self.get_roles(role) and - access_role.course_id == course_id and - access_role.org == org - for access_role in self._roles + access_role.role in self.get_roles(role) and access_role.org == org + for access_role in course_roles ) diff --git a/common/djangoapps/student/tests/test_roles.py b/common/djangoapps/student/tests/test_roles.py index 9037eb902f61..da1aad19a803 100644 --- a/common/djangoapps/student/tests/test_roles.py +++ b/common/djangoapps/student/tests/test_roles.py @@ -6,6 +6,7 @@ import ddt from django.test import TestCase from opaque_keys.edx.keys import CourseKey +from opaque_keys.edx.locator import LibraryLocator from common.djangoapps.student.roles import ( CourseAccessRole, @@ -22,7 +23,9 @@ OrgContentCreatorRole, OrgInstructorRole, OrgStaffRole, - RoleCache + RoleCache, + get_role_cache_key_for_course, + ROLE_CACHE_UNGROUPED_ROLES__KEY ) from common.djangoapps.student.role_helpers import get_course_roles, has_staff_roles from common.djangoapps.student.tests.factories import AnonymousUserFactory, InstructorFactory, StaffFactory, UserFactory @@ -35,7 +38,7 @@ class RolesTestCase(TestCase): def setUp(self): super().setUp() - self.course_key = CourseKey.from_string('edX/toy/2012_Fall') + self.course_key = CourseKey.from_string('course-v1:course-v1:edX+toy+2012_Fall') self.course_loc = self.course_key.make_usage_key('course', '2012_Fall') self.anonymous_user = AnonymousUserFactory() self.student = UserFactory() @@ -189,8 +192,9 @@ def test_get_orgs_for_user(self): @ddt.ddt class RoleCacheTestCase(TestCase): # lint-amnesty, pylint: disable=missing-class-docstring - IN_KEY = CourseKey.from_string('edX/toy/2012_Fall') - NOT_IN_KEY = CourseKey.from_string('edX/toy/2013_Fall') + IN_KEY_STRING = 'course-v1:edX+toy+2012_Fall' + IN_KEY = CourseKey.from_string(IN_KEY_STRING) + NOT_IN_KEY = CourseKey.from_string('course-v1:edX+toy+2013_Fall') ROLES = ( (CourseStaffRole(IN_KEY), ('staff', IN_KEY, 'edX')), @@ -233,3 +237,75 @@ def test_only_in_role(self, role, target): def test_empty_cache(self, role, target): # lint-amnesty, pylint: disable=unused-argument cache = RoleCache(self.user) assert not cache.has_role(*target) + + def test_get_role_cache_key_for_course_for_course_object_gets_string(self): + """ + Given a valid course key object, get_role_cache_key_for_course + should return the string representation of the key. + """ + course_string = 'course-v1:edX+toy+2012_Fall' + key = CourseKey.from_string(course_string) + key = get_role_cache_key_for_course(key) + assert key == course_string + + def test_get_role_cache_key_for_course_for_undefined_object_returns_default(self): + """ + Given a value None, get_role_cache_key_for_course + should return the default key for ungrouped courses. + """ + key = get_role_cache_key_for_course(None) + assert key == ROLE_CACHE_UNGROUPED_ROLES__KEY + + def test_role_cache_get_roles_set(self): + """ + Test that the RoleCache.all_roles_set getter method returns a flat set of all roles for a user + and that the ._roles attribute is the same as the set to avoid legacy behavior being broken. + """ + lib0 = LibraryLocator.from_string('library-v1:edX+quizzes') + course0 = CourseKey.from_string('course-v1:edX+toy+2012_Summer') + course1 = CourseKey.from_string('course-v1:edX+toy2+2013_Fall') + role_library_v1 = LibraryUserRole(lib0) + role_course_0 = CourseInstructorRole(course0) + role_course_1 = CourseInstructorRole(course1) + + role_library_v1.add_users(self.user) + role_course_0.add_users(self.user) + role_course_1.add_users(self.user) + + cache = RoleCache(self.user) + assert cache.has_role('library_user', lib0, 'edX') + assert cache.has_role('instructor', course0, 'edX') + assert cache.has_role('instructor', course1, 'edX') + + assert len(cache.all_roles_set) == 3 + roles_set = cache.all_roles_set + for role in roles_set: + assert role.course_id.course in ('quizzes', 'toy2', 'toy') + + assert roles_set == cache._roles # pylint: disable=protected-access + + def test_role_cache_roles_by_course_id(self): + """ + Test that the RoleCache.roles_by_course_id getter method returns a dictionary of roles for a user + that are grouped by course_id or if ungrouped by the ROLE_CACHE_UNGROUPED_ROLES__KEY. + """ + lib0 = LibraryLocator.from_string('library-v1:edX+quizzes') + course0 = CourseKey.from_string('course-v1:edX+toy+2012_Summer') + course1 = CourseKey.from_string('course-v1:edX+toy2+2013_Fall') + role_library_v1 = LibraryUserRole(lib0) + role_course_0 = CourseInstructorRole(course0) + role_course_1 = CourseInstructorRole(course1) + role_org_staff = OrgStaffRole('edX') + + role_library_v1.add_users(self.user) + role_course_0.add_users(self.user) + role_course_1.add_users(self.user) + role_org_staff.add_users(self.user) + + cache = RoleCache(self.user) + roles_dict = cache.roles_by_course_id + assert len(roles_dict) == 4 + assert roles_dict.get(ROLE_CACHE_UNGROUPED_ROLES__KEY).pop().role == 'staff' + assert roles_dict.get('library-v1:edX+quizzes').pop().course_id.course == 'quizzes' + assert roles_dict.get('course-v1:edX+toy+2012_Summer').pop().course_id.course == 'toy' + assert roles_dict.get('course-v1:edX+toy2+2013_Fall').pop().course_id.course == 'toy2' diff --git a/lms/djangoapps/courseware/tests/test_access.py b/lms/djangoapps/courseware/tests/test_access.py index b4101a94d84a..e0c5f59a83fb 100644 --- a/lms/djangoapps/courseware/tests/test_access.py +++ b/lms/djangoapps/courseware/tests/test_access.py @@ -56,7 +56,7 @@ SharedModuleStoreTestCase ) from xmodule.modulestore.tests.factories import CourseFactory, BlockFactory # lint-amnesty, pylint: disable=wrong-import-order -from xmodule.partitions.partitions import MINIMUM_STATIC_PARTITION_ID, Group, UserPartition # lint-amnesty, pylint: disable=wrong-import-order +from xmodule.partitions.partitions import MINIMUM_UNUSED_PARTITION_ID, Group, UserPartition # lint-amnesty, pylint: disable=wrong-import-order from openedx.features.enterprise_support.api import add_enterprise_customer_to_session from enterprise.api.v1.serializers import EnterpriseCustomerSerializer from openedx.features.enterprise_support.tests.factories import ( @@ -288,9 +288,9 @@ def test_has_access_in_preview_mode_with_group(self): """ # Note about UserPartition and UserPartition Group IDs: these must not conflict with IDs used # by dynamic user partitions. - partition_id = MINIMUM_STATIC_PARTITION_ID - group_0_id = MINIMUM_STATIC_PARTITION_ID + 1 - group_1_id = MINIMUM_STATIC_PARTITION_ID + 2 + partition_id = MINIMUM_UNUSED_PARTITION_ID + group_0_id = MINIMUM_UNUSED_PARTITION_ID + 1 + group_1_id = MINIMUM_UNUSED_PARTITION_ID + 2 user_partition = UserPartition( partition_id, 'Test User Partition', '', [Group(group_0_id, 'Group 1'), Group(group_1_id, 'Group 2')], diff --git a/lms/djangoapps/teams/team_partition_scheme.py b/lms/djangoapps/teams/team_partition_scheme.py index 8b58997caa57..7cbdaa37fbea 100644 --- a/lms/djangoapps/teams/team_partition_scheme.py +++ b/lms/djangoapps/teams/team_partition_scheme.py @@ -58,7 +58,7 @@ class TeamPartitionScheme: This is how it works: - A user partition is created for each team-set in the course with a unused partition ID generated in runtime - by using generate_int_id() with min=MINIMUM_STATIC_PARTITION_ID and max=MYSQL_MAX_INT. + by using generate_int_id() with min=MINIMUM_UNUSED_PARTITION_ID and max=MYSQL_MAX_INT. - A (Content) group is created for each team in the team-set with the database team ID as the group ID, and the team name as the group name. - A user is assigned to a group if they are a member of the team. diff --git a/openedx/core/djangoapps/verified_track_content/tests/test_partition_scheme.py b/openedx/core/djangoapps/verified_track_content/tests/test_partition_scheme.py index 87d2235e7194..a2667bfa1c0b 100644 --- a/openedx/core/djangoapps/verified_track_content/tests/test_partition_scheme.py +++ b/openedx/core/djangoapps/verified_track_content/tests/test_partition_scheme.py @@ -12,7 +12,7 @@ from common.djangoapps.student.tests.factories import UserFactory from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase # lint-amnesty, pylint: disable=wrong-import-order from xmodule.modulestore.tests.factories import CourseFactory # lint-amnesty, pylint: disable=wrong-import-order -from xmodule.partitions.partitions import MINIMUM_STATIC_PARTITION_ID, UserPartition, ReadOnlyUserPartitionError # lint-amnesty, pylint: disable=wrong-import-order +from xmodule.partitions.partitions import MINIMUM_UNUSED_PARTITION_ID, UserPartition, ReadOnlyUserPartitionError # lint-amnesty, pylint: disable=wrong-import-order from ..partition_scheme import ENROLLMENT_GROUP_IDS, EnrollmentTrackPartitionScheme, EnrollmentTrackUserPartition @@ -63,11 +63,11 @@ def test_from_json_not_supported(self): def test_group_ids(self): """ - Test that group IDs are all less than MINIMUM_STATIC_PARTITION_ID (to avoid overlapping + Test that group IDs are all less than MINIMUM_UNUSED_PARTITION_ID (to avoid overlapping with group IDs associated with cohort and random user partitions). """ for mode in ENROLLMENT_GROUP_IDS: - assert ENROLLMENT_GROUP_IDS[mode]['id'] < MINIMUM_STATIC_PARTITION_ID + assert ENROLLMENT_GROUP_IDS[mode]['id'] < MINIMUM_UNUSED_PARTITION_ID @staticmethod def get_group_by_name(partition, name): diff --git a/openedx/core/lib/teams_config.py b/openedx/core/lib/teams_config.py index c4952b6cdc3a..7210eed0f02f 100644 --- a/openedx/core/lib/teams_config.py +++ b/openedx/core/lib/teams_config.py @@ -19,7 +19,7 @@ TEAM_SCHEME = "team" TEAMS_NAMESPACE = "teams" -# .. toggle_name: course_teams.content_groups_for_teams +# .. toggle_name: teams.content_groups_for_teams # .. toggle_implementation: CourseWaffleFlag # .. toggle_default: False # .. toggle_description: This flag enables content groups for teams. Content groups are virtual groupings of learners diff --git a/requirements/constraints.txt b/requirements/constraints.txt index fbc2bf581bf9..9b5bde48be2f 100644 --- a/requirements/constraints.txt +++ b/requirements/constraints.txt @@ -23,7 +23,7 @@ click>=8.0,<9.0 # The team that owns this package will manually bump this package rather than having it pulled in automatically. # This is to allow them to better control its deployment and to do it in a process that works better # for them. -edx-enterprise==4.18.6 +edx-enterprise==4.19.1 # Stay on LTS version, remove once this is added to common constraint Django<5.0 diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index 9f97ca7f1c86..afb1c5ad427d 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -464,7 +464,7 @@ edx-drf-extensions==10.3.0 # edx-when # edxval # openedx-learning -edx-enterprise==4.18.6 +edx-enterprise==4.19.1 # via # -c requirements/edx/../constraints.txt # -r requirements/edx/kernel.in diff --git a/requirements/edx/development.txt b/requirements/edx/development.txt index 3371a4402622..8c4736447ada 100644 --- a/requirements/edx/development.txt +++ b/requirements/edx/development.txt @@ -743,7 +743,7 @@ edx-drf-extensions==10.3.0 # edx-when # edxval # openedx-learning -edx-enterprise==4.18.6 +edx-enterprise==4.19.1 # via # -c requirements/edx/../constraints.txt # -r requirements/edx/doc.txt diff --git a/requirements/edx/doc.txt b/requirements/edx/doc.txt index a36d33333762..17a26403efa9 100644 --- a/requirements/edx/doc.txt +++ b/requirements/edx/doc.txt @@ -538,7 +538,7 @@ edx-drf-extensions==10.3.0 # edx-when # edxval # openedx-learning -edx-enterprise==4.18.6 +edx-enterprise==4.19.1 # via # -c requirements/edx/../constraints.txt # -r requirements/edx/base.txt diff --git a/requirements/edx/testing.txt b/requirements/edx/testing.txt index d3af7fb45c08..e9e106a4e6a7 100644 --- a/requirements/edx/testing.txt +++ b/requirements/edx/testing.txt @@ -571,7 +571,7 @@ edx-drf-extensions==10.3.0 # edx-when # edxval # openedx-learning -edx-enterprise==4.18.6 +edx-enterprise==4.19.1 # via # -c requirements/edx/../constraints.txt # -r requirements/edx/base.txt diff --git a/xmodule/partitions/partitions.py b/xmodule/partitions/partitions.py index 107a797054a6..9a9d637d318c 100644 --- a/xmodule/partitions/partitions.py +++ b/xmodule/partitions/partitions.py @@ -10,14 +10,21 @@ # pylint: disable=redefined-builtin -# UserPartition IDs must be unique. The Cohort and Random UserPartitions (when they are -# created via Studio) choose an unused ID in the range of 100 (historical) to MAX_INT. Therefore the -# dynamic UserPartitionIDs must be under 100, and they have to be hard-coded to ensure -# they are always the same whenever the dynamic partition is added (since the UserPartition -# ID is stored in the xblock group_access dict). +# Each user partition has an ID that is unique within its learning context. +# The IDs must be valid MySQL primary keys, ie positive integers 1 -> 2^31-1. +# We must carefully manage these IDs, because once they are saved to OLX and the db, they cannot change. +# Here is how we delegate the ID range: +# * 1 -> 49: Unused/Reserved +# * 50: The enrollment track partition +# * 51: The content type gating partition (defined elsewhere) +# * 52-99: Available for other single user partitions, plugged in via setup.py. +# Operators, beware of conflicting IDs between plugins! +# * 100 -> 2^31-1: General namespace for generating IDs at runtime. +# This includes, at least: content partitions, the cohort partition, and teamset partitions. +# When using this range, user partition implementations must check to see that they +# are not conflicting with an existing partition for the course. ENROLLMENT_TRACK_PARTITION_ID = 50 - -MINIMUM_STATIC_PARTITION_ID = 100 +MINIMUM_UNUSED_PARTITION_ID = 100 class UserPartitionError(Exception): diff --git a/xmodule/tests/test_split_test_block.py b/xmodule/tests/test_split_test_block.py index 1324d3a806ea..c51c157a7760 100644 --- a/xmodule/tests/test_split_test_block.py +++ b/xmodule/tests/test_split_test_block.py @@ -10,7 +10,7 @@ from xmodule.modulestore.tests.factories import CourseFactory from xmodule.modulestore.tests.utils import MixedSplitTestCase -from xmodule.partitions.partitions import MINIMUM_STATIC_PARTITION_ID, Group, UserPartition +from xmodule.partitions.partitions import MINIMUM_UNUSED_PARTITION_ID, Group, UserPartition from xmodule.partitions.tests.test_partitions import MockPartitionService, MockUserPartitionScheme, PartitionTestCase from xmodule.split_test_block import ( SplitTestBlock, @@ -94,10 +94,10 @@ def setUp(self): self.course.user_partitions = [ self.user_partition, UserPartition( - MINIMUM_STATIC_PARTITION_ID, 'second_partition', 'Second Partition', + MINIMUM_UNUSED_PARTITION_ID, 'second_partition', 'Second Partition', [ - Group(str(MINIMUM_STATIC_PARTITION_ID + 1), 'abel'), - Group(str(MINIMUM_STATIC_PARTITION_ID + 2), 'baker'), Group("103", 'charlie') + Group(str(MINIMUM_UNUSED_PARTITION_ID + 1), 'abel'), + Group(str(MINIMUM_UNUSED_PARTITION_ID + 2), 'baker'), Group("103", 'charlie') ], MockUserPartitionScheme() )