From f253f128d223555f26f237bdaa40d8fe739afc97 Mon Sep 17 00:00:00 2001 From: Ester Beltrami Date: Sat, 14 Feb 2026 12:30:38 +0000 Subject: [PATCH] Fix grant summary to handle NULL pending_status Use Coalesce("pending_status", "status") to fall back to status when pending_status is NULL. Refactor financial and reimbursement category aggregations to use database-level annotations consistently, replacing the N+1 query loop with Case/When conditional aggregation. Add tests for NULL pending_status, mixed statuses, and financial data edge cases. Fixes PYCON-BACKEND-6H7 --- backend/grants/summary.py | 127 +++++++++++++++++---------- backend/grants/tests/test_summary.py | 116 +++++++++++++++++++++++- 2 files changed, 196 insertions(+), 47 deletions(-) diff --git a/backend/grants/summary.py b/backend/grants/summary.py index 781d6289e8..18fed9ad33 100644 --- a/backend/grants/summary.py +++ b/backend/grants/summary.py @@ -1,6 +1,7 @@ from collections import defaultdict -from django.db.models import Count, Exists, OuterRef, Sum +from django.db.models import Case, Count, DecimalField, Exists, OuterRef, Sum, When +from django.db.models.functions import Coalesce from conferences.models.conference import Conference from countries import countries @@ -26,10 +27,12 @@ def calculate(self, conference_id): """ statuses = Grant.Status.choices conference = Conference.objects.get(id=conference_id) - filtered_grants = Grant.objects.for_conference(conference) + filtered_grants = Grant.objects.for_conference(conference).annotate( + current_or_pending_status=Coalesce("pending_status", "status") + ) grants_by_country = filtered_grants.values( - "departure_country", "pending_status" + "departure_country", "current_or_pending_status" ).annotate(total=Count("id")) ( @@ -99,6 +102,7 @@ def _aggregate_data_by_country(self, grants_by_country, statuses): totals_per_continent = {} for data in grants_by_country: + current_or_pending_status: str = data["current_or_pending_status"] country = countries.get(code=data["departure_country"]) continent = country.continent.name if country else "Unknown" country_name = f"{country.name} {country.emoji}" if country else "Unknown" @@ -108,13 +112,13 @@ def _aggregate_data_by_country(self, grants_by_country, statuses): if key not in summary: summary[key] = {status[0]: 0 for status in statuses} - summary[key][data["pending_status"]] += data["total"] - status_totals[data["pending_status"]] += data["total"] + summary[key][current_or_pending_status] += data["total"] + status_totals[current_or_pending_status] += data["total"] # Update continent totals if continent not in totals_per_continent: totals_per_continent[continent] = {status[0]: 0 for status in statuses} - totals_per_continent[continent][data["pending_status"]] += data["total"] + totals_per_continent[continent][current_or_pending_status] += data["total"] return summary, status_totals, totals_per_continent @@ -123,7 +127,7 @@ def _aggregate_data_by_country_type(self, filtered_grants, statuses): Aggregates grant data by country type and status. """ country_type_data = filtered_grants.values( - "country_type", "pending_status" + "country_type", "current_or_pending_status" ).annotate(total=Count("id")) country_type_summary = defaultdict( lambda: {status[0]: 0 for status in statuses} @@ -131,9 +135,9 @@ def _aggregate_data_by_country_type(self, filtered_grants, statuses): for data in country_type_data: country_type = data["country_type"] - pending_status = data["pending_status"] + current_or_pending_status: str = data["current_or_pending_status"] total = data["total"] - country_type_summary[country_type][pending_status] += total + country_type_summary[country_type][current_or_pending_status] += total return dict(country_type_summary) @@ -141,35 +145,50 @@ def _aggregate_data_by_gender(self, filtered_grants, statuses): """ Aggregates grant data by gender and status. """ - gender_data = filtered_grants.values("gender", "pending_status").annotate( - total=Count("id") - ) + gender_data = filtered_grants.values( + "gender", "current_or_pending_status" + ).annotate(total=Count("id")) gender_summary = defaultdict(lambda: {status[0]: 0 for status in statuses}) for data in gender_data: gender = data["gender"] if data["gender"] else "" - pending_status = data["pending_status"] + current_or_pending_status: str = data["current_or_pending_status"] total = data["total"] - gender_summary[gender][pending_status] += total + gender_summary[gender][current_or_pending_status] += total return dict(gender_summary) def _aggregate_financial_data_by_status(self, filtered_grants, statuses): """ - Aggregates financial data (total amounts) by grant status. + Aggregates financial data (total amounts) by grant status + using conditional aggregation in a single query. """ - financial_summary = {status[0]: 0 for status in statuses} - overall_total = 0 + reimbursements = GrantReimbursement.objects.filter( + grant__in=filtered_grants + ).annotate( + current_or_pending_status=Coalesce("grant__pending_status", "grant__status") + ) - for status in statuses: - grants_for_status = filtered_grants.filter(pending_status=status[0]) - reimbursements = GrantReimbursement.objects.filter( - grant__in=grants_for_status + aggregations: dict[str, Sum] = { + status_value: Sum( + Case( + When(current_or_pending_status=status_value, then="granted_amount"), + default=0, + output_field=DecimalField(), + ) ) - 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 + for status_value, _ in statuses + } + result = reimbursements.aggregate(**aggregations) + + financial_summary: dict[str, int] = { + status_value: int(result[status_value] or 0) for status_value, _ in statuses + } + overall_total: int = sum( + amount + for status_value, amount in financial_summary.items() + if status_value in self.BUDGET_STATUSES + ) return financial_summary, overall_total @@ -177,12 +196,24 @@ def _aggregate_data_by_reimbursement_category(self, filtered_grants, statuses): """ Aggregates grant data by reimbursement category and status. """ - category_summary = defaultdict(lambda: {status[0]: 0 for status in statuses}) - reimbursements = GrantReimbursement.objects.filter(grant__in=filtered_grants) - for r in reimbursements: - category = r.category.category - status = r.grant.pending_status - category_summary[category][status] += 1 + category_data = ( + GrantReimbursement.objects.filter(grant__in=filtered_grants) + .annotate( + current_or_pending_status=Coalesce( + "grant__pending_status", "grant__status" + ) + ) + .values("category__category", "current_or_pending_status") + .annotate(total=Count("id")) + ) + category_summary: dict[str, dict[str, int]] = defaultdict( + lambda: {status[0]: 0 for status in statuses} + ) + for data in category_data: + category: str = data["category__category"] + current_or_pending_status: str = data["current_or_pending_status"] + total: int = data["total"] + category_summary[category][current_or_pending_status] += total return dict(category_summary) def _aggregate_data_by_grant_type(self, filtered_grants, statuses): @@ -190,16 +221,16 @@ def _aggregate_data_by_grant_type(self, filtered_grants, statuses): Aggregates grant data by grant_type and status. """ grant_type_data = filtered_grants.values( - "grant_type", "pending_status" + "grant_type", "current_or_pending_status" ).annotate(total=Count("id")) grant_type_summary = defaultdict(lambda: {status[0]: 0 for status in statuses}) for data in grant_type_data: grant_types = data["grant_type"] - pending_status = data["pending_status"] + current_or_pending_status: str = data["current_or_pending_status"] total = data["total"] for grant_type in grant_types: - grant_type_summary[grant_type][pending_status] += total + grant_type_summary[grant_type][current_or_pending_status] += total return dict(grant_type_summary) @@ -224,13 +255,13 @@ def _aggregate_data_by_speaker_status(self, filtered_grants, statuses): proposed_speaker_data = ( filtered_grants.filter(is_proposed_speaker=True) - .values("pending_status") + .values("current_or_pending_status") .annotate(total=Count("id")) ) confirmed_speaker_data = ( filtered_grants.filter(is_confirmed_speaker=True) - .values("pending_status") + .values("current_or_pending_status") .annotate(total=Count("id")) ) @@ -239,14 +270,18 @@ def _aggregate_data_by_speaker_status(self, filtered_grants, statuses): ) for data in proposed_speaker_data: - pending_status = data["pending_status"] + current_or_pending_status: str = data["current_or_pending_status"] total = data["total"] - speaker_status_summary["proposed_speaker"][pending_status] += total + speaker_status_summary["proposed_speaker"][current_or_pending_status] += ( + total + ) for data in confirmed_speaker_data: - pending_status = data["pending_status"] + current_or_pending_status: str = data["current_or_pending_status"] total = data["total"] - speaker_status_summary["confirmed_speaker"][pending_status] += total + speaker_status_summary["confirmed_speaker"][current_or_pending_status] += ( + total + ) return dict(speaker_status_summary) @@ -263,13 +298,13 @@ def _aggregate_data_by_requested_needs_summary(self, filtered_grants, statuses): for field in requested_needs_summary.keys(): field_data = ( filtered_grants.filter(**{field: True}) - .values("pending_status") + .values("current_or_pending_status") .annotate(total=Count("id")) ) for data in field_data: - pending_status = data["pending_status"] + current_or_pending_status: str = data["current_or_pending_status"] total = data["total"] - requested_needs_summary[field][pending_status] += total + requested_needs_summary[field][current_or_pending_status] += total return requested_needs_summary @@ -278,14 +313,14 @@ def _aggregate_data_by_occupation(self, filtered_grants, statuses): Aggregates grant data by occupation and status. """ occupation_data = filtered_grants.values( - "occupation", "pending_status" + "occupation", "current_or_pending_status" ).annotate(total=Count("id")) occupation_summary = defaultdict(lambda: {status[0]: 0 for status in statuses}) for data in occupation_data: occupation = data["occupation"] - pending_status = data["pending_status"] + current_or_pending_status: str = data["current_or_pending_status"] total = data["total"] - occupation_summary[occupation][pending_status] += total + occupation_summary[occupation][current_or_pending_status] += total return dict(occupation_summary) diff --git a/backend/grants/tests/test_summary.py b/backend/grants/tests/test_summary.py index eaee23d77e..3ed1523997 100644 --- a/backend/grants/tests/test_summary.py +++ b/backend/grants/tests/test_summary.py @@ -1,5 +1,9 @@ import pytest -from .factories import GrantFactory +from .factories import ( + GrantFactory, + GrantReimbursementCategoryFactory, + GrantReimbursementFactory, +) from grants.summary import GrantSummary from conferences.tests.factories import ConferenceFactory @@ -73,3 +77,113 @@ def test_grant_summary_calculation_by_status(grants_set): assert summary["status_totals"]["rejected"] == 3 assert summary["status_totals"]["waiting_list"] == 7 assert summary["total_grants"] == 15 + + +@pytest.mark.django_db +def test_grant_summary_with_null_pending_status(): + conference = ConferenceFactory() + + GrantFactory.create_batch( + 3, + conference=conference, + status="approved", + pending_status=None, + departure_country="IT", + gender="female", + ) + GrantFactory.create_batch( + 2, + conference=conference, + status="rejected", + pending_status=None, + departure_country="FR", + gender="male", + ) + + summary = GrantSummary().calculate(conference_id=conference.id) + + # status_totals should reflect the fallback status values + assert summary["status_totals"]["approved"] == 3 + assert summary["status_totals"]["rejected"] == 2 + assert summary["total_grants"] == 5 + + # country stats should also work + assert summary["country_stats"][("Europe", "Italy 🇮🇹", "IT")]["approved"] == 3 + assert summary["country_stats"][("Europe", "France 🇫🇷", "FR")]["rejected"] == 2 + + # gender stats should use the fallback too + assert summary["gender_stats"]["female"]["approved"] == 3 + assert summary["gender_stats"]["male"]["rejected"] == 2 + + +@pytest.mark.django_db +def test_grant_summary_with_mixed_pending_status(): + conference = ConferenceFactory() + + # Grant with pending_status set (pending_status takes precedence) + GrantFactory.create_batch( + 2, + conference=conference, + status="pending", + pending_status="approved", + departure_country="IT", + ) + # Grant with pending_status=None (falls back to status) + GrantFactory.create_batch( + 3, + conference=conference, + status="rejected", + pending_status=None, + departure_country="IT", + ) + + summary = GrantSummary().calculate(conference_id=conference.id) + + assert summary["status_totals"]["approved"] == 2 + assert summary["status_totals"]["rejected"] == 3 + assert summary["total_grants"] == 5 + + +@pytest.mark.django_db +def test_grant_summary_financial_data_with_null_pending_status(): + conference = ConferenceFactory() + + ticket_category = GrantReimbursementCategoryFactory( + conference=conference, + ticket=True, + ) + + grant_with_status = GrantFactory( + conference=conference, + status="approved", + pending_status=None, + departure_country="IT", + ) + GrantReimbursementFactory( + grant=grant_with_status, + category=ticket_category, + granted_amount=100, + ) + + grant_with_pending = GrantFactory( + conference=conference, + status="pending", + pending_status="confirmed", + departure_country="IT", + ) + GrantReimbursementFactory( + grant=grant_with_pending, + category=ticket_category, + granted_amount=200, + ) + + summary = GrantSummary().calculate(conference_id=conference.id) + + assert summary["financial_summary"]["approved"] == 100 + assert summary["financial_summary"]["confirmed"] == 200 + # approved + confirmed are both BUDGET_STATUSES + assert summary["total_amount"] == 300 + + # reimbursement category summary should also work + assert summary["reimbursement_category_summary"]["ticket"]["approved"] == 1 + assert summary["reimbursement_category_summary"]["ticket"]["confirmed"] == 1