From 4644cbdba0c1245c8c48b175350ec4a49d5701ca Mon Sep 17 00:00:00 2001 From: Kyle D McCormick Date: Tue, 21 Apr 2026 13:12:42 -0400 Subject: [PATCH 01/15] build: Upgrade openedx-core pin, 0.39.2 -> 0.44.0 --- 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 f051cc8f6114..73e9f6effa10 100644 --- a/requirements/constraints.txt +++ b/requirements/constraints.txt @@ -65,7 +65,7 @@ numpy<2.0.0 # breaking changes which openedx-core devs want to roll out manually. New patch versions # are OK to accept automatically. # Issue for unpinning: https://github.com/openedx/edx-platform/issues/35269 -openedx-core<0.40 +openedx-core<0.45 # Date: 2023-11-29 # Open AI version 1.0.0 dropped support for openai.ChatCompletion which is currently in use in enterprise. diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index 298f05e12aab..71790d659ff7 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -840,7 +840,7 @@ openedx-calc==5.0.0 # via # -r requirements/edx/kernel.in # xblocks-contrib -openedx-core==0.39.2 +openedx-core==0.44.0 # via # -c requirements/constraints.txt # -r requirements/edx/kernel.in diff --git a/requirements/edx/development.txt b/requirements/edx/development.txt index ea0fd8e68eae..3ad4c94197fd 100644 --- a/requirements/edx/development.txt +++ b/requirements/edx/development.txt @@ -1394,7 +1394,7 @@ openedx-calc==5.0.0 # -r requirements/edx/doc.txt # -r requirements/edx/testing.txt # xblocks-contrib -openedx-core==0.39.2 +openedx-core==0.44.0 # via # -c requirements/constraints.txt # -r requirements/edx/doc.txt diff --git a/requirements/edx/doc.txt b/requirements/edx/doc.txt index 194b5438ce1f..b7f29a68f1df 100644 --- a/requirements/edx/doc.txt +++ b/requirements/edx/doc.txt @@ -1018,7 +1018,7 @@ openedx-calc==5.0.0 # via # -r requirements/edx/base.txt # xblocks-contrib -openedx-core==0.39.2 +openedx-core==0.44.0 # via # -c requirements/constraints.txt # -r requirements/edx/base.txt diff --git a/requirements/edx/testing.txt b/requirements/edx/testing.txt index eb9732129f73..1e14484defc2 100644 --- a/requirements/edx/testing.txt +++ b/requirements/edx/testing.txt @@ -1065,7 +1065,7 @@ openedx-calc==5.0.0 # via # -r requirements/edx/base.txt # xblocks-contrib -openedx-core==0.39.2 +openedx-core==0.44.0 # via # -c requirements/constraints.txt # -r requirements/edx/base.txt From 30080e4abb52ef6f09291ba59d8ccbc81e92c302 Mon Sep 17 00:00:00 2001 From: Kyle D McCormick Date: Tue, 21 Apr 2026 13:49:39 -0400 Subject: [PATCH 02/15] refactor: Rename key_field to ref_field for openedx-core 0.43.0 Renames the openedx_django_lib.fields import in EntityLinkBase from the removed key_field helper to ref_field. Co-Authored-By: Claude Opus 4.7 (1M context) --- cms/djangoapps/contentstore/models.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cms/djangoapps/contentstore/models.py b/cms/djangoapps/contentstore/models.py index c39e04c299dc..d80517c2a842 100644 --- a/cms/djangoapps/contentstore/models.py +++ b/cms/djangoapps/contentstore/models.py @@ -17,7 +17,7 @@ from opaque_keys.edx.locator import LibraryContainerLocator from openedx_content.api import get_published_version from openedx_content.models_api import Component, Container -from openedx_django_lib.fields import immutable_uuid_field, key_field, manual_date_time_field +from openedx_django_lib.fields import immutable_uuid_field, manual_date_time_field, ref_field logger = logging.getLogger(__name__) @@ -87,7 +87,7 @@ class EntityLinkBase(models.Model): """ uuid = immutable_uuid_field() # Search by library/upstream context key - upstream_context_key = key_field( + upstream_context_key = ref_field( help_text=_("Upstream context key i.e., learning_package/library key"), db_index=True, ) From c6f593de9dd068a00008c74e9f7ec1a20f1733db Mon Sep 17 00:00:00 2001 From: Kyle D McCormick Date: Tue, 21 Apr 2026 14:02:22 -0400 Subject: [PATCH 03/15] refactor: Rename LearningPackage.key to package_ref for openedx-core 0.43.0 Updates callers of get_learning_package_by_key (renamed to get_learning_package_by_ref), create_learning_package, and update_learning_package to use the new package_ref kwarg, and switches .key attribute reads to .package_ref. Co-Authored-By: Claude Opus 4.7 (1M context) --- cms/djangoapps/modulestore_migrator/api/read_api.py | 2 +- openedx/core/djangoapps/content/search/tests/test_api.py | 2 +- openedx/core/djangoapps/content_libraries/api/libraries.py | 4 ++-- openedx/core/djangoapps/content_libraries/rest_api/blocks.py | 2 +- openedx/core/djangoapps/xblock/api.py | 2 +- .../core/djangoapps/xblock/runtime/openedx_content_runtime.py | 2 +- 6 files changed, 7 insertions(+), 7 deletions(-) diff --git a/cms/djangoapps/modulestore_migrator/api/read_api.py b/cms/djangoapps/modulestore_migrator/api/read_api.py index 064223dc9633..e62a684476f6 100644 --- a/cms/djangoapps/modulestore_migrator/api/read_api.py +++ b/cms/djangoapps/modulestore_migrator/api/read_api.py @@ -209,7 +209,7 @@ def _block_migration_success( """ Build an instance of the migration success dataclass """ - target_library_key = LibraryLocatorV2.from_string(target.learning_package.key) + target_library_key = LibraryLocatorV2.from_string(target.learning_package.package_ref) target_key: LibraryUsageLocatorV2 | LibraryContainerLocator if hasattr(target, "component"): target_key = library_component_usage_key(target_library_key, target.component) diff --git a/openedx/core/djangoapps/content/search/tests/test_api.py b/openedx/core/djangoapps/content/search/tests/test_api.py index 8a29bb450326..3a4cdc32eafa 100644 --- a/openedx/core/djangoapps/content/search/tests/test_api.py +++ b/openedx/core/djangoapps/content/search/tests/test_api.py @@ -188,7 +188,7 @@ def setUp(self) -> None: tagging_api.add_tag_to_taxonomy(self.taxonomyB, "four") # Create a collection: - self.learning_package = content_api.get_learning_package_by_key(self.library.key) + self.learning_package = content_api.get_learning_package_by_ref(str(self.library.key)) with freeze_time(self.created_date): self.collection = content_api.create_collection( learning_package_id=self.learning_package.id, diff --git a/openedx/core/djangoapps/content_libraries/api/libraries.py b/openedx/core/djangoapps/content_libraries/api/libraries.py index bf91039b686b..90451da182d8 100644 --- a/openedx/core/djangoapps/content_libraries/api/libraries.py +++ b/openedx/core/djangoapps/content_libraries/api/libraries.py @@ -461,14 +461,14 @@ def create_library( # and also update its title/description in case they differ. content_api.update_learning_package( learning_package.id, - key=str(ref.library_key), + package_ref=str(ref.library_key), title=title, description=description, ) else: # We have to generate a new LearningPackage for this library. learning_package = content_api.create_learning_package( - key=str(ref.library_key), + package_ref=str(ref.library_key), title=title, description=description, ) diff --git a/openedx/core/djangoapps/content_libraries/rest_api/blocks.py b/openedx/core/djangoapps/content_libraries/rest_api/blocks.py index 518ac5b31fe3..a1471ea65a8e 100644 --- a/openedx/core/djangoapps/content_libraries/rest_api/blocks.py +++ b/openedx/core/djangoapps/content_libraries/rest_api/blocks.py @@ -379,7 +379,7 @@ def get_component_version_asset(request, component_version_uuid, asset_path): # Permissions check... learning_package = component_version.component.learning_package - library_key = LibraryLocatorV2.from_string(learning_package.key) + library_key = LibraryLocatorV2.from_string(learning_package.package_ref) api.require_permission_for_library_key( library_key, request.user, permissions.CAN_VIEW_THIS_CONTENT_LIBRARY, ) diff --git a/openedx/core/djangoapps/xblock/api.py b/openedx/core/djangoapps/xblock/api.py index 0ab620db9ab3..211b14871e7f 100644 --- a/openedx/core/djangoapps/xblock/api.py +++ b/openedx/core/djangoapps/xblock/api.py @@ -199,7 +199,7 @@ def get_component_from_usage_key(usage_key: UsageKeyV2) -> Component: This is a lower-level function that will return a Component even if there is no current draft version of that Component (because it's been soft-deleted). """ - learning_package = content_api.get_learning_package_by_key( + learning_package = content_api.get_learning_package_by_ref( str(usage_key.context_key) ) return content_api.get_component_by_key( diff --git a/openedx/core/djangoapps/xblock/runtime/openedx_content_runtime.py b/openedx/core/djangoapps/xblock/runtime/openedx_content_runtime.py index 3a90fb27a5f7..65de9c77332b 100644 --- a/openedx/core/djangoapps/xblock/runtime/openedx_content_runtime.py +++ b/openedx/core/djangoapps/xblock/runtime/openedx_content_runtime.py @@ -326,7 +326,7 @@ def _get_component_from_usage_key(self, usage_key): TODO: This is the third place where we're implementing this. Figure out where the definitive place should be and have everything else call that. """ - learning_package = content_api.get_learning_package_by_key(str(usage_key.lib_key)) + learning_package = content_api.get_learning_package_by_ref(str(usage_key.lib_key)) try: component = content_api.get_component_by_key( learning_package.id, From 4668319b3404725cba1f33dafdd996dadcd75eb3 Mon Sep 17 00:00:00 2001 From: Kyle D McCormick Date: Tue, 21 Apr 2026 14:14:19 -0400 Subject: [PATCH 04/15] refactor: Rename PublishableEntity.key to entity_ref for openedx-core 0.43.0 Switches callers of get_publishable_entity_by_key (now _by_ref) to the new name, and renames .key attribute reads on PublishableEntity, Component, and Container (via PublishableEntityMixin) to .entity_ref. Query filters using key__in/entity__key become entity_ref__in/ entity__entity_ref. The set_library_item_collections param entity_key is renamed to entity_ref for consistency. Collection.key reads are intentionally left for the collection_code rename in a later commit. Co-Authored-By: Claude Opus 4.7 (1M context) --- openedx/core/djangoapps/content/search/api.py | 2 +- .../djangoapps/content/search/documents.py | 2 +- .../content/search/tests/test_api.py | 2 +- .../djangoapps/content_libraries/api/blocks.py | 8 ++++---- .../content_libraries/api/collections.py | 18 +++++++++--------- .../api/container_metadata.py | 3 ++- .../content_libraries/api/containers.py | 4 ++-- .../content_libraries/rest_api/blocks.py | 2 +- .../content_libraries/rest_api/containers.py | 2 +- .../core/djangoapps/content_libraries/tasks.py | 6 +++--- .../content_libraries/tests/test_api.py | 2 +- 11 files changed, 26 insertions(+), 25 deletions(-) diff --git a/openedx/core/djangoapps/content/search/api.py b/openedx/core/djangoapps/content/search/api.py index f6bfdf13be77..5c3c9e16b975 100644 --- a/openedx/core/djangoapps/content/search/api.py +++ b/openedx/core/djangoapps/content/search/api.py @@ -677,7 +677,7 @@ def index_container_batch(batch, num_done, library_key) -> int: doc.update(searchable_doc_containers(container_key, "sections")) docs.append(doc) except Exception as err: # pylint: disable=broad-except - status_cb(f"Error indexing container {container.key}: {err}") + status_cb(f"Error indexing container {container.entity_ref}: {err}") num_done += 1 if docs: diff --git a/openedx/core/djangoapps/content/search/documents.py b/openedx/core/djangoapps/content/search/documents.py index b986966ec42c..0ee3b4839287 100644 --- a/openedx/core/djangoapps/content/search/documents.py +++ b/openedx/core/djangoapps/content/search/documents.py @@ -450,7 +450,7 @@ def searchable_doc_collections(object_id: OpaqueKey) -> dict: component = lib_api.get_component_from_usage_key(object_id) collections = content_api.get_entity_collections( component.learning_package_id, - component.key, + component.entity_ref, ).values('key', 'title') elif isinstance(object_id, LibraryContainerLocator): container = lib_api.get_container(object_id, include_collections=True) diff --git a/openedx/core/djangoapps/content/search/tests/test_api.py b/openedx/core/djangoapps/content/search/tests/test_api.py index 3a4cdc32eafa..7f666ef071e8 100644 --- a/openedx/core/djangoapps/content/search/tests/test_api.py +++ b/openedx/core/djangoapps/content/search/tests/test_api.py @@ -536,7 +536,7 @@ def test_reindex_meilisearch_library_block_error(self, mock_meilisearch) -> None def mocked_from_component(lib_key, component): # Simulate an error when processing problem 1 - if component.key == 'xblock.v1:problem:p1': + if component.entity_ref == 'xblock.v1:problem:p1': raise Exception('Error') return orig_from_component(lib_key, component) diff --git a/openedx/core/djangoapps/content_libraries/api/blocks.py b/openedx/core/djangoapps/content_libraries/api/blocks.py index 2ddc06fde254..d969f9edbe8c 100644 --- a/openedx/core/djangoapps/content_libraries/api/blocks.py +++ b/openedx/core/djangoapps/content_libraries/api/blocks.py @@ -178,7 +178,7 @@ def get_library_block(usage_key: LibraryUsageLocatorV2, include_collections=Fals if include_collections: associated_collections = content_api.get_entity_collections( component.learning_package_id, - component.key, + component.entity_ref, ).values('key', 'title') else: associated_collections = None @@ -725,7 +725,7 @@ def send_block_deleted_signal(): send_block_deleted_signal() raise - affected_collections = content_api.get_entity_collections(component.learning_package_id, component.key) + affected_collections = content_api.get_entity_collections(component.learning_package_id, component.entity_ref) affected_containers = get_containers_contains_item(usage_key) content_api.soft_delete_draft(component.id, deleted_by=user_id) @@ -770,7 +770,7 @@ def restore_library_block(usage_key: LibraryUsageLocatorV2, user_id: int | None """ component = get_component_from_usage_key(usage_key) library_key = usage_key.context_key - affected_collections = content_api.get_entity_collections(component.learning_package_id, component.key) + affected_collections = content_api.get_entity_collections(component.learning_package_id, component.entity_ref) # Set draft version back to the latest available component version id. content_api.set_draft_version( @@ -985,7 +985,7 @@ def publish_component_changes(usage_key: LibraryUsageLocatorV2, user_id: int): learning_package = content_library.learning_package assert learning_package # The core publishing API is based on draft objects, so find the draft that corresponds to this component: - drafts_to_publish = content_api.get_all_drafts(learning_package.id).filter(entity__key=component.key) + drafts_to_publish = content_api.get_all_drafts(learning_package.id).filter(entity__entity_ref=component.entity_ref) # Publish the component and update anything that needs to be updated (e.g. search index): publish_log = content_api.publish_from_drafts( learning_package.id, draft_qset=drafts_to_publish, published_by=user_id, diff --git a/openedx/core/djangoapps/content_libraries/api/collections.py b/openedx/core/djangoapps/content_libraries/api/collections.py index 9d011bdae363..175304119716 100644 --- a/openedx/core/djangoapps/content_libraries/api/collections.py +++ b/openedx/core/djangoapps/content_libraries/api/collections.py @@ -127,8 +127,8 @@ def update_library_collection_items( assert content_library.learning_package_id assert content_library.library_key == library_key - # Fetch the Component.key values for the provided UsageKeys. - item_keys = [] + # Fetch the Component.entity_ref values for the provided UsageKeys. + item_refs = [] for opaque_key in opaque_keys: if isinstance(opaque_key, LibraryContainerLocator): try: @@ -139,7 +139,7 @@ def update_library_collection_items( except Collection.DoesNotExist as exc: raise ContentLibraryContainerNotFound(opaque_key) from exc - item_keys.append(container.key) + item_refs.append(container.entity_ref) elif isinstance(opaque_key, UsageKeyV2): # Parse the block_family from the key to use as namespace. block_type = BlockTypeKey.from_string(str(opaque_key)) @@ -153,13 +153,13 @@ def update_library_collection_items( except Component.DoesNotExist as exc: raise ContentLibraryBlockNotFound(opaque_key) from exc - item_keys.append(component.key) + item_refs.append(component.entity_ref) else: # This should never happen, but just in case. raise ValueError(f"Invalid opaque_key: {opaque_key}") entities_qset = PublishableEntity.objects.filter( - key__in=item_keys, + entity_ref__in=item_refs, ) if remove: @@ -181,7 +181,7 @@ def update_library_collection_items( def set_library_item_collections( library_key: LibraryLocatorV2, - entity_key: str, + entity_ref: str, *, collection_keys: list[str], created_by: int | None = None, @@ -207,12 +207,12 @@ def set_library_item_collections( assert content_library.learning_package_id assert content_library.library_key == library_key - publishable_entity = content_api.get_publishable_entity_by_key( + publishable_entity = content_api.get_publishable_entity_by_ref( content_library.learning_package_id, - key=entity_key, + entity_ref=entity_ref, ) - # Note: Component.key matches its PublishableEntity.key + # Note: Component.entity_ref matches its PublishableEntity.entity_ref collection_qs = content_api.get_collections(content_library.learning_package_id).filter( key__in=collection_keys ) diff --git a/openedx/core/djangoapps/content_libraries/api/container_metadata.py b/openedx/core/djangoapps/content_libraries/api/container_metadata.py index 5bb8bcd50ae4..ead5c6f051d6 100644 --- a/openedx/core/djangoapps/content_libraries/api/container_metadata.py +++ b/openedx/core/djangoapps/content_libraries/api/container_metadata.py @@ -366,7 +366,8 @@ def library_container_locator( if container_type_code not in LIBRARY_ALLOWED_CONTAINER_TYPES: raise ValueError(f"Unsupported container type for content libraries: {container!r}") - return LibraryContainerLocator(library_key, container_type=container_type_code, container_id=container.key) + # TODO: verify whether container_id should use entity_ref (opaque) or container_code (local slug). + return LibraryContainerLocator(library_key, container_type=container_type_code, container_id=container.entity_ref) def get_container_from_key(container_key: LibraryContainerLocator, include_deleted=False) -> Container: diff --git a/openedx/core/djangoapps/content_libraries/api/containers.py b/openedx/core/djangoapps/content_libraries/api/containers.py index 5db34dc753da..93d42b282717 100644 --- a/openedx/core/djangoapps/content_libraries/api/containers.py +++ b/openedx/core/djangoapps/content_libraries/api/containers.py @@ -226,7 +226,7 @@ def send_container_deleted_signal(): # Fetch related collections and containers before soft-delete affected_collections = content_api.get_entity_collections( container.publishable_entity.learning_package_id, - container.key, + container.entity_ref, ) affected_containers = get_containers_contains_item(container_key) # Get children containers or components to update their index data @@ -291,7 +291,7 @@ def restore_container(container_key: LibraryContainerLocator) -> None: affected_collections = content_api.get_entity_collections( container.publishable_entity.learning_package_id, - container.key, + container.entity_ref, ) content_api.set_draft_version(container.id, container.versioning.latest.pk) diff --git a/openedx/core/djangoapps/content_libraries/rest_api/blocks.py b/openedx/core/djangoapps/content_libraries/rest_api/blocks.py index a1471ea65a8e..81fc6bd2f1b6 100644 --- a/openedx/core/djangoapps/content_libraries/rest_api/blocks.py +++ b/openedx/core/djangoapps/content_libraries/rest_api/blocks.py @@ -272,7 +272,7 @@ def patch(self, request: RestRequest, usage_key_str) -> Response: collection_keys = serializer.validated_data['collection_keys'] api.set_library_item_collections( library_key=key.lib_key, - entity_key=component.publishable_entity.key, + entity_ref=component.publishable_entity.entity_ref, collection_keys=collection_keys, created_by=request.user.id, content_library=content_library, diff --git a/openedx/core/djangoapps/content_libraries/rest_api/containers.py b/openedx/core/djangoapps/content_libraries/rest_api/containers.py index 04dde384361e..12a4132920c3 100644 --- a/openedx/core/djangoapps/content_libraries/rest_api/containers.py +++ b/openedx/core/djangoapps/content_libraries/rest_api/containers.py @@ -346,7 +346,7 @@ def patch(self, request: RestRequest, container_key: LibraryContainerLocator) -> collection_keys = serializer.validated_data['collection_keys'] api.set_library_item_collections( library_key=container_key.lib_key, - entity_key=container_key.container_id, + entity_ref=container_key.container_id, collection_keys=collection_keys, created_by=request.user.id, content_library=content_library, diff --git a/openedx/core/djangoapps/content_libraries/tasks.py b/openedx/core/djangoapps/content_libraries/tasks.py index bb5db0a397c1..378ee9a19161 100644 --- a/openedx/core/djangoapps/content_libraries/tasks.py +++ b/openedx/core/djangoapps/content_libraries/tasks.py @@ -142,7 +142,7 @@ def send_events_after_publish(publish_log_pk: int, library_key_str: str) -> None pass else: log.warning( - f"PublishableEntity {record.entity.pk} / {record.entity.key} was modified during publish operation " + f"PublishableEntity {record.entity.pk} / {record.entity.entity_ref} was modified during publish operation " "but is of unknown type." ) @@ -246,13 +246,13 @@ def send_events_after_revert(draft_change_log_id: int, library_key_str: str) -> updated_container_keys.add(container_key) else: log.warning( - f"PublishableEntity {record.entity.pk} / {record.entity.key} was modified during publish operation " + f"PublishableEntity {record.entity.pk} / {record.entity.entity_ref} was modified during publish operation " "but is of unknown type." ) # If any collections contain this entity, their item count may need to be updated, e.g. if this was a # newly created component in the collection and is now deleted, or this was deleted and is now re-added. for parent_collection in content_api.get_entity_collections( - record.entity.learning_package_id, record.entity.key, + record.entity.learning_package_id, record.entity.entity_ref, ): collection_key = api.library_collection_locator( library_key=library_key, diff --git a/openedx/core/djangoapps/content_libraries/tests/test_api.py b/openedx/core/djangoapps/content_libraries/tests/test_api.py index 408d16618569..c62cf8eda861 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_api.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_api.py @@ -318,7 +318,7 @@ def test_set_library_component_collections(self) -> None: component = api.get_component_from_usage_key(UsageKeyV2.from_string(self.lib2_problem_block["id"])) api.set_library_item_collections( library_key=self.lib2.library_key, - entity_key=component.publishable_entity.key, + entity_ref=component.publishable_entity.entity_ref, collection_keys=[self.col2.key, self.col3.key], ) From 85dcf9627e19fc276862f0bae4c14fdf449391c2 Mon Sep 17 00:00:00 2001 From: Kyle D McCormick Date: Tue, 21 Apr 2026 14:16:09 -0400 Subject: [PATCH 05/15] refactor: Rename Component.local_key to component_code for openedx-core 0.43.0 Updates callers of get_component_by_key/component_exists_by_key (now _by_code) and switches the local_key kwarg on create_component, create_component_and_version, and related queries to component_code. Also renames component.local_key reads to component.component_code and adjusts a modulestore_migrator container query that filtered on publishable_entity__key (now entity_ref). Co-Authored-By: Claude Opus 4.7 (1M context) --- cms/djangoapps/modulestore_migrator/tasks.py | 8 ++++---- .../modulestore_migrator/tests/test_tasks.py | 12 ++++++------ .../core/djangoapps/content_libraries/api/blocks.py | 4 ++-- .../djangoapps/content_libraries/api/collections.py | 4 ++-- .../djangoapps/content_libraries/api/libraries.py | 2 +- .../djangoapps/content_libraries/library_context.py | 4 ++-- openedx/core/djangoapps/xblock/api.py | 4 ++-- .../xblock/runtime/openedx_content_runtime.py | 4 ++-- 8 files changed, 21 insertions(+), 21 deletions(-) diff --git a/cms/djangoapps/modulestore_migrator/tasks.py b/cms/djangoapps/modulestore_migrator/tasks.py index b54b9191e6ba..95048b92feb2 100644 --- a/cms/djangoapps/modulestore_migrator/tasks.py +++ b/cms/djangoapps/modulestore_migrator/tasks.py @@ -346,13 +346,13 @@ def _import_structure( LibraryUsageLocatorV2(target_library.key, block_type, block_id) # type: ignore[abstract] for block_type, block_id in content_api.get_components(migration.target.id).values_list( - "component_type__name", "local_key" + "component_type__name", "component_code" ) ), used_container_slugs=set( content_api.get_containers( migration.target.id - ).values_list("publishable_entity__key", flat=True) + ).values_list("publishable_entity__entity_ref", flat=True) ), previous_block_migrations=( get_migration_blocks(source_data.previous_migration.pk) @@ -932,7 +932,7 @@ def _migrate_component( try: component = content_api.get_components(context.target_package_id).get( component_type=component_type, - local_key=target_key.block_id, + component_code=target_key.block_id, ) component_existed = True # Do we have a specific method for this? @@ -953,7 +953,7 @@ def _migrate_component( component = content_api.create_component( context.target_package_id, component_type=component_type, - local_key=target_key.block_id, + component_code=target_key.block_id, created=context.created_at, created_by=context.created_by, ) diff --git a/cms/djangoapps/modulestore_migrator/tests/test_tasks.py b/cms/djangoapps/modulestore_migrator/tests/test_tasks.py index 2285dd7d77e8..76f94da16bc2 100644 --- a/cms/djangoapps/modulestore_migrator/tests/test_tasks.py +++ b/cms/djangoapps/modulestore_migrator/tests/test_tasks.py @@ -718,7 +718,7 @@ def test_migrate_container_creates_new_container(self): component_type=content_api.get_or_create_component_type( "xblock.v1", "problem" ), - local_key="child_problem_1", + component_code="child_problem_1", created=timezone.now(), created_by=self.user.id, ) @@ -734,7 +734,7 @@ def test_migrate_container_creates_new_container(self): component_type=content_api.get_or_create_component_type( "xblock.v1", "html" ), - local_key="child_html_1", + component_code="child_html_1", created=timezone.now(), created_by=self.user.id, ) @@ -906,7 +906,7 @@ def test_migrate_container_preserves_child_order(self): component_type=content_api.get_or_create_component_type( "xblock.v1", "problem" ), - local_key=f"child_problem_{i}", + component_code=f"child_problem_{i}", created=timezone.now(), created_by=self.user.id, ) @@ -946,7 +946,7 @@ def test_migrate_container_with_mixed_child_types(self): component_type=content_api.get_or_create_component_type( "xblock.v1", "problem" ), - local_key="mixed_problem", + component_code="mixed_problem", created=timezone.now(), created_by=self.user.id, ) @@ -962,7 +962,7 @@ def test_migrate_container_with_mixed_child_types(self): component_type=content_api.get_or_create_component_type( "xblock.v1", "html" ), - local_key="mixed_html", + component_code="mixed_html", created=timezone.now(), created_by=self.user.id, ) @@ -978,7 +978,7 @@ def test_migrate_container_with_mixed_child_types(self): component_type=content_api.get_or_create_component_type( "xblock.v1", "video" ), - local_key="mixed_video", + component_code="mixed_video", created=timezone.now(), created_by=self.user.id, ) diff --git a/openedx/core/djangoapps/content_libraries/api/blocks.py b/openedx/core/djangoapps/content_libraries/api/blocks.py index d969f9edbe8c..f2f415dfa2e0 100644 --- a/openedx/core/djangoapps/content_libraries/api/blocks.py +++ b/openedx/core/djangoapps/content_libraries/api/blocks.py @@ -425,7 +425,7 @@ def _import_staged_block( component = content_api.create_component( # noqa: F841 learning_package.id, component_type=component_type, - local_key=usage_key.block_id, + component_code=usage_key.block_id, created=now, created_by=user.id, ) @@ -1046,7 +1046,7 @@ def _create_component_for_block( component, component_version = content_api.create_component_and_version( learning_package.id, component_type=component_type, - local_key=usage_key.block_id, + component_code=usage_key.block_id, title=display_name, created=now, created_by=user_id, diff --git a/openedx/core/djangoapps/content_libraries/api/collections.py b/openedx/core/djangoapps/content_libraries/api/collections.py index 175304119716..6ab2a98c563b 100644 --- a/openedx/core/djangoapps/content_libraries/api/collections.py +++ b/openedx/core/djangoapps/content_libraries/api/collections.py @@ -144,11 +144,11 @@ def update_library_collection_items( # Parse the block_family from the key to use as namespace. block_type = BlockTypeKey.from_string(str(opaque_key)) try: - component = content_api.get_component_by_key( + component = content_api.get_component_by_code( content_library.learning_package_id, namespace=block_type.block_family, type_name=opaque_key.block_type, - local_key=opaque_key.block_id, + component_code=opaque_key.block_id, ) except Component.DoesNotExist as exc: raise ContentLibraryBlockNotFound(opaque_key) from exc diff --git a/openedx/core/djangoapps/content_libraries/api/libraries.py b/openedx/core/djangoapps/content_libraries/api/libraries.py index 90451da182d8..68d4258bb065 100644 --- a/openedx/core/djangoapps/content_libraries/api/libraries.py +++ b/openedx/core/djangoapps/content_libraries/api/libraries.py @@ -718,7 +718,7 @@ def library_component_usage_key( return LibraryUsageLocatorV2( # type: ignore[abstract] library_key, block_type=component.component_type.name, - usage_id=component.local_key, + usage_id=component.component_code, ) diff --git a/openedx/core/djangoapps/content_libraries/library_context.py b/openedx/core/djangoapps/content_libraries/library_context.py index bd91a3f89250..5d2c25c4286e 100644 --- a/openedx/core/djangoapps/content_libraries/library_context.py +++ b/openedx/core/djangoapps/content_libraries/library_context.py @@ -95,11 +95,11 @@ def block_exists(self, usage_key: LibraryUsageLocatorV2): if learning_package is None: return False - return content_api.component_exists_by_key( + return content_api.component_exists_by_code( learning_package.id, namespace='xblock.v1', type_name=usage_key.block_type, - local_key=usage_key.block_id, + component_code=usage_key.block_id, ) def send_block_updated_event(self, usage_key: UsageKeyV2): diff --git a/openedx/core/djangoapps/xblock/api.py b/openedx/core/djangoapps/xblock/api.py index 211b14871e7f..cc684e72b13f 100644 --- a/openedx/core/djangoapps/xblock/api.py +++ b/openedx/core/djangoapps/xblock/api.py @@ -202,11 +202,11 @@ def get_component_from_usage_key(usage_key: UsageKeyV2) -> Component: learning_package = content_api.get_learning_package_by_ref( str(usage_key.context_key) ) - return content_api.get_component_by_key( + return content_api.get_component_by_code( learning_package.id, namespace='xblock.v1', type_name=usage_key.block_type, - local_key=usage_key.block_id, + component_code=usage_key.block_id, ) diff --git a/openedx/core/djangoapps/xblock/runtime/openedx_content_runtime.py b/openedx/core/djangoapps/xblock/runtime/openedx_content_runtime.py index 65de9c77332b..53d2267e92a4 100644 --- a/openedx/core/djangoapps/xblock/runtime/openedx_content_runtime.py +++ b/openedx/core/djangoapps/xblock/runtime/openedx_content_runtime.py @@ -328,11 +328,11 @@ def _get_component_from_usage_key(self, usage_key): """ learning_package = content_api.get_learning_package_by_ref(str(usage_key.lib_key)) try: - component = content_api.get_component_by_key( + component = content_api.get_component_by_code( learning_package.id, namespace='xblock.v1', type_name=usage_key.block_type, - local_key=usage_key.block_id, + component_code=usage_key.block_id, ) except ObjectDoesNotExist as exc: raise NoSuchUsage(usage_key) from exc From 1b0989e102fdafeba8f277b05618ebdabd1dcf25 Mon Sep 17 00:00:00 2001 From: Kyle D McCormick Date: Tue, 21 Apr 2026 14:18:06 -0400 Subject: [PATCH 06/15] refactor: Rename Container key and ComponentVersionMedia.key for openedx-core 0.43.0 Switches get_container_by_key to get_container_by_code and renames the container creation kwarg from key to container_code for both create_container_and_version and its platform wrapper. Updates ComponentVersionMedia accesses: the model field .key becomes .path, and create_component_version_media's key kwarg becomes path. Co-Authored-By: Claude Opus 4.7 (1M context) --- cms/djangoapps/modulestore_migrator/tasks.py | 2 +- .../core/djangoapps/content_libraries/api/blocks.py | 10 +++++----- .../djangoapps/content_libraries/api/collections.py | 4 ++-- .../content_libraries/api/container_metadata.py | 2 +- .../djangoapps/content_libraries/api/containers.py | 2 +- .../djangoapps/content_libraries/rest_api/blocks.py | 2 +- .../xblock/runtime/openedx_content_runtime.py | 6 +++--- 7 files changed, 14 insertions(+), 14 deletions(-) diff --git a/cms/djangoapps/modulestore_migrator/tasks.py b/cms/djangoapps/modulestore_migrator/tasks.py index 95048b92feb2..701a638b909b 100644 --- a/cms/djangoapps/modulestore_migrator/tasks.py +++ b/cms/djangoapps/modulestore_migrator/tasks.py @@ -971,7 +971,7 @@ def _migrate_component( continue new_path = f"static/{filename}" content_api.create_component_version_media( - component_version.pk, media_pk, key=new_path + component_version.pk, media_pk, path=new_path ) # Publish the component diff --git a/openedx/core/djangoapps/content_libraries/api/blocks.py b/openedx/core/djangoapps/content_libraries/api/blocks.py index f2f415dfa2e0..0476bd8829ac 100644 --- a/openedx/core/djangoapps/content_libraries/api/blocks.py +++ b/openedx/core/djangoapps/content_libraries/api/blocks.py @@ -490,7 +490,7 @@ def _import_staged_block( content_api.create_component_version_media( component_version.pk, content.id, - key=filename, + path=filename, ) # Emit library block created event @@ -852,7 +852,7 @@ def get_library_block_static_asset_files(usage_key: LibraryUsageLocatorV2) -> li component_version .componentversionmedia_set .filter(media__has_file=True) - .order_by('key') + .order_by('path') .select_related('media') ) @@ -860,13 +860,13 @@ def get_library_block_static_asset_files(usage_key: LibraryUsageLocatorV2) -> li return [ LibraryXBlockStaticFile( - path=cvm.key, + path=cvm.path, size=cvm.media.size, url=site_root_url + reverse( 'content_libraries:library-assets', kwargs={ 'component_version_uuid': component_version.uuid, - 'asset_path': cvm.key, + 'asset_path': cvm.path, } ), ) @@ -1061,7 +1061,7 @@ def _create_component_for_block( content_api.create_component_version_media( component_version.pk, content.id, - key="block.xml", + path="block.xml", ) return component_version diff --git a/openedx/core/djangoapps/content_libraries/api/collections.py b/openedx/core/djangoapps/content_libraries/api/collections.py index 6ab2a98c563b..e2efc5d250fa 100644 --- a/openedx/core/djangoapps/content_libraries/api/collections.py +++ b/openedx/core/djangoapps/content_libraries/api/collections.py @@ -132,9 +132,9 @@ def update_library_collection_items( for opaque_key in opaque_keys: if isinstance(opaque_key, LibraryContainerLocator): try: - container = content_api.get_container_by_key( + container = content_api.get_container_by_code( content_library.learning_package_id, - key=opaque_key.container_id, + container_code=opaque_key.container_id, ) except Collection.DoesNotExist as exc: raise ContentLibraryContainerNotFound(opaque_key) from exc diff --git a/openedx/core/djangoapps/content_libraries/api/container_metadata.py b/openedx/core/djangoapps/content_libraries/api/container_metadata.py index ead5c6f051d6..a0a73deea6cd 100644 --- a/openedx/core/djangoapps/content_libraries/api/container_metadata.py +++ b/openedx/core/djangoapps/content_libraries/api/container_metadata.py @@ -380,7 +380,7 @@ def get_container_from_key(container_key: LibraryContainerLocator, include_delet content_library = ContentLibrary.objects.get_by_key(container_key.lib_key) learning_package = content_library.learning_package assert learning_package is not None - container = content_api.get_container_by_key(learning_package.id, key=container_key.container_id) + container = content_api.get_container_by_code(learning_package.id, container_code=container_key.container_id) assert content_api.get_container_type_code_of(container) in LIBRARY_ALLOWED_CONTAINER_TYPES # We only return the container if it exists and either: # 1. the container has a draft version (which means it is not soft-deleted) OR diff --git a/openedx/core/djangoapps/content_libraries/api/containers.py b/openedx/core/djangoapps/content_libraries/api/containers.py index 93d42b282717..e33ef2fa373e 100644 --- a/openedx/core/djangoapps/content_libraries/api/containers.py +++ b/openedx/core/djangoapps/content_libraries/api/containers.py @@ -121,7 +121,7 @@ def create_container( # Then try creating the actual container: container, _initial_version = content_api.create_container_and_version( content_library.learning_package_id, - key=slug, + container_code=slug, title=title, container_cls=container_cls, entities=[], diff --git a/openedx/core/djangoapps/content_libraries/rest_api/blocks.py b/openedx/core/djangoapps/content_libraries/rest_api/blocks.py index 81fc6bd2f1b6..8dd70a069edb 100644 --- a/openedx/core/djangoapps/content_libraries/rest_api/blocks.py +++ b/openedx/core/djangoapps/content_libraries/rest_api/blocks.py @@ -402,7 +402,7 @@ def get_component_version_asset(request, component_version_uuid, asset_path): return redirect_response # If we got here, we know that the asset exists and it's okay to download. - cv_media = component_version.componentversionmedia_set.get(key=asset_path) + cv_media = component_version.componentversionmedia_set.get(path=asset_path) media = cv_media.media # Delete the re-direct part of the response headers. We'll copy the rest. diff --git a/openedx/core/djangoapps/xblock/runtime/openedx_content_runtime.py b/openedx/core/djangoapps/xblock/runtime/openedx_content_runtime.py index 53d2267e92a4..92caecccd32b 100644 --- a/openedx/core/djangoapps/xblock/runtime/openedx_content_runtime.py +++ b/openedx/core/djangoapps/xblock/runtime/openedx_content_runtime.py @@ -251,13 +251,13 @@ def get_block_assets(self, block, fetch_asset_data): .componentversionmedia_set .filter(media__has_file=True) .select_related('media') - .order_by('key') + .order_by('path') ) return [ StaticFile( - name=cvm.key, - url=self._absolute_url_for_asset(component_version, cvm.key), + name=cvm.path, + url=self._absolute_url_for_asset(component_version, cvm.path), data=cvm.media.read_file().read() if fetch_asset_data else None, ) for cvm in cvm_list From dd83c3d0417bc17e992a88e62b5c50a233e94785 Mon Sep 17 00:00:00 2001 From: Kyle D McCormick Date: Tue, 21 Apr 2026 14:22:03 -0400 Subject: [PATCH 07/15] refactor: Rename Collection.key and update backup/restore for openedx-core 0.43.0 Renames all Collection APIs that took key/collection_key to use collection_code: create_collection, update_collection, delete_collection, restore_collection, add_to_collection, etc. Switches Collection.key attribute reads to .collection_code across tests, signal handlers, search indexers, and modulestore_migrator. Filters like target_collection__key become target_collection__collection_code. Also updates the library restore serializer to track the renamed lp_restored_data fields: archive_org_key -> archive_org_code, archive_slug -> archive_package_code, key -> package_ref, archive_lp_key -> archive_package_ref. The archive_org_code and archive_package_code fields now allow None, since openedx-core no longer raises ValueError when the archive_package_ref cannot be parsed as {prefix}:{org_code}:{package_code}. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../rest_api/v1/views/tests/test_home.py | 2 +- .../modulestore_migrator/api/read_api.py | 6 +-- cms/djangoapps/modulestore_migrator/tasks.py | 2 +- .../modulestore_migrator/tests/test_api.py | 10 ++-- openedx/core/djangoapps/content/search/api.py | 4 +- .../djangoapps/content/search/documents.py | 8 ++-- .../content/search/tests/test_api.py | 18 +++---- .../content/search/tests/test_documents.py | 4 +- .../content_libraries/api/blocks.py | 4 +- .../content_libraries/api/collections.py | 8 ++-- .../content_libraries/api/containers.py | 4 +- .../content_libraries/rest_api/collections.py | 2 +- .../content_libraries/rest_api/serializers.py | 10 ++-- .../content_libraries/signal_handlers.py | 6 +-- .../djangoapps/content_libraries/tasks.py | 2 +- .../content_libraries/tests/test_api.py | 48 +++++++++---------- .../tests/test_containers.py | 4 +- .../tests/test_views_collections.py | 42 ++++++++-------- 18 files changed, 93 insertions(+), 91 deletions(-) diff --git a/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_home.py b/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_home.py index bebcdfd53dc5..7ac9e8e479ee 100644 --- a/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_home.py +++ b/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_home.py @@ -274,7 +274,7 @@ def setUp(self): collection_key = "test-collection" content_api.create_collection( learning_package_id=learning_package.id, - key=collection_key, + collection_code=collection_key, title="Test Collection", created_by=self.user.id, ) diff --git a/cms/djangoapps/modulestore_migrator/api/read_api.py b/cms/djangoapps/modulestore_migrator/api/read_api.py index e62a684476f6..9b5aa0b8b8ef 100644 --- a/cms/djangoapps/modulestore_migrator/api/read_api.py +++ b/cms/djangoapps/modulestore_migrator/api/read_api.py @@ -137,7 +137,7 @@ def get_migrations( if target_key: migrations = migrations.filter(target__key=str(target_key)) if target_collection_slug: - migrations = migrations.filter(target_collection__key=target_collection_slug) + migrations = migrations.filter(target_collection__collection_code=target_collection_slug) if task_uuid: migrations = migrations.filter(task_status__uuid=task_uuid) if is_failed is not None: @@ -176,9 +176,9 @@ def _migration(m: models.ModulestoreMigration) -> ModulestoreMigration: return ModulestoreMigration( pk=m.id, source_key=m.source.key, - target_key=LibraryLocatorV2.from_string(m.target.key), + target_key=LibraryLocatorV2.from_string(m.target.package_ref), target_title=m.target.title, - target_collection_slug=(m.target_collection.key if m.target_collection else None), + target_collection_slug=(m.target_collection.collection_code if m.target_collection else None), target_collection_title=(m.target_collection.title if m.target_collection else None), is_failed=m.is_failed, task_uuid=m.task_status.uuid, diff --git a/cms/djangoapps/modulestore_migrator/tasks.py b/cms/djangoapps/modulestore_migrator/tasks.py index 701a638b909b..c878d75d2c6c 100644 --- a/cms/djangoapps/modulestore_migrator/tasks.py +++ b/cms/djangoapps/modulestore_migrator/tasks.py @@ -409,7 +409,7 @@ def _populate_collection(user_id: int, migration: models.ModulestoreMigration) - if block_target_pks: content_api.add_to_collection( learning_package_id=migration.target.pk, - key=migration.target_collection.key, + collection_code=migration.target_collection.collection_code, entities_qset=PublishableEntity.objects.filter(id__in=block_target_pks), created_by=user_id, ) diff --git a/cms/djangoapps/modulestore_migrator/tests/test_api.py b/cms/djangoapps/modulestore_migrator/tests/test_api.py index 311d2b5b69ea..7e88e031a1ad 100644 --- a/cms/djangoapps/modulestore_migrator/tests/test_api.py +++ b/cms/djangoapps/modulestore_migrator/tests/test_api.py @@ -232,7 +232,7 @@ def test_start_migration_to_library_with_collection(self): collection_key = "test-collection" content_api.create_collection( learning_package_id=self.learning_package.id, - key=collection_key, + collection_code=collection_key, title="Test Collection", created_by=user.id, ) @@ -249,7 +249,7 @@ def test_start_migration_to_library_with_collection(self): ) modulestoremigration = ModulestoreMigration.objects.get() - assert modulestoremigration.target_collection.key == collection_key + assert modulestoremigration.target_collection.collection_code == collection_key def test_start_migration_to_library_with_strategy_skip(self): """ @@ -487,19 +487,19 @@ def test_migration_api_for_various_scenarios(self): # Lib 2 has Collection C content_api.create_collection( learning_package_id=self.learning_package.id, - key="test-collection-1a", + collection_code="test-collection-1a", title="Test Collection A in Lib 1", created_by=user.id, ) content_api.create_collection( learning_package_id=self.learning_package.id, - key="test-collection-1b", + collection_code="test-collection-1b", title="Test Collection B in Lib 1", created_by=user.id, ) content_api.create_collection( learning_package_id=self.learning_package_2.id, - key="test-collection-2c", + collection_code="test-collection-2c", title="Test Collection C in Lib 2", created_by=user.id, ) diff --git a/openedx/core/djangoapps/content/search/api.py b/openedx/core/djangoapps/content/search/api.py index 5c3c9e16b975..2d044cd4210d 100644 --- a/openedx/core/djangoapps/content/search/api.py +++ b/openedx/core/djangoapps/content/search/api.py @@ -641,7 +641,7 @@ def index_collection_batch(batch, num_done, library_key) -> int: docs = [] for collection in batch: try: - collection_key = lib_api.library_collection_locator(library_key, collection.key) + collection_key = lib_api.library_collection_locator(library_key, collection.collection_code) doc = searchable_doc_for_collection(collection_key, collection=collection) doc.update(searchable_doc_tags(collection_key)) docs.append(doc) @@ -1032,7 +1032,7 @@ def upsert_content_library_index_docs(library_key: LibraryLocatorV2, full_index: docs.append(doc) for collection in lib_api.get_library_collections(library_key): - collection_key = lib_api.library_collection_locator(library_key, collection.key) + collection_key = lib_api.library_collection_locator(library_key, collection.collection_code) doc = searchable_doc_for_collection(collection_key, collection=collection) docs.append(doc) diff --git a/openedx/core/djangoapps/content/search/documents.py b/openedx/core/djangoapps/content/search/documents.py index 0ee3b4839287..a98a02764bbe 100644 --- a/openedx/core/djangoapps/content/search/documents.py +++ b/openedx/core/djangoapps/content/search/documents.py @@ -33,7 +33,7 @@ class Fields: usage_key = "usage_key" type = "type" # DocType.course_block or DocType.library_block (see below) # The block_id part of the usage key for course or library blocks. - # If it's a collection, the collection.key is stored here. + # If it's a collection, the collection.collection_code is stored here. # Sometimes human-readable, sometimes a random hex ID # Is only unique within the given context_key. block_id = "block_id" @@ -64,7 +64,7 @@ class Fields: tags_level2 = "level2" tags_level3 = "level3" # Collections (dictionary) that this object belongs to. - # Similarly to tags above, we collect the collection.titles and collection.keys into hierarchical facets. + # Similarly to tags above, we collect the collection.titles and collection.collection_codes into hierarchical facets. collections = "collections" collections_display_name = "display_name" collections_key = "key" @@ -543,7 +543,7 @@ def searchable_doc_for_collection( pass if collection: - assert collection.key == collection_key.collection_id + assert collection.collection_code == collection_key.collection_id draft_num_children = content_api.filter_publishable_entities( collection.entities, @@ -558,7 +558,7 @@ def searchable_doc_for_collection( Fields.context_key: str(collection_key.context_key), Fields.org: str(collection_key.org), Fields.usage_key: str(collection_key), - Fields.block_id: collection.key, + Fields.block_id: collection.collection_code, Fields.type: DocType.collection, Fields.display_name: collection.title, Fields.description: collection.description, diff --git a/openedx/core/djangoapps/content/search/tests/test_api.py b/openedx/core/djangoapps/content/search/tests/test_api.py index 7f666ef071e8..981bb5858b80 100644 --- a/openedx/core/djangoapps/content/search/tests/test_api.py +++ b/openedx/core/djangoapps/content/search/tests/test_api.py @@ -192,7 +192,7 @@ def setUp(self) -> None: with freeze_time(self.created_date): self.collection = content_api.create_collection( learning_package_id=self.learning_package.id, - key="MYCOL", + collection_code="MYCOL", title="my_collection", created_by=None, description="my collection description" @@ -202,7 +202,7 @@ def setUp(self) -> None: ) self.collection_dict = { "id": "lib-collectionorg1libmycol-5b647617", - "block_id": self.collection.key, + "block_id": self.collection.collection_code, "usage_key": str(self.collection_key), "type": "collection", "display_name": "my_collection", @@ -722,7 +722,7 @@ def test_index_library_block_and_collections(self, mock_meilisearch) -> None: for collection in (collection2, collection1): library_api.update_library_collection_items( self.library.key, - collection_key=collection.key, + collection_key=collection.collection_code, opaque_keys=[ self.problem1.usage_key, ], @@ -904,7 +904,7 @@ def test_delete_collection(self, mock_meilisearch) -> None: with freeze_time(updated_date): library_api.update_library_collection_items( self.library.key, - collection_key=self.collection.key, + collection_key=self.collection.collection_code, opaque_keys=[ self.problem1.usage_key, self.unit.container_key @@ -918,14 +918,14 @@ def test_delete_collection(self, mock_meilisearch) -> None: "id": self.doc_problem1["id"], "collections": { "display_name": [self.collection.title], - "key": [self.collection.key], + "key": [self.collection.collection_code], }, } doc_unit_with_collection = { "id": self.unit_dict["id"], "collections": { "display_name": [self.collection.title], - "key": [self.collection.key], + "key": [self.collection.collection_code], }, } @@ -944,7 +944,7 @@ def test_delete_collection(self, mock_meilisearch) -> None: # Soft-delete the collection content_api.delete_collection( self.collection.learning_package_id, - self.collection.key, + self.collection.collection_code, ) doc_problem_without_collection = { @@ -979,7 +979,7 @@ def test_delete_collection(self, mock_meilisearch) -> None: with freeze_time(restored_date): content_api.restore_collection( self.collection.learning_package_id, - self.collection.key, + self.collection.collection_code, ) doc_collection = copy.deepcopy(self.collection_dict) @@ -1001,7 +1001,7 @@ def test_delete_collection(self, mock_meilisearch) -> None: # Hard-delete the collection content_api.delete_collection( self.collection.learning_package_id, - self.collection.key, + self.collection.collection_code, hard_delete=True, ) diff --git a/openedx/core/djangoapps/content/search/tests/test_documents.py b/openedx/core/djangoapps/content/search/tests/test_documents.py index a9aea3ab3cfb..ee4a1d613f61 100644 --- a/openedx/core/djangoapps/content/search/tests/test_documents.py +++ b/openedx/core/djangoapps/content/search/tests/test_documents.py @@ -492,7 +492,7 @@ def test_collection_with_library(self): assert doc == { "id": "lib-collectionedx2012_falltoy_collection-d1d907a4", - "block_id": self.collection.key, + "block_id": self.collection.collection_code, "usage_key": str(self.collection_key), "type": "collection", "org": "edX", @@ -521,7 +521,7 @@ def test_collection_with_published_library(self): assert doc == { "id": "lib-collectionedx2012_falltoy_collection-d1d907a4", - "block_id": self.collection.key, + "block_id": self.collection.collection_code, "usage_key": str(self.collection_key), "type": "collection", "org": "edX", diff --git a/openedx/core/djangoapps/content_libraries/api/blocks.py b/openedx/core/djangoapps/content_libraries/api/blocks.py index 0476bd8829ac..63327280d021 100644 --- a/openedx/core/djangoapps/content_libraries/api/blocks.py +++ b/openedx/core/djangoapps/content_libraries/api/blocks.py @@ -743,7 +743,7 @@ def send_block_deleted_signal(): library_collection=LibraryCollectionData( collection_key=library_collection_locator( library_key=library_key, - collection_key=collection.key, + collection_key=collection.collection_code, ), background=True, ) @@ -809,7 +809,7 @@ def restore_library_block(usage_key: LibraryUsageLocatorV2, user_id: int | None library_collection=LibraryCollectionData( collection_key=library_collection_locator( library_key=library_key, - collection_key=collection.key, + collection_key=collection.collection_code, ), background=True, ) diff --git a/openedx/core/djangoapps/content_libraries/api/collections.py b/openedx/core/djangoapps/content_libraries/api/collections.py index e2efc5d250fa..41cf425c111c 100644 --- a/openedx/core/djangoapps/content_libraries/api/collections.py +++ b/openedx/core/djangoapps/content_libraries/api/collections.py @@ -54,7 +54,7 @@ def create_library_collection( try: collection = content_api.create_collection( learning_package_id=content_library.learning_package_id, - key=collection_key, + collection_code=collection_key, title=title, description=description, created_by=created_by, @@ -86,7 +86,7 @@ def update_library_collection( try: collection = content_api.update_collection( learning_package_id=content_library.learning_package_id, - key=collection_key, + collection_code=collection_key, title=title, description=description, ) @@ -214,7 +214,7 @@ def set_library_item_collections( # Note: Component.entity_ref matches its PublishableEntity.entity_ref collection_qs = content_api.get_collections(content_library.learning_package_id).filter( - key__in=collection_keys + collection_code__in=collection_keys ) affected_collections = content_api.set_collections( @@ -232,7 +232,7 @@ def set_library_item_collections( library_collection=LibraryCollectionData( collection_key=library_collection_locator( library_key=library_key, - collection_key=collection.key, + collection_key=collection.collection_code, ), background=True, ) diff --git a/openedx/core/djangoapps/content_libraries/api/containers.py b/openedx/core/djangoapps/content_libraries/api/containers.py index e33ef2fa373e..d357cdbe78f7 100644 --- a/openedx/core/djangoapps/content_libraries/api/containers.py +++ b/openedx/core/djangoapps/content_libraries/api/containers.py @@ -249,7 +249,7 @@ def send_container_deleted_signal(): library_collection=LibraryCollectionData( collection_key=library_collection_locator( library_key=library_key, - collection_key=collection.key, + collection_key=collection.collection_code, ), background=True, ) @@ -333,7 +333,7 @@ def restore_container(container_key: LibraryContainerLocator) -> None: library_collection=LibraryCollectionData( collection_key=library_collection_locator( library_key=library_key, - collection_key=collection.key, + collection_key=collection.collection_code, ), ) ) diff --git a/openedx/core/djangoapps/content_libraries/rest_api/collections.py b/openedx/core/djangoapps/content_libraries/rest_api/collections.py index 3f67f5e777a8..4ccbae2ba36b 100644 --- a/openedx/core/djangoapps/content_libraries/rest_api/collections.py +++ b/openedx/core/djangoapps/content_libraries/rest_api/collections.py @@ -184,7 +184,7 @@ def destroy(self, request: RestRequest, *args, **kwargs) -> Response: assert collection.learning_package_id content_api.delete_collection( collection.learning_package_id, - collection.key, + collection.collection_code, hard_delete=False, ) return Response(None, status=HTTP_204_NO_CONTENT) diff --git a/openedx/core/djangoapps/content_libraries/rest_api/serializers.py b/openedx/core/djangoapps/content_libraries/rest_api/serializers.py index 72c59f695833..b30906d58d89 100644 --- a/openedx/core/djangoapps/content_libraries/rest_api/serializers.py +++ b/openedx/core/djangoapps/content_libraries/rest_api/serializers.py @@ -447,14 +447,16 @@ class RestoreSuccessDataSerializer(serializers.Serializer): """ learning_package_id = serializers.IntegerField(source="lp_restored_data.id") title = serializers.CharField(source="lp_restored_data.title") - org = serializers.CharField(source="lp_restored_data.archive_org_key") - slug = serializers.CharField(source="lp_restored_data.archive_slug") + # archive_org_code and archive_package_code may be None when archive_package_ref cannot be parsed + # as {prefix}:{org_code}:{package_code} (previously this raised ValueError in openedx-core). + org = serializers.CharField(source="lp_restored_data.archive_org_code", allow_null=True) + slug = serializers.CharField(source="lp_restored_data.archive_package_code", allow_null=True) # The `key` is a unique temporary key assigned to the learning package during the restore process, # whereas the `archive_key` is the original key of the learning package from the backup. # The temporary learning package key is replaced with a standard key once it is added to a content library. - key = serializers.CharField(source="lp_restored_data.key") - archive_key = serializers.CharField(source="lp_restored_data.archive_lp_key") + key = serializers.CharField(source="lp_restored_data.package_ref") + archive_key = serializers.CharField(source="lp_restored_data.archive_package_ref") containers = serializers.IntegerField(source="lp_restored_data.num_containers") components = serializers.IntegerField(source="lp_restored_data.num_components") diff --git a/openedx/core/djangoapps/content_libraries/signal_handlers.py b/openedx/core/djangoapps/content_libraries/signal_handlers.py index 041a49b473e0..fada1cb2f874 100644 --- a/openedx/core/djangoapps/content_libraries/signal_handlers.py +++ b/openedx/core/djangoapps/content_libraries/signal_handlers.py @@ -43,7 +43,7 @@ def library_collection_saved(sender, instance, created, **kwargs): library_collection=LibraryCollectionData( collection_key=library_collection_locator( library_key=library.library_key, - collection_key=instance.key, + collection_key=instance.collection_code, ), ) ) @@ -54,7 +54,7 @@ def library_collection_saved(sender, instance, created, **kwargs): library_collection=LibraryCollectionData( collection_key=library_collection_locator( library_key=library.library_key, - collection_key=instance.key, + collection_key=instance.collection_code, ), ) ) @@ -77,7 +77,7 @@ def library_collection_deleted(sender, instance, **kwargs): library_collection=LibraryCollectionData( collection_key=library_collection_locator( library_key=library.library_key, - collection_key=instance.key, + collection_key=instance.collection_code, ), ) ) diff --git a/openedx/core/djangoapps/content_libraries/tasks.py b/openedx/core/djangoapps/content_libraries/tasks.py index 378ee9a19161..2fe28c7476a8 100644 --- a/openedx/core/djangoapps/content_libraries/tasks.py +++ b/openedx/core/djangoapps/content_libraries/tasks.py @@ -651,7 +651,7 @@ def restore_library(self, user_id, storage_path): TASK_LOGGER.info( 'Restored learning package (id: %s) with key %s', learning_package_data.get('id'), - learning_package_data.get('key') + learning_package_data.get('package_ref') ) # Save the restore details as an artifact in JSON format diff --git a/openedx/core/djangoapps/content_libraries/tests/test_api.py b/openedx/core/djangoapps/content_libraries/tests/test_api.py index c62cf8eda861..7d2e84c10280 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_api.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_api.py @@ -115,7 +115,7 @@ def test_create_library_collection(self) -> None: description="Description for Collection 4", created_by=self.user.id, ) - assert collection.key == "COL4" + assert collection.collection_code == "COL4" assert collection.title == "Collection 4" assert collection.description == "Description for Collection 4" assert collection.created_by == self.user @@ -150,10 +150,10 @@ def test_update_library_collection(self) -> None: self.col1 = api.update_library_collection( self.lib1.library_key, - self.col1.key, + self.col1.collection_code, title="New title for Collection 1", ) - assert self.col1.key == "COL1" + assert self.col1.collection_code == "COL1" assert self.col1.title == "New title for Collection 1" assert self.col1.description == "Description for Collection 1" assert self.col1.created_by == self.user @@ -177,7 +177,7 @@ def test_update_library_collection_wrong_library(self) -> None: with self.assertRaises(api.ContentLibraryCollectionNotFound) as exc: # noqa: F841, PT027 api.update_library_collection( self.lib1.library_key, - self.col2.key, + self.col2.collection_code, ) def test_delete_library_collection(self) -> None: @@ -187,7 +187,7 @@ def test_delete_library_collection(self) -> None: assert self.lib1.learning_package_id is not None content_api.delete_collection( self.lib1.learning_package_id, - self.col1.key, + self.col1.collection_code, hard_delete=True, ) @@ -211,7 +211,7 @@ def test_update_library_collection_items(self) -> None: self.col1 = api.update_library_collection_items( self.lib1.library_key, - self.col1.key, + self.col1.collection_code, opaque_keys=[ LibraryUsageLocatorV2.from_string(self.lib1_problem_block["id"]), LibraryUsageLocatorV2.from_string(self.lib1_html_block["id"]), @@ -222,7 +222,7 @@ def test_update_library_collection_items(self) -> None: self.col1 = api.update_library_collection_items( self.lib1.library_key, - self.col1.key, + self.col1.collection_code, opaque_keys=[ LibraryUsageLocatorV2.from_string(self.lib1_html_block["id"]), ], @@ -240,7 +240,7 @@ def test_update_library_collection_components_event(self) -> None: api.update_library_collection_items( self.lib1.library_key, - self.col1.key, + self.col1.collection_code, opaque_keys=[ LibraryUsageLocatorV2.from_string(self.lib1_problem_block["id"]), LibraryUsageLocatorV2.from_string(self.lib1_html_block["id"]), @@ -300,7 +300,7 @@ def test_update_collection_components_from_wrong_library(self) -> None: with self.assertRaises(api.ContentLibraryBlockNotFound) as exc: # noqa: PT027 api.update_library_collection_items( self.lib2.library_key, - self.col2.key, + self.col2.collection_code, opaque_keys=[ LibraryUsageLocatorV2.from_string(self.lib1_problem_block["id"]), LibraryUsageLocatorV2.from_string(self.lib1_html_block["id"]), @@ -319,12 +319,12 @@ def test_set_library_component_collections(self) -> None: api.set_library_item_collections( library_key=self.lib2.library_key, entity_ref=component.publishable_entity.entity_ref, - collection_keys=[self.col2.key, self.col3.key], + collection_keys=[self.col2.collection_code, self.col3.collection_code], ) assert self.lib2.learning_package_id is not None - assert len(content_api.get_collection(self.lib2.learning_package_id, self.col2.key).entities.all()) == 1 - assert len(content_api.get_collection(self.lib2.learning_package_id, self.col3.key).entities.all()) == 1 + assert len(content_api.get_collection(self.lib2.learning_package_id, self.col2.collection_code).entities.all()) == 1 + assert len(content_api.get_collection(self.lib2.learning_package_id, self.col3.collection_code).entities.all()) == 1 self.assertDictContainsEntries( event_receiver.call_args_list[0].kwargs, @@ -343,11 +343,11 @@ def test_set_library_component_collections(self) -> None: assert all(event["signal"] == LIBRARY_COLLECTION_UPDATED for event in collection_update_events) assert {event["library_collection"] for event in collection_update_events} == { LibraryCollectionData( - collection_key=api.library_collection_locator(self.lib2.library_key, collection_key=self.col2.key), + collection_key=api.library_collection_locator(self.lib2.library_key, collection_key=self.col2.collection_code), background=True, ), LibraryCollectionData( - collection_key=api.library_collection_locator(self.lib2.library_key, collection_key=self.col3.key), + collection_key=api.library_collection_locator(self.lib2.library_key, collection_key=self.col3.collection_code), background=True, ) } @@ -355,7 +355,7 @@ def test_set_library_component_collections(self) -> None: def test_delete_library_block(self) -> None: api.update_library_collection_items( self.lib1.library_key, - self.col1.key, + self.col1.collection_code, opaque_keys=[ LibraryUsageLocatorV2.from_string(self.lib1_problem_block["id"]), LibraryUsageLocatorV2.from_string(self.lib1_html_block["id"]), @@ -376,7 +376,7 @@ def test_delete_library_block(self) -> None: "library_collection": LibraryCollectionData( collection_key=api.library_collection_locator( self.lib1.library_key, - collection_key=self.col1.key, + collection_key=self.col1.collection_code, ), background=True, ), @@ -386,7 +386,7 @@ def test_delete_library_block(self) -> None: def test_delete_library_container(self) -> None: api.update_library_collection_items( self.lib1.library_key, - self.col1.key, + self.col1.collection_code, opaque_keys=[ LibraryUsageLocatorV2.from_string(self.lib1_problem_block["id"]), LibraryUsageLocatorV2.from_string(self.lib1_html_block["id"]), @@ -415,7 +415,7 @@ def test_delete_library_container(self) -> None: "library_collection": LibraryCollectionData( collection_key=api.library_collection_locator( self.lib1.library_key, - collection_key=self.col1.key, + collection_key=self.col1.collection_code, ), background=True, ), @@ -499,7 +499,7 @@ def test_delete_library_block_when_component_does_not_exist(self) -> None: def test_restore_library_block(self) -> None: api.update_library_collection_items( self.lib1.library_key, - self.col1.key, + self.col1.collection_code, opaque_keys=[ LibraryUsageLocatorV2.from_string(self.lib1_problem_block["id"]), LibraryUsageLocatorV2.from_string(self.lib1_html_block["id"]), @@ -520,7 +520,7 @@ def test_restore_library_block(self) -> None: "library_collection": LibraryCollectionData( collection_key=api.library_collection_locator( self.lib1.library_key, - collection_key=self.col1.key, + collection_key=self.col1.collection_code, ), background=True, ), @@ -539,7 +539,7 @@ def test_add_component_and_revert(self) -> None: # Add component. Note: collections are not part of the draft/publish cycle so this is not a draft change. api.update_library_collection_items( self.lib1.library_key, - self.col1.key, + self.col1.collection_code, opaque_keys=[ LibraryUsageLocatorV2.from_string(self.lib1_html_block["id"]), LibraryUsageLocatorV2.from_string(new_problem_block["id"]), @@ -560,7 +560,7 @@ def test_add_component_and_revert(self) -> None: "library_collection": LibraryCollectionData( collection_key=api.library_collection_locator( self.lib1.library_key, - collection_key=self.col1.key, + collection_key=self.col1.collection_code, ), ), }, @@ -574,7 +574,7 @@ def test_delete_component_and_revert(self) -> None: # Add components and publish api.update_library_collection_items( self.lib1.library_key, - self.col1.key, + self.col1.collection_code, opaque_keys=[ LibraryUsageLocatorV2.from_string(self.lib1_problem_block["id"]), LibraryUsageLocatorV2.from_string(self.lib1_html_block["id"]) @@ -599,7 +599,7 @@ def test_delete_component_and_revert(self) -> None: "library_collection": LibraryCollectionData( collection_key=api.library_collection_locator( self.lib1.library_key, - collection_key=self.col1.key, + collection_key=self.col1.collection_code, ), ), }, diff --git a/openedx/core/djangoapps/content_libraries/tests/test_containers.py b/openedx/core/djangoapps/content_libraries/tests/test_containers.py index 37ad621d26e6..a95b238b78a4 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_containers.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_containers.py @@ -591,7 +591,7 @@ def test_unit_collections(self) -> None: result = self._patch_container_collections( self.unit["id"], - collection_keys=[col1.key], + collection_keys=[col1.collection_code], ) assert result['count'] == 1 @@ -600,7 +600,7 @@ def test_unit_collections(self) -> None: unit_as_read = self._get_container(self.unit["id"]) # Verify the collections - assert unit_as_read['collections'] == [{"title": col1.title, "key": col1.key}] + assert unit_as_read['collections'] == [{"title": col1.title, "key": col1.collection_code}] def test_section_hierarchy(self): with self.assertNumQueries(126): diff --git a/openedx/core/djangoapps/content_libraries/tests/test_views_collections.py b/openedx/core/djangoapps/content_libraries/tests/test_views_collections.py index 270580cb2a61..a25b18dc3777 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_views_collections.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_views_collections.py @@ -88,7 +88,7 @@ def test_get_library_collection(self): Test retrieving a Content Library Collection """ resp = self.client.get( - URL_LIB_COLLECTION.format(lib_key=self.lib2.library_key, collection_key=self.col3.key) + URL_LIB_COLLECTION.format(lib_key=self.lib2.library_key, collection_key=self.col3.collection_code) ) # Check that correct Content Library Collection data retrieved @@ -103,7 +103,7 @@ def test_get_library_collection(self): random_user = UserFactory.create(username="Random", email="random@example.com") with self.as_user(random_user): resp = self.client.get( - URL_LIB_COLLECTION.format(lib_key=self.lib2.library_key, collection_key=self.col3.key) + URL_LIB_COLLECTION.format(lib_key=self.lib2.library_key, collection_key=self.col3.collection_code) ) assert resp.status_code == 403 @@ -113,7 +113,7 @@ def test_get_invalid_library_collection(self): """ # Fetch collection that belongs to a different library, it should fail resp = self.client.get( - URL_LIB_COLLECTION.format(lib_key=self.lib1.library_key, collection_key=self.col3.key) + URL_LIB_COLLECTION.format(lib_key=self.lib1.library_key, collection_key=self.col3.collection_code) ) assert resp.status_code == 404 @@ -249,7 +249,7 @@ def test_create_invalid_library_collection(self): # Create collection with an existing collection.key; it should fail post_data_existing_key = { - "key": self.col1.key, + "key": self.col1.collection_code, "title": "Collection 4", } resp = self.client.post( @@ -275,7 +275,7 @@ def test_update_library_collection(self): "title": "Collection 3 Updated", } resp = self.client.patch( - URL_LIB_COLLECTION.format(lib_key=self.lib2.library_key, collection_key=self.col3.key), + URL_LIB_COLLECTION.format(lib_key=self.lib2.library_key, collection_key=self.col3.collection_code), patch_data, format="json" ) @@ -297,7 +297,7 @@ def test_update_library_collection(self): "title": "Collection 3 should not update", } resp = self.client.patch( - URL_LIB_COLLECTION.format(lib_key=self.lib2.library_key, collection_key=self.col3.key), + URL_LIB_COLLECTION.format(lib_key=self.lib2.library_key, collection_key=self.col3.collection_code), patch_data, format="json" ) @@ -313,7 +313,7 @@ def test_update_invalid_library_collection(self): } # Update collection that belongs to a different library, it should fail resp = self.client.patch( - URL_LIB_COLLECTION.format(lib_key=self.lib1.library_key, collection_key=self.col3.key), + URL_LIB_COLLECTION.format(lib_key=self.lib1.library_key, collection_key=self.col3.collection_code), patch_data, format="json" ) @@ -331,7 +331,7 @@ def test_update_invalid_library_collection(self): # Update collection with invalid library_key provided, it should fail resp = self.client.patch( - URL_LIB_COLLECTION.format(lib_key=123, collection_key=self.col3.key), + URL_LIB_COLLECTION.format(lib_key=123, collection_key=self.col3.collection_code), patch_data, format="json" ) @@ -342,22 +342,22 @@ def test_delete_library_collection(self): Test soft-deleting and restoring a Content Library Collection """ resp = self.client.delete( - URL_LIB_COLLECTION.format(lib_key=self.lib2.library_key, collection_key=self.col3.key) + URL_LIB_COLLECTION.format(lib_key=self.lib2.library_key, collection_key=self.col3.collection_code) ) assert resp.status_code == 204 resp = self.client.get( - URL_LIB_COLLECTION.format(lib_key=self.lib2.library_key, collection_key=self.col3.key) + URL_LIB_COLLECTION.format(lib_key=self.lib2.library_key, collection_key=self.col3.collection_code) ) assert resp.status_code == 404 resp = self.client.post( - URL_LIB_COLLECTION_RESTORE.format(lib_key=self.lib2.library_key, collection_key=self.col3.key) + URL_LIB_COLLECTION_RESTORE.format(lib_key=self.lib2.library_key, collection_key=self.col3.collection_code) ) assert resp.status_code == 204 resp = self.client.get( - URL_LIB_COLLECTION.format(lib_key=self.lib2.library_key, collection_key=self.col3.key) + URL_LIB_COLLECTION.format(lib_key=self.lib2.library_key, collection_key=self.col3.collection_code) ) # Check that correct Content Library Collection data retrieved expected_collection = { @@ -375,7 +375,7 @@ def test_get_components(self): resp = self.client.get( URL_LIB_COLLECTION_COMPONENTS.format( lib_key=self.lib1.library_key, - collection_key=self.col1.key, + collection_key=self.col1.collection_code, ), ) assert resp.status_code == 405 @@ -388,7 +388,7 @@ def test_update_components(self): resp = self.client.patch( URL_LIB_COLLECTION_COMPONENTS.format( lib_key=self.lib1.library_key, - collection_key=self.col1.key, + collection_key=self.col1.collection_code, ), data={ "usage_keys": [ @@ -404,7 +404,7 @@ def test_update_components(self): resp = self.client.delete( URL_LIB_COLLECTION_COMPONENTS.format( lib_key=self.lib1.library_key, - collection_key=self.col1.key, + collection_key=self.col1.collection_code, ), data={ "usage_keys": [ @@ -423,7 +423,7 @@ def test_update_containers(self): resp = self.client.patch( URL_LIB_COLLECTION_COMPONENTS.format( lib_key=self.lib1.library_key, - collection_key=self.col1.key, + collection_key=self.col1.collection_code, ), data={ "usage_keys": [ @@ -440,7 +440,7 @@ def test_update_containers(self): resp = self.client.delete( URL_LIB_COLLECTION_COMPONENTS.format( lib_key=self.lib1.library_key, - collection_key=self.col1.key, + collection_key=self.col1.collection_code, ), data={ "usage_keys": [ @@ -460,7 +460,7 @@ def test_update_components_wrong_collection(self, method): resp = getattr(self.client, method)( URL_LIB_COLLECTION_COMPONENTS.format( lib_key=self.lib2.library_key, - collection_key=self.col1.key, + collection_key=self.col1.collection_code, ), data={ "usage_keys": [ @@ -478,7 +478,7 @@ def test_update_components_missing_data(self, method): resp = getattr(self.client, method)( URL_LIB_COLLECTION_COMPONENTS.format( lib_key=self.lib2.library_key, - collection_key=self.col3.key, + collection_key=self.col3.collection_code, ), ) assert resp.status_code == 400 @@ -494,7 +494,7 @@ def test_update_components_from_another_library(self, method): resp = getattr(self.client, method)( URL_LIB_COLLECTION_COMPONENTS.format( lib_key=self.lib2.library_key, - collection_key=self.col3.key, + collection_key=self.col3.collection_code, ), data={ "usage_keys": [ @@ -515,7 +515,7 @@ def test_update_components_permissions(self, method): resp = getattr(self.client, method)( URL_LIB_COLLECTION_COMPONENTS.format( lib_key=self.lib1.library_key, - collection_key=self.col1.key, + collection_key=self.col1.collection_code, ), ) assert resp.status_code == 403 From 5778155cdf077dbb803d7ce59c100998c4b07c05 Mon Sep 17 00:00:00 2001 From: Kyle D McCormick Date: Tue, 21 Apr 2026 20:25:09 -0400 Subject: [PATCH 08/15] fix: various fixes to Claude's output... fix: build locator with container_code fix: pylint and mypy fix: queries for search index fix: some missed cvm.key -> cvm.path fix: undo breaking library changes fix: openedx-core no longer raises integrityerror on conflict fix: misses in modulestore_migrator fix: search tests docs: Improve collection_code/key TODO comment --- .../modulestore_migrator/api/read_api.py | 2 +- .../rest_api/v1/serializers.py | 4 +++ .../modulestore_migrator/rest_api/v1/views.py | 2 +- cms/djangoapps/modulestore_migrator/tasks.py | 2 +- .../modulestore_migrator/tests/test_tasks.py | 10 ++++---- .../djangoapps/content/search/documents.py | 9 +++++-- .../content/search/tests/test_api.py | 16 ++++++------ .../content_libraries/api/blocks.py | 7 ++++-- .../content_libraries/api/collections.py | 25 +++++++++---------- .../api/container_metadata.py | 6 ++--- .../content_libraries/api/containers.py | 6 ++++- .../content_libraries/rest_api/collections.py | 5 +++- .../content_libraries/rest_api/serializers.py | 5 +++- .../djangoapps/content_libraries/tasks.py | 12 ++++----- .../content_libraries/tests/test_api.py | 14 ++++++++--- .../tests/test_views_collections.py | 2 +- .../video_config/transcripts_utils.py | 2 +- openedx/core/djangoapps/xblock/api.py | 4 +-- .../xblock/runtime/openedx_content_runtime.py | 6 ++--- 19 files changed, 83 insertions(+), 56 deletions(-) diff --git a/cms/djangoapps/modulestore_migrator/api/read_api.py b/cms/djangoapps/modulestore_migrator/api/read_api.py index 9b5aa0b8b8ef..cd9962c4aa19 100644 --- a/cms/djangoapps/modulestore_migrator/api/read_api.py +++ b/cms/djangoapps/modulestore_migrator/api/read_api.py @@ -135,7 +135,7 @@ def get_migrations( if source_key: migrations = migrations.filter(source__key=source_key) if target_key: - migrations = migrations.filter(target__key=str(target_key)) + migrations = migrations.filter(target__package_ref=str(target_key)) if target_collection_slug: migrations = migrations.filter(target_collection__collection_code=target_collection_slug) if task_uuid: diff --git a/cms/djangoapps/modulestore_migrator/rest_api/v1/serializers.py b/cms/djangoapps/modulestore_migrator/rest_api/v1/serializers.py index 797c42d9a5b3..9dc0c5dda5d3 100644 --- a/cms/djangoapps/modulestore_migrator/rest_api/v1/serializers.py +++ b/cms/djangoapps/modulestore_migrator/rest_api/v1/serializers.py @@ -18,6 +18,10 @@ class LibraryMigrationCollectionSerializer(serializers.ModelSerializer): """ Serializer for the target collection of a library migration. """ + # Expose Collection.collection_code as "key" to preserve the REST API field name. + # This is temporary: https://github.com/openedx/openedx-platform/issues/38406 + key = serializers.CharField(source='collection_code') + class Meta: model = Collection fields = ["key", "title"] diff --git a/cms/djangoapps/modulestore_migrator/rest_api/v1/views.py b/cms/djangoapps/modulestore_migrator/rest_api/v1/views.py index 0b76c01cdaaa..594c9518a2ae 100644 --- a/cms/djangoapps/modulestore_migrator/rest_api/v1/views.py +++ b/cms/djangoapps/modulestore_migrator/rest_api/v1/views.py @@ -540,7 +540,7 @@ def get_queryset(self): self.request.user, lib_api.permissions.CAN_VIEW_THIS_CONTENT_LIBRARY ) - queryset = queryset.filter(target__key=library_key, source__key__startswith='course-v1') + queryset = queryset.filter(target__package_ref=str(library_key), source__key__startswith='course-v1') return queryset diff --git a/cms/djangoapps/modulestore_migrator/tasks.py b/cms/djangoapps/modulestore_migrator/tasks.py index c878d75d2c6c..15b25524b871 100644 --- a/cms/djangoapps/modulestore_migrator/tasks.py +++ b/cms/djangoapps/modulestore_migrator/tasks.py @@ -867,7 +867,7 @@ def _migrate_container( container_exists = False if PublishableEntity.objects.filter( learning_package_id=context.target_package_id, - key=target_key.container_id, + entity_ref=target_key.container_id, ).exists(): libraries_api.restore_container(container_key=target_key) container = libraries_api.get_container(target_key) diff --git a/cms/djangoapps/modulestore_migrator/tests/test_tasks.py b/cms/djangoapps/modulestore_migrator/tests/test_tasks.py index 76f94da16bc2..ae4ad1548937 100644 --- a/cms/djangoapps/modulestore_migrator/tests/test_tasks.py +++ b/cms/djangoapps/modulestore_migrator/tests/test_tasks.py @@ -89,12 +89,12 @@ def setUp(self): ) self.collection = Collection.objects.create( learning_package=self.learning_package, - key="test_collection", + collection_code="test_collection", title="Test Collection", ) self.collection2 = Collection.objects.create( learning_package=self.learning_package, - key="test_collection2", + collection_code="test_collection2", title="Test Collection 2", ) @@ -426,7 +426,7 @@ def test_migrate_component_with_static_content(self): self.assertIsNone(reason) # noqa: PT009 component_media = result.componentversion.componentversionmedia_set.filter( - key="static/test_image.png" + path="static/test_image.png" ).first() self.assertIsNotNone(component_media) # noqa: PT009 self.assertEqual(component_media.media.id, test_media.id) # noqa: PT009 @@ -673,12 +673,12 @@ def test_migrate_component_content_filename_not_in_olx(self): referenced_content_exists = ( result.componentversion.componentversionmedia_set.filter( - key="static/referenced.png" + path="static/referenced.png" ).exists() ) unreferenced_content_exists = ( result.componentversion.componentversionmedia_set.filter( - key="static/unreferenced.png" + path="static/unreferenced.png" ).exists() ) diff --git a/openedx/core/djangoapps/content/search/documents.py b/openedx/core/djangoapps/content/search/documents.py index a98a02764bbe..678281191eeb 100644 --- a/openedx/core/djangoapps/content/search/documents.py +++ b/openedx/core/djangoapps/content/search/documents.py @@ -7,6 +7,7 @@ from hashlib import blake2b from django.core.exceptions import ObjectDoesNotExist +from django.db.models import F from django.utils.text import slugify from opaque_keys.edx.keys import ContainerKey, LearningContextKey, OpaqueKey, UsageKey from opaque_keys.edx.locator import LibraryCollectionLocator, LibraryContainerLocator @@ -64,7 +65,8 @@ class Fields: tags_level2 = "level2" tags_level3 = "level3" # Collections (dictionary) that this object belongs to. - # Similarly to tags above, we collect the collection.titles and collection.collection_codes into hierarchical facets. + # Similarly to tags above, we collect the collection.titles and collection.collection_codes + # into hierarchical facets. collections = "collections" collections_display_name = "display_name" collections_key = "key" @@ -448,10 +450,13 @@ def searchable_doc_collections(object_id: OpaqueKey) -> dict: try: if isinstance(object_id, UsageKey): component = lib_api.get_component_from_usage_key(object_id) + # Temporarily alias collection_code to "key" so downstream consumers + # (search indexer, REST API) keep the same field name. We will update + # downstream consumers later: https://github.com/openedx/openedx-platform/issues/38406 collections = content_api.get_entity_collections( component.learning_package_id, component.entity_ref, - ).values('key', 'title') + ).values("title", key=F('collection_code')) elif isinstance(object_id, LibraryContainerLocator): container = lib_api.get_container(object_id, include_collections=True) collections = container.collections diff --git a/openedx/core/djangoapps/content/search/tests/test_api.py b/openedx/core/djangoapps/content/search/tests/test_api.py index 981bb5858b80..afa847748e89 100644 --- a/openedx/core/djangoapps/content/search/tests/test_api.py +++ b/openedx/core/djangoapps/content/search/tests/test_api.py @@ -732,8 +732,8 @@ def test_index_library_block_and_collections(self, mock_meilisearch) -> None: lib_access, _ = SearchAccess.objects.get_or_create(context_key=self.library.key) doc_collection1_created = { "id": "lib-collectionorg1libcol1-283a79c9", - "block_id": collection1.key, - "usage_key": f"lib-collection:org1:lib:{collection1.key}", + "block_id": collection1.collection_code, + "usage_key": f"lib-collection:org1:lib:{collection1.collection_code}", "type": "collection", "display_name": "Collection 1", "description": "First Collection", @@ -750,8 +750,8 @@ def test_index_library_block_and_collections(self, mock_meilisearch) -> None: } doc_collection2_created = { "id": "lib-collectionorg1libcol2-46823d4d", - "block_id": collection2.key, - "usage_key": f"lib-collection:org1:lib:{collection2.key}", + "block_id": collection2.collection_code, + "usage_key": f"lib-collection:org1:lib:{collection2.collection_code}", "type": "collection", "display_name": "Collection 2", "description": "Second Collection", @@ -768,8 +768,8 @@ def test_index_library_block_and_collections(self, mock_meilisearch) -> None: } doc_collection2_updated = { "id": "lib-collectionorg1libcol2-46823d4d", - "block_id": collection2.key, - "usage_key": f"lib-collection:org1:lib:{collection2.key}", + "block_id": collection2.collection_code, + "usage_key": f"lib-collection:org1:lib:{collection2.collection_code}", "type": "collection", "display_name": "Collection 2", "description": "Second Collection", @@ -786,8 +786,8 @@ def test_index_library_block_and_collections(self, mock_meilisearch) -> None: } doc_collection1_updated = { "id": "lib-collectionorg1libcol1-283a79c9", - "block_id": collection1.key, - "usage_key": f"lib-collection:org1:lib:{collection1.key}", + "block_id": collection1.collection_code, + "usage_key": f"lib-collection:org1:lib:{collection1.collection_code}", "type": "collection", "display_name": "Collection 1", "description": "First Collection", diff --git a/openedx/core/djangoapps/content_libraries/api/blocks.py b/openedx/core/djangoapps/content_libraries/api/blocks.py index 63327280d021..f8d45b78d5fe 100644 --- a/openedx/core/djangoapps/content_libraries/api/blocks.py +++ b/openedx/core/djangoapps/content_libraries/api/blocks.py @@ -15,7 +15,7 @@ from django.core.exceptions import ObjectDoesNotExist, ValidationError from django.core.validators import validate_unicode_slug from django.db import transaction -from django.db.models import QuerySet +from django.db.models import F, QuerySet from django.urls import reverse from django.utils.text import slugify from django.utils.translation import gettext as _ @@ -176,10 +176,13 @@ def get_library_block(usage_key: LibraryUsageLocatorV2, include_collections=Fals raise ContentLibraryBlockNotFound(usage_key) if include_collections: + # Temporarily alias collection_code to "key" so downstream consumers + # (search indexer, REST API) keep the same field name. We will update + # downstream consumers later: https://github.com/openedx/openedx-platform/issues/38406 associated_collections = content_api.get_entity_collections( component.learning_package_id, component.entity_ref, - ).values('key', 'title') + ).values("title", key=F('collection_code')) else: associated_collections = None xblock_metadata = LibraryXBlockMetadata.from_component( diff --git a/openedx/core/djangoapps/content_libraries/api/collections.py b/openedx/core/djangoapps/content_libraries/api/collections.py index 41cf425c111c..1c87acd21232 100644 --- a/openedx/core/djangoapps/content_libraries/api/collections.py +++ b/openedx/core/djangoapps/content_libraries/api/collections.py @@ -2,7 +2,6 @@ Python API for library collections ================================== """ -from django.db import IntegrityError from opaque_keys import OpaqueKey from opaque_keys.edx.keys import BlockTypeKey, UsageKeyV2 from opaque_keys.edx.locator import LibraryCollectionLocator, LibraryContainerLocator, LibraryLocatorV2 @@ -51,18 +50,18 @@ def create_library_collection( assert content_library.learning_package_id assert content_library.library_key == library_key - try: - collection = content_api.create_collection( - learning_package_id=content_library.learning_package_id, - collection_code=collection_key, - title=title, - description=description, - created_by=created_by, - ) - except IntegrityError as err: - raise LibraryCollectionAlreadyExists from err - - return collection + if Collection.objects.filter( + learning_package_id=content_library.learning_package_id, + collection_code=collection_key, + ).exists(): + raise LibraryCollectionAlreadyExists(f"Collection {collection_key} already exists in {library_key}") + return content_api.create_collection( + learning_package_id=content_library.learning_package_id, + collection_code=collection_key, + title=title, + description=description, + created_by=created_by, + ) def update_library_collection( diff --git a/openedx/core/djangoapps/content_libraries/api/container_metadata.py b/openedx/core/djangoapps/content_libraries/api/container_metadata.py index a0a73deea6cd..98a5024ac674 100644 --- a/openedx/core/djangoapps/content_libraries/api/container_metadata.py +++ b/openedx/core/djangoapps/content_libraries/api/container_metadata.py @@ -365,9 +365,9 @@ def library_container_locator( container_type_code = content_api.get_container_type_code_of(container) if container_type_code not in LIBRARY_ALLOWED_CONTAINER_TYPES: raise ValueError(f"Unsupported container type for content libraries: {container!r}") - - # TODO: verify whether container_id should use entity_ref (opaque) or container_code (local slug). - return LibraryContainerLocator(library_key, container_type=container_type_code, container_id=container.entity_ref) + return LibraryContainerLocator( + library_key, container_type=container_type_code, container_id=container.container_code, + ) def get_container_from_key(container_key: LibraryContainerLocator, include_deleted=False) -> Container: diff --git a/openedx/core/djangoapps/content_libraries/api/containers.py b/openedx/core/djangoapps/content_libraries/api/containers.py index d357cdbe78f7..0b126e5665df 100644 --- a/openedx/core/djangoapps/content_libraries/api/containers.py +++ b/openedx/core/djangoapps/content_libraries/api/containers.py @@ -10,6 +10,7 @@ from uuid import uuid4 from django.db import transaction +from django.db.models import F from django.utils.text import slugify from opaque_keys.edx.locator import LibraryContainerLocator, LibraryLocatorV2, LibraryUsageLocatorV2 from openedx_content import api as content_api @@ -73,10 +74,13 @@ def get_container( """ container = get_container_from_key(container_key) if include_collections: + # Temporarily alias collection_code to "key" so downstream consumers + # (search indexer, REST API) keep the same field name. We will update + # downstream consumers later: https://github.com/openedx/openedx-platform/issues/38406 associated_collections = content_api.get_entity_collections( container.publishable_entity.learning_package_id, container_key.container_id, - ).values("key", "title") + ).values("title", key=F("collection_code")) else: associated_collections = None container_meta = ContainerMetadata.from_container( diff --git a/openedx/core/djangoapps/content_libraries/rest_api/collections.py b/openedx/core/djangoapps/content_libraries/rest_api/collections.py index 4ccbae2ba36b..9875f31d79d5 100644 --- a/openedx/core/djangoapps/content_libraries/rest_api/collections.py +++ b/openedx/core/djangoapps/content_libraries/rest_api/collections.py @@ -33,7 +33,10 @@ class LibraryCollectionsView(ModelViewSet): """ serializer_class = ContentLibraryCollectionSerializer - lookup_field = 'key' + # URL kwarg is `key` for backwards compatibility. + # https://github.com/openedx/openedx-platform/issues/38406 + lookup_field = 'collection_code' + lookup_url_kwarg = 'key' def __init__(self, *args, **kwargs) -> None: """ diff --git a/openedx/core/djangoapps/content_libraries/rest_api/serializers.py b/openedx/core/djangoapps/content_libraries/rest_api/serializers.py index b30906d58d89..425bb75c10d0 100644 --- a/openedx/core/djangoapps/content_libraries/rest_api/serializers.py +++ b/openedx/core/djangoapps/content_libraries/rest_api/serializers.py @@ -284,10 +284,13 @@ class ContentLibraryCollectionSerializer(serializers.ModelSerializer): """ Serializer for a Content Library Collection """ + # Expose Collection.collection_code as "key" to preserve the REST API field name. + # https://github.com/openedx/openedx-platform/issues/38406 + key = serializers.CharField(source='collection_code') class Meta: model = Collection - fields = '__all__' + exclude = ['collection_code'] class ContentLibraryCollectionUpdateSerializer(serializers.Serializer): diff --git a/openedx/core/djangoapps/content_libraries/tasks.py b/openedx/core/djangoapps/content_libraries/tasks.py index 2fe28c7476a8..ee972b248649 100644 --- a/openedx/core/djangoapps/content_libraries/tasks.py +++ b/openedx/core/djangoapps/content_libraries/tasks.py @@ -142,8 +142,8 @@ def send_events_after_publish(publish_log_pk: int, library_key_str: str) -> None pass else: log.warning( - f"PublishableEntity {record.entity.pk} / {record.entity.entity_ref} was modified during publish operation " - "but is of unknown type." + f"PublishableEntity {record.entity.pk} / {record.entity.entity_ref} " + "was modified during publish operation but is of unknown type." ) for container_key in affected_containers: @@ -246,8 +246,8 @@ def send_events_after_revert(draft_change_log_id: int, library_key_str: str) -> updated_container_keys.add(container_key) else: log.warning( - f"PublishableEntity {record.entity.pk} / {record.entity.entity_ref} was modified during publish operation " - "but is of unknown type." + f"PublishableEntity {record.entity.pk} / {record.entity.entity_ref} " + "was modified during publish operation but is of unknown type." ) # If any collections contain this entity, their item count may need to be updated, e.g. if this was a # newly created component in the collection and is now deleted, or this was deleted and is now re-added. @@ -256,7 +256,7 @@ def send_events_after_revert(draft_change_log_id: int, library_key_str: str) -> ): collection_key = api.library_collection_locator( library_key=library_key, - collection_key=parent_collection.key, + collection_key=parent_collection.collection_code, ) affected_collection_keys.add(collection_key) @@ -541,7 +541,7 @@ def backup_library(self, user_id: int, library_key_str: str) -> None: file_path = os.path.join(root_dir, filename) user = User.objects.get(id=user_id) origin_server = getattr(settings, 'CMS_BASE', None) - create_lib_zip_file(lp_key=str(library_key), path=file_path, user=user, origin_server=origin_server) + create_lib_zip_file(package_ref=str(library_key), path=file_path, user=user, origin_server=origin_server) set_custom_attribute("exporting_completed", str(library_key)) with open(file_path, 'rb') as zipfile: diff --git a/openedx/core/djangoapps/content_libraries/tests/test_api.py b/openedx/core/djangoapps/content_libraries/tests/test_api.py index 7d2e84c10280..cc0564964fb8 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_api.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_api.py @@ -323,8 +323,10 @@ def test_set_library_component_collections(self) -> None: ) assert self.lib2.learning_package_id is not None - assert len(content_api.get_collection(self.lib2.learning_package_id, self.col2.collection_code).entities.all()) == 1 - assert len(content_api.get_collection(self.lib2.learning_package_id, self.col3.collection_code).entities.all()) == 1 + col2 = content_api.get_collection(self.lib2.learning_package_id, self.col2.collection_code) + col3 = content_api.get_collection(self.lib2.learning_package_id, self.col3.collection_code) + assert len(col2.entities.all()) == 1 + assert len(col3.entities.all()) == 1 self.assertDictContainsEntries( event_receiver.call_args_list[0].kwargs, @@ -343,11 +345,15 @@ def test_set_library_component_collections(self) -> None: assert all(event["signal"] == LIBRARY_COLLECTION_UPDATED for event in collection_update_events) assert {event["library_collection"] for event in collection_update_events} == { LibraryCollectionData( - collection_key=api.library_collection_locator(self.lib2.library_key, collection_key=self.col2.collection_code), + collection_key=api.library_collection_locator( + self.lib2.library_key, collection_key=self.col2.collection_code, + ), background=True, ), LibraryCollectionData( - collection_key=api.library_collection_locator(self.lib2.library_key, collection_key=self.col3.collection_code), + collection_key=api.library_collection_locator( + self.lib2.library_key, collection_key=self.col3.collection_code, + ), background=True, ) } diff --git a/openedx/core/djangoapps/content_libraries/tests/test_views_collections.py b/openedx/core/djangoapps/content_libraries/tests/test_views_collections.py index a25b18dc3777..9f8e5dc1cde3 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_views_collections.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_views_collections.py @@ -247,7 +247,7 @@ def test_create_invalid_library_collection(self): assert resp.status_code == 400 - # Create collection with an existing collection.key; it should fail + # Create collection with an existing collection.collection_code; it should fail post_data_existing_key = { "key": self.col1.collection_code, "title": "Collection 4", diff --git a/openedx/core/djangoapps/video_config/transcripts_utils.py b/openedx/core/djangoapps/video_config/transcripts_utils.py index c76a2b8b0377..6e6f0fde7b8c 100644 --- a/openedx/core/djangoapps/video_config/transcripts_utils.py +++ b/openedx/core/djangoapps/video_config/transcripts_utils.py @@ -1016,7 +1016,7 @@ def get_transcript_from_openedx_content(video_block, language, output_format, tr .componentversionmedia_set .filter(media__has_file=True) .select_related('media') - .get(key=file_path) + .get(path=file_path) .media ) data = media.read_file().read() diff --git a/openedx/core/djangoapps/xblock/api.py b/openedx/core/djangoapps/xblock/api.py index cc684e72b13f..a4661f67b164 100644 --- a/openedx/core/djangoapps/xblock/api.py +++ b/openedx/core/djangoapps/xblock/api.py @@ -232,9 +232,9 @@ def get_block_olx( raise NoSuchUsage(usage_key) # TODO: we should probably make a method on ComponentVersion that returns - # a content based on the name. Accessing by componentversionmedia__key is + # a content based on the name. Accessing by componentversionmedia__path is # awkward. - content = component_version.media.get(componentversionmedia__key="block.xml") + content = component_version.media.get(componentversionmedia__path="block.xml") return content.text diff --git a/openedx/core/djangoapps/xblock/runtime/openedx_content_runtime.py b/openedx/core/djangoapps/xblock/runtime/openedx_content_runtime.py index 92caecccd32b..1da6f048b1cc 100644 --- a/openedx/core/djangoapps/xblock/runtime/openedx_content_runtime.py +++ b/openedx/core/djangoapps/xblock/runtime/openedx_content_runtime.py @@ -191,7 +191,7 @@ def get_block(self, usage_key, for_parent=None, *, version: int | LatestVersion raise NoSuchUsage(usage_key) content = component_version.media.get( - componentversionmedia__key="block.xml" + componentversionmedia__path="block.xml" ) xml_node = etree.fromstring(content.text) block_type = usage_key.block_type @@ -447,7 +447,7 @@ def _lookup_asset_url(self, block: XBlock, asset_path: str) -> str | None: component_version .componentversionmedia_set .filter(media__has_file=True) - .get(key=f"static/{asset_path}") + .get(path=f"static/{asset_path}") ) except ObjectDoesNotExist: try: @@ -458,7 +458,7 @@ def _lookup_asset_url(self, block: XBlock, asset_path: str) -> str | None: component_version .componentversionmedia_set .filter(media__has_file=True) - .get(key=f"static/{asset_path}") + .get(path=f"static/{asset_path}") ) except ObjectDoesNotExist: # This means we see a path that _looks_ like it should be a static From bf0ac231d1128202448d0cf6119eb34b80b719a2 Mon Sep 17 00:00:00 2001 From: Kyle D McCormick Date: Wed, 22 Apr 2026 11:54:47 -0400 Subject: [PATCH 09/15] fix: Return 'unknown/unknown' for un-parseable org/lib archive slugs This is necessary because we are no longer presuming that package_ref follows the same format as a Content Library. In the future, we may want a more graceful way of handling this. --- .../content_libraries/rest_api/serializers.py | 28 +++++++++++++------ 1 file changed, 20 insertions(+), 8 deletions(-) diff --git a/openedx/core/djangoapps/content_libraries/rest_api/serializers.py b/openedx/core/djangoapps/content_libraries/rest_api/serializers.py index 425bb75c10d0..f8dd8b18a839 100644 --- a/openedx/core/djangoapps/content_libraries/rest_api/serializers.py +++ b/openedx/core/djangoapps/content_libraries/rest_api/serializers.py @@ -450,14 +450,14 @@ class RestoreSuccessDataSerializer(serializers.Serializer): """ learning_package_id = serializers.IntegerField(source="lp_restored_data.id") title = serializers.CharField(source="lp_restored_data.title") - # archive_org_code and archive_package_code may be None when archive_package_ref cannot be parsed - # as {prefix}:{org_code}:{package_code} (previously this raised ValueError in openedx-core). - org = serializers.CharField(source="lp_restored_data.archive_org_code", allow_null=True) - slug = serializers.CharField(source="lp_restored_data.archive_package_code", allow_null=True) - - # The `key` is a unique temporary key assigned to the learning package during the restore process, - # whereas the `archive_key` is the original key of the learning package from the backup. - # The temporary learning package key is replaced with a standard key once it is added to a content library. + org = serializers.SerializerMethodField() + slug = serializers.SerializerMethodField() + + # The `package_ref` is a unique temporary key assigned to the learning + # package during the restore process, whereas the `archive_package_ref` is + # the original key of the learning package from the backup. The temporary + # learning package_ref is replaced with a standard key once it is added to a + # content library. key = serializers.CharField(source="lp_restored_data.package_ref") archive_key = serializers.CharField(source="lp_restored_data.archive_package_ref") @@ -472,6 +472,18 @@ class RestoreSuccessDataSerializer(serializers.Serializer): created_at = serializers.DateTimeField(source="backup_metadata.created_at", format=DATETIME_FORMAT) created_by = serializers.SerializerMethodField() + def get_org(self, obj) -> str: + """ + The org code/slug, as parsed from archive_package_ref, or "unknown" if unparseable. + """ + return obj["lp_restored_data"]["archive_org_code"] or "unknown" + + def get_slug(self, obj) -> str: + """ + The library code/slug, as parsed from archive_package_ref, or "unknown" if unparseable. + """ + return obj["lp_restored_data"]["archive_package_code"] or "unknown" + def get_created_by(self, obj): """ Get the user information of the archive creator, if available. From 1be040132624a1a61a5710dad3eedd06c21b18b6 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Mon, 20 Apr 2026 20:11:54 -0700 Subject: [PATCH 10/15] feat: update content libraries API to use upstream collection events --- .../content_libraries/api/blocks.py | 39 ---- .../content_libraries/api/collections.py | 19 +- .../content_libraries/api/containers.py | 49 +---- .../content_libraries/signal_handlers.py | 203 +++++------------- .../djangoapps/content_libraries/tasks.py | 73 +++++-- .../content_libraries/tests/test_api.py | 67 +++--- .../content_libraries/tests/test_events.py | 6 +- 7 files changed, 138 insertions(+), 318 deletions(-) diff --git a/openedx/core/djangoapps/content_libraries/api/blocks.py b/openedx/core/djangoapps/content_libraries/api/blocks.py index f8d45b78d5fe..091c17b2ed88 100644 --- a/openedx/core/djangoapps/content_libraries/api/blocks.py +++ b/openedx/core/djangoapps/content_libraries/api/blocks.py @@ -28,7 +28,6 @@ from openedx_events.content_authoring.data import ( ContentObjectChangedData, LibraryBlockData, - LibraryCollectionData, LibraryContainerData, ) from openedx_events.content_authoring.signals import ( @@ -36,7 +35,6 @@ LIBRARY_BLOCK_CREATED, LIBRARY_BLOCK_DELETED, LIBRARY_BLOCK_UPDATED, - LIBRARY_COLLECTION_UPDATED, LIBRARY_CONTAINER_UPDATED, ) from xblock.core import XBlock @@ -52,7 +50,6 @@ from .. import tasks from ..models import ContentLibrary from .block_metadata import LibraryXBlockMetadata, LibraryXBlockStaticFile -from .collections import library_collection_locator from .container_metadata import container_subclass_for_olx_tag from .containers import ( ContainerMetadata, @@ -728,30 +725,12 @@ def send_block_deleted_signal(): send_block_deleted_signal() raise - affected_collections = content_api.get_entity_collections(component.learning_package_id, component.entity_ref) affected_containers = get_containers_contains_item(usage_key) content_api.soft_delete_draft(component.id, deleted_by=user_id) send_block_deleted_signal() - # For each collection, trigger LIBRARY_COLLECTION_UPDATED signal and set background=True to trigger - # collection indexing asynchronously. - # - # To delete the component on collections - for collection in affected_collections: - # .. event_implemented_name: LIBRARY_COLLECTION_UPDATED - # .. event_type: org.openedx.content_authoring.content_library.collection.updated.v1 - LIBRARY_COLLECTION_UPDATED.send_event( - library_collection=LibraryCollectionData( - collection_key=library_collection_locator( - library_key=library_key, - collection_key=collection.collection_code, - ), - background=True, - ) - ) - # For each container, trigger LIBRARY_CONTAINER_UPDATED signal and set background=True to trigger # container indexing asynchronously. # @@ -773,7 +752,6 @@ def restore_library_block(usage_key: LibraryUsageLocatorV2, user_id: int | None """ component = get_component_from_usage_key(usage_key) library_key = usage_key.context_key - affected_collections = content_api.get_entity_collections(component.learning_package_id, component.entity_ref) # Set draft version back to the latest available component version id. content_api.set_draft_version( @@ -801,23 +779,6 @@ def restore_library_block(usage_key: LibraryUsageLocatorV2, user_id: int | None ), ) - # For each collection, trigger LIBRARY_COLLECTION_UPDATED signal and set background=True to trigger - # collection indexing asynchronously. - # - # To restore the component in the collections - for collection in affected_collections: - # .. event_implemented_name: LIBRARY_COLLECTION_UPDATED - # .. event_type: org.openedx.content_authoring.content_library.collection.updated.v1 - LIBRARY_COLLECTION_UPDATED.send_event( - library_collection=LibraryCollectionData( - collection_key=library_collection_locator( - library_key=library_key, - collection_key=collection.collection_code, - ), - background=True, - ) - ) - # For each container, trigger LIBRARY_CONTAINER_UPDATED signal and set background=True to trigger # container indexing asynchronously. # diff --git a/openedx/core/djangoapps/content_libraries/api/collections.py b/openedx/core/djangoapps/content_libraries/api/collections.py index 1c87acd21232..a8b715750d64 100644 --- a/openedx/core/djangoapps/content_libraries/api/collections.py +++ b/openedx/core/djangoapps/content_libraries/api/collections.py @@ -7,8 +7,6 @@ from opaque_keys.edx.locator import LibraryCollectionLocator, LibraryContainerLocator, LibraryLocatorV2 from openedx_content import api as content_api from openedx_content.models_api import Collection, Component, PublishableEntity -from openedx_events.content_authoring.data import LibraryCollectionData -from openedx_events.content_authoring.signals import LIBRARY_COLLECTION_UPDATED from ..models import ContentLibrary from .exceptions import ( @@ -216,27 +214,12 @@ def set_library_item_collections( collection_code__in=collection_keys ) - affected_collections = content_api.set_collections( + content_api.set_collections( publishable_entity, collection_qs, created_by=created_by, ) - # For each collection, trigger LIBRARY_COLLECTION_UPDATED signal and set background=True to trigger - # collection indexing asynchronously. - for collection in affected_collections: - # .. event_implemented_name: LIBRARY_COLLECTION_UPDATED - # .. event_type: org.openedx.content_authoring.content_library.collection.updated.v1 - LIBRARY_COLLECTION_UPDATED.send_event( - library_collection=LibraryCollectionData( - collection_key=library_collection_locator( - library_key=library_key, - collection_key=collection.collection_code, - ), - background=True, - ) - ) - return publishable_entity diff --git a/openedx/core/djangoapps/content_libraries/api/containers.py b/openedx/core/djangoapps/content_libraries/api/containers.py index 0b126e5665df..883fa7cc0d11 100644 --- a/openedx/core/djangoapps/content_libraries/api/containers.py +++ b/openedx/core/djangoapps/content_libraries/api/containers.py @@ -15,17 +15,14 @@ from opaque_keys.edx.locator import LibraryContainerLocator, LibraryLocatorV2, LibraryUsageLocatorV2 from openedx_content import api as content_api from openedx_content.models_api import Container, Unit -from openedx_events.content_authoring.data import ContentObjectChangedData, LibraryCollectionData, LibraryContainerData +from openedx_events.content_authoring.data import ContentObjectChangedData, LibraryContainerData from openedx_events.content_authoring.signals import ( CONTENT_OBJECT_ASSOCIATIONS_CHANGED, - LIBRARY_COLLECTION_UPDATED, LIBRARY_CONTAINER_CREATED, LIBRARY_CONTAINER_DELETED, LIBRARY_CONTAINER_UPDATED, ) -from openedx.core.djangoapps.content_libraries.api.collections import library_collection_locator - from .. import tasks from ..models import ContentLibrary from .block_metadata import LibraryXBlockMetadata @@ -225,13 +222,6 @@ def send_container_deleted_signal(): send_container_deleted_signal() raise - library_key = container_key.lib_key - - # Fetch related collections and containers before soft-delete - affected_collections = content_api.get_entity_collections( - container.publishable_entity.learning_package_id, - container.entity_ref, - ) affected_containers = get_containers_contains_item(container_key) # Get children containers or components to update their index data children = get_container_children( @@ -242,22 +232,6 @@ def send_container_deleted_signal(): send_container_deleted_signal() - # For each collection, trigger LIBRARY_COLLECTION_UPDATED signal and set background=True to trigger - # collection indexing asynchronously. - # - # To delete the container on collections - for collection in affected_collections: - # .. event_implemented_name: LIBRARY_COLLECTION_UPDATED - # .. event_type: org.openedx.content_authoring.content_library.collection.updated.v1 - LIBRARY_COLLECTION_UPDATED.send_event( - library_collection=LibraryCollectionData( - collection_key=library_collection_locator( - library_key=library_key, - collection_key=collection.collection_code, - ), - background=True, - ) - ) # Send events related to the containers that contains the updated container. # This is to update the children display names used in the section/subsection previews. for affected_container in affected_containers: @@ -290,14 +264,8 @@ def restore_container(container_key: LibraryContainerLocator) -> None: """ [ 🛑 UNSTABLE ] Restore the specified library container. """ - library_key = container_key.lib_key container = get_container_from_key(container_key, include_deleted=True) - affected_collections = content_api.get_entity_collections( - container.publishable_entity.learning_package_id, - container.entity_ref, - ) - content_api.set_draft_version(container.id, container.versioning.latest.pk) # Fetch related containers after restore affected_containers = get_containers_contains_item(container_key) @@ -326,21 +294,6 @@ def restore_container(container_key: LibraryContainerLocator) -> None: ), ) - # For each collection, trigger LIBRARY_COLLECTION_UPDATED signal and set background=True to trigger - # collection indexing asynchronously. - # - # To restore the container on collections - for collection in affected_collections: - # .. event_implemented_name: LIBRARY_COLLECTION_UPDATED - # .. event_type: org.openedx.content_authoring.content_library.collection.updated.v1 - LIBRARY_COLLECTION_UPDATED.send_event( - library_collection=LibraryCollectionData( - collection_key=library_collection_locator( - library_key=library_key, - collection_key=collection.collection_code, - ), - ) - ) # Send events related to the containers that contains the updated container. # This is to update the children display names used in the section/subsection previews. for affected_container in affected_containers: diff --git a/openedx/core/djangoapps/content_libraries/signal_handlers.py b/openedx/core/djangoapps/content_libraries/signal_handlers.py index fada1cb2f874..48221ddfd304 100644 --- a/openedx/core/djangoapps/content_libraries/signal_handlers.py +++ b/openedx/core/djangoapps/content_libraries/signal_handlers.py @@ -4,179 +4,74 @@ import logging -from django.db.models.signals import m2m_changed, post_delete, post_save from django.dispatch import receiver -from opaque_keys import OpaqueKey -from opaque_keys.edx.locator import LibraryLocatorV2 -from openedx_content.api import get_components, get_containers -from openedx_content.models_api import Collection, CollectionPublishableEntity, PublishableEntity -from openedx_events.content_authoring.data import ContentObjectChangedData, LibraryCollectionData +from openedx_content.api import signals as content_signals +from openedx_events.content_authoring.data import LibraryCollectionData from openedx_events.content_authoring.signals import ( - CONTENT_OBJECT_ASSOCIATIONS_CHANGED, LIBRARY_COLLECTION_CREATED, LIBRARY_COLLECTION_DELETED, LIBRARY_COLLECTION_UPDATED, ) -from .api import library_collection_locator, library_component_usage_key, library_container_locator +from . import tasks +from .api import library_collection_locator from .models import ContentLibrary log = logging.getLogger(__name__) -@receiver(post_save, sender=Collection, dispatch_uid="library_collection_saved") -def library_collection_saved(sender, instance, created, **kwargs): +@receiver(content_signals.LEARNING_PACKAGE_COLLECTION_CHANGED) +def collection_updated( + learning_package: content_signals.LearningPackageEventData, + change: content_signals.CollectionChangeData, + **kwargs, +): """ - Raises LIBRARY_COLLECTION_CREATED if the Collection is new, - or LIBRARY_COLLECTION_UPDATED if updated an existing Collection. + A Collection has been updated - handle that as needed. + + We receive this low-level event from `openedx_content`, and check if it + happened in a library. If so, we emit more detailed library-specific events. + + ⏳ This event is emitted synchronously and this handler is called + synchronously. If a lot of entities were changed, we need to dispatch an + asynchronous handler to deal with them to avoid slowdowns. """ try: - library = ContentLibrary.objects.get(learning_package_id=instance.learning_package_id) + library = ContentLibrary.objects.get(learning_package_id=learning_package.id) except ContentLibrary.DoesNotExist: - log.error("{instance} is not associated with a content library.") - return + return # We don't care about non-library events. + + collection_key = library_collection_locator(library_key=library.library_key, collection_key=change.collection_code) + entities_changed = change.entities_added + change.entities_removed - if created: + if change.created: # This is a newly-created collection, or was "un-deleted": # .. event_implemented_name: LIBRARY_COLLECTION_CREATED # .. event_type: org.openedx.content_authoring.content_library.collection.created.v1 - LIBRARY_COLLECTION_CREATED.send_event( - library_collection=LibraryCollectionData( - collection_key=library_collection_locator( - library_key=library.library_key, - collection_key=instance.collection_code, - ), - ) - ) - else: + LIBRARY_COLLECTION_CREATED.send_event(library_collection=LibraryCollectionData(collection_key=collection_key)) + # As an example of what this event triggers, Collections are listed in the Meilisearch index as items in the + # library. So the handler will add this Collection as an entry in the Meilisearch index. + elif change.metadata_modified or entities_changed: + # The collection was renamed or its items were changed. + # This event is ambiguous but because the search index of the collection itself may have something like + # "contains 15 items", we _do_ need to emit it even when only the items have changed and not the metadata. # .. event_implemented_name: LIBRARY_COLLECTION_UPDATED # .. event_type: org.openedx.content_authoring.content_library.collection.updated.v1 - LIBRARY_COLLECTION_UPDATED.send_event( - library_collection=LibraryCollectionData( - collection_key=library_collection_locator( - library_key=library.library_key, - collection_key=instance.collection_code, - ), - ) - ) - - -@receiver(post_delete, sender=Collection, dispatch_uid="library_collection_deleted") -def library_collection_deleted(sender, instance, **kwargs): - """ - Raises LIBRARY_COLLECTION_DELETED for the deleted Collection. - """ - try: - library = ContentLibrary.objects.get(learning_package_id=instance.learning_package_id) - except ContentLibrary.DoesNotExist: - log.error("{instance} is not associated with a content library.") - return - - # .. event_implemented_name: LIBRARY_COLLECTION_DELETED - # .. event_type: org.openedx.content_authoring.content_library.collection.deleted.v1 - LIBRARY_COLLECTION_DELETED.send_event( - library_collection=LibraryCollectionData( - collection_key=library_collection_locator( - library_key=library.library_key, - collection_key=instance.collection_code, - ), - ) - ) - - -def _library_collection_entity_changed( - publishable_entity: PublishableEntity, - library_key: LibraryLocatorV2 | None = None, -) -> None: - """ - Sends a CONTENT_OBJECT_ASSOCIATIONS_CHANGED event for the entity. - """ - if not library_key: - try: - library = ContentLibrary.objects.get( - learning_package_id=publishable_entity.learning_package_id, - ) - except ContentLibrary.DoesNotExist: - log.error("{publishable_entity} is not associated with a content library.") - return - - library_key = library.library_key - - assert library_key - - opaque_key: OpaqueKey - - if hasattr(publishable_entity, 'component'): - opaque_key = library_component_usage_key( - library_key, - publishable_entity.component, + LIBRARY_COLLECTION_UPDATED.send_event(library_collection=LibraryCollectionData(collection_key=collection_key)) + elif change.deleted: + # .. event_implemented_name: LIBRARY_COLLECTION_DELETED + # .. event_type: org.openedx.content_authoring.content_library.collection.deleted.v1 + LIBRARY_COLLECTION_DELETED.send_event(library_collection=LibraryCollectionData(collection_key=collection_key)) + + # Now, what about the actual entities (containers/components) in the collection? + if entities_changed: + if len(entities_changed) == 1: + # If there's only one changed entity, emit the event synchronously: + fn = tasks.send_collections_changed_events + else: + # If there are more than one changed entities, emit the events asynchronously: + fn = tasks.send_collections_changed_events.delay + fn( + publishable_entity_ids=sorted(entities_changed), # sorted() is mostly for test purposes + learning_package_id=learning_package.id, + library_key_str=str(library.library_key), ) - elif hasattr(publishable_entity, 'container'): - opaque_key = library_container_locator( - library_key, - publishable_entity.container, - ) - else: - log.error("Unknown publishable entity type: %s", publishable_entity) - return - - # .. event_implemented_name: CONTENT_OBJECT_ASSOCIATIONS_CHANGED - # .. event_type: org.openedx.content_authoring.content.object.associations.changed.v1 - CONTENT_OBJECT_ASSOCIATIONS_CHANGED.send_event( - content_object=ContentObjectChangedData( - object_id=str(opaque_key), - changes=["collections"], - ), - ) - - -@receiver(post_save, sender=CollectionPublishableEntity, dispatch_uid="library_collection_entity_saved") -def library_collection_entity_saved(sender, instance, created, **kwargs): - """ - Sends a CONTENT_OBJECT_ASSOCIATIONS_CHANGED event for components added to a collection. - """ - if created: - _library_collection_entity_changed(instance.entity) - - -@receiver(post_delete, sender=CollectionPublishableEntity, dispatch_uid="library_collection_entity_deleted") -def library_collection_entity_deleted(sender, instance, **kwargs): - """ - Sends a CONTENT_OBJECT_ASSOCIATIONS_CHANGED event for components removed from a collection. - """ - # Only trigger component updates if CollectionPublishableEntity was cascade deleted due to deletion of a collection. - if isinstance(kwargs.get('origin'), Collection): - _library_collection_entity_changed(instance.entity) - - -@receiver(m2m_changed, sender=CollectionPublishableEntity, dispatch_uid="library_collection_entities_changed") -def library_collection_entities_changed(sender, instance, action, pk_set, **kwargs): - """ - Sends a CONTENT_OBJECT_ASSOCIATIONS_CHANGED event for components added/removed/cleared from a collection. - """ - if action not in ["post_add", "post_remove", "post_clear"]: - return - - try: - library = ContentLibrary.objects.get( - learning_package_id=instance.learning_package_id, - ) - except ContentLibrary.DoesNotExist: - log.error("{instance} is not associated with a content library.") - return - - if isinstance(instance, PublishableEntity): - _library_collection_entity_changed(instance, library.library_key) - return - - # When action=="post_clear", pk_set==None - # Since the collection instance now has an empty entities set, - # we don't know which ones were removed, so we need to update associations for all library - # components and containers. - components = get_components(instance.learning_package_id) - containers = get_containers(instance.learning_package_id) - if pk_set: - components = components.filter(pk__in=pk_set) - containers = containers.filter(pk__in=pk_set) - - for entity in list(components.all()) + list(containers.all()): - _library_collection_entity_changed(entity.publishable_entity, library.library_key) diff --git a/openedx/core/djangoapps/content_libraries/tasks.py b/openedx/core/djangoapps/content_libraries/tasks.py index ee972b248649..814624633c85 100644 --- a/openedx/core/djangoapps/content_libraries/tasks.py +++ b/openedx/core/djangoapps/content_libraries/tasks.py @@ -14,6 +14,7 @@ A longer-term solution to this issue would be to move the content_libraries app to cms: https://github.com/openedx/edx-platform/issues/33428 """ + from __future__ import annotations import json @@ -38,22 +39,26 @@ set_code_owner_attribute_from_module, set_custom_attribute, ) +from opaque_keys import OpaqueKey from opaque_keys.edx.locator import ( BlockUsageLocator, - LibraryCollectionLocator, LibraryContainerLocator, LibraryLocatorV2, ) from openedx_content import api as content_api from openedx_content.api import create_zip_file as create_lib_zip_file -from openedx_content.models_api import DraftChangeLog, PublishLog -from openedx_events.content_authoring.data import LibraryBlockData, LibraryCollectionData, LibraryContainerData +from openedx_content.models_api import DraftChangeLog, LearningPackage, PublishableEntity, PublishLog +from openedx_events.content_authoring.data import ( + ContentObjectChangedData, + LibraryBlockData, + LibraryContainerData, +) from openedx_events.content_authoring.signals import ( + CONTENT_OBJECT_ASSOCIATIONS_CHANGED, LIBRARY_BLOCK_CREATED, LIBRARY_BLOCK_DELETED, LIBRARY_BLOCK_PUBLISHED, LIBRARY_BLOCK_UPDATED, - LIBRARY_COLLECTION_UPDATED, LIBRARY_CONTAINER_CREATED, LIBRARY_CONTAINER_DELETED, LIBRARY_CONTAINER_PUBLISHED, @@ -83,6 +88,48 @@ DATETIME_FORMAT = '%Y-%m-%dT%H:%M:%SZ' # Should match serializer format. Redefined to avoid circular import. +@shared_task(base=LoggedTask) +@set_code_owner_attribute +def send_collections_changed_events( + publishable_entity_ids: list[PublishableEntity.ID], + learning_package_id: LearningPackage.ID, + library_key_str: str, +): + """ + Sends a CONTENT_OBJECT_ASSOCIATIONS_CHANGED event for each modified library + entity in the given list, because their associated collections have changed. + + ⏳ This task is designed to be run asynchronously so it can handle many + entities, but you can also call it synchronously if you are only + processing a single entity. Handlers should be synchronous and fast, to + support the "update one item synchronously" use case, but can be async if + needed. + """ + library_key = LibraryLocatorV2.from_string(library_key_str) + entities = ( + content_api.get_publishable_entities(learning_package_id) + .filter(id__in=publishable_entity_ids) + .select_related("component", "container") + ) + + for entity in entities: + opaque_key: OpaqueKey + + if hasattr(entity, "component"): + opaque_key = api.library_component_usage_key(library_key, entity.component) + elif hasattr(entity, "container"): + opaque_key = api.library_container_locator(library_key, entity.container) + else: + log.error("Unknown publishable entity type: %s", entity) + continue + + # .. event_implemented_name: CONTENT_OBJECT_ASSOCIATIONS_CHANGED + # .. event_type: org.openedx.content_authoring.content.object.associations.changed.v1 + CONTENT_OBJECT_ASSOCIATIONS_CHANGED.send_event( + content_object=ContentObjectChangedData(object_id=str(opaque_key), changes=["collections"]), + ) + + @shared_task(base=LoggedTask) @set_code_owner_attribute def send_events_after_publish(publish_log_pk: int, library_key_str: str) -> None: @@ -199,7 +246,6 @@ def send_events_after_revert(draft_change_log_id: int, library_key_str: str) -> created_container_keys: set[LibraryContainerLocator] = set() updated_container_keys: set[LibraryContainerLocator] = set() deleted_container_keys: set[LibraryContainerLocator] = set() - affected_collection_keys: set[LibraryCollectionLocator] = set() # Update anything that needs to be updated (e.g. search index): for record in affected_entities: @@ -249,16 +295,6 @@ def send_events_after_revert(draft_change_log_id: int, library_key_str: str) -> f"PublishableEntity {record.entity.pk} / {record.entity.entity_ref} " "was modified during publish operation but is of unknown type." ) - # If any collections contain this entity, their item count may need to be updated, e.g. if this was a - # newly created component in the collection and is now deleted, or this was deleted and is now re-added. - for parent_collection in content_api.get_entity_collections( - record.entity.learning_package_id, record.entity.entity_ref, - ): - collection_key = api.library_collection_locator( - library_key=library_key, - collection_key=parent_collection.collection_code, - ) - affected_collection_keys.add(collection_key) for container_key in deleted_container_keys: # .. event_implemented_name: LIBRARY_CONTAINER_DELETED @@ -283,13 +319,6 @@ def send_events_after_revert(draft_change_log_id: int, library_key_str: str) -> library_container=LibraryContainerData(container_key=container_key) ) - for collection_key in affected_collection_keys: - # .. event_implemented_name: LIBRARY_COLLECTION_UPDATED - # .. event_type: org.openedx.content_authoring.content_library.collection.updated.v1 - LIBRARY_COLLECTION_UPDATED.send_event( - library_collection=LibraryCollectionData(collection_key=collection_key) - ) - def wait_for_post_revert_events(draft_change_log: DraftChangeLog, library_key: LibraryLocatorV2): """ diff --git a/openedx/core/djangoapps/content_libraries/tests/test_api.py b/openedx/core/djangoapps/content_libraries/tests/test_api.py index cc0564964fb8..8b9503a3fc49 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_api.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_api.py @@ -28,6 +28,8 @@ LIBRARY_CONTAINER_DELETED, LIBRARY_CONTAINER_UPDATED, ) +from openedx_events.testing import EventsIsolationMixin +from openedx_events.tooling import OpenEdxPublicSignal from user_tasks.models import UserTaskStatus from common.djangoapps.student.tests.factories import UserFactory @@ -37,13 +39,21 @@ from .base import ContentLibrariesRestApiTest -class ContentLibraryCollectionsTest(ContentLibrariesRestApiTest): +class ContentLibraryCollectionsTest(ContentLibrariesRestApiTest, EventsIsolationMixin): """ Tests for Content Library API collections methods. Same guidelines as ContentLibrariesTestCase. """ + @classmethod + def setUpClass(cls): + """Test setup""" + super().setUpClass() + # By default, errors thrown in signal handlers get suppressed. We want to see them though! + # https://github.com/openedx/openedx-events/issues/569 + cls.allow_send_events_failure(*(s.event_type for s in OpenEdxPublicSignal.all_events())) + def setUp(self) -> None: super().setUp() @@ -252,11 +262,12 @@ def test_update_library_collection_components_event(self) -> None: self.assertDictContainsEntries( event_receiver.call_args_list[0].kwargs, { - "signal": CONTENT_OBJECT_ASSOCIATIONS_CHANGED, - "sender": None, - "content_object": ContentObjectChangedData( - object_id=self.lib1_problem_block["id"], - changes=["collections"], + "signal": LIBRARY_COLLECTION_UPDATED, + "library_collection": LibraryCollectionData( + collection_key=api.library_collection_locator( + self.lib1.library_key, + collection_key="COL1", + ), ), }, ) @@ -264,9 +275,8 @@ def test_update_library_collection_components_event(self) -> None: event_receiver.call_args_list[1].kwargs, { "signal": CONTENT_OBJECT_ASSOCIATIONS_CHANGED, - "sender": None, "content_object": ContentObjectChangedData( - object_id=self.lib1_html_block["id"], + object_id=self.lib1_problem_block["id"], changes=["collections"], ), }, @@ -275,9 +285,8 @@ def test_update_library_collection_components_event(self) -> None: event_receiver.call_args_list[2].kwargs, { "signal": CONTENT_OBJECT_ASSOCIATIONS_CHANGED, - "sender": None, "content_object": ContentObjectChangedData( - object_id=self.unit1["id"], + object_id=self.lib1_html_block["id"], changes=["collections"], ), }, @@ -285,13 +294,10 @@ def test_update_library_collection_components_event(self) -> None: self.assertDictContainsEntries( event_receiver.call_args_list[3].kwargs, { - "signal": LIBRARY_COLLECTION_UPDATED, - "sender": None, - "library_collection": LibraryCollectionData( - collection_key=api.library_collection_locator( - self.lib1.library_key, - collection_key="COL1", - ), + "signal": CONTENT_OBJECT_ASSOCIATIONS_CHANGED, + "content_object": ContentObjectChangedData( + object_id=self.unit1["id"], + changes=["collections"], ), }, ) @@ -345,16 +351,10 @@ def test_set_library_component_collections(self) -> None: assert all(event["signal"] == LIBRARY_COLLECTION_UPDATED for event in collection_update_events) assert {event["library_collection"] for event in collection_update_events} == { LibraryCollectionData( - collection_key=api.library_collection_locator( - self.lib2.library_key, collection_key=self.col2.collection_code, - ), - background=True, + collection_key=api.library_collection_locator(self.lib2.library_key, collection_key=self.col2.collection_code) ), LibraryCollectionData( - collection_key=api.library_collection_locator( - self.lib2.library_key, collection_key=self.col3.collection_code, - ), - background=True, + collection_key=api.library_collection_locator(self.lib2.library_key, collection_key=self.col3.collection_code) ) } @@ -380,11 +380,7 @@ def test_delete_library_block(self) -> None: "signal": LIBRARY_COLLECTION_UPDATED, "sender": None, "library_collection": LibraryCollectionData( - collection_key=api.library_collection_locator( - self.lib1.library_key, - collection_key=self.col1.collection_code, - ), - background=True, + collection_key=api.library_collection_locator(self.lib1.library_key, collection_key=self.col1.collection_code), ), }, ) @@ -423,7 +419,6 @@ def test_delete_library_container(self) -> None: self.lib1.library_key, collection_key=self.col1.collection_code, ), - background=True, ), }, ) @@ -503,19 +498,22 @@ def test_delete_library_block_when_component_does_not_exist(self) -> None: ) def test_restore_library_block(self) -> None: + usage_key = LibraryUsageLocatorV2.from_string(self.lib1_problem_block["id"]) api.update_library_collection_items( self.lib1.library_key, self.col1.collection_code, opaque_keys=[ - LibraryUsageLocatorV2.from_string(self.lib1_problem_block["id"]), + usage_key, LibraryUsageLocatorV2.from_string(self.lib1_html_block["id"]), ], ) + api.delete_library_block(usage_key) + event_receiver = mock.Mock() LIBRARY_COLLECTION_UPDATED.connect(event_receiver) - api.restore_library_block(LibraryUsageLocatorV2.from_string(self.lib1_problem_block["id"])) + api.restore_library_block(usage_key) assert event_receiver.call_count == 1 self.assertDictContainsEntries( @@ -528,7 +526,6 @@ def test_restore_library_block(self) -> None: self.lib1.library_key, collection_key=self.col1.collection_code, ), - background=True, ), }, ) @@ -555,6 +552,8 @@ def test_add_component_and_revert(self) -> None: collection_update_event_receiver = mock.Mock() LIBRARY_COLLECTION_UPDATED.connect(collection_update_event_receiver) + # Reverting the change essentially deletes the item, so we should get an event that the collection's contents + # have been updated. api.revert_changes(self.lib1.library_key) assert collection_update_event_receiver.call_count == 1 diff --git a/openedx/core/djangoapps/content_libraries/tests/test_events.py b/openedx/core/djangoapps/content_libraries/tests/test_events.py index bf4857a3cae7..d6b7306a8c31 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_events.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_events.py @@ -611,11 +611,11 @@ def test_collection_crud(self) -> None: "library_collection": LibraryCollectionData(collection_key), }) - # Soft delete the collection. NOTE: at the moment, it's only possible to "soft delete" collections via - # the REST API, which sends an UPDATED event because the collection is now "disabled" but not deleted. + # Soft delete the collection. Whether we "soft" or "hard" delete, it sends a "DELETED" event. + # If we later restore it, it would send a "CREATED" event. self._soft_delete_collection(collection_key) self.expect_new_events({ - "signal": LIBRARY_COLLECTION_UPDATED, # UPDATED not DELETED. If we do a hard delete, it should be DELETED. + "signal": LIBRARY_COLLECTION_DELETED, "library_collection": LibraryCollectionData(collection_key), }) From 12ba335a511a93a2f962305e4b49f1bf8842782d Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Tue, 21 Apr 2026 19:15:03 -0700 Subject: [PATCH 11/15] feat: update content libraries API to use upstream entity changed events --- .../content_libraries/api/blocks.py | 149 +-------- .../content_libraries/api/containers.py | 170 +---------- .../content_libraries/api/libraries.py | 5 +- .../content_libraries/library_context.py | 33 -- .../content_libraries/signal_handlers.py | 38 ++- .../djangoapps/content_libraries/tasks.py | 287 ++++++++++-------- .../content_libraries/tests/test_events.py | 263 +++++++++++++--- .../learning_context/learning_context.py | 15 - .../xblock/runtime/openedx_content_runtime.py | 5 - 9 files changed, 436 insertions(+), 529 deletions(-) diff --git a/openedx/core/djangoapps/content_libraries/api/blocks.py b/openedx/core/djangoapps/content_libraries/api/blocks.py index 091c17b2ed88..d10aed954016 100644 --- a/openedx/core/djangoapps/content_libraries/api/blocks.py +++ b/openedx/core/djangoapps/content_libraries/api/blocks.py @@ -25,18 +25,8 @@ from opaque_keys.edx.locator import LibraryContainerLocator, LibraryLocatorV2, LibraryUsageLocatorV2 from openedx_content import api as content_api from openedx_content.models_api import Collection, Component, ComponentVersion, Container, LearningPackage, MediaType -from openedx_events.content_authoring.data import ( - ContentObjectChangedData, - LibraryBlockData, - LibraryContainerData, -) -from openedx_events.content_authoring.signals import ( - CONTENT_OBJECT_ASSOCIATIONS_CHANGED, - LIBRARY_BLOCK_CREATED, - LIBRARY_BLOCK_DELETED, - LIBRARY_BLOCK_UPDATED, - LIBRARY_CONTAINER_UPDATED, -) +from openedx_events.content_authoring.data import LibraryBlockData +from openedx_events.content_authoring.signals import LIBRARY_BLOCK_DELETED from xblock.core import XBlock from openedx.core.djangoapps.content_staging.data import StagedContentID @@ -55,7 +45,6 @@ ContainerMetadata, create_container, get_container, - get_containers_contains_item, update_container_children, ) from .exceptions import ( @@ -252,29 +241,6 @@ def set_library_block_olx(usage_key: LibraryUsageLocatorV2, new_olx_str: str) -> created=now, ) - # .. event_implemented_name: LIBRARY_BLOCK_UPDATED - # .. event_type: org.openedx.content_authoring.library_block.updated.v1 - transaction.on_commit(lambda: LIBRARY_BLOCK_UPDATED.send_event( - library_block=LibraryBlockData( - library_key=usage_key.context_key, - usage_key=usage_key - ) - )) - - # For each container, trigger LIBRARY_CONTAINER_UPDATED signal and set background=True to trigger - # container indexing asynchronously. - affected_containers = get_containers_contains_item(usage_key) - for container in affected_containers: - # .. event_implemented_name: LIBRARY_CONTAINER_UPDATED - # .. event_type: org.openedx.content_authoring.content_library.container.updated.v1 - container_key = container.container_key - transaction.on_commit(lambda ck=container_key: LIBRARY_CONTAINER_UPDATED.send_event( # type: ignore[misc] - library_container=LibraryContainerData( - container_key=ck, - background=True, - ) - )) - return new_component_version @@ -351,16 +317,6 @@ def create_library_block( _create_component_for_block(content_library, usage_key, user_id, can_stand_alone) # Now return the metadata about the new block: - - # .. event_implemented_name: LIBRARY_BLOCK_CREATED - # .. event_type: org.openedx.content_authoring.library_block.created.v1 - LIBRARY_BLOCK_CREATED.send_event( - library_block=LibraryBlockData( - library_key=content_library.library_key, - usage_key=usage_key - ) - ) - return get_library_block(usage_key) @@ -493,16 +449,6 @@ def _import_staged_block( path=filename, ) - # Emit library block created event - # .. event_implemented_name: LIBRARY_BLOCK_CREATED - # .. event_type: org.openedx.content_authoring.library_block.created.v1 - transaction.on_commit(lambda: LIBRARY_BLOCK_CREATED.send_event( - library_block=LibraryBlockData( - library_key=content_library.library_key, - usage_key=usage_key - ) - )) - # Now return the metadata about the new block return get_library_block(usage_key) @@ -704,16 +650,6 @@ def delete_library_block( """ library_key = usage_key.context_key - def send_block_deleted_signal(): - # .. event_implemented_name: LIBRARY_BLOCK_DELETED - # .. event_type: org.openedx.content_authoring.library_block.deleted.v1 - LIBRARY_BLOCK_DELETED.send_event( - library_block=LibraryBlockData( - library_key=library_key, - usage_key=usage_key - ) - ) - try: component = get_component_from_usage_key(usage_key) except Component.DoesNotExist: @@ -722,37 +658,22 @@ def send_block_deleted_signal(): # (an intermediate error occurred). # In that case, we keep the index updated by removing the entry, # but still raise the error so the caller knows the component did not exist. - send_block_deleted_signal() - raise - affected_containers = get_containers_contains_item(usage_key) + # .. event_implemented_name: LIBRARY_BLOCK_DELETED + # .. event_type: org.openedx.content_authoring.library_block.deleted.v1 + LIBRARY_BLOCK_DELETED.send_event( + library_block=LibraryBlockData(library_key=library_key, usage_key=usage_key) + ) + raise content_api.soft_delete_draft(component.id, deleted_by=user_id) - send_block_deleted_signal() - - # For each container, trigger LIBRARY_CONTAINER_UPDATED signal and set background=True to trigger - # container indexing asynchronously. - # - # To update the components count in containers - for container in affected_containers: - # .. event_implemented_name: LIBRARY_CONTAINER_UPDATED - # .. event_type: org.openedx.content_authoring.content_library.container.updated.v1 - LIBRARY_CONTAINER_UPDATED.send_event( - library_container=LibraryContainerData( - container_key=container.container_key, - background=True, - ) - ) - def restore_library_block(usage_key: LibraryUsageLocatorV2, user_id: int | None = None) -> None: """ Restore the specified library block. """ component = get_component_from_usage_key(usage_key) - library_key = usage_key.context_key - # Set draft version back to the latest available component version id. content_api.set_draft_version( component.id, @@ -760,40 +681,6 @@ def restore_library_block(usage_key: LibraryUsageLocatorV2, user_id: int | None set_by=user_id, ) - # .. event_implemented_name: LIBRARY_BLOCK_CREATED - # .. event_type: org.openedx.content_authoring.library_block.created.v1 - LIBRARY_BLOCK_CREATED.send_event( - library_block=LibraryBlockData( - library_key=library_key, - usage_key=usage_key - ) - ) - - # Add tags and collections back to index - # .. event_implemented_name: CONTENT_OBJECT_ASSOCIATIONS_CHANGED - # .. event_type: org.openedx.content_authoring.content.object.associations.changed.v1 - CONTENT_OBJECT_ASSOCIATIONS_CHANGED.send_event( - content_object=ContentObjectChangedData( - object_id=str(usage_key), - changes=["collections", "tags", "units"], - ), - ) - - # For each container, trigger LIBRARY_CONTAINER_UPDATED signal and set background=True to trigger - # container indexing asynchronously. - # - # To update the components count in containers - affected_containers = get_containers_contains_item(usage_key) - for container in affected_containers: - # .. event_implemented_name: LIBRARY_CONTAINER_UPDATED - # .. event_type: org.openedx.content_authoring.content_library.container.updated.v1 - LIBRARY_CONTAINER_UPDATED.send_event( - library_container=LibraryContainerData( - container_key=container.container_key, - background=True, - ) - ) - def get_library_block_static_asset_files(usage_key: LibraryUsageLocatorV2) -> list[LibraryXBlockStaticFile]: """ @@ -879,16 +766,6 @@ def add_library_block_static_asset_file( created=datetime.now(tz=timezone.utc), # noqa: UP017 created_by=user.id if user else None, ) - transaction.on_commit( - # .. event_implemented_name: LIBRARY_BLOCK_UPDATED - # .. event_type: org.openedx.content_authoring.library_block.updated.v1 - lambda: LIBRARY_BLOCK_UPDATED.send_event( - library_block=LibraryBlockData( - library_key=usage_key.context_key, - usage_key=usage_key, - ) - ) - ) # Now figure out the URL for the newly created asset... site_root_url = get_xblock_app_config().get_site_root_url() @@ -927,16 +804,6 @@ def delete_library_block_static_asset_file(usage_key, file_path, user=None): created=now, created_by=user.id if user else None, ) - transaction.on_commit( - # .. event_implemented_name: LIBRARY_BLOCK_UPDATED - # .. event_type: org.openedx.content_authoring.library_block.updated.v1 - lambda: LIBRARY_BLOCK_UPDATED.send_event( - library_block=LibraryBlockData( - library_key=usage_key.context_key, - usage_key=usage_key, - ) - ) - ) def publish_component_changes(usage_key: LibraryUsageLocatorV2, user_id: int): diff --git a/openedx/core/djangoapps/content_libraries/api/containers.py b/openedx/core/djangoapps/content_libraries/api/containers.py index 883fa7cc0d11..08a3dcd59c9c 100644 --- a/openedx/core/djangoapps/content_libraries/api/containers.py +++ b/openedx/core/djangoapps/content_libraries/api/containers.py @@ -9,19 +9,13 @@ from datetime import datetime, timezone from uuid import uuid4 -from django.db import transaction from django.db.models import F from django.utils.text import slugify from opaque_keys.edx.locator import LibraryContainerLocator, LibraryLocatorV2, LibraryUsageLocatorV2 from openedx_content import api as content_api -from openedx_content.models_api import Container, Unit -from openedx_events.content_authoring.data import ContentObjectChangedData, LibraryContainerData -from openedx_events.content_authoring.signals import ( - CONTENT_OBJECT_ASSOCIATIONS_CHANGED, - LIBRARY_CONTAINER_CREATED, - LIBRARY_CONTAINER_DELETED, - LIBRARY_CONTAINER_UPDATED, -) +from openedx_content.models_api import Container +from openedx_events.content_authoring.data import LibraryContainerData +from openedx_events.content_authoring.signals import LIBRARY_CONTAINER_DELETED from .. import tasks from ..models import ContentLibrary @@ -110,7 +104,7 @@ def create_container( # Automatically generate a slug. Append a random suffix so it should be unique. slug = slugify(title, allow_unicode=True) + "-" + uuid4().hex[-6:] # Make sure the slug is valid by first creating a key for the new container: - container_key = LibraryContainerLocator( + _container_key = LibraryContainerLocator( library_key, container_type=container_cls.type_code, container_id=slug, @@ -130,14 +124,6 @@ def create_container( created_by=user_id, ) - # .. event_implemented_name: LIBRARY_CONTAINER_CREATED - # .. event_type: org.openedx.content_authoring.content_library.container.created.v1 - transaction.on_commit(lambda: LIBRARY_CONTAINER_CREATED.send_event( - library_container=LibraryContainerData( - container_key=container_key, - ) - )) - return ContainerMetadata.from_container(library_key, container) @@ -153,9 +139,6 @@ def update_container( library_key = container_key.lib_key created = datetime.now(tz=timezone.utc) # noqa: UP017 - # Get children containers or components to update their index data - children = get_container_children(container_key, published=False) - version = content_api.create_next_container_version( container, title=display_name, @@ -163,34 +146,6 @@ def update_container( created_by=user_id, ) - # Send event related to the updated container - # .. event_implemented_name: LIBRARY_CONTAINER_UPDATED - # .. event_type: org.openedx.content_authoring.content_library.container.updated.v1 - LIBRARY_CONTAINER_UPDATED.send_event(library_container=LibraryContainerData(container_key=container_key)) - - # Send events related to the containers that contains the updated container. - # This is to update the children display names used in the section/subsection previews. - affected_containers = get_containers_contains_item(container_key) - for affected_container in affected_containers: - # .. event_implemented_name: LIBRARY_CONTAINER_UPDATED - # .. event_type: org.openedx.content_authoring.content_library.container.updated.v1 - LIBRARY_CONTAINER_UPDATED.send_event( - library_container=LibraryContainerData(container_key=affected_container.container_key) - ) - # Update children components and containers index data, for example, - # All subsections under a section have section key in index that needs to be updated. - # So if parent section name has been changed, it needs to be reflected in sections key of children - is_unit = container_key.container_type == Unit.type_code - for child in children: - # .. event_implemented_name: CONTENT_OBJECT_ASSOCIATIONS_CHANGED - # .. event_type: org.openedx.content_authoring.content.object.associations.changed.v1 - CONTENT_OBJECT_ASSOCIATIONS_CHANGED.send_event( - content_object=ContentObjectChangedData( - object_id=str(child.usage_key if is_unit else child.container_key), # type: ignore[union-attr] - changes=[container_key.container_type + "s"], # e.g. "units" - ), - ) - return ContainerMetadata.from_container(library_key, version.container) @@ -202,15 +157,6 @@ def delete_container( No-op if container doesn't exist or has already been soft-deleted. """ - def send_container_deleted_signal(): - # .. event_implemented_name: LIBRARY_CONTAINER_DELETED - # .. event_type: org.openedx.content_authoring.content_library.container.deleted.v1 - LIBRARY_CONTAINER_DELETED.send_event( - library_container=LibraryContainerData( - container_key=container_key, - ) - ) - try: container = get_container_from_key(container_key) except Container.DoesNotExist: @@ -219,105 +165,20 @@ def send_container_deleted_signal(): # (an intermediate error occurred). # In that case, we keep the index updated by removing the entry, # but still raise the error so the caller knows the container did not exist. - send_container_deleted_signal() + # .. event_implemented_name: LIBRARY_CONTAINER_DELETED + # .. event_type: org.openedx.content_authoring.content_library.container.deleted.v1 + LIBRARY_CONTAINER_DELETED.send_event(library_container=LibraryContainerData(container_key=container_key)) raise - affected_containers = get_containers_contains_item(container_key) - # Get children containers or components to update their index data - children = get_container_children( - container_key, - published=False, - ) content_api.soft_delete_draft(container.id) - send_container_deleted_signal() - - # Send events related to the containers that contains the updated container. - # This is to update the children display names used in the section/subsection previews. - for affected_container in affected_containers: - # .. event_implemented_name: LIBRARY_CONTAINER_UPDATED - # .. event_type: org.openedx.content_authoring.content_library.container.updated.v1 - LIBRARY_CONTAINER_UPDATED.send_event( - library_container=LibraryContainerData( - container_key=affected_container.container_key, - ) - ) - key_name = "container_key" - if isinstance(container, Unit): - # Components have usage_key instead of container_key - key_name = "usage_key" - # Update children components and containers index data, for example, - # All subsections under a section have section key in index that needs to be updated. - # So if parent section is deleted, it needs to be removed from sections key of children - for child in children: - # .. event_implemented_name: CONTENT_OBJECT_ASSOCIATIONS_CHANGED - # .. event_type: org.openedx.content_authoring.content.object.associations.changed.v1 - CONTENT_OBJECT_ASSOCIATIONS_CHANGED.send_event( - content_object=ContentObjectChangedData( - object_id=str(getattr(child, key_name)), - changes=[container_key.container_type + "s"], - ), - ) - def restore_container(container_key: LibraryContainerLocator) -> None: """ [ 🛑 UNSTABLE ] Restore the specified library container. """ container = get_container_from_key(container_key, include_deleted=True) - content_api.set_draft_version(container.id, container.versioning.latest.pk) - # Fetch related containers after restore - affected_containers = get_containers_contains_item(container_key) - # Get children containers or components to update their index data - children = get_container_children(container_key, published=False) - - # .. event_implemented_name: LIBRARY_CONTAINER_CREATED - # .. event_type: org.openedx.content_authoring.content_library.container.created.v1 - LIBRARY_CONTAINER_CREATED.send_event( - library_container=LibraryContainerData( - container_key=container_key, - ) - ) - - content_changes = ["collections", "tags"] - if affected_containers and len(affected_containers) > 0: - # Update parent key data in index. Eg. `sections` key in index for subsection - content_changes.append(str(affected_containers[0].container_type_code) + "s") - # Add tags, collections and parent data back to index - # .. event_implemented_name: CONTENT_OBJECT_ASSOCIATIONS_CHANGED - # .. event_type: org.openedx.content_authoring.content.object.associations.changed.v1 - CONTENT_OBJECT_ASSOCIATIONS_CHANGED.send_event( - content_object=ContentObjectChangedData( - object_id=str(container_key), - changes=content_changes, - ), - ) - - # Send events related to the containers that contains the updated container. - # This is to update the children display names used in the section/subsection previews. - for affected_container in affected_containers: - # .. event_implemented_name: LIBRARY_CONTAINER_UPDATED - # .. event_type: org.openedx.content_authoring.content_library.container.updated.v1 - LIBRARY_CONTAINER_UPDATED.send_event( - library_container=LibraryContainerData( - container_key=affected_container.container_key, - ) - ) - - is_unit = container_key.container_type == Unit.type_code - # Update children components and containers index data, for example, - # All subsections under a section have section key in index that needs to be updated. - # Should restore removed parent section in sections key of children subsections - for child in children: - # .. event_implemented_name: CONTENT_OBJECT_ASSOCIATIONS_CHANGED - # .. event_type: org.openedx.content_authoring.content.object.associations.changed.v1 - CONTENT_OBJECT_ASSOCIATIONS_CHANGED.send_event( - content_object=ContentObjectChangedData( - object_id=str(child.usage_key if is_unit else child.container_key), # type: ignore[union-attr] - changes=[container_key.container_type + "s"], - ), - ) def get_container_children( @@ -372,23 +233,6 @@ def update_container_children( entities=[get_entity_from_key(key) for key in children_keys], entities_action=entities_action, ) - for key in children_keys: - # .. event_implemented_name: CONTENT_OBJECT_ASSOCIATIONS_CHANGED - # .. event_type: org.openedx.content_authoring.content.object.associations.changed.v1 - CONTENT_OBJECT_ASSOCIATIONS_CHANGED.send_event( - content_object=ContentObjectChangedData( - object_id=str(key), - changes=[f"{container_key.container_type}s"], # "units", "subsections", "sections" - ), - ) - - # .. event_implemented_name: LIBRARY_CONTAINER_UPDATED - # .. event_type: org.openedx.content_authoring.content_library.container.updated.v1 - LIBRARY_CONTAINER_UPDATED.send_event( - library_container=LibraryContainerData( - container_key=container_key, - ) - ) return ContainerMetadata.from_container(container_key.lib_key, new_version.container) diff --git a/openedx/core/djangoapps/content_libraries/api/libraries.py b/openedx/core/djangoapps/content_libraries/api/libraries.py index 68d4258bb065..f61d8b90c835 100644 --- a/openedx/core/djangoapps/content_libraries/api/libraries.py +++ b/openedx/core/djangoapps/content_libraries/api/libraries.py @@ -776,12 +776,9 @@ def revert_changes(library_key: LibraryLocatorV2, user_id: int | None = None) -> """ learning_package = ContentLibrary.objects.get_by_key(library_key).learning_package assert learning_package is not None # shouldn't happen but it's technically possible. - with content_api.bulk_draft_changes_for(learning_package.id) as draft_change_log: + with content_api.bulk_draft_changes_for(learning_package.id): content_api.reset_drafts_to_published(learning_package.id, reset_by=user_id) - # Call the event handlers as needed. - tasks.wait_for_post_revert_events(draft_change_log, library_key) - def get_backup_task_status( user_id: int, diff --git a/openedx/core/djangoapps/content_libraries/library_context.py b/openedx/core/djangoapps/content_libraries/library_context.py index 5d2c25c4286e..7cba96e0da94 100644 --- a/openedx/core/djangoapps/content_libraries/library_context.py +++ b/openedx/core/djangoapps/content_libraries/library_context.py @@ -7,8 +7,6 @@ from opaque_keys.edx.keys import UsageKeyV2 from opaque_keys.edx.locator import LibraryLocatorV2, LibraryUsageLocatorV2 from openedx_content import api as content_api -from openedx_events.content_authoring.data import LibraryBlockData, LibraryContainerData -from openedx_events.content_authoring.signals import LIBRARY_BLOCK_UPDATED, LIBRARY_CONTAINER_UPDATED from rest_framework.exceptions import NotFound from openedx.core.djangoapps.content_libraries import api, permissions @@ -101,34 +99,3 @@ def block_exists(self, usage_key: LibraryUsageLocatorV2): type_name=usage_key.block_type, component_code=usage_key.block_id, ) - - def send_block_updated_event(self, usage_key: UsageKeyV2): - """ - Send a "block updated" event for the library block with the given usage_key. - """ - assert isinstance(usage_key, LibraryUsageLocatorV2) - # .. event_implemented_name: LIBRARY_BLOCK_UPDATED - # .. event_type: org.openedx.content_authoring.library_block.updated.v1 - LIBRARY_BLOCK_UPDATED.send_event( - library_block=LibraryBlockData( - library_key=usage_key.lib_key, - usage_key=usage_key, - ) - ) - - def send_container_updated_events(self, usage_key: UsageKeyV2): - """ - Send "container updated" events for containers that contains the library block - with the given usage_key. - """ - assert isinstance(usage_key, LibraryUsageLocatorV2) - affected_containers = api.get_containers_contains_item(usage_key) - for container in affected_containers: - # .. event_implemented_name: LIBRARY_CONTAINER_UPDATED - # .. event_type: org.openedx.content_authoring.content_library.container.updated.v1 - LIBRARY_CONTAINER_UPDATED.send_event( - library_container=LibraryContainerData( - container_key=container.container_key, - background=True, - ) - ) diff --git a/openedx/core/djangoapps/content_libraries/signal_handlers.py b/openedx/core/djangoapps/content_libraries/signal_handlers.py index 48221ddfd304..15e029d54b87 100644 --- a/openedx/core/djangoapps/content_libraries/signal_handlers.py +++ b/openedx/core/djangoapps/content_libraries/signal_handlers.py @@ -4,6 +4,7 @@ import logging +from attrs import asdict from django.dispatch import receiver from openedx_content.api import signals as content_signals from openedx_events.content_authoring.data import LibraryCollectionData @@ -20,12 +21,47 @@ log = logging.getLogger(__name__) +@receiver(content_signals.LEARNING_PACKAGE_ENTITIES_CHANGED) +def entities_updated( + learning_package: content_signals.LearningPackageEventData, + change_log: content_signals.DraftChangeLogEventData, + **kwargs, +) -> None: + """ + Entities (containers/components) have been changed - handle that as needed. + + We receive this low-level event from `openedx_content`, and check if it + happened in a library. If so, we emit more detailed library-specific events. + + 💾 This event is only received after the transaction has committed. + ⏳ This event is emitted synchronously and this handler is called + synchronously. If a lot of entities were changed, we need to dispatch an + asynchronous handler to deal with them to avoid slowdowns. If only one + entity is changed, we want to deal with that synchronously so that we + can show the user correct data when the current requests completes. + """ + try: + ContentLibrary.objects.get(learning_package_id=learning_package.id) + except ContentLibrary.DoesNotExist: + return # We don't care about non-library events. + + entities_changed = [change.entity_id for change in change_log.changes] + + if len(entities_changed) == 1: + fn = tasks.send_change_events_for_modified_entities + else: + # More than one entity was changed at once. Handle asynchronously: + fn = tasks.send_change_events_for_modified_entities.delay + + fn(learning_package_id=learning_package.id, change_list=[asdict(chg) for chg in change_log.changes]) + + @receiver(content_signals.LEARNING_PACKAGE_COLLECTION_CHANGED) def collection_updated( learning_package: content_signals.LearningPackageEventData, change: content_signals.CollectionChangeData, **kwargs, -): +) -> None: """ A Collection has been updated - handle that as needed. diff --git a/openedx/core/djangoapps/content_libraries/tasks.py b/openedx/core/djangoapps/content_libraries/tasks.py index 814624633c85..0799ace7c4d7 100644 --- a/openedx/core/djangoapps/content_libraries/tasks.py +++ b/openedx/core/djangoapps/content_libraries/tasks.py @@ -47,7 +47,7 @@ ) from openedx_content import api as content_api from openedx_content.api import create_zip_file as create_lib_zip_file -from openedx_content.models_api import DraftChangeLog, LearningPackage, PublishableEntity, PublishLog +from openedx_content.models_api import LearningPackage, PublishableEntity, PublishLog from openedx_events.content_authoring.data import ( ContentObjectChangedData, LibraryBlockData, @@ -79,6 +79,7 @@ from xmodule.modulestore.mixed import MixedModuleStore from . import api +from .models import ContentLibrary log = logging.getLogger(__name__) TASK_LOGGER = get_task_logger(__name__) @@ -88,6 +89,170 @@ DATETIME_FORMAT = '%Y-%m-%dT%H:%M:%SZ' # Should match serializer format. Redefined to avoid circular import. +@shared_task(base=LoggedTask) +@set_code_owner_attribute +def send_change_events_for_modified_entities( + learning_package_id: LearningPackage.ID, + change_list: list[dict], # we want list[ChangeLogRecordData], but that's not JSON serializable, so use dicts +): + """ + Sends a various library-specific events for each modified library entity in + the given change log, after any kind of edit was made in the library. This + could be in response to an entity (component or container) being created, + modified, deleted, un-deleted, or one of its dependencies doing those + things. + + ⏳ This task is designed to be run asynchronously so it can handle many + entities, but you can also call it synchronously if you are only + processing a single entity. Handlers of the events that we emit here + should be synchronous and fast, to support the "update one item + synchronously" use case, but can be async if needed. + """ + changes = [content_api.signals.ChangeLogRecordData(**r) for r in change_list] + library = ContentLibrary.objects.get(learning_package_id=learning_package_id) + changes_by_entity_id = {change.entity_id: change for change in changes} + entities = ( + content_api.get_publishable_entities(learning_package_id) + .filter(id__in=changes_by_entity_id.keys()) + .select_related("component", "container") + ) + + for entity in entities: + change = changes_by_entity_id[entity.id] + if hasattr(entity, "component"): + # This is a library XBlock (component) + block_key = api.library_component_usage_key(library.library_key, entity.component) + event_data = LibraryBlockData(library_key=library.library_key, usage_key=block_key) + if change.old_version is None and change.new_version: + # .. event_implemented_name: LIBRARY_BLOCK_CREATED + # .. event_type: org.openedx.content_authoring.library_block.created.v1 + LIBRARY_BLOCK_CREATED.send_event(library_block=event_data) + elif change.old_version and change.new_version is None: + # .. event_implemented_name: LIBRARY_BLOCK_DELETED + # .. event_type: org.openedx.content_authoring.library_block.deleted.v1 + LIBRARY_BLOCK_DELETED.send_event(library_block=event_data) + else: + # If the version numbers are different, this block was modified. + # If not, it was included as a side effect of some other change, like renaming a parent container. + # .. event_implemented_name: LIBRARY_BLOCK_UPDATED + # .. event_type: org.openedx.content_authoring.library_block.updated.v1 + LIBRARY_BLOCK_UPDATED.send_event(library_block=event_data) + + elif hasattr(entity, "container"): + container_key = api.library_container_locator(library.library_key, entity.container) + event_data = LibraryContainerData(container_key=container_key) + if change.old_version is None and change.new_version: + # .. event_implemented_name: LIBRARY_CONTAINER_CREATED + # .. event_type: org.openedx.content_authoring.content_library.container.created.v1 + LIBRARY_CONTAINER_CREATED.send_event(library_container=event_data) + elif change.old_version and change.new_version is None: + # .. event_implemented_name: LIBRARY_CONTAINER_DELETED + # .. event_type: org.openedx.content_authoring.content_library.container.deleted.v1 + LIBRARY_CONTAINER_DELETED.send_event(library_container=event_data) + else: + # If the version numbers are different, this container was modified. + # If not, it was included as a side effect of some other change, like its child being modified. + container_itself_changed = change.old_version != change.new_version + + # .. event_implemented_name: LIBRARY_CONTAINER_UPDATED + # .. event_type: org.openedx.content_authoring.content_library.container.updated.v1 + LIBRARY_CONTAINER_UPDATED.send_event(library_container=event_data) + # TODO: to optimze this, once we have https://github.com/openedx/openedx-events/pull/570 merged, + # change the above event to use `send_async=not container_itself_changed`, so that direct changes are + # processed immediately but side effects can happen async. + + if container_itself_changed: + # If entities were added/removed from this container, we need to notify things like the search index + # that the list of parent containers for each entity has changed. + check_container_content_changes.delay( + container_key_str=str(container_key), + old_version_id=change.old_version_id, + new_version_id=change.new_version_id, + ) + else: + log.error("Unknown publishable entity type: %s", entity) + continue + + +@shared_task(base=LoggedTask) +@set_code_owner_attribute +def check_container_content_changes( + container_key_str: str, + old_version_id: int | None, + new_version_id: int | None, +): + """ + Whenever a container is edited, we need to check if child entities were + added or removed, and if so send out a CONTENT_OBJECT_ASSOCIATIONS_CHANGED + event for each added/removed child. + + For example, removing an entity from a unit should result in:: + + CONTENT_OBJECT_ASSOCIATIONS_CHANGED( + object_id=..., + changes=["units"], + ) + + ⏳ This task is always run asynchronously. + """ + if old_version_id == new_version_id: + return # Same versions + + if old_version_id: + old_version = content_api.get_container_version(old_version_id) + old_entity_list_id = old_version.entity_list_id + else: + old_entity_list_id = None + if new_version_id: + new_version = content_api.get_container_version(new_version_id) if new_version_id else None + new_entity_list_id = new_version.entity_list_id + else: + new_entity_list_id = None + + # If the title has changed, we notify ALL children that their parent container(s) have changed, e.g. to update the + # list of "units this component is used in", "sections this subsection is used in", etc. in the search index + title_changed: bool = old_version and new_version and old_version.title != new_version.title + if title_changed: + # TODO: there is no "get entity list for container version" API in openedx_content + new_child_ids = new_version.entity_list.entitylistrow_set.values_list("entity_id", flat=True) + old_child_ids = old_version.entity_list.entitylistrow_set.values_list("entity_id", flat=True) + # notify ALL current children, plus any deleted children, that their parent container(s) changed + changed_child_ids = list(set(new_child_ids) | (set(old_child_ids) - set(new_child_ids))) + elif old_entity_list_id == new_entity_list_id: + # Different container versions but same list of child entities. For now we don't need to do anything, but in the + # future if we have some other kind of per-container settings relevant to child entities we might need to handle + # this the same way as title_changed. + return + else: + # TODO: there is no "get entity list for container version" API in openedx_content + old_child_ids = old_version.entity_list.entitylistrow_set.values_list("entity_id", flat=True) if old_version else [] + new_child_ids = new_version.entity_list.entitylistrow_set.values_list("entity_id", flat=True) if new_version else [] + # We only need to notify any added or removed children that their parent container(s) changed: + changed_child_ids = list(set(old_child_ids) ^ set(new_child_ids)) + + container_key = LibraryContainerLocator.from_string(container_key_str) + library = ContentLibrary.objects.get_by_key(container_key.lib_key) + entities = ( + content_api.get_publishable_entities(library.learning_package_id) + .filter(id__in=changed_child_ids) + .select_related("component", "container") + ) + for entity in entities: + if hasattr(entity, "component"): + child_key = api.library_component_usage_key(library.library_key, entity.component) + elif hasattr(entity, "container"): + child_key = api.library_container_locator(library.library_key, entity.container) + else: + log.error("Unknown publishable entity type: %s", entity) + continue + CONTENT_OBJECT_ASSOCIATIONS_CHANGED.send_event( + content_object=ContentObjectChangedData( + object_id=str(child_key), + changes=[container_key.container_type + "s"], # e.g. "units" + ), + ) + + @shared_task(base=LoggedTask) @set_code_owner_attribute def send_collections_changed_events( @@ -221,126 +386,6 @@ def wait_for_post_publish_events(publish_log: PublishLog, library_key: LibraryLo # the background by the celery worker until everything is updated. -@shared_task(base=LoggedTask) -@set_code_owner_attribute -def send_events_after_revert(draft_change_log_id: int, library_key_str: str) -> None: - """ - Send events to trigger actions like updating the search index, after we've - reverted some unpublished changes in a library. - - See notes on the analogous function above, send_events_after_publish. - """ - try: - draft_change_log = DraftChangeLog.objects.get(id=draft_change_log_id) - except DraftChangeLog.DoesNotExist: - # When a revert operation is a no-op, openedx_content deletes the empty - # DraftChangeLog, so we'll assume that's what happened here. - log.info(f"Library revert in {library_key_str} did not result in any changes.") - return - - library_key = LibraryLocatorV2.from_string(library_key_str) - affected_entities = draft_change_log.records.select_related( - "entity", "entity__container", "entity__component", - ).all() - - created_container_keys: set[LibraryContainerLocator] = set() - updated_container_keys: set[LibraryContainerLocator] = set() - deleted_container_keys: set[LibraryContainerLocator] = set() - - # Update anything that needs to be updated (e.g. search index): - for record in affected_entities: - # This will be true if the entity was [soft] deleted, but we're now reverting that deletion: - is_undeleted = (record.old_version is None and record.new_version is not None) - # This will be true if the entity was created and we're now deleting it by reverting that creation: - is_deleted = (record.old_version is not None and record.new_version is None) - if hasattr(record.entity, "component"): - usage_key = api.library_component_usage_key(library_key, record.entity.component) - event = LIBRARY_BLOCK_UPDATED - if is_deleted: - event = LIBRARY_BLOCK_DELETED - elif is_undeleted: - event = LIBRARY_BLOCK_CREATED - - # .. event_implemented_name: LIBRARY_BLOCK_UPDATED - # .. event_type: org.openedx.content_authoring.library_block.updated.v1 - - # .. event_implemented_name: LIBRARY_BLOCK_DELETED - # .. event_type: org.openedx.content_authoring.library_block.deleted.v1 - - # .. event_implemented_name: LIBRARY_BLOCK_CREATED - # .. event_type: org.openedx.content_authoring.library_block.created.v1 - event.send_event(library_block=LibraryBlockData(library_key=library_key, usage_key=usage_key)) - # If any containers contain this component, their child list / component count may need to be updated - # e.g. if this was a newly created component in the container and is now deleted, or this was deleted and - # is now restored. - # TODO: we should be able to rewrite this to use the "side effects" functionality of the publishing API. - try: - for parent_container in api.get_containers_contains_item(usage_key): - updated_container_keys.add(parent_container.container_key) - except api.ContentLibraryBlockNotFound: - pass # The item 'usage_key' has been deleted. But shouldn't we still handle that? - - # TODO: do we also need to send CONTENT_OBJECT_ASSOCIATIONS_CHANGED for this component, or is - # LIBRARY_BLOCK_UPDATED sufficient? - elif hasattr(record.entity, "container"): - container_key = api.library_container_locator(library_key, record.entity.container) - if is_deleted: - deleted_container_keys.add(container_key) - elif is_undeleted: - created_container_keys.add(container_key) - else: - updated_container_keys.add(container_key) - else: - log.warning( - f"PublishableEntity {record.entity.pk} / {record.entity.entity_ref} " - "was modified during publish operation but is of unknown type." - ) - - for container_key in deleted_container_keys: - # .. event_implemented_name: LIBRARY_CONTAINER_DELETED - # .. event_type: org.openedx.content_authoring.content_library.container.deleted.v1 - LIBRARY_CONTAINER_DELETED.send_event( - library_container=LibraryContainerData(container_key=container_key) - ) - # Don't bother sending UPDATED events for these containers that are now deleted - created_container_keys.discard(container_key) - - for container_key in created_container_keys: - # .. event_implemented_name: LIBRARY_CONTAINER_CREATED - # .. event_type: org.openedx.content_authoring.content_library.container.created.v1 - LIBRARY_CONTAINER_CREATED.send_event( - library_container=LibraryContainerData(container_key=container_key) - ) - - for container_key in updated_container_keys: - # .. event_implemented_name: LIBRARY_CONTAINER_UPDATED - # .. event_type: org.openedx.content_authoring.content_library.container.updated.v1 - LIBRARY_CONTAINER_UPDATED.send_event( - library_container=LibraryContainerData(container_key=container_key) - ) - - -def wait_for_post_revert_events(draft_change_log: DraftChangeLog, library_key: LibraryLocatorV2): - """ - After discard all changes in a library, trigger the required event handlers - (e.g. update the search index). Try to wait for that to complete before - returning, up to some reasonable timeout, and then finish anything remaining - asynchonrously. - """ - # Update the search index (and anything else) for the affected blocks - result = send_events_after_revert.apply_async(args=(draft_change_log.pk, str(library_key))) - # Try waiting a bit for those post-publish events to be handled: - try: - result.get(timeout=15) - except TimeoutError: - pass - # This is fine! The search index is still being updated, and/or other - # event handlers are still following up on the results, but the revert - # already *did* succeed, and the events will continue to be processed in - # the background by the celery worker until everything is updated. - - - def _filter_child(store, usage_key, capa_type): """ Return whether this block is both a problem and has a `capa_type` which is included in the filter. diff --git a/openedx/core/djangoapps/content_libraries/tests/test_events.py b/openedx/core/djangoapps/content_libraries/tests/test_events.py index d6b7306a8c31..58d2557208ba 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_events.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_events.py @@ -1,7 +1,6 @@ """ Tests for openedx_content-based Content Libraries """ - from unittest import mock from django.db import transaction @@ -115,11 +114,23 @@ def expect_new_events(self, *expected_events: dict) -> None: found = True break if not found: - raise AssertionError(f"Event {expected} not found among actual events: {self.new_events}") + raise AssertionError(f"Event {expected} not found among actual events:\n{self.new_events_str}") if len(self.new_events) > 0: - raise AssertionError(f"Events were emitted but not expected: {self.new_events}") + raise AssertionError(f"Events were emitted but not expected:\n{self.new_events_str}") self.clear_events() + @property + def new_events_str(self) -> str: + """Friendly-ish string representation of self.new_events""" + simplified_events = [e.copy() for e in self.new_events] + for e in simplified_events: + if e["sender"] is None: + del e["sender"] + if e["from_event_bus"] is False: + del e["from_event_bus"] + del e["metadata"] + return "\n".join([str(e) for e in simplified_events]) + @skip_unless_cms class ContentLibrariesEventsTestCase(BaseEventsTestCase): @@ -543,13 +554,10 @@ def test_restore_unit(self) -> None: "signal": LIBRARY_CONTAINER_CREATED, "library_container": LibraryContainerData(container_key), }, - { - "signal": CONTENT_OBJECT_ASSOCIATIONS_CHANGED, - "content_object": ContentObjectChangedData( - object_id=str(container_key), - changes=["collections", "tags"], - ), - }, + # We used to emit CONTENT_OBJECT_ASSOCIATIONS_CHANGED here for the restored container, specifically noting + # that changes=["collections", "tags"], because deleted things may have collections+tags that are once + # again relevant when it is restored. However, the CREATED event should be sufficient for notifying of that. + # (Or should we emit CREATED+UPDATED to be extra sure?) ) def test_restore_unit_via_revert(self) -> None: @@ -624,6 +632,22 @@ class ContentLibraryContainerEventsTest(BaseEventsTestCase): """ Event tests for container operations: signals emitted when components and containers are created, updated, deleted, and associated with one another. + + setUp() builds the following structure in lib1 (note that some entities + are shared across multiple parents, so this is a DAG, not a strict tree):: + + Section 1 Section 2 + ├── Subsection 1 ◄───────── (shared) ────────┴── Subsection 1 + │ ├── Unit 1 ◄────────────────┐ (shared) + │ │ ├── problem1 │ + │ │ └── html1 ◄──┐ │ + │ └── Unit 2 │(shared) │ + │ └── html1 ◄──┘ │ + └── Subsection 2 │ + └── Unit 1 ◄────────────────┘ + + Orphans (created but not attached to any parent): + Unit 3, problem2 """ def setUp(self) -> None: @@ -689,21 +713,38 @@ def setUp(self) -> None: def test_container_updated_when_component_deleted(self) -> None: api.delete_library_block(self.html_block_usage_key) self.expect_new_events( + # The block itself was deleted: { "signal": LIBRARY_BLOCK_DELETED, "library_block": LibraryBlockData(self.lib1_key, self.html_block_usage_key), }, + # That block was a child of two units, so both parent units are flagged as updated + # e.g. to update their "child_display_names" in the search index. { "signal": LIBRARY_CONTAINER_UPDATED, - "library_container": LibraryContainerData( - container_key=self.unit1.container_key, background=True, - ), + "library_container": LibraryContainerData(container_key=self.unit1.container_key), }, { "signal": LIBRARY_CONTAINER_UPDATED, - "library_container": LibraryContainerData( - container_key=self.unit2.container_key, background=True, - ), + "library_container": LibraryContainerData(container_key=self.unit2.container_key), + }, + # openedx_content also lists ancestor containers of the affected units as changed. + # We don't strictly need this at the moment, at least as far as keeping our search index updated. + { + "signal": LIBRARY_CONTAINER_UPDATED, + "library_container": LibraryContainerData(container_key=self.subsection1.container_key), + }, + { + "signal": LIBRARY_CONTAINER_UPDATED, + "library_container": LibraryContainerData(container_key=self.subsection2.container_key), + }, + { + "signal": LIBRARY_CONTAINER_UPDATED, + "library_container": LibraryContainerData(container_key=self.section1.container_key), + }, + { + "signal": LIBRARY_CONTAINER_UPDATED, + "library_container": LibraryContainerData(container_key=self.section2.container_key), }, ) @@ -713,49 +754,79 @@ def test_container_updated_when_component_restored(self) -> None: api.restore_library_block(self.html_block_usage_key) self.expect_new_events( + # Restoring the block re-creates it: { "signal": LIBRARY_BLOCK_CREATED, "library_block": LibraryBlockData(self.lib1_key, self.html_block_usage_key), }, + # We used to emit CONTENT_OBJECT_ASSOCIATIONS_CHANGED here for the restored block, specifically noting + # that changes=["collections", "tags", "units"], because deleted things may have collections+tags+containers + # that are once again relevant when it is restored. However, the CREATED event should be sufficient for + # notifying of that. (Or should we emit CREATED+UPDATED to be extra sure?) + # The restored block is a child of two units, so both parent units are flagged as updated: { - "signal": CONTENT_OBJECT_ASSOCIATIONS_CHANGED, - "content_object": ContentObjectChangedData( - object_id=str(self.html_block_usage_key), - changes=["collections", "tags", "units"], - ), + "signal": LIBRARY_CONTAINER_UPDATED, + "library_container": LibraryContainerData(container_key=self.unit1.container_key), }, { "signal": LIBRARY_CONTAINER_UPDATED, - "library_container": LibraryContainerData( - container_key=self.unit1.container_key, background=True, - ), + "library_container": LibraryContainerData(container_key=self.unit2.container_key), }, + # openedx_content also lists ancestor containers of the affected units as changed. + # We don't strictly need this at the moment, at least as far as keeping our search index updated. { "signal": LIBRARY_CONTAINER_UPDATED, - "library_container": LibraryContainerData( - container_key=self.unit2.container_key, background=True, - ), + "library_container": LibraryContainerData(container_key=self.subsection1.container_key), + }, + { + "signal": LIBRARY_CONTAINER_UPDATED, + "library_container": LibraryContainerData(container_key=self.subsection2.container_key), + }, + { + "signal": LIBRARY_CONTAINER_UPDATED, + "library_container": LibraryContainerData(container_key=self.section1.container_key), + }, + { + "signal": LIBRARY_CONTAINER_UPDATED, + "library_container": LibraryContainerData(container_key=self.section2.container_key), }, ) def test_container_updated_when_component_olx_updated(self) -> None: self._set_library_block_olx(self.html_block_usage_key, "Hello world!") self.expect_new_events( + # The block's OLX changed: { "signal": LIBRARY_BLOCK_UPDATED, "library_block": LibraryBlockData(self.lib1_key, self.html_block_usage_key), }, + # That block is used in two units, so both parent units are flagged as updated + # e.g. to update their "child_display_names" in the search index, if the child's name has changed. { "signal": LIBRARY_CONTAINER_UPDATED, - "library_container": LibraryContainerData( - container_key=self.unit1.container_key, background=True, - ), + "library_container": LibraryContainerData(container_key=self.unit1.container_key), }, { "signal": LIBRARY_CONTAINER_UPDATED, - "library_container": LibraryContainerData( - container_key=self.unit2.container_key, background=True, - ), + "library_container": LibraryContainerData(container_key=self.unit2.container_key), + }, + # openedx_content also lists ancestor containers of the affected units as changed. + # We don't strictly need this at the moment, at least as far as keeping our search index updated. + { + "signal": LIBRARY_CONTAINER_UPDATED, + "library_container": LibraryContainerData(container_key=self.subsection1.container_key), + }, + { + "signal": LIBRARY_CONTAINER_UPDATED, + "library_container": LibraryContainerData(container_key=self.subsection2.container_key), + }, + { + "signal": LIBRARY_CONTAINER_UPDATED, + "library_container": LibraryContainerData(container_key=self.section1.container_key), + }, + { + "signal": LIBRARY_CONTAINER_UPDATED, + "library_container": LibraryContainerData(container_key=self.section2.container_key), }, ) @@ -767,17 +838,33 @@ def test_container_updated_when_component_fields_updated(self) -> None: "signal": LIBRARY_BLOCK_UPDATED, "library_block": LibraryBlockData(self.lib1_key, self.html_block_usage_key), }, + # That block is used in two containers, so we expect events for them too: + # This is used e.g. to update "child_display_names" in the search index, if the child's name has changed. { "signal": LIBRARY_CONTAINER_UPDATED, - "library_container": LibraryContainerData( - container_key=self.unit1.container_key, background=True, - ), + "library_container": LibraryContainerData(container_key=self.unit1.container_key), }, { "signal": LIBRARY_CONTAINER_UPDATED, - "library_container": LibraryContainerData( - container_key=self.unit2.container_key, background=True, - ), + "library_container": LibraryContainerData(container_key=self.unit2.container_key), + }, + # openedx_content also lists and parent containers of affected containers as changed, and so on... + # We don't strictly need this at the moment, at least as far as keeping our search index updated. + { + "signal": LIBRARY_CONTAINER_UPDATED, + "library_container": LibraryContainerData(container_key=self.subsection1.container_key), + }, + { + "signal": LIBRARY_CONTAINER_UPDATED, + "library_container": LibraryContainerData(container_key=self.subsection2.container_key), + }, + { + "signal": LIBRARY_CONTAINER_UPDATED, + "library_container": LibraryContainerData(container_key=self.section1.container_key), + }, + { + "signal": LIBRARY_CONTAINER_UPDATED, + "library_container": LibraryContainerData(container_key=self.section2.container_key), }, ) @@ -786,10 +873,13 @@ def test_container_updated_when_component_fields_updated(self) -> None: def test_container_updated_when_unit_updated(self) -> None: self._update_container(self.unit1.container_key, 'New Unit Display Name') self.expect_new_events( + # We renamed this unit, so we get an UPDATED event for it: { "signal": LIBRARY_CONTAINER_UPDATED, "library_container": LibraryContainerData(container_key=self.unit1.container_key), }, + # We also get events for its parent containers + # e.g. to update their "child_display_names" in the search index. { "signal": LIBRARY_CONTAINER_UPDATED, "library_container": LibraryContainerData(container_key=self.subsection1.container_key), @@ -798,6 +888,18 @@ def test_container_updated_when_unit_updated(self) -> None: "signal": LIBRARY_CONTAINER_UPDATED, "library_container": LibraryContainerData(container_key=self.subsection2.container_key), }, + # openedx_content also lists ancestor containers of the affected unit as changed. + # We don't strictly need this at the moment, at least as far as keeping our search index updated. + { + "signal": LIBRARY_CONTAINER_UPDATED, + "library_container": LibraryContainerData(container_key=self.section1.container_key), + }, + { + "signal": LIBRARY_CONTAINER_UPDATED, + "library_container": LibraryContainerData(container_key=self.section2.container_key), + }, + # Finally, any child components receive a "units changed" notification + # e.g. to update the "units this component is used in" in the search index. { "signal": CONTENT_OBJECT_ASSOCIATIONS_CHANGED, "content_object": ContentObjectChangedData( @@ -815,10 +917,13 @@ def test_container_updated_when_unit_updated(self) -> None: def test_container_updated_when_subsection_updated(self) -> None: self._update_container(self.subsection1.container_key, 'New Subsection Display Name') self.expect_new_events( + # We renamed this container, so we get an UPDATED event for it: { "signal": LIBRARY_CONTAINER_UPDATED, "library_container": LibraryContainerData(container_key=self.subsection1.container_key), }, + # We also get events for its parent containers + # e.g. to update their "child_display_names" in the search index { "signal": LIBRARY_CONTAINER_UPDATED, "library_container": LibraryContainerData(container_key=self.section1.container_key), @@ -827,6 +932,8 @@ def test_container_updated_when_subsection_updated(self) -> None: "signal": LIBRARY_CONTAINER_UPDATED, "library_container": LibraryContainerData(container_key=self.section2.container_key), }, + # Finally, any child containers receive a "subsections changed" notification + # e.g. to update the "subsections this unit is used in" in the search index. { "signal": CONTENT_OBJECT_ASSOCIATIONS_CHANGED, "content_object": ContentObjectChangedData( @@ -865,10 +972,10 @@ def test_container_updated_when_section_updated(self) -> None: ############################## Association change signals ################################## def test_associations_changed_when_component_removed(self) -> None: - html_block_1 = self._add_block_to_library(self.lib1_key, "html", "html3") + html_block_3 = self._add_block_to_library(self.lib1_key, "html", "html3") api.update_container_children( self.unit2.container_key, - [LibraryUsageLocatorV2.from_string(html_block_1["id"])], + [LibraryUsageLocatorV2.from_string(html_block_3["id"])], None, entities_action=content_api.ChildrenEntitiesAction.APPEND, ) @@ -876,7 +983,7 @@ def test_associations_changed_when_component_removed(self) -> None: api.update_container_children( self.unit2.container_key, - [LibraryUsageLocatorV2.from_string(html_block_1["id"])], + [LibraryUsageLocatorV2.from_string(html_block_3["id"])], None, entities_action=content_api.ChildrenEntitiesAction.REMOVE, ) @@ -884,13 +991,27 @@ def test_associations_changed_when_component_removed(self) -> None: { "signal": CONTENT_OBJECT_ASSOCIATIONS_CHANGED, "content_object": ContentObjectChangedData( - object_id=html_block_1["id"], changes=["units"], + object_id=html_block_3["id"], changes=["units"], ), }, { "signal": LIBRARY_CONTAINER_UPDATED, "library_container": LibraryContainerData(container_key=self.unit2.container_key), }, + # Because we removed html3 from unit2, the ancestor containers of unit2 are also emitted as changed. + # We don't strictly need this at the moment, at least as far as keeping our search index updated. + { + "signal": LIBRARY_CONTAINER_UPDATED, + "library_container": LibraryContainerData(container_key=self.subsection1.container_key), + }, + { + "signal": LIBRARY_CONTAINER_UPDATED, + "library_container": LibraryContainerData(container_key=self.section1.container_key), + }, + { + "signal": LIBRARY_CONTAINER_UPDATED, + "library_container": LibraryContainerData(container_key=self.section2.container_key), + }, ) def test_associations_changed_when_unit_removed(self) -> None: @@ -910,16 +1031,24 @@ def test_associations_changed_when_unit_removed(self) -> None: entities_action=content_api.ChildrenEntitiesAction.REMOVE, ) self.expect_new_events( + # unit4 was removed from subsection2, so we get a notification that "parent subsection(s) have changed": { "signal": CONTENT_OBJECT_ASSOCIATIONS_CHANGED, "content_object": ContentObjectChangedData( object_id=str(unit4.container_key), changes=["subsections"], ), }, + # We modified subsection2 by changing its list of children, so we get a CONTAINER_UPDATED event for it: { "signal": LIBRARY_CONTAINER_UPDATED, "library_container": LibraryContainerData(container_key=self.subsection2.container_key), }, + # Because subsection2 itself was changed, we get change notifications for its ancestors. + # We don't strictly need this at the moment, at least as far as keeping our search index updated. + { + "signal": LIBRARY_CONTAINER_UPDATED, + "library_container": LibraryContainerData(container_key=self.section1.container_key), + }, ) def test_associations_changed_when_subsection_removed(self) -> None: @@ -968,6 +1097,7 @@ def test_associations_changed_when_components_added(self) -> None: entities_action=content_api.ChildrenEntitiesAction.APPEND, ) self.expect_new_events( + # We added html4 and html4 to a new unit, so they get "parent unit(s) changed" events: { "signal": CONTENT_OBJECT_ASSOCIATIONS_CHANGED, "content_object": ContentObjectChangedData( @@ -980,13 +1110,29 @@ def test_associations_changed_when_components_added(self) -> None: object_id=html_block_2["id"], changes=["units"], ), }, + # We modified unit2 by changing its list of children, so we get a CONTAINER_UPDATED event for it: { "signal": LIBRARY_CONTAINER_UPDATED, "library_container": LibraryContainerData(container_key=self.unit2.container_key), }, + # Because the unit itself was changed, we get change notifications for its ancestors. + # We don't strictly need this at the moment, at least as far as keeping our search index updated. + { + "signal": LIBRARY_CONTAINER_UPDATED, + "library_container": LibraryContainerData(container_key=self.subsection1.container_key), + }, + { + "signal": LIBRARY_CONTAINER_UPDATED, + "library_container": LibraryContainerData(container_key=self.section1.container_key), + }, + { + "signal": LIBRARY_CONTAINER_UPDATED, + "library_container": LibraryContainerData(container_key=self.section2.container_key), + }, ) def test_associations_changed_when_units_added(self) -> None: + # Create "unit4" and "unit5" and add them to subsection2: unit4 = api.create_container(self.lib1_key, content_models.Unit, 'unit-4', 'Unit 4', None) unit5 = api.create_container(self.lib1_key, content_models.Unit, 'unit-5', 'Unit 5', None) self.clear_events() @@ -998,6 +1144,7 @@ def test_associations_changed_when_units_added(self) -> None: entities_action=content_api.ChildrenEntitiesAction.APPEND, ) self.expect_new_events( + # Each unit was added to a new subsection, so we get a "subsections changed" event for each: { "signal": CONTENT_OBJECT_ASSOCIATIONS_CHANGED, "content_object": ContentObjectChangedData( @@ -1010,10 +1157,16 @@ def test_associations_changed_when_units_added(self) -> None: object_id=str(unit5.container_key), changes=["subsections"], ), }, + # The subsection itself was updated (its list of children changed): { "signal": LIBRARY_CONTAINER_UPDATED, "library_container": LibraryContainerData(container_key=self.subsection2.container_key), }, + # And because the subsection itself was changed, we get change notifications for its ancestors. + { + "signal": LIBRARY_CONTAINER_UPDATED, + "library_container": LibraryContainerData(container_key=self.section1.container_key), + }, ) def test_associations_changed_when_subsections_added(self) -> None: @@ -1111,11 +1264,29 @@ def test_signal_emitted_when_set_block_olx_succeeds(self) -> None: usage_key=self.problem_block_usage_key, ), }, + # Since the problem is part of a unit, we also get LIBRARY_CONTAINER_UPDATED on the parent unit. + # This is used e.g. to update "child_display_names" in the search index, if the child's name has changed. { "signal": LIBRARY_CONTAINER_UPDATED, - "library_container": LibraryContainerData( - container_key=self.unit1.container_key, background=True, - ), + "library_container": LibraryContainerData(container_key=self.unit1.container_key), + }, + # openedx_content also lists and parent containers of affected containers as changed, and so on... + # We don't strictly need this at the moment, at least as far as keeping our search index updated. + { + "signal": LIBRARY_CONTAINER_UPDATED, + "library_container": LibraryContainerData(container_key=self.subsection1.container_key), + }, + { + "signal": LIBRARY_CONTAINER_UPDATED, + "library_container": LibraryContainerData(container_key=self.subsection2.container_key), + }, + { + "signal": LIBRARY_CONTAINER_UPDATED, + "library_container": LibraryContainerData(container_key=self.section1.container_key), + }, + { + "signal": LIBRARY_CONTAINER_UPDATED, + "library_container": LibraryContainerData(container_key=self.section2.container_key), }, ) diff --git a/openedx/core/djangoapps/xblock/learning_context/learning_context.py b/openedx/core/djangoapps/xblock/learning_context/learning_context.py index d3b4c8643a2f..22bd92701350 100644 --- a/openedx/core/djangoapps/xblock/learning_context/learning_context.py +++ b/openedx/core/djangoapps/xblock/learning_context/learning_context.py @@ -70,18 +70,3 @@ def definition_for_usage(self, usage_key, **kwargs): Retuns None if the usage key doesn't exist in this context. """ raise NotImplementedError - - def send_block_updated_event(self, usage_key): - """ - Send a "block updated" event for the block with the given usage_key in this context. - - usage_key: the UsageKeyV2 subclass used for this learning context - """ - - def send_container_updated_events(self, usage_key): - """ - Send "container updated" events for containers that contains the block with - the given usage_key in this context. - - usage_key: the UsageKeyV2 subclass used for this learning context - """ diff --git a/openedx/core/djangoapps/xblock/runtime/openedx_content_runtime.py b/openedx/core/djangoapps/xblock/runtime/openedx_content_runtime.py index 1da6f048b1cc..8c4996869324 100644 --- a/openedx/core/djangoapps/xblock/runtime/openedx_content_runtime.py +++ b/openedx/core/djangoapps/xblock/runtime/openedx_content_runtime.py @@ -312,11 +312,6 @@ def save_block(self, block): ) self.authored_data_store.mark_unchanged(block) - # Signal that we've modified this block - learning_context = get_learning_context_impl(usage_key) - learning_context.send_block_updated_event(usage_key) - learning_context.send_container_updated_events(usage_key) - def _get_component_from_usage_key(self, usage_key): """ Note that Components aren't ever really truly deleted, so this will From ed8c89557f2110c883313b3d772eaba53795d179 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Wed, 22 Apr 2026 19:27:07 -0700 Subject: [PATCH 12/15] feat: update content libraries API to use upstream entity published events --- cms/djangoapps/modulestore_migrator/tasks.py | 4 - .../content_libraries/api/blocks.py | 9 +- .../content_libraries/api/containers.py | 14 +-- .../content_libraries/api/libraries.py | 12 +- .../content_libraries/signal_handlers.py | 33 +++++ .../djangoapps/content_libraries/tasks.py | 66 +++------- .../content_libraries/tests/test_events.py | 118 ++++++++++++++++-- 7 files changed, 157 insertions(+), 99 deletions(-) diff --git a/cms/djangoapps/modulestore_migrator/tasks.py b/cms/djangoapps/modulestore_migrator/tasks.py index 15b25524b871..1c4736a3d76a 100644 --- a/cms/djangoapps/modulestore_migrator/tasks.py +++ b/cms/djangoapps/modulestore_migrator/tasks.py @@ -895,13 +895,9 @@ def _migrate_container( ).publishable_entity_version # Publish the container - # Call post publish events synchronously to avoid - # an error when calling `wait_for_post_publish_events` - # inside a celery task. libraries_api.publish_container_changes( container.container_key, context.created_by, - call_post_publish_events_sync=True, ) context.used_container_slugs.add(container.container_key.container_id) return container_publishable_entity_version, None diff --git a/openedx/core/djangoapps/content_libraries/api/blocks.py b/openedx/core/djangoapps/content_libraries/api/blocks.py index d10aed954016..7aada0ecdc90 100644 --- a/openedx/core/djangoapps/content_libraries/api/blocks.py +++ b/openedx/core/djangoapps/content_libraries/api/blocks.py @@ -37,7 +37,6 @@ ) from openedx.core.types import User as UserType -from .. import tasks from ..models import ContentLibrary from .block_metadata import LibraryXBlockMetadata, LibraryXBlockStaticFile from .container_metadata import container_subclass_for_olx_tag @@ -818,13 +817,7 @@ def publish_component_changes(usage_key: LibraryUsageLocatorV2, user_id: int): # The core publishing API is based on draft objects, so find the draft that corresponds to this component: drafts_to_publish = content_api.get_all_drafts(learning_package.id).filter(entity__entity_ref=component.entity_ref) # Publish the component and update anything that needs to be updated (e.g. search index): - publish_log = content_api.publish_from_drafts( - learning_package.id, draft_qset=drafts_to_publish, published_by=user_id, - ) - # Since this is a single component, it should be safe to process synchronously and in-process: - tasks.send_events_after_publish(publish_log.pk, str(library_key)) - # IF this is found to be a performance issue, we could instead make it async where necessary: - # tasks.wait_for_post_publish_events(publish_log, library_key=library_key) + content_api.publish_from_drafts(learning_package.id, draft_qset=drafts_to_publish, published_by=user_id) def _component_exists(usage_key: UsageKeyV2) -> bool: diff --git a/openedx/core/djangoapps/content_libraries/api/containers.py b/openedx/core/djangoapps/content_libraries/api/containers.py index 08a3dcd59c9c..92707bb860b5 100644 --- a/openedx/core/djangoapps/content_libraries/api/containers.py +++ b/openedx/core/djangoapps/content_libraries/api/containers.py @@ -17,7 +17,6 @@ from openedx_events.content_authoring.data import LibraryContainerData from openedx_events.content_authoring.signals import LIBRARY_CONTAINER_DELETED -from .. import tasks from ..models import ContentLibrary from .block_metadata import LibraryXBlockMetadata from .container_metadata import ( @@ -249,7 +248,6 @@ def get_containers_contains_item(key: LibraryUsageLocatorV2 | LibraryContainerLo def publish_container_changes( container_key: LibraryContainerLocator, user_id: int | None, - call_post_publish_events_sync=False, ) -> None: """ [ 🛑 UNSTABLE ] Publish all unpublished changes in a container and all its child @@ -263,17 +261,7 @@ def publish_container_changes( # The core publishing API is based on draft objects, so find the draft that corresponds to this container: drafts_to_publish = content_api.get_all_drafts(learning_package.id).filter(entity__pk=container.id) # Publish the container, which will also auto-publish any unpublished child components: - publish_log = content_api.publish_from_drafts( - learning_package.id, - draft_qset=drafts_to_publish, - published_by=user_id, - ) - # Update the search index (and anything else) for the affected container + blocks - # This is mostly synchronous but may complete some work asynchronously if there are a lot of changes. - if call_post_publish_events_sync: - tasks.send_events_after_publish(publish_log.pk, str(library_key)) - else: - tasks.wait_for_post_publish_events(publish_log, library_key) + content_api.publish_from_drafts(learning_package.id, draft_qset=drafts_to_publish, published_by=user_id) def copy_container(container_key: LibraryContainerLocator, user_id: int) -> UserClipboardData: diff --git a/openedx/core/djangoapps/content_libraries/api/libraries.py b/openedx/core/djangoapps/content_libraries/api/libraries.py index f61d8b90c835..bfce8c2d5959 100644 --- a/openedx/core/djangoapps/content_libraries/api/libraries.py +++ b/openedx/core/djangoapps/content_libraries/api/libraries.py @@ -72,7 +72,7 @@ from openedx.core.types import User as UserType -from .. import permissions, tasks +from .. import permissions from ..constants import ALL_RIGHTS_RESERVED from ..models import ContentLibrary, ContentLibraryPermission from .exceptions import LibraryAlreadyExists, LibraryPermissionIntegrityError @@ -758,15 +758,7 @@ def publish_changes(library_key: LibraryLocatorV2, user_id: int | None = None): """ learning_package = ContentLibrary.objects.get_by_key(library_key).learning_package assert learning_package is not None # shouldn't happen but it's technically possible. - publish_log = content_api.publish_all_drafts(learning_package.id, published_by=user_id) - - # Update the search index (and anything else) for the affected blocks - # This is mostly synchronous but may complete some work asynchronously if there are a lot of changes. - tasks.wait_for_post_publish_events(publish_log, library_key) - - # Unlike revert_changes below, we do not have to re-index collections, - # because publishing changes does not affect the component counts, and - # collections themselves don't have draft/published/unpublished status. + content_api.publish_all_drafts(learning_package.id, published_by=user_id) def revert_changes(library_key: LibraryLocatorV2, user_id: int | None = None) -> None: diff --git a/openedx/core/djangoapps/content_libraries/signal_handlers.py b/openedx/core/djangoapps/content_libraries/signal_handlers.py index 15e029d54b87..21d5d92f795c 100644 --- a/openedx/core/djangoapps/content_libraries/signal_handlers.py +++ b/openedx/core/djangoapps/content_libraries/signal_handlers.py @@ -56,6 +56,39 @@ def entities_updated( fn(learning_package_id=learning_package.id, change_list=[asdict(chg) for chg in change_log.changes]) +@receiver(content_signals.LEARNING_PACKAGE_ENTITIES_PUBLISHED) +def entities_published( + learning_package: content_signals.LearningPackageEventData, + change_log: content_signals.PublishLogEventData, + **kwargs, +) -> None: + """ + Entities (containers/components) have been published - handle that as needed. + + We receive this low-level event from `openedx_content`, and check if it + happened in a library. If so, we emit more detailed library-specific events. + + 💾 This event is only received after the transaction has committed. + ⏳ This event is emitted synchronously and this handler is called + synchronously. If a lot of entities were published, we need to dispatch + an asynchronous handler to deal with them to avoid slowdowns. If only one + entity was published, we want to deal with that synchronously so that we + can show the user correct data when the current requests completes. + """ + try: + library = ContentLibrary.objects.get(learning_package_id=learning_package.id) + except ContentLibrary.DoesNotExist: + return # We don't care about non-library events. + + if len(change_log.changes) == 1: + fn = tasks.send_events_after_publish + else: + # More than one entity was published at once. Handle asynchronously: + fn = tasks.send_events_after_publish.delay + + fn(publish_log_id=change_log.publish_log_id, library_key_str=str(library.library_key)) + + @receiver(content_signals.LEARNING_PACKAGE_COLLECTION_CHANGED) def collection_updated( learning_package: content_signals.LearningPackageEventData, diff --git a/openedx/core/djangoapps/content_libraries/tasks.py b/openedx/core/djangoapps/content_libraries/tasks.py index 0799ace7c4d7..baacad4b4582 100644 --- a/openedx/core/djangoapps/content_libraries/tasks.py +++ b/openedx/core/djangoapps/content_libraries/tasks.py @@ -21,6 +21,7 @@ import logging import os import shutil +from collections.abc import Iterable from datetime import datetime from io import StringIO from tempfile import NamedTemporaryFile, mkdtemp @@ -204,14 +205,16 @@ def check_container_content_changes( else: old_entity_list_id = None if new_version_id: - new_version = content_api.get_container_version(new_version_id) if new_version_id else None + new_version = content_api.get_container_version(new_version_id) new_entity_list_id = new_version.entity_list_id else: new_entity_list_id = None + old_child_ids: Iterable[PublishableEntity.ID] + new_child_ids: Iterable[PublishableEntity.ID] # If the title has changed, we notify ALL children that their parent container(s) have changed, e.g. to update the # list of "units this component is used in", "sections this subsection is used in", etc. in the search index - title_changed: bool = old_version and new_version and old_version.title != new_version.title + title_changed: bool = bool(old_version and new_version) and (old_version.title != new_version.title) if title_changed: # TODO: there is no "get entity list for container version" API in openedx_content new_child_ids = new_version.entity_list.entitylistrow_set.values_list("entity_id", flat=True) @@ -297,7 +300,7 @@ def send_collections_changed_events( @shared_task(base=LoggedTask) @set_code_owner_attribute -def send_events_after_publish(publish_log_pk: int, library_key_str: str) -> None: +def send_events_after_publish(publish_log_id: int, library_key_str: str) -> None: """ Send events to trigger actions like updating the search index, after we've published some items in a library. @@ -311,12 +314,11 @@ def send_events_after_publish(publish_log_pk: int, library_key_str: str) -> None event handlers like updating the search index may a while to complete in that case. """ - publish_log = PublishLog.objects.get(pk=publish_log_pk) + publish_log = PublishLog.objects.get(id=publish_log_id) library_key = LibraryLocatorV2.from_string(library_key_str) affected_entities = publish_log.records.select_related( "entity", "entity__container", "entity__container__container_type", "entity__component", ).all() - affected_containers: set[LibraryContainerLocator] = set() # Update anything that needs to be updated (e.g. search index): for record in affected_entities: @@ -330,61 +332,21 @@ def send_events_after_publish(publish_log_pk: int, library_key_str: str) -> None LIBRARY_BLOCK_PUBLISHED.send_event( library_block=LibraryBlockData(library_key=library_key, usage_key=usage_key) ) - # Publishing a container will auto-publish its children, but publishing a single component or all changes - # in the library will NOT usually include any parent containers. But we do need to notify listeners that the - # parent container(s) have changed, e.g. so the search index can update the "has_unpublished_changes" - try: - for parent_container in api.get_containers_contains_item(usage_key): - affected_containers.add(parent_container.container_key) - # TODO: should this be a CONTAINER_CHILD_PUBLISHED event instead of CONTAINER_PUBLISHED ? - except api.ContentLibraryBlockNotFound: - # The component has been deleted. - pass elif hasattr(record.entity, "container"): container_key = api.library_container_locator(library_key, record.entity.container) - affected_containers.add(container_key) - - try: - # We do need to notify listeners that the parent container(s) have changed, - # e.g. so the search index can update the "has_unpublished_changes" - for parent_container in api.get_containers_contains_item(container_key): - affected_containers.add(parent_container.container_key) - except api.ContentLibraryContainerNotFound: - # The deleted children remains in the entity, so, in this case, the container may not be found. - pass + # Note: this container may have been directly published, or perhaps one of its children was published and + # it hasn't technically changed. Such ancestors of published entities are still included in the publish log. + # .. event_implemented_name: LIBRARY_CONTAINER_PUBLISHED + # .. event_type: org.openedx.content_authoring.content_library.container.published.v1 + LIBRARY_CONTAINER_PUBLISHED.send_event( + library_container=LibraryContainerData(container_key=container_key) + ) else: log.warning( f"PublishableEntity {record.entity.pk} / {record.entity.entity_ref} " "was modified during publish operation but is of unknown type." ) - for container_key in affected_containers: - # .. event_implemented_name: LIBRARY_CONTAINER_PUBLISHED - # .. event_type: org.openedx.content_authoring.content_library.container.published.v1 - LIBRARY_CONTAINER_PUBLISHED.send_event( - library_container=LibraryContainerData(container_key=container_key) - ) - - -def wait_for_post_publish_events(publish_log: PublishLog, library_key: LibraryLocatorV2): - """ - After publishing some changes, trigger the required event handlers (e.g. - update the search index). Try to wait for that to complete before returning, - up to some reasonable timeout, and then finish anything remaining - asynchonrously. - """ - # Update the search index (and anything else) for the affected blocks - result = send_events_after_publish.apply_async(args=(publish_log.pk, str(library_key))) - # Try waiting a bit for those post-publish events to be handled: - try: - result.get(timeout=15) - except TimeoutError: - pass - # This is fine! The search index is still being updated, and/or other - # event handlers are still following up on the results, but the publish - # already *did* succeed, and the events will continue to be processed in - # the background by the celery worker until everything is updated. - def _filter_child(store, usage_key, capa_type): """ diff --git a/openedx/core/djangoapps/content_libraries/tests/test_events.py b/openedx/core/djangoapps/content_libraries/tests/test_events.py index 58d2557208ba..c89905d7dd37 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_events.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_events.py @@ -462,21 +462,87 @@ def test_publish_container(self) -> None: self.lib1_key, LibraryUsageLocatorV2.from_string(html_block["id"]), ), }, - { # Not 100% sure we want this, but a PUBLISHED event is emitted for container 2 - # because one of its children's published versions has changed, so whether or - # not it contains unpublished changes may have changed and the search index - # may need to be updated. It is not actually published though. - # TODO: should this be a CONTAINER_CHILD_PUBLISHED event? + # No PUBLISHED event is emitted for container 2, because it doesn't have a published version yet. + # Publishing 'html_block' would have potentially affected it if container 2's published version had a + # reference to 'html_block', but it doesn't yet until we publish it. + ) + + # note that container 2 is still unpublished + c2_after = self._get_container(container2["id"]) + assert c2_after["has_unpublished_changes"] + + # publish container2 now: + self._publish_container(container2["id"]) + self.expect_new_events( + { # An event for container 1 being published: "signal": LIBRARY_CONTAINER_PUBLISHED, "library_container": LibraryContainerData( container_key=LibraryContainerLocator.from_string(container2["id"]), ), }, + { # An event for the html block in container 2 only: + "signal": LIBRARY_BLOCK_PUBLISHED, + "library_block": LibraryBlockData( + self.lib1_key, LibraryUsageLocatorV2.from_string(html_block2["id"]), + ), + }, ) - # note that container 2 is still unpublished - c2_after = self._get_container(container2["id"]) - assert c2_after["has_unpublished_changes"] + def test_publish_container_propagation(self) -> None: + """ + Test the events that get emitted when we publish the changes to an entity + that is used in multiple published containers + """ + # Create two containers and add the same component to both: + container1 = self._create_container(self.lib1_key, "unit", display_name="Alpha Unit", slug=None) + container2 = self._create_container(self.lib1_key, "unit", display_name="Bravo Unit", slug=None) + problem_block = self._add_block_to_library(self.lib1_key, "problem", "Problem1", can_stand_alone=False) + self._add_container_children(container1["id"], children_ids=[problem_block["id"]]) + self._add_container_children(container2["id"], children_ids=[problem_block["id"]]) + # Publish everything: + self._commit_library_changes(self.lib1_key) + + # clear event log after the initial mock data setup is complete: + self.clear_events() + + # Now modify the problem that's shared by both containers and publish the new version + self._set_library_block_olx(problem_block["id"], "UPDATED") + self.clear_events() # Clears the LIBRARY_BLOCK_UPDATED event + 2x LIBRARY_CONTAINER_UPDATED events + + # Now both containers have unpublished changes: + assert self._get_container(container1["id"])["has_unpublished_changes"] + assert self._get_container(container2["id"])["has_unpublished_changes"] + # Publish container1, which also published the shared problem component: + self._publish_container(container1["id"]) + # Now neither container has unpublished changes (even though we never touched container2): + assert self._get_container(container1["id"])["has_unpublished_changes"] is False + assert self._get_container(container2["id"])["has_unpublished_changes"] is False + + # And publish events were emitted: + self.expect_new_events( + # An event for the problem block in container 1 being indirectly published: + { + "signal": LIBRARY_BLOCK_PUBLISHED, + "library_block": LibraryBlockData( + self.lib1_key, LibraryUsageLocatorV2.from_string(problem_block["id"]), + ), + }, + # An event for container 1 being published *directly*: + { + "signal": LIBRARY_CONTAINER_PUBLISHED, + "library_container": LibraryContainerData( + container_key=LibraryContainerLocator.from_string(container1["id"]), + ), + }, + # And this time a PUBLISHED event should also be emitted for container2. + # It's published version hasn't changed, but its "contains unpublished changes" status has. + { + "signal": LIBRARY_CONTAINER_PUBLISHED, + "library_container": LibraryContainerData( + container_key=LibraryContainerLocator.from_string(container2["id"]), + ), + }, + ) def test_publish_child_container(self): """ @@ -513,7 +579,17 @@ def test_publish_child_container(self): container_key=LibraryContainerLocator.from_string(unit["id"]), ), }, - { # An event for parent (subsection): + # No PUBLISHED event is emitted for the subsection, because it doesn't have a published version yet. + ) + + # note that subsection is still unpublished + c2_after = self._get_container(subsection["id"]) + assert c2_after["has_unpublished_changes"] + + # Now publish the subsection + self._publish_container(subsection["id"]) + self.expect_new_events( + { # An event for the subsection being published: "signal": LIBRARY_CONTAINER_PUBLISHED, "library_container": LibraryContainerData( container_key=LibraryContainerLocator.from_string(subsection["id"]), @@ -521,9 +597,27 @@ def test_publish_child_container(self): }, ) - # note that subsection is still unpublished - c2_after = self._get_container(subsection["id"]) - assert c2_after["has_unpublished_changes"] + # Now rename the unit: + self._update_container(unit["id"], 'New Unit Display Name') + self.clear_events() + # Publish changes to the unit: + self._publish_container(unit["id"]) + self.expect_new_events( + { # An event for the unit being published: + "signal": LIBRARY_CONTAINER_PUBLISHED, + "library_container": LibraryContainerData( + container_key=LibraryContainerLocator.from_string(unit["id"]), + ), + }, + # And this time we DO get notified that the parent container is affected, because the unit is in its + # published version, and this publish affects the parent's "contains_unpublished_changes" status. + { + "signal": LIBRARY_CONTAINER_PUBLISHED, + "library_container": LibraryContainerData( + container_key=LibraryContainerLocator.from_string(subsection["id"]), + ), + }, + ) def test_restore_unit(self) -> None: """ From df458d73e1e32f356038df1b2908c867af2be0b4 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Thu, 23 Apr 2026 10:15:42 -0700 Subject: [PATCH 13/15] temp: use openedx-core 0.45-preview with new events --- requirements/edx/base.txt | 2 +- requirements/edx/development.txt | 2 +- requirements/edx/doc.txt | 2 +- requirements/edx/testing.txt | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index 71790d659ff7..6dcdb1f96219 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -840,7 +840,7 @@ openedx-calc==5.0.0 # via # -r requirements/edx/kernel.in # xblocks-contrib -openedx-core==0.44.0 +openedx-core @ git+https://github.com/openedx/openedx-core.git@braden/emit-events # via # -c requirements/constraints.txt # -r requirements/edx/kernel.in diff --git a/requirements/edx/development.txt b/requirements/edx/development.txt index 3ad4c94197fd..e7e5e9c37765 100644 --- a/requirements/edx/development.txt +++ b/requirements/edx/development.txt @@ -1394,7 +1394,7 @@ openedx-calc==5.0.0 # -r requirements/edx/doc.txt # -r requirements/edx/testing.txt # xblocks-contrib -openedx-core==0.44.0 +openedx-core @ git+https://github.com/openedx/openedx-core.git@braden/emit-events # via # -c requirements/constraints.txt # -r requirements/edx/doc.txt diff --git a/requirements/edx/doc.txt b/requirements/edx/doc.txt index b7f29a68f1df..473b3bf81fa2 100644 --- a/requirements/edx/doc.txt +++ b/requirements/edx/doc.txt @@ -1018,7 +1018,7 @@ openedx-calc==5.0.0 # via # -r requirements/edx/base.txt # xblocks-contrib -openedx-core==0.44.0 +openedx-core @ git+https://github.com/openedx/openedx-core.git@braden/emit-events # via # -c requirements/constraints.txt # -r requirements/edx/base.txt diff --git a/requirements/edx/testing.txt b/requirements/edx/testing.txt index 1e14484defc2..39da746771df 100644 --- a/requirements/edx/testing.txt +++ b/requirements/edx/testing.txt @@ -1065,7 +1065,7 @@ openedx-calc==5.0.0 # via # -r requirements/edx/base.txt # xblocks-contrib -openedx-core==0.44.0 +openedx-core @ git+https://github.com/openedx/openedx-core.git@braden/emit-events # via # -c requirements/constraints.txt # -r requirements/edx/base.txt From 070363eb4f70a9549a0533801a4bc4fd87b1ba02 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Thu, 23 Apr 2026 10:39:42 -0700 Subject: [PATCH 14/15] feat: more sophisticated handling of sync-vs-async for library edits --- .../content_libraries/signal_handlers.py | 24 ++++++++++++++----- 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/openedx/core/djangoapps/content_libraries/signal_handlers.py b/openedx/core/djangoapps/content_libraries/signal_handlers.py index 21d5d92f795c..99647e389f39 100644 --- a/openedx/core/djangoapps/content_libraries/signal_handlers.py +++ b/openedx/core/djangoapps/content_libraries/signal_handlers.py @@ -3,6 +3,7 @@ """ import logging +from functools import partial from attrs import asdict from django.dispatch import receiver @@ -45,15 +46,26 @@ def entities_updated( except ContentLibrary.DoesNotExist: return # We don't care about non-library events. - entities_changed = [change.entity_id for change in change_log.changes] + # Which entities were _directly_ changed here? + direct_changes = [asdict(change) for change in change_log.changes if change.new_version != change.old_version] + # And which entities were indirectly affected (e.g. parent containers)? + indirect_changes = [asdict(change) for change in change_log.changes if change.new_version == change.old_version] - if len(entities_changed) == 1: - fn = tasks.send_change_events_for_modified_entities + update_task_fn = tasks.send_change_events_for_modified_entities + update_sync = partial(update_task_fn, learning_package_id=learning_package.id) + update_async = partial(update_task_fn.delay, learning_package_id=learning_package.id) + + if len(direct_changes) == 1: + # We directly changed only one entity. Update it synchronously so that the UI will reflect changes right away. + if len(indirect_changes) <= 1: + # And update any other affected entity synchronously too; there's at most one. (More efficient, better UX.) + update_sync(change_list=[*direct_changes, *indirect_changes]) + else: + update_sync(change_list=direct_changes) # Update this one entity synchronously, and + update_async(change_list=indirect_changes) # update the many other affects entities async. else: # More than one entity was changed at once. Handle asynchronously: - fn = tasks.send_change_events_for_modified_entities.delay - - fn(learning_package_id=learning_package.id, change_list=[asdict(chg) for chg in change_log.changes]) + update_async(change_list=[*direct_changes, *indirect_changes]) @receiver(content_signals.LEARNING_PACKAGE_ENTITIES_PUBLISHED) From 9870a832817319859c9985357bcdecca18009d0a Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Thu, 23 Apr 2026 13:36:08 -0700 Subject: [PATCH 15/15] chore: fix lint issues --- .../core/djangoapps/content_libraries/tasks.py | 10 ++++++++-- .../content_libraries/tests/test_api.py | 16 +++++++++++----- 2 files changed, 19 insertions(+), 7 deletions(-) diff --git a/openedx/core/djangoapps/content_libraries/tasks.py b/openedx/core/djangoapps/content_libraries/tasks.py index baacad4b4582..b3d38e9549f1 100644 --- a/openedx/core/djangoapps/content_libraries/tasks.py +++ b/openedx/core/djangoapps/content_libraries/tasks.py @@ -45,6 +45,7 @@ BlockUsageLocator, LibraryContainerLocator, LibraryLocatorV2, + LibraryUsageLocatorV2, ) from openedx_content import api as content_api from openedx_content.api import create_zip_file as create_lib_zip_file @@ -228,8 +229,12 @@ def check_container_content_changes( return else: # TODO: there is no "get entity list for container version" API in openedx_content - old_child_ids = old_version.entity_list.entitylistrow_set.values_list("entity_id", flat=True) if old_version else [] - new_child_ids = new_version.entity_list.entitylistrow_set.values_list("entity_id", flat=True) if new_version else [] + old_child_ids = ( + old_version.entity_list.entitylistrow_set.values_list("entity_id", flat=True) if old_version else [] + ) + new_child_ids = ( + new_version.entity_list.entitylistrow_set.values_list("entity_id", flat=True) if new_version else [] + ) # We only need to notify any added or removed children that their parent container(s) changed: changed_child_ids = list(set(old_child_ids) ^ set(new_child_ids)) @@ -241,6 +246,7 @@ def check_container_content_changes( .select_related("component", "container") ) for entity in entities: + child_key: LibraryUsageLocatorV2 | LibraryContainerLocator if hasattr(entity, "component"): child_key = api.library_component_usage_key(library.library_key, entity.component) elif hasattr(entity, "container"): diff --git a/openedx/core/djangoapps/content_libraries/tests/test_api.py b/openedx/core/djangoapps/content_libraries/tests/test_api.py index 8b9503a3fc49..99f2568d6114 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_api.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_api.py @@ -351,11 +351,15 @@ def test_set_library_component_collections(self) -> None: assert all(event["signal"] == LIBRARY_COLLECTION_UPDATED for event in collection_update_events) assert {event["library_collection"] for event in collection_update_events} == { LibraryCollectionData( - collection_key=api.library_collection_locator(self.lib2.library_key, collection_key=self.col2.collection_code) + collection_key=api.library_collection_locator( + self.lib2.library_key, collection_key=self.col2.collection_code + ) ), LibraryCollectionData( - collection_key=api.library_collection_locator(self.lib2.library_key, collection_key=self.col3.collection_code) - ) + collection_key=api.library_collection_locator( + self.lib2.library_key, collection_key=self.col3.collection_code + ) + ), } def test_delete_library_block(self) -> None: @@ -380,7 +384,9 @@ def test_delete_library_block(self) -> None: "signal": LIBRARY_COLLECTION_UPDATED, "sender": None, "library_collection": LibraryCollectionData( - collection_key=api.library_collection_locator(self.lib1.library_key, collection_key=self.col1.collection_code), + collection_key=api.library_collection_locator( + self.lib1.library_key, collection_key=self.col1.collection_code + ), ), }, ) @@ -430,7 +436,7 @@ def test_delete_library_container(self) -> None: "library_container": LibraryContainerData( container_key=self.subsection1.container_key, background=False, - ) + ), }, )