EDUCATOR-1757

Move contentstore to fallback for the following and remove feature flag from edx-val transcripts retrieval, those will now be included by default except for the bumper transcripts.
 - Download and Translation dispatches
 - Mobile-only Transcript Retrieval View

common/lib/xmodule/xmodule/video_module/transcripts_utils.py

@@ -646,7 +646,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, include_val_transcripts=True):
         """
         Return a list of language codes for which we have transcripts.
 
@@ -770,13 +770,12 @@ 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.
 
         Arguments:
             is_bumper(bool): If True, the request is for the bumper transcripts
-            include_val_transcripts(bool): If True, include edx-val transcripts as well
         """
         if is_bumper:
             transcripts = copy.deepcopy(get_bumper_settings(self).get('transcripts', {}))
@@ -791,9 +790,10 @@ 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:
+        # Do not include VAL transcripts into the transcripts metadata
+        # if the request is for bumper transcripts. Currently, we don't
+        # handle video pre-load/bumper transcripts in edx-val.
+        if not is_bumper:
             transcript_languages = get_available_transcript_languages(
                 edx_video_id=self.edx_video_id,
                 youtube_id_1_0=self.youtube_id_1_0,
@@ -804,6 +804,7 @@ class VideoTranscriptsMixin(object):
             # data is migrated to edx-val.
             for language_code in transcript_languages:
                 if language_code == 'en' and not sub:
+                    # TODO This is where we will be including val-identifier
                     sub = NON_EXISTENT_TRANSCRIPT
                 elif not transcripts.get(language_code):
                     transcripts[language_code] = NON_EXISTENT_TRANSCRIPT

common/lib/xmodule/xmodule/video_module/video_module.py

@@ -44,7 +44,6 @@ from .bumper_utils import bumperize
 from .transcripts_utils import (
     get_html5_ids,
     get_video_ids_info,
-    is_val_transcript_feature_enabled_for_course,
     Transcript,
     VideoTranscriptsMixin,
 )
@@ -289,8 +288,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
@@ -1023,9 +1021,8 @@ 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(include_val_transcripts=feature_enabled)
-        available_translations = self.available_translations(transcripts_info, include_val_transcripts=feature_enabled)
+        transcripts_info = self.get_transcripts_info()
+        available_translations = self.available_translations(transcripts_info)
         transcripts = {
             lang: self.runtime.handler_url(self, 'transcript', 'download', query="lang=" + lang, thirdparty=True)
             for lang in available_translations

lms/djangoapps/courseware/tests/test_video_handlers.py

@@ -248,7 +248,6 @@ class TestTranscriptAvailableTranslationsDispatch(TestVideo):
         response = self.item.transcript(request=request, dispatch='available_translations')
         self.assertEqual(json.loads(response.body), ['en', 'uk'])
 
-    @patch('xmodule.video_module.transcripts_utils.VideoTranscriptEnabledFlag.feature_enabled', Mock(return_value=True))
     @patch('xmodule.video_module.transcripts_utils.get_available_transcript_languages')
     @ddt.data(
         (
@@ -285,8 +284,7 @@ class TestTranscriptAvailableTranslationsDispatch(TestVideo):
     @ddt.unpack
     def test_val_available_translations(self, val_transcripts, sub, transcripts, result, mock_get_transcript_languages):
         """
-        Tests available translations with video component's and val's transcript languages
-        while the feature is enabled.
+        Tests available translations with video component as well as val transcript languages.
         """
         for lang_code, in_content_store in dict(transcripts).iteritems():
             if in_content_store:
@@ -308,20 +306,6 @@ class TestTranscriptAvailableTranslationsDispatch(TestVideo):
         response = self.item.transcript(request=request, dispatch='available_translations')
         self.assertItemsEqual(json.loads(response.body), result)
 
-    @patch(
-        'xmodule.video_module.transcripts_utils.VideoTranscriptEnabledFlag.feature_enabled',
-        Mock(return_value=False),
-    )
-    @patch('xmodule.video_module.transcripts_utils.edxval_api.get_available_transcript_languages')
-    def test_val_available_translations_feature_disabled(self, mock_get_available_transcript_languages):
-        """
-        Tests available translations with val transcript languages when feature is disabled.
-        """
-        mock_get_available_transcript_languages.return_value = ['en', 'de', 'ro']
-        request = Request.blank('/available_translations')
-        response = self.item.transcript(request=request, dispatch='available_translations')
-        self.assertEqual(response.status_code, 404)
-
 
 @attr(shard=1)
 @ddt.ddt
@@ -363,16 +347,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('xmodule.video_module.transcripts_utils.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 the available translations dispatch works as expected in the presence
+        of edx-val's translations along with multiple bumper 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()
@@ -459,11 +439,9 @@ class TestTranscriptDownloadDispatch(TestVideo):
         self.assertEqual(response.headers['Content-Disposition'], 'attachment; filename="塞.srt"')
 
     @patch('xmodule.video_module.transcripts_utils.edxval_api.get_video_transcript_data')
-    @patch('xmodule.video_module.transcripts_utils.VideoTranscriptEnabledFlag.feature_enabled', Mock(return_value=True))
-    @patch('xmodule.video_module.VideoModule.get_transcript', Mock(side_effect=NotFoundError))
-    def test_download_fallback_transcript(self, mock_get_video_transcript_data):
+    def test_download_val_transcript(self, mock_get_video_transcript_data):
         """
-        Verify val transcript is returned as a fallback if it is not found in the content store.
+        Tests that downloading VAL transcripts/translations works as expected.
         """
         mock_get_video_transcript_data.return_value = {
             'content': json.dumps({
@@ -492,21 +470,6 @@ class TestTranscriptDownloadDispatch(TestVideo):
         for attribute, value in expected_headers.iteritems():
             self.assertEqual(response.headers[attribute], value)
 
-    @patch(
-        'xmodule.video_module.transcripts_utils.VideoTranscriptEnabledFlag.feature_enabled',
-        Mock(return_value=False),
-    )
-    @patch('xmodule.video_module.VideoModule.get_transcript', Mock(side_effect=NotFoundError))
-    def test_download_fallback_transcript_feature_disabled(self):
-        """
-        Verify val transcript if its feature is disabled.
-        """
-        # Make request to XModule transcript handler
-        request = Request.blank('/download')
-        response = self.item.transcript(request=request, dispatch='download')
-        # Assert the actual response
-        self.assertEqual(response.status_code, 404)
-
 
 @attr(shard=1)
 @ddt.ddt
@@ -740,13 +703,9 @@ class TestTranscriptTranslationGetDispatch(TestVideo):
             store.update_item(self.course, self.user.id)
 
     @patch('xmodule.video_module.transcripts_utils.edxval_api.get_video_transcript_data')
-    @patch('xmodule.video_module.transcripts_utils.VideoTranscriptEnabledFlag.feature_enabled', Mock(return_value=True))
-    @patch('xmodule.video_module.VideoModule.translation', Mock(side_effect=NotFoundError))
-    @patch('xmodule.video_module.VideoModule.get_static_transcript', Mock(return_value=Response(status=404)))
-    def test_translation_fallback_transcript(self, mock_get_video_transcript_data):
+    def test_translation_dispatch_with_val_transcript(self, mock_get_video_transcript_data):
         """
-        Verify that the val transcript is returned as a fallback,
-        if it is not found in the content store.
+        Verify that the 'translations/' dispatch is working as expected with VAL transcripts.
         """
         transcript = {
             'content': json.dumps({
@@ -773,29 +732,11 @@ class TestTranscriptTranslationGetDispatch(TestVideo):
         for attribute, value in expected_headers.iteritems():
             self.assertEqual(response.headers[attribute], value)
 
-    @patch(
-        'xmodule.video_module.transcripts_utils.VideoTranscriptEnabledFlag.feature_enabled',
-        Mock(return_value=False),
-    )
-    @patch('xmodule.video_module.VideoModule.translation', Mock(side_effect=NotFoundError))
-    @patch('xmodule.video_module.VideoModule.get_static_transcript', Mock(return_value=Response(status=404)))
-    def test_translation_fallback_transcript_feature_disabled(self):
-        """
-        Verify that val transcript is not returned when its feature is disabled.
-        """
-        # Make request to XModule transcript handler
-        response = self.item.transcript(request=Request.blank('/translation/en'), dispatch='translation/en')
-        # 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('xmodule.video_module.transcripts_utils.VideoTranscriptEnabledFlag.feature_enabled')
-    def test_translations_bumper_transcript(self, feature_enabled,
-                                            mock_val_video_transcript_feature, mock_get_video_transcript_data):
+    def test_translations_bumper_transcript(self, mock_get_video_transcript_data):
         """
-        Tests that the translations dispatch response remains the same with or without enabling
-        video transcript feature.
+        Tests that the translations dispatch's response remains the same for bumper transcripts
+        irrespective of the presence of VAL and Contentstore transcripts.
         """
         # Mock val api util and return the valid transcript file.
         transcript = {
@@ -808,11 +749,11 @@ class TestTranscriptTranslationGetDispatch(TestVideo):
         }
         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.
+        # Assert that despite the existence of val video transcripts, response remains 404
+        # as there isn't any transcript for video bumper.
         self.assertEqual(response.status_code, 404)
 
 

lms/djangoapps/courseware/tests/test_video_mongo.py

@@ -1467,12 +1467,11 @@ class TestVideoDescriptorStudentViewJson(TestCase):
         ({'uk': 1, 'de': 1}, 'en-subs', ['de', 'en'], ['en', 'uk', 'de']),
     )
     @ddt.unpack
-    @patch('xmodule.video_module.transcripts_utils.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):
+    def test_student_view_with_val_transcripts(self, transcripts, english_sub, val_transcripts,
+                                               expected_transcripts, mock_get_transcript_languages):
         """
-        Test `student_view_data` with edx-val transcripts enabled.
+        Test that `student_view_data` works as expected with edx-val transcripts.
         """
         mock_get_transcript_languages.return_value = val_transcripts
         self.video.transcripts = transcripts
@@ -1480,21 +1479,6 @@ class TestVideoDescriptorStudentViewJson(TestCase):
         student_view_response = self.get_result()
         self.assertItemsEqual(student_view_response['transcripts'].keys(), expected_transcripts)
 
-    @patch(
-        'xmodule.video_module.transcripts_utils.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)
 class VideoDescriptorTest(TestCase, VideoDescriptorTestBase):

lms/djangoapps/mobile_api/video_outlines/serializers.py

@@ -11,7 +11,6 @@ from courseware.module_render import get_module_for_descriptor
 from util.module_utils import get_dynamic_descriptor_children
 from xmodule.modulestore.django import modulestore
 from xmodule.modulestore.mongo.base import BLOCK_TYPES_WITH_CHILDREN
-from xmodule.video_module.transcripts_utils import is_val_transcript_feature_enabled_for_course
 
 
 class BlockOutline(object):
@@ -209,12 +208,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(include_val_transcripts=feature_enabled)
-    transcript_langs = video_descriptor.available_translations(
-        transcripts=transcripts_info,
-        include_val_transcripts=feature_enabled
-    )
+    transcripts_info = video_descriptor.get_transcripts_info()
+    transcript_langs = video_descriptor.available_translations(transcripts=transcripts_info)
 
     transcripts = {
         lang: reverse(

lms/djangoapps/mobile_api/video_outlines/tests.py

@@ -887,10 +887,12 @@ class TestVideoSummaryList(TestVideoAPITestCase, MobileAuthTestMixin, MobileCour
         ({'uk': 1, 'de': 1}, 'en-subs', ['de', 'en'], ['en', 'uk', 'de']),
     )
     @ddt.unpack
-    @patch('xmodule.video_module.transcripts_utils.VideoTranscriptEnabledFlag.feature_enabled', Mock(return_value=True))
     @patch('xmodule.video_module.transcripts_utils.edxval_api.get_available_transcript_languages')
-    def test_val_transcripts_with_feature_enabled(self, transcripts, english_sub, val_transcripts,
+    def test_val_transcript_languages(self, transcripts, english_sub, val_transcripts,
                                                   expected_transcripts, mock_get_transcript_languages):
+        """
+        Tests that serialized course outline includes the VAL transcripts/translations languages as well.
+        """
         self.login_and_enroll()
         video = ItemFactory.create(
             parent=self.nameless_unit,
@@ -939,10 +941,6 @@ class TestTranscriptsDetail(TestVideoAPITestCase, MobileAuthTestMixin, MobileCou
         self.api_response(expected_response_code=200, lang='en')
 
     @patch(
-        'xmodule.video_module.transcripts_utils.VideoTranscriptEnabledFlag.feature_enabled',
-        Mock(return_value=True),
-    )
-    @patch(
         'xmodule.video_module.transcripts_utils.edxval_api.get_available_transcript_languages',
         Mock(return_value=['uk']),
     )
@@ -974,20 +972,3 @@ class TestTranscriptsDetail(TestVideoAPITestCase, MobileAuthTestMixin, MobileCou
         self.assertEqual(response.content, expected_content)
         for attribute, value in expected_headers.iteritems():
             self.assertEqual(response.get(attribute), value)
-
-    @patch(
-        'xmodule.video_module.transcripts_utils.VideoTranscriptEnabledFlag.feature_enabled',
-        Mock(return_value=False),
-    )
-    @patch(
-        'xmodule.video_module.transcripts_utils.edxval_api.get_available_transcript_languages',
-        Mock(return_value=['uk']),
-    )
-    def test_val_transcript_feature_disabled(self):
-        """
-        Tests transcript retrieval view with val transcripts when
-        the corresponding feature is disabled.
-        """
-        self.login_and_enroll()
-        # request to retrieval endpoint will result in 404 as val transcripts are disabled.
-        self.api_response(expected_response_code=404, lang='uk')

lms/djangoapps/mobile_api/video_outlines/views.py

@@ -17,11 +17,7 @@ from rest_framework.response import Response
 from mobile_api.models import MobileApiConfig
 from xmodule.exceptions import NotFoundError
 from xmodule.modulestore.django import modulestore
-from xmodule.video_module.transcripts_utils import (
-    get_video_transcript_content,
-    is_val_transcript_feature_enabled_for_course,
-    Transcript,
-)
+from xmodule.video_module.transcripts_utils import get_video_transcript_content, Transcript
 
 from ..decorators import mobile_course_access, mobile_view
 from .serializers import BlockOutline, video_summary
@@ -119,30 +115,27 @@ 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(include_val_transcripts=feature_enabled)
-            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(
-                    language_code=lang,
-                    edx_video_id=video_descriptor.edx_video_id,
-                    youtube_id_1_0=video_descriptor.youtube_id_1_0,
-                    html5_sources=video_descriptor.html5_sources,
-                )
-
-            if not transcript:
-                raise Http404(u'Transcript not found for {}, lang: {}'.format(block_id, lang))
 
+        # Retrieve transcript from edx-val
+        transcript = get_video_transcript_content(
+            language_code=lang,
+            edx_video_id=video_descriptor.edx_video_id,
+            youtube_id_1_0=video_descriptor.youtube_id_1_0,
+            html5_sources=video_descriptor.html5_sources,
+        )
+        if transcript:
             base_name, __ = os.path.splitext(os.path.basename(transcript['file_name']))
             filename = '{base_name}.srt'.format(base_name=base_name)
             content = Transcript.convert(transcript['content'], 'sjson', 'srt')
             mimetype = Transcript.mime_types['srt']
-        except KeyError:
-            raise Http404(u"Transcript not found for {}, lang: {}".format(block_id, lang))
+        else:
+            # The contentstore is used as a fallback for transcripts if edx-val hasn't got the
+            # transcript.
+            try:
+                transcripts = video_descriptor.get_transcripts_info()
+                content, filename, mimetype = video_descriptor.get_transcript(transcripts, lang=lang)
+            except (NotFoundError, ValueError, KeyError):
+                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'))