diff --git a/app/components/event_card_component.html.erb b/app/components/event_card_component.html.erb index a591e3226..915a6efad 100644 --- a/app/components/event_card_component.html.erb +++ b/app/components/event_card_component.html.erb @@ -1,4 +1,4 @@ -<% cache @event.cache_key_with_version do %> +<% cache [@event.cache_key_with_version, I18n.locale, :v2] do %>
@@ -8,18 +8,7 @@ <%= link_to @event.chapter.name, @event.chapter.slug, class: "text-light text-decoration-none" %> <% end %> - <% if user_presenter %> - <% if user_presenter.attending?(@event) %> - - <%= link_to "Attending", @event.path, class: "text-light text-decoration-none" %> - - <% end %> - <% if user_presenter.organiser? && user_presenter.event_organiser?(@event) %> - - <%= link_to "Manage", @event.admin_path, class: "text-light text-decoration-none" %> - - <% end %> - <% end %> +

diff --git a/app/components/event_card_component.rb b/app/components/event_card_component.rb index 1bbed31a1..b27d85b1b 100644 --- a/app/components/event_card_component.rb +++ b/app/components/event_card_component.rb @@ -1,12 +1,6 @@ class EventCardComponent < ViewComponent::Base - def initialize(event_card:, user: nil) + def initialize(event_card:) super() @event = event_card - @user = user - end - - # Wraps raw Member in MemberPresenter; double-wrapping a presenter is a no-op. - def user_presenter - @user && MemberPresenter.new(@user) end end diff --git a/app/controllers/chapter_controller.rb b/app/controllers/chapter_controller.rb index a321f36d2..8c2b485b0 100644 --- a/app/controllers/chapter_controller.rb +++ b/app/controllers/chapter_controller.rb @@ -19,8 +19,8 @@ def slug end def upcoming_events_by_chapter(chapter) - workshops = chapter.upcoming_workshops.includes(:sponsors) - events = chapter.events.upcoming + workshops = chapter.upcoming_workshops.eager_load(:sponsors, :organisers, :permissions, :workshop_host) + events = chapter.events.upcoming.eager_load(:venue, :sponsors, :sponsorships, :permissions, :organisers) [*workshops, *events].uniq.compact.sort_by(&:date_and_time).group_by(&:date) end diff --git a/app/controllers/events_controller.rb b/app/controllers/events_controller.rb index 6c6d81fbc..fb9bd6ffd 100644 --- a/app/controllers/events_controller.rb +++ b/app/controllers/events_controller.rb @@ -157,12 +157,12 @@ def load_events(rows) (hash[row['event_type']] ||= []) << row['id'].to_i end - workshops = Workshop.includes(:chapter, :sponsors, :host, :permissions) + workshops = Workshop.eager_load(:chapter, :sponsors, :organisers, :permissions, :workshop_host) .where(id: grouped['Workshop']) .to_a.index_by(&:id) - meetings = Meeting.includes(:venue).where(id: grouped['Meeting']) + meetings = Meeting.eager_load(:venue, :organisers, :permissions).where(id: grouped['Meeting']) .to_a.index_by(&:id) - events = Event.includes(:venue, :sponsors, :sponsorships, :permissions) + events = Event.eager_load(:venue, :sponsors, :sponsorships, :permissions, :organisers) .where(id: grouped['Event']) .to_a.index_by(&:id) diff --git a/app/models/concerns/invitation_concerns.rb b/app/models/concerns/invitation_concerns.rb index 62f6d6399..faa27160a 100644 --- a/app/models/concerns/invitation_concerns.rb +++ b/app/models/concerns/invitation_concerns.rb @@ -18,7 +18,6 @@ module InvitationConcerns scope :taken_place, -> { where('date_and_time < ?', Time.zone.now) } before_create :set_token - after_save :clear_member_cache, if: :saved_change_to_attending? end module InstanceMethods @@ -40,10 +39,6 @@ def for_participant? private - def clear_member_cache - member.clear_attending_event_ids_cache! - end - def set_token self.token = loop do random_token = SecureRandom.urlsafe_base64(nil, false) diff --git a/app/models/member.rb b/app/models/member.rb index 5b7ed238d..55d3ab42c 100644 --- a/app/models/member.rb +++ b/app/models/member.rb @@ -156,19 +156,6 @@ def past_rsvps @past_rsvps ||= rsvps(period: :past).reverse end - def attending_event_ids - @attending_event_ids ||= begin - event_ids = invitations.accepted.pluck(:event_id) - workshop_ids = workshop_invitations.accepted.pluck(:workshop_id) - meeting_ids = meeting_invitations.accepted.pluck(:meeting_id) - (event_ids + workshop_ids + meeting_ids).to_set - end - end - - def clear_attending_event_ids_cache! - @attending_event_ids = nil - end - def flag_to_organisers? return admin_workshop_flags[:flag_to_organisers] if admin_workshop_flags diff --git a/app/presenters/member_presenter.rb b/app/presenters/member_presenter.rb index 7a334b5ec..a314b4449 100644 --- a/app/presenters/member_presenter.rb +++ b/app/presenters/member_presenter.rb @@ -1,25 +1,10 @@ class MemberPresenter < BasePresenter - def organiser? - @organiser ||= has_role? :organiser, :any - end - - def event_organiser?(event) - event_types = %w[Workshop Meeting Event] - organising_events.slice(*event_types).values.any? { |ids| ids.include?(event.id) } || - (event.chapter && organising_events['Chapter']&.include?(event.chapter.id)) || - admin? - end - def newbie? return model.admin_workshop_flags[:newbie] if model.admin_workshop_flags !workshop_invitations.attended.exists? end - def attending?(event) - model.attending_event_ids.include?(event.id) - end - def subscribed_to_newsletter? opt_in_newsletter_at.present? end @@ -38,17 +23,6 @@ def displayed_dietary_restrictions private - def organising_events - @organising_events ||= roles.where(name: 'organiser') - .pluck(:resource_type, :resource_id) - .group_by(&:first) - .transform_values { |pairs| pairs.map(&:last) } - end - - def admin? - @admin ||= has_role?(:admin) - end - def coach_pairing_details(note) [newbie?, full_name, 'Coach', 'N/A', note, skill_list.to_s] end diff --git a/app/views/dashboard/dashboard.html.haml b/app/views/dashboard/dashboard.html.haml index 618f5478f..51e237cba 100644 --- a/app/views/dashboard/dashboard.html.haml +++ b/app/views/dashboard/dashboard.html.haml @@ -32,4 +32,4 @@ There are no upcoming events announced for the chapters you are subscribed to. - @ordered_events.each do |date, workshops| %h4= date - = render EventCardComponent.with_collection(workshops, user: current_user) + = render EventCardComponent.with_collection(workshops) diff --git a/app/views/dashboard/show.html.haml b/app/views/dashboard/show.html.haml index f52f2ed53..eb369da2e 100644 --- a/app/views/dashboard/show.html.haml +++ b/app/views/dashboard/show.html.haml @@ -71,7 +71,7 @@ %h2.h3.mb-4= t('homepage.events.upcoming') - @upcoming_workshops.each do |date, workshops| %h3.h5= date - = render EventCardComponent.with_collection(workshops, user: current_user) + = render EventCardComponent.with_collection(workshops) - if @has_more_events = link_to 'Explore all events →', upcoming_events_path, class: 'btn btn-outline-primary mt-3' diff --git a/spec/components/event_card_component_spec.rb b/spec/components/event_card_component_spec.rb index a80624db2..a94ca45c5 100644 --- a/spec/components/event_card_component_spec.rb +++ b/spec/components/event_card_component_spec.rb @@ -71,17 +71,40 @@ let(:presenter) { WorkshopPresenter.new(workshop) } let(:member) { Fabricate(:member) } - it 'renders attending badge when user is attending (as presenter)' do + it 'renders user-independent output even with a warm cache' do Fabricate(:workshop_invitation, workshop:, member:, attending: true) - user_presenter = MemberPresenter.new(member) - render_inline(described_class.new(event_card: presenter, user: user_presenter)) - expect(page).to have_text('Attending') + cache = ActiveSupport::Cache::MemoryStore.new + + # Regression: user-dependent badges (Attending/Manage) were rendered inside + # the cached fragment, so an organiser's dashboard render leaked badges to + # every other user, including signed-out visitors. The card must contain + # nothing user-specific, ever. + ActionController::Base.cache_store = cache + first = render_inline(described_class.new(event_card: presenter)).to_html + second = render_inline(described_class.new(event_card: presenter)).to_html + + expect(first).not_to include('Attending') + expect(first).not_to include('Manage') + expect(first).to eq(second) + + # The fragment key includes I18n.locale, so a render under another locale + # must not reuse (or poison) the :en fragment. + begin + I18n.locale = :fr + french = render_inline(described_class.new(event_card: presenter)).to_html + expect(french).not_to eq(first) + ensure + I18n.locale = :en + end + ensure + ActionController::Base.cache_store = :null_store end - it 'renders attending badge when raw Member is passed' do - Fabricate(:workshop_invitation, workshop:, member:, attending: true) - render_inline(described_class.new(event_card: presenter, user: member)) - expect(page).to have_text('Attending') + it 'no longer accepts a user (card must be user-agnostic)' do + # Pins the fix for #2869: badges rendered behind the removed `user:` kwarg, + # so a reintroduction must fail loudly here. + expect { described_class.new(event_card: presenter, user: member) } + .to raise_error(ArgumentError, /unknown keyword/) end end end diff --git a/spec/features/member_portal_spec.rb b/spec/features/member_portal_spec.rb index d639c7e4b..eadb92f74 100644 --- a/spec/features/member_portal_spec.rb +++ b/spec/features/member_portal_spec.rb @@ -29,6 +29,34 @@ expect(page).to have_text("#{presenter} at #{presenter.venue.name}", count: 1) end + it 'does not leak attending badges through the shared card cache' do + # Regression for #2869: the first member's dashboard render warms the + # event-card fragment; a second member viewing the same card must not + # see the first member's badge baked into the cached fragment. + chapter = Fabricate(:chapter_with_groups) + workshop = Fabricate(:workshop, chapter:) + Fabricate(:subscription, member:, group: chapter.groups.first) + Fabricate(:attending_workshop_invitation, member:, workshop:) + other_member = Fabricate(:member) + Fabricate(:subscription, member: other_member, group: chapter.groups.first) + presenter = WorkshopPresenter.new(workshop) + + ActionController::Base.cache_store = ActiveSupport::Cache::MemoryStore.new + begin + visit dashboard_path + expect(page).to have_text("#{presenter} at #{presenter.venue.name}", count: 1) + + login(other_member) + visit dashboard_path + expect(page).to have_text("#{presenter} at #{presenter.venue.name}", count: 1) + # 'Manage' is legitimately in the nav ("Manage subscriptions"), so only + # the Attending badge is asserted absent — it never appears in chrome. + expect(page).to have_no_text('Attending') + ensure + ActionController::Base.cache_store = :null_store + end + end + it 'can view upcoming workshops for their chapters' do c1_workshop = Fabricate(:workshop, chapter: Fabricate(:chapter_with_groups)) Fabricate(:subscription, member:, group: c1_workshop.chapter.groups.first) diff --git a/spec/models/member_spec.rb b/spec/models/member_spec.rb index f460b4575..0e46ba6fe 100644 --- a/spec/models/member_spec.rb +++ b/spec/models/member_spec.rb @@ -310,47 +310,4 @@ expect(managers.size).to eq(managers.distinct.size) end end - - describe '#attending_event_ids' do - let(:member) { Fabricate(:member) } - - it 'returns event IDs where member has accepted invitation' do - event = Fabricate(:event) - Fabricate(:invitation, member:, event:, attending: true) - expect(member.attending_event_ids).to include(event.id) - end - - it 'does not include events where invitation is not accepted' do - event = Fabricate(:event) - Fabricate(:invitation, member:, event:, attending: false) - expect(member.attending_event_ids).not_to include(event.id) - end - - it 'includes workshop IDs' do - workshop = Fabricate(:workshop) - Fabricate(:workshop_invitation, member:, workshop:, attending: true) - expect(member.attending_event_ids).to include(workshop.id) - end - - it 'includes meeting IDs' do - meeting = Fabricate(:meeting) - Fabricate(:meeting_invitation, member:, meeting:, attending: true) - expect(member.attending_event_ids).to include(meeting.id) - end - - it 'caches result in instance variable' do - event = Fabricate(:event) - Fabricate(:invitation, member:, event:, attending: true) - first_call = member.attending_event_ids - expect(member.attending_event_ids).to equal(first_call) - end - - it 'can be cleared and re-queries on next call' do - event = Fabricate(:event) - Fabricate(:invitation, member:, event:, attending: true) - member.attending_event_ids - member.clear_attending_event_ids_cache! - expect(member.attending_event_ids).to include(event.id) - end - end end diff --git a/spec/presenters/member_presenter_spec.rb b/spec/presenters/member_presenter_spec.rb index 8b0241d4c..41b008174 100644 --- a/spec/presenters/member_presenter_spec.rb +++ b/spec/presenters/member_presenter_spec.rb @@ -4,14 +4,6 @@ let(:member) { Fabricate(:member, skill_list: 'java, ruby') } let(:member_presenter) { described_class.new(member) } - it '#organiser?' do - allow(member).to receive(:has_role?).with(:organiser, :any) - - member_presenter.organiser? - - expect(member).to have_received(:has_role?).with(:organiser, :any) - end - it '#subscribed_to_newsletter?' do allow(member).to receive(:opt_in_newsletter_at) @@ -31,83 +23,4 @@ .to eq([member_presenter.newbie?, member.full_name, 'Coach', 'N/A', 'A note', 'java, ruby']) end end - - describe '#attending?' do - let(:event) { Fabricate(:event) } - - it 'returns true when member is attending event' do - Fabricate(:invitation, member:, event:, attending: true) - expect(member_presenter.attending?(event)).to be true - end - - it 'returns false when member is not attending' do - expect(member_presenter.attending?(event)).to be false - end - end - - describe '#event_organiser?' do - let(:chapter) { Fabricate(:chapter) } - let(:workshop) { Fabricate(:workshop_no_sponsor, chapter:) } - - it 'returns true when user is admin' do - admin = Fabricate(:member) - admin.add_role(:admin) - presenter = described_class.new(admin) - - expect(presenter.event_organiser?(workshop)).to be true - end - - it 'returns true when user is direct organiser of the event' do - member.add_role(:organiser, workshop) - - expect(member_presenter.event_organiser?(workshop)).to be true - end - - it 'returns true when user is organiser of the event chapter' do - organiser = Fabricate(:member) - organiser.add_role(:organiser, chapter) - presenter = described_class.new(organiser) - - expect(presenter.event_organiser?(workshop)).to be true - end - - it 'returns false for a regular member' do - regular = Fabricate(:member) - presenter = described_class.new(regular) - - expect(presenter.event_organiser?(workshop)).to be false - end - - it 'returns false for organiser of a different chapter' do - other_chapter = Fabricate(:chapter) - Fabricate(:workshop_no_sponsor, chapter: other_chapter) - organiser = Fabricate(:member) - organiser.add_role(:organiser, other_chapter) - presenter = described_class.new(organiser) - - expect(presenter.event_organiser?(workshop)).to be false - end - - it 'handles events without a singular chapter association (e.g. Meeting) without crashing' do - meeting = Fabricate(:meeting) - meeting_presenter = MeetingPresenter.new(meeting) - admin = Fabricate(:member) - admin.add_role(:admin) - presenter = described_class.new(admin) - - expect(presenter.event_organiser?(meeting_presenter)).to be true - end - - it 'memoizes private methods across calls' do - member.add_role(:admin) - - allow(member).to receive(:has_role?).with(:admin).once.and_call_original - # First call loads and caches - member_presenter.event_organiser?(workshop) - # Second call uses cache - member_presenter.event_organiser?(workshop) - - expect(member).to have_received(:has_role?).with(:admin).once - end - end end diff --git a/spec/requests/event_card_render_query_cost_spec.rb b/spec/requests/event_card_render_query_cost_spec.rb new file mode 100644 index 000000000..f11028ffe --- /dev/null +++ b/spec/requests/event_card_render_query_cost_spec.rb @@ -0,0 +1,56 @@ +require 'rails_helper' + +RSpec.describe 'Event card render query cost' do + # Regression guard for the cache-miss render path of EventCardComponent: + # organisers/venue lookups are eager-loaded with the relation, so the SQL + # cost of a listing page must not grow per rendered card. + + def count_queries + queries = 0 + subscriber = ActiveSupport::Notifications.subscribe('sql.active_record') do + queries += 1 + end + yield + queries + ensure + ActiveSupport::Notifications.unsubscribe(subscriber) + end + + it 'does not add per-card queries on /events/upcoming as cards grow' do + chapter = Fabricate(:chapter, active: true) + organiser = Fabricate(:member) + first_workshop = Fabricate(:workshop, chapter:) + organiser.add_role(:organiser, first_workshop) + + get '/events/upcoming' + one_card = count_queries { get '/events/upcoming' } + + 3.times do + workshop = Fabricate(:workshop, chapter:) + organiser.add_role(:organiser, workshop) + end + + four_cards = count_queries { get '/events/upcoming' } + + expect(four_cards).to be <= one_card + 8 + end + + it 'does not add per-card queries on the chapter page as cards grow' do + chapter = Fabricate(:chapter, active: true) + organiser = Fabricate(:member) + first_workshop = Fabricate(:workshop, chapter:) + organiser.add_role(:organiser, first_workshop) + + get "/#{chapter.slug}" + one_card = count_queries { get "/#{chapter.slug}" } + + 3.times do + workshop = Fabricate(:workshop, chapter:) + organiser.add_role(:organiser, workshop) + end + + four_cards = count_queries { get "/#{chapter.slug}" } + + expect(four_cards).to be <= one_card + 8 + end +end diff --git a/spec/support/shared_examples/behaves_like_an_invitation.rb b/spec/support/shared_examples/behaves_like_an_invitation.rb index d3ae945fb..9524f2544 100644 --- a/spec/support/shared_examples/behaves_like_an_invitation.rb +++ b/spec/support/shared_examples/behaves_like_an_invitation.rb @@ -6,15 +6,6 @@ expect(invitation.token).not_to be_nil end - describe 'cache invalidation' do - it 'clears member cache when attending changes' do - allow(invitation.member).to receive(:clear_attending_event_ids_cache!) - invitation.update!(attending: !invitation.attending) - - expect(invitation.member).to have_received(:clear_attending_event_ids_cache!) - end - end - describe '#scopes' do describe '#not_accepted' do it 'selects when attended nil' do