Comments

cwgordon7’s picture

Assigned: Unassigned » cwgordon7
Status: Active » Needs review
StatusFileSize
new13.07 KB

Setting to patch (code needs review) for lack of a better option.

File attached:

cwgordon7’s picture

StatusFileSize
new13.07 KB

Oops, I left debug code in there:

cwgordon7’s picture

StatusFileSize
new9.47 KB

Sorry, it's as a patch now.

webchick’s picture

Status: Needs review » Needs work

A very short review, since I'm off in a second.

a) Contrary to the task description, this should actually be against the DRUPAL-5 branch rather than HEAD, since it has some additional bug fixes/features. Sorry about that. :( The patch doesn't cleanly apply to DRUPAL-5, unfortunately, but probably just minor things.

b) Don't worry about porting Views integration. Views for D6 isn't ready, so this is out of scope of the ticket. You hadn't tried this yet, but I just wanted to point this out "for the record." :)

c) The actions integration stuff needs updating to Drupal 6, and this is a bit more involved than simply removing the "if module_exists()" stuff. Without giving away too many details (since you like challenges :)) it's a new hook that needs to be implemented. You'll know it's working properly when Trigger module is enabled, and you see Revision Moderation's actions show up under admin/build/trigger/node.

cwgordon7’s picture

Status: Needs work » Needs review
StatusFileSize
new12.37 KB

Patch re-rolled for review with actions support & against DRUPAL-5 branch.

cwgordon7’s picture

StatusFileSize
new12.09 KB

Patch re-rolled after discussion with chx on irc.

webchick’s picture

Status: Needs review » Needs work

I tested this pretty thoroughly, and confirmed that it's almost entirely there. There were only a couple of regressions:

a) Exemption is not working properly... users with administer nodes permissions are /always/ exempt from the revision moderation, regardless of whether the setting on the administer form is checked or not.

b) If, as a non-privileged user, I edit a node that has one of my existing revisions in moderation, I get the notice:

notice: Undefined property: stdClass::$menu in /Users/webchick/Sites/head/modules/menu/menu.module on line 371.

I cannot seem to duplicate this notice on content types without the revision moderation checkbox flagged.

Otherwise, tested action integration, editing with unprivileged/privileged users, etc. Fix those two things and this should be RTBC. :)

cwgordon7’s picture

Status: Needs work » Needs review
StatusFileSize
new12.95 KB

Ok, this, I believe fixes problem a.

webchick’s picture

Hm. That didn't seem to fix it. I still can't get users with 'administer nodes' permissions to go through the same revision process as everyone else, regardless of the setting on the admin form.

Create a role 'admin', give that role 'administer nodes' permissions, and create a user 'blah' and add them to the admin role.
Uncheck the "Exempt admins from moderation" checkbox on the settings form.

In 5.x, upon editing a node, you get the message that your revision is pending moderation
In 6.x, it just edits it directly.

We want the behvaviour in 5.x

On a quick inspection of the code though, I'm not sure /why/ it's not working. You didn't seem to touch that line. The only thing I can think is if the menu permission is overriding it somehow, but that seems odd.

Spend another half hour or so and see if you can fix it, and if not we'll go ahead and mark it complete anyway, since it's 99.9% there. I'll have to look sometime when I have a bit more time (unfortunately, I'm leaving Wed and will be sans Internet access for about 2 weeks so I'm swamped with last-minute work stuff atm).

cwgordon7’s picture

StatusFileSize
new25.96 KB

I was unable to duplicate the problem, even after following your instructions precisely. I get this screenshot message for a test admin with the administer nodes permission (the checkbox does not show up, and posts are forced into "pending revision" mode.)

add1sun’s picture

Ok, I talked to webchick and she said go ahead and mark the google task closed and she'll continue the weird troubleshooting here.

"This message approved by webchick." ;-)

webchick’s picture

Status: Needs review » Fixed

Crap. I am SO sorry, cwgordon. I just did a fresh install and indeed was unable to reproduce the bugs I was seeing. NO idea what was messed up on my previous install, but I apologize for the false alarm.

Thanks a lot for your patience, and also your hard work on this issue. :)

Committing this in a sec.

Anonymous’s picture

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.

ogunden’s picture

I'd love to be able to use this, but I can't get the patch to apply cleanly to any of the released versions. Can this be released as a 6.x development release?