Refuse destroying an asset that is still attached

6 files changed, 42 insertions, 106 deletions

diff --git a/app/helpers/node_actions_helper.rb b/app/helpers/node_actions_helper.rb
index 960e28b3..09de4250 100644
--- a/app/helpers/node_actions_helper.rb
+++ b/app/helpers/node_actions_helper.rb
@@ -330,22 +330,9 @@ module NodeActionsHelper
        :asset => asset_ref(action)).html_safe
   end
 
-  def summarize_asset_attach action
-    m = action.metadata
-    key = m["headline"] ? "node_actions.asset_attach_headline" : "node_actions.asset_attach"
-    t(key, :actor => actor_ref(action), :subject => subject_ref(action),
-       :asset => asset_ref(action)).html_safe
-  end
-
   def summarize_asset_destroy action
-    m = action.metadata
-    parts = [t("node_actions.asset_destroy", :actor => actor_ref(action),
-                :asset => asset_ref(action))]
-    parts << t("node_actions.asset_destroy_detached",
-                :paths => h(Array(m["detached_from"]).join(", "))) if m["detached_from"].present?
-    parts << t("node_actions.asset_destroy_headlines",
-                :paths => h(Array(m["headline_removed_from"]).join(", "))) if m["headline_removed_from"].present?
-    safe_join(parts, " ")
+    t("node_actions.asset_destroy", :actor => actor_ref(action),
+       :asset => asset_ref(action)).html_safe
   end
 
   def summarize_otp_enroll action
diff --git a/app/models/asset.rb b/app/models/asset.rb
index 4d43b18f..08ef5a78 100644
--- a/app/models/asset.rb
+++ b/app/models/asset.rb
@@ -41,44 +41,23 @@ class Asset < ApplicationRecord
                :ids => page_ids).distinct
   end
 
-  # An asset's reach is the reach of the pages carrying it: destroying one
-  # removes it from every live page at once, so a single restricted
-  # attachment makes the destruction a restricted act.
-  def restricted?
-    attached_nodes.any?(&:restricted?)
-  end
-
-  # Witnessed destruction. Destroying an asset is a public-facing act
-  # even when unattached. The original and its variants are publicly
-  # reachable under /system/uploads, so an entry is always written,
-  # before the row and its files die. Every currently-attached node
-  # participates so its zoomed history shows the loss; the asset itself
-  # participates as the first non-Node subject (its participant row
-  # dangles after destroy, by design, the name lives on in metadata).
+  # Witnessed destruction, refused while the asset is attached to any
+  # current row. Detaching stays an in-editor act, so nothing
+  # destroyed here is carried by a page. The entry is still warranted:
+  # the original and its variants are reachable under /system/uploads
+  # until the row and its files die.
   def destroy_witnessed! user:
-    if user && !user.may_change_live?(self)
-      errors.add(:base, :not_permitted)
+    if attached_nodes.any?
+      errors.add(:base, :destroy_while_attached)
       raise ActiveRecord::RecordInvalid.new(self)
     end
 
     ActiveRecord::Base.transaction do
-      affected = attached_nodes.to_a
-      headline_losses = affected.select do |node|
-        [node.head, node.draft, node.autosave].compact.any? do |row|
-          row.related_assets.exists?(:asset_id => id, :headline => true)
-        end
-      end
-
-      metadata = {
-        :asset_name   => name,
-        :content_type => upload_content_type,
-        :path         => upload.url.sub(/\?\d+$/, ""),
-      }
-      metadata[:detached_from]         = affected.map(&:unique_name)        if affected.any?
-      metadata[:headline_removed_from] = headline_losses.map(&:unique_name) if headline_losses.any?
-
-      NodeAction.record!(:participants => [self] + affected, :user => user,
-                          :action => "asset_destroy", **metadata)
+      NodeAction.record!(:participants => [self], :user => user,
+                          :action       => "asset_destroy",
+                          :asset_name   => name,
+                          :content_type => upload_content_type,
+                          :path         => upload.url.sub(/\?\d+$/, ""))
       destroy!
     end
   end
diff --git a/app/models/node_action.rb b/app/models/node_action.rb
index 1e64861e..aab1d238 100644
--- a/app/models/node_action.rb
+++ b/app/models/node_action.rb
@@ -82,22 +82,13 @@ class NodeAction < ApplicationRecord
   # "asset_create" (witnessed upload; participants: the asset alone):
   #   "asset_name", "content_type", "path" -- flat strings
   #
