Skip to content
Original file line number Diff line number Diff line change
Expand Up @@ -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'",
Expand Down
6 changes: 3 additions & 3 deletions cms/lib/xblock/upstream_sync_container.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -54,15 +54,15 @@ 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.

The container version only retrieves values from *published* containers.

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))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Was this even working before?! Thanks for fixing.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thank mypy :) It caught it when I changed get_container to require named parameters after the key.

_update_customizable_fields(upstream=upstream, downstream=downstream, only_fetch=True)


Expand Down
37 changes: 32 additions & 5 deletions openedx/core/djangoapps/content/search/documents.py
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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
Expand Down Expand Up @@ -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)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The docstring for get_container_from_key says it's an "Internal method", but in #36504 we added it to __all__ so it's now also part of the public API.

The thing is, it doesn't make sense as a public API since both get_container and get_container_from_key take a container key as their first argument. I think we should rename it to _get_container_from_key, and keep it as internal (remove it from __all__) and create a separate public function called get_container_obj which wraps _get_container_from_key. This would be much cleaner and clearer.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I ended up being able to remove get_container_from_key from the public API without having to add a get_container_obj method. I had to add container_pk to ContainerMetadata (since ContainerLink needed it), but this seems cleaner to me too.

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
Expand All @@ -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,
})
Expand All @@ -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
1 change: 1 addition & 0 deletions openedx/core/djangoapps/content/search/tests/test_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
24 changes: 24 additions & 0 deletions openedx/core/djangoapps/content/search/tests/test_documents.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand All @@ -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",
],
},
},
}

Expand Down Expand Up @@ -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,
Expand All @@ -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",
],
},
},
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down
17 changes: 14 additions & 3 deletions openedx/core/djangoapps/content_libraries/api/containers.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

@bradenmacdonald bradenmacdonald Apr 30, 2025 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is fine to leave as-is, but I'm personally not a huge fan of this API style of taking an ID and optionally a pre-loaded object.

I would suggest instead:

def get_container(
    container_or_key: LibraryContainerLocator | Container,
    /, *,
    include_collections=False,
) -> ContainerMetadata:
    """
    Get metadata about a container (a Section, Subsection, or Unit).
    """
    container = container_or_key if isinstance(container_or_key, Container) else _get_container_from_key(container_or_key)

Otherwise, I don't think the messiness it adds to the code from the API consumer side is worth the two tiny database queries it saves.

The /, * will help ensure better compatibility with similar changes to the API in the future. And I think it's better to make this change now to bake it into the new Teak API contract, if others agree. Though all the containers APIs are considered unstable and subject to change anyways.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Reverted this, cf 49557f5

if include_collections:
associated_collections = authoring_api.get_entity_collections(
container.publishable_entity.learning_package_id,
Expand Down Expand Up @@ -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(
Expand Down
1 change: 1 addition & 0 deletions openedx/core/djangoapps/content_libraries/api/libraries.py
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,7 @@ class LibraryItem:
created: datetime
modified: datetime
display_name: str
tags_count: int = 0


@dataclass(frozen=True, kw_only=True)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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?
Comment thread
pomegranited marked this conversation as resolved.
Outdated
if container_key.container_type == api.ContainerType.Unit.value:
data = serializers.LibraryXBlockMetadataSerializer(child_entities, many=True).data
else:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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)
Expand Down