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.

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