Update IsoTIme to be timedelta and update tests.

common/lib/xmodule/xmodule/fields.py

@@ -2,7 +2,7 @@ import time
 import logging
 import re
 
-from xblock.fields import Field, String
+from xblock.fields import Field
 import datetime
 import dateutil.parser
 
@@ -118,14 +118,85 @@ class Timedelta(Field):
         return ' '.join(values)
 
 
-class IsoTime(String):
+class IsoTime(Field):
+    """
+    Field for start_time and end_time video module properties.
+
+    It was decided, that python representation of start_time and end_time
+    should be python datetime.timedelta object, to be consistent with
+    common time representation.
+
+    At the same time, serialized representation should be"HH:MM:SS"
+    This format is convenient to use in XML (and it is used now),
+    and also it is used in frond-end studio editor of video module as format
+    for start and end time fields.
+
+    In database we previously had float type for start_time and end_time fields,
+    so we are checking it also.
+
+    Python object of IsoTime is datetime.timedelta.
+    JSONed representation of IsoTime is "HH:MM:SS"
+    """
+    # Timedeltas are immutable, see http://docs.python.org/2/library/datetime.html#available-types
+    MUTABLE = False
+
+    def _isotime_to_timedelta(self, value):
+        """
+        Validate that value in "HH:MM:SS" format and convert to timedelta.
+
+        Validate that user, that edits XML, sets proper format, and
+         that max value that can be used by user is "23:59:59".
+        """
+        try:
+            obj_time = time.strptime(value, '%H:%M:%S')
+        except ValueError as e:
+            raise e(
+                "Incorrect IsoTime value {} was set in XML or serialized."
+                "Original parse message is {}".format(value, e.message)
+            )
+        return datetime.timedelta(
+            hours=obj_time.tm_hour,
+            minutes=obj_time.tm_min,
+            seconds=obj_time.tm_sec
+        )
 
     def from_json(self, value):
+        """
+        Convert value in 'HH:MM:SS' format to datetime.timedelta.
+
+        If not value, returns 0.
+        If value is float (backward compatibility issue), convert to timedelta.
+        """
+        if not value:
+            return datetime.timedelta(seconds=0)
+
+        # We've seen serialized versions of float in this field
         if isinstance(value, float):
+            return datetime.timedelta(seconds=value)
+
+        return self._isotime_to_timedelta(value)
+
+    def to_json(self, value):
+        """
+        Convert datetime.timedelta to "HH:MM:SS" format.
+
+        If not value, return "00:00:00"
+
+        Backward compatibility: check if value is float, and convert it. No exceptions here.
+
+        If value is not float, but is exceed 23:59:59, raise exception.
+        """
         if not value:
             return "00:00:00"
-            else:
+
+        if isinstance(value, float):  # backward compatibility
+            if value > 86400:
+                value = 86400
             return str(datetime.timedelta(seconds=value))
-        else:
-            return super(IsoTime, self).from_json(value)
 
+        if value.total_seconds() > 86400:  # sanity check
+            raise ValueError(
+                "IsoTime max value is 23:59:59=86400 seconds"
+                "but {} seconds is passed".format(value.total_seconds())
+            )
+        return str(value)

common/lib/xmodule/xmodule/tests/test_video.py

