Condense the action log into a scannable table

Several minor improvements to the action log presentation:
* Now a table with date and human readable presentation as
  rows is displayed
* If no changes in a title were detected, the old version
  is omitted
* The "inferred" flag is demoted to the end of the line
* You can zoom in on the node's history directly from a
  log line
* byline for the first publish action is preserved
* Revisions are directly linked to when a new one i
  published

9 files changed, 222 insertions, 71 deletions

diff --git a/app/helpers/node_actions_helper.rb b/app/helpers/node_actions_helper.rb
index 55ca982..996f98d 100644
--- a/app/helpers/node_actions_helper.rb
+++ b/app/helpers/node_actions_helper.rb
@@ -16,8 +16,57 @@ module NodeActionsHelper
        :action => h(action.action), :subject => subject_ref(action)).html_safe
   end
 
+
+  def action_details? action
+    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
+       abstract_changed body_changed].any? { |key| m[key].present? }
+  end
+
+  # Plain strings by design -- safe_join in the template escapes them.
+  def default_locale_changes action
+    m = action.metadata
+    items = []
+    if m["title"].is_a?(Hash) && m.dig("title", "from") && m.dig("title", "from") != m.dig("title", "to")
+      items << t("node_actions.detail_title",
+                  :from => m.dig("title", "from"), :to => m.dig("title", "to"))
+    end
+    items << t("node_actions.detail_author",
+                :from => m.dig("author", "from"), :to => m.dig("author", "to")) if m["author"]
+    items << t("node_actions.detail_tags",
+                :from => Array(m.dig("tags", "from")).join(", "),
+                :to   => Array(m.dig("tags", "to")).join(", ")) if m["tags"]
+    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"]
+    items << t("node_actions.assets_changed")   if m["assets_changed"]
+    items
+  end
+
+  def translation_changes diff
+    case diff["status"]
+    when "added"   then [t("node_actions.locale_added",   :title => diff.dig("title", "to"))]
+    when "removed" then [t("node_actions.locale_removed", :title => diff.dig("title", "from"))]
+    else
+      items = []
+      items << t("node_actions.detail_title",
+                  :from => diff.dig("title", "from"), :to => diff.dig("title", "to")) if diff["title"]
+      items << t("node_actions.abstract_changed") if diff["abstract_changed"]
+      items << t("node_actions.body_changed")     if diff["body_changed"]
+      items
+    end
+  end
+
   private
 
+  def revision_ref action, key
+    label = t(key)
+    return label unless action.node && action.page
+    link_to(label, node_revision_path(action.node, action.page))
+  end
+
   def actor_ref action
     action.user ? link_to(h(action.actor_name), admin_log_path(:user_id => action.user_id))
                 : h(action.actor_name)
@@ -29,17 +78,20 @@ module NodeActionsHelper
   end
 
   def summarize_publish action
-    key = if action.metadata["via"] == "revision"
-            "node_actions.publish_rollback"
-          elsif action.metadata.dig("title", "from").nil?
-            "node_actions.publish_first"
-          else
-            "node_actions.publish"
-          end
-
-    t(key, :actor => actor_ref(action), :subject => subject_ref(action),
-            :from => h(action.metadata.dig("title", "from")),
-            :to   => h(action.metadata.dig("title", "to"))).html_safe
+    if action.metadata["via"] == "revision"
+      t("node_actions.publish_rollback",
+         :actor => actor_ref(action), :subject => subject_ref(action),
+         :revision => revision_ref(action, "node_actions.revision_earlier")).html_safe
+    elsif action.metadata.dig("title", "from").nil?
+      author = action.metadata.dig("author", "to")
+      key = author ? "node_actions.publish_first_with_author" : "node_actions.publish_first"
+      t(key, :actor => actor_ref(action), :subject => subject_ref(action),
+              :author => h(author)).html_safe
+    else
+      t("node_actions.publish",
+         :actor => actor_ref(action), :subject => subject_ref(action),
+         :revision => revision_ref(action, "node_actions.revision_new")).html_safe
+    end
   end
 
   def summarize_move action
