From 370786fee41f8c0b91a8026f8d206362aad95bee Mon Sep 17 00:00:00 2001 From: Yifei Lu Date: Wed, 26 Aug 2026 09:21:36 -0400 Subject: [PATCH 1/3] AO3-7468 Preload user roles from session --- app/models/user.rb | 6 ++++++ spec/models/user_spec.rb | 26 ++++++++++++++++++++++++++ 2 files changed, 32 insertions(+) diff --git a/app/models/user.rb b/app/models/user.rb index 4dcabe22b5f..5af23e989f7 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -274,6 +274,12 @@ def self.find_first_by_auth_conditions(tainted_conditions, options = {}) relation.first end + # Override of Devise method to preload roles for permission checks. + def self.serialize_from_session(key, salt) + record = includes(:roles).where(primary_key => key).first + record if record && record.authenticatable_salt == salt + end + def self.for_claims(claims_ids) joins(:request_claims) .where("challenge_claims.id IN (?)", claims_ids) diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index e02b1b4efe2..2326c0f91bc 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -1,6 +1,32 @@ require "spec_helper" describe User do + describe ".serialize_from_session" do + let(:role) { create(:role) } + let(:user) { create(:user, roles: [role]) } + + it "returns the user with roles loaded when the salt matches" do + deserialized_user = described_class.serialize_from_session(user.to_key, user.authenticatable_salt) + + expect(deserialized_user).to eq(user) + expect(deserialized_user.association(:roles)).to be_loaded + expect(deserialized_user.roles).to contain_exactly(role) + end + + it "loads the roles association when the user has no roles" do + user = create(:user) + + deserialized_user = described_class.serialize_from_session(user.to_key, user.authenticatable_salt) + + expect(deserialized_user.association(:roles)).to be_loaded + expect(deserialized_user.roles).to be_empty + end + + it "returns nil when the salt does not match" do + expect(described_class.serialize_from_session(user.to_key, "invalid salt")).to be_nil + end + end + describe "audits" do let(:user) { create(:user) } From a5f1745b1faf0793bc2f47dbb5bdc04be627b2d2 Mon Sep 17 00:00:00 2001 From: Yifei Lu Date: Wed, 26 Aug 2026 09:22:40 -0400 Subject: [PATCH 2/3] AO3-7468 Use preloaded roles in admin views --- app/controllers/admin/admin_users_controller.rb | 2 +- app/views/admin/admin_users/_user_form.html.erb | 2 +- spec/controllers/admin/admin_users_controller_spec.rb | 8 ++++++++ spec/models/user_spec.rb | 2 ++ 4 files changed, 12 insertions(+), 2 deletions(-) diff --git a/app/controllers/admin/admin_users_controller.rb b/app/controllers/admin/admin_users_controller.rb index 0df19517da9..f877e8d4dcb 100644 --- a/app/controllers/admin/admin_users_controller.rb +++ b/app/controllers/admin/admin_users_controller.rb @@ -11,7 +11,7 @@ def set_roles end def load_user - @user = User.find_by!(login: params[:id]) + @user = User.includes(:roles).find_by!(login: params[:id]) end def user_is_banned diff --git a/app/views/admin/admin_users/_user_form.html.erb b/app/views/admin/admin_users/_user_form.html.erb index addc627d6f4..ad445bb231e 100644 --- a/app/views/admin/admin_users/_user_form.html.erb +++ b/app/views/admin/admin_users/_user_form.html.erb @@ -12,7 +12,7 @@ <%= text_field_tag "user[email]", user.email, title: ts("Email"), disabled: !admin_can_update_user_email? %> <% for role in @roles %> - <%= check_box_tag "user[roles][]", role.id, user.roles.include?(role), title: role.name, id: "user_roles_#{role.id}", disabled: !policy(User).can_edit_user_role?(role) %> + <%= check_box_tag "user[roles][]", role.id, user.has_role?(role.name), title: role.name, id: "user_roles_#{role.id}", disabled: !policy(User).can_edit_user_role?(role) %> <% end %> diff --git a/spec/controllers/admin/admin_users_controller_spec.rb b/spec/controllers/admin/admin_users_controller_spec.rb index 320b6c5217b..b689dd81d1f 100644 --- a/spec/controllers/admin/admin_users_controller_spec.rb +++ b/spec/controllers/admin/admin_users_controller_spec.rb @@ -160,6 +160,14 @@ expect(response).to have_http_status(:success) end + it "preloads the user's roles" do + fake_login_admin(admin) + expect(User).to receive(:includes).with(:roles).and_call_original + get :show, params: { id: user.login } + + expect(assigns(:user).association(:roles)).to be_loaded + end + it "if user does not exist, raises a 404" do fake_login_admin(admin) params = { id: "not_existing_id" } diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 2326c0f91bc..9e20a571562 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -11,6 +11,8 @@ expect(deserialized_user).to eq(user) expect(deserialized_user.association(:roles)).to be_loaded expect(deserialized_user.roles).to contain_exactly(role) + expect(Role).not_to receive(:find_by) + expect(deserialized_user.has_role?(role.name)).to be(true) end it "loads the roles association when the user has no roles" do From 67bc6f1e58bcf1aca20cdaaf89475576d3478013 Mon Sep 17 00:00:00 2001 From: Yifei Lu Date: Fri, 4 Sep 2026 01:06:03 -0400 Subject: [PATCH 3/3] AO3-7468 Add request coverage for role query counts --- .../admin/admin_users_controller.rb | 2 +- .../admin/admin_users_controller_spec.rb | 8 ----- spec/models/user_spec.rb | 2 -- spec/requests/admin_users_n_plus_one_spec.rb | 32 +++++++++++++++++++ 4 files changed, 33 insertions(+), 11 deletions(-) create mode 100644 spec/requests/admin_users_n_plus_one_spec.rb diff --git a/app/controllers/admin/admin_users_controller.rb b/app/controllers/admin/admin_users_controller.rb index f877e8d4dcb..06b7ac338a8 100644 --- a/app/controllers/admin/admin_users_controller.rb +++ b/app/controllers/admin/admin_users_controller.rb @@ -248,6 +248,6 @@ def search_params end def log_items - @log_items ||= @user.log_items.sort_by(&:created_at).reverse + @log_items ||= @user.log_items.includes(:role).sort_by(&:created_at).reverse end end diff --git a/spec/controllers/admin/admin_users_controller_spec.rb b/spec/controllers/admin/admin_users_controller_spec.rb index b689dd81d1f..320b6c5217b 100644 --- a/spec/controllers/admin/admin_users_controller_spec.rb +++ b/spec/controllers/admin/admin_users_controller_spec.rb @@ -160,14 +160,6 @@ expect(response).to have_http_status(:success) end - it "preloads the user's roles" do - fake_login_admin(admin) - expect(User).to receive(:includes).with(:roles).and_call_original - get :show, params: { id: user.login } - - expect(assigns(:user).association(:roles)).to be_loaded - end - it "if user does not exist, raises a 404" do fake_login_admin(admin) params = { id: "not_existing_id" } diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 9e20a571562..2326c0f91bc 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -11,8 +11,6 @@ expect(deserialized_user).to eq(user) expect(deserialized_user.association(:roles)).to be_loaded expect(deserialized_user.roles).to contain_exactly(role) - expect(Role).not_to receive(:find_by) - expect(deserialized_user.has_role?(role.name)).to be(true) end it "loads the roles association when the user has no roles" do diff --git a/spec/requests/admin_users_n_plus_one_spec.rb b/spec/requests/admin_users_n_plus_one_spec.rb new file mode 100644 index 00000000000..c3453c8089f --- /dev/null +++ b/spec/requests/admin_users_n_plus_one_spec.rb @@ -0,0 +1,32 @@ +# frozen_string_literal: true + +require "spec_helper" + +describe "n+1 queries in the admin users controller" do + include LoginMacros + + describe "#show", n_plus_one: true do + let!(:user) { create(:user) } + let!(:admin) { create(:policy_and_abuse_admin) } + + before { fake_login_admin(admin) } + + populate do |n| + role_names = %w[archivist no_resets official opendoors protected_user] + # Assigning roles also creates the role-change history rendered on this page. + user.reload.roles = role_names.first(n).map { |name| create(:role, name: name) } + end + + warmup { get admin_user_path(user) } + + it "performs a constant number of role queries" do + expect do + get admin_user_path(user) + expect(response).to have_http_status(:success) + expect(response.body).to include("user_history") + end.to perform_constant_number_of_queries + .matching(/\b(?:roles|roles_users)\b/) + .with_scale_factors(2, 5) + end + end +end