issue1408570: fix that form values are lost

schlatterbeck · schlatterbeck · commit 643d58666cde · 2016-08-22T22:19:48.000+02:00
.. on edit exceptions. This occured for example if editing an issue with
the classic template and setting 'superseder' to a non-existing issue
number. All changes to the form where the original field was non-empty
were lost.
diff --git a/CHANGES.txt b/CHANGES.txt
@@ -317,10 +317,11 @@ Fixed:
   %20. Fixes to allow a url_query method to be applied to
   HTMLStringProperty to properly quote string values passed as part of
   a url.
-- issue2550755: exceptions.NotFound(msg) msg is not reported to user
-  in cgi. When an invalid column is specified return error code 400
-  rather than 404. Make error code 400 also return an error message to
-  the user. Reported by: Bernhard Reiter, analysis, fix by John Rouillard.
+- issue1408570: Finally fix that form values are lost on edit
+  exceptions. This occured for example if editing an issue with the
+  classic template and setting 'superseder' to a non-existing issue
+  number. All changes to the form where the original field was non-empty
+  were lost.
 
 2016-01-11: 1.5.1
 
diff --git a/roundup/cgi/client.py b/roundup/cgi/client.py
@@ -295,6 +295,10 @@ def __init__(self, instance, request, env, form=None, translator=None):
         self.env = env
         self.setTranslator(translator)
         self.mailer = Mailer(instance.config)
+        # If True the form contents wins over the database contents when
+        # rendering html properties. This is set when an error occurs so
+        # that we don't lose submitted form contents.
+        self.form_wins = False
 
         # save off the path
         self.path = env['PATH_INFO']
@@ -421,6 +425,9 @@ def add_ok_message(self, msg, escape=True):
 
     def add_error_message(self, msg, escape=True):
         add_message(self._error_message, msg, escape)
+        # Want to interpret form values when rendering when an error
+        # occurred:
+        self.form_wins = True
 
     def inner_main(self):
         """Process a request.
diff --git a/roundup/cgi/templating.py b/roundup/cgi/templating.py
@@ -1251,7 +1251,7 @@ def __init__(self, client, classname, nodeid, prop, name, value,
             is_in = form.has_key(self._formname)
         except TypeError:
             is_in = False
-        if not self._value and is_in:
+        if is_in and (not self._value or self._client.form_wins):
             if isinstance(prop, hyperdb.Multilink):
                 value = lookupIds(self._db, prop,
                                   handleListCGIValue(form[self._formname]),
diff --git a/test/test_cgi.py b/test/test_cgi.py
@@ -13,9 +13,13 @@
 from roundup.cgi import client, actions, exceptions
 from roundup.cgi.exceptions import FormError
 from roundup.cgi.templating import HTMLItem, HTMLRequest, NoTemplate
+from roundup.cgi.templating import HTMLProperty, _HTMLItem
 from roundup.cgi.form_parser import FormParser
 from roundup import init, instance, password, hyperdb, date
 
+# For testing very simple rendering
+from roundup.cgi.engine_zopetal import RoundupPageTemplate
+
 from mocknull import MockNull
 
 import db_test_base
@@ -117,13 +121,20 @@ def setUp(self):
         self.FV_SPECIAL = re.compile(FormParser.FV_LABELS%classes,
             re.VERBOSE)
 
-    def parseForm(self, form, classname='test', nodeid=None):
+    def setupClient(self, form, classname, nodeid=None, template='item'):
         cl = client.Client(self.instance, None, {'PATH_INFO':'/',
             'REQUEST_METHOD':'POST'}, makeForm(form))
         cl.classname = classname
         cl.nodeid = nodeid
         cl.language = ('en',)
+        cl.userid = '1'
         cl.db = self.db
+        cl.user = 'admin'
+        cl.template = template
+        return cl
+
+    def parseForm(self, form, classname='test', nodeid=None):
+        cl = self.setupClient(form, classname, nodeid)
         return cl.parsePropsFromForm(create=1)
 
     def tearDown(self):
@@ -772,6 +783,76 @@ def testBackwardsCompat(self):
             'name': 'foo.txt', 'type': 'text/plain'}},
             [('issue', None, 'files', [('file', '-1')])]))
 
+    def testFormValuePreserveOnError(self):
+        page_template = """
+        <html>
+         <body>
+          <p tal:condition="options/error_message|nothing"
+             tal:repeat="m options/error_message"
+             tal:content="structure m"/>
+          <p tal:content="context/title/plain"/>
+          <p tal:content="context/priority/plain"/>
+          <p tal:content="context/status/plain"/>
+          <p tal:content="context/nosy/plain"/>
+          <p tal:content="context/keyword/plain"/>
+          <p tal:content="structure context/superseder/field"/>
+         </body>
+        </html>
+        """.strip ()
+        self.db.keyword.create (name = 'key1')
+        self.db.keyword.create (name = 'key2')
+        nodeid = self.db.issue.create (title = 'Title', priority = '1',
+            status = '1', nosy = ['1'], keyword = ['1'])
+        self.db.commit ()
+        form = {':note': 'msg-content', 'title': 'New title',
+            'priority': '2', 'status': '2', 'nosy': '1,2', 'keyword': '',
+            'superseder': '5000', ':action': 'edit'}
+        cl = self.setupClient(form, 'issue', '1')
+        pt = RoundupPageTemplate()
+        pt.pt_edit(page_template, 'text/html')
+        out = []
+        def wh(s):
+            out.append(s)
+        cl.write_html = wh
+        # Enable the following if we get a templating error:
+        #def send_error (*args, **kw):
+        #    import pdb; pdb.set_trace()
+        #cl.send_error_to_admin = send_error
+        # Need to rollback the database on error -- this usually happens
+        # in web-interface (and for other databases) anyway, need it for
+        # testing that the form values are really used, not the database!
+        # We do this together with the setup of the easy template above
+        def load_template(x):
+            cl.db.rollback()
+            return pt
+        cl.instance.templates.load = load_template
+        cl.selectTemplate = MockNull()
+        cl.determine_context = MockNull ()
+        def hasPermission(s, p, classname=None, d=None, e=None, **kw):
+            return True
+        actions.Action.hasPermission = hasPermission
+        e1 = _HTMLItem.is_edit_ok
+        _HTMLItem.is_edit_ok = lambda x : True
+        e2 = HTMLProperty.is_edit_ok
+        HTMLProperty.is_edit_ok = lambda x : True
+        cl.inner_main()
+        _HTMLItem.is_edit_ok = e1
+        HTMLProperty.is_edit_ok = e2
+        self.assertEqual(len(out), 1)
+        self.assertEqual(out [0].strip (), """
+        <html>
+         <body>
+          <p>Edit Error: issue has no node 5000</p>
+          <p>New title</p>
+          <p>urgent</p>
+          <p>deferred</p>
+          <p>admin, anonymous</p>
+          <p></p>
+          <p><input type="text" name="superseder" value="5000" size="30"></p>
+         </body>
+        </html>
+        """.strip ())
+
     #
     # SECURITY
     #
