From 6ec7cd702a43ba8134ba93a96ff43477df424dd6 Mon Sep 17 00:00:00 2001 From: vangberg Date: Thu, 17 Sep 2026 10:07:43 +0200 Subject: [PATCH 01/41] Add minimal Versioned concern and enable it for Name --- app/models/concerns/versioned.rb | 26 +++++++ app/models/name.rb | 1 + app/models/version.rb | 3 + db/migrate/20260917100000_create_versions.rb | 10 +++ db/schema.rb | 11 ++- test/models/name/versioned_test.rb | 77 ++++++++++++++++++++ 6 files changed, 127 insertions(+), 1 deletion(-) create mode 100644 app/models/concerns/versioned.rb create mode 100644 app/models/version.rb create mode 100644 db/migrate/20260917100000_create_versions.rb create mode 100644 test/models/name/versioned_test.rb diff --git a/app/models/concerns/versioned.rb b/app/models/concerns/versioned.rb new file mode 100644 index 00000000..ac6b7b74 --- /dev/null +++ b/app/models/concerns/versioned.rb @@ -0,0 +1,26 @@ +module Versioned + extend ActiveSupport::Concern + + included do + has_many :versions, as: :record + + after_create :record_create_version + after_update :record_update_version + after_destroy :record_delete_version + end + + private + + def record_create_version + versions.create!(operation: 'create', changeset: {}) + end + + def record_update_version + changeset = saved_changes.except('created_at', 'updated_at') + versions.create!(operation: 'update', changeset: changeset) if changeset.any? + end + + def record_delete_version + Version.create!(record: self, operation: 'delete', changeset: {}) + end +end diff --git a/app/models/name.rb b/app/models/name.rb index 9fef19ca..4e950b22 100644 --- a/app/models/name.rb +++ b/app/models/name.rb @@ -147,6 +147,7 @@ class Name < ApplicationRecord include Name::Wiki include Name::FuzzySearch include TypeMaterial + include Versioned attr_accessor :only_display attr_accessor :nomenclatural_type_entry diff --git a/app/models/version.rb b/app/models/version.rb new file mode 100644 index 00000000..588d1a2a --- /dev/null +++ b/app/models/version.rb @@ -0,0 +1,3 @@ +class Version < ApplicationRecord + belongs_to :record, polymorphic: true, optional: true +end diff --git a/db/migrate/20260917100000_create_versions.rb b/db/migrate/20260917100000_create_versions.rb new file mode 100644 index 00000000..886bd899 --- /dev/null +++ b/db/migrate/20260917100000_create_versions.rb @@ -0,0 +1,10 @@ +class CreateVersions < ActiveRecord::Migration[6.1] + def change + create_table :versions do |t| + t.references :record, polymorphic: true, null: false + t.string :operation, null: false + t.jsonb :changeset, null: false + t.datetime :created_at, null: false + end + end +end diff --git a/db/schema.rb b/db/schema.rb index 6f900aa0..e166d71f 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 2026_08_19_100000) do +ActiveRecord::Schema.define(version: 2026_09_17_100000) do # These are extensions that must be enabled in order to support this database enable_extension "fuzzystrmatch" @@ -568,6 +568,15 @@ t.index ["username"], name: "index_users_on_username", unique: true end + create_table "versions", force: :cascade do |t| + t.string "record_type", null: false + t.bigint "record_id", null: false + t.string "operation", null: false + t.jsonb "changeset", null: false + t.datetime "created_at", null: false + t.index ["record_type", "record_id"], name: "index_versions_on_record" + end + create_table "wikispecies_credentials", force: :cascade do |t| t.bigint "user_id", null: false t.text "encrypted_access_token", null: false diff --git a/test/models/name/versioned_test.rb b/test/models/name/versioned_test.rb new file mode 100644 index 00000000..358e88c8 --- /dev/null +++ b/test/models/name/versioned_test.rb @@ -0,0 +1,77 @@ +require 'test_helper' + +class Name::VersionedTest < ActiveSupport::TestCase + setup do + @name = Name.new(name: 'E. coli', rank: 'species') + end + + test 'creating a name records an empty changeset' do + assert_difference -> { @name.versions.count } do + @name.save! + end + + version = @name.versions.last + assert_equal 'create', version.operation + assert_empty version.changeset + end + + test 'updating a name records only changed attributes' do + @name.save! + + assert_difference -> { @name.versions.count } do + @name.update!(syllabication: 'co.li') + end + + version = @name.versions.last + assert_equal 'update', version.operation + assert_equal [nil, 'co.li'], version.changeset['syllabication'] + assert_equal %w[syllabication], version.changeset.keys + end + + test 'updating only timestamps does not add a version' do + @name.save! + + assert_no_difference -> { @name.versions.count } do + @name.update!(created_at: 1.day.ago, updated_at: 1.hour.ago) + end + end + + test 'saving an unchanged name does not add a version' do + @name.save! + + assert_no_difference -> { @name.versions.count } do + @name.save! + end + end + + test 'deleting a name records an empty changeset' do + @name.save! + versions = Version.where(record: @name) + + assert_difference -> { versions.count } do + @name.destroy! + end + + version = versions.last + assert_equal 'delete', version.operation + assert_empty version.changeset + end + + test 'deleting a name retains its history' do + @name.save! + version = @name.versions.last + + @name.destroy! + + assert Version.exists?(version.id) + end + + test 'versions roll back with the name' do + assert_no_difference -> { Version.count } do + Name.transaction do + @name.save! + raise ActiveRecord::Rollback + end + end + end +end From 1d3878d71544231b47051da366a873d3c838453c Mon Sep 17 00:00:00 2001 From: vangberg Date: Thu, 17 Sep 2026 10:57:46 +0200 Subject: [PATCH 02/41] Record related Name attributes together --- app/models/concerns/versioned.rb | 38 +++++++++++++++++++++++++++++- app/models/name.rb | 3 +++ test/models/name/versioned_test.rb | 15 ++++++++++++ 3 files changed, 55 insertions(+), 1 deletion(-) diff --git a/app/models/concerns/versioned.rb b/app/models/concerns/versioned.rb index ac6b7b74..d0a8ed37 100644 --- a/app/models/concerns/versioned.rb +++ b/app/models/concerns/versioned.rb @@ -3,12 +3,20 @@ module Versioned included do has_many :versions, as: :record + class_attribute :versioned_attribute_sets, default: [] after_create :record_create_version after_update :record_update_version after_destroy :record_delete_version end + class_methods do + # Changing any member records before/after values for every member. + def versioned_together(*attributes) + self.versioned_attribute_sets = versioned_attribute_sets + [attributes.map(&:to_s)] + end + end + private def record_create_version @@ -16,11 +24,39 @@ def record_create_version end def record_update_version - changeset = saved_changes.except('created_at', 'updated_at') + changeset = versioned_changes versions.create!(operation: 'update', changeset: changeset) if changeset.any? end def record_delete_version + # The owner is already destroyed, so create directly rather than through + # its association. Deletion records only the event and retains history. Version.create!(record: self, operation: 'delete', changeset: {}) end + + # An update changeset maps each changed attribute to [before, after], e.g. + # { 'syllabication' => [nil, 'co.li'] } for E. coli. + # Create and delete versions have an empty changeset; timestamps are omitted. + def versioned_changes + changeset = saved_changes.except('created_at', 'updated_at') + + changed_columns = changeset.keys + self.class.versioned_attribute_sets.each do |columns| + changeset.merge!(versioned_together_changes(columns, changed_columns)) + end + changeset + end + + # If any listed column changed, include each member with a value as an + # ordinary [before, after] pair, including unchanged columns such as language: + # { 'etymology_xx_lang' => ['N.L.', 'N.L.'], + # 'etymology_xx_description' => ['of the colon', 'from the colon'] }. + def versioned_together_changes(columns, changed_columns) + return {} if (changed_columns & columns).empty? + + columns.filter_map do |column| + values = [attribute_before_last_save(column), self[column]] + [column, values] unless values == [nil, nil] + end.to_h + end end diff --git a/app/models/name.rb b/app/models/name.rb index 4e950b22..c6a240ff 100644 --- a/app/models/name.rb +++ b/app/models/name.rb @@ -149,6 +149,9 @@ class Name < ApplicationRecord include TypeMaterial include Versioned + ETYMOLOGY_COLUMNS = column_names.grep(/\Aetymology_/).freeze + versioned_together(:name, *ETYMOLOGY_COLUMNS) + attr_accessor :only_display attr_accessor :nomenclatural_type_entry attr_accessor :qc_for_tutorial diff --git a/test/models/name/versioned_test.rb b/test/models/name/versioned_test.rb index 358e88c8..f1c18178 100644 --- a/test/models/name/versioned_test.rb +++ b/test/models/name/versioned_test.rb @@ -36,6 +36,21 @@ class Name::VersionedTest < ActiveSupport::TestCase end end + test 'changing an etymology description snapshots valued etymology attributes' do + @name.assign_attributes( + etymology_xx_lang: 'N.L.', etymology_xx_description: 'of the colon' + ) + @name.save! + + @name.update!(etymology_xx_description: 'from the colon') + + changeset = @name.versions.last.changeset + assert_equal %w[etymology_xx_description etymology_xx_lang name], changeset.keys.sort + assert_equal ['of the colon', 'from the colon'], changeset['etymology_xx_description'] + assert_equal ['N.L.', 'N.L.'], changeset['etymology_xx_lang'] + assert_equal ['E. coli', 'E. coli'], changeset['name'] + end + test 'saving an unchanged name does not add a version' do @name.save! From 9afc744b2e4e08448f57986250d87c25bcf0929f Mon Sep 17 00:00:00 2001 From: vangberg Date: Thu, 17 Sep 2026 12:03:22 +0200 Subject: [PATCH 03/41] Add public name history view --- Gemfile | 1 + Gemfile.lock | 2 + app/assets/stylesheets/names.scss | 61 ++++++++++++++++++++++++++ app/controllers/names_controller.rb | 9 +++- app/helpers/names_helper.rb | 29 ++++++++++++ app/models/name/public_history.rb | 52 ++++++++++++++++++++++ app/views/names/_title.html.erb | 2 +- app/views/names/history.html.erb | 34 ++++++++++++++ config/routes.rb | 1 + test/controllers/names/history_test.rb | 31 +++++++++++++ 10 files changed, 220 insertions(+), 2 deletions(-) create mode 100644 app/models/name/public_history.rb create mode 100644 app/views/names/history.html.erb create mode 100644 test/controllers/names/history_test.rb diff --git a/Gemfile b/Gemfile index 6cc3e289..8b4a2ef4 100644 --- a/Gemfile +++ b/Gemfile @@ -14,6 +14,7 @@ gem 'bootstrap', '~> 4.6.2' gem 'coffee-rails', '~> 5.0' gem 'csv' gem 'd3-rails' +gem 'diff-lcs', '~> 2.0' gem 'dartsass-sprockets' # Sass engine (required by bootstrap gem 'devise' gem 'dkim', '~> 1.1' diff --git a/Gemfile.lock b/Gemfile.lock index 9e1cbdb6..27949cdc 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -114,6 +114,7 @@ GEM railties (>= 4.1.0) responders warden (~> 1.2.3) + diff-lcs (2.0.0) dkim (1.1.0) erb (6.0.4) erubi (1.13.1) @@ -487,6 +488,7 @@ DEPENDENCIES d3-rails dartsass-sprockets devise + diff-lcs (~> 2.0) dkim (~> 1.1) font_awesome5_rails (~> 1.5) httparty (~> 0.23) diff --git a/app/assets/stylesheets/names.scss b/app/assets/stylesheets/names.scss index 6064a6a6..c88ec0d7 100644 --- a/app/assets/stylesheets/names.scss +++ b/app/assets/stylesheets/names.scss @@ -16,3 +16,64 @@ } } +.name-history-version { + display: grid; + grid-template-columns: 11rem minmax(0, 1fr); + gap: 1rem; + padding: 0.65rem 0.75rem; + + .name-history-date { + color: #6c757d; + font-size: 0.875rem; + white-space: nowrap; + } +} + +.name-history-change { + display: grid; + grid-template-columns: 10rem minmax(0, 1fr); + gap: 0.5rem; + padding: 0.3rem 0.5rem; + + &:nth-child(even) { + background-color: #f7f8fa; + } +} + +.name-history-values { + display: grid; + grid-template-columns: minmax(0, 1fr) 2rem minmax(0, 1fr); + align-items: center; + min-width: 0; + overflow-wrap: anywhere; +} + +.name-history-before, +.name-history-after { + white-space: pre-wrap; +} + +.name-history-arrow { + color: #6c757d; + text-align: center; +} + +@media (max-width: 767px) { + .name-history-version { + display: block; + + .name-history-date { + display: block; + margin-bottom: 0.2rem; + } + } + + .name-history-change { + display: block; + + > strong { + display: block; + margin-bottom: 0.15rem; + } + } +} diff --git a/app/controllers/names_controller.rb b/app/controllers/names_controller.rb index a7c26ee3..c9870b03 100644 --- a/app/controllers/names_controller.rb +++ b/app/controllers/names_controller.rb @@ -12,7 +12,7 @@ class NamesController < ApplicationController before_action( :set_name, only: %i[ - edit update destroy network wiki + edit update destroy network wiki history proposed_in not_validly_proposed_in emended_in assigned_in corrigendum_in corrigendum_orphan corrigendum edit_description edit_rank edit_notes edit_etymology edit_links edit_type @@ -215,6 +215,13 @@ def show end end + # GET /names/1/history + def history + @history = Name::PublicHistory.new(@name) + @versions = @history.versions(page: params[:page]) + @crumbs = [['Names', names_path], [@name.abbr_name, @name], 'History'] + end + # GET /names/linkout.xml # GET /names/1/linkout.xml def linkout diff --git a/app/helpers/names_helper.rb b/app/helpers/names_helper.rb index 0fd59158..038f87bc 100644 --- a/app/helpers/names_helper.rb +++ b/app/helpers/names_helper.rb @@ -1,4 +1,30 @@ +require 'diff/lcs' + module NamesHelper + def history_before_after_diff(before, after, attribute:) + if attribute == 'etymology' + before = Name.new(before).full_etymology + after = Name.new(after).full_etymology + end + + changes = Diff::LCS.sdiff(before.to_s.scan(/\X/), after.to_s.scan(/\X/)) + + before_parts = changes.filter_map do |change| + case change.action + when '=' then change.old_element + when '-', '!' then content_tag(:del, change.old_element, class: 'text-danger') + end + end + after_parts = changes.filter_map do |change| + case change.action + when '=' then change.new_element + when '+', '!' then content_tag(:ins, change.new_element, class: 'text-success') + end + end + + [safe_join(before_parts).presence || '—', safe_join(after_parts).presence || '—'] + end + def link_to_name_type(name) if name.type_is_name? if name.type_name @@ -64,4 +90,7 @@ def name_lineage(name, links: true, last: true, register: nil, visited: []) out.inject(:+) end + + private + end diff --git a/app/models/name/public_history.rb b/app/models/name/public_history.rb new file mode 100644 index 00000000..09ba3392 --- /dev/null +++ b/app/models/name/public_history.rb @@ -0,0 +1,52 @@ +class Name::PublicHistory + # Only these recorded attributes appear in the public history. + KEYS = %w[ + name rank syllabication status priority_date authority + nomenclatural_status taxonomic_status proposal_kind corrigendum_from + type_material type_accession + ].concat(Name::ETYMOLOGY_COLUMNS).freeze + + def initialize(name) + @name = name + end + + # Filter before paginating so each page contains public updates only. + def versions(page:) + @name.versions + .where(operation: 'update') + .where( + 'EXISTS (SELECT 1 FROM jsonb_object_keys(versions.changeset) AS key WHERE key IN (?))', + KEYS + ) + .order(created_at: :desc, id: :desc) + .paginate(page: page, per_page: 25) + end + + # Ordinary fields keep their [before, after] pairs. Etymology fields become + # one pair of attribute snapshots for the view to render together. + def changes_for(version) + changes = version.changeset.slice(*KEYS) + etymology = changes.extract!(*Name::ETYMOLOGY_COLUMNS) + if etymology.any? + changes['etymology'] = etymology_snapshots(etymology, changes['name']) + end + # versioned_together records unchanged companion values too. + changes.reject! { |_, values| values.first == values.last } + changes + end + + private + + # Include the name because full_etymology derives the :xx particle from its + # last word. Older versions without a name pair use the current name. + def etymology_snapshots(etymology, names) + before_name, after_name = names || [@name.name, @name.name] + before_attributes = etymology.transform_values(&:first) + after_attributes = etymology.transform_values(&:last) + + [ + before_attributes.merge('name' => before_name), + after_attributes.merge('name' => after_name) + ] + end +end diff --git a/app/views/names/_title.html.erb b/app/views/names/_title.html.erb index 636bf2e2..b85062bb 100644 --- a/app/views/names/_title.html.erb +++ b/app/views/names/_title.html.erb @@ -18,6 +18,7 @@ %> <%= download_buttons([ + [history_name_path(@name), 'history', 'History'], [name_url(@name, format: :json), 'file-code', 'JSON'], [name_url(@name, format: :pdf), 'file-pdf', 'PDF'], observe @@ -78,4 +79,3 @@ <% end %>
- diff --git a/app/views/names/history.html.erb b/app/views/names/history.html.erb new file mode 100644 index 00000000..cea97867 --- /dev/null +++ b/app/views/names/history.html.erb @@ -0,0 +1,34 @@ +<% provide(:title, "History of #{@name.abbr_name_raw}") %> + +

History of <%= @name.name_html %>

+

<%= link_to('Back to name', @name) %>

+ +<% if @versions.empty? %> +

No history has been recorded for this name.

