Currently on the last line of privatemsg_message_change_recipient, hook_privatemsg_message_recipient_changed is invoked, this happens even if the recipient we're adding already exists, I don't think this is correct. This is causing multiple email notifications to be sent to the same user, if this user belongs to multiple non-user recipients (for example, roles).

Files: 
CommentFileSizeAuthor
#3 privatemsg-1529498-3.patch1.6 KBjrao
PASSED: [[SimpleTest]]: [MySQL] 4,913 pass(es).
[ View ]
#1 privatemsg-1529498.patch1.66 KBjrao
PASSED: [[SimpleTest]]: [MySQL] 3,715 pass(es).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new1.66 KB
PASSED: [[SimpleTest]]: [MySQL] 3,715 pass(es).
[ View ]

Surprisingly hard to get this right, and the tests took forever to run. Let's see if this patch works.

Status:Needs review» Needs work

+++ b/privatemsg.moduleundefined
@@ -2484,6 +2484,7 @@ function privatemsg_recipient_get_type($type) {
function privatemsg_message_change_recipient($mid, $uid, $type = 'user', $add = TRUE) {
+ $author_added = &drupal_static(__FUNCTION__, array());
   // The message is statically cached, so only a single load is necessary.
   $message = privatemsg_message_load($mid);
   $thread_id = $message->thread_id;
@@ -2517,6 +2518,17 @@ function privatemsg_message_change_recipient($mid, $uid, $type = 'user', $add =
@@ -2517,6 +2518,17 @@ function privatemsg_message_change_recipient($mid, $uid, $type = 'user', $add =
           'deleted' => 0,
         ))
         ->execute();
+      module_invoke_all('privatemsg_message_recipient_changed', $mid, $thread_id, $uid, $type, $add);
+    }
+    else if ($uid == $message->author->uid) {
+    // Special handling for author, author already exists in pm_index, we still need to send notification once.
+    if (!isset($author_added[$mid])) {
+    $author_added[$mid] = FALSE;
+    }
+    if (!$author_added[$mid]) {
+    $author_added[$mid] = TRUE;
+    module_invoke_all('privatemsg_message_recipient_changed', $mid, $thread_id, $uid, $type, $add);
+    }
     }

Should use two spaces instead of tabs.

Took me a while to understand that the first if is just setting the default value. Shouldn't it be possible to change that to a empty($author_added[$mid]), which would make the first condition unessary?

I have to say that I'm not sure if I do understand why this is necessary. Why exactly do we need to send the author a notification? There's always the possibility that there is a bug in the tests :)

Status:Needs work» Needs review
StatusFileSize
new1.6 KB
PASSED: [[SimpleTest]]: [MySQL] 4,913 pass(es).
[ View ]

Tab issue and empty($author_added[$mid]) issue fixed.

For why we need to send author a notification, the reason is privatemsg_message_change_recipient is called when we need to add user as message recipient when handling non-user recipients, so if privatemsg_message_change_recipient is adding author, this means one or more non-user recipient included the author, I think in this case sending notification to author is the right thing to do. The test case for this is in pm_email_notify.test, search for "Send a message to all users"

#3: privatemsg-1529498-3.patch queued for re-testing.