-  # "asset_attach" (out-of-band attach via Node#attach_asset!; written
-  # only when at least one new join was created, per the tandem rule --
-  # in-editor curation stays draft-scoped and surfaces at publish.
-  # participants: the node (primary) and the asset):
-  #   "asset_name", "path" -- flat strings
-  #   "headline"           -- boolean, only when set by this attach
-  #
-  # "asset_destroy" (witnessed asset deletion; always written, even for
-  # unattached assets -- the files were publicly reachable; node column
-  # nil, subjects via participants: the asset plus every then-attached
-  # node):
-  #   "asset_name"            -- flat string
-  #   "content_type"          -- flat string
-  #   "path"                  -- public original path, flat string
-  #   "detached_from"         -- array of unique_names, only when any
-  #   "headline_removed_from" -- array of unique_names, only when any
+  # "asset_destroy" (witnessed asset deletion; the files were publicly
+  # reachable, so an entry is always written. Destruction is refused
+  # while the asset is attached to any current row, so no node is ever
+  # affected: node column nil, the asset the sole participant):
+  #   "asset_name"   -- flat string
+  #   "content_type" -- flat string
+  #   "path"         -- public original path, flat string
   #
   # "otp_enroll" / "otp_disable" / "otp_reset" (second-factor
   # lifecycle) and "user_create" / "user_deactivate" /
diff --git a/config/locales/de.yml b/config/locales/de.yml
index 58367aaf..92a46768 100644
--- a/config/locales/de.yml
+++ b/config/locales/de.yml
@@ -155,7 +155,7 @@ de:
         asset:
           attributes:
             base:
-              not_permitted: "Dieses Asset ist an geschützte Seiten angehängt; nur die Redaktion darf es löschen"
+              destroy_while_attached: "Ein Asset, das noch an Seiten angehängt ist, kann nicht gelöscht werden. Bitte entferne es zuerst im Editor"
             upload:
               unreadable_image: "konnte nicht als Bild gelesen werden. Möglicherweise ist sie beschädigt oder hat ein anderes Format, als der Name vermuten lässt."
         event:
@@ -218,11 +218,7 @@ de:
     revision_restored: "wiederhergestellt am %{date} von %{actor}"
     asset_history: "Chronik"
     asset_create: "%{actor} hat das Asset „%{asset}“ hochgeladen"
-    asset_attach: "%{actor} hat „%{asset}“ an %{subject} angehängt"
-    asset_attach_headline: "%{actor} hat „%{asset}“ als Aufmacher an %{subject} angehängt"
     asset_destroy: "%{actor} hat das Asset „%{asset}“ gelöscht"
-    asset_destroy_detached: "— entfernt von %{paths}"
-    asset_destroy_headlines: "(war Aufmacher von %{paths})"
     detail_assets_added: "Anhänge hinzugefügt: %{names}"
     detail_assets_removed: "Anhänge entfernt: %{names}"
     assets_reordered: "Anhänge umsortiert"
@@ -814,6 +810,7 @@ de:
       locked_prefix: "Die Seite ist von %{login} gesperrt —"
       locked_link: "dort entsperren"
       locked_suffix: ", dann von der Asset-Seite aus erneut anhängen."
+      resolve_link: "Seite öffnen, um den Bearbeitungskonflikt zu lösen"
       headline_prefix: "Der bestehende Aufmacher der Seite wurde behalten —"
       headline_link: "dort ändern"
       headline_suffix: ", falls gewünscht."
diff --git a/config/locales/en.yml b/config/locales/en.yml
index 4bdca31b..458a4069 100644
--- a/config/locales/en.yml
+++ b/config/locales/en.yml
@@ -106,7 +106,7 @@ en:
         asset:
           attributes:
             base:
-              not_permitted: "Only Redaktion members may change assets related to nodes in this section"
+              destroy_while_attached: "Cannot destroy an asset that is still attached to pages; remove it in each page's editor first"
             upload:
               unreadable_image: "could not be read as an image. It may be damaged, or not the format its name suggests."
         event:
@@ -170,11 +170,7 @@ en:
     revision_restored: "restored %{date} by %{actor}"
     asset_history: "History"
     asset_destroy: "%{actor} destroyed asset “%{asset}”"
