Fix waiting-list gaps in reject and destroy, pin promotion behaviour

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)
 

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?

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'
 

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
 

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