Merge lp:~jcsackett/launchpad/dumb-date-formats into lp:launchpad

[j.c.sackett]

[r=benji][bug=134063] Updates DateTime widget to kick back formats it can't possibly cope with.

Summary
=======

On the new sprint page, an OOPS can be triggered when dates that zope.date.parse can't understand get entered. Rather than throwing back an error, parse munges the dates into a format it can deal with, resulting in data that throws of db constraints.

This branch updates the widget to check if the date string is in any supported format, and kicks back an error message (rather than an OOPS) if it isn't.

Preimplementation
=================

Spoke with Curtis Hovey

Proposed Fix
============

Update the widget so it kicks back the input *before* the widget converts it into an erroneous datetime object.

Implementation
==============

The DateTimeWidget in lp.apps.widgets.date is provided with a new attribute, supported_input_formats, which is a list of format strings for datetime.strptime.

It also has a new method, _checkSupportedFormat, which uses the list of format strings to see if a meaningful datetime can be extracted from the input. If not, an error is raised.

Tests
=====

bin/test -m lp.app.widgets.tests

Demo & QA
=========

Try to enter the dates listed in the bug; they should come back with errors, rather than triggering an OOPS.

Lint
=====

make lint output:

= Launchpad lint =
Checking for conflicts and issues in changed files.
Linting changed files:
  lib/lp/app/widgets/date.py

[Benji York (benji)]

This branch looks good. A few things to consider:

Deepening on the text of an exception message can be dangerous. Most
libraries don't consider them to be part of the API, so they may change
without warning. I would add a small test that ensures that the text
you expect ("unconverted data remains") is generated when you expect it
to be. That way if different text gets generated later we'll have a
test specifically about that fail instead of a hard to diagnose edge
case failure.

If that particular behavior had been in an upstream library and not the
Python stdlib, I would have suggested promoting a patch to the library
to raise two different exceptions so you wouldn't have to check the
message. However, given Python's (long) release cycle and the immanent
end of Python 2 releases, I wouldn't bother.

Also, trailing whitespace triggers the "unconverted data remains"
message, so I believe entering a date in an unsupported format with
trailing whitespace will cause the input to be considered valid and
cause an OOPS when it gets deeper into the app.

Someone other than myself might want the new test be done as a unittest
instead of a doctest. Since you're only adding a small test to an
already large set of doctests, I think it's fine as-is.

Lastly, all the accepted formats requires seconds. Do we also want to
accept date/times without seconds?

review: Approve (code)

=== modified file 'lib/lp/app/widgets/date.py'
--- lib/lp/app/widgets/date.py	2011-02-01 21:01:02 +0000
+++ lib/lp/app/widgets/date.py	2011-02-22 15:24:38 +0000
@@ -109,6 +109,8 @@
     """
 
     timeformat = '%Y-%m-%d %H:%M:%S'
+    
+
     required_time_zone = None
     display_zone = True
     from_date = None
@@ -121,7 +123,28 @@
     def __init__(self, context, request):
         request.needs_datetimepicker_iframe = True
         super(DateTimeWidget, self).__init__(context, request)
-        self.system_time_zone = getUtility(ILaunchBag).time_zone
+        launchbag = getUtility(ILaunchBag)
+        self.system_time_zone = launchbag.time_zone
+
+    @property
+    def supported_input_formats(self):
+        date_formats = [
+            '%Y-%m-%d', '%m-%d-%Y', '%m-%d-%y',
+            '%m/%d/%Y', '%m/%d/%y',
+            '%d %b, %Y', '%d %b %Y', '%b %d, %Y', '%b %d %Y',
+            '%d %B, %Y', '%d %B %Y', '%B %d, %Y', 'B% %d %Y',
+            ]
+
+        time_formats = [
+            '%H:%M:%S',
+            '%H:%M',
+            '',
+            ]
+        outputs = []
+        for fmt in time_formats:
+           outputs.extend(['%s %s' % (d, fmt) for d in date_formats])
+
+        return [o.strip() for o in outputs]
 
     #@property  XXX: do as a property when we have python2.5 for tests of
     #properties
@@ -306,6 +329,21 @@
             raise self._error
         return value
 
+    def _checkSupportedFormat(self, input):
+        """Checks that the input is in a usable format."""
+        for fmt in self.supported_input_formats:
+            try:
+                datetime.strptime(input.strip(), fmt)
+            except (ValueError), e:
+                if 'unconverted data remains' in e.message:
+                    return
+                else:
+                    failure = e
+            else:
+                return
+        if failure:
+            raise ConversionError('Invalid date value', failure)
+
     def _toFieldValue(self, input):
         """Return parsed input (datetime) as a date."""
         return self._parseInput(input)
@@ -346,6 +384,7 @@
         """
         if input == self._missing:
             return self.context.missing_value
+        self._checkSupportedFormat(input)
         try:
             year, month, day, hour, minute, second, dummy_tz = parse(input)
             second, micro = divmod(second, 1.0)
@@ -560,6 +599,7 @@
 
 class DatetimeDisplayWidget(DisplayWidget):
     """Display timestamps in the users preferred time zone"""
+
     def __call__(self):
         time_zone = getUtility(ILaunchBag).time_zone
         if self._renderedValueSet():

=== added file 'lib/lp/app/widgets/tests/test_datetime.py'
--- lib/lp/app/widgets/tests/test_datetime.py	1970-01-01 00:00:00 +0000
+++ lib/lp/app/widgets/tests/test_datetime.py	2011-02-22 15:24:38 +0000
@@ -0,0 +1,44 @@
+from datetime import datetime
+
+from zope.app.form.interfaces import ConversionError
+from zope.schema import Field
+
+from canonical.launchpad.webapp.servers import LaunchpadTestRequest
+from canonical.testing.layers import DatabaseFunctionalLayer
+from lp.app.widgets.date import DateTimeWidget
+from lp.testing import TestCase
+
+class TestDateTimeWidget(TestCase):
+
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        super(TestDateTimeWidget, self).setUp()
+        field = Field(__name__='foo', title=u'Foo')
+        request = LaunchpadTestRequest()
+        self.widget = DateTimeWidget(field, request) 
+
+    def test_unsupported_format_errors(self):
+        # Dates in unsupported formats result in a ConversionError
+        self.assertRaises(
+            ConversionError,
+            self.widget._checkSupportedFormat,
+            '15-5-2010')
+
+    def test_unconverted_message(self):
+        # The widget format checker relies on a particular mesage
+        # being returned. If that breaks, this will tell us.
+        test_str = "2010-01-01 10:10:10"
+        fmt = "%Y-%m-%d"
+        try:
+            datetime.strptime(test_str, fmt)
+        except (ValueError,), e:
+            self.assertTrue('unconverted data' in e.message)
+
+    def test_whitespace_does_not_trick_validation(self):
+        # Trailing whitespace doesn't get through because of the
+        # unconverted data issue.
+        self.assertRaises(
+            ConversionError,
+            self.widget._checkSupportedFormat,
+            '15-5-2010 ')