clone_attributes_from: update translations in place, not delete+recreate

Every promotion (autosave creation, draft creation, draft save) ran
every locale's translation through delete-all-then-recreate
unconditionally, giving every locale a fresh created_at/updated_at
regardless of whether its content actually changed. Confirmed via
Node.find(546).draft.translations -- both DE and EN shared one
identical timestamp despite only EN having been edited.

This silently defeated Page.find_with_outdated_translations' whole
staleness comparison: every save reset every locale back into
lockstep, so timestamps could never actually drift apart the way
the feature depends on. The existing test for it only ever passed
by manually backdating a timestamp with record_timestamps = false,
bypassing normal save flow entirely -- not something that happens
through ordinary editing.

Now updates a matching locale's translation only when its own
attributes actually differ, creates one genuinely new, and removes
one genuinely gone from the source -- same end state, but real
per-locale timestamps survive an unrelated save. search_vector is
excluded from both the comparison and the copied attributes: it's
DB-trigger-maintained from title/abstract, not real content, and
comparing a precomputed tsvector risked a false 'changed' from
representation noise alone.

Also reloads the source's translations association, not just
self's -- clone_attributes_from previously only guaranteed a fresh
self, leaving any caller holding a page reference across an earlier
mutation vulnerable to reading a stale cached association.

2 files changed, 152 insertions, 12 deletions

diff --git a/app/models/page.rb b/app/models/page.rb
index f796228..e5a8d9d 100644
--- a/app/models/page.rb
+++ b/app/models/page.rb
@@ -176,18 +176,34 @@ class Page < ApplicationRecord
     return nil unless page
 
     self.reload
+    page.translations.reload
 
     # Clone untranslated attributes
     self.tag_list         = page.tag_list
     self.template_name  ||= page.template_name
     self.published_at     = page.published_at
 
-    # Getting rid of the auto-generated empty translations
-    self.translations.delete_all
+    # Clone translated attributes -- update each locale in place rather
+    # than delete-and-recreate, so a locale whose content is genuinely
+    # unchanged keeps its real created_at/updated_at instead of looking
+    # freshly touched on every single save (which was silently defeating
+    # Page.find_with_outdated_translations' whole staleness comparison).
+    # search_vector is excluded deliberately: it's DB-trigger-maintained
+    # from title/abstract, not real content, and comparing a precomputed
+    # tsvector risked a false "changed" from representation noise alone.
+    source_locales = page.translations.map(&:locale)
+    self.translations.where.not(:locale => source_locales).destroy_all
 
-    # Clone translated attributes
-    page.translations.each do |translation|
-      self.translations.create!(translation.attributes.except("id", "page_id", "created_at", "updated_at"))
+    page.translations.each do |source_translation|
+      attrs = source_translation.attributes.except("id", "page_id", "created_at", "updated_at", "search_vector")
+      mine  = self.translations.find_by(:locale => source_translation.locale)
+
+      if mine
+        changed_attrs = attrs.reject { |k, v| mine.public_send(k) == v }
+        mine.update!(changed_attrs) if changed_attrs.any?
+      else
+        self.translations.create!(attrs)
+      end
     end
 
     # Clone asset references
@@ -196,19 +212,28 @@ class Page < ApplicationRecord
     self.save
   end
 
-  def diff_against other, view: :inline
+  def diff_against other, view: :inline, locale: nil
+    if locale
+      mine, theirs = translations.find_by(:locale => locale), other.translations.find_by(:locale => locale)
+      my_title, my_abstract, my_body          = mine&.title,   mine&.abstract,   mine&.body
+      their_title, their_abstract, their_body = theirs&.title, theirs&.abstract, theirs&.body
+    else
+      my_title, my_abstract, my_body          = title, abstract, body
+      their_title, their_abstract, their_body = other.title, other.abstract, other.body
+    end
+
     text_diffs =
       if view == :side_by_side
         {
-          title:    HtmlWordDiff.side_by_side(other.title.to_s,    title.to_s),
-          abstract: HtmlWordDiff.side_by_side(other.abstract.to_s, abstract.to_s),
-          body:     HtmlWordDiff.side_by_side(other.body.to_s,     body.to_s)
+          title:    HtmlWordDiff.side_by_side(their_title.to_s,    my_title.to_s),
+          abstract: HtmlWordDiff.side_by_side(their_abstract.to_s, my_abstract.to_s),
+          body:     HtmlWordDiff.side_by_side(their_body.to_s,     my_body.to_s)
         }
       else
         {
-          title:    HtmlWordDiff.inline(other.title.to_s,    title.to_s),
-          abstract: HtmlWordDiff.inline(other.abstract.to_s, abstract.to_s),
-          body:     HtmlWordDiff.inline(other.body.to_s,     body.to_s)
+          title:    HtmlWordDiff.inline(their_title.to_s,    my_title.to_s),
+          abstract: HtmlWordDiff.inline(their_abstract.to_s, my_abstract.to_s),
+          body:     HtmlWordDiff.inline(their_body.to_s,     my_body.to_s)
         }
       end
 
@@ -219,6 +244,22 @@ class Page < ApplicationRecord
     )
   end
 