-    asset_destroy_detached: "— detached from %{paths}"
-    asset_destroy_headlines: "(was the headline of %{paths})"
     asset_create: "%{actor} uploaded asset “%{asset}”"
-    asset_attach: "%{actor} attached “%{asset}” to %{subject}"
-    asset_attach_headline: "%{actor} attached “%{asset}” to %{subject} as its headline"
     detail_assets_added: "attachments added: %{names}"
     detail_assets_removed: "attachments removed: %{names}"
     assets_reordered: "attachments reordered"
@@ -766,6 +762,7 @@ en:
       locked_prefix: "The page is locked by %{login} —"
       locked_link: "unlock it there"
       locked_suffix: " first, then attach from this asset's page."
+      resolve_link: "Open the page to resolve this"
       headline_prefix: "The page's existing headline was kept —"
       headline_link: "change it there"
       headline_suffix: " if needed."
diff --git a/test/models/asset_destroy_test.rb b/test/models/asset_destroy_test.rb
index 2f38d692..54512b16 100644
--- a/test/models/asset_destroy_test.rb
+++ b/test/models/asset_destroy_test.rb
@@ -8,29 +8,6 @@ class AssetDestroyTest < ActiveSupport::TestCase
                             :upload_content_type => "image/png")
   end
 
-  test "destroying an attached asset logs nodes and asset as participants" do
-    node = Node.root.children.create!(:slug => "asset_destroy_attached")
-    node.attach_asset!(@asset, :user => @user)
-
-    @asset.destroy_witnessed!(:user => @user)
-
-    action = NodeAction.where(:action => "asset_destroy").last
-    subjects = action.action_participants.map { |p| [p.subject_type, p.subject_id] }
-    assert_includes subjects, ["Asset", @asset.id]
-    assert_includes subjects, ["Node", node.id]
-    assert_equal [node.unique_name], action.metadata["detached_from"]
-  end
-
-  test "records which nodes lost their headline" do
-    node = Node.root.children.create!(:slug => "asset_destroy_headline")
-    node.attach_asset!(@asset, :user => @user, :headline => true)
-
-    @asset.destroy_witnessed!(:user => @user)
-
-    action = NodeAction.where(:action => "asset_destroy").last
-    assert_equal [node.unique_name], action.metadata["headline_removed_from"]
-  end
-
   test "an unattached asset is still witnessed" do
     @asset.destroy_witnessed!(:user => @user)
 
@@ -49,16 +26,24 @@ class AssetDestroyTest < ActiveSupport::TestCase
     assert_nil action.action_participants.first.subject
   end
 
-  test "destroying an asset attached to a restricted node needs the redaktion role" do
-    editor = User.create!(:login => "asset_gate", :email => "ag@example.com",
-                          :password => "secret", :password_confirmation => "secret")
-    updates = Node.root.children.create!(:slug => "updates")
-    node    = updates.children.create!(:slug => "gated-attachment")
-    node.reload.attach_asset!(@asset, :user => nil)
+  test "destruction is refused while the asset is attached" do
+    node = Node.root.children.create!(:slug => "asset_destroy_attached")
+    node.attach_asset!(@asset, :user => @user)
+
+    assert_raises(ActiveRecord::RecordInvalid) { @asset.destroy_witnessed!(:user => @user) }
+    assert Asset.exists?(@asset.id)
+    assert_equal 0, NodeAction.where(:action => "asset_destroy").count
+  end
+
+  test "an attachment on a draft alone is enough to refuse" do
+    node = Node.root.children.create!(:slug => "asset_destroy_draft_only")
+    node.attach_asset!(@asset, :user => @user)
+    node.publish_draft!(@user)
+    node.lock_for_editing!(@user)
+    node.create_new_draft(@user)
+    node.head.related_assets.destroy_all
 
-    error = assert_raises(ActiveRecord::RecordInvalid) { @asset.destroy_witnessed!(:user => editor) }
-    assert_includes error.message,
-                    I18n.t("activerecord.errors.models.asset.attributes.base.not_permitted")
+    assert_raises(ActiveRecord::RecordInvalid) { @asset.destroy_witnessed!(:user => @user) }
     assert Asset.exists?(@asset.id)
   end
 end