From 6ea3c8cbdd2877ed7d08522d818a2bedb1316806 Mon Sep 17 00:00:00 2001
From: Alexander Kryklia <kryklia@gmail.com>
Date: Tue, 10 Jun 2014 13:04:40 +0300
Subject: [PATCH] Fix failing test and update docstrings for video player

Fixes BLD-1115
---
 .../xmodule/video_module/video_module.py      | 36 ++++++++++++++-----
 common/lib/xmodule/xmodule/x_module.py        |  4 +--
 .../tests/video/test_video_module.py          |  6 ++--
 3 files changed, 33 insertions(+), 13 deletions(-)

diff --git a/common/lib/xmodule/xmodule/video_module/video_module.py b/common/lib/xmodule/xmodule/video_module/video_module.py
index a28468edd5..8cbbcfa370 100644
--- a/common/lib/xmodule/xmodule/video_module/video_module.py
+++ b/common/lib/xmodule/xmodule/video_module/video_module.py
@@ -25,7 +25,7 @@ from django.conf import settings
 from xblock.fields import ScopeIds
 from xblock.runtime import KvsFieldData
 
-from xmodule.modulestore.inheritance import InheritanceKeyValueStore
+from xmodule.modulestore.inheritance import InheritanceKeyValueStore, own_metadata
 from xmodule.x_module import XModule, module_attr
 from xmodule.editing_module import TabsEditingDescriptor
 from xmodule.raw_module import EmptyDataRawDescriptor
@@ -231,14 +231,34 @@ class VideoDescriptor(VideoFields, VideoStudioViewHandlers, TabsEditingDescripto
 
     def editor_saved(self, user, old_metadata, old_content):
         """
-        Used to update video subtitles.
+        Used to update video values during `self`:save method from CMS.
+
+        old_metadata: dict, values of fields of `self` with scope=settings which were explicitly set by user.
+        old_content, same as `old_metadata` but for scope=content.
+
+        Due to nature of code flow in item.py::_save_item, before current function is called,
+        fields of `self` instance have been already updated, but not yet saved.
+
+        To obtain values, which were changed by user input,
+        one should compare own_metadata(self) and old_medatada.
+
+        Video player has two tabs, and due to nature of sync between tabs,
+        metadata from Basic tab is always sent when video player is edited and saved first time, for example:
+        {'youtube_id_1_0': u'OEoXaMPEzfM', 'display_name': u'Video', 'sub': u'OEoXaMPEzfM', 'html5_sources': []},
+        that's why these fields will always present in old_metadata after first save. This should be fixed.
+
+        At consequent save requests html5_sources are always sent too, disregard of their change by user.
+        That means that html5_sources are always in list of fields that were changed (`metadata` param in save_item).
+        This should be fixed too.
         """
-        manage_video_subtitles_save(
-            self,
-            user,
-            old_metadata if old_metadata else None,
-            generate_translation=True
-        )
+        metadata_was_changed_by_user = old_metadata != own_metadata(self)
+        if metadata_was_changed_by_user:
+            manage_video_subtitles_save(
+                self,
+                user,
+                old_metadata if old_metadata else None,
+                generate_translation=True
+            )
 
     def save_with_metadata(self, user):
         """
diff --git a/common/lib/xmodule/xmodule/x_module.py b/common/lib/xmodule/xmodule/x_module.py
index cc4a2e37b8..305d83730c 100644
--- a/common/lib/xmodule/xmodule/x_module.py
+++ b/common/lib/xmodule/xmodule/x_module.py
@@ -777,8 +777,8 @@ class XModuleDescriptor(XModuleMixin, HTMLSnippet, ResourceTemplates, XBlock):
 
         Note that after this method is called, the modulestore update_item method will
         be called on this xmodule. Therefore, any modifications to the xmodule that are
-        performed in editor_saved will automatically be persisted (implementors of this method
-        should not call update_item themselves).
+        performed in editor_saved will automatically be persisted (calling update_item
+        from implementors of this method is not necessary).
 
         Args:
             user: the user who requested the save (as obtained from the request)
diff --git a/common/test/acceptance/tests/video/test_video_module.py b/common/test/acceptance/tests/video/test_video_module.py
index 25d21bc562..060d8eae6e 100644
--- a/common/test/acceptance/tests/video/test_video_module.py
+++ b/common/test/acceptance/tests/video/test_video_module.py
@@ -267,7 +267,6 @@ class YouTubeVideoTest(VideoBaseTest):
         unicode_text = "好 各位同学".decode('utf-8')
         self.assertIn(unicode_text, self.video.captions_text())
 
-    @skip("Failing Intermittently in master. BLD-1115")
     def test_cc_button_transcripts_and_sub_fields_empty(self):
         """
         Scenario: CC button works correctly if transcripts and sub fields are empty,
@@ -276,8 +275,9 @@ class YouTubeVideoTest(VideoBaseTest):
         And I have uploaded a .srt.sjson file to assets
         Then I see the correct english text in the captions
         """
-        self.assets.append('subs_OEoXaMPEzfM.srt.sjson')
-        self.navigate_to_video()
+        self._install_course_fixture()
+        self.course_fixture._upload_assets(['subs_OEoXaMPEzfM.srt.sjson'])
+        self._navigate_to_courseware_video_and_render()
         self.video.show_captions()
 
         # Verify that we see "Hi, welcome to Edx." text in the captions