From d41ee504caedbe858e24ab2a23c7a804454c64c8 Mon Sep 17 00:00:00 2001 From: erdgeist Date: Sat, 18 Jul 2026 02:15:29 +0200 Subject: Add Trash affordances: cockpit, listing, dashboard entry nodes#show gains a Trash section on trashed nodes: provenance from the trash entry, a restore form whose parent picker pre-fills the old parent while it still lives, and permanent deletion. A Move-to-Trash button joins the status actions on living nodes. nodes#trashed lists trashed subtree roots with weight, provenance, and deletion; the dashboard housekeeping row links to it, and trash/destroy redirect there. Deletion from Trash now removes the whole subtree, deepest first, each node through a real destroy! so every per-node cascade runs -- amending the never-recursive rule for this one sanctioned path (both confirms state the count; the root entry carries destroyed_descendants). Bare Node#destroy still refuses children. --- app/controllers/nodes_controller.rb | 10 ++++-- app/models/node.rb | 43 +++++++++++++++++++++---- app/models/node_action.rb | 2 ++ app/views/admin/index.html.erb | 4 +++ app/views/nodes/show.html.erb | 53 ++++++++++++++++++++++++++++++- app/views/nodes/trashed.html.erb | 42 ++++++++++++++++++++++++ config/routes.rb | 1 + public/javascripts/admin_interface.js | 4 +++ public/javascripts/admin_search.js | 13 ++++++++ public/stylesheets/admin.css | 35 ++++++++++++++++++++ test/controllers/nodes_controller_test.rb | 31 +++++++++++++++--- test/models/node_trash_test.rb | 41 +++++++++++++++++++++--- 12 files changed, 260 insertions(+), 19 deletions(-) create mode 100644 app/views/nodes/trashed.html.erb diff --git a/app/controllers/nodes_controller.rb b/app/controllers/nodes_controller.rb index 9ea66ad..021f8ff 100644 --- a/app/controllers/nodes_controller.rb +++ b/app/controllers/nodes_controller.rb @@ -144,7 +144,7 @@ class NodesController < ApplicationController def trash if @node.trash!(current_user) flash[:notice] = "Page has been moved to the Trash" - redirect_to node_path(Node.trash) + redirect_to trashed_nodes_path else flash[:notice] = "Page is already in the Trash" redirect_to node_path(@node) @@ -170,7 +170,8 @@ class NodesController < ApplicationController def destroy @node.destroy_from_trash!(current_user) flash[:notice] = "Page has been permanently deleted" - redirect_to node_path(Node.trash) + redirect_to trashed_nodes_path + rescue ActiveRecord::RecordInvalid, ActiveRecord::RecordNotDestroyed => e flash[:error] = e.message redirect_to node_path(@node) @@ -247,6 +248,11 @@ class NodesController < ApplicationController @sitemap_descendant_counts = descendant_counts_for(@sitemap) end + def trashed + @nodes = Node.trash.children.order(:slug) + .paginate(:page => params[:page], :per_page => 50) + end + private def slug_for(title) diff --git a/app/models/node.rb b/app/models/node.rb index 4dae287..8d0756a 100644 --- a/app/models/node.rb +++ b/app/models/node.rb @@ -4,6 +4,7 @@ class Node < ApplicationRecord # Associations has_many :pages, -> { order("revision ASC") }, :dependent => :destroy + has_many :node_actions, :dependent => :nullify belongs_to :head, :class_name => "Page", :foreign_key => :head_id, optional: true belongs_to :draft, :class_name => "Page", :foreign_key => :draft_id, optional: true # Autosave pages carry no node_id, so has_many :pages does not cover @@ -351,19 +352,49 @@ class Node < ApplicationRecord end end - # Final deletion. Only from inside the Trash, never with children. - # The log entry is written first, in the same transaction; node_id - # nullifies when the row dies, and the metadata carries what remains. + # Final deletion -- only from inside the Trash. Removes the whole + # subtree, deepest first, each node through a real destroy! so every + # per-node cascade runs (the categorical difference from the old + # delete_all nuke). refuse_destroy_with_children on bare destroy is + # untouched + # One log entry at the root, per the subtree rule, written before the + # rows die. def destroy_from_trash! current_user = nil raise ActiveRecord::RecordInvalid.new(self), "Nodes are only destroyed from the Trash" unless in_trash? ActiveRecord::Base.transaction do - NodeAction.record!(:node => self, :user => current_user, :action => "destroy", - :path => unique_name) - destroy! + doomed = self_and_descendants_ordered_with_level + .sort_by { |_, level| -level } + .map(&:first) + + metadata = { :path => unique_name } + metadata[:destroyed_descendants] = doomed.size - 1 if doomed.size > 1 + + NodeAction.record!(:node => self, :user => current_user, :action => "destroy", **metadata) + doomed.each(&:destroy!) end end + # The most recent trash entry. Its path.from is the restore hint. + def last_trash_entry + node_actions.where(:action => "trash").order(:occurred_at => :desc, :id => :desc).first + end + + # The node's pre-trash parent, if a living node still answers to + # that path. Nil when the parent was itself trashed (its unique_name + # changed) or deleted; a different node that has since taken over + # the path is a legitimate suggestion. + def suggested_restore_parent + from = last_trash_entry&.metadata&.dig("path", "from") + return nil unless from + + parent_path = from.rpartition("/").first + return Node.root if parent_path.empty? + + candidate = Node.find_by(:unique_name => parent_path) + candidate unless candidate.nil? || candidate.in_trash? || candidate.trash_node? + end + # returns an array with all parts of a unique_name rather than a string def unique_path unique_name.to_s.split("/") diff --git a/app/models/node_action.rb b/app/models/node_action.rb index 6e09b5b..13bd5ba 100644 --- a/app/models/node_action.rb +++ b/app/models/node_action.rb @@ -65,6 +65,8 @@ class NodeAction < ApplicationRecord # "destroy" (only from inside the Trash, never with children; the # entry is written in the same transaction before the row dies): # "path" -- final path, flat string (create-symmetric) + # "destroyed_descendants" -- integer, only when positive; one entry + # at the root, per the subtree rule. # # Reserved: "demote" (via "trash" | "depublish") for an explicit # depublish workflow, if ever built. diff --git a/app/views/admin/index.html.erb b/app/views/admin/index.html.erb index 7933cba..319530d 100644 --- a/app/views/admin/index.html.erb +++ b/app/views/admin/index.html.erb @@ -68,5 +68,9 @@ <%= link_to menu_items_path, class: "action_button" do %> <%= icon("menu-2", library: "tabler", "aria-hidden": true) %> Navigation <% end %> + <% trash_count = Node.trash.children.count %> + <%= link_to trashed_nodes_path, class: "action_button" do %> + <%= icon("trash", library: "tabler", "aria-hidden": true) %> Trash<%= " (#{trash_count})" if trash_count > 0 %> + <% end %> diff --git a/app/views/nodes/show.html.erb b/app/views/nodes/show.html.erb index ae25571..4d4f6d5 100644 --- a/app/views/nodes/show.html.erb +++ b/app/views/nodes/show.html.erb @@ -46,7 +46,7 @@ <% end %> <% unless locked_by_other %> - <% if @node.draft && !@node.autosave %> + <% if @node.draft && !@node.autosave && !@node.in_trash? && !@node.trash_node? %>
<%= button_to 'Publish', publish_node_path(@node), method: :put, form: { data: { confirm: "Publish this draft?" }, class: 'button_to state_changing' } %> @@ -62,6 +62,15 @@ <% end %>
<% end %> + <% unless @node.trash_node? || @node.in_trash? || @node.root? %> +
+ <%= button_to trash_node_path(@node), method: :put, + form: { data: { confirm: "Move this page and everything beneath it to the Trash? All published content in it will go offline." }, class: 'button_to destructive' } do %> + <%= icon("trash", library: "tabler", "aria-hidden": true) %> + Move to Trash + <% end %> +
+ <% end %> <% end %> @@ -70,6 +79,48 @@ <% end %> + <% if @node.in_trash? %> +
Trash
+
+
+ <% if (entry = @node.last_trash_entry) && entry.metadata.dig("path", "from") %> +
+ Was at + <%= entry.metadata.dig("path", "from") %> +
+ <% end %> +
+ Restore to + <% suggestion = @node.suggested_restore_parent %> + <%= form_tag restore_from_trash_node_path(@node), :method => :put, :class => "aligned_action_row" do %> +
+ <%= text_field_tag :restore_search_term, + suggestion && (suggestion.head&.title || suggestion.draft&.title || suggestion.slug), + :placeholder => "Search for a new parent…" %> +
+
+ <%= hidden_field_tag :parent_id, suggestion&.id %> + <%= submit_tag "Restore here", :class => "action_button action_button_no_margin_top" %> + <% end %> + Restored pages come back unpublished. Republish each page deliberately. +
+
+
+
+ <% doomed_below = @node.descendants.count %> + <%= button_to node_path(@node), method: :delete, + form: { data: { confirm: doomed_below > 0 ? + "Delete this page and the #{doomed_below} pages beneath it permanently? This cannot be undone." : + "Delete this page permanently? This cannot be undone." }, + class: 'button_to destructive' } do %> + <%= icon("trash-x", library: "tabler", "aria-hidden": true) %> + Delete permanently<%= " (including #{doomed_below} beneath)" if doomed_below > 0 %> + <% end %> +
+
+
+ <% end %> +
Translations
diff --git a/app/views/nodes/trashed.html.erb b/app/views/nodes/trashed.html.erb new file mode 100644 index 0000000..9a4808e --- /dev/null +++ b/app/views/nodes/trashed.html.erb @@ -0,0 +1,42 @@ +

