From 1d9ca333cf1d26c40c55fb4dfb4e394f6fa90808 Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Mon, 5 Jan 2026 16:35:31 -0500 Subject: [PATCH] fix: correct upstream field for migrated libraries (#37804) This is to fix an issue in the following common migration situation: 1. An existing course references content in a legacy content library. 2. The legacy content library is migrated to the new library system. 3. The user clicks on "Update reference" from the Randomized Content Block in the course. This action is supposed to update the children of the LibraryContentBlock (usually ProblemBlocks) so that the "upstream" attribute is set to point at the UsageKeys of the content in the new libraries they were migrated to. What was happening instead was that the upstream entries for these child blocks were left blank, breaking the upstream/sync connection and making it so that the courses did not receive any updates from the migrated libraries. There were two issues: 1. get_forwarding_for_blocks() was being called with the child UsageKeys in the course, when it should have been called with the v1 library usage keys instead (since those are the things being forwarded). 2. We were checking that the target_key was a v2 Library key, but really the upstream target_key is supposed to be a LibraryUsageLocatorV2, i.e. the key of the specific piece of content, not the library it ended up in. Note on testing: Although there were unit tests for the migration of legacy content libraries, there were not any unit tests for the migration of legacy library *blocks*. This commit adds a minimal test, which would have caught the bug we're fixing. It would be good to add more comprehensive testing unit testing for this part of the migration flow. --------- Co-authored-by: Kyle McCormick --- xmodule/library_content_block.py | 14 ++++--- xmodule/tests/test_library_content.py | 53 ++++++++++++++++++++++----- 2 files changed, 53 insertions(+), 14 deletions(-) diff --git a/xmodule/library_content_block.py b/xmodule/library_content_block.py index a57e4cd0e1..2f9e403b26 100644 --- a/xmodule/library_content_block.py +++ b/xmodule/library_content_block.py @@ -17,7 +17,7 @@ from gettext import gettext, ngettext import nh3 from django.core.exceptions import ObjectDoesNotExist, PermissionDenied -from opaque_keys.edx.locator import LibraryLocator, LibraryLocatorV2 +from opaque_keys.edx.locator import LibraryLocator, LibraryUsageLocatorV2 from web_fragments.fragment import Fragment from webob import Response from xblock.core import XBlock @@ -324,11 +324,15 @@ class LegacyLibraryContentBlock(ItemBankMixin, XModuleToXBlockMixin, XBlock): store = modulestore() with store.bulk_operations(self.course_id): children = self.get_children() - child_migrations = migrator_api.get_forwarding_for_blocks([child.usage_key for child in children]) - for child in children: - old_upstream_key, _ = self.runtime.modulestore.get_block_original_usage(child.usage_key) + # These are the v1 library item upstream UsageKeys + child_old_upstream_keys = [ + self.runtime.modulestore.get_block_original_usage(child.usage_key)[0] + for child in children + ] + child_migrations = migrator_api.get_forwarding_for_blocks(child_old_upstream_keys) + for child, old_upstream_key in zip(children, child_old_upstream_keys): upstream_migration = child_migrations.get(old_upstream_key) - if upstream_migration and isinstance(upstream_migration.target_key, LibraryLocatorV2): + if upstream_migration and isinstance(upstream_migration.target_key, LibraryUsageLocatorV2): child.upstream = str(upstream_migration.target_key) if upstream_migration.target_version_num: child.upstream_version = upstream_migration.target_version_num diff --git a/xmodule/tests/test_library_content.py b/xmodule/tests/test_library_content.py index 0f7807d61b..d1b0d4422b 100644 --- a/xmodule/tests/test_library_content.py +++ b/xmodule/tests/test_library_content.py @@ -736,9 +736,9 @@ class TestLibraryContentAnalytics(LegacyLibraryContentTest): ) @patch('xmodule.html_block.HtmlBlock.author_view', dummy_render, create=True) @patch('xmodule.x_module.ModuleStoreRuntime.applicable_aside_types', lambda self, block: []) -class TestMigratedLibraryContentRender(LegacyLibraryContentTest): +class TestLegacyLibraryContentBlockMigration(LegacyLibraryContentTest): """ - Rendering unit tests for LegacyLibraryContentBlock + Unit tests for LegacyLibraryContentBlock """ def setUp(self): @@ -747,16 +747,14 @@ class TestMigratedLibraryContentRender(LegacyLibraryContentTest): super().setUp() user = UserFactory() self._sync_lc_block_from_library() - self.organization = OrganizationFactory() - self.lib_key_v2 = LibraryLocatorV2.from_string( - f"lib:{self.organization.short_name}:test-key" - ) + self.organization = OrganizationFactory(short_name="myorg") + self.lib_key_v2 = LibraryLocatorV2.from_string("lib:myorg:mylib") lib_api.create_library( org=self.organization, - slug=self.lib_key_v2.slug, - title="Test Library", + slug="mylib", + title="My Test V2 Library", ) - self.library_v2 = lib_api.ContentLibrary.objects.get(slug=self.lib_key_v2.slug) + self.library_v2 = lib_api.ContentLibrary.objects.get(slug="mylib") api.start_migration_to_library( user=user, source_key=self.library.location.library_key, @@ -770,6 +768,43 @@ class TestMigratedLibraryContentRender(LegacyLibraryContentTest): # Migrate block self.lc_block.upgrade_to_v2_library(None, None) + def test_migration_of_fields(self): + """ + Test that the LC block migration correctly updates the metadata of the LC block and its children. + + This tests only the simplest state: The source lib has been migrated with forwarding, exactly once, + and the LC block has also been migrated. + + TODO(https://github.com/openedx/edx-platform/issues/37837): + It would be good to also test more cases, including: + * When migration occurs which is non-forwarding, it does *not* affect the childen of this block. + * When the library migration HAS happend but the LC block migration HASN'T YET, then the fields of + the block and its children will be unchanged, but the user will be prompted to upgrade. + * When some or all of the blocks already exist in the target library before the migration, then + the migration target versions will NOT all be 1, and the upstream_versions should reflect that. + * When the target library blocks have been edited and published AFTER the legacy library migration + but BEFORE the LC block migration, then executing the LC block migration will set upstream_version + based on the migration target versions, NOT the latest versions. + """ + assert self.lc_block.is_migrated_to_v2 is True + children = self.lc_block.get_children() + assert len(children) == len(self.lib_blocks) + # The children's legacy library blocks have been migrated to a V2 library. + # We expect that each child's `upstream` has been updated to point at + # the target of each library block's migration. + assert children[0].upstream == "lb:myorg:mylib:html:html_1" + assert children[1].upstream == "lb:myorg:mylib:html:html_2" + assert children[2].upstream == "lb:myorg:mylib:html:html_3" + assert children[3].upstream == "lb:myorg:mylib:html:html_4" + # We also expect that each child's `upstream_version` has been set to the + # version of the migrated library block at the time of its migration, which + # we are assuming is `1` (i.e., the first version, as the blocks did not + # previously exist in the target library). + assert children[0].upstream_version == 1 + assert children[1].upstream_version == 1 + assert children[2].upstream_version == 1 + assert children[3].upstream_version == 1 + def test_preview_view(self): """ Test preview view rendering """ assert len(self.lc_block.children) == len(self.lib_blocks)