Skip to content
Merged
6 changes: 3 additions & 3 deletions cms/djangoapps/contentstore/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -354,7 +354,7 @@ def summarize_by_downstream_context(cls, downstream_context_key: CourseKey) -> Q
@classmethod
def update_or_create(
cls,
upstream_container: Container | None,
upstream_container_id: int | None,
/,
upstream_container_key: LibraryContainerLocator,
upstream_context_key: str,
Expand All @@ -377,8 +377,8 @@ def update_or_create(
'version_synced': version_synced,
'version_declined': version_declined,
}
if upstream_container:
new_values['upstream_container'] = upstream_container
if upstream_container_id:
new_values['upstream_container_id'] = upstream_container_id
try:
link = cls.objects.get(downstream_usage_key=downstream_usage_key)
has_changes = False
Expand Down
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
4 changes: 2 additions & 2 deletions cms/djangoapps/contentstore/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
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
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
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
42 changes: 30 additions & 12 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 @@ -347,13 +349,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
Copy link
Contributor

@bradenmacdonald bradenmacdonald May 1, 2025

Choose a reason for hiding this comment

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

Not a change for this PR: but in the future, I think we should just refactor collections to be an independent API like tagging. Right now we have get_container(object_id, include_collections=True) and container.collections and set_container_collections(...) etc. as well as similar things for components. But why not just get_collections(entity), add_to_collection(entity), etc. that works with any library item? I think that would be cleaner and simpler.

Edit: never mind, this just seems to be a problem with the REST API. I don't understand why we have LibraryBlockCollectionsView and LibraryContainerCollectionsView for updating specific item's collections when we have LibraryCollectionsView.update_items which does the same thing generically.

else:
log.warning(f"Unexpected key type for {object_id}")

Expand All @@ -364,8 +363,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

Expand Down Expand Up @@ -581,9 +580,13 @@ def searchable_doc_for_container(
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")
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,
)
publish_status = PublishStatus.published
if container.last_published is None:
publish_status = PublishStatus.never
Expand All @@ -594,7 +597,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 @@ -603,10 +612,19 @@ 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,
)
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 @@ -238,6 +238,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 @@ -531,6 +531,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 @@ -571,6 +574,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 @@ -585,6 +593,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 @@ -627,6 +640,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 @@ -641,6 +660,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
7 changes: 6 additions & 1 deletion openedx/core/djangoapps/content_libraries/api/collections.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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
Expand Down
28 changes: 17 additions & 11 deletions openedx/core/djangoapps/content_libraries/api/containers.py
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,6 @@
"ContainerMetadata",
"ContainerType",
# API methods
"get_container_from_key",
"get_container",
"create_container",
"get_container_children",
Expand Down Expand Up @@ -111,6 +110,7 @@ class ContainerMetadata(PublishableItem):
"""
container_key: LibraryContainerLocator
container_type: ContainerType
container_pk: int
published_display_name: str | None

@classmethod
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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

Expand All @@ -192,11 +193,15 @@ 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,
) -> ContainerMetadata:
"""
Get a container (a Section, Subsection, or Unit).
"""
container = get_container_from_key(container_key)
container = _get_container_from_key(container_key)
if include_collections:
associated_collections = authoring_api.get_entity_collections(
container.publishable_entity.learning_package_id,
Expand Down Expand Up @@ -268,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
Expand Down Expand Up @@ -297,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,
Expand Down Expand Up @@ -332,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,
Expand Down Expand Up @@ -372,12 +377,13 @@ def restore_container(container_key: LibraryContainerLocator) -> None:

def get_container_children(
container_key: LibraryContainerLocator,
*,
published=False,
) -> 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)
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(
Expand All @@ -399,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)


Expand All @@ -414,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]
Expand Down Expand Up @@ -459,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
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
Loading
Loading