diff --git a/app/models/node_action.rb b/app/models/node_action.rb
index 39935c7..792ae1e 100644
--- a/app/models/node_action.rb
+++ b/app/models/node_action.rb
@@ -82,21 +82,23 @@ class NodeAction < ApplicationRecord
   # user agent, ever. Success only -- rejected attempts are not
   # logged.
 
-  def self.record!(node:, action:, user: nil, page: nil, locale: nil, **extra)
+  def self.record!(node:, action:, user: nil, page: nil, locale: nil,
+                    occurred_at: nil, inferred_from: nil, **extra)
     create!(
-      :node        => node,
-      :page        => page,
-      :user        => user,
-      :action      => action,
-      :locale      => locale,
-      :occurred_at => Time.now,
-      :metadata    => {
+      :node          => node,
+      :page          => page,
+      :user          => user,
+      :action        => action,
+      :locale        => locale,
+      :occurred_at   => occurred_at || Time.now,
+      :inferred_from => inferred_from,
+      :metadata      => {
         "username"                 => user&.login,
         "human_readable_node_name" => Globalize.with_locale(I18n.default_locale) {
                                         node&.head&.title || node&.draft&.title
                                       },
       }.merge(extra.stringify_keys)
-   )
+    )
   end
 
   # Computes the publish-entry diff between an outgoing head and the
@@ -113,7 +115,10 @@ class NodeAction < ApplicationRecord
     title_of = ->(page) { page&.translations&.find_by(:locale => default)&.title }
     diff = { :title => { "from" => title_of.call(old_page),
                          "to"   => title_of.call(new_page) } }
-    return diff unless old_page
+    unless old_page
+      diff[:author] = { "from" => nil, "to" => new_page.user&.login } if new_page.user
+      return diff
+    end
 
     old_author, new_author = old_page.user&.login, new_page.user&.login
     diff[:author] = { "from" => old_author, "to" => new_author } if old_author != new_author
diff --git a/app/views/node_actions/_change_details.html.erb b/app/views/node_actions/_change_details.html.erb
index 383f53a..2583e8b 100644
--- a/app/views/node_actions/_change_details.html.erb
+++ b/app/views/node_actions/_change_details.html.erb
@@ -1,27 +1,27 @@
 <details class="node_action_details">
   <summary><%= t("node_actions.show_changes") %></summary>
   <table>
-    <% action_entry.metadata["translation_diff"].each do |locale, diff| %>
+    <% if (default_items = default_locale_changes(action_entry)).any? %>
+      <tr>
+        <th><%= I18n.default_locale.to_s.upcase %></th>
+        <td>
+          <%= safe_join(default_items, ", ") %>
+          <% if action_entry.page && action_entry.node %>
+            <%= link_to t("node_actions.view_revision"), node_revision_path(action_entry.node, action_entry.page) %>
+          <% end %>
+        </td>
+      </tr>
+    <% end %>
+    <% (action_entry.metadata["translation_diff"] || {}).each do |locale, diff| %>
       <tr>
         <th><%= locale.upcase %></th>
         <td>
-          <% case diff["status"] %>
-          <% when "added" %>
-            <%= t("node_actions.locale_added", :title => diff.dig("title", "to")) %>
-          <% when "removed" %>
-            <%= t("node_actions.locale_removed", :title => diff.dig("title", "from")) %>
-          <% else %>
-            <% if diff["title"] %>
-              <%= diff.dig("title", "from") %> → <%= diff.dig("title", "to") %>
-            <% end %>
-            <%= t("node_actions.abstract_changed") if diff["abstract_changed"] %>
-            <%= t("node_actions.body_changed") if diff["body_changed"] %>
+          <%= safe_join(translation_changes(diff), ", ") %>
+          <% if action_entry.page && action_entry.node %>
+            <%= link_to t("node_actions.view_revision"), node_revision_path(action_entry.node, action_entry.page, :locale => locale) %>
           <% end %>
         </td>
       </tr>
     <% end %>
   </table>
