9 files changed, 149 insertions, 100 deletions

diff --git a/app/controllers/assets_controller.rb b/app/controllers/assets_controller.rb
index f8a7d1e4..d86370f4 100644
--- a/app/controllers/assets_controller.rb
+++ b/app/controllers/assets_controller.rb
@@ -111,8 +111,10 @@ class AssetsController < ApplicationController
       flash[:notice] =
         if result[:attached].zero?
           t("flash.assets.already_attached", :title => node.title)
+        elsif result[:draft_created]
+          t("flash.assets.attached_new_draft", :title => node.title)
         else
-          t("flash.assets.attached", :title => node.title)
+          t("flash.assets.attached_to_draft", :title => node.title)
         end
       case result[:headline]
       when :set           then flash[:notice] += " " + t("flash.common.now_headline")
@@ -122,5 +124,8 @@ class AssetsController < ApplicationController
     rescue LockedByAnotherUser
       flash[:locked_by]        = node.lock_owner&.login
       flash[:locked_node_path] = node_path(node)
+    rescue ActiveRecord::RecordInvalid => e
+      flash[:error]             = e.record.errors.full_messages.to_sentence
+      flash[:resolve_node_path] = node_path(node)
     end
 end
diff --git a/app/helpers/node_actions_helper.rb b/app/helpers/node_actions_helper.rb
index f21fee61..960e28b3 100644
--- a/app/helpers/node_actions_helper.rb
+++ b/app/helpers/node_actions_helper.rb
@@ -14,7 +14,6 @@ module NodeActionsHelper
     "discard_autosave"   => "eraser",
     "destroy_draft"      => "eraser",
     "asset_create"       => "upload",
-    "asset_attach"       => "paperclip",
     "asset_destroy"      => "file-x",
     "otp_enroll"         => "shield-lock",
     "otp_disable"        => "shield-off",
diff --git a/app/models/node.rb b/app/models/node.rb
index f5b76b45..a401ed69 100644
--- a/app/models/node.rb
+++ b/app/models/node.rb
@@ -6,8 +6,8 @@ class Node < ApplicationRecord
   has_many    :pages, -> { order("revision ASC") }, :dependent => :destroy
 
   # Entries where this node is the primary subject (fast-path column).
-  # For a *complete* history -- including subtree trash/destroy entries
-  # recorded at an ancestor -- use participated_actions instead.
+  # For a *complete* history, including subtree trash/destroy entries
+  # recorded at an ancestor, use participated_actions instead.
   has_many    :node_actions, :dependent => :nullify
   has_many    :action_participations, :class_name => "ActionParticipant", :as => :subject
   has_many    :participated_actions, :through => :action_participations, :source => :node_action
@@ -15,7 +15,7 @@ class Node < ApplicationRecord
   belongs_to  :head,  :class_name => "Page",  :foreign_key => :head_id, optional: true
   belongs_to  :draft, :class_name => "Page",  :foreign_key => :draft_id, optional: true
   # Autosave pages carry no node_id, so has_many :pages does not cover
-  # them -- this dependent: :destroy is their only cleanup on node destroy.
+  # them. This dependent: :destroy is their only cleanup on node destroy.
   belongs_to  :autosave, :class_name => "Page", :foreign_key => :autosave_id, :dependent => :destroy, optional: true
 
   has_many    :events, :dependent => :destroy
@@ -44,8 +44,8 @@ class Node < ApplicationRecord
             :if          => :default_template_name_changed?
 
   # Everything outside the Trash subtree, the Trash node included.
