From d0cd247d085d9f22c32299412a7f8307d3f6261d Mon Sep 17 00:00:00 2001 From: muhammad-ammar Date: Thu, 15 Mar 2018 11:56:54 +0500 Subject: [PATCH 01/18] Deprecate Contentstore for Video Component Preview LMS/CMS EDUCATOR-1756 --- .../contentstore/views/transcripts_ajax.py | 36 +---- .../lib/xmodule/xmodule/tests/test_video.py | 15 +- .../xmodule/video_module/transcripts_utils.py | 7 +- .../xmodule/video_module/video_handlers.py | 140 +++++++++--------- .../xmodule/video_module/video_module.py | 5 +- .../courseware/tests/test_video_handlers.py | 51 +++---- .../courseware/tests/test_video_mongo.py | 15 -- .../mobile_api/video_outlines/serializers.py | 2 +- .../mobile_api/video_outlines/views.py | 2 +- 9 files changed, 116 insertions(+), 157 deletions(-) diff --git a/cms/djangoapps/contentstore/views/transcripts_ajax.py b/cms/djangoapps/contentstore/views/transcripts_ajax.py index 1c44a23391..6eafaaba46 100644 --- a/cms/djangoapps/contentstore/views/transcripts_ajax.py +++ b/cms/djangoapps/contentstore/views/transcripts_ajax.py @@ -39,6 +39,7 @@ from xmodule.video_module.transcripts_utils import ( Transcript, TranscriptsRequestValidationException, youtube_video_transcript_name, + get_transcript, ) from xmodule.video_module.transcripts_model_utils import ( is_val_transcript_feature_enabled_for_course @@ -148,9 +149,7 @@ def download_transcripts(request): Raises Http404 if unsuccessful. """ - locator = request.GET.get('locator') - subs_id = request.GET.get('subs_id') - if not locator: + if not request.GET.get('locator'): log.debug('GET data without "locator" property.') raise Http404 @@ -165,37 +164,16 @@ def download_transcripts(request): raise Http404 try: - if not subs_id: - raise NotFoundError - - filename = subs_id - content_location = StaticContent.compute_location( - item.location.course_key, - 'subs_{filename}.srt.sjson'.format(filename=filename), + content, filename, mimetype = get_transcript( + item, + lang=u'en', ) - input_format = Transcript.SJSON - transcript_content = contentstore().find(content_location).data except NotFoundError: - # Try searching in VAL for the transcript as a last resort - transcript = None - if is_val_transcript_feature_enabled_for_course(item.location.course_key): - transcript = get_video_transcript_content(edx_video_id=item.edx_video_id, language_code=u'en') - - if not transcript: - raise Http404 - - name_and_extension = os.path.splitext(transcript['file_name']) - filename, input_format = name_and_extension[0], name_and_extension[1][1:] - transcript_content = transcript['content'] - - # convert sjson content into srt format. - transcript_content = Transcript.convert(transcript_content, input_format=input_format, output_format=Transcript.SRT) - if not transcript_content: raise Http404 # Construct an HTTP response - response = HttpResponse(transcript_content, content_type='application/x-subrip; charset=utf-8') - response['Content-Disposition'] = 'attachment; filename="{filename}.srt"'.format(filename=filename) + response = HttpResponse(content, content_type=mimetype) + response['Content-Disposition'] = 'attachment; filename="{filename}"'.format(filename=filename.encode('utf-8')) return response diff --git a/common/lib/xmodule/xmodule/tests/test_video.py b/common/lib/xmodule/xmodule/tests/test_video.py index e6b0766a7a..d7a307e515 100644 --- a/common/lib/xmodule/xmodule/tests/test_video.py +++ b/common/lib/xmodule/xmodule/tests/test_video.py @@ -12,6 +12,7 @@ You can then use the CourseFactory and XModuleItemFactory as defined in common/lib/xmodule/xmodule/modulestore/tests/factories.py to create the course, section, subsection, unit, etc. """ +import json import os import unittest import datetime @@ -913,9 +914,11 @@ class VideoDescriptorStudentViewDataTestCase(unittest.TestCase): @patch('xmodule.video_module.video_module.HLSPlaybackEnabledFlag.feature_enabled', Mock(return_value=True)) @patch('xmodule.video_module.video_module.is_val_transcript_feature_enabled_for_course', Mock(return_value=False)) + @patch('xmodule.video_module.transcripts_utils.get_available_transcript_languages', Mock(return_value=['es'])) @patch('edxval.api.get_video_info_for_course_and_profiles', Mock(return_value={})) + @patch('xmodule.video_module.transcripts_utils.get_video_transcript_content') @patch('edxval.api.get_video_info') - def test_student_view_data_with_hls_flag(self, mock_get_video_info): + def test_student_view_data_with_hls_flag(self, mock_get_video_info, mock_get_video_transcript_content): mock_get_video_info.return_value = { 'url': '/edxval/video/example', 'edx_video_id': u'example_id', @@ -931,8 +934,18 @@ class VideoDescriptorStudentViewDataTestCase(unittest.TestCase): ] } + mock_get_video_transcript_content.return_value = { + 'content': json.dumps({ + "start": [10], + "end": [100], + "text": ["Hi, welcome to Edx."], + }), + 'file_name': 'edx.sjson' + } + descriptor = instantiate_descriptor(edx_video_id='example_id', only_on_web=False) descriptor.runtime.course_id = MagicMock() + descriptor.runtime.handler_url = MagicMock() student_view_data = descriptor.student_view_data() expected_video_data = {u'hls': {'url': u'http://www.meowmix.com', 'file_size': 25556}} self.assertDictEqual(student_view_data.get('encoded_videos'), expected_video_data) diff --git a/common/lib/xmodule/xmodule/video_module/transcripts_utils.py b/common/lib/xmodule/xmodule/video_module/transcripts_utils.py index 99456f21f2..348cb6ffc4 100644 --- a/common/lib/xmodule/xmodule/video_module/transcripts_utils.py +++ b/common/lib/xmodule/xmodule/video_module/transcripts_utils.py @@ -827,7 +827,7 @@ class VideoTranscriptsMixin(object): transcript_language = u'en' return transcript_language - def get_transcripts_info(self, is_bumper=False, include_val_transcripts=False): + def get_transcripts_info(self, is_bumper=False): """ Returns a transcript dictionary for the video. @@ -848,9 +848,8 @@ class VideoTranscriptsMixin(object): for language_code, transcript_file in transcripts.items() if transcript_file != '' } - # For phase 2, removing `include_val_transcripts` will make edx-val - # taking over the control for transcripts. - if include_val_transcripts: + # bumper transcripts are stored in content store so we don't need check them in val + 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 # transcript module and contentstore will only function as fallback until all the diff --git a/common/lib/xmodule/xmodule/video_module/video_handlers.py b/common/lib/xmodule/xmodule/video_module/video_handlers.py index 5b5c57dbf3..529e525121 100644 --- a/common/lib/xmodule/xmodule/video_module/video_handlers.py +++ b/common/lib/xmodule/xmodule/video_module/video_handlers.py @@ -31,6 +31,8 @@ from .transcripts_utils import ( TranscriptException, TranscriptsGenerationException, youtube_speed_dict, + get_transcript, + get_transcript_from_contentstore ) from .transcripts_model_utils import ( is_val_transcript_feature_enabled_for_course @@ -246,6 +248,36 @@ class VideoStudentViewHandlers(object): self.runtime.publish(self, "completion", data) return {"result": "ok"} + @staticmethod + def make_transcript_http_response(content, filename, language, content_type, add_attachment_header=True): + """ + Construct `Response` object. + + Arguments: + content (unicode): transcript content + filename (unicode): transcript filename + language (unicode): transcript language + mimetype (unicode): transcript content type + add_attachment_header (bool): whether to add attachment header or not + """ + headerlist = [ + ('Content-Language', language), + ] + + if add_attachment_header: + headerlist.append( + ('Content-Disposition', 'attachment; filename="{}"'.format(filename.encode('utf-8'))) + ) + + response = Response( + content, + headerlist=headerlist, + charset='utf8' + ) + response.content_type = content_type + + return response + @XBlock.handler def transcript(self, request, dispatch): """ @@ -270,9 +302,8 @@ class VideoStudentViewHandlers(object): For 'en' check if SJSON exists. For non-`en` check if SRT file exists. """ is_bumper = request.GET.get('is_bumper', False) - # Currently, we don't handle video pre-load/bumper transcripts in edx-val. - feature_enabled = is_val_transcript_feature_enabled_for_course(self.course_id) and not is_bumper - transcripts = self.get_transcripts_info(is_bumper, include_val_transcripts=feature_enabled) + transcripts = self.get_transcripts_info(is_bumper) + if dispatch.startswith('translation'): language = dispatch.replace('translation', '').strip('/') @@ -288,85 +319,52 @@ class VideoStudentViewHandlers(object): self.transcript_language = language try: - transcript = self.translation(request.GET.get('videoId', None), transcripts) - except (TypeError, TranscriptException, NotFoundError) as ex: - # Catching `TranscriptException` because its also getting raised at places - # when transcript is not found in contentstore. - log.debug(six.text_type(ex)) - # Try to return static URL redirection as last resort - # if no translation is required - response = self.get_static_transcript(request, transcripts) - if response.status_code == 404 and feature_enabled: - # Try to get transcript from edx-val as a last resort. - transcript = get_video_transcript_content(self.edx_video_id, self.transcript_language) - if transcript: - transcript_conversion_props = dict(transcript, output_format=Transcript.SJSON) - transcript = convert_video_transcript(**transcript_conversion_props) - response = Response( - transcript['content'], - headerlist=[('Content-Language', self.transcript_language)], - charset='utf8', - ) - response.content_type = Transcript.mime_types[Transcript.SJSON] + if is_bumper: + content, filename, mimetype = get_transcript_from_contentstore( + self, + self.transcript_language, + Transcript.SJSON, + transcripts + ) + else: + content, filename, mimetype = get_transcript( + self, + lang=self.transcript_language, + output_format=Transcript.SJSON, + youtube_id=request.GET.get('videoId'), + ) - return response - except (UnicodeDecodeError, TranscriptsGenerationException) as ex: - log.info(six.text_type(ex)) - response = Response(status=404) - else: - response = Response(transcript, headerlist=[('Content-Language', language)]) - response.content_type = Transcript.mime_types['sjson'] + response = self.make_transcript_http_response( + content, + filename, + self.transcript_language, + mimetype, + add_attachment_header=False + ) + except NotFoundError: + log.exception('[Translation Dispatch] %s', self.location) + response = self.get_static_transcript(request, transcripts) elif dispatch == 'download': lang = request.GET.get('lang', None) + try: - transcript_content, transcript_filename, transcript_mime_type = self.get_transcript( - transcripts, transcript_format=self.transcript_download_format, lang=lang - ) - except (KeyError, UnicodeDecodeError): + content, filename, mimetype = get_transcript(self, lang) + except NotFoundError: return Response(status=404) - except (ValueError, NotFoundError): - response = Response(status=404) - # Check for transcripts in edx-val as a last resort if corresponding feature is enabled. - if feature_enabled: - # Make sure the language is set. - if not lang: - lang = self.get_default_transcript_language(transcripts) - - transcript = get_video_transcript_content(edx_video_id=self.edx_video_id, language_code=lang) - if transcript: - transcript_conversion_props = dict(transcript, output_format=self.transcript_download_format) - transcript = convert_video_transcript(**transcript_conversion_props) - response = Response( - transcript['content'], - headerlist=[ - ('Content-Disposition', 'attachment; filename="{filename}"'.format( - filename=transcript['filename'] - )), - ('Content-Language', lang), - ], - charset='utf8', - ) - response.content_type = Transcript.mime_types[self.transcript_download_format] - - return response - else: - response = Response( - transcript_content, - headerlist=[ - ('Content-Disposition', 'attachment; filename="{}"'.format(transcript_filename.encode('utf8'))), - ('Content-Language', self.transcript_language), - ], - charset='utf8' - ) - response.content_type = transcript_mime_type + response = self.make_transcript_http_response( + content, + filename, + self.transcript_language, + 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, + include_val_transcripts=feature_enabled ) 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 6737765c83..acc0c006e9 100644 --- a/common/lib/xmodule/xmodule/video_module/video_module.py +++ b/common/lib/xmodule/xmodule/video_module/video_module.py @@ -304,8 +304,7 @@ class VideoModule(VideoFields, VideoTranscriptsMixin, VideoStudentViewHandlers, if download_video_link and download_video_link.endswith('.m3u8'): download_video_link = None - feature_enabled = is_val_transcript_feature_enabled_for_course(self.course_id) - transcripts = self.get_transcripts_info(include_val_transcripts=feature_enabled) + transcripts = self.get_transcripts_info() track_url, transcript_language, sorted_languages = self.get_transcripts_for_student(transcripts=transcripts) # CDN_VIDEO_URLS is only to be used here and will be deleted @@ -1099,7 +1098,7 @@ class VideoDescriptor(VideoFields, VideoTranscriptsMixin, VideoStudioViewHandler } feature_enabled = is_val_transcript_feature_enabled_for_course(self.runtime.course_id.for_branch(None)) - transcripts_info = self.get_transcripts_info(include_val_transcripts=feature_enabled) + transcripts_info = self.get_transcripts_info() available_translations = self.available_translations(transcripts_info, include_val_transcripts=feature_enabled) transcripts = { lang: self.runtime.handler_url(self, 'transcript', 'download', query="lang=" + lang, thirdparty=True) diff --git a/lms/djangoapps/courseware/tests/test_video_handlers.py b/lms/djangoapps/courseware/tests/test_video_handlers.py index 4f6b7a99db..7743ad5169 100644 --- a/lms/djangoapps/courseware/tests/test_video_handlers.py +++ b/lms/djangoapps/courseware/tests/test_video_handlers.py @@ -21,12 +21,13 @@ from xmodule.contentstore.django import contentstore from xmodule.exceptions import NotFoundError from xmodule.modulestore import ModuleStoreEnum from xmodule.modulestore.django import modulestore -from xmodule.video_module.transcripts_utils import TranscriptException, TranscriptsGenerationException +from xmodule.video_module.transcripts_utils import TranscriptException, TranscriptsGenerationException, Transcript from xmodule.x_module import STUDENT_VIEW from .helpers import BaseTestXmodule from .test_video_xml import SOURCE_XML + TRANSCRIPT = {"start": [10], "end": [100], "text": ["Hi, welcome to Edx."]} BUMPER_TRANSCRIPT = {"start": [1], "end": [10], "text": ["A bumper"]} SRT_content = textwrap.dedent(""" @@ -435,7 +436,10 @@ class TestTranscriptDownloadDispatch(TestVideo): response = self.item.transcript(request=request, dispatch='download') self.assertEqual(response.status, '404 Not Found') - @patch('xmodule.video_module.VideoModule.get_transcript', return_value=('Subs!', 'test_filename.srt', 'application/x-subrip; charset=utf-8')) + @patch( + 'xmodule.video_module.video_handlers.get_transcript', + return_value=('Subs!', 'test_filename.srt', 'application/x-subrip; charset=utf-8') + ) def test_download_srt_exist(self, __): request = Request.blank('/download') response = self.item.transcript(request=request, dispatch='download') @@ -443,7 +447,10 @@ class TestTranscriptDownloadDispatch(TestVideo): self.assertEqual(response.headers['Content-Type'], 'application/x-subrip; charset=utf-8') self.assertEqual(response.headers['Content-Language'], 'en') - @patch('xmodule.video_module.VideoModule.get_transcript', return_value=('Subs!', 'txt', 'text/plain; charset=utf-8')) + @patch( + 'xmodule.video_module.video_handlers.get_transcript', + return_value=('Subs!', 'txt', 'text/plain; charset=utf-8') + ) def test_download_txt_exist(self, __): self.item.transcript_format = 'txt' request = Request.blank('/download') @@ -460,13 +467,16 @@ class TestTranscriptDownloadDispatch(TestVideo): with self.assertRaises(NotFoundError): self.item.get_transcript(transcripts) - @patch('xmodule.video_module.VideoModule.get_transcript', return_value=('Subs!', u"塞.srt", 'application/x-subrip; charset=utf-8')) + @patch( + 'xmodule.video_module.transcripts_utils.get_transcript_for_video', + return_value=(Transcript.SRT, u"塞", 'Subs!') + ) def test_download_non_en_non_ascii_filename(self, __): request = Request.blank('/download') response = self.item.transcript(request=request, dispatch='download') self.assertEqual(response.body, 'Subs!') self.assertEqual(response.headers['Content-Type'], 'application/x-subrip; charset=utf-8') - self.assertEqual(response.headers['Content-Disposition'], 'attachment; filename="塞.srt"') + self.assertEqual(response.headers['Content-Disposition'], 'attachment; filename="en_塞.srt"') @patch('xmodule.video_module.transcripts_utils.edxval_api.get_video_transcript_data') @patch('openedx.core.djangoapps.video_config.models.VideoTranscriptEnabledFlag.feature_enabled', Mock(return_value=True)) @@ -600,6 +610,7 @@ class TestTranscriptTranslationGetDispatch(TestVideo): # youtube 1_0 request, will generate for all speeds for existing ids self.item.youtube_id_1_0 = subs_id self.item.youtube_id_0_75 = '0_75' + self.store.update_item(self.item, self.user.id) request = Request.blank('/translation/uk?videoId={}'.format(subs_id)) response = self.item.transcript(request=request, dispatch='translation/uk') self.assertDictEqual(json.loads(response.body), subs) @@ -614,9 +625,11 @@ class TestTranscriptTranslationGetDispatch(TestVideo): u'\u041f\u0440\u0438\u0432\u0456\u0442, edX \u0432\u0456\u0442\u0430\u0454 \u0432\u0430\u0441.' ] } + self.assertDictEqual(json.loads(response.body), calculated_0_75) # 1_5 will be generated from 1_0 self.item.youtube_id_1_5 = '1_5' + self.store.update_item(self.item, self.user.id) request = Request.blank('/translation/uk?videoId={}'.format('1_5')) response = self.item.transcript(request=request, dispatch='translation/uk') calculated_1_5 = { @@ -638,6 +651,7 @@ class TestTranscriptTranslationGetDispatch(TestVideo): subs_id = _get_subs_id(good_sjson.name) attach(self.item, subs_id) + self.store.update_item(self.item, self.user.id) request = Request.blank(url) response = self.item.transcript(request=request, dispatch=dispatch) self.assertDictEqual(json.loads(response.body), TRANSCRIPT) @@ -800,33 +814,6 @@ class TestTranscriptTranslationGetDispatch(TestVideo): # Assert the actual response self.assertEqual(response.status_code, 404) - @ddt.data(True, False) - @patch('xmodule.video_module.transcripts_utils.edxval_api.get_video_transcript_data') - @patch('openedx.core.djangoapps.video_config.models.VideoTranscriptEnabledFlag.feature_enabled') - def test_translations_bumper_transcript(self, feature_enabled, - mock_val_video_transcript_feature, mock_get_video_transcript_data): - """ - Tests that the translations dispatch response remains the same with or without enabling - video transcript feature. - """ - # Mock val api util and return the valid transcript file. - transcript = { - 'content': json.dumps({ - "start": [10], - "end": [100], - "text": ["Hi, welcome to Edx."], - }), - 'file_name': 'edx.sjson' - } - mock_get_video_transcript_data.return_value = transcript - - mock_val_video_transcript_feature.return_value = feature_enabled - self.item.video_bumper = {"transcripts": {"en": "unknown.srt.sjson"}} - request = Request.blank('/translations/en?is_bumper=1') - response = self.item.transcript(request=request, dispatch='translation/en') - # Assert that despite the existence of val video transcripts, response remains 404. - self.assertEqual(response.status_code, 404) - @attr(shard=1) class TestStudioTranscriptTranslationGetDispatch(TestVideo): diff --git a/lms/djangoapps/courseware/tests/test_video_mongo.py b/lms/djangoapps/courseware/tests/test_video_mongo.py index 7d690d82e1..672f29c2da 100644 --- a/lms/djangoapps/courseware/tests/test_video_mongo.py +++ b/lms/djangoapps/courseware/tests/test_video_mongo.py @@ -1507,21 +1507,6 @@ class TestVideoDescriptorStudentViewJson(TestCase): student_view_response = self.get_result() self.assertItemsEqual(student_view_response['transcripts'].keys(), expected_transcripts) - @patch( - 'openedx.core.djangoapps.video_config.models.VideoTranscriptEnabledFlag.feature_enabled', - Mock(return_value=False), - ) - @patch( - 'xmodule.video_module.transcripts_utils.edxval_api.get_available_transcript_languages', - Mock(return_value=['ro', 'es']), - ) - def test_student_view_with_val_transcripts_disabled(self): - """ - Test `student_view_data` with edx-val transcripts disabled. - """ - student_view_response = self.get_result() - self.assertDictEqual(student_view_response['transcripts'], {self.TEST_LANGUAGE: self.transcript_url}) - @attr(shard=1) @ddt.ddt diff --git a/lms/djangoapps/mobile_api/video_outlines/serializers.py b/lms/djangoapps/mobile_api/video_outlines/serializers.py index 5ae31685d3..d902d5aeba 100644 --- a/lms/djangoapps/mobile_api/video_outlines/serializers.py +++ b/lms/djangoapps/mobile_api/video_outlines/serializers.py @@ -217,7 +217,7 @@ def video_summary(video_profiles, course_id, video_descriptor, request, local_ca # Transcripts... feature_enabled = is_val_transcript_feature_enabled_for_course(course_id) - transcripts_info = video_descriptor.get_transcripts_info(include_val_transcripts=feature_enabled) + transcripts_info = video_descriptor.get_transcripts_info() transcript_langs = video_descriptor.available_translations( transcripts=transcripts_info, include_val_transcripts=feature_enabled diff --git a/lms/djangoapps/mobile_api/video_outlines/views.py b/lms/djangoapps/mobile_api/video_outlines/views.py index 46314ed7a1..b198677bf3 100644 --- a/lms/djangoapps/mobile_api/video_outlines/views.py +++ b/lms/djangoapps/mobile_api/video_outlines/views.py @@ -124,7 +124,7 @@ class VideoTranscripts(generics.RetrieveAPIView): video_descriptor = modulestore().get_item(usage_key) feature_enabled = is_val_transcript_feature_enabled_for_course(usage_key.course_key) try: - transcripts = video_descriptor.get_transcripts_info(include_val_transcripts=feature_enabled) + transcripts = video_descriptor.get_transcripts_info() content, filename, mimetype = video_descriptor.get_transcript(transcripts, lang=lang) except (ValueError, NotFoundError): # Fallback mechanism for edx-val transcripts From cda7fe9a293b6dbe457255468d725c68de843e70 Mon Sep 17 00:00:00 2001 From: muhammad-ammar Date: Mon, 12 Mar 2018 16:20:45 +0500 Subject: [PATCH 02/18] 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( From a6bfd5f604f84d82ee54fe67be5b42f669b4046d Mon Sep 17 00:00:00 2001 From: muhammad-ammar Date: Thu, 8 Mar 2018 10:19:06 +0500 Subject: [PATCH 03/18] Deprecate Contentstore for Mobile Video Outline API EDUCATOR-2130 --- .../mobile_api/video_outlines/views.py | 24 ++++--------------- 1 file changed, 5 insertions(+), 19 deletions(-) diff --git a/lms/djangoapps/mobile_api/video_outlines/views.py b/lms/djangoapps/mobile_api/video_outlines/views.py index b198677bf3..68b9ecc41e 100644 --- a/lms/djangoapps/mobile_api/video_outlines/views.py +++ b/lms/djangoapps/mobile_api/video_outlines/views.py @@ -21,6 +21,7 @@ from xmodule.video_module.transcripts_utils import ( convert_video_transcript, get_video_transcript_content, Transcript, + get_transcript, ) from xmodule.video_module.transcripts_model_utils import ( is_val_transcript_feature_enabled_for_course @@ -122,26 +123,11 @@ class VideoTranscripts(generics.RetrieveAPIView): usage_key = BlockUsageLocator(course.id, block_type='video', block_id=block_id) video_descriptor = modulestore().get_item(usage_key) - feature_enabled = is_val_transcript_feature_enabled_for_course(usage_key.course_key) + try: - transcripts = video_descriptor.get_transcripts_info() - content, filename, mimetype = video_descriptor.get_transcript(transcripts, lang=lang) - except (ValueError, NotFoundError): - # Fallback mechanism for edx-val transcripts - transcript = None - if feature_enabled: - transcript = get_video_transcript_content(video_descriptor.edx_video_id, lang) - - if not transcript: - raise Http404(u'Transcript not found for {}, lang: {}'.format(block_id, lang)) - - transcript_conversion_props = dict(transcript, output_format=Transcript.SRT) - transcript = convert_video_transcript(**transcript_conversion_props) - filename = transcript['filename'] - content = transcript['content'] - mimetype = Transcript.mime_types[Transcript.SRT] - except KeyError: - raise Http404(u"Transcript not found for {}, lang: {}".format(block_id, lang)) + content, filename, mimetype = get_transcript(video_descriptor, lang=lang) + except NotFoundError: + raise Http404(u'Transcript not found for {}, lang: {}'.format(block_id, lang)) response = HttpResponse(content, content_type=mimetype) response['Content-Disposition'] = 'attachment; filename="{}"'.format(filename.encode('utf-8')) From c5a30f8eb5291d3103fe5e8bd8c41c2b086a5a44 Mon Sep 17 00:00:00 2001 From: muhammad-ammar Date: Tue, 20 Mar 2018 16:16:36 +0500 Subject: [PATCH 04/18] update check_transcripts handler EDUCATOR-2129 --- .../views/tests/test_transcripts.py | 26 +++-- .../contentstore/views/transcripts_ajax.py | 106 +++++++++--------- 2 files changed, 67 insertions(+), 65 deletions(-) diff --git a/cms/djangoapps/contentstore/views/tests/test_transcripts.py b/cms/djangoapps/contentstore/views/tests/test_transcripts.py index 9d99c072d9..d9fffdb515 100644 --- a/cms/djangoapps/contentstore/views/tests/test_transcripts.py +++ b/cms/djangoapps/contentstore/views/tests/test_transcripts.py @@ -824,19 +824,21 @@ class TestCheckTranscripts(BaseTranscripts): self.assertEqual(resp.status_code, 400) self.assertEqual(json.loads(resp.content).get('status'), 'Transcripts are supported only for "video" modules.') - @ddt.data( - (True, 'found'), - (False, 'not_found') - ) - @ddt.unpack - @patch('openedx.core.djangoapps.video_config.models.VideoTranscriptEnabledFlag.feature_enabled') - @patch('xmodule.video_module.transcripts_utils.edxval_api.get_video_transcript_data', Mock(return_value=True)) - def test_command_for_fallback_transcript(self, feature_enabled, expected_command, video_transcript_feature): + @patch('xmodule.video_module.transcripts_utils.get_video_transcript_content') + def test_command_for_fallback_transcript(self, mock_get_video_transcript_content): """ - Verify the command if a transcript is not found in content-store but - its there in edx-val. + Verify the command if a transcript is there in edx-val. """ - video_transcript_feature.return_value = feature_enabled + mock_get_video_transcript_content.return_value = { + 'content': json.dumps({ + "start": [10], + "end": [100], + "text": ["Hi, welcome to Edx."], + }), + 'file_name': 'edx.sjson' + } + + # video_transcript_feature.return_value = feature_enabled self.item.data = textwrap.dedent("""