feat(grants): Pretix voucher and email when grantee confirms

[estyxx]

When a grantee confirms via sendGrantReply (status becomes confirmed), enqueue create_and_send_voucher_to_grantee: ensure a GRANT ConferenceVoucher in Pretix (create_conference_voucher), then email with send_conference_voucher_email (same Voucher code template as the conference admin action).

Admin: Create grant vouchers uses Pretix creation and co-speaker → grant upgrade. Confirm pending status is unchanged (no vouchers there).

Data: Migration rewrites any grant_voucher_code email templates to voucher_code and drops that identifier from choices.

Tests: GraphQL confirm path, grant admin voucher action, grant task branches.

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
pycon	Error		Apr 10, 2026 10:21am

[claude]

This PR wires up Pretix voucher creation and email sending when a grantee confirms their grant, and consolidates the grant_voucher_code email template identifier into the existing voucher_code one.

Missing transaction in create_grant_vouchers admin action

The @transaction.atomic decorator was removed from grants/admin.py. The old code batched all DB writes atomically. Now each grant calls create_conference_voucher() (Pretix API call + DB save) followed by admin log writes, with no outer transaction. If processing fails mid-loop after a Pretix voucher was already created, you get partial results and potentially orphaned Pretix vouchers. Each per-grant block should at minimum be wrapped in transaction.atomic() so a failure for one grant does not corrupt its own log entries.

Race condition in create_and_send_voucher_to_grantee

grants/tasks.py uses a check-then-create pattern that is not atomic. ConferenceVoucher has unique_together = (conference, user), so a concurrent duplicate task execution would raise an unhandled IntegrityError at conference_voucher.save() inside create_conference_voucher() — after the Pretix API call already succeeded — leaving an orphaned Pretix voucher with no DB record. Using select_for_update() on the lookup or catching IntegrityError around the create would close this.

Silent skip for SPEAKER voucher type in admin action

When an existing GRANT or SPEAKER voucher is found in create_grant_vouchers, the code does a silent continue with no admin message. A SPEAKER skip is particularly surprising since the admin selected these grants explicitly to create vouchers. At least a messages.info noting the skip reason would prevent confusion.

Missing test: Pretix failure path in create_and_send_voucher_to_grantee

No test covers create_conference_voucher() raising (e.g., Pretix is down). The task has no error handling around that call, so the exception propagates to Celery for retry. Worth adding a test confirming no email is dispatched when voucher creation fails.

[codecov]

Codecov Report

❌ Patch coverage is 96.96970% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 92.52%. Comparing base (4b6e29a) to head (318c9fb).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #4602   +/-   ##
=======================================
  Coverage   92.51%   92.52%           
=======================================
  Files         359      359           
  Lines       10764    10791   +27     
  Branches      814      821    +7     
=======================================
+ Hits         9958     9984   +26     
  Misses        696      696           
- Partials      110      111    +1     

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[estyxx]

@claude do the code review again

[claude]

Claude finished @estyxx's task in 1m 14s —— View job

---

Code Review Complete

• Read changed files and understand the implementation
• Review grants/tasks.py - new voucher task logic
• Review grants/admin.py - refactored admin action
• Review api/grants/mutations.py - mutation changes
• Review test coverage
• Provide comprehensive feedback

---

Overall Assessment

The implementation is well-structured and the test coverage is comprehensive. The PR successfully wires up Pretix voucher creation when a grantee confirms, and properly reuses send_conference_voucher_email instead of a separate grant_voucher_code email. Below are my findings:

---

Issues to Address

1. External API Call Inside @transaction.atomic (Medium Risk)

File: grants/admin.py:302-374

The create_grant_vouchers admin action is wrapped in @transaction.atomic but calls create_conference_voucher() which makes an external Pretix API call before saving to DB:

@transaction.atomic
def create_grant_vouchers(modeladmin, request, queryset):
    ...
    new_voucher = create_conference_voucher(...)  # Calls Pretix API

