Unexpected behaviour: deleting a page removes entire menu branch

[afdev82]

Steps to reproduce

Create a menu branch in this way

• Node 1: linked to Page 1
  • Node 2

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

• Alchemy Version: v6.0.0
• Rails Version: v6.1.5

---

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):

• Page 1
  • Page 2 (published)
  • Page 3 (published)
  • ...

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:

• Node 1 (linked to Page 2)
  • Node 2 (linked to Page 2)
  • Node 3 (linked to Page 3)
  • ...
    • ...

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 with ON 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!

[tvdeyen]

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

validates :name, presence: true, if: -> { page.nil? }

1. We could copy the page name to the node before_destroy of the page
2. We could add a check to the page before_destroy and return an error if the page is still attached to a node.
3. We should nullify the foreign_key instead of deleting the foreign record

[afdev82]

I'm not sure I've got completely what should be the behavior if I remove a page with nodes attached to it.

1. Remove the page
2. In the before_destroy of the page, copy the name of the page to the node
3. nullify the foreign_key of the node to the page

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 will provide a PR for that soon, maybe it's better to get the code first.

[tvdeyen]

I think the easiest is to add a before_destroy hook into the page model and return an error message, like we do with languages and sites.

alchemy_cms/app/models/alchemy/language.rb

Lines 57 to 60 in 5418846

	before_destroy if: -> { pages.any? } do
	errors.add(:pages, :still_present)
	throw(:abort)
	end

alchemy_cms/app/models/alchemy/site.rb

Lines 26 to 29 in 5418846

	before_destroy if: -> { languages.any? } do
	errors.add(:languages, :still_present)
	throw(:abort)
	end

I think also we should change the foreign key to nullify on delete. To avoid accidental removals of nodes in the database. WDYT?

[afdev82]

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.
To delete the page, the user should manually remove the page from the node and then delete it.
It's more work to do for the user, but it's more transparent. Usually, there should not be many nodes for one page.

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 don't know if we are using this pattern somewhere else in Alchemy and since most users probably don't read alerts carefully, it's better to abort.

[mamhoff]

I actually think we should be setting the foreign key to restrict, mirroring the behavior of the Ruby models. There could be a manual "dereference" button that converts a page's foreign key to string foreign keys if necessary, but I think magically removing whole trees of nodes is... uncalled for :). The nullify foreign key constraint would also not set the URL path on the node, would it?

[afdev82]

I agree with you, in fact restrict would be more appropriate in this case.

[github-actions]

This issue has not seen any activity in a long time.
Please create a pull request with a fix or ask someone of the community if they can help.
This issue will be closed in 7 days if no further activity happens.

[afdev82]

Still relevant... I've opened the PR #2338, but I was not able to fix the new tests I've added.
I don't know why they are failing. In the browser it's ok.