From 4dd49b1eebb0a99d3aee66b7eca539c87a9c6332 Mon Sep 17 00:00:00 2001 From: erdgeist Date: Sun, 28 Jun 2026 04:43:28 +0200 Subject: Phase 2: chapter nodes, aggregate partial, fixes - _chapter.html.erb: new partial for erfa/chaostreff aggregated lists; renders title, location, external_url, sanitized body - content_helper: fix aggregate attr regex to allow hyphens in values (erfa-detail tag was silently dropped); add debug logging (remove) - page.rb: suppress libxml stderr noise in rewrite_links_in_body - db/seeds/chapters.rb: one-shot seed script for erfa and chaostreff chapter nodes under parent nodes 548/549; creates bilingual pages, external_url, primary events with RRULEs where known Note: run Node.rebuild!(false) after execution to fix lft/rgt values --- app/models/page.rb | 8 ++++++++ 1 file changed, 8 insertions(+) (limited to 'app/models/page.rb') diff --git a/app/models/page.rb b/app/models/page.rb index e6baf20..385b3f6 100644 --- a/app/models/page.rb +++ b/app/models/page.rb @@ -63,6 +63,14 @@ class Page < ApplicationRecord end end + if options[:order_by] == "title" + return scope + .joins(:translations) + .where(page_translations: { locale: I18n.locale }) + .order("page_translations.title #{options[:order_direction]}") + .paginate(:page => page, :per_page => options[:limit]) + end + scope.order("#{options[:order_by]} #{options[:order_direction]}") .paginate(:page => page, :per_page => options[:limit]) end -- cgit v1.3 From 51629c5c42270a346885057a441095c964101cc1 Mon Sep 17 00:00:00 2001 From: erdgeist Date: Tue, 30 Jun 2026 03:55:42 +0200 Subject: Fix events CRUD for standalone events and add events to admin menu - event_params now permits title, description, is_primary - event_information helper lists all node.events, not just the first - Occurrence.generate handles nil node (standalone events) - Page.aggregate order_by title uses correlated subquery to avoid GROUP BY conflict with tag-filter path; order_direction whitelisted to ASC/DESC to prevent SQL injection - Events link added to admin menu bar - events/index shows title, is_primary; drops latitude/longitude columns --- app/controllers/events_controller.rb | 2 +- app/helpers/nodes_helper.rb | 20 +++++++++----------- app/models/occurrence.rb | 2 +- app/models/page.rb | 8 ++++---- app/views/admin/_menu.html.erb | 1 + app/views/events/index.html.erb | 8 ++++---- 6 files changed, 20 insertions(+), 21 deletions(-) (limited to 'app/models/page.rb') diff --git a/app/controllers/events_controller.rb b/app/controllers/events_controller.rb index f50da3e..3a60cf9 100644 --- a/app/controllers/events_controller.rb +++ b/app/controllers/events_controller.rb @@ -94,6 +94,6 @@ class EventsController < ApplicationController private def event_params - params.require(:event).permit(:start_time, :end_time, :rrule, :custom_rrule, :allday, :url, :latitude, :longitude, :node_id, :location) + params.require(:event).permit(:title, :description, :is_primary, :start_time, :end_time, :rrule, :custom_rrule, :allday, :url, :latitude, :longitude, :node_id, :location) end end diff --git a/app/helpers/nodes_helper.rb b/app/helpers/nodes_helper.rb index d88d12a..4293628 100644 --- a/app/helpers/nodes_helper.rb +++ b/app/helpers/nodes_helper.rb @@ -30,19 +30,17 @@ module NodesHelper end def event_information - if @node.events.first - event = @node.events.first + events = @node.events.order(:start_time) + items = events.map do |event| safe_join([ - "#{event.start_time.to_fs(:db)} - #{event.end_time.to_fs(:db)} > ", - link_to('show', event_path(event)), - ' > ', - link_to('edit', edit_event_path(event)) - ]) - else - safe_join([ - 'no event attached > ', - link_to('add', new_event_path(:node_id => @node.id)) + "#{event.start_time&.to_fs(:db)} - #{event.end_time&.to_fs(:db)} > ", + link_to('edit', edit_event_path(event)), ]) end + safe_join([ + safe_join(items, ' | '), + ' > ', + link_to('add event', new_event_path(:node_id => @node.id)) + ]) end end diff --git a/app/models/occurrence.rb b/app/models/occurrence.rb index 143124f..777be24 100644 --- a/app/models/occurrence.rb +++ b/app/models/occurrence.rb @@ -35,7 +35,7 @@ class Occurrence < ApplicationRecord self.create( :start_time => occurrence, :end_time => (occurrence + duration), - :node_id => node.id, + :node_id => node&.id, :event_id => event.id ) end diff --git a/app/models/page.rb b/app/models/page.rb index 385b3f6..c982c2e 100644 --- a/app/models/page.rb +++ b/app/models/page.rb @@ -63,15 +63,15 @@ class Page < ApplicationRecord end end + direction = %w[ASC DESC].include?(options[:order_direction]&.upcase) ? options[:order_direction].upcase : "ASC" + if options[:order_by] == "title" return scope - .joins(:translations) - .where(page_translations: { locale: I18n.locale }) - .order("page_translations.title #{options[:order_direction]}") + .order(Arel.sql("(SELECT pt.title FROM page_translations pt WHERE pt.page_id = pages.id AND pt.locale = #{ActiveRecord::Base.connection.quote(I18n.locale.to_s)}) #{direction}")) .paginate(:page => page, :per_page => options[:limit]) end - scope.order("#{options[:order_by]} #{options[:order_direction]}") + scope.order("#{options[:order_by]} #{direction}") .paginate(:page => page, :per_page => options[:limit]) end diff --git a/app/views/admin/_menu.html.erb b/app/views/admin/_menu.html.erb index 6dba085..c87c5f7 100644 --- a/app/views/admin/_menu.html.erb +++ b/app/views/admin/_menu.html.erb @@ -4,5 +4,6 @@ search <%= link_to 'Nodes', nodes_path, selected?('nodes') %> <%= link_to 'Assets', assets_path, selected?('assets') %> +<%= link_to 'Events', events_path, selected?('events') %> <%= link_to 'User', users_path, selected?('users') %> <%= link_to 'Navigation', menu_items_path, selected?('menu_items') %> >  diff --git a/app/views/events/index.html.erb b/app/views/events/index.html.erb index 19b21ce..064fa86 100644 --- a/app/views/events/index.html.erb +++ b/app/views/events/index.html.erb @@ -2,27 +2,27 @@ + + - - <% @events.each do |event| %> + + - - -- cgit v1.3 From 51d491a3d67e8c9efde8019ecb8a8e1a8195ec99 Mon Sep 17 00:00:00 2001 From: erdgeist Date: Sun, 5 Jul 2026 21:51:15 +0200 Subject: Add erfa/chaostreff node-kind creation conventions lib/ccc_conventions.rb: NODE_KINDS registry replaces the kind-specific branches previously scattered across nodes_controller#create (tags), unique_path-position check). Each kind declares its parent lookup (a Proc - Update's "update"/"press_release" continue to genuinely find-or-create their year-folder via Update.find_or_create_parent; erfa/chaostreff use a plain find_by_unique_name!, since their parent nodes are fixed and a missing one should fail loudly, not be silently created), its tags, and (new) its default template. Node gains a default_template_name column (migration, with backfill for existing update-tree nodes via node.update? - reusing that method rather than re-deriving its unique_path check in raw SQL). Page#set_template now inherits from node.default_template_name, falling back to the old update?-based check only when the column is blank, and only fills in template_name when nothing's already been explicitly chosen - a deliberate change from the previous behavior, which unconditionally overwrote template_name on every save regardless of manual selection. Node#update? itself is unchanged and still used as-is by admin_controller's sitemap filtering - a genuinely different, still valid use of that check. "generic" stays special-cased in the controller, parametrized by params[:parent_id] at request time - doesn't fit "kind implies a fixed lookup" and isn't in the registry. nodes#new's four hardcoded radio buttons and nodes_controller's kind-specific case/when branches are both replaced by iterating/looking up this one registry - adding a new kind now means one new hash entry, not four scattered edits. --- app/controllers/nodes_controller.rb | 35 ++++++++++++++++++++--------------- app/models/page.rb | 5 ++--- config/routes.rb | 4 ++++ 3 files changed, 26 insertions(+), 18 deletions(-) (limited to 'app/models/page.rb') diff --git a/app/controllers/nodes_controller.rb b/app/controllers/nodes_controller.rb index 494887d..1e1def2 100644 --- a/app/controllers/nodes_controller.rb +++ b/app/controllers/nodes_controller.rb @@ -33,17 +33,17 @@ class NodesController < ApplicationController @node = Node.new @node.parent_id = find_parent - @node.slug = params[:title].parameterize.to_s - + @node.slug = slug_for(params[:title]) + + config = CccConventions::NODE_KINDS[params[:kind]] + if @node.save @node.draft.update(:title => params[:title]) - case params[:kind] - when "update" - @node.draft.tag_list.add("update") - when "press_release" - @node.draft.tag_list.add("update", "pressemitteilung") - end + Array(config && config[:tags]).each { |t| @node.draft.tag_list.add(t) } @node.draft.save! + + @node.update!(default_template_name: config[:template]) if config && config[:template] + redirect_to(edit_node_path(@node)) else render :new @@ -103,9 +103,17 @@ class NodesController < ApplicationController redirect_to node_path(@node) end - + + def parameterize_preview + render plain: slug_for(params[:title]) + end + private + def slug_for(title) + title.to_s.parameterize + end + def node_params params.fetch(:node, {}).permit(:slug, :parent_id, :staged_slug, :staged_parent_id) end @@ -120,18 +128,15 @@ class NodesController < ApplicationController def find_parent case params[:kind] - when "top_level" - Node.root.id - when "update" - Update.find_or_create_parent.id - when "press_release" - Update.find_or_create_parent.id when "generic" if params[:parent_id] && Node.find(params[:parent_id]) params[:parent_id] else nil end + else + config = CccConventions::NODE_KINDS[params[:kind]] + config && config[:parent] ? config[:parent].call.id : nil end end end diff --git a/app/models/page.rb b/app/models/page.rb index c982c2e..20461bf 100644 --- a/app/models/page.rb +++ b/app/models/page.rb @@ -225,9 +225,8 @@ class Page < ApplicationRecord end def set_template - if node && node.update? - self.template_name = "update" - end + return if template_name.present? + self.template_name = node&.default_template_name || (node&.update? ? "update" : nil) end def rewrite_links_in_body diff --git a/config/routes.rb b/config/routes.rb index a7775b3..cf4733c 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -31,6 +31,10 @@ Cccms::Application.routes.draw do end resources :nodes do + collection do + get :parameterize_preview + end + member do put :unlock put :publish -- cgit v1.3 From b547decd1f85b43d5fb9e86cd7462b455059b1d1 Mon Sep 17 00:00:00 2001 From: erdgeist Date: Mon, 6 Jul 2026 06:05:06 +0200 Subject: Add preview_token to pages New column + unique index, plus ensure_preview_token!/revoke_preview_token! on Page. Generated lazily (only when explicitly requested) rather than via has_secure_token's default auto-generate-on-create, so a live, shareable secret isn't silently minted for every page ever created. --- app/models/page.rb | 9 +++++++++ db/migrate/20260706030547_add_preview_token_to_pages.rb | 6 ++++++ 2 files changed, 15 insertions(+) create mode 100644 db/migrate/20260706030547_add_preview_token_to_pages.rb (limited to 'app/models/page.rb') diff --git a/app/models/page.rb b/app/models/page.rb index 20461bf..4d1e94a 100644 --- a/app/models/page.rb +++ b/app/models/page.rb @@ -136,6 +136,15 @@ class Page < ApplicationRecord "/#{node.unique_name}" end + def ensure_preview_token! + update!(preview_token: SecureRandom.urlsafe_base64(24)) unless preview_token.present? + preview_token + end + + def revoke_preview_token! + update!(preview_token: nil) + end + def clone_attributes_from page return nil unless page diff --git a/db/migrate/20260706030547_add_preview_token_to_pages.rb b/db/migrate/20260706030547_add_preview_token_to_pages.rb new file mode 100644 index 0000000..794cbb2 --- /dev/null +++ b/db/migrate/20260706030547_add_preview_token_to_pages.rb @@ -0,0 +1,6 @@ +class AddPreviewTokenToPages < ActiveRecord::Migration[8.1] + def change + add_column :pages, :preview_token, :string + add_index :pages, :preview_token, unique: true + end +end -- cgit v1.3 From 527376039c527eb8f559c5e6da76429bc3f3ee4f Mon Sep 17 00:00:00 2001 From: erdgeist Date: Mon, 6 Jul 2026 19:40:48 +0200 Subject: Add tree-position scoping to Page.aggregate New :children option ("direct" or "all") on the [aggregate] shortcode, composable with the existing :tags filter - both apply as independent successive .where clauses, ANDing together automatically, no special casing needed. content_helper.rb passes the current @page.node through as :node whenever :children is requested, since Page.aggregate has no way to resolve "which node" from the shortcode's bare option strings on its own. Guarded so nothing changes for any existing [aggregate] shortcode that never uses children= - :node stays nil, the new branch never fires. "all" resolves via node.descendants.pluck(:id) rather than embedding the descendants relation directly as a subquery. Not proven strictly necessary - extensive debugging this session initially pointed at a subquery/table-alias collision, but every actual failure traced instead to accumulated test-node debris left in the dev DB by earlier interrupted runs, confirmed by re-running against a cleaned database. Kept anyway as a one-round-trip-cheaper, more defensive default regardless. --- app/helpers/content_helper.rb | 1 + app/models/page.rb | 6 ++++++ 2 files changed, 7 insertions(+) (limited to 'app/models/page.rb') diff --git a/app/helpers/content_helper.rb b/app/helpers/content_helper.rb index 6043089..6bcd437 100644 --- a/app/helpers/content_helper.rb +++ b/app/helpers/content_helper.rb @@ -108,6 +108,7 @@ module ContentHelper end options[:partial] = select_partial(options[:partial]) + options[:node] = @page.node if options[:children].present? sanitize(content.sub(tag, render_collection(options)), :attributes => cccms_attributes) else diff --git a/app/models/page.rb b/app/models/page.rb index 4d1e94a..6fff7d6 100644 --- a/app/models/page.rb +++ b/app/models/page.rb @@ -63,6 +63,12 @@ class Page < ApplicationRecord end end + if options[:node] && options[:children] == "direct" + scope = scope.where(nodes: { parent_id: options[:node].id }) + elsif options[:node] && options[:children] == "all" + scope = scope.where(nodes: { id: options[:node].descendants.pluck(:id) }) + end + direction = %w[ASC DESC].include?(options[:order_direction]&.upcase) ? options[:order_direction].upcase : "ASC" if options[:order_by] == "title" -- cgit v1.3 From 131658e53f34964221e7f031b91f9dc184fe7481 Mon Sep 17 00:00:00 2001 From: erdgeist Date: Wed, 8 Jul 2026 11:51:45 +0200 Subject: Make sure that page title actually is saved through to ORM --- app/models/page.rb | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) (limited to 'app/models/page.rb') diff --git a/app/models/page.rb b/app/models/page.rb index 6fff7d6..fff044e 100644 --- a/app/models/page.rb +++ b/app/models/page.rb @@ -234,9 +234,7 @@ class Page < ApplicationRecord private def set_page_title - if title.nil? - title = "Untitled" - end + self.title = "Untitled" if title.nil? end def set_template -- cgit v1.3 From 9c3217df50d462d6be4399e3e654d591bf704df7 Mon Sep 17 00:00:00 2001 From: erdgeist Date: Thu, 9 Jul 2026 15:29:40 +0200 Subject: Fix malformed-HTML fallout across RSS, link rewriting, and flash messaging Editors' TinyMCE output can contain unclosed void elements like
, which is valid HTML5 but invalid XML -- three different places assumed the stricter rule and broke or silently misbehaved on the looser one. The Atom feed's block required real, well-formed XML structure but was handed a raw, unescaped body string; switched to type="html" with CGI.escapeHTML, matching how title/summary already handle the same content. rewrite_links_in_body used libxml's strict XML parser to rewrite internal links to be locale-prefixed, which raised on exactly this class of malformed markup -- silently, since the whole method was wrapped in rescue; nil, meaning the link rewrite (not the save) quietly failed with no error anywhere. Replaced with Nokogiri's lenient HTML parser, which repairs malformed void elements rather than rejecting them; also drops the bare rescue now that the actual failure mode it was guarding against shouldn't occur, and fixes two adjacent bugs found while in this method: a typo'd /sytem/uploads/ regex that could never match, and a missing https:// exclusion alongside the existing http:// one. Also addresses stale flash messaging surfaced while testing the above: update's save confirmation was being clobbered by edit's own "locked and ready" notice on the very next request, since nothing distinguished a fresh lock acquisition from a redirect back after saving. The save confirmation now names the next step (publish from Status) and flags a stale translation if one exists, using Page#outdated_translations?, already present but previously unused by any controller. --- app/controllers/nodes_controller.rb | 18 ++++++++++++++--- app/models/page.rb | 39 ++++++++++++++----------------------- app/views/layouts/admin.html.erb | 7 +++++++ app/views/rss/updates.xml.builder | 4 +--- public/stylesheets/admin.css | 9 +++++++++ 5 files changed, 47 insertions(+), 30 deletions(-) (limited to 'app/models/page.rb') diff --git a/app/controllers/nodes_controller.rb b/app/controllers/nodes_controller.rb index 72d4a3e..38d42d9 100644 --- a/app/controllers/nodes_controller.rb +++ b/app/controllers/nodes_controller.rb @@ -65,15 +65,16 @@ class NodesController < ApplicationController end def edit + freshly_locked = @node.lock_owner != current_user @node.lock_for_editing!( current_user ) @page = @node.autosave || @node.draft || @node.head - flash.now[:notice] = "Node locked and ready to edit" unless @node.autosave - if @node.autosave flash.now[:notice] = "This page has unsaved changes from a previous session, shown below. " \ "Save to keep them, or use \"Discard changes\" below to go back to the last saved version." + elsif freshly_locked + flash.now[:notice] = "Node locked and ready to edit" end rescue LockedByAnotherUser => e flash[:error] = e.message @@ -85,7 +86,18 @@ class NodesController < ApplicationController @node.autosave!( page_params.merge(:tag_list => params[:tag_list]), current_user ) @node.save_draft!(current_user) - flash[:notice] = "Draft has been saved: #{Time.now}" + flash[:notice] = "Draft saved. Publish your changes in the Status section once you're done." + flash[:status_path] = node_path(@node) + + if @node.draft.translated_locales.size > 1 + stale_locale = @node.draft.translated_locales.find do |locale| + @node.draft.outdated_translations?(locale: locale) + end + if stale_locale + flash[:stale_locale] = stale_locale + flash[:stale_locale_path] = edit_node_path(@node, locale: stale_locale) + end + end if params[:commit] == "Save + Unlock + Exit" @node.unlock! diff --git a/app/models/page.rb b/app/models/page.rb index fff044e..1a98e08 100644 --- a/app/models/page.rb +++ b/app/models/page.rb @@ -243,31 +243,22 @@ class Page < ApplicationRecord end def rewrite_links_in_body - begin - if self.body - tmp_body = "
#{self.body}
" - xml_string = XML::Parser.string( tmp_body ) - xml_doc = xml_string.parse - links = xml_doc.find("//a[not(starts-with(@href, 'http://'))]") - links = links.reject { |l| l[:href] =~ /system\/uploads/ } - locales = I18n.available_locales.reject {|l| l == :root} - - links.each do |link| - unless locales.include? link[:href].slice(1,2).to_sym - unless link[:href] =~ /sytem\/uploads/ - link[:href] = link[:href].sub(/^\//, "/#{I18n.locale}/") - end - end - end - - tmp_body = xml_doc.to_s.gsub(/(\n\|\<\/div\>\n)/, "") - tmp_body.gsub!("", "") - - self.body = tmp_body + return unless self.body + + doc = Nokogiri::HTML::DocumentFragment.parse(self.body) + locales = I18n.available_locales.reject { |l| l == :root } + + doc.css('a').each do |link| + href = link['href'] + next unless href + next if href.start_with?('http://', 'https://') + next if href =~ /system\/uploads/ + + unless locales.include?(href.slice(1, 2)&.to_sym) + link['href'] = href.sub(/^\//, "/#{I18n.locale}/") end - rescue - nil end - end + self.body = doc.to_html + end end diff --git a/app/views/layouts/admin.html.erb b/app/views/layouts/admin.html.erb index 340eaf2..79b1380 100644 --- a/app/views/layouts/admin.html.erb +++ b/app/views/layouts/admin.html.erb @@ -33,6 +33,13 @@ <% if flash[:notice].present? || flash[:error].present? %>
<%= flash[:notice] %> + <% if flash[:status_path] %> + <%= link_to 'Go to Status', flash[:status_path] %> + <% end %> + <% if flash[:stale_locale_path] %> + The <%= flash[:stale_locale] %> translation may be out of date — + <%= link_to 'review it', flash[:stale_locale_path] %> too. + <% end %> <% if flash[:error] %> <%= flash[:error] %> <% end %> diff --git a/app/views/rss/updates.xml.builder b/app/views/rss/updates.xml.builder index 4b2e2f7..27845c4 100644 --- a/app/views/rss/updates.xml.builder +++ b/app/views/rss/updates.xml.builder @@ -22,9 +22,7 @@ xml.feed(:xmlns => "http://www.w3.org/2005/Atom", "xml:base" => @host) do xml.updated(item.updated_at.xmlschema) xml.published(item.published_at.xmlschema) xml.summary(CGI.escapeHTML(item.abstract.to_s)) - xml.content(:type => "xhtml") do - xml.div(item.body, :xmlns => "http://www.w3.org/1999/xhtml") - end + xml.content(CGI.escapeHTML(item.body.to_s), :type => "html") end end diff --git a/public/stylesheets/admin.css b/public/stylesheets/admin.css index f00a658..3f95b0c 100644 --- a/public/stylesheets/admin.css +++ b/public/stylesheets/admin.css @@ -193,6 +193,15 @@ input[type=radio] { margin-left: 5px; } +#flash a { + text-decoration: underline; + -webkit-text-decoration-style: wavy; + text-decoration-style: wavy; + text-decoration-color: #b0b0b0; + text-decoration-thickness: 1px; + text-underline-offset: 2px; +} + #flash span { letter-spacing: 1px; margin-right: 10px; -- cgit v1.3 From a25d90335ad3738b6831288190132c2f7498465c Mon Sep 17 00:00:00 2001 From: erdgeist Date: Fri, 10 Jul 2026 00:35:24 +0200 Subject: Retire vendored cacycle_diff.js for a diff-lcs-based diff view Revisions#diff now computes an inline or side-by-side word diff server-side (Page#diff_against, lib/html_word_diff.rb) instead of shipping raw/escaped content to the browser for a 2008-era vendored JS differ to mangle. View mode is picked via a `view` param, settable either on the diff page itself or directly from the revisions#index sticky bar next to "Diff revisions". Also fixes a pre-existing bug in RevisionsController#diff's single- revision branch, which set params[:start]/params[:end] instead of params[:start_revision]/params[:end_revision]. --- Gemfile | 3 +- Gemfile.lock | 3 + app/controllers/revisions_controller.rb | 12 +- app/models/page.rb | 16 + app/views/revisions/diff.html.erb | 78 +- app/views/revisions/index.html.erb | 8 + lib/html_word_diff.rb | 52 ++ public/javascripts/cacycle_diff.js | 1112 ------------------------- public/stylesheets/admin.css | 25 + test/controllers/revisions_controller_test.rb | 29 + test/models/page_test.rb | 34 + 11 files changed, 204 insertions(+), 1168 deletions(-) create mode 100644 lib/html_word_diff.rb delete mode 100644 public/javascripts/cacycle_diff.js (limited to 'app/models/page.rb') diff --git a/Gemfile b/Gemfile index d136eb5..19d92d7 100644 --- a/Gemfile +++ b/Gemfile @@ -45,10 +45,11 @@ gem 'acts-as-taggable-on', git: 'https://github.com/mbleigh/acts-as-taggable-on.git', branch: 'master' -# ── XML / parsing ───────────────────────────────────────────────────────────── +# ── XML / parsing / diffing ─────────────────────────────────────────────────── gem 'libxml-ruby', '~> 5.0', require: 'xml' # body link rewriting in Page model gem 'nokogiri', '~> 1.18' +gem 'diff-lcs', require: 'diff/lcs' # ── Operational ─────────────────────────────────────────────────────────────── diff --git a/Gemfile.lock b/Gemfile.lock index 02c71ff..e6f5fa0 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -108,6 +108,7 @@ GEM connection_pool (3.0.2) crass (1.0.7) date (3.5.1) + diff-lcs (2.0.0) drb (2.2.3) erb (6.0.4) erubi (1.13.1) @@ -330,6 +331,7 @@ DEPENDENCIES chaos_calendar! coffee-rails (~> 4.0) concurrent-ruby (~> 1.3) + diff-lcs exception_notification (~> 4.5) globalize (~> 7.0) jquery-rails @@ -377,6 +379,7 @@ CHECKSUMS connection_pool (3.0.2) sha256=33fff5ba71a12d2aa26cb72b1db8bba2a1a01823559fb01d29eb74c286e62e0a crass (1.0.7) sha256=94868719948664c89ddcaf0a37c65048413dfcb1c869470a5f7a7ceb5390b295 date (3.5.1) sha256=750d06384d7b9c15d562c76291407d89e368dda4d4fff957eb94962d325a0dc0 + diff-lcs (2.0.0) sha256=708a5d52ec2945b50f8f53a181174aa1ef2c496edf81c05957fe956dabb363d5 drb (2.2.3) sha256=0b00d6fdb50995fe4a45dea13663493c841112e4068656854646f418fda13373 erb (6.0.4) sha256=38e3803694be357fe2bfe312487c74beaf9fb4e5beb3e22498952fe1645b95d9 erubi (1.13.1) sha256=a082103b0885dbc5ecf1172fede897f9ebdb745a4b97a5e8dc63953db1ee4ad9 diff --git a/app/controllers/revisions_controller.rb b/app/controllers/revisions_controller.rb index 42d667e..9acb26f 100644 --- a/app/controllers/revisions_controller.rb +++ b/app/controllers/revisions_controller.rb @@ -13,16 +13,18 @@ class RevisionsController < ApplicationController def diff @node = Node.find(params[:node_id]) - + if @node.pages.length > 1 params[:start_revision] ||= @node.pages.all[-2].revision params[:end_revision] ||= @node.pages.all[-1].revision else - params[:start], params[:end] = 1, 1 + params[:start_revision], params[:end_revision] = 1, 1 end - - @start = @node.pages.find_by_revision( params[:start_revision] ) - @end = @node.pages.find_by_revision( params[:end_revision] ) + + @start = @node.pages.find_by_revision( params[:start_revision] ) + @end = @node.pages.find_by_revision( params[:end_revision] ) + @diff_view = params[:view] == "side_by_side" ? :side_by_side : :inline + @diff = @end.diff_against( @start, view: @diff_view ) end def show diff --git a/app/models/page.rb b/app/models/page.rb index 1a98e08..ea04cd6 100644 --- a/app/models/page.rb +++ b/app/models/page.rb @@ -175,6 +175,22 @@ class Page < ApplicationRecord self.save end + def diff_against other, view: :inline + 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) + } + 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) + } + end + end + def public? published_at.nil? ? true : published_at < Time.now end diff --git a/app/views/revisions/diff.html.erb b/app/views/revisions/diff.html.erb index d8c6a47..d7bb528 100644 --- a/app/views/revisions/diff.html.erb +++ b/app/views/revisions/diff.html.erb @@ -7,58 +7,36 @@ <%= form_tag diff_node_revisions_path do %> <%= select_tag :start_revision, options_for_select(@node.pages.map{|x| x.revision}, params[:start_revision].to_i) %> <%= select_tag :end_revision, options_for_select(@node.pages.map{|x| x.revision}, params[:end_revision].to_i) %> + <%= select_tag :view, options_for_select([['Inline', 'inline'], ['Side by side', 'side_by_side']], @diff_view) %> <%= submit_tag 'Diff' %> <% end %> - - - - - - - - -
-  
-
-
-  
-
- - - -
-

