Trac Notifications

[Doki Pen]

I want to revisit this topic[0]:

I took over the abandoned AnnouncerPlugin[1] last year. I've been working on
the AnnouncerPlugin quite a bit[2]. I'd like to know what we need to do to
get this thing into trac.

One thing that is becoming cumbersome is supporting backward compatibility
with trac notifications. Specifically, other plugins use the Notification api
to send their own notifications. This code has to be reimplemented using the
AnnouncerPlugin api since the Notification api is not very extensible.

I still think AnnouncerPlugin needs a good amount of work before being
included in core, but I think it's at a point where I need specific feedback.
The core API is very stable. The existing notification functionality is
there. I think the arch is well described in api.py, but I'd be happy to
write up a proposal on the wiki.

What do you think?

[0] http://groups.google.com/group/trac-dev/browse_thread/thread/93190fbd9eae48d9/25ccafab5d89ed02
[1] http://trac-hacks.org/wiki/AnnouncerPlugin
[2] http://trac-hacks.org/query?status=new&status=assigned&status=reopened&status=closed&group=status&component=AnnouncerPlugin&order=priority

[Remy Blank]

> One thing that is becoming cumbersome is supporting backward compatibility
> with trac notifications. Specifically, other plugins use the Notification api
> to send their own notifications. This code has to be reimplemented using the
> AnnouncerPlugin api since the Notification api is not very extensible.

So the least we could do is add an extension point allowing to re-route
the calls to the current notification system.

> What do you think?

I'd certainly welcome a more flexible notification system. The plugin
looks quite complex, but very well designed (I haven't looked at the
code yet). Personally, I'm most interested in the "watch this"
functionality, but I'm sure others will find the other features useful
as well.

I only read the wiki page for the plugin, and haven't played with the
plugin yet, but my impression is that the configuration can be quite
complicated. I'm a bit concerned about this, as Trac already seems to be
a challenge in its current state. But maybe this is a wrong impression,
I'll try the plugin to get a better idea.

-- Remy

[doki...@doki-pen.org]

In gmane.comp.version-control.subversion.trac.devel, you wrote:
>> One thing that is becoming cumbersome is supporting backward compatibility
>> with trac notifications. Specifically, other plugins use the Notification api
>> to send their own notifications. This code has to be reimplemented using the
>> AnnouncerPlugin api since the Notification api is not very extensible.
>
> So the least we could do is add an extension point allowing to re-route
> the calls to the current notification system.

I'd hate to spend time making the old system more like announcer.

>> What do you think?
>
> I'd certainly welcome a more flexible notification system. The plugin
> looks quite complex, but very well designed (I haven't looked at the
> code yet). Personally, I'm most interested in the "watch this"
> functionality, but I'm sure others will find the other features useful
> as well.

I'm thinking that I could strip everything but the ticket_compat module
and make any new functionality into plugins. There is some code
duplication between modules that I could start moving into some utility
module. This is the type of stuff that was hard to predict, but once
you start making plugins, it becomes clear.

>
> I only read the wiki page for the plugin, and haven't played with the
> plugin yet, but my impression is that the configuration can be quite
> complicated. I'm a bit concerned about this, as Trac already seems to be
> a challenge in its current state. But maybe this is a wrong impression,
> I'll try the plugin to get a better idea.

Most of the complication comes from compatibility with notification.
But it replace notification simply by renaming [notification] to
[announcer]. Configuration is much less important with announcer since
each user has more control over what notifications that they receive.
Most of the configuration values are default settings that the user can
override. I don't hate the idea of getting rid of them entirely and
having some sensible defaults, or at least burying them in the doc as
advanced functionality.

Thanks for your feedback and I look forward to hearing your impression
after trying it.

[Remy Blank]

doki...@doki-pen.org wrote:
> I'd hate to spend time making the old system more like announcer.

That's not what I meant, sorry for being unclear. I understood that the
lack of a hook in the current notification system was a burden on
AnnouncerPlugin, and I was suggesting we add that hook (as an extension
point) to make it easier for you to catch calls from other plugins
sending notifications. Did I get this wrong?

