Do not add ticket change entry for default value of new ticket custom field

[Ryan J Ollos]

Adding a new custom field, such as:

[ticket-custom]
checkbox = checkbox
checkbox.value = 0

results in a ticket change entry the next time an existing ticket is updated.

I'd like to discuss if it makes sense to skip adding the ticket_change table entry when the ticket custom field has a default value.

Issue raised in ​gdiscussion:trac-users:Kind9BaSR4Q.

[Ryan J Ollos]

Proposed changes in log:rjollos.git:t13133_ticket_change_custom_fields.

[Ryan J Ollos]

Changes committed in r17027, r17028.

[Jun Omae]

Replying to Ryan J Ollos:

Changes committed in r17027, r17028.

If a time field is updated from None to the default value, the field is not listed in changelog.

>>> from datetime import datetime
>>> from trac.util.datefmt import utc
>>> from trac.test import EnvironmentStub
>>> from trac.ticket.model import Ticket
>>> env = EnvironmentStub()
>>> env.config.set('ticket-custom', 'date1', 'time')
>>> env.config.set('ticket-custom', 'date1.value', '2019-08-07T12:34:56Z')
>>> env.config.set('ticket-custom', 'date1.format', 'datetime')
>>> env.config.save()
>>> t = Ticket(env)
>>> t['summary'] = 'Summ'
>>> t['status'] = 'new'
>>> t['date1'] = None
>>> t.insert()
1
>>> env.db_query('SELECT * FROM ticket_custom WHERE ticket=1')
[(1, u'date1', None)]
>>>
>>> t = Ticket(env, 1)
>>> t['date1'] = datetime(2019, 8, 7, 12, 34, 56, tzinfo=utc)
>>> t.save_changes(comment='Set date1')  # Saving with the default value for `date1` field
1
>>> env.db_query('SELECT * FROM ticket_custom WHERE ticket=1')
[(1, u'date1', u'001565181296000000')]
>>> list(t.get_changelog())  # `date1` field is not listed
[(datetime.datetime(2019, 8, 8, 4, 42, 35, 873743, tzinfo=<FixedOffset "UTC" 0:00:00>), None, u'comment', u'1', u'Set date1', 1)]

[Ryan J Ollos]

Proposed change:

trac/ticket/model.py

@@ -399 +399 @@
                         db("""INSERT INTO ticket_custom (ticket,name,value)
                               VALUES(%s,%s,%s)
                               """, (self.id, name, db_val))
-                    # Don't add ticket change entry for custom field that
-                    # was added after ticket was created.
-                    if old_db_val is None:
-                        field = self.fields.by_name(name)
-                        default = self._custom_field_default(field)
-                        if self.values.get(name) == default:
-                            continue
+                        # Don't add ticket change entry for custom field that
+                        # was added after ticket was created.
+                        if old_db_val is None:
+                            field = self.fields.by_name(name)
+                            default = self._custom_field_default(field)
+                            if self.values.get(name) == default:
+                                continue
                 else:
                     db("UPDATE ticket SET %s=%%s WHERE id=%%s"
                        % name, (db_val, self.id))

I'll add test coverage.

[Ryan J Ollos]

Proposed changes in [af0276770/rjollos.git].

[Ryan J Ollos]

Regression fix committed in r17047.

[Ryan J Ollos]

​CI Test failure for PostgreSQL on OSX. Should we order the changelog by field?

trac/ticket/model.py

@@ -462 +462 @@
                 SELECT time, author, 'comment', null, description,
                   0 AS permanent
                 FROM attachment WHERE type='ticket' AND id=%s AND time=%s
-                ORDER BY time,permanent,author
+                ORDER BY time,permanent,author,field
                 """
             args = (self.id, when_ts, sid, when_ts, sid, when_ts)
         else:
@@ -477 +477 @@
                 SELECT time, author, 'comment', null, description,
                   0 AS permanent
                 FROM attachment WHERE type='ticket' AND id=%s
-                ORDER BY time,permanent,author
+                ORDER BY time,permanent,author,field
                 """
             args = (self.id, sid, sid)
         log = []

[Ryan J Ollos]

Proposed changes in [91ede4ff0/rjollos.git], targeted to 1.0-stable. In the meantime, I've applied a fix in r17052.

[Jun Omae]

Proposed changes in [91ede4ff0/rjollos.git], targeted to 1.0-stable. In the meantime, I've applied a fix in r17052.

The changes look good to me.

Trivial thing, Ticket.get_changelog() is invoked twice in trunk:trac/ticket/tests/model.py.

-        changelog = sorted(ticket.get_changelog())
-        self.assertEqual(7, len(ticket.get_changelog()))
+        changelog = ticket.get_changelog()
+        self.assertEqual(7, len(changelog))

[Ryan J Ollos]

Thanks for the review. Committed to 1.0-stable in r17060, merged in r17061, r17062 (including comment:13 change).