Better handling of the form variable labels. It's done in one step rather

than the split step of last week. This means that "msg-1@link@files" will
work now.

I had to change the password confirmation special var from "name:confirm"
to ":confirm:name" so it conformed with the pattern set by ":add:" etc.

git-svn-id: http://svn.roundup-tracker.org/svnroot/roundup/trunk@1516 57a73879-2fb5-44c3-a270-3262357dd7e2

diff --git a/roundup/cgi/client.py b/roundup/cgi/client.py
index 75c0348f7f783ee0b901391703b647aaf8102d72..3559e75d2975c532ed27348ffb8aca2f651af6b9 100644 (file)
--- a/roundup/cgi/client.py
+++ b/roundup/cgi/client.py
@@ -1,4 +1,4 @@
-# $Id: client.py,v 1.88 2003-02-17 01:04:31 richard Exp $
+# $Id: client.py,v 1.89 2003-02-17 06:44:00 richard Exp $

 
 __doc__ = """
 WWW request handler (also used in the stand-alone server).
 
 __doc__ = """
 WWW request handler (also used in the stand-alone server).


@@ -93,16 +93,27 @@ class Client:
     FV_OK_MESSAGE = re.compile(r'[@:]ok_message')
     FV_ERROR_MESSAGE = re.compile(r'[@:]error_message')
 
     FV_OK_MESSAGE = re.compile(r'[@:]ok_message')
     FV_ERROR_MESSAGE = re.compile(r'[@:]error_message')
 

-    # specials for parsePropsFromForm
-    FV_REQUIRED = re.compile(r'[@:]required')
-    FV_ADD = re.compile(r'([@:])add\1')
-    FV_REMOVE = re.compile(r'([@:])remove\1')
-    FV_CONFIRM = re.compile(r'.+[@:]confirm')
-    FV_LINK = re.compile(r'([@:])link\1(.+)')
-
-    # deprecated
-    FV_NOTE = re.compile(r'[@:]note')
-    FV_FILE = re.compile(r'[@:]file')
+    # edit form variable handling (see unit tests)
+    FV_LABELS = r'''
+       ^(
+         (?P<note>[@:]note)|
+         (?P<file>[@:]file)|
+         (
+          ((?P<classname>%s)(?P<id>[-\d]+))?  # optional leading designator
+          ((?P<required>[@:]required$)|       # :required
+           (
+            (
+             (?P<add>[@:]add[@:])|            # :add:<prop>
+             (?P<remove>[@:]remove[@:])|      # :remove:<prop>
+             (?P<confirm>[@:]confirm[@:])|    # :confirm:<prop>
+             (?P<link>[@:]link[@:])|          # :link:<prop>
+             ([@:])                           # just a separator
+            )?
+            (?P<propname>[^@:]+)             # <prop>
+           )
+          )
+         )
+        )$'''

 
     # Note: index page stuff doesn't appear here:
     # columns, sort, sortdir, filter, group, groupdir, search_text,
 
     # Note: index page stuff doesn't appear here:
     # columns, sort, sortdir, filter, group, groupdir, search_text,


@@ -1097,39 +1108,44 @@ class Client:
         return cl.create(**props)
 
     def parsePropsFromForm(self, num_re=re.compile('^\d+$')):
         return cl.create(**props)
 
     def parsePropsFromForm(self, num_re=re.compile('^\d+$')):

-        ''' Pull properties for the given class out of the form.
+        ''' Pull properties out of the form.

 
             In the following, <bracketed> values are variable, ":" may be
             one of ":" or "@", and other text "required" is fixed.
 
 
             In the following, <bracketed> values are variable, ":" may be
             one of ":" or "@", and other text "required" is fixed.
 

-            Properties are specified as form variables
+            Properties are specified as form variables:
+
+             <propname>
+              - property on the current context item
+

              <designator>:<propname>
              <designator>:<propname>

+              - property on the indicated item
+
+             <classname>-<N>:<propname>
+              - property on the Nth new item of classname

 
 

