Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pwolanin’s picture

Status: Active » Fixed

Go to the Outline tab. Hit "Remove from book outline". This is never allowed for the top-level node - hmm, I think that's a bug though, since you should be able to remove a top-level node (i.e. delete the book) if it has no children.

marcp’s picture

Title: Allow pages to be removed from books » Allow top-level pages to be removed from books

Thanks, Peter - I hadn't looked on the Outline tab. When it didn't appear in the Book outline fieldset on the Edit tab I assumed there was no way to do this.

Changing the title to reflect the current issue, which is that a top-level page can't be turned into a regular node without belonging to a book.

marcp’s picture

Status: Fixed » Active

Re-opening this.

pwolanin’s picture

Category: feature » bug

The bug is thus: a top-level node (a.k.a a book) should be able to be removed from the outline if (and only if) it has no children. I think the current code is preventing this even when there are no children. In the absence of a "remove" button the top level node should ideally also get some message explaining the reason why.

This is mostly a "bug" as opposed to a feature, so changing Category.

gpk’s picture

Subs, bump! (also present in 6.5)

gpk’s picture

There is actually a (slightly obtuse) workaround, provided that you aren't trying to remove the *only* top-level book node. Simply move the top-level node you want to remove into another book...

catch’s picture

Just ran into this.

pwolanin’s picture

Status: Active » Needs review
FileSize
2.27 KB

Sorry this fell off my radar - it's certainly a bug and the fix should be backported to 6 too

---
Work: Acquia

catch’s picture

Patch works well for me. Also applies to D6 with fuzz.

gpk’s picture

@8: that was PDQ!

Removing single extra space on line 13 of patch file.

Status: Needs review » Needs work

The last submitted patch failed testing.

gpk’s picture

fgm’s picture

Still there in latest head, almost 1 year later.

liquid06’s picture

Experiencing this issue in version 6.16

couloir007’s picture

As of June 19, 2010 it still there. This is critical for me.

pwolanin’s picture

@couloir007 - please test the patch against Drupal 7 and see what needs to be done.

catch’s picture

Status: Needs work » Needs review
pwolanin’s picture

oh, yay - looks like the fail before was testobt wonkiness.

catch’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me, very small fix for very annoying bug.

aspilicious’s picture

Tests?

japanitrat’s picture

subscribe

sun’s picture