-  # Relies on unique_name being authoritative for tree position --
-  # the same trust public routing places in it.
+  # Relies on unique_name being authoritative for tree position.
+  # The same trust public routing places in it.
   scope :not_in_trash, -> {
     where.not(:unique_name => CccConventions::TRASH_SLUG)
       .where("unique_name NOT LIKE ?", "#{CccConventions::TRASH_SLUG}/%")
@@ -93,7 +93,7 @@ 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
+  # 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
@@ -133,11 +133,11 @@ class Node < ApplicationRecord
 
   # 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 --
+  # 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
+  # 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
@@ -178,7 +178,7 @@ class Node < ApplicationRecord
 
   # Which layer-pairs are meaningful to compare right now, given this
   # node's actual state. Head vs autosave only shows up when no draft
-  # sits between them -- with a draft present, autosave is compared
+  # sits between them, with a draft present, autosave is compared
   # against the draft, never past it straight to head.
   def available_layer_pairs
     pairs = []
@@ -201,8 +201,8 @@ class Node < ApplicationRecord
     self.draft.reload
   end
 
-  # Discards exactly the topmost non-empty layer -- autosave if present,
-  # else draft -- and reveals whatever's beneath it. Releases the lock
+  # Discards exactly the topmost non-empty layer: autosave if present,
+  # else draft, and reveals whatever's beneath it. Releases the lock
   # only once nothing is left to protect (no draft survives); leaves it
   # alone whenever a draft remains, since #edit still has real content
   # open.
@@ -313,7 +313,7 @@ class Node < ApplicationRecord
   # Moves this node and its subtree into the Trash. Demotes every head
   # in the subtree first (aggregators and search operate on heads
   # regardless of tree position); where a node has no draft, the former
-  # head becomes its draft so content stays editable and restorable --
+  # head becomes its draft so content stays editable and restorable,
   # otherwise the former head remains a plain revision. One log entry,
   # at the root, carrying the leaving-public-view snapshot.
   def trash! current_user = nil
@@ -475,28 +475,17 @@ class Node < ApplicationRecord
     end
   end
 
-  # Attaches an asset to every current lifecycle row -- head, draft and
-  # autosave -- that does not already carry it. Attachments are page-
-  # scoped content (RelatedAsset belongs_to :page; drafts and autosaves
-  # are wholesale clones), so attaching to a single layer is how an
-  # attachment gets silently lost when another layer replaces it at
-  # publish or save. This is the out-of-band counterpart to the
-  # in-editor attach UI; it refuses when someone else holds the editing
-  # lock. Attaching to a head row changes the public page immediately,
-  # by design -- same reasoning as formalizing an already-existing
-  # editorial link.
+  # Attaches an asset to the node's draft, creating one from the head if
+  # none exists.
   #
-  # headline is a node-level decision: the flag is set on the newly
-  # created joins only when no current row has a headline yet and the
-  # asset is eligible; otherwise the asset is attached plain and the
-  # result says why, so the caller can point the editor at the star in
-  # the editor instead.
+  # Refuses when an autosave exists: it predates this attach and would
+  # overwrite the draft at save_draft!, silently dropping the join.
+  # Refuses when someone else holds the lock, so an editor's work is not
+  # altered under them.
   #
-  # Returns { :attached => n, :already => n,
+  # Returns { :attached => n, :already => n, :draft_created => bool,
   #           :headline => nil | :set | :kept_existing | :not_eligible }
   def attach_asset! asset, user:, headline: false
-    guard_live_change!(user)
-
     if in_trash? || trash_node?
       errors.add(:base, :attach_in_trash)
       raise ActiveRecord::RecordInvalid.new(self)
@@ -510,37 +499,43 @@ class Node < ApplicationRecord
       )
     end
 
-    rows      = [head, draft, autosave].compact
-    to_attach = rows.reject { |row| row.related_assets.exists?(:asset_id => asset.id) }
+    if autosave
+      errors.add(:base, :attach_with_autosave)
+      raise ActiveRecord::RecordInvalid.new(self)
+    end
 
-    headline_state =
-      if headline && to_attach.any?
-        if !(asset.image? || asset.pdf?)
-          :not_eligible
-        elsif rows.any? { |row| row.headline_asset.present? }
-          :kept_existing
-        else
-          :set
-        end
-      end
+    source = draft || head
+    if source && source.related_assets.exists?(:asset_id => asset.id)
+      return { :attached      => 0,
+               :already       => 1,
+               :draft_created => false,
+               :headline      => nil }
+    end
+
+    draft_created = draft.nil?
 
     ActiveRecord::Base.transaction do
-      to_attach.each do |row|
-        row.related_assets.create!(:asset => asset, :headline => headline_state == :set)
-      end
+      create_new_draft(user) if draft_created
+      page = draft.reload
 
