From 4bd3c78f1d5f457f07a2b0950430d6c0e104d5d6 Mon Sep 17 00:00:00 2001 From: Morgan Roderick Date: Thu, 9 Apr 2026 17:52:01 +0200 Subject: [PATCH 1/3] fix: handle duplicate InvitationLogEntry on retry Second attempt at fixing the "Member has already been taken" validation error when re-sending workshop invitations. Problem: The previous fix (PR #2556) used find_or_initialize_by with the status included in the query. However, the InvitationLogEntry uniqueness constraint only validates (member_id, invitation_type, invitation_id), NOT status. This means: - A member can have ONE success entry AND ONE failure entry for the same invitation - find_or_initialize_by(member, invitation, :success) would return a persisted entry with status=:failed if one existed, causing the wrong status to be used - Or if building a new entry with status=:success when status=:failed already existed, the uniqueness validation would fail Fix: 1. Split the check: first find any existing entry by (member, invitation), ignoring status 2. If found, return it (either success or failure) 3. Only if no entry exists, create a new one with the specified status Additionally, add autosave: false to the has_many :entries association to prevent Rails from attempting to autosave unpersisted entries when fail_batch is called, which was causing "Validation failed: Entries is invalid" errors. Changes: - app/services/invitation_logger.rb: Separate find_or_build_entry logic - app/models/invitation_log.rb: Add autosave: false to entries association Tests: - All 41 existing tests pass - Retry behavior tests verify no duplicate entries are created --- app/models/invitation_log.rb | 2 +- app/services/invitation_logger.rb | 11 +++++++---- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/app/models/invitation_log.rb b/app/models/invitation_log.rb index 83cb0f13e..199c34d6d 100644 --- a/app/models/invitation_log.rb +++ b/app/models/invitation_log.rb @@ -6,7 +6,7 @@ class InvitationLog < ApplicationRecord belongs_to :loggable, polymorphic: true belongs_to :initiator, class_name: 'Member', optional: true belongs_to :chapter, optional: true - has_many :entries, class_name: 'InvitationLogEntry', dependent: :destroy + has_many :entries, class_name: 'InvitationLogEntry', dependent: :destroy, autosave: false before_create :set_expires_at diff --git a/app/services/invitation_logger.rb b/app/services/invitation_logger.rb index fc81c31a0..d9274712e 100644 --- a/app/services/invitation_logger.rb +++ b/app/services/invitation_logger.rb @@ -78,10 +78,13 @@ def fail_batch(error) private def find_or_build_entry(member, invitation, status) - @log.entries.find_or_initialize_by( - member:, - invitation:, - status: + existing_entry = @log.entries.find_by(member: member, invitation: invitation) + return existing_entry if existing_entry + + @log.entries.new( + member: member, + invitation: invitation, + status: status ) end From c11268413cd66314c8c44eeab2d2a072176317a9 Mon Sep 17 00:00:00 2001 From: Morgan Roderick Date: Fri, 10 Apr 2026 08:15:44 +0200 Subject: [PATCH 2/3] test: add cross-status retry tests for PR #2558 Verify that when logging success/failure for a member+invitation that already has an entry with a different status, the existing entry is returned (not a new one created, which would cause uniqueness violation) --- spec/services/invitation_logger_spec.rb | 28 +++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/spec/services/invitation_logger_spec.rb b/spec/services/invitation_logger_spec.rb index 14cd4b3c9..6595009d8 100644 --- a/spec/services/invitation_logger_spec.rb +++ b/spec/services/invitation_logger_spec.rb @@ -129,4 +129,32 @@ expect(log.completed_at).to be_present end end + + describe 'cross-status retry (PR #2558 fix)' do + let(:logger) { described_class.new(workshop, initiator, 'students', :invite) } + let!(:log) { logger.start_batch } + + it 'returns existing failure entry when logging success for same member+invitation' do + error = StandardError.new('SMTP error') + failure_entry = logger.log_failure(member, invitation, error) + + success_entry = logger.log_success(member, invitation) + + expect(success_entry).to eq failure_entry + expect(success_entry.status).to eq 'failed' + expect(log.reload.success_count).to eq 0 + expect(log.reload.failure_count).to eq 1 + end + + it 'returns existing success entry when logging failure for same member+invitation' do + logger.log_success(member, invitation) + + error = StandardError.new('SMTP error') + failure_entry = logger.log_failure(member, invitation, error) + + expect(failure_entry.status).to eq 'success' + expect(log.reload.success_count).to eq 1 + expect(log.reload.failure_count).to eq 0 + end + end end From 72f387ce6830a46656242de6618511a68becebcd Mon Sep 17 00:00:00 2001 From: Morgan Roderick Date: Fri, 10 Apr 2026 08:49:29 +0200 Subject: [PATCH 3/3] refactor: use find_or_create_by with block (PR #2558) Refactored per olleolleolle's suggestion: - Use find_or_create_by(member, invitation) with block for status - Check processed_at instead of persisted? for retry detection - Cleaner, more idiomatic Rails pattern --- app/services/invitation_logger.rb | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/app/services/invitation_logger.rb b/app/services/invitation_logger.rb index d9274712e..9c5e31898 100644 --- a/app/services/invitation_logger.rb +++ b/app/services/invitation_logger.rb @@ -23,7 +23,7 @@ def log_success(member, invitation = nil) return unless @log entry = find_or_build_entry(member, invitation, :success) - return entry if entry.persisted? + return entry if entry.processed_at entry.assign_attributes(processed_at: Time.current) save_entry(entry, :success_count) @@ -33,7 +33,7 @@ def log_failure(member, invitation, error) return unless @log entry = find_or_build_entry(member, invitation, :failed) - return entry if entry.persisted? + return entry if entry.processed_at entry.assign_attributes( failure_reason: error.message, @@ -46,7 +46,7 @@ def log_skipped(member, invitation, reason) return unless @log entry = find_or_build_entry(member, invitation, :skipped) - return entry if entry.persisted? + return entry if entry.processed_at entry.assign_attributes( failure_reason: reason, @@ -78,14 +78,9 @@ def fail_batch(error) private def find_or_build_entry(member, invitation, status) - existing_entry = @log.entries.find_by(member: member, invitation: invitation) - return existing_entry if existing_entry - - @log.entries.new( - member: member, - invitation: invitation, - status: status - ) + @log.entries.find_or_create_by(member: member, invitation: invitation) do |entry| + entry.status = status + end end def save_entry(entry, counter)