-            where the propery belongs to the context class or item if the
-            designator is not specified. The designator may specify a
-            negative item id value (ie. "issue-1") and a new item of the
-            specified class will be created for each negative id found.
+            Once we have determined the "propname", we check to see if it
+            is one of the special form values:

 
 

-            If a "<designator>:required" parameter is supplied,
-            then the named property values must be supplied or a
-            ValueError will be raised.
+             :required
+              The named property values must be supplied or a ValueError
+              will be raised.

 
 

-            Other special form values:
-             [classname|designator]:remove:<propname>=id(s)
+             :remove:<propname>=id(s)

               The ids will be removed from the multilink property.
               The ids will be removed from the multilink property.

-             [classname|designator]:add:<propname>=id(s)
+
+             :add:<propname>=id(s)

               The ids will be added to the multilink property.
 
               The ids will be added to the multilink property.
 

-             [classname|designator]:link:<propname>=<classname>
+             :link:<propname>=<designator>

               Used to add a link to new items created during edit.
               These are collected up and returned in all_links. This will
               result in an additional linking operation (either Link set or
               Multilink append) after the edit/create is done using
               Used to add a link to new items created during edit.
               These are collected up and returned in all_links. This will
               result in an additional linking operation (either Link set or
               Multilink append) after the edit/create is done using

-              all_props in _editnodes. The <propname> on
-              [classname|designator] will be set/appended the id of the
-              newly created item of class <classname>.
-
-            Note: the colon may be either ":" or "@".
+              all_props in _editnodes. The <propname> on the current item
+              will be set/appended the id of the newly created item of
+              class <designator> (where <designator> must be
+              <classname>-<N>).

 
             Any of the form variables may be prefixed with a classname or
             designator.
 
             Any of the form variables may be prefixed with a classname or
             designator.


@@ -1149,19 +1165,21 @@ class Client:
             Two special form values are supported for backwards
             compatibility:
              :note - create a message (with content, author and date), link
             Two special form values are supported for backwards
             compatibility:
              :note - create a message (with content, author and date), link

-                     to the context item
+                     to the context item. This is ALWAYS desginated "msg-1".

              :file - create a file, attach to the current item and any
              :file - create a file, attach to the current item and any

-                     message created by :note
+                     message created by :note. This is ALWAYS designated
+                     "file-1".

         '''
         # some very useful variables
         db = self.db
         form = self.form
 
         '''
         # some very useful variables
         db = self.db
         form = self.form
 

-        if not hasattr(self, 'FV_ITEMSPEC'):
-            # generate the regexp for detecting
-            # <classname|designator>[@:+]property
+        if not hasattr(self, 'FV_SPECIAL'):
+            # generate the regexp for handling special form values

             classes = '|'.join(db.classes.keys())
             classes = '|'.join(db.classes.keys())

-            self.FV_ITEMSPEC = re.compile(r'(%s)([-\d]+)[@:](.+)$'%classes)
+            # specials for parsePropsFromForm
+            # handle the various forms (see unit tests)
+            self.FV_SPECIAL = re.compile(self.FV_LABELS%classes, re.VERBOSE)

             self.FV_DESIGNATOR = re.compile(r'(%s)([-\d]+)'%classes)
 
         # these indicate the default class / item
             self.FV_DESIGNATOR = re.compile(r'(%s)([-\d]+)'%classes)
 
         # these indicate the default class / item


@@ -1181,54 +1199,52 @@ class Client:
         keys = form.keys()
         timezone = db.getUserTimezone()
 
         keys = form.keys()
         timezone = db.getUserTimezone()
 

+        # sentinels for the :note and :file props
+        have_note = have_file = 0
+
+        # extract the usable form labels from the form
+        matches = []

         for key in keys:
         for key in keys:

-            # see if this value modifies a different item to the default
-            m = self.FV_ITEMSPEC.match(key)
+            m = self.FV_SPECIAL.match(key)

             if m:
             if m:

+                matches.append((key, m.groupdict()))
+
+        # now handle the matches
+        for key, d in matches:
+            if d['classname']:

                 # we got a designator
                 # we got a designator

-                cn = m.group(1)
+                cn = d['classname']

                 cl = self.db.classes[cn]
                 cl = self.db.classes[cn]

