From 8abca25e049cf56f72d2c2af9be24f0f43141b69 Mon Sep 17 00:00:00 2001 From: maebeale Date: Sat, 4 Jul 2026 22:43:31 -0400 Subject: [PATCH 1/2] Share author credit resolution across creditable models MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Move author display, credited-name search, and author sort into AuthorCreditable so Workshop/Resource/Story/CommunityNews/WorkshopVariation (and the *_idea models) behave identically. Credit resolves through a single precedence — explicit author person, legacy free-text/user author, creating user's person, then an overridable missing_author_label (Workshop shows "Facilitator"). Default author_credit_preference to full_name in the UI and treat a blank preference as full_name in logic, with no data backfill. Rename the legacy community_news author column to legacy_author_user_id. Co-Authored-By: Claude Opus 4.8 (1M context) --- app/models/community_news.rb | 24 ++- app/models/concerns/author_creditable.rb | 158 ++++++++++++++++-- app/models/resource.rb | 18 +- app/models/story.rb | 24 +-- app/models/story_idea.rb | 1 - app/models/workshop.rb | 15 ++ app/models/workshop_variation.rb | 7 +- app/models/workshop_variation_idea.rb | 1 - ...022703_standardize_author_credit_fields.rb | 24 +++ db/schema.rb | 8 +- spec/models/community_news_spec.rb | 8 +- spec/models/workshop_variation_spec.rb | 3 +- .../shared_examples/author_creditable.rb | 54 +++++- 13 files changed, 287 insertions(+), 58 deletions(-) create mode 100644 db/migrate/20260705022703_standardize_author_credit_fields.rb diff --git a/app/models/community_news.rb b/app/models/community_news.rb index 39b66d070a..1be866ae3c 100644 --- a/app/models/community_news.rb +++ b/app/models/community_news.rb @@ -8,7 +8,7 @@ class CommunityNews < ApplicationRecord belongs_to :author, class_name: "Person", inverse_of: :community_news_as_author, optional: true # Legacy "display author" pick, before author became a person. Kept so # existing rows credit the chosen person without a backfill. - belongs_to :user_author, class_name: "User", foreign_key: :user_author_id, optional: true + belongs_to :legacy_author_user, class_name: "User", foreign_key: :legacy_author_user_id, optional: true belongs_to :created_by, class_name: "User" belongs_to :updated_by, class_name: "User" has_many :bookmarks, as: :bookmarkable, dependent: :destroy @@ -34,10 +34,16 @@ class CommunityNews < ApplicationRecord validates :rhino_body, presence: true # Credit the explicit person author, then the legacy display-author user's - # person, then the creating user's person — so existing rows keep their - # attribution without a backfill. Overrides AuthorCreditable's default. - def author_person - author || user_author&.person || created_by&.person + # person (the creating user's person is AuthorCreditable's final fallback) — + # so existing rows keep their attribution without a backfill. + def primary_author_person + author || legacy_author_user&.person + end + + # Fold the legacy display-author user's person into credited-name search and + # sort, matching author_person's precedence. + def self.legacy_credited_user_columns + [ [ "legacy_author_user_id", "credited_legacy_author" ] ] end # Nested attributes @@ -74,6 +80,14 @@ def self.search_by_params(params) community_news = self.search(conditions) end + # SearchCop's free-text query covers title + body + the explicit author. Also + # match the credited author/legacy author/creator by name, OR-ed in via id + # subqueries so the extra person joins stay isolated from SearchCop's joins. + if params[:query].present? + community_news = self.where(id: community_news.select("community_news.id")) + .or(self.where(id: by_credited_person_name(params[:query]).select("community_news.id"))) + end + community_news = community_news.by_year(params[:year]) if params[:year].present? && params[:year].match?(/\A\d{4}\z/) community_news = community_news.sector_names_all(params[:sector_names_all]) if params[:sector_names_all].present? community_news = community_news.category_names_all(params[:category_names_all]) if params[:category_names_all].present? diff --git a/app/models/concerns/author_creditable.rb b/app/models/concerns/author_creditable.rb index 8988788ee9..37543b6f16 100644 --- a/app/models/concerns/author_creditable.rb +++ b/app/models/concerns/author_creditable.rb @@ -19,24 +19,160 @@ module AuthorCreditable "Anonymous" => "anonymous" }.freeze - # The person whose name is credited: the explicitly chosen author when the - # model has one (e.g. Story, Workshop), otherwise the creating user's person. + included do + # Default to the full name in the UI (new records and unset form fields). + # Existing rows with no preference are treated as "full_name" — at read time + # by `author_credit` and, on their next save, normalized by the callback below. + # There is no data backfill. + attribute :author_credit_preference, :string, default: "full_name" + before_validation :default_author_credit_preference + validates :author_credit_preference, inclusion: { in: AUTHOR_CREDIT_PREFERENCES } + end + + # The linkable credited person, in precedence order: the explicit/legacy author + # person, otherwise the creating user's person. This is what views link to. def author_person - (author if respond_to?(:author)) || created_by&.person + primary_author_person || created_by&.person + end + + # The credited author person that is *not* the creator fallback — the explicit + # author. Overridden by models with an additional legacy person tier (e.g. + # CommunityNews folds in its legacy_author_user's person). + def primary_author_person + author if respond_to?(:author) end + # A legacy free-text author name (no linkable person), ranked between the + # explicit author and the creator. Overridden by models that have one + # (Workshop, Resource). + def legacy_author_name_text + nil + end + + # Display string for the credited author, honoring the credit preference. + # Precedence: an explicit "anonymous" preference always renders "Anonymous"; + # then the primary author person, then the legacy free-text name, then the + # creating user's person, then `missing_author_label`. def author_credit - person = author_person + return "Anonymous" if author_credit_preference == "anonymous" + person = primary_author_person + return format_person_credit(person) if person + return legacy_author_name_text if legacy_author_name_text.present? + format_person_credit(created_by&.person) + end + + # Shown when there is no credited person or legacy name. Overridable per model + # (e.g. Workshop shows "Facilitator"). + def missing_author_label + "Anonymous" + end + + # Treat an unset preference as "full_name" so legacy rows normalize on save + # without a bulk backfill. + def default_author_credit_preference + self.author_credit_preference = "full_name" if author_credit_preference.blank? + end + + # Formats a person's name per the credit preference, falling back to + # `missing_author_label` when the person or the requested name part is missing. + private def format_person_credit(person) case author_credit_preference - when "full_name" then person&.full_name || "Anonymous" when "first_name_last_initial" first = person&.first_name - last_initial = person&.last_name&.first - first.present? ? "#{first} #{last_initial}." : "Anonymous" - when "first_name_only" then person&.first_name || "Anonymous" - when "last_name_only" then person&.last_name || "Anonymous" - when "anonymous" then "Anonymous" - else person&.name || "Anonymous" + first.present? ? "#{first} #{person.last_name&.first}." : missing_author_label + when "first_name_only" + person&.first_name.presence || missing_author_label + when "last_name_only" + person&.last_name.presence || missing_author_label + else # full_name — the default, and the fallback for any unknown value + person&.full_name.presence || missing_author_label + end + end + + class_methods do + # Legacy free-text columns (fully qualified, e.g. "resources.legacy_author_name") + # that also hold an author's name. Overridden per model that has one. + def legacy_author_name_columns + [] + end + + # Legacy "belongs_to a User" columns whose person should also be credited, + # as [ foreign_key_column, sql_alias ] pairs. Overridden per model (e.g. + # CommunityNews folds in its legacy_author_user). Alias must be SQL-safe. + def legacy_credited_user_columns + [] + end + + # Records whose credited author's name resembles `query`: the explicit author + # person, the creating user's person, plus any legacy sources the model folds + # in. Uses explicit LEFT JOIN aliases so it composes safely — SearchCop can't + # join `people` more than once, so callers OR this into full-text results via + # an id subquery. + def by_credited_person_name(query) + sanitized = query.to_s.strip.gsub(/\s+/, "") + return none if sanitized.blank? + + clauses = credited_person_aliases.flat_map { |a| person_name_match_clauses(a) } + clauses += legacy_author_name_columns.map { |col| "LOWER(REPLACE(#{col}, ' ', '')) LIKE :name" } + joins(credited_person_join_sql).where(clauses.join(" OR "), name: "%#{sanitized}%") + end + + # Orders by the credited author's name, matching `author_person` precedence: + # explicit author, then any legacy sources, then the creating user's person. + def order_by_author(direction) + ascending = direction.to_s.casecmp("asc").zero? + joins(credited_person_join_sql) + .reorder(coalesced_author_arel(:last_name, ascending), coalesced_author_arel(:first_name, ascending)) + end + + private + + # Person SQL aliases in credit precedence order. + def credited_person_aliases + aliases = [] + aliases << "credited_author" if column_names.include?("author_id") + legacy_credited_user_columns.each { |_, sql_alias| aliases << sql_alias } + aliases << "credited_creator" + aliases + end + + # Explicit LEFT JOINs (as raw SQL strings with unique aliases) reaching every + # person that can be credited, so the aliases never collide with SearchCop's + # or Rails' own joins. + def credited_person_join_sql + sql = [] + if column_names.include?("author_id") + sql << "LEFT OUTER JOIN people credited_author ON credited_author.id = #{table_name}.author_id" + end + legacy_credited_user_columns.each do |fk_column, sql_alias| + sql << "LEFT OUTER JOIN users #{sql_alias}_user ON #{sql_alias}_user.id = #{table_name}.#{fk_column}" + sql << "LEFT OUTER JOIN people #{sql_alias} ON #{sql_alias}.id = #{sql_alias}_user.person_id" + end + sql << "LEFT OUTER JOIN users credited_creator_user ON credited_creator_user.id = #{table_name}.created_by_id" + sql << "LEFT OUTER JOIN people credited_creator ON credited_creator.id = credited_creator_user.person_id" + sql + end + + # Arel COALESCE over every credited person alias (and legacy name column), + # so the ORDER BY carries no interpolated SQL. Aliases and column names come + # from model config / column_names, never user input. + def coalesced_author_arel(field, ascending) + parts = credited_person_aliases.map { |sql_alias| Arel::Table.new(sql_alias)[field] } + parts += legacy_author_name_columns.map do |col| + table, column = col.split(".") + Arel::Table.new(table)[column] + end + node = Arel::Nodes::NamedFunction.new("COALESCE", parts) + ascending ? node.asc : node.desc + end + + def person_name_match_clauses(sql_alias) + [ + "LOWER(REPLACE(CONCAT(#{sql_alias}.first_name, #{sql_alias}.last_name), ' ', '')) LIKE :name", + "LOWER(REPLACE(CONCAT(#{sql_alias}.last_name, #{sql_alias}.first_name), ' ', '')) LIKE :name", + "LOWER(REPLACE(#{sql_alias}.first_name, ' ', '')) LIKE :name", + "LOWER(REPLACE(#{sql_alias}.last_name, ' ', '')) LIKE :name" + ] end end end diff --git a/app/models/resource.rb b/app/models/resource.rb index a06afba9db..3bed0f440c 100644 --- a/app/models/resource.rb +++ b/app/models/resource.rb @@ -83,6 +83,16 @@ def self.mentionable_rich_text_fields options :action_text_body, type: :text, default: true, default_operator: :or end + # Fold the legacy free-text author into credit display, credited-name search, + # and sort. + def self.legacy_author_name_columns + [ "resources.legacy_author_name" ] + end + + def legacy_author_name_text + legacy_author_name + end + # Scopes scope :by_created, -> { order(created_at: :desc) } scope :by_featured_first, -> { order(featured: :desc, created_at: :desc) } @@ -107,7 +117,13 @@ def self.mentionable_rich_text_fields def self.search_by_params(params) resources = is_a?(ActiveRecord::Relation) ? self : all - resources = resources.search(params[:query]) if params[:query].present? # SearchCop incl title, author, body + if params[:query].present? + # SearchCop covers title + legacy author name + body; OR in the credited + # author/creator person name via id subqueries (isolated person joins). + by_text = resources.search(params[:query]).select("resources.id") + by_person = resources.by_credited_person_name(params[:query]).select("resources.id") + resources = resources.where(id: by_text).or(resources.where(id: by_person)) + end resources = resources.sector_names_all(params[:sector_names_all]) if params[:sector_names_all].present? resources = resources.category_names_all(params[:category_names_all]) if params[:category_names_all].present? resources = resources.windows_type_name(params[:windows_type_name]) if params[:windows_type_name].present? diff --git a/app/models/story.rb b/app/models/story.rb index 9457204cd5..1e1321ce22 100644 --- a/app/models/story.rb +++ b/app/models/story.rb @@ -54,28 +54,8 @@ class Story < ApplicationRecord options :action_text_body, type: :text, default: true, default_operator: :or end - # Matches the credited author by name: the creator's person (`people`) and the - # explicit author (a second join to the same table, aliased `author_person`). - # SearchCop can't join one table twice, so this is a plain scope OR-ed into the - # full-text results in `search_by_params`. - scope :by_credited_person_name, ->(query) { - sanitized = query.to_s.strip.gsub(/\s+/, "") - return none if sanitized.blank? - - left_joins(created_by: :person) - .joins("LEFT OUTER JOIN people author_person ON author_person.id = stories.author_id") - .where( - "LOWER(REPLACE(CONCAT(people.first_name, people.last_name), ' ', '')) LIKE :name - OR LOWER(REPLACE(CONCAT(people.last_name, people.first_name), ' ', '')) LIKE :name - OR LOWER(REPLACE(people.first_name, ' ', '')) LIKE :name - OR LOWER(REPLACE(people.last_name, ' ', '')) LIKE :name - OR LOWER(REPLACE(CONCAT(author_person.first_name, author_person.last_name), ' ', '')) LIKE :name - OR LOWER(REPLACE(CONCAT(author_person.last_name, author_person.first_name), ' ', '')) LIKE :name - OR LOWER(REPLACE(author_person.first_name, ' ', '')) LIKE :name - OR LOWER(REPLACE(author_person.last_name, ' ', '')) LIKE :name", - name: "%#{sanitized}%" - ) - } + # Credited-author name search (explicit author + creator fallback) comes from + # AuthorCreditable#by_credited_person_name, OR-ed into full-text results below. # Scopes # See Featureable, Publishable, TagFilterable, Trendable, WindowsTypeFilterable, RichTextSearchable diff --git a/app/models/story_idea.rb b/app/models/story_idea.rb index 65155be0bb..923963a66b 100644 --- a/app/models/story_idea.rb +++ b/app/models/story_idea.rb @@ -41,7 +41,6 @@ def self.search_by_params(params) validates :organization_id, presence: true validates :windows_type_id, presence: true validates :permission_given, presence: true - validates :author_credit_preference, presence: true validates :rhino_body, presence: true # Nested attributes diff --git a/app/models/workshop.rb b/app/models/workshop.rb index f6d461e52b..27a3260ed4 100644 --- a/app/models/workshop.rb +++ b/app/models/workshop.rb @@ -185,6 +185,21 @@ def self.grouped_by_sector Sector.all.map { |sector| Hash[sector.name, sector.workshops] }.flatten end + # Legacy free-text author name (pre-person authors), still named `full_name`. + # Folded into credit display, credited-name search, and sort. + def self.legacy_author_name_columns + [ "workshops.full_name" ] + end + + def legacy_author_name_text + full_name + end + + # With no credited person or legacy name, attribute to the generic facilitator. + def missing_author_label + "Facilitator" + end + def author_name author_person&.full_name.presence || full_name.presence end diff --git a/app/models/workshop_variation.rb b/app/models/workshop_variation.rb index 965c2b0f27..f87226112b 100644 --- a/app/models/workshop_variation.rb +++ b/app/models/workshop_variation.rb @@ -13,7 +13,11 @@ class WorkshopVariation < ApplicationRecord def self.search_by_params(params) results = is_a?(ActiveRecord::Relation) ? self : all - results = results.search(params[:query]) if params[:query].present? + if params[:query].present? + by_text = results.search(params[:query]).select("workshop_variations.id") + by_person = results.by_credited_person_name(params[:query]).select("workshop_variations.id") + results = results.where(id: by_text).or(results.where(id: by_person)) + end results = results.where(author_id: params[:author_id]) if params[:author_id].present? results end @@ -40,7 +44,6 @@ def self.search_by_params(params) validates :name, presence: true, uniqueness: { scope: :workshop_id, case_sensitive: false } validates :windows_type_id, presence: true - validates :author_credit_preference, presence: true validates :rhino_body, presence: true accepts_nested_attributes_for :primary_asset, allow_destroy: true, reject_if: :all_blank diff --git a/app/models/workshop_variation_idea.rb b/app/models/workshop_variation_idea.rb index cabf89496d..378425bbef 100644 --- a/app/models/workshop_variation_idea.rb +++ b/app/models/workshop_variation_idea.rb @@ -36,7 +36,6 @@ def self.search_by_params(params) validates :organization_id, presence: true validates :workshop_id, presence: true validates :windows_type_id, presence: true - validates :author_credit_preference, presence: true validates :permission_given, acceptance: true validates :rhino_body, presence: true diff --git a/db/migrate/20260705022703_standardize_author_credit_fields.rb b/db/migrate/20260705022703_standardize_author_credit_fields.rb new file mode 100644 index 0000000000..0b75f86d93 --- /dev/null +++ b/db/migrate/20260705022703_standardize_author_credit_fields.rb @@ -0,0 +1,24 @@ +class StandardizeAuthorCreditFields < ActiveRecord::Migration[8.1] + # The community news "display author" is a legacy pick (a User) that predates + # person authors; name it as legacy so it reads like resources.legacy_author_name. + # author_credit_preference is defaulted to "full_name" in the UI and in + # AuthorCreditable (attribute default + author_credit fallback), not by a data + # backfill, so there's no column change here. + def up + if column_exists?(:community_news, :user_author_id) && !column_exists?(:community_news, :legacy_author_user_id) + rename_column :community_news, :user_author_id, :legacy_author_user_id + end + if index_name_exists?(:community_news, "index_community_news_on_user_author_id") + rename_index :community_news, "index_community_news_on_user_author_id", "index_community_news_on_legacy_author_user_id" + end + end + + def down + if index_name_exists?(:community_news, "index_community_news_on_legacy_author_user_id") + rename_index :community_news, "index_community_news_on_legacy_author_user_id", "index_community_news_on_user_author_id" + end + if column_exists?(:community_news, :legacy_author_user_id) && !column_exists?(:community_news, :user_author_id) + rename_column :community_news, :legacy_author_user_id, :user_author_id + end + end +end diff --git a/db/schema.rb b/db/schema.rb index 344a94cbd2..4ba40ef337 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[8.1].define(version: 2026_07_05_014631) do +ActiveRecord::Schema[8.1].define(version: 2026_07_05_022703) do create_table "action_text_mentions", charset: "utf8mb4", collation: "utf8mb4_unicode_ci", force: :cascade do |t| t.bigint "action_text_rich_text_id", null: false t.datetime "created_at", null: false @@ -390,6 +390,7 @@ t.datetime "created_at", null: false t.integer "created_by_id", null: false t.boolean "featured" + t.integer "legacy_author_user_id" t.integer "organization_id" t.boolean "publicly_featured", default: false, null: false t.boolean "publicly_visible", default: false, null: false @@ -398,15 +399,14 @@ t.string "title" t.datetime "updated_at", null: false t.integer "updated_by_id", null: false - t.integer "user_author_id" t.integer "windows_type_id" t.string "youtube_url" t.index ["author_id"], name: "index_community_news_on_author_id" t.index ["body"], name: "index_community_news_on_body", type: :fulltext t.index ["created_by_id"], name: "index_community_news_on_created_by_id" + t.index ["legacy_author_user_id"], name: "index_community_news_on_legacy_author_user_id" t.index ["organization_id"], name: "index_community_news_on_organization_id" t.index ["updated_by_id"], name: "index_community_news_on_updated_by_id" - t.index ["user_author_id"], name: "index_community_news_on_user_author_id" t.index ["windows_type_id"], name: "index_community_news_on_windows_type_id" end @@ -1696,8 +1696,8 @@ add_foreign_key "community_news", "organizations" add_foreign_key "community_news", "people", column: "author_id" add_foreign_key "community_news", "users", column: "created_by_id" + add_foreign_key "community_news", "users", column: "legacy_author_user_id" add_foreign_key "community_news", "users", column: "updated_by_id" - add_foreign_key "community_news", "users", column: "user_author_id" add_foreign_key "community_news", "windows_types" add_foreign_key "contact_methods", "addresses" add_foreign_key "continuing_education_registrations", "event_registrations" diff --git a/spec/models/community_news_spec.rb b/spec/models/community_news_spec.rb index e880fe880e..7aaed7ea8b 100644 --- a/spec/models/community_news_spec.rb +++ b/spec/models/community_news_spec.rb @@ -5,7 +5,7 @@ describe "#author_person" do let(:creator) { create(:user, :with_person) } - let(:legacy_user_author) { create(:user, :with_person) } + let(:legacy_author_user) { create(:user, :with_person) } let(:person) { create(:person) } it "returns the explicit person author when present" do @@ -14,12 +14,12 @@ end it "falls back to the legacy display-author user's person" do - news = create(:community_news, created_by: creator, author: nil, user_author: legacy_user_author) - expect(news.author_person).to eq(legacy_user_author.person) + news = create(:community_news, created_by: creator, author: nil, legacy_author_user: legacy_author_user) + expect(news.author_person).to eq(legacy_author_user.person) end it "falls back to the creating user's person when nothing else is set" do - news = create(:community_news, created_by: creator, author: nil, user_author: nil) + news = create(:community_news, created_by: creator, author: nil, legacy_author_user: nil) expect(news.author_person).to eq(creator.person) end end diff --git a/spec/models/workshop_variation_spec.rb b/spec/models/workshop_variation_spec.rb index 5ec93e63aa..2d61718c98 100644 --- a/spec/models/workshop_variation_spec.rb +++ b/spec/models/workshop_variation_spec.rb @@ -38,7 +38,8 @@ it { should validate_presence_of(:name) } it { should validate_presence_of(:rhino_body) } it { should validate_presence_of(:windows_type_id) } - it { should validate_presence_of(:author_credit_preference) } + # author_credit_preference default + inclusion are covered by the shared + # "author_creditable" examples. it { should validate_uniqueness_of(:name).scoped_to(:workshop_id).case_insensitive } end diff --git a/spec/support/shared_examples/author_creditable.rb b/spec/support/shared_examples/author_creditable.rb index 3ac1b03f8c..8cc57b8545 100644 --- a/spec/support/shared_examples/author_creditable.rb +++ b/spec/support/shared_examples/author_creditable.rb @@ -39,19 +39,61 @@ end end - context "when author_credit_preference is nil", unless: described_class.validators_on(:author_credit_preference).any? { |v| v.is_a?(ActiveModel::Validations::PresenceValidator) } do - it "falls back to the person's display name" do + context "when the preference is unset" do + it "defaults new records to full_name" do + expect(described_class.new.author_credit_preference).to eq("full_name") + end + + it "treats a blank preference as full_name at read time" do + record.author_credit_preference = nil + expect(record.author_credit).to eq(person.full_name) + end + + it "normalizes a blank preference to full_name on save (no backfill)" do record.update!(author_credit_preference: nil) - expect(record.author_credit).to eq(person.name) + expect(record.reload.author_credit_preference).to eq("full_name") end end - context "when user has no person" do - it "returns Anonymous" do + context "when the preference is not one of the allowed values" do + it "is invalid" do + record.author_credit_preference = "sideways" + expect(record).not_to be_valid + expect(record.errors[:author_credit_preference]).to be_present + end + end + + context "when there is no credited person" do + it "falls back to the model's missing_author_label" do user_without_person = create(:user, person: nil) record.update!(created_by: user_without_person) - expect(record.author_credit).to eq("Anonymous") + expect(record.author_credit).to eq(record.missing_author_label) + expect(record.missing_author_label).to be_present end end end + + describe ".by_credited_person_name" do + let(:author_user) { create(:user, :with_person) } + let!(:record) { create(factory, created_by: author_user) } + + it "matches the creating user's person by name" do + author_user.person.update!(first_name: "Zephyrine", last_name: "Quixotel") + expect(described_class.by_credited_person_name("Zephyrine")).to include(record) + expect(described_class.by_credited_person_name("Quixotel")).to include(record) + end + + it "does not match an unrelated name" do + author_user.person.update!(first_name: "Zephyrine", last_name: "Quixotel") + expect(described_class.by_credited_person_name("Nonexistententry")).not_to include(record) + end + end + + describe ".order_by_author" do + it "runs without error in both directions" do + create(factory) + expect { described_class.order_by_author("asc").to_a }.not_to raise_error + expect { described_class.order_by_author("desc").to_a }.not_to raise_error + end + end end From 78b4b0e7d57d169d8aefbca5e1f4ef39b969f06c Mon Sep 17 00:00:00 2001 From: maebeale Date: Sat, 4 Jul 2026 23:03:15 -0400 Subject: [PATCH 2/2] Standardize author display, search, and sort in views/controllers Route every credited-author byline through a shared credited_author_link helper that gates the profile link on profile_is_searchable and never links an anonymous or legacy free-text credit (fixes the Workshop index leaking names for anonymous authors and Workshop/WorkshopVariation linking non-searchable profiles). Replace the bespoke author-sort SQL in the Story/CommunityNews controllers and the WorkshopSearchService author filter with the shared order_by_author / by_credited_person_name scopes, and add author search+sort to Workshop and WorkshopVariation. Cover the new behavior in helper, service, request, and shared model specs. Co-Authored-By: Claude Opus 4.8 (1M context) --- app/controllers/community_news_controller.rb | 16 +------- app/controllers/stories_controller.rb | 11 +----- .../workshop_variations_controller.rb | 8 +++- app/decorators/resource_decorator.rb | 20 +--------- app/decorators/workshop_decorator.rb | 4 -- app/helpers/application_helper.rb | 13 +++++++ app/models/concerns/author_creditable.rb | 13 ++++++- app/services/workshop_search_service.rb | 36 ++++++++---------- app/views/community_news/show.html.erb | 7 +--- app/views/resources/show.html.erb | 9 +---- app/views/stories/show.html.erb | 7 +--- app/views/workshop_variations/index.html.erb | 19 +++++----- app/views/workshop_variations/show.html.erb | 2 +- app/views/workshops/_flex.html.erb | 2 +- app/views/workshops/_index_row.html.erb | 10 +---- app/views/workshops/_show_header.html.erb | 2 +- app/views/workshops/_sort_by_options.html.erb | 8 ++++ spec/helpers/application_helper_spec.rb | 38 +++++++++++++++++++ spec/requests/workshop_variations_spec.rb | 22 +++++++++++ spec/services/workshop_search_service_spec.rb | 30 +++++++++++++++ 20 files changed, 168 insertions(+), 109 deletions(-) diff --git a/app/controllers/community_news_controller.rb b/app/controllers/community_news_controller.rb index 3c3fdb3fc6..2c91b390dc 100644 --- a/app/controllers/community_news_controller.rb +++ b/app/controllers/community_news_controller.rb @@ -9,25 +9,13 @@ def index if turbo_frame_request? per_page = params[:number_of_items_per_page].presence || 12 base_scope = authorized_scope(CommunityNews.includes([ :bookmarks, :primary_asset, :author, - :organization, { user_author: :person }, { created_by: :person } ])) + :organization, { legacy_author_user: :person }, { created_by: :person } ])) filtered = base_scope.search_by_params(params) @sort = %w[created_at title author organization].include?(params[:sort]) ? params[:sort] : "created_at" @sort_direction = params[:direction] == "asc" ? "asc" : "desc" filtered = case @sort when "author" - # Match author_credit, which renders author_person: author, then the - # legacy user_author's person, then created_by's person. The join order - # fixes the people aliases (people_users, people_users_2), so keep it. - author_person_column = ->(field) { - Arel::Nodes::NamedFunction.new("COALESCE", [ - Person.arel_table[field], - Person.arel_table.alias("people_users")[field], - Person.arel_table.alias("people_users_2")[field] - ]) - } - dir = @sort_direction.to_sym - filtered.left_joins(:author, { user_author: :person }, { created_by: :person }) - .reorder(author_person_column.call(:last_name).public_send(dir), author_person_column.call(:first_name).public_send(dir)) + filtered.order_by_author(@sort_direction) when "organization" filtered.left_joins(:organization).reorder("organizations.name #{@sort_direction}") else diff --git a/app/controllers/stories_controller.rb b/app/controllers/stories_controller.rb index a4754ba58a..c2282466b3 100644 --- a/app/controllers/stories_controller.rb +++ b/app/controllers/stories_controller.rb @@ -175,16 +175,7 @@ def apply_sort(scope, column, direction) scope.left_joins(:workshop) .reorder(Workshop.arel_table[:title].public_send(dir)) when "author" - # Match Story#author_person, which the column renders: sort by the explicit - # author, falling back to the creating user's person. `created_by: :person` - # is the second join to `people`, so Rails aliases it `people_users`. - creator_person = Person.arel_table.alias("people_users") - author_first_name = Arel::Nodes::NamedFunction.new( - "COALESCE", - [ Person.arel_table[:first_name], creator_person[:first_name] ] - ) - scope.left_joins(:author, created_by: :person) - .reorder(author_first_name.public_send(dir)) + scope.order_by_author(direction) when "organization" scope.left_joins(:organization) .reorder(Organization.arel_table[:name].public_send(dir)) diff --git a/app/controllers/workshop_variations_controller.rb b/app/controllers/workshop_variations_controller.rb index 7a2a4b1143..19a43b2ede 100644 --- a/app/controllers/workshop_variations_controller.rb +++ b/app/controllers/workshop_variations_controller.rb @@ -6,8 +6,14 @@ def index base_scope = WorkshopVariation.includes(:workshop, :author, created_by: :person) .joins(:workshop).where(workshops: { published: true }) + @sort = params[:sort] + @sort_direction = params[:direction] == "asc" ? "asc" : "desc" filtered = base_scope.search_by_params(params) - .order("workshop_variations.created_at DESC, workshops.title, workshop_variations.name") + filtered = if @sort == "author" + filtered.order_by_author(@sort_direction) + else + filtered.order("workshop_variations.created_at DESC, workshops.title, workshop_variations.name") + end @workshop_variations = filtered.paginate(page: params[:page], per_page: 25).decorate @workshop_variations_count = filtered.count == base_scope.count ? base_scope.count : "#{filtered.count}/#{base_scope.count}" end diff --git a/app/decorators/resource_decorator.rb b/app/decorators/resource_decorator.rb index 6781bdb817..172ac2fa97 100644 --- a/app/decorators/resource_decorator.rb +++ b/app/decorators/resource_decorator.rb @@ -13,23 +13,7 @@ def kind_display end def truncated_author - h.truncate credited_author_name, length: 20 - end - - # Name shown on the author/credit line: the chosen person's credit when a - # person author is set, otherwise the preserved legacy free-text author, - # otherwise the creating user's person credit (AuthorCreditable fallback). - def credited_author_name - return author_credit if author.present? - legacy_author_name.presence || author_credit - end - - # The person to link the credit to, when there is a linkable one. A legacy - # free-text author is just a string, so it links nowhere. - def credited_author_link_person - return author if author.present? - return nil if legacy_author_name.present? - created_by&.person + h.truncate author_credit, length: 20 end def truncated_title @@ -53,7 +37,7 @@ def breadcrumbs end def author_full_name - credited_author_name + author_credit end def display_date diff --git a/app/decorators/workshop_decorator.rb b/app/decorators/workshop_decorator.rb index 1ec1c48801..733f646aea 100644 --- a/app/decorators/workshop_decorator.rb +++ b/app/decorators/workshop_decorator.rb @@ -51,10 +51,6 @@ def detail_breadcrumbs "#{breadcrumbs_title} >> #{breadcrumb_link}".html_safe end - def author - author_credit_preference.present? ? author_credit : full_name.to_s - end - def list_sectors sectorable_items.map(&:sector).map(&:name).to_sentence end diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb index 4010b43ebf..0d1ffdec7d 100644 --- a/app/helpers/application_helper.rb +++ b/app/helpers/application_helper.rb @@ -1,4 +1,17 @@ module ApplicationHelper + # Byline for an AuthorCreditable record. Links to the credited author's person + # profile when the credit resolves to a searchable person; otherwise renders + # plain text. The text always honors the credit preference (author_credit), so + # anonymous and legacy free-text credits never link to a profile. + def credited_author_link(record, **link_options) + person = record.author_credit_person + if person&.profile_is_searchable + link_to record.author_credit, person_path(person), **link_options + else + record.author_credit + end + end + # Tags an admin may use in a form field name / group header that should # render (rather than escape) on the public form. Block + inline formatting, # links, line breaks, and font sizing/coloring (via or inline style). diff --git a/app/models/concerns/author_creditable.rb b/app/models/concerns/author_creditable.rb index 37543b6f16..16233924bc 100644 --- a/app/models/concerns/author_creditable.rb +++ b/app/models/concerns/author_creditable.rb @@ -61,6 +61,15 @@ def author_credit format_person_credit(created_by&.person) end + # The person the credit should link to, or nil when the displayed credit is an + # anonymous, legacy free-text, or generic label that must not resolve to a + # profile. Mirrors author_credit's precedence so the linked person always + # matches the shown name. + def author_credit_person + return nil if author_credit_preference == "anonymous" + primary_author_person || (created_by&.person if legacy_author_name_text.blank?) + end + # Shown when there is no credited person or legacy name. Overridable per model # (e.g. Workshop shows "Facilitator"). def missing_author_label @@ -121,8 +130,10 @@ def by_credited_person_name(query) # explicit author, then any legacy sources, then the creating user's person. def order_by_author(direction) ascending = direction.to_s.casecmp("asc").zero? + # By first name then last name, matching the credit displayed by default + # ("First Last"), so the ordering follows the visible column. joins(credited_person_join_sql) - .reorder(coalesced_author_arel(:last_name, ascending), coalesced_author_arel(:first_name, ascending)) + .reorder(coalesced_author_arel(:first_name, ascending), coalesced_author_arel(:last_name, ascending)) end private diff --git a/app/services/workshop_search_service.rb b/app/services/workshop_search_service.rb index ad029e714e..9a2045dfee 100644 --- a/app/services/workshop_search_service.rb +++ b/app/services/workshop_search_service.rb @@ -161,27 +161,10 @@ def filter_by_query def search_by_author_name(workshops, author_name) return workshops if author_name.blank? - sanitized = author_name.strip.gsub(/\s+/, "") - # `created_by: :person` joins `people` (the creator's person); the explicit - # author is a second join to the same table, so alias it `author_person`. - workshops.left_outer_joins(created_by: :person) - .joins("LEFT OUTER JOIN people author_person ON author_person.id = workshops.author_id") - .where( - "LOWER(REPLACE(workshops.full_name, ' ', '')) LIKE :name - OR LOWER(REPLACE(CONCAT(users.first_name, users.last_name), ' ', '')) LIKE :name - OR LOWER(REPLACE(CONCAT(users.last_name, users.first_name), ' ', '')) LIKE :name - OR LOWER(REPLACE(users.first_name, ' ', '')) LIKE :name - OR LOWER(REPLACE(users.last_name, ' ', '')) LIKE :name - OR LOWER(REPLACE(CONCAT(people.first_name, people.last_name), ' ', '')) LIKE :name - OR LOWER(REPLACE(CONCAT(people.last_name, people.first_name), ' ', '')) LIKE :name - OR LOWER(REPLACE(people.first_name, ' ', '')) LIKE :name - OR LOWER(REPLACE(people.last_name, ' ', '')) LIKE :name - OR LOWER(REPLACE(CONCAT(author_person.first_name, author_person.last_name), ' ', '')) LIKE :name - OR LOWER(REPLACE(CONCAT(author_person.last_name, author_person.first_name), ' ', '')) LIKE :name - OR LOWER(REPLACE(author_person.first_name, ' ', '')) LIKE :name - OR LOWER(REPLACE(author_person.last_name, ' ', '')) LIKE :name", - name: "%#{sanitized}%" - ) + # Credited-author name match (explicit author + legacy full_name + creator), + # shared via AuthorCreditable#by_credited_person_name. Isolated in an id + # subquery so its person joins don't collide with the current scope's joins. + workshops.where(id: workshops.by_credited_person_name(author_name).select("workshops.id")) end def search_by_categories(workshops, categories) @@ -237,6 +220,8 @@ def order_by_params @workshops = @workshops.order(bookmarks_count: :desc, title: :asc) when "title" @workshops = @workshops.order(Arel.sql(TITLE_SORT_SQL)) + when "author" + @workshops = @workshops.order_by_author("asc") when "keywords" # already ordered else @@ -249,6 +234,15 @@ def order_by_params def resolve_ids_order return if sort.in?(%w[keywords popularity]) + if sort == "author" + # order_by_author already ordered @workshops via joined COALESCE columns; + # preserve that order through the id round-trip, deduping join fan-out in Ruby. + ids = @workshops.pluck(:id).uniq + @workshops = ids.empty? ? Workshop.none : + Workshop.where(id: ids).order(Arel.sql("FIELD(id, #{ids.join(',')})")) + return + end + sort_columns = case sort when "created" then [ :id, :created_at, :year, :month, :title ] diff --git a/app/views/community_news/show.html.erb b/app/views/community_news/show.html.erb index 0a54f07b09..5c0a37ec72 100644 --- a/app/views/community_news/show.html.erb +++ b/app/views/community_news/show.html.erb @@ -28,12 +28,7 @@

