Re-enable refreshing from library only on initial insertion of lib (#25347)

If the library content block is already in the course, then don't
refresh the children when we re-import it. This lets us address
TNL-7507 (Randomized Content Block Settings Lost in Course Import)
while still avoiding AA-310, where the IDs of the children for an
existing library_content block might be altered, losing student
user state.

When importing, we only copy the default values from content
in a library the first time that library_content block is created.
Future imports ignore what's in the library so as not to disrupt
course state. You can still update to the library via the Studio
UI for updating to the latest version of a library for this block.

cms/djangoapps/contentstore/views/tests/test_import_export.py

@@ -141,7 +141,6 @@ class ImportTestCase(CourseTestCase):
     """
     Unit tests for importing a course or Library
     """
-
     CREATE_USER = True
 
     def setUp(self):
@@ -985,44 +984,44 @@ class TestCourseExportImport(LibraryTestCase):
         dest_child = self.store.get_item(dest_child_location)
         self.assertEqual(source_child.display_name, dest_child.display_name)
 
-    # @ddt.data(True, False)
-    # def test_library_content_on_course_export_import(self, publish_item):
-    #     """
-    #     Verify that library contents in destination and source courses are same after importing
-    #     the source course into destination course.
-    #     """
-    #     self._setup_source_course_with_library_content(publish=publish_item)
-
-    #     # Create a course to import source course.
-    #     dest_course = CourseFactory.create(default_store=ModuleStoreEnum.Type.split)
-
-    #     # Export the source course.
-    #     export_course_to_xml(
-    #         self.store,
-    #         contentstore(),
-    #         self.source_course.location.course_key,
-    #         self.export_dir,
-    #         'exported_source_course',
-    #     )
-
-    #     # Now, import it back to dest_course.
-    #     import_course_from_xml(
-    #         self.store,
-    #         self.user.id,
-    #         self.export_dir,
-    #         ['exported_source_course'],
-    #         static_content_store=contentstore(),
-    #         target_id=dest_course.location.course_key,
-    #         load_error_modules=False,
-    #         raise_on_failure=True,
-    #         create_if_not_present=True,
-    #     )
-
-    #     self.assert_problem_display_names(
-    #         self.source_course.location,
-    #         dest_course.location,
-    #         publish_item
-    #     )
+    @ddt.data(True, False)
+    def test_library_content_on_course_export_import(self, publish_item):
+        """
+        Verify that library contents in destination and source courses are same after importing
+        the source course into destination course.
+        """
+        self._setup_source_course_with_library_content(publish=publish_item)
+
+        # Create a course to import source course.
+        dest_course = CourseFactory.create(default_store=ModuleStoreEnum.Type.split)
+
+        # Export the source course.
+        export_course_to_xml(
+            self.store,
+            contentstore(),
+            self.source_course.location.course_key,
+            self.export_dir,
+            'exported_source_course',
+        )
+
+        # Now, import it back to dest_course.
+        import_course_from_xml(
+            self.store,
+            self.user.id,
+            self.export_dir,
+            ['exported_source_course'],
+            static_content_store=contentstore(),
+            target_id=dest_course.location.course_key,
+            load_error_modules=False,
+            raise_on_failure=True,
+            create_if_not_present=True,
+        )
+
+        self.assert_problem_display_names(
+            self.source_course.location,
+            dest_course.location,
+            publish_item
+        )
 
 
 @ddt.ddt

common/lib/xmodule/xmodule/modulestore/xml_importer.py

@@ -813,6 +813,10 @@ def _update_and_import_module(
     fields = _update_module_references(module, source_course_id, dest_course_id)
     asides = module.get_asides() if isinstance(module, XModuleMixin) else None
 
+    if module.location.block_type == 'library_content':
+        with store.branch_setting(branch_setting=ModuleStoreEnum.Branch.published_only):
+            lib_content_block_already_published = store.has_item(module.location)
+
     block = store.import_xblock(
         user_id, dest_course_id, module.location.block_type,
         module.location.block_id, fields, runtime, asides=asides
@@ -823,25 +827,41 @@ def _update_and_import_module(
     # modulestore that is eventually going to store the data.
     # Ticket: https://openedx.atlassian.net/browse/PLAT-1046
 
-    ## Disabling library content children update during import as we saw it cause
-    ## an issue where the children were recalculated causing learner's to lose their
-    ## current state.
-    ## If this is brought back in, also uncomment the tests in
-    ## cms/djangoapps/contentstore/views/tests/test_import_export.py
-    # if block.location.block_type == 'library_content':
-    #     # if library exists, update source_library_version and children
-    #     # according to this existing library and library content block.
-    #     if store.get_library(block.source_library_key):
-    #         # Update library content block's children on draft branch
-    #         with store.branch_setting(branch_setting=ModuleStoreEnum.Branch.draft_preferred):
-    #             LibraryToolsService(store, user_id).update_children(
-    #                 block,
-    #                 version=block.source_library_version,
-    #             )
-
-    #         # Publish it if importing the course for branch setting published_only.
-    #         if store.get_branch_setting() == ModuleStoreEnum.Branch.published_only:
-    #             store.publish(block.location, user_id)
+    # Special case handling for library content blocks. The fact that this is
+    # in Modulestore code is _bad_ and breaks abstraction barriers, but is too
+    # much work to factor out at this point.
+    if block.location.block_type == 'library_content':
+        # If library exists, update source_library_version and children
+        # according to this existing library and library content block.
+        if store.get_library(block.source_library_key):
+            # If the library content block is already in the course, then don't
+            # refresh the children when we re-import it. This lets us address
+            # TNL-7507 (Randomized Content Block Settings Lost in Course Import)
+            # while still avoiding AA-310, where the IDs of the children for an
+            # existing library_content block might be altered, losing student
+            # user state.
+            #
+            # Note that while this method is run on import, it's also run when
+            # adding the library content from Studio for the first time.
+            #
+            # TLDR: When importing, we only copy the default values from content
+            # in a library the first time that library_content block is created.
+            # Future imports ignore what's in the library so as not to disrupt
+            # course state. You _can_ still update to the library via the Studio
+            # UI for updating to the latest version of a library for this block.
+            if lib_content_block_already_published:
+                return block
+
+            # Update library content block's children on draft branch
+            with store.branch_setting(branch_setting=ModuleStoreEnum.Branch.draft_preferred):
+                LibraryToolsService(store, user_id).update_children(
+                    block,
+                    version=block.source_library_version,
+                )
+
+            # Publish it if importing the course for branch setting published_only.
+            if store.get_branch_setting() == ModuleStoreEnum.Branch.published_only:
+                store.publish(block.location, user_id)
 
     return block