Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: rename minimum partition ID constant to be more generic #34529

Merged
merged 3 commits into from
May 20, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

# 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

To make the new name clearer, I've rewritten the comment above. What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's much better! Thank you for all the suggestions.



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
Loading