Cannot turn off ticket notification system via [components] section

[dbytesguard-trackhacks@…]

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.

[dbytesguard-trackhacks@…]

Proposed Change

[Christian Boos]

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

[Christian Boos]

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!)

[dbytesguard-trackhacks@…]

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

[Remy Blank]

I'll have a look at that.

[Remy Blank]

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?

[Christian Boos]

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 ;-)

[Remy Blank]

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.

[Remy Blank]

#8621 has been closed as a duplicate.

[Christian Boos]

See also #8834 for the discussion of IResourceChangeListener.

[Remy Blank]

Refocusing.

[Ryan J Ollos]

This might be resolved by TracDev/Proposals/AdvancedNotification.

[Ryan J Ollos]

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.