Title

-

- -

Abstract

-

- -

Body

-

+ <% if @diff_view == :side_by_side %> +
+
+

Title

+

<%= raw @diff[:title][0] %>

+

Abstract

+

<%= raw @diff[:abstract][0] %>

+

Body

+ <%= raw @diff[:body][0] %> +
+
+

Title

+

<%= raw @diff[:title][1] %>

+

Abstract

+

<%= raw @diff[:abstract][1] %>

+

Body

+ <%= raw @diff[:body][1] %> +
+
+ <% else %> +

Title

+

<%= raw @diff[:title] %>

+

Abstract

+

<%= raw @diff[:abstract] %>

+

Body

+ <%= raw @diff[:body] %> + <% end %>
diff --git a/app/views/revisions/index.html.erb b/app/views/revisions/index.html.erb index a6a981a..58c08b7 100644 --- a/app/views/revisions/index.html.erb +++ b/app/views/revisions/index.html.erb @@ -19,6 +19,8 @@ form: { id: 'diff_form', class: 'button_to computation' }, disabled: true %> + +
@@ -68,6 +70,7 @@ document.getElementById('diff_form').addEventListener('submit', function(e) { var start = document.querySelector('input[name="start_revision"]:checked'); var end = document.querySelector('input[name="end_revision"]:checked'); + var view = document.querySelector('input[name="view"]:checked'); if (start) { var s = document.createElement('input'); s.type = 'hidden'; s.name = 'start_revision'; s.value = start.value; @@ -78,5 +81,10 @@ en.type = 'hidden'; en.name = 'end_revision'; en.value = end.value; this.appendChild(en); } + if (view) { + var v = document.createElement('input'); + v.type = 'hidden'; v.name = 'view'; v.value = view.value; + this.appendChild(v); + } }); diff --git a/lib/html_word_diff.rb b/lib/html_word_diff.rb new file mode 100644 index 0000000..1f28cf5 --- /dev/null +++ b/lib/html_word_diff.rb @@ -0,0 +1,52 @@ +require 'diff/lcs' + +module HtmlWordDiff + TOKEN_REGEXP = /<[^>]*>|[[:space:]]+|[[:alnum:]]+|[^[:space:][:alnum:]<>]/ + + def self.inline(old_html, new_html) + html = +'' + each_change(old_html, new_html) do |action, old_token, new_token| + case action + when '=' + html << new_token + when '-' + html << "#{old_token}" + when '+' + html << "#{new_token}" + when '!' + html << "#{old_token}#{new_token}" + end + end + html + end + + def self.side_by_side(old_html, new_html) + old_out = +'' + new_out = +'' + each_change(old_html, new_html) do |action, old_token, new_token| + case action + when '=' + old_out << old_token + new_out << new_token + when '-' + old_out << "#{old_token}" + when '+' + new_out << "#{new_token}" + when '!' + old_out << "#{old_token}" + new_out << "#{new_token}" + end + end + [old_out, new_out] + end + + def self.each_change(old_html, new_html) + Diff::LCS.sdiff(tokenize(old_html), tokenize(new_html)).each do |change| + yield change.action, change.old_element, change.new_element + end + end + + def self.tokenize(html) + html.to_s.scan(TOKEN_REGEXP) + end +end diff --git a/public/javascripts/cacycle_diff.js b/public/javascripts/cacycle_diff.js deleted file mode 100644 index 24f9d0b..0000000 --- a/public/javascripts/cacycle_diff.js +++ /dev/null @@ -1,1112 +0,0 @@ -//

