diff --git a/app/controllers/accounts/users_controller.rb b/app/controllers/accounts/users_controller.rb index 8adefef..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.without_bots, 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 dda9d83..2043d20 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 = User.visible_to(Current.user).ordered + @administrators = users.administrator + set_page_and_extract_portion_from users.member, per_page: 500 end def update @@ -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/app/views/accounts/edit.html.erb b/app/views/accounts/edit.html.erb index f2d1ae8..9fc6bf2 100644 --- a/app/views/accounts/edit.html.erb +++ b/app/views/accounts/edit.html.erb @@ -101,11 +101,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 8cbe222..ca06102 100644 --- a/test/controllers/accounts_controller_test.rb +++ b/test/controllers/accounts_controller_test.rb @@ -52,6 +52,19 @@ 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" } } + users(:kevin).banned! + + 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.where(status: [ :active, :banned ]).without_bots.ids.sort, (first_page + next_page).sort + end + test "update" do assert users(:david).administrator? @@ -68,4 +81,9 @@ class AccountsControllerTest < ActionDispatch::IntegrationTest put account_url, params: { account: { name: "Different" } } assert_response :forbidden end + + private + def listed_user_ids + 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