By: - <% credited_person = @community_news.author_person %> - <% if credited_person&.profile_is_searchable %> - <%= link_to @community_news.author_credit, person_path(credited_person) %> - <% else %> - <%= @community_news.author_credit %> - <% end %> + <%= credited_author_link(@community_news) %>

Organization Name: <%= @community_news.organization&.name %>

diff --git a/app/views/resources/show.html.erb b/app/views/resources/show.html.erb index 9c00e601c6..3d4e613154 100644 --- a/app/views/resources/show.html.erb +++ b/app/views/resources/show.html.erb @@ -28,14 +28,9 @@
<% unless @resource.toolkit_and_form? %>
- <% credited_name = @resource.credited_author_name %> - <% link_person = @resource.credited_author_link_person %> + <% credited_name = @resource.author_credit %> - <% if link_person&.profile_is_searchable %> - <%= link_to credited_name, person_path(link_person), class: "hover:underline" %> - <% else %> - <%= credited_name %> - <% end %> + <%= credited_author_link(@resource, class: "hover:underline") %> <% unless @resource.story? %> "> diff --git a/app/views/stories/show.html.erb b/app/views/stories/show.html.erb index f1b31cc195..5db9a2aafb 100644 --- a/app/views/stories/show.html.erb +++ b/app/views/stories/show.html.erb @@ -28,12 +28,7 @@

