From 0ff62e766e9b594856da671519c43d73e60c6980 Mon Sep 17 00:00:00 2001
From: Qubad786 <muhammadrehan69@gmail.com>
Date: Thu, 9 Mar 2017 21:30:05 +0500
Subject: [PATCH] prioritize youtube transcripts over html5 ones

---
 .../views/tests/test_transcripts.py           | 113 +++++++++++++-----
 .../xmodule/video_module/transcripts_utils.py |   5 +-
 2 files changed, 87 insertions(+), 31 deletions(-)

diff --git a/cms/djangoapps/contentstore/views/tests/test_transcripts.py b/cms/djangoapps/contentstore/views/tests/test_transcripts.py
index 5b9b406ecc..fc120638b8 100644
--- a/cms/djangoapps/contentstore/views/tests/test_transcripts.py
+++ b/cms/djangoapps/contentstore/views/tests/test_transcripts.py
@@ -40,6 +40,20 @@ class BaseTranscripts(CourseTestCase):
             except NotFoundError:
                 pass
 
+    def save_subs_to_store(self, subs, subs_id):
+        """
+        Save transcripts into `StaticContent`.
+        """
+        filedata = json.dumps(subs, indent=2)
+        mime_type = 'application/json'
+        filename = 'subs_{0}.srt.sjson'.format(subs_id)
+
+        content_location = StaticContent.compute_location(self.course.id, filename)
+        content = StaticContent(content_location, filename, mime_type, filedata)
+        contentstore().save(content)
+        del_cached_content(content_location)
+        return content_location
+
     def setUp(self):
         """Create initial data."""
         super(BaseTranscripts, self).setUp()
@@ -82,8 +96,9 @@ class BaseTranscripts(CourseTestCase):
 
 
 class TestUploadTranscripts(BaseTranscripts):
-    """Tests for '/transcripts/upload' url."""
-
+    """
+    Tests for '/transcripts/upload' url.
+    """
     def setUp(self):
         """Create initial data."""
         super(TestUploadTranscripts, self).setUp()
@@ -347,20 +362,9 @@ class TestUploadTranscripts(BaseTranscripts):
 
 
 class TestDownloadTranscripts(BaseTranscripts):
-    """Tests for '/transcripts/download' url."""
-
-    def save_subs_to_store(self, subs, subs_id):
-        """Save transcripts into `StaticContent`."""
-        filedata = json.dumps(subs, indent=2)
-        mime_type = 'application/json'
-        filename = 'subs_{0}.srt.sjson'.format(subs_id)
-
-        content_location = StaticContent.compute_location(self.course.id, filename)
-        content = StaticContent(content_location, filename, mime_type, filedata)
-        contentstore().save(content)
-        del_cached_content(content_location)
-        return content_location
-
+    """
+    Tests for '/transcripts/download' url.
+    """
     def test_success_download_youtube(self):
         self.item.data = '<video youtube="1:JMD_ifUUfsU" />'
         modulestore().update_item(self.item, self.user.id)
@@ -523,20 +527,9 @@ class TestDownloadTranscripts(BaseTranscripts):
 
 
 class TestCheckTranscripts(BaseTranscripts):
-    """Tests for '/transcripts/check' url."""
-
-    def save_subs_to_store(self, subs, subs_id):
-        """Save transcripts into `StaticContent`."""
-        filedata = json.dumps(subs, indent=2)
-        mime_type = 'application/json'
-        filename = 'subs_{0}.srt.sjson'.format(subs_id)
-
-        content_location = StaticContent.compute_location(self.course.id, filename)
-        content = StaticContent(content_location, filename, mime_type, filedata)
-        contentstore().save(content)
-        del_cached_content(content_location)
-        return content_location
-
+    """
+    Tests for '/transcripts/check' url.
+    """
     def test_success_download_nonyoutube(self):
         subs_id = str(uuid4())
         self.item.data = textwrap.dedent("""
@@ -766,3 +759,63 @@ class TestCheckTranscripts(BaseTranscripts):
         resp = self.client.get(link, {'data': json.dumps(data)})
         self.assertEqual(resp.status_code, 400)
         self.assertEqual(json.loads(resp.content).get('status'), 'Transcripts are supported only for "video" modules.')
+
+
+class TestSaveTranscripts(BaseTranscripts):
+    """
+    Tests for '/transcripts/save' url.
+    """
+    def assert_current_subs(self, expected_subs):
+        """
+        Asserts the current subtitles set on the video module.
+
+        Arguments:
+            expected_subs (String): Expected current subtitles for video.
+        """
+        item = modulestore().get_item(self.video_usage_key)
+        self.assertEqual(item.sub, expected_subs)
+
+    def test_prioritize_youtube_sub_on_save(self):
+        """
+        Test that the '/transcripts/save' endpoint prioritises youtube subtitles over html5 ones
+        while deciding the current subs for video module.
+        """
+        # Update video module to contain 1 youtube and 2 html5 sources.
+        youtube_id = str(uuid4())
+        self.item.data = textwrap.dedent(
+            """
+            <video youtube="1:{youtube_id}" sub="">
+                <source src="http://www.testvid.org/html5/videos/testvid.mp4"/>
+                <source src="http://www.testvid2.org/html5/videos/testvid2.webm"/>
+            </video>
+            """.format(youtube_id=youtube_id)
+        )
+        modulestore().update_item(self.item, self.user.id)
+        self.assert_current_subs(expected_subs='')
+
+        # Save new subs in the content store.
+        subs = {
+            'start': [100, 200, 240],
+            'end': [200, 240, 380],
+            'text': [
+                'subs #1',
+                'subs #2',
+                'subs #3'
+            ]
+        }
+        self.save_subs_to_store(subs, youtube_id)
+
+        # Now, make request to /transcripts/save endpoint with new subs.
+        data = {
+            'locator': unicode(self.video_usage_key),
+            'metadata': {
+                'sub': youtube_id
+            }
+        }
+        resp = self.client.get(reverse('save_transcripts'), {'data': json.dumps(data)})
+        self.assertEqual(resp.status_code, 200)
+        self.assertEqual(json.loads(resp.content), {"status": "Success"})
+
+        # Now check item.sub, it should be same as youtube id because /transcripts/save prioritize
+        # youtube subs over html5 ones.
+        self.assert_current_subs(expected_subs=youtube_id)
diff --git a/common/lib/xmodule/xmodule/video_module/transcripts_utils.py b/common/lib/xmodule/xmodule/video_module/transcripts_utils.py
index f464a9d1db..2eeb3ff888 100644
--- a/common/lib/xmodule/xmodule/video_module/transcripts_utils.py
+++ b/common/lib/xmodule/xmodule/video_module/transcripts_utils.py
@@ -338,7 +338,10 @@ def manage_video_subtitles_save(item, user, old_metadata=None, generate_translat
 
     # 1.
     html5_ids = get_html5_ids(item.html5_sources)
-    possible_video_id_list = [item.youtube_id_1_0] + html5_ids
+
+    # Youtube transcript source should always have a higher priority than html5 sources. Appending
+    # `youtube_id_1_0` at the end helps achieve this when we read transcripts list.
+    possible_video_id_list = html5_ids + [item.youtube_id_1_0]
     sub_name = item.sub
     for video_id in possible_video_id_list:
         if not video_id: