Edgewall Software
Modify

Opened 7 years ago

Closed 6 years ago

Last modified 6 years ago

#10486 closed defect (fixed)

[PATCH] Deleting a ticket comment does not update ticket changetime

Reported by: txcraig@… Owned by: Jaya Kirtani <jaya.kirtani@…>
Priority: normal Milestone: 1.0.1
Component: ticket system Version: 0.12-stable
Severity: minor Keywords: bitesized
Cc:
Release Notes:

Update the ticket changetime to the current time when deleting a ticket comment.

API Changes:

Description

This is similar to #10086 which noted that editing comments does not update the ticket changetime.

When a comment is deleted (using new feature in .12 #3641) the ticket changetime is not updated. The ticket is in fact modified, so it seems like changetime should be updated. This is important to people trying to pickup ticket changes using getRecentChanges API.

Attachments (2)

patch_10486.diff (5.6 KB ) - added by Jaya Kirtani <jaya.kirtani@…> 6 years ago.
Patch for proposed fix of updating ticket last modified time to reflect comment deletion time
10486-delete-changetime-r11535.patch (6.6 KB ) - added by Remy Blank 6 years ago.
Update the changetime when deleting comments.

Download all attachments as: .zip

Change History (13)

comment:1 Changed 7 years ago by Remy Blank

Keywords: bitesized added
Milestone: next-minor-0.12.x

I have just written a nice explanation about why deleting a comment should not update the changetime, but looking at #10086, it doesn't make sense. So yes, I agree with you, the changetime should be updated on comment deletion (it's actually changed, but only when deleting the last comment: it's reset to the value it had before the comment was created).

Patch welcome, of course.

comment:2 Changed 7 years ago by AdrianFritz

#10086 already has a patch :-)

comment:3 Changed 7 years ago by Remy Blank

That patch fixed updating the changetime when editing a ticket comment, and was applied. This ticket is about the case where you delete a ticket comment.

Changed 6 years ago by Jaya Kirtani <jaya.kirtani@…>

Attachment: patch_10486.diff added

Patch for proposed fix of updating ticket last modified time to reflect comment deletion time

comment:4 Changed 6 years ago by Jaya Kirtani <jaya.kirtani@…>

Summary: Deleting a ticket comment does not update ticket changetime[PATCH] Deleting a ticket comment does not update ticket changetime

Above patch updates changetime to comment deletion time as suggested in comment 1.

The patch also updates the comment deletion unit test cases in ticket/tests/model.py

Also verified that all the ticket unit tests pass after the changes.

comment:5 Changed 6 years ago by Remy Blank

Milestone: next-minor-0.12.x1.0.1
Owner: set to Remy Blank
Status: newassigned

Thanks.

Changed 6 years ago by Remy Blank

Update the changetime when deleting comments.

comment:6 Changed 6 years ago by Remy Blank

10486-delete-changetime-r11535.patch is an updated patch that adds an optional when argument to delete_change(), and sets the changetime to that instead of always taking the current time. This is useful for unit testing, and also consistent with save_changes() and modify_comment().

But before I apply it, a quick sanity check. Is this really what we want? Removing ticket changes is usually done to correct mistakes and remove spam. In both cases, I would prefer the changetime to be reverted to the value it had before the mistake or spam (i.e. the current behavior).

Opinions?

comment:7 Changed 6 years ago by Jaya Kirtani <jaya.kirtani@…>

Yes, come to think of it - updating changetime on comment deletion will make sense only if we also record that event in the ticket change history with something like 'Comment deleted'. That way, there will be some event that matches with the updated time. We will probably need a way to summarize it, if a bunch of comments are deleted one after the other. So updating the changetime makes sense only if one is interested in keeping track of comment deletions. Otherwise probably existing behavior is good enough.

comment:8 Changed 6 years ago by txcraig@…

updating changetime on comment deletion will make sense only if we also record that event in the ticket change history with something like 'Comment deleted'.

I agree with this, although I would reword it as "allowing comment deletions only makes sense if we also record …". Similar to Subversion - when you revert a change, you aren't deleting the revision. I realize that is a much bigger change.

So updating the changetime makes sense only if one is interested in keeping track of comment deletions. Otherwise probably existing behavior is good enough.

I would think the most common case would be that one is interested in keeping track of 'the ticket', where 'the ticket' is made up of its parts, and comments are certainly part of a ticket. While some users may think of deleted spam or deleted comments as unimportant, others might deem those events (and the new state of the ticket) as critical. I certainly do.

So for some scenarios, it is more convenient to not update the changetime (spam). For others (external connectors that sync Trac tickets) it is the opposite. Should this be decided based on convenience for a particular scenario? I would argue against that - the way to decide is to determine which choice results in Trac being the most truthful about a ticket's modtime. For an external observer who is watching a Trac ticket over time, the ticket inarguably has changed when a comment is deleted.

We will probably need a way to summarize it, if a bunch of comments are deleted one after the other

Perhaps the ticket view could hide deleted comments by default and have a checkbox to toggle them on.

record that event in the ticket change history with something like 'Comment deleted'.

Is it OK to open a new ticket for this functionality or would that be covered by this ticket?

Thanks for everyone's time working on this issue.

comment:9 in reply to:  6 Changed 6 years ago by Christian Boos

Replying to rblank:

10486-delete-changetime-r11535.patch is an updated patch that adds an optional when argument to delete_change(), and sets the changetime to that instead of always taking the current time. This is useful for unit testing, and also consistent with save_changes() and modify_comment().

Tested, works well.

But before I apply it, a quick sanity check. Is this really what we want? Removing ticket changes is usually done to correct mistakes and remove spam. In both cases, I would prefer the changetime to be reverted to the value it had before the mistake or spam (i.e. the current behavior).

But that would mean that the people who want to be in sync with the ticket would miss the opportunity to also remove the deleted comment on their side.

So while I agree it would even be better to store a "comment deleted" event and by default not show it in the changelog, this is a good first step. And in the meantime, the lack of a corresponding changelog entry matching the last modified timestamp will mean "last change was a comment deletion".

comment:10 Changed 6 years ago by Remy Blank

Release Notes: modified (diff)
Resolution: fixed
Status: assignedclosed

Thanks for the feedback. Patch applied in [11547].

comment:11 Changed 6 years ago by Remy Blank

Owner: changed from Remy Blank to Jaya Kirtani <jaya.kirtani@…>

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Jaya Kirtani <jaya.kirtani@…>.
The resolution will be deleted.
to The owner will be changed from Jaya Kirtani <jaya.kirtani@…> 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.