Edgewall Software
Modify

Opened 4 years ago

Last modified 17 months ago

#13029 new defect

Attachment notifications despite Never notify: I update a ticket

Reported by: anonymous Owned by:
Priority: normal Milestone: next-dev-1.7.x
Component: attachment Version: 1.2.2
Severity: normal Keywords:
Cc: Branch:
Release Notes:
API Changes:
Internal Changes:

Description (last modified by Jun Omae)

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.

Attachments (0)

Change History (16)

comment:1 by anonymous, 4 years ago

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

in reply to:  description ; comment:2 by Jun Omae, 4 years ago

Description: modified (diff)
Type: enhancementdefect
Version: 1.2.2

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

    diff --git a/trac/ticket/notification.py b/trac/ticket/notification.py
    index 92957d016..e34cda923 100644
    a b class TicketAttachmentNotifier(Component):  
    896896
    897897    def attachment_added(self, attachment):
    898898        self._notify_attachment(attachment, 'attachment added',
    899                                 attachment.date)
     899                                attachment.date, attachment.author)
    900900
    901901    def attachment_deleted(self, attachment):
    902         self._notify_attachment(attachment, 'attachment deleted', None)
     902        self._notify_attachment(attachment, 'attachment deleted', None, None)
    903903
    904904    def attachment_reparented(self, attachment, old_parent_realm,
    905905                              old_parent_id):
    class TicketAttachmentNotifier(Component):  
    907907
    908908    # Internal methods
    909909
    910     def _notify_attachment(self, attachment, category, time):
     910    def _notify_attachment(self, attachment, category, time, author):
    911911        resource = attachment.resource.parent
    912912        if resource.realm != 'ticket':
    913913            return
    914914        ticket = Ticket(self.env, resource.id)
    915         event = TicketChangeEvent(category, ticket, time, ticket['reporter'],
     915        event = TicketChangeEvent(category, ticket, time, author,
    916916                                  attachment=attachment)
    917917        try:
    918918            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.

comment:3 by Ryan J Ollos, 4 years ago

Milestone: 1.2.4

comment:4 by Ryan J Ollos, 4 years ago

#13072 closed as a duplicate.

comment:5 by Ryan J Ollos, 4 years ago

Milestone: 1.2.41.2.5

comment:6 by Ryan J Ollos, 3 years ago

Owner: set to Ryan J Ollos
Status: newassigned

in reply to:  2 ; comment:7 by Ryan J Ollos, 3 years ago

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?

in reply to:  7 comment:8 by Ryan J Ollos, 3 years ago

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.

in reply to:  7 ; comment:9 by Jun Omae, 3 years ago

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)

in reply to:  9 comment:10 by Ryan J Ollos, 3 years ago

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

comment:11 by Ryan J Ollos, 3 years ago

Milestone: 1.2.51.2.6

in reply to:  9 comment:12 by Ryan J Ollos, 3 years ago

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.

comment:13 by Ryan J Ollos, 2 years ago

Milestone: 1.2.61.4.2

in reply to:  2 comment:14 by Ryan J Ollos, 2 years ago

Milestone: 1.4.2next-dev-1.5.x

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.

comment:15 by Ryan J Ollos, 2 years ago

Owner: Ryan J Ollos removed
Status: assignednew

comment:16 by Ryan J Ollos, 17 months ago

Milestone: next-dev-1.5.xnext-dev-1.7.x

Milestone renamed

Modify Ticket

Change Properties
Set your email in Preferences
Action
as new The ticket will remain with no owner.
The ticket will be disowned.
as The resolution will be set. Next status will be 'closed'.
The owner will be changed from (none) to anonymous. Next status will be 'assigned'.

Add Comment


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