diff --git a/xmodule/library_tools.py b/xmodule/library_tools.py index 417f08c8e2fe..2843a9c42ed2 100644 --- a/xmodule/library_tools.py +++ b/xmodule/library_tools.py @@ -121,7 +121,7 @@ def trigger_duplication( ensure_cms("library_content block children may only be duplicated in a CMS context") if not isinstance(dest_block, LegacyLibraryContentBlock): raise ValueError(f"Can only duplicate children for library_content blocks, not {dest_block.tag} blocks.") - if source_block.scope_ids.usage_id.context_key != source_block.scope_ids.usage_id.context_key: + if source_block.scope_ids.usage_id.context_key != dest_block.scope_ids.usage_id.context_key: raise ValueError( "Cannot duplicate_children across different learning contexts " f"(source={source_block.scope_ids.usage_id}, dest={dest_block.scope_ids.usage_id})" @@ -131,10 +131,17 @@ def trigger_duplication( "Cannot duplicate_children across different source libraries or versions thereof " f"({source_block.source_library_key=}, {dest_block.source_library_key=})." ) - library_tasks.duplicate_children.delay( - user_id=self.user_id, - source_block_id=str(source_block.scope_ids.usage_id), - dest_block_id=str(dest_block.scope_ids.usage_id), + # TODO: This task runs synchronously to avoid a race with duplicate_block's + # enclosing store.bulk_operations(), which buffers writes thread-locally. + # An out-of-process Celery worker would not see the dest block and would + # produce a duplicate with no children. This mirrors the identical fix + # applied to sync_from_library in TNL-11339 / #34029. See #36544. + library_tasks.duplicate_children.apply( + kwargs=dict( + user_id=self.user_id, + source_block_id=str(source_block.scope_ids.usage_id), + dest_block_id=str(dest_block.scope_ids.usage_id), + ), ) def are_children_syncing(self, library_content_block: LegacyLibraryContentBlock) -> bool: diff --git a/xmodule/tests/test_library_tools.py b/xmodule/tests/test_library_tools.py index 1388797bf94b..21208bef692c 100644 --- a/xmodule/tests/test_library_tools.py +++ b/xmodule/tests/test_library_tools.py @@ -99,3 +99,80 @@ def test_update_children(self): self.tools.trigger_library_sync(content_block, library_version=None) content_block = self.store.get_item(content_block.location) assert len(content_block.children) == 1 + + def _make_library_content_with_child(self): + """Build a library + course + populated library_content block.""" + library = LibraryFactory.create(modulestore=self.store) + self.make_block("html", library, data="Hello from the library") + course = CourseFactory.create(modulestore=self.store) + source_lc = self.make_block( + "library_content", + course, + max_count=1, + source_library_id=str(library.location.library_key), + ) + self.tools.trigger_library_sync(source_lc, library_version=None) + return course, library, self.store.get_item(source_lc.location) + + def test_unit_trigger_duplication_does_not_enqueue_async_task(self): + """ + Unit test for bug #36544: trigger_duplication must invoke + duplicate_children.apply (synchronous), never .delay (async). + """ + course, library, source_lc = self._make_library_content_with_child() + dest_lc = self.make_block( + "library_content", + course, + max_count=1, + source_library_id=str(library.location.library_key), + source_library_version=source_lc.source_library_version, + ) + with mock.patch( + "openedx.core.djangoapps.content_libraries.tasks.duplicate_children.delay" + ) as mocked_delay, mock.patch( + "openedx.core.djangoapps.content_libraries.tasks.duplicate_children.apply" + ) as mocked_apply: + self.tools.trigger_duplication(source_block=source_lc, dest_block=dest_lc) + assert not mocked_delay.called, "must not enqueue an async task; see #36544" + assert mocked_apply.called + + def test_integration_trigger_duplication_inside_bulk_operations(self): + """ + Integration test for bug #36544: trigger_duplication must run in-process + so its writes land inside the enclosing store.bulk_operations() context, + producing a duplicate block whose children are visible immediately. + """ + course, library, source_lc = self._make_library_content_with_child() + assert len(source_lc.children) == 1 + + with self.store.bulk_operations(course.id): + dest_lc = self.make_block( + "library_content", + course, + max_count=1, + source_library_id=str(library.location.library_key), + source_library_version=source_lc.source_library_version, + ) + self.tools.trigger_duplication(source_block=source_lc, dest_block=dest_lc) + dest_lc_reloaded = self.store.get_item(dest_lc.location) + assert len(dest_lc_reloaded.children) == 1 + + def test_bug_36544_regression_cross_context_guard(self): + """ + Regression for bug #36544: the cross-learning-context guard was a + self-comparison (always False) and never fired. Verify that passing + source and dest in different courses now raises ValueError. + """ + course_a = CourseFactory.create(modulestore=self.store) + course_b = CourseFactory.create(modulestore=self.store) + library = LibraryFactory.create(modulestore=self.store) + source_lc = self.make_block( + "library_content", course_a, + source_library_id=str(library.location.library_key), + ) + dest_lc = self.make_block( + "library_content", course_b, + source_library_id=str(library.location.library_key), + ) + with self.assertRaisesRegex(ValueError, "different learning contexts"): + self.tools.trigger_duplication(source_block=source_lc, dest_block=dest_lc)