From c9a3c284b2d433e5789282804c45a771c7a80c63 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Wed, 17 Apr 2024 09:32:43 -0400 Subject: [PATCH 1/3] refactor: rename minimum partition ID constant to be more generic Rename MINIMUM_STATIC_PARTITION_ID to MINIMUM_UNUSED_PARTITION_ID so it's not confusing when used to generate IDs for static or dynamic partitions. --- .../contentstore/course_group_config.py | 4 +-- .../contentstore/views/tests/test_block.py | 32 +++++++++---------- cms/lib/xblock/test/test_authoring_mixin.py | 4 +-- .../courseware/tests/test_access.py | 8 ++--- .../tests/test_partition_scheme.py | 6 ++-- xmodule/partitions/partitions.py | 2 +- xmodule/tests/test_split_test_block.py | 8 ++--- 7 files changed, 32 insertions(+), 32 deletions(-) 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/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/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/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/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/xmodule/partitions/partitions.py b/xmodule/partitions/partitions.py index 107a797054a6..efdae5a9e449 100644 --- a/xmodule/partitions/partitions.py +++ b/xmodule/partitions/partitions.py @@ -17,7 +17,7 @@ # ID is stored in the xblock group_access dict). 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() ) From 90ed49c8c7630b8eb47a9c06436e267af8eb79ea Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Wed, 17 Apr 2024 13:06:53 -0400 Subject: [PATCH 2/3] refactor: address PR review --- xmodule/partitions/partitions.py | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/xmodule/partitions/partitions.py b/xmodule/partitions/partitions.py index efdae5a9e449..9a9d637d318c 100644 --- a/xmodule/partitions/partitions.py +++ b/xmodule/partitions/partitions.py @@ -10,13 +10,20 @@ # 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_UNUSED_PARTITION_ID = 100 From 3b10d4c75e2df796e3db9171c042a1f7c400e495 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Fri, 17 May 2024 11:24:30 -0400 Subject: [PATCH 3/3] refactor: change variable name for master changes --- cms/djangoapps/models/settings/course_metadata.py | 4 ++-- lms/djangoapps/teams/team_partition_scheme.py | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) 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/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.