more CGI fixes and tests

Author: Richard Jones

CHANGES.txt

@@ -15,6 +15,9 @@ are given with the most recent entry first.
 - added mysql backend
 - fixes to CGI form handling
 - switch metakit to use "compressed" multilink journal change representation
+- fixed bug in metakit unlink journalling
+- metakit now handles "unset" for most types (not Number and Boolean)
+- fixed bug in metakit search-by-ID
 - applied unicode patch. All data is stored in utf-8. Incoming messages
   converted from any encoding to utf-8, outgoing messages are encoded 
   according to rfc2822 (sf bug 568873)

roundup/cgi/client.py

@@ -1,4 +1,4 @@
-# $Id: client.py,v 1.71 2003-01-15 22:39:07 richard Exp $
+# $Id: client.py,v 1.72 2003-01-20 23:05:19 richard Exp $
 
 __doc__ = """
 WWW request handler (also used in the stand-alone server).
@@ -1254,10 +1254,8 @@ def parsePropsFromForm(db, cl, form, nodeid=0, num_re=re.compile('^\d+$')):
             # surrounding whitespace
             value = value.value.strip()
 
-        if isinstance(proptype, hyperdb.String):
-            # fix the CRLF/CR -> LF stuff
-            value = fixNewlines(value)
-        elif isinstance(proptype, hyperdb.Password):
+        # handle by type now
+        if isinstance(proptype, hyperdb.Password):
             if not value:
                 # ignore empty password values
                 continue
@@ -1270,16 +1268,7 @@ def parsePropsFromForm(db, cl, form, nodeid=0, num_re=re.compile('^\d+$')):
             if value != confirm.value:
                 raise ValueError, 'Password and confirmation text do not match'
             value = password.Password(value)
-        elif isinstance(proptype, hyperdb.Date):
-            if value:
-                value = date.Date(value)
-            else:
-                value = None
-        elif isinstance(proptype, hyperdb.Interval):
-            if value:
-                value = date.Interval(value)
-            else:
-                value = None
+
         elif isinstance(proptype, hyperdb.Link):
             # see if it's the "no selection" choice
             if value == '-1' or not value:
@@ -1354,14 +1343,24 @@ def parsePropsFromForm(db, cl, form, nodeid=0, num_re=re.compile('^\d+$')):
                 value = existing
                 value.sort()
 
-        elif isinstance(proptype, hyperdb.Boolean):
-            value = value.lower() in ('yes', 'true', 'on', '1')
-        elif isinstance(proptype, hyperdb.Number):
-            value = int(value)
-
-        # register this as received if required?
-        if propname in required and value is not None:
-            required.remove(propname)
+        # other types should be None'd if there's no value
+        elif value:
+            if isinstance(proptype, hyperdb.String):
+                # fix the CRLF/CR -> LF stuff
+                value = fixNewlines(value)
+            elif isinstance(proptype, hyperdb.Date):
+                value = date.Date(value)
+            elif isinstance(proptype, hyperdb.Interval):
+                value = date.Interval(value)
+            elif isinstance(proptype, hyperdb.Boolean):
+                value = value.lower() in ('yes', 'true', 'on', '1')
+            elif isinstance(proptype, hyperdb.Number):
+                value = int(value)
+        else:
+            # if we're creating, just don't include this property
+            if not nodeid:
+                continue
+            value = None
 
         # get the old value
         if nodeid:
@@ -1373,23 +1372,40 @@ def parsePropsFromForm(db, cl, form, nodeid=0, num_re=re.compile('^\d+$')):
                 if not properties.has_key(propname):
                     raise
 
-            # existing may be None, which won't equate to empty strings
-            if not existing and not value:
-                continue
-
-            # existing will come out unsorted in some cases
+            # make sure the existing multilink is sorted
             if isinstance(proptype, hyperdb.Multilink):
                 existing.sort()
 
+            # "missing" existing values may not be None
+            if not existing:
+                if isinstance(proptype, hyperdb.String) and not existing:
+                    # some backends store "missing" Strings as empty strings
+                    existing = None
+                elif isinstance(proptype, hyperdb.Number) and not existing:
+                    # some backends store "missing" Numbers as 0 :(
+                    existing = 0
+                elif isinstance(proptype, hyperdb.Boolean) and not existing:
+                    # likewise Booleans
+                    existing = 0
+
             # if changed, set it
             if value != existing:
                 props[propname] = value
         else:
             # don't bother setting empty/unset values
-            if not value:
+            if value is None:
+                continue
+            elif isinstance(proptype, hyperdb.Multilink) and value == []:
                 continue
+            elif isinstance(proptype, hyperdb.String) and value == '':
+                continue
+
             props[propname] = value
 
+        # register this as received if required?
+        if propname in required and value is not None:
+            required.remove(propname)
+
     # see if all the required properties have been supplied
     if required:
         if len(required) > 1:
@@ -1400,4 +1416,3 @@ def parsePropsFromForm(db, cl, form, nodeid=0, num_re=re.compile('^\d+$')):
 
     return props
 
-

test/test_cgi.py

@@ -8,12 +8,12 @@
 # but WITHOUT ANY WARRANTY; without even the implied warranty of
 # MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
 #
-# $Id: test_cgi.py,v 1.5 2003-01-15 22:39:07 richard Exp $
+# $Id: test_cgi.py,v 1.6 2003-01-20 23:05:20 richard Exp $
 
 import unittest, os, shutil, errno, sys, difflib, cgi
 
 from roundup.cgi import client
-from roundup import init, instance, password
+from roundup import init, instance, password, hyperdb, date
 
 def makeForm(args):
     form = cgi.FieldStorage()
@@ -43,6 +43,11 @@ def setUp(self):
         self.db.user.create(username='mary', address='mary@test',
             roles='User', realname='Contrary, Mary')
 
+        test = self.instance.dbinit.Class(self.db, "test",
+            boolean=hyperdb.Boolean(), link=hyperdb.Link('test'),
+            multilink=hyperdb.Multilink('test'), date=hyperdb.Date(),
+            interval=hyperdb.Interval())
+
     def tearDown(self):
         self.db.close()
         try:
@@ -66,6 +71,19 @@ def testNothingWithRequired(self):
         self.assertRaises(ValueError, client.parsePropsFromForm, self.db,
             self.db.issue, makeForm({':required': ['title','status'],
             'status':'1'}))
+        self.assertRaises(ValueError, client.parsePropsFromForm, self.db,
+            self.db.issue, makeForm({':required': 'status',
+            'status':''}))
+        self.assertRaises(ValueError, client.parsePropsFromForm, self.db,
+            self.db.issue, makeForm({':required': 'nosy',
+            'nosy':''}))
+
+    #
+    # Nonexistant edit
+    #
+    def testEditNonexistant(self):
+        self.assertRaises(IndexError, client.parsePropsFromForm, self.db,
+            self.db.test, makeForm({'boolean': ''}), '1')
 
     #
     # String
@@ -83,14 +101,17 @@ def testSetString(self):
             makeForm({'title': 'foo'})), {'title': 'foo'})
         self.assertEqual(client.parsePropsFromForm(self.db, self.db.issue,
             makeForm({'title': 'a\r\nb\r\n'})), {'title': 'a\nb'})
+        nodeid = self.db.issue.create(title='foo')
+        self.assertEqual(client.parsePropsFromForm(self.db, self.db.issue,
+            makeForm({'title': 'foo'}), nodeid), {})
 
     def testEmptyStringSet(self):
         nodeid = self.db.issue.create(title='foo')
         self.assertEqual(client.parsePropsFromForm(self.db, self.db.issue,
-            makeForm({'title': ''}), nodeid), {'title': ''})
+            makeForm({'title': ''}), nodeid), {'title': None})
         nodeid = self.db.issue.create(title='foo')
         self.assertEqual(client.parsePropsFromForm(self.db, self.db.issue,
-            makeForm({'title': ' '}), nodeid), {'title': ''})
+            makeForm({'title': ' '}), nodeid), {'title': None})
 
     #
     # Link
@@ -110,6 +131,9 @@ def testSetLink(self):
             makeForm({'status': 'unread'})), {'status': '1'})
         self.assertEqual(client.parsePropsFromForm(self.db, self.db.issue,
             makeForm({'status': '1'})), {'status': '1'})
+        nodeid = self.db.issue.create(status='unread')
+        self.assertEqual(client.parsePropsFromForm(self.db, self.db.issue,
+            makeForm({'status': 'unread'}), nodeid), {})
 
     def testUnsetLink(self):
         nodeid = self.db.issue.create(status='unread')
@@ -122,7 +146,9 @@ def testInvalidLinkValue(self):
 #            self.db.issue, makeForm({'status': '4'}))
         self.assertRaises(ValueError, client.parsePropsFromForm, self.db,
             self.db.issue, makeForm({'status': 'frozzle'}))
-# XXX need a test for the TypeError where the link class doesn't define a key?
+
+        self.assertRaises(ValueError, client.parsePropsFromForm, self.db,
+            self.db.test, makeForm({'link': 'frozzle'}))
 
     #
     # Multilink
@@ -152,6 +178,8 @@ def testEmptyMultilinkSet(self):
         nodeid = self.db.issue.create(nosy=['1','2'])
         self.assertEqual(client.parsePropsFromForm(self.db, self.db.issue,
             makeForm({'nosy': ' '}), nodeid), {'nosy': []})
+        self.assertEqual(client.parsePropsFromForm(self.db, self.db.issue,
+            makeForm({'nosy': '1,2'}), nodeid), {})
 
     def testInvalidMultilinkValue(self):
 # XXX This is not the current behaviour - should we enforce this?
@@ -161,7 +189,9 @@ def testInvalidMultilinkValue(self):
             self.db.issue, makeForm({'nosy': 'frozzle'}))
         self.assertRaises(ValueError, client.parsePropsFromForm, self.db,
             self.db.issue, makeForm({'nosy': '1,frozzle'}))
-# XXX need a test for the TypeError (where the ML class doesn't define a key?
+
+        self.assertRaises(ValueError, client.parsePropsFromForm, self.db,
+            self.db.test, makeForm({'multilink': 'frozzle'}))
 
     def testMultilinkAdd(self):
         nodeid = self.db.issue.create(nosy=['1'])
@@ -241,40 +271,73 @@ def testSetPasswordConfirmBad(self):
             self.db.user, makeForm({'password': 'foo',
             'password:confirm': 'bar'}))
 
-    def testEmptyPasswordNOTSet(self):
-        nodeid = self.db.user.create(username='1', password=password.Password('foo'))
+    def testEmptyPasswordNotSet(self):
+        nodeid = self.db.user.create(username='1',
+            password=password.Password('foo'))
         self.assertEqual(client.parsePropsFromForm(self.db, self.db.user,
             makeForm({'password': ''}), nodeid), {})
-        nodeid = self.db.user.create(username='2', password=password.Password('foo'))
+        nodeid = self.db.user.create(username='2',
+            password=password.Password('foo'))
         self.assertEqual(client.parsePropsFromForm(self.db, self.db.user,
             makeForm({'password': '', 'password:confirm': ''}), nodeid), {})
 
     #
     # Boolean
     #
-# XXX this needs a property to work on.
-#    def testEmptyBoolean(self):
-#        self.assertEqual(client.parsePropsFromForm(self.db, self.db.issue,
-#            makeForm({'title': ''})), {})
-#        self.assertEqual(client.parsePropsFromForm(self.db, self.db.issue,
-#            makeForm({'title': ' '})), {})
-#        self.assertRaises(ValueError, client.parsePropsFromForm, self.db,
-#            self.db.issue, makeForm({'title': ['', '']}))
-
-#    def testSetBoolean(self):
-#        self.assertEqual(client.parsePropsFromForm(self.db, self.db.issue,
-#            makeForm({'title': 'foo'})), {'title': 'foo'})
-#        self.assertEqual(client.parsePropsFromForm(self.db, self.db.issue,
-#            makeForm({'title': 'a\r\nb\r\n'})), {'title': 'a\nb'})
-
-#    def testEmptyBooleanSet(self):
-#        nodeid = self.db.issue.create(title='foo')
-#        self.assertEqual(client.parsePropsFromForm(self.db, self.db.issue,
-#            makeForm({'title': ''}), nodeid), {'title': ''})
-#        nodeid = self.db.issue.create(title='foo')
-#        self.assertEqual(client.parsePropsFromForm(self.db, self.db.issue,
-#            makeForm({'title': ' '}), nodeid), {'title': ''})
+    def testEmptyBoolean(self):
+        self.assertEqual(client.parsePropsFromForm(self.db, self.db.test,
+            makeForm({'boolean': ''})), {})
+        self.assertEqual(client.parsePropsFromForm(self.db, self.db.test,
+            makeForm({'boolean': ' '})), {})
+        self.assertRaises(ValueError, client.parsePropsFromForm, self.db,
+            self.db.test, makeForm({'boolean': ['', '']}))
+
+    def testSetBoolean(self):
+        self.assertEqual(client.parsePropsFromForm(self.db, self.db.test,
+            makeForm({'boolean': 'yes'})), {'boolean': 1})
+        self.assertEqual(client.parsePropsFromForm(self.db, self.db.test,
+            makeForm({'boolean': 'a\r\nb\r\n'})), {'boolean': 0})
+        nodeid = self.db.test.create(boolean=1)
+        self.assertEqual(client.parsePropsFromForm(self.db, self.db.test,
+            makeForm({'boolean': 'yes'}), nodeid), {})
+        nodeid = self.db.test.create(boolean=0)
+        self.assertEqual(client.parsePropsFromForm(self.db, self.db.test,
+            makeForm({'boolean': 'no'}), nodeid), {})
+
+    def testEmptyBooleanSet(self):
+        nodeid = self.db.test.create(boolean=0)
+        self.assertEqual(client.parsePropsFromForm(self.db, self.db.test,
+            makeForm({'boolean': ''}), nodeid), {'boolean': None})
+        nodeid = self.db.test.create(boolean=1)
+        self.assertEqual(client.parsePropsFromForm(self.db, self.db.test,
+            makeForm({'boolean': ' '}), nodeid), {'boolean': None})
 
+    #
+    # Date
+    #
+    def testEmptyDate(self):
+        self.assertEqual(client.parsePropsFromForm(self.db, self.db.test,
+            makeForm({'date': ''})), {})
+        self.assertEqual(client.parsePropsFromForm(self.db, self.db.test,
+            makeForm({'date': ' '})), {})
+        self.assertRaises(ValueError, client.parsePropsFromForm, self.db,
+            self.db.test, makeForm({'date': ['', '']}))
+
+    def testSetDate(self):
+        self.assertEqual(client.parsePropsFromForm(self.db, self.db.test,
+            makeForm({'date': '2003-01-01'})),
+            {'date': date.Date('2003-01-01')})
+        nodeid = self.db.test.create(date=date.Date('2003-01-01'))
+        self.assertEqual(client.parsePropsFromForm(self.db, self.db.test,
+            makeForm({'date': '2003-01-01'}), nodeid), {})
+
+    def testEmptyDateSet(self):
+        nodeid = self.db.test.create(date=date.Date('.'))
+        self.assertEqual(client.parsePropsFromForm(self.db, self.db.test,
+            makeForm({'date': ''}), nodeid), {'date': None})
+        nodeid = self.db.test.create(date=date.Date('1970-01-01.00:00:00'))
+        self.assertEqual(client.parsePropsFromForm(self.db, self.db.test,
+            makeForm({'date': ' '}), nodeid), {'date': None})
 
 def suite():
     l = [unittest.makeSuite(FormTestCase),