diff --git a/openedx/core/djangoapps/content/search/api.py b/openedx/core/djangoapps/content/search/api.py index 49bf4be7687d..9b57a9c70938 100644 --- a/openedx/core/djangoapps/content/search/api.py +++ b/openedx/core/djangoapps/content/search/api.py @@ -17,37 +17,39 @@ from meilisearch import Client as MeilisearchClient from meilisearch.errors import MeilisearchApiError, MeilisearchError from meilisearch.models.task import TaskInfo -from opaque_keys.edx.keys import UsageKey, OpaqueKey +from opaque_keys import OpaqueKey +from opaque_keys.edx.keys import UsageKey from opaque_keys.edx.locator import ( LibraryCollectionLocator, LibraryContainerLocator, LibraryLocatorV2, ) from openedx_learning.api import authoring as authoring_api -from common.djangoapps.student.roles import GlobalStaff from rest_framework.request import Request + from common.djangoapps.student.role_helpers import get_course_roles +from common.djangoapps.student.roles import GlobalStaff from openedx.core.djangoapps.content.course_overviews.models import CourseOverview -from openedx.core.djangoapps.content.search.models import get_access_ids_for_request, IncrementalIndexCompleted from openedx.core.djangoapps.content.search.index_config import ( INDEX_DISTINCT_ATTRIBUTE, INDEX_FILTERABLE_ATTRIBUTES, - INDEX_SEARCHABLE_ATTRIBUTES, - INDEX_SORTABLE_ATTRIBUTES, INDEX_RANKING_RULES, + INDEX_SEARCHABLE_ATTRIBUTES, + INDEX_SORTABLE_ATTRIBUTES ) +from openedx.core.djangoapps.content.search.models import IncrementalIndexCompleted, get_access_ids_for_request from openedx.core.djangoapps.content_libraries import api as lib_api from xmodule.modulestore.django import modulestore from .documents import ( Fields, meili_id_from_opaque_key, - searchable_doc_for_course_block, + searchable_doc_collections, searchable_doc_for_collection, searchable_doc_for_container, + searchable_doc_for_course_block, searchable_doc_for_library_block, searchable_doc_for_key, - searchable_doc_collections, searchable_doc_tags, searchable_doc_tags_for_collection, ) @@ -492,6 +494,7 @@ def index_container_batch(batch, num_done, library_key) -> int: ) doc = searchable_doc_for_container(container_key) doc.update(searchable_doc_tags(container_key)) + doc.update(searchable_doc_collections(container_key)) docs.append(doc) except Exception as err: # pylint: disable=broad-except status_cb(f"Error indexing container {container.key}: {err}") @@ -722,7 +725,7 @@ def upsert_library_collection_index_doc(collection_key: LibraryCollectionLocator _delete_index_doc(doc[Fields.id]) - update_components = True + update_items = True # Hard-deleted collections are also deleted from the index, # but their components are automatically updated as part of the deletion process, so we don't have to. @@ -735,15 +738,17 @@ def upsert_library_collection_index_doc(collection_key: LibraryCollectionLocator else: already_indexed = _get_document_from_index(doc[Fields.id]) if not already_indexed: - update_components = True + update_items = True _update_index_docs([doc]) # Asynchronously update the collection's components "collections" field - if update_components: - from .tasks import update_library_components_collections as update_task + if update_items: + from .tasks import update_library_components_collections as update_components_task + from .tasks import update_library_containers_collections as update_containers_task - update_task.delay(str(collection_key)) + update_components_task.delay(str(collection_key)) + update_containers_task.delay(str(collection_key)) def update_library_components_collections( @@ -781,6 +786,41 @@ def update_library_components_collections( _update_index_docs(docs) +def update_library_containers_collections( + collection_key: LibraryCollectionLocator, + batch_size: int = 1000, +) -> None: + """ + Updates the "collections" field for all containers associated with a given Library Collection. + + Because there may be a lot of containers, we send these updates to Meilisearch in batches. + """ + library_key = collection_key.library_key + library = lib_api.get_library(library_key) + containers = authoring_api.get_collection_containers( + library.learning_package_id, + collection_key.collection_id, + ) + + paginator = Paginator(containers, batch_size) + for page in paginator.page_range: + docs = [] + + for container in paginator.page(page).object_list: + container_key = lib_api.library_container_locator( + library_key, + container, + ) + doc = searchable_doc_collections(container_key) + docs.append(doc) + + log.info( + f"Updating document.collections for library {library_key} containers" + f" page {page} / {paginator.num_pages}" + ) + _update_index_docs(docs) + + def upsert_library_container_index_doc(container_key: LibraryContainerLocator) -> None: """ Creates, updates, or deletes the document for the given Library Container in the search index. @@ -827,12 +867,12 @@ def upsert_content_object_tags_index_doc(key: OpaqueKey): _update_index_docs([doc]) -def upsert_block_collections_index_docs(usage_key: UsageKey): +def upsert_item_collections_index_docs(opaque_key: OpaqueKey): """ - Updates the collections data in documents for the given Course/Library block + Updates the collections data in documents for the given Course/Library block, or Container """ - doc = {Fields.id: meili_id_from_opaque_key(usage_key)} - doc.update(searchable_doc_collections(usage_key)) + doc = {Fields.id: meili_id_from_opaque_key(opaque_key)} + doc.update(searchable_doc_collections(opaque_key)) _update_index_docs([doc]) diff --git a/openedx/core/djangoapps/content/search/documents.py b/openedx/core/djangoapps/content/search/documents.py index 1c093034271d..c07048057a3f 100644 --- a/openedx/core/djangoapps/content/search/documents.py +++ b/openedx/core/djangoapps/content/search/documents.py @@ -309,7 +309,7 @@ def _tags_for_content_object(object_id: OpaqueKey) -> dict: return {Fields.tags: result} -def _collections_for_content_object(object_id: UsageKey | LearningContextKey) -> dict: +def _collections_for_content_object(object_id: OpaqueKey) -> dict: """ Given an XBlock, course, library, etc., get the collections for its index doc. @@ -340,13 +340,23 @@ def _collections_for_content_object(object_id: UsageKey | LearningContextKey) -> # Gather the collections associated with this object collections = None try: - component = lib_api.get_component_from_usage_key(object_id) - collections = authoring_api.get_entity_collections( - component.learning_package_id, - component.key, - ) + if isinstance(object_id, UsageKey): + component = lib_api.get_component_from_usage_key(object_id) + collections = authoring_api.get_entity_collections( + component.learning_package_id, + component.key, + ) + 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, + ) + else: + log.warning(f"Unexpected key type for {object_id}") + except ObjectDoesNotExist: - log.warning(f"No component found for {object_id}") + log.warning(f"No library item found for {object_id}") if not collections: return result @@ -438,13 +448,13 @@ def searchable_doc_tags(key: OpaqueKey) -> dict: return doc -def searchable_doc_collections(usage_key: UsageKey) -> dict: +def searchable_doc_collections(opaque_key: OpaqueKey) -> dict: """ Generate a dictionary document suitable for ingestion into a search engine like Meilisearch or Elasticsearch, with the collections data for the given content object. """ - doc = searchable_doc_for_key(usage_key) - doc.update(_collections_for_content_object(usage_key)) + doc = searchable_doc_for_key(opaque_key) + doc.update(_collections_for_content_object(opaque_key)) return doc diff --git a/openedx/core/djangoapps/content/search/handlers.py b/openedx/core/djangoapps/content/search/handlers.py index d817de1519bd..998b2ef870ab 100644 --- a/openedx/core/djangoapps/content/search/handlers.py +++ b/openedx/core/djangoapps/content/search/handlers.py @@ -40,9 +40,9 @@ from .api import ( only_if_meilisearch_enabled, - upsert_block_collections_index_docs, upsert_content_object_tags_index_doc, upsert_collection_tags_index_docs, + upsert_item_collections_index_docs, ) from .tasks import ( delete_library_block_index_doc, @@ -211,15 +211,15 @@ def content_object_associations_changed_handler(**kwargs) -> None: try: # Check if valid course or library block - usage_key = UsageKey.from_string(str(content_object.object_id)) + opaque_key = UsageKey.from_string(str(content_object.object_id)) except InvalidKeyError: try: # Check if valid library collection - usage_key = LibraryCollectionLocator.from_string(str(content_object.object_id)) + opaque_key = LibraryCollectionLocator.from_string(str(content_object.object_id)) except InvalidKeyError: try: # Check if valid library container - usage_key = LibraryContainerLocator.from_string(str(content_object.object_id)) + opaque_key = LibraryContainerLocator.from_string(str(content_object.object_id)) except InvalidKeyError: # Invalid content object id log.error("Received invalid content object id") @@ -228,12 +228,12 @@ def content_object_associations_changed_handler(**kwargs) -> None: # This event's changes may contain both "tags" and "collections", but this will happen rarely, if ever. # So we allow a potential double "upsert" here. if not content_object.changes or "tags" in content_object.changes: - if isinstance(usage_key, LibraryCollectionLocator): - upsert_collection_tags_index_docs(usage_key) + if isinstance(opaque_key, LibraryCollectionLocator): + upsert_collection_tags_index_docs(opaque_key) else: - upsert_content_object_tags_index_doc(usage_key) + upsert_content_object_tags_index_doc(opaque_key) if not content_object.changes or "collections" in content_object.changes: - upsert_block_collections_index_docs(usage_key) + upsert_item_collections_index_docs(opaque_key) @receiver(LIBRARY_CONTAINER_CREATED) diff --git a/openedx/core/djangoapps/content/search/tasks.py b/openedx/core/djangoapps/content/search/tasks.py index 48fc6b327c0a..a1d47fc6f92a 100644 --- a/openedx/core/djangoapps/content/search/tasks.py +++ b/openedx/core/djangoapps/content/search/tasks.py @@ -119,6 +119,20 @@ def update_library_components_collections(collection_key_str: str) -> None: api.update_library_components_collections(collection_key) +@shared_task(base=LoggedTask, autoretry_for=(MeilisearchError, ConnectionError)) +@set_code_owner_attribute +def update_library_containers_collections(collection_key_str: str) -> None: + """ + Celery task to update the "collections" field for containers in the given content library collection. + """ + collection_key = LibraryCollectionLocator.from_string(collection_key_str) + library_key = collection_key.library_key + + log.info("Updating document.collections for library %s collection %s containers", library_key, collection_key) + + api.update_library_containers_collections(collection_key) + + @shared_task(base=LoggedTask, autoretry_for=(MeilisearchError, ConnectionError)) @set_code_owner_attribute def update_library_container_index_doc(container_key_str: str) -> None: diff --git a/openedx/core/djangoapps/content/search/tests/test_api.py b/openedx/core/djangoapps/content/search/tests/test_api.py index bd9fb803c1aa..8a486aa99c60 100644 --- a/openedx/core/djangoapps/content/search/tests/test_api.py +++ b/openedx/core/djangoapps/content/search/tests/test_api.py @@ -265,6 +265,7 @@ def test_reindex_meilisearch(self, mock_meilisearch): doc_collection["tags"] = {} doc_unit = copy.deepcopy(self.unit_dict) doc_unit["tags"] = {} + doc_unit["collections"] = {'display_name': [], 'key': []} api.rebuild_index() assert mock_meilisearch.return_value.index.return_value.add_documents.call_count == 4 @@ -296,6 +297,7 @@ def test_reindex_meilisearch_incremental(self, mock_meilisearch): doc_collection["tags"] = {} doc_unit = copy.deepcopy(self.unit_dict) doc_unit["tags"] = {} + doc_unit["collections"] = {"display_name": [], "key": []} api.rebuild_index(incremental=True) assert mock_meilisearch.return_value.index.return_value.add_documents.call_count == 4 @@ -584,16 +586,16 @@ def test_index_library_block_and_collections(self, mock_meilisearch): description="Second Collection", ) - # Add Problem1 to both Collections (these internally call `upsert_block_collections_index_docs` and + # Add Problem1 to both Collections (these internally call `upsert_item_collections_index_docs` and # `upsert_library_collection_index_doc`) # (adding in reverse order to test sorting of collection tag) updated_date = datetime(2023, 6, 7, 8, 9, 10, tzinfo=timezone.utc) with freeze_time(updated_date): for collection in (collection2, collection1): - library_api.update_library_collection_components( + library_api.update_library_collection_items( self.library.key, collection_key=collection.key, - usage_keys=[ + opaque_keys=[ self.problem1.usage_key, ], ) @@ -776,16 +778,17 @@ def test_delete_collection(self, mock_meilisearch): # Add a component to the collection updated_date = datetime(2023, 6, 7, 8, 9, 10, tzinfo=timezone.utc) with freeze_time(updated_date): - library_api.update_library_collection_components( + library_api.update_library_collection_items( self.library.key, collection_key=self.collection.key, - usage_keys=[ + opaque_keys=[ self.problem1.usage_key, + self.unit.container_key ], ) doc_collection = copy.deepcopy(self.collection_dict) - doc_collection["num_children"] = 1 + doc_collection["num_children"] = 2 doc_collection["modified"] = updated_date.timestamp() doc_problem_with_collection = { "id": self.doc_problem1["id"], @@ -794,13 +797,21 @@ def test_delete_collection(self, mock_meilisearch): "key": [self.collection.key], }, } + doc_unit_with_collection = { + "id": self.unit_dict["id"], + "collections": { + "display_name": [self.collection.title], + "key": [self.collection.key], + }, + } # Should update the collection and its component - assert mock_meilisearch.return_value.index.return_value.update_documents.call_count == 2 + assert mock_meilisearch.return_value.index.return_value.update_documents.call_count == 3 mock_meilisearch.return_value.index.return_value.update_documents.assert_has_calls( [ call([doc_collection]), call([doc_problem_with_collection]), + call([doc_unit_with_collection]), ], any_order=True, ) @@ -816,15 +827,23 @@ def test_delete_collection(self, mock_meilisearch): "id": self.doc_problem1["id"], "collections": {'display_name': [], 'key': []}, } + doc_unit_without_collection = { + "id": self.unit_dict["id"], + "collections": {'display_name': [], 'key': []}, + } # Should delete the collection document mock_meilisearch.return_value.index.return_value.delete_document.assert_called_once_with( self.collection_dict["id"], ) # ...and update the component's "collections" field - mock_meilisearch.return_value.index.return_value.update_documents.assert_called_once_with([ - doc_problem_without_collection, - ]) + mock_meilisearch.return_value.index.return_value.update_documents.assert_has_calls( + [ + call([doc_problem_without_collection]), + call([doc_unit_without_collection]), + ], + any_order=True, + ) mock_meilisearch.return_value.index.reset_mock() # We need to mock get_document here so that when we restore the collection below, meilisearch knows the @@ -840,15 +859,16 @@ def test_delete_collection(self, mock_meilisearch): ) doc_collection = copy.deepcopy(self.collection_dict) - doc_collection["num_children"] = 1 + doc_collection["num_children"] = 2 doc_collection["modified"] = restored_date.timestamp() # Should update the collection and its component's "collections" field - assert mock_meilisearch.return_value.index.return_value.update_documents.call_count == 2 + assert mock_meilisearch.return_value.index.return_value.update_documents.call_count == 3 mock_meilisearch.return_value.index.return_value.update_documents.assert_has_calls( [ call([doc_collection]), call([doc_problem_with_collection]), + call([doc_unit_with_collection]), ], any_order=True, ) @@ -866,9 +886,13 @@ def test_delete_collection(self, mock_meilisearch): self.collection_dict["id"], ) # ...and cascade delete updates the "collections" field for the associated components - mock_meilisearch.return_value.index.return_value.update_documents.assert_called_once_with([ - doc_problem_without_collection, - ]) + mock_meilisearch.return_value.index.return_value.update_documents.assert_has_calls( + [ + call([doc_problem_without_collection]), + call([doc_unit_without_collection]), + ], + any_order=True, + ) @override_settings(MEILISEARCH_ENABLED=True) def test_delete_index_container(self, mock_meilisearch): diff --git a/openedx/core/djangoapps/content/search/tests/test_documents.py b/openedx/core/djangoapps/content/search/tests/test_documents.py index 1f6c56f7d691..c4c3213ce33d 100644 --- a/openedx/core/djangoapps/content/search/tests/test_documents.py +++ b/openedx/core/djangoapps/content/search/tests/test_documents.py @@ -102,10 +102,10 @@ def setUpClass(cls): ) # Add the problem block to the collection - library_api.update_library_collection_components( + library_api.update_library_collection_items( cls.library.key, collection_key="TOY_COLLECTION", - usage_keys=[ + opaque_keys=[ cls.library_block.usage_key, ] ) diff --git a/openedx/core/djangoapps/content_libraries/api/blocks.py b/openedx/core/djangoapps/content_libraries/api/blocks.py index 04649a83503f..5a328594cc21 100644 --- a/openedx/core/djangoapps/content_libraries/api/blocks.py +++ b/openedx/core/djangoapps/content_libraries/api/blocks.py @@ -3,52 +3,46 @@ These methods don't enforce permissions (only the REST APIs do). """ -from dataclasses import dataclass -from datetime import datetime, timezone import logging import mimetypes +from dataclasses import dataclass +from datetime import datetime, timezone from django.conf import settings from django.core.exceptions import ObjectDoesNotExist from django.core.validators import validate_unicode_slug -from django.db.models import QuerySet from django.db import transaction -from django.utils.translation import gettext as _ +from django.db.models import QuerySet from django.urls import reverse +from django.utils.translation import gettext as _ from lxml import etree -from opaque_keys.edx.locator import LibraryLocatorV2, LibraryUsageLocatorV2 from opaque_keys.edx.keys import UsageKeyV2 +from opaque_keys.edx.locator import LibraryLocatorV2, LibraryUsageLocatorV2 from openedx_events.content_authoring.data import ( + ContentObjectChangedData, LibraryBlockData, LibraryCollectionData, - LibraryContainerData, - ContentObjectChangedData, + LibraryContainerData ) from openedx_events.content_authoring.signals import ( + CONTENT_OBJECT_ASSOCIATIONS_CHANGED, LIBRARY_BLOCK_CREATED, LIBRARY_BLOCK_DELETED, LIBRARY_BLOCK_UPDATED, LIBRARY_COLLECTION_UPDATED, - LIBRARY_CONTAINER_UPDATED, - CONTENT_OBJECT_ASSOCIATIONS_CHANGED, + LIBRARY_CONTAINER_UPDATED ) -from xblock.core import XBlock - from openedx_learning.api import authoring as authoring_api -from openedx_learning.api.authoring_models import ( - Component, - ComponentVersion, - LearningPackage, - MediaType, -) +from openedx_learning.api.authoring_models import Component, ComponentVersion, LearningPackage, MediaType +from xblock.core import XBlock +from openedx.core.djangoapps.content_libraries import api as lib_api from openedx.core.djangoapps.xblock.api import ( get_component_from_usage_key, get_xblock_app_config, - xblock_type_display_name, + xblock_type_display_name ) from openedx.core.types import User as UserType -from openedx.core.djangoapps.content_libraries import api as lib_api from ..models import ContentLibrary from ..permissions import CAN_EDIT_THIS_CONTENT_LIBRARY diff --git a/openedx/core/djangoapps/content_libraries/api/collections.py b/openedx/core/djangoapps/content_libraries/api/collections.py index f94c9e5401a1..8e20710ab86a 100644 --- a/openedx/core/djangoapps/content_libraries/api/collections.py +++ b/openedx/core/djangoapps/content_libraries/api/collections.py @@ -3,35 +3,28 @@ ================================== """ from django.db import IntegrityError +from opaque_keys import OpaqueKey from opaque_keys.edx.keys import BlockTypeKey, UsageKeyV2 -from opaque_keys.edx.locator import ( - LibraryLocatorV2, - LibraryCollectionLocator, -) - +from opaque_keys.edx.locator import LibraryCollectionLocator, LibraryContainerLocator, LibraryLocatorV2 from openedx_events.content_authoring.data import LibraryCollectionData from openedx_events.content_authoring.signals import LIBRARY_COLLECTION_UPDATED - from openedx_learning.api import authoring as authoring_api -from openedx_learning.api.authoring_models import ( - Collection, - Component, - PublishableEntity, -) +from openedx_learning.api.authoring_models import Collection, Component, PublishableEntity +from ..models import ContentLibrary from .exceptions import ( ContentLibraryBlockNotFound, ContentLibraryCollectionNotFound, + ContentLibraryContainerNotFound, LibraryCollectionAlreadyExists, ) -from ..models import ContentLibrary # The public API is only the following symbols: __all__ = [ "create_library_collection", "update_library_collection", - "update_library_collection_components", - "set_library_component_collections", + "update_library_collection_items", + "set_library_item_collections", "library_collection_locator", "get_library_collection_from_locator", ] @@ -103,27 +96,28 @@ def update_library_collection( return collection -def update_library_collection_components( +def update_library_collection_items( library_key: LibraryLocatorV2, collection_key: str, *, - usage_keys: list[UsageKeyV2], + opaque_keys: list[OpaqueKey], created_by: int | None = None, remove=False, # As an optimization, callers may pass in a pre-fetched ContentLibrary instance content_library: ContentLibrary | None = None, ) -> Collection: """ - Associates the Collection with Components for the given UsageKeys. + Associates the Collection with items (XBlocks, Containers) for the given OpaqueKeys. - By default the Components are added to the Collection. - If remove=True, the Components are removed from the Collection. + By default the items are added to the Collection. + If remove=True, the items are removed from the Collection. If you've already fetched the ContentLibrary, pass it in to avoid refetching. Raises: * ContentLibraryCollectionNotFound if no Collection with the given pk is found in the given library. - * ContentLibraryBlockNotFound if any of the given usage_keys don't match Components in the given library. + * ContentLibraryBlockNotFound if any of the given opaque_keys don't match Components in the given library. + * ContentLibraryContainerNotFound if any of the given opaque_keys don't match Containers in the given library. Returns the updated Collection. """ @@ -134,26 +128,38 @@ def update_library_collection_components( assert content_library.library_key == library_key # Fetch the Component.key values for the provided UsageKeys. - component_keys = [] - for usage_key in usage_keys: - # Parse the block_family from the key to use as namespace. - block_type = BlockTypeKey.from_string(str(usage_key)) - - try: - component = authoring_api.get_component_by_key( - content_library.learning_package_id, - namespace=block_type.block_family, - type_name=usage_key.block_type, - local_key=usage_key.block_id, - ) - except Component.DoesNotExist as exc: - raise ContentLibraryBlockNotFound(usage_key) from exc + item_keys = [] + for opaque_key in opaque_keys: + if isinstance(opaque_key, LibraryContainerLocator): + try: + container = authoring_api.get_container_by_key( + content_library.learning_package_id, + key=opaque_key.container_id, + ) + except Collection.DoesNotExist as exc: + raise ContentLibraryContainerNotFound(opaque_key) from exc + + item_keys.append(container.key) + elif isinstance(opaque_key, UsageKeyV2): + # Parse the block_family from the key to use as namespace. + block_type = BlockTypeKey.from_string(str(opaque_key)) + try: + component = authoring_api.get_component_by_key( + content_library.learning_package_id, + namespace=block_type.block_family, + type_name=opaque_key.block_type, + local_key=opaque_key.block_id, + ) + except Component.DoesNotExist as exc: + raise ContentLibraryBlockNotFound(opaque_key) from exc + + item_keys.append(component.key) + else: + # This should never happen, but just in case. + raise ValueError(f"Invalid opaque_key: {opaque_key}") - component_keys.append(component.key) - - # Note: Component.key matches its PublishableEntity.key entities_qset = PublishableEntity.objects.filter( - key__in=component_keys, + key__in=item_keys, ) if remove: @@ -173,19 +179,19 @@ def update_library_collection_components( return collection -def set_library_component_collections( +def set_library_item_collections( library_key: LibraryLocatorV2, - component: Component, + publishable_entity: PublishableEntity, *, collection_keys: list[str], created_by: int | None = None, # As an optimization, callers may pass in a pre-fetched ContentLibrary instance content_library: ContentLibrary | None = None, -) -> Component: +) -> PublishableEntity: """ - It Associates the component with collections for the given collection keys. + It Associates the publishable_entity with collections for the given collection keys. - Only collections in queryset are associated with component, all previous component-collections + Only collections in queryset are associated with publishable_entity, all previous publishable_entity-collections associations are removed. If you've already fetched the ContentLibrary, pass it in to avoid refetching. @@ -193,7 +199,7 @@ def set_library_component_collections( Raises: * ContentLibraryCollectionNotFound if any of the given collection_keys don't match Collections in the given library. - Returns the updated Component. + Returns the updated PublishableEntity. """ if not content_library: content_library = ContentLibrary.objects.get_by_key(library_key) # type: ignore[attr-defined] @@ -207,8 +213,7 @@ def set_library_component_collections( ) affected_collections = authoring_api.set_collections( - content_library.learning_package_id, - component, + publishable_entity, collection_qs, created_by=created_by, ) @@ -226,7 +231,7 @@ def set_library_component_collections( ) ) - return component + return publishable_entity def library_collection_locator( diff --git a/openedx/core/djangoapps/content_libraries/api/containers.py b/openedx/core/djangoapps/content_libraries/api/containers.py index 31eae20e7fa6..526a556a8ef0 100644 --- a/openedx/core/djangoapps/content_libraries/api/containers.py +++ b/openedx/core/djangoapps/content_libraries/api/containers.py @@ -9,20 +9,19 @@ from uuid import uuid4 from django.utils.text import slugify -from opaque_keys.edx.locator import ( - LibraryContainerLocator, - LibraryLocatorV2, - UsageKeyV2, - LibraryUsageLocatorV2, -) -from openedx_events.content_authoring.data import LibraryContainerData +from opaque_keys.edx.keys import UsageKeyV2 +from opaque_keys.edx.locator import LibraryContainerLocator, LibraryLocatorV2, LibraryUsageLocatorV2 +from openedx_events.content_authoring.data import ContentObjectChangedData, LibraryCollectionData, 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_learning.api import authoring as authoring_api from openedx_learning.api.authoring_models import Container +from openedx.core.djangoapps.content_libraries.api.collections import library_collection_locator from openedx.core.djangoapps.xblock.api import get_component_from_usage_key @@ -30,13 +29,13 @@ from .exceptions import ContentLibraryContainerNotFound from .libraries import LibraryXBlockMetadata, PublishableItem - # The public API is only the following symbols: __all__ = [ # Models "ContainerMetadata", "ContainerType", # API methods + "get_container_from_key", "get_container", "create_container", "get_container_children", @@ -122,7 +121,7 @@ def library_container_locator( ) -def _get_container(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 @@ -141,12 +140,23 @@ def _get_container(container_key: LibraryContainerLocator, isDeleted=False) -> C raise ContentLibraryContainerNotFound -def get_container(container_key: LibraryContainerLocator) -> ContainerMetadata: +def get_container(container_key: LibraryContainerLocator, include_collections=False) -> ContainerMetadata: """ Get a container (a Section, Subsection, or Unit). """ - container = _get_container(container_key) - container_meta = ContainerMetadata.from_container(container_key.library_key, container) + container = get_container_from_key(container_key) + if include_collections: + associated_collections = authoring_api.get_entity_collections( + container.publishable_entity.learning_package_id, + container_key.container_id, + ).values('key', 'title') + else: + associated_collections = None + container_meta = ContainerMetadata.from_container( + container_key.library_key, + container, + associated_collections=associated_collections, + ) assert container_meta.container_type.value == container_key.container_type return container_meta @@ -205,7 +215,7 @@ def update_container( """ Update a container (e.g. a Unit) title. """ - container = _get_container(container_key) + container = get_container_from_key(container_key) library_key = container_key.library_key assert container.unit @@ -233,8 +243,13 @@ def delete_container( No-op if container doesn't exist or has already been soft-deleted. """ - container = _get_container(container_key) + library_key = container_key.library_key + container = get_container_from_key(container_key) + affected_collections = authoring_api.get_entity_collections( + container.publishable_entity.learning_package_id, + container.key, + ) authoring_api.soft_delete_draft(container.pk) LIBRARY_CONTAINER_DELETED.send_event( @@ -243,14 +258,33 @@ def delete_container( ) ) - # TODO: trigger a LIBRARY_COLLECTION_UPDATED for each collection the container was in + # 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: + LIBRARY_COLLECTION_UPDATED.send_event( + library_collection=LibraryCollectionData( + collection_key=library_collection_locator( + library_key=library_key, + collection_key=collection.key, + ), + background=True, + ) + ) def restore_container(container_key: LibraryContainerLocator) -> None: """ Restore the specified library container. """ - container = _get_container(container_key, isDeleted=True) + library_key = container_key.library_key + container = get_container_from_key(container_key, isDeleted=True) + + affected_collections = authoring_api.get_entity_collections( + container.publishable_entity.learning_package_id, + container.key, + ) authoring_api.set_draft_version(container.pk, container.versioning.latest.pk) @@ -260,6 +294,28 @@ def restore_container(container_key: LibraryContainerLocator) -> None: ) ) + # Add tags and collections back to index + CONTENT_OBJECT_ASSOCIATIONS_CHANGED.send_event( + content_object=ContentObjectChangedData( + object_id=str(container_key), + changes=["collections", "tags"], + ), + ) + + # 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: + LIBRARY_COLLECTION_UPDATED.send_event( + library_collection=LibraryCollectionData( + collection_key=library_collection_locator( + library_key=library_key, + collection_key=collection.key, + ), + ) + ) + def get_container_children( container_key: LibraryContainerLocator, @@ -268,7 +324,7 @@ def get_container_children( """ Get the entities contained in the given container (e.g. the components/xblocks in a unit) """ - container = _get_container(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( @@ -290,7 +346,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(container_key) + container = get_container_from_key(container_key) return authoring_api.get_container_children_count(container, published=published) @@ -305,7 +361,7 @@ def update_container_children( """ library_key = container_key.library_key container_type = container_key.container_type - container = _get_container(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] diff --git a/openedx/core/djangoapps/content_libraries/rest_api/blocks.py b/openedx/core/djangoapps/content_libraries/rest_api/blocks.py index 192b90964c0d..cf49f8068c02 100644 --- a/openedx/core/djangoapps/content_libraries/rest_api/blocks.py +++ b/openedx/core/djangoapps/content_libraries/rest_api/blocks.py @@ -1,12 +1,15 @@ """ Content Library REST APIs related to XBlocks/Components and their static assets """ +import edx_api_doc_tools as apidocs from django.core.exceptions import ObjectDoesNotExist from django.db.transaction import non_atomic_requests from django.http import Http404, HttpResponse, StreamingHttpResponse from django.urls import reverse from django.utils.decorators import method_decorator from drf_yasg.utils import swagger_auto_schema +from opaque_keys.edx.locator import LibraryLocatorV2, LibraryUsageLocatorV2 +from openedx_learning.api import authoring as authoring_api from rest_framework import status from rest_framework.exceptions import NotFound, ValidationError from rest_framework.generics import GenericAPIView @@ -14,22 +17,18 @@ from rest_framework.response import Response from rest_framework.views import APIView -import edx_api_doc_tools as apidocs -from opaque_keys.edx.locator import LibraryLocatorV2, LibraryUsageLocatorV2 -from openedx_learning.api import authoring as authoring_api - from openedx.core.djangoapps.content_libraries import api, permissions from openedx.core.djangoapps.content_libraries.rest_api.serializers import ( - ContentLibraryComponentCollectionsUpdateSerializer, + ContentLibraryItemCollectionsUpdateSerializer, LibraryXBlockCreationSerializer, LibraryXBlockMetadataSerializer, LibraryXBlockOlxSerializer, LibraryXBlockStaticFileSerializer, LibraryXBlockStaticFilesSerializer, ) +from openedx.core.djangoapps.xblock import api as xblock_api from openedx.core.lib.api.view_utils import view_auth_classes from openedx.core.types.http import RestRequest -from openedx.core.djangoapps.xblock import api as xblock_api from .libraries import LibraryApiPaginationDocs from .utils import convert_exceptions @@ -259,13 +258,13 @@ def patch(self, request: RestRequest, usage_key_str) -> Response: permissions.CAN_EDIT_THIS_CONTENT_LIBRARY ) component = api.get_component_from_usage_key(key) - serializer = ContentLibraryComponentCollectionsUpdateSerializer(data=request.data) + serializer = ContentLibraryItemCollectionsUpdateSerializer(data=request.data) serializer.is_valid(raise_exception=True) collection_keys = serializer.validated_data['collection_keys'] - api.set_library_component_collections( + api.set_library_item_collections( library_key=key.lib_key, - component=component, + publishable_entity=component.publishable_entity, collection_keys=collection_keys, created_by=request.user.id, content_library=content_library, diff --git a/openedx/core/djangoapps/content_libraries/rest_api/collections.py b/openedx/core/djangoapps/content_libraries/rest_api/collections.py index c49822ae2f0f..d893d766d80f 100644 --- a/openedx/core/djangoapps/content_libraries/rest_api/collections.py +++ b/openedx/core/djangoapps/content_libraries/rest_api/collections.py @@ -22,7 +22,7 @@ from .serializers import ( ContentLibraryCollectionSerializer, ContentLibraryCollectionUpdateSerializer, - ContentLibraryComponentKeysSerializer, + ContentLibraryItemKeysSerializer, ) from openedx.core.types.http import RestRequest @@ -190,27 +190,27 @@ def restore(self, request: RestRequest, *args, **kwargs) -> Response: return Response(None, status=HTTP_204_NO_CONTENT) @convert_exceptions - @action(detail=True, methods=['delete', 'patch'], url_path='components', url_name='components-update') - def update_components(self, request: RestRequest, *args, **kwargs) -> Response: + @action(detail=True, methods=['delete', 'patch'], url_path='items', url_name='items-update') + def update_items(self, request: RestRequest, *args, **kwargs) -> Response: """ - Adds (PATCH) or removes (DELETE) Components to/from a Collection. + Adds (PATCH) or removes (DELETE) items to/from a Collection. - Collection and Components must all be part of the given library/learning package. + Collection and items must all be part of the given library/learning package. """ content_library = self.get_content_library() collection_key = kwargs["key"] - serializer = ContentLibraryComponentKeysSerializer(data=request.data) + serializer = ContentLibraryItemKeysSerializer(data=request.data) serializer.is_valid(raise_exception=True) - usage_keys = serializer.validated_data["usage_keys"] - api.update_library_collection_components( + opaque_keys = serializer.validated_data["usage_keys"] + api.update_library_collection_items( library_key=content_library.library_key, content_library=content_library, collection_key=collection_key, - usage_keys=usage_keys, + opaque_keys=opaque_keys, created_by=request.user.id, remove=(request.method == "DELETE"), ) - return Response({'count': len(usage_keys)}) + return Response({'count': len(opaque_keys)}) diff --git a/openedx/core/djangoapps/content_libraries/rest_api/containers.py b/openedx/core/djangoapps/content_libraries/rest_api/containers.py index 3c65fc006bf6..6c76bf3a2dc6 100644 --- a/openedx/core/djangoapps/content_libraries/rest_api/containers.py +++ b/openedx/core/djangoapps/content_libraries/rest_api/containers.py @@ -18,6 +18,7 @@ from openedx.core.djangoapps.content_libraries import api, permissions from openedx.core.lib.api.view_utils import view_auth_classes +from openedx.core.types.http import RestRequest from . import serializers from .utils import convert_exceptions @@ -80,7 +81,7 @@ def get(self, request, container_key: LibraryContainerLocator): request.user, permissions.CAN_VIEW_THIS_CONTENT_LIBRARY, ) - container = api.get_container(container_key) + container = api.get_container(container_key, include_collections=True) return Response(serializers.LibraryContainerMetadataSerializer(container).data) @convert_exceptions @@ -293,3 +294,37 @@ def post(self, request, container_key: LibraryContainerLocator) -> Response: ) api.restore_container(container_key) return Response(None, status=HTTP_204_NO_CONTENT) + + +@method_decorator(non_atomic_requests, name="dispatch") +@view_auth_classes() +class LibraryContainerCollectionsView(GenericAPIView): + """ + View to set collections for a container. + """ + @convert_exceptions + def patch(self, request: RestRequest, container_key: LibraryContainerLocator) -> Response: + """ + Sets Collections for a Component. + + Collection and Components must all be part of the given library/learning package. + """ + content_library = api.require_permission_for_library_key( + container_key.library_key, + 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.library_key, + publishable_entity=container.publishable_entity, + collection_keys=collection_keys, + created_by=request.user.id, + content_library=content_library, + ) + + return Response({'count': len(collection_keys)}) diff --git a/openedx/core/djangoapps/content_libraries/rest_api/serializers.py b/openedx/core/djangoapps/content_libraries/rest_api/serializers.py index 46bec36e3c94..d171afe6ca0b 100644 --- a/openedx/core/djangoapps/content_libraries/rest_api/serializers.py +++ b/openedx/core/djangoapps/content_libraries/rest_api/serializers.py @@ -6,7 +6,9 @@ from rest_framework import serializers from rest_framework.exceptions import ValidationError +from opaque_keys import OpaqueKey from opaque_keys.edx.keys import UsageKeyV2 +from opaque_keys.edx.locator import LibraryContainerLocator from opaque_keys import InvalidKeyError from openedx_learning.api.authoring_models import Collection @@ -357,9 +359,42 @@ class ContentLibraryComponentKeysSerializer(serializers.Serializer): usage_keys = serializers.ListField(child=UsageKeyV2Serializer(), allow_empty=False) -class ContentLibraryComponentCollectionsUpdateSerializer(serializers.Serializer): +class OpaqueKeySerializer(serializers.BaseSerializer): """ - Serializer for adding/removing Collections to/from a Component. + Serializes a OpaqueKey with the correct class. + """ + def to_representation(self, value: OpaqueKey) -> str: + """ + Returns the OpaqueKey value as a string. + """ + return str(value) + + def to_internal_value(self, value: str) -> OpaqueKey: + """ + Returns a UsageKeyV2 or a LibraryContainerLocator from the string value. + + Raises ValidationError if invalid UsageKeyV2 or LibraryContainerLocator. + """ + try: + return UsageKeyV2.from_string(value) + except InvalidKeyError: + try: + return LibraryContainerLocator.from_string(value) + except InvalidKeyError as err: + raise ValidationError from err + + +class ContentLibraryItemKeysSerializer(serializers.Serializer): + """ + Serializer for adding/removing items to/from a Collection. + """ + + usage_keys = serializers.ListField(child=OpaqueKeySerializer(), allow_empty=False) + + +class ContentLibraryItemCollectionsUpdateSerializer(serializers.Serializer): + """ + Serializer for adding/removing Collections to/from a Library Item (component, unit, etc..). """ collection_keys = serializers.ListField(child=serializers.CharField(), allow_empty=True) diff --git a/openedx/core/djangoapps/content_libraries/signal_handlers.py b/openedx/core/djangoapps/content_libraries/signal_handlers.py index 08b30b10bba6..fe5489493641 100644 --- a/openedx/core/djangoapps/content_libraries/signal_handlers.py +++ b/openedx/core/djangoapps/content_libraries/signal_handlers.py @@ -7,25 +7,21 @@ from django.conf import settings from django.db.models.signals import m2m_changed, post_delete, post_save from django.dispatch import receiver - -from opaque_keys import InvalidKeyError +from opaque_keys import InvalidKeyError, OpaqueKey from opaque_keys.edx.locator import LibraryLocatorV2, LibraryUsageLocatorV2 -from openedx_events.content_authoring.data import ( - ContentObjectChangedData, - LibraryCollectionData, -) +from openedx_events.content_authoring.data import ContentObjectChangedData, LibraryCollectionData from openedx_events.content_authoring.signals import ( CONTENT_OBJECT_ASSOCIATIONS_CHANGED, LIBRARY_COLLECTION_CREATED, LIBRARY_COLLECTION_DELETED, - LIBRARY_COLLECTION_UPDATED, + LIBRARY_COLLECTION_UPDATED ) -from openedx_learning.api.authoring import get_component, get_components -from openedx_learning.api.authoring_models import Collection, CollectionPublishableEntity, Component, PublishableEntity +from openedx_learning.api.authoring import get_components, get_containers +from openedx_learning.api.authoring_models import Collection, CollectionPublishableEntity, PublishableEntity from lms.djangoapps.grades.api import signals as grades_signals -from .api import library_collection_locator, library_component_usage_key +from .api import library_collection_locator, library_component_usage_key, library_container_locator from .models import ContentLibrary, LtiGradedResource log = logging.getLogger(__name__) @@ -124,33 +120,45 @@ def library_collection_deleted(sender, instance, **kwargs): ) -def _library_collection_component_changed( - component: Component, +def _library_collection_entity_changed( + publishable_entity: PublishableEntity, library_key: LibraryLocatorV2 | None = None, ) -> None: """ - Sends a CONTENT_OBJECT_ASSOCIATIONS_CHANGED event for the component. + Sends a CONTENT_OBJECT_ASSOCIATIONS_CHANGED event for the entity. """ if not library_key: try: library = ContentLibrary.objects.get( - learning_package_id=component.learning_package_id, + learning_package_id=publishable_entity.learning_package_id, ) except ContentLibrary.DoesNotExist: - log.error("{component} is not associated with a content library.") + log.error("{publishable_entity} is not associated with a content library.") return library_key = library.library_key assert library_key - usage_key = library_component_usage_key( - library_key, - component, - ) + opaque_key: OpaqueKey + + if hasattr(publishable_entity, 'component'): + opaque_key = library_component_usage_key( + library_key, + publishable_entity.component, + ) + 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 + CONTENT_OBJECT_ASSOCIATIONS_CHANGED.send_event( content_object=ContentObjectChangedData( - object_id=str(usage_key), + object_id=str(opaque_key), changes=["collections"], ), ) @@ -162,9 +170,7 @@ def library_collection_entity_saved(sender, instance, created, **kwargs): Sends a CONTENT_OBJECT_ASSOCIATIONS_CHANGED event for components added to a collection. """ if created: - # Component.pk matches its entity.pk - component = get_component(instance.entity_id) - _library_collection_component_changed(component) + _library_collection_entity_changed(instance.entity) @receiver(post_delete, sender=CollectionPublishableEntity, dispatch_uid="library_collection_entity_deleted") @@ -174,9 +180,7 @@ def library_collection_entity_deleted(sender, instance, **kwargs): """ # Only trigger component updates if CollectionPublishableEntity was cascade deleted due to deletion of a collection. if isinstance(kwargs.get('origin'), Collection): - # Component.pk matches its entity.pk - component = get_component(instance.entity_id) - _library_collection_component_changed(component) + _library_collection_entity_changed(instance.entity) @receiver(m2m_changed, sender=CollectionPublishableEntity, dispatch_uid="library_collection_entities_changed") @@ -196,15 +200,18 @@ def library_collection_entities_changed(sender, instance, action, pk_set, **kwar return if isinstance(instance, PublishableEntity): - _library_collection_component_changed(instance.component, library.library_key) + _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. + # 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 component in components.all(): - _library_collection_component_changed(component, library.library_key) + 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/tests/base.py b/openedx/core/djangoapps/content_libraries/tests/base.py index 3c07aabe06c4..c015886af70e 100644 --- a/openedx/core/djangoapps/content_libraries/tests/base.py +++ b/openedx/core/djangoapps/content_libraries/tests/base.py @@ -35,6 +35,7 @@ URL_LIB_CONTAINER = URL_PREFIX + 'containers/{container_key}/' # Get a container in this library URL_LIB_CONTAINER_COMPONENTS = URL_LIB_CONTAINER + 'children/' # Get, add or delete a component in this container URL_LIB_CONTAINER_RESTORE = URL_LIB_CONTAINER + 'restore/' # Restore a deleted container +URL_LIB_CONTAINER_COLLECTIONS = URL_LIB_CONTAINER + 'collections/' # Handle associated collections URL_LIB_LTI_PREFIX = URL_PREFIX + 'lti/1.3/' URL_LIB_LTI_JWKS = URL_LIB_LTI_PREFIX + 'pub/jwks/' @@ -441,3 +442,17 @@ def _patch_container_components( {'usage_keys': children_ids}, expect_response ) + + def _patch_container_collections( + self, + container_key: str, + collection_keys: list[str], + expect_response=200, + ): + """ Update container collections""" + return self._api( + 'patch', + URL_LIB_CONTAINER_COLLECTIONS.format(container_key=container_key), + {'collection_keys': collection_keys}, + expect_response + ) diff --git a/openedx/core/djangoapps/content_libraries/tests/test_api.py b/openedx/core/djangoapps/content_libraries/tests/test_api.py index b0934768b794..7a20f75f8574 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_api.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_api.py @@ -12,7 +12,7 @@ CourseKey, UsageKey, ) -from opaque_keys.edx.locator import LibraryLocatorV2 +from opaque_keys.edx.locator import LibraryContainerLocator, LibraryLocatorV2 from openedx_events.content_authoring.data import ( ContentObjectChangedData, LibraryCollectionData, @@ -325,6 +325,12 @@ def setUp(self): self.lib1_html_block = self._add_block_to_library( self.lib1.library_key, "html", "html1", ) + # Create a container in lib1 + self.unit1 = self._create_container( + str(self.lib1.library_key), + "unit", 'unit-1', 'Unit 1' + ) + # Create some library blocks in lib2 self.lib2_problem_block = self._add_block_to_library( self.lib2.library_key, "problem", "problem2", @@ -431,28 +437,29 @@ def test_delete_library_collection(self): event_receiver.call_args_list[0].kwargs, ) - def test_update_library_collection_components(self): + def test_update_library_collection_items(self): assert not list(self.col1.entities.all()) - self.col1 = api.update_library_collection_components( + self.col1 = api.update_library_collection_items( self.lib1.library_key, self.col1.key, - usage_keys=[ + opaque_keys=[ UsageKey.from_string(self.lib1_problem_block["id"]), UsageKey.from_string(self.lib1_html_block["id"]), + LibraryContainerLocator.from_string(self.unit1["container_key"]), ], ) - assert len(self.col1.entities.all()) == 2 + assert len(self.col1.entities.all()) == 3 - self.col1 = api.update_library_collection_components( + self.col1 = api.update_library_collection_items( self.lib1.library_key, self.col1.key, - usage_keys=[ + opaque_keys=[ UsageKey.from_string(self.lib1_html_block["id"]), ], remove=True, ) - assert len(self.col1.entities.all()) == 1 + assert len(self.col1.entities.all()) == 2 def test_update_library_collection_components_event(self): """ @@ -462,16 +469,17 @@ def test_update_library_collection_components_event(self): CONTENT_OBJECT_ASSOCIATIONS_CHANGED.connect(event_receiver) LIBRARY_COLLECTION_UPDATED.connect(event_receiver) - api.update_library_collection_components( + api.update_library_collection_items( self.lib1.library_key, self.col1.key, - usage_keys=[ + opaque_keys=[ UsageKey.from_string(self.lib1_problem_block["id"]), UsageKey.from_string(self.lib1_html_block["id"]), + LibraryContainerLocator.from_string(self.unit1["container_key"]), ], ) - assert event_receiver.call_count == 3 + assert event_receiver.call_count == 4 self.assertDictContainsSubset( { "signal": CONTENT_OBJECT_ASSOCIATIONS_CHANGED, @@ -494,6 +502,17 @@ def test_update_library_collection_components_event(self): }, event_receiver.call_args_list[1].kwargs, ) + self.assertDictContainsSubset( + { + "signal": CONTENT_OBJECT_ASSOCIATIONS_CHANGED, + "sender": None, + "content_object": ContentObjectChangedData( + object_id=self.unit1["container_key"], + changes=["collections"], + ), + }, + event_receiver.call_args_list[2].kwargs, + ) self.assertDictContainsSubset( { "signal": LIBRARY_COLLECTION_UPDATED, @@ -505,17 +524,18 @@ def test_update_library_collection_components_event(self): ), ), }, - event_receiver.call_args_list[2].kwargs, + event_receiver.call_args_list[3].kwargs, ) def test_update_collection_components_from_wrong_library(self): with self.assertRaises(api.ContentLibraryBlockNotFound) as exc: - api.update_library_collection_components( + api.update_library_collection_items( self.lib2.library_key, self.col2.key, - usage_keys=[ + opaque_keys=[ UsageKey.from_string(self.lib1_problem_block["id"]), UsageKey.from_string(self.lib1_html_block["id"]), + LibraryContainerLocator.from_string(self.unit1["container_key"]), ], ) assert self.lib1_problem_block["id"] in str(exc.exception) @@ -528,9 +548,9 @@ def test_set_library_component_collections(self): assert not list(self.col2.entities.all()) component = api.get_component_from_usage_key(UsageKey.from_string(self.lib2_problem_block["id"])) - api.set_library_component_collections( + api.set_library_item_collections( self.lib2.library_key, - component, + component.publishable_entity, collection_keys=[self.col2.key, self.col3.key], ) @@ -577,10 +597,10 @@ def test_set_library_component_collections(self): ) def test_delete_library_block(self): - api.update_library_collection_components( + api.update_library_collection_items( self.lib1.library_key, self.col1.key, - usage_keys=[ + opaque_keys=[ UsageKey.from_string(self.lib1_problem_block["id"]), UsageKey.from_string(self.lib1_html_block["id"]), ], @@ -607,11 +627,43 @@ def test_delete_library_block(self): event_receiver.call_args_list[0].kwargs, ) + def test_delete_library_container(self): + api.update_library_collection_items( + self.lib1.library_key, + self.col1.key, + opaque_keys=[ + UsageKey.from_string(self.lib1_problem_block["id"]), + UsageKey.from_string(self.lib1_html_block["id"]), + LibraryContainerLocator.from_string(self.unit1["container_key"]), + ], + ) + + event_receiver = mock.Mock() + LIBRARY_COLLECTION_UPDATED.connect(event_receiver) + + api.delete_container(LibraryContainerLocator.from_string(self.unit1["container_key"])) + + assert event_receiver.call_count == 1 + self.assertDictContainsSubset( + { + "signal": LIBRARY_COLLECTION_UPDATED, + "sender": None, + "library_collection": LibraryCollectionData( + collection_key=api.library_collection_locator( + self.lib1.library_key, + collection_key=self.col1.key, + ), + background=True, + ), + }, + event_receiver.call_args_list[0].kwargs, + ) + def test_restore_library_block(self): - api.update_library_collection_components( + api.update_library_collection_items( self.lib1.library_key, self.col1.key, - usage_keys=[ + opaque_keys=[ UsageKey.from_string(self.lib1_problem_block["id"]), UsageKey.from_string(self.lib1_html_block["id"]), ], @@ -640,20 +692,20 @@ def test_restore_library_block(self): def test_add_component_and_revert(self): # Add component and publish - api.update_library_collection_components( + api.update_library_collection_items( self.lib1.library_key, self.col1.key, - usage_keys=[ + opaque_keys=[ UsageKey.from_string(self.lib1_problem_block["id"]), ], ) api.publish_changes(self.lib1.library_key) # Add component and revert - api.update_library_collection_components( + api.update_library_collection_items( self.lib1.library_key, self.col1.key, - usage_keys=[ + opaque_keys=[ UsageKey.from_string(self.lib1_html_block["id"]), ], ) @@ -706,10 +758,10 @@ def test_add_component_and_revert(self): def test_delete_component_and_revert(self): # Add components and publish - api.update_library_collection_components( + api.update_library_collection_items( self.lib1.library_key, self.col1.key, - usage_keys=[ + opaque_keys=[ UsageKey.from_string(self.lib1_problem_block["id"]), UsageKey.from_string(self.lib1_html_block["id"]) ], diff --git a/openedx/core/djangoapps/content_libraries/tests/test_containers.py b/openedx/core/djangoapps/content_libraries/tests/test_containers.py index 85e04260cf52..41752eb41e17 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_containers.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_containers.py @@ -17,6 +17,7 @@ from openedx_events.tests.utils import OpenEdxEventsTestMixin from common.djangoapps.student.tests.factories import UserFactory +from openedx.core.djangoapps.content_libraries import api from openedx.core.djangoapps.content_libraries.tests.base import ContentLibrariesRestApiTest from openedx.core.djangolib.testing.utils import skip_unless_cms @@ -382,3 +383,33 @@ def test_restore_unit(self): }, create_receiver.call_args_list[0].kwargs, ) + + def test_container_collections(self): + # Create a library + lib = self._create_library(slug="containers", title="Container Test Library", description="Units and more") + lib_key = LibraryLocatorV2.from_string(lib["id"]) + + # Create a unit + container_data = self._create_container(lib["id"], "unit", display_name="Alpha Bravo", slug=None) + + # Create a collection + col1 = api.create_library_collection( + lib_key, + "COL1", + title="Collection 1", + created_by=self.user.id, + description="Description for Collection 1", + ) + + result = self._patch_container_collections( + container_data["container_key"], + collection_keys=[col1.key], + ) + + assert result['count'] == 1 + + # Fetch the unit + unit_as_read = self._get_container(container_data["container_key"]) + + # Verify the collections + assert unit_as_read['collections'] == [{"title": col1.title, "key": col1.key}] 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 43c1627c2c76..283f5fbe97e5 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_views_collections.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_views_collections.py @@ -18,7 +18,7 @@ URL_LIB_COLLECTIONS = URL_PREFIX + 'collections/' URL_LIB_COLLECTION = URL_LIB_COLLECTIONS + '{collection_key}/' URL_LIB_COLLECTION_RESTORE = URL_LIB_COLLECTIONS + '{collection_key}/restore/' -URL_LIB_COLLECTION_COMPONENTS = URL_LIB_COLLECTION + 'components/' +URL_LIB_COLLECTION_COMPONENTS = URL_LIB_COLLECTION + 'items/' @ddt.ddt diff --git a/openedx/core/djangoapps/content_libraries/urls.py b/openedx/core/djangoapps/content_libraries/urls.py index c46cc8641cd8..adbc89b2cc9b 100644 --- a/openedx/core/djangoapps/content_libraries/urls.py +++ b/openedx/core/djangoapps/content_libraries/urls.py @@ -3,12 +3,10 @@ """ from django.urls import include, path, re_path, register_converter - from rest_framework import routers from .rest_api import blocks, collections, containers, libraries, url_converters - # Django application name. app_name = 'openedx.core.djangoapps.content_libraries' @@ -85,7 +83,7 @@ # Restore a soft-deleted container path('restore/', containers.LibraryContainerRestore.as_view()), # Update collections for a given container - # path('collections/', views.LibraryContainerCollectionsView.as_view(), name='update-collections-ct'), + path('collections/', containers.LibraryContainerCollectionsView.as_view(), name='update-collections-ct'), # path('publish/', views.LibraryContainerPublishView.as_view()), ])), re_path(r'^lti/1.3/', include([ diff --git a/requirements/constraints.txt b/requirements/constraints.txt index 6633650f8440..bcba26a09bd8 100644 --- a/requirements/constraints.txt +++ b/requirements/constraints.txt @@ -112,7 +112,7 @@ numpy<2.0.0 # Date: 2023-09-18 # pinning this version to avoid updates while the library is being developed # Issue for unpinning: https://github.com/openedx/edx-platform/issues/35269 -openedx-learning==0.19.2 +openedx-learning==0.22.0 # 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 f1ef8845df80..7d85b8ed360b 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -820,7 +820,7 @@ openedx-filters==2.0.1 # ora2 openedx-forum==0.2.0 # via -r requirements/edx/kernel.in -openedx-learning==0.19.2 +openedx-learning==0.22.0 # via # -c requirements/edx/../constraints.txt # -r requirements/edx/kernel.in diff --git a/requirements/edx/development.txt b/requirements/edx/development.txt index 5bfbca431f47..3c8230e19f0f 100644 --- a/requirements/edx/development.txt +++ b/requirements/edx/development.txt @@ -1383,7 +1383,7 @@ openedx-forum==0.2.0 # via # -r requirements/edx/doc.txt # -r requirements/edx/testing.txt -openedx-learning==0.19.2 +openedx-learning==0.22.0 # via # -c requirements/edx/../constraints.txt # -r requirements/edx/doc.txt diff --git a/requirements/edx/doc.txt b/requirements/edx/doc.txt index 7b4fb5682a82..ef33c0f7c08e 100644 --- a/requirements/edx/doc.txt +++ b/requirements/edx/doc.txt @@ -992,7 +992,7 @@ openedx-filters==2.0.1 # ora2 openedx-forum==0.2.0 # via -r requirements/edx/base.txt -openedx-learning==0.19.2 +openedx-learning==0.22.0 # via # -c requirements/edx/../constraints.txt # -r requirements/edx/base.txt diff --git a/requirements/edx/testing.txt b/requirements/edx/testing.txt index 404104e9750d..7452cb3d53c0 100644 --- a/requirements/edx/testing.txt +++ b/requirements/edx/testing.txt @@ -1050,7 +1050,7 @@ openedx-filters==2.0.1 # ora2 openedx-forum==0.2.0 # via -r requirements/edx/base.txt -openedx-learning==0.19.2 +openedx-learning==0.22.0 # via # -c requirements/edx/../constraints.txt # -r requirements/edx/base.txt