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.

4 files changed, 62 insertions, 58 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") %>