The roundup web interface looses content of form fields
if submission triggers an error (such as from auditors).

It should be possible for roundup to preserve the state
a form was in when it was submitted, and send it back
for correction (possibly with wrong entries highlighted).

Logged In: YES 
user_id=6405

Yes, with better form generation / handling this might be 
an option. It's not at the moment though, sorry. 

This is a very old issue -- still probably one of the topmost usability
problems of roundup. See issue712084 for a still older bug-report about
the same problem (why was that one closed, there is no log about it and
no close message)

I know some changes were made to make this work better.
Is this still an issue?

On Mon, Apr 11, 2016 at 12:43:19AM +0000, John Rouillard wrote:
> 
> I know some changes were made to make this work better.
> Is this still an issue?

Yes, I'm quite sure it is.
Reason is that roundup (as far as I remember) processes fields until an
error occurs and leaves the other fields unprocessed. The result is that
some fields revert back to the original version before editing when an
auditor exception occurs (or similar).

I guess the first step would be to code up a test for this...

I may have some interest to fix this myself...

Ralf
-- 
Dr. Ralf Schlatterbeck                  Tel:   +43/2243/26465-16
Open Source Consulting                  www:   http://www.runtux.com
Reichergasse 131, A-3411 Weidling       email: office@runtux.com

I've now traced this a little further, it turns out that
- Form values *are* preserved internally. The whole form is present in
  the cgi client used to render an issue
- But: The database wins. Only if a field is empty in the database
  (return None but for Number a 0 will probably also be overridden) the
  form will override this value.

Code is in cgi/templating.py notably the HTMLProperty class (from which
all the special classes like DateHTMLProperty inherit) does a lookup of
the form value only if it is passed an empty value in the constructor.

This in turn is called from __getitem__ of _HTMLItem which looks up the
value from the database.

Now to fix this we need some way to pass information to HTMLProperty to
indicate that it should prefer the form contents over the database contents.

I don't know if this would be a security risk to *always* prefer the
form values...

Finally fixed in commit 232c74973a56

Hi Ralf:

It looks like your change to tests/test_cgi.py broke a couple of tests.

=================================== FAILURES
===================================

___________________ FormTestCase.testCSVExportBadColumnName
____________________

self = <test.test_cgi.FormTestCase testMethod=testCSVExportBadColumnName>

    def testCSVExportBadColumnName(self):

        cl = self._make_client({'@columns': 'falseid,name'}, nodeid=None,

            userid='1')

        cl.classname = 'status'

        output = StringIO.StringIO()

        cl.request = MockNull()

        cl.request.wfile = output

        self.assertRaises(exceptions.SeriousError,

>           actions.ExportCSVAction(cl).handle)

test/test_cgi.py:1155: 

_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
_ _ _ _ 

self = <roundup.cgi.actions.ExportCSVAction instance at 0x7f1d05f56c20>

    def handle(self):

        ''' Export the specified search query as CSV. '''

        # figure the request

        request = templating.HTMLRequest(self.client)

        filterspec = request.filterspec

        sort = request.sort

        group = request.group

        columns = request.columns

        klass = self.db.getclass(request.classname)

    

        # check if all columns exist on class

        # the exception must be raised before sending header

        props = klass.getprops()

        for cname in columns:

            if cname not in props:

                # use error code 400: Bad Request. Do not use

                # error code 404: Not Found.

                self.client.response_code = 400

                raise exceptions.NotFound(

                    self._('Column "%(column)s" not found in %(class)s')

>                   % {'column': cgi.escape(cname), 'class':
request.classname})

E               NotFound: Column "falseid" not found in status

roundup/cgi/actions.py:1264: NotFound

___________________ FormTestCase.testCSVExportFailPermission
___________________

