From 5d81d111a2ab5bf6590ad20ca1245a95370f542d Mon Sep 17 00:00:00 2001 From: David M <207745043+davidmillen50@users.noreply.github.com> Date: Tue, 13 Oct 2026 21:03:57 +0100 Subject: [PATCH 1/2] refactor: extract events feed query from events controller into query object chore: add query file chore: add more event feed query tests --- app/controllers/events_controller.rb | 112 +------------- app/queries/events_feed_query.rb | 114 ++++++++++++++ spec/controllers/events_controller_spec.rb | 77 ---------- spec/queries/events_feed_query_spec.rb | 167 +++++++++++++++++++++ 4 files changed, 283 insertions(+), 187 deletions(-) create mode 100644 app/queries/events_feed_query.rb create mode 100644 spec/queries/events_feed_query_spec.rb diff --git a/app/controllers/events_controller.rb b/app/controllers/events_controller.rb index e44839606..cc66ab51e 100644 --- a/app/controllers/events_controller.rb +++ b/app/controllers/events_controller.rb @@ -14,7 +14,7 @@ def upcoming # (mirrors workshops#show). return if performed? - @events, @pagy = fetch_upcoming_events + @events, @pagy = EventsFeedQuery.call(upcoming: true, page: requested_page, request:) end def past @@ -29,7 +29,7 @@ def past # key alone cannot see them — the short expiry bounds that staleness. return if @past_events_page_html - @past_events, @pagy = fetch_past_events + @past_events, @pagy = EventsFeedQuery.call(upcoming: false, page: requested_page, request:) # Only mint a key for a page that exists: bogus page numbers would # otherwise flood the cache store with garbage keys, and Solid Cache # evicts oldest entries globally when over max_size. @@ -111,112 +111,4 @@ def find_or_create_invitation(event, member, role) def set_event @event = Event.find_by!(slug: params[:event_id]) end - - def fetch_upcoming_events - result = paginated_events(upcoming: true) - return [{}, nil] if result.nil? - - rows, pagy = result - events = load_events(rows) - grouped = events.group_by(&:date) - decorated = grouped.transform_values { |items| EventPresenter.decorate_collection(items) } - - [decorated, pagy] - end - - def fetch_past_events - result = paginated_events(upcoming: false) - return [{}, nil] if result.nil? - - rows, pagy = result - events = load_events(rows) - grouped = events.group_by(&:date) - decorated = grouped.transform_values { |items| EventPresenter.decorate_collection(items) } - - [decorated, pagy] - end - - # Builds a UNION ALL query across Workshops, Meetings, and Events, - # paginates at the database level, and returns (id, event_type) pairs - # for the current page. Only the 20 visible rows come back from the DB. - def paginated_events(upcoming:) - now = Time.zone.now - page = requested_page - direction = upcoming ? 'ASC' : 'DESC' - comparator = upcoming ? :gteq : :lt - - # Pure Arel subqueries — no ActiveRecord relation bind params to leak - w = Arel::Table.new(:workshops) - ch = Arel::Table.new(:chapters) - ws = Arel::SelectManager.new(w) - ws.project(w[:id], w[:date_and_time], Arel.sql("'Workshop' AS event_type")) - ws.join(ch).on(w[:chapter_id].eq(ch[:id])) - ws.where(ch[:active].eq(true).and(w[:date_and_time].public_send(comparator, now))) - - m = Arel::Table.new(:meetings) - ms = Arel::SelectManager.new(m) - ms.project(m[:id], m[:date_and_time], Arel.sql("'Meeting' AS event_type")) - ms.where(m[:date_and_time].public_send(comparator, now)) - - e = Arel::Table.new(:events) - es = Arel::SelectManager.new(e) - es.project(e[:id], e[:date_and_time], Arel.sql("'Event' AS event_type")) - es.where(e[:date_and_time].public_send(comparator, now)) - - union = Arel::Nodes::UnionAll.new( - Arel::Nodes::UnionAll.new(ws, ms), es - ) - - # COUNT at DB level — single query - count_query = Arel::SelectManager.new - count_query.from(union.as('events')) - count_query.project(Arel.star.count) - total = ActiveRecord::Base.connection.select_value(count_query.to_sql).to_i - return nil if total.zero? - - pagy_opts = { count: total, page:, limit: 20, request: } - pagy_opts[:request] = Pagy::Request.new(pagy_opts) - pagy = Pagy::Offset.new(**pagy_opts) - - # Only 20 rows leave the database - pagination_query = Arel::SelectManager.new - pagination_query.from(union.as('events')) - pagination_query.project(:id, :event_type) - pagination_query.order(Arel.sql("date_and_time #{direction}")) - pagination_query.skip(pagy.offset).take(pagy.limit) - - rows = ActiveRecord::Base.connection.select_all(pagination_query.to_sql) - [rows, pagy] - end - - # Loads full ActiveRecord objects for the paginated (id, event_type) pairs - # with eager loading, preserving the UNION order. - def load_events(rows) - grouped = rows.each_with_object({}) do |row, hash| - (hash[row['event_type']] ||= []) << row['id'].to_i - end - - # workshop_host must be preloaded: eager_loading it alongside :sponsors - # routes both through the workshop_sponsors table in one join, and the - # host scope then binds to the wrong table alias — the host comes back - # nil or points at a non-host sponsor. Same pattern as Listable.most_recent. - workshops = Workshop.eager_load(:sponsors, :organisers, - chapter: { permissions: :members }) - .preload(workshop_host: :sponsor) - .where(id: grouped['Workshop']) - .to_a.index_by(&:id) - meetings = Meeting.eager_load(:venue, :organisers).where(id: grouped['Meeting']) - .to_a.index_by(&:id) - events = Event.eager_load(:venue, :sponsors, :organisers) - .where(id: grouped['Event']) - .to_a.index_by(&:id) - - rows.filter_map do |row| - case row['event_type'] - when 'Workshop' then workshops[row['id'].to_i] - when 'Meeting' then meetings[row['id'].to_i] - when 'Event' then events[row['id'].to_i] - end - end - end end diff --git a/app/queries/events_feed_query.rb b/app/queries/events_feed_query.rb new file mode 100644 index 000000000..d8d1c8814 --- /dev/null +++ b/app/queries/events_feed_query.rb @@ -0,0 +1,114 @@ +require 'pagy/classes/request' + +class EventsFeedQuery + def self.call(...) + new(...).call + end + + def initialize(upcoming:, page:, request:) + @upcoming = upcoming + @page = page + @request = request + end + + def call + fetch_events(upcoming: @upcoming) + end + + private + + # Builds a UNION ALL query across Workshops, Meetings, and Events, + # paginates at the database level, and returns (id, event_type) pairs + # for the current page. Only the 20 visible rows come back from the DB. + def paginated_events(upcoming:) + now = Time.zone.now + direction = upcoming ? 'ASC' : 'DESC' + comparator = upcoming ? :gteq : :lt + + # Pure Arel subqueries — no ActiveRecord relation bind params to leak + w = Arel::Table.new(:workshops) + ch = Arel::Table.new(:chapters) + ws = Arel::SelectManager.new(w) + ws.project(w[:id], w[:date_and_time], Arel.sql("'Workshop' AS event_type")) + ws.join(ch).on(w[:chapter_id].eq(ch[:id])) + ws.where(ch[:active].eq(true).and(w[:date_and_time].public_send(comparator, now))) + + m = Arel::Table.new(:meetings) + ms = Arel::SelectManager.new(m) + ms.project(m[:id], m[:date_and_time], Arel.sql("'Meeting' AS event_type")) + ms.where(m[:date_and_time].public_send(comparator, now)) + + e = Arel::Table.new(:events) + es = Arel::SelectManager.new(e) + es.project(e[:id], e[:date_and_time], Arel.sql("'Event' AS event_type")) + es.where(e[:date_and_time].public_send(comparator, now)) + + union = Arel::Nodes::UnionAll.new( + Arel::Nodes::UnionAll.new(ws, ms), es + ) + + # COUNT at DB level — single query + count_query = Arel::SelectManager.new + count_query.from(union.as('events')) + count_query.project(Arel.star.count) + total = ActiveRecord::Base.connection.select_value(count_query.to_sql).to_i + return nil if total.zero? + + pagy_opts = { count: total, page: @page, limit: 20, request: @request } + pagy_opts[:request] = Pagy::Request.new(pagy_opts) + pagy = Pagy::Offset.new(**pagy_opts) + + # Only 20 rows leave the database + pagination_query = Arel::SelectManager.new + pagination_query.from(union.as('events')) + pagination_query.project(:id, :event_type) + pagination_query.order(Arel.sql("date_and_time #{direction}")) + pagination_query.skip(pagy.offset).take(pagy.limit) + + rows = ActiveRecord::Base.connection.select_all(pagination_query.to_sql) + [rows, pagy] + end + + # Loads full ActiveRecord objects for the paginated (id, event_type) pairs + # with eager loading, preserving the UNION order. + def load_events(rows) + grouped = rows.each_with_object({}) do |row, hash| + (hash[row['event_type']] ||= []) << row['id'].to_i + end + + # workshop_host must be preloaded: eager_loading it alongside :sponsors + # routes both through the workshop_sponsors table in one join, and the + # host scope then binds to the wrong table alias — the host comes back + # nil or points at a non-host sponsor. Same pattern as Listable.most_recent. + workshops = Workshop.eager_load(:sponsors, :organisers, + chapter: { permissions: :members }) + .preload(workshop_host: :sponsor) + .where(id: grouped['Workshop']) + .to_a.index_by(&:id) + meetings = Meeting.eager_load(:venue, :organisers).where(id: grouped['Meeting']) + .to_a.index_by(&:id) + events = Event.eager_load(:venue, :sponsors, :organisers) + .where(id: grouped['Event']) + .to_a.index_by(&:id) + + rows.filter_map do |row| + case row['event_type'] + when 'Workshop' then workshops[row['id'].to_i] + when 'Meeting' then meetings[row['id'].to_i] + when 'Event' then events[row['id'].to_i] + end + end + end + + def fetch_events(upcoming:) + result = paginated_events(upcoming:) + return [{}, nil] if result.nil? + + rows, pagy = result + events = load_events(rows) + grouped = events.group_by(&:date) + decorated = grouped.transform_values { |items| EventPresenter.decorate_collection(items) } + + [decorated, pagy] + end +end diff --git a/spec/controllers/events_controller_spec.rb b/spec/controllers/events_controller_spec.rb index 6b0a6b029..b1b54b1d1 100644 --- a/spec/controllers/events_controller_spec.rb +++ b/spec/controllers/events_controller_spec.rb @@ -1,22 +1,6 @@ require 'rails_helper' RSpec.describe EventsController do - describe '#fetch_upcoming_events' do - context 'when no events exist' do - it 'returns empty hash and nil pagy' do - expect(controller.send(:fetch_upcoming_events)).to eq([{}, nil]) - end - end - end - - describe '#fetch_past_events' do - context 'when no events exist' do - it 'returns empty hash and nil pagy' do - expect(controller.send(:fetch_past_events)).to eq([{}, nil]) - end - end - end - describe 'GET #student' do let(:member) { Fabricate(:member) } let(:event) { Fabricate(:event) } @@ -244,65 +228,4 @@ expect(response).to have_http_status(:ok) end end - - describe '#load_events' do - let(:chapter) { Fabricate(:chapter) } - let(:host_sponsor) { Fabricate(:sponsor, name: 'Zulu GmbH') } - let(:extra_sponsor) { Fabricate(:sponsor, name: 'Alpha AG') } - - def rows_for(workshops) - workshops.map { |w| { 'event_type' => 'Workshop', 'id' => w.id } } - end - - it 'loads the host sponsor as workshop_host for every workshop' do - workshops = Fabricate.times(6, :workshop_no_sponsor, chapter:) - workshops.each do |workshop| - Fabricate(:workshop_sponsor, workshop:, sponsor: host_sponsor, host: true) - Fabricate(:workshop_sponsor, workshop:, sponsor: extra_sponsor, host: false) - end - - loaded = controller.send(:load_events, rows_for(workshops)) - - expect(loaded.map { |e| e.workshop_host&.sponsor&.id }).to all(eq(host_sponsor.id)) - end - - it 'lists all sponsors of a workshop' do - workshop = Fabricate(:workshop_no_sponsor, chapter:) - Fabricate(:workshop_sponsor, workshop:, sponsor: host_sponsor, host: true) - Fabricate(:workshop_sponsor, workshop:, sponsor: extra_sponsor, host: false) - - loaded = controller.send(:load_events, rows_for([workshop])) - - expect(loaded.first.sponsors).to contain_exactly(host_sponsor, extra_sponsor) - end - - it 'renders sponsors and organisers in name order' do - workshop = Fabricate(:workshop_no_sponsor, chapter:) - Fabricate(:workshop_sponsor, workshop:, sponsor: host_sponsor, host: true) - Fabricate(:workshop_sponsor, workshop:, sponsor: extra_sponsor, host: false) - anton = Fabricate(:member, name: 'Anton') - zora = Fabricate(:member, name: 'Zora') - anton.add_role :organiser, workshop - zora.add_role :organiser, workshop - - loaded = controller.send(:load_events, rows_for([workshop])) - presenter = EventPresenter.decorate(loaded.first) - - expect(presenter.sponsors.map(&:name)).to eq(['Alpha AG', 'Zulu GmbH']) - expect(presenter.organisers.map(&:name)).to eq(%w[Anton Zora]) - end - - it 'sorts organisers with blank names without raising' do - workshop = Fabricate(:workshop_no_sponsor, chapter:) - anton = Fabricate(:member, name: 'Anton') - anton.update_column(:name, nil) - anton.add_role :organiser, workshop - Fabricate(:member, name: 'Zora').add_role :organiser, workshop - - loaded = controller.send(:load_events, rows_for([workshop])) - presenter = EventPresenter.decorate(loaded.first) - - expect(presenter.organisers.map(&:name)).to eq([nil, 'Zora']) - end - end end diff --git a/spec/queries/events_feed_query_spec.rb b/spec/queries/events_feed_query_spec.rb new file mode 100644 index 000000000..3087f81c0 --- /dev/null +++ b/spec/queries/events_feed_query_spec.rb @@ -0,0 +1,167 @@ +require 'rails_helper' + +RSpec.describe EventsFeedQuery do + let(:request) do + { + base_url: 'http://example.test', + path: '/events', + params: {}, + cookie: {} + } + end + + describe '#call' do + context 'when fetching past events' do + it 'returns an empty hash and nil pagy when no events exist' do + result = described_class.new(upcoming: false, page: 1, request:).call + + expect(result).to eq([{}, nil]) + end + + it 'returns events from each type in reverse date order and excludes inactive chapters' do + now = Time.zone.now + meeting = Fabricate(:meeting, date_and_time: now - 1.day) + workshop = Fabricate(:workshop, date_and_time: now - 2.days, ends_at: now - 2.days + 2.hours) + event = Fabricate(:event, date_and_time: now - 3.days) + Fabricate(:event, date_and_time: now + 1.day) + Fabricate(:workshop, chapter: Fabricate(:chapter, active: false), + date_and_time: now - 4.days, + ends_at: now - 4.days + 2.hours) + + events, pagy = described_class.new(upcoming: false, page: 1, request:).call + listed_events = + events + .values + .flat_map { |day_events| day_events } + .map { |listed_event| [listed_event.class_string, listed_event.id] } + + expect(listed_events).to eq([['meeting', meeting.id], ['workshop', workshop.id], ['event', event.id]]) + expect(events.keys).to eq([meeting.date, workshop.date, event.date]) + expect(pagy.count).to eq(3) + end + + it 'paginates in reverse chronological order and builds links for the past path' do + venue = Fabricate(:sponsor) + meetings = Array.new(21) do |index| + date_and_time = Time.zone.now - (index + 1).days + Fabricate(:meeting, venue:, date_and_time:, ends_at: date_and_time + 2.hours) + end + Fabricate(:meeting, venue:, date_and_time: Time.zone.now + 1.day) + past_request = request.merge(path: '/events/past') + + events, pagy = described_class.new(upcoming: false, page: 1, request: past_request).call + next_page_events, next_page_pagy = described_class.new(upcoming: false, page: 2, request: past_request).call + + expected_first_page = meetings.sort_by(&:date_and_time).reverse.first(20) + expect(events.values.flat_map { |day_events| day_events }.map(&:id)).to eq(expected_first_page.map(&:id)) + expect(pagy.count).to eq(21) + expect(pagy.page_url(:next)).to eq('/events/past?page=2') + expect(next_page_events.values.flat_map { |day_events| day_events }.map(&:id)) + .to eq([meetings.min_by(&:date_and_time).id]) + expect(next_page_pagy.count).to eq(21) + end + end + + context 'when fetching upcoming events' do + it 'returns an empty hash and nil pagy when no events exist' do + result = described_class.new(upcoming: true, page: 1, request:).call + + expect(result).to eq([{}, nil]) + end + + it 'returns events from each type in date order and excludes inactive chapters' do + now = Time.zone.now + meeting = Fabricate(:meeting, date_and_time: now + 1.day, ends_at: now + 1.day + 2.hours) + workshop = Fabricate(:workshop, date_and_time: now + 2.days, ends_at: now + 2.days + 2.hours) + event = Fabricate(:event, date_and_time: now + 3.days) + + events, pagy = described_class.new(upcoming: true, page: 1, request:).call + listed_events = + events + .values + .flat_map { |day_events| day_events } + .map { |listed_event| [listed_event.class_string, listed_event.id] } + + expect(listed_events).to eq([['meeting', meeting.id], ['workshop', workshop.id], ['event', event.id]]) + expect(events.keys).to eq([meeting.date, workshop.date, event.date]) + expect(pagy.count).to eq(3) + end + + it 'builds pagination links for the upcoming events path' do + 21.times do |index| + date_and_time = Time.zone.now + (index + 1).days + Fabricate(:meeting, date_and_time:, ends_at: date_and_time + 2.hours) + end + upcoming_request = request.merge(path: '/events/upcoming') + + events, pagy = described_class.new(upcoming: true, page: 1, request: upcoming_request).call + + expect(events.values.flat_map { |day_events| day_events }.size).to eq(20) + expect(pagy.count).to eq(21) + expect(pagy.page_url(:next)).to eq('/events/upcoming?page=2') + end + end + end + + describe '#load_events' do + let(:query) { described_class.new(upcoming: true, page: 1, request:) } + let(:chapter) { Fabricate(:chapter) } + let(:host_sponsor) { Fabricate(:sponsor, name: 'Zulu GmbH') } + let(:extra_sponsor) { Fabricate(:sponsor, name: 'Alpha AG') } + + def rows_for(workshops) + workshops.map { |workshop| { 'event_type' => 'Workshop', 'id' => workshop.id } } + end + + it 'loads the host sponsor as workshop_host for every workshop' do + workshops = Fabricate.times(6, :workshop_no_sponsor, chapter:) + workshops.each do |workshop| + Fabricate(:workshop_sponsor, workshop:, sponsor: host_sponsor, host: true) + Fabricate(:workshop_sponsor, workshop:, sponsor: extra_sponsor, host: false) + end + + loaded = query.send(:load_events, rows_for(workshops)) + + expect(loaded.map { |event| event.workshop_host&.sponsor&.id }).to all(eq(host_sponsor.id)) + end + + it 'lists all sponsors of a workshop' do + workshop = Fabricate(:workshop_no_sponsor, chapter:) + Fabricate(:workshop_sponsor, workshop:, sponsor: host_sponsor, host: true) + Fabricate(:workshop_sponsor, workshop:, sponsor: extra_sponsor, host: false) + + loaded = query.send(:load_events, rows_for([workshop])) + + expect(loaded.first.sponsors).to contain_exactly(host_sponsor, extra_sponsor) + end + + it 'renders sponsors and organisers in name order' do + workshop = Fabricate(:workshop_no_sponsor, chapter:) + Fabricate(:workshop_sponsor, workshop:, sponsor: host_sponsor, host: true) + Fabricate(:workshop_sponsor, workshop:, sponsor: extra_sponsor, host: false) + anton = Fabricate(:member, name: 'Anton') + zora = Fabricate(:member, name: 'Zora') + anton.add_role :organiser, workshop + zora.add_role :organiser, workshop + + loaded = query.send(:load_events, rows_for([workshop])) + presenter = EventPresenter.decorate(loaded.first) + + expect(presenter.sponsors.map(&:name)).to eq(['Alpha AG', 'Zulu GmbH']) + expect(presenter.organisers.map(&:name)).to eq(%w[Anton Zora]) + end + + it 'sorts organisers with blank names without raising' do + workshop = Fabricate(:workshop_no_sponsor, chapter:) + anton = Fabricate(:member, name: 'Anton') + anton.update_column(:name, nil) + anton.add_role :organiser, workshop + Fabricate(:member, name: 'Zora').add_role :organiser, workshop + + loaded = query.send(:load_events, rows_for([workshop])) + presenter = EventPresenter.decorate(loaded.first) + + expect(presenter.organisers.map(&:name)).to eq([nil, 'Zora']) + end + end +end From 5584612387526bd9e8443032b012f0d519eea63e Mon Sep 17 00:00:00 2001 From: David M <207745043+davidmillen50@users.noreply.github.com> Date: Wed, 14 Oct 2026 12:45:11 +0100 Subject: [PATCH 2/2] refactor: move event feed union logic into its own file and refactor event_feed_query --- app/queries/event_feed_union.rb | 50 +++++++++++ app/queries/events_feed_query.rb | 110 +++++++++++++------------ spec/queries/event_feed_union_spec.rb | 52 ++++++++++++ spec/queries/events_feed_query_spec.rb | 25 +----- 4 files changed, 162 insertions(+), 75 deletions(-) create mode 100644 app/queries/event_feed_union.rb create mode 100644 spec/queries/event_feed_union_spec.rb diff --git a/app/queries/event_feed_union.rb b/app/queries/event_feed_union.rb new file mode 100644 index 000000000..6e74db0cd --- /dev/null +++ b/app/queries/event_feed_union.rb @@ -0,0 +1,50 @@ +class EventFeedUnion + def self.call(upcoming:) + new.call(upcoming:) + end + + def call(upcoming:) + build_event_feed_union(upcoming:) + end + + private + + def build_event_feed_union(upcoming:) + now = Time.zone.now + comparator = upcoming ? :gteq : :lt + + # Pure Arel subqueries — no ActiveRecord relation bind params to leak + workshop_subquery = workshop_feed_subquery(comparator:, now:) + meeting_subquery = meeting_feed_subquery(comparator:, now:) + event_subquery = event_feed_subquery(comparator:, now:) + + Arel::Nodes::UnionAll.new( + Arel::Nodes::UnionAll.new(workshop_subquery, meeting_subquery), event_subquery + ) + end + + def workshop_feed_subquery(comparator:, now:) + workshops = Arel::Table.new(:workshops) + chapters = Arel::Table.new(:chapters) + + query = timed_feed_subquery(table: workshops, type: 'Workshop', comparator:, now:) + query.join(chapters).on(workshops[:chapter_id].eq(chapters[:id])) + query.where(chapters[:active].eq(true).and(workshops[:date_and_time].public_send(comparator, now))) + query + end + + def timed_feed_subquery(table:, type:, comparator:, now:) + query = Arel::SelectManager.new(table) + query.project(table[:id], table[:date_and_time], Arel.sql("'#{type}' AS event_type")) + query.where(table[:date_and_time].public_send(comparator, now)) + query + end + + def meeting_feed_subquery(comparator:, now:) + timed_feed_subquery(table: Arel::Table.new(:meetings), type: 'Meeting', comparator:, now:) + end + + def event_feed_subquery(comparator:, now:) + timed_feed_subquery(table: Arel::Table.new(:events), type: 'Event', comparator:, now:) + end +end diff --git a/app/queries/events_feed_query.rb b/app/queries/events_feed_query.rb index d8d1c8814..ab53bcdc3 100644 --- a/app/queries/events_feed_query.rb +++ b/app/queries/events_feed_query.rb @@ -21,37 +21,9 @@ def call # paginates at the database level, and returns (id, event_type) pairs # for the current page. Only the 20 visible rows come back from the DB. def paginated_events(upcoming:) - now = Time.zone.now direction = upcoming ? 'ASC' : 'DESC' - comparator = upcoming ? :gteq : :lt - - # Pure Arel subqueries — no ActiveRecord relation bind params to leak - w = Arel::Table.new(:workshops) - ch = Arel::Table.new(:chapters) - ws = Arel::SelectManager.new(w) - ws.project(w[:id], w[:date_and_time], Arel.sql("'Workshop' AS event_type")) - ws.join(ch).on(w[:chapter_id].eq(ch[:id])) - ws.where(ch[:active].eq(true).and(w[:date_and_time].public_send(comparator, now))) - - m = Arel::Table.new(:meetings) - ms = Arel::SelectManager.new(m) - ms.project(m[:id], m[:date_and_time], Arel.sql("'Meeting' AS event_type")) - ms.where(m[:date_and_time].public_send(comparator, now)) - - e = Arel::Table.new(:events) - es = Arel::SelectManager.new(e) - es.project(e[:id], e[:date_and_time], Arel.sql("'Event' AS event_type")) - es.where(e[:date_and_time].public_send(comparator, now)) - - union = Arel::Nodes::UnionAll.new( - Arel::Nodes::UnionAll.new(ws, ms), es - ) - - # COUNT at DB level — single query - count_query = Arel::SelectManager.new - count_query.from(union.as('events')) - count_query.project(Arel.star.count) - total = ActiveRecord::Base.connection.select_value(count_query.to_sql).to_i + union = EventFeedUnion.call(upcoming:) + total = count_query(union:) return nil if total.zero? pagy_opts = { count: total, page: @page, limit: 20, request: @request } @@ -59,11 +31,7 @@ def paginated_events(upcoming:) pagy = Pagy::Offset.new(**pagy_opts) # Only 20 rows leave the database - pagination_query = Arel::SelectManager.new - pagination_query.from(union.as('events')) - pagination_query.project(:id, :event_type) - pagination_query.order(Arel.sql("date_and_time #{direction}")) - pagination_query.skip(pagy.offset).take(pagy.limit) + pagination_query = pagination_query(union:, pagy:, direction:) rows = ActiveRecord::Base.connection.select_all(pagination_query.to_sql) [rows, pagy] @@ -80,24 +48,11 @@ def load_events(rows) # routes both through the workshop_sponsors table in one join, and the # host scope then binds to the wrong table alias — the host comes back # nil or points at a non-host sponsor. Same pattern as Listable.most_recent. - workshops = Workshop.eager_load(:sponsors, :organisers, - chapter: { permissions: :members }) - .preload(workshop_host: :sponsor) - .where(id: grouped['Workshop']) - .to_a.index_by(&:id) - meetings = Meeting.eager_load(:venue, :organisers).where(id: grouped['Meeting']) - .to_a.index_by(&:id) - events = Event.eager_load(:venue, :sponsors, :organisers) - .where(id: grouped['Event']) - .to_a.index_by(&:id) + workshops = load_workshops_by_id(grouped:) + meetings = load_meetings_by_id(grouped:) + events = load_events_by_id(grouped:) - rows.filter_map do |row| - case row['event_type'] - when 'Workshop' then workshops[row['id'].to_i] - when 'Meeting' then meetings[row['id'].to_i] - when 'Event' then events[row['id'].to_i] - end - end + resolve_events_in_order(rows:, workshops:, meetings:, events:) end def fetch_events(upcoming:) @@ -111,4 +66,55 @@ def fetch_events(upcoming:) [decorated, pagy] end + + def count_query(union:) + # COUNT at DB level — single query + count_query = Arel::SelectManager.new + count_query.from(union.as('events')) + count_query.project(Arel.star.count) + ActiveRecord::Base.connection.select_value(count_query.to_sql).to_i + end + + def pagination_query(union:, pagy:, direction:) + pagination_query = Arel::SelectManager.new + pagination_query.from(union.as('events')) + pagination_query.project(:id, :event_type) + pagination_query.order(Arel.sql("date_and_time #{direction}")) + pagination_query.skip(pagy.offset).take(pagy.limit) + pagination_query + end + + def load_workshops_by_id(grouped:) + Workshop + .eager_load(:sponsors, :organisers, chapter: { permissions: :members }) + .preload(workshop_host: :sponsor) + .where(id: grouped['Workshop']) + .to_a + .index_by(&:id) + end + + def load_meetings_by_id(grouped:) + Meeting + .eager_load(:venue, :organisers).where(id: grouped['Meeting']) + .to_a + .index_by(&:id) + end + + def load_events_by_id(grouped:) + Event + .eager_load(:venue, :sponsors, :organisers) + .where(id: grouped['Event']) + .to_a + .index_by(&:id) + end + + def resolve_events_in_order(rows:, workshops:, meetings:, events:) + rows.filter_map do |row| + case row['event_type'] + when 'Workshop' then workshops[row['id'].to_i] + when 'Meeting' then meetings[row['id'].to_i] + when 'Event' then events[row['id'].to_i] + end + end + end end diff --git a/spec/queries/event_feed_union_spec.rb b/spec/queries/event_feed_union_spec.rb new file mode 100644 index 000000000..97d4e3805 --- /dev/null +++ b/spec/queries/event_feed_union_spec.rb @@ -0,0 +1,52 @@ +require 'rails_helper' + +RSpec.describe EventFeedUnion do + describe '.call' do + it 'includes upcoming meetings, events, and active-chapter workshops' do + now = Time.zone.now + workshop = Fabricate(:workshop, date_and_time: now + 2.days, ends_at: now + 2.days + 2.hours) + meeting = Fabricate(:meeting, date_and_time: now + 1.day, ends_at: now + 1.day + 2.hours) + event = Fabricate(:event, date_and_time: now + 3.days) + Fabricate(:workshop, chapter: Fabricate(:chapter, active: false), + date_and_time: now + 4.days, + ends_at: now + 4.days + 2.hours) + Fabricate(:meeting, date_and_time: now - 1.day) + Fabricate(:event, date_and_time: now - 2.days) + + rows = event_rows(upcoming: true) + + expect(rows).to contain_exactly( + ['Workshop', workshop.id], + ['Meeting', meeting.id], + ['Event', event.id] + ) + end + + it 'includes past meetings, events, and active-chapter workshops' do + now = Time.zone.now + workshop = Fabricate(:workshop, date_and_time: now - 2.days, ends_at: now - 2.days + 2.hours) + meeting = Fabricate(:meeting, date_and_time: now - 1.day) + event = Fabricate(:event, date_and_time: now - 3.days) + Fabricate(:workshop, chapter: Fabricate(:chapter, active: false), + date_and_time: now - 4.days, + ends_at: now - 4.days + 2.hours) + Fabricate(:meeting, date_and_time: now + 1.day) + Fabricate(:event, date_and_time: now + 2.days) + + rows = event_rows(upcoming: false) + + expect(rows).to contain_exactly( + ['Workshop', workshop.id], + ['Meeting', meeting.id], + ['Event', event.id] + ) + end + end + + def event_rows(upcoming:) + union = described_class.call(upcoming:) + ActiveRecord::Base.connection.select_all(union.to_sql).map do |row| + [row.fetch('event_type'), row.fetch('id').to_i] + end + end +end diff --git a/spec/queries/events_feed_query_spec.rb b/spec/queries/events_feed_query_spec.rb index 3087f81c0..ef9ab5cb1 100644 --- a/spec/queries/events_feed_query_spec.rb +++ b/spec/queries/events_feed_query_spec.rb @@ -18,28 +18,6 @@ expect(result).to eq([{}, nil]) end - it 'returns events from each type in reverse date order and excludes inactive chapters' do - now = Time.zone.now - meeting = Fabricate(:meeting, date_and_time: now - 1.day) - workshop = Fabricate(:workshop, date_and_time: now - 2.days, ends_at: now - 2.days + 2.hours) - event = Fabricate(:event, date_and_time: now - 3.days) - Fabricate(:event, date_and_time: now + 1.day) - Fabricate(:workshop, chapter: Fabricate(:chapter, active: false), - date_and_time: now - 4.days, - ends_at: now - 4.days + 2.hours) - - events, pagy = described_class.new(upcoming: false, page: 1, request:).call - listed_events = - events - .values - .flat_map { |day_events| day_events } - .map { |listed_event| [listed_event.class_string, listed_event.id] } - - expect(listed_events).to eq([['meeting', meeting.id], ['workshop', workshop.id], ['event', event.id]]) - expect(events.keys).to eq([meeting.date, workshop.date, event.date]) - expect(pagy.count).to eq(3) - end - it 'paginates in reverse chronological order and builds links for the past path' do venue = Fabricate(:sponsor) meetings = Array.new(21) do |index| @@ -69,7 +47,7 @@ expect(result).to eq([{}, nil]) end - it 'returns events from each type in date order and excludes inactive chapters' do + it 'returns decorated events from each type grouped by date' do now = Time.zone.now meeting = Fabricate(:meeting, date_and_time: now + 1.day, ends_at: now + 1.day + 2.hours) workshop = Fabricate(:workshop, date_and_time: now + 2.days, ends_at: now + 2.days + 2.hours) @@ -84,6 +62,7 @@ expect(listed_events).to eq([['meeting', meeting.id], ['workshop', workshop.id], ['event', event.id]]) expect(events.keys).to eq([meeting.date, workshop.date, event.date]) + events.each_value { |day_events| expect(day_events).to all(be_a(EventPresenter)) } expect(pagy.count).to eq(3) end