From 9d32458491d79f8e06a706375030c78282f4427b Mon Sep 17 00:00:00 2001 From: erdgeist Date: Sun, 9 Aug 2026 15:45:56 +0200 Subject: 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. --- app/models/page.rb | 6 ++ app/views/revisions/diff.html.erb | 133 ++++++++++++++++---------- config/locales/de.yml | 5 +- config/locales/en.yml | 5 +- public/stylesheets/admin.css | 6 ++ test/controllers/revisions_controller_test.rb | 22 ++++- test/models/page_test.rb | 28 ++++++ 7 files changed, 148 insertions(+), 57 deletions(-) diff --git a/app/models/page.rb b/app/models/page.rb index 3240057f..c2dc227b 100644 --- a/app/models/page.rb +++ b/app/models/page.rb @@ -277,6 +277,12 @@ class Page < ApplicationRecord text_diffs.merge( address: address_diff_against(other), + external_url: { from: other.external_url, to: external_url, + changed: external_url.presence != other.external_url.presence }, + published_at: { from: other.published_at, to: published_at, + changed: published_at != other.published_at }, + user: { from: other.user, to: user, + changed: user_id != other.user_id }, tags: { added: tag_list.to_a - other.tag_list.to_a, removed: other.tag_list.to_a - tag_list.to_a }, template_name: { from: other.template_name, to: template_name, changed: template_name != other.template_name }, assets: { added: assets.to_a - other.assets.to_a, removed: other.assets.to_a - assets.to_a } 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 @@

<% end %> - <% if @available_layer_pairs.present? %>
<% @available_layer_pairs.each do |pair| %> @@ -71,21 +70,88 @@ <% end %>
- <% 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]) } %> -

- <% 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? %> + +

+
+ <% unless meta_changed %> +

<%= t(".no_metadata_change") %>

+ <% end %> + + <% if @diff[:address][:changed] %> +

<%= t(".address") %>

+

+ <%= @diff[:address][:from] || t(".none_marker") %> + <%= @diff[:address][:to] || t(".none_marker") %> +

+ <% end %> + + <% if @diff[:external_url][:changed] %> +

<%= Page.human_attribute_name(:external_url) %>

+

+ <%= @diff[:external_url][:from].presence || t(".none_marker") %> + <%= @diff[:external_url][:to].presence || t(".none_marker") %> +

+ <% end %> + + <% if @diff[:published_at][:changed] %> +

<%= Page.human_attribute_name(:published_at) %>

+

+ <%= @diff[:published_at][:from] ? admin_datetime(@diff[:published_at][:from]) : t(".none_marker") %> + <%= @diff[:published_at][:to] ? admin_datetime(@diff[:published_at][:to]) : t(".none_marker") %> +

<% end %> -

- <% end %> + + <% if @diff[:user][:changed] %> +

<%= Page.human_attribute_name(:user) %>

+

+ <%= @diff[:user][:from]&.login || t(".none_marker") %> + <%= @diff[:user][:to]&.login || t(".none_marker") %> +

+ <% end %> + + <% if @diff[:tags][:added].any? || @diff[:tags][:removed].any? %> +

<%= Page.human_attribute_name(:tag_list) %>

+
    + <% @diff[:tags][:added].each do |tag| %>
  • <%= tag %>
    • <% end %> + <% @diff[:tags][:removed].each do |tag| %>
  • <%= tag %>
    • <% end %> +
+ <% end %> + + <% if @diff[:template_name][:changed] %> +

<%= Page.human_attribute_name(:template_name) %>

+

<%= @diff[:template_name][:from] || t(".none_marker") %> <%= @diff[:template_name][:to] || t(".none_marker") %>

+ <% end %> + + <% if @diff[:assets][:added].any? || @diff[:assets][:removed].any? %> +

<%= Page.human_attribute_name(:assets) %>

