Repository navigation
Fix n+1 in upcoming_events_for_user - #3074
davidmillen50 wants to merge 5 commits into
Conversation
…ery-upcoming-events-for-user
|
Reviewed with a multi-persona review pass. The fix itself looks correct: the preload target ( 1. The chapter-workshops branch has no host/query-count regression test (P2, All three new examples run with a member whose Suggestion: add one example for it 'preloads workshop hosts for chapter workshops without growing query count' do
chapter = Fabricate(:chapter)
member = Fabricate(:member, groups: [Fabricate(:students, chapter:)])
host = Fabricate(:sponsor)
extra_sponsor = Fabricate(:sponsor)
create_chapter_workshops(chapter:, count: 1, host:, extra_sponsor:)
_, one_query_count = count_queries { described_class.upcoming_events_for_user(member) }
create_chapter_workshops(chapter:, count: 3, host:, extra_sponsor:)
result, four_query_count = count_queries { described_class.upcoming_events_for_user(member) }
expect(result.values.flatten.count { |event| event.is_a?(Workshop) }).to eq(4)
expect(result.values.flatten.select { |e| e.is_a?(Workshop) }.map(&:host)).to all(eq(host))
expect(four_query_count).to eq(one_query_count)
endwith a 2. The standalone multi-sponsor example duplicates the count-stability example (P3, The Suggestion: either drop the standalone multi-sponsor example and let the count-stability example own that contract, or keep the standalone pair ( Two smaller notes, no action needed for this PR:
|
… and reduce query count test's strictness
|
Points 1 and 2 have been addressed and I have reduced the query count strictness issue you raised below so it is more inline with the repo's other examples.
|

Related to issue #3070
Implement the same fix as #3066 to use preload logic similar to
.preload(workshop_host: :sponsor)to fix an issue whereeager_loadingthe workshop_host returns nil or points at a non-host sponsor