From 3faa773bb9d1bfc72b36a5ed79f76f34c8b80727 Mon Sep 17 00:00:00 2001 From: Irtaza Akram Date: Mon, 1 Apr 2024 16:02:06 +0500 Subject: [PATCH 1/6] feat: add support for xblock 2 --- .github/workflows/unit-tests.yml | 1 + cms/djangoapps/contentstore/helpers.py | 4 +-- .../courseware/tests/test_video_mongo.py | 14 +++++----- .../xblock/runtime/learning_core_runtime.py | 2 +- .../core/djangoapps/xblock/runtime/runtime.py | 4 +-- requirements/edx/base.txt | 8 +++--- requirements/edx/development.txt | 8 +++--- requirements/edx/doc.txt | 8 +++--- requirements/edx/testing.txt | 8 +++--- xmodule/course_block.py | 4 +-- xmodule/discussion_block.py | 4 +-- xmodule/error_block.py | 4 +-- xmodule/modulestore/xml.py | 2 +- xmodule/raw_block.py | 2 +- xmodule/template_block.py | 2 +- xmodule/tests/test_discussion.py | 6 ++--- xmodule/tests/test_library_content.py | 4 +-- xmodule/tests/test_poll.py | 3 ++- xmodule/tests/test_randomize_block.py | 3 +-- xmodule/tests/test_split_test_block.py | 3 +-- xmodule/tests/test_video.py | 26 ++++++++++--------- xmodule/tests/test_word_cloud.py | 4 +-- xmodule/tests/xml/__init__.py | 1 + xmodule/video_block/video_block.py | 12 ++++----- xmodule/x_module.py | 10 +++---- xmodule/xml_block.py | 19 +++++--------- 26 files changed, 80 insertions(+), 86 deletions(-) diff --git a/.github/workflows/unit-tests.yml b/.github/workflows/unit-tests.yml index d08dc1aadc81..b3eab8f6ec86 100644 --- a/.github/workflows/unit-tests.yml +++ b/.github/workflows/unit-tests.yml @@ -12,6 +12,7 @@ jobs: if: (github.repository == 'edx/edx-platform-private') || (github.repository == 'openedx/edx-platform' && (startsWith(github.base_ref, 'open-release') == false)) runs-on: [ edx-platform-runner ] strategy: + fail-fast: false matrix: python-version: - "3.8" diff --git a/cms/djangoapps/contentstore/helpers.py b/cms/djangoapps/contentstore/helpers.py index 88771cfc3324..c824b9d6dcba 100644 --- a/cms/djangoapps/contentstore/helpers.py +++ b/cms/djangoapps/contentstore/helpers.py @@ -344,7 +344,7 @@ def _import_xml_node_to_parent( if not xblock_class.has_children: # No children to worry about. The XML may contain child nodes, but they're not XBlocks. - temp_xblock = xblock_class.parse_xml(node, runtime, keys, id_generator) + temp_xblock = xblock_class.parse_xml(node, runtime, keys) else: # We have to handle the children ourselves, because there are lots of complex interactions between # * the vanilla XBlock parse_xml() method, and its lack of API for "create and save a new XBlock" @@ -354,7 +354,7 @@ def _import_xml_node_to_parent( # serialization of a child block, in order. For blocks that don't support children, their XML content/nodes # could be anything (e.g. HTML, capa) node_without_children = etree.Element(node.tag, **node.attrib) - temp_xblock = xblock_class.parse_xml(node_without_children, runtime, keys, id_generator) + temp_xblock = xblock_class.parse_xml(node_without_children, runtime, keys) child_nodes = list(node) if xblock_class.has_children and temp_xblock.children: raise NotImplementedError("We don't yet support pasting XBlocks with children") diff --git a/lms/djangoapps/courseware/tests/test_video_mongo.py b/lms/djangoapps/courseware/tests/test_video_mongo.py index c1df491f47a4..4667fa941a6f 100644 --- a/lms/djangoapps/courseware/tests/test_video_mongo.py +++ b/lms/djangoapps/courseware/tests/test_video_mongo.py @@ -2027,7 +2027,8 @@ def test_import_val_data_internal(self): xml_object = etree.fromstring(xml_data) id_generator = Mock() id_generator.target_course_id = "test_course_id" - video = self.block.parse_xml(xml_object, module_system, None, id_generator) + module_system.id_generator = id_generator + video = self.block.parse_xml(xml_object, module_system, None) assert video.edx_video_id == 'test_edx_video_id' video_data = get_video_info(video.edx_video_id) @@ -2075,12 +2076,11 @@ def test_import_no_video_id(self): xml_data = """""" xml_object = etree.fromstring(xml_data) module_system = DummySystem(load_error_blocks=True) - id_generator = Mock() # Verify edx_video_id is empty before. assert self.block.edx_video_id == '' - video = self.block.parse_xml(xml_object, module_system, None, id_generator) + video = self.block.parse_xml(xml_object, module_system, None) # Verify edx_video_id is populated after the import. assert video.edx_video_id != '' @@ -2112,7 +2112,6 @@ def test_import_val_transcript(self): ) xml_object = etree.fromstring(xml_data) module_system = DummySystem(load_error_blocks=True) - id_generator = Mock() # Create static directory in import file system and place transcript files inside it. module_system.resources_fs.makedirs(EXPORT_IMPORT_STATIC_DIR, recreate=True) @@ -2128,7 +2127,7 @@ def test_import_val_transcript(self): # Verify edx_video_id is empty before. assert self.block.edx_video_id == '' - video = self.block.parse_xml(xml_object, module_system, None, id_generator) + video = self.block.parse_xml(xml_object, module_system, None) # Verify edx_video_id is populated after the import. assert video.edx_video_id != '' @@ -2218,7 +2217,6 @@ def test_import_val_transcript_priority(self, sub_id, external_transcripts, val_ language_code = 'en' module_system = DummySystem(load_error_blocks=True) - id_generator = Mock() # Create static directory in import file system and place transcript files inside it. module_system.resources_fs.makedirs(EXPORT_IMPORT_STATIC_DIR, recreate=True) @@ -2270,7 +2268,7 @@ def test_import_val_transcript_priority(self, sub_id, external_transcripts, val_ # Verify edx_video_id is empty before import. assert self.block.edx_video_id == '' - video = self.block.parse_xml(xml_object, module_system, None, id_generator) + video = self.block.parse_xml(xml_object, module_system, None) # Verify edx_video_id is not empty after import. assert video.edx_video_id != '' @@ -2298,7 +2296,7 @@ def test_import_val_data_invalid(self): """ xml_object = etree.fromstring(xml_data) with pytest.raises(ValCannotCreateError): - VideoBlock.parse_xml(xml_object, module_system, None, id_generator=Mock()) + VideoBlock.parse_xml(xml_object, module_system, None) with pytest.raises(ValVideoNotFoundError): get_video_info("test_edx_video_id") diff --git a/openedx/core/djangoapps/xblock/runtime/learning_core_runtime.py b/openedx/core/djangoapps/xblock/runtime/learning_core_runtime.py index f476e36c7f8e..2875c4b468d5 100644 --- a/openedx/core/djangoapps/xblock/runtime/learning_core_runtime.py +++ b/openedx/core/djangoapps/xblock/runtime/learning_core_runtime.py @@ -205,7 +205,7 @@ def get_block(self, usage_key, for_parent=None): # plus some minor additional lines of code as needed. block = block_class.parse_xml_new_runtime(xml_node, runtime=self, keys=keys) else: - block = block_class.parse_xml(xml_node, runtime=self, keys=keys, id_generator=None) + block = block_class.parse_xml(xml_node, runtime=self, keys=keys) # Update field data with parsed values. We can't call .save() because it will call save_block(), below. block.force_save_fields(block._get_fields_to_save()) # pylint: disable=protected-access diff --git a/openedx/core/djangoapps/xblock/runtime/runtime.py b/openedx/core/djangoapps/xblock/runtime/runtime.py index 09f5fa93a0e2..15e0f3f0617e 100644 --- a/openedx/core/djangoapps/xblock/runtime/runtime.py +++ b/openedx/core/djangoapps/xblock/runtime/runtime.py @@ -217,11 +217,11 @@ def applicable_aside_types(self, block: XBlock): """ Disable XBlock asides in this runtime """ return [] - def parse_xml_file(self, fileobj, id_generator=None): + def parse_xml_file(self, fileobj): # Deny access to the inherited method raise NotImplementedError("XML Serialization is only supported with BlockstoreXBlockRuntime") - def add_node_as_child(self, block, node, id_generator=None): + def add_node_as_child(self, block, node): """ Called by XBlock.parse_xml to treat a child node as a child block. """ diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index 233cc573ec6e..34da6a184a63 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -6,7 +6,7 @@ # -e git+https://github.com/anupdhabarde/edx-proctoring-proctortrack.git@31c6c9923a51c903ae83760ecbbac191363aa2a2#egg=edx_proctoring_proctortrack # via -r requirements/edx/github.in -acid-xblock==0.2.1 +acid-xblock==0.3.0 # via -r requirements/edx/kernel.in aiohttp==3.9.3 # via @@ -528,7 +528,7 @@ edx-rest-api-client==5.6.1 # edx-proctoring edx-search==3.8.2 # via -r requirements/edx/kernel.in -edx-sga==0.23.1 +edx-sga==0.24.1 # via -r requirements/edx/bundled.in edx-submissions==3.6.0 # via @@ -804,7 +804,7 @@ optimizely-sdk==4.1.1 # via # -c requirements/edx/../constraints.txt # -r requirements/edx/bundled.in -ora2==6.5.1 +ora2==6.6.1 # via -r requirements/edx/bundled.in packaging==23.2 # via @@ -1222,7 +1222,7 @@ webob==1.8.7 # xblock wrapt==1.16.0 # via -r requirements/edx/paver.txt -xblock[django]==1.10.0 +xblock[django]==2.0.0 # via # -r requirements/edx/kernel.in # acid-xblock diff --git a/requirements/edx/development.txt b/requirements/edx/development.txt index bece134fd5f4..cb6518778b1d 100644 --- a/requirements/edx/development.txt +++ b/requirements/edx/development.txt @@ -12,7 +12,7 @@ accessible-pygments==0.0.4 # via # -r requirements/edx/doc.txt # pydata-sphinx-theme -acid-xblock==0.2.1 +acid-xblock==0.3.0 # via # -r requirements/edx/doc.txt # -r requirements/edx/testing.txt @@ -825,7 +825,7 @@ edx-search==3.8.2 # via # -r requirements/edx/doc.txt # -r requirements/edx/testing.txt -edx-sga==0.23.1 +edx-sga==0.24.1 # via # -r requirements/edx/doc.txt # -r requirements/edx/testing.txt @@ -1333,7 +1333,7 @@ optimizely-sdk==4.1.1 # -c requirements/edx/../constraints.txt # -r requirements/edx/doc.txt # -r requirements/edx/testing.txt -ora2==6.5.1 +ora2==6.6.1 # via # -r requirements/edx/doc.txt # -r requirements/edx/testing.txt @@ -2185,7 +2185,7 @@ wrapt==1.16.0 # -r requirements/edx/doc.txt # -r requirements/edx/testing.txt # astroid -xblock[django]==1.10.0 +xblock[django]==2.0.0 # via # -r requirements/edx/doc.txt # -r requirements/edx/testing.txt diff --git a/requirements/edx/doc.txt b/requirements/edx/doc.txt index 90e4911e698d..bc5ac7cd0515 100644 --- a/requirements/edx/doc.txt +++ b/requirements/edx/doc.txt @@ -8,7 +8,7 @@ # via -r requirements/edx/base.txt accessible-pygments==0.0.4 # via pydata-sphinx-theme -acid-xblock==0.2.1 +acid-xblock==0.3.0 # via -r requirements/edx/base.txt aiohttp==3.9.3 # via @@ -607,7 +607,7 @@ edx-rest-api-client==5.6.1 # edx-proctoring edx-search==3.8.2 # via -r requirements/edx/base.txt -edx-sga==0.23.1 +edx-sga==0.24.1 # via -r requirements/edx/base.txt edx-submissions==3.6.0 # via @@ -942,7 +942,7 @@ optimizely-sdk==4.1.1 # via # -c requirements/edx/../constraints.txt # -r requirements/edx/base.txt -ora2==6.5.1 +ora2==6.6.1 # via -r requirements/edx/base.txt packaging==23.2 # via @@ -1488,7 +1488,7 @@ webob==1.8.7 # xblock wrapt==1.16.0 # via -r requirements/edx/base.txt -xblock[django]==1.10.0 +xblock[django]==2.0.0 # via # -r requirements/edx/base.txt # acid-xblock diff --git a/requirements/edx/testing.txt b/requirements/edx/testing.txt index 8819b1e48abb..8b0f800cc158 100644 --- a/requirements/edx/testing.txt +++ b/requirements/edx/testing.txt @@ -6,7 +6,7 @@ # -e git+https://github.com/anupdhabarde/edx-proctoring-proctortrack.git@31c6c9923a51c903ae83760ecbbac191363aa2a2#egg=edx_proctoring_proctortrack # via -r requirements/edx/base.txt -acid-xblock==0.2.1 +acid-xblock==0.3.0 # via -r requirements/edx/base.txt aiohttp==3.9.3 # via @@ -633,7 +633,7 @@ edx-rest-api-client==5.6.1 # edx-proctoring edx-search==3.8.2 # via -r requirements/edx/base.txt -edx-sga==0.23.1 +edx-sga==0.24.1 # via -r requirements/edx/base.txt edx-submissions==3.6.0 # via @@ -997,7 +997,7 @@ optimizely-sdk==4.1.1 # via # -c requirements/edx/../constraints.txt # -r requirements/edx/base.txt -ora2==6.5.1 +ora2==6.6.1 # via -r requirements/edx/base.txt packaging==23.2 # via @@ -1604,7 +1604,7 @@ wrapt==1.16.0 # via # -r requirements/edx/base.txt # astroid -xblock[django]==1.10.0 +xblock[django]==2.0.0 # via # -r requirements/edx/base.txt # acid-xblock diff --git a/xmodule/course_block.py b/xmodule/course_block.py index ae6d88b7b7b5..a74abdb3f617 100644 --- a/xmodule/course_block.py +++ b/xmodule/course_block.py @@ -1166,8 +1166,8 @@ def read_grading_policy(cls, paths, system): return policy_str @classmethod - def parse_xml(cls, node, runtime, keys, id_generator): - instance = super().parse_xml(node, runtime, keys, id_generator) + def parse_xml(cls, node, runtime, keys): + instance = super().parse_xml(node, runtime, keys) policy_dir = None url_name = node.get('url_name') diff --git a/xmodule/discussion_block.py b/xmodule/discussion_block.py index 943c6359dd16..89e573c07c83 100644 --- a/xmodule/discussion_block.py +++ b/xmodule/discussion_block.py @@ -233,7 +233,7 @@ def student_view_data(self): return {'topic_id': self.discussion_id} @classmethod - def parse_xml(cls, node, runtime, keys, id_generator): + def parse_xml(cls, node, runtime, keys): """ Parses OLX into XBlock. @@ -246,7 +246,7 @@ def parse_xml(cls, node, runtime, keys, id_generator): XBlock.parse_xml. Otherwise this method parses file in "discussion" folder (known as definition_xml), applies policy.json and updates fields accordingly. """ - block = super().parse_xml(node, runtime, keys, id_generator) + block = super().parse_xml(node, runtime, keys) cls._apply_metadata_and_policy(block, node, runtime) diff --git a/xmodule/error_block.py b/xmodule/error_block.py index 9ee1d47f6714..a3620be87688 100644 --- a/xmodule/error_block.py +++ b/xmodule/error_block.py @@ -178,13 +178,13 @@ def from_xml(cls, xml_data, system, id_generator, # pylint: disable=arguments-d return cls._construct(system, xml_data, error_msg, location=id_generator.create_definition('error')) @classmethod - def parse_xml(cls, node, runtime, keys, id_generator): # lint-amnesty, pylint: disable=unused-argument + def parse_xml(cls, node, runtime, keys): # lint-amnesty, pylint: disable=unused-argument """ Interpret the parsed XML in `node`, creating an XModuleDescriptor. """ # It'd be great to not reserialize and deserialize the xml xml = etree.tostring(node).decode('utf-8') - block = cls.from_xml(xml, runtime, id_generator) + block = cls.from_xml(xml, runtime, runtime.id_generator) return block def export_to_xml(self, resource_fs): diff --git a/xmodule/modulestore/xml.py b/xmodule/modulestore/xml.py index c4a4d66cbf64..738035b19438 100644 --- a/xmodule/modulestore/xml.py +++ b/xmodule/modulestore/xml.py @@ -259,7 +259,7 @@ def construct_xblock_from_class(self, cls, scope_ids, field_data=None, *args, ** # id_generator is ignored, because each ImportSystem is already local to # a course, and has it's own id_generator already in place - def add_node_as_child(self, block, node, id_generator): # lint-amnesty, pylint: disable=signature-differs + def add_node_as_child(self, block, node): # lint-amnesty, pylint: disable=signature-differs child_block = self.process_xml(etree.tostring(node)) block.children.append(child_block.scope_ids.usage_id) diff --git a/xmodule/raw_block.py b/xmodule/raw_block.py index 2b88e4a3a266..58b21795f282 100644 --- a/xmodule/raw_block.py +++ b/xmodule/raw_block.py @@ -78,7 +78,7 @@ def parse_xml_new_runtime(cls, node, runtime, keys): try: block = super().parse_xml_new_runtime(node, runtime, keys) except AttributeError: - block = super().parse_xml(node, runtime, keys, id_generator=None) + block = super().parse_xml(node, runtime, keys) block.data = data_field_value return block diff --git a/xmodule/template_block.py b/xmodule/template_block.py index c48137125318..e6e69742cfbf 100644 --- a/xmodule/template_block.py +++ b/xmodule/template_block.py @@ -130,7 +130,7 @@ class TranslateCustomTagBlock( # pylint: disable=abstract-method resources_dir = None @classmethod - def parse_xml(cls, node, runtime, _keys, _id_generator): + def parse_xml(cls, node, runtime, _keys): """ Transforms the xml_data from <$custom_tag attr="" attr=""/> to diff --git a/xmodule/tests/test_discussion.py b/xmodule/tests/test_discussion.py index 54e95e017e51..f50006d1b90e 100644 --- a/xmodule/tests/test_discussion.py +++ b/xmodule/tests/test_discussion.py @@ -69,7 +69,7 @@ def setUp(self): self.runtime_mock = mock.Mock(spec=Runtime) self.runtime_mock.construct_xblock_from_class = mock.Mock(side_effect=self._construct_xblock_mock) self.runtime_mock.get_policy = mock.Mock(return_value={}) - self.id_gen_mock = mock.Mock() + self.id_generator = mock.Mock() def _construct_xblock_mock(self, cls, keys): # pylint: disable=unused-argument """ @@ -102,7 +102,7 @@ def test_xblock_export_format(self, id_pair, category_pair, target_pair, patched patched_load_definition_xml.side_effect = Exception("Irrelevant") - block = DiscussionXBlock.parse_xml(node, self.runtime_mock, self.keys, self.id_gen_mock) + block = DiscussionXBlock.parse_xml(node, self.runtime_mock, self.keys) try: assert block.discussion_id == id_pair.value assert block.discussion_category == category_pair.value @@ -134,7 +134,7 @@ def test_legacy_export_format(self, id_pair, category_pair, target_pair, patched patched_load_definition_xml.return_value = (definition_node, "irrelevant") - block = DiscussionXBlock.parse_xml(node, self.runtime_mock, self.keys, self.id_gen_mock) + block = DiscussionXBlock.parse_xml(node, self.runtime_mock, self.keys) try: assert block.discussion_id == id_pair.value assert block.discussion_category == category_pair.value diff --git a/xmodule/tests/test_library_content.py b/xmodule/tests/test_library_content.py index fd4b97fd546e..e30e19922be5 100644 --- a/xmodule/tests/test_library_content.py +++ b/xmodule/tests/test_library_content.py @@ -195,7 +195,7 @@ def test_xml_export_import_cycle(self): # Now import it. olx_element = etree.fromstring(exported_olx) - imported_lc_block = LibraryContentBlock.parse_xml(olx_element, self.runtime, None, self.id_generator) + imported_lc_block = LibraryContentBlock.parse_xml(olx_element, self.runtime, None) self._verify_xblock_properties(imported_lc_block) @@ -219,7 +219,7 @@ def test_xml_import_with_comments(self): # Import the olx. olx_element = etree.fromstring(olx_with_comments) - imported_lc_block = LibraryContentBlock.parse_xml(olx_element, self.runtime, None, self.id_generator) + imported_lc_block = LibraryContentBlock.parse_xml(olx_element, self.runtime, None) self._verify_xblock_properties(imported_lc_block) diff --git a/xmodule/tests/test_poll.py b/xmodule/tests/test_poll.py index 6fdf3f4592ce..b9cb5d82502b 100644 --- a/xmodule/tests/test_poll.py +++ b/xmodule/tests/test_poll.py @@ -72,7 +72,8 @@ def test_poll_export_with_unescaped_characters_xml(self): ''' node = etree.fromstring(sample_poll_xml) - output = PollBlock.parse_xml(node, module_system, self.scope_ids, id_generator) + module_system.id_generator = id_generator + output = PollBlock.parse_xml(node, module_system, self.scope_ids) # Update the answer with invalid character. invalid_characters_poll_answer = output.answers[0] # Invalid less-than character. diff --git a/xmodule/tests/test_randomize_block.py b/xmodule/tests/test_randomize_block.py index 55529050e62f..deebdfe4f1d6 100644 --- a/xmodule/tests/test_randomize_block.py +++ b/xmodule/tests/test_randomize_block.py @@ -83,8 +83,7 @@ def test_xml_export_import_cycle(self): # Now import it. olx_element = etree.fromstring(exported_olx) - id_generator = Mock() - imported_randomize_block = RandomizeBlock.parse_xml(olx_element, runtime, None, id_generator) + imported_randomize_block = RandomizeBlock.parse_xml(olx_element, runtime, None) # Check the new XBlock has the same properties as the old one. assert imported_randomize_block.display_name == randomize_block.display_name diff --git a/xmodule/tests/test_split_test_block.py b/xmodule/tests/test_split_test_block.py index bfecce065631..1324d3a806ea 100644 --- a/xmodule/tests/test_split_test_block.py +++ b/xmodule/tests/test_split_test_block.py @@ -586,8 +586,7 @@ def test_xml_export_import_cycle(self): # Now import it. olx_element = lxml.etree.fromstring(exported_olx) - id_generator = Mock() - imported_split_test_block = SplitTestBlock.parse_xml(olx_element, runtime, None, id_generator) + imported_split_test_block = SplitTestBlock.parse_xml(olx_element, runtime, None) # Check the new XBlock has the same properties as the old one. assert imported_split_test_block.display_name == split_test_block.display_name diff --git a/xmodule/tests/test_video.py b/xmodule/tests/test_video.py index e5180843d7c6..c2c7ac084662 100644 --- a/xmodule/tests/test_video.py +++ b/xmodule/tests/test_video.py @@ -298,7 +298,7 @@ def test_parse_xml(self): ''' xml_object = etree.fromstring(xml_data) - output = VideoBlock.parse_xml(xml_object, module_system, None, Mock()) + output = VideoBlock.parse_xml(xml_object, module_system, None) self.assert_attributes_equal(output, { 'youtube_id_0_75': 'izygArpw-Qo', 'youtube_id_1_0': 'p2Q6BrNhdh8', @@ -347,7 +347,8 @@ def test_parse_xml_when_handout_is_course_asset(self, course_id_string, expected id_generator = Mock() id_generator.target_course_id = course_id - output = VideoBlock.parse_xml(xml_object, module_system, None, id_generator) + module_system.id_generator = id_generator + output = VideoBlock.parse_xml(xml_object, module_system, None) self.assert_attributes_equal(output, { 'youtube_id_0_75': 'izygArpw-Qo', 'youtube_id_1_0': 'p2Q6BrNhdh8', @@ -379,7 +380,7 @@ def test_parse_xml_missing_attributes(self): ''' xml_object = etree.fromstring(xml_data) - output = VideoBlock.parse_xml(xml_object, module_system, None, Mock()) + output = VideoBlock.parse_xml(xml_object, module_system, None) self.assert_attributes_equal(output, { 'youtube_id_0_75': '', 'youtube_id_1_0': 'p2Q6BrNhdh8', @@ -411,7 +412,7 @@ def test_parse_xml_missing_download_track(self): ''' xml_object = etree.fromstring(xml_data) - output = VideoBlock.parse_xml(xml_object, module_system, None, Mock()) + output = VideoBlock.parse_xml(xml_object, module_system, None) self.assert_attributes_equal(output, { 'youtube_id_0_75': '', 'youtube_id_1_0': 'p2Q6BrNhdh8', @@ -435,7 +436,7 @@ def test_parse_xml_no_attributes(self): module_system = DummySystem(load_error_blocks=True) xml_data = '' xml_object = etree.fromstring(xml_data) - output = VideoBlock.parse_xml(xml_object, module_system, None, Mock()) + output = VideoBlock.parse_xml(xml_object, module_system, None) self.assert_attributes_equal(output, { 'youtube_id_0_75': '', 'youtube_id_1_0': '3_yD_cEKoCk', @@ -475,7 +476,7 @@ def test_parse_xml_double_quotes(self): /> ''' xml_object = etree.fromstring(xml_data) - output = VideoBlock.parse_xml(xml_object, module_system, None, Mock()) + output = VideoBlock.parse_xml(xml_object, module_system, None) self.assert_attributes_equal(output, { 'youtube_id_0_75': 'OEoXaMPEzf65', 'youtube_id_1_0': 'OEoXaMPEzf10', @@ -500,7 +501,7 @@ def test_parse_xml_double_quote_concatenated_youtube(self): ''' xml_object = etree.fromstring(xml_data) - output = VideoBlock.parse_xml(xml_object, module_system, None, Mock()) + output = VideoBlock.parse_xml(xml_object, module_system, None) self.assert_attributes_equal(output, { 'youtube_id_0_75': '', 'youtube_id_1_0': 'p2Q6BrNhdh8', @@ -534,7 +535,7 @@ def test_old_video_format(self): """ xml_object = etree.fromstring(xml_data) - output = VideoBlock.parse_xml(xml_object, module_system, None, Mock()) + output = VideoBlock.parse_xml(xml_object, module_system, None) self.assert_attributes_equal(output, { 'youtube_id_0_75': 'izygArpw-Qo', 'youtube_id_1_0': 'p2Q6BrNhdh8', @@ -565,7 +566,7 @@ def test_old_video_data(self): """ xml_object = etree.fromstring(xml_data) - video = VideoBlock.parse_xml(xml_object, module_system, None, Mock()) + video = VideoBlock.parse_xml(xml_object, module_system, None) self.assert_attributes_equal(video, { 'youtube_id_0_75': 'izygArpw-Qo', 'youtube_id_1_0': 'p2Q6BrNhdh8', @@ -596,7 +597,7 @@ def test_import_with_float_times(self): """ xml_object = etree.fromstring(xml_data) - video = VideoBlock.parse_xml(xml_object, module_system, None, Mock()) + video = VideoBlock.parse_xml(xml_object, module_system, None) self.assert_attributes_equal(video, { 'youtube_id_0_75': 'izygArpw-Qo', 'youtube_id_1_0': 'p2Q6BrNhdh8', @@ -645,7 +646,8 @@ def mock_val_import(xml, edx_video_id, resource_fs, static_dir, external_transcr xml_object = etree.fromstring(xml_data) id_generator = Mock() id_generator.target_course_id = 'test_course_id' - video = VideoBlock.parse_xml(xml_object, module_system, None, id_generator) + module_system.id_generator = id_generator + video = VideoBlock.parse_xml(xml_object, module_system, None) self.assert_attributes_equal(video, {'edx_video_id': edx_video_id}) mock_val_api.import_from_xml.assert_called_once_with( @@ -671,7 +673,7 @@ def test_import_val_data_invalid(self, mock_val_api): """ xml_object = etree.fromstring(xml_data) with pytest.raises(mock_val_api.ValCannotCreateError): - VideoBlock.parse_xml(xml_object, module_system, None, Mock()) + VideoBlock.parse_xml(xml_object, module_system, None) class VideoExportTestCase(VideoBlockTestBase): diff --git a/xmodule/tests/test_word_cloud.py b/xmodule/tests/test_word_cloud.py index 79923d7c88fb..6c928465262b 100644 --- a/xmodule/tests/test_word_cloud.py +++ b/xmodule/tests/test_word_cloud.py @@ -41,8 +41,8 @@ def test_xml_import_export_cycle(self): ) olx_element = etree.fromstring(original_xml) - id_generator = Mock() - block = WordCloudBlock.parse_xml(olx_element, runtime, None, id_generator) + runtime.id_generator = Mock() + block = WordCloudBlock.parse_xml(olx_element, runtime, None) block.location = BlockUsageLocator( CourseLocator('org', 'course', 'run', branch='revision'), 'word_cloud', 'block_id' ) diff --git a/xmodule/tests/xml/__init__.py b/xmodule/tests/xml/__init__.py index 77ca1882f6a3..90545f1b88bb 100644 --- a/xmodule/tests/xml/__init__.py +++ b/xmodule/tests/xml/__init__.py @@ -40,6 +40,7 @@ def get_policy(usage_id): render_template=lambda template, context: pprint.pformat((template, context)), services={'field-data': KvsFieldData(DictKeyValueStore())}, ) + self.id_generator = Mock() def process_xml(self, xml): # pylint: disable=method-hidden """Parse `xml` as an XBlock, and add it to `self._blocks`""" diff --git a/xmodule/video_block/video_block.py b/xmodule/video_block/video_block.py index e28b47f56fc6..0df838f90c09 100644 --- a/xmodule/video_block/video_block.py +++ b/xmodule/video_block/video_block.py @@ -727,7 +727,7 @@ def parse_xml_new_runtime(cls, node, runtime, keys): return video_block @classmethod - def parse_xml(cls, node, runtime, _keys, id_generator): + def parse_xml(cls, node, runtime, _keys): """ Use `node` to construct a new block. @@ -735,13 +735,13 @@ def parse_xml(cls, node, runtime, _keys, id_generator): """ url_name = node.get('url_name') block_type = 'video' - definition_id = id_generator.create_definition(block_type, url_name) - usage_id = id_generator.create_usage(definition_id) + definition_id = runtime.id_generator.create_definition(block_type, url_name) + usage_id = runtime.id_generator.create_usage(definition_id) if is_pointer_tag(node): filepath = cls._format_filepath(node.tag, name_to_pathname(url_name)) node = cls.load_file(filepath, runtime.resources_fs, usage_id) - runtime.parse_asides(node, definition_id, usage_id, id_generator) - field_data = cls.parse_video_xml(node, id_generator) + runtime.parse_asides(node, definition_id, usage_id, runtime.id_generator) + field_data = cls.parse_video_xml(node, runtime.id_generator) kvs = InheritanceKeyValueStore(initial_values=field_data) field_data = KvsFieldData(kvs) video = runtime.construct_xblock_from_class( @@ -757,7 +757,7 @@ def parse_xml(cls, node, runtime, _keys, id_generator): video.edx_video_id = video.import_video_info_into_val( node, runtime.resources_fs, - getattr(id_generator, 'target_course_id', None) + getattr(runtime.id_generator, 'target_course_id', None) ) return video diff --git a/xmodule/x_module.py b/xmodule/x_module.py index 73588d7cf299..d25012275a5e 100644 --- a/xmodule/x_module.py +++ b/xmodule/x_module.py @@ -1529,18 +1529,16 @@ def __init__(self, process_xml, **kwargs): super().__init__(**kwargs) self.process_xml = process_xml - def _usage_id_from_node(self, node, parent_id, id_generator=None): + def _usage_id_from_node(self, node, parent_id): """Create a new usage id from an XML dom node. Args: node (lxml.etree.Element): The DOM node to interpret. parent_id: The usage ID of the parent block - id_generator (IdGenerator): The :class:`.IdGenerator` to use - for creating ids Returns: UsageKey: the usage key for the new xblock """ - return self.xblock_from_node(node, parent_id, id_generator).scope_ids.usage_id + return self.xblock_from_node(node, parent_id, self.id_generator).scope_ids.usage_id def xblock_from_node(self, node, parent_id, id_generator=None): """ @@ -1575,7 +1573,7 @@ def xblock_from_node(self, node, parent_id, id_generator=None): aside_children = self.parse_asides(node, def_id, usage_id, id_generator) asides_tags = [x.tag for x in aside_children] - block = block_class.parse_xml(node, self, keys, id_generator) + block = block_class.parse_xml(node, self, keys) self._convert_reference_fields_to_keys(block) # difference from XBlock.runtime block.parent = parent_id block.save() @@ -1599,7 +1597,7 @@ def parse_asides(self, node, def_id, usage_id, id_generator): aside_children.append(child) # now process them & remove them from the xml payload for child in aside_children: - self._aside_from_xml(child, def_id, usage_id, id_generator) + self._aside_from_xml(child, def_id, usage_id) node.remove(child) return aside_children diff --git a/xmodule/xml_block.py b/xmodule/xml_block.py index 00e396e4da04..cc118a49ef57 100644 --- a/xmodule/xml_block.py +++ b/xmodule/xml_block.py @@ -285,7 +285,7 @@ def apply_policy(cls, metadata, policy): metadata[attr] = value @classmethod - def parse_xml(cls, node, runtime, keys, id_generator): # pylint: disable=too-many-statements + def parse_xml(cls, node, runtime, keys): # pylint: disable=too-many-statements """ Use `node` to construct a new block. @@ -297,20 +297,15 @@ def parse_xml(cls, node, runtime, keys, id_generator): # pylint: disable=too-ma keys (:class:`.ScopeIds`): The keys identifying where this block will store its data. - id_generator (:class:`.IdGenerator`): An object that will allow the - runtime to generate correct definition and usage ids for - children of this block. - Returns (XBlock): The newly parsed XBlock """ from xmodule.modulestore.xml import ImportSystem # done here to avoid circular import - if id_generator is None: - id_generator = runtime.id_generator + if keys is None: # Passing keys=None is against the XBlock API but some platform tests do it. - def_id = id_generator.create_definition(node.tag, node.get('url_name')) - keys = ScopeIds(None, node.tag, def_id, id_generator.create_usage(def_id)) + def_id = runtime.id_generator.create_definition(node.tag, node.get('url_name')) + keys = ScopeIds(None, node.tag, def_id, runtime.id_generator.create_usage(def_id)) aside_children = [] # VS[compat] @@ -324,13 +319,13 @@ def parse_xml(cls, node, runtime, keys, id_generator): # pylint: disable=too-ma # new style: # read the actual definition file--named using url_name.replace(':','/') definition_xml, filepath = cls.load_definition_xml(node, runtime, keys.def_id) - aside_children = runtime.parse_asides(definition_xml, keys.def_id, keys.usage_id, id_generator) + aside_children = runtime.parse_asides(definition_xml, keys.def_id, keys.usage_id, runtime.id_generator) else: filepath = None definition_xml = node # Note: removes metadata. - definition, children = cls.load_definition(definition_xml, runtime, keys.def_id, id_generator) + definition, children = cls.load_definition(definition_xml, runtime, keys.def_id, runtime.id_generator) # VS[compat] # Make Ike's github preview links work in both old and new file layouts. @@ -399,7 +394,7 @@ def parse_xml_new_runtime(cls, node, runtime, keys): try: return super().parse_xml_new_runtime(node, runtime, keys) except AttributeError: - return super().parse_xml(node, runtime, keys, id_generator=None) + return super().parse_xml(node, runtime, keys) @classmethod def load_definition_xml(cls, node, runtime, def_id): From 834384c88eecf0c2fd3f32bfe054d0bf49660e87 Mon Sep 17 00:00:00 2001 From: Irtaza Akram Date: Mon, 1 Apr 2024 17:47:57 +0500 Subject: [PATCH 2/6] fix: runtim id_generator issue --- cms/djangoapps/contentstore/helpers.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/cms/djangoapps/contentstore/helpers.py b/cms/djangoapps/contentstore/helpers.py index c824b9d6dcba..935ce7f170b1 100644 --- a/cms/djangoapps/contentstore/helpers.py +++ b/cms/djangoapps/contentstore/helpers.py @@ -316,9 +316,9 @@ def _import_xml_node_to_parent( block_type = node.tag # Generate the new ID: - id_generator = ImportIdGenerator(parent_key.context_key) - def_id = id_generator.create_definition(block_type, slug_hint) - usage_id = id_generator.create_usage(def_id) + runtime.id_generator = ImportIdGenerator(parent_key.context_key) + def_id = runtime.id_generator.create_definition(block_type, slug_hint) + usage_id = runtime.id_generator.create_usage(def_id) keys = ScopeIds(None, block_type, def_id, usage_id) # parse_xml is a really messy API. We pass both 'keys' and 'id_generator' and, depending on the XBlock, either # one may be used to determine the new XBlock's usage key, and the other will be ignored. e.g. video ignores From acc86dd3fdf8fe7840f7f4ece99cf980d3c72d2f Mon Sep 17 00:00:00 2001 From: Irtaza Akram Date: Tue, 2 Apr 2024 14:19:34 +0500 Subject: [PATCH 3/6] fix: review changes --- .github/workflows/unit-tests.yml | 1 - xmodule/tests/test_discussion.py | 1 - 2 files changed, 2 deletions(-) diff --git a/.github/workflows/unit-tests.yml b/.github/workflows/unit-tests.yml index b3eab8f6ec86..d08dc1aadc81 100644 --- a/.github/workflows/unit-tests.yml +++ b/.github/workflows/unit-tests.yml @@ -12,7 +12,6 @@ jobs: if: (github.repository == 'edx/edx-platform-private') || (github.repository == 'openedx/edx-platform' && (startsWith(github.base_ref, 'open-release') == false)) runs-on: [ edx-platform-runner ] strategy: - fail-fast: false matrix: python-version: - "3.8" diff --git a/xmodule/tests/test_discussion.py b/xmodule/tests/test_discussion.py index f50006d1b90e..4bea706be51f 100644 --- a/xmodule/tests/test_discussion.py +++ b/xmodule/tests/test_discussion.py @@ -69,7 +69,6 @@ def setUp(self): self.runtime_mock = mock.Mock(spec=Runtime) self.runtime_mock.construct_xblock_from_class = mock.Mock(side_effect=self._construct_xblock_mock) self.runtime_mock.get_policy = mock.Mock(return_value={}) - self.id_generator = mock.Mock() def _construct_xblock_mock(self, cls, keys): # pylint: disable=unused-argument """ From 682a80adddfca953f86b8fb7464f06b6e9d34357 Mon Sep 17 00:00:00 2001 From: Irtaza Akram Date: Wed, 3 Apr 2024 16:03:56 +0500 Subject: [PATCH 4/6] fix: review changes --- cms/djangoapps/contentstore/helpers.py | 9 +++++++++ lms/djangoapps/courseware/tests/test_video_mongo.py | 7 +++---- xmodule/tests/test_import.py | 1 + xmodule/tests/test_poll.py | 4 +--- xmodule/tests/test_video.py | 8 ++------ 5 files changed, 16 insertions(+), 13 deletions(-) diff --git a/cms/djangoapps/contentstore/helpers.py b/cms/djangoapps/contentstore/helpers.py index f16198dcd943..a4ece6c85d59 100644 --- a/cms/djangoapps/contentstore/helpers.py +++ b/cms/djangoapps/contentstore/helpers.py @@ -319,6 +319,11 @@ def _import_xml_node_to_parent( parent_key = parent_xblock.scope_ids.usage_id block_type = node.tag + # Modulestore's IdGenerator here is SplitMongoIdManager which is assigned + # by CachingDescriptorSystem Runtime and since we need our custom ImportIdGenerator + # here we are temporaraliy swtiching it. + original_id_generator = runtime.id_generator + # Generate the new ID: runtime.id_generator = ImportIdGenerator(parent_key.context_key) def_id = runtime.id_generator.create_definition(block_type, slug_hint) @@ -360,6 +365,10 @@ def _import_xml_node_to_parent( node_without_children = etree.Element(node.tag, **node.attrib) temp_xblock = xblock_class.parse_xml(node_without_children, runtime, keys) child_nodes = list(node) + + # Restore the original id_generator + runtime.id_generator = original_id_generator + if xblock_class.has_children and temp_xblock.children: raise NotImplementedError("We don't yet support pasting XBlocks with children") temp_xblock.parent = parent_key diff --git a/lms/djangoapps/courseware/tests/test_video_mongo.py b/lms/djangoapps/courseware/tests/test_video_mongo.py index 4667fa941a6f..d45bb94816b9 100644 --- a/lms/djangoapps/courseware/tests/test_video_mongo.py +++ b/lms/djangoapps/courseware/tests/test_video_mongo.py @@ -2025,9 +2025,8 @@ def test_import_val_data_internal(self): val_transcript_provider=val_transcript_provider ) xml_object = etree.fromstring(xml_data) - id_generator = Mock() - id_generator.target_course_id = "test_course_id" - module_system.id_generator = id_generator + module_system.id_generator.target_course_id = "test_course_id" + video = self.block.parse_xml(xml_object, module_system, None) assert video.edx_video_id == 'test_edx_video_id' @@ -2035,7 +2034,7 @@ def test_import_val_data_internal(self): assert video_data['client_video_id'] == 'test_client_video_id' assert video_data['duration'] == 111.0 assert video_data['status'] == 'imported' - assert video_data['courses'] == [{id_generator.target_course_id: None}] + assert video_data['courses'] == [{module_system.id_generator.target_course_id: None}] assert video_data['encoded_videos'][0]['profile'] == 'mobile' assert video_data['encoded_videos'][0]['url'] == 'http://example.com/video' assert video_data['encoded_videos'][0]['file_size'] == 222 diff --git a/xmodule/tests/test_import.py b/xmodule/tests/test_import.py index 95feedeb9c76..3052a90acfb3 100644 --- a/xmodule/tests/test_import.py +++ b/xmodule/tests/test_import.py @@ -50,6 +50,7 @@ def __init__(self, load_error_blocks, library=False): mixins=(InheritanceMixin, XModuleMixin), services={'field-data': KvsFieldData(DictKeyValueStore())}, ) + self.id_generator = Mock() class BaseCourseTestCase(TestCase): diff --git a/xmodule/tests/test_poll.py b/xmodule/tests/test_poll.py index b9cb5d82502b..f265c97c6c12 100644 --- a/xmodule/tests/test_poll.py +++ b/xmodule/tests/test_poll.py @@ -62,8 +62,7 @@ def test_poll_export_with_unescaped_characters_xml(self): unescaped characters. """ module_system = DummySystem(load_error_blocks=True) - id_generator = Mock() - id_generator.target_course_id = self.xblock.course_id + module_system.id_generator.target_course_id = self.xblock.course_id sample_poll_xml = '''

How old are you?

@@ -72,7 +71,6 @@ def test_poll_export_with_unescaped_characters_xml(self): ''' node = etree.fromstring(sample_poll_xml) - module_system.id_generator = id_generator output = PollBlock.parse_xml(node, module_system, self.scope_ids) # Update the answer with invalid character. invalid_characters_poll_answer = output.answers[0] diff --git a/xmodule/tests/test_video.py b/xmodule/tests/test_video.py index c2c7ac084662..5e95f77082b1 100644 --- a/xmodule/tests/test_video.py +++ b/xmodule/tests/test_video.py @@ -344,10 +344,8 @@ def test_parse_xml_when_handout_is_course_asset(self, course_id_string, expected ''' xml_object = etree.fromstring(xml_data) - id_generator = Mock() - id_generator.target_course_id = course_id + module_system.id_generator.target_course_id = course_id - module_system.id_generator = id_generator output = VideoBlock.parse_xml(xml_object, module_system, None) self.assert_attributes_equal(output, { 'youtube_id_0_75': 'izygArpw-Qo', @@ -644,9 +642,7 @@ def mock_val_import(xml, edx_video_id, resource_fs, static_dir, external_transcr edx_video_id=edx_video_id ) xml_object = etree.fromstring(xml_data) - id_generator = Mock() - id_generator.target_course_id = 'test_course_id' - module_system.id_generator = id_generator + module_system.id_generator.target_course_id = 'test_course_id' video = VideoBlock.parse_xml(xml_object, module_system, None) self.assert_attributes_equal(video, {'edx_video_id': edx_video_id}) From 49f5a4f1e3e60a509e4daeab7aa27d83ec8cf3f6 Mon Sep 17 00:00:00 2001 From: Irtaza Akram Date: Thu, 4 Apr 2024 16:06:26 +0500 Subject: [PATCH 5/6] fix: pylint and unit test --- xmodule/tests/test_import.py | 3 ++- xmodule/tests/test_poll.py | 1 - 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/xmodule/tests/test_import.py b/xmodule/tests/test_import.py index 3052a90acfb3..0f9ab2e9377b 100644 --- a/xmodule/tests/test_import.py +++ b/xmodule/tests/test_import.py @@ -50,7 +50,6 @@ def __init__(self, load_error_blocks, library=False): mixins=(InheritanceMixin, XModuleMixin), services={'field-data': KvsFieldData(DictKeyValueStore())}, ) - self.id_generator = Mock() class BaseCourseTestCase(TestCase): @@ -360,7 +359,9 @@ def test_metadata_override_default(self): '''.format(due=course_due, org=ORG, course=COURSE, url_name=url_name) block = system.process_xml(start_xml) + print("\n\n\n\n\n", block.runtime.id_generator, "\n\n\n\n") child = block.get_children()[0] + print("\n\n\n\n\n", child, "\n\n\n\n\n") # pylint: disable=protected-access child._field_data.set(child, 'due', child_due) compute_inherited_metadata(block) diff --git a/xmodule/tests/test_poll.py b/xmodule/tests/test_poll.py index f265c97c6c12..68bddce43945 100644 --- a/xmodule/tests/test_poll.py +++ b/xmodule/tests/test_poll.py @@ -2,7 +2,6 @@ import json import unittest -from unittest.mock import Mock from opaque_keys.edx.keys import CourseKey from xblock.field_data import DictFieldData From d9458ceab887e1c5ed0dea1940348faef1bc73b8 Mon Sep 17 00:00:00 2001 From: Irtaza Akram Date: Thu, 4 Apr 2024 16:15:56 +0500 Subject: [PATCH 6/6] fix: remove print statements --- xmodule/tests/test_import.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/xmodule/tests/test_import.py b/xmodule/tests/test_import.py index 0f9ab2e9377b..95feedeb9c76 100644 --- a/xmodule/tests/test_import.py +++ b/xmodule/tests/test_import.py @@ -359,9 +359,7 @@ def test_metadata_override_default(self): '''.format(due=course_due, org=ORG, course=COURSE, url_name=url_name) block = system.process_xml(start_xml) - print("\n\n\n\n\n", block.runtime.id_generator, "\n\n\n\n") child = block.get_children()[0] - print("\n\n\n\n\n", child, "\n\n\n\n\n") # pylint: disable=protected-access child._field_data.set(child, 'due', child_due) compute_inherited_metadata(block)