E-mails not sent if message was sent to role members

Awesome, marked #931454: problem with sending mail notifications to roles as a duplicate since your issue comes with a patch :)

First things first, when you provide a patch, please set the status to "needs review". Then the patch is automatically tested by the testbot. We don't have tests for this just yet obviously but it at least tells me if the patch applies cleanly and doesn't break anything else.

Looking at your patch now. This might look like a long list but it's mostly minor stuff, like coding style issues, so don't worry :)

+++ pm_email_notify/pm_email_notify.module	9 Nov 2010 19:09:10 -0000
@@ -62,6 +62,50 @@ function pm_email_notify_privatemsg_mess
+ * This adds support of notifying users, added to message as recipients after message was sent.
+ * That technique is used in privatemsg_roles, so this adds support of it.

Although correct, the wording of it can be improved a bit. Also, try to stay below 80 characters per line (just for comments, not code).

"This adds support for notifying recipients added after the message was sent. This is used to send messages to non-user recipient types, e.g. roles."

+++ pm_email_notify/pm_email_notify.module	9 Nov 2010 19:09:10 -0000
@@ -62,6 +62,50 @@ function pm_email_notify_privatemsg_mess
+  if (!$add)
+    return;

Always use curly braces ({}), even for single line conditions.

+++ pm_email_notify/pm_email_notify.module	9 Nov 2010 19:09:10 -0000
@@ -62,6 +62,50 @@ function pm_email_notify_privatemsg_mess
+  $thread_id = db_result(db_query('SELECT thread_id FROM {pm_index} WHERE mid = %d', $mid));
+  // Only add the recipient if he does not block the author.
+  $author_uid = db_result(db_query('SELECT author FROM {pm_message} WHERE mid = %d', $mid));
+  $recipient = privatemsg_user_load($uid);
+  $user_blocked = module_invoke_all('privatemsg_block_message', privatemsg_user_load($author_uid), array(privatemsg_recipient_key($recipient) => $recipient));
+  if (count($user_blocked) <> 0) {
+    return;
+  }
+
+  // Make sure to only add a recipient once. The types user and hidden are
+  // considered equal here.
+  if ($type == 'user' || $type == 'hidden') {
+    $exists = db_result(db_query("SELECT 1 FROM {pm_index} WHERE type IN ('user', 'hidden') AND recipient = %d AND mid = %d", $uid, $mid));
+  }
+  else {
+    $exists = db_result(db_query("SELECT 1 FROM {pm_index} WHERE type = '%s' AND recipient = %d AND mid = %d", $type, $uid, $mid));
+  }
+
+  if (!$exists)
+    return;

This part shouldn't be necessary at all. This is already checked in privatemsg.module and the hook is never executed if the recipient is blocked.

The only necessary thing is probably the line where $recipient is loaded.

+++ pm_email_notify/pm_email_notify.module	9 Nov 2010 19:09:10 -0000
@@ -62,6 +62,50 @@ function pm_email_notify_privatemsg_mess
+  // check if recipient enabled email notifications
+  if (!(isset($recipient->uid) && _pm_email_notify_is_enabled($recipient->uid)))
+    return;

Always start a comment with an upper case character and finish with a point. Also, curly braces..

+++ pm_email_notify/pm_email_notify.module	9 Nov 2010 19:09:10 -0000
@@ -62,6 +62,50 @@ function pm_email_notify_privatemsg_mess
+  $message = privatemsg_message_load($mid);

Nothing wrong with your code here, this is necessary. The problem is that privatemsg_message_load() currently has no static cache (Not in D6, D7 uses entity loading and that has built in static caching). Since this hook can be called very often (hundreds of times during batch processing), this places quite a load on the database. But this is nothing that needs to be changed here.

Powered by Dreditor.

Don't worry, I have commited only very few patches without at least a single re-roll (= updating a patch based on review feedback).

Also, I think this feature is important and I will commit once I think the patch is ready. privatemsg_message_load() doesn't have a static cache right now because there was no reason to add one. If this patch gives us a reason to do so, then that's fine with me.

I didn't know if it was better to revive this thread or make a new one, but I have a patch that fixes the OP's problem. I have tested it with roles and it is also working with my custom og recipients.

