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.

3 files changed, 80 insertions, 39 deletions

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