Edgewall Software
Modify

Opened 8 years ago

Closed 8 years ago

#10748 closed defect (fixed)

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

Reported by: Ryan J Ollos <ryano@…> Owned by: Ryan J Ollos <ryano@…>
Priority: normal Milestone: 1.0
Component: ticket system Version: 1.0dev
Severity: minor Keywords: bitesized
Cc: Branch:
Release Notes:
API Changes:
Internal Changes:

Description (last modified by 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.

Attachments (6)

EmptyComments.png (57.4 KB ) - added by Ryan J Ollos <ryano@…> 8 years ago.
t10748-r11098.patch (567 bytes ) - added by Ryan J Ollos <ryano@…> 8 years ago.
Patch against r11098 of the trunk.
t10748-r11103-1.patch (1.6 KB ) - added by Ryan J Ollos <ryano@…> 8 years ago.
Patch against r11103 of trunk, which adds checks for whitespace-only comments.
t10748-r11111-1.patch (1.7 KB ) - added by Ryan J Ollos <ryano@…> 8 years ago.
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.
t10748-r11111-2.patch (1.6 KB ) - added by Ryan J Ollos <ryano@…> 8 years ago.
Removed unintended change from t10748-r11111-1.patch​.
t10748-r11115-1.patch (1.6 KB ) - added by Ryan J Ollos <ryano@…> 8 years ago.
Patch against r11115 of the trunk. Refactored comparison of values and _old.

Download all attachments as: .zip

Change History (17)

by Ryan J Ollos <ryano@…>, 8 years ago

Attachment: EmptyComments.png added

by Ryan J Ollos <ryano@…>, 8 years ago

Attachment: t10748-r11098.patch added

Patch against r11098 of the trunk.

comment:1 by Ryan J Ollos <ryano@…>, 8 years ago

Description: modified (diff)
Keywords: bitsized added
Version: 1.0dev

comment:2 by Ryan J Ollos <ryano@…>, 8 years ago

Keywords: bitesized added; bitsized removed

by Ryan J Ollos <ryano@…>, 8 years ago

Attachment: t10748-r11103-1.patch added

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

comment:3 by Ryan J Ollos <ryano@…>, 8 years ago

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.

comment:4 by Christian Boos, 8 years ago

Milestone: next-dev-1.1.x

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.

in reply to:  4 ; comment:5 by Ryan J Ollos <ryano@…>, 8 years ago

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()))

by Ryan J Ollos <ryano@…>, 8 years ago

Attachment: t10748-r11111-1.patch added

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.

by Ryan J Ollos <ryano@…>, 8 years ago

Attachment: t10748-r11111-2.patch added

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

in reply to:  5 comment:6 by Christian Boos, 8 years ago

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())?

comment:7 by Remy Blank, 8 years ago

Or even (using an iterator):

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

comment:8 by Ryan J Ollos <ryano@…>, 8 years ago

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.

by Ryan J Ollos <ryano@…>, 8 years ago

Attachment: t10748-r11115-1.patch added

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

comment:9 by Christian Boos, 8 years ago

Milestone: next-dev-1.1.x1.0

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

Thanks!

comment:10 by Christian Boos, 8 years ago

Owner: set to Ryan J Ollos <ryano@…>

comment:11 by Christian Boos, 8 years ago

Resolution: fixed
Status: newclosed

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Ryan J Ollos <ryano@…>.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from Ryan J Ollos <ryano@…> to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.