Merge pull request #1729 from MITx/bug/dhm/date-parse

Bug/dhm/date parse

cms/djangoapps/contentstore/tests/test_course_settings.py

@@ -22,25 +22,61 @@ from xmodule.modulestore.tests.factories import CourseFactory
 from models.settings.course_metadata import CourseMetadata
 from xmodule.modulestore.xml_importer import import_from_xml
 from xmodule.modulestore.django import modulestore
+import time
 
 
 # YYYY-MM-DDThh:mm:ss.s+/-HH:MM
 class ConvertersTestCase(TestCase):
     @staticmethod
     def struct_to_datetime(struct_time):
-        return datetime.datetime(struct_time.tm_year, struct_time.tm_mon, struct_time.tm_mday, struct_time.tm_hour,
-                                 struct_time.tm_min, struct_time.tm_sec, tzinfo=UTC())
+        return datetime.datetime(struct_time.tm_year, struct_time.tm_mon,
+            struct_time.tm_mday, struct_time.tm_hour,
+            struct_time.tm_min, struct_time.tm_sec, tzinfo=UTC())
 
     def compare_dates(self, date1, date2, expected_delta):
         dt1 = ConvertersTestCase.struct_to_datetime(date1)
         dt2 = ConvertersTestCase.struct_to_datetime(date2)
-        self.assertEqual(dt1 - dt2, expected_delta, str(date1) + "-" + str(date2) + "!=" + str(expected_delta))
+        self.assertEqual(dt1 - dt2, expected_delta, str(date1) + "-"
+            + str(date2) + "!=" + str(expected_delta))
 
     def test_iso_to_struct(self):
-        self.compare_dates(converters.jsdate_to_time("2013-01-01"), converters.jsdate_to_time("2012-12-31"), datetime.timedelta(days=1))
-        self.compare_dates(converters.jsdate_to_time("2013-01-01T00"), converters.jsdate_to_time("2012-12-31T23"), datetime.timedelta(hours=1))
-        self.compare_dates(converters.jsdate_to_time("2013-01-01T00:00"), converters.jsdate_to_time("2012-12-31T23:59"), datetime.timedelta(minutes=1))
-        self.compare_dates(converters.jsdate_to_time("2013-01-01T00:00:00"), converters.jsdate_to_time("2012-12-31T23:59:59"), datetime.timedelta(seconds=1))
+        '''Test conversion from iso compatible date strings to struct_time'''
+        self.compare_dates(converters.jsdate_to_time("2013-01-01"),
+            converters.jsdate_to_time("2012-12-31"),
+            datetime.timedelta(days=1))
+        self.compare_dates(converters.jsdate_to_time("2013-01-01T00"),
+            converters.jsdate_to_time("2012-12-31T23"),
+            datetime.timedelta(hours=1))
+        self.compare_dates(converters.jsdate_to_time("2013-01-01T00:00"),
+            converters.jsdate_to_time("2012-12-31T23:59"),
+            datetime.timedelta(minutes=1))
+        self.compare_dates(converters.jsdate_to_time("2013-01-01T00:00:00"),
+            converters.jsdate_to_time("2012-12-31T23:59:59"),
+            datetime.timedelta(seconds=1))
+        self.compare_dates(converters.jsdate_to_time("2013-01-01T00:00:00Z"),
+            converters.jsdate_to_time("2012-12-31T23:59:59Z"),
+            datetime.timedelta(seconds=1))
+        self.compare_dates(
+            converters.jsdate_to_time("2012-12-31T23:00:01-01:00"),
+            converters.jsdate_to_time("2013-01-01T00:00:00+01:00"),
+            datetime.timedelta(hours=1, seconds=1))
+
+    def test_struct_to_iso(self):
+        '''
+        Test converting time reprs to iso dates
+        '''
+        self.assertEqual(
+            converters.time_to_isodate(
+                time.strptime("2012-12-31T23:59:59Z", "%Y-%m-%dT%H:%M:%SZ")),
+            "2012-12-31T23:59:59Z")
+        self.assertEqual(
+            converters.time_to_isodate(
+                jsdate_to_time("2012-12-31T23:59:59Z")),
+            "2012-12-31T23:59:59Z")
+        self.assertEqual(
+            converters.time_to_isodate(
+                jsdate_to_time("2012-12-31T23:00:01-01:00")),
+            "2013-01-01T00:00:01Z")
 
 
 class CourseTestCase(ModuleStoreTestCase):
