Report every editorial attribute in the revision diff

diff_against gained external_url, published_at and user. The view renders
each metadata section only when that attribute changed, and says so once
when none did.

Metadata now precedes the locale pointer, which speaks only of translations:
above the metadata it read as "nothing changed" on a revision that had moved
the page.

1 files changed, 81 insertions, 52 deletions

diff --git a/app/views/revisions/diff.html.erb b/app/views/revisions/diff.html.erb
index 0fd9ccb9..25f5ad04 100644
--- a/app/views/revisions/diff.html.erb
+++ b/app/views/revisions/diff.html.erb
@@ -48,7 +48,6 @@
   </p>
 <% end %>
 
-
 <% if @available_layer_pairs.present? %>
   <div class="node_action_bar standalone_action_bar">
     <% @available_layer_pairs.each do |pair| %>
@@ -71,21 +70,88 @@
 <% end %>
 
 <div id="diffview">
-  <% current_summary = @locale_summary.find { |s| s[:locale] == @translation_locale } %>
-  <% if current_summary && !current_summary[:changed] %>
-    <% elsewhere = @locale_summary.select { |s| s[:changed] }.map { |s|
-         link_to s[:locale].to_s.upcase, diff_node_revisions_path(@node,
-           start_revision: params[:start_revision], end_revision: params[:end_revision],
-           view: @diff_view, translation_locale: s[:locale]) } %>
-    <p class="diff_unchanged diff_locale_pointer">
-      <% if elsewhere.any? %>
-        <%= t(".unchanged_here_html", :lang => @translation_locale.to_s.upcase,
-              :others => safe_join(elsewhere, ", ")) %>
-      <% else %>
-        <%= t(".unchanged_anywhere") %>
+  <% meta_changed = @diff[:address][:changed] || @diff[:external_url][:changed] ||
+                    @diff[:template_name][:changed] ||
+                    @diff[:published_at][:changed] || @diff[:user][:changed] ||
+                    @diff[:tags][:added].any?   || @diff[:tags][:removed].any? ||
+                    @diff[:assets][:added].any? || @diff[:assets][:removed].any? %>
+
+  <div class="diff_preamble">
+    <div class="diff_meta">
+      <% unless meta_changed %>
+        <p class="diff_unchanged"><%= t(".no_metadata_change") %></p>
+      <% end %>
+
+      <% if @diff[:address][:changed] %>
+        <h3><%= t(".address") %></h3>
+        <p>
+          <del><%= @diff[:address][:from] || t(".none_marker") %></del>
+          <ins><%= @diff[:address][:to] || t(".none_marker") %></ins>
+        </p>
+      <% end %>
+
+      <% if @diff[:external_url][:changed] %>
+        <h3><%= Page.human_attribute_name(:external_url) %></h3>
+        <p>
+          <del><%= @diff[:external_url][:from].presence || t(".none_marker") %></del>
+          <ins><%= @diff[:external_url][:to].presence || t(".none_marker") %></ins>
+        </p>
+      <% end %>
+
+      <% if @diff[:published_at][:changed] %>
+        <h3><%= Page.human_attribute_name(:published_at) %></h3>
+        <p>
+          <del><%= @diff[:published_at][:from] ? admin_datetime(@diff[:published_at][:from]) : t(".none_marker") %></del>
+          <ins><%= @diff[:published_at][:to] ? admin_datetime(@diff[:published_at][:to]) : t(".none_marker") %></ins>
+        </p>
       <% end %>
