Add comment notifications via e-mail

Some quick thoughts:

- The correct way to do this is using triggers
- token module makes things like this way easier. Dries - make your stance on token known so that we can get it into core too because it is a bigger enabler than comment notifications.

I am the current maintainer of notify module, and Dries contacted me about this issue. For the benefit of community discussion, here is my response to him:

I am successfully using the 6.x-dev version on a couple sites, although it has not had as much testing from the community as I'd hoped. The module has a convoluted history (I think I'm the fourth maintainer) and the code would need a lot of attention to be core-worthy, in my opinion.

As compared to the other solutions, Notify has the advantage that it can notify of both new comments and new nodes. It has the disadvantage that it can not notify anonymous users.

I maintain the module because it has become a key component for many of my clients; but I'm willing to consider other solutions in the future. I suspect the Drupal community might have better luck using another module as a starting point, but if you think notify is the best candidate, I am willing to assist as much as I can, though I would have limited time available. I would think the next step would be to find some more experienced programmer(s) to become co-maintainer(s).

All the Best,

Matt

I'm going to go out on a limb here and agree with robertDouglass.

Token obviously leaves parts of the solution unmet, and this is where chx has a point, what are the specs? "After publishing a comment" "Send email to user" "Containing comment". Code = Trigger/Rules + Action + Token. We have the same issue in the project module: Allow (un)subscription to individual project issues

Both issues are only missing an action to send generic text.

@Dries, what did you mean by, "Using token module might make the code a bit more compact, but also more obscure."? I'm looking at this more as a user than developer, and tokens would be right up there in obscurity with actions, triggers, and nodes.

To put comments above and possibly below into more context, some details from Dries' comments on his blog:

I'd like to get the comment notify module included into Drupal core. [...] Creating a dependency on token module is going to complicate that. Looking at the code of the module, I don't see a strong need to use token module. The code is easy to read and understand as it is. Using token module might make the code a bit more compact, but also more obscure.

Either we massage the comment notify module into what we need (e.g. drop some of the options, remove a number of the tokens and fix some user interface issues), or we start from scratch. Either works for me.

I thought about this some more and I think I would prefer to only deal with comment notifications -- or better yet, fold that part in the the comment module in core.

Node notifications for node authors are relatively easy to implement using triggers and actions. We should leverage them better and this sounds like an excellent use case. I think that part of the comment notify module should be removed or refactored.

@Gabor: I understand this seems like a simple inclusion, but perhaps this is a feature too specific for drupal core; a job for an installation profile.

Core is powerful when it is paired with d.o to give any user the power to make a site that does polling and emailing comments and whatever the person or group needs. I disagree with this statement on d.o's about page, that "[it] allows an individual or a community of users to easily publish, manage and organize a wide variety of content on a website." Every example would require some contrib code and guidance to "easily" make a beautiful drupal site.

People I have ever recommended drupal to have been impressed, but then they're met with a tremendous hurdle after installation, and it's always a different hurdle. This is where the improved accessibility of Installation Profiles, Modules, Themes, and Documentation will make the D7 user experience, not a feature that can already be accomplished with the tools in core (trigger + actions). Linux adoption doesn't occur by wanting Linux, it occurs by people wanting more Ubuntu desktops/laptops, EEEPCs, Red Hat LAMP stacks, and Google Searches.

I’ve made a patch that adds comment notification in comment module.
A part of it is based on comment_notify module.

Main things:
1. There is a new table {comments_subscribe}. It holds subscriptions per an user and a node.
2. Admin can enable/disable comment notifications at all and edit the email’s template.
3. It allows to immediate subscribe/unsubscribe to comments for authenticated users by clicking on a special node’s link (used hook_link()). No need to post 'Subscribe...' or something like.
4. Added new permission: 'subscribe to comments'
5. Added a fieldset ('Subscribe to comments') with a checkbox ('Recieve notifications for all comments on this post') in the comment_form. A checkbox reflects a subscription status of the authenticated user for the current node. If an anonymous provided an email before then the checkbox can reflect his status too.
6. A notify email body includes a link with a hash value to quick unsubscribe from comments of the node. It is convenient for anonymous users that cannot properly handle their subscriptions from the site.
7. Added comment_install() and comment_uninstall() function to add a new table to the module's schema.

