Modification to ticket summary that changes leading or trailing whitespace only results in an empty comment

[Ryan J Ollos <ryano@…>]

A modification to the ticket summary that changes only the amount of leading or trailing whitespace results in an empty comment in the change history. The leading and trailing whitespace seems to gets stripped out on submit anyway, so Trac should probably just respond that there was no change to the ticket.

[Ryan J Ollos <ryano@…>]

Patch against r11098 of the trunk.

[Ryan J Ollos <ryano@…>]

Patch against r11103 of trunk, which adds checks for whitespace-only comments.

[Ryan J Ollos <ryano@…>]

The patch t10748-r11103-1.patch​ adds some unit tests and addresses an issue discussed in comment:5:ticket:10749 through comment:8:ticket:10749: comments that contain only whitespace are stripped, which avoids outcomes such as comment:1:ticket:10749.

[Christian Boos]

Looks good, except for the following:

props_unchanged = all(item in self.values.items() for item in self._old.items())

I think we could directly compare them: self.values == self.old (per ​5.9).

Also, verify this still works for checkbox fields.

[Ryan J Ollos <ryano@…>]

Replying to cboos:

I think we could directly compare them: self.values == self.old (per ​5.9).

I'm not seeing how that would work in this case. If I change the summary field by adding some whitespace, then:

self.values is:

{'status': u'new', 'changetime': datetime.datetime(2012, 7, 13, 20, 38, 18, 935274, tzinfo=<FixedOffset "UTC" 0:00:00>), 'description': u'', 'reporter': u'anonymous', 'cc': u'', 'resolution': u'', 'time': datetime.datetime(2012, 7, 13, 20, 21, 56, 277782, tzinfo=<FixedOffset "UTC" 0:00:00>), 'component': u'component1', 'summary': u'This is a summary', 'priority': u'major', 'keywords': u'', 'version': u'1.0', 'milestone': u'milestone1', 'owner': u'somebody', 'type': u'defect'}

self._old is:

{'summary': u'This is a summary'}

Also, verify this still works for checkbox fields.

I'm not sure what I should be looking for here. Changing the value of a checkbox adds an entry to self._old, so the behavior seems to be the same as for any other field.

I checked this with manual testing and then wrote a simple unit test to demonstrate this, but I'm not sure the unit test adds much value to the suite:

    def test_prop_checkbox_change_is_saved(self):
        self.env.config.set('ticket-custom', 'cb', 'checkbox')
        ticket = Ticket(self.env)
        ticket.populate({'cb': '1'})
        ticket.insert()

        ticket['cb'] = '0'
        ticket.save_changes()        
        
        self.assertEqual('0', ticket['cb'])
        self.assertEqual(2, len(ticket.get_changelog()))

[Ryan J Ollos <ryano@…>]

Patch against r11111 of the trunk. Minor change: in calls to assertEqual, arguments have been re-ordered to expected, actual; per the unit testing convention.

[Ryan J Ollos <ryano@…>]

Removed unintended change from t10748-r11111-1.patch​.

[Christian Boos]

Replying to Ryan J Ollos <ryano@…>:

Replying to cboos:

I think we could directly compare them: self.values == self.old (per ​5.9).

I'm not seeing how that would work in this case. If I change the summary field by adding some whitespace, then: …

self._old is:

{'summary': u'This is a summary'}

Ok, my bad, I made wrong assumptions (that _old would contain everything).

But still, what I don't like is the (k,v) in dict.items() part which doesn't look optimal. What about: all(values.get(k) == v for k, v in old.items())?

[Remy Blank]

Or even (using an iterator):

all(values.get(k) == v for k, v in old.iteritems())

[Ryan J Ollos <ryano@…>]

It sounds like the iterator is more efficient (​SO:10458437, though I can't imagine it makes a difference in this case), so I went ahead and implemented that in the latest version of the patch.

[Ryan J Ollos <ryano@…>]

Patch against r11115 of the trunk. Refactored comparison of values and _old.

[Christian Boos]

Last patch looks good, and all tests pass. Applied in r11219.

Thanks!