Trash

+ +<% if @nodes.empty? %> +

The Trash is empty.

+<% else %> + <%= will_paginate @nodes %> + + + + + + + + <% @nodes.each do |node| %> + <% entry = node.last_trash_entry %> + + <% doomed_below = node.descendants.count %> + + + + + + <% end %> +
TitleWas atTrashedActions
+ <%= link_to (node.draft&.title || node.slug), node_path(node) %><%= " (+ #{doomed_below} beneath)" if doomed_below > 0 %> + <%= entry&.metadata&.dig("path", "from") %> + <% if entry %> + <%= entry.occurred_at.strftime("%Y-%m-%d %H:%M") %> by <%= entry.actor_name %> + <% end %> + + <%= button_to node_path(node), method: :delete, + form: { data: { confirm: doomed_below > 0 ? + "Delete \"#{node.draft&.title || node.slug}\" and the #{doomed_below} pages beneath it permanently? This cannot be undone." : + "Delete \"#{node.draft&.title || node.slug}\" permanently? This cannot be undone." }, + class: 'button_to destructive' } do %> + <%= icon("trash-x", library: "tabler", "aria-hidden": true) %> + Delete permanently + <% end %> +
+ <%= will_paginate @nodes %> +

To restore a page, open it and pick a new parent in its Trash section.

