Skip to content

Fix n+1 in upcoming_events_for_user - #3074

Open
davidmillen50 wants to merge 5 commits into
codebar:masterfrom
davidmillen50:3070-preload-workshop-host-in-dashboard-query-upcoming-events-for-user
Open

davidmillen50 wants to merge 5 commits into
codebar:masterfrom
davidmillen50:3070-preload-workshop-host-in-dashboard-query-upcoming-events-for-user

Conversation

@davidmillen50

@davidmillen50 davidmillen50 commented Nov 2, 2026 •

Copy link
Copy Markdown
Contributor

Related to issue #3070

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

@mroderick

Copy link
Copy Markdown
Collaborator

Reviewed with a multi-persona review pass. The fix itself looks correct: the preload target (workshop_host → sponsor) matches how Workshop#host resolves, and the pattern follows existing usage in listable.rb and events_feed_query.rb. Two findings, both about test coverage, neither blocking:

1. The chapter-workshops branch has no host/query-count regression test (P2, app/queries/dashboard_query.rb:59)

All three new examples run with a member whose chapters are empty, so they only exercise accepted_workshops_for. The preload added to chapter_workshops_for is half of this fix, and nothing guards it: the only chapter-owning example predates the PR and asserts no host behaviour.

Suggestion: add one example for .upcoming_events_for_user where the member's chapter owns a multi-sponsor workshop, and assert both the resolved host and query-count stability:

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)
end

with a create_chapter_workshops helper mirroring create_invited_workshops but fabricating workshop_no_sponsor with chapter: instead of the invitation.

2. The standalone multi-sponsor example duplicates the count-stability example (P3, spec/queries/dashboard_query_spec.rb:182)

The load_user_events_and_count_host_queries helper already asserts expect(workshops.map(&:host)).to all(eq(host)) over workshops built with the same extra-sponsor + host shape, so the :182 example adds no distinct risk.

Suggestion: either drop the standalone multi-sponsor example and let the count-stability example own that contract, or keep the standalone pair (:182 and the nil-host one) as explicit documentation and accept the overlap. The nil-host example is not redundant — keep it either way.

Two smaller notes, no action needed for this PR:

  • The exact query-count assertion (four == one) is stricter than the expect(many).to be <= one + 2 pattern used elsewhere in this repo. It is deterministic today, but it will break if unrelated queries join the path. Your call whether the strictness is worth it.
  • chapter_workshops_for has no LIMIT, unlike upcoming_workshops' MAX_WORKSHOP_QUERY, and all_events re-issues its Meeting/Event queries on every call. Both pre-date this PR; worth separate issues if member chapter counts grow.

@davidmillen50

Copy link
Copy Markdown
Contributor Author

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.

  • The exact query-count assertion (four == one) is stricter than the expect(many).to be <= one + 2 pattern used elsewhere in this repo. It is deterministic today, but it will break if unrelated queries join the path. Your call whether the strictness is worth it.

@davidmillen50

Copy link
Copy Markdown
Contributor Author

Testing

The dashboard shows three test workshops I created:

UI TEST — Chapter workshop with host is included through my chapter subscription.
UI TEST — Accepted invitation with host is included through my accepted invitation.
UI TEST — Accepted invitation with no host is also included through an accepted invitation, even though no host is designated.

Seeing these cards confirms that both workshop paths appear on the dashboard and that a missing host doesn’t prevent a card from rendering. The query-count improvement is verified by the automated RSpec tests, not by viewing the dashboard.

image

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants