I´ll describe what is happening:

- A thread was started in March among 10 users
- One of them was blocked in Drupal because he´s not part of the program anymore (but we don´t want to delete him)
- The conversation is still active and that user which was blocked is still receiving email alerts for updates in the conversation

This looks like a bug for me. No?

Regards,
Wilton

CommentFileSizeAuthor
#77 privatemsg-do-not-send-messages-to-blocked-users-834706-77-tests-only.patch4.28 KBptmkenny
#77 privatemsg-do-not-send-messages-to-blocked-users-834706-77.patch5.08 KBptmkenny
#74 privatemsg-do-not-send-messages-to-blocked-users-834706-75-tests-only.patch4.28 KBptmkenny
#74 privatemsg-do-not-send-messages-to-blocked-users-834706-75.patch5.08 KBptmkenny
#72 privatemsg-do-not-send-messages-to-blocked-users-834706-71.patch8.97 KBptmkenny
#71 privatemsg-do-not-send-messages-to-blocked-users-834706-71-tests-only.patch7.43 KBptmkenny
#71 privatemsg-do-not-send-messages-to-blocked-users-834706-71.patch8.97 KBptmkenny
#66 privatemsg-do-not-send-messages-to-blocked-users-834706-66-tests-only.patch6.54 KBptmkenny
#66 privatemsg-do-not-send-messages-to-blocked-users-834706-66.patch7.34 KBptmkenny
#61 privatemsg-do-not-send-messages-to-blocked-users-834706-61-no-tests.patch6.19 KBptmkenny
#61 privatemsg-do-not-send-messages-to-blocked-users-834706-61.patch6.99 KBptmkenny
#59 privatemsg-do-not-send-messages-to-blocked-users-834706-59-tests-only.patch6.2 KBptmkenny
#59 privatemsg-do-not-send-messages-to-blocked-users-834706-59.patch7 KBptmkenny
#55 privatemsg-do-not-send-messages-to-blocked-users-834706-55.patch6.99 KBptmkenny
#53 privatemsg-do-not-send-messages-to-blocked-users-834706-53-tests-only.patch6.19 KBptmkenny
#53 privatemsg-do-not-send-messages-to-blocked-users-834706-53.patch7.15 KBptmkenny
#51 privatemsg-do-not-send-messages-to-blocked-users-834706-51.patch6.99 KBptmkenny
#49 privatemsg-do-not-send-messages-to-blocked-users-834706-49.patch6.99 KBptmkenny
#45 privatemsg-do-not-send-messages-to-blocked-users-834706-45.patch6.99 KBptmkenny
#43 privatemsg-do-not-send-messages-to-blocked-users-834706-43.patch7.03 KBptmkenny
#41 privatemsg-do-not-send-messages-to-blocked-users-834706-41.patch6.94 KBptmkenny
#39 privatemsg-do-not-send-messages-to-blocked-users-834706-39-test-only.patch6.08 KBptmkenny
#39 privatemsg-do-not-send-messages-to-blocked-users-834706-39.patch6.88 KBptmkenny
#35 privatemsg-do-not-send-messages-to-blocked-users-834706-35.patch6.93 KBptmkenny
#33 privatemsg-do-not-send-messages-to-blocked-users-834706-33.patch6.89 KBptmkenny
#30 privatemsg-do-not-send-messages-to-blocked-users-834706-30.patch2.99 KBptmkenny
#28 privatemsg-do-not-send-messages-to-blocked-users-834706-28.patch2.99 KBptmkenny
#23 privatemsg-do-not-send-messages-to-blocked-users-834706-23.patch825 bytesptmkenny
#17 privatemsg-do-not-send-messages-to-blocked-users-834706-17.patch653 bytesptmkenny
#15 privatemsg-do-not-send-messages-to-blocked-users-834706-15.patch623 bytesptmkenny
#13 privatemsg-do-not-send-messages-to-blocked-users-834706-13.patch701 bytesptmkenny
#10 privatemsg-do-not-send-messages-to-blocked-users-834706-10.patch751 bytesptmkenny
#8 privatemsg-do-not-send-messages-to-blocked-users-834706-8.patch1.32 KBptmkenny
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir’s picture