One note, if you have a lot of users on you site , sending a message to 'authenticated' would cause a horrendous amount of drupal_mail() calls. This could be alleviated by using the messaging framework with it's cron queing, but I haven't delved into that yet.

Sorry, patch was rolled at the wrong folder level. Here's another try.

Ok this has got to stop :)

I'm going to start using simpletest locally to catch these notices. New patch incoming.

Fixed notice (hopefully)

My suggestion is to use the development branch (eg cvs or git checkout) when developing. Then, E_NOTICE is enabled and you'll see them always.

That usually leads to better code :)

The problem with your current patch is that it's doing what we are trying to avoid with Privatemsg. Load all members of a role at once. If there are more than 100, they are processed with batch API after the message has been sent. However, hook_privatemsg_message_insert() is not called then.

But we have added another hook to solve that problem, and that hook has been used in the first patch already.

See http://blog.worldempire.ch/api/privatemsg/privatemsg.api.php/function/ho..., that should only require a few lines of code.

I see. I'll have to dive in and see where that logic is invoked. And check out the original patch better.

Here is a stab at something much closer to the OP.

Bare with me if there are any weird issues with the patch. Managing all these changes forced my hand and I am officially trying to learn Git :)

Straight from my local commit notes:

Added static caching to privatemsg_message_load and ..._load_multiple
Added a help function for sending an email notification to a recipient
Implemented hook_privatemsg_message_recipient_changed() to send emails to recipients that are added.
Changed hook_privatemsg_message_recipient_changed argument from'$recipient' to '$recipient_id' for clarity.

I'll remember some day...

Okay, apprently #14 was a reverse patch of what I really want :) Let me try again....

This should be better...

