diff --git a/cms/djangoapps/contentstore/views/tests/test_clipboard_paste.py b/cms/djangoapps/contentstore/views/tests/test_clipboard_paste.py index 1430bbc4dc65..8bed7e6d1031 100644 --- a/cms/djangoapps/contentstore/views/tests/test_clipboard_paste.py +++ b/cms/djangoapps/contentstore/views/tests/test_clipboard_paste.py @@ -5,6 +5,7 @@ """ import ddt from opaque_keys.edx.keys import UsageKey +from openedx_content.api import signals as content_signals from openedx_events.content_authoring.signals import ( LIBRARY_BLOCK_DELETED, XBLOCK_CREATED, @@ -405,6 +406,7 @@ class ClipboardPasteFromV2LibraryTestCase(OpenEdxEventsTestMixin, ImmediateOnCom Test Clipboard Paste functionality with a "new" (as of Sumac) library """ ENABLED_OPENEDX_EVENTS = [ + content_signals.ENTITIES_DRAFT_CHANGED.event_type, # Required for library events to work LIBRARY_BLOCK_DELETED.event_type, XBLOCK_CREATED.event_type, XBLOCK_DELETED.event_type, @@ -491,7 +493,8 @@ def test_paste_from_library_read_only_tags(self): assert object_tag.is_copied # If we delete the upstream library block... - library_api.delete_library_block(self.lib_block_key) + with self.captureOnCommitCallbacks(execute=True): # make event handlers fire now, within TestCase transaction + library_api.delete_library_block(self.lib_block_key) # ...the copied tags remain, but should no longer be marked as "copied" object_tags = tagging_api.get_object_tags(new_block_key) diff --git a/cms/djangoapps/modulestore_migrator/tasks.py b/cms/djangoapps/modulestore_migrator/tasks.py index 15b25524b871..1c4736a3d76a 100644 --- a/cms/djangoapps/modulestore_migrator/tasks.py +++ b/cms/djangoapps/modulestore_migrator/tasks.py @@ -895,13 +895,9 @@ def _migrate_container( ).publishable_entity_version # Publish the container - # Call post publish events synchronously to avoid - # an error when calling `wait_for_post_publish_events` - # inside a celery task. libraries_api.publish_container_changes( container.container_key, context.created_by, - call_post_publish_events_sync=True, ) context.used_container_slugs.add(container.container_key.container_id) return container_publishable_entity_version, None diff --git a/openedx/core/djangoapps/content/search/tests/test_api.py b/openedx/core/djangoapps/content/search/tests/test_api.py index afa847748e89..7ffa5d756d02 100644 --- a/openedx/core/djangoapps/content/search/tests/test_api.py +++ b/openedx/core/djangoapps/content/search/tests/test_api.py @@ -901,7 +901,9 @@ def test_delete_collection(self, mock_meilisearch) -> None: """ # Add a component to the collection updated_date = datetime(2023, 6, 7, 8, 9, 10, tzinfo=timezone.utc) # noqa: UP017 - with freeze_time(updated_date): + # Note: because TestCase keeps the transaction open, we need self.captureOnCommitCallbacks(execute=True) to + # ensure events get emitted here as if this part were its own transaction as it normally would be. + with freeze_time(updated_date), self.captureOnCommitCallbacks(execute=True): library_api.update_library_collection_items( self.library.key, collection_key=self.collection.collection_code, @@ -942,10 +944,11 @@ def test_delete_collection(self, mock_meilisearch) -> None: mock_meilisearch.return_value.index.reset_mock() # Soft-delete the collection - content_api.delete_collection( - self.collection.learning_package_id, - self.collection.collection_code, - ) + with self.captureOnCommitCallbacks(execute=True): + content_api.delete_collection( + self.collection.learning_package_id, + self.collection.collection_code, + ) doc_problem_without_collection = { "id": self.doc_problem1["id"], @@ -976,7 +979,7 @@ def test_delete_collection(self, mock_meilisearch) -> None: # Restore the collection restored_date = datetime(2023, 8, 9, 10, 11, 12, tzinfo=timezone.utc) # noqa: UP017 - with freeze_time(restored_date): + with freeze_time(restored_date), self.captureOnCommitCallbacks(execute=True): content_api.restore_collection( self.collection.learning_package_id, self.collection.collection_code, @@ -1055,7 +1058,10 @@ def clear_contents(data: dict): 'sections': {'display_name': [], 'key': []}, }])) - library_api.delete_container(container.container_key) + # Note: because TestCase keeps the transaction open, we need self.captureOnCommitCallbacks(execute=True) to + # ensure events get emitted here as if this part were its own transaction as it normally would be. + with self.captureOnCommitCallbacks(execute=True): + library_api.delete_container(container.container_key) mock_meilisearch.return_value.index.return_value.delete_document.assert_called_once_with( container_dict["id"], diff --git a/openedx/core/djangoapps/content/search/tests/test_handlers.py b/openedx/core/djangoapps/content/search/tests/test_handlers.py index bb3654942b64..6ee28a658ed3 100644 --- a/openedx/core/djangoapps/content/search/tests/test_handlers.py +++ b/openedx/core/djangoapps/content/search/tests/test_handlers.py @@ -13,7 +13,6 @@ from openedx.core.djangolib.testing.utils import skip_unless_cms from xmodule.modulestore.tests.django_utils import ( TEST_DATA_SPLIT_MODULESTORE, - ImmediateOnCommitMixin, ModuleStoreTestCase, ) @@ -29,7 +28,7 @@ @patch("openedx.core.djangoapps.content.search.api.MeilisearchClient") @override_settings(MEILISEARCH_ENABLED=True) @skip_unless_cms -class TestUpdateIndexHandlers(ImmediateOnCommitMixin, ModuleStoreTestCase, LiveServerTestCase): +class TestUpdateIndexHandlers(ModuleStoreTestCase, LiveServerTestCase): """ Test that the search index is updated when XBlocks and Library Blocks are modified """ @@ -63,7 +62,9 @@ def test_create_delete_xblock(self, meilisearch_client): # Create XBlocks created_date = datetime(2023, 4, 5, 6, 7, 8, tzinfo=timezone.utc) # noqa: UP017 - with freeze_time(created_date): + # Note: because TestCase keeps the transaction open, we need self.captureOnCommitCallbacks(execute=True) to + # ensure events get emitted here as if this part were its own transaction as it normally would be. + with freeze_time(created_date), self.captureOnCommitCallbacks(execute=True): sequential = self.store.create_child(self.user_id, course.location, "sequential", "test_sequential") doc_sequential = { "id": "block-v1orgatest_coursetest_runtypesequentialblocktest_sequential-0cdb9395", @@ -86,7 +87,7 @@ def test_create_delete_xblock(self, meilisearch_client): meilisearch_client.return_value.index.return_value.update_documents.assert_called_with([doc_sequential]) - with freeze_time(created_date): + with freeze_time(created_date), self.captureOnCommitCallbacks(execute=True): vertical = self.store.create_child(self.user_id, sequential.location, "vertical", "test_vertical") doc_vertical = { "id": "block-v1orgatest_coursetest_runtypeverticalblocktest_vertical-011f143b", @@ -117,7 +118,7 @@ def test_create_delete_xblock(self, meilisearch_client): sequential = self.store.get_item(sequential.location, self.user_id) # Refresh the XBlock sequential.display_name = "Updated Sequential" modified_date = datetime(2024, 5, 6, 7, 8, 9, tzinfo=timezone.utc) # noqa: UP017 - with freeze_time(modified_date): + with freeze_time(modified_date), self.captureOnCommitCallbacks(execute=True): self.store.update_item(sequential, self.user_id) # The display name and the child's breadcrumbs should be updated @@ -131,7 +132,8 @@ def test_create_delete_xblock(self, meilisearch_client): ]) # Delete the XBlock - self.store.delete_item(vertical.location, self.user_id) + with self.captureOnCommitCallbacks(execute=True): + self.store.delete_item(vertical.location, self.user_id) meilisearch_client.return_value.index.return_value.delete_document.assert_called_with( "block-v1orgatest_coursetest_runtypeverticalblocktest_vertical-011f143b" @@ -166,8 +168,12 @@ def test_create_delete_library_block(self, meilisearch_client): # Populate it with a problem, freezing the date so we can verify created date serializes correctly. created_date = datetime(2023, 4, 5, 6, 7, 8, tzinfo=timezone.utc) # noqa: UP017 - with freeze_time(created_date): + + # Note: because TestCase keeps the transaction open, we need self.captureOnCommitCallbacks(execute=True) to + # ensure events get emitted here as if this part were its own transaction as it normally would be. + with freeze_time(created_date), self.captureOnCommitCallbacks(execute=True): problem = library_api.create_library_block(library.key, "problem", "Problem1") + doc_problem = { "id": "lborgalib_aproblemproblem1-ca3186e9", "type": "library_block", @@ -197,14 +203,14 @@ def test_create_delete_library_block(self, meilisearch_client): # Edit the problem block, freezing the date so we can verify modified date serializes correctly modified_date = datetime(2024, 5, 6, 7, 8, 9, tzinfo=timezone.utc) # noqa: UP017 - with freeze_time(modified_date): + with freeze_time(modified_date), self.captureOnCommitCallbacks(execute=True): library_api.set_library_block_olx(problem.usage_key, "") doc_problem["modified"] = modified_date.timestamp() meilisearch_client.return_value.index.return_value.update_documents.assert_called_with([doc_problem]) # Publish the content library, freezing the date so we can verify last_published date serializes correctly published_date = datetime(2024, 6, 7, 8, 9, 10, tzinfo=timezone.utc) # noqa: UP017 - with freeze_time(published_date): + with freeze_time(published_date), self.captureOnCommitCallbacks(execute=True): library_api.publish_changes(library.key) doc_problem["last_published"] = published_date.timestamp() doc_problem["published"] = {"display_name": "Blank Problem"} @@ -212,18 +218,14 @@ def test_create_delete_library_block(self, meilisearch_client): meilisearch_client.return_value.index.return_value.update_documents.assert_called_with([doc_problem]) # Delete the Library Block - library_api.delete_library_block(problem.usage_key) + with self.captureOnCommitCallbacks(execute=True): + library_api.delete_library_block(problem.usage_key) meilisearch_client.return_value.index.return_value.delete_document.assert_called_with( "lborgalib_aproblemproblem1-ca3186e9" ) # Restore the Library Block - library_api.restore_library_block(problem.usage_key) + with self.captureOnCommitCallbacks(execute=True): + library_api.restore_library_block(problem.usage_key) meilisearch_client.return_value.index.return_value.update_documents.assert_any_call([doc_problem]) - meilisearch_client.return_value.index.return_value.update_documents.assert_any_call( - [{'id': doc_problem['id'], 'collections': {'display_name': [], 'key': []}}] - ) - meilisearch_client.return_value.index.return_value.update_documents.assert_any_call( - [{'id': doc_problem['id'], 'tags': {}}] - ) diff --git a/openedx/core/djangoapps/content_libraries/api/blocks.py b/openedx/core/djangoapps/content_libraries/api/blocks.py index f8d45b78d5fe..7aada0ecdc90 100644 --- a/openedx/core/djangoapps/content_libraries/api/blocks.py +++ b/openedx/core/djangoapps/content_libraries/api/blocks.py @@ -25,20 +25,8 @@ from opaque_keys.edx.locator import LibraryContainerLocator, LibraryLocatorV2, LibraryUsageLocatorV2 from openedx_content import api as content_api from openedx_content.models_api import Collection, Component, ComponentVersion, Container, LearningPackage, MediaType -from openedx_events.content_authoring.data import ( - ContentObjectChangedData, - LibraryBlockData, - LibraryCollectionData, - LibraryContainerData, -) -from openedx_events.content_authoring.signals import ( - CONTENT_OBJECT_ASSOCIATIONS_CHANGED, - LIBRARY_BLOCK_CREATED, - LIBRARY_BLOCK_DELETED, - LIBRARY_BLOCK_UPDATED, - LIBRARY_COLLECTION_UPDATED, - LIBRARY_CONTAINER_UPDATED, -) +from openedx_events.content_authoring.data import LibraryBlockData +from openedx_events.content_authoring.signals import LIBRARY_BLOCK_DELETED from xblock.core import XBlock from openedx.core.djangoapps.content_staging.data import StagedContentID @@ -49,16 +37,13 @@ ) from openedx.core.types import User as UserType -from .. import tasks from ..models import ContentLibrary from .block_metadata import LibraryXBlockMetadata, LibraryXBlockStaticFile -from .collections import library_collection_locator from .container_metadata import container_subclass_for_olx_tag from .containers import ( ContainerMetadata, create_container, get_container, - get_containers_contains_item, update_container_children, ) from .exceptions import ( @@ -255,29 +240,6 @@ def set_library_block_olx(usage_key: LibraryUsageLocatorV2, new_olx_str: str) -> created=now, ) - # .. event_implemented_name: LIBRARY_BLOCK_UPDATED - # .. event_type: org.openedx.content_authoring.library_block.updated.v1 - transaction.on_commit(lambda: LIBRARY_BLOCK_UPDATED.send_event( - library_block=LibraryBlockData( - library_key=usage_key.context_key, - usage_key=usage_key - ) - )) - - # For each container, trigger LIBRARY_CONTAINER_UPDATED signal and set background=True to trigger - # container indexing asynchronously. - affected_containers = get_containers_contains_item(usage_key) - for container in affected_containers: - # .. event_implemented_name: LIBRARY_CONTAINER_UPDATED - # .. event_type: org.openedx.content_authoring.content_library.container.updated.v1 - container_key = container.container_key - transaction.on_commit(lambda ck=container_key: LIBRARY_CONTAINER_UPDATED.send_event( # type: ignore[misc] - library_container=LibraryContainerData( - container_key=ck, - background=True, - ) - )) - return new_component_version @@ -354,16 +316,6 @@ def create_library_block( _create_component_for_block(content_library, usage_key, user_id, can_stand_alone) # Now return the metadata about the new block: - - # .. event_implemented_name: LIBRARY_BLOCK_CREATED - # .. event_type: org.openedx.content_authoring.library_block.created.v1 - LIBRARY_BLOCK_CREATED.send_event( - library_block=LibraryBlockData( - library_key=content_library.library_key, - usage_key=usage_key - ) - ) - return get_library_block(usage_key) @@ -496,16 +448,6 @@ def _import_staged_block( path=filename, ) - # Emit library block created event - # .. event_implemented_name: LIBRARY_BLOCK_CREATED - # .. event_type: org.openedx.content_authoring.library_block.created.v1 - transaction.on_commit(lambda: LIBRARY_BLOCK_CREATED.send_event( - library_block=LibraryBlockData( - library_key=content_library.library_key, - usage_key=usage_key - ) - )) - # Now return the metadata about the new block return get_library_block(usage_key) @@ -707,16 +649,6 @@ def delete_library_block( """ library_key = usage_key.context_key - def send_block_deleted_signal(): - # .. event_implemented_name: LIBRARY_BLOCK_DELETED - # .. event_type: org.openedx.content_authoring.library_block.deleted.v1 - LIBRARY_BLOCK_DELETED.send_event( - library_block=LibraryBlockData( - library_key=library_key, - usage_key=usage_key - ) - ) - try: component = get_component_from_usage_key(usage_key) except Component.DoesNotExist: @@ -725,46 +657,15 @@ def send_block_deleted_signal(): # (an intermediate error occurred). # In that case, we keep the index updated by removing the entry, # but still raise the error so the caller knows the component did not exist. - send_block_deleted_signal() - raise - affected_collections = content_api.get_entity_collections(component.learning_package_id, component.entity_ref) - affected_containers = get_containers_contains_item(usage_key) - - content_api.soft_delete_draft(component.id, deleted_by=user_id) - - send_block_deleted_signal() - - # For each collection, trigger LIBRARY_COLLECTION_UPDATED signal and set background=True to trigger - # collection indexing asynchronously. - # - # To delete the component on collections - for collection in affected_collections: - # .. event_implemented_name: LIBRARY_COLLECTION_UPDATED - # .. event_type: org.openedx.content_authoring.content_library.collection.updated.v1 - LIBRARY_COLLECTION_UPDATED.send_event( - library_collection=LibraryCollectionData( - collection_key=library_collection_locator( - library_key=library_key, - collection_key=collection.collection_code, - ), - background=True, - ) + # .. event_implemented_name: LIBRARY_BLOCK_DELETED + # .. event_type: org.openedx.content_authoring.library_block.deleted.v1 + LIBRARY_BLOCK_DELETED.send_event( + library_block=LibraryBlockData(library_key=library_key, usage_key=usage_key) ) + raise - # For each container, trigger LIBRARY_CONTAINER_UPDATED signal and set background=True to trigger - # container indexing asynchronously. - # - # To update the components count in containers - for container in affected_containers: - # .. event_implemented_name: LIBRARY_CONTAINER_UPDATED - # .. event_type: org.openedx.content_authoring.content_library.container.updated.v1 - LIBRARY_CONTAINER_UPDATED.send_event( - library_container=LibraryContainerData( - container_key=container.container_key, - background=True, - ) - ) + content_api.soft_delete_draft(component.id, deleted_by=user_id) def restore_library_block(usage_key: LibraryUsageLocatorV2, user_id: int | None = None) -> None: @@ -772,9 +673,6 @@ def restore_library_block(usage_key: LibraryUsageLocatorV2, user_id: int | None Restore the specified library block. """ component = get_component_from_usage_key(usage_key) - library_key = usage_key.context_key - affected_collections = content_api.get_entity_collections(component.learning_package_id, component.entity_ref) - # Set draft version back to the latest available component version id. content_api.set_draft_version( component.id, @@ -782,57 +680,6 @@ def restore_library_block(usage_key: LibraryUsageLocatorV2, user_id: int | None set_by=user_id, ) - # .. event_implemented_name: LIBRARY_BLOCK_CREATED - # .. event_type: org.openedx.content_authoring.library_block.created.v1 - LIBRARY_BLOCK_CREATED.send_event( - library_block=LibraryBlockData( - library_key=library_key, - usage_key=usage_key - ) - ) - - # Add tags and collections back to index - # .. event_implemented_name: CONTENT_OBJECT_ASSOCIATIONS_CHANGED - # .. event_type: org.openedx.content_authoring.content.object.associations.changed.v1 - CONTENT_OBJECT_ASSOCIATIONS_CHANGED.send_event( - content_object=ContentObjectChangedData( - object_id=str(usage_key), - changes=["collections", "tags", "units"], - ), - ) - - # For each collection, trigger LIBRARY_COLLECTION_UPDATED signal and set background=True to trigger - # collection indexing asynchronously. - # - # To restore the component in the collections - for collection in affected_collections: - # .. event_implemented_name: LIBRARY_COLLECTION_UPDATED - # .. event_type: org.openedx.content_authoring.content_library.collection.updated.v1 - LIBRARY_COLLECTION_UPDATED.send_event( - library_collection=LibraryCollectionData( - collection_key=library_collection_locator( - library_key=library_key, - collection_key=collection.collection_code, - ), - background=True, - ) - ) - - # For each container, trigger LIBRARY_CONTAINER_UPDATED signal and set background=True to trigger - # container indexing asynchronously. - # - # To update the components count in containers - affected_containers = get_containers_contains_item(usage_key) - for container in affected_containers: - # .. event_implemented_name: LIBRARY_CONTAINER_UPDATED - # .. event_type: org.openedx.content_authoring.content_library.container.updated.v1 - LIBRARY_CONTAINER_UPDATED.send_event( - library_container=LibraryContainerData( - container_key=container.container_key, - background=True, - ) - ) - def get_library_block_static_asset_files(usage_key: LibraryUsageLocatorV2) -> list[LibraryXBlockStaticFile]: """ @@ -918,16 +765,6 @@ def add_library_block_static_asset_file( created=datetime.now(tz=timezone.utc), # noqa: UP017 created_by=user.id if user else None, ) - transaction.on_commit( - # .. event_implemented_name: LIBRARY_BLOCK_UPDATED - # .. event_type: org.openedx.content_authoring.library_block.updated.v1 - lambda: LIBRARY_BLOCK_UPDATED.send_event( - library_block=LibraryBlockData( - library_key=usage_key.context_key, - usage_key=usage_key, - ) - ) - ) # Now figure out the URL for the newly created asset... site_root_url = get_xblock_app_config().get_site_root_url() @@ -966,16 +803,6 @@ def delete_library_block_static_asset_file(usage_key, file_path, user=None): created=now, created_by=user.id if user else None, ) - transaction.on_commit( - # .. event_implemented_name: LIBRARY_BLOCK_UPDATED - # .. event_type: org.openedx.content_authoring.library_block.updated.v1 - lambda: LIBRARY_BLOCK_UPDATED.send_event( - library_block=LibraryBlockData( - library_key=usage_key.context_key, - usage_key=usage_key, - ) - ) - ) def publish_component_changes(usage_key: LibraryUsageLocatorV2, user_id: int): @@ -990,13 +817,7 @@ def publish_component_changes(usage_key: LibraryUsageLocatorV2, user_id: int): # The core publishing API is based on draft objects, so find the draft that corresponds to this component: drafts_to_publish = content_api.get_all_drafts(learning_package.id).filter(entity__entity_ref=component.entity_ref) # Publish the component and update anything that needs to be updated (e.g. search index): - publish_log = content_api.publish_from_drafts( - learning_package.id, draft_qset=drafts_to_publish, published_by=user_id, - ) - # 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) + content_api.publish_from_drafts(learning_package.id, draft_qset=drafts_to_publish, published_by=user_id) def _component_exists(usage_key: UsageKeyV2) -> bool: diff --git a/openedx/core/djangoapps/content_libraries/api/collections.py b/openedx/core/djangoapps/content_libraries/api/collections.py index 1c87acd21232..a8b715750d64 100644 --- a/openedx/core/djangoapps/content_libraries/api/collections.py +++ b/openedx/core/djangoapps/content_libraries/api/collections.py @@ -7,8 +7,6 @@ from opaque_keys.edx.locator import LibraryCollectionLocator, LibraryContainerLocator, LibraryLocatorV2 from openedx_content import api as content_api from openedx_content.models_api import Collection, Component, PublishableEntity -from openedx_events.content_authoring.data import LibraryCollectionData -from openedx_events.content_authoring.signals import LIBRARY_COLLECTION_UPDATED from ..models import ContentLibrary from .exceptions import ( @@ -216,27 +214,12 @@ def set_library_item_collections( collection_code__in=collection_keys ) - affected_collections = content_api.set_collections( + content_api.set_collections( publishable_entity, collection_qs, created_by=created_by, ) - # For each collection, trigger LIBRARY_COLLECTION_UPDATED signal and set background=True to trigger - # collection indexing asynchronously. - for collection in affected_collections: - # .. event_implemented_name: LIBRARY_COLLECTION_UPDATED - # .. event_type: org.openedx.content_authoring.content_library.collection.updated.v1 - LIBRARY_COLLECTION_UPDATED.send_event( - library_collection=LibraryCollectionData( - collection_key=library_collection_locator( - library_key=library_key, - collection_key=collection.collection_code, - ), - background=True, - ) - ) - return publishable_entity diff --git a/openedx/core/djangoapps/content_libraries/api/containers.py b/openedx/core/djangoapps/content_libraries/api/containers.py index 0b126e5665df..92707bb860b5 100644 --- a/openedx/core/djangoapps/content_libraries/api/containers.py +++ b/openedx/core/djangoapps/content_libraries/api/containers.py @@ -9,24 +9,14 @@ from datetime import datetime, timezone from uuid import uuid4 -from django.db import transaction from django.db.models import F from django.utils.text import slugify from opaque_keys.edx.locator import LibraryContainerLocator, LibraryLocatorV2, LibraryUsageLocatorV2 from openedx_content import api as content_api -from openedx_content.models_api import Container, Unit -from openedx_events.content_authoring.data import ContentObjectChangedData, LibraryCollectionData, LibraryContainerData -from openedx_events.content_authoring.signals import ( - CONTENT_OBJECT_ASSOCIATIONS_CHANGED, - LIBRARY_COLLECTION_UPDATED, - LIBRARY_CONTAINER_CREATED, - LIBRARY_CONTAINER_DELETED, - LIBRARY_CONTAINER_UPDATED, -) - -from openedx.core.djangoapps.content_libraries.api.collections import library_collection_locator +from openedx_content.models_api import Container +from openedx_events.content_authoring.data import LibraryContainerData +from openedx_events.content_authoring.signals import LIBRARY_CONTAINER_DELETED -from .. import tasks from ..models import ContentLibrary from .block_metadata import LibraryXBlockMetadata from .container_metadata import ( @@ -113,7 +103,7 @@ def create_container( # Automatically generate a slug. Append a random suffix so it should be unique. slug = slugify(title, allow_unicode=True) + "-" + uuid4().hex[-6:] # Make sure the slug is valid by first creating a key for the new container: - container_key = LibraryContainerLocator( + _container_key = LibraryContainerLocator( library_key, container_type=container_cls.type_code, container_id=slug, @@ -133,14 +123,6 @@ def create_container( created_by=user_id, ) - # .. event_implemented_name: LIBRARY_CONTAINER_CREATED - # .. event_type: org.openedx.content_authoring.content_library.container.created.v1 - transaction.on_commit(lambda: LIBRARY_CONTAINER_CREATED.send_event( - library_container=LibraryContainerData( - container_key=container_key, - ) - )) - return ContainerMetadata.from_container(library_key, container) @@ -156,9 +138,6 @@ def update_container( library_key = container_key.lib_key created = datetime.now(tz=timezone.utc) # noqa: UP017 - # Get children containers or components to update their index data - children = get_container_children(container_key, published=False) - version = content_api.create_next_container_version( container, title=display_name, @@ -166,34 +145,6 @@ def update_container( created_by=user_id, ) - # Send event related to the updated container - # .. event_implemented_name: LIBRARY_CONTAINER_UPDATED - # .. event_type: org.openedx.content_authoring.content_library.container.updated.v1 - LIBRARY_CONTAINER_UPDATED.send_event(library_container=LibraryContainerData(container_key=container_key)) - - # Send events related to the containers that contains the updated container. - # This is to update the children display names used in the section/subsection previews. - affected_containers = get_containers_contains_item(container_key) - for affected_container in affected_containers: - # .. event_implemented_name: LIBRARY_CONTAINER_UPDATED - # .. event_type: org.openedx.content_authoring.content_library.container.updated.v1 - LIBRARY_CONTAINER_UPDATED.send_event( - library_container=LibraryContainerData(container_key=affected_container.container_key) - ) - # Update children components and containers index data, for example, - # All subsections under a section have section key in index that needs to be updated. - # So if parent section name has been changed, it needs to be reflected in sections key of children - is_unit = container_key.container_type == Unit.type_code - for child in children: - # .. event_implemented_name: CONTENT_OBJECT_ASSOCIATIONS_CHANGED - # .. event_type: org.openedx.content_authoring.content.object.associations.changed.v1 - CONTENT_OBJECT_ASSOCIATIONS_CHANGED.send_event( - content_object=ContentObjectChangedData( - object_id=str(child.usage_key if is_unit else child.container_key), # type: ignore[union-attr] - changes=[container_key.container_type + "s"], # e.g. "units" - ), - ) - return ContainerMetadata.from_container(library_key, version.container) @@ -205,15 +156,6 @@ def delete_container( No-op if container doesn't exist or has already been soft-deleted. """ - def send_container_deleted_signal(): - # .. event_implemented_name: LIBRARY_CONTAINER_DELETED - # .. event_type: org.openedx.content_authoring.content_library.container.deleted.v1 - LIBRARY_CONTAINER_DELETED.send_event( - library_container=LibraryContainerData( - container_key=container_key, - ) - ) - try: container = get_container_from_key(container_key) except Container.DoesNotExist: @@ -222,149 +164,20 @@ def send_container_deleted_signal(): # (an intermediate error occurred). # In that case, we keep the index updated by removing the entry, # but still raise the error so the caller knows the container did not exist. - send_container_deleted_signal() + # .. event_implemented_name: LIBRARY_CONTAINER_DELETED + # .. event_type: org.openedx.content_authoring.content_library.container.deleted.v1 + LIBRARY_CONTAINER_DELETED.send_event(library_container=LibraryContainerData(container_key=container_key)) raise - library_key = container_key.lib_key - - # Fetch related collections and containers before soft-delete - affected_collections = content_api.get_entity_collections( - container.publishable_entity.learning_package_id, - container.entity_ref, - ) - affected_containers = get_containers_contains_item(container_key) - # Get children containers or components to update their index data - children = get_container_children( - container_key, - published=False, - ) content_api.soft_delete_draft(container.id) - send_container_deleted_signal() - - # For each collection, trigger LIBRARY_COLLECTION_UPDATED signal and set background=True to trigger - # collection indexing asynchronously. - # - # To delete the container on collections - for collection in affected_collections: - # .. event_implemented_name: LIBRARY_COLLECTION_UPDATED - # .. event_type: org.openedx.content_authoring.content_library.collection.updated.v1 - LIBRARY_COLLECTION_UPDATED.send_event( - library_collection=LibraryCollectionData( - collection_key=library_collection_locator( - library_key=library_key, - collection_key=collection.collection_code, - ), - background=True, - ) - ) - # Send events related to the containers that contains the updated container. - # This is to update the children display names used in the section/subsection previews. - for affected_container in affected_containers: - # .. event_implemented_name: LIBRARY_CONTAINER_UPDATED - # .. event_type: org.openedx.content_authoring.content_library.container.updated.v1 - LIBRARY_CONTAINER_UPDATED.send_event( - library_container=LibraryContainerData( - container_key=affected_container.container_key, - ) - ) - key_name = "container_key" - if isinstance(container, Unit): - # Components have usage_key instead of container_key - key_name = "usage_key" - # Update children components and containers index data, for example, - # All subsections under a section have section key in index that needs to be updated. - # So if parent section is deleted, it needs to be removed from sections key of children - for child in children: - # .. event_implemented_name: CONTENT_OBJECT_ASSOCIATIONS_CHANGED - # .. event_type: org.openedx.content_authoring.content.object.associations.changed.v1 - CONTENT_OBJECT_ASSOCIATIONS_CHANGED.send_event( - content_object=ContentObjectChangedData( - object_id=str(getattr(child, key_name)), - changes=[container_key.container_type + "s"], - ), - ) - def restore_container(container_key: LibraryContainerLocator) -> None: """ [ 🛑 UNSTABLE ] Restore the specified library container. """ - library_key = container_key.lib_key container = get_container_from_key(container_key, include_deleted=True) - - affected_collections = content_api.get_entity_collections( - container.publishable_entity.learning_package_id, - container.entity_ref, - ) - content_api.set_draft_version(container.id, container.versioning.latest.pk) - # Fetch related containers after restore - affected_containers = get_containers_contains_item(container_key) - # Get children containers or components to update their index data - children = get_container_children(container_key, published=False) - - # .. event_implemented_name: LIBRARY_CONTAINER_CREATED - # .. event_type: org.openedx.content_authoring.content_library.container.created.v1 - LIBRARY_CONTAINER_CREATED.send_event( - library_container=LibraryContainerData( - container_key=container_key, - ) - ) - - content_changes = ["collections", "tags"] - if affected_containers and len(affected_containers) > 0: - # Update parent key data in index. Eg. `sections` key in index for subsection - content_changes.append(str(affected_containers[0].container_type_code) + "s") - # Add tags, collections and parent data back to index - # .. event_implemented_name: CONTENT_OBJECT_ASSOCIATIONS_CHANGED - # .. event_type: org.openedx.content_authoring.content.object.associations.changed.v1 - CONTENT_OBJECT_ASSOCIATIONS_CHANGED.send_event( - content_object=ContentObjectChangedData( - object_id=str(container_key), - changes=content_changes, - ), - ) - - # For each collection, trigger LIBRARY_COLLECTION_UPDATED signal and set background=True to trigger - # collection indexing asynchronously. - # - # To restore the container on collections - for collection in affected_collections: - # .. event_implemented_name: LIBRARY_COLLECTION_UPDATED - # .. event_type: org.openedx.content_authoring.content_library.collection.updated.v1 - LIBRARY_COLLECTION_UPDATED.send_event( - library_collection=LibraryCollectionData( - collection_key=library_collection_locator( - library_key=library_key, - collection_key=collection.collection_code, - ), - ) - ) - # Send events related to the containers that contains the updated container. - # This is to update the children display names used in the section/subsection previews. - for affected_container in affected_containers: - # .. event_implemented_name: LIBRARY_CONTAINER_UPDATED - # .. event_type: org.openedx.content_authoring.content_library.container.updated.v1 - LIBRARY_CONTAINER_UPDATED.send_event( - library_container=LibraryContainerData( - container_key=affected_container.container_key, - ) - ) - - is_unit = container_key.container_type == Unit.type_code - # Update children components and containers index data, for example, - # All subsections under a section have section key in index that needs to be updated. - # Should restore removed parent section in sections key of children subsections - for child in children: - # .. event_implemented_name: CONTENT_OBJECT_ASSOCIATIONS_CHANGED - # .. event_type: org.openedx.content_authoring.content.object.associations.changed.v1 - CONTENT_OBJECT_ASSOCIATIONS_CHANGED.send_event( - content_object=ContentObjectChangedData( - object_id=str(child.usage_key if is_unit else child.container_key), # type: ignore[union-attr] - changes=[container_key.container_type + "s"], - ), - ) def get_container_children( @@ -419,23 +232,6 @@ def update_container_children( entities=[get_entity_from_key(key) for key in children_keys], entities_action=entities_action, ) - for key in children_keys: - # .. event_implemented_name: CONTENT_OBJECT_ASSOCIATIONS_CHANGED - # .. event_type: org.openedx.content_authoring.content.object.associations.changed.v1 - CONTENT_OBJECT_ASSOCIATIONS_CHANGED.send_event( - content_object=ContentObjectChangedData( - object_id=str(key), - changes=[f"{container_key.container_type}s"], # "units", "subsections", "sections" - ), - ) - - # .. event_implemented_name: LIBRARY_CONTAINER_UPDATED - # .. event_type: org.openedx.content_authoring.content_library.container.updated.v1 - LIBRARY_CONTAINER_UPDATED.send_event( - library_container=LibraryContainerData( - container_key=container_key, - ) - ) return ContainerMetadata.from_container(container_key.lib_key, new_version.container) @@ -452,7 +248,6 @@ def get_containers_contains_item(key: LibraryUsageLocatorV2 | LibraryContainerLo def publish_container_changes( container_key: LibraryContainerLocator, user_id: int | None, - call_post_publish_events_sync=False, ) -> None: """ [ 🛑 UNSTABLE ] Publish all unpublished changes in a container and all its child @@ -466,17 +261,7 @@ def publish_container_changes( # The core publishing API is based on draft objects, so find the draft that corresponds to this container: drafts_to_publish = content_api.get_all_drafts(learning_package.id).filter(entity__pk=container.id) # Publish the container, which will also auto-publish any unpublished child components: - publish_log = content_api.publish_from_drafts( - learning_package.id, - draft_qset=drafts_to_publish, - published_by=user_id, - ) - # 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. - if call_post_publish_events_sync: - tasks.send_events_after_publish(publish_log.pk, str(library_key)) - else: - tasks.wait_for_post_publish_events(publish_log, library_key) + content_api.publish_from_drafts(learning_package.id, draft_qset=drafts_to_publish, published_by=user_id) def copy_container(container_key: LibraryContainerLocator, user_id: int) -> UserClipboardData: diff --git a/openedx/core/djangoapps/content_libraries/api/libraries.py b/openedx/core/djangoapps/content_libraries/api/libraries.py index 68d4258bb065..bfce8c2d5959 100644 --- a/openedx/core/djangoapps/content_libraries/api/libraries.py +++ b/openedx/core/djangoapps/content_libraries/api/libraries.py @@ -72,7 +72,7 @@ from openedx.core.types import User as UserType -from .. import permissions, tasks +from .. import permissions from ..constants import ALL_RIGHTS_RESERVED from ..models import ContentLibrary, ContentLibraryPermission from .exceptions import LibraryAlreadyExists, LibraryPermissionIntegrityError @@ -758,15 +758,7 @@ def publish_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. - publish_log = content_api.publish_all_drafts(learning_package.id, published_by=user_id) - - # 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. + content_api.publish_all_drafts(learning_package.id, published_by=user_id) def revert_changes(library_key: LibraryLocatorV2, user_id: int | None = None) -> None: @@ -776,12 +768,9 @@ 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. - with content_api.bulk_draft_changes_for(learning_package.id) as draft_change_log: + with content_api.bulk_draft_changes_for(learning_package.id): content_api.reset_drafts_to_published(learning_package.id, reset_by=user_id) - # Call the event handlers as needed. - tasks.wait_for_post_revert_events(draft_change_log, library_key) - def get_backup_task_status( user_id: int, diff --git a/openedx/core/djangoapps/content_libraries/library_context.py b/openedx/core/djangoapps/content_libraries/library_context.py index 5d2c25c4286e..7cba96e0da94 100644 --- a/openedx/core/djangoapps/content_libraries/library_context.py +++ b/openedx/core/djangoapps/content_libraries/library_context.py @@ -7,8 +7,6 @@ from opaque_keys.edx.keys import UsageKeyV2 from opaque_keys.edx.locator import LibraryLocatorV2, LibraryUsageLocatorV2 from openedx_content import api as content_api -from openedx_events.content_authoring.data import LibraryBlockData, LibraryContainerData -from openedx_events.content_authoring.signals import LIBRARY_BLOCK_UPDATED, LIBRARY_CONTAINER_UPDATED from rest_framework.exceptions import NotFound from openedx.core.djangoapps.content_libraries import api, permissions @@ -101,34 +99,3 @@ def block_exists(self, usage_key: LibraryUsageLocatorV2): type_name=usage_key.block_type, component_code=usage_key.block_id, ) - - def send_block_updated_event(self, usage_key: UsageKeyV2): - """ - Send a "block updated" event for the library block with the given usage_key. - """ - assert isinstance(usage_key, LibraryUsageLocatorV2) - # .. event_implemented_name: LIBRARY_BLOCK_UPDATED - # .. event_type: org.openedx.content_authoring.library_block.updated.v1 - LIBRARY_BLOCK_UPDATED.send_event( - library_block=LibraryBlockData( - library_key=usage_key.lib_key, - usage_key=usage_key, - ) - ) - - def send_container_updated_events(self, usage_key: UsageKeyV2): - """ - Send "container updated" events for containers that contains the library block - with the given usage_key. - """ - assert isinstance(usage_key, LibraryUsageLocatorV2) - affected_containers = api.get_containers_contains_item(usage_key) - for container in affected_containers: - # .. event_implemented_name: LIBRARY_CONTAINER_UPDATED - # .. event_type: org.openedx.content_authoring.content_library.container.updated.v1 - LIBRARY_CONTAINER_UPDATED.send_event( - library_container=LibraryContainerData( - container_key=container.container_key, - background=True, - ) - ) diff --git a/openedx/core/djangoapps/content_libraries/signal_handlers.py b/openedx/core/djangoapps/content_libraries/signal_handlers.py index fada1cb2f874..dd206c5a97cd 100644 --- a/openedx/core/djangoapps/content_libraries/signal_handlers.py +++ b/openedx/core/djangoapps/content_libraries/signal_handlers.py @@ -3,180 +3,156 @@ """ import logging +from functools import partial -from django.db.models.signals import m2m_changed, post_delete, post_save +from attrs import asdict from django.dispatch import receiver -from opaque_keys import OpaqueKey -from opaque_keys.edx.locator import LibraryLocatorV2 -from openedx_content.api import get_components, get_containers -from openedx_content.models_api import Collection, CollectionPublishableEntity, PublishableEntity -from openedx_events.content_authoring.data import ContentObjectChangedData, LibraryCollectionData +from openedx_content.api import signals as content_signals +from openedx_events.content_authoring.data import LibraryCollectionData from openedx_events.content_authoring.signals import ( - CONTENT_OBJECT_ASSOCIATIONS_CHANGED, LIBRARY_COLLECTION_CREATED, LIBRARY_COLLECTION_DELETED, LIBRARY_COLLECTION_UPDATED, ) -from .api import library_collection_locator, library_component_usage_key, library_container_locator +from . import tasks +from .api import library_collection_locator from .models import ContentLibrary log = logging.getLogger(__name__) -@receiver(post_save, sender=Collection, dispatch_uid="library_collection_saved") -def library_collection_saved(sender, instance, created, **kwargs): - """ - Raises LIBRARY_COLLECTION_CREATED if the Collection is new, - or LIBRARY_COLLECTION_UPDATED if updated an existing Collection. +@receiver(content_signals.ENTITIES_DRAFT_CHANGED) +def entities_updated( + learning_package: content_signals.LearningPackageEventData, + change_log: content_signals.DraftChangeLogEventData, + **kwargs, +) -> None: """ - try: - library = ContentLibrary.objects.get(learning_package_id=instance.learning_package_id) - except ContentLibrary.DoesNotExist: - log.error("{instance} is not associated with a content library.") - return - - if created: - # .. event_implemented_name: LIBRARY_COLLECTION_CREATED - # .. event_type: org.openedx.content_authoring.content_library.collection.created.v1 - LIBRARY_COLLECTION_CREATED.send_event( - library_collection=LibraryCollectionData( - collection_key=library_collection_locator( - library_key=library.library_key, - collection_key=instance.collection_code, - ), - ) - ) - else: - # .. event_implemented_name: LIBRARY_COLLECTION_UPDATED - # .. event_type: org.openedx.content_authoring.content_library.collection.updated.v1 - LIBRARY_COLLECTION_UPDATED.send_event( - library_collection=LibraryCollectionData( - collection_key=library_collection_locator( - library_key=library.library_key, - collection_key=instance.collection_code, - ), - ) - ) + Entities (containers/components) have been changed - handle that as needed. + We receive this low-level event from `openedx_content`, and check if it + happened in a library. If so, we emit more detailed library-specific events. -@receiver(post_delete, sender=Collection, dispatch_uid="library_collection_deleted") -def library_collection_deleted(sender, instance, **kwargs): - """ - Raises LIBRARY_COLLECTION_DELETED for the deleted Collection. + 💾 This event is only received after the transaction has committed. + ⏳ This event is emitted synchronously and this handler is called + synchronously. If a lot of entities were changed, we need to dispatch an + asynchronous handler to deal with them to avoid slowdowns. If only one + entity is changed, we want to deal with that synchronously so that we + can show the user correct data when the current requests completes. """ try: - library = ContentLibrary.objects.get(learning_package_id=instance.learning_package_id) + ContentLibrary.objects.get(learning_package_id=learning_package.id) except ContentLibrary.DoesNotExist: - log.error("{instance} is not associated with a content library.") - return - - # .. event_implemented_name: LIBRARY_COLLECTION_DELETED - # .. event_type: org.openedx.content_authoring.content_library.collection.deleted.v1 - LIBRARY_COLLECTION_DELETED.send_event( - library_collection=LibraryCollectionData( - collection_key=library_collection_locator( - library_key=library.library_key, - collection_key=instance.collection_code, - ), - ) - ) + return # We don't care about non-library events. + + # Which entities were _directly_ changed here? + direct_changes = [asdict(change) for change in change_log.changes if change.new_version != change.old_version] + # And which entities were indirectly affected (e.g. parent containers)? + indirect_changes = [asdict(change) for change in change_log.changes if change.new_version == change.old_version] + + update_task_fn = tasks.send_change_events_for_modified_entities + update_sync = partial(update_task_fn, learning_package_id=learning_package.id) + update_async = partial(update_task_fn.delay, learning_package_id=learning_package.id) + + if len(direct_changes) == 1: + # We directly changed only one entity. Update it synchronously so that the UI will reflect changes right away. + if len(indirect_changes) <= 1: + # And update any other affected entity synchronously too; there's at most one. (More efficient, better UX.) + update_sync(change_list=[*direct_changes, *indirect_changes]) + else: + update_sync(change_list=direct_changes) # Update this one entity synchronously, and + update_async(change_list=indirect_changes) # update the many other affects entities async. + else: + # More than one entity was changed at once. Handle asynchronously: + update_async(change_list=[*direct_changes, *indirect_changes]) -def _library_collection_entity_changed( - publishable_entity: PublishableEntity, - library_key: LibraryLocatorV2 | None = None, +@receiver(content_signals.ENTITIES_PUBLISHED) +def entities_published( + learning_package: content_signals.LearningPackageEventData, + change_log: content_signals.PublishLogEventData, + **kwargs, ) -> None: """ - Sends a CONTENT_OBJECT_ASSOCIATIONS_CHANGED event for the entity. - """ - if not library_key: - try: - library = ContentLibrary.objects.get( - learning_package_id=publishable_entity.learning_package_id, - ) - except ContentLibrary.DoesNotExist: - log.error("{publishable_entity} is not associated with a content library.") - return + Entities (containers/components) have been published - handle that as needed. - library_key = library.library_key + We receive this low-level event from `openedx_content`, and check if it + happened in a library. If so, we emit more detailed library-specific events. - assert library_key - - opaque_key: OpaqueKey + 💾 This event is only received after the transaction has committed. + ⏳ This event is emitted synchronously and this handler is called + synchronously. If a lot of entities were published, we need to dispatch + an asynchronous handler to deal with them to avoid slowdowns. If only one + entity was published, we want to deal with that synchronously so that we + can show the user correct data when the current requests completes. + """ + try: + library = ContentLibrary.objects.get(learning_package_id=learning_package.id) + except ContentLibrary.DoesNotExist: + return # We don't care about non-library events. - if hasattr(publishable_entity, 'component'): - opaque_key = library_component_usage_key( - library_key, - publishable_entity.component, - ) - elif hasattr(publishable_entity, 'container'): - opaque_key = library_container_locator( - library_key, - publishable_entity.container, - ) + if len(change_log.changes) == 1: + fn = tasks.send_events_after_publish else: - log.error("Unknown publishable entity type: %s", publishable_entity) - return + # More than one entity was published at once. Handle asynchronously: + fn = tasks.send_events_after_publish.delay - # .. event_implemented_name: CONTENT_OBJECT_ASSOCIATIONS_CHANGED - # .. event_type: org.openedx.content_authoring.content.object.associations.changed.v1 - CONTENT_OBJECT_ASSOCIATIONS_CHANGED.send_event( - content_object=ContentObjectChangedData( - object_id=str(opaque_key), - changes=["collections"], - ), - ) + fn(publish_log_id=change_log.publish_log_id, library_key_str=str(library.library_key)) -@receiver(post_save, sender=CollectionPublishableEntity, dispatch_uid="library_collection_entity_saved") -def library_collection_entity_saved(sender, instance, created, **kwargs): - """ - Sends a CONTENT_OBJECT_ASSOCIATIONS_CHANGED event for components added to a collection. +@receiver(content_signals.COLLECTION_CHANGED) +def collection_updated( + learning_package: content_signals.LearningPackageEventData, + change: content_signals.CollectionChangeData, + **kwargs, +) -> None: """ - if created: - _library_collection_entity_changed(instance.entity) + A Collection has been updated - handle that as needed. + We receive this low-level event from `openedx_content`, and check if it + happened in a library. If so, we emit more detailed library-specific events. -@receiver(post_delete, sender=CollectionPublishableEntity, dispatch_uid="library_collection_entity_deleted") -def library_collection_entity_deleted(sender, instance, **kwargs): - """ - Sends a CONTENT_OBJECT_ASSOCIATIONS_CHANGED event for components removed from a collection. + ⏳ This event is emitted synchronously and this handler is called + synchronously. If a lot of entities were changed, we need to dispatch an + asynchronous handler to deal with them to avoid slowdowns. """ - # Only trigger component updates if CollectionPublishableEntity was cascade deleted due to deletion of a collection. - if isinstance(kwargs.get('origin'), Collection): - _library_collection_entity_changed(instance.entity) - + try: + library = ContentLibrary.objects.get(learning_package_id=learning_package.id) + except ContentLibrary.DoesNotExist: + return # We don't care about non-library events. -@receiver(m2m_changed, sender=CollectionPublishableEntity, dispatch_uid="library_collection_entities_changed") -def library_collection_entities_changed(sender, instance, action, pk_set, **kwargs): - """ - Sends a CONTENT_OBJECT_ASSOCIATIONS_CHANGED event for components added/removed/cleared from a collection. - """ - if action not in ["post_add", "post_remove", "post_clear"]: - return + collection_key = library_collection_locator(library_key=library.library_key, collection_key=change.collection_code) + entities_changed = change.entities_added + change.entities_removed - try: - library = ContentLibrary.objects.get( - learning_package_id=instance.learning_package_id, + if change.created: # This is a newly-created collection, or was "un-deleted": + # .. event_implemented_name: LIBRARY_COLLECTION_CREATED + # .. event_type: org.openedx.content_authoring.content_library.collection.created.v1 + LIBRARY_COLLECTION_CREATED.send_event(library_collection=LibraryCollectionData(collection_key=collection_key)) + # As an example of what this event triggers, Collections are listed in the Meilisearch index as items in the + # library. So the handler will add this Collection as an entry in the Meilisearch index. + elif change.metadata_modified or entities_changed: + # The collection was renamed or its items were changed. + # This event is ambiguous but because the search index of the collection itself may have something like + # "contains 15 items", we _do_ need to emit it even when only the items have changed and not the metadata. + # .. event_implemented_name: LIBRARY_COLLECTION_UPDATED + # .. event_type: org.openedx.content_authoring.content_library.collection.updated.v1 + LIBRARY_COLLECTION_UPDATED.send_event(library_collection=LibraryCollectionData(collection_key=collection_key)) + elif change.deleted: + # .. event_implemented_name: LIBRARY_COLLECTION_DELETED + # .. event_type: org.openedx.content_authoring.content_library.collection.deleted.v1 + LIBRARY_COLLECTION_DELETED.send_event(library_collection=LibraryCollectionData(collection_key=collection_key)) + + # Now, what about the actual entities (containers/components) in the collection? + if entities_changed: + if len(entities_changed) == 1: + # If there's only one changed entity, emit the event synchronously: + fn = tasks.send_collections_changed_events + else: + # If there are more than one changed entities, emit the events asynchronously: + fn = tasks.send_collections_changed_events.delay + fn( + publishable_entity_ids=sorted(entities_changed), # sorted() is mostly for test purposes + learning_package_id=learning_package.id, + library_key_str=str(library.library_key), ) - except ContentLibrary.DoesNotExist: - log.error("{instance} is not associated with a content library.") - return - - if isinstance(instance, PublishableEntity): - _library_collection_entity_changed(instance, library.library_key) - return - - # When action=="post_clear", pk_set==None - # Since the collection instance now has an empty entities set, - # we don't know which ones were removed, so we need to update associations for all library - # components and containers. - components = get_components(instance.learning_package_id) - containers = get_containers(instance.learning_package_id) - if pk_set: - components = components.filter(pk__in=pk_set) - containers = containers.filter(pk__in=pk_set) - - for entity in list(components.all()) + list(containers.all()): - _library_collection_entity_changed(entity.publishable_entity, library.library_key) diff --git a/openedx/core/djangoapps/content_libraries/tasks.py b/openedx/core/djangoapps/content_libraries/tasks.py index ee972b248649..b3d38e9549f1 100644 --- a/openedx/core/djangoapps/content_libraries/tasks.py +++ b/openedx/core/djangoapps/content_libraries/tasks.py @@ -14,12 +14,14 @@ A longer-term solution to this issue would be to move the content_libraries app to cms: https://github.com/openedx/edx-platform/issues/33428 """ + from __future__ import annotations import json import logging import os import shutil +from collections.abc import Iterable from datetime import datetime from io import StringIO from tempfile import NamedTemporaryFile, mkdtemp @@ -38,22 +40,27 @@ set_code_owner_attribute_from_module, set_custom_attribute, ) +from opaque_keys import OpaqueKey from opaque_keys.edx.locator import ( BlockUsageLocator, - LibraryCollectionLocator, LibraryContainerLocator, LibraryLocatorV2, + LibraryUsageLocatorV2, ) from openedx_content import api as content_api from openedx_content.api import create_zip_file as create_lib_zip_file -from openedx_content.models_api import DraftChangeLog, PublishLog -from openedx_events.content_authoring.data import LibraryBlockData, LibraryCollectionData, LibraryContainerData +from openedx_content.models_api import LearningPackage, PublishableEntity, PublishLog +from openedx_events.content_authoring.data import ( + ContentObjectChangedData, + LibraryBlockData, + LibraryContainerData, +) from openedx_events.content_authoring.signals import ( + CONTENT_OBJECT_ASSOCIATIONS_CHANGED, LIBRARY_BLOCK_CREATED, LIBRARY_BLOCK_DELETED, LIBRARY_BLOCK_PUBLISHED, LIBRARY_BLOCK_UPDATED, - LIBRARY_COLLECTION_UPDATED, LIBRARY_CONTAINER_CREATED, LIBRARY_CONTAINER_DELETED, LIBRARY_CONTAINER_PUBLISHED, @@ -74,6 +81,7 @@ from xmodule.modulestore.mixed import MixedModuleStore from . import api +from .models import ContentLibrary log = logging.getLogger(__name__) TASK_LOGGER = get_task_logger(__name__) @@ -85,7 +93,220 @@ @shared_task(base=LoggedTask) @set_code_owner_attribute -def send_events_after_publish(publish_log_pk: int, library_key_str: str) -> None: +def send_change_events_for_modified_entities( + learning_package_id: LearningPackage.ID, + change_list: list[dict], # we want list[ChangeLogRecordData], but that's not JSON serializable, so use dicts +): + """ + Sends a various library-specific events for each modified library entity in + the given change log, after any kind of edit was made in the library. This + could be in response to an entity (component or container) being created, + modified, deleted, un-deleted, or one of its dependencies doing those + things. + + ⏳ This task is designed to be run asynchronously so it can handle many + entities, but you can also call it synchronously if you are only + processing a single entity. Handlers of the events that we emit here + should be synchronous and fast, to support the "update one item + synchronously" use case, but can be async if needed. + """ + changes = [content_api.signals.ChangeLogRecordData(**r) for r in change_list] + library = ContentLibrary.objects.get(learning_package_id=learning_package_id) + changes_by_entity_id = {change.entity_id: change for change in changes} + entities = ( + content_api.get_publishable_entities(learning_package_id) + .filter(id__in=changes_by_entity_id.keys()) + .select_related("component", "container") + ) + + for entity in entities: + change = changes_by_entity_id[entity.id] + if hasattr(entity, "component"): + # This is a library XBlock (component) + block_key = api.library_component_usage_key(library.library_key, entity.component) + event_data = LibraryBlockData(library_key=library.library_key, usage_key=block_key) + if change.old_version is None and change.new_version: + # .. event_implemented_name: LIBRARY_BLOCK_CREATED + # .. event_type: org.openedx.content_authoring.library_block.created.v1 + LIBRARY_BLOCK_CREATED.send_event(library_block=event_data) + elif change.old_version and change.new_version is None: + # .. event_implemented_name: LIBRARY_BLOCK_DELETED + # .. event_type: org.openedx.content_authoring.library_block.deleted.v1 + LIBRARY_BLOCK_DELETED.send_event(library_block=event_data) + else: + # If the version numbers are different, this block was modified. + # If not, it was included as a side effect of some other change, like renaming a parent container. + # .. event_implemented_name: LIBRARY_BLOCK_UPDATED + # .. event_type: org.openedx.content_authoring.library_block.updated.v1 + LIBRARY_BLOCK_UPDATED.send_event(library_block=event_data) + + elif hasattr(entity, "container"): + container_key = api.library_container_locator(library.library_key, entity.container) + event_data = LibraryContainerData(container_key=container_key) + if change.old_version is None and change.new_version: + # .. event_implemented_name: LIBRARY_CONTAINER_CREATED + # .. event_type: org.openedx.content_authoring.content_library.container.created.v1 + LIBRARY_CONTAINER_CREATED.send_event(library_container=event_data) + elif change.old_version and change.new_version is None: + # .. event_implemented_name: LIBRARY_CONTAINER_DELETED + # .. event_type: org.openedx.content_authoring.content_library.container.deleted.v1 + LIBRARY_CONTAINER_DELETED.send_event(library_container=event_data) + else: + # If the version numbers are different, this container was modified. + # If not, it was included as a side effect of some other change, like its child being modified. + container_itself_changed = change.old_version != change.new_version + + # .. event_implemented_name: LIBRARY_CONTAINER_UPDATED + # .. event_type: org.openedx.content_authoring.content_library.container.updated.v1 + LIBRARY_CONTAINER_UPDATED.send_event(library_container=event_data) + # TODO: to optimze this, once we have https://github.com/openedx/openedx-events/pull/570 merged, + # change the above event to use `send_async=not container_itself_changed`, so that direct changes are + # processed immediately but side effects can happen async. + + if container_itself_changed: + # If entities were added/removed from this container, we need to notify things like the search index + # that the list of parent containers for each entity has changed. + check_container_content_changes.delay( + container_key_str=str(container_key), + old_version_id=change.old_version_id, + new_version_id=change.new_version_id, + ) + else: + log.error("Unknown publishable entity type: %s", entity) + continue + + +@shared_task(base=LoggedTask) +@set_code_owner_attribute +def check_container_content_changes( + container_key_str: str, + old_version_id: int | None, + new_version_id: int | None, +): + """ + Whenever a container is edited, we need to check if child entities were + added or removed, and if so send out a CONTENT_OBJECT_ASSOCIATIONS_CHANGED + event for each added/removed child. + + For example, removing an entity from a unit should result in:: + + CONTENT_OBJECT_ASSOCIATIONS_CHANGED( + object_id=..., + changes=["units"], + ) + + ⏳ This task is always run asynchronously. + """ + if old_version_id == new_version_id: + return # Same versions + + if old_version_id: + old_version = content_api.get_container_version(old_version_id) + old_entity_list_id = old_version.entity_list_id + else: + old_entity_list_id = None + if new_version_id: + new_version = content_api.get_container_version(new_version_id) + new_entity_list_id = new_version.entity_list_id + else: + new_entity_list_id = None + + old_child_ids: Iterable[PublishableEntity.ID] + new_child_ids: Iterable[PublishableEntity.ID] + # If the title has changed, we notify ALL children that their parent container(s) have changed, e.g. to update the + # list of "units this component is used in", "sections this subsection is used in", etc. in the search index + title_changed: bool = bool(old_version and new_version) and (old_version.title != new_version.title) + if title_changed: + # TODO: there is no "get entity list for container version" API in openedx_content + new_child_ids = new_version.entity_list.entitylistrow_set.values_list("entity_id", flat=True) + old_child_ids = old_version.entity_list.entitylistrow_set.values_list("entity_id", flat=True) + # notify ALL current children, plus any deleted children, that their parent container(s) changed + changed_child_ids = list(set(new_child_ids) | (set(old_child_ids) - set(new_child_ids))) + elif old_entity_list_id == new_entity_list_id: + # Different container versions but same list of child entities. For now we don't need to do anything, but in the + # future if we have some other kind of per-container settings relevant to child entities we might need to handle + # this the same way as title_changed. + return + else: + # TODO: there is no "get entity list for container version" API in openedx_content + old_child_ids = ( + old_version.entity_list.entitylistrow_set.values_list("entity_id", flat=True) if old_version else [] + ) + new_child_ids = ( + new_version.entity_list.entitylistrow_set.values_list("entity_id", flat=True) if new_version else [] + ) + # We only need to notify any added or removed children that their parent container(s) changed: + changed_child_ids = list(set(old_child_ids) ^ set(new_child_ids)) + + container_key = LibraryContainerLocator.from_string(container_key_str) + library = ContentLibrary.objects.get_by_key(container_key.lib_key) + entities = ( + content_api.get_publishable_entities(library.learning_package_id) + .filter(id__in=changed_child_ids) + .select_related("component", "container") + ) + for entity in entities: + child_key: LibraryUsageLocatorV2 | LibraryContainerLocator + if hasattr(entity, "component"): + child_key = api.library_component_usage_key(library.library_key, entity.component) + elif hasattr(entity, "container"): + child_key = api.library_container_locator(library.library_key, entity.container) + else: + log.error("Unknown publishable entity type: %s", entity) + continue + CONTENT_OBJECT_ASSOCIATIONS_CHANGED.send_event( + content_object=ContentObjectChangedData( + object_id=str(child_key), + changes=[container_key.container_type + "s"], # e.g. "units" + ), + ) + + +@shared_task(base=LoggedTask) +@set_code_owner_attribute +def send_collections_changed_events( + publishable_entity_ids: list[PublishableEntity.ID], + learning_package_id: LearningPackage.ID, + library_key_str: str, +): + """ + Sends a CONTENT_OBJECT_ASSOCIATIONS_CHANGED event for each modified library + entity in the given list, because their associated collections have changed. + + ⏳ This task is designed to be run asynchronously so it can handle many + entities, but you can also call it synchronously if you are only + processing a single entity. Handlers should be synchronous and fast, to + support the "update one item synchronously" use case, but can be async if + needed. + """ + library_key = LibraryLocatorV2.from_string(library_key_str) + entities = ( + content_api.get_publishable_entities(learning_package_id) + .filter(id__in=publishable_entity_ids) + .select_related("component", "container") + ) + + for entity in entities: + opaque_key: OpaqueKey + + if hasattr(entity, "component"): + opaque_key = api.library_component_usage_key(library_key, entity.component) + elif hasattr(entity, "container"): + opaque_key = api.library_container_locator(library_key, entity.container) + else: + log.error("Unknown publishable entity type: %s", entity) + continue + + # .. event_implemented_name: CONTENT_OBJECT_ASSOCIATIONS_CHANGED + # .. event_type: org.openedx.content_authoring.content.object.associations.changed.v1 + CONTENT_OBJECT_ASSOCIATIONS_CHANGED.send_event( + content_object=ContentObjectChangedData(object_id=str(opaque_key), changes=["collections"]), + ) + + +@shared_task(base=LoggedTask) +@set_code_owner_attribute +def send_events_after_publish(publish_log_id: 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. @@ -99,12 +320,11 @@ def send_events_after_publish(publish_log_pk: int, library_key_str: str) -> None event handlers like updating the search index may a while to complete in that case. """ - publish_log = PublishLog.objects.get(pk=publish_log_pk) + publish_log = PublishLog.objects.get(id=publish_log_id) library_key = LibraryLocatorV2.from_string(library_key_str) affected_entities = publish_log.records.select_related( "entity", "entity__container", "entity__container__container_type", "entity__component", ).all() - affected_containers: set[LibraryContainerLocator] = set() # Update anything that needs to be updated (e.g. search index): for record in affected_entities: @@ -118,198 +338,20 @@ def send_events_after_publish(publish_log_pk: int, library_key_str: str) -> None LIBRARY_BLOCK_PUBLISHED.send_event( library_block=LibraryBlockData(library_key=library_key, usage_key=usage_key) ) - # Publishing a container will auto-publish its children, but publishing a single component or all changes - # in the library will NOT usually include any parent containers. But we do need to notify listeners that the - # parent container(s) have changed, e.g. so the search index can update the "has_unpublished_changes" - try: - for parent_container in api.get_containers_contains_item(usage_key): - affected_containers.add(parent_container.container_key) - # TODO: should this be a CONTAINER_CHILD_PUBLISHED event instead of CONTAINER_PUBLISHED ? - except api.ContentLibraryBlockNotFound: - # The component has been deleted. - pass elif hasattr(record.entity, "container"): container_key = api.library_container_locator(library_key, record.entity.container) - affected_containers.add(container_key) - - try: - # We do need to notify listeners that the parent container(s) have changed, - # e.g. so the search index can update the "has_unpublished_changes" - for parent_container in api.get_containers_contains_item(container_key): - affected_containers.add(parent_container.container_key) - except api.ContentLibraryContainerNotFound: - # The deleted children remains in the entity, so, in this case, the container may not be found. - pass - else: - log.warning( - f"PublishableEntity {record.entity.pk} / {record.entity.entity_ref} " - "was modified during publish operation but is of unknown type." + # Note: this container may have been directly published, or perhaps one of its children was published and + # it hasn't technically changed. Such ancestors of published entities are still included in the publish log. + # .. event_implemented_name: LIBRARY_CONTAINER_PUBLISHED + # .. event_type: org.openedx.content_authoring.content_library.container.published.v1 + LIBRARY_CONTAINER_PUBLISHED.send_event( + library_container=LibraryContainerData(container_key=container_key) ) - - for container_key in affected_containers: - # .. event_implemented_name: LIBRARY_CONTAINER_PUBLISHED - # .. event_type: org.openedx.content_authoring.content_library.container.published.v1 - LIBRARY_CONTAINER_PUBLISHED.send_event( - library_container=LibraryContainerData(container_key=container_key) - ) - - -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, openedx_content 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() - - 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): - 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) - event = LIBRARY_BLOCK_UPDATED - if is_deleted: - event = LIBRARY_BLOCK_DELETED - elif is_undeleted: - event = LIBRARY_BLOCK_CREATED - - # .. event_implemented_name: LIBRARY_BLOCK_UPDATED - # .. event_type: org.openedx.content_authoring.library_block.updated.v1 - - # .. event_implemented_name: LIBRARY_BLOCK_DELETED - # .. event_type: org.openedx.content_authoring.library_block.deleted.v1 - - # .. event_implemented_name: LIBRARY_BLOCK_CREATED - # .. event_type: org.openedx.content_authoring.library_block.created.v1 - 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. - # TODO: we should be able to rewrite this to use the "side effects" functionality of the publishing API. - try: - for parent_container in api.get_containers_contains_item(usage_key): - updated_container_keys.add(parent_container.container_key) - except api.ContentLibraryBlockNotFound: - pass # The item 'usage_key' has been deleted. But shouldn't we still handle that? - - # 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) - 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.entity_ref} " "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 content_api.get_entity_collections( - record.entity.learning_package_id, record.entity.entity_ref, - ): - collection_key = api.library_collection_locator( - library_key=library_key, - collection_key=parent_collection.collection_code, - ) - affected_collection_keys.add(collection_key) - - for container_key in deleted_container_keys: - # .. event_implemented_name: LIBRARY_CONTAINER_DELETED - # .. event_type: org.openedx.content_authoring.content_library.container.deleted.v1 - 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.discard(container_key) - - for container_key in created_container_keys: - # .. event_implemented_name: LIBRARY_CONTAINER_CREATED - # .. event_type: org.openedx.content_authoring.content_library.container.created.v1 - LIBRARY_CONTAINER_CREATED.send_event( - library_container=LibraryContainerData(container_key=container_key) - ) - - for container_key in updated_container_keys: - # .. event_implemented_name: LIBRARY_CONTAINER_UPDATED - # .. event_type: org.openedx.content_authoring.content_library.container.updated.v1 - LIBRARY_CONTAINER_UPDATED.send_event( - library_container=LibraryContainerData(container_key=container_key) - ) - - for collection_key in affected_collection_keys: - # .. event_implemented_name: LIBRARY_COLLECTION_UPDATED - # .. event_type: org.openedx.content_authoring.content_library.collection.updated.v1 - LIBRARY_COLLECTION_UPDATED.send_event( - library_collection=LibraryCollectionData(collection_key=collection_key) - ) - - -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. - def _filter_child(store, usage_key, capa_type): diff --git a/openedx/core/djangoapps/content_libraries/tests/test_api.py b/openedx/core/djangoapps/content_libraries/tests/test_api.py index cc0564964fb8..99f2568d6114 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_api.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_api.py @@ -28,6 +28,8 @@ LIBRARY_CONTAINER_DELETED, LIBRARY_CONTAINER_UPDATED, ) +from openedx_events.testing import EventsIsolationMixin +from openedx_events.tooling import OpenEdxPublicSignal from user_tasks.models import UserTaskStatus from common.djangoapps.student.tests.factories import UserFactory @@ -37,13 +39,21 @@ from .base import ContentLibrariesRestApiTest -class ContentLibraryCollectionsTest(ContentLibrariesRestApiTest): +class ContentLibraryCollectionsTest(ContentLibrariesRestApiTest, EventsIsolationMixin): """ Tests for Content Library API collections methods. Same guidelines as ContentLibrariesTestCase. """ + @classmethod + def setUpClass(cls): + """Test setup""" + super().setUpClass() + # By default, errors thrown in signal handlers get suppressed. We want to see them though! + # https://github.com/openedx/openedx-events/issues/569 + cls.allow_send_events_failure(*(s.event_type for s in OpenEdxPublicSignal.all_events())) + def setUp(self) -> None: super().setUp() @@ -252,11 +262,12 @@ def test_update_library_collection_components_event(self) -> None: self.assertDictContainsEntries( event_receiver.call_args_list[0].kwargs, { - "signal": CONTENT_OBJECT_ASSOCIATIONS_CHANGED, - "sender": None, - "content_object": ContentObjectChangedData( - object_id=self.lib1_problem_block["id"], - changes=["collections"], + "signal": LIBRARY_COLLECTION_UPDATED, + "library_collection": LibraryCollectionData( + collection_key=api.library_collection_locator( + self.lib1.library_key, + collection_key="COL1", + ), ), }, ) @@ -264,9 +275,8 @@ def test_update_library_collection_components_event(self) -> None: event_receiver.call_args_list[1].kwargs, { "signal": CONTENT_OBJECT_ASSOCIATIONS_CHANGED, - "sender": None, "content_object": ContentObjectChangedData( - object_id=self.lib1_html_block["id"], + object_id=self.lib1_problem_block["id"], changes=["collections"], ), }, @@ -275,9 +285,8 @@ def test_update_library_collection_components_event(self) -> None: event_receiver.call_args_list[2].kwargs, { "signal": CONTENT_OBJECT_ASSOCIATIONS_CHANGED, - "sender": None, "content_object": ContentObjectChangedData( - object_id=self.unit1["id"], + object_id=self.lib1_html_block["id"], changes=["collections"], ), }, @@ -285,13 +294,10 @@ def test_update_library_collection_components_event(self) -> None: self.assertDictContainsEntries( event_receiver.call_args_list[3].kwargs, { - "signal": LIBRARY_COLLECTION_UPDATED, - "sender": None, - "library_collection": LibraryCollectionData( - collection_key=api.library_collection_locator( - self.lib1.library_key, - collection_key="COL1", - ), + "signal": CONTENT_OBJECT_ASSOCIATIONS_CHANGED, + "content_object": ContentObjectChangedData( + object_id=self.unit1["id"], + changes=["collections"], ), }, ) @@ -346,16 +352,14 @@ def test_set_library_component_collections(self) -> None: 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.collection_code, - ), - background=True, + self.lib2.library_key, collection_key=self.col2.collection_code + ) ), LibraryCollectionData( collection_key=api.library_collection_locator( - self.lib2.library_key, collection_key=self.col3.collection_code, - ), - background=True, - ) + self.lib2.library_key, collection_key=self.col3.collection_code + ) + ), } def test_delete_library_block(self) -> None: @@ -381,10 +385,8 @@ def test_delete_library_block(self) -> None: "sender": None, "library_collection": LibraryCollectionData( collection_key=api.library_collection_locator( - self.lib1.library_key, - collection_key=self.col1.collection_code, + self.lib1.library_key, collection_key=self.col1.collection_code ), - background=True, ), }, ) @@ -423,7 +425,6 @@ def test_delete_library_container(self) -> None: self.lib1.library_key, collection_key=self.col1.collection_code, ), - background=True, ), }, ) @@ -435,7 +436,7 @@ def test_delete_library_container(self) -> None: "library_container": LibraryContainerData( container_key=self.subsection1.container_key, background=False, - ) + ), }, ) @@ -503,19 +504,22 @@ def test_delete_library_block_when_component_does_not_exist(self) -> None: ) def test_restore_library_block(self) -> None: + usage_key = LibraryUsageLocatorV2.from_string(self.lib1_problem_block["id"]) api.update_library_collection_items( self.lib1.library_key, self.col1.collection_code, opaque_keys=[ - LibraryUsageLocatorV2.from_string(self.lib1_problem_block["id"]), + usage_key, LibraryUsageLocatorV2.from_string(self.lib1_html_block["id"]), ], ) + api.delete_library_block(usage_key) + event_receiver = mock.Mock() LIBRARY_COLLECTION_UPDATED.connect(event_receiver) - api.restore_library_block(LibraryUsageLocatorV2.from_string(self.lib1_problem_block["id"])) + api.restore_library_block(usage_key) assert event_receiver.call_count == 1 self.assertDictContainsEntries( @@ -528,7 +532,6 @@ def test_restore_library_block(self) -> None: self.lib1.library_key, collection_key=self.col1.collection_code, ), - background=True, ), }, ) @@ -555,6 +558,8 @@ def test_add_component_and_revert(self) -> None: collection_update_event_receiver = mock.Mock() LIBRARY_COLLECTION_UPDATED.connect(collection_update_event_receiver) + # Reverting the change essentially deletes the item, so we should get an event that the collection's contents + # have been updated. api.revert_changes(self.lib1.library_key) assert collection_update_event_receiver.call_count == 1 diff --git a/openedx/core/djangoapps/content_libraries/tests/test_events.py b/openedx/core/djangoapps/content_libraries/tests/test_events.py index bf4857a3cae7..c89905d7dd37 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_events.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_events.py @@ -1,7 +1,6 @@ """ Tests for openedx_content-based Content Libraries """ - from unittest import mock from django.db import transaction @@ -115,11 +114,23 @@ def expect_new_events(self, *expected_events: dict) -> None: found = True break if not found: - raise AssertionError(f"Event {expected} not found among actual events: {self.new_events}") + raise AssertionError(f"Event {expected} not found among actual events:\n{self.new_events_str}") if len(self.new_events) > 0: - raise AssertionError(f"Events were emitted but not expected: {self.new_events}") + raise AssertionError(f"Events were emitted but not expected:\n{self.new_events_str}") self.clear_events() + @property + def new_events_str(self) -> str: + """Friendly-ish string representation of self.new_events""" + simplified_events = [e.copy() for e in self.new_events] + for e in simplified_events: + if e["sender"] is None: + del e["sender"] + if e["from_event_bus"] is False: + del e["from_event_bus"] + del e["metadata"] + return "\n".join([str(e) for e in simplified_events]) + @skip_unless_cms class ContentLibrariesEventsTestCase(BaseEventsTestCase): @@ -451,21 +462,87 @@ def test_publish_container(self) -> None: self.lib1_key, LibraryUsageLocatorV2.from_string(html_block["id"]), ), }, - { # Not 100% sure we want this, but a PUBLISHED event is emitted for container 2 - # because one of its children's published versions has changed, so whether or - # not it contains unpublished changes may have changed and the search index - # may need to be updated. It is not actually published though. - # TODO: should this be a CONTAINER_CHILD_PUBLISHED event? + # No PUBLISHED event is emitted for container 2, because it doesn't have a published version yet. + # Publishing 'html_block' would have potentially affected it if container 2's published version had a + # reference to 'html_block', but it doesn't yet until we publish it. + ) + + # note that container 2 is still unpublished + c2_after = self._get_container(container2["id"]) + assert c2_after["has_unpublished_changes"] + + # publish container2 now: + self._publish_container(container2["id"]) + self.expect_new_events( + { # An event for container 1 being published: "signal": LIBRARY_CONTAINER_PUBLISHED, "library_container": LibraryContainerData( container_key=LibraryContainerLocator.from_string(container2["id"]), ), }, + { # An event for the html block in container 2 only: + "signal": LIBRARY_BLOCK_PUBLISHED, + "library_block": LibraryBlockData( + self.lib1_key, LibraryUsageLocatorV2.from_string(html_block2["id"]), + ), + }, ) - # note that container 2 is still unpublished - c2_after = self._get_container(container2["id"]) - assert c2_after["has_unpublished_changes"] + def test_publish_container_propagation(self) -> None: + """ + Test the events that get emitted when we publish the changes to an entity + that is used in multiple published containers + """ + # Create two containers and add the same component to both: + 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) + self._add_container_children(container1["id"], children_ids=[problem_block["id"]]) + self._add_container_children(container2["id"], children_ids=[problem_block["id"]]) + # Publish everything: + self._commit_library_changes(self.lib1_key) + + # clear event log after the initial mock data setup is complete: + self.clear_events() + + # Now modify the problem that's shared by both containers and publish the new version + self._set_library_block_olx(problem_block["id"], "UPDATED") + self.clear_events() # Clears the LIBRARY_BLOCK_UPDATED event + 2x LIBRARY_CONTAINER_UPDATED events + + # Now both containers have unpublished changes: + assert self._get_container(container1["id"])["has_unpublished_changes"] + assert self._get_container(container2["id"])["has_unpublished_changes"] + # Publish container1, which also published the shared problem component: + self._publish_container(container1["id"]) + # Now neither container has unpublished changes (even though we never touched container2): + assert self._get_container(container1["id"])["has_unpublished_changes"] is False + assert self._get_container(container2["id"])["has_unpublished_changes"] is False + + # And publish events were emitted: + self.expect_new_events( + # An event for the problem block in container 1 being indirectly published: + { + "signal": LIBRARY_BLOCK_PUBLISHED, + "library_block": LibraryBlockData( + self.lib1_key, LibraryUsageLocatorV2.from_string(problem_block["id"]), + ), + }, + # An event for container 1 being published *directly*: + { + "signal": LIBRARY_CONTAINER_PUBLISHED, + "library_container": LibraryContainerData( + container_key=LibraryContainerLocator.from_string(container1["id"]), + ), + }, + # And this time a PUBLISHED event should also be emitted for container2. + # It's published version hasn't changed, but its "contains unpublished changes" status has. + { + "signal": LIBRARY_CONTAINER_PUBLISHED, + "library_container": LibraryContainerData( + container_key=LibraryContainerLocator.from_string(container2["id"]), + ), + }, + ) def test_publish_child_container(self): """ @@ -502,7 +579,17 @@ def test_publish_child_container(self): container_key=LibraryContainerLocator.from_string(unit["id"]), ), }, - { # An event for parent (subsection): + # No PUBLISHED event is emitted for the subsection, because it doesn't have a published version yet. + ) + + # note that subsection is still unpublished + c2_after = self._get_container(subsection["id"]) + assert c2_after["has_unpublished_changes"] + + # Now publish the subsection + self._publish_container(subsection["id"]) + self.expect_new_events( + { # An event for the subsection being published: "signal": LIBRARY_CONTAINER_PUBLISHED, "library_container": LibraryContainerData( container_key=LibraryContainerLocator.from_string(subsection["id"]), @@ -510,9 +597,27 @@ def test_publish_child_container(self): }, ) - # note that subsection is still unpublished - c2_after = self._get_container(subsection["id"]) - assert c2_after["has_unpublished_changes"] + # Now rename the unit: + self._update_container(unit["id"], 'New Unit Display Name') + self.clear_events() + # Publish changes to the unit: + self._publish_container(unit["id"]) + self.expect_new_events( + { # An event for the unit being published: + "signal": LIBRARY_CONTAINER_PUBLISHED, + "library_container": LibraryContainerData( + container_key=LibraryContainerLocator.from_string(unit["id"]), + ), + }, + # And this time we DO get notified that the parent container is affected, because the unit is in its + # published version, and this publish affects the parent's "contains_unpublished_changes" status. + { + "signal": LIBRARY_CONTAINER_PUBLISHED, + "library_container": LibraryContainerData( + container_key=LibraryContainerLocator.from_string(subsection["id"]), + ), + }, + ) def test_restore_unit(self) -> None: """ @@ -543,13 +648,10 @@ def test_restore_unit(self) -> None: "signal": LIBRARY_CONTAINER_CREATED, "library_container": LibraryContainerData(container_key), }, - { - "signal": CONTENT_OBJECT_ASSOCIATIONS_CHANGED, - "content_object": ContentObjectChangedData( - object_id=str(container_key), - changes=["collections", "tags"], - ), - }, + # We used to emit CONTENT_OBJECT_ASSOCIATIONS_CHANGED here for the restored container, specifically noting + # that changes=["collections", "tags"], because deleted things may have collections+tags that are once + # again relevant when it is restored. However, the CREATED event should be sufficient for notifying of that. + # (Or should we emit CREATED+UPDATED to be extra sure?) ) def test_restore_unit_via_revert(self) -> None: @@ -611,11 +713,11 @@ def test_collection_crud(self) -> None: "library_collection": LibraryCollectionData(collection_key), }) - # Soft delete the collection. NOTE: at the moment, it's only possible to "soft delete" collections via - # the REST API, which sends an UPDATED event because the collection is now "disabled" but not deleted. + # Soft delete the collection. Whether we "soft" or "hard" delete, it sends a "DELETED" event. + # If we later restore it, it would send a "CREATED" event. self._soft_delete_collection(collection_key) self.expect_new_events({ - "signal": LIBRARY_COLLECTION_UPDATED, # UPDATED not DELETED. If we do a hard delete, it should be DELETED. + "signal": LIBRARY_COLLECTION_DELETED, "library_collection": LibraryCollectionData(collection_key), }) @@ -624,6 +726,22 @@ class ContentLibraryContainerEventsTest(BaseEventsTestCase): """ Event tests for container operations: signals emitted when components and containers are created, updated, deleted, and associated with one another. + + setUp() builds the following structure in lib1 (note that some entities + are shared across multiple parents, so this is a DAG, not a strict tree):: + + Section 1 Section 2 + ├── Subsection 1 ◄───────── (shared) ────────┴── Subsection 1 + │ ├── Unit 1 ◄────────────────┐ (shared) + │ │ ├── problem1 │ + │ │ └── html1 ◄──┐ │ + │ └── Unit 2 │(shared) │ + │ └── html1 ◄──┘ │ + └── Subsection 2 │ + └── Unit 1 ◄────────────────┘ + + Orphans (created but not attached to any parent): + Unit 3, problem2 """ def setUp(self) -> None: @@ -689,21 +807,38 @@ def setUp(self) -> None: def test_container_updated_when_component_deleted(self) -> None: api.delete_library_block(self.html_block_usage_key) self.expect_new_events( + # The block itself was deleted: { "signal": LIBRARY_BLOCK_DELETED, "library_block": LibraryBlockData(self.lib1_key, self.html_block_usage_key), }, + # That block was a child of two units, so both parent units are flagged as updated + # e.g. to update their "child_display_names" in the search index. { "signal": LIBRARY_CONTAINER_UPDATED, - "library_container": LibraryContainerData( - container_key=self.unit1.container_key, background=True, - ), + "library_container": LibraryContainerData(container_key=self.unit1.container_key), }, { "signal": LIBRARY_CONTAINER_UPDATED, - "library_container": LibraryContainerData( - container_key=self.unit2.container_key, background=True, - ), + "library_container": LibraryContainerData(container_key=self.unit2.container_key), + }, + # openedx_content also lists ancestor containers of the affected units as changed. + # We don't strictly need this at the moment, at least as far as keeping our search index updated. + { + "signal": LIBRARY_CONTAINER_UPDATED, + "library_container": LibraryContainerData(container_key=self.subsection1.container_key), + }, + { + "signal": LIBRARY_CONTAINER_UPDATED, + "library_container": LibraryContainerData(container_key=self.subsection2.container_key), + }, + { + "signal": LIBRARY_CONTAINER_UPDATED, + "library_container": LibraryContainerData(container_key=self.section1.container_key), + }, + { + "signal": LIBRARY_CONTAINER_UPDATED, + "library_container": LibraryContainerData(container_key=self.section2.container_key), }, ) @@ -713,49 +848,79 @@ def test_container_updated_when_component_restored(self) -> None: api.restore_library_block(self.html_block_usage_key) self.expect_new_events( + # Restoring the block re-creates it: { "signal": LIBRARY_BLOCK_CREATED, "library_block": LibraryBlockData(self.lib1_key, self.html_block_usage_key), }, + # We used to emit CONTENT_OBJECT_ASSOCIATIONS_CHANGED here for the restored block, specifically noting + # that changes=["collections", "tags", "units"], because deleted things may have collections+tags+containers + # that are once again relevant when it is restored. However, the CREATED event should be sufficient for + # notifying of that. (Or should we emit CREATED+UPDATED to be extra sure?) + # The restored block is a child of two units, so both parent units are flagged as updated: { - "signal": CONTENT_OBJECT_ASSOCIATIONS_CHANGED, - "content_object": ContentObjectChangedData( - object_id=str(self.html_block_usage_key), - changes=["collections", "tags", "units"], - ), + "signal": LIBRARY_CONTAINER_UPDATED, + "library_container": LibraryContainerData(container_key=self.unit1.container_key), }, { "signal": LIBRARY_CONTAINER_UPDATED, - "library_container": LibraryContainerData( - container_key=self.unit1.container_key, background=True, - ), + "library_container": LibraryContainerData(container_key=self.unit2.container_key), }, + # openedx_content also lists ancestor containers of the affected units as changed. + # We don't strictly need this at the moment, at least as far as keeping our search index updated. { "signal": LIBRARY_CONTAINER_UPDATED, - "library_container": LibraryContainerData( - container_key=self.unit2.container_key, background=True, - ), + "library_container": LibraryContainerData(container_key=self.subsection1.container_key), + }, + { + "signal": LIBRARY_CONTAINER_UPDATED, + "library_container": LibraryContainerData(container_key=self.subsection2.container_key), + }, + { + "signal": LIBRARY_CONTAINER_UPDATED, + "library_container": LibraryContainerData(container_key=self.section1.container_key), + }, + { + "signal": LIBRARY_CONTAINER_UPDATED, + "library_container": LibraryContainerData(container_key=self.section2.container_key), }, ) def test_container_updated_when_component_olx_updated(self) -> None: self._set_library_block_olx(self.html_block_usage_key, "Hello world!") self.expect_new_events( + # The block's OLX changed: { "signal": LIBRARY_BLOCK_UPDATED, "library_block": LibraryBlockData(self.lib1_key, self.html_block_usage_key), }, + # That block is used in two units, so both parent units are flagged as updated + # e.g. to update their "child_display_names" in the search index, if the child's name has changed. { "signal": LIBRARY_CONTAINER_UPDATED, - "library_container": LibraryContainerData( - container_key=self.unit1.container_key, background=True, - ), + "library_container": LibraryContainerData(container_key=self.unit1.container_key), }, { "signal": LIBRARY_CONTAINER_UPDATED, - "library_container": LibraryContainerData( - container_key=self.unit2.container_key, background=True, - ), + "library_container": LibraryContainerData(container_key=self.unit2.container_key), + }, + # openedx_content also lists ancestor containers of the affected units as changed. + # We don't strictly need this at the moment, at least as far as keeping our search index updated. + { + "signal": LIBRARY_CONTAINER_UPDATED, + "library_container": LibraryContainerData(container_key=self.subsection1.container_key), + }, + { + "signal": LIBRARY_CONTAINER_UPDATED, + "library_container": LibraryContainerData(container_key=self.subsection2.container_key), + }, + { + "signal": LIBRARY_CONTAINER_UPDATED, + "library_container": LibraryContainerData(container_key=self.section1.container_key), + }, + { + "signal": LIBRARY_CONTAINER_UPDATED, + "library_container": LibraryContainerData(container_key=self.section2.container_key), }, ) @@ -767,17 +932,33 @@ def test_container_updated_when_component_fields_updated(self) -> None: "signal": LIBRARY_BLOCK_UPDATED, "library_block": LibraryBlockData(self.lib1_key, self.html_block_usage_key), }, + # That block is used in two containers, so we expect events for them too: + # This is used e.g. to update "child_display_names" in the search index, if the child's name has changed. { "signal": LIBRARY_CONTAINER_UPDATED, - "library_container": LibraryContainerData( - container_key=self.unit1.container_key, background=True, - ), + "library_container": LibraryContainerData(container_key=self.unit1.container_key), }, { "signal": LIBRARY_CONTAINER_UPDATED, - "library_container": LibraryContainerData( - container_key=self.unit2.container_key, background=True, - ), + "library_container": LibraryContainerData(container_key=self.unit2.container_key), + }, + # openedx_content also lists and parent containers of affected containers as changed, and so on... + # We don't strictly need this at the moment, at least as far as keeping our search index updated. + { + "signal": LIBRARY_CONTAINER_UPDATED, + "library_container": LibraryContainerData(container_key=self.subsection1.container_key), + }, + { + "signal": LIBRARY_CONTAINER_UPDATED, + "library_container": LibraryContainerData(container_key=self.subsection2.container_key), + }, + { + "signal": LIBRARY_CONTAINER_UPDATED, + "library_container": LibraryContainerData(container_key=self.section1.container_key), + }, + { + "signal": LIBRARY_CONTAINER_UPDATED, + "library_container": LibraryContainerData(container_key=self.section2.container_key), }, ) @@ -786,10 +967,13 @@ def test_container_updated_when_component_fields_updated(self) -> None: def test_container_updated_when_unit_updated(self) -> None: self._update_container(self.unit1.container_key, 'New Unit Display Name') self.expect_new_events( + # We renamed this unit, so we get an UPDATED event for it: { "signal": LIBRARY_CONTAINER_UPDATED, "library_container": LibraryContainerData(container_key=self.unit1.container_key), }, + # We also get events for its parent containers + # e.g. to update their "child_display_names" in the search index. { "signal": LIBRARY_CONTAINER_UPDATED, "library_container": LibraryContainerData(container_key=self.subsection1.container_key), @@ -798,6 +982,18 @@ def test_container_updated_when_unit_updated(self) -> None: "signal": LIBRARY_CONTAINER_UPDATED, "library_container": LibraryContainerData(container_key=self.subsection2.container_key), }, + # openedx_content also lists ancestor containers of the affected unit as changed. + # We don't strictly need this at the moment, at least as far as keeping our search index updated. + { + "signal": LIBRARY_CONTAINER_UPDATED, + "library_container": LibraryContainerData(container_key=self.section1.container_key), + }, + { + "signal": LIBRARY_CONTAINER_UPDATED, + "library_container": LibraryContainerData(container_key=self.section2.container_key), + }, + # Finally, any child components receive a "units changed" notification + # e.g. to update the "units this component is used in" in the search index. { "signal": CONTENT_OBJECT_ASSOCIATIONS_CHANGED, "content_object": ContentObjectChangedData( @@ -815,10 +1011,13 @@ def test_container_updated_when_unit_updated(self) -> None: def test_container_updated_when_subsection_updated(self) -> None: self._update_container(self.subsection1.container_key, 'New Subsection Display Name') self.expect_new_events( + # We renamed this container, so we get an UPDATED event for it: { "signal": LIBRARY_CONTAINER_UPDATED, "library_container": LibraryContainerData(container_key=self.subsection1.container_key), }, + # We also get events for its parent containers + # e.g. to update their "child_display_names" in the search index { "signal": LIBRARY_CONTAINER_UPDATED, "library_container": LibraryContainerData(container_key=self.section1.container_key), @@ -827,6 +1026,8 @@ def test_container_updated_when_subsection_updated(self) -> None: "signal": LIBRARY_CONTAINER_UPDATED, "library_container": LibraryContainerData(container_key=self.section2.container_key), }, + # Finally, any child containers receive a "subsections changed" notification + # e.g. to update the "subsections this unit is used in" in the search index. { "signal": CONTENT_OBJECT_ASSOCIATIONS_CHANGED, "content_object": ContentObjectChangedData( @@ -865,10 +1066,10 @@ def test_container_updated_when_section_updated(self) -> None: ############################## Association change signals ################################## def test_associations_changed_when_component_removed(self) -> None: - html_block_1 = self._add_block_to_library(self.lib1_key, "html", "html3") + html_block_3 = self._add_block_to_library(self.lib1_key, "html", "html3") api.update_container_children( self.unit2.container_key, - [LibraryUsageLocatorV2.from_string(html_block_1["id"])], + [LibraryUsageLocatorV2.from_string(html_block_3["id"])], None, entities_action=content_api.ChildrenEntitiesAction.APPEND, ) @@ -876,7 +1077,7 @@ def test_associations_changed_when_component_removed(self) -> None: api.update_container_children( self.unit2.container_key, - [LibraryUsageLocatorV2.from_string(html_block_1["id"])], + [LibraryUsageLocatorV2.from_string(html_block_3["id"])], None, entities_action=content_api.ChildrenEntitiesAction.REMOVE, ) @@ -884,13 +1085,27 @@ def test_associations_changed_when_component_removed(self) -> None: { "signal": CONTENT_OBJECT_ASSOCIATIONS_CHANGED, "content_object": ContentObjectChangedData( - object_id=html_block_1["id"], changes=["units"], + object_id=html_block_3["id"], changes=["units"], ), }, { "signal": LIBRARY_CONTAINER_UPDATED, "library_container": LibraryContainerData(container_key=self.unit2.container_key), }, + # Because we removed html3 from unit2, the ancestor containers of unit2 are also emitted as changed. + # We don't strictly need this at the moment, at least as far as keeping our search index updated. + { + "signal": LIBRARY_CONTAINER_UPDATED, + "library_container": LibraryContainerData(container_key=self.subsection1.container_key), + }, + { + "signal": LIBRARY_CONTAINER_UPDATED, + "library_container": LibraryContainerData(container_key=self.section1.container_key), + }, + { + "signal": LIBRARY_CONTAINER_UPDATED, + "library_container": LibraryContainerData(container_key=self.section2.container_key), + }, ) def test_associations_changed_when_unit_removed(self) -> None: @@ -910,16 +1125,24 @@ def test_associations_changed_when_unit_removed(self) -> None: entities_action=content_api.ChildrenEntitiesAction.REMOVE, ) self.expect_new_events( + # unit4 was removed from subsection2, so we get a notification that "parent subsection(s) have changed": { "signal": CONTENT_OBJECT_ASSOCIATIONS_CHANGED, "content_object": ContentObjectChangedData( object_id=str(unit4.container_key), changes=["subsections"], ), }, + # We modified subsection2 by changing its list of children, so we get a CONTAINER_UPDATED event for it: { "signal": LIBRARY_CONTAINER_UPDATED, "library_container": LibraryContainerData(container_key=self.subsection2.container_key), }, + # Because subsection2 itself was changed, we get change notifications for its ancestors. + # We don't strictly need this at the moment, at least as far as keeping our search index updated. + { + "signal": LIBRARY_CONTAINER_UPDATED, + "library_container": LibraryContainerData(container_key=self.section1.container_key), + }, ) def test_associations_changed_when_subsection_removed(self) -> None: @@ -968,6 +1191,7 @@ def test_associations_changed_when_components_added(self) -> None: entities_action=content_api.ChildrenEntitiesAction.APPEND, ) self.expect_new_events( + # We added html4 and html4 to a new unit, so they get "parent unit(s) changed" events: { "signal": CONTENT_OBJECT_ASSOCIATIONS_CHANGED, "content_object": ContentObjectChangedData( @@ -980,13 +1204,29 @@ def test_associations_changed_when_components_added(self) -> None: object_id=html_block_2["id"], changes=["units"], ), }, + # We modified unit2 by changing its list of children, so we get a CONTAINER_UPDATED event for it: { "signal": LIBRARY_CONTAINER_UPDATED, "library_container": LibraryContainerData(container_key=self.unit2.container_key), }, + # Because the unit itself was changed, we get change notifications for its ancestors. + # We don't strictly need this at the moment, at least as far as keeping our search index updated. + { + "signal": LIBRARY_CONTAINER_UPDATED, + "library_container": LibraryContainerData(container_key=self.subsection1.container_key), + }, + { + "signal": LIBRARY_CONTAINER_UPDATED, + "library_container": LibraryContainerData(container_key=self.section1.container_key), + }, + { + "signal": LIBRARY_CONTAINER_UPDATED, + "library_container": LibraryContainerData(container_key=self.section2.container_key), + }, ) def test_associations_changed_when_units_added(self) -> None: + # Create "unit4" and "unit5" and add them to subsection2: unit4 = api.create_container(self.lib1_key, content_models.Unit, 'unit-4', 'Unit 4', None) unit5 = api.create_container(self.lib1_key, content_models.Unit, 'unit-5', 'Unit 5', None) self.clear_events() @@ -998,6 +1238,7 @@ def test_associations_changed_when_units_added(self) -> None: entities_action=content_api.ChildrenEntitiesAction.APPEND, ) self.expect_new_events( + # Each unit was added to a new subsection, so we get a "subsections changed" event for each: { "signal": CONTENT_OBJECT_ASSOCIATIONS_CHANGED, "content_object": ContentObjectChangedData( @@ -1010,10 +1251,16 @@ def test_associations_changed_when_units_added(self) -> None: object_id=str(unit5.container_key), changes=["subsections"], ), }, + # The subsection itself was updated (its list of children changed): { "signal": LIBRARY_CONTAINER_UPDATED, "library_container": LibraryContainerData(container_key=self.subsection2.container_key), }, + # And because the subsection itself was changed, we get change notifications for its ancestors. + { + "signal": LIBRARY_CONTAINER_UPDATED, + "library_container": LibraryContainerData(container_key=self.section1.container_key), + }, ) def test_associations_changed_when_subsections_added(self) -> None: @@ -1111,11 +1358,29 @@ def test_signal_emitted_when_set_block_olx_succeeds(self) -> None: usage_key=self.problem_block_usage_key, ), }, + # Since the problem is part of a unit, we also get LIBRARY_CONTAINER_UPDATED on the parent unit. + # This is used e.g. to update "child_display_names" in the search index, if the child's name has changed. { "signal": LIBRARY_CONTAINER_UPDATED, - "library_container": LibraryContainerData( - container_key=self.unit1.container_key, background=True, - ), + "library_container": LibraryContainerData(container_key=self.unit1.container_key), + }, + # openedx_content also lists and parent containers of affected containers as changed, and so on... + # We don't strictly need this at the moment, at least as far as keeping our search index updated. + { + "signal": LIBRARY_CONTAINER_UPDATED, + "library_container": LibraryContainerData(container_key=self.subsection1.container_key), + }, + { + "signal": LIBRARY_CONTAINER_UPDATED, + "library_container": LibraryContainerData(container_key=self.subsection2.container_key), + }, + { + "signal": LIBRARY_CONTAINER_UPDATED, + "library_container": LibraryContainerData(container_key=self.section1.container_key), + }, + { + "signal": LIBRARY_CONTAINER_UPDATED, + "library_container": LibraryContainerData(container_key=self.section2.container_key), }, ) diff --git a/openedx/core/djangoapps/xblock/learning_context/learning_context.py b/openedx/core/djangoapps/xblock/learning_context/learning_context.py index d3b4c8643a2f..22bd92701350 100644 --- a/openedx/core/djangoapps/xblock/learning_context/learning_context.py +++ b/openedx/core/djangoapps/xblock/learning_context/learning_context.py @@ -70,18 +70,3 @@ def definition_for_usage(self, usage_key, **kwargs): Retuns None if the usage key doesn't exist in this context. """ raise NotImplementedError - - def send_block_updated_event(self, usage_key): - """ - Send a "block updated" event for the block with the given usage_key in this context. - - usage_key: the UsageKeyV2 subclass used for this learning context - """ - - def send_container_updated_events(self, usage_key): - """ - Send "container updated" events for containers that contains the block with - the given usage_key in this context. - - usage_key: the UsageKeyV2 subclass used for this learning context - """ diff --git a/openedx/core/djangoapps/xblock/runtime/openedx_content_runtime.py b/openedx/core/djangoapps/xblock/runtime/openedx_content_runtime.py index 1da6f048b1cc..8c4996869324 100644 --- a/openedx/core/djangoapps/xblock/runtime/openedx_content_runtime.py +++ b/openedx/core/djangoapps/xblock/runtime/openedx_content_runtime.py @@ -312,11 +312,6 @@ def save_block(self, block): ) self.authored_data_store.mark_unchanged(block) - # Signal that we've modified this block - learning_context = get_learning_context_impl(usage_key) - learning_context.send_block_updated_event(usage_key) - learning_context.send_container_updated_events(usage_key) - def _get_component_from_usage_key(self, usage_key): """ Note that Components aren't ever really truly deleted, so this will diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index 71790d659ff7..6dcdb1f96219 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -840,7 +840,7 @@ openedx-calc==5.0.0 # via # -r requirements/edx/kernel.in # xblocks-contrib -openedx-core==0.44.0 +openedx-core @ git+https://github.com/openedx/openedx-core.git@braden/emit-events # via # -c requirements/constraints.txt # -r requirements/edx/kernel.in diff --git a/requirements/edx/development.txt b/requirements/edx/development.txt index 3ad4c94197fd..e7e5e9c37765 100644 --- a/requirements/edx/development.txt +++ b/requirements/edx/development.txt @@ -1394,7 +1394,7 @@ openedx-calc==5.0.0 # -r requirements/edx/doc.txt # -r requirements/edx/testing.txt # xblocks-contrib -openedx-core==0.44.0 +openedx-core @ git+https://github.com/openedx/openedx-core.git@braden/emit-events # via # -c requirements/constraints.txt # -r requirements/edx/doc.txt diff --git a/requirements/edx/doc.txt b/requirements/edx/doc.txt index b7f29a68f1df..473b3bf81fa2 100644 --- a/requirements/edx/doc.txt +++ b/requirements/edx/doc.txt @@ -1018,7 +1018,7 @@ openedx-calc==5.0.0 # via # -r requirements/edx/base.txt # xblocks-contrib -openedx-core==0.44.0 +openedx-core @ git+https://github.com/openedx/openedx-core.git@braden/emit-events # via # -c requirements/constraints.txt # -r requirements/edx/base.txt diff --git a/requirements/edx/testing.txt b/requirements/edx/testing.txt index 1e14484defc2..39da746771df 100644 --- a/requirements/edx/testing.txt +++ b/requirements/edx/testing.txt @@ -1065,7 +1065,7 @@ openedx-calc==5.0.0 # via # -r requirements/edx/base.txt # xblocks-contrib -openedx-core==0.44.0 +openedx-core @ git+https://github.com/openedx/openedx-core.git@braden/emit-events # via # -c requirements/constraints.txt # -r requirements/edx/base.txt diff --git a/xmodule/modulestore/tests/django_utils.py b/xmodule/modulestore/tests/django_utils.py index 2448c5d6fc21..0c8f4196f150 100644 --- a/xmodule/modulestore/tests/django_utils.py +++ b/xmodule/modulestore/tests/django_utils.py @@ -617,7 +617,22 @@ class ImmediateOnCommitMixin: Mixin for tests that want `on_commit` callbacks to run immediately, even under TestCase (which normally wraps tests in a transaction that never commits). - Especially useful when the test needs to execute an event that occurs after an `on_commit` + Especially useful when the test needs to execute an event that occurs after an `on_commit`. + + Note: this _may_ not work if code imports `from django.db.transaction import on_commit` + instead of using `from django.db import transaction` and `transaction.on_commit`. + + 🛑 For more realistic behavior, it's better to use Django's built-in helper + for this, as it still defers the callbacks until the end of the current + context block instead of running them immediately. + + ``` + with self.captureOnCommitCallbacks(execute=True): + do_something() + # on_commit hooks will run after this block ends. + ``` + + https://docs.djangoproject.com/en/6.0/topics/testing/tools/#django.test.TestCase.captureOnCommitCallbacks """ @classmethod