Update grant approved email to show combined reimbursement total

[estyxx]

Summary

• Add has_approved_ticket(), has_ticket_only() methods to Grant model
• Add total_grantee_reimbursement_amount property (excludes ticket)
• Refactor has_approved() to be base method for all has_approved_* methods
• Replace old placeholders (has_approved_travel, has_approved_accommodation, travel_amount) with simpler total_amount and ticket_only
• Update plain_cards to show total reimbursement instead of just travel
• Update visa/models.py to use has_approved_ticket()

This allows showing grantees a single total amount they can use flexibly for travel and/or accommodation, rather than separate category amounts.

New Email Template Variables

• {{ total_amount }} - total reimbursement (excluding ticket), formatted as string, or None if zero
• {{ ticket_only }} - True if grant has only ticket, no travel or accommodation

Removed Variables

• has_approved_travel
• has_approved_accommodation
• travel_amount

After merging

1. Update the grant_approved email template body in Django admin with the new variables

Test plan

• Add tests for has_approved_ticket()
• Add tests for has_ticket_only()
• Add tests for total_grantee_reimbursement_amount
• Update task tests for new placeholder format
• Update plain_cards test for total reimbursement

[vercel]

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

Project	Deployment	Review	Updated (UTC)
pycon	Ready	Preview	Jan 27, 2026 3:47pm

[estyxx]

@claude do the code review

[claude]

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

---

Code Review

• Gather context and read changed files
• Review migration file
• Review model changes
• Review admin changes
• Review task changes
• Review test changes
• Review notification model changes
• Provide summary and feedback

---

Overall, this is a well-structured PR that simplifies the email placeholder logic. The changes are clean and the test coverage is thorough. I have a few observations:

✅ What looks good

1. Migration (backend/grants/migrations/0032_add_is_internal_to_reimbursement_category.py): Clean, straightforward migration adding the is_internal boolean field with a sensible default of False.

2. Model properties (backend/grants/models.py:282-306): The new total_grantee_reimbursement_amount and has_only_internal_reimbursements properties are well-documented and logically correct.

3. Admin integration (backend/grants/admin.py:429-431): The is_internal field is properly added to both list_display and list_filter.

4. Test coverage (backend/grants/tests/test_tasks.py): Excellent test coverage with cases for:

   • Ticket only grants
   • Travel + accommodation + ticket grants
   • Travel + ticket grants
   • Reminder emails

5. Factory updates (backend/grants/tests/factories.py:78-108): Good addition of traits for different category types, making tests cleaner.

⚠️ Minor observations

1. Potential N+1 query (backend/grants/models.py:293-296):

   @property
   def total_grantee_reimbursement_amount(self):
       return sum(
           r.granted_amount
           for r in self.reimbursements.filter(category__is_internal=False)
       )

   This queries the database each time the property is accessed. In the task context (backend/grants/tasks.py:30), this is fine since it's a single grant. However, if used in a list view, this could cause N+1 queries. Consider using prefetch_related('reimbursements__category') when fetching grants in bulk (already done in GrantAdmin.get_queryset at line 703).

2. Consistency with existing methods (backend/grants/models.py:272-280): The existing has_approved_travel() and has_approved_accommodation() methods remain but aren't used in the email anymore. Consider adding a deprecation note or removing them if they're not used elsewhere. Fix this →

3. Missing user_name in AVAILABLE_PLACEHOLDERS (backend/notifications/models.py:114-125): The grant_approved template doesn't list user_name in AVAILABLE_PLACEHOLDERS, but it's passed in backend/grants/tasks.py:163. This is fine functionally (it will work), but for documentation consistency, consider adding "user_name" to the list.

4. Breaking change documentation: The PR description correctly documents that the email template body needs to be updated after merging. Make sure to update the template before sending emails, as the old placeholders (has_approved_travel, has_approved_accommodation, travel_amount) will no longer be available.

💡 Suggestion

The has_only_internal_reimbursements property at backend/grants/models.py:299-306 could be slightly simplified:

@property
def has_only_internal_reimbursements(self):
    reimbursements = self.reimbursements.all()
    if not reimbursements:
        return False
    return all(r.category.is_internal for r in reimbursements)

But this would require a join/prefetch. The current implementation with two queries is actually more efficient if the prefetch isn't already in place.

---

Verdict: This PR is ready to merge. The implementation is solid, well-tested, and the changes are well-documented. Just be sure to follow the post-merge checklist in the PR description (migration → mark Ticket as internal → update email template).

[codecov]

Codecov Report