@@ -104,7 +140,7 @@ class CourseDetailsTestCase(CourseTestCase):
         self.assertIsNone(jsondetails['effort'], "effort somehow initialized")
 
     def test_update_and_fetch(self):
-        ## NOTE: I couldn't figure out how to validly test time setting w/ all the conversions
+        # # NOTE: I couldn't figure out how to validly test time setting w/ all the conversions
         jsondetails = CourseDetails.fetch(self.course_location)
         jsondetails.syllabus = "<a href='foo'>bar</a>"
         # encode - decode to convert date fields and other data which changes form
@@ -182,7 +218,7 @@ class CourseDetailsViewTest(CourseTestCase):
                     details_encoded = jsdate_to_time(details[field])
                     dt2 = ConvertersTestCase.struct_to_datetime(details_encoded)
 
-                expected_delta =  datetime.timedelta(0)
+                expected_delta = datetime.timedelta(0)
                 self.assertEqual(dt1 - dt2, expected_delta, str(dt1) + "!=" + str(dt2) + " at " + context)
             else:
                 self.fail(field + " missing from encoded but in details at " + context)
@@ -269,7 +305,7 @@ class CourseMetadataEditingTest(CourseTestCase):
         CourseTestCase.setUp(self)
         # add in the full class too
         import_from_xml(modulestore(), 'common/test/data/', ['full'])
-        self.fullcourse_location = Location(['i4x','edX','full','course','6.002_Spring_2012', None])
+        self.fullcourse_location = Location(['i4x', 'edX', 'full', 'course', '6.002_Spring_2012', None])
 
 
     def test_fetch_initial_fields(self):

cms/djangoapps/models/settings/course_metadata.py

@@ -3,19 +3,24 @@ from contentstore.utils import get_modulestore
 from xmodule.x_module import XModuleDescriptor
 from xmodule.modulestore.inheritance import own_metadata
 from xblock.core import Scope
+from xmodule.course_module import CourseDescriptor
 
 
 class CourseMetadata(object):
     '''
-    For CRUD operations on metadata fields which do not have specific editors on the other pages including any user generated ones.
-    The objects have no predefined attrs but instead are obj encodings of the editable metadata.
+    For CRUD operations on metadata fields which do not have specific editors
+    on the other pages including any user generated ones.
+    The objects have no predefined attrs but instead are obj encodings of the
+    editable metadata.
     '''
-    FILTERED_LIST = XModuleDescriptor.system_metadata_fields + ['start', 'end', 'enrollment_start', 'enrollment_end', 'tabs', 'graceperiod', 'checklists']
+    FILTERED_LIST = XModuleDescriptor.system_metadata_fields + ['start', 'end',
+        'enrollment_start', 'enrollment_end', 'tabs', 'graceperiod', 'checklists']
 
     @classmethod
     def fetch(cls, course_location):
         """
-        Fetch the key:value editable course details for the given course from persistence and return a CourseMetadata model.
+        Fetch the key:value editable course details for the given course from
+        persistence and return a CourseMetadata model.
         """
         if not isinstance(course_location, Location):
             course_location = Location(course_location)
@@ -29,7 +34,7 @@ class CourseMetadata(object):
                 continue
 
             if field.name not in cls.FILTERED_LIST:
-                course[field.name] = field.read_from(descriptor)
+                course[field.name] = field.read_json(descriptor)
 
         return course
 
@@ -51,22 +56,26 @@ class CourseMetadata(object):
 
             if hasattr(descriptor, k) and getattr(descriptor, k) != v:
                 dirty = True
-                setattr(descriptor, k, v)
+                value = getattr(CourseDescriptor, k).from_json(v)
+                setattr(descriptor, k, value)
             elif hasattr(descriptor.lms, k) and getattr(descriptor.lms, k) != k:
                 dirty = True
