Setting to define certain roles (e.g. staff) which a user cannot block PMs from

good feature.i'll integrate it for the d6 release.

Any chance of getting it in a D5 release? ... or suggestions on how to patch it for D5?
Need to be able to allow users to disable PM but still get PMs from Mods and Admins.

Chances of this being backported to Drupal 5 are highly unlikely unless someone is willing to do the work.

This may however be a useful addition to the sub module is Drupal 6 version of privatemsg.

I envision it not as a person being able to send a message even if blocked, but rather a mechanism to not block users from some roles.

Can't we use drupal's permission system for that ? Add a permission like "unblockable" and ignore the block request if a user has this permission ? It should also be possible to backport that do d5 but I will not do it because I don't use drupal5.

Here is a simple patch it does work but the permission name "unblockable" is ugly, has anybody a better idea?

@berdir,

It is better if we offer UI for user to select which roles cannot be blocked. more flexible that way and we dont have to deal with actual permission strings.

I like the UI idea, but the permissions route is also a pretty good idea, IMO.
Instead of "unblockable" how about "PMs override blocking" or "overrides disabled PM" or something along those lines? BTW, does the patch work w/5.x? Couldn't quite tell by looking at it and I don't have a good patch applyer so I'd probably have to do it by hand or something... (last time I looked into it, getting a patching thing working on Windows looked harder than editing patches by hand... maybe I just didn't find the better tools?)

Drupal 6 privatemsg is a totally different beast from the drupla 5 version. It will most likely not apply.

re-rolled the patch to use own code to check roles.

Currently relies on #288960: Search private messages function requested to provide a default local tab and needs to re-rolled after #288183: Provide api function for other modules to send messages. because the hook for the (un)block link changes, but it does work. I also thought about implementing #315684: Block users from sending messages to specific user groups here, but I need to think about how to do this.

Updating title.

I've rolled a new patch for people to have a look at. It documents the new functions and changed the settings page title to 'Block author settings'.

I'd set this for review, but there's a weird issue where adding a role that cannot be blocked excludes them from 'Send reply to *name*:' Maybe I've broken something in these changes, but can't work out what it is at the moment, so will see if anyone else can.

I have some code for block_author which hides all participants, messages and threads (which only contain messages from blocked users) and adds a separate tab which lists all blocked users so that they can be unblocked.

Imho, such a feature is nice, because if I block a user, I don't want to see the messages he sent to me anymore. Currently, I need to manually delete the messages he sent and if I did that, I can't unblock him anymore. That user might be a spammer, which sent me 10 or more messages. With my thread action patch andp privatemsg_filter, it's easy to delete all of them, but still..

Should I include that into the patch or make a new one?

@liam
In pm_block_user_block_message(), the check should be moved above the query, as it does not need to be executed when a recipient can't be blocked.

Some minor changes of liam's updated patch.

Thinking about #12, I will open a new issue when that one is commited. No need to kill kittens here.. :)

@liam
Please test if you can reproduce your described issue with my patch... I can't.

Thinking about #12, I will open a new issue when that one is commited. No need to kill kittens here.. :)

Agreed. :)

Please test if you can reproduce your described issue with my patch... I can't.

Yes! It now works perfectly. Also, coder.module found no problems, I've had another read through the code and I see you've incorporated #13 into the latest patch (thanks for that, I have no idea why I put that SQL statement in before the check!).

Looks great! This is ready to be committed IMO. :)

The bad news is that this patch needs work. The good news is I have a mockup of the possible new look, complete with (mostly) working AHAH goodness! This change was decided in a conversation on IRC, and will also fix #315684: Block users from sending messages to specific user groups. Known problems/to-dos:

• none of the logic is hooked up to the form, so don't expect this patch to do anything; :)
• needs some help text added to the form, it's a confusing form otherwise;
• text/headings need to be reviewed;
• remove buttons must have unique #value's, I've added the $delta (unique identifier for the row of roles and actions) to the #value for now, we could use CSS image replacement -- like Quicktabs -- or some other plan, anyone have any thoughts/preference?
• AHAH event handlers don't seem to be attaching themselves to dynamically created remove buttons;

This is just a glorified mockup, to let everyone see the proposed interface we discussed in IRC, before going to the effort of implementing the logic behind it. :)

Screenshot attached.

