diff --git a/controlpanel/services/statistics.py b/controlpanel/services/statistics.py index 1b23dc1..406fb91 100644 --- a/controlpanel/services/statistics.py +++ b/controlpanel/services/statistics.py @@ -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 diff --git a/controlpanel/templates/controlpanel/dashboard.html b/controlpanel/templates/controlpanel/dashboard.html index 1485b2b..ff02672 100644 --- a/controlpanel/templates/controlpanel/dashboard.html +++ b/controlpanel/templates/controlpanel/dashboard.html @@ -50,6 +50,7 @@

{% lucide "user-plus" size=18 %} Signups per month

+

New members against returning ones, across every club.

@@ -142,15 +143,22 @@

Clubs

+ {% 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 %}
- - - - + + + + + + @@ -158,16 +166,27 @@ - - - - + + + + + + {% empty %} - + {% endfor %} @@ -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(); diff --git a/controlpanel/tests.py b/controlpanel/tests.py index acda275..b52ba57 100644 --- a/controlpanel/tests.py +++ b/controlpanel/tests.py @@ -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") diff --git a/controlpanel/views.py b/controlpanel/views.py index 93d87b1..4547c3b 100644 --- a/controlpanel/views.py +++ b/controlpanel/views.py @@ -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, ) diff --git a/static/css/app.css b/static/css/app.css index 080d6b1..6c628d3 100644 --- a/static/css/app.css +++ b/static/css/app.css @@ -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;
ClubMembersTeamsEventsAdminsMembersUnpaidOwedTeamsUpcomingAdmins
{{ club.name }} -
{{ club.slug }}
+
+ {{ club.slug }} + {% if not club.has_season %}{% lucide "calendar-x" size=10 %} No season{% endif %} + {% if not club.upcoming_events %}{% lucide "moon-star" size=10 %} Dormant{% endif %} +
{{ club.member_count }}{{ club.team_count }}{{ club.event_count }}{{ club.admin_count }}{{ club.active_members }}{{ club.unpaid_members }}€{{ club.outstanding|floatformat:2 }} + {{ club.team_count }} + {% if club.teams_without_coach %} + {{ club.teams_without_coach }} no coach + {% endif %} + {{ club.upcoming_events }}{{ club.admin_count }}
No clubs yet.No clubs yet.