Edgewall Software
Modify

Opened 11 years ago

Closed 4 years ago

#7758 closed defect (worksforme)

Cannot turn off ticket notification system via [components] section

Reported by: dbytesguard-trackhacks@… Owned by:
Priority: normal Milestone:
Component: notification Version:
Severity: normal Keywords:
Cc: Branch:
Release Notes:
API Changes:

Description

Adding the following line does not work.

[components]
trac.ticket.notification.* = disabled

See the attached change I made to enable this feature.

Granted there is the 'smtp_enabled' option but this turns the whole notification system off. If you are using a plugin that extends the notification system such as the th:DiscussionPlugin then this is bad.

I'm turning this off because I want to use the th:AnnouncerPlugin for ticket emails. I wish Trac would support its features because it is sad that it basically reimplements Trac's notification system. But none the less, I think the posted change is a good one to have.

Attachments (1)

ticket_notifcation.diff (3.4 KB ) - added by dbytesguard-trackhacks@… 11 years ago.
Proposed Change

Download all attachments as: .zip

Change History (13)

by dbytesguard-trackhacks@…, 11 years ago

Attachment: ticket_notifcation.diff added

Proposed Change

comment:1 by Christian Boos, 11 years ago

Component: generalnotification
Milestone: 0.12
Owner: set to Emmanuel Blot

attachment:ticket_notifcation.diff looks nice and is a step in the right direction.

comment:2 by Christian Boos, 11 years ago

Ah, and about your class TicketNotifyEmail: pass on line 30, there's no such things as forward declarations in Python :-) (i.e. you can safely remove that line!)

comment:3 by dbytesguard-trackhacks@…, 11 years ago

Summary: Cannot turn off ticket notifcation system via ![components] sectionCannot turn off ticket notifcation system via [components] section

Well, I guess you know my programming background now. Removing the "forward declaration" works for me. I must have misunderstood an error message.

comment:4 by Remy Blank, 10 years ago

Owner: changed from Emmanuel Blot to Remy Blank

I'll have a look at that.

comment:5 by Remy Blank, 10 years ago

Using ITicketChangeListener indeed makes sense, but there's a drawback: every call to Ticket.save_changes() will now trigger a notification, whereas before only changes made through the ticket page did, and there is no way of disabling notification on a call-by-call basis.

For example, the CommitTicketUpdater in multirepos can be configured to disable notification for updates from commits. This wouldn't be possible anymore with this change.

We could add an optional argument notify=False (False for backward compatibility) to Ticket.save_changes(), but this information must also be passed to the change listeners. I would like to avoid breaking the ITicketChangeListener, so this could be made an attribute of Ticket for the duration of the call. It's a bit of a hack, but not that bad.

Actually, how about using the same convention for all the change listeners? Add an optional notify=False argument to all model mutators, and a .notify attribute to the model object for the duration of the change listener call.

Thoughts?

comment:6 by Christian Boos, 10 years ago

If I see ticket.save_changes(..., notify=False), my first reaction is to think "this disables all notifications", i.e. it will bypass the call to listeners…

For the same reason, I don't like so much the temporary notify attribute on the model object (also because modifying the model object at all in this situation is not a good idea IMO).

So what about passing a more general parameter to save_changes(), like notifications options? In CommitTicketUpdater we could then set an option more explicitly targeting the TicketNotificationSystem, e.g.

ticket.save_changes(..., options={'ticket_notification': False})

And test that in TicketNotificationSystem.ticket_created/ticket_changed(..., options):

    if options and not options.get('ticket_notification', True):
        return

Better, we could have a more general convention, perform the call like:

options = {'disabled': 
           [component_name('trac.ticket.notification.TicketNotificationSystem')]}
ticket.save_changes(..., options)

and have save_changes do the filtering:

                options = options or {}
                disabled = options.get('disabled', [])
                for listener in TicketSystem(self.env).change_listeners:
                    if component_name(listener) not in disabled:
  	                listener.ticket_changed(self, comment, author, old_values, 
                                                options)

(with component_name a helper function doing what Environment._component_name currently does).

If modifying the ITicketChangeListener is too heavy, then why not doing that using the IResourceChangeListener proposed in ticket:6543#comment:13? The TicketNotificationSystem which would then be the first user of this new interface ;-)

in reply to:  6 comment:7 by Remy Blank, 10 years ago

Milestone: 0.12next-major-0.1X

Replying to cboos:

So what about passing a more general parameter to save_changes(), like notifications options?

In that case, we could as well just define the signature with **options:

ticket.save_changes(..., ticket_notification=False)

And in the change listener:

def ticket_changed(..., **options):
    if options.get('ticket_notification', True):
        return

Better, we could have a more general convention, perform the call like:

I find that a bit over-engineered, and it has the disadvantage that you have to know at the call site that you don't want an effect in a specific component (vs. in that component itself).

If modifying the ITicketChangeListener is too heavy, then why not doing that using the IResourceChangeListener proposed in ticket:6543#comment:13? The TicketNotificationSystem which would then be the first user of this new interface ;-)

That's a good idea indeed. Too bad there's no consensus yet on that new interface… Also, we could optionally pass the model object in that new interface (I remember some discussion about that, but I can't find the corresponding ticket).

So I'll move this ticket alongside #6543 for now.

comment:8 by Remy Blank, 10 years ago

#8621 has been closed as a duplicate.

comment:9 by Christian Boos, 10 years ago

See also #8834 for the discussion of IResourceChangeListener.

comment:10 by Remy Blank, 8 years ago

Owner: Remy Blank removed

Refocusing.

comment:11 by Ryan J Ollos, 4 years ago

Summary: Cannot turn off ticket notifcation system via [components] sectionCannot turn off ticket notification system via [components] section

This might be resolved by TracDev/Proposals/AdvancedNotification.

in reply to:  11 comment:12 by Ryan J Ollos, 4 years ago

Milestone: next-major-releases
Resolution: worksforme
Status: newclosed

Replying to rjollos:

This might be resolved by TracDev/Proposals/AdvancedNotification.

I confirmed that the following disables ticket notifications with Trac 1.1.6.

[components]
trac.ticket.notification.* = disabled

Closing as a duplicate of #11854, #4056, …

I don't foresee any more improvements to the notification system on 1.0-stable.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The ticket will remain with no owner.
The resolution will be deleted. Next status will be 'reopened'.

Add Comment


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