+
    + <% @diff[:assets][:added].each do |asset| %>
  • <%= asset.upload_file_name %>
    • <% end %> + <% @diff[:assets][:removed].each do |asset| %>
  • <%= asset.upload_file_name %>
    • <% end %> +
+ <% end %> +
+ + <% 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]) } %> +

+ <% if elsewhere.any? %> + <%= t(".unchanged_here_html", :lang => @translation_locale.to_s.upcase, + :others => safe_join(elsewhere, ", ")) %> + <% else %> + <%= t(".unchanged_anywhere") %> + <% end %> +

+ <% end %> +
<% if @diff_view == :side_by_side %>
@@ -114,41 +180,4 @@

<%= Page.human_attribute_name(:body) %>

<%= raw @diff[:body] %> <% end %> - -

<%= t(".address") %>

- <% if @diff[:address][:changed] %> -

- <%= @diff[:address][:from] || t(".none_marker") %> - <%= @diff[:address][:to] || t(".none_marker") %> -

- <% else %> -

<%= t(".no_change") %>

- <% end %> - -

<%= Page.human_attribute_name(:tag_list) %>

- <% if @diff[:tags][:added].empty? && @diff[:tags][:removed].empty? %> -

<%= t(".no_change") %>

- <% else %> -
    - <% @diff[:tags][:added].each do |tag| %>
  • <%= tag %>
    • <% end %> - <% @diff[:tags][:removed].each do |tag| %>
  • <%= tag %>
    • <% end %> -
- <% end %> - -

<%= Page.human_attribute_name(:template_name) %>

- <% if @diff[:template_name][:changed] %> -

<%= @diff[:template_name][:from] || t(".none_marker") %> <%= @diff[:template_name][:to] || t(".none_marker") %>

- <% else %> -

<%= t(".no_change") %>

- <% end %> - -

<%= Page.human_attribute_name(:assets) %>

- <% if @diff[:assets][:added].empty? && @diff[:assets][:removed].empty? %> -

<%= t(".no_change") %>

- <% else %> -
    - <% @diff[:assets][:added].each do |asset| %>
  • <%= asset.upload_file_name %>
    • <% end %> - <% @diff[:assets][:removed].each do |asset| %>
  • <%= asset.upload_file_name %>
    • <% end %> -