-                nodeid = m.group(2)
-                propname = m.group(3)
-            elif key == ':note':
-                # backwards compatibility: the special note field
+                nodeid = d['id']
+                propname = d['propname']
+            elif d['note']:
+                # the special note field

                 cn = 'msg'
                 cl = self.db.classes[cn]
                 nodeid = '-1'
                 propname = 'content'
                 all_links.append((default_cn, default_nodeid, 'messages',
                     [('msg', '-1')]))
                 cn = 'msg'
                 cl = self.db.classes[cn]
                 nodeid = '-1'
                 propname = 'content'
                 all_links.append((default_cn, default_nodeid, 'messages',
                     [('msg', '-1')]))

-            elif key == ':file':
-                # backwards compatibility: the special file field
+                have_note = 1
+            elif d['file']:
+                # the special file field

                 cn = 'file'
                 cl = self.db.classes[cn]
                 nodeid = '-1'
                 propname = 'content'
                 all_links.append((default_cn, default_nodeid, 'files',
                     [('file', '-1')]))
                 cn = 'file'
                 cl = self.db.classes[cn]
                 nodeid = '-1'
                 propname = 'content'
                 all_links.append((default_cn, default_nodeid, 'files',
                     [('file', '-1')]))

-                if self.form.has_key(':note'):
-                    all_links.append(('msg', '-1', 'files', [('file', '-1')]))
+                have_file = 1

             else:
                 # default
                 cn = default_cn
                 cl = default_cl
                 nodeid = default_nodeid
             else:
                 # default
                 cn = default_cn
                 cl = default_cl
                 nodeid = default_nodeid

-                propname = key
+                propname = d['propname']

 
             # the thing this value relates to is...
             this = (cn, nodeid)
 
 
             # the thing this value relates to is...
             this = (cn, nodeid)
 

-            # is this a link command?
-            if self.FV_LINK.match(propname):
-                value = []
-                for entry in extractFormList(form[key]):
-                    m = self.FV_DESIGNATOR.match(entry)
-                    if not m:
-                        raise ValueError, \
-                            'link "%s" value "%s" not a designator'%(key, entry)
-                    value.append((m.groups(1), m.groups(2)))
-                all_links.append((cn, nodeid, propname[6:], value))
-

             # get more info about the class, and the current set of
             # form props for it
             if not all_propdef.has_key(cn):
             # get more info about the class, and the current set of
             # form props for it
             if not all_propdef.has_key(cn):


@@ -1238,8 +1254,20 @@ class Client:
                 all_props[this] = {}
             props = all_props[this]
 
                 all_props[this] = {}
             props = all_props[this]
 

+            # is this a link command?
+            if d['link']:
+                value = []
+                for entry in extractFormList(form[key]):
+                    m = self.FV_DESIGNATOR.match(entry)
+                    if not m:
+                        raise ValueError, \
+                            'link "%s" value "%s" not a designator'%(key, entry)
+                    value.append((m.group(1), m.group(2)))
+                all_links.append((cn, nodeid, propname, value))
+                continue
+

             # detect the special ":required" variable
             # detect the special ":required" variable

-            if self.FV_REQUIRED.match(key):
+            if d['required']:

                 all_required[this] = extractFormList(form[key])
                 continue
 
                 all_required[this] = extractFormList(form[key])
                 continue
 


@@ -1250,11 +1278,9 @@ class Client:
 
             # see if we're performing a special multilink action
             mlaction = 'set'
 
             # see if we're performing a special multilink action
             mlaction = 'set'

-            if self.FV_REMOVE.match(propname):
-                propname = propname[8:]
+            if d['remove']:

                 mlaction = 'remove'
                 mlaction = 'remove'

-            elif self.FV_ADD.match(propname):
-                propname = propname[5:]
+            elif d['add']:

                 mlaction = 'add'
 
             # does the property exist?
                 mlaction = 'add'
 
             # does the property exist?


@@ -1263,6 +1289,8 @@ class Client:
                     raise ValueError, 'You have submitted a %s action for'\
                         ' the property "%s" which doesn\'t exist'%(mlaction,
                         propname)
                     raise ValueError, 'You have submitted a %s action for'\
                         ' the property "%s" which doesn\'t exist'%(mlaction,
                         propname)