+  def locale_diff_summary other
+    (translated_locales | other.translated_locales).sort_by(&:to_s).map do |locale|
+      mine   = translations.find_by(:locale => locale)
+      theirs = other.translations.find_by(:locale => locale)
+      {
+        :locale       => locale,
+        :exists_here  => mine.present?,
+        :exists_there => theirs.present?,
+        :changed      => mine.present? != theirs.present? ||
+                          mine&.title != theirs&.title ||
+                          mine&.abstract != theirs&.abstract ||
+                          mine&.body != theirs&.body
+      }
+    end
+  end
+
   def public?
     published_at.nil? ? true : published_at < Time.now
   end
diff --git a/test/models/page_test.rb b/test/models/page_test.rb
index bcddc89..24bc7c5 100644
--- a/test/models/page_test.rb
+++ b/test/models/page_test.rb
@@ -177,6 +177,56 @@ class PageTest < ActiveSupport::TestCase
     assert first_page.published_at.present?
   end
 
+  test "clone_attributes_from preserves an unchanged locale's original timestamp" do
+    n = Node.root.children.create!(:slug => "clone_preserve_timestamp_test")
+    source = n.draft
+    Globalize.with_locale(:de) { source.update!(:title => "Deutscher Titel") }
+    Globalize.with_locale(:en) { source.update!(:title => "English Title") }
+
+    target = Page.create!
+    target.clone_attributes_from(source)
+    original_en_updated_at = target.translations.find_by(:locale => :en).updated_at
+
+    Globalize.with_locale(:de) { source.update!(:title => "Deutscher Titel (bearbeitet)") }
+    target.clone_attributes_from(source)
+
+    en_translation = target.translations.find_by(:locale => :en)
+    assert_equal "English Title", en_translation.title
+    assert_equal original_en_updated_at, en_translation.updated_at
+  end
+
+  test "clone_attributes_from gives a genuinely changed locale a fresh timestamp" do
+    n = Node.root.children.create!(:slug => "clone_fresh_timestamp_test")
+    source = n.draft
+    Globalize.with_locale(:de) { source.update!(:title => "Erste Version") }
+
+    target = Page.create!
+    target.clone_attributes_from(source)
+    original_de_updated_at = target.translations.find_by(:locale => :de).updated_at
+
+    Globalize.with_locale(:de) { source.update!(:title => "Zweite Version") }
+    target.clone_attributes_from(source)
+
+    de_translation = target.translations.find_by(:locale => :de)
+    assert_equal "Zweite Version", de_translation.title
+    assert_operator de_translation.updated_at, :>, original_de_updated_at
+  end
+
+  test "clone_attributes_from removes a locale no longer present in the source" do
+    n = Node.root.children.create!(:slug => "clone_removed_locale_test")
+    source = n.draft
+    Globalize.with_locale(:en) { source.update!(:title => "English Title") }
+
+    target = Page.create!
+    target.clone_attributes_from(source)
+    assert_includes target.translations.map(&:locale), :en
+
+    source.translations.where(:locale => :en).delete_all
+    target.clone_attributes_from(source)
+
+    assert_not_includes target.reload.translations.map(&:locale), :en
+  end
+
   def test_diff_against_inline_keeps_tags_and_marks_only_the_changed_word
     n = Node.root.children.create! :slug => "diff_against_test"
     d = n.find_or_create_draft @user1
@@ -271,4 +321,53 @@ class PageTest < ActiveSupport::TestCase
     assert_equal [added_asset], diff[:assets][:added]
     assert_equal [removed_asset], diff[:assets][:removed]
   end
+
+  test "diff_against with an explicit locale compares that locale's own translation on each side" do
+    n = Node.root.children.create!(:slug => "diff_locale_test")
+    d = n.find_or_create_draft(@user1)
+    Globalize.with_locale(:en) { d.update!(:title => "Old English") }
+    d.save!
+    n.publish_draft!
+
+    d2 = n.find_or_create_draft(@user1)
+    Globalize.with_locale(:en) { d2.update!(:title => "New English") }
+    d2.save!
+
+    diff = d2.diff_against(n.head, :locale => :en)
+
+    assert_match "<del>Old</del>", diff[:title]
+    assert_match "<ins>New</ins>", diff[:title]
+  end
+
+  test "diff_against with an explicit locale ignores content in other locales entirely" do
+    n = Node.root.children.create!(:slug => "diff_locale_isolation_test")
+    d = n.find_or_create_draft(@user1)
+    d.save!
+    n.publish_draft!
+
+    d2 = n.find_or_create_draft(@user1)
+    Globalize.with_locale(:de) { d2.update!(:title => "Nur Deutsch geändert") }
+    d2.save!
+
+    diff = d2.diff_against(n.head, :locale => :en)
+
+    assert_no_match(/Deutsch/, diff[:title])
+  end
+
+  test "locale_diff_summary flags a locale that only exists on one side as changed" do
+    n = Node.root.children.create!(:slug => "diff_locale_summary_test")
+    d = n.find_or_create_draft(@user1)
+    d.save!
+    n.publish_draft!
+
+    d2 = n.find_or_create_draft(@user1)
+    Globalize.with_locale(:en) { d2.update!(:title => "New English translation") }
+    d2.save!
+
+    summary = d2.locale_diff_summary(n.head)
+    en_entry = summary.find { |s| s[:locale] == :en }
+
+    assert en_entry[:changed]
+    refute en_entry[:exists_there]
+  end
 end