-  <% if action_entry.page && action_entry.node %>
-    <%= link_to t("node_actions.view_revision"), node_revision_path(action_entry.node, action_entry.page) %>
-  <% end %>
 </details>
diff --git a/app/views/node_actions/index.html.erb b/app/views/node_actions/index.html.erb
index 92740cd..ee06213 100644
--- a/app/views/node_actions/index.html.erb
+++ b/app/views/node_actions/index.html.erb
@@ -6,17 +6,22 @@
 
 <%= will_paginate @actions %>
 
-<ul id="node_action_list">
+<table id="node_action_list">
   <% @actions.each do |action| %>
-    <li class="node_action node_action--<%= action.action %>">
-      <span class="node_action_time"><%= l(action.occurred_at, :format => :ccc) %></span>
-      <% if action.inferred_from %>
-        <span class="node_action_inferred" title="<%= action.inferred_from %>"><%= t("node_actions.backfilled") %></span>
-      <% end %>
-      <span class="node_action_summary"><%= action_summary(action) %></span>
-      <%= render "change_details", :action_entry => action if action.metadata["translation_diff"].present? %>
-    </li>
+    <tr class="node_action node_action--<%= action.action %>">
+      <td class="node_action_time"><%= action.occurred_at.strftime("%Y-%m-%d %H:%M") %></td>
+      <td class="node_action_body">
+        <%= action_summary(action) %>
+        <% if action.node_id && params[:node_id].blank? %>
+          <%= link_to t("node_actions.node_history"), admin_log_path(:node_id => action.node_id), :class => "node_action_zoom" %>
+        <% end %>
+        <% if action.inferred_from %>
+          <span class="node_action_inferred" title="<%= action.inferred_from %>"><%= t("node_actions.backfilled") %></span>
+        <% end %>
+        <%= render "change_details", :action_entry => action if action_details?(action) %>
+      </td>
+    </tr>
   <% end %>
-</ul>
+</table>
 
 <%= will_paginate @actions %>
diff --git a/config/locales/de.yml b/config/locales/de.yml
index 795e4d0..9de6aac 100644
--- a/config/locales/de.yml
+++ b/config/locales/de.yml
@@ -90,9 +90,10 @@ de:
     show_changes: "Änderungen an Übersetzungen"
     view_revision: "Diese Revision ansehen"
     unknown: "%{actor} hat %{action} auf %{subject} aus"
-    publish: "%{actor} hat %{subject} veröffentlicht, (vorher: %{from})"
+    publish: "%{actor} hat %{revision} von %{subject} veröffentlicht"
+    publish_rollback: "%{actor} hat %{subject} auf %{revision} zurückgesetzt"
     publish_first: "%{actor} hat %{subject} zum ersten Mal veröffentlicht"
-    publish_rollback: "%{actor} hat %{subject} auf eine frühere Revision zurückgesetzt (vorher: %{from})"
+    publish_first_with_author: "%{actor} hat %{subject} zum ersten Mal veröffentlicht (Autor: %{author})"
     create: "%{actor} hat %{subject} unter %{path} angelegt"
     move: "%{actor} hat %{subject} von %{from} nach %{to} verschoben"
     discard_autosave: "%{actor} hat ungespeicherte Änderungen an %{subject} verschoben"
@@ -101,3 +102,11 @@ de:
     locale_removed: "Ãœbersetzung entfernt, letzter Titel \"%{title}\""
     abstract_changed: "Abstract geändert"
     body_changed: "Text geändert"