If an exception occurs after the Pretix voucher is created but before the DB save completes (or if a later grant in the loop fails), the transaction rolls back but the Pretix voucher already exists — creating orphaned vouchers.

Suggested fix: Remove @transaction.atomic (the continue pattern already handles partial progress), or wrap each grant's voucher creation in a try/except with messages.error() so exceptions don't bubble up.

Fix this →

---

2. Race Condition in create_and_send_voucher_to_grantee Task (Low-Medium Risk)

File: grants/tasks.py:27-69

The task checks for existing vouchers, then creates a new one — but this check-then-act isn't atomic:

conference_voucher = ConferenceVoucher.objects.for_conference(conference).for_user(user).first()

if conference_voucher:
    # handle existing
    ...

new_voucher = create_conference_voucher(...)  # Creates in Pretix first

If Celery retries this task (e.g., after a transient failure), two concurrent executions could both pass the existence check. The second would:

1. Call Pretix API → voucher created (irreversible)
2. Hit the unique_together constraint → IntegrityError

Result: orphaned Pretix voucher with no DB record.

Suggested fix: Use select_for_update() on the existence check, or catch IntegrityError after create_conference_voucher() and log a warning (the voucher exists, which is the desired end state).

Fix this →

---

3. Irreversible Migration

File: notifications/migrations/0022_remove_grant_voucher_code_template_identifier.py:13-14

The noop_reverse function means rolling back the migration doesn't restore the original grant_voucher_code identifiers:

def noop_reverse(apps, schema_editor):
    pass

This is probably intentional since you're consolidating to voucher_code, but worth documenting or adding a real reverse that maps back to grant_voucher_code if needed.

---

Positive Observations

1. Excellent test coverage — The new task tests (grants/tests/test_tasks.py:476-643) thoroughly cover all branches: not confirmed, no user, existing speaker/grant voucher, co-speaker upgrade, and voucher on other conference.

2. Idempotent co-speaker upgrade — The logic correctly upgrades CO_SPEAKER to GRANT without creating duplicates, and resets voucher_email_sent_at to ensure the email is re-sent.

3. Good separation of concerns — Reusing send_conference_voucher_email instead of a separate grant_voucher_code email simplifies the notification system.

4. Proper admin audit logging — The admin action correctly logs voucher creation and co-speaker upgrades.

5. GraphQL mutation correctly gates voucher creation — Only triggers on status change to confirmed (api/grants/mutations.py:353-354).

---

Minor Suggestions

1. Consider adding acks_late=True to the Celery task to reduce the retry window for the race condition mentioned above.

2. Prefetch optimization — The admin get_queryset already prefetches vouchers via annotation, which is good for list display performance.

---

Summary

The PR is well-implemented with good test coverage. The main concerns are:

• Medium: @transaction.atomic wrapper in admin action that calls external API
• Low-Medium: Race condition in task (can be mitigated with IntegrityError handling)
• Informational: Irreversible migration

These are edge cases that may rarely occur in practice, but worth addressing for robustness.
|

[estyxx]

@claude review

[claude]

Claude finished @estyxx's task in 1m 34s —— View job

---

Code Review

• Read changed files and understand the implementation
• Review grants/tasks.py - voucher task logic
• Review grants/admin.py - admin action
• Review api/grants/mutations.py - mutation changes
• Review test coverage
• Check if previous review issues were addressed
• Provide comprehensive feedback

---

Summary

The implementation is well-structured overall. The test coverage is comprehensive and the code successfully wires up Pretix voucher creation when a grantee confirms. Below I've identified issues that should be addressed, including some from the previous review that remain unfixed.

---

Issues to Address

1. Task Retry Permanently Drops the Voucher Email (Still Present)

File: grants/tasks.py:42-52