@@ -14,6 +14,7 @@ the course, section, subsection, unit, etc.
 """
 
 import unittest
+import datetime
 from mock import Mock
 
 from . import LogicTest
@@ -36,24 +37,6 @@ class VideoModuleTest(LogicTest):
         'data': '<video />'
     }
 
-    def test_parse_time_empty(self):
-        """Ensure parse_time returns correctly with None or empty string."""
-        expected = ''
-        self.assertEqual(VideoDescriptor._parse_time(None), expected)
-        self.assertEqual(VideoDescriptor._parse_time(''), expected)
-
-    def test_parse_time(self):
-        """Ensure that times are parsed correctly into seconds."""
-        expected = 247
-        output = VideoDescriptor._parse_time('00:04:07')
-        self.assertEqual(output, expected)
-
-    def test_parse_time_with_float(self):
-        """Ensure that times are parsed correctly into seconds."""
-        expected = 247
-        output = VideoDescriptor._parse_time('247.0')
-        self.assertEqual(output, expected)
-
     def test_parse_youtube(self):
         """Test parsing old-style Youtube ID strings into a dict."""
         youtube_str = '0.75:jNCf2gIqpeE,1.00:ZwkTiUPN0mg,1.25:rsq9auxASqI,1.50:kMyNdzVHHgg'
@@ -224,8 +207,8 @@ class VideoDescriptorImportTestCase(unittest.TestCase):
             'youtube_id_1_25': '1EeWXzPdhSA',
             'youtube_id_1_5': 'rABDYkeK0x8',
             'show_captions': False,
-            'start_time': 1.0,
-            'end_time': 60,
+            'start_time': datetime.timedelta(seconds=1),
+            'end_time': datetime.timedelta(seconds=60),
             'track': 'http://www.example.com/track',
             'html5_sources': ['http://www.example.com/source.mp4', 'http://www.example.com/source.ogg'],
             'data': ''
@@ -250,8 +233,8 @@ class VideoDescriptorImportTestCase(unittest.TestCase):
             'youtube_id_1_25': '1EeWXzPdhSA',
             'youtube_id_1_5': 'rABDYkeK0x8',
             'show_captions': False,
-            'start_time': 1.0,
-            'end_time': 60,
+            'start_time': datetime.timedelta(seconds=1),
+            'end_time': datetime.timedelta(seconds=60),
             'track': 'http://www.example.com/track',
             'source': 'http://www.example.com/source.mp4',
             'html5_sources': ['http://www.example.com/source.mp4'],
@@ -279,8 +262,8 @@ class VideoDescriptorImportTestCase(unittest.TestCase):
             'youtube_id_1_25': '1EeWXzPdhSA',
             'youtube_id_1_5': '',
             'show_captions': True,
-            'start_time': 0.0,
-            'end_time': 0.0,
+            'start_time': datetime.timedelta(seconds=0.0),
+            'end_time': datetime.timedelta(seconds=0.0),
             'track': 'http://www.example.com/track',
             'source': 'http://www.example.com/source.mp4',
             'html5_sources': ['http://www.example.com/source.mp4'],
@@ -300,8 +283,8 @@ class VideoDescriptorImportTestCase(unittest.TestCase):
             'youtube_id_1_25': '',
             'youtube_id_1_5': '',
             'show_captions': True,
-            'start_time': 0.0,
-            'end_time': 0.0,
+            'start_time': datetime.timedelta(seconds=0.0),
+            'end_time': datetime.timedelta(seconds=0.0),
             'track': '',
             'source': '',
             'html5_sources': [],
@@ -334,8 +317,8 @@ class VideoDescriptorImportTestCase(unittest.TestCase):
             'youtube_id_1_25': 'OEoXaMPEzf125',
             'youtube_id_1_5': 'OEoXaMPEzf15',
             'show_captions': False,
-            'start_time': 0.0,
-            'end_time': 0.0,
+            'start_time': datetime.timedelta(seconds=0.0),
+            'end_time': datetime.timedelta(seconds=0.0),
             'track': 'http://download_track',
             'source': 'http://download_video',
             'html5_sources': ["source_1", "source_2"],
@@ -356,8 +339,8 @@ class VideoDescriptorImportTestCase(unittest.TestCase):
             'youtube_id_1_25': '1EeWXzPdhSA',
             'youtube_id_1_5': '',
             'show_captions': True,
-            'start_time': 0.0,
-            'end_time': 0.0,
+            'start_time': datetime.timedelta(seconds=0.0),
+            'end_time': datetime.timedelta(seconds=0.0),
             'track': '',
             'source': '',
             'html5_sources': [],
@@ -386,8 +369,8 @@ class VideoDescriptorImportTestCase(unittest.TestCase):
             'youtube_id_1_25': '1EeWXzPdhSA',
             'youtube_id_1_5': 'rABDYkeK0x8',
             'show_captions': False,
-            'start_time': 1.0,
-            'end_time': 60,
+            'start_time': datetime.timedelta(seconds=1),
+            'end_time': datetime.timedelta(seconds=60),
             'track': 'http://www.example.com/track',
             'html5_sources': ['http://www.example.com/source.mp4'],
             'data': ''
@@ -415,8 +398,8 @@ class VideoDescriptorImportTestCase(unittest.TestCase):
             'youtube_id_1_25': '1EeWXzPdhSA',
             'youtube_id_1_5': 'rABDYkeK0x8',
             'show_captions': False,
-            'start_time': 1.0,
-            'end_time': 60,
+            'start_time': datetime.timedelta(seconds=1),
+            'end_time': datetime.timedelta(seconds=60),
             'track': 'http://www.example.com/track',
             'html5_sources': ['http://www.example.com/source.mp4'],
             'data': ''
@@ -444,8 +427,8 @@ class VideoDescriptorImportTestCase(unittest.TestCase):
             'youtube_id_1_25': '1EeWXzPdhSA',
             'youtube_id_1_5': 'rABDYkeK0x8',
             'show_captions': False,
-            'start_time': 1.0,
-            'end_time': 60.0,
+            'start_time': datetime.timedelta(seconds=1),
+            'end_time': datetime.timedelta(seconds=60),
             'track': 'http://www.example.com/track',
             'html5_sources': ['http://www.example.com/source.mp4'],
             'data': ''
@@ -474,8 +457,8 @@ class VideoExportTestCase(unittest.TestCase):
         desc.youtube_id_1_25 = '1EeWXzPdhSA'
         desc.youtube_id_1_5 = 'rABDYkeK0x8'
         desc.show_captions = False
-        desc.start_time = 1.0
-        desc.end_time = 60
+        desc.start_time = datetime.timedelta(seconds=1.0)
+        desc.end_time = datetime.timedelta(seconds=60)
         desc.track = 'http://www.example.com/track'
         desc.html5_sources = ['http://www.example.com/source.mp4', 'http://www.example.com/source.ogg']
 
@@ -490,6 +473,33 @@ class VideoExportTestCase(unittest.TestCase):
 
         self.assertXmlEqual(expected, xml)
 
+    def test_export_to_xml_empty_end_time(self):
+        """Test that we write the correct XML on export."""
+        module_system = DummySystem(load_error_modules=True)
+        location = Location(["i4x", "edX", "video", "default", "SampleProblem1"])
+        desc = VideoDescriptor(module_system, DictFieldData({}), ScopeIds(None, None, location, location))
+
+        desc.youtube_id_0_75 = 'izygArpw-Qo'
+        desc.youtube_id_1_0 = 'p2Q6BrNhdh8'
+        desc.youtube_id_1_25 = '1EeWXzPdhSA'
+        desc.youtube_id_1_5 = 'rABDYkeK0x8'
+        desc.show_captions = False
+        desc.start_time = datetime.timedelta(seconds=5.0)
+        desc.end_time = datetime.timedelta(seconds=0.0)
+        desc.track = 'http://www.example.com/track'
+        desc.html5_sources = ['http://www.example.com/source.mp4', 'http://www.example.com/source.ogg']
+
+        xml = desc.definition_to_xml(None)  # We don't use the `resource_fs` parameter
+        expected = etree.fromstring('''\
+         <video url_name="SampleProblem1" start_time="0:00:05" youtube="0.75:izygArpw-Qo,1.00:p2Q6BrNhdh8,1.25:1EeWXzPdhSA,1.50:rABDYkeK0x8" show_captions="false">
+           <source src="http://www.example.com/source.mp4"/>
+           <source src="http://www.example.com/source.ogg"/>
+           <track src="http://www.example.com/track"/>
+         </video>
+        ''')
+
+        self.assertXmlEqual(expected, xml)
+
     def test_export_to_xml_empty_parameters(self):
         """Test XML export with defaults."""
         module_system = DummySystem(load_error_modules=True)

common/lib/xmodule/xmodule/video_module.py

@@ -36,34 +36,6 @@ from xblock.runtime import DbModel
 log = logging.getLogger(__name__)
 
 
-
-def parse_time_from_str_to_float(str_time):
-    """
-    Converts s in '12:34:45' format to seconds.
-
-    If s is None, returns 0"""
-    if not str_time:
-        return 0
-    else:
-        obj_time = time.strptime(str_time, '%H:%M:%S')
-        return datetime.timedelta(
-            hours=obj_time.tm_hour,
-            minutes=obj_time.tm_min,
-            seconds=obj_time.tm_sec
-        ).total_seconds()
-
-
-def parse_time_from_float_to_str(s):
-    """
-    Converts s from seconds to  '12:34:45' format.
-
-    If s is None, returns "00:00:00"
-    """
-    if not s:
-        return "00:00:00"
-    else:
-        return str(datetime.timedelta(seconds=s))
-
 class VideoFields(object):
     """Fields for `VideoModule` and `VideoDescriptor`."""
     display_name = String(
@@ -108,18 +80,20 @@ class VideoFields(object):
         scope=Scope.settings,
         default=""
     )
-    start_time = IsoTime(
+    start_time = IsoTime(  # datetime.timedelta object
         help="Start time for the video.",
         display_name="Start Time",
         scope=Scope.settings,
-        default="00:00:00"
+        default=datetime.timedelta(seconds=0)
     )
-    end_time = IsoTime(
+    end_time = IsoTime(  # datetime.timedelta object
         help="End time for the video.",
         display_name="End Time",
         scope=Scope.settings,
-        default="00:00:00"
+        default=datetime.timedelta(seconds=0)
     )
+    #front-end code of video player checks logical validity of (start_time, end_time) pair.
+
     source = String(
         help="The external URL to download the video. This appears as a link beneath the video.",
         display_name="Download Video",
@@ -211,8 +185,8 @@ class VideoModule(VideoFields, XModule):
             'data_dir': getattr(self, 'data_dir', None),
             'caption_asset_path': caption_asset_path,
             'show_captions': json.dumps(self.show_captions),
-            'start': parse_time_from_str_to_float(self.start_time),
-            'end': parse_time_from_str_to_float(self.end_time),
+            'start': self.start_time.total_seconds(),
+            'end': self.end_time.total_seconds(),
             'autoplay': settings.MITX_FEATURES.get('AUTOPLAY_VIDEOS', False),
             # TODO: Later on the value 1500 should be taken from some global
             # configuration setting field.
@@ -388,9 +362,10 @@ class VideoDescriptor(VideoFields, TabsEditingDescriptor, EmptyDataRawDescriptor
         xml = etree.fromstring(xml_data)
         field_data = {}
 
+        # Convert between key types for certain attributes --
+        # necessary for backwards compatibility.
         conversions = {
-            # 'start_time': cls._parse_time,
-            # 'end_time': cls._parse_time
+            # example: 'start_time': cls._example_convert_start_time
         }
 
         # Convert between key names for certain attributes --
@@ -435,24 +410,6 @@ class VideoDescriptor(VideoFields, TabsEditingDescriptor, EmptyDataRawDescriptor
 
         return field_data
 
-    @classmethod
-    def _parse_time(cls, str_time):
-        """Converts s in '12:34:45' format to seconds. If s is
-        None, returns empty string"""
-        if not str_time:
-            return ''
-        else:
-            try:
-                obj_time = time.strptime(str_time, '%H:%M:%S')
-                return datetime.timedelta(
-                    hours=obj_time.tm_hour,
-                    minutes=obj_time.tm_min,
-                    seconds=obj_time.tm_sec
-                ).total_seconds()
-            except ValueError:
-                # We've seen serialized versions of float in this field
-                return float(str_time)
-
 
 def _create_youtube_string(module):
     """

lms/djangoapps/courseware/tests/test_video_xml.py

@@ -15,7 +15,6 @@ common/lib/xmodule/xmodule/modulestore/tests/factories.py to create the
 course, section, subsection, unit, etc.
 """
 
-import json
 import unittest
 
 from django.conf import settings
@@ -109,21 +108,6 @@ class VideoModuleLogicTest(LogicTest):
         'data': '<video />'
     }
 
-    def test_parse_time(self):
-        """Ensure that times are parsed correctly into seconds."""
-        output = VideoDescriptor._parse_time('00:04:07')
-        self.assertEqual(output, 247)
-
-    def test_parse_time_none(self):
-        """Check parsing of None."""
-        output = VideoDescriptor._parse_time(None)
-        self.assertEqual(output, '')
-
-    def test_parse_time_empty(self):
-        """Check parsing of the empty string."""
-        output = VideoDescriptor._parse_time('')
-        self.assertEqual(output, '')
-
     def test_parse_youtube(self):
         """Test parsing old-style Youtube ID strings into a dict."""
         youtube_str = '0.75:jNCf2gIqpeE,1.00:ZwkTiUPN0mg,1.25:rsq9auxASqI,1.50:kMyNdzVHHgg'