From bb6a1e077af48f7ea05146e5099eac6e49e91b4d Mon Sep 17 00:00:00 2001 From: richard Date: Mon, 17 Feb 2003 06:44:01 +0000 Subject: [PATCH] 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 --- roundup/cgi/client.py | 164 +++++++++++++++++++++++--------------- roundup/cgi/templating.py | 4 +- test/test_cgi.py | 82 +++++++++++++++---- 3 files changed, 165 insertions(+), 85 deletions(-) diff --git a/roundup/cgi/client.py b/roundup/cgi/client.py index 75c0348..3559e75 100644 --- 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). @@ -93,16 +93,27 @@ class Client: 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)| + (?P[@:]file)| + ( + ((?P%s)(?P[-\d]+))? # optional leading designator + ((?P[@:]required$)| # :required + ( + ( + (?P[@:]add[@:])| # :add: + (?P[@:]remove[@:])| # :remove: + (?P[@:]confirm[@:])| # :confirm: + (?P[@:]link[@:])| # :link: + ([@:]) # just a separator + )? + (?P[^@:]+) # + ) + ) + ) + )$''' # 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+$')): - ''' Pull properties for the given class out of the form. + ''' Pull properties out of the form. In the following, 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: + + + - property on the current context item + : + - property on the indicated item + + -: + - 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 ":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:=id(s) + :remove:=id(s) The ids will be removed from the multilink property. - [classname|designator]:add:=id(s) + + :add:=id(s) The ids will be added to the multilink property. - [classname|designator]:link:= + :link:= 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 on - [classname|designator] will be set/appended the id of the - newly created item of class . - - Note: the colon may be either ":" or "@". + all_props in _editnodes. The on the current item + will be set/appended the id of the newly created item of + class (where must be + -). 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 - 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 - message created by :note + message created by :note. This is ALWAYS designated + "file-1". ''' # some very useful variables db = self.db form = self.form - if not hasattr(self, 'FV_ITEMSPEC'): - # generate the regexp for detecting - # [@:+]property + if not hasattr(self, 'FV_SPECIAL'): + # generate the regexp for handling special form values 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 @@ -1181,54 +1199,52 @@ class Client: 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: - # 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: + matches.append((key, m.groupdict())) + + # now handle the matches + for key, d in matches: + if d['classname']: # we got a designator - cn = m.group(1) + cn = d['classname'] 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')])) - 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')])) - 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 - propname = key + propname = d['propname'] # 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): @@ -1238,8 +1254,20 @@ class Client: 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 - if self.FV_REQUIRED.match(key): + if d['required']: all_required[this] = extractFormList(form[key]) continue @@ -1250,11 +1278,9 @@ class Client: # 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' - elif self.FV_ADD.match(propname): - propname = propname[5:] + elif d['add']: 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) + # the form element is probably just something we don't care + # about - ignore it 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... - if key == ':note' and value: + if d['note'] and value: props['author'] = self.db.getuid() props['date'] = date.Date() @@ -1294,8 +1322,8 @@ class Client: 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: @@ -1466,6 +1494,10 @@ class Client: 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(): diff --git a/roundup/cgi/templating.py b/roundup/cgi/templating.py index 6992e06..57d3f21 100644 --- 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 - a field with name "name:confirm". + a field with name ":confirm:name". ''' - return ''%( + return ''%( self._name, size) class NumberHTMLProperty(HTMLProperty): diff --git a/test/test_cgi.py b/test/test_cgi.py index 4cb0113..92db0e7 100644 --- 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. # -# $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 @@ -62,6 +62,11 @@ class FormTestCase(unittest.TestCase): 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)) @@ -77,6 +82,44 @@ class FormTestCase(unittest.TestCase): 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('', None, None, None, '') + self.tl(':required', None, None, 'required', None) + self.tl(':confirm:', None, None, 'confirm', '') + self.tl(':add:', None, None, 'add', '') + self.tl(':remove:', None, None, 'remove', '') + self.tl(':link:', None, None, 'link', '') + self.tl('test1:', 'test', '1', None, '') + self.tl('test1:required', 'test', '1', 'required', None) + self.tl('test1:add:', 'test', '1', 'add', '') + self.tl('test1:remove:', 'test', '1', 'remove', '') + self.tl('test1:link:', 'test', '1', 'link', '') + self.tl('test1:confirm:', 'test', '1', 'confirm', '') + self.tl('test-1:', 'test', '-1', None, '') + self.tl('test-1:required', 'test', '-1', 'required', None) + self.tl('test-1:add:', 'test', '-1', 'add', '') + self.tl('test-1:remove:', 'test', '-1', 'remove', '') + self.tl('test-1:link:', 'test', '-1', 'link', '') + self.tl('test-1:confirm:', 'test', '-1', 'confirm', '') + self.tl(':note', None, None, 'note', None) + self.tl(':file', None, None, 'file', None) + # # Empty form # @@ -277,18 +320,18 @@ class FormTestCase(unittest.TestCase): 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', - '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', - 'password:confirm': 'bar'}, 'user') + ':confirm:password': 'bar'}, 'user') 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': '', - 'password:confirm': ''}, 'user', nodeid), + ':confirm:password': ''}, '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'}), - ({('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'}, - 'test', nodeid), ({('test', nodeid): {'string': 'a'}, + 'test', nodeid),({('test', nodeid): {'string': 'a'}, ('issue', '-1'): {'title': 'b'}}, [])) + + def testLinking(self): 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']}, - ('issue', '-2'): {}}, - [('issue', '-2', 'superseder', - [(('issue', '-1'), ('issue', '-1'))]) - ] + ('issue', '-2'): {} + }, + [('issue', '-2', 'superseder', [('issue', '-1')]) + ] ) ) def testLinkBadDesignator(self): self.assertRaises(ValueError, self.parseForm, - {'test-1@:link:link': 'blah'}) + {'test-1@link@link': 'blah'}) self.assertRaises(ValueError, self.parseForm, - {'test-1@:link:link': 'issue'}) + {'test-1@link@link': 'issue'}) def testBackwardsCompat(self): res = self.parseForm({':note': 'spam'}, 'issue') -- 2.30.2