if conference_voucher:
    if conference_voucher.voucher_type in (
        ConferenceVoucher.VoucherType.GRANT,
        ConferenceVoucher.VoucherType.SPEAKER,
    ):
        logger.info(...)
        return  # ← Returns without checking voucher_email_sent_at

If create_conference_voucher succeeds but send_conference_voucher_email.delay fails (e.g., Redis blip), the task retries. On retry, it finds the existing GRANT voucher and returns early — the email is never sent. The grantee has a Pretix voucher but never receives notification.

Suggested fix: Before returning early, check if voucher_email_sent_at is None and dispatch the email if so:

if conference_voucher.voucher_type in (...):
    if conference_voucher.voucher_email_sent_at is None:
        send_conference_voucher_email.delay(conference_voucher_id=conference_voucher.id)
    return

Fix this →

---

2. External API Call Inside @transaction.atomic (Still Present)

File: grants/admin.py:302-374

The create_grant_vouchers admin action is wrapped in @transaction.atomic but calls create_conference_voucher() which makes an external Pretix API call:

@admin.action(description="Create grant vouchers")
@validate_single_conference_selection
@transaction.atomic  # ← Wraps external API call
def create_grant_vouchers(modeladmin, request, queryset):
    ...
    new_voucher = create_conference_voucher(...)  # Calls Pretix API

If an exception occurs after the Pretix voucher is created but before the DB save completes (or if a later grant in the loop fails), the transaction rolls back but the Pretix voucher already exists — creating orphaned vouchers in Pretix with no DB record.

Suggested fix: Remove @transaction.atomic decorator since the function already handles partial progress with continue statements, or wrap each grant's processing in its own try/except.

Fix this →

---

3. Race Condition in Task (Low-Medium Risk)

File: grants/tasks.py:38-69

The task checks for existing vouchers then creates a new one — this check-then-act isn't atomic. If Celery executes this task concurrently (e.g., duplicate trigger, or retry overlaps with original), both executions could pass the existence check, both call Pretix API, and one hits the unique_together constraint with IntegrityError — leaving an orphaned Pretix voucher.

Suggested fix: Wrap the create_conference_voucher() call and subsequent code in a try/except IntegrityError block, logging a warning if caught (the voucher exists, which is the desired end state).

Fix this →

---

4. Missing Test for Retry/Idempotency Scenario

File: grants/tests/test_tasks.py

There's no test covering: voucher already exists with GRANT type but voucher_email_sent_at is None (the broken-retry state). The current suite would pass while issue #1 above is live.

---

Minor / Informational

Irreversible Migration

File: notifications/migrations/0022_remove_grant_voucher_code_template_identifier.py:13-14

The noop_reverse means rolling back this migration leaves all formerly grant_voucher_code templates permanently renamed to voucher_code. Worth adding a comment explaining this is intentional consolidation if that's the case.

---

Positive Observations

1. Excellent test coverage — The new task tests (grants/tests/test_tasks.py:476-643) thoroughly cover all branches: not confirmed, no user, existing speaker/grant voucher, co-speaker upgrade, and voucher on other conference.

2. Idempotent co-speaker upgrade — The logic correctly upgrades CO_SPEAKER to GRANT without creating duplicates, and resets voucher_email_sent_at to ensure the email is re-sent.

3. Good separation of concerns — Reusing send_conference_voucher_email instead of a separate grant_voucher_code email simplifies the notification system.

4. Proper admin audit logging — The admin action correctly logs voucher creation and co-speaker upgrades.

5. GraphQL mutation correctly gates voucher creation — Only triggers on status change to confirmed (api/grants/mutations.py:353-354).

---

Summary

The main concerns are:

• High: Task retry can permanently drop the voucher email (issue Explore Apollo #1)
• Medium: @transaction.atomic wrapper in admin action with external API call (issue Login / Authentication #2)
• Low-Medium: Race condition in task without IntegrityError handling (issue Explore Wagtail #3)

I recommend addressing issues #1 and #2 before merging. Issue #3 is an edge case but worth considering for robustness.

---