diff --git a/lms/djangoapps/courseware/tests/test_video_mongo.py b/lms/djangoapps/courseware/tests/test_video_mongo.py index bb4feb852d..ede041dc5e 100644 --- a/lms/djangoapps/courseware/tests/test_video_mongo.py +++ b/lms/djangoapps/courseware/tests/test_video_mongo.py @@ -1839,9 +1839,10 @@ class VideoBlockTest(TestCase, VideoBlockTestBase): val_transcript_language_code=val_transcript_language_code, val_transcript_provider=val_transcript_provider ) + xml_object = etree.fromstring(xml_data) id_generator = Mock() id_generator.target_course_id = "test_course_id" - video = self.descriptor.from_xml(xml_data, module_system, id_generator) + video = self.descriptor.parse_xml(xml_object, module_system, None, id_generator) assert video.edx_video_id == 'test_edx_video_id' video_data = get_video_info(video.edx_video_id) @@ -1887,13 +1888,14 @@ class VideoBlockTest(TestCase, VideoBlockTestBase): Test that importing a video with no video id, creates a new external video. """ xml_data = """""" + xml_object = etree.fromstring(xml_data) module_system = DummySystem(load_error_modules=True) id_generator = Mock() # Verify edx_video_id is empty before. assert self.descriptor.edx_video_id == '' - video = self.descriptor.from_xml(xml_data, module_system, id_generator) + video = self.descriptor.parse_xml(xml_object, module_system, None, id_generator) # Verify edx_video_id is populated after the import. assert video.edx_video_id != '' @@ -1923,6 +1925,7 @@ class VideoBlockTest(TestCase, VideoBlockTestBase): val_transcript_language_code=val_transcript_language_code, val_transcript_provider=val_transcript_provider ) + xml_object = etree.fromstring(xml_data) module_system = DummySystem(load_error_modules=True) id_generator = Mock() @@ -1940,7 +1943,7 @@ class VideoBlockTest(TestCase, VideoBlockTestBase): # Verify edx_video_id is empty before. assert self.descriptor.edx_video_id == '' - video = self.descriptor.from_xml(xml_data, module_system, id_generator) + video = self.descriptor.parse_xml(xml_object, module_system, None, id_generator) # Verify edx_video_id is populated after the import. assert video.edx_video_id != '' @@ -2077,11 +2080,12 @@ class VideoBlockTest(TestCase, VideoBlockTestBase): xml_data += val_transcripts xml_data += '' + xml_object = etree.fromstring(xml_data) # Verify edx_video_id is empty before import. assert self.descriptor.edx_video_id == '' - video = self.descriptor.from_xml(xml_data, module_system, id_generator) + video = self.descriptor.parse_xml(xml_object, module_system, None, id_generator) # Verify edx_video_id is not empty after import. assert video.edx_video_id != '' @@ -2107,8 +2111,9 @@ class VideoBlockTest(TestCase, VideoBlockTestBase): """ + xml_object = etree.fromstring(xml_data) with pytest.raises(ValCannotCreateError): - VideoBlock.from_xml(xml_data, module_system, id_generator=Mock()) + VideoBlock.parse_xml(xml_object, module_system, None, id_generator=Mock()) with pytest.raises(ValVideoNotFoundError): get_video_info("test_edx_video_id") diff --git a/xmodule/tests/test_video.py b/xmodule/tests/test_video.py index 79e2522ce2..6fcb118130 100644 --- a/xmodule/tests/test_video.py +++ b/xmodule/tests/test_video.py @@ -282,7 +282,7 @@ class VideoBlockImportTestCase(TestCase): 'transcripts': {'ua': 'ukrainian_translation.srt', 'ge': 'german_translation.srt'} }) - def test_from_xml(self): + def test_parse_xml(self): module_system = DummySystem(load_error_modules=True) xml_data = ''' ''' - output = VideoBlock.from_xml(xml_data, module_system, Mock()) + xml_object = etree.fromstring(xml_data) + output = VideoBlock.parse_xml(xml_object, module_system, None, Mock()) self.assert_attributes_equal(output, { 'youtube_id_0_75': 'izygArpw-Qo', 'youtube_id_1_0': 'p2Q6BrNhdh8', @@ -323,7 +324,7 @@ class VideoBlockImportTestCase(TestCase): ('test_org/test_course/test_run', '/c4x/test_org/test_course/asset/test.png') ) @ddt.unpack - def test_from_xml_when_handout_is_course_asset(self, course_id_string, expected_handout_link): + def test_parse_xml_when_handout_is_course_asset(self, course_id_string, expected_handout_link): """ Test that if handout link is course_asset then it will contain targeted course_id in handout link. """ @@ -344,10 +345,11 @@ class VideoBlockImportTestCase(TestCase): ''' + xml_object = etree.fromstring(xml_data) id_generator = Mock() id_generator.target_course_id = course_id - output = VideoBlock.from_xml(xml_data, module_system, id_generator) + output = VideoBlock.parse_xml(xml_object, module_system, None, id_generator) self.assert_attributes_equal(output, { 'youtube_id_0_75': 'izygArpw-Qo', 'youtube_id_1_0': 'p2Q6BrNhdh8', @@ -365,7 +367,7 @@ class VideoBlockImportTestCase(TestCase): 'transcripts': {'uk': 'ukrainian_translation.srt', 'de': 'german_translation.srt'}, }) - def test_from_xml_missing_attributes(self): + def test_parse_xml_missing_attributes(self): """ Ensure that attributes have the right values if they aren't explicitly set in XML. @@ -378,7 +380,8 @@ class VideoBlockImportTestCase(TestCase): ''' - output = VideoBlock.from_xml(xml_data, module_system, Mock()) + xml_object = etree.fromstring(xml_data) + output = VideoBlock.parse_xml(xml_object, module_system, None, Mock()) self.assert_attributes_equal(output, { 'youtube_id_0_75': '', 'youtube_id_1_0': 'p2Q6BrNhdh8', @@ -395,7 +398,7 @@ class VideoBlockImportTestCase(TestCase): 'data': '' }) - def test_from_xml_missing_download_track(self): + def test_parse_xml_missing_download_track(self): """ Ensure that attributes have the right values if they aren't explicitly set in XML. @@ -409,7 +412,8 @@ class VideoBlockImportTestCase(TestCase): ''' - output = VideoBlock.from_xml(xml_data, module_system, Mock()) + xml_object = etree.fromstring(xml_data) + output = VideoBlock.parse_xml(xml_object, module_system, None, Mock()) self.assert_attributes_equal(output, { 'youtube_id_0_75': '', 'youtube_id_1_0': 'p2Q6BrNhdh8', @@ -426,13 +430,14 @@ class VideoBlockImportTestCase(TestCase): 'transcripts': {}, }) - def test_from_xml_no_attributes(self): + def test_parse_xml_no_attributes(self): """ Make sure settings are correct if none are explicitly set in XML. """ module_system = DummySystem(load_error_modules=True) xml_data = '' - output = VideoBlock.from_xml(xml_data, module_system, Mock()) + xml_object = etree.fromstring(xml_data) + output = VideoBlock.parse_xml(xml_object, module_system, None, Mock()) self.assert_attributes_equal(output, { 'youtube_id_0_75': '', 'youtube_id_1_0': '3_yD_cEKoCk', @@ -450,7 +455,7 @@ class VideoBlockImportTestCase(TestCase): 'transcripts': {}, }) - def test_from_xml_double_quotes(self): + def test_parse_xml_double_quotes(self): """ Make sure we can handle the double-quoted string format (which was used for exporting for a few weeks). @@ -471,7 +476,8 @@ class VideoBlockImportTestCase(TestCase): youtube_id_1_0=""OEoXaMPEzf10"" /> ''' - output = VideoBlock.from_xml(xml_data, module_system, Mock()) + xml_object = etree.fromstring(xml_data) + output = VideoBlock.parse_xml(xml_object, module_system, None, Mock()) self.assert_attributes_equal(output, { 'youtube_id_0_75': 'OEoXaMPEzf65', 'youtube_id_1_0': 'OEoXaMPEzf10', @@ -488,14 +494,15 @@ class VideoBlockImportTestCase(TestCase): 'data': '' }) - def test_from_xml_double_quote_concatenated_youtube(self): + def test_parse_xml_double_quote_concatenated_youtube(self): module_system = DummySystem(load_error_modules=True) xml_data = ''' ''' - output = VideoBlock.from_xml(xml_data, module_system, Mock()) + xml_object = etree.fromstring(xml_data) + output = VideoBlock.parse_xml(xml_object, module_system, None, Mock()) self.assert_attributes_equal(output, { 'youtube_id_0_75': '', 'youtube_id_1_0': 'p2Q6BrNhdh8', @@ -528,7 +535,8 @@ class VideoBlockImportTestCase(TestCase): """ - output = VideoBlock.from_xml(xml_data, module_system, Mock()) + xml_object = etree.fromstring(xml_data) + output = VideoBlock.parse_xml(xml_object, module_system, None, Mock()) self.assert_attributes_equal(output, { 'youtube_id_0_75': 'izygArpw-Qo', 'youtube_id_1_0': 'p2Q6BrNhdh8', @@ -558,7 +566,8 @@ class VideoBlockImportTestCase(TestCase): """ - video = VideoBlock.from_xml(xml_data, module_system, Mock()) + xml_object = etree.fromstring(xml_data) + video = VideoBlock.parse_xml(xml_object, module_system, None, Mock()) self.assert_attributes_equal(video, { 'youtube_id_0_75': 'izygArpw-Qo', 'youtube_id_1_0': 'p2Q6BrNhdh8', @@ -588,7 +597,8 @@ class VideoBlockImportTestCase(TestCase): """ - video = VideoBlock.from_xml(xml_data, module_system, Mock()) + xml_object = etree.fromstring(xml_data) + video = VideoBlock.parse_xml(xml_object, module_system, None, Mock()) self.assert_attributes_equal(video, { 'youtube_id_0_75': 'izygArpw-Qo', 'youtube_id_1_0': 'p2Q6BrNhdh8', @@ -606,10 +616,10 @@ class VideoBlockImportTestCase(TestCase): @patch('xmodule.video_module.video_module.edxval_api') def test_import_val_data(self, mock_val_api): """ - Test that `from_xml` works method works as expected. + Test that `parse_xml` works method works as expected. """ def mock_val_import(xml, edx_video_id, resource_fs, static_dir, external_transcripts, course_id): - """Mock edxval.api.import_from_xml""" + """Mock edxval.api.import_parse_xml""" assert xml.tag == 'video_asset' assert dict(list(xml.items())) == {'mock_attr': ''} assert edx_video_id == 'test_edx_video_id' @@ -634,9 +644,10 @@ class VideoBlockImportTestCase(TestCase): """.format( edx_video_id=edx_video_id ) + xml_object = etree.fromstring(xml_data) id_generator = Mock() id_generator.target_course_id = 'test_course_id' - video = VideoBlock.from_xml(xml_data, module_system, id_generator) + video = VideoBlock.parse_xml(xml_object, module_system, None, id_generator) self.assert_attributes_equal(video, {'edx_video_id': edx_video_id}) mock_val_api.import_from_xml.assert_called_once_with( @@ -660,8 +671,9 @@ class VideoBlockImportTestCase(TestCase): """ + xml_object = etree.fromstring(xml_data) with pytest.raises(mock_val_api.ValCannotCreateError): - VideoBlock.from_xml(xml_data, module_system, id_generator=Mock()) + VideoBlock.parse_xml(xml_object, module_system, None, Mock()) class VideoExportTestCase(VideoBlockTestBase): diff --git a/xmodule/video_module/video_module.py b/xmodule/video_module/video_module.py index 7ee3b0c0f8..df27c2d7f8 100644 --- a/xmodule/video_module/video_module.py +++ b/xmodule/video_module/video_module.py @@ -624,8 +624,7 @@ class VideoBlock( def parse_xml_new_runtime(cls, node, runtime, keys): """ Implement the video block's special XML parsing requirements for the - new runtime only. For all other runtimes, use the existing XModule-style - methods like .from_xml(). + new runtime only. For all other runtimes, use .parse_xml(). """ video_block = runtime.construct_xblock_from_class(cls, keys) field_data = cls.parse_video_xml(node) @@ -638,28 +637,24 @@ class VideoBlock( return video_block @classmethod - def from_xml(cls, xml_data, system, id_generator): + def parse_xml(cls, node, runtime, _keys, id_generator): """ - Creates an instance of this descriptor from the supplied xml_data. - This may be overridden by subclasses - xml_data: A string of xml that will be translated into data and children for - this module - system: A DescriptorSystem for interacting with external resources - id_generator is used to generate course-specific urls and identifiers + Use `node` to construct a new block. + + See XmlMixin.parse_xml for the detailed description. """ - xml_object = etree.fromstring(xml_data) - url_name = xml_object.get('url_name', xml_object.get('slug')) + 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) - if is_pointer_tag(xml_object): - filepath = cls._format_filepath(xml_object.tag, name_to_pathname(url_name)) - xml_object = cls.load_file(filepath, system.resources_fs, usage_id) - system.parse_asides(xml_object, definition_id, usage_id, id_generator) - field_data = cls.parse_video_xml(xml_object, id_generator) + 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) kvs = InheritanceKeyValueStore(initial_values=field_data) field_data = KvsFieldData(kvs) - video = system.construct_xblock_from_class( + video = runtime.construct_xblock_from_class( cls, # We're loading a descriptor, so student_id is meaningless # We also don't have separate notions of definition and usage ids yet, @@ -668,10 +663,10 @@ class VideoBlock( field_data, ) - # Update VAL with info extracted from `xml_object` + # Update VAL with info extracted from `node` video.edx_video_id = video.import_video_info_into_val( - xml_object, - system.resources_fs, + node, + runtime.resources_fs, getattr(id_generator, 'target_course_id', None) )