diff --git a/common/lib/xmodule/xmodule/fields.py b/common/lib/xmodule/xmodule/fields.py
index dfb6a4f484..e33bc02b0f 100644
--- a/common/lib/xmodule/xmodule/fields.py
+++ b/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):
- if isinstance(value, float):
- if not value:
- return "00:00:00"
- else:
- return str(datetime.timedelta(seconds=value))
- else:
- return super(IsoTime, self).from_json(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"
+
+ if isinstance(value, float): # backward compatibility
+ if value > 86400:
+ value = 86400
+ return str(datetime.timedelta(seconds=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)
diff --git a/common/lib/xmodule/xmodule/tests/test_video.py b/common/lib/xmodule/xmodule/tests/test_video.py
index b6475c22e1..5e2f915ba4 100644
--- a/common/lib/xmodule/xmodule/tests/test_video.py
+++ b/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': ''
}
- 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('''\
+
+ ''')
+
+ self.assertXmlEqual(expected, xml)
+
def test_export_to_xml_empty_parameters(self):
"""Test XML export with defaults."""
module_system = DummySystem(load_error_modules=True)
diff --git a/common/lib/xmodule/xmodule/video_module.py b/common/lib/xmodule/xmodule/video_module.py
index 99842ce197..30c877917c 100644
--- a/common/lib/xmodule/xmodule/video_module.py
+++ b/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):
"""
diff --git a/lms/djangoapps/courseware/tests/test_video_xml.py b/lms/djangoapps/courseware/tests/test_video_xml.py
index 616c518ffe..cf202db212 100644
--- a/lms/djangoapps/courseware/tests/test_video_xml.py
+++ b/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': ''
}
- 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'