Reroll with small typo.

What's the status on this one? Are we still going strong on adding comment_notify to core? Or have I missed out on stuff?

I did a visual review of the patch:

Why are you changing comment_admin_overview() signature from comment_admin_overview($type = 'new', $arg) to comment_admin_overview($type = 'new', $arg = NULL)?

variable_get('comment_subscribe_enabled', 0) should be variable_get('comment_subscribe_enabled', FALSE), there are several calls of this kind.

I think you should re-add the "settings" local task for comments, and add subscription settings there instead of a new local task called "subscribe".

Available tokens list doesn't generate valid xhtml, it's missing </li>'s, also I should tokens need more descriptive names, e.g. !comment-text instead of !commtext. This will eliminate the need for descriptions, so they can be listed inline.

Please don't remove new lines from the end of module's files.

drupal_install_schema('comment') is already called in system.install, so you don't need to introduce hook_install() and hook_uninstall() to comment.install.

The default email text shouldn't be a constant, instead, it should follow how core handles other kind of emails, see user_mail() and _user_mail_text().

Need a better default email text.

'subscribe to comments' permission needs better 'description' than a copy of 'title'.

SQL query in comment_subscribe_mail() needs to be converted to new DB layer methods.

comment_subscribe_hash() is not necessary, since this is called only once, you can user drupal_get_token() directly.

There are some typos: 'comment_subscribtion_enable' in comment_subscribe_access() and 'Recieve notifications for all comments on this post' in comment_validate().

return $access && ($subscribe?user_is_logged_in():TRUE); code style issue here.

I will be spending a bit of (Acquia's) time working on this patch at some point during the next week. The goal is to hopefully get it ready to be committed soon, although if someone wants to beat me to it that's fine too :)

Reviewing this issue and the patch it looks like a pretty good start, but I had some thoughts I wanted to mention. Please note that I, personally, do not think that the arguments for putting comment notification into Drupal core are particularly compelling. However, if it's going to go in, I guess it will be better if a skeptic works on the patch....

I do think that a minimum bar for this kind of patch going into core should be that the patch provides a lightweight interface that other modules can build off of, rather than one they have to push out of way. Andrew Fountain's comment on Dries's blog says it best:

My problem with putting comment notifications in core is that it is a specific solution to a general problem (notifications) and its status of being in core will hinder the development of a general notifications framework for Drupal.

So the goal here should be to not have that happen. Specific comments follow:

1. I think this should be a separate module (as @mfer suggested in the original post), not part of comment.module. If this is part of comment module, then anybody who wants to use a contrib module which provides different (better, more powerful, more site-specific) functionality to do the same thing is still going to have remnants of the core feature they can't get rid of -- for example, a "subscribe to comments" permission on the Permission page which they have to remember not to use, etc. I think that is bad for usability. Fortunately, I don't think it would be that difficult to modify Vladimir's patch to turn it into a separate module.

2. I noticed that even though the notifications are on the node level rather than the comment level (i.e., you can subscribe to receive notification for all comments on a node or none of them, nothing in between), the {comments_subscribe} table in this patch still stores things on a per-comment basis. It seems the reason for this is that the email addresses of anonymous users are stored in the {comment} table on a per-comment basis, and it needs to join against those. This suggests to me that the email addresses should be taken out of the {comment} table altogether and put into their own very simple table that other modules can easily access, with a simple API for doing so (maybe a "visitor" table for non-registered users that is an ultra-stripped down analogue of the "user" table we have for registered users?). It seems to me that this is the interesting framework that this patch could provide -- Drupal would then have a centralized way to store information about site visitors even if those visitors do not have accounts on the site. Make sense?

