Basic CRUD REST Endpoints for units in content libraries [FC-0083] (#36371)

* refactor: convert libraries API from attr.s to dataclass, fix types

* fix: make corresponding updates to 'search' code

* feat: use new version of openedx-learning with containers support

* temp: Use opencraft branch of opaquekeys

* refactor: Use LibraryElementKey instead of LibraryCollectionKey

* refactor: split libraries API & REST API up into smaller modules

* feat: new REST API for units in content libraries

* feat: python+REST API to get a unit

* feat: auto-generate slug/key/ID from title of units

* feat: generate search index documents for containers

* refactor: rename LibraryElementKey to LibraryItemKey

* fix: lint error

* feat: adds new units to search index on create/update

and when running reindex_studio.

Updates requirements for openedx-events and openedx-learning to support
these changes.

* fix: pylint

* fix: temp requirement

* fix: search index container events/tasks

* feat: add get_library_container_usage_key to libraries API

and use it when search indexing containers

* fix: index all containers during reindex_studio

* chore: bump openedx-events requirement

* fix: address review comments

* chore: bumps openedx-learning to 0.19.1

* fix: rename api method to library_container_locator

since container keys are locators, not usage keys

* chore: bumps opaque-keys dependency

* test: fix misnamed unit_usage_key

* feat: adds APIs to update or delete a container (#757)

* feat: adds python and REST APIs to update a container's display_name
* refactor: adds _get_container method to api to reduce code duplication
* feat: adds python and REST APIs to delete a container
* test: add container permission tests

---------

Co-authored-by: XnpioChV <xnpiochv@gmail.com>
Co-authored-by: Jillian Vogel <jill@opencraft.com>
Co-authored-by: Rômulo Penido <romulo.penido@gmail.com>

Author: 4 people

cms/envs/common.py

@@ -1879,6 +1879,7 @@
     "openedx_learning.apps.authoring.components",
     "openedx_learning.apps.authoring.contents",
     "openedx_learning.apps.authoring.publishing",
+    "openedx_learning.apps.authoring.units",
 ]
 
 

lms/envs/common.py

@@ -3369,6 +3369,7 @@ def _make_locale_paths(settings):  # pylint: disable=missing-function-docstring
     "openedx_learning.apps.authoring.components",
     "openedx_learning.apps.authoring.contents",
     "openedx_learning.apps.authoring.publishing",
+    "openedx_learning.apps.authoring.units",
 ]
 
 

openedx/core/djangoapps/content/search/api.py

@@ -18,7 +18,7 @@
 from meilisearch.errors import MeilisearchApiError, MeilisearchError
 from meilisearch.models.task import TaskInfo
 from opaque_keys.edx.keys import UsageKey
-from opaque_keys.edx.locator import LibraryLocatorV2, LibraryCollectionLocator
+from opaque_keys.edx.locator import LibraryContainerLocator, LibraryLocatorV2, LibraryCollectionLocator
 from openedx_learning.api import authoring as authoring_api
 from common.djangoapps.student.roles import GlobalStaff
 from rest_framework.request import Request
@@ -40,6 +40,7 @@
     meili_id_from_opaque_key,
     searchable_doc_for_course_block,
     searchable_doc_for_collection,
+    searchable_doc_for_container,
     searchable_doc_for_library_block,
     searchable_doc_for_usage_key,
     searchable_doc_collections,
@@ -475,6 +476,31 @@ def index_collection_batch(batch, num_done, library_key) -> int:
                     status_cb(f"Error indexing collection batch {p}: {err}")
             return num_done
 
+        ############## Containers ##############
+        def index_container_batch(batch, num_done, library_key) -> int:
+            docs = []
+            for container in batch:
+                try:
+                    container_key = lib_api.library_container_locator(
+                        library_key,
+                        container,
+                    )
+                    doc = searchable_doc_for_container(container_key)
+                    # TODO: when we add container tags
+                    # doc.update(searchable_doc_tags_for_container(container_key))
+                    docs.append(doc)
+                except Exception as err:  # pylint: disable=broad-except
+                    status_cb(f"Error indexing container {container.key}: {err}")
+                num_done += 1
+
+            if docs:
+                try:
+                    # Add docs in batch of 100 at once (usually faster than adding one at a time):
+                    _wait_for_meili_task(client.index(index_name).add_documents(docs))
+                except (TypeError, KeyError, MeilisearchError) as err:
+                    status_cb(f"Error indexing container batch {p}: {err}")
+            return num_done
+
         for lib_key in lib_keys:
             status_cb(f"{num_contexts_done + 1}/{num_contexts}. Now indexing blocks in library {lib_key}")
             lib_docs = index_library(lib_key)
