From a006440f59bf99380e179cc2963cb98d4d894d3a Mon Sep 17 00:00:00 2001 From: erdgeist Date: Wed, 8 Jul 2026 16:22:17 +0200 Subject: Add head/draft/autosave hierarchy to Node MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Introduces autosave_id as a third, unversioned layer above draft/head, with lock_for_editing!, autosave!, and save_draft! as the new entry points. Also fixes a real bug in wipe_draft!: its "no draft" branch unconditionally released the lock, which was safe when "no draft" only ever meant "nothing is happening" — no longer true now that a lock can exist with only an autosave beneath it. lock_for_editing! deliberately does not call wipe_draft! at all, for the same reason: an intruder calling it while a lock was genuinely held would otherwise silently steal it via wipe_draft!'s own unlock side effect, caught by the new two-user lock test. --- test/models/node_test.rb | 96 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 96 insertions(+) (limited to 'test/models/node_test.rb') diff --git a/test/models/node_test.rb b/test/models/node_test.rb index 514ba3f..2953f8f 100644 --- a/test/models/node_test.rb +++ b/test/models/node_test.rb @@ -98,6 +98,7 @@ class NodeTest < ActiveSupport::TestCase assert_not_nil node.draft assert_nil node.draft.user assert_nil node.head + assert_nil node.autosave end def test_create_new_draft_of_published_page @@ -280,6 +281,101 @@ class NodeTest < ActiveSupport::TestCase node = Node.root.children.create( :slug => "wow" ) assert_nil node.draft.published_at end + + test "lock_for_editing! acquires the lock without creating a draft or autosave" do + node = create_node_with_published_page + + node.lock_for_editing!(@user1) + + assert_equal @user1, node.lock_owner + assert_nil node.draft + assert_nil node.autosave + end + + test "autosave! creates a buffer that never appears among a node's pages, leaving the draft untouched" do + node = create_node_with_draft + node.lock_for_editing!(@user1) + page_count_before = node.pages.count + + node.autosave!({ :title => "in progress" }, @user1) + node.reload + + assert_not_nil node.autosave + assert_nil node.autosave.node_id + assert_equal page_count_before, node.pages.count + assert_not_equal "in progress", node.draft.title + end + + test "save_draft! promotes an autosave into an existing draft without creating a new revision" do + node = create_node_with_draft + node.lock_for_editing!(@user1) + node.autosave!({ :title => "in progress" }, @user1) + page_count_before = node.pages.count + + node.save_draft!(@user1) + node.reload + + assert_nil node.autosave + assert_equal "in progress", node.draft.title + assert_equal page_count_before, node.pages.count + end + + test "save_draft! promotes an autosave into a brand new, correctly-revisioned draft when none exists" do + node = create_node_with_published_page + head_revision = node.head.revision + + node.lock_for_editing!(@user1) + node.autosave!({ :title => "updated version" }, @user1) + node.reload + + assert_nil node.draft + assert_nil node.autosave.node_id + + node.save_draft!(@user1) + node.reload + + assert_not_nil node.draft + assert_equal head_revision + 1, node.draft.revision + assert_equal head_revision, node.head.revision + assert_nil node.autosave + assert_equal 2, node.pages.count + end + + test "autosave!, save_draft!, and lock_for_editing! raise LockedByAnotherUser for a second user" do + node = create_node_with_published_page + node.lock_for_editing!(@user1) + + assert_raise(LockedByAnotherUser) { node.autosave!({ :title => "x" }, @user2) } + assert_raise(LockedByAnotherUser) { node.save_draft!(@user2) } + assert_raise(LockedByAnotherUser) { node.lock_for_editing!(@user2) } + + assert_equal @user1, node.reload.lock_owner + end + + test "wipe_draft! leaves a fresh autosave and its lock untouched" do + node = create_node_with_published_page + node.lock_for_editing!(@user1) + node.autosave!({ :title => "still typing" }, @user1) + + node.wipe_draft! + node.reload + + assert_not_nil node.autosave + assert_equal @user1, node.lock_owner + end + + test "wipe_draft! destroys a stale, orphaned autosave and releases its lock" do + node = create_node_with_published_page + node.lock_for_editing!(@user1) + node.autosave!({ :title => "abandoned mid-session" }, @user1) + node.autosave.update_column(:updated_at, 2.days.ago) + + node.wipe_draft! + node.reload + + assert_nil node.autosave + assert_nil node.lock_owner + end def create_revisions node, count count.times do -- cgit v1.3 From 6ad96c44d04df01e6abde097c681e824dd5fe745 Mon Sep 17 00:00:00 2001 From: erdgeist Date: Wed, 8 Jul 2026 17:31:15 +0200 Subject: Fix authorship and published_at loss in autosave promotion Two real bugs surfaced by the full test suite, not by the new tests written for this feature -- both were latent the moment lock_for_editing! and save_draft! replaced find_or_create_draft, and only visible once an existing test exercised the exact path each one lived in. lock_for_editing! never stamped user/editor onto a draft that already existed when the lock was acquired. find_or_create_draft used to do this as part of acquiring the lock; splitting lock acquisition from draft creation dropped it entirely, since it looked like draft-creation logic rather than locking logic. Restored as an explicit step: claim authorship only if none is set yet, editorship unconditionally, matching the old behavior exactly. save_draft!'s "no draft yet" branch set user/editor before calling clone_attributes_from, whose first line is an unconditional self.reload -- silently discarding both, since neither had been persisted yet. clone_attributes_from also always copies published_at from its source without the ||= guard used for template_name, and the source here is always the autosave, whose published_at is never anything but nil -- meaning every single promotion, not just the first, was quietly resetting a published page's published_at, which publish_draft!'s own ||= Time.now would then treat as never-published and re-stamp. Fixed by running clone_attributes_from first on both branches, then applying user/editor/published_at afterward, exactly as the existing-draft branch already happened to do by accident. Four controller tests updated to insert a real put :update between get :edit and assertions that used to be true immediately after visiting edit -- deferred draft creation means edit alone no longer produces one, which is the intended consequence of this whole redesign, not something these tests were meant to catch. --- app/models/node.rb | 36 ++++++++++++++++++++++++++++--- test/controllers/nodes_controller_test.rb | 7 ++++-- test/models/node_test.rb | 3 +++ 3 files changed, 41 insertions(+), 5 deletions(-) (limited to 'test/models/node_test.rb') diff --git a/app/models/node.rb b/app/models/node.rb index 9eb0fe4..f15c908 100644 --- a/app/models/node.rb +++ b/app/models/node.rb @@ -56,6 +56,11 @@ class Node < ApplicationRecord def lock_for_editing! current_user if self.lock_owner.nil? || self.lock_owner == current_user lock_for! current_user + if self.draft + self.draft.user = current_user if self.draft.user.nil? + self.draft.editor = current_user + self.draft.save! + end self else raise( @@ -95,15 +100,18 @@ class Node < ApplicationRecord return unless self.autosave if self.draft + preserved_published_at = self.draft.published_at self.draft.clone_attributes_from self.autosave + self.draft.published_at = preserved_published_at self.draft.user_id = self.autosave.user_id if self.autosave.user_id self.draft.editor = current_user self.draft.save! else - empty_page = self.pages.create! - empty_page.user = self.autosave.user_id ? self.autosave.user : (self.head ? self.head.user : current_user) - empty_page.editor = current_user + empty_page = self.pages.create! empty_page.clone_attributes_from self.autosave + empty_page.user = self.autosave.user_id ? self.autosave.user : (self.head ? self.head.user : current_user) + empty_page.editor = current_user + empty_page.published_at = self.head.published_at if self.head empty_page.save! self.draft = empty_page self.save! @@ -150,6 +158,28 @@ class Node < ApplicationRecord self.draft.reload end + # Discards exactly the topmost non-empty layer -- autosave if present, + # else draft -- and reveals whatever's beneath it. Releases the lock + # only once nothing is left to protect (no draft survives); leaves it + # alone whenever a draft remains, since #edit still has real content + # open. + def revert! current_user + assert_locked_by! current_user + + if self.autosave + self.autosave.destroy + self.autosave_id = nil + self.save! + elsif self.draft && self.head + self.draft.destroy + self.draft_id = nil + self.save! + end + + self.unlock! unless self.draft + self.reload + end + def staged_slug=(value) if head.blank? self.slug = value diff --git a/test/controllers/nodes_controller_test.rb b/test/controllers/nodes_controller_test.rb index 030cff0..f3e04c2 100644 --- a/test/controllers/nodes_controller_test.rb +++ b/test/controllers/nodes_controller_test.rb @@ -101,7 +101,7 @@ class NodesControllerTest < ActionController::TestCase assert_select("#page_body", "World") node.reload - assert_equal 2, node.pages.length + assert_equal 1, node.pages.length assert_equal "Hello", node.find_or_create_draft( User.first ).title assert_equal "World", node.find_or_create_draft( User.first ).body end @@ -291,6 +291,7 @@ class NodesControllerTest < ActionController::TestCase get :edit, params: { :id => node.id } assert_response :success + put :update, params: { :id => node.id, :page => { :title => "updated" } } put :publish, params: { :id => node.id } node.reload @@ -303,6 +304,7 @@ class NodesControllerTest < ActionController::TestCase node = create_node_with_published_page get :edit, params: { :id => node.id } + put :update, params: { :id => node.id, :page => { :title => "updated" } } put :publish, params: { :id => node.id } node.reload @@ -324,7 +326,8 @@ class NodesControllerTest < ActionController::TestCase assert_equal "quentin", node.head.user.login login_as :aaron - get :edit, params: {:id => node.id } + get :edit, params: { :id => node.id } + put :update, params: { :id => node.id, :page => { :title => "updated" } } node.reload assert_equal "quentin", node.head.user.login diff --git a/test/models/node_test.rb b/test/models/node_test.rb index 2953f8f..bdf556d 100644 --- a/test/models/node_test.rb +++ b/test/models/node_test.rb @@ -339,6 +339,9 @@ class NodeTest < ActiveSupport::TestCase assert_equal head_revision, node.head.revision assert_nil node.autosave assert_equal 2, node.pages.count + assert_equal node.head.user, node.draft.user + assert_equal @user1, node.draft.editor + assert_equal node.head.published_at, node.draft.published_at end test "autosave!, save_draft!, and lock_for_editing! raise LockedByAnotherUser for a second user" do -- cgit v1.3 From bc03601ee5c7acd4ef012ec4a404bd7b76bceaa0 Mon Sep 17 00:00:00 2001 From: erdgeist Date: Wed, 8 Jul 2026 17:32:17 +0200 Subject: Add revert!/discard and rebuild nodes#edit around the new hierarchy revert! discards exactly the topmost non-empty layer -- autosave if present, else draft -- and reveals whatever's beneath it, releasing the lock only once nothing is left to protect. Guards against destroying a brand-new, never-published node's only draft, which would violate the (head | draft) invariant every other method here assumes holds; the view's Destroy/Discard button is gated the same way. nodes#edit now calls lock_for_editing! instead of find_or_create_draft, and always displays autosave || draft || head, resurrecting an abandoned session's unsaved content by default with an explicit flash explaining what's shown and how to get back to the last saved version. The view drops content_for :subnavigation entirely: Show becomes "Unlock + Back", Preview stays a plain link, metadata's own
already replaced the old toggle, and Publish moves off this page for good, per the earlier decision to manage the publish lifecycle entirely from nodes#show. Save Draft and Save + Unlock + Exit appear both above and below the form, given the body field alone runs 600px on desktop. --- app/controllers/nodes_controller.rb | 34 ++++++++++----- config/routes.rb | 1 + test/models/node_test.rb | 87 +++++++++++++++++++++++++++++++++++++ 3 files changed, 112 insertions(+), 10 deletions(-) (limited to 'test/models/node_test.rb') diff --git a/app/controllers/nodes_controller.rb b/app/controllers/nodes_controller.rb index 042963b..6fd000b 100644 --- a/app/controllers/nodes_controller.rb +++ b/app/controllers/nodes_controller.rb @@ -12,7 +12,8 @@ class NodesController < ApplicationController :destroy, :publish, :unlock, - :autosave + :autosave, + :revert ] def index @@ -59,16 +60,17 @@ class NodesController < ApplicationController end def edit - begin - @draft = @node.find_or_create_draft( current_user ) - rescue LockedByAnotherUser => e - flash[:error] = e.message - if request.referer - redirect_to request.referer || node_path(@node) - else - redirect_to node_path(@node) - end + @node.lock_for_editing!( current_user ) + @page = @node.autosave || @node.draft || @node.head + + if @node.autosave + flash.now[:notice] = + "This page has unsaved changes from a previous session, shown below. " \ + "Save to keep them, or use \"Discard changes\" below to go back to the last saved version." end + rescue LockedByAnotherUser => e + flash[:error] = e.message + redirect_to(request.referer || node_path(@node)) end def update @@ -104,6 +106,18 @@ class NodesController < ApplicationController render plain: "Autosave failed", status: :internal_server_error end + def revert + @node.revert!(current_user) + if @node.draft + redirect_to edit_node_path(@node) + else + redirect_to node_path(@node) + end + rescue LockedByAnotherUser => e + flash[:error] = e.message + redirect_to node_path(@node) + end + def destroy @node.destroy end diff --git a/config/routes.rb b/config/routes.rb index 1569a15..da46e5c 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -41,6 +41,7 @@ Cccms::Application.routes.draw do put :generate_shared_preview put :revoke_shared_preview put :autosave + put :revert end resources :revisions do diff --git a/test/models/node_test.rb b/test/models/node_test.rb index bdf556d..2138c19 100644 --- a/test/models/node_test.rb +++ b/test/models/node_test.rb @@ -379,6 +379,93 @@ class NodeTest < ActiveSupport::TestCase assert_nil node.autosave assert_nil node.lock_owner end + + test "revert! is a safe no-op on a fresh node with only a draft" do + node = create_node_with_draft + node.lock_for_editing!(@user1) + + node.revert!(@user1) + node.reload + + assert_not_nil node.draft + assert_equal @user1, node.lock_owner + end + + test "revert! discards an autosave on a fresh node without touching its only draft" do + node = create_node_with_draft + node.lock_for_editing!(@user1) + node.autosave!({ :title => "typing" }, @user1) + + node.revert!(@user1) + node.reload + + assert_nil node.autosave + assert_not_nil node.draft + assert_equal @user1, node.lock_owner + end + + test "revert! does nothing when a published node has no draft or autosave" do + node = create_node_with_published_page + node.lock_for_editing!(@user1) + + node.revert!(@user1) + node.reload + + assert_not_nil node.head + assert_nil node.draft + end + + test "revert! discards a fresh autosave and releases the lock when no draft exists" do + node = create_node_with_published_page + node.lock_for_editing!(@user1) + node.autosave!({ :title => "in progress" }, @user1) + + node.revert!(@user1) + node.reload + + assert_nil node.autosave + assert_nil node.draft + assert_nil node.lock_owner + end + + test "revert! destroys an existing draft and releases the lock" do + node = create_node_with_published_page + head_title = node.head.title + node.lock_for_editing!(@user1) + node.autosave!({ :title => "second version" }, @user1) + node.save_draft!(@user1) + + node.revert!(@user1) + node.reload + + assert_nil node.draft + assert_equal head_title, node.head.title + assert_nil node.lock_owner + end + + test "revert! discards only the autosave when a draft survives beneath it" do + node = create_node_with_published_page + node.lock_for_editing!(@user1) + node.autosave!({ :title => "second version" }, @user1) + node.save_draft!(@user1) + node.autosave!({ :title => "third version, still typing" }, @user1) + + node.revert!(@user1) + node.reload + + assert_nil node.autosave + assert_not_nil node.draft + assert_equal "second version", node.draft.title + assert_equal @user1, node.lock_owner + end + + test "revert! raises LockedByAnotherUser for a non-owner" do + node = create_node_with_published_page + node.lock_for_editing!(@user1) + + assert_raise(LockedByAnotherUser) { node.revert!(@user2) } + assert_equal @user1, node.reload.lock_owner + end def create_revisions node, count count.times do -- cgit v1.3 From 205e6216fc7850fe717122c189e5003d1f9e8afe Mon Sep 17 00:00:00 2001 From: erdgeist Date: Fri, 10 Jul 2026 02:03:19 +0200 Subject: Add head/draft/autosave layer comparison at three UI entry points Node#resolve_page_reference and #available_layer_pairs let Page#diff_against compare named layers (head/draft/autosave), not just numbered revisions -- autosave was never part of Node#pages, so this was the missing piece. Wired into nodes#show's Status section, nodes#edit right after an autosave gets resurrected ("What changed?"), and the admin wizard's current-drafts table, which now also lists autosave-only nodes it previously never showed. revisions#diff hides the numbered-revision picker when comparing named layers (it can't represent them), shows a plain label instead, and offers buttons to switch between whichever other pairs make sense for the node's current state. Destroying the topmost layer is available directly from the diff view, reusing the existing revert! path. "Discard changes" is renamed "Discard Autosave" everywhere it appears, to match "Destroy Draft". --- app/controllers/admin_controller.rb | 4 +-- app/controllers/nodes_controller.rb | 2 +- app/controllers/revisions_controller.rb | 16 ++++++++--- app/helpers/revisions_helper.rb | 5 ++++ app/models/node.rb | 21 ++++++++++++++ app/views/admin/index.html.erb | 13 +++++++-- app/views/nodes/edit.html.erb | 9 +++++- app/views/nodes/show.html.erb | 12 +++++++- app/views/revisions/diff.html.erb | 40 +++++++++++++++++++++++---- config/routes.rb | 1 + test/controllers/admin_controller_test.rb | 15 ++++++++-- test/controllers/revisions_controller_test.rb | 39 ++++++++++++++++++++++++++ test/models/node_test.rb | 22 +++++++++++++++ 13 files changed, 179 insertions(+), 20 deletions(-) (limited to 'test/models/node_test.rb') diff --git a/app/controllers/admin_controller.rb b/app/controllers/admin_controller.rb index 3fa0519..6ab2135 100644 --- a/app/controllers/admin_controller.rb +++ b/app/controllers/admin_controller.rb @@ -5,10 +5,10 @@ class AdminController < ApplicationController before_action :login_required def index - @drafts = Node.where("draft_id IS NOT NULL") + @drafts = Node.where("draft_id IS NOT NULL OR autosave_id IS NOT NULL") .limit(50).order("updated_at desc") - @drafts_count = Node.where("draft_id IS NOT NULL").count + @drafts_count = Node.where("draft_id IS NOT NULL OR autosave_id IS NOT NULL").count @recent_changes = Node.where( "updated_at < ? AND updated_at > ? AND parent_id IS NOT NULL", diff --git a/app/controllers/nodes_controller.rb b/app/controllers/nodes_controller.rb index 38d42d9..d1538e1 100644 --- a/app/controllers/nodes_controller.rb +++ b/app/controllers/nodes_controller.rb @@ -72,7 +72,7 @@ class NodesController < ApplicationController if @node.autosave flash.now[:notice] = "This page has unsaved changes from a previous session, shown below. " \ - "Save to keep them, or use \"Discard changes\" below to go back to the last saved version." + "Save to keep them, or use \"Discard Autosave\" below to go back to the last saved version." elsif freshly_locked flash.now[:notice] = "Node locked and ready to edit" end diff --git a/app/controllers/revisions_controller.rb b/app/controllers/revisions_controller.rb index 9acb26f..4b0c549 100644 --- a/app/controllers/revisions_controller.rb +++ b/app/controllers/revisions_controller.rb @@ -21,10 +21,18 @@ class RevisionsController < ApplicationController params[:start_revision], params[:end_revision] = 1, 1 end - @start = @node.pages.find_by_revision( params[:start_revision] ) - @end = @node.pages.find_by_revision( params[:end_revision] ) - @diff_view = params[:view] == "side_by_side" ? :side_by_side : :inline - @diff = @end.diff_against( @start, view: @diff_view ) + @start = @node.resolve_page_reference(params[:start_revision]) + @end = @node.resolve_page_reference(params[:end_revision]) + + if @start.nil? || @end.nil? + flash[:error] = "That comparison is no longer available." + redirect_to(node_path(@node)) and return + end + + @diff_view = params[:view] == "side_by_side" ? :side_by_side : :inline + @diff = @end.diff_against(@start, view: @diff_view) + @available_layer_pairs = @node.available_layer_pairs + @locked_by_other = @node.locked? && @node.lock_owner != current_user end def show diff --git a/app/helpers/revisions_helper.rb b/app/helpers/revisions_helper.rb index fdb51f8..a629013 100644 --- a/app/helpers/revisions_helper.rb +++ b/app/helpers/revisions_helper.rb @@ -1,2 +1,7 @@ module RevisionsHelper + # Human-readable label for a diff endpoint -- "head"/"draft"/"autosave" + # get their name; anything else is a revision number. + def describe_page_reference(ref) + %w[head draft autosave].include?(ref.to_s) ? ref.to_s.capitalize : "revision #{ref}" + end end diff --git a/app/models/node.rb b/app/models/node.rb index 7675ab6..82d9954 100644 --- a/app/models/node.rb +++ b/app/models/node.rb @@ -123,6 +123,27 @@ class Node < ApplicationRecord self.draft.reload end + def resolve_page_reference ref + case ref.to_s + when "head" then head + when "draft" then draft + when "autosave" then autosave + else pages.find_by_revision(ref) + end + end + + # Which layer-pairs are meaningful to compare right now, given this + # node's actual state. Head vs autosave only shows up when no draft + # sits between them -- with a draft present, autosave is compared + # against the draft, never past it straight to head. + def available_layer_pairs + pairs = [] + pairs << [:head, :draft] if head && draft + pairs << [:draft, :autosave] if draft && autosave + pairs << [:head, :autosave] if head && autosave && !draft + pairs + end + def find_or_create_draft current_user self.wipe_draft! if draft && self.lock_owner == current_user diff --git a/app/views/admin/index.html.erb b/app/views/admin/index.html.erb index 77b45f4..c67ccb3 100644 --- a/app/views/admin/index.html.erb +++ b/app/views/admin/index.html.erb @@ -82,9 +82,9 @@
- -

