I seem to have found a bug bug relating to date fields when other fields
on a form a invalid.

How to (I believe) repoduce:
- Have a node type that has a form with a date field
- Have an auditor for the same node that checks certain values and
raises a 'ValueError' if there is some problem

- Create or edit the node (ensuring there is no existing value for the
date field)
- Enter a date into the date field
- Enter a value into another fieldf that will cause a 'ValueError'
- Submit the form

The process should fail because the template engine(?) treats the date
value as a 'Date', when instead it is still a 'String'. See attached
traceback.

I have attached a potential fix/patch. I'm not sure this is the best way
to fix it, but it seems to do what I want.

Turns out that this issue is not related to displaying the date using
field(), but attempting to display the date in the history section of
the issue. When generating the "current state" the DateHTMLProperty
._value contains a str instead of Date which causes problems when
calling .plain()

The actual issue is therefore: values that have not been committed to
the database (due to an auditor error) are showing up in this history
when an auditor error occurs.

To reproduce this subtle issue (in a visual way):
 * Create a new issue that is assigned to someone ("demo")
 * Edit the issue, and un-assign it from anyone ("- no selection -")
 * Edit the issue once again, and:
    * assign the issue to someone ("demo")
    * delete the contents of the title (to trigger an auditor error)

The history should look like:
  2012-03-06 09:28:06	demo	set	assignedto: demo -> 
  2012-03-06 09:27:54	demo	create	

But instead it looks like:
  2012-03-06 09:28:06	demo	set	assignedto: demo -> demo
  2012-03-06 09:27:54	demo	create

A new patch that fixes the underlying history problem...

It checks the DB to see if the prop value exists before trying to fetch
it from the DB. This stops the value in the form from being fetched if
there is no value for the prop in the DB.

This probably isn't the most elegant solution, because it requires
hitting the DB twice if a prop value does exist in the DB (instead of
just once), but it is preferable to rewriting or duplicating a decent
amount of code.

----------Original Message----------
From: Anthony Pankov <ant_mail@inbox.ru>
Sent: Friday 25 April 2014, 14:09:11
To: "roundup-devel" <roundup-devel@lists.sourceforge.net>
Cc: 
Subject: [Roundup-devel] 4 years opened issue and templating.py 
restructuring

Hello, All.

There is an issue http://issues.roundup-tracker.org/issue2550612 which
i can confirm.

As  i  see,  incorrect  date value is raised as DateHTMLProperty class
after  post.  For  example,  '999'  entered in date field.  '999' will
appear  as  string  _value  in base  HTMLProperty class and date value will 
be
None      in     ancestor     DateHTMLProperty.     When    displaying
DateHTMLProperty.pretty(),  DateHTMLProperty.plain()
functions   check   whether   _value   is  None  and  if  not  try  to
self._value.local(offset)  which  raise  an  error. This is because of
_value is not None, it is incorrect date string.

Converting in templating.py DateHTMLProperty.pretty() function from:
       if not self._value:
             return ''
       elif format is not self._marker:
            return self._value.local(offset).pretty(format)
       else:
           return self._value.local(offset).pretty()
to:
       if not self._value:
             return ''
       elif isinstance(self._value, str):
             return self._value
       elif format is not self._marker:
            return self._value.local(offset).pretty(format)
       else:
           return self._value.local(offset).pretty()

solve issue for me.

But  this  additional check must be copy-pasted in 
DateHTMLProperty.plain()
function     and    other.    It   is   hard   to   force myself to do
it.

There  is  a  20  entry  of  "  if  not  self.is_view_ok():"  check in
templating.py.

Is  there any plan to restructuring templating.py. What about to place
some common functionality to base HTMLproperty class?




Or,  may be somebody suggest another way to solve issue.

I am confused.

Is this the same problem as: 

 issue1169513: History breaks when props are inaccessible