❌ Patch coverage is 95.65217% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 94.43%. Comparing base (fcbab6e) to head (f44d5eb).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #4541   +/-   ##
=======================================
  Coverage   94.43%   94.43%           
=======================================
  Files         352      352           
  Lines       10162    10166    +4     
  Branches      738      737    -1     
=======================================
+ Hits         9596     9600    +4     
  Misses        468      468           
  Partials       98       98           

🚀 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 final code review

[claude]

Claude finished @estyxx's task in 50s —— View job

---

Final Code Review

• Gather context and read all changed files
• Review migration file
• Review model changes
• Review admin changes
• Review task changes
• Review test changes
• Review notification model changes
• Provide final summary and recommendations

---

This PR is ready to merge. The implementation is clean, well-tested, and achieves the stated goal of simplifying grant email placeholders.

Summary of Changes

File	Change
backend/grants/migrations/0032_...	Adds is_internal boolean field to GrantReimbursementCategory
backend/grants/models.py	Adds is_internal field, total_grantee_reimbursement_amount and has_only_internal_reimbursements properties
backend/grants/admin.py	Exposes is_internal in list display and filters
backend/grants/tasks.py	Replaces old placeholders with total_amount and ticket_only
backend/grants/tests/factories.py	Adds factory traits for different category types
backend/grants/tests/test_tasks.py	Comprehensive test coverage for all scenarios
backend/notifications/models.py	Updates AVAILABLE_PLACEHOLDERS for grant_approved

✅ What Looks Good

1. Clean migration (0032_add_is_internal_to_reimbursement_category.py): Simple field addition with sensible default.

2. Well-documented model properties (models.py:282-308):

   • total_grantee_reimbursement_amount correctly excludes internal categories
   • has_only_internal_reimbursements handles edge cases properly

3. Thorough test coverage (test_tasks.py): Tests cover:

   • Ticket-only grants (lines 253-314)
   • Travel + accommodation + ticket grants (lines 174-250)
   • Travel + ticket grants (lines 317-386)
   • Reminder emails (lines 112-171)

4. Proper placeholder documentation (notifications/models.py:114-127): The AVAILABLE_PLACEHOLDERS is correctly updated with total_amount, ticket_only, and user_name.

5. Factory traits (factories.py:80-108): Clean implementation making tests more readable.

⚠️ Minor Observations (Non-blocking)

1. Unused methods (models.py:272-280): The has_approved_travel() and has_approved_accommodation() methods are no longer used in the email task but remain in the model. If they're not used elsewhere, consider removing them in a follow-up PR to avoid dead code.

2. Query efficiency note (models.py:291-300): The total_grantee_reimbursement_amount property iterates over prefetched reimbursements in Python rather than using a database aggregate. This is fine for single-grant access (like in the email task) since GrantAdmin.get_queryset already prefetches reimbursements__category (line 703). Just be mindful if using this property in new contexts without proper prefetching.

Post-Merge Checklist (from PR description)

1. ✅ Run the migration
2. ✅ Mark the Ticket category as is_internal=True in Django admin
3. ✅ Update the email template body to use {{ total_amount }} and {{ ticket_only }} instead of the removed placeholders

---

[estyxx]

@claude do the code review

[claude]

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

---

Code Review

• Read changed files and gather context
• Review model changes (models.py)
• Review task changes (tasks.py)
• Review test changes (test_tasks.py)
• Review plain_cards.py changes
• Review notification model changes
• Provide summary and feedback

---

This PR simplifies the grant approved email by replacing per-category placeholders with a single total_amount and ticket_only flag. The implementation is clean and well-tested.

✅ What Looks Good

1. Model property (models.py:283-293): The new total_grantee_reimbursement_amount property correctly:

   • Excludes TICKET category from the total
   • Uses proper Decimal arithmetic with start=Decimal(0)
   • Has clear docstring explaining its purpose

2. Task simplification (tasks.py:30-42): The email logic is much cleaner:

   • Single total_amount instead of per-category checks
   • Clean ticket_only detection: total_amount == 0 and grant.reimbursements.exists()
   • Removed the complex travel amount validation that was raising ValueError

3. Test coverage (test_tasks.py): Thorough tests covering:

   • Ticket-only grants (lines 253-314)
   • Travel + accommodation + ticket (lines 174-251)
   • Travel + ticket only (lines 317-386)
   • Reminder emails (lines 112-171)

4. plain_cards.py update: Correctly uses the new property and displays "Total reimbursement" instead of "Travel amount"

5. AVAILABLE_PLACEHOLDERS (notifications/models.py:114-127): Updated correctly with conference_name, user_name, total_amount, ticket_only, and is_reminder