+                # the form element is probably just something we don't care
+                # about - ignore it

                 continue
             proptype = propdef[propname]
 
                 continue
             proptype = propdef[propname]
 


@@ -1285,7 +1313,7 @@ class Client:
 
             # now that we have the props field, we need a teensy little
             # extra bit of help for the old :note field...
 
             # now that we have the props field, we need a teensy little
             # extra bit of help for the old :note field...

-            if key == ':note' and value:
+            if d['note'] and value:

                 props['author'] = self.db.getuid()
                 props['date'] = date.Date()
 
                 props['author'] = self.db.getuid()
                 props['date'] = date.Date()
 


@@ -1294,8 +1322,8 @@ class Client:
                 if not value:
                     # ignore empty password values
                     continue
                 if not value:
                     # ignore empty password values
                     continue

-                for key in keys:
-                    if self.FV_CONFIRM.match(key):
+                for key, d in matches:
+                    if d['confirm'] and d['propname'] == propname:

                         confirm = form[key]
                         break
                 else:
                         confirm = form[key]
                         break
                 else:


@@ -1466,6 +1494,10 @@ class Client:
             if propname in required and value is not None:
                 required.remove(propname)
 
             if propname in required and value is not None:
                 required.remove(propname)
 

+        # check to see if we need to specially link a file to the note
+        if have_note and have_file:
+            all_links.append(('msg', '-1', 'files', [('file', '-1')]))
+

         # see if all the required properties have been supplied
         s = []
         for thing, required in all_required.items():
         # see if all the required properties have been supplied
         s = []
         for thing, required in all_required.items():

diff --git a/roundup/cgi/templating.py b/roundup/cgi/templating.py
index 6992e061f3625c758953e864dd880af262cb00fe..57d3f21427fb66b1034aa9e135f4fbc43afeea0d 100644 (file)
--- a/roundup/cgi/templating.py
+++ b/roundup/cgi/templating.py
@@ -876,9 +876,9 @@ class PasswordHTMLProperty(HTMLProperty):
     def confirm(self, size = 30):
         ''' Render a second form edit field for the property, used for 
             confirmation that the user typed the password correctly. Generates
     def confirm(self, size = 30):
         ''' Render a second form edit field for the property, used for 
             confirmation that the user typed the password correctly. Generates

-            a field with name "name:confirm".
+            a field with name ":confirm:name".

         '''
         '''

-        return '<input type="password" name="%s:confirm" size="%s">'%(
+        return '<input type="password" name=":confirm:name" size="%s">'%(

             self._name, size)
 
 class NumberHTMLProperty(HTMLProperty):
             self._name, size)
 
 class NumberHTMLProperty(HTMLProperty):

diff --git a/test/test_cgi.py b/test/test_cgi.py
index 4cb01137d93fcc57102ff0b0478c61fc58edbfa4..92db0e75371d2a116786fe279bc936cf8d5f9700 100644 (file)
--- a/test/test_cgi.py
+++ b/test/test_cgi.py
@@ -8,9 +8,9 @@
 # but WITHOUT ANY WARRANTY; without even the implied warranty of
 # MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
 #
 # but WITHOUT ANY WARRANTY; without even the implied warranty of
 # MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
 #

-# $Id: test_cgi.py,v 1.9 2003-02-14 00:31:46 richard Exp $
+# $Id: test_cgi.py,v 1.10 2003-02-17 06:44:01 richard Exp $

 
 

-import unittest, os, shutil, errno, sys, difflib, cgi
+import unittest, os, shutil, errno, sys, difflib, cgi, re

 
 from roundup.cgi import client
 from roundup import init, instance, password, hyperdb, date
 
 from roundup.cgi import client
 from roundup import init, instance, password, hyperdb, date


@@ -62,6 +62,11 @@ class FormTestCase(unittest.TestCase):
             multilink=hyperdb.Multilink('test'), date=hyperdb.Date(),
             interval=hyperdb.Interval())
 
             multilink=hyperdb.Multilink('test'), date=hyperdb.Date(),
             interval=hyperdb.Interval())
 