3. The idea of doing this via actions and triggers makes a lot of sense in theory and originally sounded to me like the way to go, but the problem seems to be that you need a UI change which depends on the trigger-action pair; for example, if the site admin has assigned the "send email" action to one of the "comment" triggers, then (and only then) would we want to form_alter in a specific checkbox with a specific wording that allows users to participate in the action on a per-user basis.... this sounds complicated to me, and doesn't seem exactly like what triggers and actions were intended for (I get the impression they are designed more for things that happen automatically without the end user getting a choice). However, my knowledge of triggers and actions is relatively weak -- does anyone know a good way to do this?

4. Finally, a very nice feature of Vladimir's patch is the way that it allows authenticated users to subscribe without leaving a comment. I believe that if we go with the database design I described in my point #2, it would be easy to open this up to anonymous users as well. (In that case, there should probably be a separate permission that controls access to this feature.)

I do like this feature but I would of thought that RSS comments would be more core worthy.

1. I'm OK with making it a separate module, but it will have the drawback that it requires more configuration work (bigger barrier).

2. I'm not sure how #2 would improve things so let's talk about that some more. Maybe I misunderstood what you are suggesting. In your model, what would tie comments to visitors? Visitors would have to store (comment ID, e-mail address, ...)-tupples. I'm not sure I understand the benefit. It seems to introduce more JOINs, and nothing else ...

3. I think that a triggers and actions scenario sounds nice in theory, but in practice it would be a bit of disaster.

4. I need to better understand #2 before #4 makes sense to me. :)

I've thought a bit about this problem. To me it seems that that Triggers and actions are missing one significant element: There is no ui for configuring the context under which they fire nor to control the specifics of what they do once activated. In the case of comments, It seems kind of ridiculous to send the user a copy of his own comment through email, the same could be said about post. Also in some situations you don't want to get a notification when authenticated users post content, but rather when anonymous users do. However in the current set up there really isn't a way to control those options.

The solution that makes the most sense to me is something like cck for triggers and actions... a Trigger Construction kit and an action construction kit if you will. Nodes types used to only be created programmatically and triggers and actions for the most part still are. What if another module were to assemble triggers based on options chosen through a ui. So in the case of comments we could go into a admin page and build a trigger where we decide how and when we want it to respond to comments. Do we want to send an email to the node author, the admin or both? Do we want anonymous users to have the option of subscribing? Can a person subscribe to a post they haven't commented on? These could all be options that the admin could select while creating a new action or a new trigger with this theoretical tck / ack module. It seems more the drupal way to build pieces that work in tangent with others rather than trying to build a one size fits all solution.

One other thing that I would add that I agree with Robert Douglass that tokens needs to be involved. The current actions module makes use of some tokens, but it's limited. It seems to me that tokens is the established method of dealing with inline string replacements, I can't imagine a good reason for having a core module for comments copy its functionality. I would even go so far as to say that tokens would be better suited for adding to core than a specific comment module.

Dries: The key point I didn't describe clearly is that I'm actually talking about adding two tables, only one of which would have anything to do with comment notification directly.

