Skip to content

Commit

Permalink
refactor: rename minimum partition ID constant to be more generic (#3…
Browse files Browse the repository at this point in the history
…4529)

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.
  • Loading branch information
mariajgrimaldi authored May 20, 2024
1 parent 031df2d commit 33b8137
Show file tree
Hide file tree
Showing 9 changed files with 48 additions and 41 deletions.
4 changes: 2 additions & 2 deletions cms/djangoapps/contentstore/course_group_config.py
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
32 changes: 16 additions & 16 deletions cms/djangoapps/contentstore/views/tests/test_block.py
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@
)
from xmodule.partitions.partitions import (
ENROLLMENT_TRACK_PARTITION_ID,
MINIMUM_STATIC_PARTITION_ID,
MINIMUM_UNUSED_PARTITION_ID,
Group,
UserPartition,
)
Expand Down Expand Up @@ -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.
],
),
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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],
Expand All @@ -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",
[
Expand Down Expand Up @@ -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.
Expand Down
4 changes: 2 additions & 2 deletions cms/djangoapps/models/settings/course_metadata.py
Original file line number Diff line number Diff line change
Expand Up @@ -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__)
Expand Down Expand Up @@ -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(
Expand Down
4 changes: 2 additions & 2 deletions cms/lib/xblock/test/test_authoring_mixin.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
)
Expand Down Expand Up @@ -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,
Expand Down
8 changes: 4 additions & 4 deletions lms/djangoapps/courseware/tests/test_access.py
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
Expand Down Expand Up @@ -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')],
Expand Down
2 changes: 1 addition & 1 deletion lms/djangoapps/teams/team_partition_scheme.py
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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):
Expand Down
21 changes: 14 additions & 7 deletions xmodule/partitions/partitions.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down
8 changes: 4 additions & 4 deletions xmodule/tests/test_split_test_block.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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()
)
Expand Down

0 comments on commit 33b8137

Please sign in to comment.