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