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.

2 files changed, 13 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",