Dear Eugen,

since I am using your fantastic module for a client who got some special needs, I spent some time on this, as you probably noticed.
Now I did finish my first release candidate for the client. I did clean up some spelling errors and inserted some spaces to add more readability.
Among the new features is my second version of a content_moderation admin UI and a support for drafts.

In your documentation it is stated that publishing new nodes is necessary and therefore forced by the module. I did deactivate this as I needed to have drafts, too. Instead, I extended the function _content_moderation_set_live (workflow.inc) to alter the state upon setting a node live and in the content_moderation block it will say 'Draft' instead of 'Live'. This way one may have drafts first (unpublished) and have them being published upon setting a revision live.
My question here: what is the reason for forcing publishing new nodes?

However, since I don't see a point in branching this promising module, I would kindly ask you (Eugen) as well as the community to review my review of code using the patch provided.
It includes the new content_moderation.admin.inc (patch is assuming empty php file).

CommentFileSizeAuthor
#2 content_moderation_code_review.patch20.08 KBbroon
#1 review.patch19.32 KBbroon

Comments

broon’s picture

broon
Location Potsdam
commented
StatusFileSize
new review.patch19.32 KB

Somehow the patch got stripped.

broon’s picture

broon
Location Potsdam
commented
StatusFileSize
new content_moderation_code_review.patch20.08 KB

Had been a long night, but I tested a couple of things with that, especially the forcing of published status.
How it works for me now: A content-type is under moderation, initial (new) nodes are set to be unpublished. Therefore any new nodes of that CT are only drafts, however these drafts are under moderation as long as no revision reaches moderation state "live". When this happens for the first time, the whole node will be set to state = 1 (= published), there's no need to check the checkbox on node edit form (which in turn means, the users who will be able to moderate nodes as "live" do NOT need the "administer nodes" permission).

But since this is by no means intended by Eugen in the first place (which I do understand for various reasons) I turned this "feature" into an option within the new admin interface and defaulting it's behaviour to force publishing of new nodes.

So, here's the updated patch of my code review.

Regards,
Paul

eugenmayer’s picture

eugenmayer commented

First off, great work.

Iam perfectly fine with including that option and its great that we can have both approaches. Your case is equal valid compared to my cases, so actually its great to have those features in.
Thank you for the hard work sin.star.

Just asking you once again, to be sure to have a clear "yes or no". do you want to be the co-maintainer fo this module? Due to your huge improvementes and contributions i really see a fair point here. please let me know.

Thanks :)

captainack’s picture

captainack commented

Great patch to an already awesome module :) Thanks guys!

Just chiming in to give you guys some early feedback. I'm running with this patch now, and it works pretty well. I had one issue, though, that I haven't been able to reproduce unfortunately. Here's what I'd done:

- Disabled the module
- Applied the patch
- Enabled the module, and disabled "force publish" and "publish"
- Added the transitions from every state to live, including none -> live
- As anonymous, created a new node of my custom content type
- As admin, promoted the "none" revision of that node DIRECTLY to "live"

At this point, I couldn't view the node as anonymous (access denied), but I could view the node as admin, and "status" was 1 in the database.

I edited the node as admin, creating a new revision in the "none" state, and anonymous could view it again. Very strange.

I've done whatever I can to try to reproduce it, but with no luck. Hopefully this sets a lightbulb off in your heads. Otherwise, I'll keep my eye out and report if I'm able to reproduce it.

Best,
Al

eugenmayer’s picture

eugenmayer commented

@ #4: anons cant see non published nodes or revisions. could there be a bug that setting node to "live" did not automatically publish it?

direct all the koodos to sin.star :)

captainack’s picture

captainack commented

Hi.

That's the strange part. The node was published in the node table. I checked before the problem disappeared. I probably should have backed up while it was still not working so I could investigate :(. Hopefully it pops up again.

Thanks,
Al

broon’s picture

broon
Location Potsdam
commented

Hey Al,

thanks for the feedback. I tested it quite intensively before uploading the patch here and I didn't run into this kind of error, though. However, as I also included in the admin ui, this is marked experimental since I am not fully aware of the impact it causes yet.
As soon as my main machine is running again (or I got a new one, my graphic card stopped working last night) I will try to recreate this issue.

As far as my changes go I altered the content_moderation.workflow.inc in line 134 now reading as

db_query('UPDATE {node} SET vid = %d, status = 1 WHERE nid = %d', $vid, $nid);