-- Remy

[Doki Pen]

On 2009-12-01, Remy Blank <remy....@pobox.com> wrote:
> This is an OpenPGP/MIME signed message (RFC 2440 and 3156)
> --------------enigB922B00F8E9BE87F390DD041
> Content-Type: text/plain; charset=ISO-8859-1
> Content-Transfer-Encoding: quoted-printable

It makes sense in theory, but not in practice. The way the system
currently works is that plugins extend NotifyEmail. In their
constructor the call NotifyEmail.notify, which calls there email
constructing logic(get_recipients(), send()). Announcer seperates these
steps into

Producer : produces and event
Subscriber : figures out what users to notify of the event
Formatter : creates the content for the event
Distributer : actually create and distribute the email (including
headers)

Most plugins want to do the first three, but not that last one. With
the old system they are very much doing the last one, but worse,
formatting and distributing are mixed together. Also, the notify method
is called directly from the NotifyEmail class. I guess it could be
stubbed out.

But any refactoring would involve breaking backwards compatibility, at
which point I say why bother? Unless we decide we want to keep the
current notification system, in which case the whole argument is moot.

If I'm wrong, I'd be happy to admit it. Any other suggestions?

[Christian Boos]

Doki Pen wrote:
> I want to revisit this topic[0]:
>
> I took over the abandoned AnnouncerPlugin[1] last year. I've been working on
> the AnnouncerPlugin quite a bit[2]. I'd like to know what we need to do to
> get this thing into trac.
>
> One thing that is becoming cumbersome is supporting backward compatibility
> with trac notifications. Specifically, other plugins use the Notification api
> to send their own notifications. This code has to be reimplemented using the
> AnnouncerPlugin api since the Notification api is not very extensible.
>
> I still think AnnouncerPlugin needs a good amount of work before being
> included in core, but I think it's at a point where I need specific feedback.
> The core API is very stable. The existing notification functionality is
> there. I think the arch is well described in api.py, but I'd be happy to
> write up a proposal on the wiki.
>

Good timing, we're wrapping up for 0.12, so it's appropriate to start
discussing the steps ahead. As a notification overhaul has always been
on the table, I think your proposal is very welcome.

I looked a bit in the code, without actually testing the plugin yet, but
I must say that at this point I'm already pretty much convinced by what
I've seen. The code is very clean (1), understandable, and its general
architecture seems sound. Great work, from both you and Stephen!

Now we have to think about how we could integrate this in Trac itself,
as a replacement to the current notification system. I think the
separation we have introduced between trac and tracopt lately will be
very useful here, as a way to separate the core announcer components
from those which are not always required, like some subscribers.

The question of backward compatibility is also worth mentioning, and
that can perhaps be done with a transition period where the legacy
modules are first part of trac.announcer.subscribers, then in a next
release moved to tracopt.announcer.subscribers, before their complete
removal(?).

Concerning the producers, I'd welcome your opinion on the discussion
about IResourceChangeListener (http://trac.edgewall.org/ticket/8834), I
think it can be used to simplify things, eventually getting rid of the
producers entirely.


But I think that the main issue here is not really about the code, more
generally about the contribution process. Integrating such a large
subsystem can't be done without integrating at the same time a dedicated
contributor ;-) Judging by the amount of support you've already done on
the Announcer, I see that you're able to undertake this task. Of course,
what I'm asking for is not a "lifetime commitment" on your part for
supporting the new notification / announcer subsystem in Trac, but
rather the dedication necessary to get a high quality starting point, in
the release of Trac where this feature will ship. The new system has to
blend naturally with the rest of Trac and the code must lend itself to
be easily extended and maintained. As I've said before, the plugin as it
stands now shows every promise for that.

