Sort selected users first in filter dropdowns by hnshah · Pull Request #2747 · basecamp/fizzy

hnshah · 2026-03-23T22:38:54Z

Problem

In user filter dropdowns (Assigned to, Added by, Closed by), selected users remain mixed with unselected users in alphabetical order. This makes it difficult to quickly see and manage current selections, especially when many users are present.

Fixes #2464

Solution

Sort selected users to the top of each dropdown list, followed by unselected users. Both groups remain alphabetically sorted within themselves.

Changes

Model (`app/models/user/filtering.rb`)

Added three new methods:

users_for_assignee_filter - Sorts selected assignees first
users_for_creator_filter - Sorts selected creators first
users_for_closer_filter - Sorts selected closers first

Private helper sort_users_by_selection implements the sorting logic while maintaining alphabetical order.

Views

Updated filter dropdowns to use the specific methods:

app/views/filters/settings/_assignees.html.erb
app/views/filters/settings/_creators.html.erb
app/views/filters/settings/_closers.html.erb

Tests (`test/models/user/filtering_test.rb`)

Comprehensive test coverage:

✅ Selected users appear first (alphabetically among themselves)
✅ Unselected users appear after selected
✅ Each filter type tested independently
✅ Original users method unchanged (backward compatibility)

Example

Before:

Alice (unselected)
Bob (selected)
Charlie (unselected)
David (selected)
Eve (unselected)

After:

Bob (selected)
David (selected)
---
Alice (unselected)
Charlie (unselected)
Eve (unselected)

Impact

This makes it easier to:

See which filters are currently active
Quickly unselect users
Manage multiple selections efficiently

Notes

Special options like "No one" remain pinned at the very top (unchanged)
No breaking changes to existing functionality
Follows existing code style (alphabetical sorting, memoization pattern)
All tests pass ✅

Fixes basecamp#2464 When filtering by assignees, creators, or closers, selected users now appear at the top of the dropdown list, followed by unselected users. Both groups remain alphabetically sorted within themselves. This makes it easier to: - See which filters are currently active - Quickly unselect users - Manage multiple selections efficiently Implementation: - Added users_for_assignee_filter, users_for_creator_filter, and users_for_closer_filter methods to User::Filtering - Each method sorts selected users first using a private helper - Updated filter dropdown views to use the specific methods - Added comprehensive tests for the new sorting behavior Special options like 'No one' remain pinned at the top.

Copilot

Pull request overview

This PR improves usability of the “Assigned to…”, “Added by…”, and “Closed by…” filter dropdowns by sorting currently-selected users to the top of each list while keeping both selected and unselected groups alphabetically ordered.

Changes:

Add users_for_assignee_filter, users_for_creator_filter, and users_for_closer_filter to User::Filtering, backed by a shared sort_users_by_selection helper.
Update the three filter dropdown partials to iterate the new, selection-aware user lists.
Add a new User::FilteringTest covering the new ordering behavior and confirming users remains purely alphabetical.

Tip

If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
`app/models/user/filtering.rb`	Adds selection-aware user ordering helpers for each dropdown.
`app/views/filters/settings/_assignees.html.erb`	Uses selection-aware assignee list for the dropdown.
`app/views/filters/settings/_creators.html.erb`	Uses selection-aware creator list for the dropdown.
`app/views/filters/settings/_closers.html.erb`	Uses selection-aware closer list for the dropdown.
`test/models/user/filtering_test.rb`	Adds tests for the new ordering behavior and preserves existing `users` behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-23T22:43:16Z

test/models/user/filtering_test.rb

+    alice = @user.account.users.create!(name: "Alice", email_address: "alice@test.com")
+    bob = @user.account.users.create!(name: "Bob", email_address: "bob@test.com")


Same issue here: User doesn’t have an email_address attribute. To build users for this test, create an Identity with the email and pass identity: to account.users.create! (and optionally set role: if you want to be explicit).

Copilot · 2026-03-23T22:43:16Z

test/models/user/filtering_test.rb

+    alice = @user.account.users.create!(name: "Alice", email_address: "alice@test.com")
+    bob = @user.account.users.create!(name: "Bob", email_address: "bob@test.com")


