privatemsg_message_recipient_changed should not be invoked if recipient to add already exists [#1529498]

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

Comment	File	Size	Author
#3	privatemsg-1529498-3.patch	1.6 KB	jrao
#1	privatemsg-1529498.patch	1.66 KB	jrao

Comments

Comment #1

jrao commented 14 April 2012 at 10:35

Status:

Active

» Needs review

Status	File	Size
new	privatemsg-1529498.patch	1.66 KB

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

Comment #2

berdir

German

Switzerland

commented 25 April 2012 at 21:36

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

Comment #3

jrao commented 26 April 2012 at 10:26

Status:

Needs work

» Needs review

Status	File	Size
new	privatemsg-1529498-3.patch	1.6 KB

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"