Titles of book pages can be lost while modifing a book structure
| Project: | Drupal |
| Version: | 6.x-dev |
| Component: | book.module |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed |
Let me describe a problem:
I have opened a 2 sites pages:
1st: admin/content/book/234 (this is a structure of new book)
2nd: node/137/outline (This is a top level book page)
Then in 2nd window I set new parent of this book page to page with id=234 (the same as I see in 1st window).
I have saved a changes in 2nd window and switched to window 1.
There is no changes visible in window 1 because I didn't refresh a page (Just forgot) so I didn't saw a new subpages...
I have changed a title of one of the pages and saved changes.
Page was reloaded and I have seen a book pages without titles!
(To fix this: switch to previous version of page)
Who I think It should work:
I pages was not listed at the book's structure page (admin/content/book/234) book.module shouldn't change a title.
OR
Just write a warning like "structure was modified by other user... You should reload page"
I'm using this module a lot and want to say "Thanks for a great module".
#1
Right - it sounds like this is a bug in the form submit handler on the admin screen. It should be fixed in 7.x first. I'm not sure how we should handle the lockout. Maybe the hash of the menu tree?
#2
Here's a starting patch - saves the hash of the tree structure as a hidden value and compares it to the current value. Please test, and comment on the error message.
note- I found a serious bug in book at the same time: http://drupal.org/node/276846
#3
for comparison the error message node module throws is:
form_set_error('changed', t('This content has been modified by another user, changes cannot be saved.'));#4
I think we should keep this consistent with node.module, so once the message is changed I'll RTBC - I testred and it works as advertised.
#5
#6
Ok, revised the error text and added another code comment.
#7
Tested, works well, and patch makes sense.
#8
Committed to CVS HEAD. Thanks.
#9
same bug is in 6.x
#10
oops - patch above fails on 6.x (last hunk doesn't apply) - not sure why. Here's a re-roll that applies cleanly to 6.x. Re-tested too.
#11
Thanks, committed to 6.x.
#12
Automatically closed -- issue fixed for two weeks with no activity.