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. --- app/models/node.rb | 80 ++++++++++++++++++ .../20260708095943_add_autosave_id_to_nodes.rb | 5 ++ test/models/node_test.rb | 96 ++++++++++++++++++++++ 3 files changed, 181 insertions(+) create mode 100644 db/migrate/20260708095943_add_autosave_id_to_nodes.rb diff --git a/app/models/node.rb b/app/models/node.rb index fc23dc1..9eb0fe4 100644 --- a/app/models/node.rb +++ b/app/models/node.rb @@ -6,6 +6,7 @@ class Node < ApplicationRecord has_many :pages, -> { order("revision ASC") }, :dependent => :destroy belongs_to :head, :class_name => "Page", :foreign_key => :head_id, :dependent => :destroy, optional: true belongs_to :draft, :class_name => "Page", :foreign_key => :draft_id, :dependent => :destroy, optional: true + belongs_to :autosave, :class_name => "Page", :foreign_key => :autosave_id, :dependent => :destroy, optional: true has_many :permissions, :dependent => :destroy has_many :events, :dependent => :destroy belongs_to :lock_owner, :class_name => "User", :foreign_key => :locking_user_id, optional: true @@ -49,6 +50,71 @@ class Node < ApplicationRecord # Instance Methods + # Acquires (or reaffirms) the editing lock without creating a draft or + # an autosave -- both are now deferred until there is real content to + # hold. + def lock_for_editing! current_user + if self.lock_owner.nil? || self.lock_owner == current_user + lock_for! current_user + self + else + raise( + LockedByAnotherUser, + "Page is locked by another user who is working on it! " \ + "Last modification: #{(self.autosave || self.draft || self.head).updated_at.to_fs(:db)}" + ) + end + end + + # Creates or updates the autosave buffer from the given attributes. + # Autosave rows are never associated to the node via node_id -- they + # must never appear in self.pages / the revisions list, which is the + # whole reason autosave exists as a separate, unversioned layer. + def autosave! attributes, current_user + assert_locked_by! current_user + + unless self.autosave + self.autosave = Page.create!(:editor => current_user) + self.save! + end + self.autosave.assign_attributes(attributes) + self.autosave.save! + self.autosave + end + + # Promotes the current autosave into the draft (creating the draft if + # none exists yet) and destroys the autosave afterward. This is what + # the explicit "Save" action does; it never creates a new revision -- + # same as any other in-place draft edit. The new draft is created via + # self.pages.create! rather than by repointing the autosave's own + # node_id, because acts_as_list assigns the revision number at create + # time, scoped to node_id -- a page created with node_id nil and + # reassigned afterward would carry a wrong or missing revision number. + def save_draft! current_user + assert_locked_by! current_user + return unless self.autosave + + if self.draft + self.draft.clone_attributes_from self.autosave + 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.clone_attributes_from self.autosave + empty_page.save! + self.draft = empty_page + self.save! + end + + self.autosave.destroy + self.autosave_id = nil + self.save! + self.draft.reload + end + def find_or_create_draft current_user self.wipe_draft! if draft && self.lock_owner == current_user @@ -129,8 +195,13 @@ class Node < ApplicationRecord # removes a draft and the lock if it is older than a day and still # identical to head def wipe_draft! + return if self.autosave && self.autosave.updated_at > 1.day.ago + unless self.draft + self.autosave&.destroy + self.autosave_id = nil self.unlock! + self.save! return end return unless self.head @@ -224,6 +295,15 @@ class Node < ApplicationRecord self.save end + def assert_locked_by! current_user + return if self.lock_owner == current_user + raise( + LockedByAnotherUser, + "Page is locked by another user who is working on it! " \ + "Last modification: #{(self.autosave || self.draft || self.head).updated_at.to_fs(:db)}" + ) + end + # Creates an empty page and associates it to the given node. This means # freshly created node has an empty draft. A user can create nodes as he # wants to which will not appear on the public page until the author edits diff --git a/db/migrate/20260708095943_add_autosave_id_to_nodes.rb b/db/migrate/20260708095943_add_autosave_id_to_nodes.rb new file mode 100644 index 0000000..eedcc00 --- /dev/null +++ b/db/migrate/20260708095943_add_autosave_id_to_nodes.rb @@ -0,0 +1,5 @@ +class AddAutosaveIdToNodes < ActiveRecord::Migration[8.1] + def change + add_column :nodes, :autosave_id, :integer + end +end 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