+        # compile the labels re
+        classes = '|'.join(self.db.classes.keys())
+        self.FV_SPECIAL = re.compile(client.Client.FV_LABELS%classes,
+            re.VERBOSE)
+

     def parseForm(self, form, classname='test', nodeid=None):
         cl = client.Client(self.instance, None, {'PATH_INFO':'/'},
             makeForm(form))
     def parseForm(self, form, classname='test', nodeid=None):
         cl = client.Client(self.instance, None, {'PATH_INFO':'/'},
             makeForm(form))


@@ -77,6 +82,44 @@ class FormTestCase(unittest.TestCase):
         except OSError, error:
             if error.errno not in (errno.ENOENT, errno.ESRCH): raise
 
         except OSError, error:
             if error.errno not in (errno.ENOENT, errno.ESRCH): raise
 

+    #
+    # form label extraction
+    #
+    def tl(self, s, c, i, a, p):
+        m = self.FV_SPECIAL.match(s)
+        self.assertNotEqual(m, None)
+        d = m.groupdict()
+        self.assertEqual(d['classname'], c)
+        self.assertEqual(d['id'], i)
+        for action in 'required add remove link note file'.split():
+            if a == action:
+                self.assertNotEqual(d[action], None)
+            else:
+                self.assertEqual(d[action], None)
+        self.assertEqual(d['propname'], p)
+
+    def testLabelMatching(self):
+        self.tl('<propname>', None, None, None, '<propname>')
+        self.tl(':required', None, None, 'required', None)
+        self.tl(':confirm:<propname>', None, None, 'confirm', '<propname>')
+        self.tl(':add:<propname>', None, None, 'add', '<propname>')
+        self.tl(':remove:<propname>', None, None, 'remove', '<propname>')
+        self.tl(':link:<propname>', None, None, 'link', '<propname>')
+        self.tl('test1:<prop>', 'test', '1', None, '<prop>')
+        self.tl('test1:required', 'test', '1', 'required', None)
+        self.tl('test1:add:<prop>', 'test', '1', 'add', '<prop>')
+        self.tl('test1:remove:<prop>', 'test', '1', 'remove', '<prop>')
+        self.tl('test1:link:<prop>', 'test', '1', 'link', '<prop>')
+        self.tl('test1:confirm:<prop>', 'test', '1', 'confirm', '<prop>')
+        self.tl('test-1:<prop>', 'test', '-1', None, '<prop>')
+        self.tl('test-1:required', 'test', '-1', 'required', None)
+        self.tl('test-1:add:<prop>', 'test', '-1', 'add', '<prop>')
+        self.tl('test-1:remove:<prop>', 'test', '-1', 'remove', '<prop>')
+        self.tl('test-1:link:<prop>', 'test', '-1', 'link', '<prop>')
+        self.tl('test-1:confirm:<prop>', 'test', '-1', 'confirm', '<prop>')
+        self.tl(':note', None, None, 'note', None)
+        self.tl(':file', None, None, 'file', None)
+

     #
     # Empty form
     #
     #
     # Empty form
     #


