Custom ticket field edits from None to '' show as deletion

[Tim Hatch]

I started this ​on trac-dev but convinced myself that it was a real issue. See that message for steps to reproduce

Just as a followup, templates/ticket_view.html displays field_name deleted when a custom field goes from None to ''. This is caused by a custom field being added after a ticket is created. I don't believe this to be a change (much less a deletion) so propose the following:

trac/ticket/web_ui.py

@@ -849 +849 @@
                     else:
                         this_num = old
                     current['cnum'] = int(this_num)
-            else:
+            elif old or new:
                 current['fields'][field] = {'old': old, 'new': new}
         if current:
             yield current

The unit tests appear to still pass as expected.

---

The ticket_change table is storing:

434|1166731555|<my username>|due_date||

And pysqlite2 is giving me the tuple

(434, 1166731555, u'<my username>', u'due_date', None, u'')

[Christian Boos]

The problem is that we can't distinguish between a new custom field not set and a new custom field explicitly set to blank, otherwise I would have said we shouldn't write the custom field in the first place, therefore we would even avoid the problem.

But this could perhaps be an alternative solution. What would happen if we would not save fields with blank values (they'd simply be not there in the ticket_custom ). Wouldn't that work?

[Tim Hatch]

Not writing None-to-'' on saving tickets works for me. Basically if ticket_custom doesn't have a value already for the relevant field, it wouldn't be added as an entry into ticket_change when the new value is ''?

[Tim Hatch]

See attachment:t4447-dont-save-empty-field-changes.diff​ for an implementation of cboos' suggestion. I think it's doing a string comparison so the only way the change won't get written is if they're both in (None, ).

[Christian Boos]

I believe that change is correct, please commit.

[ThurnerRupert]

no regression, so leave at 0.11.1 as it seems to block 0.11 unnecessarily.

[osimons]

Just ran into this one as well today - tested it and, and the custom field no longer appear as deleted just because it had been created in the timespan between comments. Good.

Anyone committing this, or should I just go ahead? thatch?

[Christian Boos]

Yes, just go ahead.

[Tim Hatch]

It's fine to me, so please go ahead and apply, osimons.

[osimons]

OK. Applied as [6330].

[Ryan J Ollos]

I encountered a random functional test case failure because delete was one of the words in the random content. The functional test was added in [6784].

======================================================================
FAIL: runTest (trac.ticket.tests.functional.RegressionTestTicket4447)
Test for regression of http://trac.edgewall.org/ticket/4447
----------------------------------------------------------------------
Traceback (most recent call last):
  File "trac/ticket/tests/functional.py", line 1349, in runTest
    tc.notfind('deleted')
  File "trac/tests/functional/better_twill.py", line 226, in better_notfind
    raise twill.errors.TwillAssertionError(*args)
TwillAssertionError: ("match to 'deleted'", '/home/user/Workspace/t5658/teo-rjollos.git/testenv/trac/log/RegressionTestTicket4447.html')

----------------------------------------------------------------------
Ran 1578 tests in 272.221s

FAILED (failures=1)

I've reproduced the defect with this change:

trac/ticket/web_ui.py

@@ -1813 +1813 @@
                 comment_history.setdefault(rev, {}).update({'comment': old})
                 comment_history.setdefault(rev + 1, {}).update(
                         {'author': author, 'date': from_utimestamp(long(new))})
-            elif (old or new) and old != new:
+            else:
                 current['fields'][field] = {
                     'old': old, 'new': new,
                     'label': field_labels.get(field, field)}

A fix to avoid a random failure of the regression test was applied to 1.0-stable in [12155] and merged to trunk in [12156].

[Jun Omae]

I got another random failure. But, I cannot reproduce the issue with reverting [6330]. It seems no need for the tc.notfind('set to').

======================================================================
FAIL: runTest (trac.ticket.tests.functional.RegressionTestTicket4447)
Test for regression of http://trac.edgewall.org/ticket/4447
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/run/shm/4cc6ddc6443b8a6f6d79ebceac6e21dfcbc4b726/py27-mysql/trac/ticket/tests/functional.py", line 1558, in runTest
    tc.notfind('set to')
  File "/run/shm/4cc6ddc6443b8a6f6d79ebceac6e21dfcbc4b726/py27-mysql/trac/tests/functional/better_twill.py", line 234, in better_notfind
    raise twill.errors.TwillAssertionError(*args)
TwillAssertionError: ("match to 'set to'", '/run/shm/4cc6ddc6443b8a6f6d79ebceac6e21dfcbc4b726/py27-mysql/testenv/trac/log/RegressionTestTicket4447.html')