From 26d949930ac609deb4050a1e79a49c0767e9e41f Mon Sep 17 00:00:00 2001 From: Morgan Roderick Date: Fri, 11 Sep 2026 13:56:52 +0200 Subject: [PATCH 1/4] fix(invitations): one invitation per member per event or workshop Members subscribed to a chapter as both student and coach received two invitation emails, one per role, because each invitation pass matched on (event or workshop, member, role). Match on identity only and set the role on create, so the second pass finds the existing invitation and skips the member. For events with a blank audience, coach emails were labelled "Coach Invitation" instead of a general "Invitation"; label coach emails as such only when the event is actually for coaches. Fixes #2861 --- app/mailers/event_invitation_mailer.rb | 4 +- app/services/invitation_manager.rb | 6 +- spec/mailers/event_invitation_mailer_spec.rb | 9 ++ spec/services/invitation_manager_spec.rb | 129 +++++++----------- .../behaves_like_sending_workshop_emails.rb | 14 +- 5 files changed, 72 insertions(+), 90 deletions(-) diff --git a/app/mailers/event_invitation_mailer.rb b/app/mailers/event_invitation_mailer.rb index aaf22740f..b1c4b5177 100644 --- a/app/mailers/event_invitation_mailer.rb +++ b/app/mailers/event_invitation_mailer.rb @@ -20,7 +20,9 @@ def invite_coach(event, member, invitation) @member = member @invitation = invitation @host_address = AddressPresenter.new(@event.venue.address) if @event.venue.present? - @everyone_is_invited = !event.audience + # Coach emails are labelled as such only when the event is actually for + # coaches; blank or missing audience means a general invitation. + @everyone_is_invited = !event.audience.eql?('Coaches') mail_to_member(member, @everyone_is_invited ? "Invitation: #{@event.name}" : "Coach Invitation: #{@event.name}", &:html) diff --git a/app/services/invitation_manager.rb b/app/services/invitation_manager.rb index b4b39d58d..6a0f42c91 100644 --- a/app/services/invitation_manager.rb +++ b/app/services/invitation_manager.rb @@ -164,14 +164,16 @@ def chapter_coaches(chapter) end def create_invitation(workshop, member, role) - WorkshopInvitation.find_or_create_by!(workshop:, member:, role:) + # Identity is workshop + member; role applies only on create so a member + # subscribed as both student and coach gets one invite, not one per role. + WorkshopInvitation.find_or_create_by!(workshop:, member:) { |invitation| invitation.role = role } rescue StandardError => e log_invitation_failure(workshop, member, role, e) nil end def create_event_invitation(event, member, role) - Invitation.find_or_create_by!(event:, member:, role:) + Invitation.find_or_create_by!(event:, member:) { |invitation| invitation.role = role } rescue StandardError => e log_event_meeting_invitation_failure("event_id=#{event.id}", member, e) nil diff --git a/spec/mailers/event_invitation_mailer_spec.rb b/spec/mailers/event_invitation_mailer_spec.rb index bf34c61c7..d8c407d03 100644 --- a/spec/mailers/event_invitation_mailer_spec.rb +++ b/spec/mailers/event_invitation_mailer_spec.rb @@ -52,6 +52,15 @@ expect(email.body.encoded).to match('hello@codebar.io') end + it 'sends a generic invitation if the event audience is blank' do + blank_audience_event = Fabricate(:event, name: 'Test event', audience: '') + blank_invitation = Fabricate(:invitation, event: blank_audience_event, member:) + + described_class.invite_coach(blank_audience_event, member, blank_invitation).deliver_now + + expect(email.subject).to eq("Invitation: #{blank_audience_event.name}") + end + it 'sends a coach invitation of the event is for coaches' do email_subject = "Coach Invitation: #{event.name}" described_class.invite_coach(coach_event, member, invitation).deliver_now diff --git a/spec/services/invitation_manager_spec.rb b/spec/services/invitation_manager_spec.rb index d352421aa..3d0e45a02 100644 --- a/spec/services/invitation_manager_spec.rb +++ b/spec/services/invitation_manager_spec.rb @@ -30,67 +30,49 @@ it 'can email only students' do event = Fabricate(:event, chapters: [chapter], audience: 'Students') - students.each do |student| - allow(Invitation).to receive(:find_or_create_by!).with( - event:, member: student, role: 'Student' - ).and_call_original - end - - manager.send_event_emails(event, chapter) - students.each do |student| - expect(Invitation).to have_received(:find_or_create_by!).with(event:, member: student, role: 'Student') - end + expect do + manager.send_event_emails(event, chapter) + end.to change(Invitation, :count).by(students.count) - coaches.each do |student| - expect(Invitation).not_to have_received(:find_or_create_by!).with(event:, member: student, role: 'Coach') - end + expect(event.invitations.students.map(&:member)).to match_array(students) + expect(event.invitations.coaches).to be_empty end it 'can email only coaches' do event = Fabricate(:event, chapters: [chapter], audience: 'Coaches') - coaches.each do |student| - allow(Invitation).to receive(:find_or_create_by!).with( - event:, member: student, role: 'Coach' - ).and_call_original - end - - manager.send_event_emails(event, chapter) - - students.each do |student| - expect(Invitation).not_to have_received(:find_or_create_by!).with(event:, member: student, role: 'Student') - end + expect do + manager.send_event_emails(event, chapter) + end.to change(Invitation, :count).by(coaches.count) - coaches.each do |student| - expect(Invitation).to have_received(:find_or_create_by!).with(event:, member: student, role: 'Coach') - end + expect(event.invitations.coaches.map(&:member)).to match_array(coaches) + expect(event.invitations.students).to be_empty end it 'can email both students and coaches' do event = Fabricate(:event, chapters: [chapter]) - students.each do |student| - allow(Invitation).to receive(:find_or_create_by!).with( - event:, member: student, role: 'Student' - ).and_call_original - end + expect do + manager.send_event_emails(event, chapter) + end.to change(Invitation, :count).by(students.count + coaches.count) - coaches.each do |student| - allow(Invitation).to receive(:find_or_create_by!).with( - event:, member: student, role: 'Coach' - ).and_call_original - end + expect(event.invitations.students.map(&:member)).to match_array(students) + expect(event.invitations.coaches.map(&:member)).to match_array(coaches) + end - manager.send_event_emails(event, chapter) + it 'sends one invitation to a member subscribed as both student and coach' do + dual_member = Fabricate(:member) + Fabricate(:students, chapter:, members: [dual_member]) + Fabricate(:coaches, chapter:, members: [dual_member]) + event = Fabricate(:event, chapters: [chapter]) - students.each do |student| - expect(Invitation).to have_received(:find_or_create_by!).with(event:, member: student, role: 'Student') - end + expect do + manager.send_event_emails(event, chapter) + end.to change { Invitation.where(event:, member: dual_member).count }.by(1) - coaches.each do |student| - expect(Invitation).to have_received(:find_or_create_by!).with(event:, member: student, role: 'Coach') - end + delivered = ActionMailer::Base.deliveries.count { |e| e.to.include?(dual_member.email) } + expect(delivered).to eq(1) end it 'emails only students that accepted toc' do @@ -99,21 +81,11 @@ first_student, *other_students = students first_student.update(accepted_toc_at: nil) - other_students.each do |other_student| - allow(Invitation).to( - receive(:find_or_create_by!) - .with(event:, member: other_student, role: 'Student') - .and_call_original - ) - end - - manager.send_event_emails(event, chapter) - - expect(Invitation).not_to have_received(:find_or_create_by!).with(event:, member: first_student, role: 'Student') + expect do + manager.send_event_emails(event, chapter) + end.to change(Invitation, :count).by(other_students.count) - other_students.each do |other_student| - expect(Invitation).to have_received(:find_or_create_by!).with(event:, member: other_student, role: 'Student') - end + expect(event.invitations.students.map(&:member)).to match_array(other_students) end it 'emails only coaches that accepted toc' do @@ -122,21 +94,11 @@ first_coach, *other_coaches = coaches first_coach.update(accepted_toc_at: nil) - other_coaches.each do |other_coach| - allow(Invitation).to( - receive(:find_or_create_by!) - .with(event:, member: other_coach, role: 'Coach') - .and_call_original - ) - end - - manager.send_event_emails(event, chapter) - - expect(Invitation).not_to have_received(:find_or_create_by!).with(event:, member: first_coach, role: 'Coach') + expect do + manager.send_event_emails(event, chapter) + end.to change(Invitation, :count).by(other_coaches.count) - other_coaches.each do |other_coach| - expect(Invitation).to have_received(:find_or_create_by!).with(event:, member: other_coach, role: 'Coach') - end + expect(event.invitations.coaches.map(&:member)).to match_array(other_coaches) end end @@ -279,6 +241,16 @@ expect(invitation.role).to eq('Student') end + it 'returns existing invitation with previously_new_record? as false when called with a different role' do + invitation1 = manager.send(:create_invitation, workshop, member, 'Student') + + invitation2 = manager.send(:create_invitation, workshop, member, 'Coach') + + expect(invitation2.previously_new_record?).to be false + expect(invitation2.id).to eq(invitation1.id) + expect(invitation2.role).to eq('Student') + end + it 'returns existing invitation with previously_new_record? as false on duplicate call' do # First call creates the invitation invitation1 = manager.send(:create_invitation, workshop, member, 'Student') @@ -462,17 +434,14 @@ coaches_group.members << member_in_both_groups end - it 'creates one invitation per role when audience is everyone' do + it 'sends one invitation and one email when audience is everyone' do expect do manager.send_workshop_emails(workshop, 'everyone') - end.to change(WorkshopInvitation, :count).by(2) - - student_invitation = WorkshopInvitation.find_by(workshop:, member: member_in_both_groups, role: 'Student') - coach_invitation = WorkshopInvitation.find_by(workshop:, member: member_in_both_groups, role: 'Coach') + end.to change(WorkshopInvitation, :count).by(1) + .and change { ActionMailer::Base.deliveries.count }.by(1) - expect(student_invitation).to be_present - expect(coach_invitation).to be_present - expect(student_invitation.id).not_to eq(coach_invitation.id) + invitation = WorkshopInvitation.find_by(workshop:, member: member_in_both_groups) + expect(invitation).to be_present end end end diff --git a/spec/support/shared_examples/behaves_like_sending_workshop_emails.rb b/spec/support/shared_examples/behaves_like_sending_workshop_emails.rb index ef1907f99..38c810634 100644 --- a/spec/support/shared_examples/behaves_like_sending_workshop_emails.rb +++ b/spec/support/shared_examples/behaves_like_sending_workshop_emails.rb @@ -3,7 +3,7 @@ Fabricate(:students, chapter:, members: students) students.each do |student| - allow(WorkshopInvitation).to receive(:find_or_create_by!).with(workshop:, member: student, role: 'Student').and_call_original + allow(WorkshopInvitation).to receive(:find_or_create_by!).with(workshop:, member: student).and_call_original end expect do @@ -12,7 +12,7 @@ .and change { WorkshopInvitation.where(workshop:, role: 'Student').count }.by(students.count) students.each do |student| - expect(WorkshopInvitation).to have_received(:find_or_create_by!).with(workshop:, member: student, role: 'Student') + expect(WorkshopInvitation).to have_received(:find_or_create_by!).with(workshop:, member: student) end # Verify emails were sent to the right recipients @@ -25,7 +25,7 @@ Fabricate(:coaches, chapter:, members: coaches) coaches.each do |coach| - allow(WorkshopInvitation).to receive(:find_or_create_by!).with(workshop:, member: coach, role: 'Coach').and_call_original + allow(WorkshopInvitation).to receive(:find_or_create_by!).with(workshop:, member: coach).and_call_original end expect do @@ -34,7 +34,7 @@ .and change { WorkshopInvitation.where(workshop:, role: 'Coach').count }.by(coaches.count) coaches.each do |coach| - expect(WorkshopInvitation).to have_received(:find_or_create_by!).with(workshop:, member: coach, role: 'Coach') + expect(WorkshopInvitation).to have_received(:find_or_create_by!).with(workshop:, member: coach) end # Verify emails were sent to the right recipients @@ -48,15 +48,15 @@ Fabricate(:coaches, chapter:, members: coaches + [banned_coach]) coaches.each do |coach| - allow(WorkshopInvitation).to receive(:find_or_create_by!).with(workshop:, member: coach, role: 'Coach').and_call_original + allow(WorkshopInvitation).to receive(:find_or_create_by!).with(workshop:, member: coach).and_call_original end manager.send(send_email, workshop, 'coaches') coaches.each do |coach| - expect(WorkshopInvitation).to have_received(:find_or_create_by!).with(workshop:, member: coach, role: 'Coach') + expect(WorkshopInvitation).to have_received(:find_or_create_by!).with(workshop:, member: coach) end - expect(WorkshopInvitation).not_to have_received(:find_or_create_by!).with(workshop:, member: banned_coach, role: 'Coach') + expect(WorkshopInvitation).not_to have_received(:find_or_create_by!).with(workshop:, member: banned_coach) end it 'sends emails when a WorkshopInvitation is created' do From 791186b84b8d73800cda71b55a8136374a536005 Mon Sep 17 00:00:00 2001 From: Morgan Roderick Date: Fri, 11 Sep 2026 14:17:29 +0200 Subject: [PATCH 2/4] fix(review): one invitation per member on workshop self-RSVP; pin dual-role invite contract Applies the two validated findings from the ce-code-review run on PR #2867: - WorkshopsController#find_or_create_invitation keyed on (workshop, member, role), so a dual student+coach member self-RSVPing with the other role created a second invitation. Key on (workshop, member) and update the existing invitation to the chosen role, matching the InvitationManager identity semantics. - The dual-role specs asserted counts only; pin the surviving role (Coach for events, Student for workshops - pass order decides) and the delivered email subject so a pass reorder cannot silently flip which email dual members receive. --- app/controllers/workshops_controller.rb | 11 +++++++---- spec/controllers/workshops_controller_spec.rb | 15 +++++++++++++++ spec/services/invitation_manager_spec.rb | 13 ++++++++++--- 3 files changed, 32 insertions(+), 7 deletions(-) diff --git a/app/controllers/workshops_controller.rb b/app/controllers/workshops_controller.rb index a11ac7cce..1cb23df2e 100644 --- a/app/controllers/workshops_controller.rb +++ b/app/controllers/workshops_controller.rb @@ -44,10 +44,13 @@ def find_attending_invitation(workshop, user) end def find_or_create_invitation(workshop, user, role) - invitation = WorkshopInvitation.create_or_find_by(workshop:, - member: user, - role:) - invitation.persisted? ? invitation : WorkshopInvitation.find_by(workshop:, member: user, role:) + # Identity is workshop + member, matching InvitationManager; the member's + # role choice wins, so an existing invitation with the other role is updated. + invitation = WorkshopInvitation.find_or_create_by!(workshop:, member: user) { |record| record.role = role } + invitation.update!(role:) unless invitation.role.eql?(role) + invitation + rescue ActiveRecord::RecordNotUnique + WorkshopInvitation.find_by(workshop:, member: user) end def user_attending_or_waitlisted?(workshop, user) diff --git a/spec/controllers/workshops_controller_spec.rb b/spec/controllers/workshops_controller_spec.rb index d6421424e..79f7694fd 100644 --- a/spec/controllers/workshops_controller_spec.rb +++ b/spec/controllers/workshops_controller_spec.rb @@ -25,6 +25,21 @@ end end + context 'when the member has an invitation with a different role' do + let!(:invitation) do + Fabricate(:workshop_invitation, workshop:, member:, role: 'Student', attending: nil) + end + + it 'updates the existing invitation to the requested role instead of creating a second one' do + expect do + post :rsvp, params: { id: workshop.id, role: 'Coach' } + end.not_to change(WorkshopInvitation, :count) + + expect(invitation.reload.role).to eq('Coach') + expect(response).to redirect_to(invitation_path(invitation)) + end + end + context 'when the member does not have an invitation for the workshop and role' do it 'creates a new invitation and redirects' do expect do diff --git a/spec/services/invitation_manager_spec.rb b/spec/services/invitation_manager_spec.rb index 3d0e45a02..dbea8e83b 100644 --- a/spec/services/invitation_manager_spec.rb +++ b/spec/services/invitation_manager_spec.rb @@ -71,8 +71,13 @@ manager.send_event_emails(event, chapter) end.to change { Invitation.where(event:, member: dual_member).count }.by(1) - delivered = ActionMailer::Base.deliveries.count { |e| e.to.include?(dual_member.email) } - expect(delivered).to eq(1) + invitation = Invitation.find_by(event:, member: dual_member) + # Coaches are invited first for events, so the coach pass wins for dual members. + expect(invitation.role).to eq('Coach') + + deliveries_to_dual_member = ActionMailer::Base.deliveries.select { |e| e.to.include?(dual_member.email) } + expect(deliveries_to_dual_member.count).to eq(1) + expect(deliveries_to_dual_member.first.subject).to eq("Invitation: #{event.name}") end it 'emails only students that accepted toc' do @@ -441,7 +446,9 @@ .and change { ActionMailer::Base.deliveries.count }.by(1) invitation = WorkshopInvitation.find_by(workshop:, member: member_in_both_groups) - expect(invitation).to be_present + # Students are invited first for workshops, so the student pass wins for dual members. + expect(invitation.role).to eq('Student') + expect(ActionMailer::Base.deliveries.last.subject).to start_with('Workshop Invitation') end end end From c5d3dcfc4ac98a3466581b049603b2c031d832d4 Mon Sep 17 00:00:00 2001 From: Morgan Roderick Date: Fri, 18 Sep 2026 12:07:46 +0200 Subject: [PATCH 3/4] fix(review): one invitation per member on event RSVP paths Applies the remaining finding from the PR #2867 review: EventsController still keyed invitation lookup on (event, member, role). - #rsvp (ticket RSVP) and find_invitation_and_redirect_to_event (#student, #coach links) could create a second invitation for a dual student+coach member using the other role. Both now share find_or_create_invitation, keyed on (event, member) with the member's role choice winning, matching the InvitationManager identity and the WorkshopsController fix. - Adds dual-role specs for #student, #coach and the first coverage for POST #rsvp. Refs #2902 #2903 #2904 --- app/controllers/events_controller.rb | 17 ++-- spec/controllers/events_controller_spec.rb | 91 ++++++++++++++++++++++ 2 files changed, 103 insertions(+), 5 deletions(-) diff --git a/app/controllers/events_controller.rb b/app/controllers/events_controller.rb index 6c6d81fbc..4315b9839 100644 --- a/app/controllers/events_controller.rb +++ b/app/controllers/events_controller.rb @@ -43,8 +43,7 @@ def rsvp set_event ticket = Services::Ticket.new(request, params) member = Member.find_by(email: ticket.email) - invitation = member.invitations.where(event: @event, role: 'Student').first - invitation ||= Invitation.create_or_find_by(event: @event, member:, role: 'Student') + invitation = find_or_create_invitation(@event, member, 'Student') invitation.update(attending: true) head :ok @@ -63,9 +62,17 @@ def latest_model_updated def find_invitation_and_redirect_to_event(role) set_event - invitation = Invitation.create_or_find_by(event: @event, member: current_user, role:) - invitation = Invitation.find_by(event: @event, member: current_user, role:) unless invitation.persisted? - redirect_to event_invitation_path(@event, invitation) + redirect_to event_invitation_path(@event, find_or_create_invitation(@event, current_user, role)) + end + + def find_or_create_invitation(event, member, role) + # Identity is event + member, matching InvitationManager; the member's + # role choice wins, so an existing invitation with the other role is updated. + invitation = Invitation.find_or_create_by!(event:, member:) { |record| record.role = role } + invitation.update!(role:) unless invitation.role.eql?(role) + invitation + rescue ActiveRecord::RecordNotUnique + Invitation.find_by(event:, member:) end def set_event diff --git a/spec/controllers/events_controller_spec.rb b/spec/controllers/events_controller_spec.rb index c846a063c..55c9c90c2 100644 --- a/spec/controllers/events_controller_spec.rb +++ b/spec/controllers/events_controller_spec.rb @@ -51,6 +51,28 @@ expect(response).to redirect_to(event_invitation_path(event, invitation)) end end + + context 'when the member has a coach invitation for the event' do + let!(:invitation) { Fabricate(:coach_invitation, event:, member:, attending: nil) } + + it 'does not create a second invitation' do + expect do + get :student, params: { event_id: event.slug } + end.not_to change(Invitation, :count) + end + + it 'updates the existing invitation to the chosen role' do + get :student, params: { event_id: event.slug } + + expect(invitation.reload.role).to eql('Student') + end + + it 'redirects to the existing invitation page' do + get :student, params: { event_id: event.slug } + + expect(response).to redirect_to(event_invitation_path(event, invitation.reload)) + end + end end describe 'GET #coach' do @@ -87,6 +109,75 @@ expect(response).to redirect_to(event_invitation_path(event, invitation)) end end + + context 'when the member has a student invitation for the event' do + let!(:invitation) { Fabricate(:invitation, event:, member:, role: 'Student', attending: nil) } + + it 'does not create a second invitation' do + expect do + get :coach, params: { event_id: event.slug } + end.not_to change(Invitation, :count) + end + + it 'updates the existing invitation to the chosen role' do + get :coach, params: { event_id: event.slug } + + expect(invitation.reload.role).to eql('Coach') + end + + it 'redirects to the existing invitation page' do + get :coach, params: { event_id: event.slug } + + expect(response).to redirect_to(event_invitation_path(event, invitation.reload)) + end + end + end + + describe 'POST #rsvp' do + let(:event) { Fabricate(:event) } + let(:member) { Fabricate(:member) } + let(:ticket_params) { { event_id: event.slug, email: member.email } } + + context 'when the member does not have an invitation for the event' do + it 'creates a student invitation and marks it attending' do + expect do + post :rsvp, params: ticket_params + end.to change(Invitation, :count).by(1) + + invitation = Invitation.last + expect(invitation.role).to eql('Student') + expect(invitation.attending).to be(true) + end + end + + context 'when the member has a coach invitation for the event' do + let!(:invitation) { Fabricate(:coach_invitation, event:, member:, attending: nil) } + + it 'does not create a second invitation' do + expect do + post :rsvp, params: ticket_params + end.not_to change(Invitation, :count) + end + + it 'updates the invitation to the student role and marks it attending' do + post :rsvp, params: ticket_params + + expect(invitation.reload.role).to eql('Student') + expect(invitation.attending).to be(true) + end + end + + context 'when the member already has a student invitation for the event' do + let!(:invitation) { Fabricate(:invitation, event:, member:, role: 'Student', attending: nil) } + + it 'marks the existing invitation attending without creating a new one' do + expect do + post :rsvp, params: ticket_params + end.not_to change(Invitation, :count) + + expect(invitation.reload.attending).to be(true) + end + end end describe '#past' do From e24e8680b49c41024b4b5b9bc2ac04c03def7963 Mon Sep 17 00:00:00 2001 From: Morgan Roderick Date: Sat, 19 Sep 2026 07:58:32 +0200 Subject: [PATCH 4/4] chore(spec): deduplicate RSVP describe block for RSpec/RepeatedExampleGroupDescription --- spec/controllers/events_controller_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/controllers/events_controller_spec.rb b/spec/controllers/events_controller_spec.rb index bea4e3052..306fa616b 100644 --- a/spec/controllers/events_controller_spec.rb +++ b/spec/controllers/events_controller_spec.rb @@ -180,7 +180,7 @@ end end - describe 'POST #rsvp' do + describe 'POST #rsvp (RSVP window enforcement)' do let(:member) { Fabricate(:member) } let(:event) { Fabricate(:event) }