⚠️ Observations

1. PR description vs implementation mismatch: The PR description mentions adding an is_internal field to GrantReimbursementCategory, but the actual implementation filters by category != TICKET hardcoded in the property. This is simpler but differs from the documented approach.

   If you intended to use is_internal, you'd need a migration to add the field. The current approach works but requires code changes if you later want to mark other categories as internal.

2. Removed test (test_tasks.py): The test test_handle_grant_approved_ticket_travel_accommodation_fails_with_no_amount was removed. This was testing that a ValueError was raised when travel amount was zero. With the new approach, a grant with only zero-amount non-ticket reimbursements would send an email with total_amount=None and ticket_only=False. Is this the intended behavior? The email template should handle this edge case.

3. Potential N+1 query: In plain_cards.py:87-93, grant.total_grantee_reimbursement_amount is called twice (once for displaying and once for the conditional). Consider storing the result in a variable:

   total = grant.total_grantee_reimbursement_amount
   # Then use `total` for both the display and conditional

   (Though this is minor since plain_cards.py already fetches a single grant)

4. Note on has_approved_* methods: I initially thought these might be dead code, but after checking, has_approved_travel() and has_approved_accommodation() are still used in:

   • visa/models.py:94-115 (for visa invitation letter logic)
   • notifications/models.py:163 (in grant_voucher_code template)

   So these methods should remain as-is. ✅

💡 Minor Suggestion

The total_allocated_amount property at models.py:275-281 was also updated with type hints and the start=Decimal(0) pattern, which is a nice consistency improvement.

---

Verdict: The implementation is solid and the simplification makes the email logic much cleaner. The main clarification needed is whether the is_internal field approach mentioned in the PR description is still planned (would require a migration) or if the current TICKET hardcoding is the intended final approach.

---

View job

[estyxx]

@claude do the code review again

[claude]

Claude finished @estyxx's task in 48s —— View job

---

Code Review

• Read and analyze all changed files
• Review model changes (models.py)
• Review task changes (tasks.py)
• Review test changes
• Review other changed files
• Provide summary and feedback

---

This PR simplifies the grant approved email by replacing per-category placeholders with a combined total_amount and a ticket_only flag. The implementation is clean and well-tested.

✅ What Looks Good

1. Model refactoring (models.py:265-304): Clean addition of:

   • has_approved() base method that reduces code duplication
   • has_approved_ticket(), has_ticket_only() helper methods
   • total_grantee_reimbursement_amount property with proper Decimal(0) initialization
   • Type hints added to all methods

2. Task simplification (tasks.py:30-42): Much cleaner email logic:

   • Single total_amount placeholder instead of per-category checks
   • Clean ticket_only detection using has_ticket_only()
   • Removed the complex travel amount validation that was raising ValueError

3. Test coverage (test_models.py and test_tasks.py): Thorough tests covering:

   • has_approved_ticket() at test_models.py:148-159
   • has_ticket_only() variations at test_models.py:190-229
   • total_grantee_reimbursement_amount at test_models.py:232-266
   • Email scenarios: ticket-only, travel+ticket, travel+accommodation+ticket

4. plain_cards.py update (plain_cards.py:72-100): Correctly uses the new property and displays "Total reimbursement" instead of "Travel amount"

5. AVAILABLE_PLACEHOLDERS (notifications/models.py:114-127): Updated correctly with total_amount, ticket_only

6. visa/models.py (visa/models.py:117): Uses the new has_approved_ticket() method

⚠️ Minor Observations (Non-blocking)

1. Removed error handling for zero travel amounts: The previous implementation raised a ValueError when travel amount was zero (tasks.py). With the new approach, a grant with only zero-amount non-ticket reimbursements would send an email with total_amount=None and ticket_only=False. Ensure the email template handles this edge case gracefully.

2. Potential N+1 query in total_grantee_reimbursement_amount (models.py:294-304): The property iterates over prefetched reimbursements in Python. This is fine for single-grant access (like in the email task). In bulk contexts, ensure prefetch_related('reimbursements__category') is used (already done in GrantAdmin.get_queryset).

3. has_approved_travel() and has_approved_accommodation() still in use: These methods are correctly retained since they're used in:

   • visa/models.py:94-102 for invitation letter logic
   • notifications/models.py:163 in grant_voucher_code template

---

Verdict: This PR is ready to merge. The implementation is solid, well-tested, and the simplification makes the email logic much cleaner. Follow the post-merge checklist in the PR description (update the email template body in Django admin).

---

View job