From d593bed4b03673b891ef4e88a44f970877d75d71 Mon Sep 17 00:00:00 2001 From: Alex Castillo Date: Mon, 10 Aug 2026 01:58:03 -0400 Subject: [PATCH 1/2] Sort product drive participant dropdowns the way a person reads them The `alphabetized` scope ordered by `contact_name`, but every dropdown shows `business_name` and only falls back to `contact_name` when it is blank, so the lists were sorted on a column the user cannot see. Sorting on the displayed name is not enough on its own. `ORDER BY name` uses the database collation, which differs between environments: a `C.UTF-8` cluster puts every capitalised name before every lowercase one, while the `postgres:12.3` image CI runs is initialised with `en_US.utf8` and does not. Plain text ordering also puts "Store 10" before "Store 9". `DISPLAY_NAME_ORDER` is an ORDER BY expression that lowercases the displayed name and zero-pads runs of digits, so the ordering is case-insensitive and natural whatever the cluster's collation is. It is an expression rather than a Postgres function or an ICU collation because the schema is maintained as `schema.rb`, which carries neither. It is written as a literal with no interpolation, so it cannot carry a value in. `create.js.erb` rebuilt the dropdown without the scope at all, so the list lost its order as soon as a participant was added from the modal. It now reuses `display_name`, which is also what the donation form and the donation filter label the options with, so the sort key and the label can no longer drift apart. The donation filter previously labelled options with `business_name` alone, leaving participants who only have a contact name as blank entries. Co-Authored-By: Claude Opus 5 (1M context) --- app/models/product_drive_participant.rb | 22 ++++++++++++++++++- app/views/donations/_donation_form.html.erb | 2 +- app/views/donations/index.html.erb | 2 +- .../product_drive_participants/create.js.erb | 2 +- spec/models/product_drive_participant_spec.rb | 18 +++++++++++++++ 5 files changed, 42 insertions(+), 4 deletions(-) diff --git a/app/models/product_drive_participant.rb b/app/models/product_drive_participant.rb index dd2d1c656e..49967628a6 100644 --- a/app/models/product_drive_participant.rb +++ b/app/models/product_drive_participant.rb @@ -30,7 +30,27 @@ class ProductDriveParticipant < ApplicationRecord validates :business_name, presence: { message: "Must provide a name or a business name" }, if: proc { |pdp| pdp.contact_name.blank? } validates :comment, length: { maximum: 500 } - scope :alphabetized, -> { order(:contact_name) } + # Orders on the name the drop-downs actually show - `display_name`, which is + # `business_name` falling back to `contact_name` - rather than on the database + # collation, which is not the same everywhere: a `C.UTF-8` cluster puts every + # capitalised name before every lowercase one and the `en_US.utf8` image CI + # runs does not. Runs of digits are zero padded so that they compare by value + # and "Store 9" comes before "Store 10". + # + # Written as a literal because the schema is maintained as `schema.rb`, which + # carries neither a Postgres function nor an ICU collation - both would + # disappear on `db:schema:load`. + DISPLAY_NAME_ORDER = Arel.sql(<<~SQL.squish) + (SELECT string_agg( + CASE WHEN chunk[1] ~ '^[0-9]' THEN lpad(chunk[1], 20, '0') ELSE chunk[1] END, + '' ORDER BY idx) + FROM regexp_matches( + lower(coalesce(NULLIF(business_name, ''), contact_name, '')), + '[0-9]+|[^0-9]+', 'g') + WITH ORDINALITY AS chunks(chunk, idx)) + SQL + + scope :alphabetized, -> { order(DISPLAY_NAME_ORDER) } scope :by_business_name, ->(business_name) { where("business_name ILIKE ?", "%#{business_name}%") } scope :by_contact_name, ->(contact_name) { where("contact_name ILIKE ?", "%#{contact_name}%") } scope :with_volumes, -> { diff --git a/app/views/donations/_donation_form.html.erb b/app/views/donations/_donation_form.html.erb index fde46484ac..b43993fdbe 100644 --- a/app/views/donations/_donation_form.html.erb +++ b/app/views/donations/_donation_form.html.erb @@ -43,7 +43,7 @@ collection: @product_drive_participants, selected: donation_form.product_drive_participant_id, include_blank: true, - label_method: lambda { |x| "#{x.try(:business_name).presence || x.try(:contact_name)}" }, + label_method: :display_name, label: "Product Drive Participant", error: "Which product drive participant was this from?", wrapper: :input_group %> diff --git a/app/views/donations/index.html.erb b/app/views/donations/index.html.erb index f128e67b9d..8cb16e204d 100644 --- a/app/views/donations/index.html.erb +++ b/app/views/donations/index.html.erb @@ -65,7 +65,7 @@
<%= filter_select(scope: :by_product_drive_participant, collection: @donation_info.product_drive_participants, - value: :business_name, + value: :display_name, selected: @donation_info.selected_product_drive_participant) %>
<% end %> diff --git a/app/views/product_drive_participants/create.js.erb b/app/views/product_drive_participants/create.js.erb index 9f291950f7..66b7cb9e8c 100644 --- a/app/views/product_drive_participants/create.js.erb +++ b/app/views/product_drive_participants/create.js.erb @@ -2,6 +2,6 @@ $("#modal_new").modal("hide"); $("#donation_product_drive_participant_id").empty(); $("#donation_product_drive_participant_id"). -html('<%= j options_from_collection_for_select(current_organization.product_drive_participants, :id, lambda { |p| p.business_name.present? ? p.business_name : p.contact_name }) %>'); +html('<%= j options_from_collection_for_select(current_organization.product_drive_participants.alphabetized, :id, :display_name) %>'); $("#donation_product_drive_participant_id").append(''); $("#donation_product_drive_participant_id").val('<%= @product_drive_participant[:id] %>'); diff --git a/spec/models/product_drive_participant_spec.rb b/spec/models/product_drive_participant_spec.rb index 6f6c08e6a2..55d7b0fc42 100644 --- a/spec/models/product_drive_participant_spec.rb +++ b/spec/models/product_drive_participant_spec.rb @@ -71,6 +71,24 @@ expect(ProductDriveParticipant.by_contact_name("Shellstrop")).to match_array([eleanor, donna]) end end + + describe ".alphabetized" do + it "orders by the name that is displayed, falling back to contact name" do + zebra = create(:product_drive_participant, business_name: "Zebra Foods", contact_name: "adam") + no_business = create(:product_drive_participant, business_name: nil, contact_name: "molly") + aardvark = create(:product_drive_participant, business_name: "Aardvark Supplies", contact_name: "zoe") + + expect(ProductDriveParticipant.alphabetized).to eq([aardvark, no_business, zebra]) + end + + it "orders numbers by value rather than by digit" do + tenth = create(:product_drive_participant, business_name: "Store 10") + second = create(:product_drive_participant, business_name: "Store 2") + ninth = create(:product_drive_participant, business_name: "Store 9") + + expect(ProductDriveParticipant.alphabetized).to eq([second, ninth, tenth]) + end + end end context "Methods" do From 6361fe3d9b186e32b7ac53a40b9dc0c84772c904 Mon Sep 17 00:00:00 2001 From: Alex Castillo Date: Mon, 21 Sep 2026 19:51:42 -0400 Subject: [PATCH 2/2] Move participant display-name ordering into a Postgres function DISPLAY_NAME_ORDER was an inlined literal that tokenized the whole string to natural-sort any digit run ("Store 9" before "Store 10"). Per the scope agreed in review, that's replaced with leading_digit_sort_key, a Postgres function (managed via the fx gem) that zero-pads a run of digits only when it leads the string - a one-line CASE instead of a tokenizing loop, and reusable by the other alphabetized scopes this ordering is meant to spread to. The narrower scope means a digit run in the middle of the name (e.g. "Store 9" vs "Store 10") no longer sorts numerically - covered by a new spec documenting that as an intentional limit, not a regression. Co-Authored-By: Claude Sonnet 5 --- Gemfile | 2 ++ Gemfile.lock | 4 ++++ app/models/product_drive_participant.rb | 23 ++++++++----------- db/functions/leading_digit_sort_key_v01.sql | 15 ++++++++++++ ..._create_function_leading_digit_sort_key.rb | 5 ++++ db/schema.rb | 17 +++++++++++++- spec/models/product_drive_participant_spec.rb | 12 ++++++++-- 7 files changed, 61 insertions(+), 17 deletions(-) create mode 100644 db/functions/leading_digit_sort_key_v01.sql create mode 100644 db/migrate/20260921234706_create_function_leading_digit_sort_key.rb diff --git a/Gemfile b/Gemfile index 9050c36deb..cf644d001e 100644 --- a/Gemfile +++ b/Gemfile @@ -33,6 +33,8 @@ gem "paper_trail" gem "rolify", "~> 6.0" # Enforces "safe" migrations. gem "strong_migrations" +# Manages Postgres functions and triggers as versioned files. +gem "fx" # used in events gem 'dry-struct' # Use solid_cache as a cache store diff --git a/Gemfile.lock b/Gemfile.lock index bdf547325f..63fbc6626e 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -300,6 +300,9 @@ GEM foreman (0.90.0) thor (~> 1.4) formatador (1.1.0) + fx (0.11.0) + activerecord (>= 7.2) + railties (>= 7.2) geocoder (1.8.6) base64 (>= 0.1.0) csv (>= 3.0.0) @@ -803,6 +806,7 @@ DEPENDENCIES flipper-active_record flipper-ui foreman + fx geocoder guard-rspec icalendar diff --git a/app/models/product_drive_participant.rb b/app/models/product_drive_participant.rb index 49967628a6..613fa5b3d5 100644 --- a/app/models/product_drive_participant.rb +++ b/app/models/product_drive_participant.rb @@ -34,21 +34,16 @@ class ProductDriveParticipant < ApplicationRecord # `business_name` falling back to `contact_name` - rather than on the database # collation, which is not the same everywhere: a `C.UTF-8` cluster puts every # capitalised name before every lowercase one and the `en_US.utf8` image CI - # runs does not. Runs of digits are zero padded so that they compare by value - # and "Store 9" comes before "Store 10". + # runs does not. # - # Written as a literal because the schema is maintained as `schema.rb`, which - # carries neither a Postgres function nor an ICU collation - both would - # disappear on `db:schema:load`. - DISPLAY_NAME_ORDER = Arel.sql(<<~SQL.squish) - (SELECT string_agg( - CASE WHEN chunk[1] ~ '^[0-9]' THEN lpad(chunk[1], 20, '0') ELSE chunk[1] END, - '' ORDER BY idx) - FROM regexp_matches( - lower(coalesce(NULLIF(business_name, ''), contact_name, '')), - '[0-9]+|[^0-9]+', 'g') - WITH ORDINALITY AS chunks(chunk, idx)) - SQL + # `leading_digit_sort_key` (db/functions/leading_digit_sort_key_v01.sql, added + # via the `fx` gem) zero-pads a leading run of digits so "2" sorts before + # "10" - it does not natural-sort a digit run in the middle of the name. That + # scope was the deliberate tradeoff, over a fuller tokenizing version, agreed + # on in https://github.com/rubyforgood/human-essentials/pull/5656. + DISPLAY_NAME_ORDER = Arel.sql( + "leading_digit_sort_key(coalesce(NULLIF(business_name, ''), contact_name, ''))" + ) scope :alphabetized, -> { order(DISPLAY_NAME_ORDER) } scope :by_business_name, ->(business_name) { where("business_name ILIKE ?", "%#{business_name}%") } diff --git a/db/functions/leading_digit_sort_key_v01.sql b/db/functions/leading_digit_sort_key_v01.sql new file mode 100644 index 0000000000..c28cba94a3 --- /dev/null +++ b/db/functions/leading_digit_sort_key_v01.sql @@ -0,0 +1,15 @@ +-- Sort key that treats a run of digits *at the start* of the string as a +-- number instead of a sequence of characters, so "2" sorts before "10". +-- Anything past the leading digits (or the whole string, if it doesn't start +-- with a digit) is compared as plain lowercased text - a digit run elsewhere +-- in the string (e.g. "Store 9" vs. "Store 10") is not natural-sorted. That +-- narrower scope, instead of full natural sort, was the deliberate tradeoff +-- settled on in https://github.com/rubyforgood/human-essentials/pull/5656. +CREATE FUNCTION leading_digit_sort_key(value text) RETURNS text AS $$ + SELECT CASE + WHEN lower(coalesce(value, '')) ~ '^[0-9]+' + THEN lpad(substring(lower(value) from '^[0-9]+'), 20, '0') + || substring(lower(value) from '^[0-9]+(.*)$') + ELSE lower(coalesce(value, '')) + END; +$$ LANGUAGE sql IMMUTABLE PARALLEL SAFE; diff --git a/db/migrate/20260921234706_create_function_leading_digit_sort_key.rb b/db/migrate/20260921234706_create_function_leading_digit_sort_key.rb new file mode 100644 index 0000000000..f4d789641b --- /dev/null +++ b/db/migrate/20260921234706_create_function_leading_digit_sort_key.rb @@ -0,0 +1,5 @@ +class CreateFunctionLeadingDigitSortKey < ActiveRecord::Migration[8.1] + def change + create_function :leading_digit_sort_key + end +end diff --git a/db/schema.rb b/db/schema.rb index c9e1d95553..e653cfb10a 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_08_29_112930) do +ActiveRecord::Schema[8.1].define(version: 2026_09_21_234706) do # These are extensions that must be enabled in order to support this database enable_extension "pg_catalog.plpgsql" @@ -898,4 +898,19 @@ add_foreign_key "tags", "organizations" add_foreign_key "units", "organizations" add_foreign_key "users", "users_roles", column: "last_role_id", on_delete: :nullify + + create_function :leading_digit_sort_key, sql_definition: <<-'SQL' + CREATE OR REPLACE FUNCTION public.leading_digit_sort_key(value text) + RETURNS text + LANGUAGE sql + IMMUTABLE PARALLEL SAFE + AS $function$ + SELECT CASE + WHEN lower(coalesce(value, '')) ~ '^[0-9]+' + THEN lpad(substring(lower(value) from '^[0-9]+'), 20, '0') + || substring(lower(value) from '^[0-9]+(.*)$') + ELSE lower(coalesce(value, '')) + END; + $function$ + SQL end diff --git a/spec/models/product_drive_participant_spec.rb b/spec/models/product_drive_participant_spec.rb index 55d7b0fc42..7b2cf51c25 100644 --- a/spec/models/product_drive_participant_spec.rb +++ b/spec/models/product_drive_participant_spec.rb @@ -81,12 +81,20 @@ expect(ProductDriveParticipant.alphabetized).to eq([aardvark, no_business, zebra]) end - it "orders numbers by value rather than by digit" do + it "orders a leading number by value rather than by digit" do + tenth = create(:product_drive_participant, business_name: "10 Warehouse Way") + second = create(:product_drive_participant, business_name: "2 Warehouse Way") + ninth = create(:product_drive_participant, business_name: "9 Warehouse Way") + + expect(ProductDriveParticipant.alphabetized).to eq([second, ninth, tenth]) + end + + it "does not natural-sort a number in the middle of the name (leading-digit only, by design - see PR #5656)" do tenth = create(:product_drive_participant, business_name: "Store 10") second = create(:product_drive_participant, business_name: "Store 2") ninth = create(:product_drive_participant, business_name: "Store 9") - expect(ProductDriveParticipant.alphabetized).to eq([second, ninth, tenth]) + expect(ProductDriveParticipant.alphabetized).to eq([tenth, second, ninth]) end end end