- 
-/*
- 
-Name:    diff.js
-Version: 0.9.5a (April 6, 2008)
-Info:    http://en.wikipedia.org/wiki/User:Cacycle/diff
-Code:    http://en.wikipedia.org/wiki/User:Cacycle/diff.js
- 
-JavaScript diff algorithm by [[en:User:Cacycle]] (http://en.wikipedia.org/wiki/User_talk:Cacycle).
-Outputs html/css-formatted new text with highlighted deletions, inserts, and block moves.
- 
-The program uses cross-browser code and should work with all modern browsers. It has been tested with:
-* Mozilla Firefox 1.5.0.1
-* Mozilla SeaMonkey 1.0
-* Opera 8.53
-* Internet Explorer 6.0.2900.2180
-* Internet Explorer 7.0.5730.11
-This program is also compatibel with Greasemonkey
- 
-An implementation of the word-based algorithm from:
- 
-Communications of the ACM 21(4):264 (1978)
-http://doi.acm.org/10.1145/359460.359467
- 
-With the following additional feature:
- 
-* Word types have been optimized for MediaWiki source texts
-* Additional post-pass 5 code for resolving islands caused by adding
-  two common words at the end of sequences of common words
-* Additional detection of block borders and color coding of moved blocks and their original position
-* Optional "intelligent" omission of unchanged parts from the output
- 
-This code is used by the MediaWiki in-browser text editors [[en:User:Cacycle/editor]] and [[en:User:Cacycle/wikEd]]
-and the enhanced diff view tool wikEdDiff [[en:User:Cacycle/wikEd]].
- 
-Usage: var htmlText = WDiffString(oldText, newText);
- 
-This code has been released into the public domain.
- 
-Datastructures:
- 
-text: an object that holds all text related datastructures
-  .newWords: consecutive words of the new text (N)
-  .oldWords: consecutive words of the old text (O)
-  .newToOld: array of corresponding word number in old text (NA)
-  .oldToNew: array of corresponding word number in new text (OA)
-  .message:  output message for testing purposes
- 
-symbol['word']: symbol table for passes 1 - 3, holds words as a hash
-  .newCtr:  new word occurences counter (NC)
-  .oldCtr:  old word occurences counter (OC)
-  .toNew:   table last old word number
-  .toOld:   last new word number (OLNA)
- 
-block: an object that holds block move information
-  blocks indexed after new text:
-  .newStart:  new text word number of start of this block
-  .newLength: element number of this block including non-words
-  .newWords:  true word number of this block
-  .newNumber: corresponding block index in old text
-  .newBlock:  moved-block-number of a block that has been moved here
-  .newLeft:   moved-block-number of a block that has been moved from this border leftwards
-  .newRight:  moved-block-number of a block that has been moved from this border rightwards
-  .newLeftIndex:  index number of a block that has been moved from this border leftwards
-  .newRightIndex: index number of a block that has been moved from this border rightwards
-  blocks indexed after old text:
-  .oldStart:  word number of start of this block
-  .oldToNew:  corresponding new text word number of start
-  .oldLength: element number of this block including non-words
-  .oldWords:  true word number of this block
- 
-*/
- 
- 
-// css for change indicators
-if (typeof(wDiffStyleDelete) == 'undefined') { window.wDiffStyleDelete = 'font-weight: normal; text-decoration: none; color: #fff; background-color: #990033;'; }
-if (typeof(wDiffStyleInsert) == 'undefined') { window.wDiffStyleInsert = 'font-weight: normal; text-decoration: none; color: #fff; background-color: #009933;'; }
-if (typeof(wDiffStyleMoved)  == 'undefined') { window.wDiffStyleMoved  = 'font-weight: bold;  color: #000; vertical-align: text-bottom; font-size: xx-small; padding: 0; border: solid 1px;'; }
-if (typeof(wDiffStyleBlock)  == 'undefined') { window.wDiffStyleBlock  = [
-  'color: #000; background-color: #ffff80;',
-  'color: #000; background-color: #c0ffff;',
-  'color: #000; background-color: #ffd0f0;',
-  'color: #000; background-color: #ffe080;',
-  'color: #000; background-color: #aaddff;',
-  'color: #000; background-color: #ddaaff;',
-  'color: #000; background-color: #ffbbbb;',
-  'color: #000; background-color: #d8ffa0;',
-  'color: #000; background-color: #d0d0d0;'
-]; }
- 
-// html for change indicators, {number} is replaced by the block number
-// {block} is replaced by the block style, class and html comments are important for shortening the output
-if (typeof(wDiffHtmlMovedRight)  == 'undefined') { window.wDiffHtmlMovedRight  = ''; }
-if (typeof(wDiffHtmlMovedLeft)   == 'undefined') { window.wDiffHtmlMovedLeft   = ''; }
- 
-if (typeof(wDiffHtmlBlockStart)  == 'undefined') { window.wDiffHtmlBlockStart  = ''; }
-if (typeof(wDiffHtmlBlockEnd)    == 'undefined') { window.wDiffHtmlBlockEnd    = ''; }
- 
-if (typeof(wDiffHtmlDeleteStart) == 'undefined') { window.wDiffHtmlDeleteStart = ''; }
-if (typeof(wDiffHtmlDeleteEnd)   == 'undefined') { window.wDiffHtmlDeleteEnd   = ''; }
- 
-if (typeof(wDiffHtmlInsertStart) == 'undefined') { window.wDiffHtmlInsertStart = ''; }
-if (typeof(wDiffHtmlInsertEnd)   == 'undefined') { window.wDiffHtmlInsertEnd   = ''; }
- 
-// minimal number of real words for a moved block (0 for always displaying block move indicators)
-if (typeof(wDiffBlockMinLength) == 'undefined') { window.wDiffBlockMinLength = 3; }
- 
-// exclude identical sequence starts and endings from change marking
-if (typeof(wDiffWordDiff) == 'undefined') { window.wDiffWordDiff = true; }
- 
-// enable recursive diff to resolve problematic sequences
-if (typeof(wDiffRecursiveDiff) == 'undefined') { window.wDiffRecursiveDiff = true; }
- 
-// enable block move display
-if (typeof(wDiffShowBlockMoves) == 'undefined') { window.wDiffShowBlockMoves = true; }
- 
-// remove unchanged parts from final output
- 
-// characters before diff tag to search for previous heading, paragraph, line break, cut characters
-if (typeof(wDiffHeadingBefore)   == 'undefined') { window.wDiffHeadingBefore   = 1500; }
-if (typeof(wDiffParagraphBefore) == 'undefined') { window.wDiffParagraphBefore = 1500; }
-if (typeof(wDiffLineBeforeMax)   == 'undefined') { window.wDiffLineBeforeMax   = 1000; }
-if (typeof(wDiffLineBeforeMin)   == 'undefined') { window.wDiffLineBeforeMin   =  500; }
-if (typeof(wDiffBlankBeforeMax)  == 'undefined') { window.wDiffBlankBeforeMax  = 1000; }
-if (typeof(wDiffBlankBeforeMin)  == 'undefined') { window.wDiffBlankBeforeMin  =  500; }
-if (typeof(wDiffCharsBefore)     == 'undefined') { window.wDiffCharsBefore     =  500; }
- 
-// characters after diff tag to search for next heading, paragraph, line break, or characters
-if (typeof(wDiffHeadingAfter)   == 'undefined') { window.wDiffHeadingAfter   = 1500; }
-if (typeof(wDiffParagraphAfter) == 'undefined') { window.wDiffParagraphAfter = 1500; }
-if (typeof(wDiffLineAfterMax)   == 'undefined') { window.wDiffLineAfterMax   = 1000; }
-if (typeof(wDiffLineAfterMin)   == 'undefined') { window.wDiffLineAfterMin   =  500; }
-if (typeof(wDiffBlankAfterMax)  == 'undefined') { window.wDiffBlankAfterMax  = 1000; }
-if (typeof(wDiffBlankAfterMin)  == 'undefined') { window.wDiffBlankAfterMin  =  500; }
-if (typeof(wDiffCharsAfter)     == 'undefined') { window.wDiffCharsAfter     =  500; }
- 
-// maximal fragment distance to join close fragments
-if (typeof(wDiffFragmentJoin)  == 'undefined') { window.wDiffFragmentJoin = 1000; }
-if (typeof(wDiffOmittedChars)  == 'undefined') { window.wDiffOmittedChars = '…'; }
-if (typeof(wDiffOmittedLines)  == 'undefined') { window.wDiffOmittedLines = '
'; } -if (typeof(wDiffNoChange) == 'undefined') { window.wDiffNoChange = '
'; } - -// compatibility fix for old name of main function -window.StringDiff = window.WDiffString; - - -// WDiffString: main program -// input: oldText, newText, strings containing the texts -// returns: html diff - -window.WDiffString = function(oldText, newText) { - -// IE / Mac fix - oldText = oldText.replace(/(\r\n)/g, '\n'); - newText = newText.replace(/(\r\n)/g, '\n'); - - var text = {}; - text.newWords = []; - text.oldWords = []; - text.newToOld = []; - text.oldToNew = []; - text.message = ''; - var block = {}; - var outText = ''; - -// trap trivial changes: no change - if (oldText == newText) { - outText = newText; - outText = WDiffEscape(outText); - outText = WDiffHtmlFormat(outText); - return(outText); - } - -// trap trivial changes: old text deleted - if ( (oldText == null) || (oldText.length == 0) ) { - outText = newText; - outText = WDiffEscape(outText); - outText = WDiffHtmlFormat(outText); - outText = wDiffHtmlInsertStart + outText + wDiffHtmlInsertEnd; - return(outText); - } - -// trap trivial changes: new text deleted - if ( (newText == null) || (newText.length == 0) ) { - outText = oldText; - outText = WDiffEscape(outText); - outText = WDiffHtmlFormat(outText); - outText = wDiffHtmlDeleteStart + outText + wDiffHtmlDeleteEnd; - return(outText); - } - -// split new and old text into words - WDiffSplitText(oldText, newText, text); - -// calculate diff information - WDiffText(text); - -//detect block borders and moved blocks - WDiffDetectBlocks(text, block); - -// process diff data into formatted html text - outText = WDiffToHtml(text, block); - -// IE fix - outText = outText.replace(/> ( *) $1<'); - - return(outText); -} - - -// WDiffSplitText: split new and old text into words -// input: oldText, newText, strings containing the texts -// changes: text.newWords and text.oldWords, arrays containing the texts in arrays of words - -window.WDiffSplitText = function(oldText, newText, text) { - -// convert strange spaces - oldText = oldText.replace(/[\t\u000b\u00a0\u2028\u2029]+/g, ' '); - newText = newText.replace(/[\t\u000b\u00a0\u2028\u2029]+/g, ' '); - -// split old text into words - -// / | | | | | | | | | | | | | | / - var pattern = /[\w]+|\[\[|\]\]|\{\{|\}\}|\n+| +|&\w+;|'''|''|=+|\{\||\|\}|\|\-|./g; - var result; - do { - result = pattern.exec(oldText); - if (result != null) { - text.oldWords.push(result[0]); - } - } while (result != null); - -// split new text into words - do { - result = pattern.exec(newText); - if (result != null) { - text.newWords.push(result[0]); - } - } while (result != null); - - return; -} - - -// WDiffText: calculate diff information -// input: text.newWords and text.oldWords, arrays containing the texts in arrays of words -// optionally for recursive calls: newStart, newEnd, oldStart, oldEnd, recursionLevel -// changes: text.newToOld and text.oldToNew, containing the line numbers in the other version - -window.WDiffText = function(text, newStart, newEnd, oldStart, oldEnd, recursionLevel) { - - symbol = new Object(); - symbol.newCtr = []; - symbol.oldCtr = []; - symbol.toNew = []; - symbol.toOld = []; - -// set defaults - newStart = newStart || 0; - newEnd = newEnd || text.newWords.length; - oldStart = oldStart || 0; - oldEnd = oldEnd || text.oldWords.length; - recursionLevel = recursionLevel || 0; - -// limit recursion depth - if (recursionLevel > 10) { - return; - } - -// pass 1: parse new text into symbol table s - - var word; - for (var i = newStart; i < newEnd; i ++) { - word = text.newWords[i]; - -// add new entry to symbol table - if ( symbol[word] == null) { - symbol[word] = { newCtr: 0, oldCtr: 0, toNew: null, toOld: null }; - } - -// increment symbol table word counter for new text - symbol[word].newCtr ++; - -// add last word number in new text - symbol[word].toNew = i; - } - -// pass 2: parse old text into symbol table - - for (var j = oldStart; j < oldEnd; j ++) { - word = text.oldWords[j]; - -// add new entry to symbol table - if ( symbol[word] == null) { - symbol[word] = { newCtr: 0, oldCtr: 0, toNew: null, toOld: null }; - } - -// increment symbol table word counter for old text - symbol[word].oldCtr ++; - -// add last word number in old text - symbol[word].toOld = j; - } - -// pass 3: connect unique words - - for (var i in symbol) { - -// find words in the symbol table that occur only once in both versions - if ( (symbol[i].newCtr == 1) && (symbol[i].oldCtr == 1) ) { - var toNew = symbol[i].toNew; - var toOld = symbol[i].toOld; - -// do not use spaces as unique markers - if ( ! /\s/.test( text.newWords[toNew] ) ) { - -// connect from new to old and from old to new - text.newToOld[toNew] = toOld; - text.oldToNew[toOld] = toNew; - } - } - } - -// pass 4: connect adjacent identical words downwards - - for (var i = newStart; i < newEnd - 1; i ++) { - -// find already connected pairs - if (text.newToOld[i] != null) { - j = text.newToOld[i]; - -// check if the following words are not yet connected - if ( (text.newToOld[i + 1] == null) && (text.oldToNew[j + 1] == null) ) { - -// if the following words are the same connect them - if ( text.newWords[i + 1] == text.oldWords[j + 1] ) { - text.newToOld[i + 1] = j + 1; - text.oldToNew[j + 1] = i + 1; - } - } - } - } - -// pass 5: connect adjacent identical words upwards - - for (var i = newEnd - 1; i > newStart; i --) { - -// find already connected pairs - if (text.newToOld[i] != null) { - j = text.newToOld[i]; - -// check if the preceeding words are not yet connected - if ( (text.newToOld[i - 1] == null) && (text.oldToNew[j - 1] == null) ) { - -// if the preceeding words are the same connect them - if ( text.newWords[i - 1] == text.oldWords[j - 1] ) { - text.newToOld[i - 1] = j - 1; - text.oldToNew[j - 1] = i - 1; - } - } - } - } - -// recursively diff still unresolved regions downwards - - if (wDiffRecursiveDiff) { - i = newStart; - j = oldStart; - while (i < newEnd) { - if (text.newToOld[i - 1] != null) { - j = text.newToOld[i - 1] + 1; - } - -// check for the start of an unresolved sequence - if ( (text.newToOld[i] == null) && (text.oldToNew[j] == null) ) { - -// determine the ends of the sequences - var iStart = i; - var iEnd = i; - while ( (text.newToOld[iEnd] == null) && (iEnd < newEnd) ) { - iEnd ++; - } - var iLength = iEnd - iStart; - - var jStart = j; - var jEnd = j; - while ( (text.oldToNew[jEnd] == null) && (jEnd < oldEnd) ) { - jEnd ++; - } - var jLength = jEnd - jStart; - -// recursively diff the unresolved sequence - if ( (iLength > 0) && (jLength > 0) ) { - if ( (iLength > 1) || (jLength > 1) ) { - if ( (iStart != newStart) || (iEnd != newEnd) || (jStart != oldStart) || (jEnd != oldEnd) ) { - WDiffText(text, iStart, iEnd, jStart, jEnd, recursionLevel + 1); - } - } - } - i = iEnd; - } - else { - i ++; - } - } - } - -// recursively diff still unresolved regions upwards - - if (wDiffRecursiveDiff) { - i = newEnd - 1; - j = oldEnd - 1; - while (i >= newStart) { - if (text.newToOld[i + 1] != null) { - j = text.newToOld[i + 1] - 1; - } - -// check for the start of an unresolved sequence - if ( (text.newToOld[i] == null) && (text.oldToNew[j] == null) ) { - -// determine the ends of the sequences - var iStart = i; - var iEnd = i + 1; - while ( (text.newToOld[iStart - 1] == null) && (iStart >= newStart) ) { - iStart --; - } - var iLength = iEnd - iStart; - - var jStart = j; - var jEnd = j + 1; - while ( (text.oldToNew[jStart - 1] == null) && (jStart >= oldStart) ) { - jStart --; - } - var jLength = jEnd - jStart; - -// recursively diff the unresolved sequence - if ( (iLength > 0) && (jLength > 0) ) { - if ( (iLength > 1) || (jLength > 1) ) { - if ( (iStart != newStart) || (iEnd != newEnd) || (jStart != oldStart) || (jEnd != oldEnd) ) { - WDiffText(text, iStart, iEnd, jStart, jEnd, recursionLevel + 1); - } - } - } - i = iStart - 1; - } - else { - i --; - } - } - } - return; -} - - -// WDiffToHtml: process diff data into formatted html text -// input: text.newWords and text.oldWords, arrays containing the texts in arrays of words -// text.newToOld and text.oldToNew, containing the line numbers in the other version -// block data structure -// returns: outText, a html string - -window.WDiffToHtml = function(text, block) { - - var outText = text.message; - - var blockNumber = 0; - var i = 0; - var j = 0; - var movedAsInsertion; - -// cycle through the new text - do { - var movedIndex = []; - var movedBlock = []; - var movedLeft = []; - var blockText = ''; - var identText = ''; - var delText = ''; - var insText = ''; - var identStart = ''; - -// check if a block ends here and finish previous block - if (movedAsInsertion != null) { - if (movedAsInsertion == false) { - identStart += wDiffHtmlBlockEnd; - } - else { - identStart += wDiffHtmlInsertEnd; - } - movedAsInsertion = null; - } - -// detect block boundary - if ( (text.newToOld[i] != j) || (blockNumber == 0 ) ) { - if ( ( (text.newToOld[i] != null) || (i >= text.newWords.length) ) && ( (text.oldToNew[j] != null) || (j >= text.oldWords.length) ) ) { - -// block moved right - var moved = block.newRight[blockNumber]; - if (moved > 0) { - var index = block.newRightIndex[blockNumber]; - movedIndex.push(index); - movedBlock.push(moved); - movedLeft.push(false); - } - -// block moved left - moved = block.newLeft[blockNumber]; - if (moved > 0) { - var index = block.newLeftIndex[blockNumber]; - movedIndex.push(index); - movedBlock.push(moved); - movedLeft.push(true); - } - -// check if a block starts here - moved = block.newBlock[blockNumber]; - if (moved > 0) { - -// mark block as inserted text - if (block.newWords[blockNumber] < wDiffBlockMinLength) { - identStart += wDiffHtmlInsertStart; - movedAsInsertion = true; - } - -// mark block by color - else { - if (moved > wDiffStyleBlock.length) { - moved = wDiffStyleBlock.length; - } - identStart += WDiffHtmlCustomize(wDiffHtmlBlockStart, moved - 1); - movedAsInsertion = false; - } - } - - if (i >= text.newWords.length) { - i ++; - } - else { - j = text.newToOld[i]; - blockNumber ++; - } - } - } - -// get the correct order if moved to the left as well as to the right from here - if (movedIndex.length == 2) { - if (movedIndex[0] > movedIndex[1]) { - movedIndex.reverse(); - movedBlock.reverse(); - movedLeft.reverse(); - } - } - -// handle left and right block moves from this position - for (var m = 0; m < movedIndex.length; m ++) { - -// insert the block as deleted text - if (block.newWords[ movedIndex[m] ] < wDiffBlockMinLength) { - var movedStart = block.newStart[ movedIndex[m] ]; - var movedLength = block.newLength[ movedIndex[m] ]; - var str = ''; - for (var n = movedStart; n < movedStart + movedLength; n ++) { - str += text.newWords[n]; - } - str = WDiffEscape(str); - str = str.replace(/\n/g, '¶
'); - blockText += wDiffHtmlDeleteStart + str + wDiffHtmlDeleteEnd; - } - -// add a placeholder / move direction indicator - else { - if (movedBlock[m] > wDiffStyleBlock.length) { - movedBlock[m] = wDiffStyleBlock.length; - } - if (movedLeft[m]) { - blockText += WDiffHtmlCustomize(wDiffHtmlMovedLeft, movedBlock[m] - 1); - } - else { - blockText += WDiffHtmlCustomize(wDiffHtmlMovedRight, movedBlock[m] - 1); - } - } - } - -// collect consecutive identical text - while ( (i < text.newWords.length) && (j < text.oldWords.length) ) { - if ( (text.newToOld[i] == null) || (text.oldToNew[j] == null) ) { - break; - } - if (text.newToOld[i] != j) { - break; - } - identText += text.newWords[i]; - i ++; - j ++; - } - -// collect consecutive deletions - while ( (text.oldToNew[j] == null) && (j < text.oldWords.length) ) { - delText += text.oldWords[j]; - j ++; - } - -// collect consecutive inserts - while ( (text.newToOld[i] == null) && (i < text.newWords.length) ) { - insText += text.newWords[i]; - i ++; - } - -// remove leading and trailing similarities betweein delText and ins from highlighting - var preText = ''; - var postText = ''; - if (wDiffWordDiff) { - if ( (delText != '') && (insText != '') ) { - -// remove leading similarities - while ( delText.charAt(0) == insText.charAt(0) && (delText != '') && (insText != '') ) { - preText = preText + delText.charAt(0); - delText = delText.substr(1); - insText = insText.substr(1); - } - -// remove trailing similarities - while ( delText.charAt(delText.length - 1) == insText.charAt(insText.length - 1) && (delText != '') && (insText != '') ) { - postText = delText.charAt(delText.length - 1) + postText; - delText = delText.substr(0, delText.length - 1); - insText = insText.substr(0, insText.length - 1); - } - } - } - -// output the identical text, deletions and inserts - -// moved from here indicator - if (blockText != '') { - outText += blockText; - } - -// identical text - if (identText != '') { - outText += identStart + WDiffEscape(identText); - } - outText += preText; - -// deleted text - if (delText != '') { - delText = wDiffHtmlDeleteStart + WDiffEscape(delText) + wDiffHtmlDeleteEnd; - delText = delText.replace(/\n/g, '¶
'); - outText += delText; - } - -// inserted text - if (insText != '') { - insText = wDiffHtmlInsertStart + WDiffEscape(insText) + wDiffHtmlInsertEnd; - insText = insText.replace(/\n/g, '¶
'); - outText += insText; - } - outText += postText; - } while (i <= text.newWords.length); - - outText += '\n'; - outText = WDiffHtmlFormat(outText); - - return(outText); -} - - -// WDiffEscape: replaces html-sensitive characters in output text with character entities - -window.WDiffEscape = function(text) { - - text = text.replace(/&/g, '&'); - text = text.replace(//g, '>'); - text = text.replace(/\"/g, '"'); - - return(text); -} - - -// HtmlCustomize: customize indicator html: replace {number} with the block number, {block} with the block style - -window.WDiffHtmlCustomize = function(text, block) { - - text = text.replace(/\{number\}/, block); - text = text.replace(/\{block\}/, wDiffStyleBlock[block]); - - return(text); -} - - -// HtmlFormat: replaces newlines and multiple spaces in text with html code - -window.WDiffHtmlFormat = function(text) { - - text = text.replace(/ /g, '  '); - text = text.replace(/\n/g, '
'); - - return(text); -} - - -// WDiffDetectBlocks: detect block borders and moved blocks -// input: text object, block object - -window.WDiffDetectBlocks = function(text, block) { - - block.oldStart = []; - block.oldToNew = []; - block.oldLength = []; - block.oldWords = []; - block.newStart = []; - block.newLength = []; - block.newWords = []; - block.newNumber = []; - block.newBlock = []; - block.newLeft = []; - block.newRight = []; - block.newLeftIndex = []; - block.newRightIndex = []; - - var blockNumber = 0; - var wordCounter = 0; - var realWordCounter = 0; - -// get old text block order - if (wDiffShowBlockMoves) { - var j = 0; - var i = 0; - do { - -// detect block boundaries on old text - if ( (text.oldToNew[j] != i) || (blockNumber == 0 ) ) { - if ( ( (text.oldToNew[j] != null) || (j >= text.oldWords.length) ) && ( (text.newToOld[i] != null) || (i >= text.newWords.length) ) ) { - if (blockNumber > 0) { - block.oldLength[blockNumber - 1] = wordCounter; - block.oldWords[blockNumber - 1] = realWordCounter; - wordCounter = 0; - realWordCounter = 0; - } - - if (j >= text.oldWords.length) { - j ++; - } - else { - i = text.oldToNew[j]; - block.oldStart[blockNumber] = j; - block.oldToNew[blockNumber] = text.oldToNew[j]; - blockNumber ++; - } - } - } - -// jump over identical pairs - while ( (i < text.newWords.length) && (j < text.oldWords.length) ) { - if ( (text.newToOld[i] == null) || (text.oldToNew[j] == null) ) { - break; - } - if (text.oldToNew[j] != i) { - break; - } - i ++; - j ++; - wordCounter ++; - if ( /\w/.test( text.newWords[i] ) ) { - realWordCounter ++; - } - } - -// jump over consecutive deletions - while ( (text.oldToNew[j] == null) && (j < text.oldWords.length) ) { - j ++; - } - -// jump over consecutive inserts - while ( (text.newToOld[i] == null) && (i < text.newWords.length) ) { - i ++; - } - } while (j <= text.oldWords.length); - -// get the block order in the new text - var lastMin; - var currMinIndex; - lastMin = null; - -// sort the data by increasing start numbers into new text block info - for (var i = 0; i < blockNumber; i ++) { - currMin = null; - for (var j = 0; j < blockNumber; j ++) { - curr = block.oldToNew[j]; - if ( (curr > lastMin) || (lastMin == null) ) { - if ( (curr < currMin) || (currMin == null) ) { - currMin = curr; - currMinIndex = j; - } - } - } - block.newStart[i] = block.oldToNew[currMinIndex]; - block.newLength[i] = block.oldLength[currMinIndex]; - block.newWords[i] = block.oldWords[currMinIndex]; - block.newNumber[i] = currMinIndex; - lastMin = currMin; - } - -// detect not moved blocks - for (var i = 0; i < blockNumber; i ++) { - if (block.newBlock[i] == null) { - if (block.newNumber[i] == i) { - block.newBlock[i] = 0; - } - } - } - -// detect switches of neighbouring blocks - for (var i = 0; i < blockNumber - 1; i ++) { - if ( (block.newBlock[i] == null) && (block.newBlock[i + 1] == null) ) { - if (block.newNumber[i] - block.newNumber[i + 1] == 1) { - if ( (block.newNumber[i + 1] - block.newNumber[i + 2] != 1) || (i + 2 >= blockNumber) ) { - -// the shorter one is declared the moved one - if (block.newLength[i] < block.newLength[i + 1]) { - block.newBlock[i] = 1; - block.newBlock[i + 1] = 0; - } - else { - block.newBlock[i] = 0; - block.newBlock[i + 1] = 1; - } - } - } - } - } - -// mark all others as moved and number the moved blocks - j = 1; - for (var i = 0; i < blockNumber; i ++) { - if ( (block.newBlock[i] == null) || (block.newBlock[i] == 1) ) { - block.newBlock[i] = j++; - } - } - -// check if a block has been moved from this block border - for (var i = 0; i < blockNumber; i ++) { - for (var j = 0; j < blockNumber; j ++) { - - if (block.newNumber[j] == i) { - if (block.newBlock[j] > 0) { - -// block moved right - if (block.newNumber[j] < j) { - block.newRight[i] = block.newBlock[j]; - block.newRightIndex[i] = j; - } - -// block moved left - else { - block.newLeft[i + 1] = block.newBlock[j]; - block.newLeftIndex[i + 1] = j; - } - } - } - } - } - } - return; -} - - -// WDiffShortenOutput: remove unchanged parts from final output -// input: the output of WDiffString -// returns: the text with removed unchanged passages indicated by (...) - -window.WDiffShortenOutput = function(diffText) { - -// html
to newlines - diffText = diffText.replace(/]*>/g, '\n'); - -// scan for diff html tags - var regExpDiff = new RegExp('<\\w+ class=\\"(\\w+)\\"[^>]*>(.|\\n)*?', 'g'); - var tagStart = []; - var tagEnd = []; - var i = 0; - var found; - while ( (found = regExpDiff.exec(diffText)) != null ) { - -// combine consecutive diff tags - if ( (i > 0) && (tagEnd[i - 1] == found.index) ) { - tagEnd[i - 1] = found.index + found[0].length; - } - else { - tagStart[i] = found.index; - tagEnd[i] = found.index + found[0].length; - i ++; - } - } - -// no diff tags detected - if (tagStart.length == 0) { - return(wDiffNoChange); - } - -// define regexps - var regExpHeading = new RegExp('\\n=+.+?=+ *\\n|\\n\\{\\||\\n\\|\\}', 'g'); - var regExpParagraph = new RegExp('\\n\\n+', 'g'); - var regExpLine = new RegExp('\\n+', 'g'); - var regExpBlank = new RegExp('(<[^>]+>)*\\s+', 'g'); - -// determine fragment border positions around diff tags - var rangeStart = []; - var rangeEnd = []; - var rangeStartType = []; - var rangeEndType = []; - for (var i = 0; i < tagStart.length; i ++) { - var found; - -// find last heading before diff tag - var lastPos = tagStart[i] - wDiffHeadingBefore; - if (lastPos < 0) { - lastPos = 0; - } - regExpHeading.lastIndex = lastPos; - while ( (found = regExpHeading.exec(diffText)) != null ) { - if (found.index > tagStart[i]) { - break; - } - rangeStart[i] = found.index; - rangeStartType[i] = 'heading'; - } - -// find last paragraph before diff tag - if (rangeStart[i] == null) { - lastPos = tagStart[i] - wDiffParagraphBefore; - if (lastPos < 0) { - lastPos = 0; - } - regExpParagraph.lastIndex = lastPos; - while ( (found = regExpParagraph.exec(diffText)) != null ) { - if (found.index > tagStart[i]) { - break; - } - rangeStart[i] = found.index; - rangeStartType[i] = 'paragraph'; - } - } - -// find line break before diff tag - if (rangeStart[i] == null) { - lastPos = tagStart[i] - wDiffLineBeforeMax; - if (lastPos < 0) { - lastPos = 0; - } - regExpLine.lastIndex = lastPos; - while ( (found = regExpLine.exec(diffText)) != null ) { - if (found.index > tagStart[i] - wDiffLineBeforeMin) { - break; - } - rangeStart[i] = found.index; - rangeStartType[i] = 'line'; - } - } - -// find blank before diff tag - if (rangeStart[i] == null) { - lastPos = tagStart[i] - wDiffBlankBeforeMax; - if (lastPos < 0) { - lastPos = 0; - } - regExpBlank.lastIndex = lastPos; - while ( (found = regExpBlank.exec(diffText)) != null ) { - if (found.index > tagStart[i] - wDiffBlankBeforeMin) { - break; - } - rangeStart[i] = found.index; - rangeStartType[i] = 'blank'; - } - } - -// fixed number of chars before diff tag - if (rangeStart[i] == null) { - rangeStart[i] = tagStart[i] - wDiffCharsBefore; - rangeStartType[i] = 'chars'; - if (rangeStart[i] < 0) { - rangeStart[i] = 0; - } - } - -// find first heading after diff tag - regExpHeading.lastIndex = tagEnd[i]; - if ( (found = regExpHeading.exec(diffText)) != null ) { - if (found.index < tagEnd[i] + wDiffHeadingAfter) { - rangeEnd[i] = found.index + found[0].length; - rangeEndType[i] = 'heading'; - } - } - -// find first paragraph after diff tag - if (rangeEnd[i] == null) { - regExpParagraph.lastIndex = tagEnd[i]; - if ( (found = regExpParagraph.exec(diffText)) != null ) { - if (found.index < tagEnd[i] + wDiffParagraphAfter) { - rangeEnd[i] = found.index; - rangeEndType[i] = 'paragraph'; - } - } - } - -// find first line break after diff tag - if (rangeEnd[i] == null) { - regExpLine.lastIndex = tagEnd[i] + wDiffLineAfterMin; - if ( (found = regExpLine.exec(diffText)) != null ) { - if (found.index < tagEnd[i] + wDiffLineAfterMax) { - rangeEnd[i] = found.index; - rangeEndType[i] = 'break'; - } - } - } - -// find blank after diff tag - if (rangeEnd[i] == null) { - regExpBlank.lastIndex = tagEnd[i] + wDiffBlankAfterMin; - if ( (found = regExpBlank.exec(diffText)) != null ) { - if (found.index < tagEnd[i] + wDiffBlankAfterMax) { - rangeEnd[i] = found.index; - rangeEndType[i] = 'blank'; - } - } - } - -// fixed number of chars after diff tag - if (rangeEnd[i] == null) { - rangeEnd[i] = tagEnd[i] + wDiffCharsAfter; - if (rangeEnd[i] > diffText.length) { - rangeEnd[i] = diffText.length; - rangeEndType[i] = 'chars'; - } - } - } - -// remove overlaps, join close fragments - var fragmentStart = []; - var fragmentEnd = []; - var fragmentStartType = []; - var fragmentEndType = []; - fragmentStart[0] = rangeStart[0]; - fragmentEnd[0] = rangeEnd[0]; - fragmentStartType[0] = rangeStartType[0]; - fragmentEndType[0] = rangeEndType[0]; - var j = 1; - for (var i = 1; i < rangeStart.length; i ++) { - if (rangeStart[i] > fragmentEnd[j - 1] + wDiffFragmentJoin) { - fragmentStart[j] = rangeStart[i]; - fragmentEnd[j] = rangeEnd[i]; - fragmentStartType[j] = rangeStartType[i]; - fragmentEndType[j] = rangeEndType[i]; - j ++; - } - else { - fragmentEnd[j - 1] = rangeEnd[i]; - fragmentEndType[j - 1] = rangeEndType[i]; - } - } - -// assemble the fragments - var outText = ''; - for (var i = 0; i < fragmentStart.length; i ++) { - -// get text fragment - var fragment = diffText.substring(fragmentStart[i], fragmentEnd[i]); - var fragment = fragment.replace(/^\n+|\n+$/g, ''); - -// add inline marks for omitted chars and words - if (fragmentStart[i] > 0) { - if (fragmentStartType[i] == 'chars') { - fragment = wDiffOmittedChars + fragment; - } - else if (fragmentStartType[i] == 'blank') { - fragment = wDiffOmittedChars + ' ' + fragment; - } - } - if (fragmentEnd[i] < diffText.length) { - if (fragmentStartType[i] == 'chars') { - fragment = fragment + wDiffOmittedChars; - } - else if (fragmentStartType[i] == 'blank') { - fragment = fragment + ' ' + wDiffOmittedChars; - } - } - -// add omitted line separator - if (fragmentStart[i] > 0) { - outText += wDiffOmittedLines; - } - -// encapsulate span errors - outText += '
' + fragment + '
'; - } - -// add trailing omitted line separator - if (fragmentEnd[i - 1] < diffText.length) { - outText = outText + wDiffOmittedLines; - } - -// remove leading and trailing empty lines - outText = outText.replace(/^(
)\n+|\n+(<\/div>)$/g, '$1$2'); - -// convert to html linebreaks - outText = outText.replace(/\n/g, '
'); - - return(outText); -} - - -//

