From 892caa83497c8c3e7c2c26d100a14ee07cb1fe85 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Wed, 16 Apr 2025 18:59:17 +0930 Subject: [PATCH 1/5] feat: add optional Container object argument to get_container + get_container_children API methods, to avoid re-fetching this data during search indexing. --- .../content_libraries/api/containers.py | 17 ++++++++++++++--- .../content_libraries/rest_api/containers.py | 3 ++- 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/openedx/core/djangoapps/content_libraries/api/containers.py b/openedx/core/djangoapps/content_libraries/api/containers.py index d7ba0fcac01f..bc265148f70e 100644 --- a/openedx/core/djangoapps/content_libraries/api/containers.py +++ b/openedx/core/djangoapps/content_libraries/api/containers.py @@ -192,11 +192,18 @@ def get_container_from_key(container_key: LibraryContainerLocator, isDeleted=Fal raise ContentLibraryContainerNotFound -def get_container(container_key: LibraryContainerLocator, include_collections=False) -> ContainerMetadata: +def get_container( + container_key: LibraryContainerLocator, + *, + include_collections=False, + container: Container | None = None, +) -> ContainerMetadata: """ Get a container (a Section, Subsection, or Unit). """ - container = get_container_from_key(container_key) + if not container: + container = get_container_from_key(container_key) + assert container.key == container_key.container_id if include_collections: associated_collections = authoring_api.get_entity_collections( container.publishable_entity.learning_package_id, @@ -372,12 +379,16 @@ def restore_container(container_key: LibraryContainerLocator) -> None: def get_container_children( container_key: LibraryContainerLocator, + *, published=False, + container: Container | None = None, ) -> list[LibraryXBlockMetadata | ContainerMetadata]: """ Get the entities contained in the given container (e.g. the components/xblocks in a unit) """ - container = get_container_from_key(container_key) + if not container: + container = get_container_from_key(container_key) + assert container.key == container_key.container_id if container_key.container_type == ContainerType.Unit.value: child_components = authoring_api.get_components_in_unit(container.unit, published=published) return [LibraryXBlockMetadata.from_component( diff --git a/openedx/core/djangoapps/content_libraries/rest_api/containers.py b/openedx/core/djangoapps/content_libraries/rest_api/containers.py index 061f4a0693d1..0c41f886201a 100644 --- a/openedx/core/djangoapps/content_libraries/rest_api/containers.py +++ b/openedx/core/djangoapps/content_libraries/rest_api/containers.py @@ -184,7 +184,8 @@ def get(self, request, container_key: LibraryContainerLocator): request.user, permissions.CAN_VIEW_THIS_CONTENT_LIBRARY, ) - child_entities = api.get_container_children(container_key, published) + child_entities = api.get_container_children(container_key, published=published) + # TODO -- this looks backwards? if container_key.container_type == api.ContainerType.Unit.value: data = serializers.LibraryXBlockMetadataSerializer(child_entities, many=True).data else: From 15fcff5041c29edea25985d62260a1f30d7fe94e Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Wed, 16 Apr 2025 19:01:52 +0930 Subject: [PATCH 2/5] feat: store content.child_usage_keys in Container search document Stores the draft children + published children (if applicable) * lib_api.get_container does not take a "user" arg * fetch_customizable_fields_from_container does not need a "user" arg --- .../rest_api/v2/views/downstreams.py | 2 +- cms/lib/xblock/upstream_sync_container.py | 6 +-- .../djangoapps/content/search/documents.py | 37 ++++++++++++++++--- .../content/search/tests/test_api.py | 1 + .../content/search/tests/test_documents.py | 24 ++++++++++++ 5 files changed, 61 insertions(+), 9 deletions(-) diff --git a/cms/djangoapps/contentstore/rest_api/v2/views/downstreams.py b/cms/djangoapps/contentstore/rest_api/v2/views/downstreams.py index 39c2649118f5..fc3e4f296bde 100644 --- a/cms/djangoapps/contentstore/rest_api/v2/views/downstreams.py +++ b/cms/djangoapps/contentstore/rest_api/v2/views/downstreams.py @@ -267,7 +267,7 @@ def put(self, request: _AuthenticatedRequest, usage_key_string: str) -> Response fetch_customizable_fields_from_block(downstream=downstream, user=request.user) else: assert isinstance(link.upstream_key, LibraryContainerLocator) - fetch_customizable_fields_from_container(downstream=downstream, user=request.user) + fetch_customizable_fields_from_container(downstream=downstream) except BadDownstream as exc: logger.exception( "'%s' is an invalid downstream; refusing to set its upstream to '%s'", diff --git a/cms/lib/xblock/upstream_sync_container.py b/cms/lib/xblock/upstream_sync_container.py index 44e3b429ec05..4e8302323808 100644 --- a/cms/lib/xblock/upstream_sync_container.py +++ b/cms/lib/xblock/upstream_sync_container.py @@ -45,7 +45,7 @@ def sync_from_upstream_container( user, permission=lib_api.permissions.CAN_VIEW_THIS_CONTENT_LIBRARY, ) - upstream_meta = lib_api.get_container(link.upstream_key, user) + upstream_meta = lib_api.get_container(link.upstream_key) upstream_children = lib_api.get_container_children(link.upstream_key, published=True) _update_customizable_fields(upstream=upstream_meta, downstream=downstream, only_fetch=False) _update_non_customizable_fields(upstream=upstream_meta, downstream=downstream) @@ -54,7 +54,7 @@ def sync_from_upstream_container( return upstream_children -def fetch_customizable_fields_from_container(*, downstream: XBlock, user: User) -> None: +def fetch_customizable_fields_from_container(*, downstream: XBlock) -> None: """ Fetch upstream-defined value of customizable fields and save them on the downstream. @@ -62,7 +62,7 @@ def fetch_customizable_fields_from_container(*, downstream: XBlock, user: User) Basically, this sets the value of "upstream_display_name" on the downstream block. """ - upstream = lib_api.get_container(LibraryContainerLocator.from_string(downstream.upstream), user) + upstream = lib_api.get_container(LibraryContainerLocator.from_string(downstream.upstream)) _update_customizable_fields(upstream=upstream, downstream=downstream, only_fetch=True) diff --git a/openedx/core/djangoapps/content/search/documents.py b/openedx/core/djangoapps/content/search/documents.py index 6a40f049bf96..6d80276ca75b 100644 --- a/openedx/core/djangoapps/content/search/documents.py +++ b/openedx/core/djangoapps/content/search/documents.py @@ -71,6 +71,7 @@ class Fields: # The "content" field is a dictionary of arbitrary data, depending on the block_type. # It comes from each XBlock's index_dictionary() method (if present) plus some processing. # Text (html) blocks have an "html_content" key in here, capa has "capa_content" and "problem_types", and so on. + # Containers store their list of child usage keys here. content = "content" # Collections use this field to communicate how many entities/components they contain. @@ -87,6 +88,7 @@ class Fields: published = "published" published_display_name = "display_name" published_description = "description" + published_content = "content" published_num_children = "num_children" # Note: new fields or values can be added at any time, but if they need to be indexed for filtering or keyword @@ -576,12 +578,21 @@ def searchable_doc_for_container( } try: - container = lib_api.get_container(container_key) + container_obj = lib_api.get_container_from_key(container_key) + container = lib_api.get_container( + container_key, + container=container_obj, + ) except lib_api.ContentLibraryContainerNotFound: # Container not found, so we can only return the base doc + log.error(f"Container {container_key} not found") return doc - draft_num_children = lib_api.get_container_children_count(container_key, published=False) + draft_children = lib_api.get_container_children( + container_key, + published=False, + container=container_obj, + ) publish_status = PublishStatus.published if container.last_published is None: publish_status = PublishStatus.never @@ -592,7 +603,13 @@ def searchable_doc_for_container( Fields.display_name: container.display_name, Fields.created: container.created.timestamp(), Fields.modified: container.modified.timestamp(), - Fields.num_children: draft_num_children, + Fields.num_children: len(draft_children), + Fields.content: { + "child_usage_keys": [ + str(child.usage_key) + for child in draft_children + ], + }, Fields.publish_status: publish_status, Fields.last_published: container.last_published.timestamp() if container.last_published else None, }) @@ -601,10 +618,20 @@ def searchable_doc_for_container( doc[Fields.breadcrumbs] = [{"display_name": library.title}] if container.published_version_num is not None: - published_num_children = lib_api.get_container_children_count(container_key, published=True) + published_children = lib_api.get_container_children( + container_key, + published=True, + container=container_obj, + ) doc[Fields.published] = { - Fields.published_num_children: published_num_children, Fields.published_display_name: container.published_display_name, + Fields.published_num_children: len(published_children), + Fields.published_content: { + "child_usage_keys": [ + str(child.usage_key) + for child in published_children + ], + }, } return doc diff --git a/openedx/core/djangoapps/content/search/tests/test_api.py b/openedx/core/djangoapps/content/search/tests/test_api.py index ee70bd444721..c1cda1ba9d52 100644 --- a/openedx/core/djangoapps/content/search/tests/test_api.py +++ b/openedx/core/djangoapps/content/search/tests/test_api.py @@ -230,6 +230,7 @@ def setUp(self): "display_name": "Unit 1", # description is not set for containers "num_children": 0, + "content": {"child_usage_keys": []}, "publish_status": "never", "context_key": "lib:org1:lib", "org": "org1", diff --git a/openedx/core/djangoapps/content/search/tests/test_documents.py b/openedx/core/djangoapps/content/search/tests/test_documents.py index 0a5d871fbb96..71a7d330c7cc 100644 --- a/openedx/core/djangoapps/content/search/tests/test_documents.py +++ b/openedx/core/djangoapps/content/search/tests/test_documents.py @@ -528,6 +528,9 @@ def test_draft_container(self): "display_name": "A Unit in the Search Index", # description is not set for containers "num_children": 0, + "content": { + "child_usage_keys": [], + }, "publish_status": "never", "context_key": "lib:edX:2012_Fall", "access_id": self.library_access_id, @@ -568,6 +571,11 @@ def test_published_container(self): "display_name": "A Unit in the Search Index", # description is not set for containers "num_children": 1, + "content": { + "child_usage_keys": [ + "lb:edX:2012_Fall:html:text2", + ], + }, "publish_status": "published", "context_key": "lib:edX:2012_Fall", "access_id": self.library_access_id, @@ -582,6 +590,11 @@ def test_published_container(self): "published": { "num_children": 1, "display_name": "A Unit in the Search Index", + "content": { + "child_usage_keys": [ + "lb:edX:2012_Fall:html:text2", + ], + }, }, } @@ -624,6 +637,12 @@ def test_published_container_with_changes(self): "display_name": "A Unit in the Search Index", # description is not set for containers "num_children": 2, + "content": { + "child_usage_keys": [ + "lb:edX:2012_Fall:html:text2", + "lb:edX:2012_Fall:html:text3", + ], + }, "publish_status": "modified", "context_key": "lib:edX:2012_Fall", "access_id": self.library_access_id, @@ -638,6 +657,11 @@ def test_published_container_with_changes(self): "published": { "num_children": 1, "display_name": "A Unit in the Search Index", + "content": { + "child_usage_keys": [ + "lb:edX:2012_Fall:html:text2", + ], + }, }, } From 0bc1a86ad915d9f9c913d56b59cddcdf9398c17e Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Tue, 29 Apr 2025 15:30:57 +0930 Subject: [PATCH 3/5] refactor: moves tags_count into LibraryItem because anything that appears in a library may be tagged. --- openedx/core/djangoapps/content_libraries/api/block_metadata.py | 2 -- openedx/core/djangoapps/content_libraries/api/libraries.py | 1 + .../core/djangoapps/content_libraries/rest_api/serializers.py | 2 +- 3 files changed, 2 insertions(+), 3 deletions(-) diff --git a/openedx/core/djangoapps/content_libraries/api/block_metadata.py b/openedx/core/djangoapps/content_libraries/api/block_metadata.py index 032d21431a3a..539b3290631b 100644 --- a/openedx/core/djangoapps/content_libraries/api/block_metadata.py +++ b/openedx/core/djangoapps/content_libraries/api/block_metadata.py @@ -26,8 +26,6 @@ class LibraryXBlockMetadata(PublishableItem): Class that represents the metadata about an XBlock in a content library. """ usage_key: LibraryUsageLocatorV2 - # TODO: move tags_count to LibraryItem as all objects under a library can be tagged. - tags_count: int = 0 @classmethod def from_component(cls, library_key, component, associated_collections=None): diff --git a/openedx/core/djangoapps/content_libraries/api/libraries.py b/openedx/core/djangoapps/content_libraries/api/libraries.py index 8e238f278096..8549060f3e64 100644 --- a/openedx/core/djangoapps/content_libraries/api/libraries.py +++ b/openedx/core/djangoapps/content_libraries/api/libraries.py @@ -184,6 +184,7 @@ class LibraryItem: created: datetime modified: datetime display_name: str + tags_count: int = 0 @dataclass(frozen=True, kw_only=True) diff --git a/openedx/core/djangoapps/content_libraries/rest_api/serializers.py b/openedx/core/djangoapps/content_libraries/rest_api/serializers.py index be386e0e9dfe..04c9e6a9901c 100644 --- a/openedx/core/djangoapps/content_libraries/rest_api/serializers.py +++ b/openedx/core/djangoapps/content_libraries/rest_api/serializers.py @@ -138,6 +138,7 @@ class PublishableItemSerializer(serializers.Serializer): """ id = serializers.SerializerMethodField() display_name = serializers.CharField() + tags_count = serializers.IntegerField(read_only=True) last_published = serializers.DateTimeField(format=DATETIME_FORMAT, read_only=True) published_by = serializers.CharField(read_only=True) last_draft_created = serializers.DateTimeField(format=DATETIME_FORMAT, read_only=True) @@ -149,7 +150,6 @@ class PublishableItemSerializer(serializers.Serializer): # When creating a new XBlock in a library, the slug becomes the ID part of # the definition key and usage key: slug = serializers.CharField(write_only=True) - tags_count = serializers.IntegerField(read_only=True) collections = CollectionMetadataSerializer(many=True, required=False) can_stand_alone = serializers.BooleanField(read_only=True) From 49557f570e45f640188897773fbbaf48f8bc0d04 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Thu, 1 May 2025 12:19:55 +0930 Subject: [PATCH 4/5] refactor: remove get_container_from_key from public API API users must use get_container and ContainerMetadata. Related changes: * made set_library_item_collections take an entity_key string instead of a PublishableEntity instance, so we don't need to fetch a Container object to call it. * added `entity_key_from_usage_key` to the xblock API to support ^ * added container_id * changed ContainerLink.update_or_create to take the container PK instead of a Container object, since this is enough. * added container_pk to ContainerMetadata to support ^ * reverted previous change that added an optional Container param to get_container and get_container_children --- cms/djangoapps/contentstore/models.py | 2 +- cms/djangoapps/contentstore/utils.py | 4 +-- .../djangoapps/content/search/documents.py | 21 +++++---------- .../content_libraries/api/collections.py | 7 ++++- .../content_libraries/api/containers.py | 27 ++++++++----------- .../content_libraries/rest_api/blocks.py | 4 +-- .../content_libraries/rest_api/containers.py | 4 +-- .../content_libraries/tests/test_api.py | 8 +++--- openedx/core/djangoapps/xblock/api.py | 9 +++++++ 9 files changed, 43 insertions(+), 43 deletions(-) diff --git a/cms/djangoapps/contentstore/models.py b/cms/djangoapps/contentstore/models.py index 24dc6748d2c7..f5e0fc0dfde0 100644 --- a/cms/djangoapps/contentstore/models.py +++ b/cms/djangoapps/contentstore/models.py @@ -293,7 +293,7 @@ def __str__(self): @classmethod def update_or_create( cls, - upstream_container: Container | None, + upstream_container: int | None, /, upstream_container_key: LibraryContainerLocator, upstream_context_key: str, diff --git a/cms/djangoapps/contentstore/utils.py b/cms/djangoapps/contentstore/utils.py index ce244f616ddb..031a9cbe4518 100644 --- a/cms/djangoapps/contentstore/utils.py +++ b/cms/djangoapps/contentstore/utils.py @@ -87,7 +87,7 @@ from common.djangoapps.xblock_django.api import deprecated_xblocks from common.djangoapps.xblock_django.user_service import DjangoXBlockUserService from openedx.core import toggles as core_toggles -from openedx.core.djangoapps.content_libraries.api import get_container_from_key +from openedx.core.djangoapps.content_libraries.api import get_container from openedx.core.djangoapps.content_tagging.toggles import is_tagging_feature_disabled from openedx.core.djangoapps.credit.api import get_credit_requirements, is_credit_course from openedx.core.djangoapps.discussions.config.waffle import ENABLE_PAGES_AND_RESOURCES_MICROFRONTEND @@ -2402,7 +2402,7 @@ def _create_or_update_container_link(course_key: CourseKey, created: datetime | """ upstream_container_key = LibraryContainerLocator.from_string(xblock.upstream) try: - lib_component = get_container_from_key(upstream_container_key) + lib_component = get_container(upstream_container_key).container_pk except ObjectDoesNotExist: log.error(f"Library component not found for {upstream_container_key}") lib_component = None diff --git a/openedx/core/djangoapps/content/search/documents.py b/openedx/core/djangoapps/content/search/documents.py index 6d80276ca75b..64c078745bcb 100644 --- a/openedx/core/djangoapps/content/search/documents.py +++ b/openedx/core/djangoapps/content/search/documents.py @@ -347,13 +347,10 @@ def _collections_for_content_object(object_id: OpaqueKey) -> dict: collections = authoring_api.get_entity_collections( component.learning_package_id, component.key, - ) + ).values('key', 'title') elif isinstance(object_id, LibraryContainerLocator): - container = lib_api.get_container_from_key(object_id) - collections = authoring_api.get_entity_collections( - container.publishable_entity.learning_package_id, - container.key, - ) + container = lib_api.get_container(object_id, include_collections=True) + collections = container.collections else: log.warning(f"Unexpected key type for {object_id}") @@ -364,8 +361,8 @@ def _collections_for_content_object(object_id: OpaqueKey) -> dict: return result for collection in collections: - result[Fields.collections][Fields.collections_display_name].append(collection.title) - result[Fields.collections][Fields.collections_key].append(collection.key) + result[Fields.collections][Fields.collections_display_name].append(collection["title"]) + result[Fields.collections][Fields.collections_key].append(collection["key"]) return result @@ -578,11 +575,7 @@ def searchable_doc_for_container( } try: - container_obj = lib_api.get_container_from_key(container_key) - container = lib_api.get_container( - container_key, - container=container_obj, - ) + container = lib_api.get_container(container_key) except lib_api.ContentLibraryContainerNotFound: # Container not found, so we can only return the base doc log.error(f"Container {container_key} not found") @@ -591,7 +584,6 @@ def searchable_doc_for_container( draft_children = lib_api.get_container_children( container_key, published=False, - container=container_obj, ) publish_status = PublishStatus.published if container.last_published is None: @@ -621,7 +613,6 @@ def searchable_doc_for_container( published_children = lib_api.get_container_children( container_key, published=True, - container=container_obj, ) doc[Fields.published] = { Fields.published_display_name: container.published_display_name, diff --git a/openedx/core/djangoapps/content_libraries/api/collections.py b/openedx/core/djangoapps/content_libraries/api/collections.py index 2b0ddc08d89c..da8b47ee5e75 100644 --- a/openedx/core/djangoapps/content_libraries/api/collections.py +++ b/openedx/core/djangoapps/content_libraries/api/collections.py @@ -181,7 +181,7 @@ def update_library_collection_items( def set_library_item_collections( library_key: LibraryLocatorV2, - publishable_entity: PublishableEntity, + entity_key: str, *, collection_keys: list[str], created_by: int | None = None, @@ -207,6 +207,11 @@ def set_library_item_collections( assert content_library.learning_package_id assert content_library.library_key == library_key + publishable_entity = authoring_api.get_publishable_entity_by_key( + content_library.learning_package_id, + key=entity_key, + ) + # Note: Component.key matches its PublishableEntity.key collection_qs = authoring_api.get_collections(content_library.learning_package_id).filter( key__in=collection_keys diff --git a/openedx/core/djangoapps/content_libraries/api/containers.py b/openedx/core/djangoapps/content_libraries/api/containers.py index bc265148f70e..36072caf8215 100644 --- a/openedx/core/djangoapps/content_libraries/api/containers.py +++ b/openedx/core/djangoapps/content_libraries/api/containers.py @@ -43,7 +43,6 @@ "ContainerMetadata", "ContainerType", # API methods - "get_container_from_key", "get_container", "create_container", "get_container_children", @@ -111,6 +110,7 @@ class ContainerMetadata(PublishableItem): """ container_key: LibraryContainerLocator container_type: ContainerType + container_pk: int published_display_name: str | None @classmethod @@ -139,6 +139,7 @@ def from_container(cls, library_key, container: Container, associated_collection return cls( container_key=container_key, container_type=container_type, + container_pk=container.pk, display_name=draft.title, created=container.created, modified=draft.created, @@ -173,7 +174,7 @@ def library_container_locator( ) -def get_container_from_key(container_key: LibraryContainerLocator, isDeleted=False) -> Container: +def _get_container_from_key(container_key: LibraryContainerLocator, isDeleted=False) -> Container: """ Internal method to fetch the Container object from its LibraryContainerLocator @@ -196,14 +197,11 @@ def get_container( container_key: LibraryContainerLocator, *, include_collections=False, - container: Container | None = None, ) -> ContainerMetadata: """ Get a container (a Section, Subsection, or Unit). """ - if not container: - container = get_container_from_key(container_key) - assert container.key == container_key.container_id + container = _get_container_from_key(container_key) if include_collections: associated_collections = authoring_api.get_entity_collections( container.publishable_entity.learning_package_id, @@ -275,7 +273,7 @@ def update_container( """ Update a container (e.g. a Unit) title. """ - container = get_container_from_key(container_key) + container = _get_container_from_key(container_key) library_key = container_key.lib_key assert container.unit @@ -304,7 +302,7 @@ def delete_container( No-op if container doesn't exist or has already been soft-deleted. """ library_key = container_key.lib_key - container = get_container_from_key(container_key) + container = _get_container_from_key(container_key) affected_collections = authoring_api.get_entity_collections( container.publishable_entity.learning_package_id, @@ -339,7 +337,7 @@ def restore_container(container_key: LibraryContainerLocator) -> None: Restore the specified library container. """ library_key = container_key.lib_key - container = get_container_from_key(container_key, isDeleted=True) + container = _get_container_from_key(container_key, isDeleted=True) affected_collections = authoring_api.get_entity_collections( container.publishable_entity.learning_package_id, @@ -381,14 +379,11 @@ def get_container_children( container_key: LibraryContainerLocator, *, published=False, - container: Container | None = None, ) -> list[LibraryXBlockMetadata | ContainerMetadata]: """ Get the entities contained in the given container (e.g. the components/xblocks in a unit) """ - if not container: - container = get_container_from_key(container_key) - assert container.key == container_key.container_id + container = _get_container_from_key(container_key) if container_key.container_type == ContainerType.Unit.value: child_components = authoring_api.get_components_in_unit(container.unit, published=published) return [LibraryXBlockMetadata.from_component( @@ -410,7 +405,7 @@ def get_container_children_count( """ Get the count of entities contained in the given container (e.g. the components/xblocks in a unit) """ - container = get_container_from_key(container_key) + container = _get_container_from_key(container_key) return authoring_api.get_container_children_count(container, published=published) @@ -425,7 +420,7 @@ def update_container_children( """ library_key = container_key.lib_key container_type = container_key.container_type - container = get_container_from_key(container_key) + container = _get_container_from_key(container_key) match container_type: case ContainerType.Unit.value: components = [get_component_from_usage_key(key) for key in children_ids] # type: ignore[arg-type] @@ -470,7 +465,7 @@ def publish_container_changes(container_key: LibraryContainerLocator, user_id: i Publish all unpublished changes in a container and all its child containers/blocks. """ - container = get_container_from_key(container_key) + container = _get_container_from_key(container_key) library_key = container_key.lib_key content_library = ContentLibrary.objects.get_by_key(library_key) # type: ignore[attr-defined] learning_package = content_library.learning_package diff --git a/openedx/core/djangoapps/content_libraries/rest_api/blocks.py b/openedx/core/djangoapps/content_libraries/rest_api/blocks.py index 6ab35c47c632..45217cdc1dc0 100644 --- a/openedx/core/djangoapps/content_libraries/rest_api/blocks.py +++ b/openedx/core/djangoapps/content_libraries/rest_api/blocks.py @@ -265,14 +265,14 @@ def patch(self, request: RestRequest, usage_key_str) -> Response: request.user, permissions.CAN_EDIT_THIS_CONTENT_LIBRARY ) - component = api.get_component_from_usage_key(key) serializer = ContentLibraryItemCollectionsUpdateSerializer(data=request.data) serializer.is_valid(raise_exception=True) + entity_key = xblock_api.entity_key_from_usage_key(key) collection_keys = serializer.validated_data['collection_keys'] api.set_library_item_collections( library_key=key.lib_key, - publishable_entity=component.publishable_entity, + entity_key=entity_key, 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 0c41f886201a..19f33b7beb2c 100644 --- a/openedx/core/djangoapps/content_libraries/rest_api/containers.py +++ b/openedx/core/djangoapps/content_libraries/rest_api/containers.py @@ -185,7 +185,6 @@ def get(self, request, container_key: LibraryContainerLocator): permissions.CAN_VIEW_THIS_CONTENT_LIBRARY, ) child_entities = api.get_container_children(container_key, published=published) - # TODO -- this looks backwards? if container_key.container_type == api.ContainerType.Unit.value: data = serializers.LibraryXBlockMetadataSerializer(child_entities, many=True).data else: @@ -315,14 +314,13 @@ def patch(self, request: RestRequest, container_key: LibraryContainerLocator) -> request.user, permissions.CAN_EDIT_THIS_CONTENT_LIBRARY ) - container = api.get_container_from_key(container_key) serializer = serializers.ContentLibraryItemCollectionsUpdateSerializer(data=request.data) serializer.is_valid(raise_exception=True) collection_keys = serializer.validated_data['collection_keys'] api.set_library_item_collections( library_key=container_key.lib_key, - publishable_entity=container.publishable_entity, + entity_key=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/tests/test_api.py b/openedx/core/djangoapps/content_libraries/tests/test_api.py index 8f79ec7f6339..624aaf2b4a49 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 @@ from openedx_events.tests.utils import OpenEdxEventsTestMixin from openedx_learning.api import authoring as authoring_api +from openedx.core.djangoapps.xblock import api as xblock_api + from .. import api from ..models import ContentLibrary from .base import ContentLibrariesRestApiTest @@ -546,11 +548,11 @@ def test_set_library_component_collections(self): collection_update_event_receiver = mock.Mock() LIBRARY_COLLECTION_UPDATED.connect(collection_update_event_receiver) assert not list(self.col2.entities.all()) - component = api.get_component_from_usage_key(UsageKey.from_string(self.lib2_problem_block["id"])) - + component_key = UsageKey.from_string(self.lib2_problem_block["id"]) + entity_key = xblock_api.entity_key_from_usage_key(component_key) api.set_library_item_collections( self.lib2.library_key, - component.publishable_entity, + entity_key, collection_keys=[self.col2.key, self.col3.key], ) diff --git a/openedx/core/djangoapps/xblock/api.py b/openedx/core/djangoapps/xblock/api.py index c806fefc87c5..1b6755b35c5e 100644 --- a/openedx/core/djangoapps/xblock/api.py +++ b/openedx/core/djangoapps/xblock/api.py @@ -193,6 +193,15 @@ def get_block_display_name(block: XBlock) -> str: return xblock_type_display_name(block.scope_ids.block_type) +def entity_key_from_usage_key(usage_key: UsageKeyV2) -> str: + """ + Construct the PublishableEntity key string for a given usage key. + + Note: this method mirrors logic in openedx-learning; see authoring_api.create_component() + """ + return f"xblock.v1:{usage_key.block_type}:{usage_key.block_id}" + + def get_component_from_usage_key(usage_key: UsageKeyV2) -> Component: """ Fetch the Component object for a given usage key. From 2e7be75c8353211dbb9e3ffcae8f49ebdc9acb9b Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Fri, 2 May 2025 10:08:01 +0930 Subject: [PATCH 5/5] revert: remove xblock_api.entity_key_from_usage_key Use get_component_from_usage_key, and get it from the component instead. Also fixes nit re ContainerLink.update_or_create --- cms/djangoapps/contentstore/models.py | 6 +++--- .../core/djangoapps/content_libraries/rest_api/blocks.py | 4 ++-- .../core/djangoapps/content_libraries/tests/test_api.py | 9 +++------ openedx/core/djangoapps/xblock/api.py | 9 --------- 4 files changed, 8 insertions(+), 20 deletions(-) diff --git a/cms/djangoapps/contentstore/models.py b/cms/djangoapps/contentstore/models.py index f5e0fc0dfde0..f91090858e63 100644 --- a/cms/djangoapps/contentstore/models.py +++ b/cms/djangoapps/contentstore/models.py @@ -293,7 +293,7 @@ def __str__(self): @classmethod def update_or_create( cls, - upstream_container: int | None, + upstream_container_id: int | None, /, upstream_container_key: LibraryContainerLocator, upstream_context_key: str, @@ -316,10 +316,10 @@ def update_or_create( 'version_synced': version_synced, 'version_declined': version_declined, } - if upstream_container: + if upstream_container_id: new_values.update( { - 'upstream_container': upstream_container, + 'upstream_container_id': upstream_container_id, } ) try: diff --git a/openedx/core/djangoapps/content_libraries/rest_api/blocks.py b/openedx/core/djangoapps/content_libraries/rest_api/blocks.py index 45217cdc1dc0..bc314099893c 100644 --- a/openedx/core/djangoapps/content_libraries/rest_api/blocks.py +++ b/openedx/core/djangoapps/content_libraries/rest_api/blocks.py @@ -268,11 +268,11 @@ def patch(self, request: RestRequest, usage_key_str) -> Response: serializer = ContentLibraryItemCollectionsUpdateSerializer(data=request.data) serializer.is_valid(raise_exception=True) - entity_key = xblock_api.entity_key_from_usage_key(key) + component = api.get_component_from_usage_key(key) collection_keys = serializer.validated_data['collection_keys'] api.set_library_item_collections( library_key=key.lib_key, - entity_key=entity_key, + entity_key=component.publishable_entity.key, collection_keys=collection_keys, created_by=request.user.id, content_library=content_library, diff --git a/openedx/core/djangoapps/content_libraries/tests/test_api.py b/openedx/core/djangoapps/content_libraries/tests/test_api.py index 624aaf2b4a49..8dbb5e11946a 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_api.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_api.py @@ -28,8 +28,6 @@ from openedx_events.tests.utils import OpenEdxEventsTestMixin from openedx_learning.api import authoring as authoring_api -from openedx.core.djangoapps.xblock import api as xblock_api - from .. import api from ..models import ContentLibrary from .base import ContentLibrariesRestApiTest @@ -548,11 +546,10 @@ def test_set_library_component_collections(self): collection_update_event_receiver = mock.Mock() LIBRARY_COLLECTION_UPDATED.connect(collection_update_event_receiver) assert not list(self.col2.entities.all()) - component_key = UsageKey.from_string(self.lib2_problem_block["id"]) - entity_key = xblock_api.entity_key_from_usage_key(component_key) + component = api.get_component_from_usage_key(UsageKey.from_string(self.lib2_problem_block["id"])) api.set_library_item_collections( - self.lib2.library_key, - entity_key, + library_key=self.lib2.library_key, + entity_key=component.publishable_entity.key, collection_keys=[self.col2.key, self.col3.key], ) diff --git a/openedx/core/djangoapps/xblock/api.py b/openedx/core/djangoapps/xblock/api.py index 1b6755b35c5e..c806fefc87c5 100644 --- a/openedx/core/djangoapps/xblock/api.py +++ b/openedx/core/djangoapps/xblock/api.py @@ -193,15 +193,6 @@ def get_block_display_name(block: XBlock) -> str: return xblock_type_display_name(block.scope_ids.block_type) -def entity_key_from_usage_key(usage_key: UsageKeyV2) -> str: - """ - Construct the PublishableEntity key string for a given usage key. - - Note: this method mirrors logic in openedx-learning; see authoring_api.create_component() - """ - return f"xblock.v1:{usage_key.block_type}:{usage_key.block_id}" - - def get_component_from_usage_key(usage_key: UsageKeyV2) -> Component: """ Fetch the Component object for a given usage key.