@@ -277,18 +320,18 @@ class FormTestCase(unittest.TestCase):
         self.assertRaises(ValueError, self.parseForm, {'password': ['', '']},
             'user')
         self.assertRaises(ValueError, self.parseForm, {'password': 'foo',
         self.assertRaises(ValueError, self.parseForm, {'password': ['', '']},
             'user')
         self.assertRaises(ValueError, self.parseForm, {'password': 'foo',

-            'password:confirm': ['', '']}, 'user')
+            ':confirm:password': ['', '']}, 'user')

 
     def testSetPassword(self):
         self.assertEqual(self.parseForm({'password': 'foo',
 
     def testSetPassword(self):
         self.assertEqual(self.parseForm({'password': 'foo',

-            'password:confirm': 'foo'}, 'user'),
+            ':confirm:password': 'foo'}, 'user'),

             ({('user', None): {'password': 'foo'}}, []))
 
     def testSetPasswordConfirmBad(self):
         self.assertRaises(ValueError, self.parseForm, {'password': 'foo'},
             'user')
         self.assertRaises(ValueError, self.parseForm, {'password': 'foo',
             ({('user', None): {'password': 'foo'}}, []))
 
     def testSetPasswordConfirmBad(self):
         self.assertRaises(ValueError, self.parseForm, {'password': 'foo'},
             'user')
         self.assertRaises(ValueError, self.parseForm, {'password': 'foo',

-            'password:confirm': 'bar'}, 'user')
+            ':confirm:password': 'bar'}, 'user')

 
     def testEmptyPasswordNotSet(self):
         nodeid = self.db.user.create(username='1',
 
     def testEmptyPasswordNotSet(self):
         nodeid = self.db.user.create(username='1',


@@ -298,7 +341,7 @@ class FormTestCase(unittest.TestCase):
         nodeid = self.db.user.create(username='2',
             password=password.Password('foo'))
         self.assertEqual(self.parseForm({'password': '',
         nodeid = self.db.user.create(username='2',
             password=password.Password('foo'))
         self.assertEqual(self.parseForm({'password': '',

-            'password:confirm': ''}, 'user', nodeid),
+            ':confirm:password': ''}, 'user', nodeid),

             ({('user', nodeid): {}}, []))
 
     #
             ({('user', nodeid): {}}, []))
 
     #


@@ -361,31 +404,36 @@ class FormTestCase(unittest.TestCase):
     #
     def testMultiple(self):
         self.assertEqual(self.parseForm({'string': 'a', 'issue-1@title': 'b'}),
     #
     def testMultiple(self):
         self.assertEqual(self.parseForm({'string': 'a', 'issue-1@title': 'b'}),

-            ({('test', None): {'string': 'a'}, ('issue', '-1'):
-            {'title': 'b'}}, []))
+            ({('test', None): {'string': 'a'},
+              ('issue', '-1'): {'title': 'b'}
+             }, []))
+
+    def testMultipleExistingContext(self):

         nodeid = self.db.test.create()
         self.assertEqual(self.parseForm({'string': 'a', 'issue-1@title': 'b'},
         nodeid = self.db.test.create()
         self.assertEqual(self.parseForm({'string': 'a', 'issue-1@title': 'b'},

-            'test', nodeid), ({('test', nodeid): {'string': 'a'},
+            'test', nodeid),({('test', nodeid): {'string': 'a'},

             ('issue', '-1'): {'title': 'b'}}, []))
             ('issue', '-1'): {'title': 'b'}}, []))

+
+    def testLinking(self):

         self.assertEqual(self.parseForm({
             'string': 'a',
         self.assertEqual(self.parseForm({
             'string': 'a',

-            'issue-1@:add:nosy': '1',
-            'issue-2@:link:superseder': 'issue-1',
+            'issue-1@add@nosy': '1',
+            'issue-2@link@superseder': 'issue-1',

             }),
             ({('test', None): {'string': 'a'},
               ('issue', '-1'): {'nosy': ['1']},
             }),
             ({('test', None): {'string': 'a'},
               ('issue', '-1'): {'nosy': ['1']},

-              ('issue', '-2'): {}},
-              [('issue', '-2', 'superseder',
-                [(('issue', '-1'), ('issue', '-1'))])
-              ]
+              ('issue', '-2'): {}
+             },
+             [('issue', '-2', 'superseder', [('issue', '-1')])
+             ]

             )
         )
 
     def testLinkBadDesignator(self):
         self.assertRaises(ValueError, self.parseForm,
             )
         )
 
     def testLinkBadDesignator(self):
         self.assertRaises(ValueError, self.parseForm,

-            {'test-1@:link:link': 'blah'})
+            {'test-1@link@link': 'blah'})

         self.assertRaises(ValueError, self.parseForm,
         self.assertRaises(ValueError, self.parseForm,

-            {'test-1@:link:link': 'issue'})
+            {'test-1@link@link': 'issue'})

 
     def testBackwardsCompat(self):
         res = self.parseForm({':note': 'spam'}, 'issue')
 
     def testBackwardsCompat(self):
         res = self.parseForm({':note': 'spam'}, 'issue')