-                setattr(descriptor.lms, k, v)
+                value = getattr(CourseDescriptor.lms, k).from_json(v)
+                setattr(descriptor.lms, k, value)
 
         if dirty:
-            get_modulestore(course_location).update_metadata(course_location, own_metadata(descriptor))
+            get_modulestore(course_location).update_metadata(course_location,
+                own_metadata(descriptor))
 
-        # Could just generate and return a course obj w/o doing any db reads, but I put the reads in as a means to confirm
-        # it persisted correctly
+        # Could just generate and return a course obj w/o doing any db reads,
+        # but I put the reads in as a means to confirm it persisted correctly
         return cls.fetch(course_location)
 
     @classmethod
     def delete_key(cls, course_location, payload):
         '''
-        Remove the given metadata key(s) from the course. payload can be a single key or [key..]
+        Remove the given metadata key(s) from the course. payload can be a
+        single key or [key..]
         '''
         descriptor = get_modulestore(course_location).get_item(course_location)
 
@@ -76,6 +85,7 @@ class CourseMetadata(object):
             elif hasattr(descriptor.lms, key):
                 delattr(descriptor.lms, key)
 
-        get_modulestore(course_location).update_metadata(course_location, own_metadata(descriptor))
+        get_modulestore(course_location).update_metadata(course_location,
+            own_metadata(descriptor))
 
         return cls.fetch(course_location)

cms/urls.py

@@ -42,36 +42,52 @@ urlpatterns = ('',
         'contentstore.views.remove_user', name='remove_user'),
     url(r'^(?P<org>[^/]+)/(?P<course>[^/]+)/course/(?P<name>[^/]+)/remove_user$',
         'contentstore.views.remove_user', name='remove_user'),
-    url(r'^(?P<org>[^/]+)/(?P<course>[^/]+)/info/(?P<name>[^/]+)$', 'contentstore.views.course_info', name='course_info'),
-    url(r'^(?P<org>[^/]+)/(?P<course>[^/]+)/course_info/updates/(?P<provided_id>.*)$', 'contentstore.views.course_info_updates', name='course_info_json'),
-    url(r'^(?P<org>[^/]+)/(?P<course>[^/]+)/settings-details/(?P<name>[^/]+)$', 'contentstore.views.get_course_settings', name='settings_details'),
-    url(r'^(?P<org>[^/]+)/(?P<course>[^/]+)/settings-grading/(?P<name>[^/]+)$', 'contentstore.views.course_config_graders_page', name='settings_grading'),
-    url(r'^(?P<org>[^/]+)/(?P<course>[^/]+)/settings-details/(?P<name>[^/]+)/section/(?P<section>[^/]+).*$', 'contentstore.views.course_settings_updates', name='course_settings'),
-    url(r'^(?P<org>[^/]+)/(?P<course>[^/]+)/settings-grading/(?P<name>[^/]+)/(?P<grader_index>.*)$', 'contentstore.views.course_grader_updates', name='course_settings'),
+    url(r'^(?P<org>[^/]+)/(?P<course>[^/]+)/info/(?P<name>[^/]+)$',
+        'contentstore.views.course_info', name='course_info'),
+    url(r'^(?P<org>[^/]+)/(?P<course>[^/]+)/course_info/updates/(?P<provided_id>.*)$',
+        'contentstore.views.course_info_updates', name='course_info_json'),
+    url(r'^(?P<org>[^/]+)/(?P<course>[^/]+)/settings-details/(?P<name>[^/]+)$',
+        'contentstore.views.get_course_settings', name='settings_details'),
+    url(r'^(?P<org>[^/]+)/(?P<course>[^/]+)/settings-grading/(?P<name>[^/]+)$',
+        'contentstore.views.course_config_graders_page', name='settings_grading'),
+    url(r'^(?P<org>[^/]+)/(?P<course>[^/]+)/settings-details/(?P<name>[^/]+)/section/(?P<section>[^/]+).*$',
+        'contentstore.views.course_settings_updates', name='course_settings'),
+    url(r'^(?P<org>[^/]+)/(?P<course>[^/]+)/settings-grading/(?P<name>[^/]+)/(?P<grader_index>.*)$',
+        'contentstore.views.course_grader_updates', name='course_settings'),
     # This is the URL to initially render the course advanced settings.