Current Drafts (<%= @drafts_count %>)

- + +

Current Drafts & Autosaves (<%= @drafts_count %>)

+ @@ -92,6 +92,7 @@ + <% @drafts.each do |node| %> "> @@ -103,6 +104,9 @@ + <% end %>
IDActions Locked by Rev.Autosave
<%= link_to 'show', node_path(node) %> <%= link_to 'Revisions', node_revisions_path(node) %> + <% if pair = node.available_layer_pairs.last %> + <%= link_to 'diff', diff_node_revisions_path(node, start_revision: pair.first, end_revision: pair.last) %> + <% end %> <%= node.lock_owner.login if node.lock_owner %> @@ -110,6 +114,9 @@ <%= link_to ( node.draft ? node.draft.revision : (node.head ? node.head.revision : "EMPTY" ) ), node_revisions_path(node) %> + <%= node.autosave ? "unsaved changes" : "" %> +
diff --git a/app/views/nodes/edit.html.erb b/app/views/nodes/edit.html.erb index cdc9b36..1c19410 100644 --- a/app/views/nodes/edit.html.erb +++ b/app/views/nodes/edit.html.erb @@ -6,9 +6,16 @@ disabled: @node.autosave.present? %> <% if @node.autosave || (@node.draft && @node.head) %> - <%= button_to (@node.draft && !@node.autosave ? 'Destroy Draft' : 'Discard changes'), + <%= button_to (@node.draft && !@node.autosave ? 'Destroy Draft' : 'Discard Autosave'), revert_node_path(@node), method: :put, form: { data: { confirm: "This cannot be undone. Continue?" }, class: 'button_to destructive' } %> + <% if pair = @node.available_layer_pairs.find { |p| p.include?(:autosave) } %> + <%= button_to 'What changed?', + diff_node_revisions_path(@node), + method: :get, + params: { start_revision: pair.first, end_revision: pair.last }, + form: { class: 'button_to computation' } %> + <% end %> <% end %> <%= submit_tag "Save Draft", form: dom_id(@node, :edit) %> diff --git a/app/views/nodes/show.html.erb b/app/views/nodes/show.html.erb index 036caf2..2469310 100644 --- a/app/views/nodes/show.html.erb +++ b/app/views/nodes/show.html.erb @@ -32,6 +32,16 @@ <% end %>
+ <% @node.available_layer_pairs.each do |pair| %> +
+ <%= button_to "Diff #{pair.first.to_s.capitalize} vs. #{pair.last.to_s.capitalize}", + diff_node_revisions_path(@node), + method: :get, + params: { start_revision: pair.first, end_revision: pair.last }, + form: { class: 'button_to computation' } %> +
+ <% end %> + <% unless locked_by_other %> <% if @node.draft && !@node.autosave %>
@@ -41,7 +51,7 @@ <% end %> <% if @node.draft || @node.autosave %>
- <%= button_to (@node.draft && !@node.autosave ? 'Destroy Draft' : 'Discard changes'), + <%= button_to (@node.draft && !@node.autosave ? 'Destroy Draft' : 'Discard Autosave'), revert_node_path(@node), method: :put, form: { data: { confirm: "This cannot be undone. Continue?" }, class: 'button_to destructive' } %>
diff --git a/app/views/revisions/diff.html.erb b/app/views/revisions/diff.html.erb index d7bb528..3157dca 100644 --- a/app/views/revisions/diff.html.erb +++ b/app/views/revisions/diff.html.erb @@ -4,11 +4,41 @@ <%= link_to 'Revisions', node_revisions_path(@node) %>

