diff --git a/app/models/concerns/workshop_invitation_manager_concerns.rb b/app/models/concerns/workshop_invitation_manager_concerns.rb index b7c41dc28..23719264d 100644 --- a/app/models/concerns/workshop_invitation_manager_concerns.rb +++ b/app/models/concerns/workshop_invitation_manager_concerns.rb @@ -95,7 +95,9 @@ def send_workshop_waiting_list_reminders(workshop) private def create_invitation(workshop, member, role) - WorkshopInvitation.find_or_create_by(workshop: workshop, member: member, role: role) + invitation = WorkshopInvitation.find_or_initialize_by(workshop: workshop, member: member, role: role) + invitation.save! if invitation.new_record? + invitation rescue StandardError => e log_invitation_failure(workshop, member, role, e) nil @@ -111,56 +113,42 @@ def log_invitation_failure(workshop, member, role, error) end def invite_coaches_to_virtual_workshop(workshop, logger = nil) - count = 0 - chapter_coaches(workshop.chapter).shuffle.each do |coach| - invitation = create_invitation(workshop, coach, 'Coach') - next unless invitation - - count += 1 - send_email_with_logging(logger, coach, invitation) do - VirtualWorkshopInvitationMailer.invite_coach(workshop, coach, invitation).deliver_now - end + invite_members(workshop, logger, chapter_coaches(workshop.chapter)) do |coach, invitation| + VirtualWorkshopInvitationMailer.invite_coach(workshop, coach, invitation).deliver_now end - count end def invite_coaches_to_workshop(workshop, logger = nil) - count = 0 - chapter_coaches(workshop.chapter).shuffle.each do |coach| - invitation = create_invitation(workshop, coach, 'Coach') - next unless invitation - - count += 1 - send_email_with_logging(logger, coach, invitation) do - WorkshopInvitationMailer.invite_coach(workshop, coach, invitation).deliver_now - end + invite_members(workshop, logger, chapter_coaches(workshop.chapter)) do |coach, invitation| + WorkshopInvitationMailer.invite_coach(workshop, coach, invitation).deliver_now end - count end def invite_students_to_virtual_workshop(workshop, logger = nil) - count = 0 - chapter_students(workshop.chapter).shuffle.each do |student| - invitation = create_invitation(workshop, student, 'Student') - next unless invitation - - count += 1 - send_email_with_logging(logger, student, invitation) do - VirtualWorkshopInvitationMailer.invite_student(workshop, student, invitation).deliver_now - end + invite_members(workshop, logger, chapter_students(workshop.chapter), 'Student') do |student, invitation| + VirtualWorkshopInvitationMailer.invite_student(workshop, student, invitation).deliver_now end - count end def invite_students_to_workshop(workshop, logger = nil) + invite_members(workshop, logger, chapter_students(workshop.chapter), 'Student') do |member, invitation| + WorkshopInvitationMailer.invite_student(workshop, member, invitation).deliver_now + end + end + + def invite_members(workshop, logger, members, role = 'Coach') count = 0 - chapter_students(workshop.chapter).shuffle.each do |student| - invitation = create_invitation(workshop, student, 'Student') + members.shuffle.each do |member| + invitation = create_invitation(workshop, member, role) next unless invitation - count += 1 - send_email_with_logging(logger, student, invitation) do - WorkshopInvitationMailer.invite_student(workshop, student, invitation).deliver_now + if invitation.previously_new_record? + count += 1 + send_email_with_logging(logger, member, invitation) do + yield member, invitation + end + else + logger&.log_skipped(member, invitation, 'Already invited to this workshop') end end count diff --git a/spec/models/invitation_manager_deduplication_spec.rb b/spec/models/invitation_manager_deduplication_spec.rb index 22438b5b6..58a3f2c5f 100644 --- a/spec/models/invitation_manager_deduplication_spec.rb +++ b/spec/models/invitation_manager_deduplication_spec.rb @@ -52,18 +52,18 @@ coaches_group.members << member_in_both_groups end - it 'sends one invitation per role when audience is everyone' do - expect(WorkshopInvitation).to receive(:find_or_create_by) - .with(workshop: workshop, member: member_in_both_groups, role: 'Student') - .and_call_original - .once + it 'creates one invitation per role when audience is everyone' do + expect do + manager.send_workshop_emails(workshop, 'everyone') + end.to change(WorkshopInvitation, :count).by(2) - expect(WorkshopInvitation).to receive(:find_or_create_by) - .with(workshop: workshop, member: member_in_both_groups, role: 'Coach') - .and_call_original - .once + # Verify the member has one invitation per role + student_invitation = WorkshopInvitation.find_by(workshop: workshop, member: member_in_both_groups, role: 'Student') + coach_invitation = WorkshopInvitation.find_by(workshop: workshop, member: member_in_both_groups, role: 'Coach') - manager.send_workshop_emails(workshop, 'everyone') + expect(student_invitation).to be_present + expect(coach_invitation).to be_present + expect(student_invitation.id).not_to eq(coach_invitation.id) end end end diff --git a/spec/models/invitation_manager_spec.rb b/spec/models/invitation_manager_spec.rb index 39ba6f75c..7059e415d 100644 --- a/spec/models/invitation_manager_spec.rb +++ b/spec/models/invitation_manager_spec.rb @@ -241,11 +241,44 @@ end end + describe '#create_invitation' do + let(:member) { Fabricate(:member) } + + it 'creates a new invitation and returns it with previously_new_record? as true' do + invitation = manager.send(:create_invitation, workshop, member, 'Student') + + expect(invitation).to be_a(WorkshopInvitation) + expect(invitation).to be_persisted + expect(invitation.previously_new_record?).to be true + expect(invitation.workshop).to eq(workshop) + expect(invitation.member).to eq(member) + expect(invitation.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') + expect(invitation1.previously_new_record?).to be true + + # Second call finds the existing invitation + invitation2 = manager.send(:create_invitation, workshop, member, 'Student') + expect(invitation2.previously_new_record?).to be false + expect(invitation2.id).to eq(invitation1.id) + end + + it 'does not create duplicate records in database' do + expect do + # Multiple calls should not create duplicates + 3.times { manager.send(:create_invitation, workshop, member, 'Student') } + end.to change(WorkshopInvitation, :count).by(1) + end + end + describe '#create_invitation resilience' do let(:member) { Fabricate(:member) } - it 'returns nil when find_or_create_by raises an exception' do - allow(WorkshopInvitation).to receive(:find_or_create_by) + it 'returns nil when find_or_initialize_by raises an exception' do + allow(WorkshopInvitation).to receive(:find_or_initialize_by) .and_raise(StandardError.new('database error')) result = manager.send(:create_invitation, workshop, member, 'Student') @@ -254,7 +287,7 @@ end it 'logs error with member_id and workshop_id but no PII' do - allow(WorkshopInvitation).to receive(:find_or_create_by) + allow(WorkshopInvitation).to receive(:find_or_initialize_by) .and_raise(StandardError.new('database error')) expect(Rails.logger).to receive(:error) do |message| @@ -272,7 +305,7 @@ Fabricate(:students, chapter: chapter, members: students) call_count = 0 - allow(WorkshopInvitation).to receive(:find_or_create_by) do + allow(WorkshopInvitation).to receive(:find_or_initialize_by) do call_count += 1 if call_count == 1 raise StandardError.new('database error') @@ -286,4 +319,49 @@ end.not_to raise_error end end + + describe 'duplicate invitation prevention' do + let(:initiator) { Fabricate(:member) } + + before do + Fabricate(:students, chapter: chapter, members: students) + end + + it 'logs skipped entries for already invited members when re-running batch' do + # First invitation round + manager.send_workshop_emails(workshop, 'students', initiator.id) + + # Get the first log + first_log = InvitationLog.where(loggable: workshop, audience: 'students').order(:created_at).first + expect(first_log.success_count).to eq(students.count) + expect(first_log.skipped_count).to eq(0) + + # Second invitation round + manager.send_workshop_emails(workshop, 'students', initiator.id) + + # Get the second log + second_log = InvitationLog.where(loggable: workshop, audience: 'students').order(:created_at).last + expect(second_log.success_count).to eq(0) + expect(second_log.skipped_count).to eq(students.count) + end + + it 'only sends emails to newly eligible members when batch is re-run' do + # First invitation round + manager.send_workshop_emails(workshop, 'students', initiator.id) + ActionMailer::Base.deliveries.clear + + # Add a new student + new_student = Fabricate(:member) + Fabricate(:students, chapter: chapter, members: [new_student]) + + # Second invitation round - should only email the new student + expect do + manager.send_workshop_emails(workshop, 'students', initiator.id) + end.to change { ActionMailer::Base.deliveries.count }.by(1) + + log = InvitationLog.where(loggable: workshop, audience: 'students').order(:created_at).last + expect(log.success_count).to eq(1) + expect(log.skipped_count).to eq(students.count) + 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 783e65d8c..925e169dc 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: chapter, members: students) students.each do |student| - expect(WorkshopInvitation).to receive(:find_or_create_by).with(workshop: workshop, member: student, role: 'Student').and_call_original + expect(WorkshopInvitation).to receive(:find_or_initialize_by).with(workshop: workshop, member: student, role: 'Student').and_call_original expect(mailer).to receive(:invite_student).and_call_original end @@ -14,7 +14,7 @@ Fabricate(:coaches, chapter: chapter, members: coaches) coaches.each do |coach| - expect(WorkshopInvitation).to receive(:find_or_create_by).with(workshop: workshop, member: coach, role: 'Coach').and_call_original + expect(WorkshopInvitation).to receive(:find_or_initialize_by).with(workshop: workshop, member: coach, role: 'Coach').and_call_original expect(mailer).to receive(:invite_coach).and_call_original end @@ -26,9 +26,9 @@ Fabricate(:coaches, chapter: chapter, members: coaches + [banned_coach]) coaches.each do |coach| - expect(WorkshopInvitation).to receive(:find_or_create_by).with(workshop: workshop, member: coach, role: 'Coach').and_call_original + expect(WorkshopInvitation).to receive(:find_or_initialize_by).with(workshop: workshop, member: coach, role: 'Coach').and_call_original end - expect(WorkshopInvitation).not_to receive(:find_or_create_by).with(workshop: workshop, member: banned_coach, role: 'Coach') + expect(WorkshopInvitation).not_to receive(:find_or_initialize_by).with(workshop: workshop, member: banned_coach, role: 'Coach') manager.send(send_email, workshop, 'coaches') end @@ -45,10 +45,25 @@ it 'does not send emails when invitation creation returns nil' do Fabricate(:students, chapter: chapter, members: students) - expect(WorkshopInvitation).to receive(:find_or_create_by).and_return(nil).exactly(students.count) + expect(WorkshopInvitation).to receive(:find_or_initialize_by).and_return(nil).exactly(students.count) expect do manager.send(send_email, workshop, 'students') end.not_to(change { ActionMailer::Base.deliveries.count }) end + + it 'does not send duplicate emails when members are already invited' do + Fabricate(:students, chapter: chapter, members: students) + + # First invitation round - creates invitations and sends emails + manager.send(send_email, workshop, 'students') + + # Clear deliveries to track second round + ActionMailer::Base.deliveries.clear + + # Second invitation round - should not send duplicate emails + expect do + manager.send(send_email, workshop, 'students') + end.not_to(change { ActionMailer::Base.deliveries.count }) + end end