Split platform signups, and rework the club table into health
The platform signups chart gets the same stacked new/returning split as the club one. "First season" is keyed on (club, member), never the member alone: the same person can be new at one club while renewing at another, and collapsing that would file their second club's very first signup as a renewal. The dashboard's club table stops reporting vanity counts. A member total says nothing you can act on; "no coach", "nothing scheduled", "€ owed" and "no admins" each name something somebody has to go and fix. Columns are now active members, unpaid members, money owed, teams (flagging those nobody can pick a squad for), upcoming events, and admins -- with No season / Dormant badges on the club itself. Every column is annotated in ONE query, each aggregate in its own subquery. That is not stylistic: aggregates spanning different joins multiply each other's rows, so a Sum of orders sitting next to a Count of memberships returns the club's debt multiplied by its membership count. A test pins €100 against three memberships and would catch it coming back as €300. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
@@ -11,8 +11,8 @@ from decimal import Decimal
|
||||
|
||||
from allauth.mfa.models import Authenticator
|
||||
from django.contrib.auth import get_user_model
|
||||
from django.db.models import Count, Q, Sum
|
||||
from django.db.models.functions import TruncMonth
|
||||
from django.db.models import Count, DecimalField, Exists, F, IntegerField, OuterRef, Q, Subquery, Sum, Value
|
||||
from django.db.models.functions import Coalesce, TruncMonth
|
||||
from django.utils import timezone
|
||||
from waffle import get_waffle_flag_model
|
||||
|
||||
@@ -44,6 +44,46 @@ def clubs_with_totals(queryset=None):
|
||||
)
|
||||
|
||||
|
||||
def _subquery(queryset, expression, output_field):
|
||||
"""One aggregate, in its own subquery.
|
||||
|
||||
Deliberately not a pile of annotate(Count(...), Sum(...)) on one queryset: aggregates
|
||||
that span *different* joins multiply each other's rows, so a club's outstanding total
|
||||
would come back doubled for every membership it happens to have. Subqueries each stand
|
||||
alone, so nothing can inflate anything else.
|
||||
"""
|
||||
return Coalesce(Subquery(queryset.filter(club=OuterRef("pk")).values("club").annotate(value=expression).values("value"), output_field=output_field), Value(0), output_field=output_field)
|
||||
|
||||
|
||||
def clubs_with_health(queryset=None, today=None, now=None):
|
||||
"""Clubs annotated with the health of each — for the dashboard table, in one query."""
|
||||
today = today or timezone.localdate()
|
||||
now = now or timezone.now()
|
||||
clubs = Club.objects.active() if queryset is None else queryset
|
||||
|
||||
in_season = Q(season__start_date__lte=today, season__end_date__gte=today)
|
||||
managed_this_season = Q(
|
||||
staff_assignments__season__start_date__lte=today,
|
||||
staff_assignments__season__end_date__gte=today,
|
||||
staff_assignments__position__management_position=True,
|
||||
)
|
||||
|
||||
return (
|
||||
clubs.annotate(
|
||||
has_season=Exists(Season.objects.filter(club=OuterRef("pk"), start_date__lte=today, end_date__gte=today)),
|
||||
active_members=_subquery(ClubMembership.objects.filter(in_season, status=ClubMembership.StatusChoices.ACTIVE), Count("pk"), IntegerField()),
|
||||
unpaid_members=_subquery(ClubMembership.objects.filter(in_season, fee_status=ClubMembership.FeeStatus.UNPAID), Count("pk"), IntegerField()),
|
||||
outstanding=_subquery(Order.objects.filter(status__in=OWED_STATUSES), Sum("total"), DecimalField(max_digits=10, decimal_places=2)),
|
||||
upcoming_events=_subquery(Event.objects.filter(start__gte=now, start__lte=now + timedelta(days=DORMANT_DAYS)), Count("pk"), IntegerField()),
|
||||
team_count=_subquery(Team.objects.all(), Count("pk"), IntegerField()),
|
||||
teams_managed=_subquery(Team.objects.filter(managed_this_season), Count("pk", distinct=True), IntegerField()),
|
||||
admin_count=_subquery(ClubRole.objects.filter(role=ClubRole.Roles.ADMIN), Count("pk"), IntegerField()),
|
||||
)
|
||||
.annotate(teams_without_coach=F("team_count") - F("teams_managed"))
|
||||
.order_by("name")
|
||||
)
|
||||
|
||||
|
||||
def platform_totals():
|
||||
return {
|
||||
"clubs": Club.objects.active().count(),
|
||||
@@ -142,7 +182,7 @@ def _monthly(queryset, field, value, months=MONTHS_OF_HISTORY):
|
||||
|
||||
def platform_charts():
|
||||
return {
|
||||
"signups": _monthly(ClubMembership.objects.filter(signed_up_at__isnull=False), "signed_up_at", Count("id")),
|
||||
"signups": signup_split(),
|
||||
"revenue": _monthly(Order.objects.filter(status__in=PAID_STATUSES), "created", Sum("total")),
|
||||
}
|
||||
|
||||
@@ -197,23 +237,29 @@ def new_members(club, season):
|
||||
return Member.objects.filter(member_of__club=club, member_of__season=season).exclude(pk__in=seen_before).distinct()
|
||||
|
||||
|
||||
def signup_split(club, months=MONTHS_OF_HISTORY):
|
||||
"""Signups per month, split into first-timers and returners.
|
||||
def signup_split(club=None, months=MONTHS_OF_HISTORY):
|
||||
"""Signups per month, split into first-timers and returners. ``club=None`` is platform-wide.
|
||||
|
||||
Which season a signup belongs to decides the split, so the member's earliest season at
|
||||
this club is looked up once for everyone rather than per row — the same question asked
|
||||
inside a loop is a query per membership.
|
||||
"First" is keyed on (club, member), never on the member alone — the same person can be
|
||||
new at one club while renewing at another, and collapsing that would mark their second
|
||||
club's very first signup as a renewal.
|
||||
|
||||
Each member's earliest season is resolved once up front rather than per row: the same
|
||||
question asked inside a loop is one query per membership.
|
||||
"""
|
||||
start = (timezone.now() - timedelta(days=30 * months)).replace(day=1, hour=0, minute=0, second=0, microsecond=0)
|
||||
|
||||
memberships = ClubMembership.objects.all() if club is None else ClubMembership.objects.filter(club=club)
|
||||
|
||||
first_season = {}
|
||||
for member_id, season_start in ClubMembership.objects.filter(club=club).values_list("member_id", "season__start_date"):
|
||||
if member_id not in first_season or season_start < first_season[member_id]:
|
||||
first_season[member_id] = season_start
|
||||
for club_id, member_id, season_start in memberships.values_list("club_id", "member_id", "season__start_date"):
|
||||
key = (club_id, member_id)
|
||||
if key not in first_season or season_start < first_season[key]:
|
||||
first_season[key] = season_start
|
||||
|
||||
counts = defaultdict(lambda: {"new": 0, "returning": 0})
|
||||
for member_id, season_start, signed_up_at in ClubMembership.objects.filter(club=club, signed_up_at__isnull=False, signed_up_at__gte=start).values_list("member_id", "season__start_date", "signed_up_at"):
|
||||
kind = "new" if season_start == first_season[member_id] else "returning"
|
||||
for club_id, member_id, season_start, signed_up_at in memberships.filter(signed_up_at__isnull=False, signed_up_at__gte=start).values_list("club_id", "member_id", "season__start_date", "signed_up_at"):
|
||||
kind = "new" if season_start == first_season[(club_id, member_id)] else "returning"
|
||||
counts[signed_up_at.strftime("%Y-%m")][kind] += 1
|
||||
|
||||
series, cursor = [], start
|
||||
|
||||
@@ -50,6 +50,7 @@
|
||||
<div class="card bg-base-100 shadow">
|
||||
<div class="card-body">
|
||||
<h2 class="card-title text-base">{% lucide "user-plus" size=18 %} Signups per month</h2>
|
||||
<p class="text-sm opacity-70">New members against returning ones, across every club.</p>
|
||||
<div class="h-56">
|
||||
<canvas id="signups-chart"></canvas>
|
||||
</div>
|
||||
@@ -142,15 +143,22 @@
|
||||
<div class="card bg-base-100 shadow">
|
||||
<div class="card-body">
|
||||
<h2 class="card-title">Clubs</h2>
|
||||
{% comment %}
|
||||
Health, not vanity: a club's member count says nothing you can act on, while
|
||||
"no coach", "nothing scheduled" and "€ owed" each name a thing somebody has to
|
||||
go and fix. Every column here is annotated in the same single query.
|
||||
{% endcomment %}
|
||||
<div class="overflow-x-auto">
|
||||
<table class="table">
|
||||
<thead>
|
||||
<tr>
|
||||
<th>Club</th>
|
||||
<th>Members</th>
|
||||
<th>Teams</th>
|
||||
<th>Events</th>
|
||||
<th>Admins</th>
|
||||
<th class="text-right">Members</th>
|
||||
<th class="text-right">Unpaid</th>
|
||||
<th class="text-right">Owed</th>
|
||||
<th class="text-right">Teams</th>
|
||||
<th class="text-right">Upcoming</th>
|
||||
<th class="text-right">Admins</th>
|
||||
</tr>
|
||||
</thead>
|
||||
<tbody>
|
||||
@@ -158,16 +166,27 @@
|
||||
<tr>
|
||||
<td>
|
||||
<a class="link link-hover font-medium" href="{% url 'controlpanel:club_detail' club.pk %}">{{ club.name }}</a>
|
||||
<div class="text-xs opacity-60">{{ club.slug }}</div>
|
||||
<div class="mt-1 flex flex-wrap items-center gap-1">
|
||||
<span class="text-xs opacity-60">{{ club.slug }}</span>
|
||||
{% if not club.has_season %}<span class="badge badge-warning badge-xs gap-1">{% lucide "calendar-x" size=10 %} No season</span>{% endif %}
|
||||
{% if not club.upcoming_events %}<span class="badge badge-ghost badge-xs gap-1">{% lucide "moon-star" size=10 %} Dormant</span>{% endif %}
|
||||
</div>
|
||||
</td>
|
||||
<td>{{ club.member_count }}</td>
|
||||
<td>{{ club.team_count }}</td>
|
||||
<td>{{ club.event_count }}</td>
|
||||
<td>{{ club.admin_count }}</td>
|
||||
<td class="text-right tabular-nums">{{ club.active_members }}</td>
|
||||
<td class="text-right tabular-nums {% if club.unpaid_members %}text-warning{% endif %}">{{ club.unpaid_members }}</td>
|
||||
<td class="text-right tabular-nums {% if club.outstanding %}font-semibold text-error{% endif %}">€{{ club.outstanding|floatformat:2 }}</td>
|
||||
<td class="text-right tabular-nums">
|
||||
{{ club.team_count }}
|
||||
{% if club.teams_without_coach %}
|
||||
<span class="badge badge-error badge-xs ml-1" title="Teams with nobody able to pick the squad">{{ club.teams_without_coach }} no coach</span>
|
||||
{% endif %}
|
||||
</td>
|
||||
<td class="text-right tabular-nums">{{ club.upcoming_events }}</td>
|
||||
<td class="text-right tabular-nums {% if not club.admin_count %}text-error{% endif %}">{{ club.admin_count }}</td>
|
||||
</tr>
|
||||
{% empty %}
|
||||
<tr>
|
||||
<td colspan="5" class="text-center opacity-60">No clubs yet.</td>
|
||||
<td colspan="7" class="text-center opacity-60">No clubs yet.</td>
|
||||
</tr>
|
||||
{% endfor %}
|
||||
</tbody>
|
||||
@@ -227,10 +246,30 @@
|
||||
},
|
||||
});
|
||||
|
||||
return [
|
||||
build("signups-chart", "Signups", data.signups, css("--color-primary", "#4f46e5"), "bar", false),
|
||||
build("revenue-chart", "Revenue", data.revenue, css("--color-accent", "#0ea5e9"), "line", true),
|
||||
];
|
||||
// Stacked, so the bar height stays "signups this month" while the split shows
|
||||
// where they came from. New is per club: joining a second club is a new
|
||||
// membership there, even for someone who has been on the platform for years.
|
||||
const signups = new Chart(document.getElementById("signups-chart"), {
|
||||
type: "bar",
|
||||
data: {
|
||||
labels: data.signups.map((point) => point.month),
|
||||
datasets: [
|
||||
{ label: "New", data: data.signups.map((point) => point.new), backgroundColor: css("--color-primary", "#4f46e5") },
|
||||
{ label: "Returning", data: data.signups.map((point) => point.returning), backgroundColor: css("--color-accent", "#0ea5e9") },
|
||||
],
|
||||
},
|
||||
options: {
|
||||
responsive: true,
|
||||
maintainAspectRatio: false,
|
||||
plugins: { legend: { position: "bottom", labels: { color: ink } } },
|
||||
scales: {
|
||||
x: { stacked: true, ticks: { color: ink }, grid: { color: grid } },
|
||||
y: { stacked: true, beginAtZero: true, ticks: { color: ink, precision: 0 }, grid: { color: grid } },
|
||||
},
|
||||
},
|
||||
});
|
||||
|
||||
return [signups, build("revenue-chart", "Revenue", data.revenue, css("--color-accent", "#0ea5e9"), "line", true)];
|
||||
};
|
||||
|
||||
let charts = render();
|
||||
|
||||
@@ -25,6 +25,7 @@ from .services.statistics import (
|
||||
attendance_rates,
|
||||
club_attention,
|
||||
club_statistics,
|
||||
clubs_with_health,
|
||||
clubs_with_totals,
|
||||
clubs_without_a_season,
|
||||
dormant_clubs,
|
||||
@@ -708,14 +709,24 @@ class PlatformChartTests(TestCase):
|
||||
series = platform_charts()["signups"]
|
||||
|
||||
self.assertEqual(len(series), 13)
|
||||
self.assertTrue(all(point["value"] == 0 for point in series))
|
||||
self.assertTrue(all(point["new"] == 0 and point["returning"] == 0 for point in series))
|
||||
|
||||
def test_signups_land_in_the_month_they_happened(self):
|
||||
ClubMembership.objects.create(club=self.club, season=self.season, member=self.member, signed_up_at=timezone.localdate())
|
||||
|
||||
series = platform_charts()["signups"]
|
||||
|
||||
self.assertEqual(series[-1]["value"], 1)
|
||||
self.assertEqual(series[-1]["new"], 1)
|
||||
|
||||
def test_new_is_per_club_not_per_platform(self):
|
||||
# A veteran of one club joining a second is new *there*. Keying "first season" on the
|
||||
# member alone would file their very first signup at the new club as a renewal.
|
||||
other = Club.objects.create(name="Feyenoord")
|
||||
old_season = Season.objects.create(club=other, start_date=timezone.localdate() - datetime.timedelta(days=400), end_date=timezone.localdate() - datetime.timedelta(days=40))
|
||||
ClubMembership.objects.create(club=other, season=old_season, member=self.member, signed_up_at=timezone.localdate() - datetime.timedelta(days=300))
|
||||
ClubMembership.objects.create(club=self.club, season=self.season, member=self.member, signed_up_at=timezone.localdate())
|
||||
|
||||
self.assertEqual(platform_charts()["signups"][-1]["new"], 1)
|
||||
|
||||
def test_only_paid_orders_count_as_revenue(self):
|
||||
Order.objects.create(club=self.club, purchaser=self.member, total=Decimal("50.00"), status=Order.OrderStatus.PAID)
|
||||
@@ -930,3 +941,70 @@ class NewMemberTests(TestCase):
|
||||
|
||||
self.assertEqual(sum(month["new"] for month in series), 1)
|
||||
self.assertEqual(sum(month["returning"] for month in series), 0)
|
||||
|
||||
|
||||
class ClubHealthTableTests(TestCase):
|
||||
def setUp(self):
|
||||
self.today = timezone.localdate()
|
||||
self.club = Club.objects.create(name="Ajax United")
|
||||
self.season = Season.objects.create(club=self.club, start_date=self.today - datetime.timedelta(days=30), end_date=self.today + datetime.timedelta(days=300))
|
||||
self.member = Member.objects.create(first_name="Ada", last_name="Lovelace")
|
||||
|
||||
def health(self):
|
||||
return clubs_with_health().get(pk=self.club.pk)
|
||||
|
||||
def test_the_whole_table_costs_one_query(self):
|
||||
Club.objects.create(name="Feyenoord")
|
||||
|
||||
with self.assertNumQueries(1):
|
||||
[(club.active_members, club.outstanding, club.teams_without_coach) for club in clubs_with_health()]
|
||||
|
||||
def test_money_is_not_inflated_by_other_joins(self):
|
||||
# The reason each aggregate is its own subquery: a Sum and a Count spanning different
|
||||
# joins multiply each other's rows, and the club's debt comes back doubled for every
|
||||
# membership it happens to have.
|
||||
for name in ("Bob", "Carol", "Dave"):
|
||||
member = Member.objects.create(first_name=name, last_name="Bobson")
|
||||
ClubMembership.objects.create(club=self.club, season=self.season, member=member, status=ClubMembership.StatusChoices.ACTIVE)
|
||||
Order.objects.create(club=self.club, purchaser=self.member, total=Decimal("100.00"), status=Order.OrderStatus.PENDING)
|
||||
|
||||
health = self.health()
|
||||
|
||||
self.assertEqual(health.outstanding, Decimal("100.00")) # not 300.00
|
||||
self.assertEqual(health.active_members, 3)
|
||||
|
||||
def test_a_club_reports_its_missing_coaches(self):
|
||||
Team.objects.create(club=self.club, name="U15")
|
||||
managed = Team.objects.create(club=self.club, name="U17")
|
||||
coach = Position.objects.create(club=self.club, name="Coach", staff_position=True, management_position=True)
|
||||
StaffAssignment.objects.create(team=managed, member=self.member, season=self.season, position=coach)
|
||||
|
||||
health = self.health()
|
||||
|
||||
self.assertEqual(health.team_count, 2)
|
||||
self.assertEqual(health.teams_without_coach, 1)
|
||||
|
||||
def test_a_club_with_no_season_and_no_events_is_marked(self):
|
||||
Season.objects.all().delete()
|
||||
|
||||
health = self.health()
|
||||
|
||||
self.assertFalse(health.has_season)
|
||||
self.assertEqual(health.upcoming_events, 0)
|
||||
|
||||
def test_upcoming_events_only_count_the_next_thirty_days(self):
|
||||
Event.objects.create(club=self.club, title="Soon", start=timezone.now() + datetime.timedelta(days=3))
|
||||
Event.objects.create(club=self.club, title="Far", start=timezone.now() + datetime.timedelta(days=90))
|
||||
|
||||
self.assertEqual(self.health().upcoming_events, 1)
|
||||
|
||||
def test_the_dashboard_table_shows_health_not_vanity(self):
|
||||
user = User.objects.create_user(email="root@example.com", password="pw-secret-123", is_staff=True)
|
||||
enrol_mfa(user)
|
||||
self.client.force_login(user)
|
||||
|
||||
response = self.client.get(reverse("controlpanel:dashboard"))
|
||||
|
||||
self.assertContains(response, "Owed")
|
||||
self.assertContains(response, "Upcoming")
|
||||
self.assertContains(response, "Unpaid")
|
||||
|
||||
@@ -17,7 +17,7 @@ from .services.platform_admins import (
|
||||
revoke_platform_access,
|
||||
set_platform_access,
|
||||
)
|
||||
from .services.statistics import club_attention, club_charts, club_statistics, clubs_with_totals, flag_adoption, onboarding_funnel, platform_attention, platform_charts, platform_totals
|
||||
from .services.statistics import club_attention, club_charts, club_statistics, clubs_with_health, clubs_with_totals, flag_adoption, onboarding_funnel, platform_attention, platform_charts, platform_totals
|
||||
|
||||
Flag = get_waffle_flag_model()
|
||||
Switch = get_waffle_switch_model()
|
||||
@@ -34,7 +34,7 @@ class DashboardView(PlatformStaffRequiredMixin, TemplateView):
|
||||
funnel=onboarding_funnel(),
|
||||
flags=flag_adoption(),
|
||||
charts=platform_charts(),
|
||||
clubs=clubs_with_totals(Club.objects.active()),
|
||||
clubs=clubs_with_health(),
|
||||
**kwargs,
|
||||
)
|
||||
|
||||
|
||||
@@ -2945,6 +2945,9 @@
|
||||
.mb-10 {
|
||||
margin-bottom: calc(var(--spacing) * 10);
|
||||
}
|
||||
.ml-1 {
|
||||
margin-left: var(--spacing);
|
||||
}
|
||||
.ml-2 {
|
||||
margin-left: calc(var(--spacing) * 2);
|
||||
}
|
||||
@@ -3602,6 +3605,12 @@
|
||||
font-size: 0.75rem;
|
||||
}
|
||||
}
|
||||
.badge-xs {
|
||||
@layer daisyui.l1.l2 {
|
||||
--size: calc(var(--size-selector, 0.25rem) * 4);
|
||||
font-size: 0.625rem;
|
||||
}
|
||||
}
|
||||
.otp-lg {
|
||||
@layer daisyui.l1.l2 {
|
||||
font-size: 2rem;
|
||||
|
||||
Reference in New Issue
Block a user