Merge pull request #2577 from mroderick/fix/duplicate-workshop-invitations

fix(invitations): prevent duplicate workshop invitation emails

Author: till

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

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

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

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