From 26d949930ac609deb4050a1e79a49c0767e9e41f Mon Sep 17 00:00:00 2001 From: Morgan Roderick Date: Fri, 11 Sep 2026 13:56:52 +0200 Subject: [PATCH 1/2] 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/2] 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