One table would be in user.module and would tentatively be called {visitor} (not sure that's the best name), and would only contain two columns, a primary key (vid) and email address. Any module that wanted to record an anonymous user's email address could insert into this table via a simple API.

Then, the comment notification module would have a separate {comment_subscribe} table that had columns nid, uid, and vid. Each row of this table would represent a subscription to a node by either a logged-in user (the "uid" column) or anonymous user (the "vid" column) -- this is a little unfortunate since only one of the last two columns is ever meaningful for a given row, but should still hopefully be less ugly than the alternative.

Also, the existing {comment} table would then have its 'mail' column replaced by a 'vid' column that references the {visitor} table. The disadvantage here is that it would add an extra JOIN in some places. However, we might be able to avoid this in performance-critical situations since we do not currently display the email address when showing a comment anyway (and guarantee that in the help text provided to the anonymous user when they enter their address). An individual site that really needed to override this behavior could load the email address in their theme function.

The upshot of all this is that the comment notification module could then easily allow anonymous user to subscribe to comments; the user would type in their address and then a new row would get inserted in the {visitor} table (if not already there) as well as in the {comment_subscribe} table and that would be it. This is the immediate impact; there might be other interesting things contrib modules could do once there is a centralized place to identify non-registered users by their email address.

Hope that all makes sense.... good plan or not? Also, in response to this:

1. I'm OK with making it a separate module, but it will have the drawback that it requires more configuration work (bigger barrier).

Maybe the module should be enabled as part of the default installation profile?

@David_Rothstein
About the visitor field... could you consider using something other than vid? We allready use that for vocabulary vid and revision vid (which is bad enough as it is) If we throw in another vid it's going to be even more confusing when we are making complicated sql queries. What about vis_id or vsid or something of that nature?

I like the idea of a visitor table in its own right - can we break that into it's own issue maybe?

In reply to Dries in #17 - if it's a separate module, we can always enable it in the default profile to save that extra configuration for new users.

@Aaron Hawkins: Rules is a replacement for Trigger; it provides a UI exactly like what you're talking about. Rules has import/export, but I'm not sure if anyone has ever made an importable trigger.

An issue for token already exists: #113614: Add centralized token/placeholder substitution to core

Some quick responses:

I like the idea of a visitor table in its own right - can we break that into it's own issue maybe?

Maybe, but whether or not that goes in might have a big effect on the final form of the code here. Essentially, I am proposing we add it here because this issue is the first time we will have more than one table in core that is interested in knowing an anonymous user's email address ('comment' and 'comment_subscribe').

About the visitor field... could you consider using something other than vid? We allready use that for vocabulary vid and revision vid (which is bad enough as it is)

Agreed. Maybe the table could be called {site_visitor} and the ID could then be svid or something like that?

@David_Rothstein this setup would allow us to convert anonymous posts to user posts if they eventually signed up for an account or do other neat stuff like that.

Agreed, a module could definitely do something like that... very interesting. Thinking down the line, you could eventually use Views to present a list of "anonymous" users who have done certain things on the site -- hm, better be careful, this is sounding a bit like Big Brother :)

the comment notification module would have a separate {comment_subscribe} table that had columns nid, uid, and vid. Each row of this table would represent a subscription to a node by either a logged-in user (the "uid" column) or anonymous user (the "vid" column) -- this is a little unfortunate since only one of the last two columns is ever meaningful for a given row, but should still hopefully be less ugly than the alternative.

Is this alternative less ugly: Columns are nid, eid, and entity with values like ($nid, $uid, 'user') and ($nid,$vsid,'visitor_email'). This would allow anything to subscribe or unsubscribe to a node, like other modules or a drupal.org testbot, for purposes to be defined by the module which defines the entity.

[edit - realized shortly after submit that the previously given examples were nonsense.]

For example, I could write a contrib module to send comment notifications by role, storing ($nid, $role_id, 'role') without having to add another table to the database.

(Trivial aside: is the term guest and the label 'gid' better than visitor/vid ? Or else aid for anonymous ? )

Now that Dries has announced code freeze on Sept 1st, its high time we got some of our favorite issues solved :-)
I've been doing some thinking on this one. Let's first collect what all we want to accomplish:

- allow anonymous and registered users to register for comment notifications on a particular node.
- allow both types of users to unsubscribe from the node as well.
- provide a permission for both types of users, like "allow subscribing to comment notifications".
- provide a permission for both types of users like "allow subscribing to all comment threads on all nodes". This one's very advantageous if I run a blog and want to get emails for every comment that reaches the approval queue. Not too sure how we'll do this, or how big a performance hit it will be.
- show a list of all subscribed threads on the user profile page and allow one-click unsubscribe?

In my opinion, we should integrate this with Comment. There is no need of a separate module just for this functionality. Imagine how weird it will look on admin/build/modules :-)

Did I miss anything in the list above? Also, what's up with the people who believed this should be separate, something like a generic notifications module?