Small update after discussing with litwol in IRC. Added an 'Enabled' column, and allowed the user to remove the second row of roles/actions. Patch and screenshot attached.

Yes, many thanks, Liam! It's looking good.

On #1, would be more clear with "Example:" in there somewhere... assuming I'm correct that that's what it's there for.

Agree w/Keyz on #2.

Also like CSS replace idea on #3.

New version of the patch. It still isn't hooked up to any logic, so it still won't do anything but look pretty. :)

I've taken a few suggestions on board:

1. Got rid of the first row: it was meant to show that the default setting is that any user can block any other user, I've replaced it with some text that only appears if there are no rules.
2. Added CSS image replacements for the add and remove buttons (used the Garland colours as the base, so they fit with the theme).
3. Added help text, feel free to improve it though. :)
4. Changed 'Then' to 'Action' and replaced drop-down select with radio buttons.

Looks great

I do not see the important of the "enabled" column. would clicking remove not be enough?

Also to cover #315684: Block users from sending messages to specific user groups there would need to be more actions such as "allow messages" and "disallow messages" in addition to allowing or disallowing the blocking of messages from users.

Actually, taking the thinking further:

1. To allow or disallow blocking, is it not more expected that only the authors role should matter?
2. For actually blocking of messages by role, the recipient's role will also matter.

(I do not think I have phrased this reply in the most eloquent of manners, but I hope its meaning is not lost?)

For my part, I don't see why it isn't enough to have a check box that says "do not allow recipient to block PMs from this role" and a role-select dropdown.
But it looks like what's in progress here will do that (though in a way that may not be quite as easy to understand) so ...will just ignore the rest I guess.

@nbz: I'm not clear on what you're asking here... wouldn't the actual blocking be done by users themselves on an individual basis? Or are you thinking of a role that is allowed or not allowed to block at all?

I am thinking of having both possibilities - per user blocking done by the user ANS per role blocking done by the sire admin.

Here's the first attempt at adding logic. Test it thoroughly, and help find bugs!

Also, don't forget to download the add button and delete button and put them in your pm_block_user directory.

Patched.
The rule setting interface seems to work well (though I'm not sure I know what happens when a person has multiple roles) and looks great.

Can't claim that I've tested thoroughly at this point, but in my tests, the "block author" link does not appear when a user receives a message from a role that the user's role is not supposed to be able to block.

So, it's looking good so far.

Can verify that blocking works.
Suggestion: "Disallowed to send message because all recipients are blocked" is a bit clunky... maybe
"Sending this msg is not allowed because..." would be better?

Ok, going through the patch...

- The UI/Buttons are somewhat uncommon ;) I'm not saying it's bad, just different ;)

+      return '<p>' . t('This area is used t.... huge text', array('@roles' => url('admin/user/roles'))) . '</p>';

- very minor, afaik it's common to split the text over several lines.

+    // $delta may be zero, if $block_actions is an empty array.
+    $delta = ($delta > 0 ? $delta : 1);

- I don't get that, if $block_actions is empty, the foreach is never executed and neither is that code. The code may make sense, but the comment does not (for me).

+  // Provide different action radios if we're using a whitelist or a blacklist.
+  if ($blacklist) {
+    $form['action']  = array(
+      '#type' => 'radios',
+      '#options' => array(
+        PM_BLOCK_USER_DISALLOW_BLOCKING => t('Disallow blocking author'),
+        PM_BLOCK_USER_DISALLOW_SENDING => t('Disallow sending message'),
+      ),
+      '#default_value' => (isset($details['action']) ? $details['action'] : PM_BLOCK_USER_DISALLOW_BLOCKING),
+      '#disabled' => $row_disabled,
+    );
+  }
+  else {
+    // TODO: add whitelist action here.
+  }

- What about just creating a $options array and a default value inside the if blacklist ? Then you don't have to duplicate the form field defintion

- I can't say much about all the FAPI stuff, it seems to be well documented.