self = <test.test_cgi.FormTestCase testMethod=testCSVExportFailPermission>

    def testCSVExportFailPermission(self):

        cl = self._make_client({'@columns': 'id,email,password'},
nodeid=None,

            userid='2')

        cl.classname = 'user'

        output = StringIO.StringIO()

        cl.request = MockNull()

        cl.request.wfile = output

        # used to be self.assertRaises(exceptions.Unauthorised,

        # but not acting like the column name is not found

        self.assertRaises(exceptions.SeriousError,

>           actions.ExportCSVAction(cl).handle)

test/test_cgi.py:1167: 

_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
_ _ _ _ 

self = <roundup.cgi.actions.ExportCSVAction instance at 0x7f1d05c4c050>

    def handle(self):

        ''' Export the specified search query as CSV. '''

        # figure the request

        request = templating.HTMLRequest(self.client)

        filterspec = request.filterspec

        sort = request.sort

        group = request.group

        columns = request.columns

        klass = self.db.getclass(request.classname)

    

        # check if all columns exist on class

        # the exception must be raised before sending header

        props = klass.getprops()

        for cname in columns:

            if cname not in props:

                # use error code 400: Bad Request. Do not use

                # error code 404: Not Found.

                self.client.response_code = 400

                raise exceptions.NotFound(

                    self._('Column "%(column)s" not found in %(class)s')

>                   % {'column': cgi.escape(cname), 'class':
request.classname})

E               NotFound: Column "email" not found in user

roundup/cgi/actions.py:1264: NotFound

I grabbed the from the travis0-ci test run, but

./run_tests.py test/test_cgi.py

looks like it's reporting the same.

It's not obvious to me that the changes in the test harness are the
problem. Can you take a look at it.

-- rouilj

On Sun, Aug 28, 2016 at 12:04:37AM +0000, John Rouillard wrote:
> 
> John Rouillard added the comment:
> 
> Hi Ralf:
> 
> It looks like your change to tests/test_cgi.py broke a couple of tests.

Hmm, I think I ran all the tests before commit.
I'll look into this later this week I'm on a trip currently.

Ralf

On Sun, Aug 28, 2016 at 12:04:37AM +0000, John Rouillard wrote:
> 
> Hi Ralf:
> 
> It looks like your change to tests/test_cgi.py broke a couple of tests.
> 
> ___________________ FormTestCase.testCSVExportBadColumnName
> ___________________ FormTestCase.testCSVExportFailPermission

Both tests fail already when I check out a86860224d80 (before my form
patch). I don't know why I missed this, I think all tests passed before
I checked this in.
Seems the problem was introduced with your commit a86860224d80,
the commit 114d9628fd77 before this passes the tests for me.

Is there a possibility that these tests were skipped when I tried to run
all tests (running the test-script without arguments)?

(I'm now running all tests and it seems the test_cgi.py doesn't skip
tests and also fails the two tests above, maybe there is something in
the reporting of failures so I missed these failing tests???).

Ralf
-- 
Dr. Ralf Schlatterbeck                  Tel:   +43/2243/26465-16
Open Source Consulting                  www:   http://www.runtux.com
Reichergasse 131, A-3411 Weidling       email: office@runtux.com

Hi Ralf:

In message <20160901084410.GB14758@runtux.com>,
Ralf Schlatterbeck writes:
>Ralf Schlatterbeck added the comment:
>On Sun, Aug 28, 2016 at 12:04:37AM +0000, John Rouillard wrote:
>> It looks like your change to tests/test_cgi.py broke a couple of tests.
>> ___________________ FormTestCase.testCSVExportBadColumnName
>> ___________________ FormTestCase.testCSVExportFailPermission
>
>Both tests fail already when I check out a86860224d80 (before my form
>patch).

Yup looks like that is my bug (in hindsight export bad column name
failing makes perfect sense). I thought I had been keeping git up to
date, but it looks like I forgot to push a8686 to git and run it
through travis ci to get the whole suite tested.

So I missed that my change required tweaking the tests.

Sorry about that. I am closing this ticket.