\ No newline at end of file
diff --git a/public/stylesheets/admin.css b/public/stylesheets/admin.css
index 38c9e5a..1bb6cf4 100644
--- a/public/stylesheets/admin.css
+++ b/public/stylesheets/admin.css
@@ -511,6 +511,31 @@ table.revisions_table tr:hover {
   background-color: #f1f1f1;
 }
 
+#diffview del {
+  background: #ffd7d5;
+  color: #82071e;
+  text-decoration: line-through;
+  padding: 0 2px;
+  border-radius: 2px;
+}
+
+#diffview ins {
+  background: #ccffd8;
+  color: #055d20;
+  text-decoration: none;
+  padding: 0 2px;
+  border-radius: 2px;
+}
+
+#diffview .diff_side_by_side {
+  display: flex;
+  gap: 1rem;
+}
+
+#diffview .diff_column {
+  flex: 1;
+}
+
 table.user_table td.user_login {
   padding-right: 30px;
 }
diff --git a/test/controllers/revisions_controller_test.rb b/test/controllers/revisions_controller_test.rb
index b4dcd8f..e2fc976 100644
--- a/test/controllers/revisions_controller_test.rb
+++ b/test/controllers/revisions_controller_test.rb
@@ -59,4 +59,33 @@ class RevisionsControllerTest < ActionController::TestCase
     assert_equal @node.head, @node.pages.first
     assert_equal "first", @node.head.reload.body
   end
