Book pages that get updated by non-admins lose their weight and mess up the layout

If I create a couple of book pages and assign them a weight to keep the order ok, it stays that way, even after editing those pages.

If one of the users on my site (who have the "maintain books" permission) edits one of these pages, the weight is set to '0' and the page "bubbles" up...

Cheers,

Max
(Updated the issue (wasn't logged-in when I created it))

I do not have this problem; the pages do not bubble with non-admin book maintainers. A scroll mouse may be the culprit; with the wrong (or right, as the case may be) focus, the scroll mouse can do naughty things to the weight. This is especially possible of heavy 'tabbers'.

We do not support older version. Can you please report back if this issue is still in 4.5? If not, please close the issue.

using 4.5
with maintain books disabled for authenticated users.

authenticated users cannot allocate weights (it doesnt show up in the create book ). Only admin could see the weight drop down.

When editing the book ，all weight items for non book nodes are set to 0. 0, messing up any previously set order。

BY my test:

drupal 4.5.2 and cvs

4.5.2=>cvs

I can confirm this same trouble for version 4.6.2 as well. Anyone have a fix? I seem to have the same trouble with "page" nodes and "book page" nodes.

The problem is that my users with "maintain books" privilege are not shown the weight pulldown menu. When they submit edits or changes to book pages the weight is reset to zero.

I will look into it.

This still exists in version 4.6.5 with book nodes. I haven't tried it any other way, I was just noticing the behavior as I was updating a book.

I haven't determined where the weight gets reset to 0, yet, but I was able to hack the book.module so that it didn't write the wrong value to the database. If anyone cares, I can create a patch, but basically I changed:

/**
 * Implementation of hook_update().
 */
function book_update($node) {
  db_query("UPDATE {book} SET parent = %d, weight = %d, log = '%s' WHERE nid = %d", $node->parent, $node->weight, $node->log, $node->nid);
}

to

/**
 * Implementation of hook_update().
 */
function book_update($node) {
  if (user_access('administer nodes')) {
    db_query("UPDATE {book} SET parent = %d, weight = %d, log = '%s' WHERE nid = %d", $node->parent, $node->weight, $node->log, $node->nid);
  }
  else {
    db_query("UPDATE {book} SET parent = %d, log = '%s' WHERE nid = %d", $node->parent, $node->log, $node->nid);
  }
}

I think the correct fix is to find where in the code it is resetting the weight and correct it. If I find it, I'll post a patch. If someone can give me a pointer as to where to look, that would be helpful, as it's not currently very obvious to me.

After reading through code and docs and messing around a bit on my test site, I believe that the culprit is the $node->weight = 0; in the following snippet:

/**
 * Implementation of hook_validate().
 */
function book_validate(&$node) {
  // Set default values for non-administrators.
  if (!user_access('administer nodes')) {
    $node->weight = 0;
    $node->revision = 1;
  }
}

Anyone care to comment?

I would just remove it, but I don't know if it's needed for something else. If it's not, I can create and submit a patch removing that line. If it is needed, I can create a patch adding the lines in my previous comment.

It seems I was wrong about my edits to book_validate(). I was back in that module today, doing several edits to four pages w/a specific order, and by the time I was done, they were all set back to 0. I went back to trying my first hack to book.module in the book_update() function, and did some tests and the pages stayed where they were supposed to. Maybe I'll have to try it again in a few days to make sure nothing changes.

Oadae, thanks for this ~ I too am gaving the problem in 4.7.2

I see a solution is to also give the user "administer nodes" rights, which I don't want to do as it then enables them to configure content types etc..?

Is your first workaround the best way to do this or is there a patch elsewhere that I haven't managed to find yet?

I actually haven't revisited this issue since moving to 4.7. It's possible that the code has changed so that it won't look exactly the same, but if the weight is still being set, a similar fix to that in comment #11 will stop that behavior. I will try to look at it again later this week and verify the results.

This issue applies also to CVS

In book_submit() the node weight is always set to 0 when non-admins update a book page :

function book_submit(&$node) {
  global $user;
  // Set default values for non-administrators.
  if (!user_access('administer nodes')) {
    $node->weight = 0;
    $node->revision = 1;

This might be right for new pages, but not for existing ones.
In book_form() we see also :

function book_form(&$node) {

// ....

  if (user_access('administer nodes')) {
    $form['weight'] = array('#type' => 'weight',
      '#title' => t('Weight'),
      '#default_value' => $node->weight,
      '#delta' => 15,
      '#weight' => 5,
      '#description' => t('Pages at a given level are ordered first by weight and then by title.'),
    );
  }
  else {
    // If a regular user updates a book page, we create a new revision
    // authored by that user:
    $form['revision'] = array('#type' => 'hidden', '#value' => 1);
  }

IMO it would be cleaner to remove $node->weight = 0; from book_submit() and set the appropriate value for node->weight in book_form()
I also think that setting an hidden value for revision in book_form() is useless, because it is overridden afterwards in node_form_array()
I would propose the following changes:

function book_submit(&$node) {
  global $user;
  // Set default values for non-administrators.
  if (!user_access('administer nodes')) {
    $node->revision = 1;




function book_form(&$node) {

// ....

  if (user_access('administer nodes')) {

    // ....

  else {
    // If a regular user updates a book page, we preserve the node weight; otherwise
    // we use 0 as the default for new pages
    $form['weight'] = array('#type' => 'value', '#value' => (isset($node->weight) ? $node->weight : 0));
  }

I have rolled a patch against HEAD, please review it.
Thanks,
Riccardo

Rerolled #16 -- (not Unix line ending)
Sorry for that

Some things I noticed just looking at the patch (not actually applying it):
* You have tabs in front of the comments.
* It looks as though you've added an extra closing brace (}).
* There appear to be 2 extra spaces in front of the new $from statement.

@Oadae: Many thanks. The patch was actually malformed.

Here is the right one as described in #16

Didn't actually test this, but as far as I can see from the patch it modifies the behavior to not generate a new revision if a regular user edits a book page. That doesn't seem correct to me.

A new revision is always generated on any drupal site when non-admins update book pages.

function book_submit(&$node) {
  global $user;
  // Set default values for non-administrators.
  if (!user_access('administer nodes')) {
    $node->weight = 0;
    $node->revision = 1;

The patch at #19 does not change this behavior at all. It only fixes the issue about node weight.
I tested it on 4.7.2 and HEAD and it works.

The additional change in book_form() is just a little optimization :
IMO to store an hidden value in the form in order to force a new revision for non-admins was useless, because it is always triggered in book_submit().

I have thoroughly tested my patch at #19 and IMO it works well:

- Node weight is preserved when non-admins (users without "administer nodes" permission) update a book page.

- New book pages created by non-admins get a default node weight of 0.

- Users with "administer nodes" permission can modify node weights as they please.

- I could not see any change about revision control with respect to a clean drupal installation.

I am sorry if the modification about revision control has caused any misunderstanding.
While I was debugging the patch I saw that the line below is redundant and I deleted it :
$form['revision'] = array('#type' => 'hidden', '#value' => 1);
Anyway, it does not concern the node weight issue and can be safely removed from the patch.

Thanks for any comment/suggestion
Riccardo

riccardo I have been using your patch thanks, I don't have enough knowledge to critique it, but it's working for my sites purposes :)

Admins may still have to add pages to the right place in outline but at least that can be done in one place and then only once, then they are preserved no matter who edits. Seems the best way for now!

thanks Again

Committed to HEAD.