From 031df2dbd3b819e951df646493e4ed62fa79756b Mon Sep 17 00:00:00 2001 From: Hamzah Ullah Date: Mon, 20 May 2024 10:11:10 -0400 Subject: [PATCH 1/6] chore: Upgrade edx-enterprise to 4.19.0 (#34821) --- requirements/constraints.txt | 2 +- requirements/edx/base.txt | 2 +- requirements/edx/development.txt | 2 +- requirements/edx/doc.txt | 2 +- requirements/edx/testing.txt | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) diff --git a/requirements/constraints.txt b/requirements/constraints.txt index fbc2bf581bf9..f28bc4e44350 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.0 # 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..65ecdbc151a0 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.0 # via # -c requirements/edx/../constraints.txt # -r requirements/edx/kernel.in diff --git a/requirements/edx/development.txt b/requirements/edx/development.txt index 6291bdbd752b..2c057a427168 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.0 # 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..c1ebbcef3662 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.0 # via # -c requirements/edx/../constraints.txt # -r requirements/edx/base.txt diff --git a/requirements/edx/testing.txt b/requirements/edx/testing.txt index eb168e25c04c..472aea8daa2f 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.0 # via # -c requirements/edx/../constraints.txt # -r requirements/edx/base.txt From 33b8137763aad944b8de72fd16c26c2dd679e914 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Mon, 20 May 2024 14:38:58 -0400 Subject: [PATCH 2/6] refactor: rename minimum partition ID constant to be more generic (#34529) 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 +++++++++---------- .../models/settings/course_metadata.py | 4 +-- cms/lib/xblock/test/test_authoring_mixin.py | 4 +-- .../courseware/tests/test_access.py | 8 ++--- lms/djangoapps/teams/team_partition_scheme.py | 2 +- .../tests/test_partition_scheme.py | 6 ++-- xmodule/partitions/partitions.py | 21 ++++++++---- xmodule/tests/test_split_test_block.py | 8 ++--- 9 files changed, 48 insertions(+), 41 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/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/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/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() ) From 9431e96d3c368924d877cdd5cc5217526d18a430 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Mon, 20 May 2024 14:50:17 -0400 Subject: [PATCH 3/6] fix: add in-line docs correct namespace for content_groups_for_teams (#34783) --- openedx/core/lib/teams_config.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 From e7b4db086296544d00814e851bfea06352d14a43 Mon Sep 17 00:00:00 2001 From: Hamzah Ullah Date: Mon, 20 May 2024 15:34:35 -0400 Subject: [PATCH 4/6] chore: Upgrade edx-enterprise to 4.19.1 (#34822) --- requirements/constraints.txt | 2 +- requirements/edx/base.txt | 2 +- requirements/edx/development.txt | 2 +- requirements/edx/doc.txt | 2 +- requirements/edx/testing.txt | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) diff --git a/requirements/constraints.txt b/requirements/constraints.txt index f28bc4e44350..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.19.0 +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 65ecdbc151a0..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.19.0 +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 2c057a427168..65d7e1912fad 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.19.0 +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 c1ebbcef3662..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.19.0 +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 472aea8daa2f..5cb71d244528 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.19.0 +edx-enterprise==4.19.1 # via # -c requirements/edx/../constraints.txt # -r requirements/edx/base.txt From e95d7e7e3239c0fe84448e1ef5314b4169dd6f7e Mon Sep 17 00:00:00 2001 From: Jesper Hodge Date: Mon, 20 May 2024 14:37:58 -0400 Subject: [PATCH 5/6] fix: libraries performance problem This is an attempt to fix a performance problem on the libraries home page. When you go to studio home and click on the libraries tab, on prod it will be quick for admins but extremely slow for course instructors (> 12 seconds) and leads to timeouts. It grows with the number of libraries that are assigned to the instructor. The Python code for the request to load libraries for a particular user goes through all existing libraries and then checks all of the user's roles for each library, which results in a complexity of O(l*r), l=libraries, r=roles. This PR improves the complexity to O(l). The BulkRoleCache and RoleCache classes were using a python set to store all roles for a particular user. A user can have a large number of roles, and lookup speed of iterating through a set is slow (O(n)). Most roles don't have the same course id, however. So if you have the course id of the role you're looking for, we can use a dict of course ids that contain related roles. The number of roles per course id is negligible, so we arrive at a lookup speed of O(1) when looking up a user's roles that belong to a specific course id. The BulkRoleCache now caches and stores user roles in a data structure like this: { 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. For example, Global Staff roles. }, user_id_2: { ... } # Similar structure for another user } While this changes the data structure used to store roles under the hood and adds the new property `roles_by_course_id` to the RoleCache, when initializing the RoleCache will store roles additionally in the previous data structure - as a flat set - in the `_roles` property accessible via `all_roles_set`. This establishes backwards compatibility. We are now storing roles twice in the RoleCache (in each of the two data structures), which means this takes twice as much memory, but only in the scope of a request. --- .../contentstore/rest_api/v1/views/home.py | 1 + cms/djangoapps/contentstore/views/course.py | 2 +- common/djangoapps/student/auth.py | 3 +- common/djangoapps/student/role_helpers.py | 2 +- common/djangoapps/student/roles.py | 85 ++++++++++++++++--- common/djangoapps/student/tests/test_roles.py | 84 +++++++++++++++++- 6 files changed, 157 insertions(+), 20 deletions(-) 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/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/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' From 362899e7972c97518c9144c9dcf59719f2e47a22 Mon Sep 17 00:00:00 2001 From: Ahtisham Shahid Date: Tue, 21 May 2024 13:31:41 +0500 Subject: [PATCH 6/6] fix: Added message to Course update notification in case of small text (#34819) --- cms/djangoapps/contentstore/utils.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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",