Sorry, I owe an explanation for what happened here. Basically, I had a day budgeted at work to write this patch, but that did not really turn out to be enough to make much progress. I do have some code written, though - so I will dedicate some time to getting the code into a shape where it makes sense to post it (e.g., with appropriate TODOs in various places), and that way someone else who is more passionate about it can try to bring the issue home. I will post the code sometime within the next week. I apologize for holding this issue up for so long.

However..... I really really really really really think it's a bad idea to make this part of comment.module. If it starts going in that direction, I'm going to kick and scream and recruit other people who feel the same way to kick and scream as well :) It's already been discussed above that we can enable this module in the default install profile, so that a new user installing Drupal is not really going to notice the difference anyway. However, the difference is this: If/when they decide they want to replace this with a different notification mechanism (e.g., a contrib module), if the functionality is part of comment.module they have no clean way to replace it.

Also, I don't think I understand why this would look bad on admin/build/modules? Basically, in keeping with other Drupal conventions, I am thinking we will call it the "Subscribe" module and give it something like the following description on the module page: "Provides a lightweight mechanism for site visitors to receive e-mail notifications about new comments." I don't think that will look awkward at all.

Also, in response to @matt2000's suggestion for the database structure in #27, I think that makes a lot of sense and will/am going with that in the initial patch. I think "entity" is a little too abstract of a term though (and it also now conflicts within the terminology used by the Field API), so I'm naming the columns "subscriber_id" and "subscriber_type" instead.

I also agree that "guest" is probably a better term than "visitor".

Should we include the option to be notified of changes to the node itself, in addition to comments? (e..g, for a wiki). Or is this better suited to a contrib module? Or a separate issue?

I'd like to be able to receive changes to a node. Infact I'd like to see a notifications API in core and not just comment subscribe but this is probably a bit to ambitious.

Well, now that's called scope creep :-)
Let's focus on what we want, a notifications API? or just comment notifications? Let's not forget that code freeze is on September 1st.

While vladimir's patch shows promise I think we should use triggers instead of directly tying into comments. Then we can get notifications of a lot more thing for (almost) free.

I want to present a complete different perspective. Why doesn't email notifications use Views? You can make a specific email notification based on the results you want. The UI in views is amazing and the project is great. Add to this the power of the Messaging Module, and then you got complete control over display of both nodes and summary results. Of course if you want this in core, you would have to add CCK and Views into core (which is probably NOT a bad thing). The issue then becomes simple user interface to help with subscription management on a user per user base. You can also add triggers and tokens to your hearts content. This is a complete solution package, but one that would probably provide built in ease of use and the power of all of the above features requested.

I know scope creep will be a response to this, but really, node notifications, group notifications and other types of content has to be included into any notification system. How can we simply limit this to comments and not build a system that allows for the hooks and over-rides needed to meet the high demands the drupal community users require in flexibility, customization, ability to hook in with other applications.

Flexibilty is important. I might want to send 1 email notification per thread and no more until a user visits that thread, while others may want to send a email notification once until the user visits that thread, and then they will continue to get additional threads. In addition template and themes of email notifications must also be a part of the system.

I think what I am saying is that this needs to remain a module and not put into core. Instead it would be better to allow core to handle a email notification module.

Just an Idea.
Mike

You mean like Notify by Views module? :-)

My one concern with this approach for Core is that Views seems to have a not-insignificant amount of overhead.

A hook-able query generator specific to comment notifications, like this, would be much more efficient:

http://groups.drupal.org/node/15928#comment-72395

Granted that it lacks the UI benefits you cite, which I agree, is the primary attraction of Views. But core seems more interested in performances than nice UI.

matt2000;

I think you hit it on the head. Isn't the idea of core is to provide the ability to add modules, and not the modules themselves? I was essentially pointing the community to the idea that a hookable system is better then a one size fits all solution. Either that, or take the dive and make a solution so flexible it can fit everyone's needs! (AKA CCK, Views, Messaging, Templates, Hooks, etc)