-    url(r'^(?P<org>[^/]+)/(?P<course>[^/]+)/settings-advanced/(?P<name>[^/]+)$', 'contentstore.views.course_config_advanced_page', name='course_advanced_settings'),
+    url(r'^(?P<org>[^/]+)/(?P<course>[^/]+)/settings-advanced/(?P<name>[^/]+)$',
+        'contentstore.views.course_config_advanced_page', name='course_advanced_settings'),
     # This is the URL used by BackBone for updating and re-fetching the model.
-    url(r'^(?P<org>[^/]+)/(?P<course>[^/]+)/settings-advanced/(?P<name>[^/]+)/update.*$', 'contentstore.views.course_advanced_updates', name='course_advanced_settings_updates'),
+    url(r'^(?P<org>[^/]+)/(?P<course>[^/]+)/settings-advanced/(?P<name>[^/]+)/update.*$',
+        'contentstore.views.course_advanced_updates', name='course_advanced_settings_updates'),
 
-    url(r'^(?P<org>[^/]+)/(?P<course>[^/]+)/(?P<category>[^/]+)/(?P<name>[^/]+)/gradeas.*$', 'contentstore.views.assignment_type_update', name='assignment_type_update'),
+    url(r'^(?P<org>[^/]+)/(?P<course>[^/]+)/(?P<category>[^/]+)/(?P<name>[^/]+)/gradeas.*$',
+        'contentstore.views.assignment_type_update', name='assignment_type_update'),
 