+++ pm_email_notify/pm_email_notify.module
@@ -51,17 +51,39 @@ function _pm_email_notify_is_enabled($uid) {
+ * Notifies users who were added to a message about new Private Messages
+ * via Email.

Since here we're talking about actual messages and not the module name, this should imho use lower-case characters. also "E-Mail". So: "about new private messages via E-mail."

+++ pm_email_notify/pm_email_notify.module
@@ -51,17 +51,39 @@ function _pm_email_notify_is_enabled($uid) {
+    if (($message = privatemsg_message_load($mid)) && ($recipient = privatemsg_user_load($recipient_id))) {
+      pm_email_notify_send_mail($recipient, $message);

Unfortunate :)

The idea of this hook was to save memory and not load all messages and recipients because hundreds or thousands might be processed during batch sending to roles.

We probably have to live with this, I'm just wondering if we can somehow check if we need to send a message before we actually send it. That might save a few queries, especially if notifications are disabled by default or something like that. Maybe split your new helper function into two separate functions? One for the checks and a second for the actual sending. Edit: Or just keep the check outside of the function, it's just used twice, is not that complicated and will most likely not be used in other places.

+++ pm_email_notify/pm_email_notify.module
@@ -51,17 +51,39 @@ function _pm_email_notify_is_enabled($uid) {
+  // check if recipient enabled email notifications
+  if (isset($recipient->uid) && _pm_email_notify_is_enabled($recipient->uid) && !empty($recipient->mail)) {
+    // send them a new pm notification email if they did

Minor: code comments should start with an upper case character and end with a period.

+++ privatemsg.module
@@ -1867,30 +1869,60 @@ function privatemsg_message_load($pmid, $account = NULL) {
+      $messages[] = $cache[$uid][$pmid];

Instead of just adding, can you use $pmid as key here? (same below). I'm not sure right now but we probably rely on that behavior somewhere.

Now we have actual test fails for a change. Possibly because of the static cache, we might need to reset that somewhere.

Another thought: I'm wondering if we should make it configurable if notifications should be sent for cases like this. Some sites might want to disable this when they are sending a privat messages to tens of thousands of users :)

Powered by Dreditor.

+++ pm_email_notify/pm_email_notify.module
@@ -51,17 +51,39 @@ function _pm_email_notify_is_enabled($uid) {
+    if (($message = privatemsg_message_load($mid)) && ($recipient = privatemsg_user_load($recipient_id))) {
+      pm_email_notify_send_mail($recipient, $message);

Unfortunate :)

One thing we could do is allow my 'send' helper function to accept objects or ids. And then it can load the recipient first, check if they want emails, then load the message second.

The second option would be to change the logic up a bit drastically, and allow the pm_email_notify module to collect up recipients and messages and only send the emails at the end of the batch process, after it has done a 'multiple' load on the messages and perhaps even a multiple load on the recipients. It would probably have to send the messages via the batch (somehow adding on more batch operations) and I think there is huge potential for memory overload if we are talking about thousands of recipients being stored after a multiple load.

In summary, I think you are right that unfortunately this is necessary unless we do more of an overhaul. I'm not sure how important it is to pass a fully loaded user object to drupal_mail? If it's not we could just load what we need (mail, name)?

Now we have actual test fails for a change. Possibly because of the static cache, we might need to reset that somewhere.

Yep, I'll see if I can track it down.

Another thought: I'm wondering if we should make it configurable if notifications should be sent for cases like this. Some sites might want to disable this when they are sending a private messages to tens of thousands of users :)

Yes, you are absolutely right.

Another thought: I'm wondering if we should make it configurable if notifications should be sent for cases like this. Some sites might want to disable this when they are sending a private messages to tens of thousands of users :)

Thinking more, the disable should probably be per recipient type. For roles, I might want it off, but for groups, or whatever, I might be fine with it.

drupal_mail() itself could not care less if there is a account object or not :) This is just passed to our own hook_mail() implementation where we need it to build the replacement tokens, see http://api.worldempire.ch/api/privatemsg/pm_email_notify--pm_email_notif... (yay, new api documentation site :))

I think trying to check before loading the recipient & message is the best we can do for now. Note that it is very unlikely that there is more than a single message processed during the batch, because a batch is triggered directly after a message was sent. It could happen during cron though, but cron handling only kicks in if batch didn't work for some reason (e.g, the message was sent through the API). Anyway, there shouldn't be more than a 2-3 messages or so in that case either. So with your added static cache, this isn't a problem.

PS: Awesome to see active developers in the issue queue, welcome to the Privatemsg team :) FYI, #drupal-games is our inofficial IRC channel, I and BenK are often there to discuss issues and so on. Feel free to join if you have questions or need help with those tests.

I think trying to check before loading the recipient & message is the best we can do for now. Note that it is very unlikely that there is more than a single message processed during the batch, because a batch is triggered directly after a message was sent. It could happen during cron though, but cron handling only kicks in if batch didn't work for some reason (e.g, the message was sent through the API). Anyway, there shouldn't be more than a 2-3 messages or so in that case either. So with your added static cache, this isn't a problem.

And on top of that, the static cache won't persist through batch process http requests anyhow :)

Happy to be contributing. We are so super busy that I rarely have time to craft proper patches, but this module is so huge that I would be in trouble if I tried to maintain our own fork like I do with a few other smaller modules :)

I've got a patch for this but I can't post it till later.

Alright new patch with new admin and user settings.

Here's a brief summary:

• Updated schema of 'pm_email_notify' table.
  • 'pm_email_notify_default' becomes 'pm_email_notify_default_level'
  • added 'pm_email_notify_only_user'
• E-mail notifications now determined by ther users 'level' setting.
  • PM_EMAIL_NOTIFY_LEVEL_DISABLED: No e-mails.
  • PM_EMAIL_NOTIFY_LEVEL_THREAD: Only email for first message in a new thread.
  • PM_EMAIL_NOTIFY_LEVEL_ACTIVE: Emails for first message in a thread, and all messages after the user replies to the thread.
  • PM_EMAIL_NOTIFY_LEVEL_ALL: Emails for every message to the user.
• Added both a site-wide and user setting for 'pm_email_notify_only_user'
  • The site wide setting overrides the user's setting.
  • Causes emails to only go out to 'user' type recipients (if you don't want emails for roles, for example)
• Added new utility functions for determining if a notification should go out.  These functions use static caching and return an quickly as possible to reduce overhead.
• All the stuff from the previous patch
  • Static caching in privatemsg_message_load()
  • Implementation of hook_privatemsg_message_recipient_changed to send out email for non-user recipients.

Note: the above patch will probably fail for the same reasons the first one did (I haven't tracked those down yet). Also, we should re-evaluate all the comment wording and whatnot, since it has changed a lot since my last patch.

Lets get a consensus on my settings changes and the wording of those, then we can work on making it pass all tests.

Tests pass, nice :)

Looks very nice overall, the usual nitpicking follows below...

+++ pm_email_notify/pm_email_notify.admin.inc
@@ -17,10 +17,24 @@ function pm_email_notify_admin_settings_form() {
-  $form['pm_email']['pm_email_notify_default'] = array(
+  $form['pm_email']['pm_email_notify_default_level'] = array(
+    '#type' => 'radios',
+    '#title' => t('Send users e-mail notifications'),
+    '#options' => array(
+      PM_EMAIL_NOTIFY_LEVEL_DISABLED => t('Never.'),
+      PM_EMAIL_NOTIFY_LEVEL_THREAD => t('Only for new threads.'),
+      PM_EMAIL_NOTIFY_LEVEL_ACTIVE => t('Only for new threads and threads they are active in.'),
+      PM_EMAIL_NOTIFY_LEVEL_ALL => t('For every message they receive.'),
+    ),
+    '#default_value' => variable_get('pm_email_notify_default_level', PM_EMAIL_NOTIFY_LEVEL_ALL),
+    '#description' => t('Default notification level.  Users may override.'),
+    '#weight' => 0,
+  );
+  $form['pm_email']['pm_email_notify_only_user'] = array(
     '#type' => 'checkbox',
-    '#title' => t('Notify users of new private messages by default'),
-    '#default_value' => variable_get('pm_email_notify_default', TRUE),
+    '#title' => t('Only send e-mail notifications to \'user\' recipients.'),
+    '#default_value' => variable_get('pm_email_notify_only_user', FALSE),
+    '#description' => t('Users may NOT override if this is checked.'),
     '#weight' => 0,
   );

The wording can be improved a bit. For example, I don't think the title for pm_email_nofity_only_user is easy to understand. Also, it is afaik discouraged to use \' in translation strings because it can get messed up in .po files. Try mixing " and ' instead.

I'll point BenK to this issue, he's good at defining understandable titles and descriptions.

+++ pm_email_notify/pm_email_notify.module
@@ -7,6 +7,24 @@
 /**
+ * Define constants.
+ */
+
+// Disable e-mail notifications.
+define('PM_EMAIL_NOTIFY_LEVEL_DISABLED', 0);
+
+// Enable e-mail notifications only for new threads.
+define('PM_EMAIL_NOTIFY_LEVEL_THREAD', 4);
+
+// Enable e-mail notifications only for new threads and messages in threads the
+// user authored or replied to.
+define('PM_EMAIL_NOTIFY_LEVEL_ACTIVE', 8);
+
+// Enable e-mail notifications for all messages.
+define('PM_EMAIL_NOTIFY_LEVEL_ALL', 12);

- Not sure why you used 0,4,8,12 :) Care to explain? It's more common to use binary series to be able to use boolean expressions if there is any use for that (Probably not).

- ++ for comments, but they should be in a docblock so that they ar picked up by api.module

- On the other side, I don't think the "Define constants" docblock is necessary, seems obvious to me.

+++ pm_email_notify/pm_email_notify.module
@@ -24,26 +42,121 @@ function pm_email_notify_menu() {
- * Retrieve notification setting of a user.
- *
- * This function retrieves user's pm notification preference from database,
+ * This function retrieves user's pm notification level from database,
  * if user preference doesn't exist - it uses default value instead
+ */

Any reason why you deleted the first line here?

Function docblocks should have a single line short description first and after an empty line, optionally a more detailed description.

Also, @param $uid and @return would be nice to have here.

+++ pm_email_notify/pm_email_notify.module
@@ -24,26 +42,121 @@ function pm_email_notify_menu() {
+ * Check if a user should only receive notifications when they are a 'user'
+ * type recipient.
+ */
+function _pm_email_notify_only_user($uid = NULL) {

Same here, try to make a single line out of that and document the arguments.

+++ pm_email_notify/pm_email_notify.module
@@ -24,26 +42,121 @@ function pm_email_notify_menu() {
+  $only_user = variable_get('pm_email_notify_only_user', FALSE);

I see why you are doing this, but the downside is that it is always called, even when there is already a value in the static cache. Would by a *tiny* bit faster when you just call it directly on either of those two lines but doesn't matter much :)

+++ pm_email_notify/pm_email_notify.module
@@ -24,26 +42,121 @@ function pm_email_notify_menu() {
+  // Cache the result set in case this method is executed in batched operation
+  // which will perform many unnecessary repeated processing.
+  if (!isset($notifications[$uid][$mid])) {

Even in a batch, a uid/mid is most likely only going to occur once in a single script run, so I'm not sure how much a static cache actually helps here.

$threads certainly does.

+++ pm_email_notify/pm_email_notify.module
@@ -24,26 +42,121 @@ function pm_email_notify_menu() {
+        while($recipient = db_result($result)) {
+          $threads[$thread_id]['replied'][$recipient] = $recipient;
+        }

Are you sure that this works? I've never used db_result() in a loop because I thought that's not really supported, but I might be wrong. See http://api.drupal.org/api/drupal/includes--database.mysql.inc/function/d...

+++ pm_email_notify/pm_email_notify.module
@@ -51,17 +164,47 @@ function _pm_email_notify_is_enabled($uid) {
+      if (in_array($type, $types) && ($recipient = privatemsg_user_load($recipient_id))) {

Forget what I said about the static cache above. Because we have that, we can actually call send_check() here too before we call privatemsg_user_load(). That will save us the privatemsg_user_load() call which means several queries and hooks to be executed and the second call to send_check() will hit the static cache.

+++ pm_email_notify/pm_email_notify.module
@@ -148,27 +291,44 @@ function pm_email_notify_user($op, &$edit, &$account, $category = NULL) {
+        // Users can't even see this setting if the default is TRUE.

Not sure why? A separate setting to configure if it is overridable sounds more useful.

PS1: The more complex pm_email_notify gets, the more important it becomes that we have some testing for this :)

PS2: I guess another useful setting would be that users only recieve a singe notification and the next only after they've read the existing messages. Not sure how complicated that would be, we can certainly do that later. It might even be not that complicated, should be enough if we just check if there are unread messages for that user in the thread, assume that a message was sent and don't send one.

PS3: One other often requested feature is digest sending, only a single message per day if there were new messages. But I'm not sure if we want to actually do this, since that would be a lot more complex than what we have now. Maybe in D7 at some point when we can rely on core's queue system and don't need to rely on a third party module or implement something ourselves.

Powered by Dreditor.

discouraged to use \' in translation strings because it can get messed up in .po files. Try mixing " and ' instead.

Ahh.. my mistake.

Not sure why you used 0,4,8,12 :) Care to explain?

To make room for more levels, while being able to have a 'hierarchy' if we need to do >, <, <= type queries. The numbers themselves are meaningless.

Any reason why you deleted the first line here?

"notifications preference" > "notifications level"

Even in a batch, a uid/mid is most likely only going to occur once in a single script run, so I'm not sure how much a static cache actually helps here.

So .. is it worth it for edge cases .. or do we just simplify the code and not cache it?

Are you sure that this works? I've never used db_result() in a loop

I do it all the time .. but that doesn't make it right or standard :)

Forget what I said about the static cache above. Because we have that ...

Ahh, yes, ... since it only needs uid. That will help a lot.

Not sure why? A separate setting to configure if it is overridable sounds more useful.

I can see that .. I guess I figured you would only stop non-user message e-mails for performance reasons .. but I guess someone could want the default to limit emails, but users may want more.

PS2: I guess another useful setting would be that users only recieve a singe notification and the next only after they've read the existing messages. Not sure how complicated that would be, we can certainly do that later. It might even be not that complicated, should be enough if we just check if there are unread messages for that user in the thread, assume that a message was sent and don't send one.

If I am understanding you correctly that may be a better approach for the 'active' level. Send an e-mail for the thread ... then just email them once when there are messages they haven't read. The whole point isn't to spam after all, its to get them to come back to their inbox.

PS3: One other often requested feature is digest sending, only a single message per day ...

That is definitely on my horizon.. but as you say there is no reason to add it here. Better to support integration with an existing digest module.

Subscribing

+1;
I sent PM to 3000+ user, then noticed the email notif function didn't work in this case.
I'm waiting for the good patch, too.

subscribe, I was planning to send the message to 12,000 users tomorrow, but I found this issue today so I need wait for some working patch.

btw, is there any confirmed limit of role reciprients from which mails are not sending?
e.g. role with to 2000 users is it ok and emails are sending and role with more then 2000 users emails are not sending?
thanks

Not sure if you are understanding the issue correctly...

E-Mail notifications for role recipients are not sent at all. The patch above should fix that, what we are working on is to improve some newly added settings (especially the wording) and some performance improvements. Testing of the patch with a large amount of notifications would be very welcome, no idea how much testing te-brian has done already, but I haven't at all.

Could be done in a testing environment for example, where email are only logged and and not sent. So that you could confirm that the notifications have been sent correctly for all recipients.

Sending private messages to roles works just fine, there are now technical limits, I've tested it myself with over 100k users.

Great new features! As requested by Berdir, I took a look at the UI and strings added as part of this patch from a usability perspective. Here are my suggestions:

A. On the global settings page, change the first setting to read: 'Only send e-mail notifications to individual users who are entered directly'

Change the description to read: 'If checked, e-mail notifications will only be sent to individual users who are entered directly in the "To" field. (A message recipient who is part of a role, group, or other list entered in the "To" field will not receive an e-mail notification.) If enabled, this option cannot be overridden by individual users.'

B. On the global settings page, change the default notification level settings to read:

Default e-mail notification level:

* Never send a notification
* Only send a notification when a new discussion thread is created
* Only send a notification when a new discussion thread is created or the user has previously participated in the discussion thread
* Always send a notification

Note that we don't need periods at the end of these settings. And change the description to read: 'Choose when e-mail notifications will be sent by the system. Users with the appropriate permission may override this setting on the account edit page.'

C. Change the text that reads: 'Customize the email messages sent to users upon receipt of a new private message.' To this: 'Customize the e-mail notification sent to users' Also, this should probably be its own collapsible fieldset in D6. In the eventual D7 version, we can use a vertical tab. Also, add a line break between the variables help text and the translation help text.

D. Add a new text field setting for the "Sender's display name". This would be handy to have since it's what most people see first in their e-mail. If possible, it would be great if the message author's name (username/realname) was available as a token so that the message author could be listed as the sender (even though the e-mail address would probably be from the site).

E. Change the existing labels as follows:

'From e-mail address for notifications:' --> 'Sender's e-mail address'
'Subject of notification messages:' --> 'Subject'
'Body of notification messages:' --> 'Body'

F. I'm hoping we can create two new permissions:

a) Permission to override default notification level: 'Set own e-mail notification level'
Description: 'Users with this permission may override the default e-mail notification level.'

b) Permission to toggle mass message notifications: 'Set own mass message notifications"
"Users with this permission may choose whether or not they will receive notifications for mass messages."

These permission are important in my own use case because we want users to be able to set their own notification level, but we don't want them to solely opt out of mass messages (which are always important site notification messages). This setup isn't currently possible in the current patch.

G. On the account edit page, create a new setting called 'Enable e-mail notifications for private messages'. The description would read: 'If checked, we will e-mail you when you receive a new message.' This setting would be visible even if the user did not have the two new permissions specified in F.

H. On the account edit page, change the user notification level options to read as follows:

Send me an e-mail notification...

* Only when I receive a message on a new subject
* When I receive a message on a new subject or a subject that I have previously commented on
* Every time I receive a message

All of these changes, I think, will help make things a bit more clear. I really like the direction this patch has gone and can do a review/test once we have a new version.

Cheers,
Ben

Thanks for the thorough review Ben!

I pretty much agree with your improvements across the board.

Re F.
I think we want to have two admin options there .. the default mass email value .. and another for if users can override it or not.
Also, I think there should probably be a way to force a particular message to be e-mailed, regardless of users' settings, for critical site messages (such as a security problem, a major change to some functionality, etc.). So this new setting would require a permission as well, and would appear on the new message form as a checkbox. ("Send e-mail notification to ALL recipients", or something)

I am slowly catching up on my workload .. so if Berdir doesn't beat me to it, I'll try and get a new patch ready sometime this week.

@te-brian:

I definitely like your suggestion about the default mass e-mail value on the global settings page. That would be handy. And specifically have an option for users to override or not would be cool, too.

As for forcing a message to be e-mailed, I think the functionality would be very useful, but there may be a different way to go about it. In particular, I'm always hesitant to add a new option on the new message form unless absolutely necessary (because it kind of clutters up the form). But here are two suggestions on how to handle this based that wouldn't require new message form changes:

1. Add an option to User Blocking Rules settings page. The "Block user messages" sub-module already has a nice interface for preventing blocking messages between two roles (admin/config/messaging/privatemsg/block). In addition to the "Disallow blocking author" and "Disallow sending message" actions, all we would need to do is create a "Disallow blocking e-mail notification" action. This could force an e-mail notification to be sent if the message is from an administrator or other role. It could work both on mass messages and individual messages, too. Perhaps we would need a separate setting for each case?

2. Create a "Mass message notification exceptions" setting. In the User Relationships module that Berdir and I also work on, there is some Private Message integration settings (if both modules are installed). Basically, the module allows a config setting that says users can only send private messages between users with confirmed relationships. But there is also a "Role exceptions" override setting (with checkboxes for each role) that allows any checked roles to be exempt from these restrictions (allowing administrative roles to send private messages to all users). So take a look at the User Relationships code and UI. We could basically do the exact same thing from a UI perspective. This would allow mass message notifications to be sent if the message is from a user with a special role that was checked.

I'd be interested to hear from Berdir, too, about which approach he prefers....

--Ben

Don't expect a patch from here, I'm still in exam mode :)

Re #36:

I don't think this should be something that is always on for a given role or something like that. IMHO, it would be very confusing if you disable e-mail notifications and then still receive them all the time.

Also, I think we should try to not kill too many kittens in this issue ;) Let's re-roll and try to get the current features commited asap. Because the main point of this issue is being able to send notifications for role etc. messages, everything else is just nice to have.

Then we have time to discuss the things we're not sure about yet.

#37
Makes sense.. none of these new options are critical, and in fact, go well beyond the intentions of the OP. I'll see about cleaning the language up based on BenK's suggestions and we can get something commit worthy. Then make some follow-up issues for settings and edge cases we want to add support for.

Hi - this is probably a very stupid question: I'm using drupal 7 and have the same issue that email notification is not sent when private message is sent to a user. Does the patch above work in 7 or are we waiting on a different patch to fix the problem in Drupal 7. Thanks

most likely when I actually get some time to finish this... it will be ported to D7 (by the mysterious forces that port my patches to D7 :)

I'm on a business trip and was hoping to have some time.. but .. well you know how things go.

@te-brian: Any more progress on this?

--Ben

@BenK - sadly no .. super swamped this month. Working on keeping a site with 60 web-heads healthy and optimized :) I would try to give a time estimate .. but it would be wrong :(

Ok, started working on this patch...

- Applied most of my clean-up points..

- BenK's points:

A: Changed.

B: Changed, note that I changed how one of those settings works and also updated the text, please review. It's not easy to descripe it in a single sentence.

C: Changed, also added a fieldset and moved around some fields and descriptions. (The text is now a description of the fieldset e.g.). Please review the fieldset title and so on...

D: Separate issue please, this patch is already huge. We might be able to simply tokenify the existing from field and then you could specify "!author ".

E: Changed Subject and Body, I don't like "Sender's ...", doesn't feel right to me. Other suggestions?

F: Added the permissions, note that there is no title/description in D6, so I had to find something that works as the actual identifier. Feel free to suggest something else but I want to have "privatemsg" somewhere in it (to keep it unique) and "mass messaging" is not a term I like nor is it correct (You could set up a random recipient type which would send a message to an arbitrary, single user and this setting would affect it)

G. Implemented slightly different for now. Instead, a user can either see the level radios *or* the checkbox which will then save the site default. See below. Not sure about this actually, makes it very complicated to handle defaults and so on.

TODO/More thinking and testing required:
- Right now, we save the chosen default values even if the user doesn't have the permission to change them. If the defaults change, existing users won't be updated automatically. Not yet sure how to handle that. There are several problems, one is that we now have different permissions for these settings but save them in the same row. So we either need to check permissions at runtime or define special values for when the default should be used (e.g. -1). Ideas?

- There is now a disable setting in privatemsg.module and two settings in this module. disable uses {users}.data, which is crap and this uses a table with custom columns. We might want to consider consolidating these settings into a generic pm_settings table with the following columns (uid, setting_key, setting_value) and then provide a generic api to get and save a setting for a user including default handling and so on. This could allow to unify all that code.

- UNREAD_ONCE isn't actually implemented yet, this is really just a preliminary patch/comment for more feedback in general and on the UI.

- I also want to work on some tests for this, this is getting complicated...

At first glance it looks good.

I noticed a lot of 'he' pronouns in the comments. I'm not sure what the standard is but I think 'they' can be used instead.

Ok, updates..

- Tests. Lots of them. By far not for everything yet, but there are now tests for all 4 notifications levels.
- Fixed the "he"'s..
- Implemented UNREAD_ONCE. This requires a query per check but it is way easier to check than the old setting and doesn't require the $threads static cache.
- I also found a bug in privatemsg_roles, it tries to add uid 0 as a recipient when sending a message to all users (authenticated role). Currently part of this patch too.

Note the patch has been created with git format-patch and consists of a separate patch for the changes in this issue. I think this makes it easier to see the changes.

About going forward:
- Separate issue for the privatemsg_roles patch
- Separate issue for the static cache in privatemsg_message_load_multiple()
- Separate issue for a privatemsg user settings API
- Once these are in, update the patch to address the remaining issues. One thing I need to check is the update function, had a failure in there...

Commited the roles fix and the static cache in separate issues, new patch to reflect that. No other changes.

BTW: OMG, git is so awesome, that was way too easy ;)

After commiting the other changes, I just had to rebase the issue branch on top of 6.x-2.x and make a new patch with git-format.

Commited the roles fix and the static cache in separate issues, new patch to reflect that. No other changes.

BTW: OMG, git is so awesome, that was way too easy ;)

After commiting the other changes, I just had to rebase the issue branch on top of 6.x-2.x and make a new patch with git-format.

#47: email_notification_levels_with_tests2.patch queued for re-testing.

Ok, finally a new patch!

- The patch now depends on the new Privatemsg Settings API.
- Contains a few bugfixes and improvements to that, which I will probably extract in yet another separate issue.
- Now has a PM_EMAIL_NOTIFY_LEVEL_DEFAULT value, which is used for users which can not configure the level but still opt out of notifications. This means that they will always use the current site default. Also, these users are only able to opt out, but not opt in into mail notifications. (unless they have the necessary permission, of course).
- Now comes with 450 lines test code and 545 test assertions to prove that everything is working smoothly. Tested are all possible site default options and all possible user overides according to their permission!

Updated patch that relies on the API for default values and fallback. Should pass the tests again.

Updated patch to remove the hunks that were commited in the privatemsg settings API issue.

These changes are coming along really nice. Great work Berdir. I will try to give it a more thorough look this weekend.

Since there was no (negative) feedback but a lot of tests, I've commited this. We can still improve it later on.

I've been driving myself crazy on this one. I have a 3 year old 6x site that I upgraded to D7 (and consequently toasted). I've been trying to send a message to all (hundreds and hundreds) of users to let them know the new D7 site is up and running. So when i do a mail test, it works great, when i send to roles, nothing happens (expecting an email). So, assuming this explains it, I'd love to try any D7 hack or patch ya got - this is killing me.

Thanks, Ray

Same deal here. I applied the patch and still emails are not received when i send a message to an entire role.

Just want to add that in my 6.x-2.x dev site the emails appear to be going out fine (I have all emails logging to files) and sent a test to my whole auth role (98,000 users).

So it would appear this issue probably only effects 7.x.

Side Note: The batch took well over an hour :)

Well, obviously, this patch has not yet been commited against 7.x-2.x and does not apply there, so I doubt you have applied it ;)

I will work on porting this soon.

Ported and commited to 7.x-2.x.

Bravo - this appears to have worked for me, and considering the overall health of this site, if it worked for me there's a great chance it will work for everyone else.

Thanks for this!!

This was already commited, no need to set it to RTBC ;)

Sorry, my exuberance got the best of me ;) Thanks again for a much needed fix.