From f864403c2c3f85fcc22e56cb80b42080b8b4c315 Mon Sep 17 00:00:00 2001 From: Marcello Costagliola Date: Mon, 5 Oct 2026 14:56:22 +0200 Subject: [PATCH 1/2] List one page of account members at a time Since administrators were grouped apart from members (b52c318), the account settings page loads every user to split them and renders all of them. The lazy next page still starts at the 501st user, so on an account with more than 500 people, scrolling down lists those users a second time. Load administrators on their own and page only members, both on the settings page and on the pages that follow it. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_0142qgjggdJ2KDdGk7RF9Xm9 --- app/controllers/accounts/users_controller.rb | 2 +- app/controllers/accounts_controller.rb | 6 +++--- app/views/accounts/edit.html.erb | 4 ++-- test/controllers/accounts_controller_test.rb | 17 +++++++++++++++++ 4 files changed, 23 insertions(+), 6 deletions(-) diff --git a/app/controllers/accounts/users_controller.rb b/app/controllers/accounts/users_controller.rb index 8adefef..94e2d8e 100644 --- a/app/controllers/accounts/users_controller.rb +++ b/app/controllers/accounts/users_controller.rb @@ -2,7 +2,7 @@ class Accounts::UsersController < ApplicationController before_action :ensure_can_administer, :set_user, only: %i[ update destroy ] def index - set_page_and_extract_portion_from User.active.ordered.without_bots, per_page: 500 + set_page_and_extract_portion_from User.active.ordered.member, per_page: 500 end def update diff --git a/app/controllers/accounts_controller.rb b/app/controllers/accounts_controller.rb index dda9d83..0de26b8 100644 --- a/app/controllers/accounts_controller.rb +++ b/app/controllers/accounts_controller.rb @@ -3,9 +3,9 @@ class AccountsController < ApplicationController before_action :set_account def edit - users = account_users.ordered.without_bots - @administrators, @members = users.partition(&:administrator?) - set_page_and_extract_portion_from users, per_page: 500 + users = account_users.ordered + @administrators = users.administrator + set_page_and_extract_portion_from users.member, per_page: 500 end def update diff --git a/app/views/accounts/edit.html.erb b/app/views/accounts/edit.html.erb index cfa750d..f74030a 100644 --- a/app/views/accounts/edit.html.erb +++ b/app/views/accounts/edit.html.erb @@ -102,11 +102,11 @@ <%= render partial: "accounts/users/user", collection: @administrators, as: :user %> - <% if @administrators.any? && @members.any? %> + <% if @administrators.any? && @page.records.any? %>
<% end %> - <%= render partial: "accounts/users/user", collection: @members, as: :user %> + <%= render partial: "accounts/users/user", collection: @page.records, as: :user %> <%= render "accounts/users/next_page_container", page: @page.next_param unless @page.last? %> diff --git a/test/controllers/accounts_controller_test.rb b/test/controllers/accounts_controller_test.rb index 8292c6f..83c77ff 100644 --- a/test/controllers/accounts_controller_test.rb +++ b/test/controllers/accounts_controller_test.rb @@ -42,6 +42,18 @@ class AccountsControllerTest < ActionDispatch::IntegrationTest end end + test "edit lists a page of members and the next page carries on from it" do + User.insert_all 501.times.map { |i| { name: "Member #{i}", email_address: "member#{i}@37signals.com" } } + + get edit_account_url + first_page = listed_user_ids + get account_users_url(page: 2, format: :turbo_stream) + next_page = listed_user_ids + + assert_not_empty next_page + assert_equal User.active.without_bots.ids.sort, (first_page + next_page).sort + end + test "update" do assert users(:david).administrator? @@ -58,4 +70,9 @@ class AccountsControllerTest < ActionDispatch::IntegrationTest put account_url, params: { account: { name: "Different" } } assert_response :forbidden end + + private + def listed_user_ids + response.body.scan(/id="role_user_(\d+)"/).flatten.map(&:to_i) + end end From ce4c05da8166463c57f1927e39d17fe3e6ad2b5b Mon Sep 17 00:00:00 2001 From: Marcello Costagliola Date: Mon, 5 Oct 2026 15:19:21 +0200 Subject: [PATCH 2/2] Apply the same user filter to every page of account members Administrators see banned users in the account settings, but only the first page counted them: the next pages listed active users alone. A banned member before the page boundary shifted the offset, so one active member was never listed. Both pages now share User.visible_to. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_0142qgjggdJ2KDdGk7RF9Xm9 --- app/controllers/accounts/users_controller.rb | 2 +- app/controllers/accounts_controller.rb | 10 +--------- app/models/user/bannable.rb | 5 +++++ test/controllers/accounts_controller_test.rb | 5 +++-- test/models/user/bannable_test.rb | 10 ++++++++++ 5 files changed, 20 insertions(+), 12 deletions(-) create mode 100644 test/models/user/bannable_test.rb diff --git a/app/controllers/accounts/users_controller.rb b/app/controllers/accounts/users_controller.rb index 94e2d8e..128a27b 100644 --- a/app/controllers/accounts/users_controller.rb +++ b/app/controllers/accounts/users_controller.rb @@ -2,7 +2,7 @@ class Accounts::UsersController < ApplicationController before_action :ensure_can_administer, :set_user, only: %i[ update destroy ] def index - set_page_and_extract_portion_from User.active.ordered.member, per_page: 500 + set_page_and_extract_portion_from User.visible_to(Current.user).ordered.member, per_page: 500 end def update diff --git a/app/controllers/accounts_controller.rb b/app/controllers/accounts_controller.rb index 0de26b8..2043d20 100644 --- a/app/controllers/accounts_controller.rb +++ b/app/controllers/accounts_controller.rb @@ -3,7 +3,7 @@ class AccountsController < ApplicationController before_action :set_account def edit - users = account_users.ordered + users = User.visible_to(Current.user).ordered @administrators = users.administrator set_page_and_extract_portion_from users.member, per_page: 500 end @@ -21,12 +21,4 @@ class AccountsController < ApplicationController def account_params params.require(:account).permit(:name, :logo, settings: {}) end - - def account_users - if Current.user.can_administer? - User.where(status: [ :active, :banned ]) - else - User.active - end - end end diff --git a/app/models/user/bannable.rb b/app/models/user/bannable.rb index 9d714ab..594993d 100644 --- a/app/models/user/bannable.rb +++ b/app/models/user/bannable.rb @@ -1,6 +1,11 @@ module User::Bannable extend ActiveSupport::Concern + included do + # Administrators still see banned users, so they can lift the ban. + scope :visible_to, ->(user) { user.can_administer? ? where(status: %i[ active banned ]) : active } + end + def ban transaction do create_bans_from_sessions diff --git a/test/controllers/accounts_controller_test.rb b/test/controllers/accounts_controller_test.rb index 83c77ff..12ea1e3 100644 --- a/test/controllers/accounts_controller_test.rb +++ b/test/controllers/accounts_controller_test.rb @@ -44,6 +44,7 @@ class AccountsControllerTest < ActionDispatch::IntegrationTest test "edit lists a page of members and the next page carries on from it" do User.insert_all 501.times.map { |i| { name: "Member #{i}", email_address: "member#{i}@37signals.com" } } + users(:kevin).banned! get edit_account_url first_page = listed_user_ids @@ -51,7 +52,7 @@ class AccountsControllerTest < ActionDispatch::IntegrationTest next_page = listed_user_ids assert_not_empty next_page - assert_equal User.active.without_bots.ids.sort, (first_page + next_page).sort + assert_equal User.where(status: [ :active, :banned ]).without_bots.ids.sort, (first_page + next_page).sort end test "update" do @@ -73,6 +74,6 @@ class AccountsControllerTest < ActionDispatch::IntegrationTest private def listed_user_ids - response.body.scan(/id="role_user_(\d+)"/).flatten.map(&:to_i) + response.body.scan(%r{href="/users/(\d+)"}).flatten.map(&:to_i) end end diff --git a/test/models/user/bannable_test.rb b/test/models/user/bannable_test.rb new file mode 100644 index 0000000..5347c76 --- /dev/null +++ b/test/models/user/bannable_test.rb @@ -0,0 +1,10 @@ +require "test_helper" + +class User::BannableTest < ActiveSupport::TestCase + test "banned users are visible to administrators only" do + users(:kevin).banned! + + assert_includes User.visible_to(users(:david)), users(:kevin) + assert_not_includes User.visible_to(users(:jz)), users(:kevin) + end +end