From aedf5778aba87c4e8d036dde2a0b6ec79cf8b342 Mon Sep 17 00:00:00 2001 From: erdgeist Date: Thu, 23 Jul 2026 19:45:47 +0200 Subject: Keep in-editor asset curation off the head, layering it like every edit ensure_autosave! gives body keystrokes and asset curation one shared layer, so head is never mutated in place and every curation change surfaces in the publish delta. Stale rendered join ids are mapped across the clone via asset_id. Curation now requires holding the lock; a missing lock answers 423, matching the autosave endpoint. --- app/controllers/related_assets_controller.rb | 18 ++++++--- app/models/node.rb | 12 +++--- test/controllers/related_assets_controller_test.rb | 44 ++++++++++++++++++---- 3 files changed, 55 insertions(+), 19 deletions(-) diff --git a/app/controllers/related_assets_controller.rb b/app/controllers/related_assets_controller.rb index ca894f2f..3aa6d3db 100644 --- a/app/controllers/related_assets_controller.rb +++ b/app/controllers/related_assets_controller.rb @@ -2,6 +2,10 @@ class RelatedAssetsController < ApplicationController before_action :login_required before_action :find_node + rescue_from LockedByAnotherUser do + head :locked + end + def search term = params[:search_term].to_s.strip attached_ids = @node.editable_page.related_assets.pluck(:asset_id) @@ -19,8 +23,9 @@ class RelatedAssetsController < ApplicationController end def create + page = @node.ensure_autosave!(current_user) asset = Asset.find(params[:asset_id]) - related = @node.editable_page.related_assets.find_or_create_by!(asset: asset) + related = page.related_assets.find_or_create_by!(asset: asset) render json: { id: related.id, @@ -35,22 +40,25 @@ class RelatedAssetsController < ApplicationController end def destroy - @node.editable_page.related_assets.find(params[:id]).destroy + page = @node.ensure_autosave!(current_user) + source = RelatedAsset.find(params[:id]) + page.related_assets.find_by!(:asset_id => source.asset_id).destroy head :ok end def update - related = @node.editable_page.related_assets.find(params[:id]) + page = @node.ensure_autosave!(current_user) + source = RelatedAsset.find(params[:id]) + related = page.related_assets.find_by!(:asset_id => source.asset_id) if params.key?(:headline) RelatedAsset.transaction do - @node.editable_page.related_assets.update_all(headline: false) + page.related_assets.update_all(headline: false) related.update!(headline: true) if params[:headline] == "true" end else related.insert_at(params[:position].to_i) end - head :ok end diff --git a/app/models/node.rb b/app/models/node.rb index 274b2f94..02502f6c 100644 --- a/app/models/node.rb +++ b/app/models/node.rb @@ -112,18 +112,18 @@ class Node < ApplicationRecord 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 + def ensure_autosave! current_user assert_locked_by! current_user - unless self.autosave self.autosave = Page.create!(:editor => current_user) self.autosave.clone_attributes_from(self.draft || self.head) if self.draft || self.head self.save! end + self.autosave + end + + def autosave! attributes, current_user + ensure_autosave!(current_user) self.autosave.assign_attributes(attributes) self.autosave.save! self.autosave diff --git a/test/controllers/related_assets_controller_test.rb b/test/controllers/related_assets_controller_test.rb index ced4b74d..fd30dddd 100644 --- a/test/controllers/related_assets_controller_test.rb +++ b/test/controllers/related_assets_controller_test.rb @@ -38,19 +38,24 @@ class RelatedAssetsControllerTest < ActionController::TestCase test "create attaches an asset to the node's editable page" do login_as :quentin node = Node.root.children.create!(:slug => "related_assets_create_test") + node.lock_for_editing!(users(:quentin)) asset = Asset.create!(:name => "erfa-photo", :upload_content_type => "image/png") post :create, params: { :node_id => node.id, :asset_id => asset.id } assert_response :success - assert_includes node.draft.reload.related_assets.map(&:asset_id), asset.id + current = node.reload.editable_page + assert_equal node.autosave, current, "mutation should have created an autosave layer" + assert_includes current.related_assets.map(&:asset_id), asset.id json = JSON.parse(response.body) assert json["url"].present? + assert_empty node.draft.reload.related_assets, "the draft must stay untouched" end test "create does not duplicate an already-attached asset" do login_as :quentin node = Node.root.children.create!(:slug => "related_assets_dup_test") + node.lock_for_editing!(users(:quentin)) asset = Asset.create!(:name => "erfa-photo-2", :upload_content_type => "image/png") node.draft.assets << asset @@ -63,19 +68,22 @@ class RelatedAssetsControllerTest < ActionController::TestCase test "destroy removes the attached asset" do login_as :quentin node = Node.root.children.create!(:slug => "related_assets_destroy_test") + node.lock_for_editing!(users(:quentin)) asset = Asset.create!(:name => "old-photo", :upload_content_type => "image/png") node.draft.assets << asset - related = node.draft.related_assets.first + related = node.draft.related_assets.first delete :destroy, params: { :node_id => node.id, :id => related.id } assert_response :success - assert_equal 0, node.draft.reload.related_assets.count + assert_equal 0, node.reload.editable_page.related_assets.count + assert_equal 1, node.draft.reload.related_assets.count, "the draft must stay untouched" end test "update reorders the attached assets" do login_as :quentin node = Node.root.children.create!(:slug => "related_assets_reorder_test") + node.lock_for_editing!(users(:quentin)) first = Asset.create!(:name => "first-photo", :upload_content_type => "image/png") second = Asset.create!(:name => "second-photo", :upload_content_type => "image/png") node.draft.assets << first @@ -85,13 +93,15 @@ class RelatedAssetsControllerTest < ActionController::TestCase patch :update, params: { :node_id => node.id, :id => second_related.id, :position => 1 } assert_response :success - ordered_asset_ids = node.draft.reload.related_assets.map(&:asset_id) + ordered_asset_ids = node.reload.editable_page.related_assets.order(:position).map(&:asset_id) + # XXXX ordered_asset_ids = node.draft.reload.related_assets.map(&:asset_id) assert_equal [second.id, first.id], ordered_asset_ids end test "update sets the headline flag" do login_as :quentin node = Node.root.children.create!(:slug => "related_assets_headline_test") + node.lock_for_editing!(users(:quentin)) asset = Asset.create!(:name => "headline-photo", :upload_content_type => "image/png") node.draft.assets << asset related = node.draft.related_assets.find_by(:asset_id => asset.id) @@ -99,12 +109,14 @@ class RelatedAssetsControllerTest < ActionController::TestCase patch :update, params: { :node_id => node.id, :id => related.id, :headline => "true" } assert_response :success - assert related.reload.headline? + assert node.reload.editable_page.related_assets.find_by!(:asset_id => asset.id).headline? + assert_not related.reload.headline?, "the draft must stay untouched" end test "update with headline=true clears any previous headline on the same page" do login_as :quentin node = Node.root.children.create!(:slug => "related_assets_headline_swap_test") + node.lock_for_editing!(users(:quentin)) first = Asset.create!(:name => "first-headline", :upload_content_type => "image/png") second = Asset.create!(:name => "second-headline", :upload_content_type => "image/png") node.draft.assets << first @@ -117,13 +129,16 @@ class RelatedAssetsControllerTest < ActionController::TestCase patch :update, params: { :node_id => node.id, :id => second_related.id, :headline => "true" } assert_response :success - assert_not first_related.reload.headline? - assert second_related.reload.headline? + current = node.reload.editable_page + assert_not current.related_assets.find_by!(:asset_id => first.id).headline? + assert current.related_assets.find_by!(:asset_id => second.id).headline? + assert first_related.reload.headline?, "the draft must stay untouched" end test "update with headline=false clears the headline" do login_as :quentin node = Node.root.children.create!(:slug => "related_assets_headline_unset_test") + node.lock_for_editing!(users(:quentin)) asset = Asset.create!(:name => "unset-headline", :upload_content_type => "image/png") node.draft.assets << asset related = node.draft.related_assets.find_by(:asset_id => asset.id) @@ -132,7 +147,8 @@ class RelatedAssetsControllerTest < ActionController::TestCase patch :update, params: { :node_id => node.id, :id => related.id, :headline => "false" } assert_response :success - assert_not related.reload.headline? + assert_not node.reload.editable_page.related_assets.find_by!(:asset_id => asset.id).headline? + assert related.reload.headline?, "the draft must stay untouched" end test "search includes PDF assets as headline-eligible candidates" do @@ -159,4 +175,16 @@ class RelatedAssetsControllerTest < ActionController::TestCase ids = JSON.parse(response.body).map { |r| r["id"] } assert_includes ids, asset.id end + + test "curation without holding the lock is refused with 423" do + login_as :quentin + node = Node.root.children.create!(:slug => "curation_lock_test") + asset = Asset.create!(:name => "Untouchable", :upload_content_type => "image/png") + node.lock_for_editing!(users(:aaron)) + + post :create, params: { :node_id => node.id, :asset_id => asset.id } + + assert_response :locked + assert_empty node.draft.assets.reload + end end -- cgit v1.3