[patch] Refactoring of general `NotifyEmail` class to make it easier to subclass

[osimons <simon-code@…>]

The NotifyEmail class in trac.notification is a great starting-point for any custom email classes.

I have made a simple class that does my special things in notify() and send(), but where 98% of the logic it is left to NotifyEmail and Notify classes to handle the practical things. Among other things I use my custom Email-sending class to send registration confirmations and similar system mail.

However, for any kind of reuse of this class there is one snag: Notification needs to be enabled for the project. So, if you want to send you own mail or have some other module that sends email there is no easy way to do this based on general classes without copy & paste of logic logic from original class (in notify() as it would need to be 'skipped' in the call-chain). My system email sending new passwords based on 'forgotten password' form should not depend on ticket notification being enabled. As it is now I have to trick the class in my notify() by enabling notification, calling NotifyEmail.notify(), and then setting it back at the end… Not pretty, and potentially a source of application misbehavior.

So much for the argument of why. Here is a small patch that extracts the enable-check logic into a method that can be easily overridden for any subclass of NotifyEmail:

Index: trac/trac/notification.py
===================================================================
--- trac/trac/notification.py   (revision 5803)
+++ trac/trac/notification.py   (working copy)
@@ -209,11 +209,14 @@
             self._charset.output_charset = 'ascii'
         else:
             raise TracError, 'Invalid email encoding setting: %s' % pref
+    
+    def notification_enabled(self, resid):
+        return self.config.getbool('notification', 'smtp_enabled')
 
     def notify(self, resid, subject):
         self.subject = subject
 
-        if not self.config.getbool('notification', 'smtp_enabled'):
+        if not self.notification_enabled(resid):
             return
         self.smtp_server = self.config['notification'].get('smtp_server')
         self.smtp_port = self.config['notification'].getint('smtp_port')

It should not be able to break any existing code anywhere as far as I can deduct, and would be a great bonus for reuse of Trac code.

[Noah Kantrowitz]

+1 from me.

[Emmanuel Blot]

While I agree the notification option should be improved, I don't think the proposal is the right way to do it.

A better option would be to enable notification on a per-component basis, without using a subclassed method. For example, for the ticket component, there should be a notification option:

[ticket]
notification = enabled

The notify method would be called by the subclass if it is needed; no notification_enabled() should be required in the top-level class.

[osimons <simon-code@…>]

I have a feeling we see this the same. We want to make it the responsibility of each component - or rather each component with some sort of email functionality that then would subclass NotifyEmail. That is what I want too, but my approach was made to ensure that nothing would break in existing code anywhere. Moving the setting up or down in the chain will have implications for third-party code, and would force these modules to implement code to handle enabled/disabled somehow.

This is not the time to break email behaviour - at least not because of a minor issue like this one. This ticket is only the small tip of the iceberg. There are heaps of pitfalls with subclassing NotifyEmail, and despite its documentation as a 'base class' it is really a part of ticket notification - making it approx. 700 lines covering 3 class definitions with mandatory call-chain for every major method… And, the [notification] section in trac.ini is really used and referred to as 'ticket notification' - not as 'project-wide general notification' settings.

I have since filing this ticket discovered more issues that for me breaks the whole idea of working within the bounds of the current NotifyEmail class.

I originally made my custom email class for 0.9 some 18 months ago, and back then I ended up essentially copying what I needed from NotifyEmail.notify() and NotifyEmail.send() methods, and bypassing them in the hierarchy call chain (they were somewhat smaller then). Re-doing everything for 0.11 I actually thought I could pull this off by 'tweaking' whenever I regained control of processing.

Now I see that in NotifyEmail.send() it always adds smtp_always_bcc and smtp_always_cc distribution lists, and this happens after I have lost control of processing. Adding distibution lists to password emails is not really what I want… The issue with notification enabled/disabled is minor in comparison, as for that I manage to reset it only a few non-dangerous statements later. Resetting the distribution lists can only happen after all emails have been been sent successfully - a virtual guarantee that those settings will be cleared in quite a short time… I suppose I now have to start adding code that loads my own disposable self.config that is not referenced by an Environment. It all starts adding up to way too many codelines to just send an email (110+ so far before starting on recent findings).

This leads me to the following conclusion: Either we..

