Refuse destroying nodes that still have children

NestedTree's before_destroy silently delete_all'd the whole
subtree, bypassing every per-node cleanup. Nodes are never
destroyed recursively; descendants must be removed individually.

3 files changed, 50 insertions, 11 deletions

diff --git a/app/models/concerns/nested_tree.rb b/app/models/concerns/nested_tree.rb
index 9a2eb0e..befcdb3 100644
--- a/app/models/concerns/nested_tree.rb
+++ b/app/models/concerns/nested_tree.rb
@@ -9,8 +9,6 @@ module NestedTree
   included do
     belongs_to :parent, class_name: name, foreign_key: :parent_id, optional: true, inverse_of: :children
     has_many :children, -> { order(:id) }, class_name: name, foreign_key: :parent_id, inverse_of: :parent
-
-    before_destroy :delete_descendants
   end
 
   class_methods do
@@ -77,10 +75,4 @@ module NestedTree
   def move_to_child_of(new_parent)
     update!(parent_id: new_parent.id)
   end
-
-  private
-
-  def delete_descendants
-    self.class.where(id: descendants.pluck(:id)).delete_all
-  end
 end
diff --git a/app/models/node.rb b/app/models/node.rb
index 70ed4da..6ce6591 100644
--- a/app/models/node.rb
+++ b/app/models/node.rb
@@ -12,9 +12,10 @@ class Node < ApplicationRecord
   belongs_to  :lock_owner, :class_name => "User", :foreign_key => :locking_user_id, optional: true
 
   # Callbacks
-  after_create  :initialize_empty_page
-  before_save   :check_for_changed_slug
-  after_save    :update_unique_names_of_children
+  after_create   :initialize_empty_page
+  before_save    :check_for_changed_slug
+  after_save     :update_unique_names_of_children
+  before_destroy :refuse_destroy_with_children
 
   # Validations
   validates_length_of     :slug, :within => 1..255,    :unless => -> { parent_id.nil? || slug.blank? }
@@ -366,6 +367,15 @@ class Node < ApplicationRecord
     )
   end
 
+  # Nodes are never destroyed recursively
+  # Descendants must be removed or reparented individually first.
+  # The Trash feature will be the ordinary path to deletion.
+  def refuse_destroy_with_children
+    return unless children.exists?
+    errors.add(:base, "Cannot destroy a node that still has children")
+    throw :abort
+  end
+
   def self.recently_changed
     includes(:head).where(
       "pages.updated_at < ? AND pages.updated_at > ? AND nodes.parent_id IS NOT NULL",
diff --git a/test/models/node_test.rb b/test/models/node_test.rb
index 4401651..9c4834e 100644
--- a/test/models/node_test.rb
+++ b/test/models/node_test.rb
@@ -770,4 +770,41 @@ class NodeTest < ActiveSupport::TestCase
     node.default_template_name = "standard_template"
     assert node.valid?
   end
+
+  test "destroying a node with children is refused" do
+    parent = Node.root.children.create!(:slug => "destroy_guard_parent")
+    parent.children.create!(:slug => "destroy_guard_child")
+
+    assert_no_difference "Node.count" do
+      assert_not parent.destroy
+    end
+    assert parent.errors[:base].any?
+  end
+
+  test "destroying a childless node leaves no orphaned pages or asset links" do
+    node = create_node_with_published_page
+    page_ids = node.pages.pluck(:id)
+    asset = Asset.create!(:name => "destroy cascade probe",
+                          :upload_file_name    => "test_image.png",
+                          :upload_content_type => "image/png",
+                          :upload_file_size    => 49854,
+                          :upload_updated_at   => Time.current)
+    node.head.related_assets.create!(:asset_id => asset.id, :position => 1)
+
+    node.destroy
+
+    assert_equal 0, Page.where(:id => page_ids).count
+    assert_equal 0, RelatedAsset.where(:page_id => page_ids).count
+  end
+
+  test "destroying a node also removes its autosave page" do
+    node = create_node_with_published_page
+    node.lock_for_editing!(@user1)
+    node.autosave!({:title => "in flight"}, @user1)
+    autosave_id = node.autosave_id
+
+    node.destroy
+
+    assert_equal 0, Page.where(:id => autosave_id).count
+  end
 end