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).

#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


jrao’s picture

Status: Active » Needs review
1.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.

Berdir’s picture

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,
+      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 :)

jrao’s picture

Status: Needs work » Needs review
1.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"

ptmkenny’s picture

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