From 0c61b0d4e2bdb51bfaa754b4f1f5201beb36e1e0 Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Wed, 21 Oct 2020 10:49:32 -0400 Subject: [PATCH] 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. --- .../views/tests/test_import_export.py | 69 +++++++++---------- .../xmodule/modulestore/xml_importer.py | 56 ++++++++++----- 2 files changed, 72 insertions(+), 53 deletions(-) diff --git a/cms/djangoapps/contentstore/views/tests/test_import_export.py b/cms/djangoapps/contentstore/views/tests/test_import_export.py index ab73cdf42a..292fb71326 100644 --- a/cms/djangoapps/contentstore/views/tests/test_import_export.py +++ b/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) + @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) + # 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', - # ) + # 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, - # ) + # 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 - # ) + self.assert_problem_display_names( + self.source_course.location, + dest_course.location, + publish_item + ) @ddt.ddt diff --git a/common/lib/xmodule/xmodule/modulestore/xml_importer.py b/common/lib/xmodule/xmodule/modulestore/xml_importer.py index ee45c4bcce..482153f37f 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml_importer.py +++ b/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, - # ) + # 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 - # # 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) + # 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