From 9573a5c9be2822c0c8aa8328c27a46c3bf08016b Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Wed, 30 Apr 2025 15:01:26 -0700 Subject: [PATCH 01/20] fix: search index wasn't updated properly on "Publish All Changes" in lib --- .../djangoapps/content/search/handlers.py | 22 -- .../core/djangoapps/content/search/tasks.py | 16 -- .../content_libraries/api/blocks.py | 6 +- .../content_libraries/api/containers.py | 26 +-- .../content_libraries/api/libraries.py | 70 ++---- .../djangoapps/content_libraries/tasks.py | 211 +++++++++++++++++- 6 files changed, 230 insertions(+), 121 deletions(-) diff --git a/openedx/core/djangoapps/content/search/handlers.py b/openedx/core/djangoapps/content/search/handlers.py index 998b2ef870ab..f42a3a7ca32b 100644 --- a/openedx/core/djangoapps/content/search/handlers.py +++ b/openedx/core/djangoapps/content/search/handlers.py @@ -19,7 +19,6 @@ ) from openedx_events.content_authoring.signals import ( CONTENT_LIBRARY_DELETED, - CONTENT_LIBRARY_UPDATED, LIBRARY_BLOCK_CREATED, LIBRARY_BLOCK_DELETED, LIBRARY_BLOCK_UPDATED, @@ -48,7 +47,6 @@ delete_library_block_index_doc, delete_library_container_index_doc, delete_xblock_index_doc, - update_content_library_index_docs, update_library_collection_index_doc, update_library_container_index_doc, upsert_library_block_index_doc, @@ -152,26 +150,6 @@ def library_block_deleted(**kwargs) -> None: delete_library_block_index_doc.apply(args=[str(library_block_data.usage_key)]) -@receiver(CONTENT_LIBRARY_UPDATED) -@only_if_meilisearch_enabled -def content_library_updated_handler(**kwargs) -> None: - """ - Update the index for the content library - """ - content_library_data = kwargs.get("content_library", 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 - - # 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)]) - - @receiver(LIBRARY_COLLECTION_CREATED) @receiver(LIBRARY_COLLECTION_DELETED) @receiver(LIBRARY_COLLECTION_UPDATED) diff --git a/openedx/core/djangoapps/content/search/tasks.py b/openedx/core/djangoapps/content/search/tasks.py index 1ab77aba383f..dde67626b33b 100644 --- a/openedx/core/djangoapps/content/search/tasks.py +++ b/openedx/core/djangoapps/content/search/tasks.py @@ -75,22 +75,6 @@ def delete_library_block_index_doc(usage_key_str: str) -> None: api.delete_index_doc(usage_key) -@shared_task(base=LoggedTask, autoretry_for=(MeilisearchError, ConnectionError)) -@set_code_owner_attribute -def update_content_library_index_docs(library_key_str: str) -> None: - """ - Celery task to update the content index documents for all library blocks in a library - """ - library_key = LibraryLocatorV2.from_string(library_key_str) - - 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)) @set_code_owner_attribute def update_library_collection_index_doc(collection_key_str: str) -> None: diff --git a/openedx/core/djangoapps/content_libraries/api/blocks.py b/openedx/core/djangoapps/content_libraries/api/blocks.py index d440055448f2..f29b3d32753b 100644 --- a/openedx/core/djangoapps/content_libraries/api/blocks.py +++ b/openedx/core/djangoapps/content_libraries/api/blocks.py @@ -63,10 +63,8 @@ ContainerMetadata, ContainerType, ) -from .libraries import ( - library_collection_locator, - PublishableItem, -) +from .collections import library_collection_locator +from .libraries import PublishableItem # 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. diff --git a/openedx/core/djangoapps/content_libraries/api/containers.py b/openedx/core/djangoapps/content_libraries/api/containers.py index d7ba0fcac01f..7e8b8db9dddb 100644 --- a/openedx/core/djangoapps/content_libraries/api/containers.py +++ b/openedx/core/djangoapps/content_libraries/api/containers.py @@ -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__ = [ @@ -472,21 +471,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) diff --git a/openedx/core/djangoapps/content_libraries/api/libraries.py b/openedx/core/djangoapps/content_libraries/api/libraries.py index 8e238f278096..3b2bc911e571 100644 --- a/openedx/core/djangoapps/content_libraries/api/libraries.py +++ b/openedx/core/djangoapps/content_libraries/api/libraries.py @@ -55,15 +55,11 @@ from opaque_keys.edx.locator import LibraryLocatorV2, LibraryUsageLocatorV2 from openedx_events.content_authoring.data import ( ContentLibraryData, - LibraryCollectionData, - ContentObjectChangedData, ) from openedx_events.content_authoring.signals import ( CONTENT_LIBRARY_CREATED, CONTENT_LIBRARY_DELETED, CONTENT_LIBRARY_UPDATED, - LIBRARY_COLLECTION_UPDATED, - CONTENT_OBJECT_ASSOCIATIONS_CHANGED, ) from openedx_learning.api import authoring as authoring_api from openedx_learning.api.authoring_models import Component @@ -75,7 +71,7 @@ from .. import permissions from ..constants import ALL_RIGHTS_RESERVED from ..models import ContentLibrary, ContentLibraryPermission -from .collections import library_collection_locator +from .. import tasks from .exceptions import ( LibraryAlreadyExists, LibraryPermissionIntegrityError, @@ -657,20 +653,24 @@ def get_allowed_block_types(library_key: LibraryLocatorV2): # pylint: disable=u return info + + + def publish_changes(library_key: LibraryLocatorV2, user_id: int | None = None): """ Publish all pending changes to the specified library. """ learning_package = ContentLibrary.objects.get_by_key(library_key).learning_package assert learning_package is not None # shouldn't happen but it's technically possible. - authoring_api.publish_all_drafts(learning_package.id, published_by=user_id) + publish_log = authoring_api.publish_all_drafts(learning_package.id, published_by=user_id) - CONTENT_LIBRARY_UPDATED.send_event( - content_library=ContentLibraryData( - library_key=library_key, - update_blocks=True - ) - ) + # Update the search index (and anything else) for the affected 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) + + # Unlike revert_changes below, we do not have to re-index collections, + # because publishing changes does not affect the component counts, and + # collections themselves don't have draft/published/unpublished status. def revert_changes(library_key: LibraryLocatorV2, user_id: int | None = None) -> None: @@ -680,46 +680,8 @@ def revert_changes(library_key: LibraryLocatorV2, user_id: int | None = None) -> """ learning_package = ContentLibrary.objects.get_by_key(library_key).learning_package assert learning_package is not None # shouldn't happen but it's technically possible. - authoring_api.reset_drafts_to_published(learning_package.id, reset_by=user_id) + with authoring_api.bulk_draft_changes_for(learning_package.id) as draft_change_log: + authoring_api.reset_drafts_to_published(learning_package.id, reset_by=user_id) - CONTENT_LIBRARY_UPDATED.send_event( - content_library=ContentLibraryData( - library_key=library_key, - update_blocks=True - ) - ) - - # For each collection, trigger LIBRARY_COLLECTION_UPDATED signal and set background=True to trigger - # collection indexing asynchronously. - # - # This is to update component counts in all library collections, - # because there may be components that have been discarded in the revert. - for collection in authoring_api.get_collections(learning_package.id): - LIBRARY_COLLECTION_UPDATED.send_event( - library_collection=LibraryCollectionData( - collection_key=library_collection_locator( - library_key=library_key, - collection_key=collection.key, - ), - background=True, - ) - ) - - # Reindex components that are in collections - # - # Use case: When a component that was within a collection has been deleted - # and the changes are reverted, the component should appear in the - # collection again. - components_in_collections = authoring_api.get_components( - learning_package.id, draft=True, namespace='xblock.v1', - ).filter(publishable_entity__collections__isnull=False) - - for component in components_in_collections: - usage_key = library_component_usage_key(library_key, component) - - CONTENT_OBJECT_ASSOCIATIONS_CHANGED.send_event( - content_object=ContentObjectChangedData( - object_id=str(usage_key), - changes=["collections"], - ), - ) + # Call the event handlers as needed. + tasks.wait_for_post_revert_events(draft_change_log, library_key) diff --git a/openedx/core/djangoapps/content_libraries/tasks.py b/openedx/core/djangoapps/content_libraries/tasks.py index b76101e1c62e..bfeca2d8a784 100644 --- a/openedx/core/djangoapps/content_libraries/tasks.py +++ b/openedx/core/djangoapps/content_libraries/tasks.py @@ -23,11 +23,30 @@ from celery.utils.log import get_task_logger from edx_django_utils.monitoring import set_code_owner_attribute, set_code_owner_attribute_from_module from opaque_keys.edx.keys import CourseKey +from opaque_keys.edx.locator import ( + BlockUsageLocator, + LibraryCollectionLocator, + LibraryContainerLocator, + LibraryLocatorV2, +) +from openedx_learning.api import authoring as authoring_api +from openedx_learning.api.authoring_models import DraftChangeLog, PublishLog +from openedx_events.content_authoring.data import ( + ContentLibraryData, + LibraryBlockData, + LibraryCollectionData, + LibraryContainerData, +) +from openedx_events.content_authoring.signals import ( + CONTENT_LIBRARY_UPDATED, + LIBRARY_BLOCK_UPDATED, + LIBRARY_COLLECTION_UPDATED, + LIBRARY_CONTAINER_UPDATED, +) from user_tasks.tasks import UserTask, UserTaskStatus from xblock.fields import Scope -from opaque_keys.edx.locator import BlockUsageLocator from openedx.core.lib import ensure_cms from xmodule.capa_block import ProblemBlock from xmodule.library_content_block import ANY_CAPA_TYPE_VALUE, LegacyLibraryContentBlock @@ -39,10 +58,182 @@ from . import api from .models import ContentLibraryBlockImportTask -logger = logging.getLogger(__name__) +log = logging.getLogger(__name__) TASK_LOGGER = get_task_logger(__name__) +@shared_task(base=LoggedTask) +@set_code_owner_attribute +def send_events_after_publish(publish_log_pk: int, library_key_str: str) -> None: + """ + Send events to trigger actions like updating the search index, after we've + published some items in a library. + + We use the PublishLog record so we can detect exactly what was changed, + including any auto-published changes like child items in containers. + + This happens in a celery task so that it can be run asynchronously if + needed, because the "publish all changes" action can potentially publish + hundreds or even thousands of components/containers at once, and synchronous + event handlers like updating the search index may a while to complete in + that case. + """ + publish_log = PublishLog.objects.get(pk=publish_log_pk) + library_key = LibraryLocatorV2.from_string(library_key_str) + affected_entities = publish_log.records.select_related("entity", "entity__container", "entity__component").all() + + # Update anything that needs to be updated (e.g. search index): + for record in affected_entities: + if hasattr(record.entity, "component"): + usage_key = api.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"): + container_key = api.library_container_locator(library_key, record.entity.container) + 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." + ) + + # This publish will have impacted fields like "last_published", + # "published_by", and possibly "has_unpublished_changes" on the library + # overall, so send a CONTENT_LIBRARY_UPDATED event. + CONTENT_LIBRARY_UPDATED.send_event( + content_library=ContentLibraryData( + library_key=library_key, + # Deprecated: this meant to re-index all blocks, but we now send specific LIBRARY_BLOCK_UPDATED + # events for each block changed. + update_blocks=True, + ) + ) + + +def wait_for_post_publish_events(publish_log: PublishLog, library_key: LibraryLocatorV2): + """ + After publishing some changes, trigger the required event handlers (e.g. + update the search index). Try to wait for that to complete before returning, + up to some reasonable timeout, and then finish anything remaining + asynchonrously. + """ + # Update the search index (and anything else) for the affected blocks + result = send_events_after_publish.apply_async(args=(publish_log.pk, str(library_key))) + # Try waiting a bit for those post-publish events to be handled: + try: + result.get(timeout=15) + except TimeoutError: + pass + # This is fine! The search index is still being updated, and/or other + # event handlers are still following up on the results, but the publish + # already *did* succeed, and the events will continue to be processed in + # the background by the celery worker until everything is updated. + + +@shared_task(base=LoggedTask) +@set_code_owner_attribute +def send_events_after_revert(draft_change_log_id: int, library_key_str: str) -> None: + """ + Send events to trigger actions like updating the search index, after we've + reverted some unpublished changes in a library. + + See notes on the analogous function above, send_events_after_publish. + """ + try: + draft_change_log = DraftChangeLog.objects.get(id=draft_change_log_id) + except DraftChangeLog.DoesNotExist: + # When a revert operation is a no-op, Learning Core deletes the empty + # DraftChangeLog, so we'll assume that's what happened here. + log.info(f"Library revert in {library_key_str} did not result in any changes.") + return + + library_key = LibraryLocatorV2.from_string(library_key_str) + affected_entities = draft_change_log.records.select_related( + "entity", "entity__container", "entity__component", + ).all() + + affected_container_keys: set[LibraryContainerLocator] = set() + affected_collection_keys: set[LibraryCollectionLocator] = set() + + # Update anything that needs to be updated (e.g. search index): + for record in affected_entities: + if hasattr(record.entity, "component"): + usage_key = api.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) + ) + # If any containers contain this component, their child list / component count may need to be updated + # e.g. if this was a newly created component in the container and is now deleted, or this was deleted and + # is now restored. + for parent_container in api.get_containers_contains_component(usage_key): + affected_container_keys.add(parent_container.container_key) + + # TODO: do we also need to send CONTENT_OBJECT_ASSOCIATIONS_CHANGED for this component, or is + # LIBRARY_BLOCK_UPDATED sufficient? + elif hasattr(record.entity, "container"): + container_key = api.library_container_locator(library_key, record.entity.container) + affected_container_keys.add(container_key) + else: + log.warning( + f"PublishableEntity {record.entity.pk} / {record.entity.key} was modified during publish operation " + "but is of unknown type." + ) + # If any collections contain this entity, their item count may need to be updated, e.g. if this was a + # newly created component in the collection and is now deleted, or this was deleted and is now re-added. + for parent_collection in authoring_api.get_entity_collections( + record.entity.learning_package_id, record.entity.key, + ): + collection_key = api.library_collection_locator( + library_key=library_key, + collection_key=parent_collection.key, + ) + affected_collection_keys.add(collection_key) + + for container_key in affected_container_keys: + LIBRARY_CONTAINER_UPDATED.send_event( + library_container=LibraryContainerData(container_key=container_key) + ) + + for collection_key in affected_collection_keys: + LIBRARY_COLLECTION_UPDATED.send_event( + library_collection=LibraryCollectionData(collection_key=collection_key) + ) + + # This revert will have impacted the "has_unpublished_changes" field on the + # library overall, so send a CONTENT_LIBRARY_UPDATED event. + CONTENT_LIBRARY_UPDATED.send_event( + content_library=ContentLibraryData( + library_key=library_key, + # Deprecated: this meant to re-index all blocks, but we now send specific LIBRARY_BLOCK_UPDATED + # events for each block changed. + update_blocks=True, + ) + ) + + +def wait_for_post_revert_events(draft_change_log: DraftChangeLog, library_key: LibraryLocatorV2): + """ + After discard all changes in a library, trigger the required event handlers + (e.g. update the search index). Try to wait for that to complete before + returning, up to some reasonable timeout, and then finish anything remaining + asynchonrously. + """ + # Update the search index (and anything else) for the affected blocks + result = send_events_after_revert.apply_async(args=(draft_change_log.pk, str(library_key))) + # Try waiting a bit for those post-publish events to be handled: + try: + result.get(timeout=15) + except TimeoutError: + pass + # This is fine! The search index is still being updated, and/or other + # event handlers are still following up on the results, but the revert + # already *did* succeed, and the events will continue to be processed in + # the background by the celery worker until everything is updated. + + @shared_task(base=LoggedTask) @set_code_owner_attribute def import_blocks_from_course(import_task_id, course_key_str, use_course_key_as_block_id_suffix=True): @@ -57,9 +248,9 @@ def import_blocks_from_course(import_task_id, course_key_str, use_course_key_as_ def on_progress(block_key, block_num, block_count, exception=None): if exception: - logger.exception('Import block failed: %s', block_key) + log.exception('Import block failed: %s', block_key) else: - logger.info('Import block succesful: %s', block_key) + log.info('Import block succesful: %s', block_key) import_task.save_progress(block_num / block_count) edx_client = api.EdxModulestoreImportClient( @@ -121,6 +312,9 @@ def sync_from_library( ) -> None: """ Celery task to update the children of the library_content block at `dest_block_id`. + + FIXME: this is related to legacy modulestore libraries and shouldn't be part of the + openedx.core.djangoapps.content_libraries app, which is the app for v2 libraries. """ set_code_owner_attribute_from_module(__name__) store = modulestore() @@ -143,6 +337,9 @@ def duplicate_children( ) -> None: """ Celery task to duplicate the children from `source_block_id` to `dest_block_id`. + + FIXME: this is related to legacy modulestore libraries and shouldn't be part of the + openedx.core.djangoapps.content_libraries app, which is the app for v2 libraries. """ set_code_owner_attribute_from_module(__name__) store = modulestore() @@ -180,6 +377,9 @@ def _sync_children( Implementation helper for `sync_from_library` and `duplicate_children` Celery tasks. Can update children with a specific library `library_version`, or latest (`library_version=None`). + + FIXME: this is related to legacy modulestore libraries and shouldn't be part of the + openedx.core.djangoapps.content_libraries app, which is the app for v2 libraries. """ source_blocks = [] library_key = dest_block.source_library_key.for_branch( @@ -220,6 +420,9 @@ def _copy_overrides( ) -> None: """ Copy any overrides the user has made on children of `source` over to the children of `dest_block`, recursively. + + FIXME: this is related to legacy modulestore libraries and shouldn't be part of the + openedx.core.djangoapps.content_libraries app, which is the app for v2 libraries. """ for field in source_block.fields.values(): if field.scope == Scope.settings and field.is_set_on(source_block): From f88ce1aa654bb763d4f7bc50243262a5dbeb5b45 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Thu, 1 May 2025 19:14:41 -0700 Subject: [PATCH 02/20] test: update tests --- .../content_libraries/tests/test_api.py | 206 +++++++----------- 1 file changed, 84 insertions(+), 122 deletions(-) diff --git a/openedx/core/djangoapps/content_libraries/tests/test_api.py b/openedx/core/djangoapps/content_libraries/tests/test_api.py index 8f79ec7f6339..7c463d16be81 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_api.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_api.py @@ -556,45 +556,36 @@ def test_set_library_component_collections(self): assert len(authoring_api.get_collection(self.lib2.learning_package_id, self.col2.key).entities.all()) == 1 assert len(authoring_api.get_collection(self.lib2.learning_package_id, self.col3.key).entities.all()) == 1 - self.assertDictContainsSubset( - { - "signal": CONTENT_OBJECT_ASSOCIATIONS_CHANGED, - "sender": None, - "content_object": ContentObjectChangedData( - object_id=self.lib2_problem_block["id"], - changes=["collections"], + assert { + "signal": CONTENT_OBJECT_ASSOCIATIONS_CHANGED, + "sender": None, + "content_object": ContentObjectChangedData( + object_id=self.lib2_problem_block["id"], + changes=["collections"], + ), + }.items() <= event_receiver.call_args_list[0].kwargs.items() + assert { + "signal": LIBRARY_COLLECTION_UPDATED, + "sender": None, + "library_collection": LibraryCollectionData( + collection_key=api.library_collection_locator( + self.lib2.library_key, + collection_key=self.col2.key, ), - }, - event_receiver.call_args_list[0].kwargs, - ) - self.assertDictContainsSubset( - { - "signal": LIBRARY_COLLECTION_UPDATED, - "sender": None, - "library_collection": LibraryCollectionData( - collection_key=api.library_collection_locator( - self.lib2.library_key, - collection_key=self.col2.key, - ), - background=True, + background=True, + ), + }.items() <= collection_update_event_receiver.call_args_list[0].kwargs.items() + assert { + "signal": LIBRARY_COLLECTION_UPDATED, + "sender": None, + "library_collection": LibraryCollectionData( + collection_key=api.library_collection_locator( + self.lib2.library_key, + collection_key=self.col3.key, ), - }, - collection_update_event_receiver.call_args_list[0].kwargs, - ) - self.assertDictContainsSubset( - { - "signal": LIBRARY_COLLECTION_UPDATED, - "sender": None, - "library_collection": LibraryCollectionData( - collection_key=api.library_collection_locator( - self.lib2.library_key, - collection_key=self.col3.key, - ), - background=True, - ), - }, - collection_update_event_receiver.call_args_list[1].kwargs, - ) + background=True, + ), + }.items() <= collection_update_event_receiver.call_args_list[1].kwargs.items() def test_delete_library_block(self): api.update_library_collection_items( @@ -691,72 +682,46 @@ def test_restore_library_block(self): ) def test_add_component_and_revert(self): - # Add component and publish - api.update_library_collection_items( - self.lib1.library_key, - self.col1.key, - opaque_keys=[ - UsageKey.from_string(self.lib1_problem_block["id"]), - ], - ) + # Publish changes api.publish_changes(self.lib1.library_key) - # Add component and revert + # Create a new component that will only exist as a draft + new_problem_block = self._add_block_to_library( + self.lib1.library_key, "problem", "problemNEW", + ) + + # Add component. Note: collections are not part of the draft/publish cycle so this is not a draft change. api.update_library_collection_items( self.lib1.library_key, self.col1.key, opaque_keys=[ UsageKey.from_string(self.lib1_html_block["id"]), + UsageKey.from_string(new_problem_block["id"]), ], ) - event_receiver = mock.Mock() - CONTENT_OBJECT_ASSOCIATIONS_CHANGED.connect(event_receiver) collection_update_event_receiver = mock.Mock() LIBRARY_COLLECTION_UPDATED.connect(collection_update_event_receiver) api.revert_changes(self.lib1.library_key) assert collection_update_event_receiver.call_count == 1 - assert event_receiver.call_count == 2 - 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, - ), - }, - collection_update_event_receiver.call_args_list[0].kwargs, - ) - self.assertDictContainsSubset( - { - "signal": CONTENT_OBJECT_ASSOCIATIONS_CHANGED, - "sender": None, - "content_object": ContentObjectChangedData( - object_id=str(self.lib1_problem_block["id"]), - changes=["collections"], + assert { + "signal": LIBRARY_COLLECTION_UPDATED, + "sender": None, + "library_collection": LibraryCollectionData( + collection_key=api.library_collection_locator( + self.lib1.library_key, + collection_key=self.col1.key, ), - }, - event_receiver.call_args_list[0].kwargs, - ) - self.assertDictContainsSubset( - { - "signal": CONTENT_OBJECT_ASSOCIATIONS_CHANGED, - "sender": None, - "content_object": ContentObjectChangedData( - object_id=str(self.lib1_html_block["id"]), - changes=["collections"], - ), - }, - event_receiver.call_args_list[1].kwargs, - ) + ), + }.items() <= collection_update_event_receiver.call_args_list[0].kwargs.items() def test_delete_component_and_revert(self): + """ + When a component is deleted and then the delete is reverted, signals + will be emitted to update any containing collections. + """ # Add components and publish api.update_library_collection_items( self.lib1.library_key, @@ -771,51 +736,22 @@ def test_delete_component_and_revert(self): # Delete component and revert api.delete_library_block(UsageKey.from_string(self.lib1_problem_block["id"])) - event_receiver = mock.Mock() - CONTENT_OBJECT_ASSOCIATIONS_CHANGED.connect(event_receiver) collection_update_event_receiver = mock.Mock() LIBRARY_COLLECTION_UPDATED.connect(collection_update_event_receiver) api.revert_changes(self.lib1.library_key) assert collection_update_event_receiver.call_count == 1 - assert event_receiver.call_count == 2 - 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, + assert { + "signal": LIBRARY_COLLECTION_UPDATED, + "sender": None, + "library_collection": LibraryCollectionData( + collection_key=api.library_collection_locator( + self.lib1.library_key, + collection_key=self.col1.key, ), - }, - collection_update_event_receiver.call_args_list[0].kwargs, - ) - self.assertDictContainsSubset( - { - "signal": CONTENT_OBJECT_ASSOCIATIONS_CHANGED, - "sender": None, - "content_object": ContentObjectChangedData( - object_id=str(self.lib1_problem_block["id"]), - changes=["collections"], - ), - }, - event_receiver.call_args_list[0].kwargs, - ) - self.assertDictContainsSubset( - { - "signal": CONTENT_OBJECT_ASSOCIATIONS_CHANGED, - "sender": None, - "content_object": ContentObjectChangedData( - object_id=str(self.lib1_html_block["id"]), - changes=["collections"], - ), - }, - event_receiver.call_args_list[1].kwargs, - ) + ), + }.items() <= collection_update_event_receiver.call_args_list[0].kwargs.items() class ContentLibraryContainersTest(ContentLibrariesRestApiTest, OpenEdxEventsTestMixin): @@ -945,3 +881,29 @@ def test_call_container_update_signal_when_update_component(self): self._set_library_block_fields(self.html_block_usage_key, {"data": block_olx, "metadata": {}}) self._validate_calls_of_html_block(container_update_event_receiver) + + def test_delete_component_and_revert(self): + """ + When a component is deleted and then the delete is reverted, signals + will be emitted to update any containing containers. + """ + # Add components and publish + api.update_container_children(self.unit1.container_key, [ + UsageKey.from_string(self.problem_block["id"]), + ], user_id=None) + api.publish_changes(self.lib1.library_key) + + # Delete component and revert + api.delete_library_block(UsageKey.from_string(self.problem_block["id"])) + + container_event_receiver = mock.Mock() + LIBRARY_CONTAINER_UPDATED.connect(container_event_receiver) + + api.revert_changes(self.lib1.library_key) + + assert container_event_receiver.call_count == 1 + assert { + "signal": LIBRARY_CONTAINER_UPDATED, + "sender": None, + "library_container": LibraryContainerData(container_key=self.unit1.container_key), + }.items() <= container_event_receiver.call_args_list[0].kwargs.items() From 4701a25f54ba3257d7bc7fcafe5045e3a7735cc0 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Thu, 1 May 2025 19:41:10 -0700 Subject: [PATCH 03/20] fix: flaky test --- .../content_libraries/tests/test_api.py | 30 +++++++------------ 1 file changed, 11 insertions(+), 19 deletions(-) diff --git a/openedx/core/djangoapps/content_libraries/tests/test_api.py b/openedx/core/djangoapps/content_libraries/tests/test_api.py index 7c463d16be81..5c29c25fe539 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_api.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_api.py @@ -564,28 +564,20 @@ def test_set_library_component_collections(self): changes=["collections"], ), }.items() <= event_receiver.call_args_list[0].kwargs.items() - assert { - "signal": LIBRARY_COLLECTION_UPDATED, - "sender": None, - "library_collection": LibraryCollectionData( - collection_key=api.library_collection_locator( - self.lib2.library_key, - collection_key=self.col2.key, - ), + + assert len(collection_update_event_receiver.call_args_list) == 2 + collection_update_events = [call.kwargs for call in collection_update_event_receiver.call_args_list] + assert all(event["signal"] == LIBRARY_COLLECTION_UPDATED for event in collection_update_events) + assert {event["library_collection"] for event in collection_update_events} == { + LibraryCollectionData( + collection_key=api.library_collection_locator(self.lib2.library_key, collection_key=self.col2.key), background=True, ), - }.items() <= collection_update_event_receiver.call_args_list[0].kwargs.items() - assert { - "signal": LIBRARY_COLLECTION_UPDATED, - "sender": None, - "library_collection": LibraryCollectionData( - collection_key=api.library_collection_locator( - self.lib2.library_key, - collection_key=self.col3.key, - ), + LibraryCollectionData( + collection_key=api.library_collection_locator(self.lib2.library_key, collection_key=self.col3.key), background=True, - ), - }.items() <= collection_update_event_receiver.call_args_list[1].kwargs.items() + ) + } def test_delete_library_block(self): api.update_library_collection_items( From 6d72525199471bb86d9a511845714b960d6dbc72 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Mon, 5 May 2025 17:02:05 -0700 Subject: [PATCH 04/20] fix: revert some previous changes --- .../djangoapps/content/search/handlers.py | 22 +++++++++++++++++++ .../core/djangoapps/content/search/tasks.py | 13 +++++++++++ 2 files changed, 35 insertions(+) diff --git a/openedx/core/djangoapps/content/search/handlers.py b/openedx/core/djangoapps/content/search/handlers.py index f42a3a7ca32b..6ecad513e258 100644 --- a/openedx/core/djangoapps/content/search/handlers.py +++ b/openedx/core/djangoapps/content/search/handlers.py @@ -19,6 +19,7 @@ ) from openedx_events.content_authoring.signals import ( CONTENT_LIBRARY_DELETED, + CONTENT_LIBRARY_UPDATED, LIBRARY_BLOCK_CREATED, LIBRARY_BLOCK_DELETED, LIBRARY_BLOCK_UPDATED, @@ -47,6 +48,7 @@ delete_library_block_index_doc, delete_library_container_index_doc, delete_xblock_index_doc, + update_content_library_index_docs, update_library_collection_index_doc, update_library_container_index_doc, upsert_library_block_index_doc, @@ -150,6 +152,26 @@ def library_block_deleted(**kwargs) -> None: delete_library_block_index_doc.apply(args=[str(library_block_data.usage_key)]) +@receiver(CONTENT_LIBRARY_UPDATED) +@only_if_meilisearch_enabled +def content_library_updated_handler(**kwargs) -> None: + """ + Update the index for the content library + """ + content_library_data = kwargs.get("content_library", 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 + + # 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) @receiver(LIBRARY_COLLECTION_DELETED) @receiver(LIBRARY_COLLECTION_UPDATED) diff --git a/openedx/core/djangoapps/content/search/tasks.py b/openedx/core/djangoapps/content/search/tasks.py index dde67626b33b..5015f6912b10 100644 --- a/openedx/core/djangoapps/content/search/tasks.py +++ b/openedx/core/djangoapps/content/search/tasks.py @@ -75,6 +75,19 @@ def delete_library_block_index_doc(usage_key_str: str) -> None: api.delete_index_doc(usage_key) +@shared_task(base=LoggedTask, autoretry_for=(MeilisearchError, ConnectionError)) +@set_code_owner_attribute +def update_content_library_index_docs(library_key_str: str) -> None: + """ + Celery task to update the content index documents for all library blocks in a library + """ + library_key = LibraryLocatorV2.from_string(library_key_str) + + log.info("Updating content index documents for library with id: %s", library_key) + + api.upsert_content_library_index_docs(library_key) + + @shared_task(base=LoggedTask, autoretry_for=(MeilisearchError, ConnectionError)) @set_code_owner_attribute def update_library_collection_index_doc(collection_key_str: str) -> None: From 1bb4fb181998a60f124542a0b94a94a28c364b91 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Mon, 5 May 2025 17:02:45 -0700 Subject: [PATCH 05/20] fix: make events more focused --- openedx/core/djangoapps/content/search/api.py | 23 ----------- .../content/search/tests/test_api.py | 15 -------- .../djangoapps/content_libraries/tasks.py | 38 ++++++------------- 3 files changed, 12 insertions(+), 64 deletions(-) diff --git a/openedx/core/djangoapps/content/search/api.py b/openedx/core/djangoapps/content/search/api.py index b3614e9cc673..b866f13dc474 100644 --- a/openedx/core/djangoapps/content/search/api.py +++ b/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 diff --git a/openedx/core/djangoapps/content/search/tests/test_api.py b/openedx/core/djangoapps/content/search/tests/test_api.py index 46354d6eb3cd..23555c6189ce 100644 --- a/openedx/core/djangoapps/content/search/tests/test_api.py +++ b/openedx/core/djangoapps/content/search/tests/test_api.py @@ -733,21 +733,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 diff --git a/openedx/core/djangoapps/content_libraries/tasks.py b/openedx/core/djangoapps/content_libraries/tasks.py index bfeca2d8a784..1b5ad13e8793 100644 --- a/openedx/core/djangoapps/content_libraries/tasks.py +++ b/openedx/core/djangoapps/content_libraries/tasks.py @@ -39,6 +39,8 @@ ) from openedx_events.content_authoring.signals import ( CONTENT_LIBRARY_UPDATED, + LIBRARY_BLOCK_CREATED, + LIBRARY_BLOCK_DELETED, LIBRARY_BLOCK_UPDATED, LIBRARY_COLLECTION_UPDATED, LIBRARY_CONTAINER_UPDATED, @@ -100,18 +102,6 @@ def send_events_after_publish(publish_log_pk: int, library_key_str: str) -> None "but is of unknown type." ) - # This publish will have impacted fields like "last_published", - # "published_by", and possibly "has_unpublished_changes" on the library - # overall, so send a CONTENT_LIBRARY_UPDATED event. - CONTENT_LIBRARY_UPDATED.send_event( - content_library=ContentLibraryData( - library_key=library_key, - # Deprecated: this meant to re-index all blocks, but we now send specific LIBRARY_BLOCK_UPDATED - # events for each block changed. - update_blocks=True, - ) - ) - def wait_for_post_publish_events(publish_log: PublishLog, library_key: LibraryLocatorV2): """ @@ -160,11 +150,18 @@ def send_events_after_revert(draft_change_log_id: int, library_key_str: str) -> # Update anything that needs to be updated (e.g. search index): for record in affected_entities: + # This will be true if the entity was [soft] deleted, but we're now reverting that deletion: + is_undeleted = (record.old_version is None and record.new_version is not None) + # This will be true if the entity was created and we're now deleting it by reverting that creation: + is_deleted = (record.old_version is not None and record.new_version is None) if hasattr(record.entity, "component"): usage_key = api.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) - ) + event = LIBRARY_BLOCK_UPDATED + if is_deleted: + event = LIBRARY_BLOCK_DELETED + elif is_undeleted: + event = LIBRARY_BLOCK_CREATED + event.send_event(library_block=LibraryBlockData(library_key=library_key, usage_key=usage_key)) # If any containers contain this component, their child list / component count may need to be updated # e.g. if this was a newly created component in the container and is now deleted, or this was deleted and # is now restored. @@ -202,17 +199,6 @@ def send_events_after_revert(draft_change_log_id: int, library_key_str: str) -> library_collection=LibraryCollectionData(collection_key=collection_key) ) - # This revert will have impacted the "has_unpublished_changes" field on the - # library overall, so send a CONTENT_LIBRARY_UPDATED event. - CONTENT_LIBRARY_UPDATED.send_event( - content_library=ContentLibraryData( - library_key=library_key, - # Deprecated: this meant to re-index all blocks, but we now send specific LIBRARY_BLOCK_UPDATED - # events for each block changed. - update_blocks=True, - ) - ) - def wait_for_post_revert_events(draft_change_log: DraftChangeLog, library_key: LibraryLocatorV2): """ From d8c6a2c3f8829f69586da8ada42df9c4ae7adf4a Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Mon, 5 May 2025 17:02:54 -0700 Subject: [PATCH 06/20] test: more comprehensive tests for events --- .../tests/test_content_libraries.py | 249 ---------------- .../content_libraries/tests/test_events.py | 270 ++++++++++++++++++ 2 files changed, 270 insertions(+), 249 deletions(-) create mode 100644 openedx/core/djangoapps/content_libraries/tests/test_events.py diff --git a/openedx/core/djangoapps/content_libraries/tests/test_content_libraries.py b/openedx/core/djangoapps/content_libraries/tests/test_content_libraries.py index e1b34ebfcbf8..288cae5a2501 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_content_libraries.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_content_libraries.py @@ -792,255 +792,6 @@ def test_library_blocks_limit(self): # Second block should throw error self._add_block_to_library(lib_id, "problem", "problem1", expect_response=400) - def test_content_library_create_event(self): - """ - Check that CONTENT_LIBRARY_CREATED event is sent when a content library is created. - """ - event_receiver = Mock() - CONTENT_LIBRARY_CREATED.connect(event_receiver) - lib = self._create_library( - slug="test_lib_event_create", - title="Event Test Library", - description="Testing event in library" - ) - library_key = LibraryLocatorV2.from_string(lib['id']) - - event_receiver.assert_called_once() - self.assertDictContainsSubset( - { - "signal": CONTENT_LIBRARY_CREATED, - "sender": None, - "content_library": ContentLibraryData( - library_key=library_key, - update_blocks=False, - ), - }, - event_receiver.call_args.kwargs - ) - - def test_content_library_update_event(self): - """ - Check that CONTENT_LIBRARY_UPDATED event is sent when a content library is updated. - """ - event_receiver = Mock() - CONTENT_LIBRARY_UPDATED.connect(event_receiver) - lib = self._create_library( - slug="test_lib_event_update", - title="Event Test Library", - description="Testing event in library" - ) - - lib2 = self._update_library(lib["id"], title="New Title") - library_key = LibraryLocatorV2.from_string(lib2['id']) - - event_receiver.assert_called_once() - self.assertDictContainsSubset( - { - "signal": CONTENT_LIBRARY_UPDATED, - "sender": None, - "content_library": ContentLibraryData( - library_key=library_key, - update_blocks=False, - ), - }, - event_receiver.call_args.kwargs - ) - - def test_content_library_delete_event(self): - """ - Check that CONTENT_LIBRARY_DELETED event is sent when a content library is deleted. - """ - event_receiver = Mock() - CONTENT_LIBRARY_DELETED.connect(event_receiver) - lib = self._create_library( - slug="test_lib_event_delete", - title="Event Test Library", - description="Testing event in library" - ) - library_key = LibraryLocatorV2.from_string(lib['id']) - - self._delete_library(lib["id"]) - - event_receiver.assert_called_once() - self.assertDictContainsSubset( - { - "signal": CONTENT_LIBRARY_DELETED, - "sender": None, - "content_library": ContentLibraryData( - library_key=library_key, - update_blocks=False, - ), - }, - event_receiver.call_args.kwargs - ) - - def test_library_block_create_event(self): - """ - Check that LIBRARY_BLOCK_CREATED event is sent when a library block is created. - """ - event_receiver = Mock() - LIBRARY_BLOCK_CREATED.connect(event_receiver) - lib = self._create_library( - slug="test_lib_block_event_create", - title="Event Test Library", - description="Testing event in library" - ) - lib_id = lib["id"] - self._add_block_to_library(lib_id, "problem", "problem1") - - library_key = LibraryLocatorV2.from_string(lib_id) - usage_key = LibraryUsageLocatorV2( - lib_key=library_key, - block_type="problem", - usage_id="problem1" - ) - - event_receiver.assert_called_once() - self.assertDictContainsSubset( - { - "signal": LIBRARY_BLOCK_CREATED, - "sender": None, - "library_block": LibraryBlockData( - library_key=library_key, - usage_key=usage_key - ), - }, - event_receiver.call_args.kwargs - ) - - def test_library_block_olx_update_event(self): - """ - Check that LIBRARY_BLOCK_CREATED event is sent when the OLX source is updated. - """ - event_receiver = Mock() - LIBRARY_BLOCK_UPDATED.connect(event_receiver) - lib = self._create_library( - slug="test_lib_block_event_olx_update", - title="Event Test Library", - description="Testing event in library" - ) - lib_id = lib["id"] - - library_key = LibraryLocatorV2.from_string(lib_id) - - block = self._add_block_to_library(lib_id, "problem", "problem1") - block_id = block["id"] - usage_key = LibraryUsageLocatorV2( - lib_key=library_key, - block_type="problem", - usage_id="problem1" - ) - - new_olx = """ - - -

This is a normal capa problem with unicode 🔥. It has "maximum attempts" set to **5**.

- - - XBlock metadata only - XBlock data/metadata and associated static asset files - Static asset files for XBlocks and courseware - XModule metadata only - - - - """.strip() - - self._set_library_block_olx(block_id, new_olx) - - event_receiver.assert_called_once() - self.assertDictContainsSubset( - { - "signal": LIBRARY_BLOCK_UPDATED, - "sender": None, - "library_block": LibraryBlockData( - library_key=library_key, - usage_key=usage_key - ), - }, - event_receiver.call_args.kwargs - ) - - def test_library_block_add_asset_update_event(self): - """ - Check that LIBRARY_BLOCK_CREATED event is sent when a static asset is - uploaded associated with the XBlock. - """ - event_receiver = Mock() - LIBRARY_BLOCK_UPDATED.connect(event_receiver) - lib = self._create_library( - slug="test_lib_block_event_add_asset_update", - title="Event Test Library", - description="Testing event in library" - ) - lib_id = lib["id"] - - library_key = LibraryLocatorV2.from_string(lib_id) - - block = self._add_block_to_library(lib_id, "html", "h1") - block_id = block["id"] - self._set_library_block_asset(block_id, "static/test.txt", b"data") - - usage_key = LibraryUsageLocatorV2( - lib_key=library_key, - block_type="html", - usage_id="h1" - ) - - event_receiver.assert_called_once() - self.assertDictContainsSubset( - { - "signal": LIBRARY_BLOCK_UPDATED, - "sender": None, - "library_block": LibraryBlockData( - library_key=library_key, - usage_key=usage_key - ), - }, - event_receiver.call_args.kwargs - ) - - def test_library_block_del_asset_update_event(self): - """ - Check that LIBRARY_BLOCK_CREATED event is sent when a static asset is - removed from XBlock. - """ - event_receiver = Mock() - LIBRARY_BLOCK_UPDATED.connect(event_receiver) - lib = self._create_library( - slug="test_lib_block_event_del_asset_update", - title="Event Test Library", - description="Testing event in library" - ) - lib_id = lib["id"] - - library_key = LibraryLocatorV2.from_string(lib_id) - - block = self._add_block_to_library(lib_id, "html", "h321") - block_id = block["id"] - self._set_library_block_asset(block_id, "static/test.txt", b"data") - - self._delete_library_block_asset(block_id, 'static/text.txt') - - usage_key = LibraryUsageLocatorV2( - lib_key=library_key, - block_type="html", - usage_id="h321" - ) - - event_receiver.assert_called() - self.assertDictContainsSubset( - { - "signal": LIBRARY_BLOCK_UPDATED, - "sender": None, - "library_block": LibraryBlockData( - library_key=library_key, - usage_key=usage_key - ), - }, - event_receiver.call_args.kwargs - ) - def test_library_block_delete_event(self): """ Check that LIBRARY_BLOCK_DELETED event is sent when a content library is deleted. diff --git a/openedx/core/djangoapps/content_libraries/tests/test_events.py b/openedx/core/djangoapps/content_libraries/tests/test_events.py new file mode 100644 index 000000000000..98a8f801b63c --- /dev/null +++ b/openedx/core/djangoapps/content_libraries/tests/test_events.py @@ -0,0 +1,270 @@ +""" +Tests for Learning-Core-based Content Libraries +""" +import ddt +from opaque_keys.edx.locator import LibraryLocatorV2, LibraryUsageLocatorV2 +from openedx_events.content_authoring.data import ( + ContentLibraryData, + LibraryBlockData, +) +from openedx_events.content_authoring.signals import ( + CONTENT_LIBRARY_CREATED, + CONTENT_LIBRARY_DELETED, + CONTENT_LIBRARY_UPDATED, + LIBRARY_BLOCK_CREATED, + LIBRARY_BLOCK_DELETED, + LIBRARY_BLOCK_UPDATED +) +from openedx_events.tests.utils import OpenEdxEventsTestMixin + +from openedx.core.djangoapps.content_libraries.tests.base import ContentLibrariesRestApiTest +from openedx.core.djangolib.testing.utils import skip_unless_cms + + +@skip_unless_cms +@ddt.ddt +class ContentLibrariesTestCase(ContentLibrariesRestApiTest, OpenEdxEventsTestMixin): + """ + General tests for Learning-Core-based Content Libraries + + These tests use the REST API, which in turn relies on the Python API. + Some tests may use the python API directly if necessary to provide + coverage of any code paths not accessible via the REST API. + + In general, these tests should + (1) Use public APIs only - don't directly create data using other methods, + which results in a less realistic test and ties the test suite too + closely to specific implementation details. + (Exception: users can be provisioned using a user factory) + (2) Assert that fields are present in responses, but don't assert that the + entire response has some specific shape. That way, things like adding + new fields to an API response, which are backwards compatible, won't + break any tests, but backwards-incompatible API changes will. + + WARNING: every test should have a unique library slug, because even though + the django/mysql database gets reset for each test case, the lookup between + library slug and bundle UUID does not because it's assumed to be immutable + and cached forever. + """ + ALL_EVENTS = [ + CONTENT_LIBRARY_CREATED, + CONTENT_LIBRARY_DELETED, + CONTENT_LIBRARY_UPDATED, + LIBRARY_BLOCK_CREATED, + LIBRARY_BLOCK_DELETED, + LIBRARY_BLOCK_UPDATED, + ] + ENABLED_OPENEDX_EVENTS = [e.event_type for e in ALL_EVENTS] + + @classmethod + def setUpClass(cls): + """ + Set up class method for the Test class. + + TODO: It's unclear why we need to call start_events_isolation ourselves rather than relying on + OpenEdxEventsTestMixin.setUpClass to handle it. It fails it we don't, and many other test cases do it, + so we're following a pattern here. But that pattern doesn't really make sense. + """ + super().setUpClass() + cls.start_events_isolation() + + def setUp(self): + super().setUp() + + # Create some useful data: + self.lib1 = self._create_library( + slug="test_lib_1", + title="Library 1", + description="First Library for testing", + ) + self.lib1_key = LibraryLocatorV2.from_string(self.lib1['id']) + + # From now on, every time an event is emitted, add it to this set: + self.new_events: list[dict] = [] + + def event_receiver(**kwargs): + self.new_events.append(kwargs) + + for e in self.ALL_EVENTS: + e.connect(event_receiver) + + def disconnect_all(): + for e in self.ALL_EVENTS: + e.disconnect(event_receiver) + + self.addCleanup(disconnect_all) + + def clear_events(self): + """ Clear the log of events that we've seen so far. """ + self.new_events.clear() + + def expect_new_events(self, *expected_events: list[dict]): + """ + assert the the specified events have been emitted since the last call to + this function. + """ + # We assume the events may not be in order. Assuming a specific order can lead to flaky tests. + for expected in expected_events: + found = False + for i, actual in enumerate(self.new_events): + if expected.items() <= actual.items(): + self.new_events.pop(i) + found = True + break + if not found: + raise AssertionError(f"Event {expected} not found among actual events: {self.new_events}") + if len(self.new_events) > 0: + raise AssertionError(f"Events were emitted but not expected: {self.new_events}") + self.clear_events() + + def test_content_library_crud_events(self): + """ + Check that CONTENT_LIBRARY_CREATED event is sent when a content library is created, updated, and deleted + """ + # Setup: none + # Action - create a library + new_lib = self._create_library( + slug="new_lib", + title="New Testing Library", + description="New Library for testing", + ) + lib_key = LibraryLocatorV2.from_string(new_lib['id']) + + # Expect a CREATED event: + self.expect_new_events({ + "signal": CONTENT_LIBRARY_CREATED, + "content_library": ContentLibraryData(library_key=lib_key), + }) + + # Action - change the library name: + self._update_library(lib_key=str(lib_key), title="New title") + # Expect an UPDATED event: + self.expect_new_events({ + "signal": CONTENT_LIBRARY_UPDATED, + "content_library": ContentLibraryData(library_key=lib_key), + }) + + # Action - delete the library: + self._delete_library(str(lib_key)) + # Expect a DELETED event: + self.expect_new_events({ + "signal": CONTENT_LIBRARY_DELETED, + "content_library": ContentLibraryData(library_key=lib_key), + }) + + # Should deleting a library send out _DELETED events for all the items in the library too? + + def test_library_block_create_event(self): + """ + Check that LIBRARY_BLOCK_CREATED event is sent when a library block is created. + """ + add_result = self._add_block_to_library(self.lib1_key, "problem", "problem1") + usage_key = LibraryUsageLocatorV2.from_string(add_result["id"]) + + self.expect_new_events({ + "signal": LIBRARY_BLOCK_CREATED, + "library_block": LibraryBlockData(self.lib1_key, usage_key), + }) + + def test_library_block_update_and_publish_events(self): + """ + Check that appropriate events are emitted when an existing block is updated. + """ + # This block should be ignored: + self._add_block_to_library(self.lib1_key, "problem", "problem1") + # This block will be used in the tests: + add_result = self._add_block_to_library(self.lib1_key, "problem", "problem2") + usage_key = LibraryUsageLocatorV2.from_string(add_result["id"]) + # Clear events from creating the blocks: + self.clear_events() + + # Now update the block's OLX: + new_olx = """ + + ... + + """.strip() + self._set_library_block_olx(usage_key, new_olx) + self.expect_new_events({ + "signal": LIBRARY_BLOCK_UPDATED, + "library_block": LibraryBlockData(self.lib1_key, usage_key), + }) + + # Now add a static asset file to the block: + self._set_library_block_asset(usage_key, "static/test.txt", b"data") + self.expect_new_events({ + "signal": LIBRARY_BLOCK_UPDATED, + "library_block": LibraryBlockData(self.lib1_key, usage_key), + }) + + # Then delete the static asset: + self._delete_library_block_asset(usage_key, 'static/text.txt') + self.expect_new_events({ + "signal": LIBRARY_BLOCK_UPDATED, + "library_block": LibraryBlockData(self.lib1_key, usage_key), + }) + + # Then publish the block: + self._publish_library_block(usage_key) + self.expect_new_events({ + "signal": LIBRARY_BLOCK_UPDATED, # FIXME: this should be a _PUBLISHED event + "library_block": LibraryBlockData(self.lib1_key, usage_key), + }) + + def test_revert_delete(self): + """ + Test that when a block is deleted and then the delete is reverted, a + _CREATED event is sent. + """ + # This block should be ignored: + self._add_block_to_library(self.lib1_key, "problem", "problem1") + # This block will be used in the tests: + add_result = self._add_block_to_library(self.lib1_key, "problem", "problem2") + usage_key = LibraryUsageLocatorV2.from_string(add_result["id"]) + # Publish changes + self._commit_library_changes(self.lib1_key) + # Clear events from creating the blocks: + self.clear_events() + + # Delete the block: + self._delete_library_block(usage_key) + # That should emit a _DELETED event: + self.expect_new_events({ + "signal": LIBRARY_BLOCK_DELETED, + "library_block": LibraryBlockData(self.lib1_key, usage_key), + }) + + # Revert the change: + self._revert_library_changes(self.lib1_key) + # That should result in a _CREATED event: + self.expect_new_events({ + "signal": LIBRARY_BLOCK_CREATED, + "library_block": LibraryBlockData(self.lib1_key, usage_key), + }) + + def test_revert_create(self): + """ + Test that when a block is created and then the changes are reverted, a + _DELETED event is sent. + """ + # Publish any changes from setUp() + self._commit_library_changes(self.lib1_key) + # Clear events: + self.clear_events() + + # Create the block: + add_result = self._add_block_to_library(self.lib1_key, "problem", "problem2") + usage_key = LibraryUsageLocatorV2.from_string(add_result["id"]) + # That should result in a _CREATED event: + self.expect_new_events({ + "signal": LIBRARY_BLOCK_CREATED, + "library_block": LibraryBlockData(self.lib1_key, usage_key), + }) + + # Revert the change: + self._revert_library_changes(self.lib1_key) + # That should result in a _DELETED event: + self.expect_new_events({ + "signal": LIBRARY_BLOCK_DELETED, + "library_block": LibraryBlockData(self.lib1_key, usage_key), + }) From f9ca9ed045792f8481e4d8a9ac5766f69df43ee1 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Tue, 6 May 2025 09:42:34 -0700 Subject: [PATCH 07/20] test: remove redundant tests --- .../tests/test_content_libraries.py | 75 +------------------ 1 file changed, 3 insertions(+), 72 deletions(-) diff --git a/openedx/core/djangoapps/content_libraries/tests/test_content_libraries.py b/openedx/core/djangoapps/content_libraries/tests/test_content_libraries.py index 288cae5a2501..e62a2b6f0243 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_content_libraries.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_content_libraries.py @@ -3,7 +3,7 @@ """ from datetime import datetime, timezone from unittest import skip -from unittest.mock import Mock, patch +from unittest.mock import patch import ddt from django.contrib.auth.models import Group @@ -11,16 +11,6 @@ from django.test.client import Client from freezegun import freeze_time from opaque_keys.edx.locator import LibraryLocatorV2, LibraryUsageLocatorV2 -from openedx_events.content_authoring.data import ContentLibraryData, LibraryBlockData -from openedx_events.content_authoring.signals import ( - CONTENT_LIBRARY_CREATED, - CONTENT_LIBRARY_DELETED, - CONTENT_LIBRARY_UPDATED, - LIBRARY_BLOCK_CREATED, - LIBRARY_BLOCK_DELETED, - LIBRARY_BLOCK_UPDATED -) -from openedx_events.tests.utils import OpenEdxEventsTestMixin from organizations.models import Organization from rest_framework.test import APITestCase @@ -31,7 +21,7 @@ URL_BLOCK_METADATA_URL, URL_BLOCK_RENDER_VIEW, URL_BLOCK_XBLOCK_HANDLER, - ContentLibrariesRestApiTest + ContentLibrariesRestApiTest, ) from openedx.core.djangoapps.xblock import api as xblock_api from openedx.core.djangolib.testing.utils import skip_unless_cms @@ -39,7 +29,7 @@ @skip_unless_cms @ddt.ddt -class ContentLibrariesTestCase(ContentLibrariesRestApiTest, OpenEdxEventsTestMixin): +class ContentLibrariesTestCase(ContentLibrariesRestApiTest): """ General tests for Learning-Core-based Content Libraries @@ -62,26 +52,6 @@ class ContentLibrariesTestCase(ContentLibrariesRestApiTest, OpenEdxEventsTestMix library slug and bundle UUID does not because it's assumed to be immutable and cached forever. """ - ENABLED_OPENEDX_EVENTS = [ - CONTENT_LIBRARY_CREATED.event_type, - CONTENT_LIBRARY_DELETED.event_type, - CONTENT_LIBRARY_UPDATED.event_type, - LIBRARY_BLOCK_CREATED.event_type, - LIBRARY_BLOCK_DELETED.event_type, - LIBRARY_BLOCK_UPDATED.event_type, - ] - - @classmethod - def setUpClass(cls): - """ - Set up class method for the Test class. - - TODO: It's unclear why we need to call start_events_isolation ourselves rather than relying on - OpenEdxEventsTestMixin.setUpClass to handle it. It fails it we don't, and many other test cases do it, - so we're following a pattern here. But that pattern doesn't really make sense. - """ - super().setUpClass() - cls.start_events_isolation() def test_library_crud(self): """ @@ -792,45 +762,6 @@ def test_library_blocks_limit(self): # Second block should throw error self._add_block_to_library(lib_id, "problem", "problem1", expect_response=400) - def test_library_block_delete_event(self): - """ - Check that LIBRARY_BLOCK_DELETED event is sent when a content library is deleted. - """ - event_receiver = Mock() - LIBRARY_BLOCK_DELETED.connect(event_receiver) - lib = self._create_library( - slug="test_lib_block_event_delete", - title="Event Test Library", - description="Testing event in library" - ) - - lib_id = lib["id"] - library_key = LibraryLocatorV2.from_string(lib_id) - - block = self._add_block_to_library(lib_id, "problem", "problem1") - block_id = block['id'] - - usage_key = LibraryUsageLocatorV2( - lib_key=library_key, - block_type="problem", - usage_id="problem1" - ) - - self._delete_library_block(block_id) - - event_receiver.assert_called() - self.assertDictContainsSubset( - { - "signal": LIBRARY_BLOCK_DELETED, - "sender": None, - "library_block": LibraryBlockData( - library_key=library_key, - usage_key=usage_key - ), - }, - event_receiver.call_args.kwargs - ) - def test_library_paste_xblock(self): """ Check the a new block is created in the library after pasting from clipboard. From 33016450608ee3383ca1ef5b3a5838443fa086d0 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Tue, 6 May 2025 09:42:47 -0700 Subject: [PATCH 08/20] chore: address lint warnings --- openedx/core/djangoapps/content_libraries/api/libraries.py | 3 --- openedx/core/djangoapps/content_libraries/tasks.py | 2 +- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/openedx/core/djangoapps/content_libraries/api/libraries.py b/openedx/core/djangoapps/content_libraries/api/libraries.py index 3b2bc911e571..ff053c9a228b 100644 --- a/openedx/core/djangoapps/content_libraries/api/libraries.py +++ b/openedx/core/djangoapps/content_libraries/api/libraries.py @@ -653,9 +653,6 @@ def get_allowed_block_types(library_key: LibraryLocatorV2): # pylint: disable=u return info - - - def publish_changes(library_key: LibraryLocatorV2, user_id: int | None = None): """ Publish all pending changes to the specified library. diff --git a/openedx/core/djangoapps/content_libraries/tasks.py b/openedx/core/djangoapps/content_libraries/tasks.py index 1b5ad13e8793..1f1fc95c4b35 100644 --- a/openedx/core/djangoapps/content_libraries/tasks.py +++ b/openedx/core/djangoapps/content_libraries/tasks.py @@ -70,7 +70,7 @@ def send_events_after_publish(publish_log_pk: int, library_key_str: str) -> None """ Send events to trigger actions like updating the search index, after we've published some items in a library. - + We use the PublishLog record so we can detect exactly what was changed, including any auto-published changes like child items in containers. From a5105576ea18a6a11a4e56d89d7df5d4d55361c4 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Tue, 6 May 2025 09:45:57 -0700 Subject: [PATCH 09/20] fix: "[created] received a naive datetime" --- .../core/djangoapps/content_libraries/api/containers.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/openedx/core/djangoapps/content_libraries/api/containers.py b/openedx/core/djangoapps/content_libraries/api/containers.py index 7e8b8db9dddb..e997e35dbec4 100644 --- a/openedx/core/djangoapps/content_libraries/api/containers.py +++ b/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 @@ -244,7 +244,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 _: @@ -274,7 +274,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, ) @@ -420,7 +420,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, ) From a13bfaa18cb4180c40fc08c5d025b6846169045b Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Tue, 6 May 2025 10:52:18 -0700 Subject: [PATCH 10/20] fix: handle publishing a single component consistently --- .../content_libraries/api/blocks.py | 21 ++++--------------- 1 file changed, 4 insertions(+), 17 deletions(-) diff --git a/openedx/core/djangoapps/content_libraries/api/blocks.py b/openedx/core/djangoapps/content_libraries/api/blocks.py index f29b3d32753b..d949904868bd 100644 --- a/openedx/core/djangoapps/content_libraries/api/blocks.py +++ b/openedx/core/djangoapps/content_libraries/api/blocks.py @@ -65,6 +65,7 @@ ) 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. @@ -834,24 +835,10 @@ 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, - ) - ) + tasks.wait_for_post_publish_events(publish_log, library_key=library_key) def _component_exists(usage_key: UsageKeyV2) -> bool: From 19fca020b0b3bb4b76458fd26f6799f3f8628fa3 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Tue, 6 May 2025 10:53:29 -0700 Subject: [PATCH 11/20] feat: distinguish PUBLISHED events from other draft CRUD events --- .../djangoapps/content_libraries/tasks.py | 8 +- .../content_libraries/tests/test_events.py | 87 +++++++++++++++++-- 2 files changed, 84 insertions(+), 11 deletions(-) diff --git a/openedx/core/djangoapps/content_libraries/tasks.py b/openedx/core/djangoapps/content_libraries/tasks.py index 1f1fc95c4b35..1ab20cc0778a 100644 --- a/openedx/core/djangoapps/content_libraries/tasks.py +++ b/openedx/core/djangoapps/content_libraries/tasks.py @@ -42,8 +42,10 @@ LIBRARY_BLOCK_CREATED, LIBRARY_BLOCK_DELETED, LIBRARY_BLOCK_UPDATED, + LIBRARY_BLOCK_PUBLISHED, LIBRARY_COLLECTION_UPDATED, LIBRARY_CONTAINER_UPDATED, + LIBRARY_CONTAINER_PUBLISHED, ) from user_tasks.tasks import UserTask, UserTaskStatus @@ -88,12 +90,14 @@ def send_events_after_publish(publish_log_pk: int, library_key_str: str) -> None for record in affected_entities: if hasattr(record.entity, "component"): usage_key = api.library_component_usage_key(library_key, record.entity.component) - LIBRARY_BLOCK_UPDATED.send_event( + # Note that this item may be newly created, updated, or even deleted - but all we care about for this event + # is that the published version is now different. Only for draft changes do we send differentiated events. + LIBRARY_BLOCK_PUBLISHED.send_event( library_block=LibraryBlockData(library_key=library_key, usage_key=usage_key) ) elif hasattr(record.entity, "container"): container_key = api.library_container_locator(library_key, record.entity.container) - LIBRARY_CONTAINER_UPDATED.send_event( + LIBRARY_CONTAINER_PUBLISHED.send_event( library_container=LibraryContainerData(container_key=container_key) ) else: diff --git a/openedx/core/djangoapps/content_libraries/tests/test_events.py b/openedx/core/djangoapps/content_libraries/tests/test_events.py index 98a8f801b63c..a47ab7a97a43 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_events.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_events.py @@ -1,19 +1,26 @@ """ Tests for Learning-Core-based Content Libraries """ -import ddt -from opaque_keys.edx.locator import LibraryLocatorV2, LibraryUsageLocatorV2 -from openedx_events.content_authoring.data import ( +from opaque_keys.edx.locator import LibraryContainerLocator, LibraryLocatorV2, LibraryUsageLocatorV2 +from openedx_events.content_authoring.signals import ( ContentLibraryData, LibraryBlockData, -) -from openedx_events.content_authoring.signals import ( + LibraryCollectionData, + LibraryContainerData, CONTENT_LIBRARY_CREATED, CONTENT_LIBRARY_DELETED, CONTENT_LIBRARY_UPDATED, LIBRARY_BLOCK_CREATED, LIBRARY_BLOCK_DELETED, - LIBRARY_BLOCK_UPDATED + 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, ) from openedx_events.tests.utils import OpenEdxEventsTestMixin @@ -22,8 +29,7 @@ @skip_unless_cms -@ddt.ddt -class ContentLibrariesTestCase(ContentLibrariesRestApiTest, OpenEdxEventsTestMixin): +class ContentLibrariesEventsTestCase(ContentLibrariesRestApiTest, OpenEdxEventsTestMixin): """ General tests for Learning-Core-based Content Libraries @@ -53,6 +59,14 @@ class ContentLibrariesTestCase(ContentLibrariesRestApiTest, OpenEdxEventsTestMix 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, ] ENABLED_OPENEDX_EVENTS = [e.event_type for e in ALL_EVENTS] @@ -207,7 +221,7 @@ def test_library_block_update_and_publish_events(self): # Then publish the block: self._publish_library_block(usage_key) self.expect_new_events({ - "signal": LIBRARY_BLOCK_UPDATED, # FIXME: this should be a _PUBLISHED event + "signal": LIBRARY_BLOCK_PUBLISHED, "library_block": LibraryBlockData(self.lib1_key, usage_key), }) @@ -268,3 +282,58 @@ def test_revert_create(self): "signal": LIBRARY_BLOCK_DELETED, "library_block": LibraryBlockData(self.lib1_key, usage_key), }) + + ############################## Containers ################################## + + def test_publish_container(self): + """ + Test that we can publish the changes to a specific container + """ + # Create two containers and add some components + container1 = self._create_container(self.lib1_key, "unit", display_name="Alpha Unit", slug=None) + container2 = self._create_container(self.lib1_key, "unit", display_name="Bravo Unit", slug=None) + problem_block = self._add_block_to_library(self.lib1_key, "problem", "Problem1", can_stand_alone=False) + html_block = self._add_block_to_library(self.lib1_key, "html", "Html1", can_stand_alone=False) + html_block2 = self._add_block_to_library(self.lib1_key, "html", "Html2", can_stand_alone=False) + self._add_container_components(container1["id"], children_ids=[problem_block["id"], html_block["id"]]) + self._add_container_components(container2["id"], children_ids=[html_block["id"], html_block2["id"]]) + # At first everything is unpublished: + c1_before = self._get_container(container1["id"]) + assert c1_before["has_unpublished_changes"] + c2_before = self._get_container(container2["id"]) + assert c2_before["has_unpublished_changes"] + + # clear event log after the initial mock data setup is complete: + self.clear_events() + + # Now publish only Container 1 + self._publish_container(container1["id"]) + + # Now it is published: + c1_after = self._get_container(container1["id"]) + assert c1_after["has_unpublished_changes"] is False + # And publish events were emitted: + self.expect_new_events( + { # An event for container 1 being published: + "signal": LIBRARY_CONTAINER_PUBLISHED, + "library_container": LibraryContainerData( + container_key=LibraryContainerLocator.from_string(container1["id"]), + ), + }, + { # An event for the problem block in container 1: + "signal": LIBRARY_BLOCK_PUBLISHED, + "library_block": LibraryBlockData( + self.lib1_key, LibraryUsageLocatorV2.from_string(problem_block["id"]), + ), + }, + { # An event for the html block in container 1 (and container 2): + "signal": LIBRARY_BLOCK_PUBLISHED, + "library_block": LibraryBlockData( + self.lib1_key, LibraryUsageLocatorV2.from_string(html_block["id"]), + ), + }, + ) + + # and container 2 is still unpublished + c2_after = self._get_container(container2["id"]) + assert c2_after["has_unpublished_changes"] From c2d81ad615ada69026a9d6c4b6ee79b0db39145d Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Tue, 6 May 2025 11:37:57 -0700 Subject: [PATCH 12/20] fix: simplify how we send single-component publish events --- openedx/core/djangoapps/content_libraries/api/blocks.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/openedx/core/djangoapps/content_libraries/api/blocks.py b/openedx/core/djangoapps/content_libraries/api/blocks.py index d949904868bd..d693ff30d7e5 100644 --- a/openedx/core/djangoapps/content_libraries/api/blocks.py +++ b/openedx/core/djangoapps/content_libraries/api/blocks.py @@ -838,7 +838,10 @@ def publish_component_changes(usage_key: LibraryUsageLocatorV2, user: UserType): publish_log = authoring_api.publish_from_drafts( learning_package.id, draft_qset=drafts_to_publish, published_by=user.id, ) - tasks.wait_for_post_publish_events(publish_log, library_key=library_key) + # 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: From 1ed70443f001c0ea33955e4ae3a9e30a389e56c8 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Tue, 6 May 2025 11:38:20 -0700 Subject: [PATCH 13/20] test: more comprehensive tests for events --- .../tests/test_containers.py | 154 +----------------- .../content_libraries/tests/test_events.py | 97 +++++++++++ 2 files changed, 99 insertions(+), 152 deletions(-) diff --git a/openedx/core/djangoapps/content_libraries/tests/test_containers.py b/openedx/core/djangoapps/content_libraries/tests/test_containers.py index db6456a14a20..6c59c8c086e4 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_containers.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_containers.py @@ -2,20 +2,11 @@ Tests for Learning-Core-based Content Libraries """ from datetime import datetime, timezone -from unittest import mock import ddt from freezegun import freeze_time -from opaque_keys.edx.locator import LibraryContainerLocator, LibraryLocatorV2, LibraryUsageLocatorV2 -from openedx_events.content_authoring.data import LibraryContainerData -from openedx_events.content_authoring.signals import ( - LIBRARY_BLOCK_UPDATED, - LIBRARY_CONTAINER_CREATED, - LIBRARY_CONTAINER_DELETED, - LIBRARY_CONTAINER_UPDATED, -) -from openedx_events.tests.utils import OpenEdxEventsTestMixin +from opaque_keys.edx.locator import LibraryLocatorV2 from common.djangoapps.student.tests.factories import UserFactory from openedx.core.djangoapps.content_libraries import api @@ -25,7 +16,7 @@ @skip_unless_cms @ddt.ddt -class ContainersTestCase(OpenEdxEventsTestMixin, ContentLibrariesRestApiTest): +class ContainersTestCase(ContentLibrariesRestApiTest): """ Tests for containers (Sections, Subsections, Units) in Content Libraries. @@ -43,12 +34,6 @@ class ContainersTestCase(OpenEdxEventsTestMixin, ContentLibrariesRestApiTest): new fields to an API response, which are backwards compatible, won't break any tests, but backwards-incompatible API changes will. """ - ENABLED_OPENEDX_EVENTS = [ - LIBRARY_BLOCK_UPDATED.event_type, - LIBRARY_CONTAINER_CREATED.event_type, - LIBRARY_CONTAINER_DELETED.event_type, - LIBRARY_CONTAINER_UPDATED.event_type, - ] def test_unit_crud(self): """ @@ -57,15 +42,6 @@ def test_unit_crud(self): lib = self._create_library(slug="containers", title="Container Test Library", description="Units and more") lib_key = LibraryLocatorV2.from_string(lib["id"]) - create_receiver = mock.Mock() - LIBRARY_CONTAINER_CREATED.connect(create_receiver) - - update_receiver = mock.Mock() - LIBRARY_CONTAINER_UPDATED.connect(update_receiver) - - delete_receiver = mock.Mock() - LIBRARY_CONTAINER_DELETED.connect(delete_receiver) - # Create a unit: create_date = datetime(2024, 9, 8, 7, 6, 5, tzinfo=timezone.utc) with freeze_time(create_date): @@ -85,20 +61,6 @@ def test_unit_crud(self): } self.assertDictContainsEntries(container_data, expected_data) - assert create_receiver.call_count == 1 - container_key = LibraryContainerLocator.from_string( - "lct:CL-TEST:containers:unit:u1", - ) - self.assertDictContainsSubset( - { - "signal": LIBRARY_CONTAINER_CREATED, - "sender": None, - "library_container": LibraryContainerData( - container_key, - ), - }, - create_receiver.call_args_list[0].kwargs, - ) # Fetch the unit: unit_as_read = self._get_container(container_data["id"]) @@ -113,18 +75,6 @@ def test_unit_crud(self): expected_data['display_name'] = 'Unit ABC' self.assertDictContainsEntries(container_data, expected_data) - assert update_receiver.call_count == 1 - self.assertDictContainsSubset( - { - "signal": LIBRARY_CONTAINER_UPDATED, - "sender": None, - "library_container": LibraryContainerData( - container_key, - ), - }, - update_receiver.call_args_list[0].kwargs, - ) - # Re-fetch the unit unit_as_re_read = self._get_container(container_data["id"]) # make sure it contains the same data when we read it back: @@ -133,17 +83,6 @@ def test_unit_crud(self): # Delete the unit self._delete_container(container_data["id"]) self._get_container(container_data["id"], expect_response=404) - assert delete_receiver.call_count == 1 - self.assertDictContainsSubset( - { - "signal": LIBRARY_CONTAINER_DELETED, - "sender": None, - "library_container": LibraryContainerData( - container_key, - ), - }, - delete_receiver.call_args_list[0].kwargs, - ) def test_unit_permissions(self): """ @@ -186,8 +125,6 @@ def test_unit_add_children(self): """ Test that we can add and get unit children components """ - update_receiver = mock.Mock() - LIBRARY_CONTAINER_UPDATED.connect(update_receiver) lib = self._create_library(slug="containers", title="Container Test Library", description="Units and more") lib_key = LibraryLocatorV2.from_string(lib["id"]) @@ -212,18 +149,6 @@ def test_unit_add_children(self): container_data["id"], children_ids=[problem_block_2["id"], html_block_2["id"]] ) - self.assertDictContainsSubset( - { - "signal": LIBRARY_CONTAINER_UPDATED, - "sender": None, - "library_container": LibraryContainerData( - container_key=LibraryContainerLocator.from_string( - container_data["id"], - ), - ), - }, - update_receiver.call_args_list[0].kwargs, - ) data = self._get_container_components(container_data["id"]) # Verify total number of components to be 2 + 2 = 4 assert len(data) == 4 @@ -236,8 +161,6 @@ def test_unit_remove_children(self): """ Test that we can remove unit children components """ - update_receiver = mock.Mock() - LIBRARY_CONTAINER_UPDATED.connect(update_receiver) lib = self._create_library(slug="containers", title="Container Test Library", description="Units and more") lib_key = LibraryLocatorV2.from_string(lib["id"]) @@ -262,25 +185,11 @@ def test_unit_remove_children(self): assert len(data) == 2 assert data[0]['id'] == html_block['id'] assert data[1]['id'] == html_block_2['id'] - self.assertDictContainsSubset( - { - "signal": LIBRARY_CONTAINER_UPDATED, - "sender": None, - "library_container": LibraryContainerData( - container_key=LibraryContainerLocator.from_string( - container_data["id"], - ), - ), - }, - update_receiver.call_args_list[0].kwargs, - ) def test_unit_replace_children(self): """ Test that we can completely replace/reorder unit children components. """ - update_receiver = mock.Mock() - LIBRARY_CONTAINER_UPDATED.connect(update_receiver) lib = self._create_library(slug="containers", title="Container Test Library", description="Units and more") lib_key = LibraryLocatorV2.from_string(lib["id"]) @@ -324,18 +233,6 @@ def test_unit_replace_children(self): assert len(data) == 2 assert data[0]['id'] == new_problem_block['id'] assert data[1]['id'] == new_html_block['id'] - self.assertDictContainsSubset( - { - "signal": LIBRARY_CONTAINER_UPDATED, - "sender": None, - "library_container": LibraryContainerData( - container_key=LibraryContainerLocator.from_string( - container_data["id"], - ), - ), - }, - update_receiver.call_args_list[0].kwargs, - ) def test_restore_unit(self): """ @@ -352,9 +249,6 @@ def test_restore_unit(self): # Delete the unit self._delete_container(container_data["id"]) - create_receiver = mock.Mock() - LIBRARY_CONTAINER_CREATED.connect(create_receiver) - # Restore container self._restore_container(container_data["id"]) new_container_data = self._get_container(container_data["id"]) @@ -372,20 +266,6 @@ def test_restore_unit(self): 'collections': [], } - self.assertDictContainsEntries(new_container_data, expected_data) - - assert create_receiver.call_count == 1 - self.assertDictContainsSubset( - { - "signal": LIBRARY_CONTAINER_CREATED, - "sender": None, - "library_container": LibraryContainerData( - container_key=LibraryContainerLocator.from_string("lct:CL-TEST:containers:unit:u1"), - ), - }, - 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") @@ -444,12 +324,6 @@ def test_publish_container(self): # pylint: disable=too-many-statements c2_before = self._get_container(container2["id"]) assert c2_before["has_unpublished_changes"] - # Set up event receivers after the initial mock data setup is complete: - updated_container_receiver = mock.Mock() - updated_block_receiver = mock.Mock() - LIBRARY_CONTAINER_UPDATED.connect(updated_container_receiver) - LIBRARY_BLOCK_UPDATED.connect(updated_block_receiver) - # Now publish only Container 1 self._publish_container(container1["id"]) @@ -476,27 +350,3 @@ def test_publish_container(self): # pylint: disable=too-many-statements assert c2_components_after[1]["id"] == html_block2["id"] assert c2_components_after[1]["has_unpublished_changes"] # unaffected assert c2_components_after[1]["published_by"] is None - - # Make sure that the right events were sent out. - # First, there should be one container updated event: - assert len(updated_container_receiver.call_args_list) == 1 - self.assertDictContainsSubset( - { - "signal": LIBRARY_CONTAINER_UPDATED, - "library_container": LibraryContainerData( - container_key=LibraryContainerLocator.from_string(container1["id"]), - ), - }, - updated_container_receiver.call_args_list[0].kwargs, - ) - - # Second, two XBlock updated events: - assert len(updated_block_receiver.call_args_list) == 2 - updated_block_ids = set( - call.kwargs["library_block"].usage_key for call in updated_block_receiver.call_args_list - ) - assert updated_block_ids == { - LibraryUsageLocatorV2.from_string(problem_block["id"]), - LibraryUsageLocatorV2.from_string(html_block["id"]), - } - assert all(call.kwargs["signal"] == LIBRARY_BLOCK_UPDATED for call in updated_block_receiver.call_args_list) diff --git a/openedx/core/djangoapps/content_libraries/tests/test_events.py b/openedx/core/djangoapps/content_libraries/tests/test_events.py index a47ab7a97a43..b82eb4b6fef9 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_events.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_events.py @@ -284,6 +284,34 @@ def test_revert_create(self): }) ############################## Containers ################################## + def test_unit_crud(self): + """ + Test Create, Read, Update, and Delete of a Unit + """ + # Create a unit: + container_data = self._create_container(self.lib1_key, "unit", slug="u1", display_name="Test Unit") + container_key = LibraryContainerLocator.from_string(container_data["id"]) + + self.expect_new_events({ + "signal": LIBRARY_CONTAINER_CREATED, + "library_container": LibraryContainerData(container_key), + }) + + # Update the unit: + self._update_container(container_key, display_name="Unit ABC") + + self.expect_new_events({ + "signal": LIBRARY_CONTAINER_UPDATED, + "library_container": LibraryContainerData(container_key), + }) + + # Delete the unit + self._delete_container(container_key) + self._get_container(container_key, expect_response=404) + self.expect_new_events({ + "signal": LIBRARY_CONTAINER_DELETED, + "library_container": LibraryContainerData(container_key), + }) def test_publish_container(self): """ @@ -337,3 +365,72 @@ def test_publish_container(self): # and container 2 is still unpublished c2_after = self._get_container(container2["id"]) assert c2_after["has_unpublished_changes"] + + def test_restore_unit(self): + """ + Test restoring a deleted unit via the "restore" API. + """ + # Create a unit: + container_data = self._create_container(self.lib1_key, "unit", slug="u1", display_name="Test Unit") + container_key=LibraryContainerLocator.from_string(container_data["id"]) + + self.expect_new_events({ + "signal": LIBRARY_CONTAINER_CREATED, + "library_container": LibraryContainerData(container_key), + }) + + # Delete the unit + self._delete_container(container_data["id"]) + + self.expect_new_events({ + "signal": LIBRARY_CONTAINER_DELETED, + "library_container": LibraryContainerData(container_key), + }) + + # Restore the unit + self._restore_container(container_data["id"]) + + self.expect_new_events({ + "signal": LIBRARY_CONTAINER_CREATED, + "library_container": LibraryContainerData(container_key), + }) + + def test_restore_unit_via_revert(self): + """ + Test restoring a deleted unit by reverting changes. + """ + # Publish the existing setup and clear events + self._commit_library_changes(self.lib1_key) + self.clear_events() + + # Create a unit: + container_data = self._create_container(self.lib1_key, "unit", slug="u1", display_name="Test Unit") + container_key=LibraryContainerLocator.from_string(container_data["id"]) + + self.expect_new_events({ + "signal": LIBRARY_CONTAINER_CREATED, + "library_container": LibraryContainerData(container_key), + }) + + # Publish changes + self._publish_container(container_key) + self.expect_new_events({ + "signal": LIBRARY_CONTAINER_PUBLISHED, + "library_container": LibraryContainerData(container_key), + }) + + # Delete the unit + self._delete_container(container_data["id"]) + + self.expect_new_events({ + "signal": LIBRARY_CONTAINER_DELETED, + "library_container": LibraryContainerData(container_key), + }) + + # Revert changes, which will re-create the unit: + self._restore_container(container_data["id"]) + + self.expect_new_events({ + "signal": LIBRARY_CONTAINER_CREATED, + "library_container": LibraryContainerData(container_key), + }) From cc1ae1ea7bdd3f1987b3445e3848c3a882588614 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Tue, 6 May 2025 11:50:52 -0700 Subject: [PATCH 14/20] test: more comprehensive tests for events --- .../djangoapps/content_libraries/tasks.py | 29 ++++++++++++++++--- .../content_libraries/tests/test_events.py | 2 +- 2 files changed, 26 insertions(+), 5 deletions(-) diff --git a/openedx/core/djangoapps/content_libraries/tasks.py b/openedx/core/djangoapps/content_libraries/tasks.py index 1ab20cc0778a..6a151acba568 100644 --- a/openedx/core/djangoapps/content_libraries/tasks.py +++ b/openedx/core/djangoapps/content_libraries/tasks.py @@ -44,6 +44,8 @@ LIBRARY_BLOCK_UPDATED, LIBRARY_BLOCK_PUBLISHED, LIBRARY_COLLECTION_UPDATED, + LIBRARY_CONTAINER_CREATED, + LIBRARY_CONTAINER_DELETED, LIBRARY_CONTAINER_UPDATED, LIBRARY_CONTAINER_PUBLISHED, ) @@ -149,7 +151,9 @@ def send_events_after_revert(draft_change_log_id: int, library_key_str: str) -> "entity", "entity__container", "entity__component", ).all() - affected_container_keys: set[LibraryContainerLocator] = set() + created_container_keys: set[LibraryContainerLocator] = set() + updated_container_keys: set[LibraryContainerLocator] = set() + deleted_container_keys: set[LibraryContainerLocator] = set() affected_collection_keys: set[LibraryCollectionLocator] = set() # Update anything that needs to be updated (e.g. search index): @@ -170,13 +174,18 @@ def send_events_after_revert(draft_change_log_id: int, library_key_str: str) -> # e.g. if this was a newly created component in the container and is now deleted, or this was deleted and # is now restored. for parent_container in api.get_containers_contains_component(usage_key): - affected_container_keys.add(parent_container.container_key) + updated_container_keys.add(parent_container.container_key) # TODO: do we also need to send CONTENT_OBJECT_ASSOCIATIONS_CHANGED for this component, or is # LIBRARY_BLOCK_UPDATED sufficient? elif hasattr(record.entity, "container"): container_key = api.library_container_locator(library_key, record.entity.container) - affected_container_keys.add(container_key) + if is_deleted: + deleted_container_keys.add(container_key) + elif is_undeleted: + created_container_keys.add(container_key) + else: + updated_container_keys.add(container_key) else: log.warning( f"PublishableEntity {record.entity.pk} / {record.entity.key} was modified during publish operation " @@ -193,7 +202,19 @@ def send_events_after_revert(draft_change_log_id: int, library_key_str: str) -> ) affected_collection_keys.add(collection_key) - for container_key in affected_container_keys: + for container_key in deleted_container_keys: + LIBRARY_CONTAINER_DELETED.send_event( + library_container=LibraryContainerData(container_key=container_key) + ) + # Don't bother sending UPDATED events for these containers that are now deleted + created_container_keys.remove(container_key) + + for container_key in created_container_keys: + LIBRARY_CONTAINER_CREATED.send_event( + library_container=LibraryContainerData(container_key=container_key) + ) + + for container_key in updated_container_keys: LIBRARY_CONTAINER_UPDATED.send_event( library_container=LibraryContainerData(container_key=container_key) ) diff --git a/openedx/core/djangoapps/content_libraries/tests/test_events.py b/openedx/core/djangoapps/content_libraries/tests/test_events.py index b82eb4b6fef9..c5d2535b891a 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_events.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_events.py @@ -428,7 +428,7 @@ def test_restore_unit_via_revert(self): }) # Revert changes, which will re-create the unit: - self._restore_container(container_data["id"]) + self._revert_library_changes(self.lib1_key) self.expect_new_events({ "signal": LIBRARY_CONTAINER_CREATED, From 53fbc05707916ac70511114f262c483c8dd5e1e9 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Tue, 6 May 2025 12:09:06 -0700 Subject: [PATCH 15/20] feat: update search handlers for new PUBLISHED events --- .../djangoapps/content/search/handlers.py | 71 ++++++++++++++++--- 1 file changed, 60 insertions(+), 11 deletions(-) diff --git a/openedx/core/djangoapps/content/search/handlers.py b/openedx/core/djangoapps/content/search/handlers.py index 6ecad513e258..315d3cde53fd 100644 --- a/openedx/core/djangoapps/content/search/handlers.py +++ b/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: @@ -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. From 47ea4cd3634c3e8acb8999029878d3492a173da0 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Tue, 6 May 2025 13:01:06 -0700 Subject: [PATCH 16/20] temp: bump version of openedx-events --- requirements/edx/base.txt | 2 +- requirements/edx/development.txt | 2 +- requirements/edx/doc.txt | 2 +- requirements/edx/testing.txt | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index 67b48030fbd6..a24aff8a5134 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -801,7 +801,7 @@ openedx-django-require==2.1.0 # via -r requirements/edx/kernel.in openedx-django-wiki==2.1.0 # via -r requirements/edx/kernel.in -openedx-events==10.0.0 +openedx-events @ git+https://github.com/open-craft/openedx-events.git@braden/clarify-library-events # via # -r requirements/edx/kernel.in # edx-enterprise diff --git a/requirements/edx/development.txt b/requirements/edx/development.txt index 53b3811e60ee..a4522e4d6c88 100644 --- a/requirements/edx/development.txt +++ b/requirements/edx/development.txt @@ -1358,7 +1358,7 @@ openedx-django-wiki==2.1.0 # via # -r requirements/edx/doc.txt # -r requirements/edx/testing.txt -openedx-events==10.0.0 +openedx-events @ git+https://github.com/open-craft/openedx-events.git@braden/clarify-library-events # via # -r requirements/edx/doc.txt # -r requirements/edx/testing.txt diff --git a/requirements/edx/doc.txt b/requirements/edx/doc.txt index d22f9a6ede8e..f4773b010dd3 100644 --- a/requirements/edx/doc.txt +++ b/requirements/edx/doc.txt @@ -972,7 +972,7 @@ openedx-django-require==2.1.0 # via -r requirements/edx/base.txt openedx-django-wiki==2.1.0 # via -r requirements/edx/base.txt -openedx-events==10.0.0 +openedx-events @ git+https://github.com/open-craft/openedx-events.git@braden/clarify-library-events # via # -r requirements/edx/base.txt # edx-enterprise diff --git a/requirements/edx/testing.txt b/requirements/edx/testing.txt index 7abfb24b9ae1..bf337ef93913 100644 --- a/requirements/edx/testing.txt +++ b/requirements/edx/testing.txt @@ -1030,7 +1030,7 @@ openedx-django-require==2.1.0 # via -r requirements/edx/base.txt openedx-django-wiki==2.1.0 # via -r requirements/edx/base.txt -openedx-events==10.0.0 +openedx-events @ git+https://github.com/open-craft/openedx-events.git@braden/clarify-library-events # via # -r requirements/edx/base.txt # edx-enterprise From 345ce524429886e1a30f00823930418a303a86b7 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Tue, 6 May 2025 13:04:44 -0700 Subject: [PATCH 17/20] test: remove unneeded call to enable events isolation --- .../content_libraries/tests/test_events.py | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-) diff --git a/openedx/core/djangoapps/content_libraries/tests/test_events.py b/openedx/core/djangoapps/content_libraries/tests/test_events.py index c5d2535b891a..21fe37b12c42 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_events.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_events.py @@ -70,18 +70,6 @@ class ContentLibrariesEventsTestCase(ContentLibrariesRestApiTest, OpenEdxEventsT ] ENABLED_OPENEDX_EVENTS = [e.event_type for e in ALL_EVENTS] - @classmethod - def setUpClass(cls): - """ - Set up class method for the Test class. - - TODO: It's unclear why we need to call start_events_isolation ourselves rather than relying on - OpenEdxEventsTestMixin.setUpClass to handle it. It fails it we don't, and many other test cases do it, - so we're following a pattern here. But that pattern doesn't really make sense. - """ - super().setUpClass() - cls.start_events_isolation() - def setUp(self): super().setUp() @@ -131,6 +119,8 @@ def expect_new_events(self, *expected_events: list[dict]): raise AssertionError(f"Events were emitted but not expected: {self.new_events}") self.clear_events() + ############################## Libraries ################################## + def test_content_library_crud_events(self): """ Check that CONTENT_LIBRARY_CREATED event is sent when a content library is created, updated, and deleted @@ -168,6 +158,8 @@ def test_content_library_crud_events(self): # Should deleting a library send out _DELETED events for all the items in the library too? + ############################## Components (XBlocks) ################################## + def test_library_block_create_event(self): """ Check that LIBRARY_BLOCK_CREATED event is sent when a library block is created. @@ -284,6 +276,7 @@ def test_revert_create(self): }) ############################## Containers ################################## + def test_unit_crud(self): """ Test Create, Read, Update, and Delete of a Unit From b69c59e41cab867e9a39e27279dea0e064512405 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Tue, 6 May 2025 14:27:47 -0700 Subject: [PATCH 18/20] test: lol, belay that order --- .../content_libraries/tests/test_events.py | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/openedx/core/djangoapps/content_libraries/tests/test_events.py b/openedx/core/djangoapps/content_libraries/tests/test_events.py index 21fe37b12c42..b1f7159ed28f 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_events.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_events.py @@ -70,6 +70,19 @@ class ContentLibrariesEventsTestCase(ContentLibrariesRestApiTest, OpenEdxEventsT ] ENABLED_OPENEDX_EVENTS = [e.event_type for e in ALL_EVENTS] + @classmethod + def setUpClass(cls): + """ + Set up class method for the Test class. + + TODO: It's unclear why we need to call start_events_isolation ourselves rather than relying on + OpenEdxEventsTestMixin.setUpClass to handle it. It fails in a *flaky/inconsistent fasion* if we don't, + and many other test cases do it, so we're following a pattern here. + But that pattern doesn't really make sense. + """ + super().setUpClass() + cls.start_events_isolation() + def setUp(self): super().setUp() From 701fd79b49bcb7f097eacd52ff8ba30a403a5895 Mon Sep 17 00:00:00 2001 From: XnpioChV Date: Tue, 6 May 2025 18:58:06 -0500 Subject: [PATCH 19/20] feat: Add ready_to_sync field to ContainerLink model --- .../0011_containerlink_ready_to_sync.py | 18 +++++++++++++ cms/djangoapps/contentstore/models.py | 27 ++++++++++--------- 2 files changed, 32 insertions(+), 13 deletions(-) create mode 100644 cms/djangoapps/contentstore/migrations/0011_containerlink_ready_to_sync.py diff --git a/cms/djangoapps/contentstore/migrations/0011_containerlink_ready_to_sync.py b/cms/djangoapps/contentstore/migrations/0011_containerlink_ready_to_sync.py new file mode 100644 index 000000000000..8a9cf04faae1 --- /dev/null +++ b/cms/djangoapps/contentstore/migrations/0011_containerlink_ready_to_sync.py @@ -0,0 +1,18 @@ +# Generated by Django 4.2.20 on 2025-05-06 23:50 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('contentstore', '0010_container_link_models'), + ] + + operations = [ + migrations.AddField( + model_name='containerlink', + name='ready_to_sync', + field=models.BooleanField(default=False, help_text='True if the downstream has an available sync from upstream. It changes to True when the upstream is published, and it changes to False when the sync is declined/accepted. The version number is not used to check this value because containers do not change version if a component is edited/deleted within them. '), + ), + ] diff --git a/cms/djangoapps/contentstore/models.py b/cms/djangoapps/contentstore/models.py index 9782a37057c1..af69a7e5f5e4 100644 --- a/cms/djangoapps/contentstore/models.py +++ b/cms/djangoapps/contentstore/models.py @@ -272,6 +272,16 @@ class ContainerLink(EntityLinkBase): "or were deleted." ) ) + ready_to_sync = models.BooleanField( + default=False, + help_text=_( + "True if the downstream has an available sync from upstream. " + "It changes to True when the upstream is published, and it changes to False " + "when the sync is declined/accepted. " + "The version number is not used to check this value because containers do not change " + "version if a component is edited/deleted within them. " + ), + ) class Meta: verbose_name = _("Container Link") @@ -303,24 +313,15 @@ def filter_links( """ Get all links along with sync flag, upstream context title and version, with optional filtering. """ - ready_to_sync = link_filter.pop('ready_to_sync', None) result = cls.objects.filter(**link_filter).select_related( "upstream_container__publishable_entity__published__version", "upstream_container__publishable_entity__learning_package" - ).annotate( - ready_to_sync=( - GreaterThan( - Coalesce("upstream_container__publishable_entity__published__version__version_num", 0), - Coalesce("version_synced", 0) - ) & GreaterThan( - Coalesce("upstream_container__publishable_entity__published__version__version_num", 0), - Coalesce("version_declined", 0) - ) - ) ) - if ready_to_sync is not None: - result = result.filter(ready_to_sync=ready_to_sync) return result + + @classmethod + def get_link(cls, downstream_key) -> "EntityLinkBase": + return cls.objects.get(downstream_usage_key=downstream_key) @classmethod def summarize_by_downstream_context(cls, downstream_context_key: CourseKey) -> QuerySet: From ee01108af37b5158f7c255acf07c4144308da322 Mon Sep 17 00:00:00 2001 From: XnpioChV Date: Wed, 7 May 2025 20:37:52 -0500 Subject: [PATCH 20/20] feat: Create handler to update ready_to_sync in ContainerLink --- cms/djangoapps/contentstore/models.py | 12 ++++--- .../contentstore/signals/handlers.py | 34 +++++++++++++++++-- .../xblock_storage_handlers/view_handlers.py | 9 +++++ cms/lib/xblock/upstream_sync.py | 19 +++++++++-- 4 files changed, 64 insertions(+), 10 deletions(-) diff --git a/cms/djangoapps/contentstore/models.py b/cms/djangoapps/contentstore/models.py index af69a7e5f5e4..131581500a7f 100644 --- a/cms/djangoapps/contentstore/models.py +++ b/cms/djangoapps/contentstore/models.py @@ -357,12 +357,13 @@ def update_or_create( cls, upstream_container: Container | None, /, - upstream_container_key: LibraryContainerLocator, - upstream_context_key: str, downstream_usage_key: UsageKey, - downstream_context_key: CourseKey, - version_synced: int, + upstream_container_key: LibraryContainerLocator | None = None, + upstream_context_key: str | None = None, + downstream_context_key: CourseKey | None = None, + version_synced: int | None = None, version_declined: int | None = None, + ready_to_sync: bool | None = None, created: datetime | None = None, ) -> "ContainerLink": """ @@ -377,6 +378,7 @@ def update_or_create( 'downstream_context_key': downstream_context_key, 'version_synced': version_synced, 'version_declined': version_declined, + 'ready_to_sync': ready_to_sync, } if upstream_container: new_values['upstream_container'] = upstream_container @@ -385,7 +387,7 @@ def update_or_create( has_changes = False for key, new_value in new_values.items(): prev_value = getattr(link, key) - if prev_value != new_value: + if new_value is not None and prev_value != new_value: has_changes = True setattr(link, key, new_value) if has_changes: diff --git a/cms/djangoapps/contentstore/signals/handlers.py b/cms/djangoapps/contentstore/signals/handlers.py index b8be6ee84f8b..8bf14a873d2f 100644 --- a/cms/djangoapps/contentstore/signals/handlers.py +++ b/cms/djangoapps/contentstore/signals/handlers.py @@ -11,18 +11,20 @@ from django.db import transaction from django.dispatch import receiver from edx_toggles.toggles import SettingToggle -from opaque_keys.edx.keys import CourseKey +from opaque_keys.edx.keys import CourseKey, UsageKey from openedx_events.content_authoring.data import ( CourseCatalogData, CourseData, CourseScheduleData, LibraryBlockData, XBlockData, + LibraryContainerData, ) from openedx_events.content_authoring.signals import ( COURSE_CATALOG_INFO_CHANGED, COURSE_IMPORT_COMPLETED, LIBRARY_BLOCK_DELETED, + LIBRARY_CONTAINER_PUBLISHED, XBLOCK_CREATED, XBLOCK_DELETED, XBLOCK_UPDATED, @@ -40,6 +42,7 @@ from openedx.core.djangoapps.content.learning_sequences.api import key_supports_outlines from openedx.core.djangoapps.discussions.tasks import update_discussions_settings_from_course_task from openedx.core.lib.gating import api as gating_api +from openedx.core.djangoapps.content_libraries import api as lib_api from xmodule.modulestore import ModuleStoreEnum from xmodule.modulestore.django import SignalHandler, modulestore from xmodule.modulestore.exceptions import ItemNotFoundError @@ -264,7 +267,6 @@ def create_or_update_upstream_downstream_link_handler(**kwargs): if not xblock_info or not isinstance(xblock_info, XBlockData): log.error("Received null or incorrect data for event") return - handle_create_or_update_xblock_upstream_link.delay(str(xblock_info.usage_key)) @@ -314,3 +316,31 @@ def unlink_upstream_block_handler(**kwargs): return handle_unlink_upstream_block.delay(str(library_block.usage_key)) + + +@receiver(LIBRARY_CONTAINER_PUBLISHED) +def library_container_published_handler(**kwargs) -> None: + """ + Handle publish a container. Mark all related links to ready to sync. + """ + 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 + + # Get all container links that has the published container as the upstream + link_filter: dict[str, CourseKey | UsageKey | bool] = {} + link_filter["upstream_container_key"] = library_container.container_key + links = ContainerLink.filter_links(**link_filter) + + # Mark all links to ready to sync + links.update(ready_to_sync=True) diff --git a/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py b/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py index 31a17466769d..51f7a513b1db 100644 --- a/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py +++ b/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py @@ -36,6 +36,7 @@ from cms.djangoapps.contentstore.config.waffle import SHOW_REVIEW_RULES_FLAG from cms.djangoapps.contentstore.helpers import StaticFileNotices +from cms.djangoapps.contentstore.models import ContainerLink from cms.djangoapps.models.settings.course_grading import CourseGradingModel from cms.lib.ai_aside_summary_config import AiAsideSummaryConfig from cms.lib.xblock.upstream_sync import BadUpstream, UpstreamLink @@ -577,6 +578,14 @@ def sync_library_content(downstream: XBlock, request, store) -> StaticFileNotice store.delete_item(child.usage_key, user_id=request.user.id) downstream.children = children store.update_item(downstream, request.user.id) + # Mark the container link as not ready to sync (synchronized) + # The other data is updated with the XBLOCK_UPDATED signal, + # see: ../signals/handlers.py + ContainerLink.update_or_create( + None, + ready_to_sync=False, + downstream_usage_key=downstream.usage_key + ) static_file_notices = concat_static_file_notices(notices) return static_file_notices diff --git a/cms/lib/xblock/upstream_sync.py b/cms/lib/xblock/upstream_sync.py index 805019f5850d..1b5685ed4b12 100644 --- a/cms/lib/xblock/upstream_sync.py +++ b/cms/lib/xblock/upstream_sync.py @@ -80,6 +80,7 @@ class UpstreamLink: version_synced: int | None # Version of the upstream to which the downstream was last synced. version_available: int | None # Latest version of the upstream that's available, or None if it couldn't be loaded. version_declined: int | None # Latest version which the user has declined to sync with, if any. + force_ready_to_sync: bool # Bypass the verification of versions to be ready to sync. This is used by containers. error_message: str | None # If link is valid, None. Otherwise, a localized, human-friendly error message. @property @@ -89,9 +90,11 @@ def ready_to_sync(self) -> bool: """ return bool( self.upstream_ref and - self.version_available and - self.version_available > (self.version_synced or 0) and - self.version_available > (self.version_declined or 0) + self.force_ready_to_sync or ( + self.version_available and + self.version_available > (self.version_synced or 0) and + self.version_available > (self.version_declined or 0) + ) ) @property @@ -141,6 +144,7 @@ def try_get_for_block(cls, downstream: XBlock, log_error: bool = True) -> t.Self version_synced=getattr(downstream, "upstream_version", None), version_available=None, version_declined=None, + force_ready_to_sync=False, error_message=str(exc), ) @@ -159,6 +163,7 @@ def get_for_block(cls, downstream: XBlock) -> t.Self: """ # We import this here b/c UpstreamSyncMixin is used by cms/envs, which loads before the djangoapps are ready. from openedx.core.djangoapps.content_libraries import api as lib_api + from cms.djangoapps.contentstore.models import ContainerLink if not isinstance(downstream, UpstreamSyncMixin): raise BadDownstream(_("Downstream is not an XBlock or is missing required UpstreamSyncMixin")) @@ -177,6 +182,7 @@ def get_for_block(cls, downstream: XBlock) -> t.Self: except InvalidKeyError as exc: raise BadUpstream(_("Reference to linked library item is malformed")) from exc + force_ready_to_sync = False if isinstance(upstream_key, LibraryUsageLocatorV2): # The upstream is an XBlock if downstream.has_children: @@ -196,6 +202,12 @@ def get_for_block(cls, downstream: XBlock) -> t.Self: container_meta = lib_api.get_container(upstream_key) except lib_api.ContentLibraryContainerNotFound as exc: raise BadUpstream(_("Linked upstream library container was not found in the system")) from exc + try: + link = ContainerLink.get_link(downstream.usage_key) + except ContainerLink.DoesNotExist as exec: + raise BadUpstream(_("Container link was not found in the system")) from exec + + force_ready_to_sync = link.ready_to_sync expected_downstream_block_type = container_meta.container_type.olx_tag version_available = container_meta.published_version_num @@ -220,6 +232,7 @@ def get_for_block(cls, downstream: XBlock) -> t.Self: version_synced=downstream.upstream_version, version_available=version_available, version_declined=downstream.upstream_version_declined, + force_ready_to_sync=force_ready_to_sync, error_message=None, )