@@ -497,6 +523,22 @@ def index_collection_batch(batch, num_done, library_key) -> int:
                 IncrementalIndexCompleted.objects.get_or_create(context_key=lib_key)
             status_cb(f"{num_collections_done}/{num_collections} collections indexed for library {lib_key}")
 
+            # Similarly, batch process Containers (units, sections, etc) in pages of 100
+            containers = authoring_api.get_containers(library.learning_package_id)
+            num_containers = containers.count()
+            num_containers_done = 0
+            status_cb(f"{num_containers_done}/{num_containers}. Now indexing containers in library {lib_key}")
+            paginator = Paginator(containers, 100)
+            for p in paginator.page_range:
+                num_containers_done = index_container_batch(
+                    paginator.page(p).object_list,
+                    num_containers_done,
+                    lib_key,
+                )
+                status_cb(f"{num_containers_done}/{num_containers} containers indexed for library {lib_key}")
+            if incremental:
+                IncrementalIndexCompleted.objects.get_or_create(context_key=lib_key)
+
             num_contexts_done += 1
 
         ############## Courses ##############
@@ -732,6 +774,30 @@ def update_library_components_collections(
         _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.
+
+    TODO: add support for indexing a container's components, like upsert_library_collection_index_doc does.
+    """
+    doc = searchable_doc_for_container(container_key)
+
+    # Soft-deleted/disabled containers are removed from the index
+    # and their components updated.
+    if doc.get('_disabled'):
+
+        _delete_index_doc(doc[Fields.id])
+
+    # Hard-deleted containers are also deleted from the index
+    elif not doc.get(Fields.type):
+
+        _delete_index_doc(doc[Fields.id])
+
+    # Otherwise, upsert the container.
+    else:
+        _update_index_docs([doc])
+
+
 def upsert_content_library_index_docs(library_key: LibraryLocatorV2) -> None:
     """
     Creates or updates the documents for the given Content Library in the search index

openedx/core/djangoapps/content/search/documents.py

@@ -10,7 +10,7 @@
 from django.core.exceptions import ObjectDoesNotExist
 from opaque_keys.edx.keys import LearningContextKey, UsageKey
 from openedx_learning.api import authoring as authoring_api
-from opaque_keys.edx.locator import LibraryLocatorV2
+from opaque_keys.edx.locator import LibraryLocatorV2, LibraryContainerLocator
 from rest_framework.exceptions import NotFound
 
 from openedx.core.djangoapps.content.search.models import SearchAccess
@@ -100,6 +100,7 @@ class DocType:
     """
     course_block = "course_block"
     library_block = "library_block"
+    library_container = "library_container"
     collection = "collection"
 
 
@@ -546,3 +547,58 @@ def searchable_doc_for_collection(
             doc['_disabled'] = True
 
     return doc
+
+
+def searchable_doc_for_container(
+    container_key: LibraryContainerLocator,
+) -> dict:
+    """
+    Generate a dictionary document suitable for ingestion into a search engine
+    like Meilisearch or Elasticsearch, so that the given collection can be
+    found using faceted search.
+
+    If no container is found for the given container key, the returned document
+    will contain only basic information derived from the container key, and no
+    Fields.type value will be included in the returned dict.
+    """
+    doc = {
+        Fields.id: meili_id_from_opaque_key(container_key),
+        Fields.context_key: str(container_key.library_key),
+        Fields.org: str(container_key.org),
+        # In the future, this may be either course_container or library_container
+        Fields.type: DocType.library_container,
+        # To check if it is "unit", "section", "subsection", etc..
+        Fields.block_type: container_key.container_type,
+        Fields.usage_key: str(container_key),  # Field name isn't exact but this is the closest match
+        Fields.block_id: container_key.container_id,  # Field name isn't exact but this is the closest match
+        Fields.access_id: _meili_access_id_from_context_key(container_key.library_key),
+    }
+
+    try:
+        container = lib_api.get_container(container_key)
+    except lib_api.ContentLibraryCollectionNotFound:
+        # Container not found, so we can only return the base doc
+        pass
+
+    if container:
+        # TODO: check if there's a more efficient way to load these num_children counts?
+        draft_num_children = len(lib_api.get_container_children(container_key, published=False))
+
+        doc.update({
+            Fields.display_name: container.display_name,
+            Fields.created: container.created.timestamp(),
+            Fields.modified: container.modified.timestamp(),
+            Fields.num_children: draft_num_children,
+        })
+        library = lib_api.get_library(container_key.library_key)
+        if library:
+            doc[Fields.breadcrumbs] = [{"display_name": library.title}]
+
+        if container.published_version_num is not None:
+            published_num_children = len(lib_api.get_container_children(container_key, published=True))
+            doc[Fields.published] = {
+                # Fields.published_display_name: container_published.title, TODO: set the published title
+                Fields.published_num_children: published_num_children,
+            }
+
+    return doc

openedx/core/djangoapps/content/search/handlers.py

@@ -14,6 +14,7 @@
     ContentObjectChangedData,
     LibraryBlockData,
     LibraryCollectionData,
+    LibraryContainerData,
     XBlockData,
 )
 from openedx_events.content_authoring.signals import (
@@ -25,6 +26,9 @@
     LIBRARY_COLLECTION_CREATED,
     LIBRARY_COLLECTION_DELETED,
     LIBRARY_COLLECTION_UPDATED,
+    LIBRARY_CONTAINER_CREATED,
+    LIBRARY_CONTAINER_DELETED,
+    LIBRARY_CONTAINER_UPDATED,
     XBLOCK_CREATED,
     XBLOCK_DELETED,
     XBLOCK_UPDATED,
@@ -45,6 +49,7 @@
     delete_xblock_index_doc,
     update_content_library_index_docs,
     update_library_collection_index_doc,
+    update_library_container_index_doc,
     upsert_library_block_index_doc,
     upsert_xblock_index_doc,
 )
@@ -225,3 +230,31 @@ def content_object_associations_changed_handler(**kwargs) -> None:
             upsert_block_tags_index_docs(usage_key)
     if not content_object.changes or "collections" in content_object.changes:
         upsert_block_collections_index_docs(usage_key)
+
+
+@receiver(LIBRARY_CONTAINER_CREATED)
+@receiver(LIBRARY_CONTAINER_DELETED)
+@receiver(LIBRARY_CONTAINER_UPDATED)
+@only_if_meilisearch_enabled
+def library_container_updated_handler(**kwargs) -> None:
+    """
+    Create or update the index for the content library container
+    """
+    library_container = kwargs.get("library_container", None)
+    if not library_container or not isinstance(library_container, LibraryContainerData):  # pragma: no cover
+        log.error("Received null or incorrect data for event")
+        return
+
+    if library_container.background:
+        update_library_container_index_doc.delay(
+            str(library_container.library_key),
+            library_container.container_key,
+        )
+    else:
+        # Update container index synchronously to make sure that search index is updated before
+        # the frontend invalidates/refetches index.
+        # See content_library_updated_handler for more details.
+        update_library_container_index_doc.apply(args=[
+            str(library_container.library_key),
+            library_container.container_key,
+        ])

openedx/core/djangoapps/content/search/tasks.py

@@ -11,7 +11,7 @@
 from edx_django_utils.monitoring import set_code_owner_attribute
 from meilisearch.errors import MeilisearchError
 from opaque_keys.edx.keys import UsageKey
-from opaque_keys.edx.locator import LibraryLocatorV2, LibraryUsageLocatorV2
+from opaque_keys.edx.locator import LibraryContainerLocator, LibraryLocatorV2, LibraryUsageLocatorV2
 
 from . import api
 
@@ -110,3 +110,17 @@ def update_library_components_collections(library_key_str: str, collection_key:
     log.info("Updating document.collections for library %s collection %s components", library_key, collection_key)
 
     api.update_library_components_collections(library_key, collection_key)
+
+
+@shared_task(base=LoggedTask, autoretry_for=(MeilisearchError, ConnectionError))
+@set_code_owner_attribute
+def update_library_container_index_doc(library_key_str: str, container_key_str: str) -> None:
+    """
+    Celery task to update the content index document for a library container
+    """
+    library_key = LibraryLocatorV2.from_string(library_key_str)
+    container_key = LibraryContainerLocator.from_string(container_key_str)
+
+    log.info("Updating content index documents for container %s in library%s", container_key, library_key)
+
+    api.upsert_library_container_index_doc(container_key)

openedx/core/djangoapps/content/search/tests/test_api.py

@@ -208,6 +208,35 @@ def setUp(self):
             "breadcrumbs": [{"display_name": "Library"}],
         }
 
+        # Create a unit:
+        with freeze_time(created_date):
+            self.unit = library_api.create_container(
+                library_key=self.library.key,
+                container_type=library_api.ContainerType.Unit,
+                slug="unit-1",
+                title="Unit 1",
+                user_id=None,
+            )
+            self.unit_key = "lct:org1:lib:unit:unit-1"
+        self.unit_dict = {
+            "id": "lctorg1libunitunit-1-e4527f7c",
+            "block_id": "unit-1",
+            "block_type": "unit",
+            "usage_key": self.unit_key,
+            "type": "library_container",
+            "display_name": "Unit 1",
+            # description is not set for containers
+            "num_children": 0,
+            "context_key": "lib:org1:lib",
+            "org": "org1",
+            "created": created_date.timestamp(),
+            "modified": created_date.timestamp(),
+            "access_id": lib_access.id,
+            "breadcrumbs": [{"display_name": "Library"}],
+            # "tags" should be here but we haven't implemented them yet
+            # "published" is not set since we haven't published it yet
+        }
+
     @override_settings(MEILISEARCH_ENABLED=False)
     def test_reindex_meilisearch_disabled(self, mock_meilisearch):
         with self.assertRaises(RuntimeError):
@@ -231,14 +260,16 @@ def test_reindex_meilisearch(self, mock_meilisearch):
         doc_problem2["collections"] = {'display_name': [], 'key': []}
         doc_collection = copy.deepcopy(self.collection_dict)
         doc_collection["tags"] = {}
+        doc_unit = copy.deepcopy(self.unit_dict)
 
         api.rebuild_index()
-        assert mock_meilisearch.return_value.index.return_value.add_documents.call_count == 3
+        assert mock_meilisearch.return_value.index.return_value.add_documents.call_count == 4
         mock_meilisearch.return_value.index.return_value.add_documents.assert_has_calls(
             [
                 call([doc_sequential, doc_vertical]),
                 call([doc_problem1, doc_problem2]),
                 call([doc_collection]),
+                call([doc_unit]),
             ],
             any_order=True,
         )
@@ -259,14 +290,16 @@ def test_reindex_meilisearch_incremental(self, mock_meilisearch):
         doc_problem2["collections"] = {"display_name": [], "key": []}
         doc_collection = copy.deepcopy(self.collection_dict)
         doc_collection["tags"] = {}
+        doc_unit = copy.deepcopy(self.unit_dict)
 
         api.rebuild_index(incremental=True)
-        assert mock_meilisearch.return_value.index.return_value.add_documents.call_count == 3
+        assert mock_meilisearch.return_value.index.return_value.add_documents.call_count == 4
         mock_meilisearch.return_value.index.return_value.add_documents.assert_has_calls(
             [
                 call([doc_sequential, doc_vertical]),
                 call([doc_problem1, doc_problem2]),
                 call([doc_collection]),
+                call([doc_unit]),
             ],
             any_order=True,
         )
@@ -280,13 +313,13 @@ def simulated_interruption(message):
         with pytest.raises(Exception, match="Simulated interruption"):
             api.rebuild_index(simulated_interruption, incremental=True)
 
-        # two more calls due to collections
-        assert mock_meilisearch.return_value.index.return_value.add_documents.call_count == 5
+        # three more calls due to collections and containers
+        assert mock_meilisearch.return_value.index.return_value.add_documents.call_count == 7
         assert IncrementalIndexCompleted.objects.all().count() == 1
         api.rebuild_index(incremental=True)
         assert IncrementalIndexCompleted.objects.all().count() == 0
         # one missing course indexed
-        assert mock_meilisearch.return_value.index.return_value.add_documents.call_count == 6
+        assert mock_meilisearch.return_value.index.return_value.add_documents.call_count == 8
 
     @override_settings(MEILISEARCH_ENABLED=True)
     def test_reset_meilisearch_index(self, mock_meilisearch):
@@ -340,6 +373,22 @@ def test_reindex_meilisearch_collection_error(self, mock_meilisearch):
             f"Error indexing collection {self.collection}: Failed to generate document"
         )
 
+    @override_settings(MEILISEARCH_ENABLED=True)
+    @patch(
+        "openedx.core.djangoapps.content.search.api.searchable_doc_for_container",
+        Mock(side_effect=Exception("Failed to generate document")),
+    )
+    def test_reindex_meilisearch_container_error(self, mock_meilisearch):
+
+        mock_logger = Mock()
+        api.rebuild_index(mock_logger)
+        assert call(
+            [self.unit_dict]
+        ) not in mock_meilisearch.return_value.index.return_value.add_documents.mock_calls
+        mock_logger.assert_any_call(
+            "Error indexing container unit-1: Failed to generate document"
+        )
+
     @override_settings(MEILISEARCH_ENABLED=True)
     def test_reindex_meilisearch_library_block_error(self, mock_meilisearch):