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	Ready	Preview	Apr 7, 2026 3:24pm

[claude]

This PR wires up Pretix voucher creation and email dispatch when a grantee confirms their grant via sendGrantReply, and refactors the admin create_grant_vouchers action to go through create_conference_voucher instead of a raw bulk-create.

External API call inside @transaction.atomic

create_grant_vouchers in grants/admin.py is decorated with @transaction.atomic and now calls create_conference_voucher per grant. create_conference_voucher in conferences/vouchers.py calls the Pretix API before saving to the DB:

pretix_voucher = create_voucher(...)           # external, non-rollbackable
conference_voucher.pretix_voucher_id = ...
conference_voucher.save()                     # inside transaction

If any unhandled exception occurs later in the loop (or the transaction is otherwise rolled back), the DB record is undone but the Pretix voucher already exists. You'd end up with orphaned Pretix vouchers with no corresponding DB row. Consider either removing the @transaction.atomic wrapper from the admin action (since partial progress is acceptable per-grant via the continue pattern), or catching exceptions per-grant and surfacing them as messages.error so the outer transaction never rolls back unexpectedly.

Race condition in create_and_send_voucher_to_grantee

The task reads the existing voucher, decides to create a new one, then calls create_conference_voucher -- but this check-then-act isn't atomic. If the task is enqueued twice (e.g., Celery retry after a transient failure partway through), two concurrent executions could both pass the existence check. The second call would then:

1. Call Pretix API -> new voucher created in Pretix (can't be rolled back)
2. Hit the unique_together = ("conference", "user") constraint -> IntegrityError

Result: an orphaned Pretix voucher. Consider using select_for_update() on the existence check, or catching the integrity error and cleaning up.

Irreversible migration

noop_reverse means rolling back the migration does nothing -- existing grant_voucher_code templates are permanently lost on rollback. This is probably intentional but worth documenting (or adding a real reverse that maps voucher_code back to grant_voucher_code for the affected rows).

[codecov]

Codecov Report

❌ Patch coverage is 97.14286% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 92.52%. Comparing base (4b6e29a) to head (180e3ae).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4602      +/-   ##
==========================================
+ Coverage   92.51%   92.52%   +0.01%     
==========================================
  Files         359      359              
  Lines       10764    10794      +30     
  Branches      814      821       +7     
==========================================
+ Hits         9958     9987      +29     
  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.
|