-    </p>
-  <% end %>
+
+      <% if @diff[:user][:changed] %>
+        <h3><%= Page.human_attribute_name(:user) %></h3>
+        <p>
+          <del><%= @diff[:user][:from]&.login || t(".none_marker") %></del>
+          <ins><%= @diff[:user][:to]&.login || t(".none_marker") %></ins>
+        </p>
+      <% end %>
+
+      <% if @diff[:tags][:added].any? || @diff[:tags][:removed].any? %>
+        <h3><%= Page.human_attribute_name(:tag_list) %></h3>
+        <ul class="diff_set_list">
+          <% @diff[:tags][:added].each do |tag| %><li><ins><%= tag %></ins></li><% end %>
+          <% @diff[:tags][:removed].each do |tag| %><li><del><%= tag %></del></li><% end %>
+        </ul>
+      <% end %>
+
+      <% if @diff[:template_name][:changed] %>
+        <h3><%= Page.human_attribute_name(:template_name) %></h3>
+        <p><del><%= @diff[:template_name][:from] || t(".none_marker") %></del> <ins><%= @diff[:template_name][:to] || t(".none_marker") %></ins></p>
+      <% end %>
+
+      <% if @diff[:assets][:added].any? || @diff[:assets][:removed].any? %>
+        <h3><%= Page.human_attribute_name(:assets) %></h3>
+        <ul class="diff_set_list">
+          <% @diff[:assets][:added].each do |asset| %><li><ins><%= asset.upload_file_name %></ins></li><% end %>
+          <% @diff[:assets][:removed].each do |asset| %><li><del><%= asset.upload_file_name %></del></li><% end %>
+        </ul>
+      <% end %>
+    </div>
+
+    <% current_summary = @locale_summary.find { |s| s[:locale] == @translation_locale } %>
+    <% if current_summary && !current_summary[:changed] %>
+      <% elsewhere = @locale_summary.select { |s| s[:changed] }.map { |s|
+           link_to s[:locale].to_s.upcase, diff_node_revisions_path(@node,
+             start_revision: params[:start_revision], end_revision: params[:end_revision],
+             view: @diff_view, translation_locale: s[:locale]) } %>
+      <p class="diff_unchanged diff_locale_pointer">
+        <% if elsewhere.any? %>
+          <%= t(".unchanged_here_html", :lang => @translation_locale.to_s.upcase,
+                :others => safe_join(elsewhere, ", ")) %>
+        <% else %>
+          <%= t(".unchanged_anywhere") %>
+        <% end %>
+      </p>
+    <% end %>
+  </div>
 
   <% if @diff_view == :side_by_side %>
     <div class="diff_side_by_side">
@@ -114,41 +180,4 @@
     <h3><%= Page.human_attribute_name(:body) %></h3>
     <%= raw @diff[:body] %>
   <% end %>
-
-  <h3><%= t(".address") %></h3>
-  <% if @diff[:address][:changed] %>
-    <p>
-      <del><%= @diff[:address][:from] || t(".none_marker") %></del>
-      <ins><%= @diff[:address][:to] || t(".none_marker") %></ins>
-    </p>
-  <% else %>
-    <p class="diff_unchanged"><%= t(".no_change") %></p>
-  <% end %>
-
-  <h3><%= Page.human_attribute_name(:tag_list) %></h3>
-  <% if @diff[:tags][:added].empty? && @diff[:tags][:removed].empty? %>
-    <p class="diff_unchanged"><%= t(".no_change") %></p>
-  <% else %>
-    <ul class="diff_set_list">
-      <% @diff[:tags][:added].each do |tag| %><li><ins><%= tag %></ins></li><% end %>
-      <% @diff[:tags][:removed].each do |tag| %><li><del><%= tag %></del></li><% end %>
-    </ul>
-  <% end %>
-
-  <h3><%= Page.human_attribute_name(:template_name) %></h3>
-  <% if @diff[:template_name][:changed] %>
-    <p><del><%= @diff[:template_name][:from] || t(".none_marker") %></del> <ins><%= @diff[:template_name][:to] || t(".none_marker") %></ins></p>
-  <% else %>
-    <p class="diff_unchanged"><%= t(".no_change") %></p>
-  <% end %>
-
-  <h3><%= Page.human_attribute_name(:assets) %></h3>
-  <% if @diff[:assets][:added].empty? && @diff[:assets][:removed].empty? %>
-    <p class="diff_unchanged"><%= t(".no_change") %></p>
-  <% else %>
-    <ul class="diff_set_list">
-      <% @diff[:assets][:added].each do |asset| %><li><ins><%= asset.upload_file_name %></ins></li><% end %>
-      <% @diff[:assets][:removed].each do |asset| %><li><del><%= asset.upload_file_name %></del></li><% end %>
-    </ul>
-  <% end %>
 </div>