From 20c735b0c5a2db9b8984848253ad99332d6211a8 Mon Sep 17 00:00:00 2001 From: erdgeist Date: Sun, 9 Aug 2026 03:33:55 +0200 Subject: Attach assets to the draft instead of to every lifecycle row Node#attach_asset! writes to the node's draft alone, creating one from the head when none is pending. It refuses when another user holds the lock, and when an autosave already exists The asset_attach verb goes with it. create_new_draft names the editor on the draft it creates and the publish entry's asset delta reports the attachment. --- test/controllers/assets_controller_test.rb | 34 ++++++++---- test/controllers/content_controller_test.rb | 2 + test/models/node_attach_asset_test.rb | 83 ++++++++++++++++++----------- 3 files changed, 80 insertions(+), 39 deletions(-) (limited to 'test') diff --git a/test/controllers/assets_controller_test.rb b/test/controllers/assets_controller_test.rb index 467e1a68..aa3e00e6 100644 --- a/test/controllers/assets_controller_test.rb +++ b/test/controllers/assets_controller_test.rb @@ -94,7 +94,7 @@ class AssetsControllerTest < ActionController::TestCase # --- create with attach --- - test "create with node_id attaches the asset to the node's draft" do + test "create with node_id attaches the asset to an existing draft" do node = Node.root.children.create!(:slug => "asset_attach_target") post :create, params: { asset: { name: 'Attach me' }, node_id: node.id } @@ -102,7 +102,21 @@ class AssetsControllerTest < ActionController::TestCase assert_response :redirect asset = Asset.last assert_includes node.draft.assets.reload, asset - assert_equal I18n.t("flash.assets.attached", :title => node.title), flash[:notice] + assert_equal I18n.t("flash.assets.attached_to_draft", :title => node.title), flash[:notice] + end + + test "create with node_id creates a draft when none is pending" do + node = Node.root.children.create!(:slug => "asset_attach_no_draft") + node.publish_draft!(users(:quentin)) + assert_nil node.reload.draft + + post :create, params: { asset: { name: 'Attach me too' }, node_id: node.id } + + node.reload + assert_includes node.draft.assets.reload, Asset.last + assert_empty node.head.assets.reload + assert_equal users(:quentin), node.draft.editor + assert_equal I18n.t("flash.assets.attached_new_draft", :title => node.title), flash[:notice] end test "create against a foreign-locked node keeps the asset but refuses the attach" do @@ -133,14 +147,16 @@ class AssetsControllerTest < ActionController::TestCase assert_equal node_path(node), flash[:headline_kept_path] end - test "create with node_id writes an asset_create and an asset_attach entry" do - node = Node.root.children.create!(:slug => "asset_log_pair") - assert_difference 'NodeAction.where(:action => "asset_create").count' do - assert_difference 'NodeAction.where(:action => "asset_attach").count' do - post :create, params: { asset: { name: 'Logged twice' }, node_id: node.id } - end + test "create with node_id writes only an asset_create entry" do + node = Node.root.children.create!(:slug => "asset_log_single") + + assert_difference 'NodeAction.count', 1 do + post :create, params: { asset: { name: 'Logged once' }, node_id: node.id } end - assert_equal users(:quentin), NodeAction.last.user + + action = NodeAction.last + assert_equal "asset_create", action.action + assert_equal users(:quentin), action.user end # --- edit --- diff --git a/test/controllers/content_controller_test.rb b/test/controllers/content_controller_test.rb index 39fe276b..482c1ddf 100644 --- a/test/controllers/content_controller_test.rb +++ b/test/controllers/content_controller_test.rb @@ -170,6 +170,8 @@ class ContentControllerTest < ActionController::TestCase :upload_content_type => "image/png", :upload_updated_at => Time.at(1_700_000_000)) node.attach_asset!(asset, :user => @user1, :headline => true) + node.publish_draft! + node.reload # has_variant? only tests File.exist?, so touching the path is enough # and no ImageMagick runs in the suite. image/png takes .jpg for the diff --git a/test/models/node_attach_asset_test.rb b/test/models/node_attach_asset_test.rb index 2df2cfbb..9ab38158 100644 --- a/test/models/node_attach_asset_test.rb +++ b/test/models/node_attach_asset_test.rb @@ -9,49 +9,67 @@ class NodeAttachAssetTest < ActiveSupport::TestCase @image = create_image_asset end - test "attaches to a draft-only node" do + test "attaches to an existing draft" do result = @node.attach_asset!(@image, :user => @user) assert_equal 1, result[:attached] + assert_not result[:draft_created] assert_includes @node.draft.assets, @image end - test "attaches to head when no draft is pending" do + test "creates a draft when none is pending and leaves head untouched" do @node.publish_draft!(@user) result = @node.attach_asset!(@image, :user => @user) assert_equal 1, result[:attached] - assert_includes @node.head.assets, @image + assert result[:draft_created] + assert_includes @node.draft.assets, @image + assert_empty @node.head.assets.reload end - test "attaches to head and pending draft alike" do + test "attaches to a pending draft and leaves head untouched" do @node.publish_draft!(@user) @node.lock_for_editing!(@user) @node.create_new_draft(@user) result = @node.attach_asset!(@image, :user => @user) - assert_equal 2, result[:attached] - assert_includes @node.head.assets, @image + assert_equal 1, result[:attached] + assert_not result[:draft_created] assert_includes @node.draft.assets, @image + assert_empty @node.head.assets.reload end - test "attaches to all three lifecycle rows" do + test "refuses when an autosave exists and writes nothing" do @node.publish_draft!(@user) @node.lock_for_editing!(@user) @node.create_new_draft(@user) @node.autosave!({ :title => "wip" }, @user) + assert_raises(ActiveRecord::RecordInvalid) { @node.attach_asset!(@image, :user => @user) } + assert_empty @node.draft.assets.reload + assert_empty @node.autosave.assets.reload + end + + test "reports an asset the draft already carries without duplicating it" do + @node.draft.related_assets.create!(:asset => @image) result = @node.attach_asset!(@image, :user => @user) - assert_equal 3, result[:attached] - [@node.head, @node.draft, @node.autosave].each do |row| - assert_includes row.assets, @image - end + assert_equal 0, result[:attached] + assert_equal 1, result[:already] + assert_not result[:draft_created] + assert_equal 1, @node.draft.related_assets.where(:asset_id => @image.id).count end - test "skips rows that already carry the asset" do + test "creates no draft when head already carries the asset" do @node.draft.related_assets.create!(:asset => @image) @node.publish_draft!(@user) - @node.lock_for_editing!(@user) - @node.create_new_draft(@user) + assert_nil @node.draft + + result = @node.attach_asset!(@image, :user => @user) + assert_equal 0, result[:attached] + assert_not result[:draft_created] + assert_nil @node.reload.draft + end + + test "attaching twice leaves one join row" do + @node.attach_asset!(@image, :user => @user) result = @node.attach_asset!(@image, :user => @user) assert_equal 0, result[:attached] - assert_equal 2, result[:already] assert_equal 1, @node.draft.related_assets.where(:asset_id => @image.id).count end @@ -82,6 +100,17 @@ class NodeAttachAssetTest < ActiveSupport::TestCase assert_includes @node.draft.assets, @image end + test "keeps a headline the new draft inherited from head" do + incumbent = create_image_asset + @node.draft.related_assets.create!(:asset => incumbent, :headline => true) + @node.publish_draft!(@user) + + result = @node.attach_asset!(@image, :user => @user, :headline => true) + assert result[:draft_created] + assert_equal :kept_existing, result[:headline] + assert_equal incumbent, @node.draft.reload.headline_asset + end + test "declines the headline flag for ineligible asset types" do plain = create_plain_asset result = @node.attach_asset!(plain, :user => @user, :headline => true) @@ -95,25 +124,19 @@ class NodeAttachAssetTest < ActiveSupport::TestCase assert_raises(ActiveRecord::RecordInvalid) { @node.attach_asset!(@image, :user => @user) } end - test "attaching writes an asset_attach entry with node and asset participants" do - result = @node.attach_asset!(@image, :user => @user, :headline => true) - assert_equal :set, result[:headline] - - action = NodeAction.where(:action => "asset_attach").last - assert_equal @node, action.node - subjects = action.action_participants.map { |p| [p.subject_type, p.subject_id] } - assert_includes subjects, ["Node", @node.id] - assert_includes subjects, ["Asset", @image.id] - assert action.metadata["headline"] - end - - test "a fully redundant attach writes no entry" do - @node.attach_asset!(@image, :user => @user) - assert_no_difference 'NodeAction.count' do + test "attaching writes no log entry -- publish carries the witnessing" do + assert_no_difference "NodeAction.count" do @node.attach_asset!(@image, :user => @user) end end + test "attaching under a restricted surface needs no redaktion role" do + updates = Node.root.children.create!(:slug => "updates") + node = updates.children.create!(:slug => "gated-attachment") + result = node.reload.attach_asset!(@image, :user => @user) + assert_equal 1, result[:attached] + end + private def create_image_asset -- cgit v1.3