Besides, we have a huge list of open tickets on t.e.o for the
notification component, so it would be useful if you could update them
and put them in perspective of the announcer merge. You could also
create a new TracDev/Proposals page (.../AnnouncerIntegration?), which
can be used to document the integration plan, as it evolves. The later
steps would be to get an overview ticket where we could get the initial
patches, then a branch in the sandbox and an associated sub-milestone, a
bit like we did for the multirepos feature (and let's hope that *this*
time, the branch won't take 2 years before being merged ;-) ).

-- Christian

(1) in particular, the SQL queries are very nicely presented, e.g.

13 cursor.execute("""
14 SELECT value, authenticated
15 FROM session_attribute
16 WHERE sid=%s
17 AND name=%s
18 """, (sid, 'email'))

We should do that in the rest of Trac as well ;-)

[Remy Blank]

Christian Boos wrote:
> (1) in particular, the SQL queries are very nicely presented, e.g.
>
> 13 cursor.execute("""
> 14 SELECT value, authenticated
> 15 FROM session_attribute
> 16 WHERE sid=%s
> 17 AND name=%s
> 18 """, (sid, 'email'))
>
> We should do that in the rest of Trac as well ;-)

(OT) We're not far from that, are we? Except that we use the "string
concatenation" style:

cursor.execute("SELECT value, authenticated "
"FROM session_attribute "
"WHERE sid=%s AND name=%s",
(sid, 'email'))

I wouldn't be against switching, though. It would allow avoiding too
much indentation of SQL queries, and keep the quotes where we can see
them ;-)

-- Remy

[David Eads]

Hey all,

> It makes sense in theory, but not in practice.  The way the system
> currently works is that plugins extend NotifyEmail.  In their
> constructor the call NotifyEmail.notify, which calls there email
> constructing logic(get_recipients(), send()).  Announcer seperates these
> steps into

I've found extending NotifyEmail to be quite cumbersome.

> Producer    : produces and event
> Subscriber  : figures out what users to notify of the event
> Formatter   : creates the content for the event
> Distributer : actually create and distribute the email (including
>              headers)

I don't know if this will help, but I've been sitting on a patch that
attempts to refactor the notification system to utilize a listener
system (i.e. you pick the notification listeners you want to catch
events in trac.ini). I'll get it up on the trac site -- perhaps it
will be useful in this discussion. It is backwards compatible, but in
a fairly ugly way.

> Most plugins want to do the first three, but not that last one.

I'd also add, with the last one, perhaps this isn't always an email --
it really should be create and distribute the message...

David

[doki...@doki-pen.org]

In gmane.comp.version-control.subversion.trac.devel, you wrote:
> Doki Pen wrote:
>> I want to revisit this topic[0]:
>>
>> I took over the abandoned AnnouncerPlugin[1] last year. I've been working on
>> the AnnouncerPlugin quite a bit[2]. I'd like to know what we need to do to
>> get this thing into trac.
>>
>> One thing that is becoming cumbersome is supporting backward compatibility
>> with trac notifications. Specifically, other plugins use the Notification api
>> to send their own notifications. This code has to be reimplemented using the
>> AnnouncerPlugin api since the Notification api is not very extensible.
>>
>> I still think AnnouncerPlugin needs a good amount of work before being
>> included in core, but I think it's at a point where I need specific feedback.
>> The core API is very stable. The existing notification functionality is
>> there. I think the arch is well described in api.py, but I'd be happy to
>> write up a proposal on the wiki.
>>
>
> Good timing, we're wrapping up for 0.12, so it's appropriate to start
> discussing the steps ahead. As a notification overhaul has always been
> on the table, I think your proposal is very welcome.

Great news!

> I looked a bit in the code, without actually testing the plugin yet, but
> I must say that at this point I'm already pretty much convinced by what
> I've seen. The code is very clean (1), understandable, and its general
> architecture seems sound. Great work, from both you and Stephen!

Thanks.

> Now we have to think about how we could integrate this in Trac itself,
> as a replacement to the current notification system. I think the
> separation we have introduced between trac and tracopt lately will be
> very useful here, as a way to separate the core announcer components
> from those which are not always required, like some subscribers.

That's great. I'll take a look at the new code.

> The question of backward compatibility is also worth mentioning, and
> that can perhaps be done with a transition period where the legacy
> modules are first part of trac.announcer.subscribers, then in a next
> release moved to tracopt.announcer.subscribers, before their complete
> removal(?).

