Edgewall Software
Modify

Opened 6 years ago

Closed 5 years ago

Last modified 5 years ago

#11377 closed enhancement (fixed)

Extension point interface for ticket's comment

Reported by: t2y <tetsuya.morimoto@…> Owned by: Jun Omae
Priority: normal Milestone: 1.0.2
Component: ticket system Version: 1.0-stable
Severity: normal Keywords:
Cc: Jun Omae, olemis+trac@…, Ryan J Ollos Branch:
Release Notes:
API Changes:

Add ticket_comment_modified and ticket_change_deleted methods to ITicketChangeListener interface.

Description

I'm testing a full-text search plugin for trac now.

I found ITicketChangeListener doesn't handle when I edit a ticket's comment, because ticket.modify_comment() method has no extension point. It means there is no way to update the index for full-text search when a comment is edited.

I need a new extension point interface for ticket's comment.

Attachments (1)

tracadvsearchplugin.diff (2.0 KB ) - added by Jun Omae 6 years ago.

Download all attachments as: .zip

Change History (16)

comment:1 by Ryan J Ollos, 6 years ago

I didn't follow the conversation in #11148 too closely, but the IResourceChangeListener might support your use-case.

comment:2 by t2y <tetsuya.morimoto@…>, 6 years ago

Thank you for good information. I'll try!

comment:3 by t2y <tetsuya.morimoto@…>, 6 years ago

I've misunderstood #11148 is not implemented yet …

BTW, why don't you use this for ticket cross-reference?

th:TracTicketReferencePlugin

Last edited 5 years ago by Ryan J Ollos (previous) (diff)

by Jun Omae, 6 years ago

Attachment: tracadvsearchplugin.diff added

comment:4 by Jun Omae, 6 years ago

Cc: Jun Omae added
Milestone: next-stable-1.0.x

We can handle modify and delete ticket's comment to use IRequestFilter and Request.add_redirect_listener. Of course, that's not elegant….

tracadvsearchplugin.diff is untested patch for th:TracAdvancedSearchPlugin.

I think we should add modified_ticket_comment and deleted_ticket_comment methods to ITicketChangeListener because it seems that we need more time to fix #11148. Thoughts?

Last edited 6 years ago by Ryan J Ollos (previous) (diff)

in reply to:  4 ; comment:5 by Ryan J Ollos, 6 years ago

Replying to jomae:

I think we should add modified_ticket_comment and deleted_ticket_comment methods to ITicketChangeListener because it seems that we need more time to fix #11148. Thoughts?

I think it makes sense to add the methods to ITicketChangeListener in 1.0.x. To have consistency in naming with ticket_created, ticket_change and ticket_deleted, the names ticket_comment_modified and ticket_comment_deleted seem better to me (or even comment_modified, comment_deleted; as long as the verb goes last).

In the case of deleting a comment though, we are really deleting a change, which includes ticket properties: Ticket.delete_change is called by TicketDeleter. When editing a comment, Ticket.modify_comment is called, but perhaps we should make the interface more generic for cases that properties might be edited through a plugin. So should the methods be ticket_change_modified and ticket_change_deleted?

Last edited 6 years ago by Ryan J Ollos (previous) (diff)

in reply to:  5 comment:6 by Olemis Lang <olemis+trac@…>, 6 years ago

Cc: olemis+trac@… added

Replying to rjollos:

Replying to jomae:

I think we should add modified_ticket_comment and deleted_ticket_comment methods to ITicketChangeListener because it seems that we need more time to fix #11148. Thoughts?

I think it makes sense to add the methods to ITicketChangeListener in 1.0.x. To have consistency in naming with ticket_created, ticket_change and ticket_deleted, the names ticket_comment_modified and ticket_comment_deleted seem better to me (or even comment_modified, comment_deleted; as long as the verb goes last).

In the case of deleting a comment though, we are really deleting a change, which includes ticket properties: Ticket.delete_change is called by TicketDeleter. When editing a comment, Ticket.modify_comment is called, but perhaps we should make the interface more generic for cases that properties might be edited through a plugin. So should the methods be ticket_change_modified and ticket_change_deleted?

I'd rather prefer to integrate this into the changes proposed for #11148 , and thereby prefer comment_modified because :

  • there are other ways to modify a ticket (e.g. BH ticket relations …)
  • comments may be leveraged as resources bound to other resources i.e. like attachments and scattered change event notification interfaces will just over complicate things

in reply to:  5 ; comment:7 by Jun Omae, 6 years ago

I think it makes sense to add the methods to ITicketChangeListener in 1.0.x. To have consistency in naming with ticket_created, ticket_change and ticket_deleted, the names ticket_comment_modified and ticket_comment_deleted seem better to me (or even comment_modified, comment_deleted; as long as the verb goes last).

Indeed. But I think ticket_change_deleted is better cause of Ticket.delete_change(). Proposed changes can be found in jomae.git@t11377.

In the case of deleting a comment though, we are really deleting a change, which includes ticket properties: Ticket.delete_change is called by TicketDeleter. When editing a comment, Ticket.modify_comment is called, but perhaps we should make the interface more generic for cases that properties might be edited through a plugin. So should the methods be ticket_change_modified and ticket_change_deleted?

No. The methods doesn't provide a way to edit the properties of ticket. The interface is a post-hook is called after a ticket is changed. If the enhancement is reasonable, we should add or improve an interface for the edit of properties, e.g. ITicketManipulator.prepare_ticket() and #11477.

Last edited 6 years ago by Jun Omae (previous) (diff)

comment:8 by Ryan J Ollos, 6 years ago

Cc: Ryan J Ollos added

in reply to:  7 ; comment:9 by Ryan J Ollos, 5 years ago

Replying to jomae:

Indeed. But I think ticket_change_deleted is better cause of Ticket.delete_change(). Proposed changes can be found in jomae.git@t11377.

I tested the change and it seems good. I rebased on 1.0-stable before testing. It would be great to commit it.

in reply to:  9 comment:10 by Jun Omae, 5 years ago

Milestone: next-stable-1.0.x1.0.2
Owner: set to Jun Omae
Status: newassigned

Replying to rjollos:

I tested the change and it seems good. I rebased on 1.0-stable before testing. It would be great to commit it.

Thanks for the testing. I'll push it with unit tests. Unit tests would be added to trac/ticket/tests/model.py.

comment:11 by Jun Omae, 5 years ago

I refreshed the branch with unit tests, jomae.git@t11377.1.

comment:12 by Jun Omae, 5 years ago

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

Committed in [12956] and merged to trunk in [12957]. Thanks.

comment:13 by Ryan J Ollos, 5 years ago

We might want to move the Release Notes in this ticket to API Changes since it's a developer-facing change.

comment:14 by Jun Omae, 5 years ago

API Changes: modified (diff)
Release Notes: modified (diff)

Indeed. Thanks for pointing out.

comment:15 by t2y <tetsuya.morimoto@…>, 5 years ago

I really wanted this enhancement. Thank you!

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Jun Omae.
The resolution will be deleted. Next status will be 'reopened'.
to as closed The owner will be changed from Jun Omae 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.