+  if (!user_access('read privatemsg', $user)) {

- We should use privatemsg_user_access() here..

+ * Checks in saved settings whether a rule exists for a given author, recipient
+ * and action. For example: if this is passed User A (who has the admin role),
+ * User B (who has the authenticated user role) and
+ * PM_BLOCK_USER_DISALLOW_BLOCKING parameters, and a rule is configured that
+ * disallows authenticated users blocking admins then this function will return
+ * TRUE.

- Split the comment, the first line should be the short description, and the rest should follow after a empty line

+ *   Author user account to check.
+ * @param $recipient
+ *   Receiver user account to check.

- maybe object instead of account?

+ * @see PM_BLOCK_USER_DISALLOW_BLOCKING
+ * @see PM_BLOCK_USER_DISALLOW_SENDING

- I'm not sure if that works (aka, if the api.module can parse it)

+ *   Boolean value which is TRUE if a rule exists for the combination of

- I usually only use "TRUE if... "

+  // TODO: should we do anything special for user 1 here?

- Good question, I was asking that myself. Probably yes, as uid 1 is normally allowed to do everything.

+  if (_pm_block_user_rule_exists($author, $recipient, PM_BLOCK_USER_DISALLOW_BLOCKING)) {
+    // It's highly unlikely a user will ever get this far, but we'll set a
+    // message explaining that a rule forbad sending their message.
+    drupal_set_message(t('Sorry, rules setup by the site administrator forbid blocking !name.', array('!name' => $recipient->name)), 'error');
+    drupal_access_denied();
+    return;
+  }

- Hm, I'm pretty sur it isn't possible to reach this, because the menu access callback should already block it.

+ foreach ($recipients as $uid => &$recipient) {
- Do we need by reference here? what for?

'Sorry, rules setup by the site administrator forbid sending messages to !name.'
- Not sure about the wording, should it be so "communicative"?

+    $recipient = $recipients[$row['recipient']];
+    // If there's a rule disallowing blocking of this message, send it anyway.
+    if (_pm_block_user_rule_exists($author, $recipient, PM_BLOCK_USER_DISALLOW_BLOCKING)) {
+      continue;
+    }

- So we do test this twice then ? the recipients aren't by reference, can't we just unset so that we already "blocked". Also, _privatemsg_validate_message() should take care of double-blocking, if not, that's a bug there.

Not yet tested this as I have to go now, but the code looks very clean and well documented, these are just nitpicks :)

- The UI/Buttons are somewhat uncommon

True, but so is an AHAH admin interface! :) Using images actually makes it easier to create multiple delete buttons with unique names. For another example of a funky admin interface, see: Quick Tabs (demonstration site).

- very minor, afaik it's common to split the text over several lines.

I was wondering the same when writing it, are you sure that's the case? I had a look at forum_helpand node_help, they both put it all on one line. :-/

+    // $delta may be zero, if $block_actions is an empty array.
+    $delta = ($delta > 0 ? $delta : 1);

Good catch, this was a throw-back to when we used a 'default' row. It's superfluous now, so I've delete it.

- What about just creating a $options array and a default value inside the if blacklist ? Then you don't have to duplicate the form field defintion

Done.

- We should use privatemsg_user_access() here..

Done.

- Split the comment, the first line should be the short description, and the rest should follow after a empty line

Done.

- maybe object instead of account?

Done.

Removed @see constants.

- I usually only use "TRUE if... "

Core prefers your method, so I've changed my documentation. :)

uid 1 is normally allowed to do everything.

Fixed.

- Hm, I'm pretty sur it isn't possible to reach this, because the menu access callback should already block it.

Tested, and you're correct, the page access function is called on a submit so it's just a waste of user_load()'s. Code removed.

- Do we need by reference here? what for?

Pants! I knew I should have documented that. Added a comment to explain why we do that, it's quite cunning. Actually it's more than quite cunning, it's as cunning as a fox that's just been appointed Professor of Cunning at Oxford University.

- Not sure about the wording, should it be so "communicative"?

Tweaked it a bit, it now reads: Sorry, private messaging rules forbid sending messages to !name.

I'd really like to avoid giving the user no information, like your average Microsoft error message, or an admin may have no idea it's their rules stopping Deirdre, the secretary, from sending a PM to Mr Verryimpohrtint, the managing director. :)

- So we do test this twice then ?

As discussed on IRC, the first call is a check whether the user is allowed to send, the second is a check whether the user is allowed to block.

Documentation, explaining this, added. :)

Whoops! That last patch had a syntax error, uploading a working one.

** EDIT **
Also, note that until #442698: Unsupported operand types in privatemsg.module on line 1277 is committed this patch may cause fatal errors, that's not a problem with this patch though. :)

Re-roll with a couple of added comments.

Committed! Thanks for the reviews and help all. :)