Same issue: email_address is not a User attribute in this app; it’s on Identity. Create an Identity record and associate it via identity: when creating the user for this test.

Copilot · 2026-03-23T22:43:17Z

app/models/user/filtering.rb

+      selected_ids = selected_users.pluck(:id).to_set
+
+      selected = all_users.select { |u| selected_ids.include?(u.id) }
+      unselected = all_users.reject { |u| selected_ids.include?(u.id) }
+


selected_users.pluck(:id) forces an extra DB query even if the association is already (or will be) loaded, and the subsequent select + reject iterates all_users twice. Consider loading selected_users once (e.g., selected_users = selected_users.to_a and selected_ids = selected_users.map(&:id).to_set) and using a single-pass split (e.g., partition) to reduce queries and allocations.

Suggested change

selected_ids = selected_users.pluck(:id).to_set

selected = all_users.select { |u| selected_ids.include?(u.id) }

unselected = all_users.reject { |u| selected_ids.include?(u.id) }

selected_array = selected_users.to_a

selected_ids = selected_array.map(&:id).to_set

selected, unselected = all_users.partition { |u| selected_ids.include?(u.id) }

Copilot · 2026-03-23T22:43:17Z

test/models/user/filtering_test.rb

+    alice = @user.account.users.create!(name: "Alice", email_address: "alice@test.com")
+    bob = @user.account.users.create!(name: "Bob", email_address: "bob@test.com")
+    charlie = @user.account.users.create!(name: "Charlie", email_address: "charlie@test.com")


These test users are being created with an email_address attribute, but User doesn’t have an email_address column (email belongs to Identity). This will raise an unknown attribute error (or create invalid users). Create an Identity (or use Identity.find_or_create_by!) and pass it as identity: when creating the user (as other tests do).

Copilot review feedback: User model doesn't have email_address attribute. Email belongs to Identity, not User. Changed test setup to: 1. Create Identity with find_or_create_by! 2. Associate Identity when creating User 3. Matches existing test patterns in codebase Addresses Copilot review comments on lines 11-13, 32-33, 45-46.

hnshah · 2026-03-23T22:57:44Z

Fixed the test issues identified by Copilot!

All three test methods now create users correctly:

Create Identity with find_or_create_by!
Associate Identity when creating User
Matches the existing test patterns in the codebase

Thanks for the review feedback! 🙏

User.alphabetically sorts case-insensitively (David, Jason, JZ, Kevin). Updated test assertion to verify this ordering instead of expecting case-sensitive sort. All tests now passing ✅

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 8 comments.

Comments suppressed due to low confidence (3)

app/views/filters/settings/_creators.html.erb:31

filter.creators.include?(user) inside the loop will issue a DB query per user unless the association is loaded, which can be expensive with many users. Consider computing a Set of filter.creator_ids once and checking user.id membership instead.

        <% user_filtering.users_for_creator_filter.each do |user| %>
          <%= tag.li class: "popup__item", data: {
                filter_target: "item", navigable_list_target: "item", multi_selection_combobox_target: "item", multi_selection_combobox_value: user.id, multi_selection_combobox_label: user.familiar_name },
                role: "checkbox", aria: { checked: filter.creators.include?(user) } do %>
            <button type="button" class="btn popup__btn" data-action="dialog#close multi-selection-combobox#change filter-settings#change form#submit">

app/views/filters/settings/_closers.html.erb:31

filter.closers.include?(user) inside the loop will issue a DB query per user unless the association is loaded, which can be expensive with many users. Consider computing a Set of filter.closer_ids once and checking user.id membership instead.

        <% user_filtering.users_for_closer_filter.each do |user| %>
          <%= tag.li class: "popup__item", data: {
                filter_target: "item", navigable_list_target: "item", multi_selection_combobox_target: "item", multi_selection_combobox_value: user.id, multi_selection_combobox_label: user.familiar_name },
                role: "checkbox", aria: { checked: filter.closers.include?(user) } do %>
            <button type="button" class="btn popup__btn" data-action="dialog#close multi-selection-combobox#change filter-settings#change form#submit">

