Incorporate changes that make sense from draft module

I'm giving the module a first round. Seems pretty clean :-)
Once again, thanks to both of you for the effort.

Here are a few remarks :

- On the revisions tab : the "revert" word seems odd when the revision is a draft - Back to the future ?
- It seems the "pending revisions" list only show the title of the first draft :
Say user_foo has "my node 1.0", and edits a new draft with the title "my node 2.0"
admin sees "my node 2.0" pending - OK
Before admin "commits", user_foo creates a new draft "my node 3.0"
admin still sees "my node 2.0" pending (though the list on the node's "revisions" tab correctly lists the two dratfs)
- Sort of related : It seems simpler to have drafts managed on the existing "revisions" tab - only problem is this currently does not display the title changes
- The publish revision ("revert") operation seems to have strange effects :
my node gets unpublished, and vid=0 in the {node} table. I currently have to manually correct this in the db.
- Is it necessary to copy the accepted draft as a new revision ? Why not simply turn the accepted draft into the node's current revision (and add a log message saying "Published by admin on (date)" ?
- BTW, the log message seems to display strangely on the node's page
if my node's body is "test", it displays "testLog:Copy of the revision from Wed, 27/09/2006 - 22:02."
(not sure this is really this module's fault, though...)

Here's for now. More thoughts later on :-)

Is it necessary to copy the accepted draft as a new revision ? Why not simply turn the accepted draft into the node's current revision (and add a log message saying "Published by admin on (date)" ?

I think this helps maintain the concept of a logical "stack" of history under the current vid, including drafts that didn't get published. Similar the current behavior if you revert to an old vid. It copies the revision to the top of the stack and logs what's happened.

Another idea... What if there was a "Create New Draft" tab next to edit, which automatically flagged "in moderation" and "create new revision". This would automate the steps and make this a bit more end-user friendly.

Here's a patch for the zero and status vid problem. Please review before trying it :) After stepping through the revert code, I found that revert calls the module's nodeapi "update" event, which was attempting to set vid and status to the values saved in "submit". So I added a test that they've been set before updating the node. Other than that, it was working as expected. The concept still looks solid after digging into the node module. At some point, it would be nice to have some tighter integration into the nodeapi.

I think this helps maintain the concept of a logical "stack" of history under the current vid, including drafts that didn't get published. Similar the current behavior if you revert to an old vid. It copies the revision to the top of the stack and logs what's happened.
OK, this makes sense.

I think one case where it makes sense to not copy is when the topmost draft gets published. Then it is somewhat of a duplicate state. But since the copy happens in the node module, I could live with it.

I'm getting caught up on the various threads regarding drafts, so I should be able to stop posting duplicate information real soon now. :)

My patch foo just isn't up to snuff... In fact, I think I ran the diff tool backwards. Oops.

I changed this (lines 66-68)...

// Update node table's vid to the original value.
db_query('UPDATE {node} SET vid = %d, status = %d WHERE nid = %d', $node->original_vid, $node->original_status, $node->nid);
drupal_set_message(t('Your changes have been submitted for moderation.'));

To this...

// revert calls this too, so check that vid and status were saved before updating the node
if (isset($node->original_vid) && isset($node->original_status)) {
  // Update node table's vid to the original value.
  db_query('UPDATE {node} SET vid = %d, status = %d WHERE nid = %d', $node->original_vid, $node->original_status, $node->nid);
  drupal_set_message(t('Your changes have been submitted for moderation.'));
}

hook_nodeapi "submit" isn't necessarily called in every case. I'm looking at line 1303 of node.module, and it says that "update" is triggered via node_save when a revision is reverted. Then, at line 489 in node_save, it calls hook_nodeapi with "insert" or "update", but not submit. So in the case of revert, "update" is called but not "submit". So neither original_vid or original_status have been set and they end up being zero in the query.

By more integrated into the node api, I mean that some of us view this as something that could or should be part of the core revision mechanism. I think that if revisions are in core, then drafts should be too, since they often go hand in hand. For example, I would have no use for CVS or Subversion if I had to commit changes every time I saved. If it were eventually integrated into core, some distinction between the draft and history states would be useful and more intuitive. For example, the "copy on revert" behavior could be slightly different when "reverting to" a draft. It just so happens that node.module's copy behavior works for us. In the future, there could be a clear distinction in node.module between "reverting" to a history state and "publishing" a draft state. It wouldn't change the database schema, but more just clarification in the source of what's happening.

I showed this to a client today, so I guess I've committed to helping out. Clicking the revision and moderation check boxes was the only part where she looked confused...

I'll look into the "Create Draft" feature. I need to study up on hooking the forms, etc... There are a ton of potential features related to this, but a "Create Draft" link would make my clients 99% happy.

But when you revert you're not really submitting a form. The parameters come in from the the URL as it's built on line 1289.

Sorry for the earlier blockquote mess. Not my first time :-)

About the "create draft" tab / link / whatever ..., well, UI.
What I've been trying to achieve so far with the "moderated revision" feature (until now I used the patch in http://drupal.org/node/48731) is this :
IMO this should kept as simple as possible for the user (especially the one whose edits go through moderation, since he is supposedly "less involved" with the site and the admin interface).
'edit' tab / 'submit' button / 'thanks, edits submitted for moderation' message : My user is happy.
No checkboxes to mess with, he does the same as for every other (unmoderated) content he could edit, he is just informed at the right moment of what is going to happen with his contribution.

What happens when the user hits 'Submit' should be decided by his access rights.
When the user has a choice (he has the rights to save as a draft OR as a plain new revision), then maybe the best thing would be to expose him a "Submit as draft" button next to the regular "Submit".

I think this is kind of related to the "access rights" question webchick evoked : default core access rights, or additional rights. I'm still uncertain about that, too...

I'm filing bugs, etc here...
http://drupal.org/project/issues/revision_moderation?categories=bug