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/app/controllers/workshop_invitation_controller.rb b/app/controllers/workshop_invitation_controller.rb index 6b3369864..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 @@ -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/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' diff --git a/spec/controllers/workshop_invitation_controller_spec.rb b/spec/controllers/workshop_invitation_controller_spec.rb index 21fbc136d..c6e328db4 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 } @@ -156,6 +163,18 @@ 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') } @@ -169,6 +188,85 @@ 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 + + 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 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