or something more? (Added the link to superseder to make
sure this possibly related issue isn't lost in the message history
although it's not really a superseder).

Using jerrykan's reproduction steps in msg4512
in the current demo.py classic tracker only produces:

Required issue property title not supplied 

as an error and the history looks like:

2016-06-26 19:04:20	admin	set	assignedto: admin ->
2016-06-26 19:03:54	admin	set	assignedto: admin
2016-06-26 19:03:49	admin	create

was this fixed and this ticket not closed?

Do I need to have a date field in the schema to see this issue?

If the problem is that an entry is added to the history
even though the entry was rolled back by an auditor error
that seems to be a different diagnosis.

This tracker is using sqlite back end. If the problem is a history
non-rollback maybe this only affects anydbm backends?

I would like to get rid of known crash bugs with patches in the 
next (1.6) release if possible.

-- rouilj

Jerry, can you try this again with a current head release.

I have a tracker with two date fields and an integer field.

If I put a valid date in one field and put a real number in
the integer field, an edit exception is raised. The date field
is still properly displayed on the form.

Same when putting the value 10 in my second date field. The first
date field is still ok and no sign of a crash.

If my test above looks valid and you can't reproduce or you are
sufficiently convinced that my test shows the problem is gone
let me know and we can close this.

Does anybody know if this issue can be closed because other
changes to the code have fixed it?

Iam unable to reproduce and have had no response to my requests.

So closing as works for me. If somebody can reproduce feel free to
re-open this.

Sorry to re-open this one, but I have been looking at upgrading one of our internal systems and am able to confirm this is still an issue.

The steps outlined in msg4512 can still be used to produce the incorrect behaviour when running `roundup-demo`.


I've done a bit more digging into the problem and it seems to be due to the section of code locate at:

  http://hg.code.sf.net/p/roundup/code/file/380dec305c28/roundup/cgi/templating.py#l1026

The `current` value is being retrieved from a `HTMLProperty`. If an auditor raises  a `ValueError` then the `HTMLProperty` will contain the value of the property that was submitted in the request, not the value of the property as it is in the DB.

My first instinct would be to change the following line (and some surrounded code) from:

    val = self[k]

to:

    val = self._klass.get(self._nodeid, k)

but depending on the property type we probably won't end up with the text we really want (ie. ids instead of names)

A quick and dirty hack might be to replace:

    val = self[k]

with:

    val = self[k]
    val._value = self._klass.get(self._nodeid, k)


It would seem to solve the immediate probably... but probably not in the most desirable of ways :D

I'm open to suggestions that can avoid a large block of if/elif/else statements to handle all property type usecases.

Jerrykan/John:

In reading your description, it looks like the problem is that the
history is being updated when it shouldn't. The rollback from the
failure should result in no change to history. But from your
description it looks like the history is updated. Is this correct?

issue2550964 describes something similar, but it's a temporary/cosmetic issue.

-- rouilj

(sending via email because tracker is throwing an error at the moment).

Hello John,

Yes issue2550964 sounds suspiciously like the same problem that that was 
causing the incorrect history to be shown, which was in turn causing the 
traceback to be shown.

Doing yet more investigation seems to reveal that the problem resulting 
in the traceback was indeed fixed by commit:

   hg:1e4c45a4254b7e3c0cba50c86e24f995e63d3509
   git:936c7ad852555644370dcb5d2abc520b10eb7e24

So I think this issue can be closed again in favour of issue2550964

Apologies for the noise in reopening this issue.

SeeYa,
John.

On 19/1/20 1:36 pm, John Rouillard wrote:
> 
> John Rouillard added the comment:
> 
> Jerrykan/John:
> 
> In reading your description, it looks like the problem is that the
> history is being updated when it shouldn't. The rollback from the
> failure should result in no change to history. But from your
> description it looks like the history is updated. Is this correct?
> 
> issue2550964 describes something similar, but it's a temporary/cosmetic issue.
> 
> -- rouilj
> 
> (sending via email because tracker is throwing an error at the moment).