+    revision_new: "eine neue Revision"
+    revision_earlier: "eine frühere Revision"
+    node_history: "Chronik"
+    detail_title: "Titel „%{from}“ → „%{to}“"
+    detail_author: "Autor %{from} → %{to}"
+    detail_tags: "Tags %{from} → %{to}"
+    template_changed: "Template geändert"
+    assets_changed: "Bildliste geändert"
diff --git a/config/locales/en.yml b/config/locales/en.yml
index 8470778..f1856c1 100644
--- a/config/locales/en.yml
+++ b/config/locales/en.yml
@@ -36,21 +36,29 @@ en:
     page_aria_label: "Page %{page}"
 
   node_actions:
-      heading: "Action log"
-      show_all: "Show all entries"
-      backfilled: "backfilled"
-      show_changes: "translation changes"
-      view_revision: "view this revision"
-      actor_self: "you"
-      unknown: "%{actor} did %{action} on %{subject}"
-      publish: "%{actor} published %{subject} (was: %{from})"
-      publish_first: "%{actor} published %{subject} for the first time"
-      publish_rollback: "%{actor} rolled %{subject} back to an earlier revision (was: %{from})"
-      create: "%{actor} created %{subject} at %{path}"
-      move: "%{actor} moved %{subject} from %{from} to %{to}"
-      discard_autosave: "%{actor} discarded unsaved changes on %{subject}"
-      destroy_draft: "%{actor} discarded the draft of %{subject}"
-      locale_added: "translation added, titled \"%{title}\""
-      locale_removed: "translation removed, last titled \"%{title}\""
-      abstract_changed: "abstract changed"
-      body_changed: "body changed"
+    heading: "Action log"
+    show_all: "Show all entries"
+    backfilled: "backfilled"
+    show_changes: "translation changes"
+    view_revision: "view this revision"
+    unknown: "%{actor} did %{action} on %{subject}"
+    publish: "%{actor} published %{revision} of %{subject}"
+    publish_rollback: "%{actor} rolled %{subject} back to %{revision}"
+    publish_first: "%{actor} published %{subject} for the first time"
+    publish_first_with_author: "%{actor} published %{subject} for the first time (author: %{author})"
+    create: "%{actor} created %{subject} at %{path}"
+    move: "%{actor} moved %{subject} from %{from} to %{to}"
+    discard_autosave: "%{actor} discarded unsaved changes on %{subject}"
+    destroy_draft: "%{actor} discarded the draft of %{subject}"
+    locale_added: "translation added, titled \"%{title}\""
+    locale_removed: "translation removed, last titled \"%{title}\""
+    abstract_changed: "abstract changed"
+    body_changed: "body changed"
+    revision_new: "a new revision"
+    revision_earlier: "an earlier revision"
+    node_history: "node history"
+    detail_title: "title \"%{from}\" → \"%{to}\""
+    detail_author: "author %{from} → %{to}"
+    detail_tags: "tags %{from} → %{to}"
+    template_changed: "template changed"
+    assets_changed: "attached images changed"
diff --git a/public/stylesheets/admin.css b/public/stylesheets/admin.css
index 5f0b165..c1273c6 100644
--- a/public/stylesheets/admin.css
+++ b/public/stylesheets/admin.css
@@ -1494,3 +1494,37 @@ div#image_browser ul li {
   gap: 8px;
   flex-wrap: wrap;
 }
+
+/* ============================================================
+   Action log
+   ============================================================ */
+
+#node_action_list td.node_action_time {
+  white-space: nowrap;
+  vertical-align: top;
+  font-family: monospace;
+  font-size: 0.8em;
+  color: #777;
+  padding-right: 1em;
+}
+
+#node_action_list td.node_action_body {
+  vertical-align: top;
+}
+
+.node_action_inferred {
+  opacity: 0.5;
+  font-size: smaller;
+  margin-left: 0.5em;
+}
+
+.node_action_zoom {
+  font-size: smaller;
+  margin-left: 0.5em;
+}
+
+.node_action_details summary {
+  cursor: pointer;
+  font-size: smaller;
+  color: #777;
+}
diff --git a/test/models/helpers/node_actions_helper_test.rb b/test/models/helpers/node_actions_helper_test.rb
index 93bbdf6..5244c19 100644
--- a/test/models/helpers/node_actions_helper_test.rb
+++ b/test/models/helpers/node_actions_helper_test.rb
@@ -21,12 +21,13 @@ class NodeActionsHelperTest < ActionView::TestCase
                                        "human_readable_node_name" => "Subject" }.merge(metadata))
   end
 