Status: Reviewed & tested by the community » Needs work
+++ modules/book/book.module	17 Dec 2008 01:17:13 -0000
@@ -174,7 +174,17 @@ function _book_outline_access($node) {
+    ¶

Very odd trailing white-space here.

+++ modules/book/book.module	17 Dec 2008 01:17:13 -0000
@@ -174,7 +174,17 @@ function _book_outline_access($node) {
+ * Helper function - determine if a node can be removed from the book.

Please drop the "Helper function - " prefix.

"Determine*s* if a node can be removed from *a* book"

is sufficient.

+++ modules/book/book.module	17 Dec 2008 01:17:13 -0000
@@ -174,7 +174,17 @@ function _book_outline_access($node) {
+ * A node can only be removed if it's actually in a book, and not a top-level
+ * page, or it is a top-level page with no children.

wow, had to read this 3 times to get it. The additionally appended condition really makes it hard to grok.

"A node may only be removed, if it is actually in a book and not the root node of a book, and if it has no child pages."

That said - is that last condition really the case? I was under the impression that books are based on menus, and the menu system already implements re-parenting logic for child links...? Is this patch valid after all?

Powered by Dreditor.

nclavaud’s picture

Actually the request is :
"A node may only be removed if it is actually in a book and, in case it is the root node of a book, if it has no child pages."

This patch looks good to me.

However, a description string should be added for that case in book_remove_form(), saying something like "This is a top-level page, you are going to erase the book itself".

mlncn’s picture

Priority: Normal » Major

Bumping to major: There is no workaround; once a node is attached to an outline, you cannot get it out.

If there isn't a new patch rolled in 48 hours, i'll be doing that; otherwise, i'll be reviewing :-)

mlncn’s picture

Status: Needs work » Needs review
FileSize
910 bytes

And here is the updated patch, with Sun's comments addressed.

The patch is valid, tested and it works.

(Aside: i needed this patch because non-book nodes were being forced into books when non-admins created them... can't find anyone else having that issue, and maybe it's a ghost from one of the dev->alpha->beta upgrades in my case.)

benjamin, agaric

mlncn’s picture

Let's try a patch with *both* files included...

sun’s picture

Status: Needs review » Needs work
+++ modules/book/book.pages.inc
@@ -207,8 +207,9 @@ function book_remove_form($form, &$form_state, $node) {
+  if (_book_node_is_removable($node)) {
+    // Only allowed when this is not a book (top-level page),
+    // or for a top-level page with no children.

Let's remove this inline comment. The phpDoc of the helper function itself is sufficient.

Powered by Dreditor.

mlncn’s picture

Status: Needs work » Needs review
FileSize
1.83 KB

And here you are!

sun’s picture

Priority: Major » Normal
Status: Needs review » Reviewed & tested by the community

Thanks, looks good now.

catch’s picture

Priority: Normal » Major
Status: Reviewed & tested by the community » Needs review

Still looks good, this is a tiny change and the bug itself while very annoying isn't dangerous, so I'm 50/50 whether we need tests before it can go in - coverage would be nice but it doesn't feel like something that will easily regress.

catch’s picture

Status: Needs review » Reviewed & tested by the community

crossposted.

webchick’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

I'd like to see tests for this. Those access checks are just convoluted enough that someone might try and "simplify" them later.

rootwork’s picture

Subscribe -- just ran into this and would love to see it get in. Don't think I know enough about tests to help, unfortunately, but will be watching to see when it gets committed.

sun’s picture

Version: 7.x-dev » 8.x-dev
Priority: Major » Normal
Issue tags: +Needs backport to D7

Not being able to remove the top-level node from a book can't be a major bug in any way.

However, it seems like we're 99% there. Anyone able to add the required tests to get this done?

Caffeine Addict’s picture

Was wondering where this functionalitly was... Applied the last patch above and it works fine. Tested in Drupal 7.7.
Definatly needed, if there is any further testing to be done i'll be happy to help.

gpk’s picture

@35: this needs test to be written per #32 and included in the patch. Can you do that?

Jody Lynn’s picture

Status: Needs work » Needs review
FileSize
3.22 KB

Here it is with a test to try out.

Status: Needs review » Needs work

The last submitted patch, 283045.patch, failed testing.

Jody Lynn’s picture

FileSize
3.88 KB

Adding permission to administer book outlines to the test admin_user to give the user access to the test deletions.

Jody Lynn’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: -Needs tests, -Needs backport to D7

The last submitted patch, 283045.patch, failed testing.

Jody Lynn’s picture

Status: Needs work » Needs review

#39: 283045.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Needs tests, +Needs backport to D7

The last submitted patch, 283045.patch, failed testing.

RedRat’s picture

Issue tags: +Needs backport to D6

More than three years for such a obvious bug! Oh, Drupal...

sun’s picture

The one who re-rolls #39 against latest core gets an RTBC from me.

(Should be a mere /core directory change, so simply apply the patch from within /core, and re-create it from root.)

pwolanin’s picture

Status: Needs work » Needs review
FileSize
4.08 KB
2.26 KB

Re-rolled the patch for 8, fixed conflict in the test hunk. The test wasn't working - did it ever?

There was maybe a change in the book test helper code, so the calls in the test case needed to be re-ordered so the admin user was actually logged in.

Also, $nodes[0] had children, so it couldn't be used to test removal, and you need to POST to the confirm form not GET, and the screen text had changed, so I altered this to actually load the node and confirm that's it's no longer in a book.

Attached a test-only patch and full patch.

sun’s picture

Status: Needs review » Reviewed & tested by the community

Thanks! RTBC on the assumption that #46 comes back green.

no_commit_credit’s picture

FileSize
4.08 KB

Attached is #46 but with one additional s (for summary verb form).

Status: Reviewed & tested by the community » Needs work
Issue tags: -Needs backport to D6, -Needs tests, -Needs backport to D7

The last submitted patch, 283045-48.patch, failed testing.

oriol_e9g’s picture

Status: Needs work » Needs review
Issue tags: +Needs backport to D6, +Needs tests, +Needs backport to D7

#48: 283045-48.patch queued for re-testing.

oriol_e9g’s picture

Status: Needs review » Reviewed & tested by the community
catch’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)
Issue tags: -Needs tests

Alright. Committed/pushed to 8.x. Moving to 7.x for backport.

Albert Volkman’s picture

Status: Patch (to be ported) » Needs review
FileSize
3.92 KB

D7 backport. So not to change D7's API, I just replaced the _book_node_is_removable() function calls with the function's body. Is this the proper way to backport a new function? If not, let me know and I'll gladly re-roll.

BenStallings’s picture

Status: Needs review » Reviewed & tested by the community

Thanks, Albert! Works as advertised.

RedRat’s picture

Is there any chance to see it backported to 6.x?

gpk’s picture

@55: yes, once it's been committed to 7.x, if someone creates a patch for 6.x and if others give it the necessary review attention. Perhaps you would be able to help with some of this?

RedRat’s picture

@56: Well, I can thoroughly test proposed patch for 6.x on my site with a lot of books, but my PHP skills are too weak to make a decent patch. :-(

catch’s picture

Status: Reviewed & tested by the community » Needs work

@Albert Volkman, since that's an entirely new function, it's OK to backport it as is, the chances of a contrib module having a function with the same name a very close to zero.

The main thing we don't do with backports is change parameters (although occasionally optional parameters could be added) or change return values.

Albert Volkman’s picture

Status: Needs work » Needs review
FileSize
4.09 KB

Cool, thanks for the info catch. Re-rolled using the new function.

rootwork’s picture

Status: Needs review » Reviewed & tested by the community

This works for me...sorry if one review isn't enough for RTBC, but since little was changed in the backport and this is an ages-old issue with no update in the past few months, I thought I'd be bold and see what happens next.

aspilicious’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs backport to D6, +Needs backport to D7

The last submitted patch, books_allow_top_level_removal-283045-d7-59.patch, failed testing.

Albert Volkman’s picture

Status: Needs work » Needs review
FileSize
4.02 KB

Thanks @rootwork!

Re-rolled against current head.

rootwork’s picture

Status: Needs review » Reviewed & tested by the community

Woooo.

David_Rothstein’s picture

Version: 7.x-dev » 6.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed to 7.x - thanks! http://drupalcode.org/project/drupal.git/commit/e2bab36

Moving to 6.x for backport.

Albert Volkman’s picture

Status: Patch (to be ported) » Needs review
FileSize
1.9 KB

D6 backport.

RedRat’s picture

I have tested patch from #66 on mine Drupal 6.26 installation, it works fine. Also, I have checked DB consistence after top-level books removing, there are no errors found. I think, this patch ready to go into the next release.

peterx’s picture

Status: Needs review » Reviewed & tested by the community

A use case/test not mentioned above. The changes to pages appear to work in Drupal 7 when the page is published but you cannot do the same for an unpublished page. If you create a new page and try to change it to a book when unpublished, it will be added to a different book. The same sort of problem occurs when you try to change book or remove the page from a book. If the page is unpublished, you cannot do what you want or the results are wrong. This makes the process of creating and editing an unpublished page a pain. The testing for this issue should include both published and unpublished pages.

My temporary workaround for some changes is to quickly publish the page then change the book outline then unpublish the page.

mgifford’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Closed (won't fix)

It's been RTBC for 2 years, but don't see this getting into Core for D6.

David_Rothstein’s picture

Version: 6.x-dev » 7.x-dev
Status: Closed (won't fix) » Fixed

Probably better to move back to the last fixed version in that case?

Status: Fixed » Closed (fixed)

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