Story by: - <% credited_person = @story.author_person %> - <% if credited_person&.profile_is_searchable %> - <%= link_to @story.author_credit, person_path(credited_person) %> - <% else %> - <%= @story.author_credit %> - <% end %> + <%= credited_author_link(@story) %>

Organization Name: <%= link_to @story.decorate.organization_name, diff --git a/app/views/workshop_variations/index.html.erb b/app/views/workshop_variations/index.html.erb index 6a6a5f717c..1c6a45a24d 100644 --- a/app/views/workshop_variations/index.html.erb +++ b/app/views/workshop_variations/index.html.erb @@ -25,7 +25,15 @@ Name Description - Author + + <%= link_to workshop_variations_path(request.query_parameters.merge(sort: "author", direction: (@sort == "author" && @sort_direction == "asc") ? "desc" : "asc", page: nil)), + class: "inline-flex items-center justify-center gap-1 text-gray-700 hover:text-gray-900" do %> + Author + <% if @sort == "author" %> + text-xs opacity-70"> + <% end %> + <% end %> + From idea Updated Edit @@ -73,14 +81,7 @@ <% end %> - <% credited_person = workshop_variation.author_person %> - <% if credited_person %> - <%= link_to workshop_variation.author_credit, - person_path(credited_person), - class: "text-gray-500 hover:text-gray-700" %> - <% else %> - <%= workshop_variation.author_credit %> - <% end %> + <%= credited_author_link(workshop_variation, class: "text-gray-500 hover:text-gray-700") %> <% if workshop_variation.workshop_variation_idea %> diff --git a/app/views/workshop_variations/show.html.erb b/app/views/workshop_variations/show.html.erb index 17b4d254c9..6b1a4bb9ba 100644 --- a/app/views/workshop_variations/show.html.erb +++ b/app/views/workshop_variations/show.html.erb @@ -39,7 +39,7 @@

