- more fixes to CGI form handling

- switch metakit to use "compressed" multilink journal change representation

Author: Richard Jones

CHANGES.txt

@@ -12,9 +12,9 @@ are given with the most recent entry first.
 - cleaning old unused sessions only once per hour, not on every cgi 
   request. It is greatly improves web interface performance, especially
   on trackers under high load
-- fix StringHTMLProperty hyperlinking
 - added mysql backend
-- fixes to CGI form handling (NEEDS BACKPORTING TO 0.5)
+- fixes to CGI form handling
+- switch metakit to use "compressed" multilink journal change representation
 - 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/backends/back_metakit.py

@@ -475,7 +475,8 @@ def set(self, nodeid, **propvalues):
                         if self.do_journal and prop.do_journal:
                             self.db.addjournal(link_class, id, _LINK,
                                 (self.classname, str(row.id), key))
-                            
+
+                # perform the modifications on the actual property value
                 sv = getattr(row, key)
                 i = 0
                 while i < len(sv):
@@ -485,7 +486,17 @@ def set(self, nodeid, **propvalues):
                         i += 1
                 for id in adds:
                     sv.append(fid=int(id))
-                changes[key] = oldvalue
+
+                # figure the journal entry
+                l = []
+                if adds:
+                    l.append(('+', adds))
+                if rmvd:
+                    l.append(('-', rmvd))
+                if l:
+                    changes[key] = tuple(l)
+                #changes[key] = oldvalue
+
                 if not rmvd and not adds:
                     del propvalues[key]
 

roundup/cgi/client.py

@@ -1,4 +1,4 @@
-# $Id: client.py,v 1.70 2003-01-15 11:14:01 richard Exp $
+# $Id: client.py,v 1.71 2003-01-15 22:39:07 richard Exp $
 
 __doc__ = """
 WWW request handler (also used in the stand-alone server).
@@ -1221,9 +1221,10 @@ def parsePropsFromForm(db, cl, form, nodeid=0, num_re=re.compile('^\d+$')):
 
         # does the property exist?
         if not properties.has_key(propname):
-            if mlaction == 'remove':
-                raise ValueError, 'You have submitted a remove action for'\
-                    ' the property "%s" which doesn\'t exist'%propname
+            if mlaction != 'set':
+                raise ValueError, 'You have submitted a %s action for'\
+                    ' the property "%s" which doesn\'t exist'%(mlaction,
+                    propname)
             continue
         proptype = properties[propname]
 
@@ -1281,7 +1282,7 @@ def parsePropsFromForm(db, cl, form, nodeid=0, num_re=re.compile('^\d+$')):
                 value = None
         elif isinstance(proptype, hyperdb.Link):
             # see if it's the "no selection" choice
-            if value == '-1':
+            if value == '-1' or not value:
                 # if we're creating, just don't include this property
                 if not nodeid:
                     continue

test/test_cgi.py

@@ -8,7 +8,7 @@
 # but WITHOUT ANY WARRANTY; without even the implied warranty of
 # MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
 #
-# $Id: test_cgi.py,v 1.4 2003-01-15 11:14:01 richard Exp $
+# $Id: test_cgi.py,v 1.5 2003-01-15 22:39:07 richard Exp $
 
 import unittest, os, shutil, errno, sys, difflib, cgi
 
@@ -58,9 +58,14 @@ def testNothing(self):
             makeForm({})), {})
 
     def testNothingWithRequired(self):
-        form = makeForm({':required': 'title'})
         self.assertRaises(ValueError, client.parsePropsFromForm, self.db,
-            self.db.issue, form)
+            self.db.issue, makeForm({':required': 'title'}))
+        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': ['title','status'],
+            'status':'1'}))
 
     #
     # String
@@ -87,6 +92,38 @@ def testEmptyStringSet(self):
         self.assertEqual(client.parsePropsFromForm(self.db, self.db.issue,
             makeForm({'title': ' '}), nodeid), {'title': ''})
 
+    #
+    # Link
+    #
+    def testEmptyLink(self):
+        self.assertEqual(client.parsePropsFromForm(self.db, self.db.issue,
+            makeForm({'status': ''})), {})
+        self.assertEqual(client.parsePropsFromForm(self.db, self.db.issue,
+            makeForm({'status': ' '})), {})
+        self.assertRaises(ValueError, client.parsePropsFromForm, self.db,
+            self.db.issue, makeForm({'status': ['', '']}))
+        self.assertEqual(client.parsePropsFromForm(self.db, self.db.issue,
+            makeForm({'status': '-1'})), {})
+
+    def testSetLink(self):
+        self.assertEqual(client.parsePropsFromForm(self.db, self.db.issue,
+            makeForm({'status': 'unread'})), {'status': '1'})
+        self.assertEqual(client.parsePropsFromForm(self.db, self.db.issue,
+            makeForm({'status': '1'})), {'status': '1'})
+
+    def testUnsetLink(self):
+        nodeid = self.db.issue.create(status='unread')
+        self.assertEqual(client.parsePropsFromForm(self.db, self.db.issue,
+            makeForm({'status': '-1'}), nodeid), {'status': None})
+
+    def testInvalidLinkValue(self):
+# XXX This is not the current behaviour - should we enforce this?
+#        self.assertRaises(IndexError, client.parsePropsFromForm, self.db,
+#            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?
+
     #
     # Multilink
     #
@@ -124,7 +161,7 @@ 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
+# XXX need a test for the TypeError (where the ML class doesn't define a key?
 
     def testMultilinkAdd(self):
         nodeid = self.db.issue.create(nosy=['1'])
@@ -162,6 +199,22 @@ def testMultilinkRemove(self):
         self.assertRaises(ValueError, client.parsePropsFromForm, self.db,
             self.db.issue, makeForm({':remove:nosy': '4'}), nodeid)
 
+    def testMultilinkRetired(self):
+        self.db.user.retire('2')
+        self.assertEqual(client.parsePropsFromForm(self.db, self.db.issue,
+            makeForm({'nosy': ['2','3']})), {'nosy': ['2','3']})
+        nodeid = self.db.issue.create(nosy=['1','2'])
+        self.assertEqual(client.parsePropsFromForm(self.db, self.db.issue,
+            makeForm({':remove:nosy': '2'}), nodeid), {'nosy': ['1']})
+        self.assertEqual(client.parsePropsFromForm(self.db, self.db.issue,
+            makeForm({':add:nosy': '3'}), nodeid), {'nosy': ['1','2','3']})
+
+    def testAddRemoveNonexistant(self):
+        self.assertRaises(ValueError, client.parsePropsFromForm, self.db,
+            self.db.issue, makeForm({':remove:foo': '2'}))
+        self.assertRaises(ValueError, client.parsePropsFromForm, self.db,
+            self.db.issue, makeForm({':add:foo': '2'}))
+
     #
     # Password
     #
@@ -196,6 +249,32 @@ def testEmptyPasswordNOTSet(self):
         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 suite():
     l = [unittest.makeSuite(FormTestCase),