fix: handle duplicate InvitationLogEntry creation on retry

[mroderick]

Summary

Fixes the "Member has already been taken" validation error when re-sending workshop invitations or sending to audiences that include members with existing invitations.

Problem

When admins re-send invitations for a workshop, the InvitationLogger would attempt to create a new InvitationLogEntry for members who already had log entries from previous sends. This caused a uniqueness validation failure:

Validation failed: Member has already been taken

This error would cause the invitation batch job to fail mid-process, requiring manual retries.

Root Cause

InvitationLogEntry has a uniqueness validation on (member_id, invitation_type, invitation_id):

validates :member_id, uniqueness: { scope: %i[invitation_type invitation_id] }

When re-sending invitations:

1. find_or_create_by returns the existing invitation (no error)
2. Email is sent successfully
3. logger.log_success(member, invitation) tries to create a new log entry
4. The uniqueness constraint fails because an entry already exists

Affected Scenarios

• Re-sending invitations for the same workshop
• Sending invitations with audience='everyone' where a member is in both Students and Coaches groups
• Retrying failed invitation batches
• Any scenario where the same member+invitation combination is processed multiple times

Solution

Replace create! with find_or_initialize_by in the logging methods:

# Before (fails on retry)
@log.entries.create!(member: member, invitation: invitation, status: :success, ...)

# After (idempotent)
entry = @log.entries.find_or_initialize_by(member: member, invitation: invitation, status: :success)
return entry if entry.persisted?  # Already logged - skip

entry.assign_attributes(processed_at: Time.current)
entry.save!
@log.increment!(:success_count)

This is the idiomatic Rails pattern for handling "create if not exists" scenarios.

Changes

• app/services/invitation_logger.rb - Core fix with helper methods
• spec/services/invitation_logger_spec.rb - Added retry tests for all three logging methods

Testing

All tests pass (41 examples):

• Original tests for log_success, log_failure, log_skipped
• New tests: "does not create duplicate entry on retry" for each method

Related

This fix works in conjunction with PR #2551 (fix: deduplicate members in chapter_students and chapter_coaches) which addressed a different aspect of the duplicate invitation issue - preventing duplicate member processing at the chapter group level.

Together, these fixes ensure:

1. Members aren't processed multiple times within a single audience send (PR fix: deduplicate members in chapter_students and chapter_coaches #2551)
2. Re-sending invitations doesn't fail with validation errors (this PR)

[mroderick]

The send invitation functionality still fails for workshops, where we've previously sent out invitations, even after #2553 has been deployed.

[olleolleolle]

Sounds like an improvement!

[mroderick]

Sounds like an improvement!

The irony is that this was introduced by me in #2546 ... no good deed goes unpunished 😉

[till]

Hi @till,

Good question! I have considered this carefully, and I believe save! is the right choice here. Let me explain:

Why the current approach is correct:

1. This fix prevents the original error - By using find_or_initialize_by, we check for existing entries before creating. The original "Member has already been taken" error happened because we blindly called create! without checking. This fix solves that root cause.

2. The race condition is very unlikely - The only scenario where save! would raise is if two processes both called find_or_initialize_by simultaneously, both got an unpersisted record, and both tried to save. In practice, invitations are processed by a single Delayed Job worker, so this is extremely rare.

3. Silently swallowing errors would be worse - If we catch and skip on failure, we lose visibility into potential issues. The uniqueness constraint exists as a safety net - if something unexpected happens, we want to know.

4. The log entries matter - Unlike optional debug logging, these entries track invitation delivery for audit purposes and debugging delivery issues. We want to know if something goes wrong.

Happy to discuss further if you think there is a real scenario I am missing!

[mroderick]

Hi @till,

Good question! I have considered this carefully, and I believe save! is the right choice here. Let me explain:

Why the current approach is correct:

1. This fix prevents the original error - By using find_or_initialize_by, we check for existing entries before creating. The original "Member has already been taken" error happened because we blindly called create! without checking. This fix solves that root cause.

2. The race condition is very unlikely - The only scenario where save! would raise is if two processes both called find_or_initialize_by simultaneously, both got an unpersisted record, and both tried to save. In practice, invitations are processed by a single Delayed Job worker, so this is extremely rare.

3. Silently swallowing errors would be worse - If we catch and skip on failure, we lose visibility into potential issues. The uniqueness constraint exists as a safety net - if something unexpected happens, we want to know.

4. The log entries matter - Unlike optional debug logging, these entries track invitation delivery for audit purposes and debugging delivery issues. We want to know if something goes wrong.

Happy to discuss further if you think there is a real scenario I am missing!