Skip to content

Alphabetize the Product Drive Participant drop-downs (#5593) #5656

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
pcbeingused333 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Alphabetize the Product Drive Participant drop-downs (#5593) #5656

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 21 additions & 1 deletion app/models/product_drive_participant.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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".

@dorner dorner Sep 4, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure that was part of the original requirements (only if the string starts with a number, not if there's a number in the middle of the string). @ruestitch thoughts? That might simplify this function.

@ruestitch ruestitch Sep 13, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From reviewing this a few times with others, the solution seems like it should work for natural sorting

Agree, store 9 should come before store 10, requiring number and letter sort to be distinct, and to sort numerically in cases of numbers

BUT this is complicated for delivering a sustainable solution across all use cases, which may include alphanumeric and instances of 2nd, 4th type entries, so we should compare this value add to the long view of the Postgres solution you mentioned, @dorner

@pcbeingused333 pcbeingused333 Sep 13, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That matches what I'd propose: scope this to what the current data actually needs rather than full generality (alphanumeric, "2nd"/"4th", etc.), since there's no evidence any of that appears in business_name today — only the digit-run case ("Store 9" vs "Store 10") is covered by the existing specs. If mid-name digit runs turn out to be the only real case, that's also the simpler of the two Postgres options in the other thread — happy to build to whichever scope you land on.

@dorner dorner Sep 18, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry @ruestitch not sure I understood your comment - are you saying that "Store 9" should come before "Store 10"? Because in that case the number is in the middle of the string, not the beginning, which means the first Postgres solution will not work.

#
# Written as a literal because the schema is maintained as `schema.rb`, which

@dorner dorner Sep 4, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like adding a Postgres function with something like this would be more generally useful. We know we want to apply this ordering to other places. You can look at something like fx to handle it.

@ruestitch ruestitch Sep 13, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed that the Postgres solution is better long term, do we want a step in the right direction first? What's the LOE for the optimal shared function in Postgres?

@pcbeingused333 pcbeingused333 Sep 13, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LOE for the two versions is genuinely different, which is why I'd frame this as staged:

  • Leading-digit-only (business_name starts with a number): collapses to one expression, roughly CASE WHEN business_name ~ '^[0-9]' THEN ... END, wrapped as a single fx-managed function. Half a day including the migration and updating both call sites.
  • Full natural sort (digit runs anywhere in the string — what this PR's Ruby implementation actually does, verified by the "Store 2"/"Store 9"/"Store 10" specs): doesn't reduce to a one-liner. It needs to tokenize the string into alternating text/number spans and build a sortable key from them, which in plain SQL means a plpgsql function looping over regexp_matches, not an expression. Different order of effort, and I'd rather not build it twice.

Given that gap: this PR as the interim step (Ruby-level, already green at 11/11), then the Postgres function once the scope question in the other thread is settled. I can start the full version now if that's the target, or the leading-digit one if that's enough.

# 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, -> {
Expand Down
2 changes: 1 addition & 1 deletion app/views/donations/_donation_form.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 %>
Expand Down
2 changes: 1 addition & 1 deletion app/views/donations/index.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@
<div class="form-group col-lg-3 col-md-4 col-sm-6 col-xs-12">
<%= 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) %>
</div>
<% end %>
Expand Down
2 changes: 1 addition & 1 deletion app/views/product_drive_participants/create.js.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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('<option value="">---Create new Participant---</option>');
$("#donation_product_drive_participant_id").val('<%= @product_drive_participant[:id] %>');
18 changes: 18 additions & 0 deletions spec/models/product_drive_participant_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading