From 0ceceebba49190675a6557b4f7ef0aa088e0ad95 Mon Sep 17 00:00:00 2001 From: Konrad du Plessis Date: Wed, 22 Apr 2026 19:52:29 +0200 Subject: [PATCH] Fix: supervisor picker hid regular active users (only admins showed) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reported: when creating a new team or project from the friendly UI (/teams/new/ or /projects/new/), the Supervisor dropdown only lists is_staff / is_superuser accounts. Users who should be eligible to supervise (e.g. eendman, supervisor_smoke) are invisible in the picker even though they are active. Root cause: core.forms._supervisor_user_queryset filtered to is_active=True AND (is_staff OR is_superuser OR groups__name='Work Logger') That was strictly more restrictive than the app's own permission helper is_supervisor(user) in views.py, which grants supervisor powers to ANYONE assigned to a team/project (via the team.supervisor FK or project.supervisors M2M), regardless of group membership. On Konrad's dev DB that excluded 2 of 6 active users from the picker (one in a custom group, one in no group) even though both were valid supervisor candidates by the permission model. Fix: Queryset now returns every active user. The act of assigning a user to a team/project is what confers supervisor-ness downstream, so the picker no longer needs a pre-registered allow-list. Inactive users (is_active=False) remain excluded — the one hard guardrail. Docstring rewritten to explain the new behavior and why. Stale comment in TeamForm.__init__ updated to match (the old comment still described the pre-fix Work-Logger-group requirement). Tests: 4 new regression tests in SupervisorPickerQuerysetTests: - regular active user is selectable (the core bug) - user in an unrelated group is selectable - inactive user is still excluded (guardrail) - admin is still selectable (no regression for prior use case) All 28 tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) --- core/forms.py | 38 +++++++++++++++++--------------- core/tests.py | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 81 insertions(+), 17 deletions(-) diff --git a/core/forms.py b/core/forms.py index 2bd5207..36dc9bc 100644 --- a/core/forms.py +++ b/core/forms.py @@ -372,18 +372,23 @@ WorkerWarningFormSet = inlineformset_factory( def _supervisor_user_queryset(): """Users eligible to supervise a team or project. - Matches the app's role model (see `is_supervisor` in views.py): - anyone who is a Django admin (is_staff/is_superuser) OR is a member - of the "Work Logger" group. Active accounts only — no deactivated - users in the picker. + Returns ALL active users. Any active user can be picked as a supervisor + — the picker doesn't need to pre-filter by group or staff flags because + the app's `is_supervisor(user)` helper (in views.py) already grants + supervisor permissions to anyone assigned to a team/project FK/M2M, + regardless of their group membership. + + Previously this filter required `is_staff`/`is_superuser` OR membership + in a "Work Logger" group, which was strictly more restrictive than the + permission model and hid field supervisors from the picker. Fix + (2026-04-23): trust the admin making the assignment; any active user + can be chosen, and the act of assignment is what confers supervisor + powers downstream. + + Inactive users (`is_active=False`) are still excluded — deactivated + accounts should never appear in dropdowns. """ - from django.db.models import Q - return ( - User.objects.filter(is_active=True) - .filter(Q(is_staff=True) | Q(is_superuser=True) | Q(groups__name='Work Logger')) - .distinct() - .order_by('username') - ) + return User.objects.filter(is_active=True).order_by('username') class TeamForm(forms.ModelForm): @@ -408,12 +413,11 @@ class TeamForm(forms.ModelForm): def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) - # Supervisor dropdown — show anyone who is either admin (is_staff/ - # is_superuser) OR a member of the "Work Logger" group. This matches - # the app's role model: team supervisors are typically Work Loggers, - # not admins, so filtering by is_staff alone hides the people who - # actually supervise teams day-to-day. We use `active=True` to drop - # deactivated accounts from the picker. + # Supervisor dropdown — show any active user. The app's is_supervisor + # helper (views.py) grants supervisor powers to whoever is assigned + # here, regardless of group or staff flags, so the picker doesn't + # need to pre-filter by role. Only is_active=True users appear, so + # deactivated accounts are hidden from the dropdown. self.fields['supervisor'].queryset = _supervisor_user_queryset() self.fields['supervisor'].required = False # Include inactive workers too — matches admin parity. The template diff --git a/core/tests.py b/core/tests.py index fae9a96..096e8f5 100644 --- a/core/tests.py +++ b/core/tests.py @@ -416,3 +416,63 @@ class ReportContextFilterInflationTests(TestCase): ctx = self._ctx(project_id=self.project.id, team_id=self.team.id) totals = {item['type']: item['total'] for item in ctx['adjustment_totals']} self.assertEqual(totals.get('Bonus'), Decimal('100.00')) + + +# ============================================================================= +# === TESTS FOR SUPERVISOR PICKER QUERYSET === +# Regression tests for the TeamForm/ProjectForm supervisor dropdown. +# +# THE BUG (fixed 2026-04-23): +# The picker required either admin flags (is_staff/is_superuser) or "Work Logger" +# group membership. Any active user NOT pre-enrolled in that specific group +# was invisible, even though the app's downstream `is_supervisor()` helper +# grants supervisor powers to anyone assigned to a team/project FK/M2M — +# regardless of group. The picker was strictly more restrictive than the +# permission model, hiding field supervisors from the person trying to assign +# them. Fix: allow any active user; let the act of assignment confer +# supervisor-ness (matching how `is_supervisor` works). +# ============================================================================= + + +class SupervisorPickerQuerysetTests(TestCase): + """Supervisor picker must surface all active users, not just admins.""" + + def test_regular_active_user_is_selectable(self): + """A plain active user (no is_staff, no is_superuser, no Work Logger + group) must appear. Pre-fix: excluded by the Q filter.""" + from core.forms import _supervisor_user_queryset + regular = User.objects.create_user( + username='field_supervisor', password='pass', + is_staff=False, is_superuser=False, + ) + self.assertIn(regular, _supervisor_user_queryset()) + + def test_user_in_unrelated_group_is_still_selectable(self): + """A user in some random group (not 'Work Logger') is still an active + user and must show up. Pre-fix: excluded because the Q filter specifically + checked groups__name='Work Logger'.""" + from django.contrib.auth.models import Group + from core.forms import _supervisor_user_queryset + user = User.objects.create_user(username='misc_group_user', password='pass') + group, _ = Group.objects.get_or_create(name='Some Other Group') + user.groups.add(group) + self.assertIn(user, _supervisor_user_queryset()) + + def test_inactive_user_still_excluded(self): + """Inactive users must NEVER appear in the picker — even if they used + to be admins or Work Loggers. Active-only is the one hard guardrail.""" + from core.forms import _supervisor_user_queryset + inactive = User.objects.create_user( + username='former_employee', password='pass', + is_active=False, is_staff=True, + ) + self.assertNotIn(inactive, _supervisor_user_queryset()) + + def test_admin_still_selectable(self): + """Defense-in-depth: admins continue to appear (no regression for the + existing use case).""" + from core.forms import _supervisor_user_queryset + admin = User.objects.create_user( + username='an_admin', password='pass', is_staff=True + ) + self.assertIn(admin, _supervisor_user_queryset())