+<% end %> diff --git a/config/routes.rb b/config/routes.rb index 16145c5..f389326 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -44,6 +44,7 @@ Cccms::Application.routes.draw do get :mine get :chapters get :sitemap + get :trashed end member do diff --git a/public/javascripts/admin_interface.js b/public/javascripts/admin_interface.js index 3988016..c0e2d9c 100644 --- a/public/javascripts/admin_interface.js +++ b/public/javascripts/admin_interface.js @@ -57,6 +57,10 @@ $(document).ready(function () { move_to_search.initialize_search(); } + if ($("#restore_search_term").length != 0) { + restore_search.initialize_search(); + } + if ($("#event_node_search_term").length != 0) { event_search.initialize_search(); } diff --git a/public/javascripts/admin_search.js b/public/javascripts/admin_search.js index d135503..b53c931 100644 --- a/public/javascripts/admin_search.js +++ b/public/javascripts/admin_search.js @@ -262,6 +262,19 @@ move_to_search = { } }; +restore_search = { + initialize_search : function() { + initSearchPicker({ + inputSelector: "#restore_search_term", + resultsSelector: "#restore_search_results", + onSelect: function(node) { + $("#restore_search_term").val(node.title); + $("#parent_id").val(node.node_id); + } + }); + } +}; + event_search = { initialize_search : function() { initSearchPicker({ diff --git a/public/stylesheets/admin.css b/public/stylesheets/admin.css index 82963dd..88fd84b 100644 --- a/public/stylesheets/admin.css +++ b/public/stylesheets/admin.css @@ -884,6 +884,10 @@ form.button_to button[type="submit"] { margin-top: -5px; } +.aligned_action_row .action_button.action_button_no_margin_top { + margin-top: 0; +} + /* ============================================================ Page editor / metadata forms ============================================================ */ @@ -1539,3 +1543,34 @@ div#image_browser ul li { font-size: smaller; color: #777; } + +/* ============================================================ + Trash section + ============================================================ */ + +.restore_form { + display: flex; + align-items: center; + gap: 8px; +} + +.restore_picker { + position: relative; +} + +.restore_picker input[type=text] { + width: 22em; +} + +.restore_picker .search_results { + position: absolute; + top: 100%; + left: 0; + right: 0; + z-index: 10; + background: #fff; + border: 1px solid #989898; + border-top: none; + max-height: 20em; + overflow-y: auto; +} diff --git a/test/controllers/nodes_controller_test.rb b/test/controllers/nodes_controller_test.rb index 7e5a990..6e2ddb3 100644 --- a/test/controllers/nodes_controller_test.rb +++ b/test/controllers/nodes_controller_test.rb @@ -427,11 +427,11 @@ class NodesControllerTest < ActionController::TestCase test "show never renders a destroy link for events" do login_as :quentin node = create_node_with_published_page - Event.create!(node_id: node.id, start_time: Time.now, end_time: Time.now + 1.hour) + event = Event.create!(node_id: node.id, start_time: Time.now, end_time: Time.now + 1.hour) get :show, params: { id: node.id } assert_response :success - assert_select "form.button_to.destructive", count: 0 + assert_select "form[action=?]", event_path(event), count: 0 end test "reverting from nodes#show returns to the show page, not the editor, even if a draft remains" do @@ -479,7 +479,8 @@ class NodesControllerTest < ActionController::TestCase login_as :quentin get :show, params: { :id => node.id } assert_response :success - assert_select "form.destructive", :count => 0 + assert_select "form[action=?]", revert_node_path(node), count: 0 + assert_select "form[action=?]", trash_node_path(node), count: 1 end test "drafts includes a never-published node with only a draft" do @@ -679,7 +680,7 @@ class NodesControllerTest < ActionController::TestCase put :trash, params: { :id => node.id } - assert_redirected_to node_path(Node.trash) + assert_redirected_to trashed_nodes_path assert node.reload.in_trash? end @@ -722,6 +723,26 @@ class NodesControllerTest < ActionController::TestCase delete :destroy, params: { :id => node.id } assert_not Node.exists?(node.id) - assert_redirected_to node_path(Node.trash) + assert_redirected_to trashed_nodes_path + end + + test "trash lists trashed subtree roots" do + login_as :quentin + node = Node.root.children.create!(:slug => "listed_in_trash") + node.trash!(users(:quentin)) + + get :trashed + assert_response :success + assert_select "a[href=?]", node_path(node) + end + + test "trashed rows carry provenance and a delete for childless roots" do + login_as :quentin + node = Node.root.children.create!(:slug => "provenance_test") + node.trash!(users(:quentin)) + + get :trashed + assert_select "td", /quentin/ + assert_select "form[action=?]", node_path(node), count: 1 end end diff --git a/test/models/node_trash_test.rb b/test/models/node_trash_test.rb index b72770b..07e5bc6 100644 --- a/test/models/node_trash_test.rb +++ b/test/models/node_trash_test.rb @@ -102,12 +102,25 @@ class NodeTrashTest < ActiveSupport::TestCase assert Node.exists?(node.id) end - test "destroy_from_trash! refuses nodes with children" do - node = create_node_with_published_page - node.children.create!(:slug => "still_here") - node.trash!(@user1) + test "destroy_from_trash! removes the whole subtree bottom-up with one entry" do + parent = create_node_with_published_page + child = parent.children.create!(:slug => "doomed_child") + grandchild = child.children.create!(:slug => "doomed_grandchild") + page_ids = [parent, child, grandchild].flat_map { |n| n.pages.pluck(:id) } + parent.trash!(@user1) + + assert_difference "NodeAction.count", 1 do + parent.reload.destroy_from_trash!(@user1) + end + + assert_not Node.exists?(parent.id) + assert_not Node.exists?(child.id) + assert_not Node.exists?(grandchild.id) + assert_equal 0, Page.where(:id => page_ids).count - assert_raises(ActiveRecord::RecordNotDestroyed) { node.reload.destroy_from_trash!(@user1) } + action = NodeAction.where(:action => "destroy").last + assert_equal 2, action.metadata["destroyed_descendants"] + assert_nil action.node_id end test "destroy_from_trash! logs an entry that survives the node" do @@ -124,4 +137,22 @@ class NodeTrashTest < ActiveSupport::TestCase assert_equal "Doomed", action.subject_name assert_equal @user1, action.user end + + test "suggested_restore_parent finds the old parent while it lives" do + parent = Node.root.children.create!(:slug => "old_home") + node = parent.children.create!(:slug => "comes_back") + node.trash!(@user1) + + assert_equal parent, node.reload.suggested_restore_parent + + parent.trash!(@user1) + assert_nil node.reload.suggested_restore_parent + end + + test "suggested_restore_parent is root for trashed top-level nodes" do + node = Node.root.children.create!(:slug => "was_top_level") + node.trash!(@user1) + + assert_equal Node.root, node.reload.suggested_restore_parent + end end -- cgit v1.3