Record asset deltas at publish, with changed assets as participants

8 files changed, 109 insertions, 11 deletions

diff --git a/app/helpers/node_actions_helper.rb b/app/helpers/node_actions_helper.rb
index ed8d0407..4041ffa9 100644
--- a/app/helpers/node_actions_helper.rb
+++ b/app/helpers/node_actions_helper.rb
@@ -49,7 +49,7 @@ module NodeActionsHelper
     m = action.metadata
     return true if m["translation_diff"].present?
     return true if m["title"].is_a?(Hash) && m.dig("title", "from") != m.dig("title", "to")
-    %w[author tags template_changed assets_changed
+    %w[author tags template_changed assets assets_changed assets_reordered
        abstract_changed body_changed].any? { |key| m[key].present? }
   end
 
@@ -69,6 +69,13 @@ module NodeActionsHelper
     items << t("node_actions.abstract_changed") if m["abstract_changed"]
     items << t("node_actions.body_changed")     if m["body_changed"]
     items << t("node_actions.template_changed") if m["template_changed"]
+    if m["assets"]
+      items << t("node_actions.detail_assets_added",
+                  :names => Array(m.dig("assets", "added")).join(", "))   if m.dig("assets", "added")
+      items << t("node_actions.detail_assets_removed",
+                  :names => Array(m.dig("assets", "removed")).join(", ")) if m.dig("assets", "removed")
+    end
+    items << t("node_actions.assets_reordered") if m["assets_reordered"]
     items << t("node_actions.assets_changed")   if m["assets_changed"]
     items
   end
diff --git a/app/models/node.rb b/app/models/node.rb
index 0a9cd2d1..274b2f94 100644
--- a/app/models/node.rb
+++ b/app/models/node.rb
@@ -249,7 +249,9 @@ class Node < ApplicationRecord
         self.head.save!
         self.draft = nil
 
-        NodeAction.record!(:node => self, :page => self.head, :user => current_user,
+        NodeAction.record!(:node => self,
+                            :participants => [self] + NodeAction.changed_assets(outgoing_head, self.head),
+                            :page => self.head, :user => current_user,
                             :action => "publish", :via => "draft",
                             **NodeAction.head_diff(outgoing_head, self.head))
       end
@@ -296,7 +298,9 @@ class Node < ApplicationRecord
       self.head = page
       self.save!
 
-      NodeAction.record!(:node => self, :page => page, :user => current_user,
+      NodeAction.record!(:node => self,
+                          :participants => [self] + NodeAction.changed_assets(outgoing_head, page),
+                          :page => page, :user => current_user,
                           :action => "publish", :via => "revision",
                           **NodeAction.head_diff(outgoing_head, page))
       self
diff --git a/app/models/node_action.rb b/app/models/node_action.rb
index aa52f489..afa2195c 100644
--- a/app/models/node_action.rb
+++ b/app/models/node_action.rb
@@ -35,8 +35,16 @@ class NodeAction < ApplicationRecord
   #   "title"  -- pair, always; "from" null on first publish
   #   "author" -- pair, when the byline changed (incl. first publish)
   #   "tags"   -- pair of arrays, when changed
-  #   "assets_changed", "template_changed",
-  #   "abstract_changed", "body_changed"
+  #   "assets" -- {"added" => [asset names], "removed" => [asset names]},
+  #               keys only when any; a delta, not a pair. The event IS
+  #               the delta, full sets would bloat every entry. Changed
+  #               assets are participants of the entry. Replaces the
+  #               legacy "assets_changed" boolean, which witnessed
+  #               pre-contract entries still carry and the renderer keeps
+  #               understanding. Assets destroyed since leave no trace in
+  #               regenerated deltas, their joins died with them.
+  #   "assets_reordered" -- boolean, set unchanged but gallery order not
+  #   "template_changed", "abstract_changed", "body_changed"
   #            -- the last two for the default locale; page_id links
   #               to the revision for the real diff (never stored)
   #   "translation_diff" -- only when a non-default locale differs:
@@ -155,7 +163,17 @@ class NodeAction < ApplicationRecord
     diff[:tags] = { "from" => old_tags, "to" => new_tags } if old_tags != new_tags
 
     diff[:template_changed] = true if old_page.template_name != new_page.template_name
-    diff[:assets_changed]   = true if old_page.assets.map(&:id) != new_page.assets.map(&:id)
+
+    old_assets, new_assets = old_page.assets.to_a, new_page.assets.to_a
+    added, removed = new_assets - old_assets, old_assets - new_assets
+    if added.any? || removed.any?
+      assets = {}
+      assets["added"]   = added.map   { |a| a.name.presence || a.upload_file_name } if added.any?
+      assets["removed"] = removed.map { |a| a.name.presence || a.upload_file_name } if removed.any?
+      diff[:assets] = assets
+    elsif old_assets.map(&:id) != new_assets.map(&:id)
+      diff[:assets_reordered] = true
+    end
 
     old_t = old_page.translations.find_by(:locale => default)
     new_t = new_page.translations.find_by(:locale => default)
@@ -188,6 +206,16 @@ class NodeAction < ApplicationRecord
     diff
   end
 
+  # The asset records added or removed between an outgoing head and its
+  # replacement -- the participant complement to head_diff's "assets"
+  # names. Empty on first publish, mirroring head_diff, which records
+  # no asset delta when everything is new.
+  def self.changed_assets old_page, new_page
+    return [] unless old_page
+    old_a, new_a = old_page.assets.to_a, new_page.assets.to_a
+    (new_a - old_a) | (old_a - new_a)
+  end
+
   def actor_name
     metadata["username"] || "unknown"
   end
diff --git a/config/locales/de.yml b/config/locales/de.yml
index fff74799..b1594f19 100644
--- a/config/locales/de.yml
+++ b/config/locales/de.yml
@@ -122,6 +122,9 @@ de:
     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"
 
   open_gallery: "Gallerie anzeigen"
   asset_licenses:
diff --git a/config/locales/en.yml b/config/locales/en.yml
index fc9f5d13..649c8812 100644
--- a/config/locales/en.yml
+++ b/config/locales/en.yml
@@ -74,6 +74,9 @@ en:
     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"
 
   open_gallery: "Open gallery"
   asset_licenses:
diff --git a/lib/tasks/node_actions.rake b/lib/tasks/node_actions.rake
index c378db24..645315b5 100644
--- a/lib/tasks/node_actions.rake
+++ b/lib/tasks/node_actions.rake
@@ -13,6 +13,7 @@ namespace :node_actions do
       stale = NodeAction.where.not(:inferred_from => nil)
       stale = stale.where(:node_id => ENV["NODE_ID"]) if ENV["NODE_ID"]
       puts "Removing #{stale.count} previously inferred entries"
+      ActionParticipant.where(:node_action => stale).delete_all
       stale.delete_all
 
       witnessed_creates  = NodeAction.where(:action => "create",  :inferred_from => nil).pluck(:node_id).to_set
@@ -43,6 +44,7 @@ namespace :node_actions do
             diff = NodeAction.head_diff(previous, page)
             NodeAction.record!(
               :node => node, :page => page, :user => page.editor,
+              :participants  => [node] + NodeAction.changed_assets(previous, page),
               :action        => "publish",
               :occurred_at   => page.updated_at,
               :inferred_from => "from_page_revision",
diff --git a/test/models/node_action_test.rb b/test/models/node_action_test.rb
index 849b36f4..4672456a 100644
--- a/test/models/node_action_test.rb
+++ b/test/models/node_action_test.rb
@@ -59,7 +59,7 @@ class NodeActionTest < ActiveSupport::TestCase
     assert_nil NodeAction.head_diff(old_page, build_page(:template_name => "standard_template"))[:template_changed]
   end
 
-  test "assets_changed flag when the attached set differs" do
+  test "asset delta when the attached set differs" do
     asset = Asset.create!(:name => "diff probe",
                           :upload_file_name    => "test_image.png",
                           :upload_content_type => "image/png",
@@ -68,10 +68,9 @@ class NodeActionTest < ActiveSupport::TestCase
     old_page, new_page = build_page, build_page
     new_page.related_assets.create!(:asset_id => asset.id, :position => 1)
 
-
-    diff = NodeAction.head_diff(old_page, new_page.reload)
-    assert diff[:assets_changed]
-    assert_nil NodeAction.head_diff(old_page, old_page)[:assets_changed]
+    diff = NodeAction.head_diff(old_page, new_page)
+    assert diff[:assets].present?
+    assert_nil NodeAction.head_diff(old_page, old_page)[:assets]
   end
 
   test "default-locale abstract and body changes become flags, only when true" do
diff --git a/test/models/node_test.rb b/test/models/node_test.rb
index 0083b088..8bdb90ee 100644
--- a/test/models/node_test.rb
+++ b/test/models/node_test.rb
@@ -728,6 +728,58 @@ class NodeTest < ActiveSupport::TestCase
                  NodeAction.order(:id).last(2).map(&:action).sort
   end
 
+  test "publish records the asset delta with changed assets as participants" do
+    node = Node.root.children.create!(:slug => "publish_asset_delta")
+    kept  = Asset.create!(:name => "Kept",  :upload_content_type => "image/png")
+    added = Asset.create!(:name => "Added", :upload_content_type => "image/png")
+    node.draft.related_assets.create!(:asset => kept)
+    node.publish_draft!(@user1)
+
+    node.lock_for_editing!(@user1)
+    node.create_new_draft(@user1)
+    node.draft.related_assets.create!(:asset => added)
+    node.publish_draft!(@user1)
+
+    action = node.node_actions.where(:action => "publish").order(:id).last
+    assert_equal ["Added"], action.metadata.dig("assets", "added")
+    assert_nil action.metadata.dig("assets", "removed")
+    subjects = action.action_participants.map { |p| [p.subject_type, p.subject_id] }
+    assert_includes subjects, ["Asset", added.id]
+    assert_not_includes subjects, ["Asset", kept.id]
+  end
+
+  test "publish without an asset change writes no assets key" do
+    node = Node.root.children.create!(:slug => "publish_asset_static")
+    node.draft.related_assets.create!(:asset => Asset.create!(:name => "Steady"))
+    node.publish_draft!(@user1)
+
+    node.lock_for_editing!(@user1)
+    node.create_new_draft(@user1)
+    node.publish_draft!(@user1)
+
+    action = node.node_actions.where(:action => "publish").order(:id).last
+    assert_nil action.metadata["assets"]
+    assert_equal [["Node", node.id]],
+                 action.action_participants.map { |p| [p.subject_type, p.subject_id] }
+  end
+
+  test "pure reordering is recorded as assets_reordered" do
+    node = Node.root.children.create!(:slug => "publish_asset_reorder")
+    a1, a2 = Asset.create!(:name => "First"), Asset.create!(:name => "Second")
+    node.draft.related_assets.create!(:asset => a1)
+    node.draft.related_assets.create!(:asset => a2)
+    node.publish_draft!(@user1)
+
+    node.lock_for_editing!(@user1)
+    node.create_new_draft(@user1)
+    node.draft.related_assets.reload.first.move_to_bottom
+    node.publish_draft!(@user1)
+
+    action = node.node_actions.where(:action => "publish").order(:id).last
+    assert action.metadata["assets_reordered"]
+    assert_nil action.metadata["assets"]
+  end
+
   test "restore_revision! logs a publish via revision" do
     node = create_node_with_published_page
     Globalize.with_locale(:de) { node.head.update!(:title => "First") }