- <%= @workshop_variation.author_credit %> + <%= credited_author_link(@workshop_variation, class: "hover:underline") %> "> <%= @workshop_variation.created_at.strftime('%B %e, %Y') %> diff --git a/app/views/workshops/_flex.html.erb b/app/views/workshops/_flex.html.erb index 5d4292b3a3..cfbe12415f 100644 --- a/app/views/workshops/_flex.html.erb +++ b/app/views/workshops/_flex.html.erb @@ -12,7 +12,7 @@

<%= link_to workshop.title, link_route, data: { turbo_prefetch: false } %>

<%= "#{workshop.windows_type.short_name unless workshop.windows_type.nil?} - #{workshop.date}" %>
Author: - <%= workshop.author %> + <%= credited_author_link(workshop) %>

<%= truncate( workshop.formatted_objective, length: 120, diff --git a/app/views/workshops/_index_row.html.erb b/app/views/workshops/_index_row.html.erb index febda8283a..d7e63ef4a8 100644 --- a/app/views/workshops/_index_row.html.erb +++ b/app/views/workshops/_index_row.html.erb @@ -53,15 +53,7 @@

By: - <% credited_person = workshop.author_person %> - <% if credited_person %> - <%= link_to workshop.author_name, - person_path(credited_person), - class: "hover:underline", - data: { turbo: false } %> - <% else %> - <%= workshop.full_name.present? ? workshop.full_name : "Facilitator" %> - <% end %> + <%= credited_author_link(workshop, class: "hover:underline", data: { turbo: false }) %> | <%= workshop.date %>
diff --git a/app/views/workshops/_show_header.html.erb b/app/views/workshops/_show_header.html.erb index 1126756258..312ac82233 100644 --- a/app/views/workshops/_show_header.html.erb +++ b/app/views/workshops/_show_header.html.erb @@ -20,7 +20,7 @@ (Unpublished) <% end %> -
Author: <%= workshop.author %>Category: <%= workshop.windows_type&.short_name %>Added: <%= workshop.date %>
+
Author: <%= credited_author_link(workshop, class: "hover:underline") %>Category: <%= workshop.windows_type&.short_name %>Added: <%= workshop.date %>
<%= render "assets/primary_image_picker", owner: workshop %>
diff --git a/app/views/workshops/_sort_by_options.html.erb b/app/views/workshops/_sort_by_options.html.erb index 4a4707faae..0c829d7416 100644 --- a/app/views/workshops/_sort_by_options.html.erb +++ b/app/views/workshops/_sort_by_options.html.erb @@ -22,6 +22,14 @@
+
+ +
+ <%# if params[:query].present? %> diff --git a/spec/helpers/application_helper_spec.rb b/spec/helpers/application_helper_spec.rb index 31f720e5cf..c2d01abcde 100644 --- a/spec/helpers/application_helper_spec.rb +++ b/spec/helpers/application_helper_spec.rb @@ -1,6 +1,44 @@ require "rails_helper" RSpec.describe ApplicationHelper, type: :helper do + describe "#credited_author_link" do + let(:person) { create(:person, first_name: "Ada", last_name: "Lovelace") } + + it "links to the person profile when the credit resolves to a searchable person" do + allow(person).to receive(:profile_is_searchable).and_return(true) + workshop = create(:workshop, author_credit_preference: "full_name") + allow(workshop).to receive(:author).and_return(person) + + html = helper.credited_author_link(workshop) + expect(html).to include("