From 1203eb5835c0a9dad129edd2e2d5d4d81985e59c Mon Sep 17 00:00:00 2001 From: noraiz-anwar Date: Fri, 18 Aug 2017 15:11:21 +0500 Subject: [PATCH] Do not override Default Video URL with value from VAL --- .../xmodule/video_module/video_module.py | 18 +++-- .../courseware/tests/test_video_mongo.py | 79 ++++++++++++------- 2 files changed, 62 insertions(+), 35 deletions(-) diff --git a/common/lib/xmodule/xmodule/video_module/video_module.py b/common/lib/xmodule/xmodule/video_module/video_module.py index f8271473e2..f93f54f50e 100644 --- a/common/lib/xmodule/xmodule/video_module/video_module.py +++ b/common/lib/xmodule/xmodule/video_module/video_module.py @@ -722,7 +722,6 @@ class VideoDescriptor(VideoFields, VideoTranscriptsMixin, VideoStudioViewHandler }) source_url = self.create_youtube_url(youtube_id_1_0['value']) - # First try a lookup in VAL. If any video encoding is found given the video id then # override the source_url with it. if self.edx_video_id and edxval_api: @@ -734,15 +733,18 @@ class VideoDescriptor(VideoFields, VideoTranscriptsMixin, VideoStudioViewHandler # Get video encodings for val profiles. val_video_encodings = edxval_api.get_urls_for_profiles(self.edx_video_id, val_profiles) - # If multiple encodings are there in val, the priority will be: youtube > hls > mp4 and webm. + # VAL's youtube source has greater priority over external youtube source. if val_video_encodings.get('youtube'): source_url = self.create_youtube_url(val_video_encodings['youtube']) - elif val_video_encodings.get('hls'): - source_url = val_video_encodings['hls'] - elif val_video_encodings.get('desktop_mp4'): - source_url = val_video_encodings['desktop_mp4'] - elif val_video_encodings.get('desktop_webm'): - source_url = val_video_encodings['desktop_webm'] + + # If no youtube source is provided externally or in VAl, update source_url in order: hls > mp4 and webm + if not source_url: + if val_video_encodings.get('hls'): + source_url = val_video_encodings['hls'] + elif val_video_encodings.get('desktop_mp4'): + source_url = val_video_encodings['desktop_mp4'] + elif val_video_encodings.get('desktop_webm'): + source_url = val_video_encodings['desktop_webm'] # Only add if html5 sources do not already contain source_url. if source_url and source_url not in video_url['value']: diff --git a/lms/djangoapps/courseware/tests/test_video_mongo.py b/lms/djangoapps/courseware/tests/test_video_mongo.py index 6f13abc44f..01fed42433 100644 --- a/lms/djangoapps/courseware/tests/test_video_mongo.py +++ b/lms/djangoapps/courseware/tests/test_video_mongo.py @@ -1085,46 +1085,28 @@ class TestVideoDescriptorInitialization(BaseTestXmodule): @ddt.data( ( { - 'desktop_webm': 'https://webm.com/dw.webm', - 'hls': 'https://hls.com/hls.m3u8', 'youtube': 'v0TFmdO4ZP0', - 'desktop_mp4': 'https://mp4.com/dm.mp4' + 'hls': 'https://hls.com/hls.m3u8', + 'desktop_mp4': 'https://mp4.com/dm.mp4', + 'desktop_webm': 'https://webm.com/dw.webm', }, ['https://www.youtube.com/watch?v=v0TFmdO4ZP0'] ), ( { - 'desktop_webm': 'https://webm.com/dw.webm', + 'youtube': None, 'hls': 'https://hls.com/hls.m3u8', - 'youtube': None, - 'desktop_mp4': 'https://mp4.com/dm.mp4' - }, - ['https://hls.com/hls.m3u8'] - ), - ( - { + 'desktop_mp4': 'https://mp4.com/dm.mp4', 'desktop_webm': 'https://webm.com/dw.webm', - 'hls': None, - 'youtube': None, - 'desktop_mp4': 'https://mp4.com/dm.mp4' }, - ['https://mp4.com/dm.mp4'] + ['https://www.youtube.com/watch?v=3_yD_cEKoCk'] ), ( { - 'desktop_webm': 'https://webm.com/dw.webm', - 'hls': None, 'youtube': None, - 'desktop_mp4': None - }, - ['https://webm.com/dw.webm'] - ), - ( - { + 'hls': None, + 'desktop_mp4': None, 'desktop_webm': None, - 'hls': None, - 'youtube': None, - 'desktop_mp4': None }, ['https://www.youtube.com/watch?v=3_yD_cEKoCk'] ), @@ -1135,6 +1117,12 @@ class TestVideoDescriptorInitialization(BaseTestXmodule): """ Tests that the val encodings correctly override the video url when the edx video id is set and one or more encodings are present. + Accepted order of source priority is: + VAL's youtube source > external youtube source > hls > mp4 > webm. + + Note that `https://www.youtube.com/watch?v=3_yD_cEKoCk` is the default youtube source with which + a video component is initialized. Current implementation considers this youtube source as a valid + external youtube source. """ with patch('xmodule.video_module.video_module.edxval_api.get_urls_for_profiles') as get_urls_for_profiles: get_urls_for_profiles.return_value = val_video_encodings @@ -1142,7 +1130,44 @@ class TestVideoDescriptorInitialization(BaseTestXmodule): data='' ) context = self.item_descriptor.get_context() - self.assertEqual(context['transcripts_basic_tab_metadata']['video_url']['value'], video_url) + self.assertEqual(context['transcripts_basic_tab_metadata']['video_url']['value'], video_url) + + @ddt.data( + ( + { + 'youtube': None, + 'hls': 'https://hls.com/hls.m3u8', + 'desktop_mp4': 'https://mp4.com/dm.mp4', + 'desktop_webm': 'https://webm.com/dw.webm', + }, + ['https://hls.com/hls.m3u8'] + ), + ( + { + 'youtube': 'v0TFmdO4ZP0', + 'hls': 'https://hls.com/hls.m3u8', + 'desktop_mp4': None, + 'desktop_webm': 'https://webm.com/dw.webm', + }, + ['https://www.youtube.com/watch?v=v0TFmdO4ZP0'] + ), + ) + @ddt.unpack + @patch('xmodule.video_module.video_module.HLSPlaybackEnabledFlag.feature_enabled', Mock(return_value=True)) + def test_val_encoding_in_context_without_external_youtube_source(self, val_video_encodings, video_url): + """ + Tests that the val encodings correctly override the video url when the edx video id is set and + one or more encodings are present. In this scenerio no external youtube source is provided. + Accepted order of source priority is: + VAL's youtube source > external youtube source > hls > mp4 > webm. + """ + with patch('xmodule.video_module.video_module.edxval_api.get_urls_for_profiles') as get_urls_for_profiles: + get_urls_for_profiles.return_value = val_video_encodings + self.initialize_module( + data='' + ) + context = self.item_descriptor.get_context() + self.assertEqual(context['transcripts_basic_tab_metadata']['video_url']['value'], video_url) @attr(shard=1)