From 2380b3c776697f85c369de6f53460c593c682fec Mon Sep 17 00:00:00 2001 From: Morgan Roderick Date: Sat, 8 Aug 2026 19:06:42 +0200 Subject: [PATCH] perf: fix N+1 queries on admin workshop show The admin workshop show page issued ~6 scoped workshop_invitations queries per attending row (newbie?, flag_to_organisers?, recent_notes) because with_notes_and_their_authors preloads member_notes and attendance_warnings but not the member's other workshop invitations. For /admin/workshops/3824 this was 210 queries / 51s DB / 40s view / 221M allocations (~92s total). Collapse the three per-row lookups into a bounded query object (AdminWorkshopAttendeeFlags.for_members) computed once and memoised on the loaded member instances. 27 attendees now need 5 queries instead of ~159. Fixes #2796 --- .../concerns/admin/workshop_concerns.rb | 19 ++-- app/models/member.rb | 13 +++ app/presenters/member_presenter.rb | 2 + app/queriers/admin_workshop_attendee_flags.rb | 102 ++++++++++++++++++ .../admin/workshops_controller_spec.rb | 24 +++++ .../admin_workshop_attendee_flags_spec.rb | 81 ++++++++++++++ 6 files changed, 235 insertions(+), 6 deletions(-) create mode 100644 app/queriers/admin_workshop_attendee_flags.rb create mode 100644 spec/queriers/admin_workshop_attendee_flags_spec.rb diff --git a/app/controllers/concerns/admin/workshop_concerns.rb b/app/controllers/concerns/admin/workshop_concerns.rb index f9cbb2a08..49c64736f 100644 --- a/app/controllers/concerns/admin/workshop_concerns.rb +++ b/app/controllers/concerns/admin/workshop_concerns.rb @@ -7,12 +7,13 @@ module Admin::WorkshopConcerns module InstanceMethods def set_admin_workshop_data - @attending_students = InvitationPresenter.decorate_collection( - @workshop.attending_students.all.with_notes_and_their_authors - ) - @attending_coaches = InvitationPresenter.decorate_collection( - @workshop.attending_coaches.all.with_notes_and_their_authors - ) + students = @workshop.attending_students.all.with_notes_and_their_authors + coaches = @workshop.attending_coaches.all.with_notes_and_their_authors + + inject_attendance_flags(students, coaches) + + @attending_students = InvitationPresenter.decorate_collection(students) + @attending_coaches = InvitationPresenter.decorate_collection(coaches) @coach_waiting_list = WaitingListPresenter.new( WaitingList.coaches_for(@workshop).with_notes_and_their_authors @@ -22,6 +23,12 @@ def set_admin_workshop_data ) end + def inject_attendance_flags(*collections) + members = collections.flat_map { |collection| collection.map(&:member) }.uniq(&:id) + flags = AdminWorkshopAttendeeFlags.for_members(members.map(&:id)) + members.each { |member| member.admin_workshop_flags = flags[member.id] } + end + private def set_workshop diff --git a/app/models/member.rb b/app/models/member.rb index bef9bddf8..b44ea7276 100644 --- a/app/models/member.rb +++ b/app/models/member.rb @@ -25,6 +25,12 @@ class Member < ApplicationRecord has_many :member_notes has_many :chapters, -> { distinct }, through: :groups has_many :announcements, -> { distinct }, through: :groups + + # Per-request aggregate results for the admin workshop attendance page, set by + # set_admin_workshop_data. When present, the flag helpers below avoid issuing + # one query per attending row. nil elsewhere -> fall back to live queries. + attr_accessor :admin_workshop_flags + has_many :meeting_invitations has_many :member_email_deliveries @@ -164,6 +170,8 @@ def clear_attending_event_ids_cache! end def flag_to_organisers? + return admin_workshop_flags[:flag_to_organisers] if admin_workshop_flags + multiple_no_shows? && attendance_warnings.last_six_months.length >= 2 end @@ -173,6 +181,11 @@ def multiple_no_shows? end def recent_notes + if admin_workshop_flags + # Only used as recent_notes.any? on the attendance rows + return (admin_workshop_flags[:recent_notes] ? [:note] : []) + end + last_five_workshops = workshop_invitations.order_by_latest.attended.take(5) return [] if last_five_workshops.empty? diff --git a/app/presenters/member_presenter.rb b/app/presenters/member_presenter.rb index b450ec78e..7a334b5ec 100644 --- a/app/presenters/member_presenter.rb +++ b/app/presenters/member_presenter.rb @@ -11,6 +11,8 @@ def event_organiser?(event) end def newbie? + return model.admin_workshop_flags[:newbie] if model.admin_workshop_flags + !workshop_invitations.attended.exists? end diff --git a/app/queriers/admin_workshop_attendee_flags.rb b/app/queriers/admin_workshop_attendee_flags.rb new file mode 100644 index 000000000..ff1e6bf2c --- /dev/null +++ b/app/queriers/admin_workshop_attendee_flags.rb @@ -0,0 +1,102 @@ +# Computes, in a bounded number of queries, the per-attendee flags the admin +# workshop show page renders (newbie / flag-to-organisers / recent-notes). +# Replaces per-row N+1 queries (Member#flag_to_organisers?, #recent_notes and +# MemberPresenter#newbie?) with a small set of aggregate queries. +class AdminWorkshopAttendeeFlags + def self.for_members(member_ids) + new(member_ids).to_h + end + + def initialize(member_ids) + @member_ids = member_ids + end + + def to_h + return {} if @member_ids.empty? + + @member_ids.index_with do |id| + { + newbie: newbies.include?(id), + flag_to_organisers: flag_for?(id), + recent_notes: recent_notes_member_ids.include?(id) + } + end + end + + private + + def newbies + @newbies ||= @member_ids - attended_ever + end + + def attended_ever + WorkshopInvitation.where(member_id: @member_ids, attended: true).distinct.pluck(:member_id) + end + + def flag_for?(member_id) + (no_shows.fetch(member_id, 0) > 3) && (warning_counts.fetch(member_id, 0) >= 2) + end + + # Accepted (attending) invitations for taken-place workshops in the last six + # months, grouped by member (mirrors Member#multiple_no_shows?). + def accepted_counts + @accepted_counts ||= six_months_taken_place.group(:member_id).count + end + + def attended_counts + @attended_counts ||= six_months_taken_place.where(attended: true).group(:member_id).count + end + + def no_shows + @no_shows ||= accepted_counts.merge(attended_counts) do |_id, accepted, attended| + accepted - attended + end + end + + def warning_counts + @warning_counts ||= AttendanceWarning.where(member_id: @member_ids) + .last_six_months + .group(:member_id) + .count + end + + def six_months_taken_place + WorkshopInvitation.joins(:workshop) + .where(member_id: @member_ids, attending: true) + .where(workshops: { date_and_time: 6.months.ago...Time.zone.now }) + end + + # Members with a note created after the (date - 1 day) of their fifth most + # recent attended workshop, replicating Member#recent_notes. + RECENT_NOTES_SQL = <<~SQL.freeze + SELECT DISTINCT mn.member_id + FROM member_notes mn + JOIN ( + SELECT member_id, (MIN(d.date_and_time) - INTERVAL '1 day') AS cutoff + FROM ( + SELECT wi.member_id, ws.date_and_time, + ROW_NUMBER() OVER ( + PARTITION BY wi.member_id ORDER BY ws.date_and_time DESC + ) AS rn + FROM workshop_invitations wi + JOIN workshops ws ON ws.id = wi.workshop_id + WHERE wi.member_id IN (%s) + AND wi.attended = TRUE + AND ws.date_and_time IS NOT NULL + ) d + WHERE d.rn <= 5 + GROUP BY d.member_id + ) c ON c.member_id = mn.member_id + WHERE mn.created_at > c.cutoff + SQL + + def recent_notes_member_ids + @recent_notes_member_ids ||= begin + rows = WorkshopInvitation.connection.select_rows( + format(RECENT_NOTES_SQL, member_ids: @member_ids.join(',')), + 'admin-workshop-recent-notes' + ) + rows.flatten.map(&:to_i) + end + end +end diff --git a/spec/controllers/admin/workshops_controller_spec.rb b/spec/controllers/admin/workshops_controller_spec.rb index 0361c0445..f87e3333b 100644 --- a/spec/controllers/admin/workshops_controller_spec.rb +++ b/spec/controllers/admin/workshops_controller_spec.rb @@ -6,6 +6,13 @@ login_as_organiser(admin, workshop.chapter) end + def count_queries(&block) + n = 0 + callback = ->(*) { n += 1 } + ActiveSupport::Notifications.subscribed(callback, 'sql.active_record', &block) + n + end + describe 'GET #show' do it 'loads the workshop attendance page with attendees' do Fabricate(:workshop_invitation, workshop: workshop, attending: true) @@ -13,6 +20,23 @@ expect(response).to have_http_status(:success) end + + it 'loads the attendance page in a bounded number of queries, regardless of attendee count' do + attendee = Fabricate(:member) + 4.times { Fabricate(:past_attending_workshop_invitation, member: attendee) } + 2.times { Fabricate(:attendance_warning, member: attendee) } + Fabricate(:member_note, member: attendee, created_at: 1.day.ago) + Fabricate(:workshop_invitation, workshop: workshop, member: attendee, attending: true, role: 'Student') + Fabricate(:workshop_invitation, workshop: workshop, attending: true, role: 'Coach') + + # adds a second attendee to catch per-row scaling + Fabricate(:workshop_invitation, workshop: workshop, attending: true, role: 'Student') + + count = count_queries { get :show, params: { id: workshop.id } } + + expect(response).to have_http_status(:success) + expect(count).to be < 50 + end end describe 'POST #create' do diff --git a/spec/queriers/admin_workshop_attendee_flags_spec.rb b/spec/queriers/admin_workshop_attendee_flags_spec.rb new file mode 100644 index 000000000..8c0c71934 --- /dev/null +++ b/spec/queriers/admin_workshop_attendee_flags_spec.rb @@ -0,0 +1,81 @@ +RSpec.describe AdminWorkshopAttendeeFlags do + subject(:flags) { described_class.for_members([member.id])[member.id] } + + let(:workshop) { Fabricate(:workshop) } + let(:member) { Fabricate(:member) } + + def count_queries(&block) + n = 0 + callback = ->(*) { n += 1 } + ActiveSupport::Notifications.subscribed(callback, 'sql.active_record', &block) + n + end + + describe '.for_members' do + context 'when determining if a member is a newbie' do + it 'is true when the member has never attended a workshop' do + Fabricate(:attending_workshop_invitation, member: member, workshop: workshop) + + expect(flags[:newbie]).to be(true) + end + + it 'is false when the member has attended a workshop in the past' do + Fabricate(:attended_workshop_invitation, member: member) + + expect(flags[:newbie]).to be(false) + end + end + + context 'when determining the flag to organisers' do + it 'is true when the member has multiple no-shows and two recent warnings' do + 4.times { Fabricate(:past_attending_workshop_invitation, member: member) } + 2.times { Fabricate(:attendance_warning, member: member) } + + expect(flags[:flag_to_organisers]).to be(true) + end + + it 'is false when the member has few no-shows' do + Fabricate(:past_attending_workshop_invitation, member: member) + 2.times { Fabricate(:attendance_warning, member: member) } + + expect(flags[:flag_to_organisers]).to be(false) + end + + it 'is false when the member has no recent warnings' do + 4.times { Fabricate(:past_attending_workshop_invitation, member: member) } + + expect(flags[:flag_to_organisers]).to be(false) + end + end + + context 'when determining recent notes' do + it 'is true when a note exists after the member\'s fifth most recent attended workshop' do + 5.times { Fabricate(:attended_workshop_invitation, member: member) } + + Fabricate(:member_note, member: member, created_at: 1.day.ago) + + expect(flags[:recent_notes]).to be(true) + end + + it 'is false when there are no notes' do + 5.times { Fabricate(:attended_workshop_invitation, member: member) } + + expect(flags[:recent_notes]).to be(false) + end + end + + it 'runs a constant number of queries regardless of member count' do + members = Array.new(5) { Fabricate(:member) } + members.each do |current_member| + 4.times { Fabricate(:past_attending_workshop_invitation, member: current_member) } + 2.times { Fabricate(:attendance_warning, member: current_member) } + Fabricate(:member_note, member: current_member, created_at: 1.day.ago) + end + + one = count_queries { described_class.for_members(members.take(1).map(&:id)) } + many = count_queries { described_class.for_members(members.map(&:id)) } + + expect(many).to be <= one + 2 + end + end +end