Allow moderation exemption to be configurable by role
mrf - December 7, 2007 - 18:00
| Project: | Revision Moderation |
| Version: | 6.x-1.x-dev |
| Component: | Code |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs review |
Description
I ran into a situation where two roles were submitting the same content type that was under revision moderation, one role needs to have posts moderated and the other published directly.
This patch adds a "bypass revision moderation" permission that can be given to a role that shouldn't have the ability to administer nodes, but doesn't need revision moderation on their posts.
| Attachment | Size |
|---|---|
| revision_moderation_role_perm.patch | 1.55 KB |
#1
Great, thanks so much for this patch. I agree this makes a lot more sense, and I'm not sure what I was thinking of keying it off administer nodes before. ;)
Question: does it make sense to remove that "allow people to bypass moderation" option in the settings page in favour of the permission? That would certainly make this line:
+ if ($node->nid && $node->revision_moderation == 1 && (!user_access('administer nodes') || !user_access('bypass revision moderation') || !variable_get('revision_moderation_exempt', 1))) {a bit shorter. ;)
Also, I don't see a need to key it off either administer nodes OR bypass revision moderation. Just the new perm is fine. So marking code needs work for that.
I think we probably also need an upgrade path to loop through each role with 'administer nodes' enabled and give them this new permission?
#2
Nice work and I can't add anything more to what webchick has posted already. I would definitely use this new permission.
#3
I definitely agree with all of your suggestions. Leaving this permission as the only way to bypass moderation would make administration of the module a lot less confusing.
Does dropping that administer nodes checkbox mean that the administration page should be dropped altogether?
#4
Oh. Yes! I suppose it does! I forgot that that was the only setting on that page. So yep, rip it out of there. :)
#5
Here's another stab at it.
I made an attempt at retaining permissions from the old admin page in the install file, I haven't set foot in an install file before, so let me know if I'm on the right track there.
#6
Wow, you even got the variable deletion in the update hook. Nice one! :D
Some notes/observations:
- There's no update for pgsql. I have no idea how many pgsql users use this module, but I think this is ok. One of them can always supply a follow-up patch. :D
- I would move the variable_del() call outside of the if. We'll want to delete that variable whether it's set to true or not.
- The update code itself doesn't seem to work properly... I tested it on a site with three roles: anonymous, authenticated, and admin. Of those, "authenticated" user was the only one with "administer nodes" checked, but yet both it and anonymous got the new bypass permission, which is.. mystifying. But in any case, we should only run this code for roles with "administer nodes" permissions, and the current code doesn't seem to adjust for that.
- You also need to clear the menu cache at the end of the update function. Right now, when I run it and go to admin/settings/revision_moderation I get:
warning: call_user_func_array() [function.call-user-func-array]: First argument is expected to be a valid callback, 'revision_moderation_settings' was given in /Users/webchick/Sites/5x/includes/form.inc on line 218.Check system_update_113() for a snippet that does this.
- A consequence of removing the settings page altogether and not checking for the variable is that user 1 will *always* be exempt from revision moderation. I kind of think this is okay, since user 1 is exempt from all other permission checking in the system. But I'd be curious to hear peoples' thoughts on this.
And now, I'm going to be super-anal, cos that's how we Drupal people roll, baby. ;) And that way you know for future patches. :)
+function revision_moderation_update_2() {- This function should have a comment block on top of it that summarizes what it is doing. Just a short sentence will suffice.
(the whole rest of the function)
- The indenting looks a bit messed up here. Remember that the Coding standards dictate two-space indentations, no tabs. You'll also want to watch your spacing on things like
){#7
After a long break, my travels brought me back into the world of revision moderation.
I realized that I once again needed this permission for a new project, I've corrected the above issues, and updated the patch for the current head (6.x) version of the module.
#8
#9
I had this same issue in d5. Back-ported the patch and added it to a new discussion here: http://drupal.org/node/421822 so as not to hijack this thread. Code works well in d5 with zero changes... just had to find the right places to paste the code. Thanks!