-    url(r'^pages/(?P<org>[^/]+)/(?P<course>[^/]+)/course/(?P<coursename>[^/]+)$', 'contentstore.views.static_pages',
+    url(r'^pages/(?P<org>[^/]+)/(?P<course>[^/]+)/course/(?P<coursename>[^/]+)$',
+        'contentstore.views.static_pages',
         name='static_pages'),
-    url(r'^edit_static/(?P<org>[^/]+)/(?P<course>[^/]+)/course/(?P<coursename>[^/]+)$', 'contentstore.views.edit_static', name='edit_static'),
-    url(r'^edit_tabs/(?P<org>[^/]+)/(?P<course>[^/]+)/course/(?P<coursename>[^/]+)$', 'contentstore.views.edit_tabs', name='edit_tabs'),
-    url(r'^(?P<org>[^/]+)/(?P<course>[^/]+)/assets/(?P<name>[^/]+)$', 'contentstore.views.asset_index', name='asset_index'),
+    url(r'^edit_static/(?P<org>[^/]+)/(?P<course>[^/]+)/course/(?P<coursename>[^/]+)$',
+        'contentstore.views.edit_static', name='edit_static'),
+    url(r'^edit_tabs/(?P<org>[^/]+)/(?P<course>[^/]+)/course/(?P<coursename>[^/]+)$',
+        'contentstore.views.edit_tabs', name='edit_tabs'),
+    url(r'^(?P<org>[^/]+)/(?P<course>[^/]+)/assets/(?P<name>[^/]+)$',
+        'contentstore.views.asset_index', name='asset_index'),
 
     # this is a generic method to return the data/metadata associated with a xmodule
-    url(r'^module_info/(?P<module_location>.*)$', 'contentstore.views.module_info', name='module_info'),
+    url(r'^module_info/(?P<module_location>.*)$',
+        'contentstore.views.module_info', name='module_info'),
 
 
     # temporary landing page for a course
-    url(r'^edge/(?P<org>[^/]+)/(?P<course>[^/]+)/course/(?P<coursename>[^/]+)$', 'contentstore.views.landing', name='landing'),
+    url(r'^edge/(?P<org>[^/]+)/(?P<course>[^/]+)/course/(?P<coursename>[^/]+)$',
+        'contentstore.views.landing', name='landing'),
 
     url(r'^not_found$', 'contentstore.views.not_found', name='not_found'),
     url(r'^server_error$', 'contentstore.views.server_error', name='server_error'),
 
-    url(r'^(?P<org>[^/]+)/(?P<course>[^/]+)/assets/(?P<name>[^/]+)$', 'contentstore.views.asset_index', name='asset_index'),
+    url(r'^(?P<org>[^/]+)/(?P<course>[^/]+)/assets/(?P<name>[^/]+)$',
+        'contentstore.views.asset_index', name='asset_index'),
 
     # temporary landing page for edge
     url(r'^edge$', 'contentstore.views.edge', name='edge'),

common/djangoapps/util/converters.py

 import time
 import datetime
-import re
 import calendar
+import dateutil.parser
 
 
 def time_to_date(time_obj):
     """
-    Convert a time.time_struct to a true universal time (can pass to js Date constructor)
+    Convert a time.time_struct to a true universal time (can pass to js Date
+    constructor)
     """
-    # TODO change to using the isoformat() function on datetime. js date can parse those
     return calendar.timegm(time_obj) * 1000
 
 
+def time_to_isodate(source):
+    '''Convert to an iso date'''
+    if isinstance(source, time.struct_time):
+        return time.strftime('%Y-%m-%dT%H:%M:%SZ', source)
+    elif isinstance(source, datetime):
+        return source.isoformat() + 'Z'
+
+
 def jsdate_to_time(field):
     """
     Convert a universal time (iso format) or msec since epoch to a time obj
@@ -19,8 +27,7 @@ def jsdate_to_time(field):
     if field is None:
         return field
     elif isinstance(field, basestring):
-        # ISO format but ignores time zone assuming it's Z.
-        d = datetime.datetime(*map(int, re.split('[^\d]', field)[:6]))  # stop after seconds. Debatable
+        d = dateutil.parser.parse(field)
         return d.utctimetuple()
     elif isinstance(field, (int, long, float)):
         return time.gmtime(field / 1000)

common/lib/xmodule/xmodule/course_module.py

 import logging
 from cStringIO import StringIO
-from math import exp, erf
+from math import exp
 from lxml import etree
 from path import path  # NOTE (THK): Only used for detecting presence of syllabus
 import requests
@@ -12,14 +12,9 @@ from xmodule.seq_module import SequenceDescriptor, SequenceModule
 from xmodule.timeparse import parse_time
 from xmodule.util.decorators import lazyproperty
 from xmodule.graders import grader_from_conf
-from datetime import datetime
 import json
-import logging
-import requests
-import time
-import copy
 
-from xblock.core import Scope, ModelType, List, String, Object, Boolean
+from xblock.core import Scope, List, String, Object, Boolean
 from .fields import Date
 
 
@@ -29,30 +24,30 @@ log = logging.getLogger(__name__)
 class StringOrDate(Date):
     def from_json(self, value):
         """
-        Parse an optional metadata key containing a time: if present, complain
-        if it doesn't parse.
-        Return None if not present or invalid.
+        Parse an optional metadata key containing a time or a string:
+        if present, assume it's a string if it doesn't parse.
         """
-        if value is None:
-            return None
-
         try:
-            return time.strptime(value, self.time_format)
+            result = super(StringOrDate, self).from_json(value)
         except ValueError:
             return value
+        if result is None:
+            return value
+        else:
+            return result
 
     def to_json(self, value):
         """
-        Convert a time struct to a string
+        Convert a time struct or string to a string.
         """
-        if value is None:
-            return None
-
         try:
-            return time.strftime(self.time_format, value)
-        except (ValueError, TypeError):
+            result = super(StringOrDate, self).to_json(value)
+        except:
             return value
-
+        if result is None:
+            return value
+        else:
+            return result
 
 
 edx_xml_parser = etree.XMLParser(dtd_validation=False, load_dtd=False,
@@ -60,6 +55,7 @@ edx_xml_parser = etree.XMLParser(dtd_validation=False, load_dtd=False,
 
 _cached_toc = {}
 
+
 class Textbook(object):
     def __init__(self, title, book_url):
         self.title = title
@@ -179,7 +175,7 @@ class CourseFields(object):
     allow_anonymous_to_peers = Boolean(scope=Scope.settings, default=False)
     advanced_modules = List(help="Beta modules used in your course", scope=Scope.settings)
     has_children = True
-    checklists=List(scope=Scope.settings)
+    checklists = List(scope=Scope.settings)
     info_sidebar_name = String(scope=Scope.settings, default='Course Handouts')
 
     # An extra property is used rather than the wiki_slug/number because
@@ -367,7 +363,7 @@ class CourseDescriptor(CourseFields, SequenceDescriptor):
             textbooks.append((textbook.get('title'), textbook.get('book_url')))
             xml_object.remove(textbook)
 
-        #Load the wiki tag if it exists
+        # Load the wiki tag if it exists
         wiki_slug = None
         wiki_tag = xml_object.find("wiki")
         if wiki_tag is not None:
@@ -675,7 +671,7 @@ class CourseDescriptor(CourseFields, SequenceDescriptor):
             # *end* of the same day, not the same time.  It's going to be used as the
             # end of the exam overall, so we don't want the exam to disappear too soon.
             # It's also used optionally as the registration end date, so time matters there too.
-            self.last_eligible_appointment_date = self._try_parse_time('Last_Eligible_Appointment_Date')   # or self.first_eligible_appointment_date
+            self.last_eligible_appointment_date = self._try_parse_time('Last_Eligible_Appointment_Date')  # or self.first_eligible_appointment_date
             if self.last_eligible_appointment_date is None:
                 raise ValueError("Last appointment date must be specified")
             self.registration_start_date = self._try_parse_time('Registration_Start_Date') or time.gmtime(0)

common/lib/xmodule/xmodule/fields.py

@@ -4,27 +4,35 @@ import re
 
 from datetime import timedelta
 from xblock.core import ModelType
+import datetime
+import dateutil.parser
 
 log = logging.getLogger(__name__)
 
 
 class Date(ModelType):
-    time_format = "%Y-%m-%dT%H:%M"
-
-    def from_json(self, value):
+    '''
+    Date fields know how to parse and produce json (iso) compatible formats.
+    '''
+    # NB: these are copies of util.converters.*
+    def from_json(self, field):
         """
         Parse an optional metadata key containing a time: if present, complain
         if it doesn't parse.
         Return None if not present or invalid.
         """
-        if value is None:
-            return None
-
-        try:
-            return time.strptime(value, self.time_format)
-        except ValueError as e:
-            msg = "Field {0} has bad value '{1}': '{2}'".format(
-                self._name, value, e)
+        if field is None:
+            return field
+        elif isinstance(field, basestring):
+            d = dateutil.parser.parse(field)
+            return d.utctimetuple()
+        elif isinstance(field, (int, long, float)):
+            return time.gmtime(field / 1000)
+        elif isinstance(field, time.struct_time):
+            return field
+        else:
+            msg = "Field {0} has bad value '{1}'".format(
+                self._name, field)
             log.warning(msg)
             return None
 
@@ -34,8 +42,11 @@ class Date(ModelType):
         """
         if value is None:
             return None
-
-        return time.strftime(self.time_format, value)
+        if isinstance(value, time.struct_time):
+            # struct_times are always utc
+            return time.strftime('%Y-%m-%dT%H:%M:%SZ', value)
+        elif isinstance(value, datetime.datetime):
+            return value.isoformat() + 'Z'
 
 
 TIMEDELTA_REGEX = re.compile(r'^((?P<days>\d+?) day(?:s?))?(\s)?((?P<hours>\d+?) hour(?:s?))?(\s)?((?P<minutes>\d+?) minute(?:s)?)?(\s)?((?P<seconds>\d+?) second(?:s)?)?$')
@@ -66,4 +77,4 @@ class Timedelta(ModelType):
             cur_value = getattr(value, attr, 0)
             if cur_value > 0:
                 values.append("%d %s" % (cur_value, attr))
-        return ' '.join(values)
+        return ' '.join(values)
\ No newline at end of file