fix: Bugs with "Publish All Changes" in Library [FC-0083] (#36640)

* fix: "[created] received a naive datetime"
* fix: leaky "isolation" of events was causing test failures
* fix: make lib events more specific, emit them async, handle hierarchy correctly
* chore: bump openedx-events to 10.2.0 for new library PUBLISHED events

Author: bradenmacdonald

cms/djangoapps/contentstore/views/tests/test_block.py

@@ -804,6 +804,12 @@ def setUpClass(cls):
         super().setUpClass()
         cls.start_events_isolation()
 
+    @classmethod
+    def tearDownClass(cls):
+        """ Don't let our event isolation affect other test cases """
+        super().tearDownClass()
+        cls.enable_all_events()  # Re-enable events other than the ENABLED_OPENEDX_EVENTS subset we isolated.
+
     def setUp(self):
         """Creates the test course structure and a few components to 'duplicate'."""
         super().setUp()

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

@@ -653,29 +653,6 @@ def _delete_index_doc(doc_id) -> None:
     _wait_for_meili_tasks(tasks)
 
 
-def delete_all_draft_docs_for_library(library_key: LibraryLocatorV2) -> None:
-    """
-    Deletes draft documents for the given XBlocks from the search index
-    """
-    current_rebuild_index_name = _get_running_rebuild_index_name()
-    client = _get_meilisearch_client()
-    # Delete all documents where last_published is null i.e. never published before.
-    delete_filter = [
-        f'{Fields.context_key}="{library_key}"',
-        # This field should only be NULL or have a value, but we're also checking IS EMPTY just in case.
-        # Inner arrays are connected by an OR
-        [f'{Fields.last_published} IS EMPTY', f'{Fields.last_published} IS NULL'],
-    ]
-
-    tasks = []
-    if current_rebuild_index_name:
-        # If there is a rebuild in progress, the documents will also be deleted from the new index.
-        tasks.append(client.index(current_rebuild_index_name).delete_documents(filter=delete_filter))
-    tasks.append(client.index(STUDIO_INDEX_NAME).delete_documents(filter=delete_filter))
-
-    _wait_for_meili_tasks(tasks)
-
-
 def upsert_library_block_index_doc(usage_key: UsageKey) -> None:
     """
     Creates or updates the document for the given Library Block in the search index

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

@@ -23,12 +23,14 @@
     LIBRARY_BLOCK_CREATED,
     LIBRARY_BLOCK_DELETED,
     LIBRARY_BLOCK_UPDATED,
+    LIBRARY_BLOCK_PUBLISHED,
     LIBRARY_COLLECTION_CREATED,
     LIBRARY_COLLECTION_DELETED,
     LIBRARY_COLLECTION_UPDATED,
     LIBRARY_CONTAINER_CREATED,
     LIBRARY_CONTAINER_DELETED,
     LIBRARY_CONTAINER_UPDATED,
+    LIBRARY_CONTAINER_PUBLISHED,
     XBLOCK_CREATED,
     XBLOCK_DELETED,
     XBLOCK_UPDATED,
@@ -37,6 +39,7 @@
 
 from openedx.core.djangoapps.content.course_overviews.models import CourseOverview
 from openedx.core.djangoapps.content.search.models import SearchAccess
+from openedx.core.djangoapps.content_libraries import api as lib_api
 
 from .api import (
     only_if_meilisearch_enabled,
@@ -136,6 +139,32 @@ def library_block_updated_handler(**kwargs) -> None:
     upsert_library_block_index_doc.apply(args=[str(library_block_data.usage_key)])
 
 
+@receiver(LIBRARY_BLOCK_PUBLISHED)
+@only_if_meilisearch_enabled
+def library_block_published_handler(**kwargs) -> None:
+    """
+    Update the index for the content library block when its published version
+    has changed.
+    """
+    library_block_data = kwargs.get("library_block", None)
+    if not library_block_data or not isinstance(library_block_data, LibraryBlockData):  # pragma: no cover
+        log.error("Received null or incorrect data for event")
+        return
+
+    # The PUBLISHED event is sent for any change to the published version including deletes, so check if it exists:
+    try:
+        lib_api.get_library_block(library_block_data.usage_key)
+    except lib_api.ContentLibraryBlockNotFound:
+        log.info(f"Observed published deletion of library block {str(library_block_data.usage_key)}.")
+        # The document should already have been deleted from the search index
+        # via the DELETED handler, so there's nothing to do now.
+        return
+
+    # Update content library index synchronously to make sure that search index is updated before
+    # the frontend invalidates/refetches results. This is only a single document update so is very fast.
+    upsert_library_block_index_doc.apply(args=[str(library_block_data.usage_key)])
+
+
 @receiver(LIBRARY_BLOCK_DELETED)
 @only_if_meilisearch_enabled
 def library_block_deleted(**kwargs) -> None:
@@ -162,14 +191,14 @@ def content_library_updated_handler(**kwargs) -> None:
     if not content_library_data or not isinstance(content_library_data, ContentLibraryData):  # pragma: no cover
         log.error("Received null or incorrect data for event")
         return
+    library_key = content_library_data.library_key
 
-    # Update content library index synchronously to make sure that search index is updated before
-    # the frontend invalidates/refetches index.
-    # Currently, this is only required to make sure that removed/discarded components are removed
-    # from the search index and displayed to user properly. If it becomes a performance bottleneck
-    # for other update operations other than discard, we can update CONTENT_LIBRARY_UPDATED event
-    # to include a parameter which can help us decide if the task needs to run sync or async.
-    update_content_library_index_docs.apply(args=[str(content_library_data.library_key)])
+    # For now we assume the library has been renamed. Few other things will trigger this event.
+
+    # Update ALL items in the library, because their breadcrumbs will be outdated.
+    # TODO: just patch the "breadcrumbs" field? It's the same on every one.
+    # TODO: check if the library display_name has actually changed before updating all items?
+    update_content_library_index_docs.apply(args=[str(library_key)])
 
 
 @receiver(LIBRARY_COLLECTION_CREATED)
@@ -248,17 +277,34 @@ def library_container_updated_handler(**kwargs) -> None:
         log.error("Received null or incorrect data for event")
         return
 
-    if library_container.background:
-        update_library_container_index_doc.delay(
-            str(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.container_key),
-        ])
+    update_library_container_index_doc.apply(args=[
+        str(library_container.container_key),
+    ])
+
+
+@receiver(LIBRARY_CONTAINER_PUBLISHED)
+@only_if_meilisearch_enabled
+def library_container_published_handler(**kwargs) -> None:
+    """
+    Update the index for the content library container when its published
+    version has changed.
+    """
+    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
+    # The PUBLISHED event is sent for any change to the published version including deletes, so check if it exists:
+    try:
+        lib_api.get_container(library_container.container_key)
+    except lib_api.ContentLibraryContainerNotFound:
+        log.info(f"Observed published deletion of container {str(library_container.container_key)}.")
+        # The document should already have been deleted from the search index
+        # via the DELETED handler, so there's nothing to do now.
+        return
+
+    update_library_container_index_doc.apply(args=[
+        str(library_container.container_key),
+    ])
 
 
 @receiver(LIBRARY_CONTAINER_DELETED)
@@ -275,3 +321,6 @@ def library_container_deleted(**kwargs) -> None:
     # Update content library index synchronously to make sure that search index is updated before
     # the frontend invalidates/refetches results. This is only a single document update so is very fast.
     delete_library_container_index_doc.apply(args=[str(library_container.container_key)])
+    # TODO: post-Teak, move all the celery tasks directly inline into this handlers? Because now the
+    # events are emitted in an [async] worker, so it doesn't matter if the handlers are synchronous.
+    # See https://github.com/openedx/edx-platform/pull/36640 discussion.

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

@@ -86,9 +86,6 @@ def update_content_library_index_docs(library_key_str: str) -> None:
     log.info("Updating content index documents for library with id: %s", library_key)
 
     api.upsert_content_library_index_docs(library_key)
-    # Delete all documents in this library that were not published by above function
-    # as this task is also triggered on discard event.
-    api.delete_all_draft_docs_for_library(library_key)
 
 
 @shared_task(base=LoggedTask, autoretry_for=(MeilisearchError, ConnectionError))

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

@@ -734,21 +734,6 @@ def test_index_content_library_metadata(self, mock_meilisearch):
             [self.doc_problem1, self.doc_problem2]
         )
 
-    @override_settings(MEILISEARCH_ENABLED=True)
-    def test_delete_all_drafts(self, mock_meilisearch):
-        """
-        Test deleting all draft documents from the index.
-        """
-        api.delete_all_draft_docs_for_library(self.library.key)
-
-        delete_filter = [
-            f'context_key="{self.library.key}"',
-            ['last_published IS EMPTY', 'last_published IS NULL'],
-        ]
-        mock_meilisearch.return_value.index.return_value.delete_documents.assert_called_once_with(
-            filter=delete_filter
-        )
-
     @override_settings(MEILISEARCH_ENABLED=True)
     def test_index_tags_in_collections(self, mock_meilisearch):
         # Tag collection

openedx/core/djangoapps/content_libraries/api/blocks.py

@@ -63,10 +63,9 @@
     ContainerMetadata,
     ContainerType,
 )
-from .libraries import (
-    library_collection_locator,
-    PublishableItem,
-)
+from .collections import library_collection_locator
+from .libraries import PublishableItem
+from .. import tasks
 
 # This content_libraries API is sometimes imported in the LMS (should we prevent that?), but the content_staging app
 # cannot be. For now we only need this one type import at module scope, so only import it during type checks.
@@ -836,24 +835,13 @@ def publish_component_changes(usage_key: LibraryUsageLocatorV2, user: UserType):
     # The core publishing API is based on draft objects, so find the draft that corresponds to this component:
     drafts_to_publish = authoring_api.get_all_drafts(learning_package.id).filter(entity__key=component.key)
     # Publish the component and update anything that needs to be updated (e.g. search index):
-    authoring_api.publish_from_drafts(learning_package.id, draft_qset=drafts_to_publish, published_by=user.id)
-    LIBRARY_BLOCK_UPDATED.send_event(
-        library_block=LibraryBlockData(
-            library_key=usage_key.lib_key,
-            usage_key=usage_key,
-        )
+    publish_log = authoring_api.publish_from_drafts(
+        learning_package.id, draft_qset=drafts_to_publish, published_by=user.id,
     )
-
-    # For each container, trigger LIBRARY_CONTAINER_UPDATED signal and set background=True to trigger
-    # container indexing asynchronously.
-    affected_containers = get_containers_contains_component(usage_key)
-    for container in affected_containers:
-        LIBRARY_CONTAINER_UPDATED.send_event(
-            library_container=LibraryContainerData(
-                container_key=container.container_key,
-                background=True,
-            )
-        )
+    # Since this is a single component, it should be safe to process synchronously and in-process:
+    tasks.send_events_after_publish(publish_log.pk, str(library_key))
+    # IF this is found to be a performance issue, we could instead make it async where necessary:
+    # tasks.wait_for_post_publish_events(publish_log, library_key=library_key)
 
 
 def _component_exists(usage_key: UsageKeyV2) -> bool:

openedx/core/djangoapps/content_libraries/api/containers.py

@@ -4,7 +4,7 @@
 from __future__ import annotations
 
 from dataclasses import dataclass
-from datetime import datetime
+from datetime import datetime, timezone
 from enum import Enum
 import logging
 from uuid import uuid4
@@ -14,13 +14,11 @@
 from opaque_keys.edx.locator import LibraryContainerLocator, LibraryLocatorV2, LibraryUsageLocatorV2
 from openedx_events.content_authoring.data import (
     ContentObjectChangedData,
-    LibraryBlockData,
     LibraryCollectionData,
     LibraryContainerData,
 )
 from openedx_events.content_authoring.signals import (
     CONTENT_OBJECT_ASSOCIATIONS_CHANGED,
-    LIBRARY_BLOCK_UPDATED,
     LIBRARY_COLLECTION_UPDATED,
     LIBRARY_CONTAINER_CREATED,
     LIBRARY_CONTAINER_DELETED,
@@ -34,8 +32,9 @@
 
 from ..models import ContentLibrary
 from .exceptions import ContentLibraryContainerNotFound
-from .libraries import PublishableItem, library_component_usage_key
+from .libraries import PublishableItem
 from .block_metadata import LibraryXBlockMetadata
+from .. import tasks
 
 # The public API is only the following symbols:
 __all__ = [
@@ -250,7 +249,7 @@ def create_container(
                 content_library.learning_package_id,
                 key=slug,
                 title=title,
-                created=created or datetime.now(),
+                created=created or datetime.now(tz=timezone.utc),
                 created_by=user_id,
             )
         case _:
@@ -280,7 +279,7 @@ def update_container(
     unit_version = authoring_api.create_next_unit_version(
         container.unit,
         title=display_name,
-        created=datetime.now(),
+        created=datetime.now(tz=timezone.utc),
         created_by=user_id,
     )
 
@@ -427,7 +426,7 @@ def update_container_children(
             new_version = authoring_api.create_next_unit_version(
                 container.unit,
                 components=components,  # type: ignore[arg-type]
-                created=datetime.now(),
+                created=datetime.now(tz=timezone.utc),
                 created_by=user_id,
                 entities_action=entities_action,
             )
@@ -478,21 +477,6 @@ def publish_container_changes(container_key: LibraryContainerLocator, user_id: i
         draft_qset=drafts_to_publish,
         published_by=user_id,
     )
-    # Update anything that needs to be updated (e.g. search index):
-    for record in publish_log.records.select_related("entity", "entity__container", "entity__component").all():
-        if hasattr(record.entity, "component"):
-            # This is a child component like an XBLock in a Unit that was published:
-            usage_key = library_component_usage_key(library_key, record.entity.component)
-            LIBRARY_BLOCK_UPDATED.send_event(
-                library_block=LibraryBlockData(library_key=library_key, usage_key=usage_key)
-            )
-        elif hasattr(record.entity, "container"):
-            # This is a child container like a Unit, or is the same "container" we published above.
-            LIBRARY_CONTAINER_UPDATED.send_event(
-                library_container=LibraryContainerData(container_key=container_key)
-            )
-        else:
-            log.warning(
-                f"PublishableEntity {record.entity.pk} / {record.entity.key} was modified during publish operation "
-                "but is of unknown type."
-            )
+    # Update the search index (and anything else) for the affected container + blocks
+    # This is mostly synchronous but may complete some work asynchronously if there are a lot of changes.
+    tasks.wait_for_post_publish_events(publish_log, library_key)