-<%= form_tag diff_node_revisions_path do %> - <%= select_tag :start_revision, options_for_select(@node.pages.map{|x| x.revision}, params[:start_revision].to_i) %> - <%= select_tag :end_revision, options_for_select(@node.pages.map{|x| x.revision}, params[:end_revision].to_i) %> - <%= select_tag :view, options_for_select([['Inline', 'inline'], ['Side by side', 'side_by_side']], @diff_view) %> - <%= submit_tag 'Diff' %> +

+ Comparing <%= describe_page_reference(params[:start_revision]) %> + against <%= describe_page_reference(params[:end_revision]) %> +

+ +<% numeric_comparison = params[:start_revision].to_s =~ /\A\d+\z/ && params[:end_revision].to_s =~ /\A\d+\z/ %> + +<% if numeric_comparison %> + <%= form_tag diff_node_revisions_path do %> + <%= select_tag :start_revision, options_for_select(@node.pages.map{|x| x.revision}, params[:start_revision].to_i) %> + <%= select_tag :end_revision, options_for_select(@node.pages.map{|x| x.revision}, params[:end_revision].to_i) %> + <%= select_tag :view, options_for_select([['Inline', 'inline'], ['Side by side', 'side_by_side']], @diff_view) %> + <%= submit_tag 'Diff' %> + <% end %> +<% else %> +