-      if to_attach.any?
-        metadata = { :asset_name => asset.name,
-                     :path       => asset.upload.url.sub(/\?\d+$/, "") }
-        metadata[:headline] = true if headline_state == :set
-        NodeAction.record!(:node => self, :participants => [self, asset],
-                            :user => user, :action => "asset_attach", **metadata)
-      end
-    end
+      headline_state =
+        if headline
+          if !(asset.image? || asset.pdf?)
+            :not_eligible
+          elsif page.headline_asset.present?
+            :kept_existing
+          else
+            :set
+          end
+        end
+
+      page.related_assets.create!(:asset => asset, :headline => headline_state == :set)
 
-    { :attached => to_attach.size,
-      :already  => rows.size - to_attach.size,
-      :headline => headline_state }
+      { :attached      => 1,
+        :already       => 0,
+        :draft_created => draft_created,
+        :headline      => headline_state }
+    end
   end
 
   def title
diff --git a/app/views/layouts/_flash.html.erb b/app/views/layouts/_flash.html.erb
index b72c377f..2cc7a5ab 100644
--- a/app/views/layouts/_flash.html.erb
+++ b/app/views/layouts/_flash.html.erb
@@ -13,6 +13,11 @@
       <%= link_to t(".locked_link"), flash[:locked_node_path] %><%= t(".locked_suffix") %>
     </span>
   <% end %>
+  <% if flash[:resolve_node_path] %>
+    <span class="warning">
+      <%= link_to t(".resolve_link"), flash[:resolve_node_path] %>
+    </span>
+  <% end %>
   <% if flash[:headline_kept_path] %>
     <span class="warning"><%= t(".headline_prefix") %>
       <%= link_to t(".headline_link"), flash[:headline_kept_path] %><%= t(".headline_suffix") %>
diff --git a/config/locales/de.yml b/config/locales/de.yml
index c6dc17b7..58367aaf 100644
--- a/config/locales/de.yml
+++ b/config/locales/de.yml
@@ -136,6 +136,7 @@ de:
               destroy_outside_trash: "Nodes können nur aus dem Papierkorb gelöscht werden"
               attach_in_trash: "An einen Node im Papierkorb können keine Assets angehängt werden"
               not_permitted: "In diesem Bereich dürfen nur Mitglieder der Redaktion veröffentlichte Inhalte ändern"
+              attach_with_autosave: "An einen Node mit ungespeicherten Änderungen im Editor können keine Assets angehängt werden"
         page:
           attributes:
             slug:
@@ -659,8 +660,9 @@ de:
     assets:
       created: "Asset wurde angelegt."
       updated: "Asset wurde aktualisiert."
-      attached: "Asset wurde angelegt und an „%{title}“ angehängt."
-      already_attached: "Asset gespeichert — es war bereits an „%{title}“ angehängt."
+      already_attached: "Asset wurde gespeichert. Es war bereits an „%{title}“ angehängt."
+      attached_to_draft: "Zum Entwurf von %{title} hinzugefügt. Zum Veröffentlichen den Entwurf freigeben."
+      attached_new_draft: "Entwurf von %{title} mit diesem Anhang angelegt. Zum Veröffentlichen den Entwurf freigeben."
     events:
       created: "Termin wurde angelegt."
       updated: "Termin wurde aktualisiert."
diff --git a/config/locales/en.yml b/config/locales/en.yml
index bd339cc9..4bdca31b 100644
--- a/config/locales/en.yml
+++ b/config/locales/en.yml
@@ -87,6 +87,7 @@ en:
               destroy_outside_trash: "Nodes are only destroyed from the Trash"
               attach_in_trash: "Cannot attach assets to a node in the Trash"
               not_permitted: "Only Redaktion members may change published content in this section"
+              attach_with_autosave: "Cannot attach assets to a node with unsaved changes in the editor"
         page:
           attributes:
             slug:
@@ -627,8 +628,9 @@ en:
     assets:
       created: "Asset was successfully created."
       updated: "Asset was successfully updated."
-      attached: "Asset was successfully created and attached to “%{title}”."
       already_attached: "Asset saved — it was already attached to “%{title}”."
+      attached_to_draft: "Attached to the draft of %{title}. Publish it to make the change live."
+      attached_new_draft: "Created a draft of %{title} with this attachment. Publish it to make the change live."
     events:
       created: "Event was successfully created."
       updated: "Event was successfully updated."
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