Configurable notification emails

That is a whopper. I'll give it a look as soon as I can.

This is good stuff. I wonder why it hasn't gotten much attention.

It will be easier to review if each new feature is in it's own patch. Once this is ready and marked code needs review, I'll take a look.

Hallo,

I really like this module and I hope that the changes you are working on will be available soon.

Until then it would be nice to know how I could do the following:
1. users can only enable/disable notifications in general, and on the users configuration page only the "Master switch" is shown.
2. notification mail should only show
- Greetings [user name],
- Number of new entries

I hope for help with a little note what to disable within the code of notify.module or notify.inc.

Thanks.

p.s. Since the module is not fully translated into German I could provide a more complete .po file. Until now I did the translations that are shown to the site user by using the local module only. I would prefer to translate a complete po file but the original doesn't contain all of the strings to translate. Where could I find a comlete one (I don't get along with these .pot files).

Since flatcap hasn't responded to your request, RobRoy, here's an attempt to give you what you asked for.

I implemented the core part of flatcap's work, i.e., administrative configuration of the e-mail, and re-patched it against the current HEAD, which has changed significantly since flatcap's patches.

I simplified a few parts of the code, fixed a few bugs, and expanded the help page to explain all the variables available. Here are the significant differences between my patch and flatcap's:

1. No user customization. (I'm trying to do this one feature at a time, as requested).
2. Flatcap had two variables, %node_teaser and %node_body, for reporting node content, but their use didn't respect the user's choice of how much content to view. I unified these into the variable %node_content, which will return what the user has asked for (i.e., nothing, if they only want the title, the teaser, or the body)
3. I added the variables %node_count, %comment_count, and %comment_node_count to run the messages 'x new node(s)/comment(s)' through format_plural().
4. I removed the 'headers' line from the fields that can be customized. This is, I know, a debatable decision, but it seemed to be asking for trouble to let people monkey with these fields. It's easy enough to add back if people feel strongly otherwise.
5. The admin help page lists all the variables available.

Been a while now...perhaps RobRoy is looking for something else?

I will take a look at this as soon as I can.

@tain - Perhaps you could apply kthagen's patch and review it? The more reviews the better. :)

I have reviewed this patch against notify.module:2.62. I fixed a few bugs and it now appears to work properly. The patch does not apply against 2.64 (HEAD). I'll update the patch to apply to 2.64 if I'm assured it will be committed when I'm done.

My comments from the review:

In _notify_send(), $substitute is built and used incrementally as each node and comment is processed without being reset between iterations. This means variables from the previous iteration are still set for parts of the next iteration and would give incorrect results if used. For example, $substitute is used on line 512 for notify_node_separator just before seven additional fields are added to it. If notify_node_separator used any of these additional fields, they would be substituted with the value from the previous node.

Additionally, strtr is run multiple times on the same field as $substitute is built (e.g. line 512 and line 523). I would prefer to see it built up and used just once per node/comment/message.

Line 607: I removed a debugging call to drupal_set_message.

Line 443: In _notify_send(), notify_send_last is always set to time()-86400 (1 day) but all callers of_notify_send() also set notify_send_last. This looks to me like leftover debugging code, and I removed it.

Line 540: In comments loop, $substitute is initialized from $node but $node still has its old value from the node loop (the unpatched version uses $nodes[$nid]). I added a line to initialize $node for each iteration through the comment loop.

Calls to format_plural use @count instead of %count. Since this is still a 4.7 module, %count is correct and I changed the code appropriately.

New patch attached.

Hi guys, I just committed a major cleanup to HEAD and DRUPAL-4-7. Could you re-roll this against HEAD?

I've FINALLY got time to get notify up-to-speed. I'm really sorry for lagging behind. Looking at the old code, I'm amazed this module was even working in the slightest (maybe it wasn't). :( I want to get this module cleaned up, then release a 1.0 version for Drupal 4.7. Then, I will start adding features to HEAD which will be ported to D5, and most new features will get backported, possibly to a new 4.7.x-2.x-dev branch for 4.7 if they are major.

http://drupal.org/cvs?commit=48716
http://drupal.org/cvs?commit=48717

OK, it's repatched against the latest HEAD, and I also addressed bjaspan's point about $substitute. It's now reset each iteration, and I've reduced the number of calls to strtr(). I stamped out one other bug along the way too.

I applied this patch against the latest revision of the DRUPAL-4-7 branch. It applies cleanly and works. I reviewed it again (somewhat more briefly than last time) and think it is ready to be committed.

One issue: strtr() is still called multiple times on the same text. If (e.g.) a node body contains %whatever that is defined in $substitute, confusing and incorrect results will occur. However, I don't want to make the perfect the enemy of the good. When this patch is committed, a new issue can be filed to fix this.

I thought about that issue when redoing the patch. As a practical matter, it seems unlikely to have much effect. At one point, I did start to code up a total protection against the condition you describe, but I came to the conclusion that there's no way to avoid doing multiple calls to strtr(), and the more cumbersome possibility that I was contemplating only protects against situations that are going to result in garbage output anyway.

Here's my thinking. Tell me if I'm missing something:

The different variables different scopes, in the sense that they will take on different values at different levels of the loops. Thus something like %node_author is going to be reset for each new bit of content, whereas %node_total has one value for each user.

One possible solution, for the variables in the inner scope would be to rewrite the template for each iteration. That is, set a new entry in $substitute along the lines of

  $replacements["%node_author$i"] = ($nodes[$nid]->name) ? $nodes[$nid]->name : variable_get('anonymous', 'Anonymous');
  $substitute["%node_author"] = "%node_author$i"
  strtr (templates->notify_comment_node, $substitute);

And then hold off doing strtr on $replacements until the very end. But notice that we're still doing strtr() in the same place, so we haven't gotten more efficient. And consider what happens if we hit a variable that isn't yet defined.

Because $substitute is now reset for each user, there won't be any lingering values from a prior user. (That was a legitimate bug with the prior versions.)

That means if a variable appears in a field before it gets set, it will simply be ignored until it gets set, and then it will be replaced on the next call to strtr().

So any variable that is set once per user will be translated correctly no matter where it appears. Consider, for example, a user who wants the node count formatted as "%node_number of %node_total". On the call to strtr(), only %node_number is set, so the result will be "1 of %node_total....2 of %node_total..." etc. After we get out of the foreach loop, %node_total is set, and on the next call, all instances will be correctly replaced.

The only place you will run into problems is if you try to use one of the comment-specific variables in the node template. Those variables will be replaced with the values of the first comment. But then that is already a nonsensical situation. There is no correct output for that case anyway, so all the work maintaining an intermediate substitution array goes for naught.

It might be useful to do some validation of the templates for such situations, but that's why the code in _notify_send() works the way it does.

What I was thinking is that instead of

foreach ($node) {
  $body .= strtr($node, $substitution);
}

we could do something like

$body = array();
foreach ($node) {
  $body[] = strtr($node, $substitution);
}

and then later

  $message = $header . implode($separator, $body) . implode($separator, $comments) . $footer;

Each of $header, $footer, and each element of $body and $comments would only be strtr'ed once.

But I did not think about it carefully so maybe it won't work. Like I said, I don't think it is a patch-stopper.

The only problem with that approach is that there are legitimate instances where we have multiple variables in the template whose values are known at different points (such as the "%node_number of %node_total" example I used earlier). A one-time call to strtr will break in these cases.

I'm just pinging this issue to keep it moving. It had momentum but seems to have stalled. The patch is ready to commit.

I missed it back in the day, but ya... where does this stand? work in drupal 5.x?

Hi,

I tried the latest patch (custmsg.patch.txt in #13) against the latest versions of the module for both drupal 4.7 and 5.x. It doesn't work. It makes a call to "_node_names()" which is never declared. I can't find this function anywhere. Please provide a corrected patch.

Cheers,
Stella

Hi,

I'm very interested by theses possibities. But Notify module seems not te be maintain...

What is the status of this patch for Drupal 4.7 and Drupal 5.0 ?

Cheers.

Hi,

No news for this patch ? Could be a good improvement of notify module.

i am the new maintainer of the module.

I will not consider this patch for inclusion at least until the second list of issues (bugs) is properly dealt with:
http://drupal.org/node/159427

Hi,

Could you include this patch in the new 6.x version ?
It would be great !

Best regards

Maybe I could help to port this to Drupal 7, I have time and very good knowledge of PHP but I have never done a patch in my life.

Creating a patch is very simple, but you need to understand git. This is a good introduction: http://git-scm.com/book

To make a patch and get it committed, you do the following:

1. Use git clone to clone the latest development snapshot to your local disk. This is now your local repository.
2. Work with your local copy to fix whatever you want to fix.
3. Test, test, test. And test. Did I say "test"?
4. When you're done, commit your changes to your local repository.
5. Use the following to create a patch git format-patch -1.
6. Post the patch (as an attachment to a new comment in this particular issue queue). Ask for a review.
7. If it passes review, it will get committed in the next dev snapshot pushed.

Roger that. Thanks one more time gisle!

This feature has now been open for seven years and have not seen any real progress during that time.

Making the notify email messages configurable is not a trivial task. It entails taking the messsage strings out of the module code and storing them as configuration variables.

This also means that by default, the same message will be used for all languages on a multilingual site, which is not what is wanted. It should be possible, on a multilingual site, to provide translations of custom notify email messages.

To get around this, there us the i18n project, which provides a means to translate configuration variables. To support variable translations in Drupal 7, the i18n module uses the Variable module and require translatable variables to be defined using implementation of its hooks.

Making Notify incompatible with a multilingual site is not acceptable, and creating the required integration with the i18n and Variables modules will add bulk and complexity. Doing so is not aligned with the primary goal of the Notify module, which is be light-weight and simple.

The bottom line is that even if someone provided a patch for this, it would probably be too large and complex to fit into the Notify module.

Time to close.