From 5ea6659ff704ed59ff74ea97f6dca3660bfafaa3 Mon Sep 17 00:00:00 2001 From: Morgan Roderick Date: Thu, 8 Oct 2026 22:46:10 +0200 Subject: [PATCH 1/4] test: pin role-scoped waitlist promotion and the promotion email Add pins for two unpinned behaviours: - WaitingList.next_spot filters by role, so an older Coach entry must not be returned when a Student seat frees up (model) and must not be promoted when a Student cancels (controller). - The reject flow sends the promoted member the waiting-list variant of the attending email; assert the delivery, the promoted copy's text, and that exactly one email goes out. --- .../workshop_invitation_controller_spec.rb | 53 +++++++++++++++++++ spec/models/waiting_list_spec.rb | 11 ++++ 2 files changed, 64 insertions(+) diff --git a/spec/controllers/workshop_invitation_controller_spec.rb b/spec/controllers/workshop_invitation_controller_spec.rb index 21fbc136d..cd8476c91 100644 --- a/spec/controllers/workshop_invitation_controller_spec.rb +++ b/spec/controllers/workshop_invitation_controller_spec.rb @@ -8,6 +8,13 @@ before { login(member) } + # The invitation mailer sends multipart/mixed > multipart/alternative > text/html; + # the root body is empty, so read the nested html part. + def html_body(mail) + parts = mail.parts.flat_map { |part| part.multipart? ? part.parts : [part] } + parts.find { |part| part.content_type.match?('text/html') }&.body&.decoded + end + describe 'GET #show' do it 'returns http success' do get :show, params: { id: invitation.token } @@ -169,6 +176,52 @@ post :reject, params: { id: invitation.token } expect(waitlisted_invitation.reload.attending).to be true end + + it 'emails the promoted member a confirmation they are attending' do + post :reject, params: { id: invitation.token } + + mail = ActionMailer::Base.deliveries.find { |m| m.to.include?(waitlisted_member.email) } + expect(mail).not_to be_nil + expect(html_body(mail)).to include('been confirmed') + end + + it 'sends the promotion variant of the email (waiting-list flag set)' do + post :reject, params: { id: invitation.token } + + mail = ActionMailer::Base.deliveries.find { |m| m.to.include?(waitlisted_member.email) } + # The promoted copy is only produced when the mailer is called with + # waiting_list: true — the plain accept copy says something else. + expect(html_body(mail)).to include('A spot became available and your attendance has now been confirmed!') + end + + it 'does not email anyone else (the rejecting member gets no promotion copy)' do + expect do + post :reject, params: { id: invitation.token } + end.to change { ActionMailer::Base.deliveries.count }.by(1) + end + end + + context 'when a coach is waitlisted and a student seat frees up' do + # Pins the cross-role filter in `WaitingList.next_spot`: the coach entry + # must not be promoted by a Student's cancellation, no matter how old it is. + let(:coach) { Fabricate(:coach) } + let(:coach_invitation) { Fabricate(:coach_workshop_invitation, workshop:, member: coach) } + + before do + invitation.update!(attending: true) + WaitingList.add(coach_invitation, auto_rsvp: true) + end + + it 'does not promote the coach invitation' do + post :reject, params: { id: invitation.token } + + expect(coach_invitation.reload.attending).to be_nil + end + + it 'leaves the waiting list unchanged' do + expect { post :reject, params: { id: invitation.token } } + .not_to change(WaitingList, :count) + end end end diff --git a/spec/models/waiting_list_spec.rb b/spec/models/waiting_list_spec.rb index df731164b..28a3985c2 100644 --- a/spec/models/waiting_list_spec.rb +++ b/spec/models/waiting_list_spec.rb @@ -24,6 +24,17 @@ expect(described_class.next_spot(workshop, 'Student').invitation).to eq(invitation) end + + it 'ignores an older entry for another role' do + # A freed seat of one role must not promote an entry of the other role, + # even when it is the oldest on the list. + coach_invitation = Fabricate(:coach_workshop_invitation, workshop:, member: Fabricate(:coach)) + described_class.add(coach_invitation) + + expect(described_class.next_spot(workshop, 'Student')).to be_nil + expect(coach_invitation.reload.attending).to be_nil + expect(described_class.by_workshop(workshop).count).to eq(1) + end end end From 34bccf09169cdcd77d51e3ba89e6a3bd50daf3e4 Mon Sep 17 00:00:00 2001 From: Morgan Roderick Date: Thu, 8 Oct 2026 22:46:47 +0200 Subject: [PATCH 2/4] fix: drop the cancelling member's waiting-list entry on reject WorkshopInvitationController#reject set `attending: false` but left the member's WaitingList row in place, so a cancelling member's own entry could be returned by `WaitingList.next_spot` for the seat they just freed: the flow destroyed that entry, re-set the freshly cancelled invitation to attending and emailed the member they are attending again. Destroy the cancelling member's entry before looking up the next spot, so a rejection cannot list the member back in and the promotion goes to the real next entry for the role. --- .../workshop_invitation_controller.rb | 6 ++++ .../workshop_invitation_controller_spec.rb | 33 +++++++++++++++++++ 2 files changed, 39 insertions(+) diff --git a/app/controllers/workshop_invitation_controller.rb b/app/controllers/workshop_invitation_controller.rb index 6b3369864..db0b2a5a1 100644 --- a/app/controllers/workshop_invitation_controller.rb +++ b/app/controllers/workshop_invitation_controller.rb @@ -68,6 +68,12 @@ def reject MemberActivityRecorder.record(actor: @invitation.member, key: 'workshop_invitation.rejected', trackable: @invitation) + # A cancelling member must drop out of the waiting list. Otherwise their + # own entry could be picked as the next spot for the seat they just + # freed (or auto-promote them later). Do this before computing the + # next spot so the rejection cannot list the member back in. + WaitingList.find_by(invitation_id: @invitation.id)&.destroy + next_spot = WaitingList.next_spot(@invitation.workshop, @invitation.role) if next_spot.present? diff --git a/spec/controllers/workshop_invitation_controller_spec.rb b/spec/controllers/workshop_invitation_controller_spec.rb index cd8476c91..a74a71a73 100644 --- a/spec/controllers/workshop_invitation_controller_spec.rb +++ b/spec/controllers/workshop_invitation_controller_spec.rb @@ -223,6 +223,39 @@ def html_body(mail) .not_to change(WaitingList, :count) end end + + context 'when the rejecting member is on the waiting list' do + # Rejecting cancels the RSVP, so the member must not keep a waiting-list + # entry that could later auto-promote them onto a seat they declined. + before do + invitation.update!(attending: true) + WaitingList.add(invitation, auto_rsvp: true) + end + + it 'removes their own waiting-list entry' do + post :reject, params: { id: invitation.token } + + expect(WaitingList.where(invitation:)).to be_empty + end + + it 'does not redeliver the seat to the cancelling member' do + post :reject, params: { id: invitation.token } + + expect(invitation.reload.attending).to be false + end + + it 'promotes the next student on the waiting list instead' do + member_behind = Fabricate(:member) + invitation_behind = Fabricate(:workshop_invitation, workshop:, member: member_behind, role: 'Student') + WaitingList.add(invitation_behind, auto_rsvp: true) + + post :reject, params: { id: invitation.token } + + expect(invitation_behind.reload.attending).to be true + expect(invitation.reload.attending).to be false + expect(WaitingList.where(invitation:)).to be_empty + end + end end describe 'PATCH #update' do From f86e5cba4725471ed863dd2f0a0216b558d2029b Mon Sep 17 00:00:00 2001 From: Morgan Roderick Date: Thu, 8 Oct 2026 22:47:17 +0200 Subject: [PATCH 3/4] fix: allow reject without a session cookie The reject endpoint authenticates by the invitation token in the URL, like accept and update. Its rejection form must survive a browser withholding the session cookie (e.g. Safari/WebKit ITP on cross-site navigation), so CSRF enforcement raises InvalidAuthenticityToken before the token can do its job. Add reject to the skip_forgery_protection list, matching accept, update and WaitingListsController, and pin the token-only, no-session request with a spec. --- app/controllers/workshop_invitation_controller.rb | 2 +- .../workshop_invitation_controller_spec.rb | 12 ++++++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/app/controllers/workshop_invitation_controller.rb b/app/controllers/workshop_invitation_controller.rb index db0b2a5a1..b5446b553 100644 --- a/app/controllers/workshop_invitation_controller.rb +++ b/app/controllers/workshop_invitation_controller.rb @@ -9,7 +9,7 @@ class WorkshopInvitationController < ApplicationController # CSRF is redundant and fails when browsers withhold the session cookie # (e.g. Safari/WebKit ITP on cross-site navigation). Same rationale as # FeedbackController#submit (PR #2641, Rollbar #535). - skip_forgery_protection only: %i[update accept] + skip_forgery_protection only: %i[update accept reject] def show @announcements = @invitation.member.announcements.active diff --git a/spec/controllers/workshop_invitation_controller_spec.rb b/spec/controllers/workshop_invitation_controller_spec.rb index a74a71a73..c6e328db4 100644 --- a/spec/controllers/workshop_invitation_controller_spec.rb +++ b/spec/controllers/workshop_invitation_controller_spec.rb @@ -163,6 +163,18 @@ def html_body(mail) end end + context 'without a session (the invitation token is the only credential)' do + include_context 'with forgery protection enforced' + + before { LoginHelpers::LoginStub.current_user = nil } + + it 'still rejects the RSVP with the token alone' do + post :reject, params: { id: invitation.token } + + expect(invitation.reload.attending).to be false + end + end + context 'when someone is on waiting list' do let(:waitlisted_member) { Fabricate(:member) } let(:waitlisted_invitation) { Fabricate(:workshop_invitation, workshop:, member: waitlisted_member, role: 'Student') } From 1e0dfa6ca4e8516a8db8269de321554aaf9e3293 Mon Sep 17 00:00:00 2001 From: Morgan Roderick Date: Thu, 8 Oct 2026 22:47:43 +0200 Subject: [PATCH 4/4] fix: redirect instead of raising when the waiting-list entry is gone MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit WaitingListsController#destroy called destroy on WaitingList.find_by(...), which is nil when the entry no longer exists — reachable by replaying a stale "Remove from the waiting list" link (double-click, or a bookmarked token URL after the entry was consumed), a 500 that Rollbar hears. Guard the lookup and redirect with a notice when the entry is gone, and skip the waiting_list.left activity record. --- app/controllers/waiting_lists_controller.rb | 8 +++++++- .../waiting_lists_controller_spec.rb | 17 +++++++++++++++++ 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/app/controllers/waiting_lists_controller.rb b/app/controllers/waiting_lists_controller.rb index 471def73d..a27f0c759 100644 --- a/app/controllers/waiting_lists_controller.rb +++ b/app/controllers/waiting_lists_controller.rb @@ -26,7 +26,13 @@ def create # rubocop:disable Metrics/MethodLength end def destroy - WaitingList.find_by(invitation_id: @invitation.id).destroy + entry = WaitingList.find_by(invitation_id: @invitation.id) + unless entry + return redirect_to(invitation_path(@invitation), + notice: 'You are not on the waiting list') + end + + entry.destroy MemberActivityRecorder.record(actor: @invitation.member, key: 'waiting_list.left', trackable: @invitation) diff --git a/spec/controllers/waiting_lists_controller_spec.rb b/spec/controllers/waiting_lists_controller_spec.rb index cf09b26c4..6106cb1cd 100644 --- a/spec/controllers/waiting_lists_controller_spec.rb +++ b/spec/controllers/waiting_lists_controller_spec.rb @@ -48,6 +48,23 @@ end describe 'DELETE #destroy' do + # Replaying a stale "Remove from the waiting list" link (double-click, or + # a bookmarked URL after the entry was consumed) must not raise. + context 'when the waiting-list entry is already gone' do + it 'redirects with a notice instead of raising' do + delete :destroy, params: { invitation_id: invitation.token } + + expect(response).to redirect_to(invitation_path(invitation)) + expect(flash[:notice]).to eq('You are not on the waiting list') + end + + it 'does not record a "waiting_list.left" activity' do + delete :destroy, params: { invitation_id: invitation.token } + + expect(PublicActivity::Activity.where(key: 'waiting_list.left')).to be_empty + end + end + context 'without a CSRF token (browser did not send session cookie)' do include_context 'with forgery protection enforced'