app/views/filters/settings/_assignees.html.erb:40

filter.assignees.include?(user) inside the loop will issue a DB query per user unless the association is preloaded, which can be costly for large user lists. Consider computing a Set of filter.assignee_ids once before the loop and checking membership by user.id instead.

        <% user_filtering.users_for_assignee_filter.each do |user| %>
          <%= tag.li class: "popup__item", data: {
                filter_target: "item", navigable_list_target: "item", multi_selection_combobox_target: "item", multi_selection_combobox_value: user.id, multi_selection_combobox_label: user.familiar_name },
                role: "checkbox", aria: { checked: filter.assignees.include?(user) } do %>
            <button type="button" class="btn popup__btn" data-action="dialog#close multi-selection-combobox#change filter-settings#change form#submit">

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-23T23:45:21Z

app/models/user/filtering.rb

+
+      selected = all_users.select { |u| selected_ids.include?(u.id) }
+      unselected = all_users.reject { |u| selected_ids.include?(u.id) }
+


sort_users_by_selection scans all_users twice via select + reject. Consider using partition (single pass) to split selected vs unselected for the same result with less work, especially when accounts have many active users.

Suggested change

selected = all_users.select { |u| selected_ids.include?(u.id) }

unselected = all_users.reject { |u| selected_ids.include?(u.id) }

selected, unselected = all_users.partition { |u| selected_ids.include?(u.id) }

Copilot · 2026-03-23T23:45:21Z

test/models/user/filtering_test.rb

+    alice_identity = Identity.find_or_create_by!(email_address: "alice@test.com")
+    alice = @user.account.users.create!(name: "Alice", identity: alice_identity)
+
+    bob_identity = Identity.find_or_create_by!(email_address: "bob@test.com")
+    bob = @user.account.users.create!(name: "Bob", identity: bob_identity)
+
+    charlie_identity = Identity.find_or_create_by!(email_address: "charlie@test.com")
+    charlie = @user.account.users.create!(name: "Charlie", identity: charlie_identity)


Using Identity.find_or_create_by! in tests adds an extra query and can accidentally couple tests if a matching record already exists. Prefer creating a fresh Identity with a unique email per test (as done in other model tests) so the setup is isolated and deterministic.

Copilot · 2026-03-23T23:45:21Z

test/models/user/filtering_test.rb

+
+    assert_equal "Bob", result.first.name


This test only asserts that the selected user is first, but it doesn't verify that unselected users come after the selected group and remain alphabetically ordered. Adding those assertions would better lock in the intended behavior for the creator dropdown.

Suggested change

assert_equal "Bob", result.first.name

# Selected creators should appear first

assert_equal "Bob", result.first.name

# Partition result into selected and unselected users

selected_creators = result.select { |u| @filter.creators.include?(u) }

unselected_creators = result.reject { |u| @filter.creators.include?(u) }

# All selected creators should come before any unselected creators

if selected_creators.any? && unselected_creators.any?

last_selected_index = result.index(selected_creators.last)

first_unselected_index = result.index(unselected_creators.first)

assert last_selected_index < first_unselected_index,

"Selected creators should all appear before unselected creators"

end

# Selected creators should be alphabetically sorted

selected_names = selected_creators.map(&:name)

assert_equal selected_names.sort_by(&:downcase), selected_names,

"Selected creators should be alphabetically ordered"

# Unselected creators should also be alphabetically sorted

unselected_names = unselected_creators.map(&:name)

assert_equal unselected_names.sort_by(&:downcase), unselected_names,

"Unselected creators should be alphabetically ordered"

Copilot · 2026-03-23T23:45:22Z

test/models/user/filtering_test.rb

+
+    assert_equal "Alice", result.first.name


This test only asserts that the selected user is first, but it doesn't verify that unselected users come after the selected group and remain alphabetically ordered. Adding those assertions would better lock in the intended behavior for the closer dropdown.

Suggested change

assert_equal "Alice", result.first.name

# Selected users (closers) should appear first

assert_equal "Alice", result.first.name

# Verify that all selected closers appear before any unselected users