- <% end %>
diff --git a/config/locales/de.yml b/config/locales/de.yml index 2aec7bfa..3f2eb0ab 100644 --- a/config/locales/de.yml +++ b/config/locales/de.yml @@ -106,6 +106,9 @@ de: tag_list: "Tags" template_name: "Template" assets: "Anhänge" + external_url: "Externe Homepage" + published_at: "Veröffentlichungsdatum" + user: "Autor" menu_item: node_id: "Node-ID" path: "Pfad" @@ -744,7 +747,7 @@ de: revisions_link: "Alle Revisionen" compare_numbered: "Stattdessen zwei nummerierte Revisionen vergleichen" none_marker: "(keins)" - no_change: "Keine Änderung." + no_metadata_change: "Keine Änderungen an den Metadaten." unchanged_here_html: "Keine Änderung in der Übersetzung %{lang} zwischen diesen Revisionen — geändert wurde %{others}." unchanged_anywhere: "Zwischen diesen Revisionen wurde keine Übersetzung geändert." address: "Adresse" diff --git a/config/locales/en.yml b/config/locales/en.yml index 965906df..e8428f69 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -57,6 +57,9 @@ en: tag_list: "Tags" template_name: "Template" assets: "Assets" + external_url: "External homepage" + published_at: "Publication date" + user: "Author" menu_item: node_id: "Node Id" path: "Path" @@ -712,7 +715,7 @@ en: revisions_link: "All revisions" compare_numbered: "Compare two numbered revisions instead" none_marker: "(none)" - no_change: "No change." + no_metadata_change: "No metadata changes." unchanged_here_html: "No change in the %{lang} translation between these revisions — %{others} changed." unchanged_anywhere: "No translation changed between these revisions." address: "Address" diff --git a/public/stylesheets/admin.css b/public/stylesheets/admin.css index 7951f667..b8a01112 100644 --- a/public/stylesheets/admin.css +++ b/public/stylesheets/admin.css @@ -799,6 +799,12 @@ table.revisions_table tr:hover { margin: 0; } +.diff_preamble { + border-bottom: 1px solid var(--hairline); + margin-bottom: 1.5rem; + padding-bottom: 0.5rem; +} + .user_table { width: 100%; max-width: 44rem; diff --git a/test/controllers/revisions_controller_test.rb b/test/controllers/revisions_controller_test.rb index 34f00a4f..d9f49e58 100644 --- a/test/controllers/revisions_controller_test.rb +++ b/test/controllers/revisions_controller_test.rb @@ -151,19 +151,35 @@ class RevisionsControllerTest < ActionController::TestCase assert_select "input[type='hidden'][name='end_revision'][value='draft']" end - test "diffing two revisions also shows tag, template, and asset changes" do + test "diffing shows tag, template, external URL and asset changes" do login_as :quentin find_or_create_draft(@node, @user) - @node.draft.tag_list = "update" - @node.draft.save! + draft = @node.draft + draft.tag_list = "update" + draft.template_name = "title_only" + draft.external_url = "https://example.org/" + draft.save! + draft.related_assets.create!(:asset => Asset.create!(:name => "diffed", + :upload_file_name => "diffed.png", + :upload_content_type => "image/png")) post(:diff, params: { :node_id => @node.id, :start_revision => @node.pages.first.revision, :end_revision => @node.pages.last.revision }) assert_response :success assert_select "h3", Page.human_attribute_name(:tag_list) assert_select "h3", Page.human_attribute_name(:template_name) + assert_select "h3", Page.human_attribute_name(:external_url) assert_select "h3", Page.human_attribute_name(:assets) end + test "a diff with no metadata changes says so once" do + login_as :quentin + + post(:diff, params: { :node_id => @node.id, :start_revision => @node.pages.first.revision, :end_revision => @node.pages.last.revision }) + assert_response :success + assert_select ".diff_meta .diff_unchanged" + assert_select ".diff_meta h3", false + end + test "revisions#index links back to the node" do login_as :quentin get :index, params: { :node_id => @node.id } diff --git a/test/models/page_test.rb b/test/models/page_test.rb index b737e8b0..f095a7e1 100644 --- a/test/models/page_test.rb +++ b/test/models/page_test.rb @@ -302,6 +302,34 @@ class PageTest < ActiveSupport::TestCase assert_equal "title_only", diff[:template_name][:to] end + test "diff_against reports external URL, publication date and author changes" do + n = Node.root.children.create! :slug => "meta_diff_test" + d = find_or_create_draft(n, @user1) + d.external_url = "https://old.example.org/" + d.published_at = Time.utc(2026, 1, 1, 12, 0, 0) + d.save! + n.publish_draft! + + new_author = User.where.not(:id => n.head.user_id).first + d2 = find_or_create_draft(n, @user1) + d2.external_url = "https://new.example.org/" + d2.published_at = Time.utc(2026, 3, 1, 12, 0, 0) + d2.user = new_author + d2.save! + + diff = d2.diff_against(n.head) + + assert diff[:external_url][:changed] + assert_equal "https://old.example.org/", diff[:external_url][:from] + assert_equal "https://new.example.org/", diff[:external_url][:to] + + assert diff[:published_at][:changed] + assert_equal Time.utc(2026, 3, 1, 12, 0, 0).to_i, diff[:published_at][:to].to_i + + assert diff[:user][:changed] + assert_equal new_author.login, diff[:user][:to].login + end + test "diff_against reports added and removed assets by filename" do n = Node.root.children.create! :slug => "asset_diff_test" d = find_or_create_draft(n, @user1) -- cgit v1.3