From d80ea8d7d47171c7eda8e136a341219c99dfae09 Mon Sep 17 00:00:00 2001 From: vangberg Date: Fri, 2 Oct 2026 10:30:27 +0200 Subject: [PATCH 1/2] 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 2/2] 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