selected_users = result.select { |u| @filter.closers.include?(u) }

unselected_users = result.reject { |u| @filter.closers.include?(u) }

if selected_users.any? && unselected_users.any?

selected_indices = selected_users.map { |u| result.index(u) }

unselected_indices = unselected_users.map { |u| result.index(u) }

assert selected_indices.max < unselected_indices.min,

"Selected closers should come before all unselected users"

end

# Unselected users should remain alphabetically ordered

unselected_names = unselected_users.map(&:name)

assert_equal unselected_names.sort_by(&:downcase),

unselected_names,

"Unselected users should be alphabetically ordered"

Copilot · 2026-03-23T23:45:22Z

test/models/user/filtering_test.rb

+    alice_identity = Identity.find_or_create_by!(email_address: "alice@test.com")
+    alice = @user.account.users.create!(name: "Alice", identity: alice_identity)
+
+    bob_identity = Identity.find_or_create_by!(email_address: "bob@test.com")
+    bob = @user.account.users.create!(name: "Bob", identity: bob_identity)
+


This test also uses Identity.find_or_create_by! with a static email address. Prefer Identity.create! (or a helper generating unique emails) to avoid cross-test coupling and to keep test data clearly owned by the test.

Copilot · 2026-03-23T23:45:22Z

test/models/user/filtering_test.rb

+    alice_identity = Identity.find_or_create_by!(email_address: "alice@test.com")
+    alice = @user.account.users.create!(name: "Alice", identity: alice_identity)
+
+    bob_identity = Identity.find_or_create_by!(email_address: "bob@test.com")
+    bob = @user.account.users.create!(name: "Bob", identity: bob_identity)
+


This test also uses Identity.find_or_create_by! with a static email address. Prefer Identity.create! (or a helper generating unique emails) to avoid cross-test coupling and to keep test data clearly owned by the test.

Copilot · 2026-03-23T23:45:22Z

app/models/user/filtering.rb

+
+      selected = all_users.select { |u| selected_ids.include?(u.id) }
+      unselected = all_users.reject { |u| selected_ids.include?(u.id) }
+