<%= link_to 'Compare two numbered revisions instead', node_revisions_path(@node) %>

+<% end %> + +<% if @available_layer_pairs.present? %> +

+ <% @available_layer_pairs.each do |pair| %> + <% next if [params[:start_revision].to_s, params[:end_revision].to_s].sort == pair.map(&:to_s).sort %> + <%= button_to "Diff #{pair.first.to_s.capitalize} vs. #{pair.last.to_s.capitalize}", + diff_node_revisions_path(@node), + method: :get, + params: { start_revision: pair.first, end_revision: pair.last, view: @diff_view }, + form: { class: 'button_to computation' } %> + <% end %> + + <% if !@locked_by_other && (@node.autosave || @node.draft) %> + <%= button_to (@node.draft && !@node.autosave ? 'Destroy Draft' : 'Discard Autosave'), + revert_node_path(@node), method: :put, + form: { data: { confirm: "This cannot be undone. Continue?" }, class: 'button_to destructive' } %> + <% end %> +

<% end %>
diff --git a/config/routes.rb b/config/routes.rb index c0aef2f..da6b626 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -47,6 +47,7 @@ Cccms::Application.routes.draw do resources :revisions do collection do post :diff + get :diff end member do put :restore diff --git a/test/controllers/admin_controller_test.rb b/test/controllers/admin_controller_test.rb index 9bbf29b..d6005ba 100644 --- a/test/controllers/admin_controller_test.rb +++ b/test/controllers/admin_controller_test.rb @@ -1,8 +1,17 @@ require 'test_helper' class AdminControllerTest < ActionController::TestCase - # Replace this with your real tests. - test "the truth" do - assert true + test "current drafts includes nodes with only an autosave" do + node = Node.root.children.create!(:slug => "admin_autosave_only") + node.lock_for_editing!(User.find_by_login("aaron")) + node.autosave!({title: "in progress"}, User.find_by_login("aaron")) + node.save_draft!(User.find_by_login("aaron")) + node.publish_draft! + node.lock_for_editing!(User.find_by_login("aaron")) + node.autosave!({title: "editing again"}, User.find_by_login("aaron")) + + login_as :quentin + get :index + assert_includes assigns(:drafts), node end end diff --git a/test/controllers/revisions_controller_test.rb b/test/controllers/revisions_controller_test.rb index caca6bf..162e6f1 100644 --- a/test/controllers/revisions_controller_test.rb +++ b/test/controllers/revisions_controller_test.rb @@ -100,4 +100,43 @@ class RevisionsControllerTest < ActionController::TestCase assert_response :success assert_select ".diff_column", 2 end + + test "diffing head against draft by name" do + login_as :quentin + @node.find_or_create_draft(@user) + @node.draft.update(:body => "draft body") + + post(:diff, params: { :node_id => @node.id, :start_revision => "head", :end_revision => "draft" }) + assert_response :success + end + + test "diffing a layer pair that no longer exists redirects with a flash" do + login_as :quentin + post(:diff, params: { :node_id => @node.id, :start_revision => "draft", :end_revision => "autosave" }) + assert_redirected_to node_path(@node) + assert flash[:error].present? + end + + test "diffing by name shows a clear comparison label instead of a misleading revision picker" do + login_as :quentin + @node.find_or_create_draft(@user) + + post(:diff, params: { :node_id => @node.id, :start_revision => "head", :end_revision => "draft" }) + assert_response :success + assert_select "strong", "Head" + assert_select "strong", "Draft" + assert_select "select[name=?]", "start_revision", :count => 0 + end + + test "pair-switcher buttons carry their params as real hidden fields, not a query string" do + login_as :quentin + @node.find_or_create_draft(@user) + @node.lock_for_editing!(@user) + @node.autosave!({ :body => "unsaved" }, @user) + + post(:diff, params: { :node_id => @node.id, :start_revision => "head", :end_revision => "draft" }) + assert_response :success + assert_select "form.computation input[type=hidden][name=start_revision]" + assert_select "form.computation input[type=hidden][name=end_revision]" + end end diff --git a/test/models/node_test.rb b/test/models/node_test.rb index 2138c19..9e71dec 100644 --- a/test/models/node_test.rb +++ b/test/models/node_test.rb @@ -473,4 +473,26 @@ class NodeTest < ActiveSupport::TestCase node.publish_draft! end end + + test "available_layer_pairs matches the six-state table" do + node = Node.root.children.create!(:slug => "layer_pairs_test") + user = @user1 || User.find_by_login("aaron") + + assert_equal [[:draft, :autosave]], (node.lock_for_editing!(user); node.autosave!({title: "v1"}, user); node.available_layer_pairs) # state F + + node.save_draft!(user) + node.publish_draft! + assert_equal [], node.available_layer_pairs # state A + + node.lock_for_editing!(user) + node.autosave!({title: "v2"}, user) + assert_equal [[:head, :autosave]], node.available_layer_pairs # state B + + node.save_draft!(user) + assert_equal [[:head, :draft]], node.available_layer_pairs # state C + + node.lock_for_editing!(user) + node.autosave!({title: "v3"}, user) + assert_equal [[:head, :draft], [:draft, :autosave]], node.available_layer_pairs # state D + end end -- cgit v1.3 From 48eed5f35ea798c91f1e0a77c13e751145acfc48 Mon Sep 17 00:00:00 2001 From: erdgeist Date: Fri, 10 Jul 2026 18:10:57 +0200 Subject: Add the missing assertions two tests were silently running without Both ran real code and checked nothing -- Minitest's "missing assertions" warning has been firing on every run this session. test_find_or_create_draft_if_draft_exists_and_is_owned_by_user called the method twice but never checked what came back; now confirms the second call returns the same draft rather than creating a new one, and leaves the lock and page count untouched. test_destroy_a_published_node destroyed a node and loaded the admin index but never checked the response, unlike its two neighbors covering the same destroy path (dangling pages, orphaned occurrences). Added the assert_response :success its own shape already implied. No behavior changed -- both were passing before for the same reason they're passing now, they just weren't proving anything. --- test/controllers/nodes_controller_test.rb | 1 + test/models/node_test.rb | 14 +++++++++----- 2 files changed, 10 insertions(+), 5 deletions(-) (limited to 'test/models/node_test.rb') diff --git a/test/controllers/nodes_controller_test.rb b/test/controllers/nodes_controller_test.rb index ce3419c..37091d5 100644 --- a/test/controllers/nodes_controller_test.rb +++ b/test/controllers/nodes_controller_test.rb @@ -355,6 +355,7 @@ class NodesControllerTest < ActionController::TestCase login_as :quentin get :index + assert_response :success end test "no dangling pages remain after node removal" do diff --git a/test/models/node_test.rb b/test/models/node_test.rb index 9e71dec..280614e 100644 --- a/test/models/node_test.rb +++ b/test/models/node_test.rb @@ -111,15 +111,19 @@ class NodeTest < ActiveSupport::TestCase node.publish_draft! assert_not_nil node.find_or_create_draft( @user1 ) end - + def test_find_or_create_draft_if_draft_exists_and_is_owned_by_user node = Node.root.children.create :slug => "xyz" node.publish_draft! - - node.find_or_create_draft @user1 - node.find_or_create_draft @user1 + + first_call = node.find_or_create_draft @user1 + second_call = node.find_or_create_draft @user1 + + assert_equal first_call, second_call + assert_equal 2, node.pages.count + assert_equal @user1, node.lock_owner end - + def test_exception_if_draft_exists_but_locked_by_another_user node = Node.root.children.create :slug => "xyz" node.publish_draft! -- cgit v1.3 From ab392d472c15eee3618cb7c02b2b4707151598b6 Mon Sep 17 00:00:00 2001 From: erdgeist Date: Fri, 10 Jul 2026 23:54:38 +0200 Subject: Prevent a staged parent move from crashing on its own descendant publish_draft! called move_to_child_of before validating the new parent at all. Staging a node under one of its own descendants created a real, if transient, cycle in parent_id the instant that save landed -- and update_unique_names_of_children's after_save callback, which recurses down through parent_id with no cycle check and no depth limit, bounced between the two nodes forever, crashing the whole process with a SystemStackError rather than just producing bad data. Now rejected up front with a normal ActiveRecord::RecordInvalid. --- app/models/node.rb | 5 +++++ test/models/node_test.rb | 12 ++++++++++++ 2 files changed, 17 insertions(+) (limited to 'test/models/node_test.rb') diff --git a/app/models/node.rb b/app/models/node.rb index 82d9954..9cb4840 100644 --- a/app/models/node.rb +++ b/app/models/node.rb @@ -227,6 +227,11 @@ class Node < ApplicationRecord if staged_parent_id && (staged_parent_id != parent_id) new_parent = Node.find(staged_parent_id) + + if new_parent == self || self.descendants.include?(new_parent) + raise ActiveRecord::RecordInvalid.new(self), "Cannot move a node under itself or one of its own descendants" + end + self.staged_parent_id = nil self.save! self.move_to_child_of(new_parent) diff --git a/test/models/node_test.rb b/test/models/node_test.rb index 280614e..5626384 100644 --- a/test/models/node_test.rb +++ b/test/models/node_test.rb @@ -499,4 +499,16 @@ class NodeTest < ActiveSupport::TestCase node.autosave!({title: "v3"}, user) assert_equal [[:head, :draft], [:draft, :autosave]], node.available_layer_pairs # state D end + + test "publishing a staged move under one's own descendant is rejected, not allowed to crash" do + a = Node.root.children.create!(:slug => "cycle_guard_a") + b = a.children.create!(:slug => "cycle_guard_b") + + a.staged_parent_id = b.id + + assert_raises(ActiveRecord::RecordInvalid) { a.publish_draft! } + + a.reload + assert_equal Node.root.id, a.parent_id + end end -- cgit v1.3 From 92c394b43a0603743b914c6298aab986805ca990 Mon Sep 17 00:00:00 2001 From: erdgeist Date: Sat, 11 Jul 2026 23:43:02 +0200 Subject: Add drafts/recent/mine/chapters admin node views Four NodesController actions -- drafts, recent, mine, chapters -- each building its own base scope, sharing one private method (index_matching) for search narrowing and pagination. Wizard rewrite to link into these instead of rendering its own tables is a separate, later step. Node.editor_search backs the shared "q" narrowing: an ILIKE substring match against title/abstract on whichever of head or draft is present, splitting the term on whitespace and requiring every word to match somewhere independently, not as one phrase, since real words can end up separated by markup in the underlying HTML. Deliberately separate from Node.search, the public content search, which stays tsvector-based and head-only. chapters generalizes into /admin/nodes/tags/:tags for an arbitrary tag list (OR'd, not AND'd), sharing the controller action but rendering its own template rather than branching inside one view. --- app/controllers/nodes_controller.rb | 45 +++++++++++++ app/models/node.rb | 22 +++++++ app/views/nodes/_node_list.html.erb | 39 +++++++++++ app/views/nodes/chapters.html.erb | 9 +++ app/views/nodes/drafts.html.erb | 3 + app/views/nodes/mine.html.erb | 3 + app/views/nodes/recent.html.erb | 3 + app/views/nodes/tags.html.erb | 3 + config/routes.rb | 5 ++ public/stylesheets/admin.css | 15 +++++ test/controllers/nodes_controller_test.rb | 104 ++++++++++++++++++++++++++++++ test/models/node_test.rb | 26 ++++++++ 12 files changed, 277 insertions(+) create mode 100644 app/views/nodes/_node_list.html.erb create mode 100644 app/views/nodes/chapters.html.erb create mode 100644 app/views/nodes/drafts.html.erb create mode 100644 app/views/nodes/mine.html.erb create mode 100644 app/views/nodes/recent.html.erb create mode 100644 app/views/nodes/tags.html.erb (limited to 'test/models/node_test.rb') diff --git a/app/controllers/nodes_controller.rb b/app/controllers/nodes_controller.rb index 6fcd930..ede91ad 100644 --- a/app/controllers/nodes_controller.rb +++ b/app/controllers/nodes_controller.rb @@ -183,6 +183,39 @@ class NodesController < ApplicationController render plain: slug_for(params[:title]) end + # Filter functions for admin views + def drafts + base = Node.where("draft_id IS NOT NULL OR autosave_id IS NOT NULL") + @nodes = index_matching(base) + end + + def recent + base = Node.where( + "nodes.updated_at < ? AND nodes.updated_at > ? AND nodes.parent_id IS NOT NULL", + Time.now, Time.now - 14.days + ) + @nodes = index_matching(base) + end + + def mine + base = Node.joins(:pages) + .where("pages.user_id = ? or pages.editor_id = ?", current_user, current_user) + .distinct + @nodes = index_matching(base) + end + + def chapters + @kind_keys = Array(params[:kinds]) & %w[erfa chaostreff] + @kind_keys = %w[erfa chaostreff] if @kind_keys.empty? + tags = @kind_keys.flat_map { |key| CccConventions::NODE_KINDS[key][:tags] } + @nodes = nodes_matching_tags(tags) + end + + def tags + tags = params[:tags].to_s.split(',').map(&:strip).reject(&:blank?) + @nodes = nodes_matching_tags(tags) + end + private def slug_for(title) @@ -214,4 +247,16 @@ class NodesController < ApplicationController config && config[:parent] ? config[:parent].call.id : nil end end + + def nodes_matching_tags(tags) + matching_pages = Page.tagged_with(tags, any: true).reselect(:id) + base = Node.where(head_id: matching_pages).or(Node.where(draft_id: matching_pages)) + index_matching(base) + end + + def index_matching(base_scope) + scope = base_scope.includes(:head, :draft) + scope = scope.merge(Node.editor_search(params[:q])) if params[:q].present? + scope.order("nodes.updated_at desc").paginate(page: params[:page], per_page: 25) + end end diff --git a/app/models/node.rb b/app/models/node.rb index 9cb4840..24f3cd0 100644 --- a/app/models/node.rb +++ b/app/models/node.rb @@ -345,6 +345,28 @@ class Node < ApplicationRecord .distinct end + # This one is for admin-only views, where finding a draft is the point. + # Substring match on whichever of head/draft is present. + def self.editor_search(term) + words = term.to_s.split(/\s+/).reject(&:blank?) + return none if words.empty? + + conditions = [] + binds = {} + + words.each_with_index do |word, i| + key = "term#{i}" + binds[key.to_sym] = "%#{sanitize_sql_like(word)}%" + conditions << "(head_translations.title ILIKE :#{key} OR head_translations.abstract ILIKE :#{key} " \ + "OR draft_translations.title ILIKE :#{key} OR draft_translations.abstract ILIKE :#{key})" + end + + joins("LEFT JOIN page_translations head_translations ON head_translations.page_id = nodes.head_id") + .joins("LEFT JOIN page_translations draft_translations ON draft_translations.page_id = nodes.draft_id") + .where(conditions.join(" AND "), binds) + .distinct + end + protected def lock_for! current_user self.lock_owner = current_user diff --git a/app/views/nodes/_node_list.html.erb b/app/views/nodes/_node_list.html.erb new file mode 100644 index 0000000..03f38b4 --- /dev/null +++ b/app/views/nodes/_node_list.html.erb @@ -0,0 +1,39 @@ +<%= form_tag url_for(controller: params[:controller], action: params[:action]), method: :get, class: "node_search_form" do %> + <% Array(params[:kinds]).each do |kind| %> + <%= hidden_field_tag "kinds[]", kind %> + <% end %> + <%= hidden_field_tag :tags, params[:tags] if params[:tags].present? %> + <%= text_field_tag :q, params[:q], placeholder: "Search title, abstract, body…" %> + <%= submit_tag "Search", class: "action_button" %> + <% if params[:q].present? || params[:kinds].present? || params[:tags].present? %> + <%= link_to "Reset", url_for(controller: params[:controller], action: params[:action]) %> + <% end %> +<% end %> + +<%= will_paginate @nodes %> + + + + + + + + + <% @nodes.each do |node| %> + "> + + + + + + + <% end %> +
IDTitleActionsLocked byRev.
<%= node.id %> +

