Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
Comments
Comment #1
BerdirYep, 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.
Comment #2
wilton.pinheiro CreditAttribution: wilton.pinheiro commentedGreat Berdir.
I'll work next days on a patch. Will let you know further updates...
Thanks,
Wilton
Comment #3
BerdirGreat.
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...
Comment #4
wilton.pinheiro CreditAttribution: wilton.pinheiro commentedDear 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!
Comment #5
BerdirThe code should look like this (untested):
- 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.
Comment #6
BerdirComment #7
ptmkenny CreditAttribution: ptmkenny commentedHi, this affects my site so I'm attempting to update this for 7.x
I attempted to modify _privatemsg_validate_message as follows:
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:
I'd appreciate any feedback that you can provide on this code.
Comment #8
ptmkenny CreditAttribution: ptmkenny commentedAttached 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.
Comment #10
ptmkenny CreditAttribution: ptmkenny commentedUpdating patch to remove "Sent messages" revision which is a separate issue
Comment #12
ptmkenny CreditAttribution: ptmkenny commentedRelated issue: http://drupal.org/node/1421494
Comment #13
ptmkenny CreditAttribution: ptmkenny commentedNew patch, this time without dpm() (thanks testbot!)
Comment #15
ptmkenny CreditAttribution: ptmkenny commentedOk, trying again.
Comment #17
ptmkenny CreditAttribution: ptmkenny commentedNot sure why the last one didn't work. Trying again.
Comment #18
ptmkenny CreditAttribution: ptmkenny commentedComment #19
ptmkenny CreditAttribution: ptmkenny commentedMarked https://drupal.org/node/2007868 as a duplicate
Comment #20
ptmkenny CreditAttribution: ptmkenny commentedAdded feature request for hiding the reply form as per the second half of #7 here: https://drupal.org/node/2008096
Comment #21
BerdirComment should start with a space and then an uppercase character and end with a "."
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.
Comment #22
emmene-moi CreditAttribution: emmene-moi commentedFor Drupal 6.x-2x-dev:
Line 1780 and below (sorry, no patch):
Comment #23
ptmkenny CreditAttribution: ptmkenny commentedAttaching 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.
Comment #24
ptmkenny CreditAttribution: ptmkenny commentedComment #26
BerdirBlocking a user should work like this in 7.x: user_save($user, array('status' => 0)). Yes, that's a strange API :)
Comment #27
BerdirAnd 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.
Comment #28
ptmkenny CreditAttribution: ptmkenny commentedOk, trying again as per #27 and now with a test.
Comment #30
ptmkenny CreditAttribution: ptmkenny commentedRe-roll of #28
Comment #31
BerdirComments 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.
Comment #33
ptmkenny CreditAttribution: ptmkenny commentedOk, 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).
Comment #35
ptmkenny CreditAttribution: ptmkenny commentedRe-roll. Remove %recipients from error message because it doesn't seem to display properly with blocked users.
Comment #37
BerdirHm, sorry, that is messier than I remembered it to be ;)
Users don't always have ->type set. Do this instead:
Also, in the test, you need 'recipient' => $blocked_recipient->name, you're missing the ->name.
Comment #38
BerdirAlso, 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.
Comment #39
ptmkenny CreditAttribution: ptmkenny commentedComment #41
ptmkenny CreditAttribution: ptmkenny commentedOk, I think I catch the error message correctly this time. The second message is about all recipients being blocked.
Comment #43
ptmkenny CreditAttribution: ptmkenny commentedStill learning how to write tests. Hopefully I got it right this time.
Comment #45
ptmkenny CreditAttribution: ptmkenny commentedTrying @recipients instead of %recipient.
Comment #47
ptmkenny CreditAttribution: ptmkenny commentedI'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."
Comment #48
BerdirIt'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
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.
Comment #49
ptmkenny CreditAttribution: ptmkenny commentedAh, I was changing the wrong line to add -> name. Thanks for all your help! Hopefully this does it...
Comment #51
ptmkenny CreditAttribution: ptmkenny commentedFixed PHP error in tests.
Comment #53
ptmkenny CreditAttribution: ptmkenny commentedOops... Put the parentheses in the wrong place. This should be it.
Comment #55
ptmkenny CreditAttribution: ptmkenny commentedAttaching again to fix the mis-included file in #53.
Comment #57
ptmkenny CreditAttribution: ptmkenny commentedHmmm... I don't understand what that last exception (in #55) has to do with my patch. Any thoughts?
Comment #58
BerdirThis, $editblocked is an array, you can't put that into a placeholder, you again want $blocked_recipient->name here.
Comment #59
ptmkenny CreditAttribution: ptmkenny commentedSorry I'm so bad at this. Here it is again.
Comment #60
BerdirDon'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?
Comment #61
ptmkenny CreditAttribution: ptmkenny commentedGood 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".
Comment #63
Berdir#61: privatemsg-do-not-send-messages-to-blocked-users-834706-61.patch queued for re-testing.
Comment #64
Berdir#61: privatemsg-do-not-send-messages-to-blocked-users-834706-61-no-tests.patch queued for re-testing.
Comment #65
ptmkenny CreditAttribution: ptmkenny commentedHmmm... Not sure why it's still not failing. "Reply" is definitely shown alongside the textarea to reply when testing manually.
Comment #66
ptmkenny CreditAttribution: ptmkenny commentedOk, I reworked the test that wasn't failing properly. A few different changes were necessary, but I think I got it.
Comment #68
ptmkenny CreditAttribution: ptmkenny commented#66: privatemsg-do-not-send-messages-to-blocked-users-834706-66-tests-only.patch queued for re-testing.
Comment #69
Berdir#66: privatemsg-do-not-send-messages-to-blocked-users-834706-66.patch queued for re-testing.
Comment #71
ptmkenny CreditAttribution: ptmkenny commentedOk, 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.
Comment #72
ptmkenny CreditAttribution: ptmkenny commentedEDIT: double post please ignore
Comment #73
BerdirGreat, thanks for pushing this forward :)
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.
This will conflict heavily with the permission name issue. I suggest you leave these changes out here as well and fix the comments there.
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.
Same here, let's not change it here as we have another important issue where we can do this that touches these lines anyway.
Comment #74
ptmkenny CreditAttribution: ptmkenny commentedOk, 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.
Comment #75
ptmkenny CreditAttribution: ptmkenny commentedComment #77
ptmkenny CreditAttribution: ptmkenny commentedWrong recipient name in the test. Fixed now.
Comment #78
BerdirThanks for your work on this, committed and pushed to 7.x-1.x and 7.x-2.x.
Comment #79
ptmkenny CreditAttribution: ptmkenny commentedThank you for all the feedback. This is a much better solution than the one I came up with originally on my own!
Comment #80
ptmkenny CreditAttribution: ptmkenny commentedPrevious 6.x thread here: https://drupal.org/node/1509436
Comment #81
oadaeh CreditAttribution: oadaeh as a volunteer commentedThis 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.