From cda7fe9a293b6dbe457255468d725c68de843e70 Mon Sep 17 00:00:00 2001 From: muhammad-ammar Date: Mon, 12 Mar 2018 16:20:45 +0500 Subject: [PATCH] update available translations retrieval interface EDUCATOR-2384 --- .../xmodule/video_module/transcripts_utils.py | 58 ++++++++----------- .../xmodule/video_module/video_handlers.py | 3 +- .../xmodule/video_module/video_module.py | 4 +- .../courseware/tests/test_video_handlers.py | 45 ++++++++++---- .../courseware/tests/test_video_mongo.py | 1 - .../mobile_api/video_outlines/serializers.py | 6 +- 6 files changed, 63 insertions(+), 54 deletions(-) diff --git a/common/lib/xmodule/xmodule/video_module/transcripts_utils.py b/common/lib/xmodule/xmodule/video_module/transcripts_utils.py index 348cb6ffc4..53d5ebbb9b 100644 --- a/common/lib/xmodule/xmodule/video_module/transcripts_utils.py +++ b/common/lib/xmodule/xmodule/video_module/transcripts_utils.py @@ -707,7 +707,7 @@ class VideoTranscriptsMixin(object): This is necessary for both VideoModule and VideoDescriptor. """ - def available_translations(self, transcripts, verify_assets=None, include_val_transcripts=None): + def available_translations(self, transcripts, verify_assets=None, is_bumper=False): """ Return a list of language codes for which we have transcripts. @@ -729,39 +729,27 @@ class VideoTranscriptsMixin(object): sub, other_langs = transcripts["sub"], transcripts["transcripts"] - # If we're not verifying the assets, we just trust our field values - if not verify_assets: - if other_langs: - translations = list(other_langs) + if verify_assets: + all_langs = dict(**other_langs) + if sub: + all_langs.update({'en': sub}) + + for language, filename in all_langs.iteritems(): + try: + # for bumper videos, transcripts are stored in content store only + if is_bumper: + get_transcript_for_video(self.location, filename, filename, language) + else: + get_transcript(self, language) + except NotFoundError: + continue + + translations.append(language) + else: + # If we're not verifying the assets, we just trust our field values + translations = list(other_langs) if not translations or sub: translations += ['en'] - return translations - - # If we've gotten this far, we're going to verify that the transcripts - # being referenced are actually either in the contentstore or in edx-val. - if include_val_transcripts: - translations = get_available_transcript_languages(edx_video_id=self.edx_video_id) - - if sub: # check if sjson exists for 'en'. - try: - Transcript.asset(self.location, sub, 'en') - except NotFoundError: - try: - Transcript.asset(self.location, None, None, sub) - except NotFoundError: - pass - else: - translations.append('en') - else: - translations.append('en') - - for lang in other_langs: - try: - Transcript.asset(self.location, None, None, other_langs[lang]) - except NotFoundError: - continue - - translations.append(lang) # to clean redundant language codes. return list(set(translations)) @@ -848,7 +836,7 @@ class VideoTranscriptsMixin(object): for language_code, transcript_file in transcripts.items() if transcript_file != '' } - # bumper transcripts are stored in content store so we don't need check them in val + # bumper transcripts are stored in content store so we don't need to include val transcripts if not is_bumper: transcript_languages = get_available_transcript_languages(edx_video_id=self.edx_video_id) # HACK Warning! this is temporary and will be removed once edx-val take over the @@ -893,6 +881,10 @@ def get_transcript_for_video(video_location, subs_id, file_name, language): """ Get video transcript from content store. + NOTE: Transcripts can be searched from content store by two ways: + 1. by an id(a.k.a subs_id) which will be used to construct transcript filename + 2. by providing transcript filename + Arguments: video_location (Locator): Video location subs_id (unicode): id for a transcript in content store diff --git a/common/lib/xmodule/xmodule/video_module/video_handlers.py b/common/lib/xmodule/xmodule/video_module/video_handlers.py index 529e525121..7c8f1560cf 100644 --- a/common/lib/xmodule/xmodule/video_module/video_handlers.py +++ b/common/lib/xmodule/xmodule/video_module/video_handlers.py @@ -360,11 +360,10 @@ class VideoStudentViewHandlers(object): mimetype ) elif dispatch.startswith('available_translations'): - feature_enabled = is_val_transcript_feature_enabled_for_course(self.course_id) and not is_bumper available_translations = self.available_translations( transcripts, verify_assets=True, - include_val_transcripts=feature_enabled + is_bumper=is_bumper ) if available_translations: response = Response(json.dumps(available_translations)) diff --git a/common/lib/xmodule/xmodule/video_module/video_module.py b/common/lib/xmodule/xmodule/video_module/video_module.py index acc0c006e9..c5815c93db 100644 --- a/common/lib/xmodule/xmodule/video_module/video_module.py +++ b/common/lib/xmodule/xmodule/video_module/video_module.py @@ -1097,9 +1097,7 @@ class VideoDescriptor(VideoFields, VideoTranscriptsMixin, VideoStudioViewHandler "file_size": 0, # File size is not relevant for external link } - feature_enabled = is_val_transcript_feature_enabled_for_course(self.runtime.course_id.for_branch(None)) - transcripts_info = self.get_transcripts_info() - available_translations = self.available_translations(transcripts_info, include_val_transcripts=feature_enabled) + available_translations = self.available_translations(self.get_transcripts_info()) transcripts = { lang: self.runtime.handler_url(self, 'transcript', 'download', query="lang=" + lang, thirdparty=True) for lang in available_translations diff --git a/lms/djangoapps/courseware/tests/test_video_handlers.py b/lms/djangoapps/courseware/tests/test_video_handlers.py index 7743ad5169..4829b0590d 100644 --- a/lms/djangoapps/courseware/tests/test_video_handlers.py +++ b/lms/djangoapps/courseware/tests/test_video_handlers.py @@ -243,7 +243,17 @@ class TestTranscriptAvailableTranslationsDispatch(TestVideo): response = self.item.transcript(request=request, dispatch='available_translations') self.assertEqual(json.loads(response.body), ['uk']) - def test_multiple_available_translations(self): + @patch('xmodule.video_module.transcripts_utils.get_video_transcript_content') + def test_multiple_available_translations(self, mock_get_video_transcript_content): + mock_get_video_transcript_content.return_value = { + 'content': json.dumps({ + "start": [10], + "end": [100], + "text": ["Hi, welcome to Edx."], + }), + 'file_name': 'edx.sjson' + } + good_sjson = _create_file(json.dumps(self.subs)) # Upload english transcript. @@ -253,12 +263,14 @@ class TestTranscriptAvailableTranslationsDispatch(TestVideo): _upload_file(self.srt_file, self.item_descriptor.location, os.path.split(self.srt_file.name)[1]) self.item.sub = _get_subs_id(good_sjson.name) + self.item.edx_video_id = 'an-edx-video-id' request = Request.blank('/available_translations') response = self.item.transcript(request=request, dispatch='available_translations') self.assertEqual(json.loads(response.body), ['en', 'uk']) @patch('openedx.core.djangoapps.video_config.models.VideoTranscriptEnabledFlag.feature_enabled', Mock(return_value=True)) + @patch('xmodule.video_module.transcripts_utils.get_video_transcript_content') @patch('xmodule.video_module.transcripts_utils.get_available_transcript_languages') @ddt.data( ( @@ -289,11 +301,19 @@ class TestTranscriptAvailableTranslationsDispatch(TestVideo): 'uk': True, 'ro': False, }, - ['en', 'uk', 'de'] + ['en', 'uk', 'de', 'ro'] ), ) @ddt.unpack - def test_val_available_translations(self, val_transcripts, sub, transcripts, result, mock_get_transcript_languages): + def test_val_available_translations( + self, + val_transcripts, + sub, + transcripts, + result, + mock_get_transcript_languages, + mock_get_video_transcript_content + ): """ Tests available translations with video component's and val's transcript languages while the feature is enabled. @@ -310,9 +330,18 @@ class TestTranscriptAvailableTranslationsDispatch(TestVideo): _upload_sjson_file(sjson_transcript, self.item_descriptor.location) sub = _get_subs_id(sjson_transcript.name) + mock_get_video_transcript_content.return_value = { + 'content': json.dumps({ + "start": [10], + "end": [100], + "text": ["Hi, welcome to Edx."], + }), + 'file_name': 'edx.sjson' + } mock_get_transcript_languages.return_value = val_transcripts self.item.transcripts = transcripts self.item.sub = sub + self.item.edx_video_id = 'an-edx-video-id' # Make request to available translations dispatch. request = Request.blank('/available_translations') response = self.item.transcript(request=request, dispatch='available_translations') @@ -373,16 +402,12 @@ class TestTranscriptAvailableTranslationsBumperDispatch(TestVideo): response = self.item.transcript(request=request, dispatch=self.dispatch) self.assertEqual(json.loads(response.body), [lang]) - @ddt.data(True, False) @patch('xmodule.video_module.transcripts_utils.get_available_transcript_languages') - @patch('openedx.core.djangoapps.video_config.models.VideoTranscriptEnabledFlag.feature_enabled') - def test_multiple_available_translations(self, feature_enabled, - mock_val_video_transcript_feature, mock_get_transcript_languages): + def test_multiple_available_translations(self, mock_get_transcript_languages): """ - Verify that the available translations dispatch works as expected for multiple translations with - or without enabling the edx-val video transcripts feature. + Verify that available translations dispatch works as expected for multiple + translations and returns both content store and edxval translations. """ - mock_val_video_transcript_feature.return_value = feature_enabled # Assuming that edx-val has German translation available for this video component. mock_get_transcript_languages.return_value = ['de'] en_translation = _create_srt_file() diff --git a/lms/djangoapps/courseware/tests/test_video_mongo.py b/lms/djangoapps/courseware/tests/test_video_mongo.py index 672f29c2da..a158986b21 100644 --- a/lms/djangoapps/courseware/tests/test_video_mongo.py +++ b/lms/djangoapps/courseware/tests/test_video_mongo.py @@ -1494,7 +1494,6 @@ class TestVideoDescriptorStudentViewJson(TestCase): ({'uk': 1, 'de': 1}, 'en-subs', ['de', 'en'], ['en', 'uk', 'de']), ) @ddt.unpack - @patch('openedx.core.djangoapps.video_config.models.VideoTranscriptEnabledFlag.feature_enabled', Mock(return_value=True)) @patch('xmodule.video_module.transcripts_utils.edxval_api.get_available_transcript_languages') def test_student_view_with_val_transcripts_enabled(self, transcripts, english_sub, val_transcripts, expected_transcripts, mock_get_transcript_languages): diff --git a/lms/djangoapps/mobile_api/video_outlines/serializers.py b/lms/djangoapps/mobile_api/video_outlines/serializers.py index d902d5aeba..22b40f32f9 100644 --- a/lms/djangoapps/mobile_api/video_outlines/serializers.py +++ b/lms/djangoapps/mobile_api/video_outlines/serializers.py @@ -216,12 +216,8 @@ def video_summary(video_profiles, course_id, video_descriptor, request, local_ca size = default_encoded_video.get('file_size', 0) # Transcripts... - feature_enabled = is_val_transcript_feature_enabled_for_course(course_id) transcripts_info = video_descriptor.get_transcripts_info() - transcript_langs = video_descriptor.available_translations( - transcripts=transcripts_info, - include_val_transcripts=feature_enabled - ) + transcript_langs = video_descriptor.available_translations(transcripts=transcripts_info) transcripts = { lang: reverse(