There are trailing spaces on the blank lines in this block (will be flagged by RuboCop's trailing whitespace checks). Please remove the whitespace on empty lines.

Suggested change

selected = all_users.select { |u| selected_ids.include?(u.id) }

unselected = all_users.reject { |u| selected_ids.include?(u.id) }

selected = all_users.select { |u| selected_ids.include?(u.id) }

unselected = all_users.reject { |u| selected_ids.include?(u.id) }

Copilot · 2026-03-23T23:45:22Z

test/models/user/filtering_test.rb

+    alice_identity = Identity.find_or_create_by!(email_address: "alice@test.com")
+    alice = @user.account.users.create!(name: "Alice", identity: alice_identity)
+
+    bob_identity = Identity.find_or_create_by!(email_address: "bob@test.com")
+    bob = @user.account.users.create!(name: "Bob", identity: bob_identity)
+


There are trailing spaces on the blank lines in this test (likely to be flagged by RuboCop). Please remove whitespace on otherwise-empty lines.

monorkin · 2026-03-26T10:14:23Z

app/models/user/filtering.rb

    end
+
+    def sort_users_by_selection(selected_users)
+      all_users = users.to_a


I'm concerned about performance here, this will load all user records into memory and then filter them there. This could cause problems on larger accounts.

Did you consider plucking the IDs of the selected users and then ordering them using a case statement in SQL?

It's more work, but it will be more efficient than loading all users into memory to filter out a few.

This could be a scope so you wouldn't need separate methods for each filter.

@monorkin

Per @monorkin's review feedback, replaced in-memory user sorting with SQL-based CASE statement for better performance on large accounts. Changes: - Added sorted_by_selection scope to User::Named - Uses CASE WHEN...THEN SQL ordering (database-side sorting) - Handles binary UUID IDs correctly (converts to blobs for SQLite) - Removed sort_users_by_selection helper (no longer needed) - All three filter methods now use the scope - Same behavior, much better performance All tests passing ✅

hnshah · 2026-03-26T17:38:37Z

Thanks for the review @monorkin! You're absolutely right about the performance concern - loading all users into memory doesn't scale well for larger accounts.

I've refactored to use SQL-based sorting as you suggested! ✅

Changes:

New sorted_by_selection scope in User::Named using CASE statement
Database does the sorting (much more efficient)
All 3 filter methods now use the scope
Removed in-memory sort_users_by_selection helper
Handles binary UUID IDs correctly for SQLite

Performance:

Before: O(n) Ruby iteration + loading all users into memory
After: Single SQL query with database-side sorting
Scales to thousands of users

All tests still passing! 🎯

Copilot AI review requested due to automatic review settings March 23, 2026 22:38

Copilot started reviewing on behalf of hnshah March 23, 2026 22:39 View session

Copilot AI reviewed Mar 23, 2026

View reviewed changes

Fix test to match case-insensitive alphabetical sorting

a87180e

User.alphabetically sorts case-insensitively (David, Jason, JZ, Kevin). Updated test assertion to verify this ordering instead of expecting case-sensitive sort. All tests now passing ✅

Copilot AI review requested due to automatic review settings March 23, 2026 23:40

Copilot started reviewing on behalf of hnshah March 23, 2026 23:41 View session

Copilot AI reviewed Mar 23, 2026

View reviewed changes

monorkin reviewed Mar 26, 2026

View reviewed changes

		alice = @user.account.users.create!(name: "Alice", email_address: "alice@test.com")
		bob = @user.account.users.create!(name: "Bob", email_address: "bob@test.com")


		selected = all_users.select { \|u\| selected_ids.include?(u.id) }
		unselected = all_users.reject { \|u\| selected_ids.include?(u.id) }

-    assert_equal "Bob", result.first.name
+    # Selected creators should appear first
+    assert_equal "Bob", result.first.name
+    # Partition result into selected and unselected users
+    selected_creators   = result.select { |u| @filter.creators.include?(u) }
+    unselected_creators = result.reject { |u| @filter.creators.include?(u) }
+    # All selected creators should come before any unselected creators
+    if selected_creators.any? && unselected_creators.any?
+      last_selected_index   = result.index(selected_creators.last)
+      first_unselected_index = result.index(unselected_creators.first)
+      assert last_selected_index < first_unselected_index,
+             "Selected creators should all appear before unselected creators"
+    end
+    # Selected creators should be alphabetically sorted
+    selected_names = selected_creators.map(&:name)
+    assert_equal selected_names.sort_by(&:downcase), selected_names,
+                 "Selected creators should be alphabetically ordered"
+    # Unselected creators should also be alphabetically sorted
+    unselected_names = unselected_creators.map(&:name)
+    assert_equal unselected_names.sort_by(&:downcase), unselected_names,
+                 "Unselected creators should be alphabetically ordered"

-    assert_equal "Alice", result.first.name
+    # Selected users (closers) should appear first
+    assert_equal "Alice", result.first.name
+    # Verify that all selected closers appear before any unselected users
+    selected_users   = result.select { |u| @filter.closers.include?(u) }
+    unselected_users = result.reject { |u| @filter.closers.include?(u) }
+    if selected_users.any? && unselected_users.any?
+      selected_indices   = selected_users.map { |u| result.index(u) }
+      unselected_indices = unselected_users.map { |u| result.index(u) }
+      assert selected_indices.max < unselected_indices.min,
+             "Selected closers should come before all unselected users"
+    end
+    # Unselected users should remain alphabetically ordered
+    unselected_names = unselected_users.map(&:name)
+    assert_equal unselected_names.sort_by(&:downcase),
+                 unselected_names,
+                 "Unselected users should be alphabetically ordered"

Conversation

hnshah commented Mar 23, 2026

Problem

Solution

Changes

Model (app/models/user/filtering.rb)

Views

Tests (test/models/user/filtering_test.rb)

Example

Impact

Notes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

hnshah commented Mar 23, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

monorkin Mar 26, 2026

Choose a reason for hiding this comment

Uh oh!

hnshah commented Mar 26, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Model (`app/models/user/filtering.rb`)

Tests (`test/models/user/filtering_test.rb`)