which publishes a node whenever it is set to live (even if it is published already). But as you wrote this works as intended. I would at first suspect a permissions setting since you are creating nodes as an anonymous user (which I indeed haven't tried yet).

Regards,
Paul

broon’s picture

broon
Location Potsdam
commented

Ok, couldn't wait to try the anonymous user stuff and now I finally started my drupal development site and tried to recreate the issue.
First, I created a node as an anonymous user on a clean installation of Drupal 6.16 with content_moderation 6.x-1.2 and applied patch.
With the anonymous user I can't see the node afterwards since it's not published. As an admin I view the unpublished node and changed moderation state from none to live. The anonymous user was immediately able to view the node as it has been published by changing moderation state to "live".

So, there might be a problem in connection with your custom module? If you have any further issues or if you are able to recreate the problem described above, please report here.

Regards,
Paul

captainack’s picture

captainack commented

Hi Paul,

Thanks for trying to reproduce the issue. Unfortunately, I didn't have too much time to try to reproduce this today, although I tried quite a bit yesterday, and had the same results as you. Since you also tried to bring it back and couldn't, I'm pretty suspicious that this has something to do with some corner cases of compatibility with other modules I have enabled (especially pathauto and domain_access).

When I get a chance, I'll try some cases involving special cases with those modules (e.g. post on one domain and promote on another, etc). Come to think of it, it may also be a different incarnation of #762790: Better integration with other modules (like pathauto) ... With the current flow of this module, node_access_acquire_grants is called by node_save with the "fake" live revision. I haven't thought about the potential side effects of this enough, and domain_access has quite a bit of code I'll have to read :P. Maybe the node_access table is getting goofed under some special situations.

Thanks again,
Al

eugenmayer’s picture

eugenmayer commented

if you like, http://github.com/EugenMayer/content_moderation its on github now. Fell free to start a push request and include the patch there :)

Please start a new branch for this feature

broon’s picture

broon
Location Potsdam
commented

Hey,

I created a github account forked/cloned it and made my changes. These are online as well and I made a pull request. Hope this meets the normal procedure (since I am new to git). Actually I think I should have changed the fork name (mine is still named 'master'). But it's a first try at least.

Regards,
Paul

eugenmayer’s picture

eugenmayer commented

Hey Paul,

well its was close to perfect. As you stated, you should have forked and then create a branch called by the ticketid mabye or descriptive name, i have chosen adminui in content_moderation brancht.
I pulled your changes, somehow the "Network graph" does not reflect this ( pretty bad), so people cant see that your fork is included in the current main project, same goes for the branch of akheaf, which is already included in the master.

Paul should we skype? (can find / see you in my skype) .. regarding the co-maint.

eugenmayer’s picture

eugenmayer commented
Status: Needs review » Needs work

Current code is check in here http://github.com/EugenMayer/content_moderation/tree/rewrite

I cant see that the admin menu is working. There is no menu defintion at all?

broon’s picture

broon
Location Potsdam
commented

There is a menu entry in my original patch. I still have to sort out which changes belong to which issue/patch. As soon as this is done, I will update the branches on github.

eugenmayer’s picture

eugenmayer commented

Oh ok, fine. Would be great if you can find time shorty as i would love to release a RC1 to let people test the wide changes. Actually first i planned to make to releases, one maintainance and then the one with new features but actually the bugfixing was only possible with a lot of rewriting which makes the changes to huge for a maintainance release.

So i would actually like to merge in all branches, also the adminui one and test our development there. I looking forward to see your great work!

eugenmayer’s picture

eugenmayer commented
Status: Needs work » Needs review

Feature is implemented and seems to work. Now we need some people to test it :)

eugenmayer’s picture

eugenmayer commented

When i create a transition and delete it, i get this :

http://www.ubuntu-pics.de/bild/52500/screenshot_127_OmGXUI.png

-> In addition the "save transition" button should be only name "save", as it also deletes those.

---

user warning: Column count doesn't match value count at row 1 query: INSERT INTO content_moderation_states VALUES ( "foo bar", "dsf" ) in /var/www/dw_basic/core/sites/all/modules/upgrade/content_moderation/content_moderation.admin.inc on line 159.

-> Thats what happens when we insert a state. Guess thats because of the introduction of weight.
We have to options, set the weight to zero by default in the query or even finish the weight implementation and let the weight be adjustable in the UI

----

States are only allowed to be machine names, so no spaces. Only uppercase, lowercaser and underscores. No umlauts. This shoudl get validated by the same regexpt used for node_types and so forth

broon’s picture

broon
Location Potsdam
commented

For the error while deleting it seems there was an older version of admin.inc in place. Corrected that as well as rewrote the database handling (now using the db schema). That way it won't need further changes when even more additional fields are going to be set up.
All changes in branch "dbhandling" (github).

eugenmayer’s picture

eugenmayer commented

Seems to work as expected now. Anybody else teste this feature?

eugenmayer’s picture

eugenmayer commented
Status: Needs review » Fixed
eugenmayer’s picture

eugenmayer commented
Status: Fixed » Closed (fixed)

released in 1.3 as submodule content moderation workflow ui