a) extend the patch and add more supporting methods like add_additional_rcpts(self): ... return cc_rcpts, bcc_rcpts that provides more 'hooks' to rework the current behavior in subclasses. This behavior should not break subclasses.
or,
b) start talking about refactoring mail and notifications with improved core design, intended to be used by any module for various purposes, and that should also be able to handle alterantive transports (like SMS, IM, IRC or whatever). Perhaps also personalised preferences is high on that list for me - 'ticket notification is enabled but I do not want them', or even 'notify me of changesets by SMS'. Likely it should be based on the component architecture, and through Context provide a means to pass req, resource, and other attributes for reuse - no doubt 99,99% of emails happen in a request context where all this already exists.
or,
c) ignore it. Me and others copy whatever we need and implement our own things as whatever fits our needs - even if it means redoing the whole NotifyEmail.

Now, this turned out to be a longer comment than expected! And, perhaps trac-dev mailing list would be more appropriate - I'll start a thread there if my comments gain any interest.

[osimons <simon-code@…>]

Oh, one more important thing: Permissions. Blindly adding people to all notifications without regarding what they are in fact allowed to see. Permission checking of smtp_always[b]cc is something that also belongs in the subclass or elsewhere close to the resource. Any permission or security policy could exclude a person from any given notification.

This flaw also exists in current TicketNotifyEmail that does no permissions checking for any receipient list. If at one time you commented on a ticket, you will forever get updates if something happens, regardless of your current status. This one is likely a separate issue, and intended only to note it here as it has bearing on the need for pathing/re-thinking notifications.

Ah. Just before hitting 'Submit', I found the thread from trac-dev mailing list ​discussion in february about notifications, and even see that there is an untouched notification branch created. Excuse my ignorance.

I'll gladly help out with this if I can be of assistance.

[osimons <simon-code@…>]

This won't make it for 0.11 - it needs quite a bit more work and clear thinking than I first imagined…

[Ryan J Ollos]

Should be looked at again in the context of TracDev/Proposals/AdvancedNotification.

[Peter Suter]

Replying to rjollos:

Should be looked at again in the context of TracDev/Proposals/AdvancedNotification.

Right. The summary of this ticket (Refactoring of general NotifyEmail class to make it easier to subclass) sounds like it is basically obsolete, as we deprecated the NotifyEmail class. Based on that I would close as wontfix. However some of the concerns may still be valid and the comments contain noteworthy insights. There are four points discussed:

1. [notification] smtp_enabled

This setting is still used as described in (deprecated) NotifyEmail, and it's now also used in the new EmailDistributor to disable sending of all emails.

The setting disables sending of all emails. Without plugins, Trac only sends ticket notification emails, so the setting has a second role to disable sending of ticket notification emails. But with plugins, the setting can not fulfill both of these roles.

The description proposes plugins opt-out of the setting. I think [notification] smtp_enabled indicates if the other [notification] smtp_* settings have been configured or not. So if it is False no emails should be sent, and plugins should not opt-out of that.

comment:1 instead proposes a second setting ([ticket] notification_enabled). I think this is not needed anymore, as it is now possible to disable ticket notifications (but allow plugins to send other emails) by disabling components (e.g. ticket subscribers).

2. smtp_always_bcc and smtp_always_cc

Same problem as above. Does always mean all emails or all ticket notification emails?

With NotifyEmail it meant all emails and plugins had no good way to opt-out.

In the new system AlwaysEmailSubscriber (since #11934) implements these and still forwards all emails. Plugins probably now have enough flexibility to deal with this. (e.g. higher priority subscriptions, disabled components, using distributors directly…)

3. Inflexibility of NotifyEmail class hierarchy

The subclass & override design is inflexible and tricky to improve without breaking compatibility. Proposal b) of comment:3 is basically TracDev/Proposals/AdvancedNotification. The inflexible base classes are deprecated, and replaced by more flexible subscriber / formatter / distributor interfaces.

4. Permissions

comment:4 describes how permissions are ignored in notifications. This is still the unchanged and probably deserves a separate ticket. (TracDev/Proposals/AdvancedNotification#Permissionfilters is a placeholder. ​th:Announcer has a Filter concept and a permission filter to drop notifications where permissions are missing. We may want to adopt this, but I have not looked deeply into it.)

→ The new system addresses these concerns, except for 4. Permissions which should be handled in a new ticket.

[Peter Suter]

2. smtp_always_bcc and smtp_always_cc

Same problem as above. Does always mean all emails or all ticket notification emails?

#9148 suggests new settings for the latter.