Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
25 changes: 25 additions & 0 deletions app/models/user/filtering.rb
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,21 @@ def users
@users ||= account.users.active.alphabetically
end

# Returns users sorted with selected assignees first
def users_for_assignee_filter
@users_for_assignee_filter ||= sort_users_by_selection(filter.assignees)
end

# Returns users sorted with selected creators first
def users_for_creator_filter
@users_for_creator_filter ||= sort_users_by_selection(filter.creators)
end

# Returns users sorted with selected closers first
def users_for_closer_filter
@users_for_closer_filter ||= sort_users_by_selection(filter.closers)
end

def filters
@filters ||= user.filters.all
end
Expand Down Expand Up @@ -82,4 +97,14 @@ def cache_key
def account
user.account
end

def sort_users_by_selection(selected_users)
all_users = users.to_a
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

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) }

Copy link

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

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

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 uses AI. Check for mistakes.
Copy link

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

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

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 uses AI. Check for mistakes.
Copy link

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

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

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 uses AI. Check for mistakes.
selected + unselected
end
end
2 changes: 1 addition & 1 deletion app/views/filters/settings/_assignees.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@
</button>
<% end %>

<% user_filtering.users.each do |user| %>
<% 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 %>
Expand Down
2 changes: 1 addition & 1 deletion app/views/filters/settings/_closers.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
<% end %>

<ul class="popup__list" data-filter-target="list" role="listbox">
<% user_filtering.users.each do |user| %>
<% 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 %>
Expand Down
2 changes: 1 addition & 1 deletion app/views/filters/settings/_creators.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
<% end %>

<ul class="popup__list" data-filter-target="list" role="listbox">
<% user_filtering.users.each do |user| %>
<% 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 %>
Expand Down
65 changes: 65 additions & 0 deletions test/models/user/filtering_test.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
require "test_helper"

class User::FilteringTest < ActiveSupport::TestCase
setup do
@user = users(:david)
@filter = @user.filters.create!
end

test "users_for_assignee_filter sorts selected assignees first" do
# Create some test users
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")
Copy link

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

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

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 uses AI. Check for mistakes.

# Select Bob and Charlie as assignees
@filter.assignees = [charlie, bob]
@filter.save!

filtering = User::Filtering.new(@user, @filter)
result = filtering.users_for_assignee_filter

# Selected users (Bob, Charlie) should appear first, both alphabetically sorted
assert_equal "Bob", result[0].name
assert_equal "Charlie", result[1].name

# Alice (unselected) should appear after selected users
assert_equal "Alice", result.find { |u| u.name == "Alice" }.name
assert result.index { |u| u.name == "Alice" } > 1, "Unselected user should appear after selected users"
end

test "users_for_creator_filter sorts selected creators first" do
alice = @user.account.users.create!(name: "Alice", email_address: "alice@test.com")
bob = @user.account.users.create!(name: "Bob", email_address: "bob@test.com")
Copy link

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

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

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 uses AI. Check for mistakes.

Comment on lines +37 to +42
Copy link

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

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

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 uses AI. Check for mistakes.
@filter.creators = [bob]
@filter.save!

filtering = User::Filtering.new(@user, @filter)
result = filtering.users_for_creator_filter

assert_equal "Bob", result.first.name
Comment on lines +48 to +49
Copy link

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

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

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 uses AI. Check for mistakes.
end

test "users_for_closer_filter sorts selected closers first" do
alice = @user.account.users.create!(name: "Alice", email_address: "alice@test.com")
bob = @user.account.users.create!(name: "Bob", email_address: "bob@test.com")
Copy link

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

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

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 uses AI. Check for mistakes.

Comment on lines +53 to +58
Copy link

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

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

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 uses AI. Check for mistakes.
@filter.closers = [alice]
@filter.save!

filtering = User::Filtering.new(@user, @filter)
result = filtering.users_for_closer_filter

assert_equal "Alice", result.first.name
Comment on lines +64 to +65
Copy link

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

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

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 uses AI. Check for mistakes.
end

test "users method returns all users alphabetically without selection sorting" do
filtering = User::Filtering.new(@user, @filter)
result = filtering.users

# Should just be alphabetical, no selection sorting
names = result.map(&:name)
assert_equal names.sort, names, "Users should be alphabetically sorted"
end
end
Loading