+
+  test "diffing two revisions renders real markup with only the changed words marked" 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 "del", "first"
+    assert_select "ins", "second"
+    assert_no_match /</, response.body
+  end
+
+  test "diffing two revisions in side by side view renders two columns" do
+    login_as :quentin
+    post(
+      :diff, params: {
+        :node_id => @node.id,
+        :start_revision => @node.pages.first.revision,
+        :end_revision => @node.pages.last.revision,
+        :view => "side_by_side"
+      }
+    )
+    assert_response :success
+    assert_select ".diff_column", 2
+  end
 end
diff --git a/test/models/page_test.rb b/test/models/page_test.rb
index ad2742f..ac5691a 100644
--- a/test/models/page_test.rb
+++ b/test/models/page_test.rb
@@ -176,4 +176,38 @@ class PageTest < ActiveSupport::TestCase
     assert_not_equal first_page.id, first_page.node.head_id
     assert first_page.published_at.present?
   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
+    d.title = "Old heading"
+    d.save!
+    n.publish_draft!
+
+    d2 = n.find_or_create_draft @user1
+    d2.title = "New heading"
+    d2.save!
+
+    diff = d2.diff_against(n.head)
+
+    assert_match "Old", diff[:title]
+    assert_match "New", diff[:title]
+  end
+
+  def test_diff_against_side_by_side_returns_two_annotated_strings
+    n = Node.root.children.create! :slug => "diff_against_sbs_test"
+    d = n.find_or_create_draft @user1
+    d.title = "Old heading"
+    d.save!
+    n.publish_draft!
+
+    d2 = n.find_or_create_draft @user1
+    d2.title = "New heading"
+    d2.save!
+
+    old_html, new_html = d2.diff_against(n.head, view: :side_by_side)[:title]
+
+    assert_match "Old", old_html
+    assert_match "New", new_html
+  end
 end