BTW, we got stories, pages, and books... yet I use none of these! I build my own content types based around the content requirements.

There is currently allot of ways to provide messaging to the users, yet even the developers, or the module users can not agree on a single solution. I love Notifications, its easy to use and works great. My biggest problem is that I can not easily switch to Subscriptions.

I could want to notify my users of added nodes by my Buddys or Relationships (based on the Relationship API). Multiply uses this method and its a great solution. Also corporate cms users might want this solution (allow Accountants to get updates of all related posts for the day).

I could want to notify users of upcoming Birthdays (using the Birthday Module), or of upcoming events (using Civilcrm or Events).

Or I could want a user to be notified using my customized content type built with CCK and displayed using its own template!

Lastly, what if I dont want to use the comment system at all, and want to use nodes as comments instead?

These are just some of the ideas that could flood this discussion, and thus make any quick solution incomplete.

Core still needs to include some baseline functionality, and I think comment notification is a reasonable one to include.... but those functionalities must be implemented in a way that contrib modules can easily modify and extend them.

Your primary concern is address by including both the 'lite' and the 'standard' installation profiles in Core.

Fundamental Question about Drupal Core?

Is Drupal Core like the "Kernel" of Linux, where it is built as the baseline for all other features, or is Drupal Core more then that? How far do we want to bring the scope out from beign the basics to a more advanced system? Should we instead have a commitment on quality modules that actually mature past Dev stages, before the newest Drupal comes out and they are abandoned to be added to the latest greatest? Should we have a commitment to Distro's, maybe allowing Dries company beign the forefront of this market place? Or should we start including the "basic" modules into core?

I can think of some Modules that could be included in Core. Most of them are on drupalmodules.com top downloaded list.

I acknowledge I do not have the say or knowledge to make these decisions, but isn't this the question we should be asking?

Where is Drupal heading? What is are market distinction? What is our "Business" Plan?

I am opposed to a comment system with-in Drupal Core itself. I think we need the people of the different Messaging frameworks to come together and build a fundimental API for messaging, for subscriptions, and then unify the multiple systems into one. I love allot of these modules. I have used all of the 5.x compatible ones (well most of them), and find none does it all. I want a complete solution, a solution on the Caliber of Panels, Views, CCK, and others. I want to see the framework put into core, so that all modules may use the same functionality allowing OG Groups, Relationships, Buddylist, Birthdays, and every module that sends an email to a user to use a unified system.

I know I dont have the answers, but what I do see is that their is no common system that can control all modules. This can litter the profile's of users with fields (example, install birthdays and user comments and you will get Many fields on the profile page that all deal with the individual notification settings.) Also realize that there is very little ability to provide for a singular header and footer to include in all emails going out.

Notifications are a potential area of core drupal that are in need of attention. I WISH so much that I can dive into development rather then having to spend so much time upgrading my site.

The one thing I can provide to the discussion is the fact that I am a module user to the extreme. I have tried just about every d5 module, and since I am just moving to d6, I think I will be on a similar path there... Soon!

D8.

Marked #146291: New module for core - replies as a duplicate of this issue, even though it was here way before this. However, everyone's been discussing this one more. So boo to mfer for not searching for duplicate issues first! :)

Sigh. I don't remember what the tags were on this before a f**king spammer screwed it up, but these are definitely wrong.

Oh. :) That was them.

Many in this discussion would like to see this depend on Trigger. But Trigger might fall out of core soon (#1255674: [meta] Make core maintainable). IMO, this might make this feature request a "won't fix" where we instead continue to rely on contrib for this (i.e. Comment notify).

But since it's a "Favorite of Dries" and marked as "Killer End-User Features" by webchick, I don't have the balls to close this one ;)

Trigger is indeed gone from core so moving to Won't Fix but if others disagree, feel free to reopen with new summary.

With this being explored #1806514: Unify anonymous and registered users I think we could start to reopen the discussion on this. It seems especially apt given David_Rothstein's comments in #15 and #20

Nice feature but comments now more complicated so better to implement a contrib module first