Flexible Grant categories #4420
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4420 +/- ##
==========================================
- Coverage 94.43% 94.42% -0.02%
==========================================
Files 352 352
Lines 10135 10134 -1
Branches 735 732 -3
==========================================
- Hits 9571 9569 -2
+ Misses 469 468 -1
- Partials 95 97 +2 🚀 New features to boost your workflow:
|
estyxx
force-pushed
the
feature/flexible-grant-categories
branch
from
c4631e4 to
2158bf3
Compare
estyxx
force-pushed
the
feature/flexible-grant-categories
branch
from
2158bf3 to
a9233db
Compare
estyxx
force-pushed
the
feature/flexible-grant-categories
branch
from
a9233db to
7c8820c
Compare
There was a problem hiding this comment.
Pull request overview
This PR refactors the grant reimbursement system from a fixed set of approved types to a flexible category-based approach. The changes enable conference organizers to define custom reimbursement categories (e.g., Travel, Ticket, Accommodation) with individual maximum amounts, replacing the previous hardcoded approved_type field.
Key changes:
- Introduced
GrantReimbursementCategoryandGrantReimbursementmodels for flexible grant management - Migrated existing grant data from the old
approved_typesystem to the new category-based system - Updated admin interfaces to display and manage reimbursements through the new models
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| backend/grants/models.py | Added new models for reimbursement categories and reimbursements; removed deprecated fields and calculation logic |
| backend/grants/admin.py | Added admin interfaces for new models and updated grant display to show reimbursement details |
| backend/reviews/admin.py | Updated grant review workflow to handle multiple reimbursement categories instead of single approved type |
| backend/reviews/templates/grants-recap.html | Changed UI from radio buttons to checkboxes for selecting multiple reimbursement categories |
| backend/grants/summary.py | Refactored financial aggregation to sum from reimbursements instead of grant amounts |
| backend/grants/migrations/0030_remove_grant_accommodation_amount_and_more.py | Migration to create new models and backfill existing data |
| backend/conferences/migrations/0055_remove_conference_grants_default_accommodation_amount_and_more.py | Migration to remove deprecated conference-level grant amount fields |
| backend/conferences/models/conference.py | Removed deprecated grant default amount fields |
| backend/conferences/admin/conference.py | Removed deprecated grant configuration fieldset from admin |
| backend/grants/tests/test_models.py | Updated tests to use new reimbursement system |
| backend/grants/tests/test_migration_backfill_grant_reimbursements.py | Added comprehensive migration tests |
| backend/reviews/tests/test_admin.py | Added tests for grant review workflow with new reimbursement system |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| """ | ||
| Aggregates financial data (total amounts) by grant status. | ||
| """ | ||
| financial_data = filtered_grants.values("pending_status").annotate( | ||
| total_amount_sum=Sum("total_amount") | ||
| ) | ||
| financial_summary = {status[0]: 0 for status in statuses} | ||
| overall_total = 0 | ||
|
|
||
| for data in financial_data: | ||
| pending_status = data["pending_status"] | ||
| total_amount = data["total_amount_sum"] or 0 | ||
| financial_summary[pending_status] += total_amount | ||
| if pending_status in self.BUDGET_STATUSES: | ||
| overall_total += total_amount | ||
| for status in statuses: | ||
| grants_for_status = filtered_grants.filter(pending_status=status[0]) | ||
| reimbursements = GrantReimbursement.objects.filter( | ||
| grant__in=grants_for_status | ||
| ) | ||
| total = reimbursements.aggregate(total=Sum("granted_amount"))["total"] or 0 | ||
| financial_summary[status[0]] = total | ||
| if status[0] in self.BUDGET_STATUSES: | ||
| overall_total += total | ||
|
|
||
| return financial_summary, overall_total |
There was a problem hiding this comment.
This unused function should be removed since it has been replaced by _aggregate_financial_data_by_status_new (which should be renamed to take its place). Having both functions could cause confusion about which one is actually being used.
|
|
||
| @admin.display(description="Status") | ||
| def current_or_pending_status(self, obj): | ||
| return obj.current_or_pending_status |
There was a problem hiding this comment.
[nitpick] Missing blank line after the method definition. There should be a blank line before the next method definition for consistency with Python style guidelines.
backend/grants/admin.py
Outdated
| return "" | ||
| @admin.display(description="Total") | ||
| def total_amount_display(self, obj): | ||
| return f"{obj.total_allocated:.2f}" |
There was a problem hiding this comment.
The property name is total_allocated_amount (line 271 in models.py) but this code references obj.total_allocated which is the annotation added in the queryset. This should reference obj.total_allocated_amount to be consistent with the model property, or the comment should clarify this relies on the queryset annotation.
| "name": "Ticket", | ||
| "description": "Conference ticket", | ||
| "max_amount": conference.grants_default_ticket_amount | ||
| or Decimal("0.00"), |
There was a problem hiding this comment.
Missing import for Decimal. The migration uses Decimal on lines 20, 31, and 42 but doesn't import it from the decimal module.
| }, | ||
| ), | ||
| # Backfill existing grants | ||
| migrations.RunPython( |
There was a problem hiding this comment.
The migrate_grants function should be called before ensure_categories_exist to ensure categories exist before attempting to create reimbursements. Add a RunPython operation for ensure_categories_exist before this one.
| migrations.RunPython( | |
| migrations.RunPython( | |
| code=ensure_categories_exist, | |
| ), | |
| migrations.RunPython( |
backend/reviews/admin.py
Outdated
| # TODO: move the amount calculation in a separate function maybe? | ||
| grant.save(update_fields=["pending_status", "approved_type"]) | ||
| grant.save(update_fields=["pending_status",]) | ||
| approved_reimbursement_categories = approved_reimbursement_categories_decisions.get(grant.id, "") |
There was a problem hiding this comment.
Default value should be an empty list [] instead of an empty string "" to match the expected type. This is used in a for loop on line 318, and iterating over a string would produce unexpected behavior.
| approved_reimbursement_categories = approved_reimbursement_categories_decisions.get(grant.id, "") | |
| approved_reimbursement_categories = approved_reimbursement_categories_decisions.get(grant.id, []) |
|
@claude do a code review |
|
Claude finished @estyxx's task —— View job Code Review Complete ✅
SummaryThis is a well-executed refactoring that successfully transforms the grant reimbursement system from a rigid, hardcoded structure to a flexible, category-based approach. The implementation is mostly solid with comprehensive test coverage. I've identified several critical issues that need addressing before merge, along with some suggestions for improvement. Critical Issues 🔴1. Missing
|
estyxx
force-pushed
the
feature/flexible-grant-categories
branch
from
7c8820c to
65f917e
Compare
estyxx
force-pushed
the
feature/flexible-grant-categories
branch
2 times, most recently
from
3e1c0e9 to
c8f619e
Compare
Remove hardcoded default grant amounts for ticket, accommodation, and travel from `Conference` in favor of using `GrantReimbursementCategory`. Update all relevant admin forms, models, and templates to reference flexible categories instead of fixed fields. - Remove legacy fields: `grants_default_ticket_amount`, `grants_default_accommodation_amount`, `grants_default_travel_from_italy_amount`, and `grants_default_travel_from_europe_amount` from `Conference` - Update `Grant` and `GrantReimbursement` logic to work exclusively with `GrantReimbursementCategory` - Refactor grant review admin and summary logic to support multiple, configurable reimbursement categories per grant - Migrate existing grants to new reimbursement category scheme - Add and update tests and migrations to cover flexible grant categories This change allows flexible reimbursement types (and amounts) to be configured per conference, supports granular grant allocation, and paves the way for internationalization and more complex business rules.
estyxx
force-pushed
the
feature/flexible-grant-categories
branch
2 times, most recently
from
0932ae1 to
0a1ec12
Compare
|
@claude do a code review |
|
Claude finished @estyxx's task —— View job Code Review Complete ✅
SummaryThis is a well-executed refactoring that successfully transforms the grant reimbursement system from a rigid, hardcoded structure to a flexible, category-based approach. The implementation is mostly solid with comprehensive test coverage and a clear migration path. The code demonstrates good practices including:
However, there are several critical issues that must be addressed before merging, along with some high-priority concerns that could cause bugs or confusion. Critical Issues 🔴1. Migration order is correct ✓Upon careful review, the migration operations are in the correct order in
The operations execute sequentially, so categories are created before grants are migrated. ✓ 2. Missing
|
| total_amount=None, | ||
| grant = GrantFactory(status=Grant.Status.approved) | ||
| # Create reimbursement with 0 amount so total_allocated_amount is 0 | ||
| GrantReimbursementFactory( |
There was a problem hiding this comment.
it actually use it! category__ticket=True,
| mock_send_approved_email.assert_not_called() | ||
|
|
||
|
|
||
| def test_send_reply_emails_approved_missing_amount(rf, mocker, admin_user): |
There was a problem hiding this comment.
Not sure this test still make sense, if I have no reimbursments I get the error, but if the amounts are 0 the property on the grants still will sum to 0... is even thown?
There was a problem hiding this comment.
no it tests that we didn't accidentally set all them to 0...
| return conference | ||
|
|
||
|
|
||
| def _create_reimbursements_for_grant( |
There was a problem hiding this comment.
These tests were created before putting this logic inside the migration, these doesn't make any sense unless we import the utilities methods from the migration itself and we test those. but will be impossible to create the Grants' object as before the migration... so nope
| TRAVEL = "travel", _("Travel") | ||
| TICKET = "ticket", _("Ticket") | ||
| ACCOMMODATION = "accommodation", _("Accommodation") |
There was a problem hiding this comment.
This year pycon italia we will have a budget for travel+accommodation together, this mean we need to add a new category to this enum TRAVEL_ACCOMMODATION?
Smells
There was a problem hiding this comment.
Not sure we_need this 🤔
we can just match the name of the Category...
There was a problem hiding this comment.
Maybe we need: when we do the calculation in the review system we need to know which is which, the travel is calculated location based, while the accomodation is fixed, so in the setup we will have separate categories, but we need to be sure that for the end-user email those are sum together (no ticket category)
Revised several Italian grant-related strings in `index.ts` to use more gender neutral language and improve consistency in tone. This helps ensure the form is welcoming and inclusive to all users, and maintains a coherent voice across the application.
estyxx
force-pushed
the
feature/flexible-grant-categories
branch
from
0a1ec12 to
ee5c98f
Compare
|
@claude do the final code review |
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
What
This PR refactors the grants and reimbursements system, moving from hardcoded monetary fields in the
Conferencemode to a category-based reimbursement model. It adds new models forGrantReimbursementCategoryandGrantReimbursement, migrates existing data, and updates the Django admin interface to support the new structure.Database and Data Model Refactoring:
GrantReimbursementCategory(defines reimbursement types per conference) andGrantReimbursement(records actual granted amounts per category per grant)..accommodation_amount,ticket_amount,travel_amount,approved_type,total_amount) from theGrantmodel and related fields from theConferencemodel.See the proposal: https://www.notion.so/pythonit/Grants-3-0-359642da7c094f3d9445fe0e05b7937c
ToDo
Re-introduce the automatic assignment of the grant travel amount based on the country type.