+<% else %> +
+ <% @versions.each do |version| %> +
+ + +
+ <% @history.changes_for(version).each do |attribute, (before, after)| %> + <% before_diff, after_diff = history_before_after_diff(before, after, attribute: attribute) %> +
+ <%= Name.human_attribute_name(attribute) %>: + + Before: <%= before_diff %> + + After: <%= after_diff %> + +
+ <% end %> +
+
+ <% end %> +
+<% end %> + +<%= will_paginate(@versions) %> diff --git a/config/routes.rb b/config/routes.rb index d8b5962c..f10d0dea 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -86,6 +86,7 @@ end member do # --> Display name + get :history get :network get :linkout get :wiki diff --git a/test/controllers/names/history_test.rb b/test/controllers/names/history_test.rb new file mode 100644 index 00000000..4fbfcad2 --- /dev/null +++ b/test/controllers/names/history_test.rb @@ -0,0 +1,31 @@ +require 'test_helper' + +class NamesHistoryTest < ActionDispatch::IntegrationTest + test 'shows public name history' do + name = names(:escherichia_coli) + name.update!(authority: 'Smith', etymology_xx_description: 'of the colon') + + get history_name_path(name) + + assert_response :success + assert_select '.name-history-version', count: 1 + assert_select '.name-history-change', count: 2 + end + + test 'shows an empty state without recorded history' do + get history_name_path(names(:escherichia_coli)) + + assert_response :success + assert_select 'p', text: 'No history has been recorded for this name.' + end + + test 'does not reveal private name history' do + name = names(:draft_by_contributor) + name.update!(syllabication: 'co.li') + + get history_name_path(name) + + assert_select 'h3', text: 'Insufficient permits' + assert_select '.name-history-version', count: 0 + end +end From 53e736ab682e52168ee4697430b4b1c5bfcd9013 Mon Sep 17 00:00:00 2001 From: vangberg Date: Fri, 25 Sep 2026 10:50:42 +0200 Subject: [PATCH 04/41] Refactor name history view into partials --- app/views/names/history.html.erb | 13 ++----------- app/views/names/history/_name.html.erb | 11 +++++++++++ 2 files changed, 13 insertions(+), 11 deletions(-) create mode 100644 app/views/names/history/_name.html.erb diff --git a/app/views/names/history.html.erb b/app/views/names/history.html.erb index cea97867..790c63a9 100644 --- a/app/views/names/history.html.erb +++ b/app/views/names/history.html.erb @@ -14,17 +14,8 @@
- <% @history.changes_for(version).each do |attribute, (before, after)| %> - <% before_diff, after_diff = history_before_after_diff(before, after, attribute: attribute) %> -
- <%= Name.human_attribute_name(attribute) %>: - - Before: <%= before_diff %> - - After: <%= after_diff %> - -
- <% end %> + <%= render "names/history/#{version.record_type.underscore}", + version: version, history: @history %>
<% end %> diff --git a/app/views/names/history/_name.html.erb b/app/views/names/history/_name.html.erb new file mode 100644 index 00000000..6beacb41 --- /dev/null +++ b/app/views/names/history/_name.html.erb @@ -0,0 +1,11 @@ +<% history.changes_for(version).each do |attribute, (before, after)| %> + <% before_diff, after_diff = history_before_after_diff(before, after, attribute: attribute) %> +
+ <%= Name.human_attribute_name(attribute) %>: + + Before: <%= before_diff %> + + After: <%= after_diff %> + +
+<% end %> From 5d53d8174403849e4af609137eb63979302db875 Mon Sep 17 00:00:00 2001 From: vangberg Date: Fri, 2 Oct 2026 11:39:34 +0200 Subject: [PATCH 05/41] Revert "Refactor name history view into partials" This reverts commit 53e736ab682e52168ee4697430b4b1c5bfcd9013. --- app/views/names/history.html.erb | 13 +++++++++++-- app/views/names/history/_name.html.erb | 11 ----------- 2 files changed, 11 insertions(+), 13 deletions(-) delete mode 100644 app/views/names/history/_name.html.erb diff --git a/app/views/names/history.html.erb b/app/views/names/history.html.erb index 790c63a9..cea97867 100644 --- a/app/views/names/history.html.erb +++ b/app/views/names/history.html.erb @@ -14,8 +14,17 @@
- <%= render "names/history/#{version.record_type.underscore}", - version: version, history: @history %> + <% @history.changes_for(version).each do |attribute, (before, after)| %> + <% before_diff, after_diff = history_before_after_diff(before, after, attribute: attribute) %> +
+ <%= Name.human_attribute_name(attribute) %>: + + Before: <%= before_diff %> + + After: <%= after_diff %> + +
+ <% end %>
<% end %> diff --git a/app/views/names/history/_name.html.erb b/app/views/names/history/_name.html.erb deleted file mode 100644 index 6beacb41..00000000 --- a/app/views/names/history/_name.html.erb +++ /dev/null @@ -1,11 +0,0 @@ -<% history.changes_for(version).each do |attribute, (before, after)| %> - <% before_diff, after_diff = history_before_after_diff(before, after, attribute: attribute) %> -
- <%= Name.human_attribute_name(attribute) %>: - - Before: <%= before_diff %> - - After: <%= after_diff %> - -
-<% end %> From 1862f5ed5c2a28498d930582f4d73062d3c62027 Mon Sep 17 00:00:00 2001 From: vangberg Date: Fri, 2 Oct 2026 12:04:21 +0200 Subject: [PATCH 06/41] Group nearby character edits in name history diffs --- app/helpers/names_helper.rb | 46 +++++++++++++++++++------------ app/views/names/history.html.erb | 7 ++--- lib/text_diff.rb | 20 ++++++++++++++ test/helpers/names_helper_test.rb | 28 +++++++++++++++++++ test/lib/text_diff_test.rb | 39 ++++++++++++++++++++++++++ 5 files changed, 119 insertions(+), 21 deletions(-) create mode 100644 lib/text_diff.rb create mode 100644 test/helpers/names_helper_test.rb create mode 100644 test/lib/text_diff_test.rb diff --git a/app/helpers/names_helper.rb b/app/helpers/names_helper.rb index 038f87bc..def28f36 100644 --- a/app/helpers/names_helper.rb +++ b/app/helpers/names_helper.rb @@ -1,28 +1,40 @@ -require 'diff/lcs' - module NamesHelper - def history_before_after_diff(before, after, attribute:) + def history_before_value(changes, attribute:) + before = changes.first if attribute == 'etymology' - before = Name.new(before).full_etymology - after = Name.new(after).full_etymology + before = history_etymology_parts(*changes).first end - changes = Diff::LCS.sdiff(before.to_s.scan(/\X/), after.to_s.scan(/\X/)) + safe_join(Array(before)).presence || '—' + end - before_parts = changes.filter_map do |change| - case change.action - when '=' then change.old_element - when '-', '!' then content_tag(:del, change.old_element, class: 'text-danger') - end + def history_after_value(changes, attribute:) + after = changes.last + if attribute == 'etymology' + after = history_etymology_parts(*changes).last end - after_parts = changes.filter_map do |change| - case change.action - when '=' then change.new_element - when '+', '!' then content_tag(:ins, change.new_element, class: 'text-success') - end + + safe_join(Array(after)).presence || '—' + end + + def history_etymology_parts(before, after) + before = Name.new(before).full_etymology + after = Name.new(after).full_etymology + + chunks = TextDiff.chunks(before, after) + + before_parts = chunks.filter_map do |unchanged, text, _| + next if text.empty? + + unchanged ? text : content_tag(:del, text, class: 'text-danger') + end + after_parts = chunks.filter_map do |unchanged, _, text| + next if text.empty? + + unchanged ? text : content_tag(:ins, text, class: 'text-success') end - [safe_join(before_parts).presence || '—', safe_join(after_parts).presence || '—'] + [before_parts, after_parts] end def link_to_name_type(name) diff --git a/app/views/names/history.html.erb b/app/views/names/history.html.erb index cea97867..e7def6e5 100644 --- a/app/views/names/history.html.erb +++ b/app/views/names/history.html.erb @@ -14,14 +14,13 @@
- <% @history.changes_for(version).each do |attribute, (before, after)| %> - <% before_diff, after_diff = history_before_after_diff(before, after, attribute: attribute) %> + <% @history.changes_for(version).each do |attribute, changes| %>
<%= Name.human_attribute_name(attribute) %>: - Before: <%= before_diff %> + Before: <%= history_before_value(changes, attribute: attribute) %> - After: <%= after_diff %> + After: <%= history_after_value(changes, attribute: attribute) %>
<% end %> diff --git a/lib/text_diff.rb b/lib/text_diff.rb new file mode 100644 index 00000000..e19acd46 --- /dev/null +++ b/lib/text_diff.rb @@ -0,0 +1,20 @@ +require 'diff/lcs' + +module TextDiff + # Returns [unchanged, before_text, after_text] chunks. For an E. coli etymology + # edit, 'of the colon' -> 'from the colon' gives: + # [[false, 'of', 'from'], [true, ' the colon', ' the colon']]. + def self.chunks(before, after) + changes = Diff::LCS.sdiff(before.to_s.scan(/\X/), after.to_s.scan(/\X/)) + # Group characters into unchanged/changed runs, combining additions and deletions. + runs = changes.chunk { |change| change.action == '=' }.to_a + # Absorb unchanged islands of up to two characters between edits; keep the ends. + runs[1...-1].to_a.each { |run| run[0] = false if run[1].size <= 2 } + # Merge the resulting runs into contiguous text chunks. + runs.chunk_while { |a, b| a[0] == b[0] }.map do |group| + chunk = group.flat_map(&:last) + [group.first[0], chunk.map(&:old_element).compact.join, + chunk.map(&:new_element).compact.join] + end + end +end diff --git a/test/helpers/names_helper_test.rb b/test/helpers/names_helper_test.rb new file mode 100644 index 00000000..924c4520 --- /dev/null +++ b/test/helpers/names_helper_test.rb @@ -0,0 +1,28 @@ +require 'test_helper' + +class NamesHelperTest < ActionView::TestCase + test 'groups nearby etymology edits into one span' do + changes = [ + { name: 'E. coli', etymology_xx_description: 'of the colon' }, + { name: 'E. coli', etymology_xx_description: 'from the colon' } + ] + before = history_before_value(changes, attribute: 'etymology') + after = history_after_value(changes, attribute: 'etymology') + + assert_equal 'coli, of the colon', before + assert_equal 'coli, from the colon', after + end + + test 'keeps distant edits separate and escapes their content' do + changes = [ + { name: 'E. coli', etymology_xx_description: '< E. coli >' }, + { name: 'E. coli', etymology_xx_description: '& E. coli !' } + ] + before = history_before_value(changes, attribute: 'etymology') + after = history_after_value(changes, attribute: 'etymology') + + assert_equal 'coli, < E. coli >', before + assert_equal 'coli, & E. coli !', after + end + +end diff --git a/test/lib/text_diff_test.rb b/test/lib/text_diff_test.rb new file mode 100644 index 00000000..c31c04dc --- /dev/null +++ b/test/lib/text_diff_test.rb @@ -0,0 +1,39 @@ +require 'minitest/autorun' +require_relative '../../lib/text_diff' + +class TextDiffTest < Minitest::Test + def test_groups_nearby_edits_and_preserves_surrounding_text + assert_equal [ + [true, 'E. coli: ', 'E. coli: '], + [false, 'of', 'from'], + [true, ' the colon', ' the colon'] + ], TextDiff.chunks('E. coli: of the colon', 'E. coli: from the colon') + end + + def test_keeps_distant_edits_separate + assert_equal [ + [false, '<', '&'], + [true, ' E. coli ', ' E. coli '], + [false, '>', '!'] + ], TextDiff.chunks('< E. coli >', '& E. coli !') + end + + def test_represents_additions_and_removals_with_an_empty_side + assert_equal [[true, 'E. coli', 'E. coli'], [false, '', ' strain']], + TextDiff.chunks('E. coli', 'E. coli strain') + assert_equal [[true, 'E. coli', 'E. coli'], [false, ' strain', '']], + TextDiff.chunks('E. coli strain', 'E. coli') + end + + def test_handles_empty_and_unchanged_text + assert_equal [], TextDiff.chunks(nil, '') + assert_equal [[true, 'E. coli', 'E. coli']], TextDiff.chunks('E. coli', 'E. coli') + end + + def test_preserves_unicode_graphemes + assert_equal [[true, 'M', 'M'], [false, 'ü', 'ö'], [true, 'ller', 'ller']], + TextDiff.chunks('Müller', 'Möller') + assert_equal [[true, 'E. coli: ', 'E. coli: '], [false, "e\u0301", 'ö']], + TextDiff.chunks("E. coli: e\u0301", 'E. coli: ö') + end +end From 78641e6e128f1bcd46c63f3718269b7752ded372 Mon Sep 17 00:00:00 2001 From: vangberg Date: Fri, 2 Oct 2026 14:30:32 +0200 Subject: [PATCH 07/41] public history: include proposed_in_id --- app/models/name/public_history.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/name/public_history.rb b/app/models/name/public_history.rb index 09ba3392..00bb1040 100644 --- a/app/models/name/public_history.rb +++ b/app/models/name/public_history.rb @@ -3,7 +3,7 @@ class Name::PublicHistory KEYS = %w[ name rank syllabication status priority_date authority nomenclatural_status taxonomic_status proposal_kind corrigendum_from - type_material type_accession + type_material type_accession proposed_in_id ].concat(Name::ETYMOLOGY_COLUMNS).freeze def initialize(name) From 1731cbaf7e0e752b08d85271a063c6b1bdda4463 Mon Sep 17 00:00:00 2001 From: vangberg Date: Fri, 2 Oct 2026 14:34:03 +0200 Subject: [PATCH 08/41] Render publication associations as citations in public history --- app/helpers/names_helper.rb | 29 +++++++++++++---------- app/views/names/history.html.erb | 5 ++-- test/controllers/names/history_test.rb | 32 ++++++++++++++++++++++++++ test/helpers/names_helper_test.rb | 14 +++++++---- 4 files changed, 62 insertions(+), 18 deletions(-) diff --git a/app/helpers/names_helper.rb b/app/helpers/names_helper.rb index def28f36..6754b3a6 100644 --- a/app/helpers/names_helper.rb +++ b/app/helpers/names_helper.rb @@ -1,20 +1,25 @@ module NamesHelper - def history_before_value(changes, attribute:) - before = changes.first - if attribute == 'etymology' - before = history_etymology_parts(*changes).first - end + def history_values(changes, attribute:) + values = + case attribute + when 'etymology' + history_etymology_parts(*changes) + when 'proposed_in_id' + changes.map { |id| history_publication_reference(id) } + else + changes + end - safe_join(Array(before)).presence || '—' + values.map { |value| safe_join(Array(value)).presence || '—' } end - def history_after_value(changes, attribute:) - after = changes.last - if attribute == 'etymology' - after = history_etymology_parts(*changes).last + def history_publication_reference(publication_id) + publication = Publication.find_by(id: publication_id) if publication_id + if publication + link_to(publication.short_citation, publication) + elsif publication_id + "Publication ##{publication_id} (deleted)" end - - safe_join(Array(after)).presence || '—' end def history_etymology_parts(before, after) diff --git a/app/views/names/history.html.erb b/app/views/names/history.html.erb index e7def6e5..fd45adea 100644 --- a/app/views/names/history.html.erb +++ b/app/views/names/history.html.erb @@ -15,12 +15,13 @@
<% @history.changes_for(version).each do |attribute, changes| %> + <% before, after = history_values(changes, attribute: attribute) %>
<%= Name.human_attribute_name(attribute) %>: - Before: <%= history_before_value(changes, attribute: attribute) %> + Before: <%= before %> - After: <%= history_after_value(changes, attribute: attribute) %> + After: <%= after %>
<% end %> diff --git a/test/controllers/names/history_test.rb b/test/controllers/names/history_test.rb index 4fbfcad2..9e5fa414 100644 --- a/test/controllers/names/history_test.rb +++ b/test/controllers/names/history_test.rb @@ -28,4 +28,36 @@ class NamesHistoryTest < ActionDispatch::IntegrationTest assert_select 'h3', text: 'Insufficient permits' assert_select '.name-history-version', count: 0 end + + test 'renders proposed publication changes as citations with links' do + name = names(:escherichia_coli) + first = publications(:one) + second = publications(:two) + name.update!(proposed_in: first) + name.update!(proposed_in: second) + name.update!(proposed_in: nil) + + get history_name_path(name) + + assert_response :success + assert_select '.name-history-version', count: 3 + assert_select '.name-history-before a[href=?]', publication_path(first), text: first.short_citation + assert_select '.name-history-after a[href=?]', publication_path(first), text: first.short_citation + assert_select '.name-history-before a[href=?]', publication_path(second), text: second.short_citation + assert_select '.name-history-after a[href=?]', publication_path(second), text: second.short_citation + assert_select '.name-history-before', text: 'Before: —', count: 1 + assert_select '.name-history-after', text: 'After: —', count: 1 + end + + test 'renders a retained publication id when the publication no longer exists' do + name = names(:escherichia_coli) + publication = publications(:no_doi) + name.update!(proposed_in: publication) + publication.destroy! + + get history_name_path(name) + + assert_response :success + assert_select '.name-history-after', text: "After: Publication ##{publication.id} (deleted)" + end end diff --git a/test/helpers/names_helper_test.rb b/test/helpers/names_helper_test.rb index 924c4520..e8613289 100644 --- a/test/helpers/names_helper_test.rb +++ b/test/helpers/names_helper_test.rb @@ -6,8 +6,7 @@ class NamesHelperTest < ActionView::TestCase { name: 'E. coli', etymology_xx_description: 'of the colon' }, { name: 'E. coli', etymology_xx_description: 'from the colon' } ] - before = history_before_value(changes, attribute: 'etymology') - after = history_after_value(changes, attribute: 'etymology') + before, after = history_values(changes, attribute: 'etymology') assert_equal 'coli, of the colon', before assert_equal 'coli, from the colon', after @@ -18,11 +17,18 @@ class NamesHelperTest < ActionView::TestCase { name: 'E. coli', etymology_xx_description: '< E. coli >' }, { name: 'E. coli', etymology_xx_description: '& E. coli !' } ] - before = history_before_value(changes, attribute: 'etymology') - after = history_after_value(changes, attribute: 'etymology') + before, after = history_values(changes, attribute: 'etymology') assert_equal 'coli, < E. coli >', before assert_equal 'coli, & E. coli !', after end + test 'renders proposed publication changes as citations' do + publication = publications(:one) + citation = link_to(publication.short_citation, publication) + assert_equal [citation, '—'], + history_values([publication.id, nil], attribute: 'proposed_in_id') + assert_equal ['—', citation], + history_values([nil, publication.id], attribute: 'proposed_in_id') + end end From 0ea55ca64700c2f2ac7e1a2d63b73a153d6e5410 Mon Sep 17 00:00:00 2001 From: vangberg Date: Fri, 2 Oct 2026 14:43:28 +0200 Subject: [PATCH 09/41] Highlight string changes in public name history --- app/helpers/names_helper.rb | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/app/helpers/names_helper.rb b/app/helpers/names_helper.rb index 6754b3a6..bd15b72a 100644 --- a/app/helpers/names_helper.rb +++ b/app/helpers/names_helper.rb @@ -3,11 +3,11 @@ def history_values(changes, attribute:) values = case attribute when 'etymology' - history_etymology_parts(*changes) + history_diff_parts(*changes.map { |attributes| Name.new(attributes).full_etymology }) when 'proposed_in_id' changes.map { |id| history_publication_reference(id) } else - changes + changes.any? { |value| value.is_a?(String) } ? history_diff_parts(*changes) : changes end values.map { |value| safe_join(Array(value)).presence || '—' } @@ -22,10 +22,7 @@ def history_publication_reference(publication_id) end end - def history_etymology_parts(before, after) - before = Name.new(before).full_etymology - after = Name.new(after).full_etymology - + def history_diff_parts(before, after) chunks = TextDiff.chunks(before, after) before_parts = chunks.filter_map do |unchanged, text, _| From 415e755c5ec0f3471d877f71188e866fa17ba869 Mon Sep 17 00:00:00 2001 From: vangberg Date: Fri, 2 Oct 2026 14:46:09 +0200 Subject: [PATCH 10/41] Show status names in public name history --- app/helpers/names_helper.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/helpers/names_helper.rb b/app/helpers/names_helper.rb index bd15b72a..fba9f636 100644 --- a/app/helpers/names_helper.rb +++ b/app/helpers/names_helper.rb @@ -6,6 +6,8 @@ def history_values(changes, attribute:) history_diff_parts(*changes.map { |attributes| Name.new(attributes).full_etymology }) when 'proposed_in_id' changes.map { |id| history_publication_reference(id) } + when 'status' + changes.map { |code| Name.status_hash.dig(code, :name) || code } else changes.any? { |value| value.is_a?(String) } ? history_diff_parts(*changes) : changes end From 10e4299733f088b19959fc23a3f439fa1487b52a Mon Sep 17 00:00:00 2001 From: vangberg Date: Fri, 2 Oct 2026 14:52:01 +0200 Subject: [PATCH 11/41] Render public name history as a table --- app/assets/stylesheets/names.scss | 66 ++++++++------------------ app/views/names/history.html.erb | 51 ++++++++++++-------- test/controllers/names/history_test.rb | 6 +-- 3 files changed, 52 insertions(+), 71 deletions(-) diff --git a/app/assets/stylesheets/names.scss b/app/assets/stylesheets/names.scss index c88ec0d7..e0c46162 100644 --- a/app/assets/stylesheets/names.scss +++ b/app/assets/stylesheets/names.scss @@ -16,64 +16,36 @@ } } -.name-history-version { - display: grid; - grid-template-columns: 11rem minmax(0, 1fr); - gap: 1rem; - padding: 0.65rem 0.75rem; +.name-history-table { + table-layout: fixed; + min-width: 42rem; + + thead th:first-child { + width: 12rem; + } + + thead th:nth-child(2) { + width: 10rem; + } .name-history-date { color: #6c757d; font-size: 0.875rem; + font-weight: normal; white-space: nowrap; } -} -.name-history-change { - display: grid; - grid-template-columns: 10rem minmax(0, 1fr); - gap: 0.5rem; - padding: 0.3rem 0.5rem; - - &:nth-child(even) { + .name-history-version:nth-of-type(even) { background-color: #f7f8fa; } -} - -.name-history-values { - display: grid; - grid-template-columns: minmax(0, 1fr) 2rem minmax(0, 1fr); - align-items: center; - min-width: 0; - overflow-wrap: anywhere; -} -.name-history-before, -.name-history-after { - white-space: pre-wrap; -} - -.name-history-arrow { - color: #6c757d; - text-align: center; -} - -@media (max-width: 767px) { - .name-history-version { - display: block; - - .name-history-date { - display: block; - margin-bottom: 0.2rem; - } + .name-history-version + .name-history-version { + border-top: 0.75rem solid var(--white); } - .name-history-change { - display: block; - - > strong { - display: block; - margin-bottom: 0.15rem; - } + .name-history-before, + .name-history-after { + white-space: pre-wrap; + overflow-wrap: anywhere; } } diff --git a/app/views/names/history.html.erb b/app/views/names/history.html.erb index fd45adea..23020cb6 100644 --- a/app/views/names/history.html.erb +++ b/app/views/names/history.html.erb @@ -6,28 +6,37 @@ <% if @versions.empty? %>

No history has been recorded for this name.

<% else %> -
- <% @versions.each do |version| %> -
- - -
- <% @history.changes_for(version).each do |attribute, changes| %> - <% before, after = history_values(changes, attribute: attribute) %> -
- <%= Name.human_attribute_name(attribute) %>: - - Before: <%= before %> - - After: <%= after %> - -
+
+ + + + + + + + + + <% @versions.each do |version| %> + <% changes = @history.changes_for(version) %> + + <% changes.each_with_index do |(attribute, values), index| %> + <% before, after = history_values(values, attribute: attribute) %> + + <% if index.zero? %> + + <% end %> + + + + <% end %> - - - <% end %> + + <% end %> +
DateAttributeBeforeAfter
+ + <%= Name.human_attribute_name(attribute) %><%= before %><%= after %>
<% end %> diff --git a/test/controllers/names/history_test.rb b/test/controllers/names/history_test.rb index 9e5fa414..f2a81998 100644 --- a/test/controllers/names/history_test.rb +++ b/test/controllers/names/history_test.rb @@ -45,8 +45,8 @@ class NamesHistoryTest < ActionDispatch::IntegrationTest assert_select '.name-history-after a[href=?]', publication_path(first), text: first.short_citation assert_select '.name-history-before a[href=?]', publication_path(second), text: second.short_citation assert_select '.name-history-after a[href=?]', publication_path(second), text: second.short_citation - assert_select '.name-history-before', text: 'Before: —', count: 1 - assert_select '.name-history-after', text: 'After: —', count: 1 + assert_select '.name-history-before', text: '—', count: 1 + assert_select '.name-history-after', text: '—', count: 1 end test 'renders a retained publication id when the publication no longer exists' do @@ -58,6 +58,6 @@ class NamesHistoryTest < ActionDispatch::IntegrationTest get history_name_path(name) assert_response :success - assert_select '.name-history-after', text: "After: Publication ##{publication.id} (deleted)" + assert_select '.name-history-after', text: "Publication ##{publication.id} (deleted)" end end From b50742627cf57fa9bb8652f9e332abeb71d91ec1 Mon Sep 17 00:00:00 2001 From: vangberg Date: Wed, 7 Oct 2026 17:01:08 +0200 Subject: [PATCH 12/41] Test public name history at the model layer --- test/controllers/names/history_test.rb | 45 +++---------------------- test/helpers/names_helper_test.rb | 7 ++++ test/models/name/public_history_test.rb | 31 +++++++++++++++++ 3 files changed, 42 insertions(+), 41 deletions(-) create mode 100644 test/models/name/public_history_test.rb diff --git a/test/controllers/names/history_test.rb b/test/controllers/names/history_test.rb index f2a81998..a8949e5b 100644 --- a/test/controllers/names/history_test.rb +++ b/test/controllers/names/history_test.rb @@ -1,7 +1,7 @@ require 'test_helper' class NamesHistoryTest < ActionDispatch::IntegrationTest - test 'shows public name history' do + test 'renders public history' do name = names(:escherichia_coli) name.update!(authority: 'Smith', etymology_xx_description: 'of the colon') @@ -12,15 +12,10 @@ class NamesHistoryTest < ActionDispatch::IntegrationTest assert_select '.name-history-change', count: 2 end - test 'shows an empty state without recorded history' do - get history_name_path(names(:escherichia_coli)) - - assert_response :success - assert_select 'p', text: 'No history has been recorded for this name.' - end - test 'does not reveal private name history' do - name = names(:draft_by_contributor) + name = Name.create!( + name: 'E. coli', rank: 'species', status: 5, created_by: users(:contributor) + ) name.update!(syllabication: 'co.li') get history_name_path(name) @@ -28,36 +23,4 @@ class NamesHistoryTest < ActionDispatch::IntegrationTest assert_select 'h3', text: 'Insufficient permits' assert_select '.name-history-version', count: 0 end - - test 'renders proposed publication changes as citations with links' do - name = names(:escherichia_coli) - first = publications(:one) - second = publications(:two) - name.update!(proposed_in: first) - name.update!(proposed_in: second) - name.update!(proposed_in: nil) - - get history_name_path(name) - - assert_response :success - assert_select '.name-history-version', count: 3 - assert_select '.name-history-before a[href=?]', publication_path(first), text: first.short_citation - assert_select '.name-history-after a[href=?]', publication_path(first), text: first.short_citation - assert_select '.name-history-before a[href=?]', publication_path(second), text: second.short_citation - assert_select '.name-history-after a[href=?]', publication_path(second), text: second.short_citation - assert_select '.name-history-before', text: '—', count: 1 - assert_select '.name-history-after', text: '—', count: 1 - end - - test 'renders a retained publication id when the publication no longer exists' do - name = names(:escherichia_coli) - publication = publications(:no_doi) - name.update!(proposed_in: publication) - publication.destroy! - - get history_name_path(name) - - assert_response :success - assert_select '.name-history-after', text: "Publication ##{publication.id} (deleted)" - end end diff --git a/test/helpers/names_helper_test.rb b/test/helpers/names_helper_test.rb index e8613289..699ab95a 100644 --- a/test/helpers/names_helper_test.rb +++ b/test/helpers/names_helper_test.rb @@ -31,4 +31,11 @@ class NamesHelperTest < ActionView::TestCase assert_equal ['—', citation], history_values([nil, publication.id], attribute: 'proposed_in_id') end + test 'renders a retained publication id when the publication no longer exists' do + publication = publications(:no_doi) + publication.destroy! + + assert_equal ['—', "Publication ##{publication.id} (deleted)"], + history_values([nil, publication.id], attribute: 'proposed_in_id') + end end diff --git a/test/models/name/public_history_test.rb b/test/models/name/public_history_test.rb new file mode 100644 index 00000000..544948f4 --- /dev/null +++ b/test/models/name/public_history_test.rb @@ -0,0 +1,31 @@ +require 'test_helper' + +class Name::PublicHistoryTest < ActiveSupport::TestCase + setup do + @name = names(:escherichia_coli) + @history = Name::PublicHistory.new(@name) + end + + test 'keeps public changes and groups etymology snapshots without unchanged companions' do + version = Version.new(record_type: 'Name', changeset: { + 'name' => ['E. coli', 'E. coli'], + 'authority' => [nil, 'Smith'], + 'proposed_in_id' => [nil, publications(:one).id], + 'gtdb_accession' => [nil, 'GB_GCA_000005845.2'], + 'etymology_xx_lang' => ['N.L.', 'N.L.'], + 'etymology_xx_description' => ['of the colon', 'from the colon'] + }) + + assert_equal({ + 'authority' => [nil, 'Smith'], + 'proposed_in_id' => [nil, publications(:one).id], + 'etymology' => [ + { 'name' => 'E. coli', 'etymology_xx_lang' => 'N.L.', + 'etymology_xx_description' => 'of the colon' }, + { 'name' => 'E. coli', 'etymology_xx_lang' => 'N.L.', + 'etymology_xx_description' => 'from the colon' } + ] + }, @history.changes_for(version)) + end + +end From d80ea8d7d47171c7eda8e136a341219c99dfae09 Mon Sep 17 00:00:00 2001 From: vangberg Date: Fri, 2 Oct 2026 10:30:27 +0200 Subject: [PATCH 13/41] Track history for PublicationName --- app/assets/stylesheets/names.scss | 4 + app/controllers/concerns/filterable.rb | 2 +- .../publication_names_controller.rb | 4 +- app/helpers/names_helper.rb | 42 +++++++-- app/models/concerns/unlinkable.rb | 15 ++++ app/models/name.rb | 10 ++- app/models/name/public_history.rb | 57 +++++++++--- app/models/publication.rb | 3 +- app/models/publication_name.rb | 3 + app/views/names/history.html.erb | 19 +++- app/views/publications/_names.html.erb | 2 +- ...00_add_unlinked_at_to_publication_names.rb | 8 ++ db/schema.rb | 5 +- test/controllers/names/history_test.rb | 5 +- test/helpers/names_helper_test.rb | 39 ++++++++- test/models/name/public_history_test.rb | 87 +++++++++++++++++++ 16 files changed, 268 insertions(+), 37 deletions(-) create mode 100644 app/models/concerns/unlinkable.rb create mode 100644 db/migrate/20261002100000_add_unlinked_at_to_publication_names.rb diff --git a/app/assets/stylesheets/names.scss b/app/assets/stylesheets/names.scss index e0c46162..620fcf95 100644 --- a/app/assets/stylesheets/names.scss +++ b/app/assets/stylesheets/names.scss @@ -43,6 +43,10 @@ border-top: 0.75rem solid var(--white); } + .name-history-record ~ .name-history-change th { + padding-left: 1.5rem; + } + .name-history-before, .name-history-after { white-space: pre-wrap; diff --git a/app/controllers/concerns/filterable.rb b/app/controllers/concerns/filterable.rb index 4d3bc2ff..8c372865 100644 --- a/app/controllers/concerns/filterable.rb +++ b/app/controllers/concerns/filterable.rb @@ -31,7 +31,7 @@ def apply_sort(query, sort_by: nil, sort_direction: nil) # Handle special sorting cases (e.g., 'citations' for Name model) case sort_by.to_s.downcase when 'citations' - query.left_joins(:publication_names).group(:id).order("COUNT(publication_names.id) #{sort_direction.upcase}") + query.left_joins(:publication_names).group(:id).order("COUNT(CASE WHEN publication_names.unlinked_at IS NULL THEN publication_names.id END) #{sort_direction.upcase}") when 'date' # Use validated_at if available, otherwise fall back to created_at if query.model.column_names.include?('validated_at') diff --git a/app/controllers/publication_names_controller.rb b/app/controllers/publication_names_controller.rb index 73808d7f..19132db7 100644 --- a/app/controllers/publication_names_controller.rb +++ b/app/controllers/publication_names_controller.rb @@ -19,7 +19,7 @@ def destroy if name.assigned_in? publication name.update(assigned_in: nil) end - @publication_name.destroy + @publication_name.unlink! end redirect_to( @@ -60,7 +60,7 @@ def link_name_commit private # Use callbacks to share common setup or constraints between actions. def set_publication_name - @publication_name = PublicationName.find(params[:id]) + @publication_name = PublicationName.linked.find(params[:id]) end def set_publication diff --git a/app/helpers/names_helper.rb b/app/helpers/names_helper.rb index fba9f636..b7aaaa24 100644 --- a/app/helpers/names_helper.rb +++ b/app/helpers/names_helper.rb @@ -1,15 +1,13 @@ module NamesHelper - def history_values(changes, attribute:) + def history_values(changes, attribute:, record_type: 'Name') values = - case attribute - when 'etymology' - history_diff_parts(*changes.map { |attributes| Name.new(attributes).full_etymology }) - when 'proposed_in_id' - changes.map { |id| history_publication_reference(id) } - when 'status' - changes.map { |code| Name.status_hash.dig(code, :name) || code } + case record_type + when 'PublicationName' + history_publication_name_values(changes) + when 'Name' + history_name_values(changes, attribute) else - changes.any? { |value| value.is_a?(String) } ? history_diff_parts(*changes) : changes + raise ArgumentError, "Unsupported public history record type: #{record_type}" end values.map { |value| safe_join(Array(value)).presence || '—' } @@ -109,4 +107,30 @@ def name_lineage(name, links: true, last: true, register: nil, visited: []) private + def history_name_values(changes, attribute) + case attribute + when 'etymology' + history_diff_parts(*changes.map { |attributes| Name.new(attributes).full_etymology }) + when 'proposed_in_id' + changes.map { |id| history_publication_reference(id) } + when 'status' + changes.map { |code| Name.status_hash.dig(code, :name) || code } + else + history_text_values(changes) + end + end + + def history_publication_name_values(changes) + history_text_values(changes).map do |value| + case value + when true then 'Yes' + when false then 'No' + else value + end + end + end + + def history_text_values(changes) + changes.any? { |value| value.is_a?(String) } ? history_diff_parts(*changes) : changes + end end diff --git a/app/models/concerns/unlinkable.rb b/app/models/concerns/unlinkable.rb new file mode 100644 index 00000000..58e98566 --- /dev/null +++ b/app/models/concerns/unlinkable.rb @@ -0,0 +1,15 @@ +module Unlinkable + extend ActiveSupport::Concern + + included do + scope :linked, -> { where(unlinked_at: nil) } + end + + def unlink! + update!(unlinked_at: Time.current) + end + + def unlinked? + unlinked_at.present? + end +end diff --git a/app/models/name.rb b/app/models/name.rb index c6a240ff..103f5bba 100644 --- a/app/models/name.rb +++ b/app/models/name.rb @@ -1,7 +1,9 @@ class Name < ApplicationRecord has_many(:pseudonyms, dependent: :destroy) + # A deleted name no longer needs its publication links, including unlinked ones. has_many(:publication_names, dependent: :destroy) - has_many(:publications, through: :publication_names) + has_many(:publications, -> { merge(PublicationName.linked) }, + through: :publication_names) has_many( :name_correspondences, -> { order(:created_at) }, dependent: :destroy ) @@ -663,7 +665,7 @@ def hard_to_pronounce? end def publication_names_ordered - publication_names.left_joins(:publication).order(journal_date: :desc) + publication_names.linked.left_joins(:publication).order(journal_date: :desc) end def citations @@ -827,7 +829,7 @@ def corrigendum_source end def emended_in - publication_names.where(emends: true).map(&:publication) + publication_names.linked.where(emends: true).map(&:publication) end def emended_in?(publication) @@ -835,7 +837,7 @@ def emended_in?(publication) end def not_validly_proposed_in - publication_names.where(not_valid_proposal: true).map(&:publication) + publication_names.linked.where(not_valid_proposal: true).map(&:publication) end def not_validly_proposed_in?(publication) diff --git a/app/models/name/public_history.rb b/app/models/name/public_history.rb index 00bb1040..1c8b7e0e 100644 --- a/app/models/name/public_history.rb +++ b/app/models/name/public_history.rb @@ -10,21 +10,39 @@ def initialize(name) @name = name end - # Filter before paginating so each page contains public updates only. + # Load both histories and filter displayable changes in Ruby before pagination. def versions(page:) - @name.versions - .where(operation: 'update') - .where( - 'EXISTS (SELECT 1 FROM jsonb_object_keys(versions.changeset) AS key WHERE key IN (?))', - KEYS - ) - .order(created_at: :desc, id: :desc) - .paginate(page: page, per_page: 25) + name_versions.or(publication_versions) + .order(created_at: :desc, id: :desc) + .select { |version| changes_for(version).any? } + .paginate(page: page, per_page: 25) + end + + def changes_for(version) + case version.record_type + when 'Name' + name_changes_for(version) + when 'PublicationName' + publication_changes_for(version) + else + raise ArgumentError, "Unsupported public history record type: #{version.record_type}" + end + end + + def publication_id_for(version) + @publication_ids ||= @name.publication_names.pluck(:id, :publication_id).to_h + @publication_ids.fetch(version.record_id) + end + + private + + def name_versions + @name.versions.where(operation: 'update') end # Ordinary fields keep their [before, after] pairs. Etymology fields become # one pair of attribute snapshots for the view to render together. - def changes_for(version) + def name_changes_for(version) changes = version.changeset.slice(*KEYS) etymology = changes.extract!(*Name::ETYMOLOGY_COLUMNS) if etymology.any? @@ -35,7 +53,24 @@ def changes_for(version) changes end - private + def publication_versions + Version.where( + record_type: 'PublicationName', record_id: @name.publication_names.select(:id) + ) + end + + def publication_changes_for(version) + return { 'linked' => [false, true] } if version.operation == 'create' + return {} unless version.operation == 'update' + + changes = version.changeset.dup + if unlinked_at = changes.delete('unlinked_at') + # A nil unlinked_at means the publication is linked. + linked = unlinked_at.map(&:nil?) + changes = { 'linked' => linked }.merge(changes) unless linked.first == linked.last + end + changes + end # Include the name because full_etymology derives the :xx particle from its # last word. Older versions without a name pair use the current name. diff --git a/app/models/publication.rb b/app/models/publication.rb index 5ab374dc..1295cde4 100644 --- a/app/models/publication.rb +++ b/app/models/publication.rb @@ -21,7 +21,8 @@ class Publication < ApplicationRecord has_many(:publication_names, dependent: :destroy) has_many(:authors, through: :publication_authors) has_many(:subjects, through: :publication_subjects) - has_many(:names, through: :publication_names) + has_many(:names, -> { merge(PublicationName.linked) }, + through: :publication_names) has_many(:registers) has_many( :proposed_names, class_name: 'Name', diff --git a/app/models/publication_name.rb b/app/models/publication_name.rb index 4d49c06d..7818ccba 100644 --- a/app/models/publication_name.rb +++ b/app/models/publication_name.rb @@ -1,4 +1,7 @@ class PublicationName < ApplicationRecord + include Unlinkable + include Versioned + belongs_to(:publication) belongs_to(:name) diff --git a/app/views/names/history.html.erb b/app/views/names/history.html.erb index 23020cb6..94d804e2 100644 --- a/app/views/names/history.html.erb +++ b/app/views/names/history.html.erb @@ -18,18 +18,31 @@ <% @versions.each do |version| %> <% changes = @history.changes_for(version) %> + <% publication_change = version.record_type == 'PublicationName' %> + <% if publication_change %> + + + + + + Publication: <%= history_publication_reference(@history.publication_id_for(version)) %> + + + <% end %> <% changes.each_with_index do |(attribute, values), index| %> - <% before, after = history_values(values, attribute: attribute) %> + <% before, after = history_values(values, attribute: attribute, record_type: version.record_type) %> - <% if index.zero? %> + <% if index.zero? && !publication_change %> <% end %> - <%= Name.human_attribute_name(attribute) %> + <%= (publication_change ? PublicationName : Name).human_attribute_name(attribute) %> <%= before %> <%= after %> diff --git a/app/views/publications/_names.html.erb b/app/views/publications/_names.html.erb index ea6e29be..6ce92fad 100644 --- a/app/views/publications/_names.html.erb +++ b/app/views/publications/_names.html.erb @@ -1,4 +1,4 @@ -<% publication.publication_names.each do |publication_name| %> +<% publication.publication_names.linked.each do |publication_name| %> <% name = publication_name.name %> <%= link_to( name.abbr_name, name, class: 'badge badge-pill badge-primary') %> diff --git a/db/migrate/20261002100000_add_unlinked_at_to_publication_names.rb b/db/migrate/20261002100000_add_unlinked_at_to_publication_names.rb new file mode 100644 index 00000000..a47a6af9 --- /dev/null +++ b/db/migrate/20261002100000_add_unlinked_at_to_publication_names.rb @@ -0,0 +1,8 @@ +class AddUnlinkedAtToPublicationNames < ActiveRecord::Migration[6.1] + def change + add_column :publication_names, :unlinked_at, :datetime + remove_index :publication_names, column: %i[publication_id name_id], unique: true + add_index :publication_names, %i[publication_id name_id], + unique: true, where: 'unlinked_at IS NULL' + end +end diff --git a/db/schema.rb b/db/schema.rb index e166d71f..3dd352c0 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 2026_09_17_100000) do +ActiveRecord::Schema.define(version: 2026_10_02_100000) do # These are extensions that must be enabled in order to support this database enable_extension "fuzzystrmatch" @@ -367,8 +367,9 @@ t.datetime "updated_at", null: false t.boolean "emends", default: false t.boolean "not_valid_proposal", default: false + t.datetime "unlinked_at" t.index ["name_id"], name: "index_publication_names_on_name_id" - t.index ["publication_id", "name_id"], name: "index_publication_names_on_publication_id_and_name_id", unique: true + t.index ["publication_id", "name_id"], name: "index_publication_names_on_publication_id_and_name_id", unique: true, where: "(unlinked_at IS NULL)" t.index ["publication_id"], name: "index_publication_names_on_publication_id" end diff --git a/test/controllers/names/history_test.rb b/test/controllers/names/history_test.rb index a8949e5b..c9179177 100644 --- a/test/controllers/names/history_test.rb +++ b/test/controllers/names/history_test.rb @@ -4,12 +4,12 @@ class NamesHistoryTest < ActionDispatch::IntegrationTest test 'renders public history' do name = names(:escherichia_coli) name.update!(authority: 'Smith', etymology_xx_description: 'of the colon') + PublicationName.create!(name: name, publication: publications(:one)) get history_name_path(name) assert_response :success - assert_select '.name-history-version', count: 1 - assert_select '.name-history-change', count: 2 + assert_select '.name-history-version', count: 2 end test 'does not reveal private name history' do @@ -17,6 +17,7 @@ class NamesHistoryTest < ActionDispatch::IntegrationTest name: 'E. coli', rank: 'species', status: 5, created_by: users(:contributor) ) name.update!(syllabication: 'co.li') + PublicationName.create!(name: name, publication: publications(:one)).unlink! get history_name_path(name) diff --git a/test/helpers/names_helper_test.rb b/test/helpers/names_helper_test.rb index 699ab95a..bf2b9aa0 100644 --- a/test/helpers/names_helper_test.rb +++ b/test/helpers/names_helper_test.rb @@ -23,7 +23,7 @@ class NamesHelperTest < ActionView::TestCase assert_equal 'coli, & E. coli !', after end - test 'renders proposed publication changes as citations' do + test 'renders publication changes as citations' do publication = publications(:one) citation = link_to(publication.short_citation, publication) assert_equal [citation, '—'], @@ -31,6 +31,35 @@ class NamesHelperTest < ActionView::TestCase assert_equal ['—', citation], history_values([nil, publication.id], attribute: 'proposed_in_id') end + + test 'diffs text additions and removals in either record context' do + %w[Name PublicationName].each do |record_type| + assert_equal ['—', 'Smith & Jones'], + history_values([nil, 'Smith & Jones'], attribute: 'authority', record_type: record_type) + assert_equal ['Smith & Jones', '—'], + history_values(['Smith & Jones', nil], attribute: 'authority', record_type: record_type) + end + end + + test 'formats name status labels without diffing them' do + assert_equal ['Draft', 'Submitted'], history_values([5, 10], attribute: 'status') + assert_equal ['—', '99'], history_values([nil, 99], attribute: 'status') + assert_equal ['5', '10'], history_values( + [5, 10], attribute: 'status', record_type: 'PublicationName' + ) + end + + test 'renders publication relationship values in their own record context' do + %w[linked emends not_valid_proposal].each do |attribute| + assert_equal ['No', 'Yes'], history_values( + [false, true], attribute: attribute, record_type: 'PublicationName' + ) + assert_equal ['Yes', 'No'], history_values( + [true, false], attribute: attribute, record_type: 'PublicationName' + ) + end + end + test 'renders a retained publication id when the publication no longer exists' do publication = publications(:no_doi) publication.destroy! @@ -38,4 +67,12 @@ class NamesHelperTest < ActionView::TestCase assert_equal ['—', "Publication ##{publication.id} (deleted)"], history_values([nil, publication.id], attribute: 'proposed_in_id') end + + test 'preserves nonboolean publication relationship values' do + ids = [publications(:one).id, publications(:no_doi).id] + + assert_equal ids.map(&:to_s), history_values( + ids, attribute: 'publication_id', record_type: 'PublicationName' + ) + end end diff --git a/test/models/name/public_history_test.rb b/test/models/name/public_history_test.rb index 544948f4..c36211f1 100644 --- a/test/models/name/public_history_test.rb +++ b/test/models/name/public_history_test.rb @@ -28,4 +28,91 @@ class Name::PublicHistoryTest < ActiveSupport::TestCase }, @history.changes_for(version)) end + test 'combines publication links unlinks and relinks with name updates newest first' do + publication = publications(:one) + start = Time.zone.local(2026, 10, 1, 12) + link = nil + travel_to(start) do + link = PublicationName.create!(name: @name, publication: publication) + end + travel_to(start + 1.minute) { @name.update!(authority: 'Smith') } + travel_to(start + 2.minutes) { link.unlink! } + travel_to(start + 3.minutes) { link.update!(unlinked_at: nil) } + + versions = @history.versions(page: 1) + + assert_equal [ + { 'linked' => [false, true] }, + { 'linked' => [true, false] }, + { 'authority' => [nil, 'Smith'] }, + { 'linked' => [false, true] } + ], versions.map { |version| @history.changes_for(version) } + assert_equal [3, 2, 1, 0].map { |minutes| start + minutes.minutes }, + versions.map(&:created_at) + end + + test 'filters nonpublic changes before paginating the combined history' do + link = PublicationName.create!(name: @name, publication: publications(:one)) + linked_version = link.versions.last + link.unlink! + unlinked_version = link.versions.last + 24.times { |index| @name.update!(authority: "Smith #{index}") } + @name.update!(gtdb_accession: 'GB_GCA_000005845.2') + link.update!(emends: true) + + first_page = @history.versions(page: 1) + second_page = @history.versions(page: 2) + + assert_equal 25, first_page.size + assert_equal({ 'emends' => [false, true] }, + @history.changes_for(first_page.first)) + assert_equal [unlinked_version, linked_version], second_page + end + + test 'identifies the publication for each relationship history including unlinks' do + first_link = PublicationName.create!(name: @name, publication: publications(:one)) + second_link = PublicationName.create!(name: @name, publication: publications(:no_doi)) + first_link.unlink! + + first_link.versions.each do |version| + assert_equal publications(:one).id, @history.publication_id_for(version) + end + assert_equal publications(:no_doi).id, @history.publication_id_for(second_link.versions.last) + end + + test 'shows publication role additions and removals alongside unlink changes' do + link = PublicationName.create!(name: @name, publication: publications(:one)) + link.update!(emends: true, not_valid_proposal: true) + + assert_equal({ + 'emends' => [false, true], + 'not_valid_proposal' => [false, true] + }, @history.changes_for(link.versions.last)) + + link.update!(emends: false, not_valid_proposal: false, unlinked_at: Time.current) + + assert_equal({ + 'linked' => [true, false], + 'emends' => [true, false], + 'not_valid_proposal' => [true, false] + }, @history.changes_for(link.versions.last)) + end + + test 'omits timestamp-only unlink changes while retaining role changes' do + version = Version.new(record_type: 'PublicationName', operation: 'update', changeset: { + 'unlinked_at' => ['2026-10-01T12:00:00Z', '2026-10-01T12:01:00Z'], + 'emends' => [false, true] + }) + + assert_equal({ 'emends' => [false, true] }, @history.changes_for(version)) + end + + test 'includes publication relationship changes without an attribute allowlist' do + values = [publications(:one).id, publications(:no_doi).id] + version = Version.new(record_type: 'PublicationName', operation: 'update', changeset: { + 'publication_id' => values + }) + + assert_equal({ 'publication_id' => values }, @history.changes_for(version)) + end end From c43a874073e8101cd05c6322b189f5e822f3985b Mon Sep 17 00:00:00 2001 From: vangberg Date: Thu, 8 Oct 2026 09:20:38 +0200 Subject: [PATCH 14/41] Simplify publication history changes with guard clauses --- app/models/name/public_history.rb | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/app/models/name/public_history.rb b/app/models/name/public_history.rb index 1c8b7e0e..67806069 100644 --- a/app/models/name/public_history.rb +++ b/app/models/name/public_history.rb @@ -64,12 +64,14 @@ def publication_changes_for(version) return {} unless version.operation == 'update' changes = version.changeset.dup - if unlinked_at = changes.delete('unlinked_at') - # A nil unlinked_at means the publication is linked. - linked = unlinked_at.map(&:nil?) - changes = { 'linked' => linked }.merge(changes) unless linked.first == linked.last - end - changes + unlinked_at = changes.delete('unlinked_at') + return changes unless unlinked_at + + # A nil unlinked_at means the publication is linked. + linked = unlinked_at.map(&:nil?) + return changes if linked.first == linked.last + + { 'linked' => linked }.merge(changes) end # Include the name because full_etymology derives the :xx particle from its From 77155f24acf9f4d3584b9c49c2030c5940fad779 Mon Sep 17 00:00:00 2001 From: vangberg Date: Tue, 20 Oct 2026 12:52:59 +0200 Subject: [PATCH 15/41] Snapshot all attributes in create and delete versions --- app/models/concerns/versioned.rb | 16 +++++++++++----- test/models/name/versioned_test.rb | 11 +++++++---- 2 files changed, 18 insertions(+), 9 deletions(-) diff --git a/app/models/concerns/versioned.rb b/app/models/concerns/versioned.rb index d0a8ed37..bdad1136 100644 --- a/app/models/concerns/versioned.rb +++ b/app/models/concerns/versioned.rb @@ -20,7 +20,7 @@ def versioned_together(*attributes) private def record_create_version - versions.create!(operation: 'create', changeset: {}) + versions.create!(operation: 'create', changeset: attributes.transform_values { |value| [nil, value] }) end def record_update_version @@ -30,13 +30,19 @@ def record_update_version def record_delete_version # The owner is already destroyed, so create directly rather than through - # its association. Deletion records only the event and retains history. - Version.create!(record: self, operation: 'delete', changeset: {}) + # its association. Retain the final attributes and history. + Version.create!( + record: self, operation: 'delete', + changeset: attributes.transform_values { |value| [value, nil] } + ) end - # An update changeset maps each changed attribute to [before, after], e.g. + # A changeset maps each changed attribute to [before, after], e.g. # { 'syllabication' => [nil, 'co.li'] } for E. coli. - # Create and delete versions have an empty changeset; timestamps are omitted. + # Create and delete versions change every attribute from or to nothing, + # retaining the full attributes: { 'rank' => [nil, 'species'] } on create and + # { 'rank' => ['species', nil] } on delete. + # Only update changesets omit timestamps. def versioned_changes changeset = saved_changes.except('created_at', 'updated_at') diff --git a/test/models/name/versioned_test.rb b/test/models/name/versioned_test.rb index f1c18178..c6361b49 100644 --- a/test/models/name/versioned_test.rb +++ b/test/models/name/versioned_test.rb @@ -5,14 +5,15 @@ class Name::VersionedTest < ActiveSupport::TestCase @name = Name.new(name: 'E. coli', rank: 'species') end - test 'creating a name records an empty changeset' do + test 'creating a name changes all attributes from nothing' do assert_difference -> { @name.versions.count } do @name.save! end version = @name.versions.last assert_equal 'create', version.operation - assert_empty version.changeset + assert_equal @name.attributes.as_json.transform_values { |value| [nil, value] }, + version.changeset end test 'updating a name records only changed attributes' do @@ -59,8 +60,10 @@ class Name::VersionedTest < ActiveSupport::TestCase end end - test 'deleting a name records an empty changeset' do + test 'deleting a name changes all final attributes to nothing' do @name.save! + @name.update!(syllabication: 'co.li') + attributes = @name.attributes.as_json versions = Version.where(record: @name) assert_difference -> { versions.count } do @@ -69,7 +72,7 @@ class Name::VersionedTest < ActiveSupport::TestCase version = versions.last assert_equal 'delete', version.operation - assert_empty version.changeset + assert_equal attributes.transform_values { |value| [value, nil] }, version.changeset end test 'deleting a name retains its history' do From 865123f0179848702a4b5e6c905e6d12d19cd1f6 Mon Sep 17 00:00:00 2001 From: vangberg Date: Tue, 20 Oct 2026 12:55:47 +0200 Subject: [PATCH 16/41] Rename Unlinkable concern to Unlinking --- app/models/concerns/{unlinkable.rb => unlinking.rb} | 2 +- app/models/publication_name.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) rename app/models/concerns/{unlinkable.rb => unlinking.rb} (92%) diff --git a/app/models/concerns/unlinkable.rb b/app/models/concerns/unlinking.rb similarity index 92% rename from app/models/concerns/unlinkable.rb rename to app/models/concerns/unlinking.rb index 58e98566..e862e14f 100644 --- a/app/models/concerns/unlinkable.rb +++ b/app/models/concerns/unlinking.rb @@ -1,4 +1,4 @@ -module Unlinkable +module Unlinking extend ActiveSupport::Concern included do diff --git a/app/models/publication_name.rb b/app/models/publication_name.rb index 7818ccba..abc8c81a 100644 --- a/app/models/publication_name.rb +++ b/app/models/publication_name.rb @@ -1,5 +1,5 @@ class PublicationName < ApplicationRecord - include Unlinkable + include Unlinking include Versioned belongs_to(:publication) From 1f792d7d9127cd127357360a56c1424616311765 Mon Sep 17 00:00:00 2001 From: vangberg Date: Tue, 20 Oct 2026 13:13:12 +0200 Subject: [PATCH 17/41] Record polymorphic relationship IDs and types together in versions --- app/models/concerns/versioned.rb | 7 ++++- test/models/name/versioned_test.rb | 41 ++++++++++++++++++++++++++++++ 2 files changed, 47 insertions(+), 1 deletion(-) diff --git a/app/models/concerns/versioned.rb b/app/models/concerns/versioned.rb index bdad1136..887bbaa9 100644 --- a/app/models/concerns/versioned.rb +++ b/app/models/concerns/versioned.rb @@ -46,8 +46,13 @@ def record_delete_version def versioned_changes changeset = saved_changes.except('created_at', 'updated_at') + # A polymorphic ID is only meaningful together with its record type. + polymorphic_sets = self.class.reflect_on_all_associations(:belongs_to) + .select(&:polymorphic?) + .map { |association| [association.foreign_key, association.foreign_type] } + changed_columns = changeset.keys - self.class.versioned_attribute_sets.each do |columns| + (self.class.versioned_attribute_sets + polymorphic_sets).each do |columns| changeset.merge!(versioned_together_changes(columns, changed_columns)) end changeset diff --git a/test/models/name/versioned_test.rb b/test/models/name/versioned_test.rb index c6361b49..ccaf33bc 100644 --- a/test/models/name/versioned_test.rb +++ b/test/models/name/versioned_test.rb @@ -60,6 +60,47 @@ class Name::VersionedTest < ActiveSupport::TestCase end end + test 'changing a polymorphic relationship id records its unchanged record type' do + @name.nomenclatural_type = strains(:one) + @name.save! + + @name.update!(nomenclatural_type: strains(:two)) + + assert_equal({ + 'nomenclatural_type_id' => [strains(:one).id, strains(:two).id], + 'nomenclatural_type_type' => ['Strain', 'Strain'] + }, @name.versions.last.changeset) + end + + test 'changing a polymorphic relationship type records its unchanged id' do + @name.nomenclatural_type = genomes(:one) + @name.save! + + @name.update!(nomenclatural_type_type: 'Strain') + + assert_equal({ + 'nomenclatural_type_id' => [genomes(:one).id, genomes(:one).id], + 'nomenclatural_type_type' => ['Genome', 'Strain'] + }, @name.versions.last.changeset) + end + + test 'adding and removing a polymorphic relationship records both attributes' do + @name.save! + @name.update!(nomenclatural_type: strains(:one)) + + assert_equal({ + 'nomenclatural_type_id' => [nil, strains(:one).id], + 'nomenclatural_type_type' => [nil, 'Strain'] + }, @name.versions.last.changeset) + + @name.update!(nomenclatural_type: nil) + + assert_equal({ + 'nomenclatural_type_id' => [strains(:one).id, nil], + 'nomenclatural_type_type' => ['Strain', nil] + }, @name.versions.last.changeset) + end + test 'deleting a name changes all final attributes to nothing' do @name.save! @name.update!(syllabication: 'co.li') From 918d2c9c152e6c5766fae4cc05d117aae9b0036c Mon Sep 17 00:00:00 2001 From: vangberg Date: Tue, 20 Oct 2026 13:28:07 +0200 Subject: [PATCH 18/41] Add Publication#display using the short citation --- app/models/publication.rb | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/app/models/publication.rb b/app/models/publication.rb index 1295cde4..5adc101d 100644 --- a/app/models/publication.rb +++ b/app/models/publication.rb @@ -304,6 +304,10 @@ def short_citation(format = :text) "#{authors_et_al(format)}, #{journal_date.year}" end + def display(_html = true) + short_citation + end + def citation "#{short_citation}, #{journal_raw || pub_type.tr('-', ' ')}" end From 494476947a64687896ed928884006e1f953d6a95 Mon Sep 17 00:00:00 2001 From: vangberg Date: Tue, 20 Oct 2026 13:28:07 +0200 Subject: [PATCH 19/41] Show relationships, types and identifiers in name history Extend the public history to cover publication roles, name relationships, the genome, the nomenclatural type and external identifiers. Group the polymorphic type's ID and type so each side renders with its own class. Resolve foreign keys through Name's non-polymorphic belongs_to reflections rather than hard-coded attribute lists, and use the generic history_record_reference for publications too. --- app/helpers/names_helper.rb | 56 +++++++++++++++++---- app/models/name/public_history.rb | 18 +++++-- app/views/names/history.html.erb | 2 +- test/controllers/names/history_test.rb | 18 +++++++ test/helpers/names_helper_test.rb | 60 +++++++++++++++++++++-- test/models/name/public_history_test.rb | 65 ++++++++++++++++++++++++- 6 files changed, 199 insertions(+), 20 deletions(-) diff --git a/app/helpers/names_helper.rb b/app/helpers/names_helper.rb index 61c0933d..27452723 100644 --- a/app/helpers/names_helper.rb +++ b/app/helpers/names_helper.rb @@ -13,12 +13,17 @@ def history_values(changes, attribute:, record_type: 'Name') values.map { |value| safe_join(Array(value)).presence || '—' } end - def history_publication_reference(publication_id) - publication = Publication.find_by(id: publication_id) if publication_id - if publication - link_to(publication.short_citation, publication) - elsif publication_id - "Publication ##{publication_id} (deleted)" + def history_record_reference(record_class, id) + return unless id + + record = record_class.find_by(id: id) + if record.nil? + "#{record_class.name} ##{id} (deleted)" + elsif record.is_a?(GenericTypeMaterial) + # Generic type material has no dedicated page to link to. + record.display + else + link_to(record.display, record) end end @@ -116,15 +121,48 @@ def name_lineage(name, links: true, last: true, register: nil, visited: []) private def history_name_values(changes, attribute) + # Grouped values and status codes need domain-specific rendering before + # ordinary foreign keys can be resolved through their associations. case attribute when 'etymology' history_diff_parts(*changes.map { |attributes| Name.new(attributes).full_etymology }) - when 'proposed_in_id' - changes.map { |id| history_publication_reference(id) } + when 'genome_id' + # This legacy column has a custom reader, but no belongs_to association. + changes.map { |id| history_record_reference(Genome, id) } + when 'nomenclatural_type' + changes.map { |type, id| history_type_reference(type, id) } + when 'corrigendum_kind' + changes.map { |kind| Name.corrigendum_kinds.dig(kind&.to_sym, :name) || kind } when 'status' changes.map { |code| Name.status_hash.dig(code, :name) || code } else - history_text_values(changes) + # Match the actual foreign key, which need not follow the association's + # name. Polymorphic references are grouped with their types above. + association = Name.reflect_on_all_associations(:belongs_to).find do |reflection| + !reflection.polymorphic? && reflection.foreign_key == attribute + end + if association + changes.map { |id| history_record_reference(association.klass, id) } + else + history_text_values(changes) + end + end + end + + def history_type_reference(type, id) + # Polymorphic reflection cannot supply one class. Resolve only the supported + # type-material classes rather than constantizing a recorded type string. + record_class = { + 'Name' => Name, 'Genome' => Genome, 'Strain' => Strain, + 'GenericTypeMaterial' => GenericTypeMaterial + }[type] + if record_class + history_record_reference(record_class, id) + elsif id + # Older versions may have recorded an ID without its unchanged type. + "#{type || 'Unknown type'} ##{id}" + else + type end end diff --git a/app/models/name/public_history.rb b/app/models/name/public_history.rb index 67806069..67ddc249 100644 --- a/app/models/name/public_history.rb +++ b/app/models/name/public_history.rb @@ -2,8 +2,12 @@ class Name::PublicHistory # Only these recorded attributes appear in the public history. KEYS = %w[ name rank syllabication status priority_date authority - nomenclatural_status taxonomic_status proposal_kind corrigendum_from - type_material type_accession proposed_in_id + nomenclatural_status taxonomic_status proposal_kind corrigendum_from corrigendum_kind + type_material type_accession nomenclatural_type_type nomenclatural_type_id + proposed_in_id corrigendum_in_id assigned_in_id + parent_id correct_name_id basonym_id redirect_id genome_id genome_strain + ncbi_taxonomy lpsn_url gtdb_accession algaebase_species algaebase_taxonomy + wikidata_item wikispecies_entry ].concat(Name::ETYMOLOGY_COLUMNS).freeze def initialize(name) @@ -40,14 +44,20 @@ def name_versions @name.versions.where(operation: 'update') end - # Ordinary fields keep their [before, after] pairs. Etymology fields become - # one pair of attribute snapshots for the view to render together. + # Group etymology and polymorphic type attributes before removing unchanged + # companions, so each side retains the context needed to render it. def name_changes_for(version) changes = version.changeset.slice(*KEYS) etymology = changes.extract!(*Name::ETYMOLOGY_COLUMNS) if etymology.any? changes['etymology'] = etymology_snapshots(etymology, changes['name']) end + type_changes = changes.extract!('nomenclatural_type_type', 'nomenclatural_type_id') + if type_changes.any? + types = type_changes.fetch('nomenclatural_type_type', [nil, nil]) + ids = type_changes.fetch('nomenclatural_type_id', [nil, nil]) + changes['nomenclatural_type'] = types.zip(ids) + end # versioned_together records unchanged companion values too. changes.reject! { |_, values| values.first == values.last } changes diff --git a/app/views/names/history.html.erb b/app/views/names/history.html.erb index 94d804e2..98f2ee73 100644 --- a/app/views/names/history.html.erb +++ b/app/views/names/history.html.erb @@ -28,7 +28,7 @@ - Publication: <%= history_publication_reference(@history.publication_id_for(version)) %> + Publication: <%= history_record_reference(Publication, @history.publication_id_for(version)) %> <% end %> diff --git a/test/controllers/names/history_test.rb b/test/controllers/names/history_test.rb index c9179177..60c53cba 100644 --- a/test/controllers/names/history_test.rb +++ b/test/controllers/names/history_test.rb @@ -24,4 +24,22 @@ class NamesHistoryTest < ActionDispatch::IntegrationTest assert_select 'h3', text: 'Insufficient permits' assert_select '.name-history-version', count: 0 end + + test 'renders changes to types classification and publication roles' do + name = names(:escherichia_coli) + name.update!(nomenclatural_type: strains(:one)) + name.update!( + parent: nil, nomenclatural_type: strains(:two), + corrigendum_in: publications(:one) + ) + + get history_name_path(name) + + assert_response :success + assert_select '.name-history-version', count: 2 + assert_select '.name-history-change a[href=?]', strain_path(strains(:one)), count: 2 + assert_select '.name-history-change a[href=?]', strain_path(strains(:two)), count: 1 + assert_select '.name-history-change a[href=?]', name_path(names(:escherichia)), count: 1 + assert_select '.name-history-change a[href=?]', publication_path(publications(:one)), count: 1 + end end diff --git a/test/helpers/names_helper_test.rb b/test/helpers/names_helper_test.rb index bf2b9aa0..c649bb6a 100644 --- a/test/helpers/names_helper_test.rb +++ b/test/helpers/names_helper_test.rb @@ -26,10 +26,62 @@ class NamesHelperTest < ActionView::TestCase test 'renders publication changes as citations' do publication = publications(:one) citation = link_to(publication.short_citation, publication) - assert_equal [citation, '—'], - history_values([publication.id, nil], attribute: 'proposed_in_id') - assert_equal ['—', citation], - history_values([nil, publication.id], attribute: 'proposed_in_id') + %w[proposed_in_id corrigendum_in_id assigned_in_id].each do |attribute| + assert_equal [citation, '—'], + history_values([publication.id, nil], attribute: attribute) + assert_equal ['—', citation], + history_values([nil, publication.id], attribute: attribute) + end + end + + test 'renders name relationships as links and retains deleted references' do + name = names(:escherichia) + %w[parent_id correct_name_id basonym_id redirect_id].each do |attribute| + assert_equal ['—', link_to(name.display, name)], + history_values([nil, name.id], attribute: attribute) + end + + name.destroy! + + assert_equal ["Name ##{name.id} (deleted)", '—'], + history_values([name.id, nil], attribute: 'parent_id') + end + + test 'renders polymorphic references using the type on each side' do + strain = strains(:one) + genome = genomes(:one) + assert_equal [link_to(strain.display, strain), link_to(genome.display, genome)], + history_values( + [['Strain', strain.id], ['Genome', genome.id]], + attribute: 'nomenclatural_type' + ) + assert_equal [link_to(genome.display, genome), '—'], + history_values([['Genome', genome.id], [nil, nil]], + attribute: 'nomenclatural_type') + assert_equal ['—', link_to(genome.display, genome)], + history_values([nil, genome.id], attribute: 'genome_id') + end + + test 'renders generic type material without requiring a dedicated page' do + material = GenericTypeMaterial.create!(text: 'Preserved E. coli specimen') + + assert_equal ['—', material.display], + history_values([[nil, nil], ['GenericTypeMaterial', material.id]], + attribute: 'nomenclatural_type') + end + + test 'preserves polymorphic references with deleted or unrecorded types' do + strain = strains(:one) + strain.destroy! + + assert_equal ["Strain ##{strain.id} (deleted)", "Unknown type ##{strain.id}"], + history_values([['Strain', strain.id], [nil, strain.id]], + attribute: 'nomenclatural_type') + end + + test 'renders corrigendum sources with their display labels' do + assert_equal ['Publication', 'SeqCode Registry'], + history_values(%w[publication seqcode], attribute: 'corrigendum_kind') end test 'diffs text additions and removals in either record context' do diff --git a/test/models/name/public_history_test.rb b/test/models/name/public_history_test.rb index c36211f1..a8ef14ad 100644 --- a/test/models/name/public_history_test.rb +++ b/test/models/name/public_history_test.rb @@ -11,7 +11,8 @@ class Name::PublicHistoryTest < ActiveSupport::TestCase 'name' => ['E. coli', 'E. coli'], 'authority' => [nil, 'Smith'], 'proposed_in_id' => [nil, publications(:one).id], - 'gtdb_accession' => [nil, 'GB_GCA_000005845.2'], + 'gtdb_accession' => [nil, 's__Escherichia coli'], + 'protect_from_lpsn' => [nil, 'name'], 'etymology_xx_lang' => ['N.L.', 'N.L.'], 'etymology_xx_description' => ['of the colon', 'from the colon'] }) @@ -19,6 +20,7 @@ class Name::PublicHistoryTest < ActiveSupport::TestCase assert_equal({ 'authority' => [nil, 'Smith'], 'proposed_in_id' => [nil, publications(:one).id], + 'gtdb_accession' => [nil, 's__Escherichia coli'], 'etymology' => [ { 'name' => 'E. coli', 'etymology_xx_lang' => 'N.L.', 'etymology_xx_description' => 'of the colon' }, @@ -57,7 +59,7 @@ class Name::PublicHistoryTest < ActiveSupport::TestCase link.unlink! unlinked_version = link.versions.last 24.times { |index| @name.update!(authority: "Smith #{index}") } - @name.update!(gtdb_accession: 'GB_GCA_000005845.2') + @name.update!(protect_from_lpsn: 'name') link.update!(emends: true) first_page = @history.versions(page: 1) @@ -69,6 +71,65 @@ class Name::PublicHistoryTest < ActiveSupport::TestCase assert_equal [unlinked_version, linked_version], second_page end + test 'includes changes to scientific relationships and publication roles' do + @name.update!( + parent: nil, correct_name: names(:escherichia_colie), + basonym: names(:escherichia_colie), redirect: names(:escherichia_colie), + corrigendum_in: publications(:one), assigned_in: publications(:one), + corrigendum_kind: 'publication', genome_id: genomes(:one).id, + genome_strain: 'ATCC 11775' + ) + + changes = @history.changes_for(@name.versions.last) + + assert_equal [names(:escherichia).id, nil], changes['parent_id'] + %w[correct_name_id basonym_id redirect_id].each do |attribute| + assert_equal [nil, names(:escherichia_colie).id], changes[attribute] + end + %w[corrigendum_in_id assigned_in_id].each do |attribute| + assert_equal [nil, publications(:one).id], changes[attribute] + end + assert_equal [nil, 'publication'], changes['corrigendum_kind'] + assert_equal [nil, genomes(:one).id], changes['genome_id'] + assert_equal [nil, 'ATCC 11775'], changes['genome_strain'] + end + + test 'includes external identifiers but excludes cached data and workflow controls' do + identifiers = { + ncbi_taxonomy: 562, lpsn_url: 'https://lpsn.dsmz.de/species/escherichia-coli', + gtdb_accession: 's__Escherichia coli', wikidata_item: 'Q25419', + wikispecies_entry: 'Escherichia coli' + } + @name.update!(identifiers.merge( + gbif_json: '{}', queued_external: Time.current, + temporary_editable_at: Time.current, protect_from_lpsn: 'name' + )) + + assert_equal identifiers.stringify_keys.keys.sort, + @history.changes_for(@name.versions.last).keys.sort + end + + test 'groups polymorphic relationship changes with their unchanged companions' do + @name.update!(nomenclatural_type: strains(:one)) + @name.update!(nomenclatural_type: strains(:two)) + + assert_equal({ 'nomenclatural_type' => [ + ['Strain', strains(:one).id], ['Strain', strains(:two).id] + ] }, @history.changes_for(@name.versions.last)) + + @name.update!(nomenclatural_type_type: 'Genome') + + assert_equal({ 'nomenclatural_type' => [ + ['Strain', strains(:two).id], ['Genome', strains(:two).id] + ] }, @history.changes_for(@name.versions.last)) + + @name.update!(nomenclatural_type: nil) + + assert_equal({ 'nomenclatural_type' => [ + ['Genome', strains(:two).id], [nil, nil] + ] }, @history.changes_for(@name.versions.last)) + end + test 'identifies the publication for each relationship history including unlinks' do first_link = PublicationName.create!(name: @name, publication: publications(:one)) second_link = PublicationName.create!(name: @name, publication: publications(:no_doi)) From d0cdbc043d33d9f97614f489ace79e28acd2cf29 Mon Sep 17 00:00:00 2001 From: vangberg Date: Tue, 20 Oct 2026 13:49:23 +0200 Subject: [PATCH 20/41] Group and resolve polymorphic references in name history generically Pair each polymorphic belongs_to's type and ID columns using its reflection rather than naming nomenclatural_type, and resolve the recorded type to any model class instead of a fixed allowlist. --- app/helpers/names_helper.rb | 41 ++++++++++++++++--------------- app/models/name/public_history.rb | 12 +++++---- test/helpers/names_helper_test.rb | 6 +++++ 3 files changed, 34 insertions(+), 25 deletions(-) diff --git a/app/helpers/names_helper.rb b/app/helpers/names_helper.rb index 27452723..a9190282 100644 --- a/app/helpers/names_helper.rb +++ b/app/helpers/names_helper.rb @@ -129,34 +129,35 @@ def history_name_values(changes, attribute) when 'genome_id' # This legacy column has a custom reader, but no belongs_to association. changes.map { |id| history_record_reference(Genome, id) } - when 'nomenclatural_type' - changes.map { |type, id| history_type_reference(type, id) } when 'corrigendum_kind' changes.map { |kind| Name.corrigendum_kinds.dig(kind&.to_sym, :name) || kind } when 'status' changes.map { |code| Name.status_hash.dig(code, :name) || code } else - # Match the actual foreign key, which need not follow the association's - # name. Polymorphic references are grouped with their types above. - association = Name.reflect_on_all_associations(:belongs_to).find do |reflection| - !reflection.polymorphic? && reflection.foreign_key == attribute - end - if association - changes.map { |id| history_record_reference(association.klass, id) } - else - history_text_values(changes) - end + history_association_values(changes, attribute) + end + end + + # Match ordinary associations by their actual foreign key, which need not + # follow the association's name. Polymorphic references arrive grouped under + # the association's name as [type, id] pairs. + def history_association_values(changes, attribute) + association = Name.reflect_on_all_associations(:belongs_to).find do |reflection| + attribute == (reflection.polymorphic? ? reflection.name.to_s : reflection.foreign_key) + end + if association.nil? + history_text_values(changes) + elsif association.polymorphic? + changes.map { |type, id| history_polymorphic_reference(type, id) } + else + changes.map { |id| history_record_reference(association.klass, id) } end end - def history_type_reference(type, id) - # Polymorphic reflection cannot supply one class. Resolve only the supported - # type-material classes rather than constantizing a recorded type string. - record_class = { - 'Name' => Name, 'Genome' => Genome, 'Strain' => Strain, - 'GenericTypeMaterial' => GenericTypeMaterial - }[type] - if record_class + def history_polymorphic_reference(type, id) + # Resolve only model classes, not any constant a recorded type string names. + record_class = type&.safe_constantize + if record_class.is_a?(Class) && record_class < ApplicationRecord history_record_reference(record_class, id) elsif id # Older versions may have recorded an ID without its unchanged type. diff --git a/app/models/name/public_history.rb b/app/models/name/public_history.rb index 67ddc249..44a616da 100644 --- a/app/models/name/public_history.rb +++ b/app/models/name/public_history.rb @@ -52,11 +52,13 @@ def name_changes_for(version) if etymology.any? changes['etymology'] = etymology_snapshots(etymology, changes['name']) end - type_changes = changes.extract!('nomenclatural_type_type', 'nomenclatural_type_id') - if type_changes.any? - types = type_changes.fetch('nomenclatural_type_type', [nil, nil]) - ids = type_changes.fetch('nomenclatural_type_id', [nil, nil]) - changes['nomenclatural_type'] = types.zip(ids) + Name.reflect_on_all_associations(:belongs_to).select(&:polymorphic?).each do |reflection| + reference = changes.extract!(reflection.foreign_type, reflection.foreign_key) + next if reference.empty? + + types = reference.fetch(reflection.foreign_type, [nil, nil]) + ids = reference.fetch(reflection.foreign_key, [nil, nil]) + changes[reflection.name.to_s] = types.zip(ids) end # versioned_together records unchanged companion values too. changes.reject! { |_, values| values.first == values.last } diff --git a/test/helpers/names_helper_test.rb b/test/helpers/names_helper_test.rb index c649bb6a..ec729c68 100644 --- a/test/helpers/names_helper_test.rb +++ b/test/helpers/names_helper_test.rb @@ -79,6 +79,12 @@ class NamesHelperTest < ActionView::TestCase attribute: 'nomenclatural_type') end + test 'does not resolve polymorphic types that are not models' do + assert_equal ['Kernel #1', 'Strain::Missing #2'], + history_values([['Kernel', 1], ['Strain::Missing', 2]], + attribute: 'nomenclatural_type') + end + test 'renders corrigendum sources with their display labels' do assert_equal ['Publication', 'SeqCode Registry'], history_values(%w[publication seqcode], attribute: 'corrigendum_kind') From 2eee6cbe3a94fdd8ff45c2b2e4fbffb38ba7483b Mon Sep 17 00:00:00 2001 From: vangberg Date: Tue, 20 Oct 2026 14:05:11 +0200 Subject: [PATCH 21/41] Move name history into Names::HistoriesController Serve the history as a nested singular resource at the same URL, and move its helpers and tests alongside it, leaving NamesHelper for displaying names. --- app/controllers/names/histories_controller.rb | 17 +++ app/controllers/names_controller.rb | 9 +- app/helpers/names/histories_helper.rb | 109 ++++++++++++++++++ app/helpers/names_helper.rb | 108 ----------------- app/views/names/_title.html.erb | 2 +- .../show.html.erb} | 0 config/routes.rb | 2 +- ...y_test.rb => histories_controller_test.rb} | 8 +- .../histories_helper_test.rb} | 2 +- 9 files changed, 134 insertions(+), 123 deletions(-) create mode 100644 app/controllers/names/histories_controller.rb create mode 100644 app/helpers/names/histories_helper.rb rename app/views/names/{history.html.erb => histories/show.html.erb} (100%) rename test/controllers/names/{history_test.rb => histories_controller_test.rb} (89%) rename test/helpers/{names_helper_test.rb => names/histories_helper_test.rb} (99%) diff --git a/app/controllers/names/histories_controller.rb b/app/controllers/names/histories_controller.rb new file mode 100644 index 00000000..f057618f --- /dev/null +++ b/app/controllers/names/histories_controller.rb @@ -0,0 +1,17 @@ +class Names::HistoriesController < ApplicationController + before_action(:set_name) + + # GET /names/1/history + def show + @history = Name::PublicHistory.new(@name) + @versions = @history.versions(page: params[:page]) + @crumbs = [['Names', names_path], [@name.abbr_name, @name], 'History'] + end + + private + + def set_name + @name = Name.find(params[:name_id]) + render('names/hidden') unless @name.can_view?(current_user, cookies[:reviewer_token]) + end +end diff --git a/app/controllers/names_controller.rb b/app/controllers/names_controller.rb index f9a141d2..8252c947 100644 --- a/app/controllers/names_controller.rb +++ b/app/controllers/names_controller.rb @@ -12,7 +12,7 @@ class NamesController < ApplicationController before_action( :set_name, only: %i[ - edit update destroy network wiki history + edit update destroy network wiki proposed_in not_validly_proposed_in emended_in assigned_in corrigendum_in corrigendum_orphan corrigendum edit_description edit_rank edit_notes edit_etymology edit_links edit_type @@ -213,13 +213,6 @@ def show end end - # GET /names/1/history - def history - @history = Name::PublicHistory.new(@name) - @versions = @history.versions(page: params[:page]) - @crumbs = [['Names', names_path], [@name.abbr_name, @name], 'History'] - end - # GET /names/linkout.xml # GET /names/1/linkout.xml def linkout diff --git a/app/helpers/names/histories_helper.rb b/app/helpers/names/histories_helper.rb new file mode 100644 index 00000000..334ed82a --- /dev/null +++ b/app/helpers/names/histories_helper.rb @@ -0,0 +1,109 @@ +module Names::HistoriesHelper + def history_values(changes, attribute:, record_type: 'Name') + values = + case record_type + when 'PublicationName' + history_publication_name_values(changes) + when 'Name' + history_name_values(changes, attribute) + else + raise ArgumentError, "Unsupported public history record type: #{record_type}" + end + + values.map { |value| safe_join(Array(value)).presence || '—' } + end + + def history_record_reference(record_class, id) + return unless id + + record = record_class.find_by(id: id) + if record.nil? + "#{record_class.name} ##{id} (deleted)" + elsif record.is_a?(GenericTypeMaterial) + # Generic type material has no dedicated page to link to. + record.display + else + link_to(record.display, record) + end + end + + def history_diff_parts(before, after) + chunks = TextDiff.chunks(before, after) + + before_parts = chunks.filter_map do |unchanged, text, _| + next if text.empty? + + unchanged ? text : content_tag(:del, text, class: 'text-danger') + end + after_parts = chunks.filter_map do |unchanged, _, text| + next if text.empty? + + unchanged ? text : content_tag(:ins, text, class: 'text-success') + end + + [before_parts, after_parts] + end + + private + + def history_name_values(changes, attribute) + # Grouped values and status codes need domain-specific rendering before + # ordinary foreign keys can be resolved through their associations. + case attribute + when 'etymology' + history_diff_parts(*changes.map { |attributes| Name.new(attributes).full_etymology }) + when 'genome_id' + # This legacy column has a custom reader, but no belongs_to association. + changes.map { |id| history_record_reference(Genome, id) } + when 'corrigendum_kind' + changes.map { |kind| Name.corrigendum_kinds.dig(kind&.to_sym, :name) || kind } + when 'status' + changes.map { |code| Name.status_hash.dig(code, :name) || code } + else + history_association_values(changes, attribute) + end + end + + # Match ordinary associations by their actual foreign key, which need not + # follow the association's name. Polymorphic references arrive grouped under + # the association's name as [type, id] pairs. + def history_association_values(changes, attribute) + association = Name.reflect_on_all_associations(:belongs_to).find do |reflection| + attribute == (reflection.polymorphic? ? reflection.name.to_s : reflection.foreign_key) + end + if association.nil? + history_text_values(changes) + elsif association.polymorphic? + changes.map { |type, id| history_polymorphic_reference(type, id) } + else + changes.map { |id| history_record_reference(association.klass, id) } + end + end + + def history_polymorphic_reference(type, id) + # Resolve only model classes, not any constant a recorded type string names. + record_class = type&.safe_constantize + if record_class.is_a?(Class) && record_class < ApplicationRecord + history_record_reference(record_class, id) + elsif id + # Older versions may have recorded an ID without its unchanged type. + "#{type || 'Unknown type'} ##{id}" + else + type + end + end + + def history_publication_name_values(changes) + history_text_values(changes).map do |value| + case value + when true then 'Yes' + when false then 'No' + else value + end + end + end + + def history_text_values(changes) + changes.any? { |value| value.is_a?(String) } ? history_diff_parts(*changes) : changes + end +end diff --git a/app/helpers/names_helper.rb b/app/helpers/names_helper.rb index a9190282..9d93b983 100644 --- a/app/helpers/names_helper.rb +++ b/app/helpers/names_helper.rb @@ -1,49 +1,4 @@ module NamesHelper - def history_values(changes, attribute:, record_type: 'Name') - values = - case record_type - when 'PublicationName' - history_publication_name_values(changes) - when 'Name' - history_name_values(changes, attribute) - else - raise ArgumentError, "Unsupported public history record type: #{record_type}" - end - - values.map { |value| safe_join(Array(value)).presence || '—' } - end - - def history_record_reference(record_class, id) - return unless id - - record = record_class.find_by(id: id) - if record.nil? - "#{record_class.name} ##{id} (deleted)" - elsif record.is_a?(GenericTypeMaterial) - # Generic type material has no dedicated page to link to. - record.display - else - link_to(record.display, record) - end - end - - def history_diff_parts(before, after) - chunks = TextDiff.chunks(before, after) - - before_parts = chunks.filter_map do |unchanged, text, _| - next if text.empty? - - unchanged ? text : content_tag(:del, text, class: 'text-danger') - end - after_parts = chunks.filter_map do |unchanged, _, text| - next if text.empty? - - unchanged ? text : content_tag(:ins, text, class: 'text-success') - end - - [before_parts, after_parts] - end - def link_to_name_type(name) if name.type_is_name? if name.type_name @@ -117,67 +72,4 @@ def name_lineage(name, links: true, last: true, register: nil, visited: []) out.inject(:+) end - - private - - def history_name_values(changes, attribute) - # Grouped values and status codes need domain-specific rendering before - # ordinary foreign keys can be resolved through their associations. - case attribute - when 'etymology' - history_diff_parts(*changes.map { |attributes| Name.new(attributes).full_etymology }) - when 'genome_id' - # This legacy column has a custom reader, but no belongs_to association. - changes.map { |id| history_record_reference(Genome, id) } - when 'corrigendum_kind' - changes.map { |kind| Name.corrigendum_kinds.dig(kind&.to_sym, :name) || kind } - when 'status' - changes.map { |code| Name.status_hash.dig(code, :name) || code } - else - history_association_values(changes, attribute) - end - end - - # Match ordinary associations by their actual foreign key, which need not - # follow the association's name. Polymorphic references arrive grouped under - # the association's name as [type, id] pairs. - def history_association_values(changes, attribute) - association = Name.reflect_on_all_associations(:belongs_to).find do |reflection| - attribute == (reflection.polymorphic? ? reflection.name.to_s : reflection.foreign_key) - end - if association.nil? - history_text_values(changes) - elsif association.polymorphic? - changes.map { |type, id| history_polymorphic_reference(type, id) } - else - changes.map { |id| history_record_reference(association.klass, id) } - end - end - - def history_polymorphic_reference(type, id) - # Resolve only model classes, not any constant a recorded type string names. - record_class = type&.safe_constantize - if record_class.is_a?(Class) && record_class < ApplicationRecord - history_record_reference(record_class, id) - elsif id - # Older versions may have recorded an ID without its unchanged type. - "#{type || 'Unknown type'} ##{id}" - else - type - end - end - - def history_publication_name_values(changes) - history_text_values(changes).map do |value| - case value - when true then 'Yes' - when false then 'No' - else value - end - end - end - - def history_text_values(changes) - changes.any? { |value| value.is_a?(String) } ? history_diff_parts(*changes) : changes - end end diff --git a/app/views/names/_title.html.erb b/app/views/names/_title.html.erb index b85062bb..7af3e1fd 100644 --- a/app/views/names/_title.html.erb +++ b/app/views/names/_title.html.erb @@ -18,7 +18,7 @@ %> <%= download_buttons([ - [history_name_path(@name), 'history', 'History'], + [name_history_path(@name), 'history', 'History'], [name_url(@name, format: :json), 'file-code', 'JSON'], [name_url(@name, format: :pdf), 'file-pdf', 'PDF'], observe diff --git a/app/views/names/history.html.erb b/app/views/names/histories/show.html.erb similarity index 100% rename from app/views/names/history.html.erb rename to app/views/names/histories/show.html.erb diff --git a/config/routes.rb b/config/routes.rb index cd9a434a..e0348e62 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -86,7 +86,6 @@ end member do # --> Display name - get :history get :network get :linkout get :wiki @@ -126,6 +125,7 @@ path: '/not_validly_proposed_in/:publication_id' end resources :pseudonyms + resource(:history, only: :show, module: :names) end # Taxonomic placements diff --git a/test/controllers/names/history_test.rb b/test/controllers/names/histories_controller_test.rb similarity index 89% rename from test/controllers/names/history_test.rb rename to test/controllers/names/histories_controller_test.rb index 60c53cba..21419794 100644 --- a/test/controllers/names/history_test.rb +++ b/test/controllers/names/histories_controller_test.rb @@ -1,12 +1,12 @@ require 'test_helper' -class NamesHistoryTest < ActionDispatch::IntegrationTest +class Names::HistoriesControllerTest < ActionDispatch::IntegrationTest test 'renders public history' do name = names(:escherichia_coli) name.update!(authority: 'Smith', etymology_xx_description: 'of the colon') PublicationName.create!(name: name, publication: publications(:one)) - get history_name_path(name) + get name_history_path(name) assert_response :success assert_select '.name-history-version', count: 2 @@ -19,7 +19,7 @@ class NamesHistoryTest < ActionDispatch::IntegrationTest name.update!(syllabication: 'co.li') PublicationName.create!(name: name, publication: publications(:one)).unlink! - get history_name_path(name) + get name_history_path(name) assert_select 'h3', text: 'Insufficient permits' assert_select '.name-history-version', count: 0 @@ -33,7 +33,7 @@ class NamesHistoryTest < ActionDispatch::IntegrationTest corrigendum_in: publications(:one) ) - get history_name_path(name) + get name_history_path(name) assert_response :success assert_select '.name-history-version', count: 2 diff --git a/test/helpers/names_helper_test.rb b/test/helpers/names/histories_helper_test.rb similarity index 99% rename from test/helpers/names_helper_test.rb rename to test/helpers/names/histories_helper_test.rb index ec729c68..389e5a78 100644 --- a/test/helpers/names_helper_test.rb +++ b/test/helpers/names/histories_helper_test.rb @@ -1,6 +1,6 @@ require 'test_helper' -class NamesHelperTest < ActionView::TestCase +class Names::HistoriesHelperTest < ActionView::TestCase test 'groups nearby etymology edits into one span' do changes = [ { name: 'E. coli', etymology_xx_description: 'of the colon' }, From 9b02b0cf7b0c256b6f14a2d08958d61c599a1838 Mon Sep 17 00:00:00 2001 From: vangberg Date: Tue, 20 Oct 2026 14:41:49 +0200 Subject: [PATCH 22/41] Drop legacy type columns from public name history Nothing writes type_material and type_accession any more. --- app/models/name/public_history.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/name/public_history.rb b/app/models/name/public_history.rb index 44a616da..0a64e072 100644 --- a/app/models/name/public_history.rb +++ b/app/models/name/public_history.rb @@ -3,7 +3,7 @@ class Name::PublicHistory KEYS = %w[ name rank syllabication status priority_date authority nomenclatural_status taxonomic_status proposal_kind corrigendum_from corrigendum_kind - type_material type_accession nomenclatural_type_type nomenclatural_type_id + nomenclatural_type_type nomenclatural_type_id proposed_in_id corrigendum_in_id assigned_in_id parent_id correct_name_id basonym_id redirect_id genome_id genome_strain ncbi_taxonomy lpsn_url gtdb_accession algaebase_species algaebase_taxonomy From 77eba84e669e5db644b15f4fd66f77503aee95aa Mon Sep 17 00:00:00 2001 From: vangberg Date: Tue, 20 Oct 2026 14:39:14 +0200 Subject: [PATCH 23/41] Allowlist publication link attributes in public name history --- app/models/name/public_history.rb | 8 +++++--- test/models/name/public_history_test.rb | 9 +++++---- 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/app/models/name/public_history.rb b/app/models/name/public_history.rb index 0a64e072..e892a70e 100644 --- a/app/models/name/public_history.rb +++ b/app/models/name/public_history.rb @@ -1,6 +1,6 @@ class Name::PublicHistory # Only these recorded attributes appear in the public history. - KEYS = %w[ + NAME_KEYS = %w[ name rank syllabication status priority_date authority nomenclatural_status taxonomic_status proposal_kind corrigendum_from corrigendum_kind nomenclatural_type_type nomenclatural_type_id @@ -9,6 +9,8 @@ class Name::PublicHistory ncbi_taxonomy lpsn_url gtdb_accession algaebase_species algaebase_taxonomy wikidata_item wikispecies_entry ].concat(Name::ETYMOLOGY_COLUMNS).freeze + # unlinked_at is shown as whether the publication is linked. + PUBLICATION_NAME_KEYS = %w[unlinked_at emends not_valid_proposal].freeze def initialize(name) @name = name @@ -47,7 +49,7 @@ def name_versions # Group etymology and polymorphic type attributes before removing unchanged # companions, so each side retains the context needed to render it. def name_changes_for(version) - changes = version.changeset.slice(*KEYS) + changes = version.changeset.slice(*NAME_KEYS) etymology = changes.extract!(*Name::ETYMOLOGY_COLUMNS) if etymology.any? changes['etymology'] = etymology_snapshots(etymology, changes['name']) @@ -75,7 +77,7 @@ def publication_changes_for(version) return { 'linked' => [false, true] } if version.operation == 'create' return {} unless version.operation == 'update' - changes = version.changeset.dup + changes = version.changeset.slice(*PUBLICATION_NAME_KEYS) unlinked_at = changes.delete('unlinked_at') return changes unless unlinked_at diff --git a/test/models/name/public_history_test.rb b/test/models/name/public_history_test.rb index a8ef14ad..79efd1ff 100644 --- a/test/models/name/public_history_test.rb +++ b/test/models/name/public_history_test.rb @@ -168,12 +168,13 @@ class Name::PublicHistoryTest < ActiveSupport::TestCase assert_equal({ 'emends' => [false, true] }, @history.changes_for(version)) end - test 'includes publication relationship changes without an attribute allowlist' do - values = [publications(:one).id, publications(:no_doi).id] + test 'keeps only allowlisted publication attributes' do version = Version.new(record_type: 'PublicationName', operation: 'update', changeset: { - 'publication_id' => values + 'publication_id' => [publications(:one).id, publications(:no_doi).id], + 'name_id' => [names(:escherichia_coli).id, names(:escherichia_colie).id], + 'emends' => [false, true] }) - assert_equal({ 'publication_id' => values }, @history.changes_for(version)) + assert_equal({ 'emends' => [false, true] }, @history.changes_for(version)) end end From 39ca45548eb270817e61481fc7b40388fdbfcab6 Mon Sep 17 00:00:00 2001 From: vangberg Date: Tue, 20 Oct 2026 14:44:10 +0200 Subject: [PATCH 24/41] Unlink placements instead of deleting them --- app/controllers/placements_controller.rb | 4 ++-- app/models/name.rb | 17 +++++++++-------- app/models/name/network.rb | 4 ++-- app/models/placement.rb | 9 +++++++-- app/models/tutorial/batch.rb | 4 ++-- ...61020100000_add_unlinked_at_to_placements.rb | 5 +++++ db/schema.rb | 3 ++- lib/tasks/import_gtdb.rake | 1 + lib/tasks/import_ncbi.rake | 1 + test/controllers/placements_controller_test.rb | 14 ++++++++++++++ test/models/placement_test.rb | 7 +++++++ 11 files changed, 52 insertions(+), 17 deletions(-) create mode 100644 db/migrate/20261020100000_add_unlinked_at_to_placements.rb diff --git a/app/controllers/placements_controller.rb b/app/controllers/placements_controller.rb index 9cc8fc36..9993aba4 100644 --- a/app/controllers/placements_controller.rb +++ b/app/controllers/placements_controller.rb @@ -149,7 +149,7 @@ def prefer def destroy Placement.transaction do @name.update(parent: nil, assigned_in: nil) - @placement.destroy + @placement.unlink! end redirect_to(@name, notice: 'Name placement was removed') end @@ -158,7 +158,7 @@ def destroy # Use callbacks to share common setup or constraints between actions. def set_placement - @placement = Placement.find(params[:id]) + @placement = Placement.linked.find(params[:id]) end def set_name diff --git a/app/models/name.rb b/app/models/name.rb index caf64e57..3aa86e53 100644 --- a/app/models/name.rb +++ b/app/models/name.rb @@ -26,6 +26,7 @@ class Name < ApplicationRecord alias :correspondences :name_correspondences has_many(:checks, dependent: :destroy) has_many(:check_users, -> { distinct }, through: :checks, source: :user) + # A deleted name no longer needs its placements, including unlinked ones. has_many( :placements, -> { includes(:parent) }, dependent: :destroy, inverse_of: :name @@ -1202,11 +1203,11 @@ def taxonomic_data? end def alt_placements - @alt_placements ||= placements.where(preferred: false) + @alt_placements ||= placements.linked.where(preferred: false) end def alt_child_placements - @alt_child_placements ||= child_placements.where(preferred: false) + @alt_child_placements ||= child_placements.linked.where(preferred: false) end def alt_children @@ -1214,7 +1215,7 @@ def alt_children end def placement - @placement ||= placements.where(preferred: true).first + @placement ||= placements.linked.where(preferred: true).first end def consistent_placement? @@ -1491,17 +1492,17 @@ def harmonize_register_and_status def ensure_consistent_placement if parent_id.present? - pp = placements.where( + pp = placements.linked.where( parent_id: parent_id, incertae_sedis: false ).first if pp.present? if pp.preferred? true else - placements.update(preferred: false) && pp.update(preferred: true) + placements.linked.update(preferred: false) && pp.update(preferred: true) end else - placements.update(preferred: false) && + placements.linked.update(preferred: false) && Placement.new( name_id: id, parent_id: parent_id, @@ -1509,11 +1510,11 @@ def ensure_consistent_placement preferred: true ).save end - elsif placements.where(preferred: true, incertae_sedis: true).exists? + elsif placements.linked.where(preferred: true, incertae_sedis: true).exists? true else # Conservatively preserve as alternative placement - placements.update(preferred: false) + placements.linked.update(preferred: false) end end diff --git a/app/models/name/network.rb b/app/models/name/network.rb index 57cf18bc..e8e2f879 100644 --- a/app/models/name/network.rb +++ b/app/models/name/network.rb @@ -81,7 +81,7 @@ def network_up_nodes @network_up_nodes = Set.new @network_up_edges = Set.new - placements.includes(:parent, :name).each do |placement| + placements.linked.includes(:parent, :name).each do |placement| next if !placement.parent.present? || placement.downwards? @network_up_nodes << placement.parent @@ -133,7 +133,7 @@ def network_down_nodes @network_down_nodes = Set.new @network_down_edges = Set.new - child_placements.includes(:parent, :name).each do |placement| + child_placements.linked.includes(:parent, :name).each do |placement| next if placement.downwards? @network_down_nodes << placement.name diff --git a/app/models/placement.rb b/app/models/placement.rb index 9c0e7e0f..35231bdd 100644 --- a/app/models/placement.rb +++ b/app/models/placement.rb @@ -1,4 +1,6 @@ class Placement < ApplicationRecord + include Unlinking + belongs_to(:name) belongs_to( :parent, optional: true, class_name: 'Name', foreign_key: 'parent_id' @@ -12,7 +14,10 @@ class Placement < ApplicationRecord validates( :incertae_sedis, inclusion: { in: [true, false] } ) - validates(:preferred, uniqueness: { scope: :name_id, if: :preferred? }) + validates( + :preferred, + uniqueness: { scope: :name_id, conditions: -> { linked }, if: :preferred? } + ) after_save(:harmonize_name_parent) @@ -46,6 +51,6 @@ def downwards? private def harmonize_name_parent - name.update(parent: incertae_sedis? ? nil : parent) if preferred + name.update(parent: incertae_sedis? ? nil : parent) if preferred && !unlinked? end end diff --git a/app/models/tutorial/batch.rb b/app/models/tutorial/batch.rb index bca322f3..3856de24 100644 --- a/app/models/tutorial/batch.rb +++ b/app/models/tutorial/batch.rb @@ -359,8 +359,8 @@ def batch_step_01(params, user) parent.save! end name = Name.find_by_variants(par['name']) - placement = name.placements.find_or_initialize_by(parent: parent) - name.placements.where(preferred: true).where.not(id: placement.id) + placement = name.placements.linked.find_or_initialize_by(parent: parent) + name.placements.linked.where(preferred: true).where.not(id: placement.id) .find_each { |current| current.update!(preferred: false) } placement.update!( incertae_sedis: par['incertae_sedis'] || false, diff --git a/db/migrate/20261020100000_add_unlinked_at_to_placements.rb b/db/migrate/20261020100000_add_unlinked_at_to_placements.rb new file mode 100644 index 00000000..c9f706eb --- /dev/null +++ b/db/migrate/20261020100000_add_unlinked_at_to_placements.rb @@ -0,0 +1,5 @@ +class AddUnlinkedAtToPlacements < ActiveRecord::Migration[6.1] + def change + add_column :placements, :unlinked_at, :datetime + end +end diff --git a/db/schema.rb b/db/schema.rb index d06727f0..a597ed1f 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 2026_10_02_100000) do +ActiveRecord::Schema.define(version: 2026_10_20_100000) do # These are extensions that must be enabled in order to support this database enable_extension "fuzzystrmatch" @@ -335,6 +335,7 @@ t.boolean "gtdb_taxonomy" t.boolean "ncbi_taxonomy" t.boolean "incertae_sedis", default: false, null: false + t.datetime "unlinked_at" t.index ["name_id"], name: "index_placements_on_name_id" t.index ["publication_id"], name: "index_placements_on_publication_id" end diff --git a/lib/tasks/import_gtdb.rake b/lib/tasks/import_gtdb.rake index a6b462ee..9e2ae0c0 100644 --- a/lib/tasks/import_gtdb.rake +++ b/lib/tasks/import_gtdb.rake @@ -58,6 +58,7 @@ namespace :gtdb do placement = Placement + .linked .create_with(gtdb_taxonomy: true) .find_or_create_by(name: name, parent: parent) diff --git a/lib/tasks/import_ncbi.rake b/lib/tasks/import_ncbi.rake index c665696d..db6c9300 100644 --- a/lib/tasks/import_ncbi.rake +++ b/lib/tasks/import_ncbi.rake @@ -104,6 +104,7 @@ namespace :ncbi do placement = Placement + .linked .create_with(ncbi_taxonomy: true) .find_or_create_by(name: name, parent: parent) diff --git a/test/controllers/placements_controller_test.rb b/test/controllers/placements_controller_test.rb index 4231ac99..0f3c0cb8 100644 --- a/test/controllers/placements_controller_test.rb +++ b/test/controllers/placements_controller_test.rb @@ -51,4 +51,18 @@ class PlacementsControllerTest < ActionDispatch::IntegrationTest assert_not_predicate placement, :incertae_sedis? assert_equal names(:escherichia), @name.parent end + + test 'unlinks an alternative placement instead of deleting it' do + name = names(:escherichia_coli) + placement = Placement.create!( + name: name, parent: names(:bacteria), incertae_sedis: true, + incertae_sedis_text: 'Family placement unresolved' + ) + + delete placement_path(placement) + + assert_redirected_to name_path(name) + assert placement.reload.unlinked? + assert_not_includes name.reload.alt_placements, placement + end end diff --git a/test/models/placement_test.rb b/test/models/placement_test.rb index 87fbf43d..2e475ffb 100644 --- a/test/models/placement_test.rb +++ b/test/models/placement_test.rb @@ -80,4 +80,11 @@ class PlacementTest < ActiveSupport::TestCase ) end + test 'unlinking the preferred placement allows a new preferred placement' do + name = names(:escherichia_coli) + placements(:escherichia_coli).unlink! + + assert_nil name.placement + assert Placement.create!(name: name, parent: names(:escherichia), preferred: true) + end end From cfab2746cfbc835988223b977f90a51d71d85f2a Mon Sep 17 00:00:00 2001 From: vangberg Date: Tue, 20 Oct 2026 15:37:06 +0200 Subject: [PATCH 25/41] Add PublicationName#display using the publication --- app/models/publication_name.rb | 4 ++++ test/models/publication_name_test.rb | 9 ++++++--- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/app/models/publication_name.rb b/app/models/publication_name.rb index abc8c81a..e8630358 100644 --- a/app/models/publication_name.rb +++ b/app/models/publication_name.rb @@ -5,6 +5,10 @@ class PublicationName < ApplicationRecord belongs_to(:publication) belongs_to(:name) + def display(html = true) + publication.display(html) + end + def proposes? name.proposed_in? publication end diff --git a/test/models/publication_name_test.rb b/test/models/publication_name_test.rb index 5717db67..b8e5e89c 100644 --- a/test/models/publication_name_test.rb +++ b/test/models/publication_name_test.rb @@ -1,7 +1,10 @@ require 'test_helper' class PublicationNameTest < ActiveSupport::TestCase - # test "the truth" do - # assert true - # end + test 'displays the short citation of its publication' do + publication = publications(:one) + link = PublicationName.new(name: names(:escherichia_coli), publication: publication) + + assert_equal publication.short_citation, link.display + end end From 7d05fa8a36b33e497b527f0f7b5c11d2b29164f5 Mon Sep 17 00:00:00 2001 From: vangberg Date: Tue, 20 Oct 2026 14:44:10 +0200 Subject: [PATCH 26/41] Track placement history and show it in public name history --- app/helpers/names/histories_helper.rb | 86 +++++++++++-------- app/models/name/public_history.rb | 58 ++++++++++--- app/models/placement.rb | 1 + app/views/names/histories/show.html.erb | 10 +-- .../names/histories_controller_test.rb | 16 +++- test/helpers/names/histories_helper_test.rb | 44 +++++++++- test/models/name/public_history_test.rb | 52 ++++++++--- 7 files changed, 197 insertions(+), 70 deletions(-) diff --git a/app/helpers/names/histories_helper.rb b/app/helpers/names/histories_helper.rb index 334ed82a..bddfae1a 100644 --- a/app/helpers/names/histories_helper.rb +++ b/app/helpers/names/histories_helper.rb @@ -1,15 +1,12 @@ module Names::HistoriesHelper + # Render a recorded [before, after] pair as two display values. def history_values(changes, attribute:, record_type: 'Name') - values = - case record_type - when 'PublicationName' - history_publication_name_values(changes) - when 'Name' - history_name_values(changes, attribute) - else - raise ArgumentError, "Unsupported public history record type: #{record_type}" - end - + # Some name attributes need domain-specific formatting; everything else, + # on any record type, is rendered generically from its model. + values = history_name_values(changes, attribute) if record_type == 'Name' + values ||= history_attribute_values(record_type.constantize, changes, attribute) + + # Diffs return arrays of parts; empty values show as a dash. values.map { |value| safe_join(Array(value)).presence || '—' } end @@ -27,6 +24,35 @@ def history_record_reference(record_class, id) end end + # Label a link's changes with what it links to. + def history_record_heading(version) + return history_link_reference(version.record) if version.record + + # The link itself was deleted, e.g. along with its publication. + "#{version.record_type.constantize.model_name.human} (deleted)" + end + + # Identify a publication link by its publication, and a placement by its + # parent, marking incertae sedis placements: + # publication link + # => Publication: Smith 2026 + # placement under Escherichia + # => Placement: Escherichia + # incertae sedis placement under Bacteria + # => Placement: incertae sedis (Bacteria) + # placement under a deleted parent + # => Placement: Name #3 (deleted) + def history_link_reference(record) + case record + when PublicationName + safe_join(['Publication: ', link_to(record.display, record.publication)]) + when Placement + parent = history_record_reference(Name, record.parent_id) + parent = safe_join([content_tag(:i, 'incertae sedis'), ' (', parent, ')']) if record.incertae_sedis? + safe_join(['Placement: ', parent]) + end + end + def history_diff_parts(before, after) chunks = TextDiff.chunks(before, after) @@ -46,9 +72,8 @@ def history_diff_parts(before, after) private + # Name attributes that need domain-specific rendering, or nil for the rest. def history_name_values(changes, attribute) - # Grouped values and status codes need domain-specific rendering before - # ordinary foreign keys can be resolved through their associations. case attribute when 'etymology' history_diff_parts(*changes.map { |attributes| Name.new(attributes).full_etymology }) @@ -59,24 +84,25 @@ def history_name_values(changes, attribute) changes.map { |kind| Name.corrigendum_kinds.dig(kind&.to_sym, :name) || kind } when 'status' changes.map { |code| Name.status_hash.dig(code, :name) || code } - else - history_association_values(changes, attribute) end end - # Match ordinary associations by their actual foreign key, which need not - # follow the association's name. Polymorphic references arrive grouped under - # the association's name as [type, id] pairs. - def history_association_values(changes, attribute) - association = Name.reflect_on_all_associations(:belongs_to).find do |reflection| + # Link foreign keys to their records, diff text, and show booleans as Yes/No. + # Ordinary associations are matched by their actual foreign key, which need + # not follow the association's name. Polymorphic references arrive grouped + # under the association's name as [type, id] pairs. + def history_attribute_values(model, changes, attribute) + association = model.reflect_on_all_associations(:belongs_to).find do |reflection| attribute == (reflection.polymorphic? ? reflection.name.to_s : reflection.foreign_key) end - if association.nil? - history_text_values(changes) - elsif association.polymorphic? + if association&.polymorphic? changes.map { |type, id| history_polymorphic_reference(type, id) } - else + elsif association changes.map { |id| history_record_reference(association.klass, id) } + elsif changes.any? { |value| value.is_a?(String) } + history_diff_parts(*changes) + else + changes.map { |value| { true => 'Yes', false => 'No' }.fetch(value, value) } end end @@ -92,18 +118,4 @@ def history_polymorphic_reference(type, id) type end end - - def history_publication_name_values(changes) - history_text_values(changes).map do |value| - case value - when true then 'Yes' - when false then 'No' - else value - end - end - end - - def history_text_values(changes) - changes.any? { |value| value.is_a?(String) } ? history_diff_parts(*changes) : changes - end end diff --git a/app/models/name/public_history.rb b/app/models/name/public_history.rb index e892a70e..16919136 100644 --- a/app/models/name/public_history.rb +++ b/app/models/name/public_history.rb @@ -9,16 +9,19 @@ class Name::PublicHistory ncbi_taxonomy lpsn_url gtdb_accession algaebase_species algaebase_taxonomy wikidata_item wikispecies_entry ].concat(Name::ETYMOLOGY_COLUMNS).freeze - # unlinked_at is shown as whether the publication is linked. + # unlinked_at is shown as whether the publication or placement is linked. PUBLICATION_NAME_KEYS = %w[unlinked_at emends not_valid_proposal].freeze + PLACEMENT_KEYS = %w[unlinked_at parent_id incertae_sedis publication_id preferred].freeze def initialize(name) @name = name end - # Load both histories and filter displayable changes in Ruby before pagination. + # Load all histories and filter displayable changes in Ruby before pagination. + # Each version's record is loaded up front to label link changes. def versions(page:) - name_versions.or(publication_versions) + name_versions.or(publication_versions).or(placement_versions) + .includes(:record) .order(created_at: :desc, id: :desc) .select { |version| changes_for(version).any? } .paginate(page: page, per_page: 25) @@ -30,16 +33,13 @@ def changes_for(version) name_changes_for(version) when 'PublicationName' publication_changes_for(version) + when 'Placement' + placement_changes_for(version) else raise ArgumentError, "Unsupported public history record type: #{version.record_type}" end end - def publication_id_for(version) - @publication_ids ||= @name.publication_names.pluck(:id, :publication_id).to_h - @publication_ids.fetch(version.record_id) - end - private def name_versions @@ -74,15 +74,49 @@ def publication_versions end def publication_changes_for(version) - return { 'linked' => [false, true] } if version.operation == 'create' - return {} unless version.operation == 'update' + link_record_changes(version, PUBLICATION_NAME_KEYS) + end + + def placement_versions + Version.where(record_type: 'Placement', record_id: @name.placements.select(:id)) + end + + def placement_changes_for(version) + link_record_changes(version, PLACEMENT_KEYS) + end + + # Linking a record is a create and unlinking is an update of unlinked_at, so + # both are shown as a change of 'linked'. A creation also lists each + # attribute it was added with that has a value: + # { 'linked' => [false, true], 'parent_id' => [nil, 2], 'incertae_sedis' => [nil, true] } + # Updates show only the changed attributes: + # { 'linked' => [true, false], 'emends' => [true, false] } + def link_record_changes(version, keys) + changes = version.changeset.slice(*keys) + + case version.operation + when 'create' + # unlinked_at is shown as 'linked'; skip attributes the link was created + # without, such as the default false roles. + added = changes.except('unlinked_at').reject { |_, (_, after)| after.blank? } + { 'linked' => [false, true] }.merge(added) + when 'update' + link_changes(changes) + else + {} + end + end - changes = version.changeset.slice(*PUBLICATION_NAME_KEYS) + # Replace an unlinked_at change with a 'linked' change listed first: + # { 'unlinked_at' => [nil, '2026-10-01T12:00:00Z'], 'emends' => [true, false] } + # => { 'linked' => [true, false], 'emends' => [true, false] } + def link_changes(changes) unlinked_at = changes.delete('unlinked_at') return changes unless unlinked_at - # A nil unlinked_at means the publication is linked. + # A nil unlinked_at means the record is linked. linked = unlinked_at.map(&:nil?) + # Changing the timestamp of an already unlinked record is not a link change. return changes if linked.first == linked.last { 'linked' => linked }.merge(changes) diff --git a/app/models/placement.rb b/app/models/placement.rb index 35231bdd..43f13a44 100644 --- a/app/models/placement.rb +++ b/app/models/placement.rb @@ -1,5 +1,6 @@ class Placement < ApplicationRecord include Unlinking + include Versioned belongs_to(:name) belongs_to( diff --git a/app/views/names/histories/show.html.erb b/app/views/names/histories/show.html.erb index 98f2ee73..0d6e154d 100644 --- a/app/views/names/histories/show.html.erb +++ b/app/views/names/histories/show.html.erb @@ -18,9 +18,9 @@ <% @versions.each do |version| %> <% changes = @history.changes_for(version) %> - <% publication_change = version.record_type == 'PublicationName' %> + <% record_change = version.record_type != 'Name' %> - <% if publication_change %> + <% if record_change %> - Publication: <%= history_record_reference(Publication, @history.publication_id_for(version)) %> + <%= history_record_heading(version) %> <% end %> <% changes.each_with_index do |(attribute, values), index| %> <% before, after = history_values(values, attribute: attribute, record_type: version.record_type) %> - <% if index.zero? && !publication_change %> + <% if index.zero? && !record_change %> <% end %> - <%= (publication_change ? PublicationName : Name).human_attribute_name(attribute) %> + <%= version.record_type.constantize.human_attribute_name(attribute) %> <%= before %> <%= after %> diff --git a/test/controllers/names/histories_controller_test.rb b/test/controllers/names/histories_controller_test.rb index 21419794..9f5c5571 100644 --- a/test/controllers/names/histories_controller_test.rb +++ b/test/controllers/names/histories_controller_test.rb @@ -36,10 +36,24 @@ class Names::HistoriesControllerTest < ActionDispatch::IntegrationTest get name_history_path(name) assert_response :success - assert_select '.name-history-version', count: 2 + # Clearing the parent also stops preferring its placement. + assert_select '.name-history-version', count: 3 assert_select '.name-history-change a[href=?]', strain_path(strains(:one)), count: 2 assert_select '.name-history-change a[href=?]', strain_path(strains(:two)), count: 1 assert_select '.name-history-change a[href=?]', name_path(names(:escherichia)), count: 1 assert_select '.name-history-change a[href=?]', publication_path(publications(:one)), count: 1 end + + test 'renders unlinked placements' do + name = names(:escherichia_coli) + Placement.create!( + name: name, parent: names(:bacteria), incertae_sedis: true, + incertae_sedis_text: 'Family placement unresolved' + ).unlink! + + get name_history_path(name) + + assert_response :success + assert_select '.name-history-record', text: /Placement:\s+incertae sedis \(Bacteria\)/, count: 2 + end end diff --git a/test/helpers/names/histories_helper_test.rb b/test/helpers/names/histories_helper_test.rb index 389e5a78..3d2b64d7 100644 --- a/test/helpers/names/histories_helper_test.rb +++ b/test/helpers/names/histories_helper_test.rb @@ -126,11 +126,47 @@ class Names::HistoriesHelperTest < ActionView::TestCase history_values([nil, publication.id], attribute: 'proposed_in_id') end - test 'preserves nonboolean publication relationship values' do - ids = [publications(:one).id, publications(:no_doi).id] + test 'renders placement parents publications and preference' do + parent = names(:escherichia) + publication = publications(:one) - assert_equal ids.map(&:to_s), history_values( - ids, attribute: 'publication_id', record_type: 'PublicationName' + assert_equal ['—', link_to(parent.display, parent)], history_values( + [nil, parent.id], attribute: 'parent_id', record_type: 'Placement' + ) + assert_equal ['—', link_to(publication.display, publication)], history_values( + [nil, publication.id], attribute: 'publication_id', record_type: 'Placement' ) + %w[linked preferred].each do |attribute| + assert_equal ['No', 'Yes'], history_values( + [false, true], attribute: attribute, record_type: 'Placement' + ) + end + end + + test 'identifies links by their publication or placement parent' do + publication = publications(:one) + parent = names(:bacteria) + parent_link = link_to(parent.display, parent) + + assert_equal "Publication: #{link_to(publication.display, publication)}", + history_link_reference(PublicationName.new(publication: publication)) + assert_equal "Placement: #{parent_link}", + history_link_reference(Placement.new(parent: parent)) + assert_equal "Placement: incertae sedis (#{parent_link})", + history_link_reference(Placement.new(parent: parent, incertae_sedis: true)) + end + + test 'labels link changes with the record they were made to' do + link = PublicationName.create!(name: names(:escherichia_coli), publication: publications(:one)) + + assert_equal history_link_reference(link), history_record_heading(link.versions.last) + end + + test 'labels link changes whose record was deleted' do + link = PublicationName.create!(name: names(:escherichia_coli), publication: publications(:one)) + version = link.versions.last + link.destroy! + + assert_equal 'Publication name (deleted)', history_record_heading(version.reload) end end diff --git a/test/models/name/public_history_test.rb b/test/models/name/public_history_test.rb index 79efd1ff..9225d714 100644 --- a/test/models/name/public_history_test.rb +++ b/test/models/name/public_history_test.rb @@ -130,17 +130,6 @@ class Name::PublicHistoryTest < ActiveSupport::TestCase ] }, @history.changes_for(@name.versions.last)) end - test 'identifies the publication for each relationship history including unlinks' do - first_link = PublicationName.create!(name: @name, publication: publications(:one)) - second_link = PublicationName.create!(name: @name, publication: publications(:no_doi)) - first_link.unlink! - - first_link.versions.each do |version| - assert_equal publications(:one).id, @history.publication_id_for(version) - end - assert_equal publications(:no_doi).id, @history.publication_id_for(second_link.versions.last) - end - test 'shows publication role additions and removals alongside unlink changes' do link = PublicationName.create!(name: @name, publication: publications(:one)) link.update!(emends: true, not_valid_proposal: true) @@ -177,4 +166,45 @@ class Name::PublicHistoryTest < ActiveSupport::TestCase assert_equal({ 'emends' => [false, true] }, @history.changes_for(version)) end + + test 'tracks placements through their addition changes and unlinking' do + placement = Placement.create!( + name: @name, parent: names(:bacteria), incertae_sedis: true, + incertae_sedis_text: 'Family placement unresolved' + ) + placement.update!(publication: publications(:one)) + placement.unlink! + + versions = @history.versions(page: 1) + + assert_equal [ + { 'linked' => [true, false] }, + { 'publication_id' => [nil, publications(:one).id] }, + { 'linked' => [false, true], + 'parent_id' => [nil, names(:bacteria).id], + 'incertae_sedis' => [nil, true] } + ], versions.map { |version| @history.changes_for(version) } + end + + test 'includes changes to placements recorded before their creation was tracked' do + placements(:escherichia_coli).update!(publication: publications(:one)) + + assert_equal [{ 'publication_id' => [nil, publications(:one).id] }], + @history.versions(page: 1).map { |version| @history.changes_for(version) } + end + + test 'excludes placements of other names' do + Placement.create!(name: names(:escherichia_colie), parent: names(:escherichia)) + + assert_empty @history.versions(page: 1) + end + + test 'shows publication roles a link was created with' do + link = PublicationName.create!( + name: @name, publication: publications(:one), emends: true + ) + + assert_equal({ 'linked' => [false, true], 'emends' => [nil, true] }, + @history.changes_for(link.versions.last)) + end end From 1f881c1cfc9989317ec78bdf36951c580b5e20a8 Mon Sep 17 00:00:00 2001 From: vangberg Date: Tue, 20 Oct 2026 15:52:26 +0200 Subject: [PATCH 27/41] Version rich text records and show Name rich text in public history --- app/helpers/names/histories_helper.rb | 8 ++++ app/models/concerns/versioned.rb | 19 +++++++-- app/models/name/public_history.rb | 27 +++++++++++- app/views/names/histories/show.html.erb | 7 ++-- config/initializers/action_text_versions.rb | 4 ++ .../names/histories_controller_test.rb | 12 ++++++ .../action_text/rich_text_versioning_test.rb | 42 +++++++++++++++++++ test/models/name/public_history_test.rb | 9 ++++ 8 files changed, 121 insertions(+), 7 deletions(-) create mode 100644 config/initializers/action_text_versions.rb create mode 100644 test/models/action_text/rich_text_versioning_test.rb diff --git a/app/helpers/names/histories_helper.rb b/app/helpers/names/histories_helper.rb index bddfae1a..86e45a99 100644 --- a/app/helpers/names/histories_helper.rb +++ b/app/helpers/names/histories_helper.rb @@ -10,6 +10,11 @@ def history_values(changes, attribute:, record_type: 'Name') values.map { |value| safe_join(Array(value)).presence || '—' } end + # Rich text changes are shown as changes of the name itself. + def history_record_type(version) + version.record_type == 'ActionText::RichText' ? 'Name' : version.record_type + end + def history_record_reference(record_class, id) return unless id @@ -75,6 +80,9 @@ def history_diff_parts(before, after) # Name attributes that need domain-specific rendering, or nil for the rest. def history_name_values(changes, attribute) case attribute + when 'description', 'etymology_text' + # Recorded as HTML; diff the readable text. + history_diff_parts(*changes.map { |html| html && ActionText::Content.new(html).to_plain_text }) when 'etymology' history_diff_parts(*changes.map { |attributes| Name.new(attributes).full_etymology }) when 'genome_id' diff --git a/app/models/concerns/versioned.rb b/app/models/concerns/versioned.rb index 887bbaa9..ed916bc8 100644 --- a/app/models/concerns/versioned.rb +++ b/app/models/concerns/versioned.rb @@ -19,8 +19,21 @@ def versioned_together(*attributes) private + def versioned_attributes + attributes.transform_values { |value| versioned_value(value) } + end + + def versioned_saved_changes + saved_changes.transform_values { |values| values.map { |value| versioned_value(value) } } + end + + # A rich text's body is a Content object; record its HTML. + def versioned_value(value) + value.is_a?(ActionText::Content) ? value.to_html.presence : value + end + def record_create_version - versions.create!(operation: 'create', changeset: attributes.transform_values { |value| [nil, value] }) + versions.create!(operation: 'create', changeset: versioned_attributes.transform_values { |value| [nil, value] }) end def record_update_version @@ -33,7 +46,7 @@ def record_delete_version # its association. Retain the final attributes and history. Version.create!( record: self, operation: 'delete', - changeset: attributes.transform_values { |value| [value, nil] } + changeset: versioned_attributes.transform_values { |value| [value, nil] } ) end @@ -44,7 +57,7 @@ def record_delete_version # { 'rank' => ['species', nil] } on delete. # Only update changesets omit timestamps. def versioned_changes - changeset = saved_changes.except('created_at', 'updated_at') + changeset = versioned_saved_changes.except('created_at', 'updated_at') # A polymorphic ID is only meaningful together with its record type. polymorphic_sets = self.class.reflect_on_all_associations(:belongs_to) diff --git a/app/models/name/public_history.rb b/app/models/name/public_history.rb index 16919136..a650f3ef 100644 --- a/app/models/name/public_history.rb +++ b/app/models/name/public_history.rb @@ -11,6 +11,8 @@ class Name::PublicHistory ].concat(Name::ETYMOLOGY_COLUMNS).freeze # unlinked_at is shown as whether the publication or placement is linked. PUBLICATION_NAME_KEYS = %w[unlinked_at emends not_valid_proposal].freeze + # Only these rich texts of the name appear in the public history. + RICH_TEXT_NAMES = %w[description etymology_text].freeze PLACEMENT_KEYS = %w[unlinked_at parent_id incertae_sedis publication_id preferred].freeze def initialize(name) @@ -20,7 +22,7 @@ def initialize(name) # Load all histories and filter displayable changes in Ruby before pagination. # Each version's record is loaded up front to label link changes. def versions(page:) - name_versions.or(publication_versions).or(placement_versions) + name_versions.or(publication_versions).or(placement_versions).or(rich_text_versions) .includes(:record) .order(created_at: :desc, id: :desc) .select { |version| changes_for(version).any? } @@ -35,6 +37,8 @@ def changes_for(version) publication_changes_for(version) when 'Placement' placement_changes_for(version) + when 'ActionText::RichText' + rich_text_changes_for(version) else raise ArgumentError, "Unsupported public history record type: #{version.record_type}" end @@ -67,6 +71,27 @@ def name_changes_for(version) changes end + def rich_text_versions + Version.where( + record_type: 'ActionText::RichText', + record_id: ActionText::RichText.where( + record: @name, name: RICH_TEXT_NAMES + ).select(:id) + ) + end + + # A rich text version is shown as a change of the name's attribute it holds, + # found in the changeset of a created or deleted rich text, or else on the + # rich text itself: + # { 'description' => [nil, '
A gut bacterium
'] } + def rich_text_changes_for(version) + body = version.changeset['body'] + attribute = version.changeset['name']&.compact&.first || version.record&.name + return {} unless body && body.first != body.last && RICH_TEXT_NAMES.include?(attribute) + + { attribute => body } + end + def publication_versions Version.where( record_type: 'PublicationName', record_id: @name.publication_names.select(:id) diff --git a/app/views/names/histories/show.html.erb b/app/views/names/histories/show.html.erb index 0d6e154d..a5b6624c 100644 --- a/app/views/names/histories/show.html.erb +++ b/app/views/names/histories/show.html.erb @@ -18,7 +18,8 @@ <% @versions.each do |version| %> <% changes = @history.changes_for(version) %> - <% record_change = version.record_type != 'Name' %> + <% record_type = history_record_type(version) %> + <% record_change = record_type != 'Name' %> <% if record_change %> @@ -33,7 +34,7 @@ <% end %> <% changes.each_with_index do |(attribute, values), index| %> - <% before, after = history_values(values, attribute: attribute, record_type: version.record_type) %> + <% before, after = history_values(values, attribute: attribute, record_type: record_type) %> <% if index.zero? && !record_change %> @@ -42,7 +43,7 @@ <% end %> - <%= version.record_type.constantize.human_attribute_name(attribute) %> + <%= record_type.constantize.human_attribute_name(attribute) %> <%= before %> <%= after %> diff --git a/config/initializers/action_text_versions.rb b/config/initializers/action_text_versions.rb new file mode 100644 index 00000000..3027b2fb --- /dev/null +++ b/config/initializers/action_text_versions.rb @@ -0,0 +1,4 @@ +# Rich text lives in its own records, so version them like any other record. +Rails.application.config.to_prepare do + ActionText::RichText.include(Versioned) +end diff --git a/test/controllers/names/histories_controller_test.rb b/test/controllers/names/histories_controller_test.rb index 9f5c5571..3f08d26f 100644 --- a/test/controllers/names/histories_controller_test.rb +++ b/test/controllers/names/histories_controller_test.rb @@ -12,6 +12,18 @@ class Names::HistoriesControllerTest < ActionDispatch::IntegrationTest assert_select '.name-history-version', count: 2 end + test 'renders description edits as changes of the name' do + name = names(:escherichia_coli) + name.update!(description: 'A gut bacterium') + name.reload.update!(description: 'A commensal gut bacterium') + + get name_history_path(name) + + assert_response :success + assert_select '.name-history-version', count: 2 + assert_select '.name-history-after ins', text: /commensal/ + end + test 'does not reveal private name history' do name = Name.create!( name: 'E. coli', rank: 'species', status: 5, created_by: users(:contributor) diff --git a/test/models/action_text/rich_text_versioning_test.rb b/test/models/action_text/rich_text_versioning_test.rb new file mode 100644 index 00000000..e6ae9b57 --- /dev/null +++ b/test/models/action_text/rich_text_versioning_test.rb @@ -0,0 +1,42 @@ +require 'test_helper' + +class ActionText::RichTextVersioningTest < ActiveSupport::TestCase + setup do + @name = Name.create!(name: 'E. coli', rank: 'species', description: 'A gut bacterium') + @rich_text = @name.rich_text_description + end + + test 'creating a rich text records its body as HTML' do + version = @rich_text.versions.first + + assert_equal 'create', version.operation + assert_nil version.changeset['body'].first + assert_includes version.changeset['body'].last, 'A gut bacterium' + assert_equal [nil, 'description'], version.changeset['name'] + end + + test 'editing a rich text records its before and after HTML' do + assert_difference -> { @rich_text.versions.count } do + @name.reload.update!(description: 'A commensal gut bacterium') + end + + changeset = @rich_text.versions.last.changeset + assert_equal %w[body], changeset.keys + assert_includes changeset['body'].first, 'A gut bacterium' + assert_includes changeset['body'].last, 'A commensal gut bacterium' + end + + test 'resaving a rich text unchanged does not add a version' do + assert_no_difference -> { @rich_text.versions.count } do + @name.reload.update!(description: 'A gut bacterium') + end + end + + test 'deleting a name retains its rich text history' do + @name.destroy! + + version = Version.where(record_type: 'ActionText::RichText', record_id: @rich_text.id).last + assert_equal 'delete', version.operation + assert_includes version.changeset['body'].first, 'A gut bacterium' + end +end diff --git a/test/models/name/public_history_test.rb b/test/models/name/public_history_test.rb index 9225d714..47df5aa1 100644 --- a/test/models/name/public_history_test.rb +++ b/test/models/name/public_history_test.rb @@ -30,6 +30,15 @@ class Name::PublicHistoryTest < ActiveSupport::TestCase }, @history.changes_for(version)) end + test 'shows description and etymology text edits as name changes' do + @name.update!(description: 'A gut bacterium', etymology_text: 'From the colon', notes: 'Internal') + @name.reload.update!(description: 'A commensal gut bacterium') + + changes = @history.versions(page: 1).flat_map { |version| @history.changes_for(version).keys } + + assert_equal %w[description description etymology_text], changes.sort + end + test 'combines publication links unlinks and relinks with name updates newest first' do publication = publications(:one) start = Time.zone.local(2026, 10, 1, 12) From 48fd7219fc18d94e68ff5586a05e7a3dbf93776a Mon Sep 17 00:00:00 2001 From: vangberg Date: Tue, 20 Oct 2026 16:07:21 +0200 Subject: [PATCH 28/41] Version rich text records and show Name rich text diffs in public history --- app/helpers/names/histories_helper.rb | 15 ++++++- lib/html_diff.rb | 45 +++++++++++++++++++++ test/helpers/names/histories_helper_test.rb | 10 +++++ test/lib/html_diff_test.rb | 29 +++++++++++++ 4 files changed, 97 insertions(+), 2 deletions(-) create mode 100644 lib/html_diff.rb create mode 100644 test/lib/html_diff_test.rb diff --git a/app/helpers/names/histories_helper.rb b/app/helpers/names/histories_helper.rb index 86e45a99..bc126872 100644 --- a/app/helpers/names/histories_helper.rb +++ b/app/helpers/names/histories_helper.rb @@ -75,14 +75,25 @@ def history_diff_parts(before, after) [before_parts, after_parts] end + # Diff rich text HTML, rendering each side as formatted text with its changes + # marked. The HTML is sanitized as ActionText content is, plus ins and del. + def history_html_diff(before, after) + HtmlDiff.diff(before, after).map do |html| + sanitize( + html, + tags: ActionText::ContentHelper.allowed_tags + %w[ins del], + attributes: ActionText::ContentHelper.allowed_attributes + ) + end + end + private # Name attributes that need domain-specific rendering, or nil for the rest. def history_name_values(changes, attribute) case attribute when 'description', 'etymology_text' - # Recorded as HTML; diff the readable text. - history_diff_parts(*changes.map { |html| html && ActionText::Content.new(html).to_plain_text }) + history_html_diff(*changes) when 'etymology' history_diff_parts(*changes.map { |attributes| Name.new(attributes).full_etymology }) when 'genome_id' diff --git a/lib/html_diff.rb b/lib/html_diff.rb new file mode 100644 index 00000000..c5b82496 --- /dev/null +++ b/lib/html_diff.rb @@ -0,0 +1,45 @@ +require 'diff/lcs' + +module HtmlDiff + TOKEN = /<[^>]*>|\s+|[^<\s]+/ + TAG = /\A 'text-danger', 'ins' => 'text-success' }.freeze + + # Returns [before_html, after_html]: each side's own HTML with its changed + # text wrapped in or . Tags are compared but never wrapped, so each + # side keeps its own well-formed markup. The result is not sanitized. For an + # E. coli description edit, '
A gut bacterium
' -> + # '
A commensal gut bacterium
' gives + # ['
A gut bacterium
', '
A commensal gut bacterium
']. + def self.diff(before, after) + changes = Diff::LCS.sdiff(tokens(before), tokens(after)) + [ + render(changes.filter_map { |c| [c.old_element, c.action != '='] if c.old_element }, 'del'), + render(changes.filter_map { |c| [c.new_element, c.action != '='] if c.new_element }, 'ins') + ] + end + + def self.tokens(html) + html.to_s.scan(TOKEN) + end + + # Wrap each run of changed text tokens, absorbing unchanged whitespace between + # two changed runs so an edited phrase is marked as one. + def self.render(tokens, tag) + runs = tokens.chunk_while { |a, b| a[1] == b[1] && text?(a[0]) == text?(b[0]) }.to_a + runs.each_with_index do |run, index| + next unless index.positive? && index < runs.size - 1 && run.all? { |t, changed| !changed && t.match?(/\A\s+\z/) } + + before, after = runs[index - 1], runs[index + 1] + run.each { |token| token[1] = true } if before.all? { |t, c| c && text?(t) } && after.all? { |t, c| c && text?(t) } + end + runs.flatten(1).chunk_while { |a, b| a[1] == b[1] && text?(a[0]) == text?(b[0]) }.map do |run| + html = run.map(&:first).join + run.first[1] && text?(run.first[0]) && html.match?(/\S/) ? %(<#{tag} class="#{CLASSES[tag]}">#{html}) : html + end.join + end + + def self.text?(token) + !token.match?(TAG) + end +end diff --git a/test/helpers/names/histories_helper_test.rb b/test/helpers/names/histories_helper_test.rb index 3d2b64d7..c29e1aec 100644 --- a/test/helpers/names/histories_helper_test.rb +++ b/test/helpers/names/histories_helper_test.rb @@ -12,6 +12,16 @@ class Names::HistoriesHelperTest < ActionView::TestCase assert_equal 'coli, from the colon', after end + test 'diffs rich text as sanitized HTML' do + changes = ['
A gut bacterium
', + '
A gut bacterium E. coli
'] + before, after = history_values(changes, attribute: 'description') + + assert_equal '
A gut bacterium
', before + assert_not_includes after, '