-- 
cgit v1.3


From 30ed9fd9a8bffb44e6ab91dfedb8c0e33837769a Mon Sep 17 00:00:00 2001
From: erdgeist 
Date: Fri, 10 Jul 2026 04:41:54 +0200
Subject: Include assets, tags and template in diff between revisions

---
 app/models/page.rb                            | 33 ++++++++++++--------
 app/views/revisions/diff.html.erb             | 27 +++++++++++++++++
 public/stylesheets/admin.css                  | 11 +++++++
 test/controllers/revisions_controller_test.rb | 13 ++++++++
 test/models/page_test.rb                      | 43 +++++++++++++++++++++++++++
 5 files changed, 114 insertions(+), 13 deletions(-)

(limited to 'app/models/page.rb')

diff --git a/app/models/page.rb b/app/models/page.rb
index ea04cd6..740d42e 100644
--- a/app/models/page.rb
+++ b/app/models/page.rb
@@ -176,19 +176,26 @@ class Page < ApplicationRecord
   end
 
   def diff_against other, view: :inline
-    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)
-      }
-    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)
-      }
-    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)
+        }
+      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)
+        }
+      end
+
+    text_diffs.merge(
+      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 }
+    )
   end
 
   def public?
diff --git a/app/views/revisions/diff.html.erb b/app/views/revisions/diff.html.erb
index d70503c..b9ce6bd 100644
--- a/app/views/revisions/diff.html.erb
+++ b/app/views/revisions/diff.html.erb
@@ -85,4 +85,31 @@
     

Body

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

Tags

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

No change.

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

Template

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

<%= @diff[:template_name][:from] || '(none)' %> <%= @diff[:template_name][:to] || '(none)' %>

+ <% else %> +

No change.

+ <% end %> + +