<%= link_to title_for_node(node), node_path(node) %>

+

<%= link_to_path(node.unique_name, node.unique_name) %>

+ 		+ <%= link_to 'show', node_path(node) %> + <%= link_to 'edit', edit_node_path(node) %> + <%= link_to 'revisions', node_revisions_path(node) %> + <%= node.lock_owner.login if node.lock_owner %><%= node.draft ? node.draft.revision : (node.head ? node.head.revision : "EMPTY") %>
+<%= will_paginate @nodes %> diff --git a/app/views/nodes/chapters.html.erb b/app/views/nodes/chapters.html.erb new file mode 100644 index 0000000..939f19c --- /dev/null +++ b/app/views/nodes/chapters.html.erb @@ -0,0 +1,9 @@ +

Chapters

+ +<%= form_tag chapters_nodes_path, method: :get do %> + + + <%= submit_tag "Filter", class: "action_button" %> +<% end %> + +<%= render 'node_list' %> diff --git a/app/views/nodes/drafts.html.erb b/app/views/nodes/drafts.html.erb new file mode 100644 index 0000000..6d0830c --- /dev/null +++ b/app/views/nodes/drafts.html.erb @@ -0,0 +1,3 @@ +

Nodes with drafts or autosaves

+ +<%= render 'node_list' %> diff --git a/app/views/nodes/mine.html.erb b/app/views/nodes/mine.html.erb new file mode 100644 index 0000000..fc5680a --- /dev/null +++ b/app/views/nodes/mine.html.erb @@ -0,0 +1,3 @@ +

My work

