From 4072efdc912ecb3b79621fb1838434f792e3a531 Mon Sep 17 00:00:00 2001 From: erdgeist Date: Mon, 13 Jul 2026 19:10:55 +0200 Subject: 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. --- app/models/page.rb | 67 +++++++++++++++++++++++++------- test/models/page_test.rb | 99 ++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 153 insertions(+), 13 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 - page.translations.each do |translation| - self.translations.create!(translation.attributes.except("id", "page_id", "created_at", "updated_at")) + # 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 + + 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 "Old", diff[:title] + assert_match "New", 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 -- cgit v1.3