-
-
Notifications
You must be signed in to change notification settings - Fork 315
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Unexpected behaviour: deleting a page removes entire menu branch #2301
Comments
Urgh, that's bad. I agree that we should fix that. One thing to keep in mind though is that the node becomes invalid if we remove the page and do not have a name anymore. alchemy_cms/app/models/alchemy/node.rb Line 23 in 025e011
|
I'm not sure I've got completely what should be the behavior if I remove a page with nodes attached to it.
Then why is it necessary to check if the page is still attached to the node in the before_destroy? I think it would be still attached, because it's not deleted yet and the foreign key is not nullified yet. Or am I missing something here? |
I think the easiest is to add a alchemy_cms/app/models/alchemy/language.rb Lines 57 to 60 in 5418846
alchemy_cms/app/models/alchemy/site.rb Lines 26 to 29 in 5418846
I think also we should change the foreign key to nullify on delete. To avoid accidental removals of nodes in the database. WDYT? |
Yes, the foreign key is necessary in the DB, as you already said. I'm working on that. So the idea here is just to avoid to delete the page if it's still linked to a node. Another idea was to add a confirm alert and then copy the name of the page to the nodes and delete the page if the user accepts to continue. |
I actually think we should be setting the foreign key to |
I agree with you, in fact |
…e if a page is still linked to nodes, AlchemyCMS#2301
…e if a page is still linked to nodes, AlchemyCMS#2301
This issue has not seen any activity in a long time. |
Still relevant... I've opened the PR #2338, but I was not able to fix the new tests I've added. |
Add foreign key on nodes to restrict on delete page_id and add a warning on delete if a page is still linked to nodes. Closes #2301
Add foreign key on nodes to restrict on delete page_id and add a warning on delete if a page is still linked to nodes. Closes #2301
Add foreign key on nodes to restrict on delete page_id and add a warning on delete if a page is still linked to nodes. Closes AlchemyCMS#2301
Steps to reproduce
Create a menu branch in this way
Delete Page 1 from the Page tree.
Expected behavior
The page reference should be deleted, but not the node itself, leaving the menu structure like it was.
Actual behavior
The Node 1 gets deleted and the children too, removing the complete menu branch under the node.
System configuration
In my case I had the "redirect to public child" of old Alchemy versions active and I've just reproduced the feature with the new menu nodes structure. I had a structure like that in the Pages tree (I report just a fragment of it):
In the menu, I replicated that structure and to be able to click on Node 1 (Page 1) and go to Page 2, I linked the Node 1 to Page 2:
I was doing some changes on the Page Tree and I deleted the Page 2... After that I found that my entire menu branch under Node 1 was gone! This is counter-intuitive and very dangerous in my case, because I didn't expect that and now I have to recreate the complete structure. Although I understand the reason for this behavior (deleting a node that is referencing a page), I think I more conservative way should be to just delete the link from the Node or give the user the option to delete also the nodes.
In my opinion the menu structure should not be touched if I change something in the Page Tree.
After digging into the source code, I've found the reason for that, there is a foreign key on the
page_id
withON DELETE CASCADE
.In my app I can easily recreate the foreign key to "fix" this issue for the future, but I wanted to know your thoughts about that.
Thank you for your support!
The text was updated successfully, but these errors were encountered: