Attachment notifications despite Never notify: I update a ticket

[anonymous]

I have configured:

Subscription rules: Never notify: I update a ticket

And this works well most of the time, but I still get a notification when I attach a file to a ticket.

[anonymous]

browser:trunk/trac/ticket/notification.py@16368:683#L658​ is this correct when the attachment is not added by the ticket reporter?

[Jun Omae]

And this works well most of the time, but I still get a notification when I attach a file to a ticket.

I tested the case on Trac 1.2.2 but works for me.

browser:trunk/trac/ticket/notification.py@16368:683#L658​​ is this correct when the attachment is not added by the ticket reporter?

You're right. We should use attachment.author as author argument.

trac/ticket/notification.py

@@ -896 +896 @@
 
     def attachment_added(self, attachment):
         self._notify_attachment(attachment, 'attachment added',
-                                attachment.date)
+                                attachment.date, attachment.author)
 
     def attachment_deleted(self, attachment):
-        self._notify_attachment(attachment, 'attachment deleted', None)
+        self._notify_attachment(attachment, 'attachment deleted', None, None)
 
     def attachment_reparented(self, attachment, old_parent_realm,
                               old_parent_id):
@@ -907 +907 @@
 
     # Internal methods
 
-    def _notify_attachment(self, attachment, category, time):
+    def _notify_attachment(self, attachment, category, time, author):
         resource = attachment.resource.parent
         if resource.realm != 'ticket':
             return
         ticket = Ticket(self.env, resource.id)
-        event = TicketChangeEvent(category, ticket, time, ticket['reporter'],
+        event = TicketChangeEvent(category, ticket, time, author,
                                   attachment=attachment)
         try:
             NotificationSystem(self.env).notify(event)

However, author is not available when deleting attachment. I consider we should implement the notification in AttachmentModule.process_request to pass req.authname to the author.

[Ryan J Ollos]

#13072 closed as a duplicate.

[Ryan J Ollos]

Replying to Jun Omae:

However, author is not available when deleting attachment. I consider we should implement the notification in AttachmentModule.process_request to pass req.authname to the author.

What if we added author parameter to Attachment.delete and IAttachmentChangeListener.attachment_deleted?

[Ryan J Ollos]

Replying to Ryan J Ollos:

What if we added author parameter to Attachment.delete and IAttachmentChangeListener.attachment_deleted?

Proposed changes: log:rjollos.git:t13029_attachment_notifications. I will add test coverage.

TODO Make IAttachmentChangeListener API changes in #12787.

[Jun Omae]

Replying to Ryan J Ollos:

Replying to Jun Omae:

However, author is not available when deleting attachment. I consider we should implement the notification in AttachmentModule.process_request to pass req.authname to the author.

What if we added author parameter to Attachment.delete and IAttachmentChangeListener.attachment_deleted?

If we add author parameter to Attachment.delete(), I think we should add to Ticket.delete() and Wiki.delete() also. If we don't change Ticket.delete(), I don't think we should change Attachment.delete().

Methods without author parameter:

• Wiki:
  • Wiki.delete(self, version=None)
  • Wiki.rename(self, new_name)
  • Wiki.edit_comment(self, new_comment)
• Ticket:
  • Ticket.insert(self, when=None)
    • ticket['reporter'] can be used
  • Ticket.delete(self)
  • Ticket.delete_change(self, cnum=None, cdate=None, when=None)
  • Ticket.modify_comment(self, cdate, author, comment, when=None)
• Milestone:
  • Milestone.insert(self)
  • Milestone.delete(self)
• Attachment:
  • Attachment.insert(self, filename, fileobj, size, t=None)
    • attachment.author can be used
  • Attachment.delete(self)
  • Attachment.move(self, new_realm=None, new_id=None, new_filename=None)
  • Attachment.reparent(self, new_realm, new_id)
  • Attachment.delete_all(cls, env, parent_realm, parent_id)
  • Attachment.reparent_all(cls, env, parent_realm, parent_id, new_realm, new_id)

[Ryan J Ollos]

Replying to Jun Omae:

If we add author parameter to Attachment.delete(), I think we should add to Ticket.delete() …

The proposed changes already include addition of author parameter to Ticket.delete().

[Ryan J Ollos]

Replying to Jun Omae:

If we add author parameter to Attachment.delete(), I think we should add to Ticket.delete() and Wiki.delete() also.

I will add to Wiki.delete. It's probably useful for #1660.

[Ryan J Ollos]

Replying to Jun Omae:

However, author is not available when deleting attachment. I consider we should implement the notification in AttachmentModule.process_request to pass req.authname to the author.

It could be fine as a short-term solution, but I don't think we should be importing from the ticket package in the attachment module. I suppose we could implement in post_process_request.

This is the same problem discussed in #12414 and #13282:

I think it's been discussed before, ITicketChangeListener might be more useful if the request was passed as an argument. One way would be to move the call to ITicketChangeListener out of the model object.

Moving the issue forward until a better solution can be arrived at, which hopefully addresses all of the issues discussed in these tickets.

[Ryan J Ollos]

Milestone renamed