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 <[email protected]>

Author: ormsbee

xmodule/library_content_block.py

@@ -17,7 +17,7 @@
 
 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 @@ def v2_update_children_upstream_version(self, user_id=None):
         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

xmodule/tests/test_library_content.py

@@ -736,9 +736,9 @@ def test_removed_invalid(self):
 )
 @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 @@ def setUp(self):
         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 @@ def setUp(self):
         # 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)