I just wanted to make it clear here that the announcer ticket_compat
module was written to duplicate trac notification. It was also designed
to be backword compatible so that announcer could be a drop in
replacement. There is no other module that reproduces this
functionality as of yet. We do have other modules that add additional
features, however. And in the future there could be a module that
replaces ticket_compat.

> Concerning the producers, I'd welcome your opinion on the discussion
> about IResourceChangeListener (http://trac.edgewall.org/ticket/8834), I
> think it can be used to simplify things, eventually getting rid of the
> producers entirely.

I think it is a great idea and something everyone who writes announcer
modules would really appreciate. I'll catch up on the tickets and give
some input. This could potential remove some LOC from announcer.

> But I think that the main issue here is not really about the code, more
> generally about the contribution process. Integrating such a large
> subsystem can't be done without integrating at the same time a dedicated
> contributor ;-) Judging by the amount of support you've already done on
> the Announcer, I see that you're able to undertake this task. Of course,
> what I'm asking for is not a "lifetime commitment" on your part for
> supporting the new notification / announcer subsystem in Trac, but
> rather the dedication necessary to get a high quality starting point, in
> the release of Trac where this feature will ship. The new system has to
> blend naturally with the rest of Trac and the code must lend itself to
> be easily extended and maintained. As I've said before, the plugin as it
> stands now shows every promise for that.

I expect to be around for a while. I really look forward to having the
feedback of the core trac contributers on this feature.

> Besides, we have a huge list of open tickets on t.e.o for the
> notification component, so it would be useful if you could update them
> and put them in perspective of the announcer merge. You could also
> create a new TracDev/Proposals page (.../AnnouncerIntegration?), which
> can be used to document the integration plan, as it evolves. The later
> steps would be to get an overview ticket where we could get the initial
> patches, then a branch in the sandbox and an associated sub-milestone, a
> bit like we did for the multirepos feature (and let's hope that *this*
> time, the branch won't take 2 years before being merged ;-) ).

OK, so my next steps are:

* update notification tickets with regard to the announcer merge
* create an Announcer proposal page
* create overview ticket with initial patches

Don't worry, it shouldn't take 2 years.


One other thing I'd like to ask about is the name of the feature. Is
evereyone happy with announcer? It feels strange to me. If we were to
change it, now would be the right time. Anyway, it's not that
important, just something to think about.

[doki...@doki-pen.org]

In gmane.comp.version-control.subversion.trac.devel, you wrote:
> Hey all,
>
>> It makes sense in theory, but not in practice. �The way the system
>> currently works is that plugins extend NotifyEmail. �In their
>> constructor the call NotifyEmail.notify, which calls there email
>> constructing logic(get_recipients(), send()). �Announcer seperates these
>> steps into
>
> I've found extending NotifyEmail to be quite cumbersome.
>
>> Producer � �: produces and event
>> Subscriber �: figures out what users to notify of the event
>> Formatter � : creates the content for the event
>> Distributer : actually create and distribute the email (including
>> � � � � � � �headers)
>
> I don't know if this will help, but I've been sitting on a patch that
> attempts to refactor the notification system to utilize a listener
> system (i.e. you pick the notification listeners you want to catch
> events in trac.ini). I'll get it up on the trac site -- perhaps it
> will be useful in this discussion. It is backwards compatible, but in
> a fairly ugly way.

I'd love to see your work.

>> Most plugins want to do the first three, but not that last one.
>
> I'd also add, with the last one, perhaps this isn't always an email --
> it really should be create and distribute the message...

This is how announcer works, but it can be tricky at times. For
instance, different types of events should get different email headers.
It's hard to do this in a generic way. We create callbacks in announcer
but now we have mixed concerns, the formatter knows about email. There
isn't a super clean way to handle the problem.

[Remy Blank]

doki...@doki-pen.org wrote:
> One other thing I'd like to ask about is the name of the feature. Is
> evereyone happy with announcer? It feels strange to me. If we were to
> change it, now would be the right time. Anyway, it's not that
> important, just something to think about.

I would keep either announcer or notification. Both describe the
function pretty well. But maybe I'm just no good at inventing good names.

-- Remy