summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorerdgeist <erdgeist@erdgeist.org>2026-07-08 17:31:15 +0200
committererdgeist <erdgeist@erdgeist.org>2026-07-08 17:31:15 +0200
commit6ad96c44d04df01e6abde097c681e824dd5fe745 (patch)
treee90fcd5f3a4544bf2fd9ddf70530a82ac37b31c0
parent087fa345853a103844375892fa0405c1c241ea92 (diff)
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.
-rw-r--r--app/models/node.rb36
-rw-r--r--test/controllers/nodes_controller_test.rb7
-rw-r--r--test/models/node_test.rb3
3 files changed, 41 insertions, 5 deletions
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
56 def lock_for_editing! current_user 56 def lock_for_editing! current_user
57 if self.lock_owner.nil? || self.lock_owner == current_user 57 if self.lock_owner.nil? || self.lock_owner == current_user
58 lock_for! current_user 58 lock_for! current_user
59 if self.draft
60 self.draft.user = current_user if self.draft.user.nil?
61 self.draft.editor = current_user
62 self.draft.save!
63 end
59 self 64 self
60 else 65 else
61 raise( 66 raise(
@@ -95,15 +100,18 @@ class Node < ApplicationRecord
95 return unless self.autosave 100 return unless self.autosave
96 101
97 if self.draft 102 if self.draft
103 preserved_published_at = self.draft.published_at
98 self.draft.clone_attributes_from self.autosave 104 self.draft.clone_attributes_from self.autosave
105 self.draft.published_at = preserved_published_at
99 self.draft.user_id = self.autosave.user_id if self.autosave.user_id 106 self.draft.user_id = self.autosave.user_id if self.autosave.user_id
100 self.draft.editor = current_user 107 self.draft.editor = current_user
101 self.draft.save! 108 self.draft.save!
102 else 109 else
103 empty_page = self.pages.create! 110 empty_page = self.pages.create!
104 empty_page.user = self.autosave.user_id ? self.autosave.user : (self.head ? self.head.user : current_user)
105 empty_page.editor = current_user
106 empty_page.clone_attributes_from self.autosave 111 empty_page.clone_attributes_from self.autosave
112 empty_page.user = self.autosave.user_id ? self.autosave.user : (self.head ? self.head.user : current_user)
113 empty_page.editor = current_user
114 empty_page.published_at = self.head.published_at if self.head
107 empty_page.save! 115 empty_page.save!
108 self.draft = empty_page 116 self.draft = empty_page
109 self.save! 117 self.save!
@@ -150,6 +158,28 @@ class Node < ApplicationRecord
150 self.draft.reload 158 self.draft.reload
151 end 159 end
152 160
161 # Discards exactly the topmost non-empty layer -- autosave if present,
162 # else draft -- and reveals whatever's beneath it. Releases the lock
163 # only once nothing is left to protect (no draft survives); leaves it
164 # alone whenever a draft remains, since #edit still has real content
165 # open.
166 def revert! current_user
167 assert_locked_by! current_user
168
169 if self.autosave
170 self.autosave.destroy
171 self.autosave_id = nil
172 self.save!
173 elsif self.draft && self.head
174 self.draft.destroy
175 self.draft_id = nil
176 self.save!
177 end
178
179 self.unlock! unless self.draft
180 self.reload
181 end
182
153 def staged_slug=(value) 183 def staged_slug=(value)
154 if head.blank? 184 if head.blank?
155 self.slug = value 185 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
101 assert_select("#page_body", "World") 101 assert_select("#page_body", "World")
102 102
103 node.reload 103 node.reload
104 assert_equal 2, node.pages.length 104 assert_equal 1, node.pages.length
105 assert_equal "Hello", node.find_or_create_draft( User.first ).title 105 assert_equal "Hello", node.find_or_create_draft( User.first ).title
106 assert_equal "World", node.find_or_create_draft( User.first ).body 106 assert_equal "World", node.find_or_create_draft( User.first ).body
107 end 107 end
@@ -291,6 +291,7 @@ class NodesControllerTest < ActionController::TestCase
291 get :edit, params: { :id => node.id } 291 get :edit, params: { :id => node.id }
292 assert_response :success 292 assert_response :success
293 293
294 put :update, params: { :id => node.id, :page => { :title => "updated" } }
294 put :publish, params: { :id => node.id } 295 put :publish, params: { :id => node.id }
295 296
296 node.reload 297 node.reload
@@ -303,6 +304,7 @@ class NodesControllerTest < ActionController::TestCase
303 node = create_node_with_published_page 304 node = create_node_with_published_page
304 get :edit, params: { :id => node.id } 305 get :edit, params: { :id => node.id }
305 306
307 put :update, params: { :id => node.id, :page => { :title => "updated" } }
306 put :publish, params: { :id => node.id } 308 put :publish, params: { :id => node.id }
307 309
308 node.reload 310 node.reload
@@ -324,7 +326,8 @@ class NodesControllerTest < ActionController::TestCase
324 assert_equal "quentin", node.head.user.login 326 assert_equal "quentin", node.head.user.login
325 327
326 login_as :aaron 328 login_as :aaron
327 get :edit, params: {:id => node.id } 329 get :edit, params: { :id => node.id }
330 put :update, params: { :id => node.id, :page => { :title => "updated" } }
328 331
329 node.reload 332 node.reload
330 assert_equal "quentin", node.head.user.login 333 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
339 assert_equal head_revision, node.head.revision 339 assert_equal head_revision, node.head.revision
340 assert_nil node.autosave 340 assert_nil node.autosave
341 assert_equal 2, node.pages.count 341 assert_equal 2, node.pages.count
342 assert_equal node.head.user, node.draft.user
343 assert_equal @user1, node.draft.editor
344 assert_equal node.head.published_at, node.draft.published_at
342 end 345 end
343 346
344 test "autosave!, save_draft!, and lock_for_editing! raise LockedByAnotherUser for a second user" do 347 test "autosave!, save_draft!, and lock_for_editing! raise LockedByAnotherUser for a second user" do