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. --- app/controllers/assets_controller.rb | 7 +- app/helpers/node_actions_helper.rb | 1 - app/models/node.rb | 107 +++++++++++++--------------- app/views/layouts/_flash.html.erb | 5 ++ config/locales/de.yml | 6 +- config/locales/en.yml | 4 +- test/controllers/assets_controller_test.rb | 34 ++++++--- test/controllers/content_controller_test.rb | 2 + test/models/node_attach_asset_test.rb | 83 +++++++++++++-------- 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 + + headline_state = + if headline + if !(asset.image? || asset.pdf?) + :not_eligible + elsif page.headline_asset.present? + :kept_existing + else + :set + end + end - 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 + 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") %> <% end %> + <% if flash[:resolve_node_path] %> + + <%= link_to t(".resolve_link"), flash[:resolve_node_path] %> + + <% end %> <% if flash[:headline_kept_path] %> <%= 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 -- cgit v1.3