Yep, I guess you're right.

However, I'd say we don't want to just block email alerts but the private messages in the first place. This also means that the pm_mail_notify module will never see the blocked recipients and will therefore not send email alerts to them.

wilton.pinheiro’s picture

Great Berdir.

I'll work next days on a patch. Will let you know further updates...

Thanks,
Wilton

Berdir’s picture

Great.

Should be just a few lines added to http://blog.worldempire.ch/api/function/_privatemsg_validate_message/1

After the check if recipients is an array("if (empty($message['recipients']) || !is_array($message['recipients']))"), you can loop over the recipients and remove the recipient + add a warning message similiar to other validation checks.

This will cover both the API functions and message sent through the form. A simple test would be awesome too...

wilton.pinheiro’s picture

Dear Berdir,

I´ve added the following code on privatemsg.module line 1831:

foreach ($message['recipients'] as $key=>$rec){
if(!$rec->status){
unset($message['recipients'][$key]);
drupal_set_message(t("Message not sent to ".$rec->name.": blocked"), 'warning');
}
}

I´ve tested and works fine. Could you please test?

Thanks!

Berdir’s picture

Status: Active » Needs review

The code should look like this (untested):

foreach ($message['recipients'] as $key => $recipient) {
  if (!$recipient->status) {
    unset($message['recipients'][$key]);
    $messages['warning'][] = t('Disallowed to send message to blocked recipient !name.', array('!name' => theme('username', $recipient));
  }
}

- Whitespaces
- Don't directly print the message but instead add it as a warning to the $messages array. There might be multiple recipients and even if one is blocked, the message will still be sent to the others
- Don't abbreviate variable names ($recipient instead of $rec)
- Use placeholders in translatable strings (also important for security)

Please test this and you're welcome to create a patch, see http://drupal.org/patch/create.

Berdir’s picture

Status: Needs review » Needs work
ptmkenny’s picture

Version: 6.x-1.2 » 7.x-2.x-dev
Status: Needs work » Needs review

Hi, this affects my site so I'm attempting to update this for 7.x

I attempted to modify _privatemsg_validate_message as follows:

  if (!empty($message->recipients) && is_array($message->recipients)) {
    foreach (module_invoke_all('privatemsg_block_message', $message->author, $message->recipients, (array)$message) as $blocked) {
      unset($message->recipients[$blocked['recipient']]);
      if ($form) {
        drupal_set_message($blocked['message'], 'warning');
      }
      else {
        $messages['warning'][] = $blocked['message'];
      }
    }
    //do not send private messages to blocked users
    foreach ($message->recipients as $key => $recipient) {
      if (!$recipient->status) {
        unset($message->recipients[$key]);
        //$key->name shows the user's private name if RealName module enabled-
        //must fix compatibility
        $messages['warning'][] = t('You cannot send a message to !name because that account has been cancelled.', array('!name' => $recipient->name));
      }
    }
  }

However, as noted in the comment, if the RealName module is enabled, the user's actual username will be revealed instead of the value that has been specified to be shown.

Also, if a user account has been cancelled, I don't think the reply form should be shown in the first place. So I made the following modification in privatemsg_view() in privatemsg.pages.inc:

  // Display the reply form if user is allowed to use it.
  if (privatemsg_user_access('write privatemsg') || privatemsg_user_access('reply only privatemsg')) {

    //the reply form should be hidden if any of the user accounts have been
    //cancelled
    $hide_reply = 0;
    foreach ($thread['participants'] as $key => $participant) {
      if (!$participant->status) {
        $hide_reply = 1;
      }
    }
    if ($hide_reply == 0) {
      $content['reply'] = drupal_get_form('privatemsg_form_reply', $thread);
      $content['reply']['#weight'] = 5;
    }
  }

I'd appreciate any feedback that you can provide on this code.

ptmkenny’s picture

Attached is a patch that simply gives an error if the user attempts to send a message to a blocked user. The warning does not include the username of the blocked user, because for RealName compatibility I tried format_username($account), but that returns Anonymous if the user account has been cancelled/blocked.

Status: Needs review » Needs work

The last submitted patch, privatemsg-do-not-send-messages-to-blocked-users-834706-8.patch, failed testing.

ptmkenny’s picture

Status: Needs work » Needs review
FileSize
751 bytes

Updating patch to remove "Sent messages" revision which is a separate issue

Status: Needs review » Needs work

The last submitted patch, privatemsg-do-not-send-messages-to-blocked-users-834706-10.patch, failed testing.

ptmkenny’s picture

ptmkenny’s picture

Status: Needs work » Needs review
FileSize
701 bytes

New patch, this time without dpm() (thanks testbot!)

Status: Needs review » Needs work

The last submitted patch, privatemsg-do-not-send-messages-to-blocked-users-834706-13.patch, failed testing.

ptmkenny’s picture

Status: Needs work » Needs review
FileSize
623 bytes

Ok, trying again.

Status: Needs review » Needs work

The last submitted patch, privatemsg-do-not-send-messages-to-blocked-users-834706-15.patch, failed testing.

ptmkenny’s picture

Not sure why the last one didn't work. Trying again.

ptmkenny’s picture

Status: Needs work » Needs review
ptmkenny’s picture

Marked https://drupal.org/node/2007868 as a duplicate

ptmkenny’s picture

Added feature request for hiding the reply form as per the second half of #7 here: https://drupal.org/node/2008096

Berdir’s picture

Priority: Minor » Normal
Status: Needs review » Needs work
+++ b/privatemsg.moduleundefined
@@ -1785,6 +1785,13 @@ function _privatemsg_validate_message(&$message, $form = FALSE) {
+      //do not send private messages to blocked users

Comment should start with a space and then an uppercase character and end with a "."

+++ b/privatemsg.moduleundefined
@@ -1785,6 +1785,13 @@ function _privatemsg_validate_message(&$message, $form = FALSE) {
+        if (!$recipient->status) {

This needs to check the recipient type as well to make sure it's a user.

If you implement this check within privatemsg_privatemsg_block_message(), then it should take care of unsetting the user and it should also make the reply form work as expected *I think*.

And a test would be great to cover this.

emmene-moi’s picture

For Drupal 6.x-2x-dev:

Line 1780 and below (sorry, no patch):

  if (!empty($message['recipients']) && is_array($message['recipients'])) {
    foreach (module_invoke_all('privatemsg_block_message', $message['author'], $message['recipients'], $message) as $blocked) {
      unset($message['recipients'][$blocked['recipient']]);
      if ($form) {
        drupal_set_message($blocked['message'], 'warning');
      }
      else {
        $messages['warning'][] = $blocked['message'];
      }
    }
    
    //do not send private messages to blocked users
    foreach ($message['recipients'] as $key => $recipient) {
      if (!$recipient->status) {
        unset($message['recipients'][$key]);
        $msg = t('A message could not be sent because !user has already cancelled his/her account.', array('!user'=>$recipient->name));
        if ($form) {
          drupal_set_message($msg, 'warning');
        }
        else {
          $messages['warning'][] = $msg;
        }
      }
    }
}
ptmkenny’s picture

Attaching a new patch that makes the revisions requested in #21. I will add tests as soon as I can but I haven't yet figured out how to block a user created in the test.

As per Berdir's comment, moving the validation to privatemsg_block_message() causes the reply form to be hidden properly, so I'm closing https://drupal.org/node/2008096 as a duplicate.

ptmkenny’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, privatemsg-do-not-send-messages-to-blocked-users-834706-23.patch, failed testing.

Berdir’s picture

Blocking a user should work like this in 7.x: user_save($user, array('status' => 0)). Yes, that's a strange API :)

Berdir’s picture

And good, now you can actually see the notices that I expected already in the previous patch. You just need to switch the conditions, so that it checks that it's a user before it accesses the user-specific status property.

ptmkenny’s picture

Status: Needs work » Needs review
FileSize
2.99 KB

Ok, trying again as per #27 and now with a test.

Status: Needs review » Needs work

The last submitted patch, privatemsg-do-not-send-messages-to-blocked-users-834706-28.patch, failed testing.

ptmkenny’s picture

Status: Needs work » Needs review
FileSize
2.99 KB

Re-roll of #28

Berdir’s picture

+++ b/privatemsg.testundefined
@@ -242,6 +242,10 @@ class PrivatemsgTestCase extends PrivatemsgBaseTestCase {
+    $blocked_recipient = $this->drupalCreateUser(array('read privatemsg', 'write privatemsg')); // set up user with read/write privatemsg permissions

Comments should always be a on a separate line, above the code they explain. Also uppercase first character and "." at the end.

Tests looks good. Would be nice to also have a test for the reply form. You could pick one of the existing messages that were sent, block the author and check that the recipient no longer sees the reply form.

Status: Needs review » Needs work

The last submitted patch, privatemsg-do-not-send-messages-to-blocked-users-834706-30.patch, failed testing.

ptmkenny’s picture

Status: Needs work » Needs review
FileSize
6.89 KB

Ok, I added another test at the end of the message tests to make sure that the reply form is not shown if the author is blocked. I also updated the comments throughout the test file (there were many comments with the incorrect style).

Status: Needs review » Needs work

The last submitted patch, privatemsg-do-not-send-messages-to-blocked-users-834706-33.patch, failed testing.

ptmkenny’s picture

Status: Needs work » Needs review
FileSize
6.93 KB

Re-roll. Remove %recipients from error message because it doesn't seem to display properly with blocked users.

Status: Needs review » Needs work

The last submitted patch, privatemsg-do-not-send-messages-to-blocked-users-834706-35.patch, failed testing.

Berdir’s picture

Hm, sorry, that is messier than I remembered it to be ;)

Users don't always have ->type set. Do this instead:

elseif (isset($recipient->status) && !$recipient->status) {

Also, in the test, you need 'recipient' => $blocked_recipient->name, you're missing the ->name.

Berdir’s picture

Also, when you re-roll, please add a -test-only.patch first, the idea is to show that the new tests to fail without the fix and then the patch with the tests and the fix to make sure that it's green.

ptmkenny’s picture

Status: Needs review » Needs work

The last submitted patch, privatemsg-do-not-send-messages-to-blocked-users-834706-39.patch, failed testing.

ptmkenny’s picture

Status: Needs work » Needs review
FileSize
6.94 KB

Ok, I think I catch the error message correctly this time. The second message is about all recipients being blocked.

Status: Needs review » Needs work

The last submitted patch, privatemsg-do-not-send-messages-to-blocked-users-834706-41.patch, failed testing.

ptmkenny’s picture

Status: Needs work » Needs review
FileSize
7.03 KB

Still learning how to write tests. Hopefully I got it right this time.

Status: Needs review » Needs work

The last submitted patch, privatemsg-do-not-send-messages-to-blocked-users-834706-43.patch, failed testing.

ptmkenny’s picture

Status: Needs work » Needs review
FileSize
6.99 KB

Trying @recipients instead of %recipient.

Status: Needs review » Needs work

The last submitted patch, privatemsg-do-not-send-messages-to-blocked-users-834706-45.patch, failed testing.

ptmkenny’s picture

I'm sorry, I don't understand why this is still failing. I created some user accounts manually on my local machine and tested this; the error messages appear properly (i.e., I get "User1 has disabled his or her account." and then "You are not allowed to send this message because all recipients are blocked."

Berdir’s picture

+++ b/privatemsg.testundefined
@@ -112,12 +113,15 @@ class PrivatemsgTestCase extends PrivatemsgBaseTestCase {
-    $user_no_write_msg = $this->drupalCreateUser(); // set up user with default permissions (meaning: no read privatemsg permission
+    // Set up a user with the default permissions (no "read privatemsg"
+    // permission).
+    $user_no_write_msg = $this->drupalCreateUser();

It's fine now but be careful with changing too much. It makes patches harder to review and increases the chance of conflicts with other patches, see http://webchick.net/please-stop-eating-baby-kittens

+++ b/privatemsg.testundefined
@@ -271,6 +279,12 @@ class PrivatemsgTestCase extends PrivatemsgBaseTestCase {
+      'recipient'   => $blocked_recipient,

Seee #37, you're passing it the user object, but you want the user name, see examples above and below. That will fix the tests.

ptmkenny’s picture

Status: Needs work » Needs review
FileSize
6.99 KB

Ah, I was changing the wrong line to add -> name. Thanks for all your help! Hopefully this does it...

Status: Needs review » Needs work

The last submitted patch, privatemsg-do-not-send-messages-to-blocked-users-834706-49.patch, failed testing.

ptmkenny’s picture

Status: Needs work » Needs review
FileSize
6.99 KB

Fixed PHP error in tests.

Status: Needs review » Needs work

The last submitted patch, privatemsg-do-not-send-messages-to-blocked-users-834706-51.patch, failed testing.

ptmkenny’s picture

Oops... Put the parentheses in the wrong place. This should be it.

Status: Needs review » Needs work

The last submitted patch, privatemsg-do-not-send-messages-to-blocked-users-834706-53.patch, failed testing.

ptmkenny’s picture

Status: Needs work » Needs review
FileSize
6.99 KB

Attaching again to fix the mis-included file in #53.

Status: Needs review » Needs work

The last submitted patch, privatemsg-do-not-send-messages-to-blocked-users-834706-55.patch, failed testing.

ptmkenny’s picture

Hmmm... I don't understand what that last exception (in #55) has to do with my patch. Any thoughts?

Berdir’s picture

+++ b/privatemsg.testundefined
@@ -324,6 +338,10 @@ class PrivatemsgTestCase extends PrivatemsgBaseTestCase {
+    $this->assertText(t('@recipients has disabled his or her account.', array('@recipients' => $editblocked)), 'Message about blocked user displayed.');

This, $editblocked is an array, you can't put that into a placeholder, you again want $blocked_recipient->name here.

ptmkenny’s picture

Sorry I'm so bad at this. Here it is again.

Berdir’s picture

Don't worry, we're getting there :)

The second part of the test about the reply box doesn't seem to fail in the test-only patch?

ptmkenny’s picture

Good point. I tested this manually and it seems that "Reply to thread" is not shown on the page, just "Reply". I have updated the test accordingly. (If this is in fact the solution, perhaps the other tests that check for the string "Reply to thread" should be updated too?)

EDIT: Oops, named the file "-no-tests" when it fact it is "-tests-only".

Status: Needs review » Needs work

The last submitted patch, privatemsg-do-not-send-messages-to-blocked-users-834706-61.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
Berdir’s picture

ptmkenny’s picture

Hmmm... Not sure why it's still not failing. "Reply" is definitely shown alongside the textarea to reply when testing manually.

ptmkenny’s picture

Ok, I reworked the test that wasn't failing properly. A few different changes were necessary, but I think I got it.

Status: Needs review » Needs work

The last submitted patch, privatemsg-do-not-send-messages-to-blocked-users-834706-66.patch, failed testing.

ptmkenny’s picture

Status: Needs work » Needs review
Berdir’s picture

Status: Needs review » Needs work

The last submitted patch, privatemsg-do-not-send-messages-to-blocked-users-834706-66.patch, failed testing.

ptmkenny’s picture

Ok, I re-did the test again and I finally got my local environment doing simpletest properly so it should be correct this time. The problem with the last test was that one of the recipients was unblocked so the reply form was still shown. To simplify things, I created a new message for the next set of tests in order to ensure that no unexpected behavior occurs.

ptmkenny’s picture

Status: Needs work » Needs review
FileSize
8.97 KB

EDIT: double post please ignore

Berdir’s picture

Status: Needs review » Needs work

Great, thanks for pushing this forward :)

+++ b/privatemsg.pages.incundefined
@@ -393,7 +393,7 @@ function privatemsg_form_reply($form, &$form_state, $thread) {
-      $blocked = t('You can not reply to this conversation because all recipients are blocked.');
+      $blocked = t('You cannot reply to this conversation because all recipients have been blocked.');

That is not the same meaning and IMHO unrelated.

The meaning of this error is that the recipients are blocked *for you*, you are not allowed to write them.

What you are changing it into means that the users were blocked and that's why you can't write them, but there can be many reasons that prevent you from writing them.

So if you want to improve this message, lets open a separate issue to discuss that.

+++ b/privatemsg.testundefined
@@ -76,7 +76,8 @@ class PrivatemsgTestCase extends PrivatemsgBaseTestCase {
-    $user_no_read_msg = $this->drupalCreateUser(); // set up user with default permissions (meaning: no read privatemsg permission
+    // Set up a user with the default permissions.
+    $user_no_read_msg = $this->drupalCreateUser();

This will conflict heavily with the permission name issue. I suggest you leave these changes out here as well and fix the comments there.

+++ b/privatemsg.testundefined
@@ -416,16 +444,28 @@ class PrivatemsgTestCase extends PrivatemsgBaseTestCase {
+    $this->assertNoText(t('Reply'), 'Reply form is not displayed.');
+    $this->assertText(t('You cannot reply to this conversation because all recipients have been blocked.'), 'All recipients blocked message is displayed.');

No need for an assertion message on the second line ( the second argument), it's almost the same as the first and by default, an assertion message is generated containing the actual text, adding one just makes it harder to find out what we are looking for if the test fails.

The first one is fine, because 'Reply' is quite short and not self-explanatory.

+++ b/privatemsg.testundefined
@@ -416,16 +444,28 @@ class PrivatemsgTestCase extends PrivatemsgBaseTestCase {
-    $enableduser = $this->drupalCreateUser(array('read privatemsg', 'write privatemsg')); // set up user with read/write privatemsg permissions
-    $enableduser2 = $this->drupalCreateUser(array('read privatemsg', 'write privatemsg')); // set up user with read/write privatemsg permissions
-    $disableduser = $this->drupalCreateUser(array('read privatemsg', 'write privatemsg', 'allow disabling privatemsg')); // set up user with read/write privatemsg permissions
+    // Set up a user with "read/write privatemsg" permissions.
+    $enableduser = $this->drupalCreateUser(array('read privatemsg', 'write privatemsg'));
+    // Set up a user with "read/write privatemsg" permissions.
+    $enableduser2 = $this->drupalCreateUser(array('read privatemsg', 'write privatemsg'));
+    // Set up a user with "read/write privatemsg" permissions.
+    $disableduser = $this->drupalCreateUser(array('read privatemsg', 'write privatemsg', 'allow disabling privatemsg'));

Same here, let's not change it here as we have another important issue where we can do this that touches these lines anyway.

ptmkenny’s picture

Ok, I didn't realize what the first error message meant, so I removed that change. I also removed the changes to the comments that can be done in the other patch. This version should have all the changes discussed in #73.

ptmkenny’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, privatemsg-do-not-send-messages-to-blocked-users-834706-75.patch, failed testing.

ptmkenny’s picture

Wrong recipient name in the test. Fixed now.

Berdir’s picture

Version: 7.x-2.x-dev » 6.x-2.x-dev
Status: Needs review » Patch (to be ported)

Thanks for your work on this, committed and pushed to 7.x-1.x and 7.x-2.x.

ptmkenny’s picture

Thank you for all the feedback. This is a much better solution than the one I came up with originally on my own!

ptmkenny’s picture

Previous 6.x thread here: https://drupal.org/node/1509436

oadaeh’s picture

Issue summary: View changes
Status: Patch (to be ported) » Closed (won't fix)

This issue is being closed because it is against a branch for a version of Drupal that is no longer supported.
If you feel that this issue is still valid, feel free to re-open and update it (and any possible patch) to work with the 7.x-1.x branch (bug fixes only) or the 7.x-2.x branch.
Thank you.