Assets

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

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/public/stylesheets/admin.css b/public/stylesheets/admin.css index e04499d..7b39c72 100644 --- a/public/stylesheets/admin.css +++ b/public/stylesheets/admin.css @@ -549,6 +549,17 @@ table.revisions_table tr:hover { cursor: help; } +.diff_unchanged { + color: #969696; + font-style: italic; +} + +.diff_set_list { + list-style: none; + padding-left: 0; + margin: 0; +} + table.user_table td.user_login { padding-right: 30px; } diff --git a/test/controllers/revisions_controller_test.rb b/test/controllers/revisions_controller_test.rb index bf92c5b..53927b4 100644 --- a/test/controllers/revisions_controller_test.rb +++ b/test/controllers/revisions_controller_test.rb @@ -148,4 +148,17 @@ class RevisionsControllerTest < ActionController::TestCase assert_response :success assert_select "a", "Side by side" end + + test "diffing two revisions also shows tag, template, and asset changes" do + login_as :quentin + @node.find_or_create_draft(@user) + @node.draft.tag_list = "update" + @node.draft.save! + + 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", "Tags" + assert_select "h3", "Template" + assert_select "h3", "Assets" + end end diff --git a/test/models/page_test.rb b/test/models/page_test.rb index 3868a53..bcddc89 100644 --- a/test/models/page_test.rb +++ b/test/models/page_test.rb @@ -228,4 +228,47 @@ class PageTest < ActiveSupport::TestCase assert_equal 2, fragment.css('ins.diff_structural').length assert_match "der Zugang erfolgt über den Hinterhof.", fragment.text end + + test "diff_against reports tag and template changes" do + n = Node.root.children.create! :slug => "field_diff_test" + d = n.find_or_create_draft @user1 + d.tag_list = "update" + d.template_name = "standard_template" + d.save! + n.publish_draft! + + d2 = n.find_or_create_draft @user1 + d2.tag_list = "update, pressemitteilung" + d2.template_name = "title_only" + d2.save! + + diff = d2.diff_against(n.head) + + assert_equal ["pressemitteilung"], diff[:tags][:added] + assert_equal [], diff[:tags][:removed] + assert diff[:template_name][:changed] + assert_equal "standard_template", diff[:template_name][:from] + assert_equal "title_only", diff[:template_name][:to] + end + + test "diff_against reports added and removed assets by filename" do + n = Node.root.children.create! :slug => "asset_diff_test" + d = n.find_or_create_draft @user1 + d.save! + n.publish_draft! + + kept_asset = Asset.create!(:upload_file_name => "kept.png", :upload_content_type => "image/png", :upload_file_size => 1) + removed_asset = Asset.create!(:upload_file_name => "removed.pdf", :upload_content_type => "application/pdf", :upload_file_size => 1) + n.head.update_assets([kept_asset.id, removed_asset.id]) + + d2 = n.find_or_create_draft @user1 + added_asset = Asset.create!(:upload_file_name => "added.png", :upload_content_type => "image/png", :upload_file_size => 1) + d2.update_assets([kept_asset.id, added_asset.id]) + d2.save! + + diff = d2.diff_against(n.head) + + assert_equal [added_asset], diff[:assets][:added] + assert_equal [removed_asset], diff[:assets][:removed] + end end -- cgit v1.3 From 47beb3fe6ed6fdb75c2dd3a55362ad1aba3bbc3b Mon Sep 17 00:00:00 2001 From: erdgeist Date: Sat, 11 Jul 2026 23:41:29 +0200 Subject: Make page_translations' search trigger self-installing search_vector is populated by a raw Postgres trigger created via execute() in a migration -- invisible to schema.rb, which only represents structure Rails understands. Any database rebuilt from schema.rb rather than replayed migrations (test, a fresh db:setup, disaster recovery) silently lost full-text search entirely, with no error -- NULL @@ anything is never true in Postgres. Page.ensure_search_vector_trigger!, called from an after_initialize initializer, reinstalls the trigger via CREATE OR REPLACE on every boot, in every environment. Idempotent and cheap. --- app/models/page.rb | 26 ++++++++++++++++++++++ .../initializers/ensure_search_vector_trigger.rb | 7 ++++++ 2 files changed, 33 insertions(+) create mode 100644 config/initializers/ensure_search_vector_trigger.rb (limited to 'app/models/page.rb') diff --git a/app/models/page.rb b/app/models/page.rb index 740d42e..db5b688 100644 --- a/app/models/page.rb +++ b/app/models/page.rb @@ -254,6 +254,32 @@ class Page < ApplicationRecord end + # Installs (or re-installs) the trigger that keeps page_translations' + # search_vector in sync. Idempotent, safe to call on every boot. + # search_vector is populated by a raw Postgres trigger, not anything + # Rails' schema dumper can represent -- a database rebuilt from + # schema.rb rather than replayed migrations silently loses it. + def self.ensure_search_vector_trigger! + connection.execute(<<~SQL) + CREATE OR REPLACE FUNCTION page_translations_search_vector_update() + RETURNS trigger AS $$ + BEGIN + NEW.search_vector := to_tsvector( + 'simple', + coalesce(NEW.title, '') || ' ' || + coalesce(NEW.abstract, '') || ' ' || + coalesce(NEW.body, '') + ); + RETURN NEW; + END; + $$ LANGUAGE plpgsql; + + CREATE OR REPLACE TRIGGER page_translations_search_vector_trigger + BEFORE INSERT OR UPDATE ON page_translations + FOR EACH ROW EXECUTE PROCEDURE page_translations_search_vector_update(); + SQL + end + private def set_page_title diff --git a/config/initializers/ensure_search_vector_trigger.rb b/config/initializers/ensure_search_vector_trigger.rb new file mode 100644 index 0000000..a750e9c --- /dev/null +++ b/config/initializers/ensure_search_vector_trigger.rb @@ -0,0 +1,7 @@ +Rails.application.config.after_initialize do + begin + Page.ensure_search_vector_trigger! if Page.connection.table_exists?(:page_translations) + rescue ActiveRecord::NoDatabaseError, ActiveRecord::ConnectionNotEstablished, PG::ConnectionBad + # Database doesn't exist yet -- e.g. mid `rails db:create`. Nothing to install yet. + end +end -- cgit v1.3 From 0b6ae26f3b4c3c8278e569fa6644a544e996c25d Mon Sep 17 00:00:00 2001 From: erdgeist Date: Mon, 13 Jul 2026 16:36:38 +0200 Subject: Add per-node translation management: list, edit, destroy, compare Replaces the old locale-switch-and-edit-the-same-screen workflow, which conflated presentation locale with content locale and let an editor silently drift into editing the wrong language with no persistent signal that anything had changed. Non-default-locale content now has its own explicit routes and screens, never sharing a route param with the ambient chrome locale. - PageTranslationsController: index/show/edit/update/destroy, scoped to Page.non_default_locales; update handles first-time creation too, so there's no separate new/create step. - Reads go through the actual PageTranslation row (Page#translation_summary), never through the Globalize fallback-bearing accessor -- fallback is correct for public rendering but wrong for editing, where a missing translation needs to look empty, not borrowed from another locale. - Translations ride on the page's own draft/head cycle; no independent publish state. - nodes#show gains a Translations section (per-locale Lock+Edit / Create+Lock, Destroy, a link into the read-only Compare view) and a locale indicator on its own default-locale content; nodes#edit, nodes#update, and nodes#autosave are pinned to the default locale via Globalize.with_locale regardless of the ambient route locale. - nodes#show no longer double-loads the node or calls wipe_draft! on every view (see previous commit for why that's now safe). - .preview_link_row is renamed .aligned_action_row now that it has a second real consumer. --- app/controllers/nodes_controller.rb | 12 ++- app/controllers/page_translations_controller.rb | 98 ++++++++++++++++++++++ app/models/page.rb | 21 +++++ app/views/nodes/show.html.erb | 49 +++++++++-- app/views/page_translations/edit.html.erb | 39 +++++++++ app/views/page_translations/index.html.erb | 19 +++++ app/views/page_translations/show.html.erb | 27 ++++++ config/routes.rb | 6 ++ public/stylesheets/admin.css | 27 +++++- .../page_translations_controller_test.rb | 78 +++++++++++++++++ 10 files changed, 366 insertions(+), 10 deletions(-) create mode 100644 app/controllers/page_translations_controller.rb create mode 100644 app/views/page_translations/edit.html.erb create mode 100644 app/views/page_translations/index.html.erb create mode 100644 app/views/page_translations/show.html.erb create mode 100644 test/controllers/page_translations_controller_test.rb (limited to 'app/models/page.rb') diff --git a/app/controllers/nodes_controller.rb b/app/controllers/nodes_controller.rb index c1468be..249602d 100644 --- a/app/controllers/nodes_controller.rb +++ b/app/controllers/nodes_controller.rb @@ -16,6 +16,8 @@ class NodesController < ApplicationController :revert ] + around_action :pin_to_default_locale, :only => [:show, :edit, :update, :autosave] + def index @nodes = Node.root.descendants.includes(:head, :draft) .order('id DESC') @@ -59,9 +61,9 @@ class NodesController < ApplicationController end def show - node = Node.find(params[:id]) - node.wipe_draft! - @page = node.draft || node.head + @page = @node.draft || @node.head + @default_translation = @page.translations.find_by(:locale => I18n.default_locale) + @translations = @page.translation_summary end def edit @@ -248,6 +250,10 @@ class NodesController < ApplicationController end end + def pin_to_default_locale + Globalize.with_locale(I18n.default_locale) { yield } + end + def descendant_counts_for(ordered_with_level) counts = Hash.new(0) stack = [] # [node, level, index] diff --git a/app/controllers/page_translations_controller.rb b/app/controllers/page_translations_controller.rb new file mode 100644 index 0000000..47f01f7 --- /dev/null +++ b/app/controllers/page_translations_controller.rb @@ -0,0 +1,98 @@ +class PageTranslationsController < ApplicationController + layout 'admin' + + before_action :login_required + before_action :find_node + before_action :find_locale, :only => [:show, :edit, :update, :destroy] + + def index + page = @node.draft || @node.head + @translations = page ? page.translation_summary : [] + end + + def show + @page = @node.draft || @node.head + @translation = @page.translations.find_by(:locale => @locale) + @default_translation = @page.translations.find_by(:locale => I18n.default_locale) + end + + def edit + @node.lock_for_editing!(current_user) + @page = @node.draft || @node.head + @translation = @page.translations.find_by(:locale => @locale) + rescue LockedByAnotherUser => e + flash[:error] = e.message + redirect_to node_path(@node) + end + + def update + draft = ensure_editable_draft + Globalize.with_locale(@locale) { draft.update!(translation_params) } + flash[:notice] = "#{@locale.upcase} translation saved. Publish the draft to make it live." + + if params[:commit] == "Save + Unlock + Exit" + @node.unlock! + redirect_to node_path(@node) + else + redirect_to edit_node_translation_path(@node, @locale) + end + rescue LockedByAnotherUser => e + flash[:error] = e.message + redirect_to node_path(@node) + end + + def destroy + base = @node.draft || @node.head + unless base && base.translated_locales.include?(@locale) + flash[:error] = "No #{@locale.to_s.upcase} translation exists to remove." + return redirect_to node_path(@node) + end + + if (base.translated_locales - [@locale]).empty? + flash[:error] = "Can't remove the only remaining translation." + return redirect_to node_path(@node) + end + + draft = ensure_editable_draft + draft.translations.where(:locale => @locale).delete_all + draft.reload + + flash[:notice] = "#{@locale.upcase} translation removed from the draft. Publish to make this permanent." + redirect_to node_path(@node) + rescue LockedByAnotherUser => e + flash[:error] = e.message + redirect_to node_path(@node) + end + + private + + def find_node + @node = Node.find(params[:node_id]) + end + + # Every remaining action's :translation_locale is already + # router-constrained to non-default locales, so this should never + # actually fire in practice -- it's a deliberate second check, kept + # as a drift detector in case the route constraint and this list + # (both driven by Page.non_default_locales) ever disagree. + def find_locale + candidate = params[:translation_locale].to_s.to_sym + unless Page.non_default_locales.include?(candidate) + raise ActionController::RoutingError, "Unknown or default locale" + end + @locale = candidate + end + + # Never mutates head directly. Locks first (same guard as the + # primary editor), then reuses an existing draft or creates one -- + # deliberately not going through the autosave buffer: this is a + # plain, explicit Save, not a live-typing surface. + def ensure_editable_draft + @node.lock_for_editing!(current_user) + @node.draft || @node.create_new_draft(current_user) + end + + def translation_params + params.fetch(:page, {}).permit(:title, :abstract, :body) + end +end diff --git a/app/models/page.rb b/app/models/page.rb index db5b688..f796228 100644 --- a/app/models/page.rb +++ b/app/models/page.rb @@ -87,6 +87,27 @@ class Page < ApplicationRecord end end + def self.non_default_locales + I18n.available_locales - [:root, I18n.default_locale] + end + + # One row per non-default locale, read from the actual translation + # row -- never through the locale-dependent accessor, so a locale + # with no real translation yet reports as absent rather than quietly + # showing a fallback value borrowed from another locale. + def translation_summary + Page.non_default_locales.map do |locale| + translation = translations.find_by(:locale => locale) + { + :locale => locale, + :exists => translation.present?, + :title => translation&.title, + :updated_at => translation&.updated_at, + :outdated => translation.present? && outdated_translations?(:locale => locale) + } + end + end + def self.untranslated(options = {:locale => :de}) PageTranslation.all.group_by(&:page_id).select do |k,v| v.size == 1 && v.map{|x| x.locale}.include?(options[:locale]) diff --git a/app/views/nodes/show.html.erb b/app/views/nodes/show.html.erb index 54bb38f..e6a922b 100644 --- a/app/views/nodes/show.html.erb +++ b/app/views/nodes/show.html.erb @@ -1,6 +1,6 @@ <% locked_by_other = @node.locked? && @node.lock_owner != current_user %>
-

<%= title_for_node(@node) %>

+

<%= title_for_node(@node) %> (<%= I18n.default_locale.to_s.upcase %>)

Status
@@ -70,6 +70,46 @@ <% end %>
+
Translations
+
+
+
+ <%= I18n.default_locale.to_s.upcase %> (default) + <%= @page.title %> — updated <%= @default_translation&.updated_at || @page.updated_at %> +
+ <% @translations.each do |t| %> +
+ <%= t[:locale].to_s.upcase %> +
+ <% if t[:exists] %> + <%= link_to t[:title], node_translation_path(@node, t[:locale]) %> — updated <%= t[:updated_at] %> + <% if t[:outdated] %> + may be outdated + <% end %> + <% else %> + Not yet translated. + <% end %> + <% if locked_by_other %> + <%= t[:exists] ? "Lock + Edit" : "Create translation + Lock" %> + <% else %> + <%= link_to edit_node_translation_path(@node, t[:locale]), class: "action_button" do %> + <%= icon(t[:exists] ? "edit" : "language", library: "tabler", "aria-hidden": true) %> + <%= t[:exists] ? "Lock + Edit" : "Create translation + Lock" %> + <% end %> + <% if t[:exists] %> + <%= button_to node_translation_path(@node, t[:locale]), method: :delete, + form: { data: { confirm: "This cannot be undone. Continue?" }, class: 'button_to destructive' } do %> + <%= icon("trash", library: "tabler", "aria-hidden": true) %> + Destroy Translation + <% end %> + <% end %> + <% end %> +
+
+ <% end %> +
+
+
People
@@ -122,7 +162,7 @@
Public Preview <% if @node.draft.preview_token.present? %> - <% end %> -
Abstract
+
Abstract (<%= I18n.default_locale.to_s.upcase %>)
<%= sanitize(@page.abstract) %>
-
Body
+
Body (<%= I18n.default_locale.to_s.upcase %>)
<%= sanitize(@page.body) %>
- <%# Assets not yet addressed - no confirmed model/association seen for this page's attached assets %>
diff --git a/app/views/page_translations/edit.html.erb b/app/views/page_translations/edit.html.erb new file mode 100644 index 0000000..7371a42 --- /dev/null +++ b/app/views/page_translations/edit.html.erb @@ -0,0 +1,39 @@ +
+ Editing the <%= @locale.to_s.upcase %> translation of <%= title_for_node(@node) %>. +
+ +

+ Metadata such as images, tags, template, and author belong to the page + as a whole, not to a single translation — change those from the + <%= link_to 'default-locale editor', edit_node_path(@node) %>. +

+ +
+ <%= button_to 'Unlock + Back', unlock_node_path(@node), method: :put, + form: { class: 'button_to state_changing' }, + disabled: @node.autosave.present? %> + <%= submit_tag "Save #{@locale.to_s.upcase} translation", form: "translation_edit_form" %> + <%= submit_tag "Save + Unlock + Exit", form: "translation_edit_form" %> + <%= link_to "Preview ↗", preview_page_path(@page, :locale => @locale), target: "_blank", rel: "noopener", class: "preview_link" %> +
+ +
+ <%= form_with url: node_translation_path(@node, @locale), method: :patch, local: true, id: "translation_edit_form" do |f| %> +
+
Title
+
+ <%= text_field_tag "page[title]", @translation&.title %> +
+ +
Abstract
+
+ <%= text_area_tag "page[abstract]", @translation&.abstract %> +
+ +
Body
+
+ <%= text_area_tag "page[body]", @translation&.body, :class => 'with_editor' %> +
+
+ <% end %> +
diff --git a/app/views/page_translations/index.html.erb b/app/views/page_translations/index.html.erb new file mode 100644 index 0000000..a8c4f5d --- /dev/null +++ b/app/views/page_translations/index.html.erb @@ -0,0 +1,19 @@ +

Translations — <%= title_for_node(@node) %>

+ +
+
+ <% @translations.each do |t| %> +
+ <%= t[:locale].to_s.upcase %> + <% if t[:exists] %> + <%= t[:title] %> — updated <%= t[:updated_at] %> + <% if t[:outdated] %>may be outdated<% end %> + <%= link_to 'Edit', edit_node_translation_path(@node, t[:locale]) %> + <% else %> + Not yet translated. + <%= link_to 'Add', edit_node_translation_path(@node, t[:locale]) %> + <% end %> +
+ <% end %> +
+
diff --git a/app/views/page_translations/show.html.erb b/app/views/page_translations/show.html.erb new file mode 100644 index 0000000..c5bd151 --- /dev/null +++ b/app/views/page_translations/show.html.erb @@ -0,0 +1,27 @@ +

Compare — <%= title_for_node(@node) %>

+ +
+
+

<%= @locale.to_s.upcase %>

+
Title
+
<%= @translation&.title %>
+ +
Abstract
+
<%= sanitize(@translation&.abstract.to_s) %>
+ +
Body
+
<%= sanitize(@translation&.body.to_s) %>
+
+ +
+

<%= I18n.default_locale.to_s.upcase %> (default)

+
Title
+
<%= @default_translation&.title %>
+ +
Abstract
+
<%= sanitize(@default_translation&.abstract.to_s) %>
+ +
Body
+
<%= sanitize(@default_translation&.body.to_s) %>
+
+
diff --git a/config/routes.rb b/config/routes.rb index 5d61bae..38a497d 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -55,6 +55,12 @@ Cccms::Application.routes.draw do put :revert end + resources :translations, controller: 'page_translations', + param: :translation_locale, + constraints: { translation_locale: /en/ }, + only: [:index, :show, :edit, :update, :destroy] + + resources :related_assets, only: [:create, :destroy, :update] do collection do get :search diff --git a/public/stylesheets/admin.css b/public/stylesheets/admin.css index c3c0cb4..f1f4c05 100644 --- a/public/stylesheets/admin.css +++ b/public/stylesheets/admin.css @@ -637,6 +637,28 @@ table.revisions_table tr:hover { margin: 0; } +/* ============================================================ + Translation compare view (page_translations#show) + ============================================================ */ + +.translation_compare { + display: flex; + flex-direction: column; + gap: 1.5rem; +} + +@media(min-width:1016px) { + .translation_compare { + flex-direction: row; + gap: 2rem; + } + + .translation_compare_column { + flex: 1; + min-width: 0; + } +} + table.user_table td.user_login { padding-right: 30px; } @@ -834,14 +856,15 @@ form.button_to button[type="submit"] { font-weight: bold; } -.preview_link_row { +.aligned_action_row { display: flex; flex-wrap: wrap; align-items: start; gap: 0.5rem; } -.preview_link_row form.button_to { +.aligned_action_row .action_button, +.aligned_action_row form.button_to { margin-top: -5px; } diff --git a/test/controllers/page_translations_controller_test.rb b/test/controllers/page_translations_controller_test.rb new file mode 100644 index 0000000..d84ac45 --- /dev/null +++ b/test/controllers/page_translations_controller_test.rb @@ -0,0 +1,78 @@ +require 'test_helper' + +class PageTranslationsControllerTest < ActionController::TestCase + test "index lists the default locale's existing translation and flags a missing one" do + login_as :quentin + node = Node.root.children.create!(:slug => "translations_index_test") + node.publish_draft! + + get :index, params: { :node_id => node.id } + + assert_response :success + assert_equal [:en], assigns(:translations).map { |t| t[:locale] } + assert_equal false, assigns(:translations).first[:exists] + end + + test "update creates a first-time translation on a fresh draft, leaving head untouched" do + login_as :quentin + node = Node.root.children.create!(:slug => "translations_create_test") + Globalize.with_locale(:de) { node.draft.update!(:title => "Deutscher Titel") } + node.publish_draft! + + patch :update, params: { :node_id => node.id, :translation_locale => "en", :page => { :title => "English Title" } } + + node.reload + assert_not_nil node.draft + assert_includes node.draft.translated_locales, :en + assert_not_includes node.head.translated_locales, :en + end + + test "no route exists for the default locale under the translations resource" do + login_as :quentin + node = Node.root.children.create!(:slug => "translations_route_constraint_test") + + assert_raises(ActionController::UrlGenerationError) do + patch :update, params: { :node_id => node.id, :translation_locale => "de", :page => { :title => "x" } } + end + end + + test "update with Save + Unlock + Exit unlocks the node and redirects to nodes#show" do + login_as :quentin + node = Node.root.children.create!(:slug => "translations_exit_test") + + patch :update, params: { :node_id => node.id, :translation_locale => "en", :page => { :title => "x" }, :commit => "Save + Unlock + Exit" } + + assert_nil node.reload.lock_owner + assert_redirected_to node_path(node) + end + + test "update saves the translation onto the draft" do + login_as :quentin + node = Node.root.children.create!(:slug => "translations_update_test") + Globalize.with_locale(:en) { node.draft.update!(:title => "Original") } + + patch :update, params: { :node_id => node.id, :translation_locale => "en", :page => { :title => "Revised" } } + + assert_equal "Revised", Globalize.with_locale(:en) { node.draft.reload.title } + end + + test "destroy refuses to remove the only remaining translation" do + login_as :quentin + node = Node.root.children.create!(:slug => "translations_destroy_last_test") + Globalize.with_locale(:en) { node.draft.update!(:title => "Only translation") } + node.draft.translations.where(:locale => :de).delete_all + + delete :destroy, params: { :node_id => node.id, :translation_locale => "en" } + + assert_equal "Can't remove the only remaining translation.", flash[:error] + end + + test "destroy is a safe no-op, not a false success, when the translation doesn't exist" do + login_as :quentin + node = Node.root.children.create!(:slug => "translations_destroy_missing_test") + + delete :destroy, params: { :node_id => node.id, :translation_locale => "en" } + + assert_match(/No EN translation exists/, flash[:error]) + end +end -- cgit v1.3 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(-) (limited to 'app/models/page.rb') 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
TitleIs primary Start time End time Rrule Custom rrule Allday UrlLatitudeLongitude Node
<%=h event.display_title %><%=h event.is_primary %> <%=h event.start_time %> <%=h event.end_time %> <%=h event.rrule %> <%=h event.custom_rrule %> <%=h event.allday %> <%=h event.url %><%=h event.latitude %><%=h event.longitude %> <%=h event.node_id %> <%= link_to 'Show', event %> <%= link_to 'Edit', edit_event_path(event) %>