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

[txcraig@…]

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.

[Remy Blank]

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.

[AdrianFritz]

#10086 already has a patch :-)

[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.

[Jaya Kirtani <jaya.kirtani@…>]

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

[Jaya Kirtani <jaya.kirtani@…>]

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.

[Remy Blank]

Thanks.

[Remy Blank]

Update the changetime when deleting comments.

[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?

[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.

[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.

[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".

[Remy Blank]

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