-  test "publish renders the title pair" do
+  test "publish renders the revision sentence, not the titles" do
     out = action_summary(entry("publish",
       { "via" => "draft", "title" => { "from" => "Old", "to" => "New" } }))
 
     assert_includes out, "quentin"
-    assert_includes out, "Old"
+    assert_includes out, "new revision"
+    assert_not_includes out, "Old"
   end
 
   test "first publish uses its own sentence" do
@@ -68,10 +69,8 @@ class NodeActionsHelperTest < ActionView::TestCase
 
   test "metadata values are escaped" do
     out = action_summary(entry("publish",
-      { "human_readable_node_name" => "<b>bold</b>",
-        "title" => { "from" => "<script>alert(1)</script>", "to" => "x" } }))
+      { "human_readable_node_name" => "<b>bold</b>" }))
 
-    assert_not_includes out, "<script>"
     assert_not_includes out, "<b>"
   end
 
@@ -82,4 +81,27 @@ class NodeActionsHelperTest < ActionView::TestCase
 
     assert_includes out, "<a "
   end
+
+  test "details are guarded off when nothing but an unchanged title is present" do
+    unchanged = entry("publish", { "title" => { "from" => "Same", "to" => "Same" } })
+    changed   = entry("publish", { "title" => { "from" => "Old",  "to" => "New" } })
+
+    assert_not action_details?(unchanged)
+    assert action_details?(changed)
+  end
+
+  test "first publish with a byline names the author" do
+    out = action_summary(entry("publish",
+      { "title" => { "from" => nil, "to" => "New" },
+        "author" => { "from" => nil, "to" => "quentin" } }))
+
+    assert_includes out, "author"
+  end
+
+  test "create entries with their flat title render and are guarded off details" do
+    e = entry("create", { "title" => "Initial", "path" => "a/b" })
+
+    assert_includes action_summary(e), "a/b"
+    assert_not action_details?(e)
+  end
 end
diff --git a/test/models/node_action_test.rb b/test/models/node_action_test.rb
index 24d8245..b177cca 100644
--- a/test/models/node_action_test.rb
+++ b/test/models/node_action_test.rb
@@ -18,6 +18,12 @@ class NodeActionTest < ActiveSupport::TestCase
     assert_equal({ :title => { "from" => nil, "to" => "Erstausgabe" } }, diff)
   end
 
+  test "first publish carries the byline when the page has one" do
+    diff = NodeAction.head_diff(nil, build_page(:user => users(:quentin)))
+
+    assert_equal({ "from" => nil, "to" => users(:quentin).login }, diff[:author])
+  end
+
   test "title pair is always present, even when unchanged" do
     diff = NodeAction.head_diff(build_page, build_page)
 
@@ -109,4 +115,14 @@ class NodeActionTest < ActiveSupport::TestCase
 
     assert_nil NodeAction.head_diff(old_page, new_page)[:translation_diff]
   end
+
+  test "record! passes provenance and historical timestamps through" do
+    long_ago = 2.years.ago
+    action = NodeAction.record!(:node => nil, :action => "publish",
+                                 :occurred_at => long_ago,
+                                 :inferred_from => "from_page_revision")
+
+    assert_equal "from_page_revision", action.inferred_from
+    assert_in_delta long_ago.to_f, action.occurred_at.to_f, 1.0
+  end
 end