+ +<%= render 'node_list' %> diff --git a/app/views/nodes/recent.html.erb b/app/views/nodes/recent.html.erb new file mode 100644 index 0000000..3602775 --- /dev/null +++ b/app/views/nodes/recent.html.erb @@ -0,0 +1,3 @@ +

Recently changed

+ +<%= render 'node_list' %> diff --git a/app/views/nodes/tags.html.erb b/app/views/nodes/tags.html.erb new file mode 100644 index 0000000..0ebc6ce --- /dev/null +++ b/app/views/nodes/tags.html.erb @@ -0,0 +1,3 @@ +

Nodes tagged: <%= params[:tags] %>

+ +<%= render 'node_list' %> diff --git a/config/routes.rb b/config/routes.rb index aebce90..2c165d2 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -37,7 +37,12 @@ Cccms::Application.routes.draw do resources :nodes do collection do + get 'tags/:tags', action: :tags, as: :tags, constraints: { tags: /[^\/]+/ } get :parameterize_preview + get :drafts + get :recent + get :mine + get :chapters end member do diff --git a/public/stylesheets/admin.css b/public/stylesheets/admin.css index 28b8494..aa8b288 100644 --- a/public/stylesheets/admin.css +++ b/public/stylesheets/admin.css @@ -748,6 +748,21 @@ form.button_to button[type="submit"] { margin-bottom: 0; } +.node_search_form { + display: flex; + align-items: center; + gap: 0.5rem; + margin-bottom: 0.75rem; +} + +.node_search_form input[type=text] { + padding: 4px 12px; + box-sizing: border-box; + height: 2.25rem; + width: 22rem; + border-radius: 2px; +} + /* Layout only -- the at-rest visibility (wavy underline) for these links comes from the scoped rule in Base elements above. */ .add_child_links { diff --git a/test/controllers/nodes_controller_test.rb b/test/controllers/nodes_controller_test.rb index b563d4d..05cb195 100644 --- a/test/controllers/nodes_controller_test.rb +++ b/test/controllers/nodes_controller_test.rb @@ -471,4 +471,108 @@ class NodesControllerTest < ActionController::TestCase assert_response :success assert_select "form.destructive", :count => 0 end + + test "drafts includes a never-published node with only a draft" do + node = Node.root.children.create!(:slug => "drafts_action_test") + login_as :quentin + get :drafts + assert_includes assigns(:nodes), node + end + + test "chapters filters by kind, matching head or draft, and shows both by default" do + erfa_node = Node.root.children.create!(:slug => "chapters_erfa_test") + erfa_node.find_or_create_draft(@user1) + erfa_node.draft.tag_list = "erfa-detail" + erfa_node.draft.save! + erfa_node.publish_draft! + + chaostreff_node = Node.root.children.create!(:slug => "chapters_chaostreff_test") + chaostreff_node.find_or_create_draft(@user1) + chaostreff_node.draft.tag_list = "chaostreff-detail" + chaostreff_node.draft.save! + chaostreff_node.publish_draft! + + login_as :quentin + + get :chapters, params: { :kinds => "erfa" } + assert_includes assigns(:nodes), erfa_node + assert_not_includes assigns(:nodes), chaostreff_node + + get :chapters + assert_includes assigns(:nodes), erfa_node + assert_includes assigns(:nodes), chaostreff_node + end + + test "recent combined with a search term does not raise an ambiguous column error" do + login_as :quentin + get :recent, params: { :q => "Zombies" } + assert_response :success + end + + test "drafts combined with a search term does not raise an ambiguous column error" do + login_as :quentin + get :drafts, params: { :q => "Zombies" } + assert_response :success + end + + test "mine combined with a search term does not raise an ambiguous column error" do + login_as :quentin + get :mine, params: { :q => "Zombies" } + assert_response :success + end + + test "chapters combined with a search term does not raise an ambiguous column error" do + login_as :quentin + get :chapters, params: { :q => "Zombies" } + assert_response :success + end + + test "tags path filters by an arbitrary raw tag, generalizing chapters" do + presse_node = Node.root.children.create!(:slug => "tags_path_presse_test") + presse_node.find_or_create_draft(@user1) + presse_node.draft.tag_list = "pressemitteilung" + presse_node.draft.save! + presse_node.publish_draft! + + erfa_node = Node.root.children.create!(:slug => "tags_path_erfa_test") + erfa_node.find_or_create_draft(@user1) + erfa_node.draft.tag_list = "erfa-detail" + erfa_node.draft.save! + erfa_node.publish_draft! + + login_as :quentin + get :tags, params: { :tags => "pressemitteilung" } + + assert_includes assigns(:nodes), presse_node + assert_not_includes assigns(:nodes), erfa_node + + assert_select "h1", "Nodes tagged: pressemitteilung" + assert_select "h1", :text => "Chapters", :count => 0 + end + + test "tags path with multiple tags matches any of them, not all" do + erfa_node = Node.root.children.create!(:slug => "tags_path_multi_erfa_test") + erfa_node.find_or_create_draft(@user1) + erfa_node.draft.tag_list = "erfa-detail" + erfa_node.draft.save! + erfa_node.publish_draft! + + chaostreff_node = Node.root.children.create!(:slug => "tags_path_multi_chaostreff_test") + chaostreff_node.find_or_create_draft(@user1) + chaostreff_node.draft.tag_list = "chaostreff-detail" + chaostreff_node.draft.save! + chaostreff_node.publish_draft! + + login_as :quentin + get :tags, params: {:tags => "erfa-detail,chaostreff-detail" } + + assert_includes assigns(:nodes), erfa_node + assert_includes assigns(:nodes), chaostreff_node + end + + test "chapters renders the curated heading" do + login_as :quentin + get :chapters + assert_select "h1", "Chapters" + end end diff --git a/test/models/node_test.rb b/test/models/node_test.rb index 5626384..15e908b 100644 --- a/test/models/node_test.rb +++ b/test/models/node_test.rb @@ -511,4 +511,30 @@ class NodeTest < ActiveSupport::TestCase a.reload assert_equal Node.root.id, a.parent_id end + + test "editor_search matches a partial substring, not just a whole word" do + node = Node.root.children.create!(:slug => "editor_search_substring_test") + node.find_or_create_draft(@user1) + node.draft.update(:title => "Biometrics Conference") + node.publish_draft! + + assert_includes Node.editor_search("bio"), node + assert_includes Node.editor_search("Conf"), node + end + + test "editor_search returns an empty relation for a blank term, not every node" do + assert_equal 0, Node.editor_search("").count + assert_equal 0, Node.editor_search(nil).count + assert_equal 0, Node.editor_search(" ").count + end + + test "editor_search requires every word to match, but each word can match a different field" do + node = Node.root.children.create!(:slug => "editor_search_multiword_test") + node.find_or_create_draft(@user1) + node.draft.update(:title => "Backspace e.V. Bamberg", :abstract => "Spiegelgraben 41, 96052 Bamberg") + node.publish_draft! + + assert_includes Node.editor_search("Backspace Spiegelgraben"), node + assert_equal 0, Node.editor_search("Backspace Nonexistentstreet").count + end end -- cgit v1.3 From 5803c59192b7fb05840d0b452eb64d9f997f3d8f Mon Sep 17 00:00:00 2001 From: erdgeist Date: Sun, 12 Jul 2026 14:26:30 +0200 Subject: Extract drafts/recent query logic from controller into Node Node.drafts_and_autosaves and Node.recently_changed replace inline query logic in NodesController#drafts/#recent -- pure refactor, no behavior change for either action. The real reason: the dashboard's upcoming abridged widgets need the exact same queries the full pages already use, just limited and (for drafts) sorted with the current user's own locked nodes first. Better to share one method than let a widget and a full page quietly drift onto two versions of "what counts as a current draft." current_user_id: is an explicit, optional argument rather than a separate method -- ordering by lock ownership only applies when a user is given; omitted entirely, it's pure recency, exactly today's behavior. Needed Arel.sql wrapping a sanitized CASE expression before .order would accept it -- sanitize_sql_array only vouches for the values inside the string, a separate Rails safety check still refuses any string shaped like more than a plain column reference unless it's explicitly marked as already-vetted. --- app/controllers/nodes_controller.rb | 9 ++------- app/models/node.rb | 16 ++++++++++++++++ test/models/node_test.rb | 24 ++++++++++++++++++++++++ 3 files changed, 42 insertions(+), 7 deletions(-) (limited to 'test/models/node_test.rb') diff --git a/app/controllers/nodes_controller.rb b/app/controllers/nodes_controller.rb index 772bf4b..c1468be 100644 --- a/app/controllers/nodes_controller.rb +++ b/app/controllers/nodes_controller.rb @@ -185,16 +185,11 @@ class NodesController < ApplicationController # Filter functions for admin views def drafts - base = Node.where("draft_id IS NOT NULL OR autosave_id IS NOT NULL") - @nodes = index_matching(base) + @nodes = index_matching(Node.drafts_and_autosaves) end def recent - base = Node.where( - "nodes.updated_at < ? AND nodes.updated_at > ? AND nodes.parent_id IS NOT NULL", - Time.now, Time.now - 14.days - ) - @nodes = index_matching(base) + @nodes = index_matching(Node.recently_changed) end def mine diff --git a/app/models/node.rb b/app/models/node.rb index 24f3cd0..aa2f7f3 100644 --- a/app/models/node.rb +++ b/app/models/node.rb @@ -367,6 +367,22 @@ class Node < ApplicationRecord .distinct end + def self.drafts_and_autosaves(current_user_id: nil) + scope = where("draft_id IS NOT NULL OR autosave_id IS NOT NULL") + return scope.order("updated_at DESC") unless current_user_id + + scope.order( + Arel.sql(sanitize_sql_array(["CASE WHEN locking_user_id = ? THEN 0 ELSE 1 END, updated_at DESC", current_user_id])) + ) + end + + def self.recently_changed + where( + "nodes.updated_at < ? AND nodes.updated_at > ? AND nodes.parent_id IS NOT NULL", + Time.now, Time.now - 14.days + ) + end + protected def lock_for! current_user self.lock_owner = current_user diff --git a/test/models/node_test.rb b/test/models/node_test.rb index 15e908b..de540f8 100644 --- a/test/models/node_test.rb +++ b/test/models/node_test.rb @@ -537,4 +537,28 @@ class NodeTest < ActiveSupport::TestCase assert_includes Node.editor_search("Backspace Spiegelgraben"), node assert_equal 0, Node.editor_search("Backspace Nonexistentstreet").count end + + test "drafts_and_autosaves without a user sorts by recency only" do + older = Node.root.children.create!(:slug => "drafts_order_older") + older.find_or_create_draft(@user1) + newer = Node.root.children.create!(:slug => "drafts_order_newer") + newer.find_or_create_draft(@user1) + + result = Node.drafts_and_autosaves.to_a + assert result.index(newer) < result.index(older) + end + + test "drafts_and_autosaves with a user puts their own locked nodes first, regardless of recency" do + mine = Node.root.children.create!(:slug => "drafts_order_mine") + mine.lock_for_editing!(@user1) + mine.autosave!({:title => "mine"}, @user1) + + someone_elses_newer = Node.root.children.create!(:slug => "drafts_order_theirs") + other_user = User.find_by_login("quentin") + someone_elses_newer.lock_for_editing!(other_user) + someone_elses_newer.autosave!({:title => "theirs"}, other_user) + + result = Node.drafts_and_autosaves(current_user_id: @user1.id).to_a + assert result.index(mine) < result.index(someone_elses_newer) + end end -- cgit v1.3 From b271648f89cba7cafafa1b73b1658b1c1bc0e4b0 Mon Sep 17 00:00:00 2001 From: erdgeist Date: Sun, 12 Jul 2026 23:42:43 +0200 Subject: Rebuild the admin dashboard: icon nav, search, signposts, widgets Replaces the old admin#index wizard -- accreted over years, never designed as a whole -- with the dashboard settled on this session: a three-icon nav (dashboard/search/log out, no locale selector), a nodes-first search bar, four task signposts, and two symmetric widgets (drafts/autosaves, recent changes) with a quiet housekeeping row beneath them. Node.recently_changed now filters and orders by the head page's own updated_at instead of the node's blanket timestamp, so a lock/unlock cycle with no actual publish no longer surfaces here, and the original publisher is no longer misattributed to someone else's housekeeping action. This also restores the "published" qualifier on each entry, which the query previously couldn't guarantee was true. @mynodes and its dedicated "My Work" table are retired along with the old wizard -- "Continue my work" is a link to the existing, already- correct NodesController#mine instead of a second, duplicate query. Its one dedicated test (dedup across multiple revisions by the same user) is ported to nodes_controller_test.rb, since mine already carries the same .distinct protection the old query did; it just had no test of its own until now. --- app/controllers/admin_controller.rb | 21 +-- app/helpers/nodes_helper.rb | 12 ++ app/models/node.rb | 6 +- app/views/admin/_menu.html.erb | 14 +- app/views/admin/index.html.erb | 210 ++++++++++-------------------- test/controllers/admin_controller_test.rb | 20 --- test/controllers/nodes_controller_test.rb | 20 +++ test/models/node_test.rb | 24 ++++ 8 files changed, 134 insertions(+), 193 deletions(-) (limited to 'test/models/node_test.rb') diff --git a/app/controllers/admin_controller.rb b/app/controllers/admin_controller.rb index 8445997..37fd78b 100644 --- a/app/controllers/admin_controller.rb +++ b/app/controllers/admin_controller.rb @@ -5,25 +5,8 @@ class AdminController < ApplicationController before_action :login_required def index - @drafts = Node.where("draft_id IS NOT NULL OR autosave_id IS NOT NULL") - .limit(50).order("updated_at desc") - - @drafts_count = Node.where("draft_id IS NOT NULL OR autosave_id IS NOT NULL").count - - @recent_changes = Node.where( - "updated_at < ? AND updated_at > ? AND parent_id IS NOT NULL", - Time.now, Time.now - 14.days - ).limit(50).order("updated_at desc") - - ordered_with_level = Node.root.self_and_descendants_ordered_with_level - @sitemap_depth = {} - ordered_with_level.each { |node, level| @sitemap_depth[node.id] = level } - @sitemap = ordered_with_level.map(&:first).reject(&:update?) - - @mynodes = Node.joins(:pages) - .where("pages.user_id = ? or pages.editor_id = ?", current_user, current_user) - .order("updated_at desc") - .distinct.first(50) + @drafts = Node.drafts_and_autosaves(current_user_id: current_user.id).limit(5) + @recent_changes = Node.recently_changed.limit(5) end def conventions diff --git a/app/helpers/nodes_helper.rb b/app/helpers/nodes_helper.rb index 1268b63..5884c8c 100644 --- a/app/helpers/nodes_helper.rb +++ b/app/helpers/nodes_helper.rb @@ -28,6 +28,18 @@ module NodesHelper User.all.map {|u| [u.login, u.id]} end + def node_last_editor(node) + editor = node.draft&.editor || node.head&.editor + return nil unless editor + editor == current_user ? t("editor_self") : editor.login + end + + def node_head_editor(node) + editor = node.head&.editor + return nil unless editor + editor == current_user ? t("publisher_self") : editor.login + end + DEFAULT_EVENT_TAG_BY_PAGE_TAG = { 'erfa-detail' => 'open-day', 'chaostreff-detail' => 'open-day' diff --git a/app/models/node.rb b/app/models/node.rb index aa2f7f3..1d0a089 100644 --- a/app/models/node.rb +++ b/app/models/node.rb @@ -377,10 +377,10 @@ class Node < ApplicationRecord end def self.recently_changed - where( - "nodes.updated_at < ? AND nodes.updated_at > ? AND nodes.parent_id IS NOT NULL", + includes(:head).where( + "pages.updated_at < ? AND pages.updated_at > ? AND nodes.parent_id IS NOT NULL", Time.now, Time.now - 14.days - ) + ).order("pages.updated_at desc").references(:head) end protected diff --git a/app/views/admin/_menu.html.erb b/app/views/admin/_menu.html.erb index 4302987..970429d 100644 --- a/app/views/admin/_menu.html.erb +++ b/app/views/admin/_menu.html.erb @@ -1,9 +1,5 @@ -<%= language_selector %> -<%= button_to 'Logout', logout_path, method: :delete %> -<%= link_to 'Overview', admin_path %> -search -<%= link_to 'Nodes', nodes_path, selected?('nodes') %> -<%= link_to 'Assets', assets_path, selected?('assets') %> -<%= link_to 'Events', events_path, selected?('events') %> -<%= link_to 'User', users_path, selected?('users') %> -<%= link_to 'Navigation', menu_items_path, selected?('menu_items') %> +<%= link_to icon("home", library: "tabler", "aria-hidden": true), admin_path, "aria-label": "Dashboard" %> +<%= icon("search", library: "tabler", "aria-hidden": true) %> +<%= button_to logout_path, method: :delete, aria: { label: "Log out" } do %> + <%= icon("logout", library: "tabler", "aria-hidden": true) %> +<% end %> diff --git a/app/views/admin/index.html.erb b/app/views/admin/index.html.erb index c67ccb3..bdb555e 100644 --- a/app/views/admin/index.html.erb +++ b/app/views/admin/index.html.erb @@ -1,155 +1,81 @@ -
-

Quick links

-
<%= link_to 'Create post or page', new_node_path, :only_path => false %>
-
Continue my work
-
Add a page in the page tree
-
<%= link_to("Upload a new asset", new_asset_path, :only_path => false) %>
+

cccms dashboard

+ -

- recent changes - my work - current drafts (<%= @drafts_count %>) - site map -

+
+ <%= link_to new_node_path, class: "action_button" do %> + <%= icon("plus", library: "tabler", "aria-hidden": true) %> Create post + <% end %> + <%= link_to chapters_nodes_path, class: "action_button" do %> + <%= icon("map-pin", library: "tabler", "aria-hidden": true) %> Find a chapter + <% end %> + <%= link_to new_asset_path, class: "action_button" do %> + <%= icon("upload", library: "tabler", "aria-hidden": true) %> Upload asset + <% end %> + <%= link_to sitemap_nodes_path, class: "action_button" do %> + <%= icon("list-tree", library: "tabler", "aria-hidden": true) %> Add a page in the page tree + <% end %> +
-
- -

Recent Changes

+
+
+

Drafts and autosaves

+
    + <% @drafts.each do |node| %> +
  • +
    + <%= link_to title_for_node(node), node_path(node) %> + <%= link_to_path("#{node.unique_name} ↗", node.unique_name) %> +
    + + <% if (editor = node_last_editor(node)) %><%= editor %>, <% end %><%= relative_time_phrase(node.updated_at) %> + +
    • + <% end %> +
+ <%= link_to "See all drafts →", drafts_nodes_path %> +
- - - - - - - - +
+

Recent changes

+
    <% @recent_changes.each do |node| %> -
"> - - - - - - +
  • +
    + <%= link_to title_for_node(node), node_path(node) %> + <%= link_to_path("#{node.unique_name} ↗", node.unique_name) %> +
    + + <%= (editor = node_head_editor(node)) ? t("published_by", editor: editor) : t("published") %>, <%= relative_time_phrase(node.head.updated_at) %> + +
    • <% end %> -
    IDTitleActionsLocked byRev.
    <%= node.id %> -

    <%= link_to title_for_node(node), node_path(node) %>

    -

    <%= link_to_path(node.unique_name, node.unique_name) %>

    -     		- <%= link_to 'show', node_path(node) %> - <%= link_to 'Revisions', node_revisions_path(node) %> - - <%= node.lock_owner.login if node.lock_owner %> - - <%= link_to ( node.draft ? node.draft.revision : (node.head ? node.head.revision : "EMPTY" ) ), node_revisions_path(node) %> -
    + + <%= link_to "See all recent changes →", recent_nodes_path %> +
    -
    - -

    My Work

    - - - - - - - - - - <% @mynodes.each do |node| %> - "> - - - - - - +
    +

    Housekeeping

    +
    + <%= link_to nodes_path, class: "action_button" do %> + <%= icon("list", library: "tabler", "aria-hidden": true) %> Nodes <% end %> -
    IDTitleActionsLocked byRev.
    <%= node.id %> -

    <%= link_to title_for_node(node), node_path(node) %>

    -

    <%= link_to_path(node.unique_name, node.unique_name) %>

    -     		- <%= link_to 'show', node_path(node) %> - <%= link_to 'Revisions', node_revisions_path(node) %> - - <%= node.lock_owner.login if node.lock_owner %> - - <%= link_to ( node.draft ? node.draft.revision : (node.head ? node.head.revision : "EMPTY" ) ), node_revisions_path(node) %> -
    -
    - -
    - -

    Current Drafts & Autosaves (<%= @drafts_count %>)

    - - - - - - - - - - - <% @drafts.each do |node| %> - "> - - - - - - - + <%= link_to events_path, class: "action_button" do %> + <%= icon("calendar", library: "tabler", "aria-hidden": true) %> Events <% end %> -
    IDTitleActionsLocked byRev.Autosave
    <%= node.id %> -

    <%= link_to title_for_node(node), node_path(node) %>

    -

    <%= link_to_path(node.unique_name, node.unique_name) %>

    -     		- <%= link_to 'show', node_path(node) %> - <%= link_to 'Revisions', node_revisions_path(node) %> - <% if pair = node.available_layer_pairs.last %> - <%= link_to 'diff', diff_node_revisions_path(node, start_revision: pair.first, end_revision: pair.last) %> - <% end %> - - <%= node.lock_owner.login if node.lock_owner %> - - <%= link_to ( node.draft ? node.draft.revision : (node.head ? node.head.revision : "EMPTY" ) ), node_revisions_path(node) %> - - <%= node.autosave ? "unsaved changes" : "" %> -
    -
    - -
    - -

    Sitemap

    - - - - - - - - - <% @sitemap.each do |node| %> - <% if !node.nil? %> - "> - - - - - + <%= link_to assets_path, class: "action_button" do %> + <%= icon("folder", library: "tabler", "aria-hidden": true) %> Assets + <% end %> + <%= link_to users_path, class: "action_button" do %> + <%= icon("users", library: "tabler", "aria-hidden": true) %> Users <% end %> + <%= link_to menu_items_path, class: "action_button" do %> + <%= icon("menu-2", library: "tabler", "aria-hidden": true) %> Navigation <% end %> -
    IDTitleActionsLocked by
    <%= node.id %> -

    <%= link_to title_for_node(node), node_path(node) %>

    -

    <%= link_to_path(node.unique_name, node.unique_name) %>

    -     		- <%= link_to 'create subpage', new_node_path(:parent_id => node.id) %> - <%= link_to 'Revisions', node_revisions_path(node) %> - - <%= node.lock_owner.login if node.lock_owner %> -
    +
    diff --git a/test/controllers/admin_controller_test.rb b/test/controllers/admin_controller_test.rb index 9beaf58..00a51e2 100644 --- a/test/controllers/admin_controller_test.rb +++ b/test/controllers/admin_controller_test.rb @@ -15,26 +15,6 @@ class AdminControllerTest < ActionController::TestCase assert_includes assigns(:drafts), node end - test "my work list shows each matching node only once, even with several revisions by the same user" do - login_as :quentin - user = User.find_by_login("quentin") - node = Node.root.children.create!(:slug => "dedup_test") - node.lock_for_editing!(user) - node.autosave!({:title => "v1"}, user) - node.save_draft!(user) - node.publish_draft! - node.lock_for_editing!(user) - node.autosave!({:title => "v2"}, user) - node.save_draft!(user) - node.publish_draft! - # three pages now exist on this node, all touched by quentin -- - # without DISTINCT, the join would return this node three times - - get :index - matches = assigns(:mynodes).select { |n| n.id == node.id } - assert_equal 1, matches.length - end - test "dashboard_search returns matching tags and nodes grouped separately" do node = Node.root.children.create!(:slug => "dashboard_search_test") node.find_or_create_draft(User.find_by_login("aaron")) diff --git a/test/controllers/nodes_controller_test.rb b/test/controllers/nodes_controller_test.rb index b43d2de..81e3f45 100644 --- a/test/controllers/nodes_controller_test.rb +++ b/test/controllers/nodes_controller_test.rb @@ -521,6 +521,26 @@ class NodesControllerTest < ActionController::TestCase assert_response :success end + test "mine shows each matching node only once, even with several revisions by the same user" do + login_as :quentin + user = User.find_by_login("quentin") + node = Node.root.children.create!(:slug => "dedup_test") + node.lock_for_editing!(user) + node.autosave!({:title => "v1"}, user) + node.save_draft!(user) + node.publish_draft! + node.lock_for_editing!(user) + node.autosave!({:title => "v2"}, user) + node.save_draft!(user) + node.publish_draft! + # three pages now exist on this node, all touched by quentin -- + # without DISTINCT, the join would return this node three times + + get :mine + matches = assigns(:nodes).select { |n| n.id == node.id } + assert_equal 1, matches.length + end + test "chapters combined with a search term does not raise an ambiguous column error" do login_as :quentin get :chapters, params: { :q => "Zombies" } diff --git a/test/models/node_test.rb b/test/models/node_test.rb index de540f8..0bce222 100644 --- a/test/models/node_test.rb +++ b/test/models/node_test.rb @@ -561,4 +561,28 @@ class NodeTest < ActiveSupport::TestCase result = Node.drafts_and_autosaves(current_user_id: @user1.id).to_a assert result.index(mine) < result.index(someone_elses_newer) end + + test "recently_changed includes a node whose head was recently published" do + node = Node.root.children.create!(:slug => "recent_changed_published") + node.find_or_create_draft(@user1) + node.autosave!({:title => "v1"}, @user1) + node.save_draft!(@user1) + node.publish_draft! + + assert_includes Node.recently_changed, node + end + + test "recently_changed excludes a node only touched by locking or unlocking after an old publish" do + node = Node.root.children.create!(:slug => "recent_changed_lock_only") + node.find_or_create_draft(@user1) + node.autosave!({:title => "v1"}, @user1) + node.save_draft!(@user1) + node.publish_draft! + node.head.update_column(:updated_at, 20.days.ago) + + node.lock_for_editing!(@user1) + node.unlock! + + assert_not_includes Node.recently_changed, node + end end -- cgit v1.3 From 8baac265059b70da0148487458ee4077b15f155e Mon Sep 17 00:00:00 2001 From: erdgeist Date: Mon, 13 Jul 2026 04:54:33 +0200 Subject: Asset picker: attach/detach/reorder UI, read-only view, autosave fix Replaces nodes#edit's old Images section -- a hidden panel dumping every image asset in the system unfiltered (#image_browser) plus a raw drag-and-drop box (#image_box) -- with a small search-and-click picker built on the endpoint from the last two commits. Attaching posts immediately and appends the new thumbnail via a cloned